Validate and track HTTP/2 server push streams - #519
Merged
Merged
Conversation
Streams are removed as soon as the server ends them with END_STREAM, so
a RST_STREAM frame on a stream still being tracked means the response
is incomplete. With the NO_ERROR code it was reported as {:done, ref},
both before any headers and in the middle of a body with a declared
content-length. It now produces {:server_closed_request, :no_error}.
…ed status codes RFC 9113 8.2.2 makes a message containing connection, keep-alive, proxy-connection, transfer-encoding or upgrade malformed, which is a stream error. The te header is only allowed in requests, so it's rejected in responses and trailers as well. A :status of 101 is not supported in HTTP/2 (RFC 9113 8.6) and used to be delivered as an interim response, and :status values below 100 were accepted although HTTP/1 rejects them; both are now stream errors.
A PUSH_PROMISE on a stream that is no longer in the stream map, for
example because the client cancelled the request before the server
processed the RST_STREAM, raised {:stream_not_found, id} out of
stream/2. The connection stayed open but the socket was never re-armed
with active: :once, frames after the PUSH_PROMISE in the same message
were dropped, and the header block was not decoded, so the next
header block from the server failed with a compression error.
RFC 9113 6.6 requires handling PUSH_PROMISE frames created before the
RST_STREAM was processed. The header block is now decoded to keep the
HPACK table in sync, the promised stream is reset with CANCEL and the
frame is otherwise ignored.
Promised stream identifiers were only checked for being even and unused, so a server could promise stream 0, promise identifiers lower than an earlier promise or reuse the identifier of a reset stream, and it could send PUSH_PROMISE on a stream it initiated itself. RFC 9113 5.1.1 and 8.4 make all of these connection errors. The highest promised identifier is now tracked and also reported as the last stream identifier in the GOAWAY frame sent on a connection error, which was hard-coded to 2. The promised request headers were delivered without validation. They now need non-empty :method, :scheme, :authority and :path pseudo-headers before any regular field, a :path starting with "/", a safe and cacheable method, no content, no connection-specific fields other than "te: trailers", and field names and values following the same rules as response headers (RFC 9113 8.2.2, 8.3.1, 8.4 and 8.4.1). A promise that fails is reset with PROTOCOL_ERROR. GOAWAY marked pushed streams above the last stream identifier as unprocessed and dropped their responses, but that identifier only covers client-initiated streams (RFC 9113 6.8).
An interim (1xx) response on a stream reserved by a PUSH_PROMISE closed the connection with a protocol error because the stream was still in the reserved state. RFC 9113 5.1 moves a reserved (remote) stream to half-closed (local) on any HEADERS frame, so the stream is now opened, and counted against the concurrency limit, before the interim response is delivered. Opening the stream first also means a pushed response that ends with its HEADERS frame no longer makes the client send a RST_STREAM on the closed stream (RFC 9113 5.1). A pushed response refused at that point because it would exceed the client's max_concurrent_streams setting was reset without any response for the promised request ref. It now returns a :too_many_concurrent_requests error for that ref.
Streams reserved by a PUSH_PROMISE were added to the stream map but not
to the ref-to-stream index, so functions taking the promised request
ref treated it as unknown. cancel_request/2 returned {:ok, conn} without
sending RST_STREAM, so there was no way to refuse a pushed response,
get_window_size/2 raised ArgumentError, and set_window_size/3 returned
:unknown_request_to_stream. The docs describe the promised ref as a
request ref like any other.
stream_request_body/3 with trailers now checks the stream state the way
DATA does, so trailers for a promised request or for a request whose
body has ended return :request_is_not_streaming instead of sending a
HEADERS frame. Trailers also no longer count the request as open a
second time in open_request_count/1.
Frames on an even stream ID that was never promised were ignored. A server only opens streams through PUSH_PROMISE (RFC 9113 8.4 and 5.1.1), so any frame other than PRIORITY on such an idle stream is now a connection error, the same treatment client streams above the next stream ID already got.
Coverage Report for CI Build 0Coverage increased (+0.6%) to 90.159%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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
2 participants
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.
Builds on #518, whose two commits are included here until it merges. The five server push commits are meant to be rebase-merged rather than squashed.
{:stream_not_found, id}. The header block is decoded and the promised stream is reset with CANCEL.:authority, and a:pathstarting with "/". The method must be safe and cacheable, there must be no content, and connection-specific fields are rejected apart fromte: trailers. Invalid promises are reset with PROTOCOL_ERROR.:max_concurrent_streamsreturns{:error, promised_ref, :too_many_concurrent_requests}instead of nothing.cancel_request/2,get_window_size/2andset_window_size/3.stream_request_body/3with trailers checks the stream state like DATA does, so it returns:request_is_not_streamingfor promised or finished requests instead of sending HEADERS. Trailers also no longer incrementopen_request_count/1.