Skip to content

fix: enforce RFC 2183/7578 compliance for quoted Content-Disposition parameters #194

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 14, 2025
Merged

fix: enforce RFC 2183/7578 compliance for quoted Content-Disposition parameters #194

merged 2 commits into from
Aug 14, 2025

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Jul 1, 2025

Summary

  • Fixes improper parsing of Content-Disposition filename parameters that violates RFC 2183 and RFC 7578
  • Ensures quoted parameter values stop at the closing quote instead of continuing to parse trailing characters
  • Prevents security issues where filename="payload.jpg".html was incorrectly parsed as payload.jpg.html instead of payload.jpg

Test plan

  • Added test cases to reproduce the issue
  • Verified the fix correctly handles improperly quoted filenames
  • Ensured all existing tests continue to pass
  • Added additional edge case tests for robustness

mcollina added 2 commits July 1, 2025 17:18
@mcollina
fix: enforce RFC 2183/7578 compliance for quoted Content-Disposition …
356a346 
…parameters

Properly parse Content-Disposition filename parameters by stopping at
the closing quote instead of continuing to parse characters after it.
This fixes the security issue where filename="payload.jpg".html was
incorrectly parsed as payload.jpg.html instead of payload.jpg.

The fix ensures that quoted parameter values are correctly terminated
at their closing quotes, preventing potential security vulnerabilities
from malformed Content-Disposition headers.
@mcollina
Merge branch 'main' into fix-file-parsing
7d8371f
@climba03003
Copy link
Member

climba03003 commented Jul 1, 2025 •
edited
Loading

I do believe our current implementation is the correct interpretation of RFC.
The filename is reference to value which is token / quoted-string.
Quoting behavior is defined on https://datatracker.ietf.org/doc/html/rfc822#section-3.4.1

From the example, "Full Name"@Domain is actually meaning Full Name@Domain.
The parsing should treat the value as a whole instead of dropping partial of value.

@RafaelGSS
Copy link
Member

@climba03003 I'm checking this PR again, and I think the RFC822 doesn't apply here. The RFC 822 example is email address syntax, not HTTP parameter parsing, so it doesn’t apply here.

Per RFC 9110 §5.6.6 / RFC 6266 §4.1, a filename parameter value is either a token or a quoted-string, and a quoted-string ends at the closing ", anything after that (before ; or end-of-field) is invalid and must not be concatenated.

Keeping .html after the closing quote introduces a parser differential - if you compare it with nginx, for instance. That's how I read this RFC.

gurgunday
gurgunday approved these changes Aug 14, 2025
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm as well

@climba03003 climba03003 merged commit 5c838c6 into main Aug 14, 2025
17 checks passed
@climba03003 climba03003 deleted the fix-file-parsing branch August 14, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@climba03003 climba03003 climba03003 approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

@gurgunday gurgunday gurgunday approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mcollina @climba03003 @RafaelGSS @gurgunday