fix(http2): honor maxSettings option per RFC 9113 §6.5#29041
fix(http2): honor maxSettings option per RFC 9113 §6.5#29041cirospaciari wants to merge 2 commits intomainfrom
Conversation
Node.js supports {maxSettings: N} (default 32) to bound entries in a received
SETTINGS frame; exceeding it tears down the session with ENHANCE_YOUR_CALM.
Bun ignored the option.
Adds maxSettings field, reads it from JS options, and rejects frames with too
many entries before applying any setting.
Ports Node's test/parallel/test-http2-max-settings.js.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Converting to draft: test hangs because maxSettings option may not be plumbed from JS createServer options to the zig parser, or the GOAWAY is not surfacing as a client error event. Needs investigation. |
| this.sendGoAway(frame.streamIdentifier, ErrorCode.FRAME_SIZE_ERROR, "Invalid settings frame size", this.lastStreamID, true); | ||
| return data.len; | ||
| } | ||
| const entry_count: u32 = @intCast(@divTrunc(frame.length, settingByteSize)); | ||
| if (entry_count > this.maxSettings) { | ||
| log("settings frame exceeds maxSettings ({} > {})", .{ entry_count, this.maxSettings }); | ||
| this.sendGoAway(frame.streamIdentifier, ErrorCode.ENHANCE_YOUR_CALM, "SETTINGS frame exceeded maxSettings", this.lastStreamID, true); | ||
| return data.len; | ||
| } | ||
| } else { | ||
| if (isACK) { | ||
| // we received an ACK |
There was a problem hiding this comment.
🔴 The new maxSettings check at line 2436–2439 calls sendGoAway then returns, but the defer if (\!isACK) this.sendSettingsACK() registered at line 2426 fires on every return while isACK is false—so the peer receives both a GOAWAY(ENHANCE_YOUR_CALM) and a SETTINGS_ACK for the same frame. RFC 9113 §6.5 defines SETTINGS_ACK as confirmation that the settings were accepted and applied, which directly contradicts the rejection signaled by GOAWAY. Fix by moving the defer below all frame-validation early-return checks (after line 2440), so it only fires when the frame is fully accepted.
Extended reasoning...
What the bug is and how it manifests
In handleSettingsFrame (line 2418), the very first thing registered after the stream-identifier guard is a deferred call at line 2426:
defer if (\!isACK) this.sendSettingsACK();This defer fires on every return from the function as long as isACK is false. The new maxSettings enforcement added by this PR (lines 2435-2440) calls sendGoAway and then returns immediately, which triggers the defer and sends a SETTINGS_ACK to the peer right after the GOAWAY.
The specific code path that triggers it
When a SETTINGS frame arrives with more entries than maxSettings:
isACKis false (if it were true, the check at line 2430 would already have returned—and that early return does NOT trigger the defer becauseisACKis true there).- Execution reaches line 2436:
entry_count > this.maxSettings. sendGoAwayis called, writing aGOAWAY(ENHANCE_YOUR_CALM)frame.return data.len(line 2439) unwinds the defer.isACKis false, sosendSettingsACK()is called unconditionally.
Why existing code does not prevent it
sendGoAway writes the GOAWAY bytes and dispatches JS events but sets no closed flag on the parser. sendSettingsACK has no guard—it unconditionally writes a SETTINGS ACK frame. write() has no closed-state check either. Nothing prevents the second frame from being enqueued.
Impact
The peer receives two contradictory frames for the same SETTINGS:
GOAWAY(ENHANCE_YOUR_CALM)— your settings were rejected, session torn downSETTINGS_ACK— your settings were received and applied
RFC 9113 §6.5 is explicit: a SETTINGS_ACK MUST only be sent when settings have been applied. Sending it alongside a GOAWAY is a protocol violation that can cause clients to briefly believe their oversized settings were accepted, masking the real error.
Step-by-step proof
Server configured with { maxSettings: 1 }, client sends SETTINGS with two entries (as in the new test):
handleSettingsFramecalled.isACK = false.- Line 2426:
defer if (\!isACK) this.sendSettingsACK();registered (capturesisACK = false). - Line 2430: condition is
false or false→ falls through. - Line 2435:
entry_count = 2. Line 2436:2 > 1→ true. - Line 2438:
sendGoAway(..., ENHANCE_YOUR_CALM, ...)writes GOAWAY frame to socket. - Line 2439:
return data.len— defer fires →sendSettingsACK()writes SETTINGS_ACK frame to socket. - Client receives GOAWAY then SETTINGS_ACK in the same TCP segment or immediately after.
How to fix it
Move the defer below all frame-validation early-return checks (i.e., place it after line 2440), so it only fires on the successful acceptance path. Alternatively, use a local accepted boolean flag set to true only on the non-error path, and condition the ACK on that flag instead of just \!isACK.
Note: the same pre-existing issue exists at the FRAME_SIZE_ERROR path (lines 2430-2433) when isACK=false and frame.length % settingByteSize \!= 0, but this PR introduces a new instance via the maxSettings check.
Node.js supports
{maxSettings: N}(default 32) to bound entries in a received SETTINGS frame; exceeding it tears down the session with ENHANCE_YOUR_CALM. Bun ignored the option.Adds
maxSettings: u32 = 32field, reads it from JS options, and rejects frames with too many entries before applying any setting.Ports Node's
test/parallel/test-http2-max-settings.js.