Conversation
…ad of 'Was there a typo in the url or port?'
When fetch({tls:{cert,key,passphrase}}) carried a bad client identity (cert/key
mismatch, wrong passphrase, non-PEM string), the rejection was always
{ code: 'FailedToOpenSocket', message: 'Was there a typo in the url or port?' }.
The SSL_CTX for a custom tls config is built lazily on the HTTP thread.
us_ssl_ctx_build_raw returns NULL for bad cert/key without setting *err (the
detail lives on the thread-local BoringSSL error queue), and HTTPContext maps
err==none to FailedToOpenSocket without reading the queue. node:tls
createSecureContext already routes the same NULL+none case through
create_bun_socket_error_to_js, which pops ERR_get_error() and builds a
Node-shaped error with code/library/reason.
Build the SSL_CTX eagerly on the JS thread when the config would require a
custom context, and route failure through the same helper. Bad material now
rejects with ERR_OSSL_X509_KEY_VALUES_MISMATCH / ERR_OSSL_BAD_DECRYPT /
ERR_OSSL_PEM_NO_START_LINE, matching node:https.
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: diff is green; waiting on a maintainer to pick between this and #36149. Reproduced on 1.4.0 and current main: $ bun repro.ts
mismatch {"code":"FailedToOpenSocket","message":"Was there a typo in the url or port?"}
wrong-pass {"code":"FailedToOpenSocket","message":"Was there a typo in the url or port?"}
junk-pem {"code":"FailedToOpenSocket","message":"Was there a typo in the url or port?"}
control "ok"Fail-before/after on the new tests: $ USE_SYSTEM_BUN=1 bun test test/js/web/fetch/fetch.tls.test.ts -t "client tls material"
1 pass 3 fail # all three collapse to FailedToOpenSocket; control passes
$ bun bd test test/js/web/fetch/fetch.tls.test.ts -t "client tls material"
4 pass 0 failCI on 2651349 (build 83648): |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
#36149 fixes the same bug with a different approach: it captures Trade-off between the two:
Either is mergeable on its own; leaving both open for a maintainer to pick. |
There was a problem hiding this comment.
No bugs found, but deferring: this touches the client TLS setup path and makes a design trade-off (probe-build the SSL_CTX on the JS thread, free it, then rebuild on the HTTP thread) that a maintainer should sign off on — in particular, the probe fires on every fetch() with requires_custom_request_ctx, even when the HTTP-thread ctx cache would have hit.
What was reviewed:
- Error-path ownership:
SSLConfighasDrop, soconfigis released on theNonearm; theSomearm's +1SSL_CTXis freed viaSSL_CTX_free. create_bun_socket_error_to_js's.nonearm readsERR_get_error()on the same thread that just failed the build, so the queue is populated.Err(throw_value(...))flows throughreject_on_exceptioninto a rejected promise, sofetch()still never throws synchronously.- Test fixtures (
agent1-cert.pem,agent2-key.pem,rsa_private_encrypted.pem,rsa_cert.crt) all exist undertest/js/node/test/fixtures/keys/.
Extended reasoning...
Overview
Adds an eager SSL_CTX build on the JS thread inside fetch_impl's tls-option handling (src/runtime/webcore/fetch.rs), gated on config.requires_custom_request_ctx. On failure it throws via the existing create_bun_socket_error_to_js helper (same one node:tls and bun:sql use), whose .none arm pulls the reason off the thread-local BoringSSL error queue. Four new tests in test/js/web/fetch/fetch.tls.test.ts cover cert/key mismatch, wrong passphrase, non-PEM cert, and a matching control.
Security risks
The change is in client-side TLS material handling. It does not weaken any check — the same us_ssl_ctx_from_options runs, just earlier and on a different thread — and rejectUnauthorized/verification behavior is untouched. No new user input reaches BoringSSL that didn't before. The risk surface is the extra unsafe SSL_CTX_free and the assumption that create_ssl_context's Some return is a +1 ref; us_ssl_ctx_from_options does return a fresh ctx, so that holds.
Level of scrutiny
Medium-high. TLS is security-sensitive per the review guidelines, and the fix is at the call site rather than at the layer that owns the invariant (uSockets' us_ssl_ctx_build_raw not setting *err, or the HTTP thread not draining the BoringSSL queue). The PR description argues this is deliberate — the error queue is thread-local and marshaling it across the HTTP→JS boundary would need new plumbing — and it matches how SecureContext::create_private and SQL's ConnectionCtorArgs already handle the same failure. That's a reasonable trade-off, but it's the kind of "fix location" call REVIEW.md says a maintainer should agree with.
Other factors
- Perf: the probe build parses PEM and validates the key pair on every
fetch()call with custom cert material, even when the HTTP thread's per-interned-config ctx cache would have hit. For a hot loop reusing onetlsconfig that's N extraSSL_CTX_new/use_certificate/use_PrivateKey/freecycles on the JS thread where previously there was one build total. Not a correctness issue, but worth a maintainer deciding whether that's acceptable vs. caching the probe result or fixing at the HTTP-thread layer. - Memory:
SSLConfigimplementsDrop(callsdeinit), so thereturn Errpath releases it; the options struct returned byas_usockets_for_client_verificationborrows raw pointers intoconfig, which stays alive across the FFI call. - Tests: verified fail-before/pass-after via the robobun comment; assertions are exact on
code/library/reasonand include a positive control.
ca-only / ciphers-only / serverName-only tls configs now skip the probe and keep hitting the HTTP-thread context cache as before. The probe only fires when cert or key is present, which is the case whose failure uSockets reports without setting *err.
|
Updated 4:51 PM PT - Jul 27th, 2026
❌ @robobun, your commit 2651349 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36152That installs a local version of the PR into your bun-36152 --bun |
There was a problem hiding this comment.
I didn't find bugs, but this needs a maintainer's call: it's one of two competing fixes for the same issue (#36149 propagates the error from the HTTP thread; this PR probes on the JS thread and discards the ctx). The author explicitly left both open for a maintainer to pick, and the trade-off — extra SSL_CTX build per fetch-with-client-cert vs. richer error shape and smaller diff — is a design decision.
Checked: the probe is gated on cert/key presence only (so {rejectUnauthorized:false}/ca-only skip it), the ? early return drops config cleanly via SSLConfig::Drop, and reject_on_exception turns the thrown value into a rejected promise. The create_bun_socket_error_to_js(.none) → ERR_get_error() pattern matches SecureContext/Listener. Test fixtures (agent1-cert.pem, rsa_private_encrypted.pem, etc.) all exist.
Extended reasoning...
Overview
Adds validate_client_tls_identity() in src/runtime/webcore/fetch.rs that eagerly builds and frees an SSL_CTX on the JS thread when a fetch tls config carries client cert/key material, so a bad cert/key rejects with the BoringSSL ERR_OSSL_* code/library/reason instead of the HTTP thread's generic FailedToOpenSocket. Four new tests in fetch.tls.test.ts cover mismatch, wrong passphrase, non-PEM, and a control.
Security risks
Touches client-side TLS setup in fetch(). The change only adds a validation probe before the existing path; it doesn't alter verification, rejectUnauthorized, or how the real SSL_CTX is built on the HTTP thread. No new attack surface identified.
Level of scrutiny
Medium-high. It's TLS-adjacent runtime code, and — more importantly — the author explicitly framed this as one of two competing implementations and left the choice to a maintainer. Per the repo's review guidance ("fix bugs at the layer that owns the violated invariant"), #36149's HTTP-thread capture is arguably the more architecturally correct layer, while this PR is smaller and reuses the existing create_bun_socket_error_to_js decoration. That's a human call.
Other factors
- The gate was narrowed (commit bd2c6c0) from
requires_custom_request_ctxto cert/key-only, soca/ciphers-only configs no longer get the probe — the trade-off table in the PR thread predates that narrowing. - Runtime cost: one extra
SSL_CTXbuild+free on the JS thread per fetch with client identity. Acceptable for the use case but worth a maintainer's ack. - Error propagation checked:
validate_client_tls_identity(...)?throws →fetch_implreturnsErr(Thrown)→reject_on_exceptionconverts to a rejected promise (fetch never throws sync).config: SSLConfigis dropped on that path and itsDropfrees the C strings. - Tests use existing Node fixture keys, spin up a local
Bun.serve({port:0}), and assert exact{code, library, reason}— strong assertions with a passing control case.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-27, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
fetch(url, { tls: { cert, key, passphrase } })with a bad client identity (cert/key mismatch, wrong passphrase, a file path passed where PEM contents are expected) rejected with the generic connection error:All three failure modes were indistinguishable, had no OpenSSL code, and the message pointed the user at the URL when the problem was their cert material.
Reproduction
Before:
After:
(Node with OpenSSL reports the second as
ERR_OSSL_EVP_BAD_DECRYPT; BoringSSL attributes it toERR_LIB_CIPHER, which is not in Node's library-prefix map, so the code has no library segment. Thelibrary/reasonfields still identify it.)Cause
The client
SSL_CTXfor a customtlsconfig is built lazily on the HTTP thread (HTTPContext::init_with_client_config->init_with_opts-> Cus_ssl_ctx_build_raw). When cert or key parsing fails, every branch doesssl_ctx_build_fail(); return NULL;without setting the*errout-param, so it staysCREATE_BUN_SOCKET_ERROR_NONE; the real reason sits on the thread-local BoringSSL error queue.init_with_optsmapsnonetoInitError::FailedToOpenSocket,HTTPThreadmaps that tohttp::Error::FailedToOpenSocket, andFetchTaskletrenders the "typo" message. There is noERR_get_erroranywhere insrc/http/.node:tlsSecureContext::create_privatecalls the samecreate_ssl_context, but onerr == noneroutes throughcreate_bun_socket_error_to_js, whose.nonearm already callserr_to_js(global, ERR_get_error())and decorates the error with Node'scode/library/reasonshape.Fix
In
fetch.rs, right afterSSLConfig::from_jsreturns a config that would require a custom SSL context (requires_custom_request_ctx), build theSSL_CTXeagerly on the JS thread. OnNone, throw viacreate_bun_socket_error_to_js(err, global)(the same helpernode:tlsandBun.listenuse) so the rejection carries the BoringSSLcode/library/reasonand a real stack. On success, release the probe context; the HTTP thread builds its own from the interned config as before. Guarding onrequires_custom_request_ctxmeans{ rejectUnauthorized: false }and other option-only configs incur no extra build.Testing
Four new cases in
test/js/web/fetch/fetch.tls.test.tsusing the existing Node key fixtures: cert/key mismatch, wrong passphrase on an encrypted key, non-PEM cert string, and a control with matching material. The first three fail on main withFailedToOpenSocketand pass with distinctERR_OSSL_*codes after the fix; the control passes on both.[review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file