node:tls: re-check server identity on resumed sessions - #36131
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 8 days. After that, they cost $0.25 per reviewed file. Or wait 4 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes TLS certificate verification behavior and deliberately diverges from Node.js compat (reversing the direction taken in #34598/#33734), so a maintainer should sign off on the tradeoff.
What was reviewed:
- The gate removal in
onClientHandshakeComplete—checkServerIdentitynow runs on resumed sessions; the surroundingrejectUnauthorized/authorizedflow is unchanged. - The BoringSSL rationale (peer chain stored on
SSL_SESSION, sogetPeerCertificate()is populated on resume) checks out against the PR's own evidence and existing tests. - The new test: both the cross-servername rejection and same-servername resume cases resolve on every path (error/close/data all wired to
finish), no sleeps, local server onport: 0.
Extended reasoning...
Overview
One-line functional change in src/js/node/net.ts removing !self.isSessionReused() from the checkServerIdentity gate in onClientHandshakeComplete, plus a ~90-line test in test/js/node/tls/node-tls-connect.test.ts covering cross-servername resume rejection and same-servername resume success.
Security risks
This is squarely in the TLS peer-verification path. The change is security-positive on its face — it closes a case where a manually-supplied session: for hostname A resumed against hostname B would report authorized: true — and restores 1.4.0 behavior. But it also changes when user-supplied checkServerIdentity hooks fire (now on every resume, not just full handshakes), which is an observable Node-compat divergence. The PR description and inline comment document this deliberately, and the author notes the Node parallel tests (test-tls-client-resume*, test-https-agent-session-*, etc.) still pass.
Level of scrutiny
High. This reverses part of #34598 (which itself superseded #33734) and encodes a deliberate "Bun is stricter than Node here" decision in a security-sensitive code path. Per REVIEW.md's Node/Web compat guidance, a documented deliberate divergence like this warrants explicit maintainer approval rather than bot sign-off.
Other factors
The test is well-constructed: hermetic (local tls.createServer on port 0), no sleeps, every terminal event wired to resolve the outcome promise, and asserts exact authorizationError codes and the checkServerIdentity call arguments. The author verified it fails with the gate in place and passes with the change. The comment left at the gate site should prevent it being re-added for parity. Nothing looks wrong — deferring purely because TLS verification semantics and Node-divergence policy are maintainer calls.
|
Pushed 36f0b8d: adds a fail-closed arm so a resumed session that (hypothetically) surfaces without a peer certificate produces Heads up for whoever merges this: #32824 currently carries a hunk that re-adds the |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 2:22 AM PT - Sep 12th, 2026
✅ @robobun, your commit 670fc2c0ae8fe15fdb61e81cdd90c37fdfdf7e0f passed in 🧪 To try this PR locally: bunx bun-pr 36131That installs a local version of the PR into your bun-36131 --bun |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/node/net.ts:320-325— Thiselse if (self.isSessionReused())arm can never execute:getPeerCertificate(true)returns{}(truthy) when_handleexists but the native side has no peer cert, andnullonly when_handleis falsy — but this callback is dispatched from the native handle, so_handleis always set here. If you want the fail-closed policy to actually fire on an empty cert, gate on something likeif (cert?.raw)instead ofif (cert); otherwise just drop this branch and its comment (REVIEW.md: every added line must be demonstrably live).Extended reasoning...
What the bug is
Commit 36f0b8d added a defensive fail-closed arm at
src/js/node/net.ts:320-325:const cert = self.getPeerCertificate(true); if (cert) { verifyError = checkServerIdentity(hostname, cert); } else if (self.isSessionReused()) { // A resumed session that surfaces no peer certificate has nothing to // verify the current servername against; refuse to authorize rather // than fall through to authorized:true. verifyError = $ERR_TLS_CERT_ALTNAME_INVALID("Cert is empty", hostname, cert); }
The intent is that if a resumed session somehow surfaces without a peer certificate, the socket refuses to authorize instead of falling through to
authorized: true. But theelse ifarm is unreachable.Why it is unreachable
TLSSocket.prototype.getPeerCertificate(src/js/node/tls.ts:1047-1060) does:const handle = this._handle; if (handle) { const cert = ... handle.getPeerCertificate?.(...); if (cert) return translatePeerCertificate(cert); return {}; // <-- truthy } return null; // <-- only when _handle is falsy
So the only way for
self.getPeerCertificate(true)to return a falsy value is forself._handleto be falsy.onClientHandshakeCompleteis dispatched from the native handle right after the handshake finishes, soself._handleis set at this point. Thereforecertis always truthy — either a translated certificate object or{}.Step-by-step proof
- Native handshake completes → native handle invokes the JS handshake callback →
onClientHandshakeComplete(self, socket, verifyError)runs withself._handlepopulated. self.getPeerCertificate(true)enters theif (handle)branch.- Suppose (hypothetically) BoringSSL had no stored peer chain:
handle.getPeerCertificate(false)returns a falsy value → the JS wrapper returns{}. - Back in
onClientHandshakeComplete,certis{}, soif (cert)is taken andcheckServerIdentity(hostname, {})runs. - The
else if (self.isSessionReused())arm is never reached.
The exact scenario the branch's comment describes — "a resumed session that surfaces no peer certificate" — flows into the
if (cert)branch withcert = {}, not into theelse if.Impact
None on runtime behavior. BoringSSL always stores the peer chain on the
SSL_SESSION, so in practicecertis always a real certificate object here. And even if the empty-cert case did occur, it would callcheckServerIdentity(hostname, {}), which with the defaulttls.checkServerIdentityreturnsERR_TLS_CERT_ALTNAME_INVALID(no SAN, no CN → hostname cannot match) — i.e. it already fails closed via theif (cert)path. So the dead arm's policy is coincidentally still enforced; it's just the arm itself that never runs.This is flagged under REVIEW.md → "Every line you add must be demonstrably live … manually exercise the failure path of any checker whose purpose is to fail": the branch was added specifically as a fail-closed guard, but tracing the flag it gates on (
certfalsiness) through the only context that sets it shows it cannot fire.How to fix
Two options:
- Make the guard live. If the intent is to encode "no verifiable certificate on a resumed session → refuse to authorize" in this function rather than relying on
checkServerIdentity({})'s behavior, test for an empty cert instead of a falsy one — e.g.if (cert?.raw) { verifyError = checkServerIdentity(hostname, cert); } else if (self.isSessionReused()) { ... }. That way the branch actually executes forcert = {}. - Delete the branch. Since BoringSSL always provides the cert and the default
checkServerIdentityalready fails closed on{}, theelse ifand its comment can simply be removed, leaving justif (cert) verifyError = checkServerIdentity(hostname, cert);as it was in 4f0b269.
Either resolves the dead-code finding; the first also documents the policy where a future reader can trace it.
- Native handshake completes → native handle invokes the JS handshake callback →
|
The + // Unlike Node, don't gate on !isSessionReused(): BoringSSL keeps the peer
+ // chain on a resumed SSL_SESSION, so re-check it against this servername.
const { checkServerIdentity } = self[bunTLSConnectOptions];
- if (!verifyError && !self.isSessionReused() && typeof checkServerIdentity === "function") {
+ if (!verifyError && typeof checkServerIdentity === "function") {The analysis is right that |
There was a problem hiding this comment.
I didn't find any bugs, but this changes TLS server-identity verification semantics and deliberately diverges from Node — worth a human sign-off on the policy call (and note the flagged conflict with #32824 that would re-add the gate).
What was reviewed:
- The gate removal in
onClientHandshakeComplete— direction is fail-closed (stricter than before, restores 1.4.0 behavior), andcheckServerIdentityis only called when!verifyErrorand a peer cert is present, so no new failure modes on the resume path. - The new test — both resume arms (mismatched servername →
ERR_TLS_CERT_ALTNAME_INVALID; matching servername →authorized:true,reused:true) assert exact outcomes and the ticket is captured via the'session'event before'close'. - Checked that the test's
authorizationErrorstring form matches howonClientHandshakeCompleteassignsverifyError.code.
Extended reasoning...
Overview
One-line runtime change in src/js/node/net.ts: drops !self.isSessionReused() from the checkServerIdentity gate in onClientHandshakeComplete, plus a two-line comment explaining the deliberate divergence from Node. A new ~90-line test in test/js/node/tls/node-tls-connect.test.ts covers cross-servername resume rejection and same-servername resume acceptance.
Security risks
This is TLS peer-identity verification. The change moves in the fail-closed direction: on main a caller who passes a session ticket captured for hosta to a tls.connect({servername: 'hostb'}) sees authorized: true; after this PR that connection is rejected with ERR_TLS_CERT_ALTNAME_INVALID, matching 1.4.0. The only new observable behavior is that a user-supplied checkServerIdentity is now invoked on resumed sessions (Node does not call it there); it receives the same certificate that issued the ticket, so pinning hooks still pass. I don't see a way this weakens verification — it strictly tightens it.
Level of scrutiny
High. Even though the diff is tiny and the direction is fail-closed, this is (a) a security-critical TLS path, (b) a deliberate Node-compat divergence that has already flip-flopped across #33734 / #34598 / #32824, and (c) the author flagged that #32824 currently carries a hunk that would silently reintroduce the fail-open on merge. That combination is exactly what a human maintainer should sign off on rather than a bot.
Other factors
The test looks solid: it awaits the observable condition ('close' after the server ends, so the TLS 1.3 NewSessionTicket has arrived), asserts exact {reused, authorized, authorizationError, identityChecks, result} objects, and covers both the negative and positive resume paths. The PR body reports the relevant Node parallel tests (test-tls-client-resume*, test-https-agent-session-*, etc.) still pass. All comment-cop threads are resolved. No prior review from me on this PR.
9020a23 to
fccd782
Compare
There was a problem hiding this comment.
I didn't find any bugs — the one-line gate removal is sound and the test covers both resume paths. Deferring because this is a deliberate Node-compat divergence in the TLS certificate-verification path, and the author flagged a merge-order hazard with #32824 that would silently reintroduce the fail-open; a maintainer should sign off on both.
What was reviewed:
onClientHandshakeComplete: removing!isSessionReused()only strengthens the check (fail-closed);getPeerCertificate(true)returns{}when empty so theif (cert)arm still runscheckServerIdentityand fails closed.- New test: ticket captured via
'session'before'close', both resume outcomes assert exact{reused, authorized, authorizationError, identityChecks, result}; no sleeps, all failure paths resolve the promise. - Confirmed this restores 1.4.0 behavior and that the divergence from Node's
onConnectSecureis because BoringSSL keeps the peer chain on theSSL_SESSIONwhere OpenSSL does not.
Extended reasoning...
Overview
Two files touched. In src/js/node/net.ts, the onClientHandshakeComplete gate !verifyError && !self.isSessionReused() && typeof checkServerIdentity === "function" loses its middle term, so checkServerIdentity runs on resumed TLS sessions as well as full handshakes. A two-line comment records why the gate diverges from Node's onConnectSecure. In test/js/node/tls/node-tls-connect.test.ts, a new test captures a TLS 1.3 session ticket, resumes it under a servername the stored certificate does not cover (expects ERR_TLS_CERT_ALTNAME_INVALID), and resumes it under the original servername (expects authorized: true, isSessionReused() === true, and checkServerIdentity called with the stored cert).
Security risks
This is TLS peer-identity verification. The change is strictly a tightening: on main a ticket captured for servername A and offered for servername B currently produces authorized: true without re-checking the stored certificate against B; after this PR that path fails with ERR_TLS_CERT_ALTNAME_INVALID, matching Bun 1.4.0. The only new surface is that a user-supplied checkServerIdentity is now invoked on resumed connections (Node does not invoke it) — it sees the same certificate that issued the ticket, so certificate-pinning hooks continue to pass. I do not see a way for this to weaken verification; the concern is policy (a deliberate, documented Node divergence) rather than correctness.
Level of scrutiny
High. The runtime change is one condition removed, and the reasoning (BoringSSL retains the peer chain on SSL_SESSION, OpenSSL does not, so Node's skip-on-resume rationale does not apply to Bun) is well-argued and evidenced by the test asserting cert.subject.CN === "server-bun" on both resume paths. But TLS hostname verification is exactly the class of code the review guidelines say should not be auto-approved, and this PR encodes a deliberate behavioral divergence from Node that has already been flip-flopped across #33734 / #34598 / #32824. A maintainer should confirm the policy and the merge ordering with #32824 (which the author notes still carries the gate).
Other factors
The comment-cop bot fired three times on the inline comment; the author trimmed it to two lines and gave a reasonable justification (the comment prevents a parity-driven re-add that has already happened three times), and all threads are resolved. The test is hermetic (local tls.createServer, port: 0), wires every failure path to resolve, and asserts full outcome objects rather than single fields. The PR body lists the Node parallel resume/ticket tests that still pass. No bugs surfaced in the multi-agent review.
fccd782 to
ee4589e
Compare
There was a problem hiding this comment.
No bugs found, but this is a deliberate Node.js divergence in the TLS client certificate-verification path (checkServerIdentity now fires on resumed sessions where Node skips it), so a maintainer should sign off on the policy. Also note the flagged conflict with #32824, which currently re-adds the gate this PR removes.
What was reviewed:
onClientHandshakeCompleteinsrc/js/node/net.ts: removing!self.isSessionReused()only tightens the check (fail-closed); the surroundingauthorized/rejectUnauthorizedflow is unchanged.- The new test's failure-case
reused: falseis an artifact ofsecureConnectnever firing (destroy happens first), not a claim the ticket wasn't offered — the emptyidentityCheckson main proves the session actually resumes, so the test does exercise the gate. getPeerCertificate(true)returning{}(truthy) on a hypothetical no-cert resume still routes throughcheckServerIdentityand fails closed withERR_TLS_CERT_ALTNAME_INVALID.
Extended reasoning...
Overview
One-line runtime change in src/js/node/net.ts (onClientHandshakeComplete): drops !self.isSessionReused() from the checkServerIdentity gate, plus a two-line comment recording why Bun diverges from Node's onConnectSecure. A new ~90-line test in test/js/node/tls/node-tls-connect.test.ts covers cross-servername resume (must reject with ERR_TLS_CERT_ALTNAME_INVALID) and same-servername resume (must still authorize with isSessionReused() === true).
Security risks
This is TLS peer-identity verification — squarely security-sensitive. The change moves in the fail-closed direction: a resumed session is now re-checked against the current servername instead of inheriting the original handshake's authorized: true. That closes a real, if narrow, hole for callers who hand a captured session: to a tls.connect() for a different hostname. It also restores 1.4.0 behavior, so it is not introducing a novel restriction. I did not find a way for this change to weaken verification.
Level of scrutiny
High. The repo's own guidance calls out security-sensitive paths and deliberate Node-compat deviations as needing maintainer agreement. This PR is both: it changes when a user-supplied checkServerIdentity callback fires (now also on resumed sessions, unlike Node) and encodes a "Bun is deliberately stricter than Node" policy in the node:tls compat layer. The reasoning in the PR body is sound and the change is small, but the policy call — and the interaction with #32824, which the author flags as re-adding the gate — should be acknowledged by a human before merge.
Other factors
The bug-hunting pass found nothing. The test is hermetic (local tls.createServer, port: 0), wires error/close to resolve the awaited promise, and asserts exact outcome objects rather than loose containment. The author reports the wider Node parallel resume/ticket suite still passes. The comment-cop bot's objections are resolved and the remaining two-line comment is the kind of "deliberate deviation from upstream" note REVIEW.md says to keep.
|
Status at 670fc2c (rebased on main df573da, one commit). CI is green: build #114646, 181 of 181 jobs passed. Reproduction: run the new test against main's Node fixed the same hole as CVE-2026-48934 (nodejs/node@9cc4e32375, in v22.23.0, v24.17.0, v26.3.1). The PR body has the comparison and the note on Node's own regression tests. |
ee4589e to
8381647
Compare
There was a problem hiding this comment.
No bugs found, but this changes TLS certificate-verification semantics and deliberately diverges from Node, so a maintainer should sign off rather than me auto-approving.
What was reviewed:
- The gate removal in
onClientHandshakeComplete— strictly tightens verification (fail-closed), restores 1.4.0 behavior; theif (cert)path stays fail-closed becausegetPeerCertificate(true)returns{}(truthy) on an empty chain. - The new
it.each(["TLSv1.3","TLSv1.2"])test — hermetic (local server,port: 0), every failure path resolves the awaited promise, asserts both the cross-servername reject and the same-servername resume-still-authorizes case. - Checked that no other
isSessionReused()gate exists on the client identity path; the https Agent session cache is already keyed by servername.
Note the merge-order caution in the PR body: #32824 currently re-adds the gate this removes.
Extended reasoning...
Overview
One-line behavior change in src/js/node/net.ts: drops !self.isSessionReused() from the checkServerIdentity gate in onClientHandshakeComplete, plus a two-line comment recording why Bun diverges from Node here. A new it.each test in test/js/node/tls/node-tls-connect.test.ts covers TLS 1.2 and 1.3: capture a ticket, resume under a servername the cert does not cover (must fail with ERR_TLS_CERT_ALTNAME_INVALID), then resume under the original servername (must authorize with isSessionReused() === true).
Security risks
This is the TLS client hostname-verification path. The change is in the stricter direction: it re-enables checkServerIdentity on resumed sessions, closing a fail-open introduced by #34598 where a session ticket captured for one servername could be offered under a different servername and produce authorized: true. I did not find any way this weakens verification — the only observable side effect beyond the fix is that a user-supplied checkServerIdentity now runs on resumed connections (with the stored certificate), which is stricter than Node but consistent with the stated policy. The empty-cert edge case falls through to checkServerIdentity(hostname, {}), which the default implementation rejects, so no new fail-open is introduced there either.
Level of scrutiny
High. Per the repo review guidance, TLS/crypto verification changes and deliberate Node divergences should get maintainer eyes. Even though the code change is a single conjunct removal and the direction is fail-closed, this is a policy decision (Bun stays stricter than Node on resumed-session identity checks) that has already been flip-flopped across #33734 / #34598 / #32824 — a human should confirm the direction and be aware of the #32824 merge-order hazard the PR body calls out.
Other factors
The new test is well-constructed against the review checklist: local tls.createServer with port: 0, await using for the server, it.each over both TLS versions, error/close/data all wired to resolve the awaited promise, and assertions on the full {reused, authorized, authorizationError, identityChecks, result} object so both the negative and positive contracts are exact. CI (build #83537) shows the changed test file green on every lane; the two red jobs are pre-existing/external-network flakes unrelated to this diff. My earlier indentation nit was fixed by autofix.ci in 4b1743b. The bug-hunting system found nothing this run.
A resumed handshake sends no certificate. BoringSSL keeps the peer chain
on the SSL_SESSION, so getPeerCertificate() returns the original
certificate on a resumed connection. Run checkServerIdentity on it
instead of skipping the check when isSessionReused() is true.
With the gate, tls.connect({servername: "b", session: <ticket for "a">})
reports authorized:true although the stored certificate does not cover
"b". Any peer that can decrypt the ticket is then authorized on the
ticket alone. Node fixed the same hole as CVE-2026-48934
(nodejs/node@9cc4e32375) by not offering a session that was
authenticated for another servername.
4b1743b to
670fc2c
Compare
There was a problem hiding this comment.
I re-reviewed after the latest push and didn't find any bugs (the earlier indentation nit is fixed). Because this removes a gate on the TLS client authorization path — a security-sensitive change with a deliberate divergence from Node's fix for the same CVE — a human sign-off is still warranted.
What was reviewed:
onClientHandshakein src/js/node/net.ts: dropping!self.isSessionReused()fails closed — on resumegetPeerCertificate(true)returns the stored chain (or{}, whichcheckServerIdentityrejects), andcheckServerIdentityis read from the symbol-keyedbunTLSConnectOptionsslot, not a user-tamperable property.- The Node-divergence comment cites the CVE and the reason (BoringSSL keeps the peer chain), satisfying the compat-divergence rule.
- New test: local server on port 0,
await using,it.eachover TLSv1.2/1.3, error events wired to resolve, exactERR_TLS_CERT_ALTNAME_INVALIDasserted via a single.toEqual({...});authorizationError: nullmatches the TLSSocket initializer in tls.ts.
Extended reasoning...
Overview
The PR removes the !self.isSessionReused() term from the checkServerIdentity gate in onClientHandshake (src/js/node/net.ts:363), so a resumed TLS session is re-checked against the current servername using the peer certificate BoringSSL keeps on the SSL_SESSION. A three-line comment documents the deliberate divergence from Node's approach and names CVE-2026-48934. The accompanying test in test/js/node/tls/node-tls-connect.test.ts spins up a local tls.createServer, captures a ticket for servername: "localhost", then asserts that resuming under not-in-cert.example fails with ERR_TLS_CERT_ALTNAME_INVALID while resuming under localhost still authorizes with isSessionReused() === true, parameterized over TLSv1.2 and TLSv1.3.
Security risks
This is a security fix in the TLS client authorization path. The direction of the change is strictly tightening: a check that was skipped on resumed sessions now always runs, so the failure mode of a mistake here would be over-rejection rather than a new bypass. I confirmed checkServerIdentity is read from the symbol-keyed internal bunTLSConnectOptions slot (not a prototype-pollutable public property), and that the empty-cert-on-resume case still fails closed because getPeerCertificate(true) returns {} (truthy) and checkServerIdentity(host, {}) returns ERR_TLS_CERT_ALTNAME_INVALID. The divergence from Node (Bun re-checks the stored cert; Node refuses to offer a cross-servername ticket) is documented inline per the Node/Web-compat convention. The PR body also flags a merge-order hazard (#32824 re-adds the gate) that a human should keep an eye on.
Level of scrutiny
High. REVIEW.md's Security section is explicit: "Never remove a flag you don't understand in a TLS/crypto path" and "Security checks fail closed and cover every path to the protected effect." Even though the diff is a one-term deletion plus a test, it changes what authorized: true means for every caller that passes session: to tls.connect, and the PR itself notes it does not yet match Node's exact error code or session-binding behavior. That's the kind of deliberate compat trade-off a maintainer should ratify.
Other factors
The test follows repo conventions closely: port: 0, await using for the server, it.each for the protocol-version matrix, failure events wired into the awaited promise (no sleeps), a single .toEqual({...}) per case with the exact error code, and no external network. I verified authorizationError: null in the happy-path assertion matches the this.authorizationError = null initializer in src/js/node/tls.ts. My only prior finding on this PR (a 6-space indentation on the first.on("session", ...) line) is fixed in the current push. No third-party CHANGES_REQUESTED reviews are outstanding; the resolved inline threads are all bot comments self-resolved by the author.
… own checkServerIdentity (#42946) ### Problem - An `https.Agent` lets a request use a connection that a different identity check approved. Request 1 passes a permissive `checkServerIdentity`, or none. Request 2 passes a rejecting one and gets `200`. Its callback never runs. - The cause is `Agent#getName()` (`src/js/node/https.ts:371`). The name has no `checkServerIdentity` part. It keys the keep-alive pool, `_sessionCache`, and the request queue. (#36131 since closed the resumed-session part.) - Node fixed this as CVE-2026-58040 (nodejs/node@52a8ace880). ### Fix - Port of 52a8ace880. A request with its own `checkServerIdentity` gets a unique Agent name and uses no cached session (nor Bun's `establishTunnel` cache). An Agent-level callback still pools. - Stricter than Node twice. `ClientRequest` marks the request, so `http.request({ protocol: "https:", agent })` is covered. The Agent's `'free'` handler refuses the socket, so agent-base style Agents are covered. - The unique name leaves the Agent's bookkeeping intact: no stale `agent.sockets` entry, no dead queue head, no wait behind an idle socket under `maxTotalSockets` (Notes). - Verified: `test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts`. Main b8eacea fails 20 of 30. Node v26.5.1 passes the 21 it runs. ### Background - `https.Agent` pools sockets in `freeSockets[name]`. `getName(options)` builds `name` from host, port and TLS options. - `_sessionCache` holds one TLS session per name. A new socket offers it to skip the full handshake. - `checkServerIdentity(hostname, cert)` is the certificate pinning hook. It runs once per full handshake. - Cost, as in Node: a full handshake per such request, and `maxSockets` does not bound them. A callback on the Agent keeps reuse. <details><summary>Notes</summary> **Repro.** One Agent, vendored `agent1` certificate. Request 2 carries `checkServerIdentity: () => new Error("PIN MISMATCH")`. ``` bun canary 09bb546 this branch node v26.3.0 node v26.5.1 request 1 default, keepAlive 200 on conn 1, 0 calls ERR PIN MISMATCH 200 ERR PIN MISMATCH request 1 default, no keepAlive 200, session resumed ERR PIN MISMATCH 200 ERR PIN MISMATCH request 1 permissive, keepAlive 200 on conn 1 ERR PIN MISMATCH 200 ERR PIN MISMATCH request 1 permissive, no keepAlive 200, session resumed ERR PIN MISMATCH 200 ERR PIN MISMATCH ``` The reverse direction and the queue have the same hole on stock bun. A default-check request for `servername: "not-agent1"` gets `200` after a permissive callback approved that name. With `maxSockets: 1`, `Agent#removeSocket` makes the socket of a queued request from the options of the socket that closed, so the queued request runs the callback of the request ahead of it. **The test file under each runtime.** | runtime | pass | fail | skipped | |---|---|---|---| | this branch (debug build) | 30 | 0 | 0 | | main b8eacea (has #36131), without this PR | 10 | 20 | 0 | | bun canary c6b7fcb (before #36131) | 6 | 24 | 0 | | node v26.5.1 (has 52a8ace880) | 21 | 0 | 8 (Bun-only) | | node v26.3.0 (does not) | 5 | 0 | 24 | The cases that pass everywhere are controls: an Agent-level callback and a request that passes `tls.checkServerIdentity` itself still share the socket or the session, and `agent.sockets` has its entry while a proxy tunnel connects. The other cases are gated on the Node versions that carry the fix (v22.23.2, v24.18.1, v26.5.1). Bun always runs them. **Difference 1: where the socket is refused.** Node marks the socket in `https.Agent`'s `createConnection` and refuses it in `https.Agent#keepSocketAlive`. An `https.Agent` subclass that replaces `createConnection`, and an `http.Agent` that borrows `https.Agent#getName` (agent-base: https-proxy-agent, socks-proxy-agent), get the unique name without the mark. Three requests with their own callback on a `keepAlive` Agent: ``` parked sockets after stock straight port node v26.5.1 this branch https.Agent 1 0 0 0 http.Agent that borrows https getName 1 3 3 0 https.Agent that replaces createConnection 1 3 3 0 ``` A parked socket under a unique name is never taken. It counts against `maxTotalSockets` until the origin closes it. The request options are what make the name unique, and `installListeners()` already hands them to the `'free'` handler, so the handler reads the mark there. The socket mark and the `keepSocketAlive` override of the upstream patch are then not needed. **Difference 2: where the mark is set.** Node sets it in `https.request()`. `http.request({ protocol: "https:", agent })` and `new http.ClientRequest()` reach the same Agent without it, and still share on Node v26.5.1. Here the `ClientRequest` constructor sets it, after it resolves the agent, so no route skips it. **Difference 3: what a failed socket creation leaves behind.** `Agent#addRequest` makes `this.sockets[name] = []` before a socket exists, in Node and here. If no socket arrives (a refused proxy tunnel, a `createConnection` that throws) nothing in Node removes the entry. With one name per Agent that was one stale entry. With one name per request it is one per failed request, and each key holds the PEM text of `ca`, `cert` and `key`. Five refused tunnels: stock 1 entry, straight port 5, node v26.5.1 5, this branch 0. The branch removes the empty entry in the error path of both creation callbacks and around a `createSocket()` that throws. A queued request whose socket could not be made also leaves its queue. In Node it stays at the head until a socket of its name frees. No socket ever has the name of such a request, and `removeSocket()` only looks at the first queue, so every queue behind it stopped for good. The same path now gives a `createConnection()` that throws to the queued request. Before, and in Node, that is an uncaught exception from a `'close'` handler. **Difference 4: an idle socket gives up its slot.** `new https.Agent({ keepAlive: true, maxTotalSockets: 1 })`, one finished request, then a request with its own callback. The pooled socket holds the only slot and the new request cannot take it. Node v26.5.1 waits until the server closes the idle socket. Stock bun reused the socket at once. Here `addRequest()` destroys one idle socket when such a request has to queue. No other request does this, so the rest of the scheduling is Node's. **The tunnel path.** `establishTunnel()` in Bun attaches the session cache listeners to the tunneled socket. Upstream caches no session there. Through an `https:` proxy the cached session is resumed, and on stock bun request 2 gets `200` without its callback. Through an `http:` proxy the session is cached and not resumed today (a bug in `tls.connect({ socket, session })` that is tracked apart from this PR). The branch caches no session for a marked request on either path. **Costs in full.** - `maxSockets` is a limit per name, so it does not bound these requests: `maxSockets: 1` and 20 concurrent requests open 20 connections here and on node v26.5.1, 1 on stock. `maxTotalSockets` still bounds them, to N+1 in one case that Node's Agent has for every pair of names: the `'free'` handler pools a socket at the limit, and `removeSocket()` then makes a socket for a queued request of another name. - No Bun user reported this hole. The report that exists, #40308, asked for more reuse with a callback on `fetch`, and #42692 (item B4) later gave that reuse up for the same reason as this PR. **Why Node's own regression test is not added.** `test-https-agent-checkserveridentity-reuse.js` ends with `second.socket.isSessionReused()` one promise hop after the response `'end'` of a `Connection: close` request. Bun has destroyed that socket by then (`Socket.prototype._final` ends in a `nextTick`, Node waits for the shutdown callback), so the call reads `false`. The session is resumed: the same call at `'response'` reads `true`. Files under `test/js/node/test/parallel/` are not edited, so the file stays out. The new test covers each of its cases, and it runs under Node. **Other open PRs.** - #36131 merged while this PR was open. It runs the identity check on resumed sessions too, so on main the four plain `keepAlive: false` cases of the new test already pass. The pooled socket, the queue, both proxy paths and the bookkeeping cases still fail there (20 of 30). The session rule of this PR stays: it keeps one cache entry per request out of the 100-entry `_sessionCache`, and it matches the upstream patch. The new test passes 30 of 30 on this branch rebased over #36131. - #42498 appends to the same tail of `getName()`. The second one to land has a small text conflict. Both suffixes must stay, the per-request one last. **Suites.** 182 files of `test/js/node/test/{parallel,sequential}/test-http*` that mention agents, sockets or keep-alive, `test-tls-check-server-identity.js`, `test-tls-client-resume*.js`, all of `node-http.test.ts` (159 pass), `node-https-checkServerIdentity.test.ts`, `node-http-agent-free-socket.test.ts`, `node-http-agent-tls-options.test.mts`, `node-http-proxy-url.test.ts`. Two vendored files fail on the debug build with and without this change, and pass on the release canary: `test-https-timeout.js` (a 10 ms request timeout never fires) and `test-http-agent-keepalive.js` (an assertion behind a 1 ms timer). **Review history.** A review of the first version (the straight port) asked for four changes: no stale `agent.sockets` entries, a test through an `https:` proxy, the full cost statement, and its execution findings (the parked sockets above, the queue cases). The review on the PR then found the `http.request()` route, the wait behind an idle socket, and the dead queue head, and asked to keep Node's `agent.sockets` entry. All are in this version. The `maxSockets` cost and the N+1 case are not changed. It also named two unrelated Node security fixes that Bun lacks (CVE-2026-48615, CVE-2026-48618). They are tracked apart from this PR. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 24 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts bun test v1.4.3 (c6b7fcb) test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts: 195 | const calls: string[] = []; 196 | const first = await exchange({ checkServerIdentity: () => void calls.push("permissive") }); 197 | const second = await exchange({ 198 | checkServerIdentity: () => (calls.push("rejecting"), new Error("PIN MISMATCH")), 199 | }); 200 | assert.deepStrictEqual( ^ AssertionError: Expected values to be strictly deep-equal: + actual - expected { calls: [ 'permissive', - 'rejecting' ], first: { reusedSocket: false, sessionReused: false, status: 200 }, second: { + reusedSocket: true, + sessionReused: false, + status: 200 - error: 'PIN MISMATCH' } } generatedMessage: true, actual: { first: [Object ...], second: [Object ...], calls: [ "permissive" ], }, expected: { first: [Object ...], ... (truncated) release without fix: 24 FAILED bun test v1.4.3-canary.1 (c6b7fcb) test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts: 195 | const calls: string[] = []; 196 | const first = await exchange({ checkServerIdentity: () => void calls.push("permissive") }); 197 | const second = await exchange({ 198 | checkServerIdentity: () => (calls.push("rejecting"), new Error("PIN MISMATCH")), 199 | }); 200 | assert.deepStrictEqual( ^ AssertionError: Expected values to be strictly deep-equal: + actual - expected { calls: [ 'permissive', - 'rejecting' ], first: { reusedSocket: false, sessionReused: false, status: 200 }, second: { + reusedSocket: true, + sessionReused: false, + status: 200 - error: 'PIN MISMATCH' } } generatedMessage: true, actual: { first: [Object ...], second: [Object ...], calls: [ "permissive" ], }, expected: { first: [Object ...], second: [Object ...], calls: [ "permissive", "rejecting" ], }, operator: "deepStrictEqual", diff: "simple", code: "ERR_ASSERTION" at /workspace/bun/test/js/node/ht ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts bun test v1.4.3 (c6b7fcb) test/js/node/http/node-https-agent-checkserveridentity-reuse.test.ts: (pass) https.Agent({ keepAlive: true }) and a request's own checkServerIdentity > a rejecting callback does not ride the pooled socket of a permissive callback [1208.38ms] (pass) https.Agent({ keepAlive: true }) and a request's own checkServerIdentity > a rejecting callback does not ride the pooled socket of the default check [306.99ms] (pass) https.Agent({ keepAlive: true }) and a request's own checkServerIdentity > the default check does not ride the pooled socket of a permissive callback [285.12ms] (pass) https.Agent({ keepAlive: true }) and a request's own checkServerIdentity > http.request({ protocol: "https:", agent }) gets the same rule for the pooled socket [349.83ms] (pass) https.Agent({ keepAlive: true }) and a request's own checkServerIdentity > every request with its own callback gets its own connection, parked nowhere [257.69ms] (pass) https.Agent({ keepAlive ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision 6481f6a features lto, baseline 23 deps, 131 codegen, 1176 objects in 713ms ninja: Entering directory `/workspace/bun/build/release' [1/1250] install /workspace/bun bun install v1.4.3-canary.1 (c6b7fcb) Checked 22 installs across 61 packages (no changes) [11.00ms] [2/1250] gen ErrorCode+*.h [3/1250] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (c6b7fcb) Checked 1 install across 2 packages (no changes) [3.00ms] [4/1250] gen bindgenv2 [5/1250] fetch libjpeg-turbo [libjpeg-turbo] up to date [6/1223] install /workspace/bun/src/node-fallbacks bun install v1.4.3-canary.1 (c6b7fcb) Checked 111 installs across 104 packages (no changes) [7.00ms] [7/1223] fetch zlib [zlib] up to date [8/1223] gen node-fallbacks/react-refresh.js Bundled 1 module in 10ms react-refresh.js 4.81 KB (entry point) [9/1223] esbuild bun-error ../../build/release/codegen/bun-error/index.js 34.9kb ../../build/release/codegen/bun-error/bun-error.css 12.8kb ⚡ Do ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/http.ts | 2 + src/js/node/_http_agent.ts | 63 ++- src/js/node/_http_client.ts | 20 +- src/js/node/https.ts | 17 +- ...e-https-agent-checkserveridentity-reuse.test.ts | 532 +++++++++++++++++++++ 5 files changed, 623 insertions(+), 11 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/http.ts 0 0 40 src/js/node/_http_agent.ts 2 0 42 src/js/node/_http_client.ts 0 0 38 src/js/node/https.ts 7 8 44 …http/node-https-agent-checkserveridentity-reuse.test.ts 1 4 37 ``` </details> <!-- robobun:evidence:end -->
2 participants
Problem
tls.connect({ servername: "hostb.test", session })with asessioncaptured forhosta.testresumes and reportsauthorized: true.checkServerIdentitynever runs, and the certificate covers onlyhosta.test.!self.isSessionReused()term in the gate ofonClientHandshake(src/js/node/net.ts:360), copied from Node in tls: close_notify on end(), injected-socket upgrades, reject-handshake wire fix, duplex data-loss, SNI, ALPN (+14 tests, tls 81%→86%) #34598. Tagsbun-v1.4.0tobun-v1.4.2contain it.Fix
checkServerIdentitynow runs on a resumed session, against the certificate Bun keeps from the original handshake.authorizedthen means the same on both handshake kinds: the certificate covers thisservername. A resume under the original name still authorizes.test/js/node/tls/node-tls-connect.test.ts(newit.eachover TLSv1.3 and TLSv1.2, fails on main, passes here). Also the vendored Node resume, ticket and agent-session tests.Background
checkServerIdentity(hostname, cert)is thenode:tlshostname check against the certificate SAN and CN.SSL_SESSION, sogetPeerCertificate()in Bun returns the original certificate.node:httpsAgent cache is keyed by servername andfetch()never resumes. Only a caller that passessession:across hostnames is affected.Notes
Outcome table (server certificate covers only
hosta.test, client connects withservername: "hostb.test"and thehosta.testsession):authorizedtruefalsetruefalseERR_TLS_CERT_ALTNAME_INVALIDcheckServerIdentityNode's own regression tests.
test-tls-session-reuse-different-servername.jsandtest-https-session-reuse-different-servername.jsdo not pass verbatim with this change. They assertUNABLE_TO_VERIFY_LEAF_SIGNATURE, the error of the full handshake that Node falls back to. Bun resumes, re-checks the kept certificate, and rejects withERR_TLS_CERT_ALTNAME_INVALID. To match the error code, Bun must also bind the session to the servername. Node does this with a prefix on thegetSession()/'session'buffer, which changes the format of that buffer. BoringSSL has noSSL_SESSION_get0_hostname, so there is no cheaper source for the name. That is a separate decision and is not part of this PR.Empty certificate on resume. No extra branch is needed.
getPeerCertificate(true)returns{}(truthy) when the native side has no certificate.checkServerIdentity(hostname, {})then returnsERR_TLS_CERT_ALTNAME_INVALID("Cert does not contain a DNS name").User
checkServerIdentityhooks now also run on resumed connections. They see the certificate that issued the ticket, so a pinning hook still passes.Two-server variant (a second server with an untrusted certificate that shares the ticket keys). Bun cannot run it yet: the server
ticketKeysoption is validated but not applied (#28691). On a resume the client keeps the certificate of the original server, whichever peer accepts the ticket. The hostname check in this PR covers that variant.Merge order. #32824 carries a hunk that adds the gate again. #33734 proposed the gate and is closed.
Suites run on the debug build:
node-tls-connect.test.ts(57 pass, 0 fail),test-tls-client-resume.js,test-tls-client-resume-12.js,test-https-client-resume.js,test-https-client-checkServerIdentity.js,test-tls-check-server-identity.js,test-tls-ticket.js,test-https-agent-session-reuse.js,test-https-agent-disable-session-reuse.js,test-https-agent-session-eviction.js,test-https-agent-session-injection.js,test/regression/issue/25190.test.ts.[human-review] gate passed · iteration 7 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 2 rejected · iteration 7
evidence per changed file