Skip to content

Consume the boundary after a multipart _charset_ part - #13640

Draft
2sumtech wants to merge 1 commit into
aio-libs:masterfrom
2sumtech:fix/multipart-charset-part-boundary
Draft

2sumtech wants to merge 1 commit into
aio-libs:masterfrom
2sumtech:fix/multipart-charset-part-boundary

Conversation

@2sumtech

@2sumtech 2sumtech commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

What do these changes do?

MultipartReader.next() has a special case for the RFC 7578 §4.6 _charset_
field: when the first part of a multipart/form-data body is named _charset_,
its value is read as the default charset and the next part is returned instead.
That branch read the _charset_ part's body and then called fetch_next_part()
directly, so the delimiter line that terminates the _charset_ part was never
consumed. fetch_next_part() therefore fed the boundary line into
HeadersParser, which raised InvalidHeader. These changes release the
_charset_ part and read its delimiter with _read_boundary() before fetching
the next part, and return None when that delimiter turns out to be the closing
one.

Are there changes in behavior for the user?

Yes — a multipart/form-data body containing a _charset_ field is now parsed
instead of raising. Previously the boundary line was consumed as a header, so
await request.post() raised InvalidHeader and the server answered
500 Internal Server Error for every boundary that does not itself parse as a
name: value pair — i.e. for every realistic boundary. The existing coverage
only passed because it uses the boundary :, which makes the delimiter line
--: parse as a (bogus) header named --, silently attached to the following
part's headers. That stray header is gone now too. No API changes.

Is it a substantial burden for the maintainers to support this?

No. It is five lines inside the existing _charset_ branch, reusing
BodyPartReader.release() and MultipartReader._read_boundary() — the same two
steps the ordinary next() path already performs between parts. No new helpers,
no new public surface.

Related issue number

None — found while probing the multipart header/boundary seams. Happy to file one
if you prefer an issue on record.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes — N/A (no API or documented-behaviour change)
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt — already listed
  • Add a new news fragment into the CHANGES/ folder

Reproducer

import asyncio, aiohttp
from aiohttp import web

B = "----WebKitFormBoundaryABC"
BODY = (
    f"--{B}\r\n"
    'Content-Disposition: form-data; name="_charset_"\r\n\r\n'
    "utf-8\r\n"
    f"--{B}\r\n"
    'Content-Disposition: form-data; name="field1"\r\n\r\n'
    "foo\r\n"
    f"--{B}--\r\n"
).encode()

async def handler(request):
    return web.json_response(dict(await request.post()))

async def main():
    app = web.Application()
    app.router.add_post("/", handler)
    runner = web.AppRunner(app); await runner.setup()
    site = web.TCPSite(runner, "127.0.0.1", 0); await site.start()
    port = runner.addresses[0][1]
    async with aiohttp.ClientSession() as s:
        async with s.post(f"http://127.0.0.1:{port}/", data=BODY,
                          headers={"Content-Type": f"multipart/form-data; boundary={B}"}) as r:
            print(r.status, await r.text())
    await runner.cleanup()

asyncio.run(main())

Before: 500 500 Internal Server Error with
aiohttp.http_exceptions.InvalidHeader: Invalid HTTP header: b'------WebKitFormBoundaryABC'
raised from multipart.py:832 fetch_next_partmultipart.py:935 _read_headers.
After: 200 {"field1": "foo"}.

Agent run output — tests before and after

Environment: pure-Python mode, AIOHTTP_NO_EXTENSIONS=1 PYTHONPATH=. python -m pytest
(CPython 3.11.15). The C extension only substitutes HttpRequestParser /
HttpResponseParser / RawRequestMessage / RawResponseMessage
(http_parser.py:1241-1248); MultipartReader._read_headers() always uses the pure-Python
HeadersParser, so this change has no Cython-dependent behaviour.

1. New tests fail on the unpatched tree (aiohttp/multipart.py restored from
origin/master, new tests present):

$ AIOHTTP_NO_EXTENSIONS=1 PYTHONPATH=. python -m pytest tests/test_multipart.py -k default_encoding -q
tests/test_multipart.py .FFF.                                            [100%]
=================================== FAILURES ===================================
E           AssertionError: assert ['--', 'Content-Disposition'] == ['Content-Disposition']
E             At index 0 diff: '--' != 'Content-Disposition'
E             Left contains one more item: 'Content-Disposition'
tests/test_multipart.py:1188: AssertionError
tests/test_multipart.py:1215:
E               aiohttp.http_exceptions.InvalidHeader: 400, message:
E                 Invalid HTTP header: b'--WebKitFormBoundary'
tests/test_multipart.py:1235:
E               aiohttp.http_exceptions.InvalidHeader: 400, message:
E                 Invalid HTTP header: b'--WebKitFormBoundary--'
=========================== short test summary info ============================
FAILED tests/test_multipart.py::TestMultipartReader::test_read_form_default_encoding
FAILED tests/test_multipart.py::TestMultipartReader::test_read_form_default_encoding_boundary_without_colon
FAILED tests/test_multipart.py::TestMultipartReader::test_read_form_default_encoding_as_last_part

2. Whole multipart suite passes with the fix:

$ AIOHTTP_NO_EXTENSIONS=1 PYTHONPATH=. python -m pytest tests/test_multipart_helpers.py tests/test_multipart.py -q
tests/test_multipart_helpers.py ................s....................... [ 14%]
tests/test_multipart.py ................................................ [ 64%]
======================== 268 passed, 7 skipped in 0.37s ========================

3. No regressions in the server request path:

$ AIOHTTP_NO_EXTENSIONS=1 PYTHONPATH=. python -m pytest tests/test_web_request.py tests/test_web_functional.py -q
======================= 292 passed, 14 skipped in 1.78s ========================

4. Toolchain on the touched files:

$ python -m black --check aiohttp/multipart.py tests/test_multipart.py
All done! 2 files would be left unchanged.
$ python -m isort --check-only aiohttp/multipart.py tests/test_multipart.py   # clean
$ python -m flake8 aiohttp/multipart.py tests/test_multipart.py               # clean
$ python -m mypy aiohttp/multipart.py
Success: no issues found in 1 source file

Drafted with Claude Code (Claude Opus 5 and Fable 5.1); human review by @2sumtech pending before this leaves draft.


@psf-chronographer psf-chronographer Bot added the bot:chronographer:provided There is a change note present in this PR label Sep 5, 2026
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.02%. Comparing base (a5fba5b) to head (337cb3e).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13640   +/-   ##
=======================================
  Coverage   99.02%   99.02%           
=======================================
  Files         135      135           
  Lines       50845    50865   +20     
  Branches     2674     2675    +1     
=======================================
+ Hits        50351    50371   +20     
  Misses        370      370           
  Partials      124      124           
Flag Coverage Δ
Autobahn 21.96% <10.00%> (-0.01%) ⬇️
CI-GHA 98.92% <100.00%> (+<0.01%) ⬆️
OS-Linux 98.70% <100.00%> (+<0.01%) ⬆️
OS-Windows 97.31% <100.00%> (+<0.01%) ⬆️
OS-macOS 98.18% <100.00%> (+<0.01%) ⬆️
Py-3.10 98.12% <100.00%> (+<0.01%) ⬆️
Py-3.11 98.35% <100.00%> (+<0.01%) ⬆️
Py-3.12 98.43% <100.00%> (+<0.01%) ⬆️
Py-3.13 98.42% <100.00%> (+<0.01%) ⬆️
Py-3.14 98.45% <100.00%> (+<0.01%) ⬆️
Py-3.14t 97.82% <100.00%> (-0.01%) ⬇️
Py-pypy-3.11 97.39% <100.00%> (-0.01%) ⬇️
VM-macos 98.18% <100.00%> (+<0.01%) ⬆️
VM-ubuntu 98.70% <100.00%> (+<0.01%) ⬆️
VM-windows 97.31% <100.00%> (+<0.01%) ⬆️
cython-coverage 83.14% <0.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

@codspeed-hq

codspeed-hq Bot commented Sep 5, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 97 untouched benchmarks
⏩ 83 skipped benchmarks1


Comparing 2sumtech:fix/multipart-charset-part-boundary (337cb3e) with master (a5fba5b)

Open in CodSpeed

Footnotes

  1. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:automated-account bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant