Repository navigation
Reject HTTP versions other than 1.0/1.1 in both parsers - #13929
choudhryfrompak wants to merge 1 commit into
Conversation
Insufficient dataNot enough activity yet to make a reliable assessment. This is an automated analysis by AgentScan |
psf-chronographer
Bot
added
the
bot:chronographer:provided
d97afc8 to
93cd141
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #13929 +/- ##
==========================================
- Coverage 99.06% 99.06% -0.01%
==========================================
Files 135 135
Lines 52551 52572 +21
Branches 2739 2739
==========================================
+ Hits 52061 52081 +20
Misses 369 369
- Partials 121 122 +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 21.26%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | test_client_send_large_websocket_compressed_messages[isal.isal_zlib] |
74.8 ms | 61.7 ms | +21.26% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing choudhryfrompak:harden-http-version-regex (b2178fc) with master (a6306fc)
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. ↩
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
VERSRE accepted any single-digit major.minor (HTTP/0.9, HTTP/2.0, HTTP/9.9, ...) and the C parser's http_version() fell through to an unchecked HttpVersion(major, minor) for anything outside 1.x. Both feed an HttpVersion object straight into keep-alive and Host-header mandation logic. Tighten VERSRE to HTTP/(1).([01]) and add the same major == 1 / minor in (0, 1) guard to http_version(), raising BadStatusLine otherwise. Closes THREAT_MODEL.md section 5.1 threat 1.5.
93cd141 to
b2178fc
Compare
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
@choudhryfrompak both halves of b2178fc are load-bearing, and I checked each against its own compiled tree. Approving.
Revert only aiohttp/_http_parser.pyx to 470daa7 and rebuild: four of the new c-parser cases fail. Revert only aiohttp/http_parser.py: six py-parser cases fail. The probe below bears out your note on llhttp: HTTP/0.9 and HTTP/2.0 are the only two the .pyx guard adds.
non-blocking: the Python parser used to take HTTP/1.2 through HTTP/1.9, which RFC 9110 section 2.5 says to process as 1.1. llhttp refuses them, and now both backends do. Nothing pins either behaviour.
$ cd /var/tmp/p9b-L15/a13929 && python -c 'import aiohttp, aiohttp._http_parser as x; print(aiohttp.__file__); print(x.__file__)'
/var/tmp/p9b-L15/a13929/aiohttp/__init__.py
/var/tmp/p9b-L15/a13929/aiohttp/_http_parser.cpython-312-x86_64-linux-gnu.so
# CPython 3.12.3, aiohttp 4.0.0a2.dev0, llhttp regenerated with `make generate-llhttp`
$ python -m pytest tests/test_http_parser.py -q -p no:randomly --no-cov -n 4
# b2178fc: 1 failed, 1309 passed, 15 skipped, 3 xfailed in 2.44s
# the 1 is TestDeflateBuffer::test_feed_eof_no_err_brotli, no brotli in this venv
$ python -m pytest tests/test_http_parser.py -q --no-cov -k '1x_version'
# b2178fc: 20 passed in 2.30s
# http_parser.py restored to 470daa7: 6 failed, 14 passed, all six py-parser
# _http_parser.pyx restored to 470daa7 and re-cythonized in a second tree:
# 4 failed, 16 passed; request and response, HTTP/0.9 and HTTP/2.0, c-parser only
$ cat probe13929.py
import asyncio, aiohttp.http_parser as hp
from aiohttp.client_proto import ResponseHandler
loop = asyncio.new_event_loop()
for v in ["HTTP/0.9","HTTP/1.0","HTTP/1.1","HTTP/1.2","HTTP/1.3","HTTP/2.0","HTTP/9.9"]:
row = [v]
for cls in (hp.HttpRequestParserPy, hp.HttpRequestParserC):
p = cls(ResponseHandler(loop), loop, 2**16, max_line_size=8190, max_headers=128, max_field_size=8190)
try: p.feed_data(f"GET /t {v}\r\nHost: a\r\n\r\n".encode()); row.append("accepted")
except Exception as e: row.append(type(e).__name__)
print(*row)
$ PYTHONPATH=. python probe13929.py # columns: py, c
# b2178fc (a13929): # 470daa7 (both files at 470daa7, .pyx rebuilt):
HTTP/0.9 BadStatusLine BadStatusLine # HTTP/0.9 accepted accepted
HTTP/1.0 accepted accepted # HTTP/1.0 accepted accepted
HTTP/1.1 accepted accepted # HTTP/1.1 accepted accepted
HTTP/1.2 BadStatusLine BadStatusLine # HTTP/1.2 accepted BadStatusLine
HTTP/1.3 BadStatusLine BadStatusLine # HTTP/1.3 accepted BadStatusLine
HTTP/2.0 BadStatusLine BadStatusLine # HTTP/2.0 accepted accepted
HTTP/9.9 BadStatusLine BadStatusLine # HTTP/9.9 accepted BadStatusLine
# not run: the rest of the suite, and no interop check against a server that answers HTTP/0.9
@Dreamsorcerer, is refusal the behaviour you want here? If it is, an HTTP/1.2 row in rejects_non_1x_version would lock it in.
|
Nice bisection, that's a cleaner check than I'd done myself. Small correction on the RFC cite though: that "treat unknown minor version as your highest supported minor version" rule is from RFC 7230 section 2.6, not 9110 section 2.5. 7230's obsolete now (replaced by 9112) and the newer doc actually dropped that language entirely, I checked. So it's a real HTTP/1.x convention, just not one that's currently written down anywhere active. On the 1.2-1.9 question: your probe already shows llhttp was rejecting 1.2 and 1.3 before this PR touched anything, it just let 0.9 and 2.0 through. So for those two versions this PR isn't really introducing a new rejection, it's just making the Python parser match what the C parser was already doing. Whether llhttp should be folding 1.2-1.9 into 1.1 instead of rejecting them is kind of a question for llhttp itself, not something this PR changes either way. I'll let @Dreamsorcerer make the call. If reject is what's wanted, I can add an HTTP/1.2 case to the rejects_non_1x_version test like you suggested. If treating 1.2-1.9 as 1.1 is preferred, that's a bigger change on the Python side and llhttp's behavior stays a separate mismatch regardless. |
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
@choudhryfrompak the section number was wrong on my side, and the rule is still current. RFC 9110 carries it in section 6.2, Control Data, as the last paragraph:
A recipient that receives a message with a major version number that it implements and a minor version number higher than what it implements SHOULD process the message as if it were in the highest minor version within that major version to which the recipient is conformant.
$ curl -s https://www.rfc-editor.org/rfc/rfc9110.txt | sed -n 2061,2065p
A recipient that receives a message with a major version number that
it implements and a minor version number higher than what it
implements SHOULD process the message as if it were in the highest
minor version within that major version to which the recipient is
conformant. A recipient can assume that a message with a higher
$ curl -s https://www.rfc-editor.org/rfc/rfc9110.txt | awk 'NR>2018 && NR<2061 && /^[0-9]+\.[0-9.]* /'
# no output: no heading between "6.2. Control Data" at line 2018 and the rule
That changes only the citation, not your PR. Refusing HTTP/1.2 to HTTP/1.9 or reading them as 1.1 is still the call you handed to Dreamsorcerer, and I will leave it with him.
|
Good catch, thanks for running it down. You're right, it's current 9110 language. I just had the wrong section. |
This branch has not been deployed
2 participants
What do these changes do?
VERSREinhttp_parser.pymatched any single-digitmajor.minor(HTTP/(\d)\.(\d)), soHTTP/0.9,HTTP/2.0,HTTP/9.9etc. all parsed as valid without rejection. The C parser'shttp_version()in_http_parser.pyxhad the same gap: it checked formajor == 1andminorin(0, 1), but fell through to an uncheckedHttpVersion(major, minor)foranything else instead of rejecting it. THREAT_MODEL.md already flagged this as threat 1.5 in
section 5.1, "Recommended" but not yet done.
I tightened
VERSREtoHTTP/(1)\.([01])and added a matchingraise BadStatusLine(...)tothe C parser's fallthrough, so both backends now reject anything outside
HTTP/1.0/HTTP/1.1on both the request and response parser. Added parameterized tests(
test_http_*_parser_rejects_non_1x_version/test_http_*_parser_accepts_1x_version) thatrun across both backends, same pattern as the existing
test_http_*_parser_bad_version*tests. Updated THREAT_MODEL.md section 5.1 (threat 1.5, the mitigations table, and the recap)
to record this as closed.
One existing test,
test_parse_unusual_request_line, asserted that the Python parser acceptsHTTP/1.3as part of a check for unrelated request-line leniency (unusual method token,unusual path). I changed its version literal to
HTTP/1.0so it still tests what it's meantto test without depending on the exact unchecked-version behavior this PR removes.
Are there changes in behavior for the user?
Yes: a server or client that previously received a status/request line with an HTTP version
other than
1.0/1.1(0.9,2.0,9.9, ...) would get anHttpVersionobject with thatvalue and keep going. Now it gets a
BadStatusLine(400 on the server side) instead. I don'tthink any real HTTP/1-only deployment relies on this - nothing in the library or its own test
suite constructs those versions on purpose - but it is a behavior change for malformed/odd
input, so flagging it here per the template.
Is it a substantial burden for the maintainers to support this?
No. It's a two-line change to each parser plus tests; no new config surface, no new public
API, no backport risk beyond the usual bugfix backport.
Related issue number
N/A - found via reading THREAT_MODEL.md section 5.1 directly, no existing issue.
Checklist
CONTRIBUTORS.txtCHANGES/folderCHANGES/13925.bugfix.rst(placeholder number - will rename to match this PR'sactual number once it's open, per the template's own instructions)
Agent run output (test logs)
Built both backends locally (
make generate-llhttp && make cythonize && pip install -e .,plus
AIOHTTP_NO_EXTENSIONS=1for the pure-Python path) on Python 3.14, currentmaster(
a6306fc43).Reproduced the bug first, before any fix - on
master, both backends acceptedHTTP/0.9/HTTP/2.0/HTTP/9.9(pure-Python accepted all three; the C parser's own vendoredllhttp already hard-rejects a few version pairs including
9.9, but still accepted0.9and2.0, matching THREAT_MODEL.md's threat 1.5 exactly).After the fix:
Baseline (
master, before this change) fortests/test_http_parser.py:1303 passed, 2 skipped, 3 xfailed- the +20 on the fixed branch is exactly the new parameterized tests(3 reject + 2 accept cases, for both request and response, across both backends = 20), with
no other pass/fail/skip/xfail count changes. Same xfail reasons, same skip reasons, before
and after.