net.http: validate HTTP/2 server request headers (RFC 9113 §8.1.2) - #27569
Merged
JalonSolov merged 5 commits intoJun 28, 2026
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc2d9b873c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
quaesitor-scientiam
added a commit
to quaesitor-scientiam/v
that referenced
this pull request
Jun 27, 2026
…length Address Codex review on vlang#27569: - A trailer section (a 2nd HEADERS block on an open stream) that lacks END_STREAM is malformed. It was returned as an error, which bubbles through serve() and GOAWAYs the whole connection; make it a stream error (RST_STREAM(PROTOCOL_ERROR)) like the other request validation, keeping the connection alive. The trailer block is now always decoded before the well-formedness check, so rejecting a bad trailer section does not desync the HPACK dynamic table for later streams. - build_request validated only the last content-length field, so a conflicting duplicate (e.g. "content-length: 5" then "content-length: 0") was accepted. Reject differing duplicate content-length values (RFC 9110 §8.6). Adds unit tests for both: trailers without END_STREAM and conflicting content-length each get RST_STREAM(PROTOCOL_ERROR). Co-Authored-By: WOZCODE <contact@withwoz.com>
Contributor
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51c8732de8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
quaesitor-scientiam
added a commit
to quaesitor-scientiam/v
that referenced
this pull request
Jun 27, 2026
…SSION_ERROR Address Codex review on vlang#27569: a trailing-HEADERS (and request-HEADERS) HPACK decode failure was answered with RST_STREAM. H2HpackDecoder.decode mutates the dynamic table as it processes a block, so a decode failure leaves the server's decoder permanently out of sync with the peer — every subsequent header block on the connection would be corrupted. RFC 9113 §4.3 requires a connection error of type COMPRESSION_ERROR. finalize_headers and finalize_trailers now send_goaway(.compression_error) and return on a decode failure (closing the connection), instead of resetting one stream. Well-decoded but malformed requests/trailers stay stream errors (RST_STREAM(PROTOCOL_ERROR)) — only the decode failure escalates. Verified with the h2spec gate (v2.6.0, --timeout 5): 120 -> 129 passing, no regressions. This closes the whole HPACK error-scope cluster — 9 cases (hpack/2.3.3 x2, 4.2, 5.2 x3, 6.1, 6.3, http2/4.3) — now removed from the baseline. Adds a unit test (an index-0 HPACK block -> GOAWAY(COMPRESSION_ERROR)). Co-Authored-By: WOZCODE <contact@withwoz.com>
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
The minimal h2 server accepted any decoded header list. Add request-side validation so malformed requests are rejected as a stream error (RST_STREAM(PROTOCOL_ERROR)) per RFC 9113 §8.1.1: - field names must be lowercase (§8.1.2) and non-empty - connection-specific fields and TE != "trailers" are forbidden (§8.2.2) - request pseudo-headers: only the known set, no duplicates, all before any regular field, with :method/:path/:scheme present and non-empty (§8.1.2.1, §8.1.2.3, §8.3) - declared content-length must equal the DATA payload length (§8.1.2.6), with the value charset-checked before parsing (a successful .int() is not validation) - a trailing HEADERS block on an open stream is accepted as trailers (§8.1): decoded to keep HPACK in sync, validated (no pseudo-headers), then dispatched Validation runs at finalize_headers (after the HPACK decode, so the dynamic table stays in sync even when the request is rejected); the connection survives. The connection-specific header list is now a shared const reused by the response path. Verified with the h2spec conformance gate (vlang#27563) in CI's configuration (v2.6.0, --timeout 5): the suite goes from 109 to 120 passing. This closes 12 baselined cases — the §8.1.2/§8.1.2.x/§8.3 request-validation set, generic/4 (POST with trailers), and 6.9.2 (negative window via SETTINGS, which the existing flow-control loop already handled correctly once the request is accepted) — all verified stable across repeated runs and removed from the known-failure baseline. Adds scripted-peer unit tests for each case. Co-Authored-By: WOZCODE <contact@withwoz.com>
…length Address Codex review on vlang#27569: - A trailer section (a 2nd HEADERS block on an open stream) that lacks END_STREAM is malformed. It was returned as an error, which bubbles through serve() and GOAWAYs the whole connection; make it a stream error (RST_STREAM(PROTOCOL_ERROR)) like the other request validation, keeping the connection alive. The trailer block is now always decoded before the well-formedness check, so rejecting a bad trailer section does not desync the HPACK dynamic table for later streams. - build_request validated only the last content-length field, so a conflicting duplicate (e.g. "content-length: 5" then "content-length: 0") was accepted. Reject differing duplicate content-length values (RFC 9110 §8.6). Adds unit tests for both: trailers without END_STREAM and conflicting content-length each get RST_STREAM(PROTOCOL_ERROR). Co-Authored-By: WOZCODE <contact@withwoz.com>
RFC 9113 §8.2.1: a field value MUST NOT contain NUL (0x00), LF (0x0a), or CR (0x0d). The new request validation checked field names but not values, so a value with an embedded CR/LF was accepted and passed through into req.header — a malformed request and, if the request were forwarded to an HTTP/1.x peer, a header-injection / request-smuggling vector. Reject such values (and the same octets in pseudo-header values) as a stream error. Found by a requirements-driven review pass (RFC §8.2.1) — not covered by the h2spec suite. Adds a unit test (CR/LF in a field value -> RST_STREAM). Co-Authored-By: WOZCODE <contact@withwoz.com>
…SSION_ERROR Address Codex review on vlang#27569: a trailing-HEADERS (and request-HEADERS) HPACK decode failure was answered with RST_STREAM. H2HpackDecoder.decode mutates the dynamic table as it processes a block, so a decode failure leaves the server's decoder permanently out of sync with the peer — every subsequent header block on the connection would be corrupted. RFC 9113 §4.3 requires a connection error of type COMPRESSION_ERROR. finalize_headers and finalize_trailers now send_goaway(.compression_error) and return on a decode failure (closing the connection), instead of resetting one stream. Well-decoded but malformed requests/trailers stay stream errors (RST_STREAM(PROTOCOL_ERROR)) — only the decode failure escalates. Verified with the h2spec gate (v2.6.0, --timeout 5): 120 -> 129 passing, no regressions. This closes the whole HPACK error-scope cluster — 9 cases (hpack/2.3.3 x2, 4.2, 5.2 x3, 6.1, 6.3, http2/4.3) — now removed from the baseline. Adds a unit test (an index-0 HPACK block -> GOAWAY(COMPRESSION_ERROR)). Co-Authored-By: WOZCODE <contact@withwoz.com>
Rebase onto master after vlang#27413 (H2MuxConn) merged: that PR added a module-level `h2_conn_specific_headers` const (the RFC 9113 §8.2.2 connection-specific set) in h2_mux_conn.v. This branch had defined the same const in h2_server.v, which now collides (duplicate const). Drop the server copy and share the existing one.
quaesitor-scientiam
force-pushed
the
h2-server-header-validation
branch
from
June 27, 2026 13:30
d6cd7f8 to
1697bc9
Compare
Collaborator
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Contributor
Author
|
CI failure analysis: the red jobs here are unrelated to this PR.
This PR's diff is limited to |
quaesitor-scientiam
added a commit
to quaesitor-scientiam/v
that referenced
this pull request
Jul 2, 2026
Three follow-ups from Codex review 4619906229 on the prior doc-sync commit: - the replaced h2spec_expected_failures.txt table row was 193 chars, over the repo's 100-char markdown line limit — shortened. - "Four follow-up PRs" listed only three PR numbers (vlang#27569, vlang#27589, vlang#27627) — corrected to "Three". - the empty-baseline statement conflicted with unchanged bullets below still describing "genuine conformance gaps" and instructing removal of lines as cases are fixed — reworded to note the baseline is currently empty while the tracking mechanism stays in place for future gaps. Doc-only change; verified with ./vnew check-md (0 errors) and a line-length scan. Both from Codex, vlang#27627 pullrequestreview-4619906229.
JalonSolov
pushed a commit
that referenced
this pull request
Jul 2, 2026
…146) (#27627) * net.http: h2 server — close the final 7 h2spec conformance gaps (146/146) Flow control (RFC 9113 §6.9/§6.9.1), in handle_control_frame so both the main dispatch and the pump_for_window stall path get the rules: - WINDOW_UPDATE with a zero increment: connection (stream 0) -> connection error PROTOCOL_ERROR; stream -> RST_STREAM(PROTOCOL_ERROR). - WINDOW_UPDATE growing a flow-control window past 2^31-1: connection -> GOAWAY(FLOW_CONTROL_ERROR); stream -> RST_STREAM(FLOW_CONTROL_ERROR). Validate-before-mutate: the new window is computed and checked before being committed. SETTINGS (RFC 9113 §6.5.2): - SETTINGS_ENABLE_PUSH accepts only 0 or 1; anything else is a connection error PROTOCOL_ERROR. Priority self-dependency (RFC 7540 §5.3.1, deprecated-but-validated in 9113): - A PRIORITY frame that depends on its own stream -> RST_STREAM(PROTOCOL_ERROR), sent at most once per id (locally_reset guard - no second RST on a stream we already reset). - A HEADERS frame (opening or trailer) whose priority section depends on its own stream: the block is still HPACK-decoded first (RFC 7541 §2.2 - the decoder is stateful), then the stream is reset with PROTOCOL_ERROR, via a self_dep flag mirroring the refused/over_end decode-then-RST pattern. With these, the server passes ALL 146 h2spec v2.6.0 cases (verified 5/5 deterministic runs at --timeout 5, plus 1 run after each review fix); the expected-failures baseline is now EMPTY, so CI fails on any h2spec failure. Tests: 9 new scripted-peer cases in h2_server_test.v (zero-increment conn/stream, overflow conn/stream, ENABLE_PUSH=2, HEADERS/PRIORITY/trailer self-dependency, repeated-PRIORITY single-RST). Full vlib/net/http suite green (23 passed, 2 skipped). * fix(h2): classify_stream must treat locally_reset ids as closed; sync h2spec docs RFC 9113 §5.1 defines "closed" as entered when either side sends RST_STREAM, and explicitly permits remembering a reset stream in that state for a bounded period (exactly what the locally_reset FIFO already implements). classify_stream only tested `stream_id & 1 == 1 && stream_id <= last_stream_id`, which misses any id we RST'd without it ever having advanced last_stream_id — a self-dependent PRIORITY frame (RFC 7540 §5.3.1) is legal on a stream the client never opened via HEADERS, so it can be locally_reset while last_stream_id still hasn't counted it. Consequence: an in-flight DATA/WINDOW_UPDATE/RST_STREAM for such an id, arriving after our RST but classified via the old rule, was misread as a frame on an IDLE stream (a connection PROTOCOL_ERROR) instead of drained per §6.4 — a spurious GOAWAY on legitimate late traffic. Fix: classify_stream checks c.locally_reset before the parity/last_stream_id test, so every caller (on_data, handle_control_frame's WINDOW_UPDATE arm, dispatch_frame's RST_STREAM arm) gets the correct closed/drain behavior from one place, rather than three duplicated per-branch checks that a future fourth caller could silently miss. Also syncs vlib/net/http/h2spec/{README.md,run_h2spec.sh} — both still described the pre-#27627 37-case baseline; the baseline has been empty (146/146) since #27627. Both from Codex, #27627 pullrequestreview-4618166175. * fix(h2): tighten h2spec README wording after conformance-doc sync Three follow-ups from Codex review 4619906229 on the prior doc-sync commit: - the replaced h2spec_expected_failures.txt table row was 193 chars, over the repo's 100-char markdown line limit — shortened. - "Four follow-up PRs" listed only three PR numbers (#27569, #27589, #27627) — corrected to "Three". - the empty-baseline statement conflicted with unchanged bullets below still describing "genuine conformance gaps" and instructing removal of lines as cases are fixed — reworded to note the baseline is currently empty while the tracking mechanism stays in place for future gaps. Doc-only change; verified with ./vnew check-md (0 errors) and a line-length scan. Both from Codex, #27627 pullrequestreview-4619906229. * fix(h2): self-dependent PRIORITY must not re-RST an already-closed stream The self-dependency guard for PRIORITY frames (RFC 7540 §5.3.1) exempted only `frame.stream_id in c.locally_reset` from re-sending RST_STREAM. That misses a stream that completed NORMALLY: run_request deletes it from c.streams without ever touching locally_reset, so classify_stream already correctly reports it as closed, but the old check saw "not locally_reset" and sent a fresh RST_STREAM anyway — itself a frame-on-a-closed-stream violation (RFC 9113 §5.1), since PRIORITY is permitted on closed streams but RST_STREAM is not. Fix: gate on `c.classify_stream(frame.stream_id) != .closed` instead of the locally_reset-only check. idle and active streams still get RST_STREAM (self-dependency remains a stream error per §5.3.1 regardless of whether the stream was ever opened); any already-closed stream — whether closed via locally_reset or via normal completion — is left alone. This reuses the same classify_stream generalization from the prior commit rather than adding a second ad-hoc exemption, so a future caller of this guard inherits the correct behavior automatically. Codex, #27627 pullrequestreview-4620220448. * fix(h2): self-dependent HEADERS over the concurrency limit must be PROTOCOL_ERROR finalize_headers checked s.refused (RFC 9113 §5.1.2, over the concurrency limit) before s.self_dep (RFC 7540 §5.3.1, priority self-dependency). A HEADERS frame that was both over-limit and self-dependent therefore answered RST_STREAM(REFUSED_STREAM) instead of PROTOCOL_ERROR. REFUSED_STREAM specifically tells the client the request is safe to retry unchanged elsewhere; a self-dependent request is malformed independent of the concurrency limit, so retrying it verbatim would just repeat the same error. Fix: swap the check order so self_dep is evaluated first — both branches still route through the identical decode-then-RST pattern (HPACK block already decoded above for dynamic-table sync), so there is no HPACK regression, only the reported error code changes. Codex, #27627 pullrequestreview-4620412384.
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.
What
Adds request-side header validation to the minimal HTTP/2 server (
h2_server.v).Previously the server accepted any decoded header list; now malformed requests are
rejected as a stream error (
RST_STREAM(PROTOCOL_ERROR)) per RFC 9113 §8.1.1,and the connection survives.
Rules enforced (RFC 9113):
connection,proxy-connection,keep-alive,transfer-encoding,upgrade) are forbidden;TEmay only carrytrailersduplicates, all before any regular field, with
:method/:path/:schemepresent and non-empty
content-lengthmust equal the DATA payload length(the value is charset-checked before
.int()— a successful parse is notvalidation)
decoded to keep the HPACK dynamic table in sync, validated (no pseudo-headers),
then dispatched
Validation runs in
finalize_headers, after the HPACK decode, so the dynamictable stays in sync even when the request is rejected. The connection-specific
header list is now a shared const reused by the response path.
Why
The h2spec conformance gate added in #27563 baselines the server's known failures.
This is the first of several follow-ups to burn that list down (the request-
validation cluster).
Verification
Ran the h2spec gate (
vlib/net/http/h2spec/run_h2spec.sh) in CI's configuration(h2spec v2.6.0,
--timeout 5) on Linux:across repeated runs
12 baselined cases now pass and are removed from
h2spec_expected_failures.txt:generic/4,http2/8.1.2,8.1.2.1,8.1.2.2×2,8.1.2.3×4,8.1.2.6×2,and
http2/6.9.2(negative window via SETTINGS — the existing flow-control loopalready handled a negative window correctly once the request is accepted).
Also adds scripted-peer unit tests in
h2_server_test.vcovering each rejectedcase and a valid POST-with-trailers, and the existing
h2_*_test.vsuite staysgreen.
🧙 Built with WOZCODE