Conversation
added 8 commits
May 9, 2026 01:22
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Contributor
|
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:
WalkthroughAdds SOCKS5/SOCKS5h proxy support: protocol handler, DNS‑pending resolution, HTTP and WebSocket client wiring and bindings, docs/type updates, error mappings, and tests. ChangesSOCKS5 Proxy Implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/http.zig (1)
1193-1201:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSplit origin HTTPS from proxy-transport TLS.
isHTTPS()is still used for origin-scheme decisions, but this change makes it answer the outer proxy transport instead. That regresses proxiedhttps://requests: for example, Alt-Svc recording at Line 3105 now stops running for HTTPS origins behindsocks5://or a plainhttp://proxy. KeepisHTTPS()tied tothis.url.isHTTPS()and add a separate helper for transport-TLS call sites likestart().Suggested direction
+pub fn usesTransportTLS(this: *HTTPClient) bool { + if (this.http_proxy) |proxy| { + return SocksProxy.Kind.fromURL(proxy) == .https; + } + return this.url.isHTTPS(); +} + /// **Not thread safe while request is in-flight** pub fn isHTTPS(this: *HTTPClient) bool { - if (this.http_proxy) |proxy| { - return SocksProxy.Kind.fromURL(proxy) == .https; - } - if (this.url.isHTTPS()) { - return true; - } - return false; + return this.url.isHTTPS(); }Then switch the transport-facing callers to
usesTransportTLS().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/http.zig` around lines 1193 - 1201, The current isHTTPS() was changed to reflect the proxy transport which breaks origin-scheme logic; restore isHTTPS(this: *HTTPClient) to return this.url.isHTTPS() only, add a new helper usesTransportTLS(this: *HTTPClient) bool that implements the proxy-aware logic (checking this.http_proxy and using SocksProxy.Kind.fromURL(proxy) == .https or falling back to this.url.isHTTPS() as appropriate), and update transport-facing callers (e.g., start(), any place previously relying on proxy-transport behavior) to call usesTransportTLS() instead of isHTTPS().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 790-807: The plain ws:// branch in WebSocketUpgradeClient leaves
socks.read_buffer.slice() unconsumed so any upstream bytes that arrived with the
SOCKS CONNECT reply are dropped; update the .connected branch to forward that
overflow just like the TLS path does: after assigning this.input_body_buf =
p.takeWebsocketRequestBuf() and computing wrote, ensure you incorporate
socks.read_buffer.slice() into the outgoing buffer (either by
prepending/appending those bytes to this.to_send or by invoking the same
forwarding helper used by startProxyTLSHandshake) so the extra bytes are
delivered to the upstream handshake processing; update references to
this.input_body_buf, this.to_send, and socket.write handling in the .connected
branch accordingly and preserve error handling via
terminate(ErrorCode.failed_to_write).
In `@src/http/AsyncHTTP.zig`:
- Around line 215-217: The init() path computes is_socks but never sets the
connection's disable_keepalive, so the very first SOCKS tunnel remains eligible
for pooling; update the initialization logic (the same place that checks
proxy.protocol and computes is_socks) to set the connection flag
disable_keepalive = true when is_socks is true (matching the behavior in
reset()), and make the same change in the other occurrence of the same
proxy/protocol check later in the file (the second block around the other proxy
handling) so initial SOCKS connections are also prevented from being pooled.
In `@src/http/HTTPThread.zig`:
- Around line 263-265: The SSL-context cache-hit branch is calling
entry.ctx.connect(...) when client.http_proxy is set but does not validate
url.scheme, causing unsupported proxy protocols to be attempted; add the same
scheme guard used elsewhere: check url.scheme for "http", "https", "socks5", or
"socks5h" and return error.UnsupportedProxyProtocol for anything else before
calling entry.ctx.connect(client, url.hostname, proxyPort(url)). Ensure you use
the same symbols (client.http_proxy, url.scheme, proxyPort(url),
entry.ctx.connect, error.UnsupportedProxyProtocol) so behavior matches
non-cached paths.
In `@src/http/SocksProxy.zig`:
- Around line 57-76: The init function (and likewise initWithCredentials) must
reject partially-specified SOCKS credentials up front: if a username is provided
but password is empty, or a password is provided but username is empty, return a
clear error (e.g., error.SocksCredentialsIncomplete) instead of allowing a
zero-length counterpart; modify SocksProxy.init to validate presence of both
proxy.username and proxy.password before decoding or accepting either, keeping
the existing PercentEncoding.decodeAlloc and length checks (and
error.SocksCredentialsTooLong) but adding the incompleteness check early, and
apply the same logic to initWithCredentials to ensure symmetric behavior during
handshake.
In `@src/runtime/webcore/fetch/FetchTasklet.zig`:
- Around line 780-786: The switch in FetchTasklet.zig that maps SOCKS proxy
errors (inside the error formatting logic) misses cases for SocksGeneralFailure,
SocksConnectionNotAllowed, SocksTTLExpired, SocksCommandNotSupported, and
SocksAddressTypeNotSupported, so those fall back to the generic formatter; add
explicit branches for those error variants (matching the symbols
error.SocksGeneralFailure, error.SocksConnectionNotAllowed,
error.SocksTTLExpired, error.SocksCommandNotSupported,
error.SocksAddressTypeNotSupported) and return appropriate bun.String.static
messages (e.g., "SOCKS proxy general failure.", "SOCKS proxy connection not
allowed.", "SOCKS proxy TTL expired.", "SOCKS proxy command not supported.",
"SOCKS proxy address type not supported.") to align with
src/http/SocksProxy.zig.replyError().
In `@test/js/bun/http/proxy.test.ts`:
- Around line 1064-1070: The test "socks5:// with unresolvable hostname rejects
request" is missing an await/return for the rejection matcher: the expectation
expect(fetch(...)).rejects.toThrow() returns a Promise and must be awaited (or
returned) so the test waits for the assertion; update the test around the fetch
call in this case to either prefix the expect(...) with await or return the
expect(...) promise to ensure the test fails properly when fetch unexpectedly
succeeds.
---
Outside diff comments:
In `@src/http/http.zig`:
- Around line 1193-1201: The current isHTTPS() was changed to reflect the proxy
transport which breaks origin-scheme logic; restore isHTTPS(this: *HTTPClient)
to return this.url.isHTTPS() only, add a new helper usesTransportTLS(this:
*HTTPClient) bool that implements the proxy-aware logic (checking
this.http_proxy and using SocksProxy.Kind.fromURL(proxy) == .https or falling
back to this.url.isHTTPS() as appropriate), and update transport-facing callers
(e.g., start(), any place previously relying on proxy-transport behavior) to
call usesTransportTLS() instead of isHTTPS().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70e08c73-d898-41f6-8481-59d22e3eb69d
📒 Files selected for processing (19)
docs/guides/http/proxy.mdxdocs/runtime/networking/fetch.mdxpackages/bun-types/bun.d.tspackages/bun-types/globals.d.tssrc/http/AsyncHTTP.zigsrc/http/HTTPThread.zigsrc/http/SocksDNSPending.zigsrc/http/SocksProxy.zigsrc/http/http.zigsrc/http_jsc/websocket_client/WebSocketProxy.zigsrc/http_jsc/websocket_client/WebSocketUpgradeClient.zigsrc/jsc/bindings/headers.hsrc/jsc/bindings/webcore/WebSocket.cppsrc/runtime/dns_jsc/dns.zigsrc/runtime/webcore/fetch/FetchTasklet.zigtest/js/bun/http/proxy.test.tstest/js/first_party/ws/ws-proxy.test.tstest/js/web/websocket/proxy-test-utils.tstest/js/web/websocket/websocket-proxy.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/AsyncHTTP.zig (1)
205-218:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve explicit
disable_keepalivewhen applying proxy defaults.Line 217 and Line 295 replace the flag instead of OR-ing into it. That means
disable_keepalive: trueis lost for plainhttp://requests going through a non-SOCKS proxy, and the reset path can re-enable pooling again after redirects.Proposed fix
if (options.disable_keepalive) |val| { this.client.flags.disable_keepalive = val; } @@ if (options.http_proxy) |proxy| { const is_socks = strings.eqlComptime(proxy.protocol, "socks5") or strings.eqlComptime(proxy.protocol, "socks5h"); - this.client.flags.disable_keepalive = this.url.isHTTPS() or is_socks; + this.client.flags.disable_keepalive = this.client.flags.disable_keepalive or this.url.isHTTPS() or is_socks; if (!is_socks and proxy.username.len > 0) {fn reset(this: *AsyncHTTP) !void { + const disable_keepalive = this.client.flags.disable_keepalive; const aborted = this.client.aborted; this.client = try HTTPClient.init(this.allocator, this.method, this.client.url, this.client.header_entries, this.client.header_buf, aborted); this.client.http_proxy = this.http_proxy; @@ if (this.http_proxy) |proxy| { const is_socks = strings.eqlComptime(proxy.protocol, "socks5") or strings.eqlComptime(proxy.protocol, "socks5h"); - this.client.flags.disable_keepalive = this.url.isHTTPS() or is_socks; + this.client.flags.disable_keepalive = disable_keepalive or this.url.isHTTPS() or is_socks; if (!is_socks and proxy.username.len > 0) {Also applies to: 287-296
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/AsyncHTTP.zig` around lines 205 - 218, The proxy-handling block in AsyncHTTP.zig currently overwrites this.client.flags.disable_keepalive with "this.url.isHTTPS() or is_socks", losing any previously-set true value; change the assignment to OR into the existing flag instead (e.g., set this.client.flags.disable_keepalive = this.client.flags.disable_keepalive or this.url.isHTTPS() or is_socks) so explicit disable_keepalive set earlier is preserved; apply the same OR-fix to the other occurrence around lines 287-296 where the flag is being replaced.
♻️ Duplicate comments (3)
src/http/SocksProxy.zig (1)
63-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject username-only SOCKS credentials too.
These constructors still allow
socks5://user@.../initWithCredentials(..., "user", ""). That later sends RFC 1929 auth with a zero-length password, so callers get an opaque proxy auth failure instead of a clear input error.Suggested fix
pub fn init(allocator: std.mem.Allocator, proxy: URL) !SocksProxy { + if ((proxy.username.len == 0) != (proxy.password.len == 0)) { + return error.SocksCredentialsIncomplete; + } + var this = SocksProxy{ .allocator = allocator, .kind = Kind.fromURL(proxy), }; - - if (proxy.password.len > 0 and proxy.username.len == 0) { - return error.SocksCredentialsIncomplete; - } @@ pub fn initWithCredentials(allocator: std.mem.Allocator, kind: Kind, username: []const u8, password: []const u8) !SocksProxy { + if ((username.len == 0) != (password.len == 0)) { + return error.SocksCredentialsIncomplete; + } + var this = SocksProxy{ .allocator = allocator, .kind = kind, }; - if (password.len > 0 and username.len == 0) { - return error.SocksCredentialsIncomplete; - }Also applies to: 87-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/SocksProxy.zig` around lines 63 - 76, The constructor currently allows a non-empty username with an empty password which results in sending a zero-length RFC1929 password; modify the validation in SocksProxy (where proxy.username and proxy.password are handled and PercentEncoding.decodeAlloc is used) to explicitly reject username-only credentials by returning error.SocksCredentialsIncomplete when proxy.username.len > 0 and proxy.password.len == 0 (in both places the username/password are processed, e.g., the blocks around this.username/this.password decoding and the alternate block at lines ~87-99), then proceed with decoding only when both are present and still enforce the existing max-length checks that return error.SocksCredentialsTooLong.src/runtime/webcore/fetch/FetchTasklet.zig (1)
780-788:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMap the remaining SOCKS reply errors here too.
replyError()can still surfaceSocksConnectionNotAllowed,SocksTTLExpired,SocksCommandNotSupported, andSocksAddressTypeNotSupported, but those still fall through to the generic formatter.Suggested fix
error.SocksGeneralFailure => bun.String.static("SOCKS proxy reported a general failure."), + error.SocksConnectionNotAllowed => bun.String.static("SOCKS proxy reported that the connection is not allowed."), error.SocksNetworkUnreachable => bun.String.static("SOCKS proxy reported the network is unreachable."), error.SocksHostUnreachable => bun.String.static("SOCKS proxy reported the host is unreachable."), + error.SocksTTLExpired => bun.String.static("SOCKS proxy reported that the TTL expired."), + error.SocksCommandNotSupported => bun.String.static("SOCKS proxy does not support the requested command."), + error.SocksAddressTypeNotSupported => bun.String.static("SOCKS proxy does not support the requested address type."), error.SocksCredentialsTooLong => bun.String.static("SOCKS proxy credentials are too long."),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/webcore/fetch/FetchTasklet.zig` around lines 780 - 788, The match in replyError() inside FetchTasklet.zig currently maps many SOCKS errors but omits SocksConnectionNotAllowed, SocksTTLExpired, SocksCommandNotSupported, and SocksAddressTypeNotSupported; update the same match arm (the error.Socks* cases) to include these four variants with clear static messages (e.g., "SOCKS proxy: connection not allowed", "SOCKS proxy: TTL expired", "SOCKS proxy: command not supported by proxy", "SOCKS proxy: address type not supported") so they no longer fall through to the generic formatter; modify the error mapping where other error.Socks* cases are listed to add these symbols and corresponding bun.String.static(...) messages.src/http_jsc/websocket_client/WebSocketUpgradeClient.zig (1)
790-808:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForward bytes buffered after the SOCKS CONNECT reply.
The plain
ws://branch still ignoressocks.read_buffer.slice(). If the proxy coalesces CONNECT success with the first upstream handshake bytes, those bytes are dropped beforeprocessResponse()sees them.Suggested fix
.connected => { this.body.clearRetainingCapacity(); if (p.isTargetHttps()) { this.startProxyTLSHandshake(socket, socks.read_buffer.slice()); return; } this.state = .reading; if (this.input_body_buf.len > 0) { bun.default_allocator.free(this.input_body_buf); } this.input_body_buf = p.takeWebsocketRequestBuf(); const wrote = socket.write(this.input_body_buf); if (wrote < 0) { this.terminate(ErrorCode.failed_to_write); return; } this.to_send = this.input_body_buf[`@as`(usize, `@intCast`(wrote))..]; + + const remain_buf = socks.read_buffer.slice(); + if (remain_buf.len > 0) { + this.handleData(socket, remain_buf); + } },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig` around lines 790 - 808, The code in WebSocketUpgradeClient (.connected branch) forwards socks.read_buffer.slice() into startProxyTLSHandshake only for HTTPS but drops that buffer for plain ws:// paths; ensure any bytes in socks.read_buffer are preserved and pushed into the read path before sending the buffered client handshake: when p.isTargetHttps() is false, prepend or append socks.read_buffer.slice() into this.input_body_buf (or set this.to_send/read buffer appropriately) so processResponse() will see those bytes; update the .connected branch around startProxyTLSHandshake, this.input_body_buf assignment, and this.to_send handling to merge the existing socks.read_buffer.slice() into the outgoing/read buffers for the plain ws:// case.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 785-788: The SOCKS flush call in WebSocketUpgradeClient currently
treats a partial write as final, which can leave socks.write_buffer.cursor with
remaining bytes and stall the handshake; update handleWritable (and any
write-path that calls socks.flush(socket)) to loop/retry flushing until the
buffer is empty or the socket returns EWouldBlock, and do not call
terminate(ErrorCode.failed_to_write) on a partial write; instead re-arm or
return so the writable event will be retried, ensuring socks.flush(socket) is
invoked again to drain socks.write_buffer fully before proceeding with the
handshake.
In `@test/js/web/websocket/proxy-test-utils.ts`:
- Around line 288-296: The current targetSocket 'error' handler sends a SOCKS
failure reply even after stage is set to "tunnel", which corrupts the proxied
stream; update the handler on targetSocket to check the stage variable and only
write/send the 0x05 failure reply (fail(0x05)) when stage !== "tunnel"; if stage
=== "tunnel", instead just close/destroy the clientSocket and/or targetSocket to
teardown the tunnel without emitting a SOCKS reply. Target symbols to change:
targetSocket.on("error", ...), stage, clientSocket and targetSocket
close/destroy calls.
---
Outside diff comments:
In `@src/http/AsyncHTTP.zig`:
- Around line 205-218: The proxy-handling block in AsyncHTTP.zig currently
overwrites this.client.flags.disable_keepalive with "this.url.isHTTPS() or
is_socks", losing any previously-set true value; change the assignment to OR
into the existing flag instead (e.g., set this.client.flags.disable_keepalive =
this.client.flags.disable_keepalive or this.url.isHTTPS() or is_socks) so
explicit disable_keepalive set earlier is preserved; apply the same OR-fix to
the other occurrence around lines 287-296 where the flag is being replaced.
---
Duplicate comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 790-808: The code in WebSocketUpgradeClient (.connected branch)
forwards socks.read_buffer.slice() into startProxyTLSHandshake only for HTTPS
but drops that buffer for plain ws:// paths; ensure any bytes in
socks.read_buffer are preserved and pushed into the read path before sending the
buffered client handshake: when p.isTargetHttps() is false, prepend or append
socks.read_buffer.slice() into this.input_body_buf (or set this.to_send/read
buffer appropriately) so processResponse() will see those bytes; update the
.connected branch around startProxyTLSHandshake, this.input_body_buf assignment,
and this.to_send handling to merge the existing socks.read_buffer.slice() into
the outgoing/read buffers for the plain ws:// case.
In `@src/http/SocksProxy.zig`:
- Around line 63-76: The constructor currently allows a non-empty username with
an empty password which results in sending a zero-length RFC1929 password;
modify the validation in SocksProxy (where proxy.username and proxy.password are
handled and PercentEncoding.decodeAlloc is used) to explicitly reject
username-only credentials by returning error.SocksCredentialsIncomplete when
proxy.username.len > 0 and proxy.password.len == 0 (in both places the
username/password are processed, e.g., the blocks around
this.username/this.password decoding and the alternate block at lines ~87-99),
then proceed with decoding only when both are present and still enforce the
existing max-length checks that return error.SocksCredentialsTooLong.
In `@src/runtime/webcore/fetch/FetchTasklet.zig`:
- Around line 780-788: The match in replyError() inside FetchTasklet.zig
currently maps many SOCKS errors but omits SocksConnectionNotAllowed,
SocksTTLExpired, SocksCommandNotSupported, and SocksAddressTypeNotSupported;
update the same match arm (the error.Socks* cases) to include these four
variants with clear static messages (e.g., "SOCKS proxy: connection not
allowed", "SOCKS proxy: TTL expired", "SOCKS proxy: command not supported by
proxy", "SOCKS proxy: address type not supported") so they no longer fall
through to the generic formatter; modify the error mapping where other
error.Socks* cases are listed to add these symbols and corresponding
bun.String.static(...) messages.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26afa98d-8f4e-4667-927b-6eccaf2e6ec8
📒 Files selected for processing (7)
src/http/AsyncHTTP.zigsrc/http/SocksProxy.zigsrc/http/http.zigsrc/http_jsc/websocket_client/WebSocketUpgradeClient.zigsrc/runtime/webcore/fetch/FetchTasklet.zigtest/js/bun/http/proxy.test.tstest/js/web/websocket/proxy-test-utils.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 213-256: SocksProxy.initWithCredentials can return credential
validation errors and the current code returns null on any error without freeing
the previously allocated body, leaking memory; before returning null from the
error path of SocksProxy.initWithCredentials (the catch after calling
SocksProxy.initWithCredentials in WebSocketUpgradeClient.zig), explicitly
free/deinit the body that was allocated earlier (the same body that would later
be passed into WebSocketProxy.init) and then return null, while keeping OOM
handling (bun.handleOom) unchanged for Allocator.Error paths.
In `@src/http/SocksProxy.zig`:
- Around line 229-239: In writeConnect (SocksProxy) the socks5h branch currently
unconditionally emits an ATYP domain (0x03); change it to first detect numeric
IP literals and write the appropriate ATYP+address bytes (IPv4 0x01 or IPv6
0x04) for both .socks5 and .socks5h, and only fall back to the domain-name path
(0x03 with length+bytes) when target_host is not a numeric IP; keep using
writePort and update this.state = .connect_response and return .written as
before.
In `@test/js/web/websocket/proxy-test-utils.ts`:
- Around line 221-229: The SOCKS5 handshake simulator is too permissive: when
handling stage "method" (using buffer, nmethods and options.requireAuth) you
must validate that the chosen method is actually advertised by the client before
replying, check the auth subnegotiation version byte in the "auth" stage for
correct 0x01 value, and in the "connect" (request) stage enforce that the
command byte equals CONNECT (0x01) and that the ATYP and address/port lengths
are consistent; on any protocol violation send the proper SOCKS5 failure reply
and close the socket. Apply the same stricter validations and rejection behavior
to the other similar blocks mentioned (the sections covering lines 232-249 and
252-277) so the fixture only accepts fully valid SOCKS5 flows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 98ada0cc-cb7b-40b2-af6f-5e968ae21082
📒 Files selected for processing (7)
src/http/AsyncHTTP.zigsrc/http/SocksProxy.zigsrc/http_jsc/websocket_client/WebSocketUpgradeClient.zigsrc/runtime/webcore/fetch/FetchTasklet.zigtest/js/bun/http/proxy.test.tstest/js/web/websocket/proxy-test-utils.tstest/js/web/websocket/websocket-proxy.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/HTTPThread.zig (1)
263-321: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDeduplicate the proxy protocol allow-list across the three connect paths.
The same
http/https/socks5/socks5h(plus empty) check is now repeated at lines 264, 306, and 317. Adding a new scheme later requires updating all three sites and risks the kind of cache-hit/cache-miss drift the previous review caught. Extract a small helper next toproxyPort.♻️ Proposed refactor
+fn isSupportedProxyProtocol(url: bun.URL) bool { + return url.protocol.len == 0 or + strings.eqlComptime(url.protocol, "https") or + strings.eqlComptime(url.protocol, "http") or + strings.eqlComptime(url.protocol, "socks5") or + strings.eqlComptime(url.protocol, "socks5h"); +} + fn proxyPort(url: bun.URL) u16 { if (strings.eqlComptime(url.protocol, "socks5") or strings.eqlComptime(url.protocol, "socks5h")) { if (url.getPort()) |_| return url.getPortAuto(); return 1080; } return url.getPortAuto(); }Then each call site collapses to:
- if (!(url.protocol.len == 0 or strings.eqlComptime(url.protocol, "https") or strings.eqlComptime(url.protocol, "http") or strings.eqlComptime(url.protocol, "socks5") or strings.eqlComptime(url.protocol, "socks5h"))) { - return error.UnsupportedProxyProtocol; - } + if (!isSupportedProxyProtocol(url)) return error.UnsupportedProxyProtocol; return try entry.ctx.connect(client, url.hostname, proxyPort(url));- if (this.http_proxy) |url| { - if (url.protocol.len == 0 or strings.eqlComptime(url.protocol, "https") or strings.eqlComptime(url.protocol, "http") or strings.eqlComptime(url.protocol, "socks5") or strings.eqlComptime(url.protocol, "socks5h")) { - return try custom_context.connect(client, url.hostname, proxyPort(url)); - } - return error.UnsupportedProxyProtocol; - } + if (this.http_proxy) |url| { + if (!isSupportedProxyProtocol(url)) return error.UnsupportedProxyProtocol; + return try custom_context.connect(client, url.hostname, proxyPort(url)); + }- if (url.protocol.len == 0 or strings.eqlComptime(url.protocol, "https") or strings.eqlComptime(url.protocol, "http") or strings.eqlComptime(url.protocol, "socks5") or strings.eqlComptime(url.protocol, "socks5h")) { - return try this.context(is_ssl).connect(client, url.hostname, proxyPort(url)); - } - return error.UnsupportedProxyProtocol; + if (!isSupportedProxyProtocol(url)) return error.UnsupportedProxyProtocol; + return try this.context(is_ssl).connect(client, url.hostname, proxyPort(url));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http/HTTPThread.zig` around lines 263 - 321, The code repeats the same proxy protocol allow-list check in three places (the entry.ctx.connect path, custom_context.connect path, and the this.context.connect path); create a small helper function (e.g., isAllowedProxyProtocol(url) placed next to proxyPort) that encapsulates the condition (url.protocol.len == 0 or strings.eqlComptime(... "https"/"http"/"socks5"/"socks5h")) and use it at each call site (replace the inline checks in the branches that guard entry.ctx.connect, custom_context.connect, and this.context.connect) so each site simply calls the helper and returns error.UnsupportedProxyProtocol when it returns false. Ensure the helper uses the same strings.eqlComptime calls and preserves existing behavior for empty protocol handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 782-785: The SOCKS catch blocks around socks.receive(...) and
socks.writeConnectResolved(...) are swallowing error.OutOfMemory by translating
it into proxy errors; update those catch handlers to call bun.handleOom(err)
before converting or terminating so OOM triggers Bun’s crash behavior.
Specifically, in the catch for socks.receive in WebSocketUpgradeClient and the
catch(s) for socks.writeConnectResolved, invoke bun.handleOom(err) (or the
equivalent handleOom function) to rethrow/crash on error.OutOfMemory, then
proceed to call this.terminate(socksErrorCode(err)) or return for other errors
as before.
In `@test/js/web/websocket/websocket-proxy.test.ts`:
- Around line 890-900: The test "ws:// through socks5 proxy rejects username
without password" currently treats both ws.onerror and ws.onclose as success,
which lets a silent close pass; change the handlers so that ws.onerror (and
optionally ws.onopen) resolve/reject appropriately but ws.onclose rejects the
test—specifically update the WebSocket event handlers in this test (ws.onopen,
ws.onerror, ws.onclose) so that ws.onerror resolves the
Promise.withResolvers<void>() and ws.onclose calls reject(new Error("Expected
SOCKS credential validation to fail with an error, not a clean close")) to
ensure a clean close fails the test.
---
Outside diff comments:
In `@src/http/HTTPThread.zig`:
- Around line 263-321: The code repeats the same proxy protocol allow-list check
in three places (the entry.ctx.connect path, custom_context.connect path, and
the this.context.connect path); create a small helper function (e.g.,
isAllowedProxyProtocol(url) placed next to proxyPort) that encapsulates the
condition (url.protocol.len == 0 or strings.eqlComptime(...
"https"/"http"/"socks5"/"socks5h")) and use it at each call site (replace the
inline checks in the branches that guard entry.ctx.connect,
custom_context.connect, and this.context.connect) so each site simply calls the
helper and returns error.UnsupportedProxyProtocol when it returns false. Ensure
the helper uses the same strings.eqlComptime calls and preserves existing
behavior for empty protocol handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75a969ad-e7cd-44cd-896f-c93456cae6ea
📒 Files selected for processing (7)
src/http/HTTPThread.zigsrc/http/SocksProxy.zigsrc/http/http.zigsrc/http_jsc/websocket_client/WebSocketUpgradeClient.zigtest/js/bun/http/proxy.test.tstest/js/web/websocket/proxy-test-utils.tstest/js/web/websocket/websocket-proxy.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http_jsc/websocket_client/WebSocketUpgradeClient.zig`:
- Around line 890-899: completeSocksFromDnsReqWs (the block using
result.info.?[0]) currently picks only the first DNS answer and calls
continueSocksAfterDNS with that single address; change the logic to preserve the
full result.info array (or its addr list) and pass the list (or an
iterator/index) to continueSocksAfterDNS so the implementation can retry
subsequent addresses on failure instead of hard-coding the first entry; ensure
you still handle the proxy null check (this.proxy) and the addrFromSockaddr
error path (terminate(ErrorCode.proxy_connect_failed)) but replace const entry =
&result.info.?[0] and single-address usage with code that retains result.info
for retries and attempts addresses sequentially (or supplies index-based retry
hooks) to preserve multi-address fallback for dual-stack hosts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 46d97e38-c015-4b23-8c86-f0e07938a412
📒 Files selected for processing (2)
src/http_jsc/websocket_client/WebSocketUpgradeClient.zigtest/js/web/websocket/websocket-proxy.test.ts
Author
|
@Jarred-Sumner can you take a look? |
Collaborator
|
Thank you for this work. Two things changed since the last update here.
Do you plan to port this PR to the Rust code? If yes, the |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds SOCKS5 proxy support across Bun’s HTTP client surfaces (#16812)
socks5://andsocks5h://proxies forfetch(), nativeWebSocket, and thewspackage wrappersocks5://resolves target hostnames locally and sends the resolved IP to the proxysocks5h://sends the hostname to the proxy for remote DNS resolutionsocks5://hostnamesHow did you verify your code works?
Tested manually with:
env.HTTP_PROXY:fetch(),http.get()env.HTTPS_PROXY:fetch(),https.get()fetch(),http.get(),https.get()with{ proxy }Automated verification:
bun bd --asan=off test test/js/bun/http/proxy.test.ts -t "SOCKS|socks5"bun bd --asan=off test test/js/web/websocket/websocket-proxy.test.tsbun bd --asan=off test test/js/first_party/ws/ws-proxy.test.tsbun run zig:check-allbun test test/integration/bun-types/bun-types.test.tsNote: local
bun bdwith ASAN was blocked by the WebKit debug ASAN prebuilt artifact in dev environment, so runtime tests were run with--asan=off;zig:check-allpassed.