Let on_headers_complete veto an upgrade and keep the body - #149
Open
afonsojanu wants to merge 1 commit into
Open
Let on_headers_complete veto an upgrade and keep the body#149afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
llhttp pauses parsing right at headers-complete whenever a request carries an Upgrade header, regardless of whether the application actually intends to switch protocols. There was no way for a Python protocol to say "ignore this one, keep parsing as an ordinary HTTP/1.1 message" - so a request declaring Upgrade: h2c (which many clients send speculatively and most servers never act on) got its body silently dropped, because llhttp's own pause fires before the body is ever read. on_headers_complete's return value was previously discarded. Now returning False from it tells the parser to skip the pause and proceed to parse the body normally, matching what RFC 7230 6.7 says a server is allowed to do when it declines an upgrade. The parser still raises HttpParserUpgrade once the message ends, so the application still learns the client asked for one - vetoing the pause doesn't erase that it happened, it just stops losing the body in the meantime. Anything that doesn't return False keeps behaving exactly as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #124.
Right now
cb_on_headers_completepauses parsing (raisingHttpParserUpgrade) the moment a request carries anUpgradeheader, no matter what the embedding application actually wants to do about it. That's a real problem for something likeUpgrade: h2c, which a lot of clients send speculatively on plain HTTP/1.1 requests and which most servers have no intention of acting on. Since the pause fires before any body gets parsed, a request that also has a body (viaContent-LengthorTransfer-Encoding: chunked) loses that body entirely - RFC 7230 6.7 explicitly allows a server to just ignore the upgrade and keep processing the message normally, but there was no way to tell the parser to do that.I traced this by reproducing the exact scenario from the issue (POST with
Upgrade: h2c+ chunked body):feed_data()raisesHttpParserUpgraderight after headers, and even if you catch it and feed the leftover bytes back in, the parser is already expecting a fresh request line at that point, not a body continuation - so there's no way to recover the body from outside.The fix is small:
on_headers_complete's return value used to be discarded. Now, returningFalsefrom it tellscb_on_headers_completeto skip the pause and let llhttp keep parsing the message as normal (following whateverContent-Length/Transfer-Encodingsays).HttpParserUpgradestill gets raised once the message is fully done, so the application still finds out the client asked for an upgrade - it's just no longer forced to lose the body to learn that. Anything that doesn't explicitly returnFalse(the existing behavior for every current caller) is unaffected.Added two tests: one confirming the veto path parses the body correctly and still eventually raises
HttpParserUpgrade, and one confirming an upgrade that isn't vetoed (the existingwebsocketfixture) still pauses immediately with no body read, exactly like before. Full suite is green (43 passed).