websocket proxy #25694
websocket proxy #25694
Conversation
WalkthroughThe changes add HTTP proxy and TLS-over-proxy support to WebSocket connections in Bun. This includes new proxy environment variable handling, CONNECT tunneling logic in C++ and Zig layers, SSL tunnel management for encrypted proxy connections, and comprehensive test coverage for proxy scenarios. Changes
Suggested reviewers
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/js/bun/http/proxy.test.js (1)
178-178: UsetempDir()from harness instead oftmpdir()from os.As per coding guidelines, prefer
tempDir()from harness for creating temporary directories in tests instead of manually constructing paths withtmpdir().🔎 Proposed fix
- const path = `${tmpdir()}/bun-test-http-proxy-env-${Date.now()}.ts`; - fs.writeFileSync(path, 'await fetch("https://example.com");'); + using dir = tempDir("bun-test-http-proxy-env", { + "test.ts": 'await fetch("https://example.com");', + }); + const path = `${dir}/test.ts`;Note: This would also require importing
tempDirfromharnessand remove the need for manualfs.unlinkSynccleanup.src/http/websocket_client.zig (2)
370-408: SSLTunnel.decrypt can strand plaintext when application data exceeds BUFFER_SIZEThe
decryptmethod loops whiletotal_read < out_buffer.lenand exits early on SSL errors orWANT_READ/WANT_WRITE. With a fixed 65536-byte buffer, if the peer sends more than that in decrypted plaintext (across multiple TLS records in a single TCP packet), BoringSSL will:
- Decrypt and return only the first ~65536 bytes of plaintext
- Buffer any remaining decrypted or undecrypted records internally
- Never drain that remainder because
decryptis only called when new encrypted data arrivesSince
BIO_writerequiresencrypted_data.len > 0, there is no way to calldecryptwith empty input to drain pending plaintext.BoringSSL provides
SSL_pending()to detect buffered decrypted plaintext. A robust fix would:
- Keep reading via
SSL_readuntilSSL_pendingreturns zero orSSL_readblocks, rather than stopping when the output buffer fills, or- Add a separate "drain pending" method that calls
SSL_readwithout writing new encrypted data, so buffered plaintext can be flushed without relying on fresh encrypted packets
741-788: TLS-over-proxy send path critically bypasses SSLTunnel for large and backpressured framesOnly small frames that fit within
stack_frame_sizeand have no backpressure useenqueueEncodedBytesfor TLS wrapping. All other sends—including frames that exceed the inline buffer or encounter backpressure—route throughsendData→sendDataUncompressed→sendBuffer, which callsthis.tcp.write(out_buf)directly without consultingssl_tunnel, sending plaintext to the network and corrupting the TLS stream.Additionally, when
enqueueEncodedBytesencounters aWouldBlockerror fromtunnel.encrypt(), it stores the original plaintext intosend_bufferviacopyToSendBuffer(bytes, false). Later,handleWritableflushessend_bufferwithout TLS encryption, transmitting those bytes in cleartext.
SSLTunnel.write_buffercan hold encrypted data between calls, andgetPendingEncrypted()exists to retrieve it, but it is never invoked from the WebSocket send path. Remaining buffered encrypted data is never flushed to the network.Restructure the send path so that all outbound frames pass through the TLS layer when
ssl_tunnelis set:
- Modify
sendBufferto consultssl_tunnel, encrypt viassl_tunnel.encrypt(out_buf), and discardsend_bufferbytes once encryption succeeds.- Add a branch in
handleWritableto flushssl_tunnel.getPendingEncrypted()before or instead ofsend_buffer.- Avoid re-enqueueing plaintext into
send_bufferfrom the TLS path; keepsend_bufferlogically "above" TLS whileSSLTunnel.write_buffersits at the network level.Without this fix, any WebSocket frame larger than the inline buffer or sent under backpressure will bypass TLS when proxied.
Also applies to: 884-905, 907-931, 991-1001, 1313-1322
📜 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 (14)
.vscode/launch.jsonsrc/bun.js/bindings/headers.hsrc/bun.js/bindings/webcore/WebSocket.cppsrc/bun.js/bindings/webcore/WebSocket.hsrc/http.zigsrc/http/proxy.zigsrc/http/websocket_client.zigsrc/http/websocket_client/CppWebSocket.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigtest/apptest/js/bun/http/http-server-chunking.test.tstest/js/bun/http/proxy.test.jstest/js/web/websocket/websocket-proxy.test.tsworkspace.code-workspace
🧰 Additional context used
📓 Path-based instructions (6)
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-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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-proxy.test.tstest/js/bun/http/http-server-chunking.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-proxy.test.tstest/js/bun/http/http-server-chunking.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/http/websocket_client/CppWebSocket.zigsrc/http.zigsrc/http/proxy.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigsrc/http/websocket_client.zig
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/http/websocket_client/CppWebSocket.zigsrc/http.zigsrc/http/proxy.zigsrc/http/websocket_client/WebSocketUpgradeClient.zigsrc/http/websocket_client.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/WebSocket.cpp
🧠 Learnings (47)
📚 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 : Use decl literals in Zig for declaration initialization: `const decl: Decl = .{ .binding = 0, .value = 0 };`
Applied to files:
workspace.code-workspace
📚 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 : Prefer `import` at the bottom of the file (auto formatter will move them automatically)
Applied to files:
workspace.code-workspace
📚 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:
.vscode/launch.jsontest/js/web/websocket/websocket-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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: Use `BUN_DEBUG_QUIET_LOGS=1` to disable debug logging, or `BUN_DEBUG_<scopeName>=1` to enable specific scoped debug logging in Zig
Applied to files:
.vscode/launch.json
📚 Learning: 2025-09-19T21:17:04.690Z
Learnt from: cirospaciari
Repo: oven-sh/bun PR: 22756
File: src/js/node/_http_server.ts:945-947
Timestamp: 2025-09-19T21:17:04.690Z
Learning: JSNodeHTTPServerSocket in src/bun.js/bindings/NodeHTTP.cpp implements both end() and close() methods. The end() method properly handles socket termination for both HTTP responses and raw CONNECT sockets by setting ended=true, checking buffered data, and flushing remaining data through the response context.
Applied to files:
src/bun.js/bindings/webcore/WebSocket.hsrc/bun.js/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 Learning: 2025-12-23T06:50:31.577Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 25429
File: src/bun.js/bindings/helpers.h:422-422
Timestamp: 2025-12-23T06:50:31.577Z
Learning: In Bun's C++ bindings, when returning an empty JSC::Identifier and a VM is accessible, prefer using vm.propertyNames->emptyIdentifier over constructing with JSC::Identifier(JSC::Identifier::EmptyIdentifierFlag::EmptyIdentifier). The cached identifier from the VM's property names table is more efficient and consistent with WebKit upgrade patterns. Apply this guidance to src/bun.js/bindings/helpers.h and similar header files in the same bindings directory (i.e., any file that constructs an EmptyIdentifier).
Applied to files:
src/bun.js/bindings/webcore/WebSocket.hsrc/bun.js/bindings/headers.h
📚 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-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.test.ts
📚 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-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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} : Never use hardcoded port numbers in tests. Always use `port: 0` to get a random port
Applied to files:
test/js/web/websocket/websocket-proxy.test.tstest/js/bun/http/proxy.test.js
📚 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-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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-proxy.test.tstest/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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.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:
test/js/web/websocket/websocket-proxy.test.tstest/js/bun/http/http-server-chunking.test.ts
📚 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:
test/js/web/websocket/websocket-proxy.test.ts
📚 Learning: 2025-09-03T01:30:58.001Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: test/js/valkey/valkey.test.ts:264-271
Timestamp: 2025-09-03T01:30:58.001Z
Learning: For test/js/valkey/valkey.test.ts PUB/SUB tests, avoid arbitrary sleeps and async-forEach. Instead, resolve a Promise from the subscriber callback when the expected number of messages is observed and await it with a bounded timeout (e.g., withTimeout + Promise.withResolvers) to account for Redis server→subscriber propagation.
Applied to files:
test/js/web/websocket/websocket-proxy.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} : Prefer async/await over callbacks in tests
Applied to files:
test/js/bun/http/proxy.test.js
📚 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} : Do not set a timeout on tests. Bun already has timeouts
Applied to files:
test/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.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.ts?(x) : For multi-file tests, prefer `tempDir` and `Bun.spawn` over single-file tests
Applied to files:
test/js/bun/http/proxy.test.js
📚 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:
test/js/bun/http/proxy.test.jstest/js/bun/http/http-server-chunking.test.ts
📚 Learning: 2025-09-05T18:45:29.200Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey.zig:0-0
Timestamp: 2025-09-05T18:45:29.200Z
Learning: In JSValkeyClient.cloneWithoutConnecting() in src/valkey/js_valkey.zig, the address/username/password fields must be repointed to the duplicated connection_strings buffer to avoid use-after-free when the original client is destroyed. The original client properly frees connection_strings in ValkeyClient.deinit().
Applied to files:
src/http/websocket_client/CppWebSocket.zigsrc/bun.js/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 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/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 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/V8*.h : Add BUN_EXPORT visibility attribute to all public V8 API functions to ensure proper symbol export across platforms
Applied to files:
src/bun.js/bindings/headers.h
📚 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 : Add iso subspaces for C++ classes with fields in JavaScript class bindings
Applied to files:
src/bun.js/bindings/headers.h
📚 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/headers.h
📚 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/bun.js/bindings/headers.h
📚 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.dyn : Add symbol names with leading underscore and semicolons in braces to src/symbols.dyn for each new V8 API method
Applied to files:
src/bun.js/bindings/headers.h
📚 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/V8*.h : Create V8 class headers with .h extension following the pattern V8ClassName.h that include pragma once, v8.h, V8Local.h, V8Isolate.h, and declare classes extending from Data with BUN_EXPORT static methods
Applied to files:
src/bun.js/bindings/headers.h
📚 Learning: 2025-11-24T18:37:47.899Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/AGENTS.md:0-0
Timestamp: 2025-11-24T18:37:47.899Z
Learning: Applies to src/bun.js/bindings/v8/**/<UNKNOWN> : <UNKNOWN>
Applied to files:
src/bun.js/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 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 : Cache structures in ZigGlobalObject for JavaScript class bindings
Applied to files:
src/bun.js/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.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/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.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/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 Learning: 2025-09-16T19:58:14.006Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: src/valkey/js_valkey_functions.zig:671-682
Timestamp: 2025-09-16T19:58:14.006Z
Learning: In Bun's Redis client implementation (src/valkey/js_valkey_functions.zig), the psubscribe and punsubscribe functions are not implemented nor supported - they are intentionally left as basic wrappers without proper subscription handling.
Applied to files:
src/bun.js/bindings/headers.hsrc/http/websocket_client/WebSocketUpgradeClient.zig
📚 Learning: 2025-10-18T20:50:47.750Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: src/bun.js/telemetry.zig:366-373
Timestamp: 2025-10-18T20:50:47.750Z
Learning: In Bun's Zig codebase (src/bun.js/bindings/JSValue.zig), the JSValue enum uses `.null` (not `.js_null`) for JavaScript's null value. Only `js_undefined` has the `js_` prefix to avoid collision with Zig's built-in `undefined` keyword. The correct enum fields are: `js_undefined`, `null`, `true`, `false`, and `zero`.
Applied to files:
src/bun.js/bindings/headers.h
📚 Learning: 2025-10-11T15:19:30.301Z
Learnt from: mastermakrela
Repo: oven-sh/bun PR: 19167
File: src/bun.js/api.zig:31-31
Timestamp: 2025-10-11T15:19:30.301Z
Learning: The learnings about `pub const js = JSC.Codegen.JS<ClassName>` and re-exporting toJS/fromJS/fromJSDirect apply to class-based Zig bindings with constructors and prototype methods (e.g., those with `new` constructors). They do NOT apply to simple namespace-style API objects like TOMLObject, YAMLObject, and CSVObject which expose static functions via a `create()` method that returns a plain JS object.
Applied to files:
src/bun.js/bindings/headers.h
📚 Learning: 2025-09-05T19:49:26.188Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 21728
File: src/valkey/js_valkey_functions.zig:852-867
Timestamp: 2025-09-05T19:49:26.188Z
Learning: In Bun’s Zig code, `.js_undefined` is a valid and preferred JSValue literal for “undefined” (e.g., resolving JSPromise). Do not refactor usages to `jsc.JSValue.jsUndefined()`, especially in src/valkey/js_valkey_functions.zig unsubscribe().
Applied to files:
src/bun.js/bindings/headers.h
📚 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 : Create classes in three parts in C++ when there is a public constructor: Foo (JSDestructibleObject), FooPrototype (JSNonFinalObject), and FooConstructor (InternalFunction)
Applied to files:
src/bun.js/bindings/headers.h
📚 Learning: 2025-09-02T05:33:37.517Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol.test.ts:74-75
Timestamp: 2025-09-02T05:33:37.517Z
Learning: In Bun's runtime, `await using` with Node.js APIs like `net.createServer()` is properly supported and should not be replaced with explicit cleanup. Bun has extended Node.js APIs with proper async dispose support.
Applied to files:
src/http/websocket_client/WebSocketUpgradeClient.zig
📚 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:
src/http/websocket_client/WebSocketUpgradeClient.zig
📚 Learning: 2025-08-30T09:09:18.384Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22231
File: src/bundler/bundle_v2.zig:48-48
Timestamp: 2025-08-30T09:09:18.384Z
Learning: In Zig, when a module exports a top-level struct, import("./Module.zig") directly returns that struct type and can be used as a type alias without needing to access a field within the module. This is a common pattern in the Bun codebase.
Applied to files:
src/http/websocket_client/WebSocketUpgradeClient.zig
📚 Learning: 2025-09-17T23:42:05.812Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 22568
File: docs/api/redis.md:240-246
Timestamp: 2025-09-17T23:42:05.812Z
Learning: The RedisClient.duplicate() method in Bun's Redis implementation returns Promise<RedisClient>, not RedisClient synchronously, so await is required when calling it.
Applied to files:
src/http/websocket_client/WebSocketUpgradeClient.zig
📚 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) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/js/bun/http/http-server-chunking.test.ts
📚 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: Applies to src/js/{builtins,node,bun,thirdparty,internal}/**/*.{ts,js} : Use `process.platform` and `process.arch` for platform detection; these values are inlined and dead-code eliminated at build time
Applied to files:
test/js/bun/http/http-server-chunking.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/bun/http/http-server-chunking.test.ts
📚 Learning: 2025-09-20T03:39:41.770Z
Learnt from: pfgithub
Repo: oven-sh/bun PR: 22534
File: test/regression/issue/21830.fixture.ts:14-63
Timestamp: 2025-09-20T03:39:41.770Z
Learning: Bun's test runner supports async describe callbacks, unlike Jest/Vitest where describe callbacks must be synchronous. The syntax `describe("name", async () => { ... })` is valid in Bun.
Applied to files:
test/js/bun/http/http-server-chunking.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/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:
test/js/bun/http/http-server-chunking.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/bun/http/http-server-chunking.test.ts
📚 Learning: 2025-10-30T03:48:10.513Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 24063
File: packages/bun-otel/test/context-propagation.test.ts:1-7
Timestamp: 2025-10-30T03:48:10.513Z
Learning: In Bun test files, `using` declarations at the describe block level execute during module load/parsing, not during test execution. This means they acquire and dispose resources before any tests run. For test-scoped resource management, use beforeAll/afterAll hooks instead. The pattern `beforeAll(beforeUsingEchoServer); afterAll(afterUsingEchoServer);` is correct for managing ref-counted test resources like the EchoServer in packages/bun-otel/test/ - the using block pattern should not be used at describe-block level for test resources.
<!-- [/add_learning]
Applied to files:
test/js/bun/http/http-server-chunking.test.ts
📚 Learning: 2025-09-02T06:10:17.252Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol-strict.test.ts:80-91
Timestamp: 2025-09-02T06:10:17.252Z
Learning: Bun's WebSocket implementation includes a terminate() method, which is available even though it's not part of the standard WebSocket API. This method can be used in Bun test files and applications for immediate WebSocket closure.
Applied to files:
src/http/websocket_client.zig
🧬 Code graph analysis (3)
src/bun.js/bindings/webcore/WebSocket.h (1)
src/bun.js/bindings/webcore/WebSocket.cpp (2)
didConnectWithSSLTunnel(1420-1457)didConnectWithSSLTunnel(1420-1420)
test/js/web/websocket/websocket-proxy.test.ts (1)
test/js/bun/http/proxy.test.js (5)
Bun(181-190)Bun(211-218)data(82-84)data(100-100)test(79-79)
src/bun.js/bindings/webcore/WebSocket.cpp (1)
src/bun.js/bindings/helpers.h (6)
toZigString(295-429)toZigString(303-310)toZigString(312-318)toZigString(320-323)toZigString(325-328)toZigString(341-360)
🔇 Additional comments (11)
workspace.code-workspace (1)
93-94: LGTM!The settings update from
zig.buildOnSavetozig.buildOnSaveProvideraligns with the Zig VS Code extension's updated configuration API.test/js/bun/http/http-server-chunking.test.ts (1)
6-6: Clarify why this test is being unconditionally skipped.The change from
describe.if(isPosix)todescribe.skip().if(isPosix)causes the test to always be skipped regardless of platform. The.if(isPosix)condition has no effect on an already-skipped describe block.Is this an intentional temporary skip, or should this be reverted? If intentional, please add a comment explaining why.
src/http/proxy.zig (1)
46-88: LGTM - well-structured response parsing.The function correctly handles both success and error response paths, properly distinguishes between incomplete reads and malformed responses, and returns meaningful status information for error reporting.
src/bun.js/bindings/webcore/WebSocket.h (1)
139-139: LGTM - well-designed SSL tunnel callback signature.The new
didConnectWithSSLTunnelmethod appropriately extends the existingdidConnectpattern to handle TLS-over-proxy connections. The signature correctly passes SSL state pointers for userspace TLS management, consistent with the implementation in WebSocket.cpp.src/http/websocket_client/CppWebSocket.zig (1)
18-26: LGTM - consistent implementation of SSL tunnel callback.The new
didConnectWithSSLTunnelextern and wrapper follow the established patterns in this file:
- Event loop enter/exit is correctly applied
- Parameter types appropriately use
*anyopaquefor SSL pointer interop with C++- Implementation mirrors the existing
didConnectmethodAlso applies to: 68-81
src/http.zig (1)
307-343: Proxy CONNECT/request refactor looks correct and keeps semanticsThe unified port resolution (
getPort() ?? 443/80) and delegation toproxy_helpers.writeConnectRequestStart/writeConnectRequestEnd/writeHeaderpreserve existing behavior while centralizing header formatting. The same logic is used consistently in bothwriteProxyConnectandwriteProxyRequest, and the newproxy_helpersimport is wired correctly.Also applies to: 345-392, 2604-2607
src/bun.js/bindings/headers.h (1)
576-588: WebSocket upgrade/connect FFI extensions match Zig exportsThe added
proxyHost/proxyPort/targetIsTlsparameters onBun__WebSocketHTTP(S)Client__connectand the newBun__WebSocketClient__setSSLTunnelprototype are consistent with the Zig signatures (?*const ZigString, u16, booland(WebSocketClient*, *SSL, *SSL_CTX)respectively). This should integrate cleanly with the new proxy/TLS-over-proxy paths, assuming all C++ call sites are updated to pass the extra arguments in the same order and only the non‑TLS WebSocket client usessetSSLTunnel.Please double‑check that:
- All callers of
Bun__WebSocketHTTPClient__connect/Bun__WebSocketHTTPSClient__connectnow passproxyHost,proxyPort, andtargetIsTlscorrectly, and- Only the plain
WebSocketClient(notWebSocketClientTLS) is wired toBun__WebSocketClient__setSSLTunnel, as intended by the TLS‑over‑proxy design.Also applies to: 592-601
src/http/websocket_client.zig (1)
57-59: TLS‑over‑proxy field wiring, lifecycle, and initial data handling look sound
ssl_tunnelis initialized to null, cleaned up inclearData(deinit+bun.destroy), and also safely replaced insetSSLTunnelwithout leaking or double‑freeing.InitialDataHandler.handleWithoutDeinitnow routes the bufferedoverflowbytes from the upgrade as already‑decrypted viahandleDecryptedData, which correctly skips the TLS layer (these bytes were produced by the HTTP/TLS stage, not by the proxy tunnel).handleDataImpl’s early decrypt branch is properly gated onssl_tunnel != nullandalready_decrypted == false, so normal non‑proxy and direct TLS flows are unaffected.- ErrorCode
proxy_connect_failedis plumbed into the enum without breaking existing discriminants and is only used by the upgrade client in proxy error paths.Once the send/flush concerns called out above are addressed, the ownership model for SSL/CTX and tunnel cleanup is in good shape.
Please confirm
SSLTunnel.deinitis only ever called after ownership of theSSL*andSSL_CTX*has been fully transferred from the HTTP upgrade client (which already nulls them out on its side) to avoid any accidental double‑free across the two layers.Also applies to: 110-129, 361-393, 1174-1205, 1313-1322, 1375-1376, 1653-1736
src/http/websocket_client/WebSocketUpgradeClient.zig (1)
33-55: Proxy + TLS‑over‑proxy upgrade state machine and cleanup are well‑structuredThe additions here hang together nicely:
- New fields (
proxy_connect_buf,using_proxy,target_host/target_port,ssl_wrapper,ssl_write_buffer,target_is_tls) are initialized, used, and freed consistently inconnect,handleOpen,handleData,handleProxyConnectResponse, andclearData.connectcorrectly choosesconnect_host/connect_portbased onusing_proxy, builds a CONNECT request when needed, and seeds the initialstateto.proxy_connectvs.reading.handleOpenfirst sends the CONNECT payload whenusing_proxy, then falls back to the normal upgrade write once the proxy tunnel is established.handleProxyConnectResponse’s use ofproxy.parseConnectResponsewith explicit handling for malformed/incomplete responses and a dedicatedproxy_connect_failederror wires cleanly into the newErrorCode.- The TLS‑over‑proxy path via
SSLWrapper(sslOnOpen/Data/Handshake/Close,sslWrite,ssl_write_buffer, and state.proxy_tls_handshake) cleanly encapsulates the handshake and then feeds the decrypted HTTP 101 response intoprocessResponse.- In
processResponse, you correctly “steal”ssl_ptr/ssl_ctx_ptrfromssl_wrapper, null them out to avoid double‑free, callclearData(which deinit’s the wrapper), and choose betweendidConnectWithSSLTunnelvsdidConnectbased on the presence of those pointers.From a lifetime and control‑flow perspective, this is a solid integration of proxy and TLS‑over‑proxy into the existing upgrade client.
Please verify that
SSLWrapper(*HTTPClient).deinit()is a no‑op forssl/ctxonce you’ve nulled them out inprocessResponse, and that the C++ side really does call the HTTP (non‑TLS) upgrade client withtargetIsTls=truefor wss:// through an HTTP proxy, so that uWS TLS and the userland TLS wrapper don’t both try to own the same connection.Also applies to: 83-145, 150-201, 236-258, 353-392, 398-462, 464-585, 589-715, 896-935, 949-1011, 1187-1207, 1193-1197
src/bun.js/bindings/webcore/WebSocket.cpp (2)
71-72: LGTM!The includes are appropriate for the new proxy environment variable handling using
getenv(),strlen(), andstd::string.
1635-1638: LGTM!The extern "C" wrapper follows the established pattern and correctly bridges to the C++ method.
| if (!entry.empty() && hostStr.length() >= entry.length()) { | ||
| if (hostStr.compare(hostStr.length() - entry.length(), entry.length(), entry) == 0) { | ||
| shouldBypassProxy = true; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Insecure NO_PROXY hostname suffix matching.
The current logic bypasses the proxy if the hostname simply ends with a NO_PROXY entry. This is a security issue: evil.example.com would incorrectly match a NO_PROXY entry of example.com.
The correct behavior should verify that the match occurs at a subdomain boundary (i.e., the character before the match is a .) or that the hostname equals the entry exactly.
🔎 Proposed fix
// Check if hostname ends with this entry
if (!entry.empty() && hostStr.length() >= entry.length()) {
- if (hostStr.compare(hostStr.length() - entry.length(), entry.length(), entry) == 0) {
+ size_t matchPos = hostStr.length() - entry.length();
+ if (hostStr.compare(matchPos, entry.length(), entry) == 0) {
+ // Exact match or subdomain boundary (preceded by '.')
+ if (matchPos == 0 || hostStr[matchPos - 1] == '.') {
shouldBypassProxy = true;
break;
+ }
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/WebSocket.cpp around lines 506 to 510, the
NO_PROXY check currently treats any hostname that ends with a NO_PROXY entry as
a match (e.g., evil.example.com matches example.com). Change the logic so that
after confirming the hostname ends with the entry you also require either the
hostname length equals the entry length (exact match) or the character
immediately before the matched suffix is a '.' (subdomain boundary). Keep the
existing empty-entry guard and only set shouldBypassProxy to true and break when
one of these stricter conditions is satisfied.
|
|
||
| bool usingProxy = proxyHost.len > 0; | ||
|
|
||
| // add a log saying hello world here |
There was a problem hiding this comment.
Remove debug comment.
This appears to be a leftover debug comment that should be removed before merging.
🔎 Proposed fix
- // add a log saying hello world here
- if (is_secure && !usingProxy) {
+ if (is_secure && !usingProxy) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // add a log saying hello world here | |
| if (is_secure && !usingProxy) { |
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/WebSocket.cpp around line 533, remove the
leftover debug comment "// add a log saying hello world here" — delete that line
(or convert it to a meaningful comment if needed) so no stray debug comments
remain in production code and then run format/lint to ensure style consistency.
| } else if (is_secure && usingProxy) { | ||
| // wss:// through HTTP proxy - use non-SSL client with TLS-over-proxy | ||
| // The Zig side will handle TLS via SSLWrapper after CONNECT succeeds | ||
| us_socket_context_t* ctx = scriptExecutionContext()->webSocketContext<false>(); | ||
| RELEASE_ASSERT(ctx); | ||
| this->m_upgradeClient = Bun__WebSocketHTTPClient__connect(scriptExecutionContext()->jsGlobalObject(), ctx, reinterpret_cast<CppWebSocket*>(this), &host, port, &path, &clientProtocolString, headerNames.begin(), headerValues.begin(), headerNames.size(), &proxyHost, proxyPort, true); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the file size to understand what we're dealing with
wc -l src/bun.js/bindings/webcore/WebSocket.cpp
# Then examine the destructor (lines 193-200)
echo "=== Destructor area (lines 193-210) ==="
sed -n '193,210p' src/bun.js/bindings/webcore/WebSocket.cpp
# Examine m_isSecure assignment (around line 459)
echo -e "\n=== m_isSecure assignment area (lines 450-470) ==="
sed -n '450,470p' src/bun.js/bindings/webcore/WebSocket.cpp
# Examine the TLS-over-proxy connection code (lines 534-544)
echo -e "\n=== TLS-over-proxy connection (lines 534-550) ==="
sed -n '534,550p' src/bun.js/bindings/webcore/WebSocket.cpp
# Search for all m_isSecure usages
echo -e "\n=== All m_isSecure usages ==="
rg -n 'm_isSecure' src/bun.js/bindings/webcore/WebSocket.cppRepository: oven-sh/bun
Length of output: 3731
🏁 Script executed:
#!/bin/bash
# Check line 324
echo "=== Line 324 context ==="
sed -n '320,330p' src/bun.js/bindings/webcore/WebSocket.cpp
# Check line 758
echo -e "\n=== Line 758 context ==="
sed -n '754,764p' src/bun.js/bindings/webcore/WebSocket.cpp
# Check line 813
echo -e "\n=== Line 813 context ==="
sed -n '809,819p' src/bun.js/bindings/webcore/WebSocket.cpp
# Check line 1407
echo -e "\n=== Line 1407 context ==="
sed -n '1403,1413p' src/bun.js/bindings/webcore/WebSocket.cppRepository: oven-sh/bun
Length of output: 2251
Fix incorrect upgrade client cancellation logic for TLS-over-proxy.
In the TLS-over-proxy scenario (wss:// through HTTP proxy at lines 539-544), m_isSecure is set to true while m_upgradeClient is created via Bun__WebSocketHTTPClient__connect (an HTTP client, not HTTPS). This causes three bugs:
- Destructor (line 195): Calls
Bun__WebSocketHTTPSClient__cancel()on an HTTP client. - Close callback (line 758): Calls
Bun__WebSocketHTTPSClient__cancel()on an HTTP client. - Abort callback (line 813): Calls
Bun__WebSocketHTTPSClient__cancel()on an HTTP client. - Memory cost calculation (line 324): Uses HTTPS cost calculation on an HTTP client.
The upgrade client type must be tracked independently of m_isSecure, or m_isSecure should reflect the actual upgrade client type rather than the original URL scheme.
🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/WebSocket.cpp around lines 539-544, the
TLS-over-proxy branch sets m_isSecure=true but constructs an HTTP upgrade
client, causing HTTPS-specific cancel calls and memory-cost math to be used
incorrectly; introduce tracking for the actual upgrade client type (e.g., a new
bool m_upgradeClientIsHTTPS) or set m_isSecure to reflect the created client,
then: 1) set m_upgradeClientIsHTTPS = false when creating the HTTP client for
TLS-over-proxy (or set m_isSecure appropriately); 2) change destructor (line
~195), close callback (line ~758), and abort callback (line ~813) to call
Bun__WebSocketHTTPSClient__cancel() only when m_upgradeClientIsHTTPS is true
(otherwise call the HTTP cancel); 3) update the memory cost calculation (line
~324) to use the HTTP-client cost path when m_upgradeClientIsHTTPS is false;
ensure all branches that create upgrade clients correctly set the new flag so
cancel/memory logic matches the actual client type.
| void WebSocket::didConnectWithSSLTunnel(us_socket_t* socket, char* bufferedData, size_t bufferedDataSize, const PerMessageDeflateParams* deflate_params, void* ssl_ptr, void* ssl_ctx_ptr) | ||
| { | ||
| // For TLS-over-proxy: the underlying socket is plain TCP, but we have SSL state for userspace TLS | ||
| this->m_upgradeClient = nullptr; | ||
|
|
||
| // Set extensions if permessage-deflate was negotiated | ||
| if (deflate_params != nullptr) { | ||
| StringBuilder extensions; | ||
| extensions.append("permessage-deflate"_s); | ||
| if (deflate_params->server_no_context_takeover) { | ||
| extensions.append("; server_no_context_takeover"_s); | ||
| } | ||
| if (deflate_params->client_no_context_takeover) { | ||
| extensions.append("; client_no_context_takeover"_s); | ||
| } | ||
| if (deflate_params->server_max_window_bits != 15) { | ||
| extensions.append("; server_max_window_bits="_s); | ||
| extensions.append(String::number(deflate_params->server_max_window_bits)); | ||
| } | ||
| if (deflate_params->client_max_window_bits != 15) { | ||
| extensions.append("; client_max_window_bits="_s); | ||
| extensions.append(String::number(deflate_params->client_max_window_bits)); | ||
| } | ||
| this->m_extensions = extensions.toString(); | ||
| } | ||
|
|
||
| // Use non-SSL WebSocket client (the TLS is handled by SSLTunnel in userspace) | ||
| us_socket_context_t* ctx = (us_socket_context_t*)this->scriptExecutionContext()->connectedWebSocketContext<false, false>(); | ||
| this->m_connectedWebSocket.client = Bun__WebSocketClient__init(reinterpret_cast<CppWebSocket*>(this), socket, ctx, this->scriptExecutionContext()->jsGlobalObject(), reinterpret_cast<unsigned char*>(bufferedData), bufferedDataSize, deflate_params); | ||
| this->m_connectedWebSocketKind = ConnectedWebSocketKind::Client; | ||
|
|
||
| // Set SSL tunnel on the WebSocket client | ||
| if (this->m_connectedWebSocket.client && ssl_ptr && ssl_ctx_ptr) { | ||
| Bun__WebSocketClient__setSSLTunnel(this->m_connectedWebSocket.client, ssl_ptr, ssl_ctx_ptr); | ||
| } | ||
|
|
||
| this->didConnect(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicate extension-building logic into a helper.
The extension-building logic (lines 1425-1444) is duplicated from didConnect (lines 1386-1405). Consider extracting this into a private helper method to reduce duplication and improve maintainability.
🔎 Proposed refactor
Add a private helper method:
void WebSocket::setExtensionsFromDeflateParams(const PerMessageDeflateParams* deflate_params)
{
if (deflate_params == nullptr)
return;
StringBuilder extensions;
extensions.append("permessage-deflate"_s);
if (deflate_params->server_no_context_takeover) {
extensions.append("; server_no_context_takeover"_s);
}
if (deflate_params->client_no_context_takeover) {
extensions.append("; client_no_context_takeover"_s);
}
if (deflate_params->server_max_window_bits != 15) {
extensions.append("; server_max_window_bits="_s);
extensions.append(String::number(deflate_params->server_max_window_bits));
}
if (deflate_params->client_max_window_bits != 15) {
extensions.append("; client_max_window_bits="_s);
extensions.append(String::number(deflate_params->client_max_window_bits));
}
this->m_extensions = extensions.toString();
}Then replace both occurrences with:
setExtensionsFromDeflateParams(deflate_params);🤖 Prompt for AI Agents
In src/bun.js/bindings/webcore/WebSocket.cpp around lines 1425-1444 (and the
matching block in didConnect at 1386-1405), the permessage-deflate
extension-building logic is duplicated; extract that logic into a private helper
method (e.g., void WebSocket::setExtensionsFromDeflateParams(const
PerMessageDeflateParams* deflate_params)) that returns/sets this->m_extensions,
early-returns on nullptr, builds the StringBuilder exactly as in the duplicated
blocks (including server/client no_context_takeover and max_window_bits checks),
and then replace both original blocks with a single call to
setExtensionsFromDeflateParams(deflate_params).
| log("proxy_host_slice: {s}", .{proxy_host_slice.slice()}); | ||
| log("proxy_host_slice: {s}", .{proxy_host_slice.slice()}); | ||
| client.poll_ref.ref(vm); | ||
| const display_host_ = host_.slice(); | ||
| const display_host = if (bun.FeatureFlags.hardcode_localhost_to_127_0_0_1 and strings.eqlComptime(display_host_, "localhost")) | ||
| "127.0.0.1" | ||
| else | ||
| display_host_; | ||
|
|
||
| log("proxy_host_slice: {s}", .{proxy_host_slice.slice()}); | ||
| // Determine connection target: proxy or direct |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove duplicated debug logging of proxy_host_slice
proxy_host_slice is logged three times in a row, which is noisy and doesn’t add extra signal:
log("proxy_host_slice: {s}", .{proxy_host_slice.slice()});
log("proxy_host_slice: {s}", .{proxy_host_slice.slice()});
...
log("proxy_host_slice: {s}", .{proxy_host_slice.slice()});Consider keeping just one of these (or gating it behind a more verbose log level) to avoid spamming logs in proxy scenarios.
🤖 Prompt for AI Agents
In src/http/websocket_client/WebSocketUpgradeClient.zig around lines 176 to 181,
the proxy_host_slice debug message is logged three times in a row; remove the
duplicate calls and keep a single log statement (or replace the repeated logs
with a single log guarded by a verbose/trace log level) so only one
proxy_host_slice message is emitted at this point.
|
|
||
| // Create a proper TCP CONNECT proxy that does bidirectional forwarding | ||
| proxyServer = net.createServer(clientSocket => { | ||
| console.log("hellp bruh"); |
There was a problem hiding this comment.
Remove debug log.
This appears to be a leftover debug statement with a typo ("hellp bruh").
🔎 Proposed fix
// Create a proper TCP CONNECT proxy that does bidirectional forwarding
proxyServer = net.createServer(clientSocket => {
- console.log("hellp bruh");
let buffer: Buffer = Buffer.alloc(0);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("hellp bruh"); | |
| // Create a proper TCP CONNECT proxy that does bidirectional forwarding | |
| proxyServer = net.createServer(clientSocket => { | |
| let buffer: Buffer = Buffer.alloc(0); |
🤖 Prompt for AI Agents
In test/js/web/websocket/websocket-proxy.test.ts around line 66 there is a
leftover debug console.log("hellp bruh"); — remove this debug statement (or
replace with an appropriate test assertion if it was intended) so the test file
contains no stray logging or typos; run tests to confirm nothing else depended
on that output.
| await new Promise<void>(resolve => { | ||
| proxyServer.listen(9091, () => { | ||
| const addr = proxyServer.address(); | ||
| proxyPort = typeof addr === "object" && addr ? addr.port : 0; | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Use port: 0 instead of hardcoded port 9091.
Per coding guidelines, always use port: 0 to get a random available port in tests. Hardcoded ports can cause test failures when the port is already in use.
🔎 Proposed fix
await new Promise<void>(resolve => {
- proxyServer.listen(9091, () => {
+ proxyServer.listen(0, () => {
const addr = proxyServer.address();
proxyPort = typeof addr === "object" && addr ? addr.port : 0;
resolve();
});
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await new Promise<void>(resolve => { | |
| proxyServer.listen(9091, () => { | |
| const addr = proxyServer.address(); | |
| proxyPort = typeof addr === "object" && addr ? addr.port : 0; | |
| resolve(); | |
| }); | |
| }); | |
| await new Promise<void>(resolve => { | |
| proxyServer.listen(0, () => { | |
| const addr = proxyServer.address(); | |
| proxyPort = typeof addr === "object" && addr ? addr.port : 0; | |
| resolve(); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In test/js/web/websocket/websocket-proxy.test.ts around lines 121 to 127, the
test binds proxyServer to a hardcoded port 9091 which can collide; change the
server listen call to request an ephemeral port (use port: 0 or pass 0 as the
first arg) and then read proxyServer.address().port to set proxyPort so the test
uses a random available port.
| test("should read HTTP_PROXY environment variable for ws:// connections", async () => { | ||
| connectRequests = []; | ||
|
|
||
| using dir = tempDir("ws-proxy-test", { | ||
| "test.ts": ` | ||
| // This test verifies that the HTTP_PROXY env var is read | ||
| // Since we don't have a full proxy implementation in the test, | ||
| // we just verify the code path is exercised | ||
| console.log("HTTP_PROXY is:", process.env.HTTP_PROXY); | ||
| console.log("test complete"); | ||
| `, | ||
| }); | ||
|
|
||
| await using proc = Bun.spawn({ | ||
| cmd: [bunExe(), "test.ts"], | ||
| cwd: String(dir), | ||
| env: { | ||
| ...bunEnv, | ||
| HTTP_PROXY: `http://localhost:${proxyPort}`, | ||
| }, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
|
|
||
| expect(stdout).toContain(`HTTP_PROXY is: http://localhost:${proxyPort}`); | ||
| expect(exitCode).toBe(0); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Test doesn't actually verify proxy usage for ws:// connections.
This test only verifies that the HTTP_PROXY environment variable is readable but doesn't actually test that WebSocket connections use the proxy. The test script just logs the env var and exits without creating a WebSocket connection through the proxy.
Consider actually establishing a WebSocket connection through the proxy and verifying connectRequests contains the expected entry, similar to the wss:// tests.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (1)
workspace.code-workspace
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 22323
File: test/js/web/websocket/websocket-subprotocol-strict.test.ts:80-91
Timestamp: 2025-09-02T06:10:17.252Z
Learning: Bun's WebSocket implementation includes a terminate() method, which is available even though it's not part of the standard WebSocket API. This method can be used in Bun test files and applications for immediate WebSocket closure.
🔇 Additional comments (2)
workspace.code-workspace (2)
92-92: Workspace configuration additions look reasonable.The new
memory_resourcefile association (line 92) andgit.ignoreLimitWarningsetting (line 94) are sensible additions for C++ development and Git workflow in VS Code.Also applies to: 94-94
1-96: Clarify scope: only workspace config provided, but PR describes proxy/TLS WebSocket changes.The AI summary indicates this PR implements HTTP proxy and TLS-over-proxy support for WebSocket connections, including new bindings, utilities, and tests. However, only the workspace configuration file is provided for review.
Please confirm whether:
- This is the only file changed in the PR, or
- Additional proxy/TLS implementation files should also be reviewed.
| { | ||
| "path": ".", | ||
| }, | ||
| ], |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the file exists and view its contents
if [ -f "workspace.code-workspace" ]; then
echo "=== File contents (all lines) ==="
cat -n workspace.code-workspace
echo ""
echo "=== JSON validation with jq ==="
jq empty workspace.code-workspace 2>&1 && echo "✓ Valid JSON" || echo "✗ Invalid JSON"
else
echo "File not found: workspace.code-workspace"
fiRepository: oven-sh/bun
Length of output: 3200
🌐 Web query:
VS Code workspace file format trailing comma support JSON specification
💡 Result:
Short answer: VS Code workspace files (.code-workspace) are JSON (or treated as JSONC). Trailing commas are not part of the JSON specification and are not generally supported; VS Code’s JSON-with-comments mode can accept trailing commas in some contexts but will warn (and support depends on the schema/parser). The underlying jsonc-parser defaults to disallowing trailing commas (so support is an extension/exception), and VS Code discussions/issues confirm trailing-comma handling is intentionally discouraged and inconsistent. [1][2][3]
Sources
- VS Code JSON docs (json vs jsonc; jsonc accepts comments and may warn on trailing commas). [1]
- JSONC specification / jsonc-parser: trailing commas are not part of the spec; parser option allowTrailingComma is false by default. [2]
- Microsoft VS Code issue discussion about tolerating trailing commas in jsonc. [3]
Remove trailing commas from JSON object properties.
The file contains invalid JSON with trailing commas on lines 5, 93, and 95. VS Code workspace files require strict JSON format, and these trailing commas will prevent the file from parsing correctly. Remove the trailing comma after each property value.
Affected lines:
- Line 5: after
"path": "." - Line 93: after
"memory_resource": "cpp" - Line 95: after
"git.ignoreLimitWarning": true
🤖 Prompt for AI Agents
In workspace.code-workspace around lines 3 to 6 (and also lines ~93 and ~95),
remove the invalid trailing commas from the JSON object properties: delete the
comma after "path": "." on line 5, delete the comma after "memory_resource":
"cpp" on line 93, and delete the comma after "git.ignoreLimitWarning": true on
line 95 so the file becomes valid strict JSON with no trailing commas.
1 participant
What does this PR do?
How did you verify your code works?