fix(http2): strip TE request header when any value is not trailers - #4194
Open
Dev-next-gen wants to merge 1 commit into
Open
Dev-next-gen wants to merge 1 commit into
Dev-next-gen wants to merge 1 commit into
Conversation
Only the first TE value was checked before sending an HTTP/2 request, so a request with `TE: trailers` followed by another TE line kept both values and the peer reset the stream with PROTOCOL_ERROR. Check every value, as the HTTP/1 side already does since hyperium#4152.
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.
After #4152 made the HTTP/1 side look at every TE line and every token, I checked the HTTP/2 path and found that
strip_connection_headersstill only reads the first TE value before sending a request. WithTE: trailersfollowed by a second line likeTE: gzip, the first value passes, nothing gets stripped, and h2's own send check also only looks at the first value, so both lines go out. The receiving side validates each field on its own, so the peer resets the stream with PROTOCOL_ERROR. Between two hyper peers,send_requestreturnsReset(StreamId(1), PROTOCOL_ERROR, Remote).The fix checks every TE value, and if any of them is not
trailersthe header is removed, which is what already happens when a single TE value is nottrailers. Requests that worked before (no TE, or oneTE: trailers) behave the same. The only requests that change are the ones that were always reset.I added an integration test with a hyper h2 client and server. It fails on master with the reset above and passes with the change. The integration, lib, client and server test suites pass locally with
--features full, as docargo fmt --checkandcargo clippy --features full.AI tools used