Skip to content

https: honor per-request TLS options over agent options - #66288

Closed
axedos wants to merge 1 commit into
nodejs:mainfrom
axedos:https-per-request-tls-override
Closed

axedos wants to merge 1 commit into
nodejs:mainfrom
axedos:https-per-request-tls-override

Conversation

@axedos

@axedos axedos commented Sep 25, 2026

Copy link
Copy Markdown

Per-request TLS options passed to https.request() / https.get() are silently
ignored when the https.Agent in use was constructed with the same option.
http.Agent merges the request options over the agent's own options, letting
the agent's value win.

That is surprising and can weaken security for TLS options in particular: a
per-request rejectUnauthorized: true on an agent configured with
rejectUnauthorized: false still sends the request with certificate
verification disabled. The same applies to ca and servername.

CVE-2026-58040 already handled this for checkServerIdentity by tagging the
request so the socket is not reused. This change generalizes that approach:

when a per-request rejectUnauthorized, ca, or servername differs from the
agent's value, request() captures the override and createConnection()
re-applies it, and the socket is not reused.

Refs: 52a8ace880d

@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 25, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Welcome to Node.js, and thank you for your first contribution!

Before review, please take a moment to read:

Please make sure every commit is signed off. For a first pull request, GitHub Actions require collaborator approval and Jenkins CI must be started by a collaborator or triager, so an initial wait is normal.

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.35%. Comparing base (bc6e1ad) to head (7145186).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #66288      +/-   ##
==========================================
- Coverage   90.37%   90.35%   -0.02%     
==========================================
  Files         790      790              
  Lines      273807   273850      +43     
  Branches    52367    52389      +22     
==========================================
- Hits       247446   247441       -5     
- Misses      16869    16918      +49     
+ Partials     9492     9491       -1     
Files with missing lines Coverage Δ
lib/https.js 98.11% <100.00%> (+0.90%) ⬆️

... and 36 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.

@MikeMcC399

Copy link
Copy Markdown
Contributor

This PR is failing linting tests. See the comments in the tests for more detail.

See also Pull requests > Step 6: Test with further details in the linked document section BUILDING > Running tests.

To run the linter, use make lint / vcbuild lint. It will lint JavaScript, C++, and Markdown files.

Per-request rejectUnauthorized/ca/servername were silently
ignored when the https.Agent set the same option, because
http.Agent merges request options over the agent's own,
letting the agent win. Capture the per-request overrides
and re-apply them in createConnection(), mirroring the
checkServerIdentity handling from CVE-2026-58040.

Refs: nodejs@52a8ace880d
Signed-off-by: axedos <acceleratingssoul@proton.me>
@axedos axedos closed this Oct 4, 2026
@axedos
axedos force-pushed the https-per-request-tls-override branch from 7145186 to 14b61cb Compare October 4, 2026 12:13
@axedos
axedos requested a review from a team as a code owner October 4, 2026 12:13
@MikeMcC399
MikeMcC399 removed the request for review from a team October 4, 2026 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants