fix: sort RFC 2231 parameter continuations numerically - #13563
Conversation
Confidence Score: 4/5The malformed-parameter exception should be fixed before merging because remote Content-Disposition input can now make filename extraction fail. The new sort key assumes every prefix-matching parameter has an integer suffix, although the parser can retain token-valid nonnumeric names, causing an uncaught ValueError on multipart and response metadata paths. Files Needing Attention: aiohttp/multipart.py Reviews (1): Last reviewed commit: "fix: sort RFC 2231 parameter continuatio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fnparams = sorted( | ||
| (key, value) for key, value in params.items() if key.startswith(name_suf) | ||
| ((key, value) for key, value in params.items() if key.startswith(name_suf)), | ||
| key=lambda kv: int(kv[0].split("*", 1)[1].rstrip("*")), |
There was a problem hiding this comment.
When a Content-Disposition header contains a token-valid parameter such as filename*abc=foo, parse_content_disposition retains it and this sort key calls int("abc"), causing an uncaught ValueError when multipart filename or response content-disposition metadata is accessed.
Knowledge Base Used: Payloads, forms, and multipart bodies
| fnparams = sorted( | ||
| (key, value) for key, value in params.items() if key.startswith(name_suf) | ||
| ((key, value) for key, value in params.items() if key.startswith(name_suf)), | ||
| key=lambda kv: int(kv[0].split("*", 1)[1].rstrip("*")), |
There was a problem hiding this comment.
This changes RFC 2231 continuation ordering without adding focused tests for indices of 10 or greater and nonnumeric continuation-like names, leaving both the intended fix and malformed-input compatibility unprotected against regression.
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!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13563 +/- ##
=======================================
Coverage 99.02% 99.02%
=======================================
Files 135 135
Lines 50481 50481
Branches 2650 2650
=======================================
Hits 49988 49988
Misses 370 370
Partials 123 123
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
Merging this PR will not alter performance
Comparing 
Footnotes
|
1 participant
Fixes #13499
Problem: content_disposition_filename() reassembles RFC 2231 continuations (filename0, filename1, ...) with a lexicographic sorted() over parameter names. String ordering puts filename10 between filename1 and filename*2, so the sequential-index check stops at the first mismatch and any filename split into 10+ sections is silently truncated to its first two.
Fix: sort by the numeric continuation index (extracted from the name), so segments are reassembled in order regardless of how many there are.