Fix WebSocket upgrade with custom Sec-WebSocket-Protocol header#25774
Fix WebSocket upgrade with custom Sec-WebSocket-Protocol header#25774Drejerdk wants to merge 6 commits intooven-sh:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/bun.js/api/server.zigtest/js/web/websocket/websocket-server-upgrade-protocol.test.ts
🧰 Additional context used
📓 Path-based instructions (5)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/bun.js/api/server.zig
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/bun.js/api/server.zig
🧠 Learnings (11)
📚 Learning: 2025-10-18T05:23:24.403Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/**/*.test.ts?(x) : Always use `port: 0` in tests - do not hardcode ports or use custom random port number functions
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-10-19T04:55:33.099Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.tssrc/bun.js/api/server.zig
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.
Applied to files:
src/bun.js/api/server.zig
📚 Learning: 2025-10-26T04:50:17.892Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24086
File: src/bun.js/api/server.zig:2534-2535
Timestamp: 2025-10-26T04:50:17.892Z
Learning: In src/bun.js/api/server.zig, u32 is acceptable for route indices and websocket_context_index fields, as the practical limit of 4 billion contexts will never be reached.
Applied to files:
src/bun.js/api/server.zig
📚 Learning: 2025-11-12T04:11:52.293Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 24622
File: src/deps/uws/us_socket_t.zig:112-113
Timestamp: 2025-11-12T04:11:52.293Z
Learning: In Bun's Zig codebase, when passing u32 values to C FFI functions that expect c_uint parameters, no explicit intCast is needed because c_uint is equivalent to u32 on Bun's target platforms and Zig allows implicit coercion between equivalent types. This pattern is used consistently throughout src/deps/uws/us_socket_t.zig in functions like setTimeout, setLongTimeout, and setKeepalive.
Applied to files:
src/bun.js/api/server.zig
🔇 Additional comments (1)
src/bun.js/api/server.zig (1)
979-985: LGTM! Clean fix for RFC 6455 compliance.The implementation correctly removes the
Sec-WebSocket-Protocolheader from custom headers before writing the upgrade response, preventing duplicate headers that would cause WebSocket clients to reject the connection with code 1002. The protocol value is preserved (extracted at line 948) and properly passed toupgrade()at line 1025.The comment clearly explains the rationale and references the relevant line number.
There was a problem hiding this comment.
Actionable comments posted: 8
Fix all issues with AI Agents 🤖
In @docs/pm/npmrc.mdx:
- Line 75: Add a one-line descriptive note for the `email` option in the npmrc
options list: explain that `email` is used as the account email associated with
the npm registry/auth credentials (e.g., "account email used for registry
authentication"), mirroring the style of other inline comments like "base64
encoded password" so users know when to set it.
- Around line 178-194: Clarify the precedence between the omit and include
settings by adding a short explanatory sentence and a concrete example after the
existing code block showing that include wins when both are set (e.g., when
omit=dev and include=dev the dev dependencies are installed), reference the
`omit` and `include` keys and the example block so readers see that include
overrides omit and which values (`dev`, `peer`, `optional`) this applies to.
- Around line 162-176: Add a matching bunfig.toml example block to the CA
certificate section showing how to configure install.ca and install.cafile
(parity with the existing npmrc examples); include a toml code fence titled like
"toml bunfig.toml icon=\"settings\"" and demonstrate a single CA via install.ca
= "...", multiple CAs as install.ca = [ "...", "..." ], and a cafile path via
install.cafile = "/path/to/ca-bundle.crt" so readers see the exact keys
(install.ca, install.cafile) and formats to use.
- Around line 114-122: Add a bunfig.toml equivalent note under the
"ignore-scripts" section: document that the same behavior is controlled via the
install.ignoreScripts setting in bunfig.toml by adding a collapsible
"bunfig.toml equivalent" details block showing the [install] table with
ignoreScripts set to true; reference the existing "ignore-scripts" heading so
readers see the bunfig.toml mapping alongside the .npmrc example.
- Around line 196-245: Add bunfig.toml equivalents and examples next to the
npmrc examples in the "install-strategy and node-linker" and
"public-hoist-pattern and hoist-pattern" sections: document that bun uses
camelCase keys install.linker (maps to install-strategy/node-linker),
install.publicHoistPattern (maps to public-hoist-pattern), and
install.hoistPattern (maps to hoist-pattern), and show concise bunfig.toml
snippets demonstrating symlinked vs hoisted linker values and how to set
publicHoistPattern/hoistPattern arrays to mirror the npmrc examples.
In @docs/style.css:
- Around line 152-172: Selectors for [language="shellscript"] and
[language="powershell"] are missing the .code-block qualifier and should be
scoped like the other rules; update the four selectors (the two for shellscript:
[language="shellscript"] code span.line:not(:empty):has(span)::before and the
two with :has(> span:first-child[style*="..."])::before, and the equivalent two
for powershell) to prefix .code-block (e.g., .code-block[language="shellscript"]
...) so the rules only target code blocks and match the established pattern used
for .code-block[language="text"].
In @src/deps/zig-clap/clap.zig:
- Around line 117-166: parseLongNames currently sets alias_count by counting '/'
separators which can overallocate the aliases array; change the logic to count
only segments that start with "--" (and exclude the primary name) before
allocating result. Specifically, in parseLongNames replace the initial
separator-based alias_count with a pass over mem.splitScalar(u8, param_str, '/')
that increments alias_count only when mem.startsWith(u8, name_part, "--") and
the part is not the primary; then allocate result: [alias_count][]const u8 and
fill it in the second pass (the aliases block) using the same filtering/skipping
logic so no uninitialized entries are returned from parseLongNames.
In @test/js/web/html/URLSearchParams.test.ts:
- Around line 215-220: The test "size property should be configurable (issue
#9251)" belongs inside the existing describe block named "non-standard
extensions" (which already contains the `.length` property test) rather than at
the top-level; move the entire it(...) block into that describe("non-standard
extensions") scope, placing it alongside the `.length` test so related
non-standard property descriptor assertions are grouped together and test
ordering remains consistent.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📥 Commits
Reviewing files that changed from the base of the PR and between 6862022 and ca7d336fd65b03d95fceb5463736fb7b20213ea1.
📒 Files selected for processing (13)
cmake/tools/SetupBuildkite.cmakedocs/pm/isolated-installs.mdxdocs/pm/npmrc.mdxdocs/project/building-windows.mdxdocs/project/contributing.mdxdocs/runtime/templating/create.mdxdocs/style.csssrc/bun.js/bindings/webcore/JSURLSearchParams.cppsrc/cli/Arguments.zigsrc/deps/zig-clap/clap.zigsrc/deps/zig-clap/clap/comptime.zigsrc/deps/zig-clap/clap/streaming.zigtest/js/web/html/URLSearchParams.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Private fields in Zig are fully supported using the#prefix:struct { #foo: u32 };
Use decl literals in Zig for declaration initialization:const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer@importat the bottom of the file (auto formatter will move them automatically)
Files:
src/deps/zig-clap/clap.zigsrc/deps/zig-clap/clap/comptime.zigsrc/cli/Arguments.zigsrc/deps/zig-clap/clap/streaming.zig
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/deps/zig-clap/clap.zigsrc/deps/zig-clap/clap/comptime.zigsrc/cli/Arguments.zigsrc/deps/zig-clap/clap/streaming.zig
src/bun.js/bindings/**/*.cpp
📄 CodeRabbit inference engine (CLAUDE.md)
src/bun.js/bindings/**/*.cpp: Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Define properties using HashTableValue arrays in C++ JavaScript class bindings
Add iso subspaces for C++ classes with fields in JavaScript class bindings
Cache structures in ZigGlobalObject for JavaScript class bindings
Files:
src/bun.js/bindings/webcore/JSURLSearchParams.cpp
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun:testwith files that end in*.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
Files:
test/js/web/html/URLSearchParams.test.ts
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/js/web/html/URLSearchParams.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/js/web/html/URLSearchParams.test.ts
🧠 Learnings (28)
📚 Learning: 2025-11-24T18:37:11.466Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/js/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:11.466Z
Learning: Write JS builtins for Bun's Node.js compatibility and APIs, and run `bun bd` after changes
Applied to files:
docs/pm/isolated-installs.mdxdocs/project/contributing.mdxdocs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/js/node/test/{parallel,sequential}/*.js : For test/js/node/test/{parallel,sequential}/*.js files without a .test extension, use `bun bd <file>` instead of `bun bd test <file>` since these expect exit code 0 and don't use bun's test runner
Applied to files:
docs/pm/isolated-installs.mdxdocs/project/building-windows.mdx
📚 Learning: 2025-11-20T19:51:32.288Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24880
File: packages/bun-vscode/package.json:382-385
Timestamp: 2025-11-20T19:51:32.288Z
Learning: In the Bun repository, dependencies may be explicitly added to package.json files (even when not directly imported in code) to force version upgrades on transitive dependencies, particularly as part of Aikido security scanner remediation to ensure vulnerable transitive dependencies resolve to patched versions.
Applied to files:
docs/pm/isolated-installs.mdxdocs/pm/npmrc.mdxdocs/project/contributing.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : When spawning Bun processes in tests, use `bunExe` and `bunEnv` from `harness` to ensure the same build of Bun is used and debug logging is silenced
Applied to files:
docs/pm/isolated-installs.mdxdocs/project/contributing.mdxdocs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
docs/pm/isolated-installs.mdxdocs/project/contributing.mdxdocs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/napi/napi.zig : For each new V8 C++ method, add both GCC/Clang and MSVC mangled symbol names to the V8API struct in src/napi/napi.zig using extern fn declarations
Applied to files:
src/deps/zig-clap/clap.zig
📚 Learning: 2025-11-24T18:36:38.104Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:38.104Z
Learning: Applies to src/**/*.zig : Private fields in Zig are fully supported using the `#` prefix: `struct { #foo: u32 };`
Applied to files:
src/deps/zig-clap/clap.zig
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/deps/zig-clap/clap.zigsrc/deps/zig-clap/clap/comptime.zigsrc/cli/Arguments.zigsrc/deps/zig-clap/clap/streaming.zig
📚 Learning: 2025-11-20T01:27:21.509Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 0
File: :0-0
Timestamp: 2025-11-20T01:27:21.509Z
Learning: In Bun's documentation, curl-based install commands should use `bun.com/install`, while Windows PowerShell irm (Invoke-RestMethod) commands should use `bun.sh/install` due to a bug with .com on Windows.
Applied to files:
docs/pm/npmrc.mdxdocs/project/contributing.mdxdocs/project/building-windows.mdx
📚 Learning: 2025-09-13T17:01:41.393Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 21459
File: docs/api/import-meta.md:0-0
Timestamp: 2025-09-13T17:01:41.393Z
Learning: For docs/api/import-meta.md, keep documentation entries concise and consistent with the existing style - brief descriptions with small inline examples, not extensive bullet points or detailed explanations.
Applied to files:
docs/project/contributing.mdx
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
docs/project/contributing.mdxdocs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Use `bun bd test <...test file>` to run tests, not `bun test`. The `bun test` command will not include your changes.
Applied to files:
docs/project/contributing.mdxdocs/project/building-windows.mdx
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Do not set a timeout when running `bun bd` (debug build command)
Applied to files:
docs/project/contributing.mdx
📚 Learning: 2025-10-01T21:59:54.571Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h:47-74
Timestamp: 2025-10-01T21:59:54.571Z
Learning: In the new bindings generator (bindgenv2) for `src/bun.js/bindings/webcore/JSDOMConvertEnumeration.h`, the context-aware enumeration conversion overloads intentionally use stricter validation (requiring `value.isString()` without ToString coercion), diverging from Web IDL semantics. This is a design decision documented in comments.
Applied to files:
src/bun.js/bindings/webcore/JSURLSearchParams.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to src/bun.js/bindings/**/*.cpp : Define properties using HashTableValue arrays in C++ JavaScript class bindings
Applied to files:
src/bun.js/bindings/webcore/JSURLSearchParams.cpp
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/src/symbols.txt : Add symbol names without leading underscore to src/symbols.txt for each new V8 API method
Applied to files:
src/bun.js/bindings/webcore/JSURLSearchParams.cpp
📚 Learning: 2025-09-05T18:44:43.223Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/bun.js/bindings/bindings.cpp:6429-6432
Timestamp: 2025-09-05T18:44:43.223Z
Learning: The JSC::JSMap::size() method in JavaScriptCore returns a value that can be represented as uint32_t, and the binding functions in src/bun.js/bindings/bindings.cpp correctly use uint32_t as the return type for JSC__JSMap__size. JavaScript Maps are practically limited to 2^32 - 1 elements.
Applied to files:
src/bun.js/bindings/webcore/JSURLSearchParams.cpp
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Code generation happens automatically during the build process - bundled modules can be reloaded without rebuilding Zig by running `bun run build`
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Run `bun run zig:check-all` to compile Zig code on all platforms when making platform-specific changes
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Only push changes after running `bun bd test <file>` and ensuring tests pass
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` with files that end in `*.test.{ts,js,jsx,tsx,mjs,cjs}`
Applied to files:
docs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `-e` flag for single-file tests when spawning Bun processes
Applied to files:
docs/project/building-windows.mdxdocs/runtime/templating/create.mdx
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Unit tests for specific features are organized by module (e.g., `/test/js/bun/`, `/test/js/node/`)
Applied to files:
docs/project/building-windows.mdx
📚 Learning: 2025-11-14T16:07:01.064Z
Learnt from: RiskyMH
Repo: oven-sh/bun PR: 24719
File: docs/bundler/executables.mdx:527-560
Timestamp: 2025-11-14T16:07:01.064Z
Learning: In the Bun repository, certain bundler features like compile with code splitting (--compile --splitting) are CLI-only and not supported in the Bun.build() JavaScript API. Tests for CLI-only features use backend: "cli" flag (e.g., test/bundler/bundler_compile_splitting.test.ts). The CompileBuildConfig interface correctly restricts these with splitting?: never;. When documenting CLI-only bundler features, add a note clarifying they're not available via the programmatic API.
Applied to files:
docs/runtime/templating/create.mdx
📚 Learning: 2025-10-19T02:52:37.412Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/tsconfig.json:1-15
Timestamp: 2025-10-19T02:52:37.412Z
Learning: In the Bun repository, packages under packages/ (e.g., bun-otel) can follow a TypeScript-first pattern where package.json exports point directly to .ts files (not compiled .js files). Bun natively runs TypeScript, so consumers import .ts sources directly and receive full type information without needing compiled .d.ts declaration files. For such packages, adding "declaration": true or "outDir" in tsconfig.json is unnecessary and would break the export structure.
<!-- [remove_learning]
ceedde95-980e-4898-a2c6-40ff73913664
Applied to files:
docs/runtime/templating/create.mdx
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/js/web/html/URLSearchParams.test.ts
🪛 LanguageTool
docs/project/contributing.mdx
[style] ~351-~351: Consider an alternative verb to strengthen your wording.
Context: ...library not found for -lSystem` If you see this error when compiling, run: ```bas...
(IF_YOU_HAVE_THIS_PROBLEM)
🔇 Additional comments (13)
docs/project/building-windows.mdx (1)
17-17: Documentation formatting improvement approved.These changes consistently remove leading whitespace/prompts from PowerShell and bash code blocks, improving readability without altering command content or instructions.
Also applies to: 25-25, 33-33, 51-54, 66-66, 72-72, 78-78, 89-89, 92-92, 98-98, 114-114, 118-118, 121-122
docs/project/contributing.mdx (1)
31-31: Documentation formatting improvement approved.Consistent removal of leading whitespace/prompts from bash code blocks across installation and build instructions. Commands and their functionality remain unchanged.
Also applies to: 35-35, 39-39, 43-43, 47-47, 59-59, 63-63, 67-68, 79-79, 82-82, 85-85, 88-88, 91-91, 103-103, 108-108, 112-112, 116-116, 120-120, 130-130, 140-140, 145-145, 165-165, 264-264, 267-267, 274-274, 325-338, 346-346, 354-354
docs/pm/isolated-installs.mdx (1)
192-192: Documentation formatting improvement approved.Removal of leading whitespace/prompt from the pnpm migration command, consistent with formatting improvements across related documentation files.
docs/runtime/templating/create.mdx (1)
22-22: LGTM! Documentation formatting improvement.Removing the shell prompt symbol (
$) makes the command easier to copy and paste.cmake/tools/SetupBuildkite.cmake (1)
51-52: LGTM! Proper JSON escaping for CMake.The backslash escaping prevents CMake from misinterpreting JSON escape sequences in commit messages (e.g.,
\n,\t) that appear in the Buildkite JSON payload.src/bun.js/bindings/webcore/JSURLSearchParams.cpp (1)
186-186: LGTM! Makes URLSearchParams.prototype.size configurable and enumerable per Web IDL spec.Removing the
DontDeleteandDontEnumattributes alignssizewith the WHATWG URL Standard, which specifies that readonly IDL attributes should have[[Configurable]]: trueand[[Enumerable]]: true. This matches test expectations in test/js/web/html/URLSearchParams.test.ts (line 215) and resolves issue #9251.src/deps/zig-clap/clap.zig (1)
14-26: LGTM! Clean implementation of alias matching.The
long_aliasesfield andmatchesLongmethod provide a clean abstraction for alias support. The implementation correctly checks the primary long name first before iterating through aliases.src/deps/zig-clap/clap/streaming.zig (1)
63-65: LGTM! Clean refactor to use the new alias-aware matching.The change correctly delegates long-name matching to
matchesLong, which handles both primary names and aliases. Thecontinuebehavior on non-match is preserved.src/deps/zig-clap/clap/comptime.zig (2)
159-163: LGTM! Alias support correctly added to hasFlag.The implementation properly iterates through
long_aliasesand applies the"--"prefix for matching, consistent with how primary long names are handled.
181-185: LGTM! Alias support correctly added to findParam.The implementation mirrors
hasFlagand ensures that queries using either the primary name or any alias will resolve to the same parameter.src/cli/Arguments.zig (2)
221-221: Good addition of --grep alias for test filtering.The
--grepalias aligns with common testing tool conventions (e.g., Mocha, Jest). SincefindParamin comptime.zig now checks aliases, values passed via--grepwill be correctly retrieved byargs.option("--test-name-pattern")at line 545.
545-559: The --grep alias is correctly handled through the clap library's alias mechanism.The parameter definition at line 221 uses the
--test-name-pattern/--grepsyntax to register--grepas an alias. ThefindParam()function incomptime.zigchecks both the primary name and all long aliases, returning the same param object for both. Sinceargs.option("--test-name-pattern")retrieves values by param ID (which is identical for the primary name and alias), it will correctly capture values passed via either--test-name-patternor--grepat runtime.docs/pm/npmrc.mdx (1)
178-194: Add cross-reference tobunfig.tomlequivalents foromitandincludeconfiguration.While
.npmrc'somitandincludeoptions are documented here, Bun also supports equivalent dependency type control viabunfig.tomlunder the[install]section usingdev,optional,peer, andproductionboolean flags. Add a note or link to thebunfig.tomldocumentation to help users discover the Bun-native configuration approach.
|
Hm, I probably messed up updating main branch in my fork and pushing the merged changes. |
Fixes oven-sh#25773 When server.upgrade() was called with custom headers including Sec-WebSocket-Protocol, the header was written twice: once by headers.toUWSResponse() and once by the upgrade() function. This caused duplicate headers in the HTTP 101 response, which violates RFC 6455 Section 4.2.2 (at most one protocol header) and causes clients to reject the connection with error code 1002 'Mismatch client protocol'. The fix removes Sec-WebSocket-Protocol from custom headers before writing them to the response, since the upgrade() function handles this header properly. The protocol value is preserved and passed to upgrade(). Test coverage: Added comprehensive tests for server.upgrade() with custom Sec-WebSocket-Protocol header selecting different protocols from the client's offered list.
|
Any chance this can be reviewed soon? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/js/web/websocket/websocket-server-upgrade-protocol.test.ts`:
- Around line 126-164: The test currently only asserts ws.protocol and never
verifies that the custom header survives; update the test (test "should work
with other custom headers alongside Sec-WebSocket-Protocol") to either (A)
explicitly assert the 101 Switching Protocols response contains
"X-Custom-Header" by capturing the raw upgrade response when server.upgrade is
called (refer to server.upgrade and the headers object where "X-Custom-Header"
is set) and assert that the client-side/upgrade response includes that header,
or (B) if you cannot access the raw response in this harness, rename the test to
reflect that it only covers protocol negotiation (e.g., change the test title
and keep the existing ws.protocol assertion) so it does not claim to verify
other custom headers.
- Around line 19-21: The tests only exercise the plain-object branch of
options.headers for server.upgrade and miss the caller-supplied Headers instance
path which can be mutated by the new fix; add test cases that call
server.upgrade with options.headers set to a new Headers(...) instance
(containing "Sec-WebSocket-Protocol": protocols[0]) for each affected location
(the calls around lines with server.upgrade at 19-21, 58-60, 99-101, 137-142)
and assert that the response includes the expected Sec-WebSocket-Protocol and
that the original Headers instance is not mutated (i.e., its entries remain
unchanged) to ensure the Headers branch is exercised and no in-place mutation or
duplicate header occurs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66e8c2f6-be8b-4579-8434-4919044d2e0a
📒 Files selected for processing (1)
test/js/web/websocket/websocket-server-upgrade-protocol.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
The bugs I flagged previously (use-after-free and missing SecWebSocketExtensions handling) have been fixed — the code now looks correct. However, the memory ownership changes in the WebSocket upgrade path warrant a human reviewer verifying the toSliceClone/fastRemove ordering and the clone-before-mutation guards.
Extended reasoning...
Overview
This PR fixes duplicate Sec-WebSocket-Protocol (and Sec-WebSocket-Extensions) headers in the HTTP 101 Switching Protocols response during server.upgrade(). It modifies src/bun.js/api/server.zig in two parallel code paths (nodeHttp and Bun.serve) and adds a comprehensive test file with 322 lines covering both plain-object and Headers-instance header variants, plus extension header tests.
Security risks
The primary risk was a use-after-free where fastGet returned a non-owning ZigString and fastRemove freed the underlying StringImpl. The current version correctly copies string data via toSliceClone before removing headers, and clones caller-owned Headers objects before mutation. No new security risks are apparent, but the correctness of the memory ownership model should be verified by someone familiar with the FetchHeaders/ZigString lifecycle.
Level of scrutiny
This deserves careful human review. The changes involve subtle memory management (owned slices with deferred cleanup, conditional cloning of caller-provided objects, ordering constraints between copy/remove operations) in the hot path of WebSocket connection upgrades. While the pattern is applied consistently across both code paths, a mistake here could cause use-after-free crashes or data corruption in production.
Other factors
The author has been waiting for review since December 30, 2025. The fix addresses two linked issues (#25773, #18243). All previously reported bugs (by both me and CodeRabbit) have been addressed. The test coverage is thorough, exercising both the protocol and extensions headers, both header input types, and raw HTTP response inspection for duplicate header detection.
1 participant
Problem
When using
server.upgrade()with custom headers includingSec-WebSocket-Protocol, the WebSocket connection would fail with error code 1002 and reason "Mismatch client protocol", even when selecting a valid protocol from the client's offered list.Root Cause
The bug was caused by duplicate
Sec-WebSocket-Protocolheaders being sent in the HTTP 101 Switching Protocols response:Sec-WebSocket-Protocol) were written viaheaders.toUWSResponse()inserver.zigline 979resp.upgrade()function was then called (line 1011), which again wrote theSec-WebSocket-Protocolheader inHttpResponse.hline 263This resulted in duplicate headers:
RFC 6455 Section 4.2.2 specifies that the server MUST include at most one
Sec-WebSocket-Protocolheader in the response. Bun's WebSocket client correctly validates this and rejects connections with duplicate headers.Solution
Remove
Sec-WebSocket-Protocolfrom custom headers before writing them to the response, since theupgrade()function handles this header properly:The protocol value is preserved and passed to
upgrade(), so the server's protocol selection is respected.Changes
src/bun.js/api/server.zig- Remove duplicate header (7 lines)test/js/web/websocket/websocket-server-upgrade-protocol.test.ts- Comprehensive test coverage (174 lines)Test Coverage
Added 4 new tests covering:
Sec-WebSocket-ProtocolAll existing WebSocket tests continue to pass (77/79 pass, 2 unrelated failures).
Why Wasn't This Caught Before?
Existing tests like
websocket-subprotocol-strict.test.tsandwebsocket-custom-headers.test.tstest:net.createServer()But no existing test covered the specific case of using
Bun.serve()withserver.upgrade()and custom response headers, which is the code path this bug affected.Impact
This fix allows servers to properly negotiate WebSocket subprotocols by selecting any protocol from the client's offered list, not just relying on automatic selection. This is essential for protocols like OCPP (Open Charge Point Protocol) where clients may offer multiple protocol versions and the server needs to select the most appropriate one.
How did you verify your code works?
A debug build of bun was executed locally and the existing and new tests are passing.
All the new tests fail when Bun 1.3.5 is used.
Fixes #25773, #18243
This is my first, open-source contribution. Please let me know if any changes are needed