Page MenuHomePhabricator

[blob-service] Validate holder and blob_hash inputs
ClosedPublic

Authored by bartek on Jun 12 2023, 2:36 AM.
Tags
None
Referenced Files
F3387787: D8180.diff
Fri, Nov 29, 11:10 AM
Unknown Object (File)
Tue, Nov 26, 2:52 AM
Unknown Object (File)
Tue, Nov 19, 7:18 AM
Unknown Object (File)
Tue, Nov 19, 3:35 AM
Unknown Object (File)
Mon, Nov 18, 11:31 PM
Unknown Object (File)
Mon, Nov 18, 8:40 PM
Unknown Object (File)
Mon, Nov 18, 8:07 PM
Unknown Object (File)
Mon, Nov 18, 7:25 PM
Subscribers

Details

Summary

Part of ENG-3810. Used function from previous diff to validate blob service holder and blob_hash inputs.

Depends on D8179.

Test Plan

Tried to upload blob with hash blob/hash$@ - a HTTP 400 error was returned.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

bartek held this revision as a draft.
bartek published this revision for review.Jun 12 2023, 3:40 AM

(Shellcheck CI job should pass after pulling in latest changes.)

jon added inline comments.
services/blob/src/http/handlers/blob.rs
263–266

to make this a bit cleaner, you could add a validate_identifier method, which returns an Result<()>, and reduce this boilerplate.

This is just my personal preference, not a blocker.

pub fn validate_identifier(holder) -> Result<()> {
  if !is_valid_identifier(&holder) {
    warn!(holder, "Holder is not a valid identifier");
    return Err(ErrorBadRequest("Bad request"));
  }
  Ok(())
}
This revision is now accepted and ready to land.Jun 12 2023, 9:56 AM
services/blob/src/http/handlers/blob.rs
263–266

Didn't delete the old snippet of code, oops

services/blob/src/http/handlers/blob.rs
263–266

Makes sense, but it's either "holder" or "blob hash" so I'd have to make this configurable. Eventually, I can write a macro

In the long term, I'm planning to use some validator at the actix_web level so this won't have to be done manually in every handler

Rebase. Extract repeating code