Skip to content

fix(http2): honor maxSettings option per RFC 9113 §6.5 #29041

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cirospaciari wants to merge 2 commits into from
Closed

fix(http2): honor maxSettings option per RFC 9113 §6.5 #29041

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6c8ae6b
fix(http2): honor maxSettings option per RFC 9113 §6.5
cirospaciari Apr 8, 2026
8eb4ce8
suppress SETTINGS ACK when rejecting frame with GOAWAY
cirospaciari Apr 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/bun.js/api/bun/h2_frame_parser.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -667,6 +667,9 @@
maxHeaderListPairs: u32 = 128,
maxRejectedStreams: u32 = 100,
maxOutstandingSettings: u32 = 10,
/// RFC 9113 §6.5: limits the number of settings entries accepted in a
/// single received SETTINGS frame. Node.js defaults to 32.
maxSettings: u32 = 32,
outstandingSettings: u32 = 0,
rejectedStreams: u32 = 0,
maxSessionMemory: u32 = 10, //this limit is in MB
Expand Down Expand Up @@ -2426,12 +2429,18 @@
if (frame.length > 0) {
if (isACK or frame.length % settingByteSize != 0) {
log("invalid settings frame size", .{});
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

Check failure on line 2443 in src/bun.js/api/bun/h2_frame_parser.zig

View check run for this annotation

Claude / Claude Code Review

Spurious SETTINGS ACK sent after GOAWAY when maxSettings is exceeded 

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-valida
Comment on lines 2436 to 2448

@claude claude Bot Apr 8, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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:

  1. isACK is false (if it were true, the check at line 2430 would already have returned—and that early return does NOT trigger the defer because isACK is true there).
  2. Execution reaches line 2436: entry_count > this.maxSettings.
  3. sendGoAway is called, writing a GOAWAY(ENHANCE_YOUR_CALM) frame.
  4. return data.len (line 2439) unwinds the defer.
  5. isACK is false, so sendSettingsACK() 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 down
  • SETTINGS_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):

  1. handleSettingsFrame called. isACK = false.
  2. Line 2426: defer if (\!isACK) this.sendSettingsACK(); registered (captures isACK = false).
  3. Line 2430: condition is false or false → falls through.
  4. Line 2435: entry_count = 2. Line 2436: 2 > 1 → true.
  5. Line 2438: sendGoAway(..., ENHANCE_YOUR_CALM, ...) writes GOAWAY frame to socket.
  6. Line 2439: return data.len — defer fires → sendSettingsACK() writes SETTINGS_ACK frame to socket.
  7. 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.

log("settings frame ACK", .{});

// we can now write any request
Expand Down Expand Up @@ -4720,6 +4729,11 @@
this.maxOutstandingSettings = @max(1, @as(u32, @truncate(max_outstanding_settings.to(u64))));
}
}
if (try settings_js.get(globalObject, "maxSettings")) |max_settings| {
if (max_settings.isNumber()) {
this.maxSettings = @max(1, @as(u32, @truncate(max_settings.to(u64))));
}
}
if (try settings_js.get(globalObject, "maxSendHeaderBlockLength")) |max_send_header_block_length| {
if (max_send_header_block_length.isNumber()) {
this.maxSendHeaderBlockLength = @bitCast(max_send_header_block_length.toInt32());
Expand Down
52 changes: 52 additions & 0 deletions test/js/node/test/parallel/test-http2-max-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');

const http2 = require('http2');

const server = http2.createServer({ maxSettings: 1 });

// TODO(@jasnell): There is still a session event
// emitted on the server side but it will be destroyed
// immediately after creation and there will be no
// stream created.
server.on('session', common.mustCall((session) => {
session.on('stream', common.mustNotCall());
session.on('remoteSettings', common.mustNotCall());
}, 2));
server.on('stream', common.mustNotCall());

server.listen(0, common.mustCall(() => {
// Specify two settings entries when a max of 1 is allowed.
// Connection should error immediately.
const client = http2.connect(
`http://localhost:${server.address().port}`, {
settings: {
// The actual settings values do not matter.
headerTableSize: 1000,
enablePush: false,
},
});

client.on('error', common.mustCall((err) => {
// The same but with custom settings
const client2 = http2.connect(
`http://localhost:${server.address().port}`, {
settings: {
// The actual settings values do not matter.
headerTableSize: 1000,
customSettings: {
0x14: 45
}
},
});

client2.on('error', common.mustCall(() => {
server.close();
}));
}));


}));
Loading