feat: Add filename field to Metadata#517
Open
lcian wants to merge 8 commits into
Open
Conversation
Add an optional filename field to Metadata, transmitted via the x-sn-filename header. When present, the server emits a Content-Disposition: attachment; filename="<filename>" header on GET and HEAD responses for browser/download-tool compatibility. - Add filename field to Metadata struct with serde and header support - Map to GCS native contentDisposition field - Emit Content-Disposition on GET, HEAD, and batch HEAD responses - Add .filename() builder methods to Rust client PutBuilder and InitiateMultipartBuilder - Add filename parameter to Python client put() and initiate_multipart_upload() Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Adding the filename field to Metadata pushed the Insert variant past clippy's large_enum_variant threshold (232 bytes vs 24 for the next largest). Box it to keep the enum small on the stack. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
![sentry-warden[bot]](https://avatars.githubusercontent.com/in/2755838?s=60&v=4){kind=small}
The filename escaping only escaped double quotes but not backslashes, allowing a stored filename with a backslash-quote sequence to terminate the quoted-string and inject extra Content-Disposition parameters. Centralize format_content_disposition and parse_content_disposition in objectstore-types so all call sites share correct escaping of both backslash and double-quote characters (in the right order). Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Content-Disposition is used by the multipart framing itself to delimit parts, so setting it on individual batch response parts would conflict. Clients should read the filename from the x-sn-filename metadata header instead, which to_headers() already emits. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Add filename assertions to existing e2e tests in both clients to verify the field roundtrips through PUT and GET (including batch GET in the Rust client). Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
lcian
changed the title
lcian
commented
Jun 24, 2026
| pub enum Operation { | ||
| /// Insert a new object. | ||
| Insert(Insert), | ||
| Insert(Box<Insert>), |
Member
Author
There was a problem hiding this comment.
insert.metadata gained one field, so this was firing clippy's large enum variant lint.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0699e52. Configure here.
jan-auer
reviewed
Jun 24, 2026
| /// Formats a `Content-Disposition: attachment` header value for the given filename. | ||
| /// | ||
| /// Escapes `\` and `"` in the filename per RFC 6266 quoted-string rules. | ||
| pub fn format_content_disposition(filename: &str) -> String { |
Member
There was a problem hiding this comment.
Note that axum-extra would have utilities for this: https://docs.rs/axum-extra/latest/axum_extra/response/struct.Attachment.html
At least, we should compare with their implementation to ensure we're not missing something.
| session = client.session(test_usecase, org=42, project=1337) | ||
|
|
||
| object_key = session.put(b"test data", origin="203.0.113.42") | ||
| object_key = session.put(b"test data", origin="203.0.113.42", filename="report.pdf") |
Member
There was a problem hiding this comment.
could we add tests for header injection, i.e.:
- backslash and double quotes in the filename: need escaping in quoted headers (which we use)
- forward slash in the filename: would create a path and we likely don't want that
.and..as filename: as above- control characters (0x00–0x1F, 0x7F) in the filename: not permitted in header values by 7230
i currently can't think of other things we should reject.
same for rust client. i think we should sanitize this in the client and validate server-side.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
For Debug Files, the current Sentry endpoint returns a
Content-Disposition: attachment; filename="<filename>"header, which prompts browsers to save the file under the given name.Objectstore doesn't currently return this, so if we want downloads from e.g. the UI to go directly to Objectstore, the user will likely be prompted with the Objectstore key as the filename, which doesn't necessarily match the logical filename we show e.g. in the UI itself.
This introduces a first-class
Metadatafield to the server and clients, which is send by clients asx-sn-filenameand rendered in GET/HEAD responses via such aContent-Dispositionheader.In the GCS backend, we store that field using the
Content-Dispositionmetadata field itself, as that's supported by the GCS JSON API.For the batch API, which uses
Content-Dispositionto delimit different parts (operations in a batch request), I opted to transmit the header as ax-sn-filenameper part header, this way we get de/serialization for free viaMetadata.Otherwise, we would have to change the
Content-Dispositionto possibly contain the filename and add some additional logic to handle it.Close FS-134