Skip to content

fix: do not forward credential headers on cross-origin redirect#813

Merged
fengmk2 merged 4 commits into
2.xfrom
fix/cross-origin-redirect-credential-leak-2.x
Jun 13, 2026
Merged

fix: do not forward credential headers on cross-origin redirect#813
fengmk2 merged 4 commits into
2.xfrom
fix/cross-origin-redirect-credential-leak-2.x

Conversation

@fengmk2

@fengmk2 fengmk2 commented Jun 13, 2026

Copy link
Copy Markdown
Member

Problem

When following a redirect to a different origin, urllib reused the caller's args verbatim, re-sending Authorization, Cookie, and Proxy-Authorization headers (and re-applying the Basic auth option) to the new origin. The only existing cleanup was the Host header.

Fix

In handleRedirect, when the redirect target origin differs from the current origin, strip those credential headers and clear auth/digestAuth before following. Same-origin redirects are unchanged; the caller's headers object is not mutated.

Tests

New test/redirect-cross-origin.test.js with two local servers on different ports: cross-origin strip (headers + auth) and same-origin preserve. Verified the cross-origin cases fail without the fix.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2739d02f-4442-4e48-9c3c-b5192cb9d48e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/cross-origin-redirect-credential-leak-2.x

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces security enhancements to prevent credential leakage by stripping sensitive headers (such as Authorization, Cookie, and Proxy-Authorization) and clearing authentication options during cross-origin redirects, while preserving them for same-origin redirects. Feedback on these changes highlights two critical issues in the fallback parser of isCrossOriginRedirect: first, comparing unnormalized hosts can cause same-origin redirects with explicit default ports to be incorrectly treated as cross-origin; second, the parser fails open by returning false on errors, which could leak credentials. It is recommended to normalize the ports and fail closed by returning true in the catch block.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lib/urllib.js Outdated
When following a redirect to a different origin, urllib reused the
caller's options verbatim, re-sending Authorization, Cookie, and
Proxy-Authorization (and re-applying the Basic auth from `auth`) to
the new origin. Strip these in handleRedirect when the redirect
crosses the origin boundary. Same-origin redirects are unchanged.
@fengmk2 fengmk2 force-pushed the fix/cross-origin-redirect-credential-leak-2.x branch from 550c293 to 1e81b92 Compare June 13, 2026 14:00
@fengmk2

fengmk2 commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Thanks, both points addressed in the fallback parser (the urlutil.parse branch, only used when the WHATWG URL global is unavailable):

  1. Default-port normalization: now compares protocol + hostname + a normalized port (defaulting to 443/80) instead of the raw host, so same-origin redirects with an explicit :80/:443 are no longer treated as cross-origin.
  2. Fail closed: the catch now returns true, so if the origin can't be determined the credentials are stripped rather than forwarded.

The primary path uses URL.origin, which already normalizes default ports and throws on a malformed location before the strip runs, so it was unaffected; the change is limited to the fallback.

@fengmk2

fengmk2 commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e81b9225c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lib/urllib.js Outdated
Comment thread lib/urllib.js

// Credential-bearing headers that must not be forwarded across an origin
// boundary when following a redirect, to avoid leaking them to a third party.
var CROSS_ORIGIN_SENSITIVE_HEADERS = [ 'authorization', 'cookie', 'proxy-authorization' ];

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve proxy auth when reusing the same proxy

For proxied requests where the caller pins a proxy with args.proxy/URLLIB_PROXY and supplies Proxy-Authorization in the request headers, a redirect to another origin still goes through that same authenticated proxy, but this list removes the proxy credential before the recursive request. In that context the header authenticates the proxy rather than the redirected origin, so authenticated proxies can start returning 407 on any cross-origin redirect; only strip it when it is actually being sent as an origin header rather than while proxying.

Useful? React with 👍 / 👎.

Comment thread lib/urllib.js Outdated
These tests depend on reaching https://www.npmjs.com and fail
deterministically in CI (and offline). They are pre-existing on the
2.x branch and unrelated to the redirect fix. Guard them with
it.skip (the npmRegistry/npmmirror keep-alive cases stay enabled).
@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.86957% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.94%. Comparing base (02cad0e) to head (44c1935).
⚠️ Report is 3 commits behind head on 2.x.

Files with missing lines Patch % Lines
lib/urllib.js 60.86% 18 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              2.x     #813      +/-   ##
==========================================
- Coverage   91.91%   89.94%   -1.98%     
==========================================
  Files           6        6              
  Lines         829      875      +46     
  Branches      225      243      +18     
==========================================
+ Hits          762      787      +25     
- Misses         67       88      +21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 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.

fengmk2 added 2 commits June 13, 2026 22:19
Node.js 8 is EOL and the current dependency tree requires >= 14
(undici, formdata-node, etc.), so the v8 jobs crash regardless of
the test changes.
- Clone args before dropping credentials so a caller reusing the same
  options object keeps its auth/digestAuth/headers (no in-place mutation).
- Keep Proxy-Authorization when the request goes through a proxy, since
  it authenticates the (unchanged) proxy hop, not the redirected origin.
- Derive the current origin from URL components when the request is made
  with an options-style object that has no href, so same-origin redirects
  are not misclassified as cross-origin by the fail-closed path.
@fengmk2

fengmk2 commented Jun 13, 2026

Copy link
Copy Markdown
Member Author

Thanks @chatgpt-codex-connector, all three addressed in 44c1935:

  1. Don't clear caller auth in place - the cross-origin branch now clones args (Object.assign({}, args)) and drops auth/digestAuth/credential headers on the clone, so reusing the same options object after a redirect still authenticates against the original origin. Added a regression test asserting opts.auth/opts.headers are untouched.

  2. Preserve proxy auth when proxied - when a proxy tunnel is in use (proxyTunnelAgent), Proxy-Authorization is kept (it authenticates the unchanged proxy hop, not the redirected origin), so pinned authenticated proxies no longer 407 on cross-origin redirects. It's still stripped when there's no proxy (where it would otherwise reach the target origin).

  3. Same-origin object-URL redirects - origin detection now derives an href from the parsed URL components (host/hostname/port/path) when the caller passes an options-style object without href, so same-origin redirects are detected correctly instead of hitting the fail-closed path and stripping credentials. (Truly-undeterminable origins still fail closed.)

The Gemini fallback-parser points (default-port normalization + fail-closed) from the earlier review are already in.

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

  • Reviewed the current PR head and confirmed the three follow-up points in the trigger are already present:
    • Options-style request objects without href are handled by deriving an origin href from host/hostname, port, and path, while undeterminable origins fail closed. lib/urllib.jsL73-L99
    • The fallback parser compares protocol, hostname, and normalized default ports, and parser errors fail closed. lib/urllib.jsL104-L115
    • Cross-origin redirects clone args, conditionally preserve Proxy-Authorization when a proxy tunnel is in use, and clear auth only on the redirect clone. lib/urllib.jsL756-L772
  • Confirmed regression coverage exists for stripping cross-origin credential headers, preserving same-origin credential headers, stripping cross-origin auth, preserving same-origin auth, and not mutating the caller’s reusable options object. test/redirect-cross-origin.test.jsL63-L135
  • No code changes were needed, so I did not create a new commit or pull request.

Testing

  • ./node_modules/.bin/mocha -t 30000 test/redirect-cross-origin.test.js
  • npm test -- --grep redirect-cross-origin — this was an invalid invocation for this repo’s npm scripts because the grep argument was forwarded as an extra file pattern; it ran broader tests and failed outside the targeted redirect test path.

View task →

@fengmk2 fengmk2 merged commit 7c86c46 into 2.x Jun 13, 2026
46 of 49 checks passed
@fengmk2 fengmk2 deleted the fix/cross-origin-redirect-credential-leak-2.x branch June 13, 2026 14:44
@fengmk2 fengmk2 mentioned this pull request Jun 13, 2026
fengmk2 added a commit that referenced this pull request Jun 13, 2026
Add the 2.44.1 entry for the cross-origin credential header fix (#813).
fengmk2 added a commit that referenced this pull request Jun 13, 2026
Release urllib v2.44.1.

Merging this PR updates the version on `2.x` and triggers the release
workflow, which publishes to npm (dist-tag `latest-2`) and creates the
GitHub Release after manual approval.

## What's Changed

### Security

* Do not forward credential headers (`Authorization`, `Cookie`,
`Proxy-Authorization`) on cross-origin redirect, and clear
`auth`/`digestAuth` before following. Same-origin redirects are
unchanged and the caller's headers object is never mutated (#813).

### Internal

* Two-stage release workflow with manual approval, publishing the 2.x
line to the `latest-2` npm dist-tag (#815).
* Use Node 24 in the release workflow for npm 11 OIDC trusted
publishing.

---------

Co-authored-by: fengmk2 <156269+fengmk2@users.noreply.github.com>
Co-authored-by: MK <fengmk2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant