Reject connection-specific headers sent via HPACK/QPACK indexed names - #67584
Conversation
Connection-specific header fields (e.g. Transfer-Encoding) were only validated on the literal name+value path. Using HPACK's indexed-name representation (transfer-encoding is static table index 57) routed the header through OnStaticIndexedHeader/OnDynamicIndexedHeader, skipping the IsConnectionSpecificHeaderField check and allowing Transfer-Encoding: chunked through on HTTP/2. HTTP/3 shares the same structural gap via QPACK dynamic-table indexed names. Move the connection-specific header check so it runs for every header representation in both Http2Connection and Http3Stream. No legitimate headers are affected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR closes a validation gap in Kestrel’s HTTP/2 and HTTP/3 header parsing where connection-specific headers could bypass rejection when the header name was represented via HPACK/QPACK indexed-name forms (rather than literal name + value). The fix ensures connection-specific header fields are rejected consistently for all header representations, per RFC 9113/9114.
Changes:
- Moved connection-specific header detection to the top of
OnHeaderCorein HTTP/2 and HTTP/3 so it runs for all header encodings (indexed, indexed-name + literal value, literal). - Removed the now-redundant connection-specific check from
ValidateHeaderContentin both implementations. - Added an HTTP/2 regression test that sends
transfer-encodingusing HPACK static indexed-name (index 57) with a literal value.
Show a summary per file
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http2/Http2Connection.cs | Runs connection-specific header rejection for every header representation by checking early in OnHeaderCore. |
| src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs | Mirrors the HTTP/2 change for HTTP/3/QPACK by checking early in OnHeaderCore. |
| src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs | Adds a regression test covering HPACK indexed-name (transfer-encoding, static index 57) bypass scenario. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 1
| // https://www.rfc-editor.org/rfc/rfc9114#section-4.2 | ||
| // Connection-specific header fields make a message malformed regardless of how the header name was | ||
| // encoded. A QPACK indexed-name representation (referencing a dynamic table entry inserted via the | ||
| // encoder stream) would otherwise let these fields bypass the check below, so validate every header type. | ||
| if (IsConnectionSpecificHeaderField(name, value)) | ||
| { | ||
| throw new Http3StreamErrorException(CoreStrings.HttpErrorConnectionSpecificHeaderField, Http3ErrorCode.MessageError); | ||
| } |
BrennanConroy
left a comment
There was a problem hiding this comment.
nit: Add an http/3 test
HTTP/3 uses QPACK which does not have connection-specific headers in the static-table https://www.rfc-editor.org/rfc/rfc9204.html#static-table. I changed Thanks for review! |
Fixes a gap in #66669. Connection-specific header fields (
connection,transfer-encoding,keep-alive,proxy-connection,upgrade,te) were only rejected when sent as an HPACK/QPACK literal name + value. When the header name was sent using an indexed-name representation, the check was skipped.Concretely,
transfer-encodingis entry 57 in the HPACK static table. A request encoding it as Literal Header Field - Indexed Name (index 57) with a literal valuechunkedwas routed throughOnStaticIndexedHeader(index, value)->HeaderType.StaticAndValue, which never calledValidateHeaderContent, soIsConnectionSpecificHeaderFieldwas never evaluated. This allowedTransfer-Encoding: chunkedthrough on HTTP/2.HTTP/3 has the same structural gap: QPACK's static table contains no connection-specific header, but an entry inserted into the QPACK dynamic table via the encoder stream (which isn't validated on that path) could then be referenced by a dynamic indexed name (
HeaderType.Dynamic), bypassing the same check.Fix
Move the
IsConnectionSpecificHeaderFieldcheck out ofValidateHeaderContentand to the top ofOnHeaderCorein bothHttp2ConnectionandHttp3Stream, so it runs for every header representation - fully indexed, indexed-name + literal value, and literal name + value - as well as trailers. The decodedname/valuespans are always populated for these callbacks, so no additional decoding is required.Only connection-specific header fields are affected; no other/legitimate headers change behavior.
Tests
HEADERS_Received_TransferEncodingWithHPackIndexedName_ConnectionError, which sends a hand-crafted HPACK block referencingtransfer-encodingby static index 57 with a literalchunkedvalue and asserts aPROTOCOL_ERRORconnection error withHttpErrorConnectionSpecificHeaderField.Refs #66669.