fix(ext/node): enforce HTTP/2 server header list size limits#33494
Conversation
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Approach is right and the test scenario works through cleanly: server's local SETTINGS_MAX_HEADER_LIST_SIZE = 100 propagates via nghttp2_session_get_local_settings, and the cumulative current_length + header_length check in add_header rejects once the running total crosses 100. The pseudo-headers themselves push the running total past 100 before the giant foo header even arrives, but that's still observable as an NGHTTP2_ENHANCE_YOUR_CALM RST to the client, which is what the test asserts. The empty-name short-circuit and the order-of-checks match Node's Http2Stream::AddHeader in src/node_http2.cc.
Two real divergences from Node's AddHeader worth resolving before this lands — both flagged inline. Neither breaks the test this PR enables, but the first one means this PR can't co-exist with #33495 (same Session::max_header_pairs field, contradictory defaulting logic), so the two need to converge. Marking COMMENT rather than approving until the overlap with #33495 is sorted.
| DEFAULT_MAX_HEADER_LIST_PAIRS | ||
| } else { | ||
| pairs | ||
| }; |
There was a problem hiding this comment.
This pairs == 0 → DEFAULT_MAX_HEADER_LIST_PAIRS mapping diverges from Node in two ways:
- Node distinguishes "user explicitly set 0" from "option not provided" by checking the
FLAGSbit (seesrc/node_http2.ccand the parallel logic in PR fix(ext/node): rebind http2 state buffers post-snapshot, enforce maxHeaderListPairs #33495). Here, both cases collapse to 128. - When the user does set a low value (or 0), Node clamps to a per-session-type minimum: 4 for servers, 1 for clients (
node_http_common-inl.h: GetServerMaxHeaderPairs / GetClientMaxHeaderPairs). This PR has no clamp, somaxHeaderListPairs: 0produces an effective limit of 128 instead of Node's 4 (server) / 1 (client).
Concrete fallout: parallel/test-http2-too-many-headers.js (which #33495 enables) constructs a server with maxHeaderListPairs: 0 and expects rejection at the 5th header. Under this PR's logic that test would fail because max_header_pairs resolves to 128, not 4.
Since #33495 already implements the flag-aware read + min-clamp on this same field, the two PRs need to converge — please coordinate and pick one defaulting strategy (the #33495 one matches Node).
| // Http2Stream::AddHeader behaviour. | ||
| if current_pairs >= max_header_pairs | ||
| || current_length.saturating_add(header_length as u64) > max_header_length | ||
| { |
There was a problem hiding this comment.
Node's Http2Stream::AddHeader also gates on session-wide memory budget:
if (!session_->has_available_session_memory(length) ||
current_headers_.size() == max_header_pairs_ ||
current_headers_length_ + length > max_header_length_) {
return false;
}This PR drops the has_available_session_memory arm. Without it a single session could accept headers that fit per-stream but exhaust the per-session memory budget. Not exercised by the test you're enabling, but worth flagging since the comment claims this "mirrors Node's Http2Stream::AddHeader behaviour" — it doesn't quite.
- Read OptionsIndex::MaxHeaderListPairs only when its FLAGS bit is set, so an unset option no longer collapses to the same path as an explicit zero. - Apply Node's per-session-type minimum (server: 4, client: 1) after defaulting, matching GetServerMaxHeaderPairs / GetClientMaxHeaderPairs in node_http_common-inl.h. - Document the Http2Stream::AddHeader divergence around session memory accounting; Deno does not yet track per-session memory, so the has_available_session_memory arm is intentionally omitted. Addresses review feedback on denoland#33494.
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM after 959a5c7a. Both inline concerns from the prior review are addressed precisely:
Http2Options::newnow gates themaxHeaderListPairsread on the FLAGS bit (flags & (1 << OptionsIndex::MaxHeaderListPairs as u32)), so an unset slot doesn't masquerade as a user-supplied 0, and applies the per-session-type minimum (Server => 4,Client => 1) afterwards. With this,parallel/test-http2-too-many-headers.js'smaxHeaderListPairs: 0 → effective 4semantics now hold, which means this PR can co-exist with #33495's enforcement-side change without contradiction.- The
add_headercomment is updated to acknowledge the missinghas_available_session_memoryarm and points atsrc/node_http2.ccso the next person to wire up per-session memory accounting in Deno knows where to plug it in.
The two PRs (#33494 and #33495) implement the same flag-aware-read + min-clamp logic on the same Http2Options struct in slightly different styles (#33495 uses a mut binding outside the with_options closure; this PR uses a tuple destructure with mut outside). They'll trivially merge-conflict; either resolution works.
- Read OptionsIndex::MaxHeaderListPairs only when its FLAGS bit is set, so an unset option no longer collapses to the same path as an explicit zero. - Apply Node's per-session-type minimum (server: 4, client: 1) after defaulting, matching GetServerMaxHeaderPairs / GetClientMaxHeaderPairs in node_http_common-inl.h. - Document the Http2Stream::AddHeader divergence around session memory accounting; Deno does not yet track per-session memory, so the has_available_session_memory arm is intentionally omitted. Addresses review feedback on denoland#33494. Co-authored-by: Divy Srivastava <[email protected]>
959a5c7 to
93c3685
Compare
|
@divybot merge main |
- Read OptionsIndex::MaxHeaderListPairs only when its FLAGS bit is set, so an unset option no longer collapses to the same path as an explicit zero. - Apply Node's per-session-type minimum (server: 4, client: 1) after defaulting, matching GetServerMaxHeaderPairs / GetClientMaxHeaderPairs in node_http_common-inl.h. - Document the Http2Stream::AddHeader divergence around session memory accounting; Deno does not yet track per-session memory, so the has_available_session_memory arm is intentionally omitted. Addresses review feedback on denoland#33494. Co-authored-by: Divy Srivastava <[email protected]>
93c3685 to
2f9edbf
Compare
Enables tests/node_compat/runner/suite/test/parallel/test-http2-too-large-headers.js Co-authored-by: Divy Srivastava <[email protected]>
- Read OptionsIndex::MaxHeaderListPairs only when its FLAGS bit is set, so an unset option no longer collapses to the same path as an explicit zero. - Apply Node's per-session-type minimum (server: 4, client: 1) after defaulting, matching GetServerMaxHeaderPairs / GetClientMaxHeaderPairs in node_http_common-inl.h. - Document the Http2Stream::AddHeader divergence around session memory accounting; Deno does not yet track per-session memory, so the has_available_session_memory arm is intentionally omitted. Addresses review feedback on denoland#33494. Co-authored-by: Divy Srivastava <[email protected]>
34646f7 to
ac3cad3
Compare
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM after the rebase. The conflict with #33495 is gone — that PR merged the Http2Options::max_header_pairs field, flag-aware read, and per-session-type minimum clamp on its own. This PR's net change against base is now the slim 34/-4 piece I expected:
add_headershort-circuits for empty header names (matches Node'sHttp2Stream::AddHeaderIsZeroLength(name)early return).- The reject condition now combines both
current_pairs >= max_header_pairsANDcurrent_length + header_length > local SETTINGS_MAX_HEADER_LIST_SIZE, returningfalseto trip the existingnghttp2_submit_rst_stream(NGHTTP2_ENHANCE_YOUR_CALM)path. - The comment honestly notes the
has_available_session_memoryarm is still missing and points at where to wire it up later if Deno gains per-session memory accounting. - Test config enables
test-http2-too-large-headers.js; the count-limit test (test-http2-too-many-headers.js) is already enabled via #33495's merge.
CI is still building (33 pending) but no failures yet.
Enables tests/node_compat/runner/suite/test/parallel/test-http2-too-large-headers.js Co-authored-by: Divy Srivastava <[email protected]>
Signed-off-by: Divy Srivastava <[email protected]>
3 participants
Summary
Enables
test-http2-too-large-headersin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-too-large-headers