Skip to content

url: compare full origin when setting credentials#21575

Closed
parasol-aser wants to merge 1 commit into
curl:masterfrom
parasol-aser:fix-creds-redirect-origin
Closed

url: compare full origin when setting credentials#21575
parasol-aser wants to merge 1 commit into
curl:masterfrom
parasol-aser:fix-creds-redirect-origin

Conversation

@parasol-aser

@parasol-aser parasol-aser commented May 12, 2026

Copy link
Copy Markdown
Contributor

When setting transfer credentials from options, use the full origin comparison instead of the destination-only comparison.

Problem:

  • By default, option credentials are only supposed to be used for the initial origin unless unrestricted auth is enabled.
  • A redirect can change scheme while keeping the same host and numeric port, for example from https://example.com:8443/ to http://example.com:8443/.
  • Curl_peer_same_destination() ignores the scheme, so option credentials could remain available for that redirected request.
  • Curl_peer_equal() includes the scheme and matches the origin check used elsewhere.

Test:

  • Added test3106, which makes an HTTPS request with Basic auth through the test HTTP proxy, redirects to HTTP on the same host and port, and verifies that the redirected HTTP request does not include Authorization.

@parasol-aser parasol-aser marked this pull request as ready for review May 12, 2026 16:17
Comment thread lib/url.c Outdated
@bagder

bagder commented May 12, 2026

Copy link
Copy Markdown
Member

Please elaborate on what problem this fixes, and please also provide a test that verifies that your fix is correct.

@parasol-aser parasol-aser force-pushed the fix-creds-redirect-origin branch from 6e54e40 to 9028d67 Compare May 12, 2026 23:29
@github-actions github-actions Bot added the tests label May 12, 2026
@parasol-aser

Copy link
Copy Markdown
Contributor Author

Updated the description to explain the origin-comparison issue, and added test3106.

The test covers an HTTPS request with Basic auth through the test HTTP proxy, followed by a redirect to HTTP on the same host and numeric port. It verifies that the redirected HTTP request does not include Authorization.

@icing icing left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work, good catch.

@parasol-aser care to apply the mentioned change?

Comment thread lib/url.c Outdated
@parasol-aser parasol-aser force-pushed the fix-creds-redirect-origin branch from 9028d67 to 739a451 Compare May 13, 2026 13:51
@parasol-aser

Copy link
Copy Markdown
Contributor Author

Updated to use the shared Curl_auth_allowed_to_host policy for the credential gate. url_set_data_creds() runs while building the not-yet-attached needle connection, so I made the helper take the origin explicitly and updated the existing call sites to pass their current origin.

Local checks:

  • scripts/checksrc.pl on the touched C files/headers
  • CMake/Ninja debug build: cmake --build build --target curl testdeps -j2
  • TFLAGS=3106 cmake --build build --target tests -j2 selected the new test, but this container skips HTTPS tests with: no stunnel

@parasol-aser parasol-aser force-pushed the fix-creds-redirect-origin branch from 739a451 to b8a3a13 Compare May 13, 2026 14:10
@bagder bagder requested a review from Copilot May 14, 2026 09:02
@bagder

bagder commented May 14, 2026

Copy link
Copy Markdown
Member

This PR has a merge conflict now. Can you rebase onto master and force-push?

Copilot AI 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.

Pull request overview

Tightens the origin check used when applying transfer credentials so that scheme differences (e.g., HTTPS→HTTP redirect on the same host:port) correctly cause option credentials to be withheld unless allow_auth_to_other_hosts is set.

Changes:

  • Introduce Curl_auth_allowed_to_origin() that compares against a caller-supplied origin using Curl_peer_equal() (scheme-sensitive), and refactor Curl_auth_allowed_to_host() to delegate to it.
  • Replace Curl_peer_same_destination() with Curl_auth_allowed_to_origin() in url_set_data_creds() so credential gating includes the scheme.
  • Add test3106 covering an HTTPS→HTTP same-host:port redirect through an HTTP proxy, verifying the redirected HTTP request lacks Authorization.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
lib/vauth/vauth.h Forward-declare Curl_peer and expose new Curl_auth_allowed_to_origin() API.
lib/vauth/vauth.c Implement origin-aware variant and route Curl_auth_allowed_to_host() through it.
lib/url.c Use scheme-aware origin check when applying option credentials; include vauth header.
tests/data/test3106 New test asserting credentials are not leaked across a same-host/port scheme-changing redirect.
tests/data/Makefile.am Register test3106 in the build/test list.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@parasol-aser parasol-aser force-pushed the fix-creds-redirect-origin branch from b8a3a13 to f41ff0e Compare May 14, 2026 13:18
@parasol-aser

parasol-aser commented May 14, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and force-pushed.

I reran 1140 1173 1177 1477 1501 and all five passed.

@bagder bagder closed this in a15483c May 15, 2026
@bagder

bagder commented May 15, 2026

Copy link
Copy Markdown
Member

Thanks!

outcast36 pushed a commit to greearb/curl that referenced this pull request Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

5 participants