Skip to content

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

Merged
climba03003 merged 2 commits intomainfrom
fix-file-parsing
Aug 14, 2025
Merged

fix: enforce RFC 2183/7578 compliance for quoted Content-Disposition parameters#194
climba03003 merged 2 commits intomainfrom
fix-file-parsing

Conversation

@mcollina
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

RafaelGSS commented Aug 11, 2025

@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
Copy Markdown
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