Sort RFC 2231 filename continuations numerically - #13500
Conversation
content_disposition_filename() assembled filename*N continuation sections with a lexicographic sort, so filename*10 sorted before filename*2 and any filename split into 10 or more sections was silently truncated after the first two. RFC 2231 Section 3 requires numeric ordering of continuation sections. Sort the sections by their integer index and decode each section according to its own extended (*N*, percent-encoded) or quoted (*N, literal) marker per RFC 2231 Section 4.1, using the charset declared by the initial extended section. This also stops apostrophes inside quoted sections from being misread as an RFC 5987 charset prefix, which previously raised ValueError or dropped parts of the name.
psf-chronographer
Bot
added
the
bot:chronographer:provided
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13500 +/- ##
=======================================
Coverage 99.00% 99.00%
=======================================
Files 132 132
Lines 49635 49701 +66
Branches 2575 2583 +8
=======================================
+ Hits 49141 49207 +66
Misses 370 370
Partials 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Confidence Score: 5/5The PR appears safe to merge. The previously reported split-multibyte decoding defect and changelog bookkeeping violation are fixed, and no blocking failure remains. Reviews (5): Last reviewed commit: "Cover a multibyte character split across..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| Fixed :func:`~aiohttp.content_disposition_filename` truncating filenames split | ||
| into 10 or more RFC 2231 continuation sections by sorting the sections | ||
| numerically instead of lexicographically, and decoding each section according | ||
| to its own extended/quoted marker -- by :user:`2sumtech`. |
There was a problem hiding this comment.
This adds only the issue-numbered fragment, while the repository convention requires a CHANGES/13500.bugfix.rst symlink to 13499.bugfix.rst when the issue and PR numbers differ. Without it, the change does not follow the project's changelog bookkeeping convention.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Merging this PR will not alter performance
Comparing 
Footnotes
|
Percent-decoding each extended section on its own dropped the whole filename when a multibyte character straddled a section boundary. Join the octets of adjacent extended sections before decoding, and reference the changelog fragment by PR number via a symlink. The changelog fragment no longer uses a :func: role, because content_disposition_filename is not in the API docs and the dangling reference failed the docs build.
Adds tests for octets that stay undecodable once the sections are joined, an unknown charset, and a non-numeric section index, so the new branches are exercised.
A three-octet character can land in three different extended sections; exercise that the whole run is still joined before decoding.
|
Please follow the PR template: https://github.com/aio-libs/aiohttp/blob/master/.github/PULL_REQUEST_TEMPLATE.md This PR description mention that is pending human review. After a human review for the operator (not aiohttp mantainers), this could be ready for review. If this was reviewed by a human, a human should change that part of the PR description |
|
fair point, restructured the description to the full template and moved it back to draft. it stays there until the operator review is done and recorded, then it goes ready |
|
Drafted with Claude Code (Fable 5); reviewed by @2sumtech |
2 participants
What do these changes do?
Fix
content_disposition_filename()assembling RFC 2231 continuation sections in lexicographic order, wherefilename*10sorted beforefilename*2, so filenames split into 10 or more sections were silently truncated after two sections. Sections are now sorted by their integer index (RFC 2231 Section 3), and each section is decoded according to its own extended (*N*, percent-encoded) or quoted (*N, literal) marker (RFC 2231 Section 4.1), using the charset declared by the initial extended section. This also stops apostrophes inside quoted sections from being misread as an RFC 5987charset'language'prefix, which previously raised an uncaughtValueErrorwith one apostrophe or dropped most of the filename with two.Are there changes in behavior for the user?
Only for inputs that were previously mishandled. Names with 10 or more sections are now complete. Apostrophes in quoted sections are literal. Extended sections are always percent-decoded even when the initial section carries no
charset''prefix. Missing-section, leading-zero, and starts-at-1 handling is unchanged and covered by the existing greenbytes-derived tests.Is it a substantial burden for the maintainers to support this?
No. The change is contained in one helper, follows the RFC text section by section, and the greenbytes-derived test corpus already pins the surrounding behavior.
Related issue number
Fixes #13499
Checklist
CONTRIBUTORS.txtCHANGES/folder (13499.bugfix.rst)Test runs
Five new tests in
tests/test_multipart_helpers.py::TestContentDispositionFilenamecovering 11-section reassembly plain and with charset, quoted/extended mix, apostrophes, and a single-section guard. All fail or crash with theValueErroron master and pass with the fix. Fulltest_multipart_helpers.py+test_multipart.py: 253 passed. flake8 and mypy clean on touched files.Drafted with Claude Code (Fable 5); opened as draft pending human review per AGENTS.md.