Conversation
…ledToOpenSocket
fetch(url, {tls: {cert, key, passphrase}}) collapsed every client
certificate/key load failure (cert/key mismatch, wrong or missing
passphrase for an encrypted key, non-PEM input) into a generic
FailedToOpenSocket "Was there a typo in the url or port?" error.
node:https in the same process already reports the real BoringSSL codes
(ERR_OSSL_X509_KEY_VALUES_MISMATCH, ERR_OSSL_BAD_DECRYPT,
ERR_OSSL_PEM_NO_START_LINE) via SecureContext.
us_ssl_ctx_build_raw returns NULL with *err left at
CREATE_BUN_SOCKET_ERROR_NONE for cert/key/passphrase failures; the real
cause is only on the calling thread's BoringSSL error queue. The
fetch-client SSL_CTX is built on the HTTP thread, and
HTTPContext::init_with_opts mapped (None, err == none) to
InitError::FailedToOpenSocket without reading the queue.
Capture ERR_get_error() in init_with_opts (same thread as the failed
SSL_CTX_* calls), carry the packed u32 across in a new
InitError::ClientTLSSetup / http::Error::ClientTLSSetup variant, and
format it on the JS thread in FetchTasklet::on_reject using the same
ERR_OSSL_<LIB>_<REASON> composition SecureContext uses. The resulting
error carries .code (ERR_OSSL_*), .message (ERR_error_string_n output)
and .path (the request URL).
WalkthroughChangesClient TLS error propagation
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
ProxyTunnel::start builds the inner-TLS SSL_CTX for the http-proxy -> https-target CONNECT tunnel via SSLWrapper::init_from_options; on bad cert/key that site returned InitError::InvalidOptions and was mapped to ConnectionRefused without reading the BoringSSL queue. Capture ERR_get_error() there too (same thread as the failed SSL_CTX_* calls) and surface Error::ClientTLSSetup(packed) so the proxy path reports the same ERR_OSSL_* code as the direct path. Also fold the ERR_OSSL_<LIB>_<REASON> code composition into a shared build_err_ossl_code helper and have err_to_js call err_code_and_message for its (code, message), so the fetch and node:https paths cannot drift.
The ERR_get_error/ERR_clear_error pattern now has a single named helper in http::error that both HTTPContext and ProxyTunnel call, so the thread-local-queue invariant is documented once and the call sites are self-explanatory.
|
Updated 5:09 PM PT - Jul 27th, 2026
❌ @robobun, your commit 96c7375 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36149That installs a local version of the PR into your bun-36149 --bun |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/http/ProxyTunnel.rs:635-641— The follow-up commit adds the sameERR_get_error() → if packed != 0 { ERR_clear_error(); ClientTLSSetup(packed) } else { <fallback> }block at bothHTTPContext::init_with_optsand here inProxyTunnel::start, each preceded by a near-identical 4-line explanatory comment (both already flagged by comment-cop). A smallfn take_boringssl_error() -> Option<u32>in the http crate would let each site collapse totake_boringssl_error().map(…::ClientTLSSetup).unwrap_or(<fallback>), centralise the queue-read discipline in one place, and drop the duplicated comment — the same within-diff dedup this PR already applied toboringssl_jsc.rs(build_err_ossl_code). Small enough to ship as-is; noting for consistency.Extended reasoning...
What the issue is
The follow-up commit (
ae8459c5, addressing the earlier proxy-tunnel review comment) introduces the same multi-line block at two sites in the same diff:src/http/HTTPContext.rs:503-509(insideinit_with_opts):let packed = bun_boringssl_sys::ERR_get_error(); if packed != 0 { bun_boringssl_sys::ERR_clear_error(); InitError::ClientTLSSetup(packed) } else { InitError::FailedToOpenSocket }
src/http/ProxyTunnel.rs:635-641(insideProxyTunnel::start):let packed = bun_boringssl_sys::ERR_get_error(); let fail = if packed != 0 { bun_boringssl_sys::ERR_clear_error(); crate::Error::ClientTLSSetup(packed) } else { crate::Error::ConnectionRefused };
Each is preceded by a near-identical 4-line comment explaining the same BoringSSL-error-queue capture rationale ("create_ssl_context leaves the detail on this thread's BoringSSL error queue … same thread as the failed SSL_CTX_* calls"). The comment-cop bot has already flagged both comments independently.
Why this is flagged
REVIEW.md, Simplest honest shape; deduplicate within your own diff: "The second time a multi-line block appears in your diff, extract a named helper and use it at EVERY parallel site." This PR already applied that exact rule once in this review cycle — the previous round extracted
build_err_ossl_codeinboringssl_jsc.rssoerr_code_and_messageanderr_to_jsshare one code-composition path. Applying it consistently here means the twocreate_ssl_context-failure sites should share the queue-read step the same way.Addressing the counterargument
It's true that (a) the codebase already open-codes
ERR_get_error()+ERR_clear_error()inline at ~10 pre-existing sites (CryptoHasher.rs,tls_socket_functions.rs,Listener.rs, etc.) with no shared helper, (b) the two blocks return different enum types with different fallbacks, and (c) REVIEW.md also says "don't ride file-wide standardization on a focused bugfix." These are fair points and are why this is a nit, not a blocker.But the within-diff rule is scoped to your own diff, not the pre-existing sites: extracting a small helper for the two new sites does not require touching the other ten (that would indeed be scope creep). The different enum types are exactly why the natural factoring is
fn take_boringssl_error() -> Option<u32>— it captures only the shared queue-read discipline and lets each caller mapSome(p)/Noneto its own variant. And the fact that both sites need the same 4-line comment to justify the same 3 lines is itself the signal: a named helper's doc-comment carries the rationale once, and comment-cop stops firing at both sites.Step-by-step drift example
- A future change decides the last queued error is the interesting one for client-cert failures (BoringSSL pushes several entries for a PEM parse failure; the deepest is often the most specific), and switches
HTTPContext.rstoERR_peek_last_error()followed byERR_clear_error(). ProxyTunnel.rsis not updated — nothing links the two sites except the duplicated comment.- Now
fetch(url, {tls: {cert: "not a pem"}})reports oneERR_OSSL_*code, whilefetch(url, {proxy: 'http://…', tls: {cert: "not a pem"}})reports a different one for the identical input — precisely the direct-vs-proxy divergence this PR is closing. - The test suite asserts
^ERR_OSSL_at both sites, so both still pass; the drift is silent.
How to fix
Add to the http crate (or wherever both sites can reach it):
/// Pop the top of this thread's BoringSSL error queue after a failed /// `create_ssl_context`. `us_ssl_ctx_build_raw` returns NULL with the enum /// left at `.none` for client cert/key/passphrase failures; the real cause /// is only here. Clears the rest of the queue on hit. fn take_boringssl_error() -> Option<u32> { let packed = bun_boringssl_sys::ERR_get_error(); if packed != 0 { bun_boringssl_sys::ERR_clear_error(); Some(packed) } else { None } }
Then
HTTPContext.rsbecomestake_boringssl_error().map(InitError::ClientTLSSetup).unwrap_or(InitError::FailedToOpenSocket)andProxyTunnel.rsbecomestake_boringssl_error().map(crate::Error::ClientTLSSetup).unwrap_or(crate::Error::ConnectionRefused). The 4-line comment at each call site collapses into the helper's doc-comment.Why nit
Nothing breaks if this ships as-is: both sites are correct today, the shared core is only ~3 substantive lines, and the open-coded pattern matches existing convention elsewhere in the tree. This is a maintainability/consistency observation — the same rule was applied to
boringssl_jsc.rsin this PR, and the duplicated comment-cop hits suggest the code wants the same treatment here — not a functional defect. - A future change decides the last queued error is the interesting one for client-cert failures (BoringSSL pushes several entries for a PEM parse failure; the deepest is often the most specific), and switches
There was a problem hiding this comment.
All three rounds of prior feedback are addressed (proxy-tunnel path now covered, err_to_js deduped through the new helper, proxy test swallows ECONNRESET). No further issues found, but this threads a new error variant across the HTTP-thread boundary and refactors the shared err_to_js used by node:https/SecureContext, so it's worth a maintainer look.
What was reviewed:
take_boringssl_error()runs on the HTTP thread immediately after each failedcreate_ssl_context(both sites), reading the correct thread-local queue; the packedu32crosses to the JS thread whereerr_code_and_messagedoes only pure table lookups.err_to_jsrefactor preserves behavior:.codestill gated on non-empty (⇔reasonpresent),.reason/.library/.functionunchanged.- All
InitError/http::Errormatch sites updated exhaustively (HTTPThread, PackageManager, on_init_error_noop). - Test fixtures
rsa_cert.crt/rsa_private_encrypted.pemexist; 10 cases cover direct + CONNECT-proxy paths.
Extended reasoning...
Overview
The PR adds a new ClientTLSSetup(u32) variant to both http::InitError and http::Error, carrying a packed BoringSSL error code. A new http::error::take_boringssl_error() helper pops and drains the thread-local BoringSSL error queue at the two create_ssl_context failure sites (HTTPContext::init_with_opts for direct TLS, ProxyTunnel::start for the CONNECT-tunnel inner TLS). FetchTasklet::on_reject formats the packed code via a new JSC-free boringssl_jsc::err_code_and_message helper into a SystemError with ERR_OSSL_* .code, the ERR_error_string_n .message, and the request URL as .path. err_to_js (used by SecureContext / node:https) is refactored to call the same helper so the two paths cannot drift. Match arms in HTTPThread.rs, PackageManager.rs, and on_init_error_noop are updated for exhaustiveness. Ten new test cases (5 direct, 5 via http CONNECT proxy) assert err.code matches ^ERR_OSSL_ and the old generic codes/messages are gone.
Security risks
None identified. This is diagnostic-only: it surfaces more information about why a client-side TLS setup failed, which was previously discarded. No new inputs are parsed, no validation is loosened, and the failure paths still fail the request. The BoringSSL error strings come from static tables, not user input.
Level of scrutiny
Medium-high. The change is well-scoped and additive (new enum variant, new fallback branch at each failure site, existing arms untouched), but it threads state across the HTTP-thread → JS-thread boundary and refactors err_to_js, a shared function on the node:https / SecureContext error path. The thread-affinity concern (BoringSSL's error queue is thread-local) is handled correctly: take_boringssl_error() runs on the HTTP thread immediately after the failing FFI call, and only the packed u32 crosses threads; err_code_and_message on the JS side calls only ERR_error_string_n / ERR_reason_error_string, which are pure lookups on the packed code, not queue reads.
Other factors
This PR has been through three prior review rounds from me, each addressed by a follow-up commit (ae8459c covered the ProxyTunnel sibling site and deduped err_to_js; 142c645 extracted the shared take_boringssl_error helper; 9314515 trimmed comments; e09a137 added the ECONNRESET swallow on the test proxy socket). The comment-cop bot's long-comment complaints are all resolved. The err_to_js refactor preserves behavior: the .code property is still only attached when the reason string is present (empty code Vec ⇔ ERR_reason_error_string returned null), and .library/.function/.reason are still looked up independently. The double ERR_reason_error_string call is a pure table lookup with no side effects. Given nine files touched across the HTTP client error plumbing plus a refactor of a shared crypto formatter, deferring to a maintainer for final sign-off.
The fixture spawns a debug+ASAN bun subprocess that drives hundreds of TLS handshakes; on main with no other changes it already ran at ~4.2-5.5s against the 5s default timeout and flaked ~1 in 5 runs. Give it an explicit isASAN-gated timeout.
There was a problem hiding this comment.
All prior feedback is addressed and the bug hunter found nothing new this round. Deferring to a human for final sign-off since this threads a new error variant through the HTTP-thread TLS setup path (direct + proxy-tunnel) and refactors err_to_js (which node:https/node:crypto also route through).
Checked: take_boringssl_error() runs on the HTTP thread immediately after create_ssl_context fails at both sites (nothing in SSLWrapper::init_from_options touches the queue between the failed build and the Err return); err_code_and_message is thread-agnostic (static-table lookups only); the err_to_js refactor preserves the .reason/.code coupling (code.is_empty() ⇔ reason lookup returned None); the referenced rsa_cert.crt/rsa_private_encrypted.pem fixtures exist.
Extended reasoning...
Overview
Adds InitError::ClientTLSSetup(u32) / http::Error::ClientTLSSetup(u32) carrying a packed BoringSSL error, populated via a new take_boringssl_error() helper at the two create_ssl_context failure sites (HTTPContext::init_with_opts and ProxyTunnel::start). FetchTasklet::on_reject formats it via a new JSC-free err_code_and_message() helper, and err_to_js is refactored to share that helper. Exhaustive-match arms added in HTTPThread.rs and PackageManager.rs. Ten new test cases (5 inputs × direct/proxy paths); an unrelated fixture gets an ASAN timeout bump.
Security risks
None identified. This only changes which error message/code surfaces when client-cert SSL_CTX construction has already failed — no validation logic, no success path, no credential handling is touched. take_boringssl_error() drains the thread-local queue after reading, so stale errors don't leak into later operations.
Level of scrutiny
Medium-high. The change is scoped (error propagation + message formatting) and has good test coverage of both entry points, but it spans the HTTP-thread/JS-thread boundary, adds a variant to two widely-matched error enums, and refactors err_to_js which is also on the node:crypto/node:https path. It went through three review rounds (proxy-tunnel sibling site, err_to_js dedup, comment-cop trims, missing socket error listener) — all addressed. That iteration count and the TLS-adjacent surface area put it just past my auto-approve bar.
Other factors
InitError/http::ErrorremainCopy/Eqwith the newu32payload.- Verified
SSLWrapper::init_from_options(src/uws/lib.rs:502-512) returns immediately aftercreate_ssl_contextfails without disturbing the BoringSSL queue, so the ProxyTunnel-sidetake_boringssl_error()reads the right error. err_code_and_messageuses onlyERR_error_string_n/ERR_reason_error_string(static-table lookups, not queue reads), so calling it on the JS thread with a u32 captured on the HTTP thread is sound.- The
err_to_jsrefactor is behavior-preserving:.codewas previously set iffERR_reason_error_stringreturned non-null; now it's set iff!code.is_empty(), andcodeis built exactly when that same lookup succeeds. - The drive-by
isASAN ? 20000 : 10000timeout on the cert-mismatch-churn fixture is justified inline (already at 4.2-5.0s vs 5s default on main).
There was a problem hiding this comment.
Beyond the inline nit: verified the err_to_js refactor is behavior-preserving (.code set iff ERR_reason_error_string non-null, same predicate as before); take_boringssl_error() runs on the HTTP thread immediately after each failed create_ssl_context so concurrent fetches (serialized there) can't cross-contaminate the thread-local queue; and err_code_and_message on the JS thread only does static-table lookups on the packed u32. All three prior findings (ProxyTunnel sibling site, err_to_js dedup, proxy-test ECONNRESET listener) are addressed in the current diff.
Extended reasoning...
All three findings from earlier passes are resolved in the current diff: ProxyTunnel::start now calls take_boringssl_error() and maps to ClientTLSSetup (with a covering via http CONNECT proxy test block), err_to_js now delegates to err_code_and_message so the two ERR_OSSL_* composition paths share one implementation, and the proxy test's net.Socket has client.on('error', () => {}). This run additionally checked that the err_to_js refactor did not change when .code/.reason are attached, and that the HTTP-thread-local BoringSSL error queue read is race-free under describe.concurrent (each create_ssl_context → ERR_get_error → ERR_clear_error sequence completes before the next queued request runs). Deferring rather than approving because the change spans 9 files including a refactor of err_to_js (used by node:crypto/https) and HTTP-thread error propagation — worth a human glance even with the nit being the only open item.
|
CI status: the diff itself is green. Remaining red is unrelated to this change:
Ready for review. |
|
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, ...}})collapsed every client certificate/key load failure into the sameFailedToOpenSocket "Was there a typo in the url or port?"error, with no OpenSSL code, whilenode:httpsin the same process reports the real codes (ERR_OSSL_X509_KEY_VALUES_MISMATCH,ERR_OSSL_BAD_DECRYPT,ERR_OSSL_PEM_NO_START_LINE). This makes mTLS / enterprise-proxy client-cert misconfiguration look like a URL typo.Reproduction
Before:
After:
Cause
us_ssl_ctx_build_raw(packages/bun-usockets/src/crypto/openssl.c) returnsNULLwith*errleft atCREATE_BUN_SOCKET_ERROR_NONEfor client cert/key/passphrase failures; the real cause is only on the calling thread's BoringSSL error queue.SecureContext(thenode:httpspath) reads that queue viaERR_get_error()on the JS thread and formats it withboringssl_jsc::err_to_js. The fetch-clientSSL_CTXis built on the HTTP thread at two sites that never read the queue:HTTPContext::init_with_opts(direct TLS connect) mapped(None, err == none)toInitError::FailedToOpenSocket, which becamehttp::Error::FailedToOpenSocketand the "typo in the url" message inFetchTasklet::on_reject.ProxyTunnel::start(http-proxy CONNECT tunnel to an https target) mappedSSLWrapper::init_from_optionsfailure tocrate::Error::ConnectionRefused.Fix
Add
http::error::take_boringssl_error()which pops and drains the thread-local BoringSSL error queue, and call it at bothcreate_ssl_contextfailure sites immediately after the failedSSL_CTX_*calls. Carry the packedu32in newInitError::ClientTLSSetup/http::Error::ClientTLSSetupvariants.FetchTasklet::on_rejectformats it via a new JSC-freeboringssl_jsc::err_code_and_messagehelper (the sameERR_OSSL_<LIB>_<REASON>compositionerr_to_jsnow also uses) into a SystemError with.code(ERR_OSSL_*),.message(ERR_error_string_noutput) and.path(the request URL). The explicit CA/CRL enum arms are unchanged.Verification
test/js/web/fetch/fetch.tls.test.tsgains twoit.eachblocks (direct + via http CONNECT proxy) covering cert/key mismatch, encrypted key with wrong/missing passphrase, and non-PEM cert/key input; each assertserr.codematches^ERR_OSSL_and the oldFailedToOpenSocket/ConnectionRefused/"typo in the url" shapes are gone. All ten cases fail onmainand pass with this change.[review] gate passed · iteration 2 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 2
evidence per changed file
Scope
The WebSocket client builds its SSL_CTX at two sibling sites (
WebSocketProxyTunnel::startandWebSocketUpgradeClient::connectinsrc/http_jsc/websocket_client/) with the same queue-discarding shape. Those route throughbun_http_jscwith an event-based error surface (close code +errorevent, not a rejected promise with aSystemError), so threadingERR_OSSL_*there needs a newhttp_jsc::Errorvariant and close-code mapping; deferred to a follow-up. This PR does not change WebSocket behavior.