fix core: don't depend on HTTP/2.0 pseudo-header field order#1302
Open
SSE4 wants to merge 1 commit into
Open
Conversation
The server matched a request against its handlers as soon as the :path pseudo-header field was decoded, using whatever method had been parsed up to that point. RFC 9113 8.3 fixes no order among the pseudo-header fields, so a client that serializes :path before :method (h2load does) got 405 Method Not Allowed for every request: matching ran with the default HttpMethod::kUnknown and the cached match result was never revisited. Match the handler once both :method and :path are decoded instead. Since all pseudo-header fields precede the regular ones, this is the earliest correct moment in every order: the per-handler request limits then cover the remaining header fields of the block, and a doomed request (405/404) is known as early as possible. The remainder of the block still has to be decoded either way - HPACK is stateful and a block cannot be abandoned midway without desyncing the connection - and is bounded by max_headers_size. A call at the end of the header block (on_frame_recv fires after all CONTINUATION frames) remains as a safety net for requests that never completed the target. Matching cannot happen later, in request finalization: the per-handler limits must be installed before the DATA frames of the request body arrive.
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
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.
The server matches a request against its handlers as soon as the
:pathpseudo-header is decoded, using whatever method has been parsed by that point.
RFC 9113 §8.3 doesn't fix an order among pseudo-header fields, and clients
disagree: curl sends
:methodfirst, h2load sends:pathfirst. As a resultevery h2load request gets
405 Method Not Allowed— matching runs with thedefault
HttpMethod::kUnknownand the cached result is never revisited:This matches the handler once both
:methodand:pathare decoded. Sincepseudo-header fields precede all regular fields, that is the earliest correct
moment in every order: the per-handler request limits then cover the rest of
the header block, and a doomed request (405/404) is known as early as
possible — no resource-usage regression versus the old eager match. The
remainder of the block still has to be decoded either way (HPACK is stateful;
abandoning a block midway would desync the connection) and is bounded by
max_headers_size. A call at end-of-header-block (OnFrameRecv, fires afterall CONTINUATION frames) remains as a safety net; matching can't be postponed
to
FinalizeRequestbecause the per-handler limits must be installed beforethe request body's DATA frames arrive.
Tested with two new parametrized tests in the http2server functional suite:
the same request over four pseudo-header orders (h2load's exact order
included), as GET and as POST with a request body — the latter checks that
the per-handler limits are installed before the body arrives. Without the fix
the four
:path-before-:methodcases fail with 405; with it all pass. Therest of the http2server suite (h2spec included) and the http2/connection unit
tests pass unchanged. Also verified with stock h2load under load: 2M
requests, 100%
200.closes: #1300