ws: forward top-level TLS options (rejectUnauthorized, ca, ...) to the connection#31397
ws: forward top-level TLS options (rejectUnauthorized, ca, ...) to the connection#31397robobun wants to merge 11 commits into
Conversation
|
Updated 9:59 PM PT - Jun 20th, 2026
❌ @robobun, your commit 2111ced has 2 failures in
🧪 To try this PR locally: bunx bun-pr 31397That installs a local version of the PR into your bun-31397 --bun |
|
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:
WalkthroughCentralizes TLS option extraction and whitelisting, applies it to agent option extraction, updates BunWebSocket to use explicit ChangesWebSocket TLS options support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/js/thirdparty/ws.js`:
- Around line 193-194: The bug is that extractTlsOptions drops explicit falsy
TLS scalar values so keys like secureOptions, clientRenegotiationLimit,
clientRenegotiationWindow, and passphrase are lost when options.tls is absent;
update extractTlsOptions to copy a key when it exists on the source (e.g., using
Object.prototype.hasOwnProperty.call(source, key) or key in source) instead of
checking truthiness, so tlsValueKeys are preserved; confirm tlsOptions
assignment (the code around extractTlsOptions, tlsValueKeys, options.tls,
tlsOptions and the merge { ...agentOpts.tls, ...tlsOptions }) will now allow
falsy values to override agentOpts.tls.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d9b435cf-6ae2-45fb-a46b-f996f579ad3e
📒 Files selected for processing (2)
src/js/thirdparty/ws.jstest/js/first_party/ws/ws-tls-options.test.ts
There was a problem hiding this comment.
All earlier nits are addressed and I have nothing further — deferring only because the precedence rules around rejectUnauthorized / agent-vs-top-level merging are TLS-verification semantics that deserve a human sign-off.
Extended reasoning...
Overview
This PR changes src/js/thirdparty/ws.js (Bun's npm ws compat shim) to forward top-level TLS options (rejectUnauthorized, ca, cert, key, …) on the WebSocket client constructor into the native WebSocket's tls option, matching how the real npm ws package forwards them to https.request/tls.connect. It adds a key-whitelist extractor (extractTlsOptions) split into boolean/scalar/file buckets, an isForwardableFileValue guard for shapes SSLConfig.fromJS can't parse, and a merge step that combines agent-supplied TLS with top-level keys unless an explicit options.tls object was passed. A new test file (test/js/first_party/ws/ws-tls-options.test.ts, ~143 lines, 6 cases) covers the primary fix plus each edge case raised in review.
Security risks
The change directly governs when TLS certificate verification is enabled for a wss:// connection. The interesting surface is the precedence between three sources — explicit options.tls, extracted top-level keys, and agent.connectOpts — and specifically whether an agent's rejectUnauthorized: false (semantically aimed at the proxy hop) can leak into the target handshake. That leak existed in an intermediate commit and was fixed in 3aaf125 (guarded on !explicitTls) with a test asserting the agent's value cannot disable verification when tls: {} is passed. I don't see a remaining path that weakens verification relative to pre-PR behavior; the net effect is that users can now turn it off via the Node-canonical top-level option, which is the intended compat fix.
Level of scrutiny
Medium-high. The implementation is option plumbing in a JS shim (no native code touched), but it controls TLS verification and the merge/override rules took five review iterations to settle (agent merge vs replace, ALPNProtocols string[] throw, explicit-tls authoritativeness, falsy scalar preservation, key: [{pem,passphrase}] object-array throw). Each was addressed with a targeted regression test, and the bug-hunting pass on b390313 found nothing new. Still, the subtlety of the precedence semantics is exactly the kind of thing a maintainer should glance at rather than auto-merge.
Other factors
No CODEOWNERS entry for src/js/thirdparty/. The single CI failure (test/bundler/transpiler/transpiler.test.js on Windows) is unrelated to this change. All prior inline comments — mine and CodeRabbit's — are resolved. I have not previously posted a review body on this PR, only inline nits, so this deferral is not redundant.
|
CI is green on every lane except This change is JS-only ( |
robobun
force-pushed
the
farm/3cdf7917/ws-top-level-tls-options
branch
from
b390313 to
64975c0
Compare
|
Rebased onto latest The previous red on this PR was entirely the flaky Windows
New CI run is in flight against the rebased tree. |
There was a problem hiding this comment.
All six rounds of prior feedback are addressed and the test coverage is solid — I don't see anything left to flag, but since this is TLS-verification plumbing (precedence around rejectUnauthorized/ca between top-level, agent, and explicit tls), I'd like a human to sign off rather than auto-approve.
Extended reasoning...
Overview
This PR changes src/js/thirdparty/ws.js so Bun's ws shim forwards top-level TLS options (rejectUnauthorized, ca, cert, key, servername, …) to the native WebSocket's tls option, matching how npm ws forwards them to https.request/tls.connect. It adds ~120 lines of new logic: three key whitelists (tlsBooleanKeys, tlsScalarKeys, tlsFileKeys), an extractTlsOptions helper with per-bucket falsy-handling, an isForwardableFileValue/isRepresentableFileElement shape guard, and merge-precedence rules between top-level options, agent connectOpts, and an explicit Bun tls:{} object. A new 7-case test file exercises the happy path, the negative path, the agent-merge path, the explicit-override path, and three no-op→throw guards.
Security risks
The change is squarely in TLS-verification territory. It does not weaken any default — it forwards user-supplied options that were previously dropped — but the precedence logic determines whether rejectUnauthorized: false from one source (e.g. an agent's proxy-hop connectOpts) can leak into the target handshake. One of my earlier review rounds (3aaf125) caught and fixed exactly that leak, and the explicitTls guard plus the "keeps an explicit tls object authoritative" test now cover it. I don't see a remaining vector, but the consequence of a precedence mistake here is silently disabled certificate verification, which is why I'm not auto-approving.
Level of scrutiny
Medium-high. This is JS-only option plumbing (no native/crypto changes), but it took six review iterations to converge — each round surfaced a real edge (agent-vs-top-level shadowing, ALPNProtocols string[] throwing, explicit-tls override leak, falsy-scalar handling, {pem,passphrase} array and bare-object forms throwing). That history says the surface is subtle enough that a maintainer familiar with SSLConfig.fromJS and the agent/proxy semantics should give it a final read.
Other factors
All inline comments from me and CodeRabbit are resolved; the bug-hunting system found nothing on e57956c. Test coverage is good and each case maps to a specific review round. The robobun CI-status comment still references an older commit (64975c0) with a failure, so the latest commit's CI state isn't visible in the thread yet — worth confirming green before merge. No CODEOWNERS entry covers this path.
…e connection
The npm ws package accepts TLS options as top-level options on the
WebSocket constructor and forwards them to https.request/tls.connect:
new WebSocket("wss://host", { rejectUnauthorized: false });
Bun's ws shim only read TLS options from options.tls, so top-level keys
were silently dropped and connecting to a self-signed wss:// server
failed with "TLS handshake failed".
Collect the TLS keys the native WebSocket understands (the set
SSLConfig.fromJS parses) off the top-level options when no explicit
options.tls object is given. An explicit options.tls stays authoritative
for back-compat with Bun's existing shape. The agent path keeps its
conservative key subset, since agents like HttpsProxyAgent put connection
options in connectOpts that aren't all valid Bun TLS options.
The ws.test.ts suite spawns a subprocess echo server for most cases, which times out under the debug/ASAN build and drowns out the new regression test. Move it to test/regression/issue/31396.test.ts, which is self-contained (Bun.serve + the ws shim, no subprocess), so the rejectUnauthorized case fails cleanly without the fix and passes with it.
A single top-level TLS key made tlsOptions truthy, which skipped the agent fallback entirely and dropped the agent's ca/cert/key/passphrase. In ws, top-level TLS options and the agent's connect options both reach the connection, so merge them (top-level wins on conflict, agent fills gaps) instead of replacing one with the other.
Issue #31396 is a never-supported feature (top-level TLS options on the ws client), not a regression that worked in a prior release, so per the test-layout convention it doesn't belong in test/regression/issue/. Move it next to the existing ws-proxy.test.ts in test/js/first_party/ws/, keeping it in its own file so the debug/ASAN subprocess timeouts in ws.test.ts don't drown out the signal.
Node/ws accept ALPNProtocols as a string[], but Bun's TLS option parser (SSLConfig.fromJS) only accepts string/ArrayBuffer/null and throws a TypeError on an array. Including ALPNProtocols in the top-level key set turned a previously-ignored option into a synchronous constructor throw. Drop it from the top-level key set (the agent path already excluded it for the same reason) so it stays a no-op. WebSocket negotiates subprotocols via Sec-WebSocket-Protocol, not TLS ALPN, so this has no practical downside. Adds a test that a string[] ALPNProtocols connects instead of throwing.
The agent-merge added earlier also ran when tlsOptions came from an
explicit options.tls object, so it was no longer a hard override: an
agent's rejectUnauthorized:false (intended for the proxy hop) could leak
into a user-supplied tls:{ca} and disable target verification, where
pre-change the agent was ignored entirely.
Guard the merge on !explicitTls so an explicit tls object stays a true
override (matching the documented behavior), while the merge still
applies to the extracted top-level path it was meant to fix. Adds a test
that an agent's rejectUnauthorized:false does not leak into an explicit
tls object.
…absent extractTlsOptions copied top-level value keys only when truthy, so an explicit falsy scalar (e.g. passphrase: "", secureOptions: 0) was dropped where options.tls would have forwarded it. Split the value keys: scalar keys now forward on != null (consistent with the boolean keys and with options.tls), while file/identity keys (ca, cert, key, caFile, servername, ciphers, ...) stay on a truthy check — an empty string there means absent, and forwarding e.g. ca: "" makes the native parser treat it as a real empty value and breaks the handshake.
Node/ws accept key/cert as an array of { pem, passphrase } objects
(per-key passphrases), but Bun's native TLS parser only understands
string/ArrayBuffer/Blob or arrays of those. Forwarding the object-array
form turned a previously-ignored option into a constructor TypeError.
key/cert can't be dropped (the string/Buffer forms are the core of this
PR), so skip only the object-array shape via isForwardableFileValue — it
falls back to the pre-PR no-op instead of throwing, while the
string/Buffer/string[]/Blob forms still forward. Adds a test that an
object-array key constructs without throwing.
…ed one
isForwardableFileValue only applied the plain-object guard to array
elements, so a bare `key: { pem, passphrase }` (or a KeyObject) fell
through and was forwarded to SSLConfig.fromJS, which throws — asymmetric
with `key: [{ pem }]`, which was correctly skipped, and contrary to the
function's own contract.
Factor the shape check into isRepresentableFileElement and apply it to
the bare value too, so both the wrapped and unwrapped object forms stay a
no-op instead of throwing at construction. Adds a test for the bare form.
robobun
force-pushed
the
farm/3cdf7917/ws-top-level-tls-options
branch
from
e57956c to
2111ced
Compare
|
Rebased onto latest The only conflicts were in
The ws TLS diff is otherwise unchanged. Verified on the rebased tree: |
|
The only red on the latest run (build 63719, sha 2111ced) is the Lint JavaScript GitHub Actions job, which failed with: That's a runner-provisioning hiccup (the The ws TLS code is lint-clean (its |
1 participant
What
The npm
wspackage accepts TLS options as top-level options on theWebSocketclient constructor and forwards them to the underlyinghttps.request/tls.connect:Bun's
wsshim (src/js/thirdparty/ws.js) only read TLS options fromoptions.tls(Bun's internal shape) or from anagent. It never read the top-level keys, sorejectUnauthorized: falsewas silently dropped and connecting to a self-signedwss://server failed withTLS handshake failed.Fixes #31396.
Reproduction
Node connects; Bun failed. Passing the same option via Bun's
tls: { rejectUnauthorized: false }shape already worked — proving the native TLS layer was fine and the bug was purely option plumbing in the shim.Cause
In the constructor, TLS was read only from
options.tls:Top-level
rejectUnauthorized/ca/cert/key/… were never consulted.Fix
When no explicit
options.tlsobject is given, collect the TLS keys the native WebSocket understands (the setSSLConfig.fromJSparses) off the top-leveloptions:options.tlsstays authoritative, preserving Bun's existing shape.rejectUnauthorized, …) are forwarded with a!== undefinedcheck so an explicitfalseis honored.HttpsProxyAgent, …) keeps its conservative key subset — agents put connection options inconnectOptsthat aren't all valid Bun TLS options (e.g. an array-formALPNProtocols, whichSSLConfig.fromJSrejects).Verification
New tests in
test/js/first_party/ws/ws-tls-options.test.ts(alongside the existingws-proxy.test.ts) stand up a self-signedwss://server viaBun.serve({ tls })and connect through thewsshim:rejectUnauthorized: falseconnects (fails before this change, passes after).caplus a top-levelservernameconnects — both TLS sources reach the handshake, covering the merge path.The
wsandws-proxysuites are otherwise unchanged from their baseline.