https: handle invalid TLS options in proxied requests by christianaurichzm · Pull Request #66096 · nodejs/node

christianaurichzm · 2026-09-17T22:40:56Z

Invalid TLS options can cause an uncaught exception when an HTTPS request uses a proxy.

After the proxy responds to CONNECT, tls.connect() is called from the socket read handler. If TLS option validation throws at that point, the exception is outside the original https.request() call.

Catch the error, close the tunnel socket, and propagate it to the request's 'error' handler.

The new test covers invalid minVersion, ciphers, and secureProtocol options.

tls.connect() can throw while validating TLS options. For proxied HTTPS requests, it is called after the CONNECT response has been received, so the throw happens asynchronously from the original https.request() call and ends the process as an uncaught exception. Catch the error, close the tunnel socket, and propagate it to the request. Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com>

nodejs-github-bot · 2026-09-17T22:41:02Z

Review requested:

@nodejs/crypto
@nodejs/http
@nodejs/net

codecov · 2026-09-18T00:18:40Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (67e66b8) to head (663dfa8).
⚠️ Report is 63 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #66096    +/-   ##
========================================
  Coverage   90.27%   90.27%            
========================================
  Files         789      790     +1     
  Lines      271473   271600   +127     
  Branches    51808    51837    +29     
========================================
+ Hits       245066   245187   +121     
- Misses      16880    16922    +42     
+ Partials     9527     9491    -36

Files with missing lines	Coverage Δ
lib/https.js	`97.21% <100.00%> (+0.03%)`	⬆️

... and 69 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

nodejs-github-bot · 2026-09-18T03:25:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/77586/

nodejs-github-bot · 2026-09-20T19:14:26Z

Landed in ada8c5c

tls.connect() can throw while validating TLS options. For proxied HTTPS requests, it is called after the CONNECT response has been received, so the throw happens asynchronously from the original https.request() call and ends the process as an uncaught exception. Catch the error, close the tunnel socket, and propagate it to the request. Signed-off-by: Christian Aurich <christian.aurichzm@gmail.com> PR-URL: #66096 Reviewed-By: Xuguang Mei <meixuguang@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Robert Nagy <ronagy@icloud.com>

…44191) ### Problem - An `https` request through a proxy emits only `'close'` when the connection ends between the `200` of the CONNECT and the end of the TLS handshake. Node emits `'error'` first: `ECONNRESET`, `Client network socket disconnected before secure TLS connection was established`. - `Agent#createSocket` gives the request the failed socket with the error (`src/js/node/_http_agent.ts:354`, `:530`). The TLS layer set `_hadError` on it, so the guard at `src/js/node/_http_client.ts:1076` skips the `'error'`. ### Fix - `Agent#createSocket` passes only the error, as Node does. `https.Agent#createConnection` closes the proxy connection after every `ERR_PROXY_TUNNEL`, which the request did before. `_http_client.ts` does not change. - Correct: a request gets no socket that it did not listen to. 13 of 13 failure routes equal Node v26.3.0 in events, code and tick (base: 0). - Verified: `test/js/node/http/node-http-proxy-url.test.ts`: 23 tests fail without the fix, 22 also under Node. Also 100 vendored Node tests. - Self-reviewed: 8 concerns raised, 8 addressed. ### Background - The forwarding is from #31587 ([thread](#31587 (comment))), to close a connection that a proxy holds after a refused CONNECT. That thread set aside the close in `createConnection`, which this PR takes. - Considered a guard in the request: one tick after Node, and a custom `createConnection` callback `(err, x)` stays broken. ### Downsides - Not as Node, on purpose: after a refused CONNECT, a direct `https.Agent#createConnection` caller gets a destroyed socket. Node leaves it open. - As in Node: `agent.createSocket()` calls back with `(err)` only. A request with no `'error'` listener throws this `ECONNRESET`. - Not covered: `https-proxy-agent`, `tunnel` and a direct `https` request still report `ERR_SOCKET_CLOSED` (#43381, item 4). <details><summary>Notes</summary> #### Reproduction ```js // bun repro.mjs / node repro.mjs import net from "node:net"; import https from "node:https"; const proxy = net.createServer(s => { s.on("error", () => {}); s.once("data", () => { s.write("HTTP/1.1 200 Connection established\r\n\r\n"); s.once("data", () => s.end()); // the ClientHello }); }); await new Promise(r => proxy.listen(0, "127.0.0.1", r)); const agent = new https.Agent({ proxyEnv: { https_proxy: `http://127.0.0.1:${proxy.address().port}` } }); const req = https.get({ host: "example.invalid", port: 443, path: "/", agent }); req.on("error", e => console.log("error:", e.code, e.message)); req.on("close", () => { console.log("close"); proxy.close(); }); ``` ``` node v26.3.0, and bun with this PR: error: ECONNRESET Client network socket disconnected before secure TLS connection was established close bun 1.4.0, 1.4.1, 1.4.2, main 1313ca6 (3 of 3 runs each, linux-x64): close ``` The proxy ends its side with `end()` after it read the ClientHello. A `destroy()` with unread bytes is a reset, which takes another path through the client and was reported before. The proxy does not have to be at fault: a proxy that relays the close of a target that hangs up during the handshake gives the same result. The report also says that the process never exits. No handle is left open: both sockets are closed and `process.getActiveResourcesInfo()` is empty. The awaited promise never settles, and Bun does not exit on an unsettled top-level await (#33283). #### Who hits it - The built-in proxy support of `node:https`: `new https.Agent({ proxyEnv })`, the global agent under `NODE_USE_ENV_PROXY=1`, and `http.setGlobalProxyFromEnv()`. - An `http://` proxy connection that ends with the `200` or after it, and an `https://` proxy connection that ends during its own TLS handshake or after its `200`. - A request that waited behind `maxSockets`. - No proxy: a custom `Agent#createConnection` that calls back with `(err, socket)`. With a second argument that has no `destroy()` the process ended with `TypeError: socket.destroy is not a function`. The request destroyed a live socket that it was given this way. Node leaves the second argument alone. Not this bug: `https-proxy-agent`, `tunnel` and a direct `https` request, where the request writes to the TLS socket before the handshake is done. They report `ERR_SOCKET_CLOSED` where Node reports `ECONNRESET`, before and after this PR. That is item 4 of #43381, and #43392 is the open PR for it. #### The earlier decision in #31587 The review thread of #31587 found that a proxy which refuses the CONNECT and holds the connection left that connection open, as in Node. It named two ways: forward the socket from the Agent to the request (taken, d677423), or close the connection in `cleanupAndPropagate` for every error (set aside as further from upstream). The forwarding applies to every failed socket creation, not only to a refused CONNECT. So the request also gets the sockets that the TLS layer already marked, and loses its `'error'`. With this PR the Agent is Node's text again, and the one line that differs from Node is the condition in `cleanupAndPropagate`. The test that #31587 added for the held connection passes with no change to its assertions. Node has the same guard since nodejs/node#61770. With an agent whose `createSocket` calls back with `(err, socket)` and a socket that the peer ended during the TLS handshake, Node v26.3.0 also emits only `'close'`. So the forwarding cannot go upstream as it is. Node v26.3.0 leaves the held connection open. #### Measurements Release builds of 1313ca6 and of the three source expressions of this PR on it, linux-x64, and Node v26.3.0. `_http_agent.ts`, `https.ts` and `_http_client.ts` are the same at 1313ca6 and at the base of this branch. Every count is the same in 5 of 5 runs. - builtin JS text: `_http_agent.js` -13 B, `https.js` -33 B, `_http_client.js` 0 B (`wc -c`, `cmp`: identical). bun binary text+data -46 B (`size`: text 80662588 to 80662542, data 110424). - success path: 0 changed bytecode instructions. `onSocketReady` 47, `onSocketCreatedForPending` 45, `cleanupAndPropagateImpl` 36 instructions (base 48/47/38, JSC bytecode dump). - successful request, `nextTick` entries / listener registrations on the socket / `bind` calls, from the moment the Agent has the socket to the `'socket'` event: http 2/9/1, keep-alive 1/5/1, https 2/10/1, tunnel 2/9/0. Base and PR are identical. `createSocket` reported an error 0 of 800 times. - failed creation, `nextTick` entries / listener registrations on the failed socket, from the failure callback to the `'close'` of the request: FIN 1/0 (base 2/1, node 1/0), held 407 1/0 (base 3/1, node 1/1). - 13 of 13 failure routes equal Node v26.3.0 in events, error code and `nextTick` turn (base 0 of 13). FIN routes: `'error'` and `'close'` at turn 1/1 (node 1/1, base no `'error'`, `'close'` at turn 2). The message equals Node's on 11 of 13. The other 2 are texts of the TLS library, the same before and after. - held non-200 CONNECT: the proxy connection is closed for 7 of 7 callers (node 0, base 2) and for 5 of 5 status lines without a code. A direct `createConnection` caller sees `socket.destroyed === true` in its callback (node `false`, base `false`). - custom `createConnection` `cb(err, x)`: 16 of 16 rows equal Node (base 0 of 16), `destroy()` is called on `x` 0 times (node 0, base 8), 0 rows end in an uncaught exception (base 10). The callback of `agent.createSocket()` gets 1 argument after a failure (node 1, base 2). The 13 routes: FIN after the ClientHello, `200` with the FIN, `https://` proxy FIN during its handshake, `https://` proxy `200` then FIN, held 407, 500, RST after the `200`, bytes that are not TLS after the `200`, certificate error, end before the `200`, tunnel timeout, request queued behind `maxSockets`, global agent with `NODE_USE_ENV_PROXY=1`. The 7 callers: a request through the agent, a request queued behind `maxSockets`, `agent.createSocket()`, a subclass that passes on only the error, the `createConnection` option of a request, `agent.createConnection()` with a callback and without one. #### Behavior that changes besides the fix - `'close'` of a request comes one `nextTick` turn earlier after every failed socket creation, on the turn where Node emits it. - The request does not call `destroy()` on what a custom `createConnection` passes with an error. - A subclass that overrides `createSocket` and calls back with `(err, socket)` for a request that waited behind `maxSockets`: the request emits `'error'`, then `'close'`, and does not call `destroy()` on that socket. Before, it emitted only `'close'` when the socket had `_hadError`, and destroyed the socket. The subclass owns that socket now, as in Node. - After a CONNECT reply with a status line that has no usable code (for example `HTTP/1.1 abc`), a direct caller of `createConnection` gets a destroyed socket, as in Node. #### Left as it is - `req.onSocket(socket, err)` called directly with a socket that has `_hadError`, and a subclass that overrides `createSocket` and calls back with `(err, socket)` for a request that did not wait: only `'close'`, also in Node. - The `createConnection` option of a request that reports an error through its callback: `'error'` and no `'close'`, also in Node. - A tunnel that is still pending: while the proxy does not answer the CONNECT, `req.destroy(err)`, `req.abort()`, an aborted signal and `agent.destroy()` emit no `'error'` and no `'close'`, and the proxy connection stays open. When the proxy closes, the request reports `ERR_PROXY_TUNNEL`. Node v26.3.0 does the same. - Two changes of Node's tunnel code after v26.3.0 are not ported here: the limit on the headers of the CONNECT reply (nodejs/node 84e367579e) and the handling of TLS options that `tls.connect()` rejects (nodejs/node#66096). Both are separate work. - Error messages of the TLS library differ from Node's (`WRONG_VERSION_NUMBER`, `self signed certificate`). - `agent.sockets[name]` is deleted after a failed creation. Node keeps an empty array. #### Tests - `test/js/node/http/node-http-proxy-url.node.mts` runs under Node and under Bun. 22 new tests: 4 ways a proxy connection ends, a destroyed and an aborted request, `new http.ClientRequest`, `http.setGlobalProxyFromEnv()`, `diagnostics_channel`, a queued request, 5 tests of a refused CONNECT, 7 of a creation callback with an error and a second argument. 6 of them also assert the `nextTick` turn of `'error'` and `'close'`. - `test/js/node/http/node-http-proxy-url.test.ts`: the global agent with `NODE_USE_ENV_PROXY=1`, in a child process. - linux-x64, debug build. With the fix: 32 of 32 in the dual file (Node v26.3.0: 32 of 32) and 4 of 4 in the wrapper. Without the fix: the 22 and the 1 fail. - Each source edit alone, reverted on a debug build: `_http_agent.ts:350` 17 tests fail, `:524` 1 test fails, `https.ts` 5 tests fail. With `req.onSocket` one `nextTick` late, the 6 turn assertions fail. - CI build 121492 at 6c55f8a ran `node-http-proxy-url.test.ts` and `node-http.test.ts` on every lane, and both pass at the first attempt: Linux glibc and musl, macOS and Windows, each on x64 and arm64, and the ASAN lane. The Node run reports 32 of 32 on each lane. - Windows x64, without the fix: the canary a7c73fd prints only `close` for the reproduction. - Unchanged and passing on linux-x64: `node-http.test.ts` (263 pass), `node-https-agent-checkserveridentity-reuse.test.ts` (30), `node-http-agent-free-socket.test.ts` (9), `node-http-client-request-gc.test.ts`, `trace-events.test.ts`, the two Agent tests of `module-graph-io.test.ts`. - Vendored Node tests: 100 of 101 pass (`test-https-proxy-request-*`, `test-http-proxy-request-*`, `test-http-agent*`, `test-https-agent*`, `test-http-client-abort*`, `test-http-set-global-proxy-from-env-*`, `test-tls-over-http-tunnel`, and more). `test-http-agent-keepalive.js` fails on a debug build with and without this change. </details>  --- **no test proof** · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/http/node-http.test.ts

nodejs-github-bot added https Issues and PRs related to the https subsystem. needs-ci PRs that need a full CI run. labels Sep 17, 2026

meixg approved these changes Sep 18, 2026

View reviewed changes

meixg added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2026

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Sep 18, 2026

inoway46 added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Sep 18, 2026

nodejs-github-bot added the lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. label Sep 19, 2026

jasnell approved these changes Sep 20, 2026

View reviewed changes

ronag approved these changes Sep 20, 2026

View reviewed changes

bjohansebas approved these changes Sep 20, 2026

View reviewed changes

nodejs-github-bot merged commit ada8c5c into nodejs:main Sep 20, 2026
91 checks passed

nodejs-github-bot removed commit-queue PRs queued for automated landing through the Commit Queue. lacks-second-approval Commit Queue PRs awaiting a second collaborator approval or completion of the required wait. labels Sep 20, 2026

robobun mentioned this pull request Sep 29, 2026

node:http: do not give a request the socket of a failed connection oven-sh/bun#44191

Merged

Navigation Menu

Uh oh!

https: handle invalid TLS options in proxied requests - #66096

https: handle invalid TLS options in proxied requests#66096
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
christianaurichzm:https-proxy-tls-connect-throw

christianaurichzm commented Sep 17, 2026

Uh oh!

nodejs-github-bot commented Sep 17, 2026

Uh oh!

codecov Bot commented Sep 18, 2026 •

edited

Loading

Uh oh!

nodejs-github-bot commented Sep 18, 2026

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Conversation

christianaurichzm commented Sep 17, 2026

Uh oh!

nodejs-github-bot commented Sep 17, 2026

Uh oh!

codecov Bot commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Sep 18, 2026

Uh oh!

Uh oh!

nodejs-github-bot commented Sep 20, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov Bot commented Sep 18, 2026 •

edited

Loading