return 415 for undecodable multipart field charset in post - #13462
return 415 for undecodable multipart field charset in post#13462dxbjavid wants to merge 3 commits into
Conversation
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 #13462 +/- ##
==========================================
+ Coverage 98.42% 99.02% +0.60%
==========================================
Files 133 135 +2
Lines 49747 50469 +722
Branches 2628 2647 +19
==========================================
+ Hits 48961 49976 +1015
+ Misses 662 370 -292
+ Partials 124 123 -1
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 improve performance by 16.21%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_list_element_pattern_redos_payload[whitespace_run_after_content] |
32.8 ms | 28.3 ms | +16.21% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing dxbjavid:multipart-charset-415 (c615ad2) with master (4602990)2
Footnotes
-
83 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
master(eb38b3c) during the generation of this report, so 4602990 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
| assert dict(result) == {"a": "b", "c": "d"} | ||
|
|
||
|
|
||
| async def test_multipart_formdata_field_unknown_charset(protocol: BaseProtocol) -> None: |
There was a problem hiding this comment.
Should be parametrized. There's only 2 words different between the tests.
There was a problem hiding this comment.
good point, merged them into one parametrized test covering the unknown-codec and undecodable-bytes cases.
Signed-off-by: Javid Khan <dxbjavid@gmail.com>
2 participants
What do these changes do?
BaseRequest.post()decodes a plain multipart/form-data text field withvalue.decode(charset), wherecharsetcomes from that part's ownContent-Typeheader and is fully attacker controlled. An unknown codec name (charset=not-a-real-codec) raisesLookupErrorand undecodable bytes raiseUnicodeDecodeError, and neither is caught, so a malformed part turns into an uncaught 500. The sibling urlencoded branch a few lines below, andBaseRequest.text(), already wrap the same decode inexcept (LookupError, UnicodeDecodeError)and raiseHTTPUnsupportedMediaType. This brings the multipart branch in line with those two, so a bad part charset now yields a 415 like the other body-parsing paths rather than a server error.Are there changes in behavior for the user?
A multipart field whose charset is unknown or whose bytes do not decode now surfaces as
HTTPUnsupportedMediaType(415) instead of an unhandled exception. Valid fields are unaffected.Is it a substantial burden for the maintainers to support this?
No. It is a two-line guard that mirrors existing code in the same function.
Related issue number
N/A
Checklist
CONTRIBUTORS.txtCHANGES/folder