fix(upload): Forward-compatible query params#6076
Conversation
| impl<'de> Deserialize<'de> for UploadParams { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| struct UploadParamsVisitor; | ||
|
|
||
| impl<'de> serde::de::Visitor<'de> for UploadParamsVisitor { | ||
| type Value = UploadParams; | ||
|
|
||
| fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| formatter.write_str("upload query parameters") | ||
| } | ||
|
|
||
| fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error> | ||
| where | ||
| A: serde::de::MapAccess<'de>, | ||
| { | ||
| let mut upload_params = BTreeMap::new(); | ||
|
|
||
| while let Some(key) = map.next_key::<&str>()? { | ||
| if key.starts_with("upload_") { | ||
| upload_params.insert(key.to_owned(), map.next_value()?); | ||
| } else { | ||
| map.next_value::<serde::de::IgnoredAny>()?; | ||
| } | ||
| } | ||
|
|
||
| Ok(UploadParams(upload_params)) | ||
| } | ||
| } | ||
|
|
||
| deserializer.deserialize_map(UploadParamsVisitor) | ||
| } |
There was a problem hiding this comment.
I added this custom parser to skip over query params that do not start with upload_ (e.g. sentry_key).
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 74c2a68. Configure here.
| let public_key = config.public_key().ok_or(Error::SigningFailed)?; | ||
| let is_valid = self.signature.verify( | ||
| self.location.as_uri().as_bytes(), | ||
| self.location.try_to_uri()?.as_bytes(), |
There was a problem hiding this comment.
Verify breaks legacy signed locations
Medium Severity
verify checks the signature against bytes from Location::try_to_uri, which always serializes upload_length. Locations signed before this change used length in that string. Deserialization aliases still accept old query names, but verification rewrites the signed payload, so valid legacy upload URLs and attachment refs fail with InvalidSignature.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 74c2a68. Configure here.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| pub upload_length: L, | ||
| #[serde(alias = "signature")] | ||
| pub upload_signature: String, | ||
| #[serde(flatten)] |
There was a problem hiding this comment.
Bug: The use of #[serde(flatten)] with a custom deserializer causes query parameters like upload_length to be captured twice, leading to duplicate parameters and breaking signature verification.
Severity: CRITICAL
Suggested Fix
Modify the custom deserializer for UploadParams to explicitly ignore keys that are already handled by named fields in the parent SignedLocationQueryParams struct, such as upload_length and upload_signature. This will prevent them from being added to the other map and creating duplicates during serialization.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: relay-server/src/services/upload.rs#L477
Potential issue: The `SignedLocationQueryParams` struct uses `#[serde(flatten)]` on the
`other` field, which has a custom deserializer that captures all query parameters
prefixed with `upload_`. When a request is deserialized, parameters like `upload_length`
and `upload_signature` are captured by both their dedicated fields and, due to serde's
behavior with non-self-describing formats, also by the flattened `other` field's
deserializer. When the URI is later reconstructed for signature verification, this
results in duplicate query parameters. The reconstructed URI will not match the original
signed URI, causing signature verification to fail for all file uploads.
Also affects:
relay-server/src/services/upload.rs:505~517
Did we get this right? 👍 / 👎 to inform future reviews.
1 participant
For the sake of forward-compatibility, add an "other" variant to the location query params enum. Instead of denylisting certain keys, like
sentry_key, prefix all upload-relevant keys withupload_such that only those get picked up by the upload service.Fixes: INGEST-807