Fix Content-Disposition header injection in GetObject#145
Open
Fix Content-Disposition header injection in GetObject#145
Conversation
The filename in the Content-Disposition header was interpolated directly without sanitization. Filenames containing double quotes could break header parsing or enable header injection. This fix: - Strips non-ASCII characters and replaces double quotes in the ASCII `filename` parameter for compatibility - Adds RFC 5987 `filename*` parameter with proper UTF-8 percent encoding, matching the approach used in getShareLink.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
r2-explorer-docs | 132583f | Commit Preview URL Branch Preview URL |
Mar 07 2026, 05:48 PM |
G4brym
commented
Mar 7, 2026
Owner
Author
G4brym
left a comment
There was a problem hiding this comment.
Automated Code Review — APPROVED ✅
Review Scores: 5/5 reviewers approved
Summary
Reviewed a focused security fix that sanitizes filenames in the Content-Disposition header to prevent header injection via double quotes. The fix adds an ASCII-safe filename parameter and a proper RFC 5987 filename* parameter with percent encoding. Clean, correct, and consistent with the existing GetShareLink implementation.
Review Perspectives
- Correctness: ✅ Properly extracts and sanitizes filename with appropriate fallback. ASCII-safe regex and quote replacement are correct.
- Security: ✅ Fixes the header injection vulnerability. CRLF and double-quote injection vectors are properly neutralized.
- Performance: ✅ Negligible overhead — two regex replacements and one
encodeURIComponentcall on a short string. - Code Quality: ✅ Clean, readable code with clear variable names. Follows existing project conventions.
- Testing: ✅ Manual test plan is reasonable for this focused fix. Low risk of regression.
Minor Notes (non-blocking)
encodeURIComponent()doesn't encode single quotes ('), which is technically non-compliant with RFC 5987'sattr-chargrammar. In practice browsers handle this fine, and it's consistent withGetShareLink. If you wanted full compliance, you could add.replace(/'/g, "%27")to theencodeURIComponentresult — but this is entirely optional.
🤖 Automated review by prodboard
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
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.
Summary
Content-Dispositionheader inpackages/worker/src/modules/buckets/getObject.tsmy "file".pdf) could produce a malformed header likefilename="my "file".pdf", breaking downloads or enabling header injectionfilenameparameter (non-ASCII chars replaced with_, double quotes replaced with') and a proper RFC 5987filename*=UTF-8''...parameter with percent encodingGetObjectin line withGetShareLink, which already usesencodeURIComponent()for the filenameTest plan
日本語.pdf) and verify the downloaded filename is correct🤖 Generated with Claude Code