fix: encode download Content-Disposition per RFC 6266 and RFC 8187 - #1386
Merged
ferhatelmas merged 2 commits intoSep 22, 2026
Merged
Conversation
Coverage Report for CI Build 35739742579Coverage increased (+0.004%) to 83.396%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats💛 - Coveralls |
The download= parameter was turned into both Content-Disposition parameters with encodeURIComponent. That leaves ' ( ) * raw in the filename* ext-value, where only attr-char is allowed and "'" is the delimiter, so a name like "John's Resume.pdf" produced a malformed value. It also percent-encoded the plain filename fallback, which clients never decode, so "my file.pdf" was saved as "my%20file.pdf", and names with parentheses produced an invalid unquoted token. Percent-encode everything outside attr-char in filename*, and build the filename fallback from printable ASCII instead: non-ASCII and control characters (keeping CR/LF out of the header) and '"'/'\' become "_", as RFC 6266 Appendix D advises, and the value is quoted only when it is not a valid token, so plain names keep today's exact header.
Signed-off-by: Ferhat Elmas <elmas.ferhat@gmail.com>
ferhatelmas
force-pushed
the
fix/content-disposition-rfc8187
branch
from
September 22, 2026 14:21
34f717b to
91eb75a
Compare
ferhatelmas
approved these changes
Sep 22, 2026
ferhatelmas
enabled auto-merge (squash)
September 22, 2026 14:25
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.
What
Encode the
?download=Content-Dispositionheader per RFC 6266 / RFC 8187.Fixes #1385. Follows up #735, which fixed the trailing semicolon in the same header.
Why
Renderer.handleDownloadusedencodeURIComponentoutput for both parameters:filename*—encodeURIComponentleaves' ( ) *raw, but an RFC 8187 ext-value only allowsattr-charunencoded, and'is its delimiter.John's Resume.pdf→UTF-8''John's%20Resume.pdfis malformed.filename— the fallback for clients that ignorefilename*is not percent-decoded by anyone, somy file.pdfwas saved asmy%20file.pdf, andreport(1).pdfwas sent as an unquoted token containing delimiters.This is the only place
download=becomes a header, so every public, signed and authenticated object and image route is covered.How
filename*: percent-encode' ( ) *on top ofencodeURIComponent, giving a value made ofattr-charand%XXonly.filename: printable ASCII fallback — non-ASCII and control characters (so CR/LF can never reach the header), plus"and\(RFC 6266 Appendix D), become_. It is quoted only when it isn't a valid token, so plain names liketestname.pngkeep today's exact header (the existing assertion insrc/test/object.test.tsis unchanged).download=report.pdffilename=report.pdf; filename*=UTF-8''report.pdfmy file.pdffilename=my%20file.pdf; filename*=UTF-8''my%20file.pdffilename="my file.pdf"; filename*=UTF-8''my%20file.pdfJohn's Resume.pdf…; filename*=UTF-8''John's%20Resume.pdffilename="John's Resume.pdf"; filename*=UTF-8''John%27s%20Resume.pdfnaïve.txtfilename=na%C3%AFve.txt; …filename=na_ve.txt; filename*=UTF-8''na%C3%AFve.txtTests
New
src/storage/renderer/renderer.test.tsparses the header with the RFC 6266 grammar (token or quoted-stringfilename) and checks thatfilename*contains onlyattr-char/%XX, round-trips the name, and that the fallback is correct. It also covers: nodownload, emptydownload, an already-valid token staying byte-identical, non-ASCII, and a CR/LF injection attempt.6 of the 11 cases fail on
master(spaces, apostrophe, parentheses,*,"/\, non-ASCII); the other 5 are guards for behavior that is already correct.Validation
Local, macOS / Node v24.9.0:
npm run test:unit— 149 files, 2149 tests passingnpm run build,npm run lint,git diff --check— cleanrenderer.tspatches from open PRs feat: allow setting stale-while-revalidate and stale-if-error headers #1369 and fix: cap signed url edge cache ttl #1188 still apply cleanly on top of this change (git apply --check)Not run:
npm run test:integration(needs the Docker Postgres stack; port 5432 is occupied on this machine). Locally, a few unrelated unit tests (s3/index,request-scoped-pg-executor,vector,listObjectsV2migration gate) fail intermittently under load. They fail the same way on cleanmasterin this environment and pass in upstream CI.