Skip to content

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

Open
christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:https-proxy-tls-connect-throw
Open

christianaurichzm wants to merge 1 commit into
nodejs:mainfrom
christianaurichzm:https-proxy-tls-connect-throw

Conversation

@christianaurichzm

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot 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
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (67e66b8) to head (663dfa8).
⚠️ Report is 32 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.

@meixg 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 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
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@inoway46 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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. https Issues and PRs related to the https subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants