Update redirect implementation to handle more cases and be more testable #13754

jtfmumm · 2025-05-31T13:46:52Z

This PR factors out and updates the redirect handling logic from #13595. It handles a few new cases:

If a 303 redirect is received for any method other than GET or HEAD, converts it to a GET. Unlike reqwest, it does not do this conversion for 301s or 302s (which is not required by RFC 7231 and was not the original intention of the spec).
If the original request did not have a Referer header, does not include a Referer header in the redirect request.
If the redirect is a cross-origin request, removes sensitive headers to avoid leaking credentials to untrusted domains.
- This change had the side effect of breaking mock server tests that redirected from localhost to pypi-proxy.fly.dev. I have added a CrossOriginCredentialsPolicy enum with a #[cfg(test)]-only Insecure variant. This allows existing tests to continue to work while still making it impossible to propagate credentials on cross-origin requests outside of tests.
- I've updated the main redirect integration test to check if cross-origin requests fail (there is, by design, no way to configure an insecure cross-origin policy from the command line). But critically, netrc credentials for the new location can still be successfully fetched on a cross-origin redirect (tested in pip_install_redirect_with_netrc_cross_origin).
One of the goals of the refactor was to make the redirect handling logic unit-testable. This PR adds a number of unit tests checking things like proper propagation of credentials on redirects on the same domain (and removal on cross-origin) and HTTP 303 POST-to-GET conversion.

The following table illustrates the different behaviors on current main, the initial (reverted) redirect handling PR (#12920), the PR that restores #12920 and fixes the 303s bug (#13595), and this PR (#13754). We want to propagate credentials on same-origin but not cross-origin redirects, and we want to look up netrc credentials on redirects.

Behavior	main	reverted #12920	fix #13595	update #13754
Propagate credentials on same-origin redirects	No	Yes	Yes	Yes
Propagate credentials on cross-origin redirects	No	Yes	Yes	No
Look up netrc credentials on redirects	No	Yes	Yes	Yes
Handle 303s without failing	Yes	No	Yes	Yes

Depends on #13595.

jtfmumm · 2025-05-31T14:17:52Z

crates/uv-client/src/base_client.rs


+    /// Allows credentials to be propagated on cross-origin redirects.
+    ///
+    /// WARNING: This should only be available for tests. In production code, propagating credentials


I've added this same warning in a few places to make sure this invariant isn't broken in the future.

jtfmumm · 2025-05-31T14:35:32Z

crates/uv/tests/it/edit.rs

+/// uv currently fails to look up keyring credentials on a cross-origin redirect.
 #[tokio::test]
-async fn add_redirect_with_keyring() -> Result<()> {
+async fn add_redirect_with_keyring_cross_origin() -> Result<()> {


On a cross-origin request (which is what happens with the mock server here), no username is propagated. It would be possible to use reqwest_middleware Extensions to do this and special-case the handling of a username propagated in that way, but we have decided to avoid the complexity around this refactor for now. Currently, this means that uv will not successfully find credentials in the keyring for cross-origin requests.

konstin

Can you add a table to the description showing what behaviors we have on main and how they are with the two PRs for the relevant cases? This makes easier to follow along what we're changing and that we're handling everything we want to handle.

jtfmumm · 2025-06-06T01:08:14Z

Can you add a table to the description showing what behaviors we have on main and how they are with the two PRs for the relevant cases? This makes easier to follow along what we're changing and that we're handling everything we want to handle.

I've added a table

jenshnielsen · 2025-06-16T13:06:05Z

@jtfmumm Tested this against a local azure feed. As of c9c44e3 this works correctly for me and I can confirm that I can install packages as expected.

jtfmumm · 2025-06-16T13:07:37Z

Thanks for the confirmation @jenshnielsen!

thomasaarholt · 2025-06-16T15:03:00Z

EDIT: Both ways worked great. No issues.

I checked out the PR, cd'd into a new folder, added a uv.toml file as per below, created a venv and ran cargo run -- pip install llms to install our custom llms package there. It ran fine. Here are the -v logs.

# uv.toml
[pip]
keyring-provider = "subprocess"
index-url = "https://VssSessionToken@<our-internal-feed>/pypi/simple/"

The (really cool looking!) uvx command gets stuck on the last compilation step. EDIT: It finished, just took quite a bit longer than compiling it myself. I then created a new folder for it, ran uv init, and ran the command again successfully.

jtfmumm · 2025-06-16T16:15:11Z

Thanks @thomasaarholt! Yeah that command can be slow, but also nice to have.

This is mostly restoring #13215. It also includes a one-line fix for #13208 (which resulted from that PR). In particular, Azure was returning 303s which were not being correctly handled. I have also opened another PR (#13754) that refactors and improves the redirect handling here. It also supersedes the fix here. There are some tests failing here but they all pass there. This PR depends on #13615, which adds a script for testing against registries. The test fails for Azure when running against the restored #13215 alone and passes with the fix. It also passes for AWS CodeArtifact, GCP Artifact Registry, JFrog Artifactory, GitLab, and Gemfury in both cases. I also plan to test against Cloudsmith and Nexus.

…ble (#13754) This PR factors out and updates the redirect handling logic from #13595. It handles a few new cases: * If a 303 redirect is received for any method other than GET or HEAD, converts it to a GET. Unlike `reqwest`, it does not do this conversion for 301s or 302s (which is not required by RFC 7231 and was not the original intention of the spec). * If the original request did not have a Referer header, does not include a Referer header in the redirect request. * If the redirect is a cross-origin request, removes sensitive headers to avoid leaking credentials to untrusted domains. * * This change had the side effect of breaking mock server tests that redirected from `localhost` to `pypi-proxy.fly.dev`. I have added a `CrossOriginCredentialsPolicy` enum with a `#[cfg(test)]`-only `Insecure` variant. This allows existing tests to continue to work while still making it impossible to propagate credentials on cross-origin requests outside of tests. * * I've updated the main redirect integration test to check if cross-origin requests fail (there is, by design, no way to configure an insecure cross-origin policy from the command line). But critically, netrc credentials for the new location can still be successfully fetched on a cross-origin redirect (tested in `pip_install_redirect_with_netrc_cross_origin`). * One of the goals of the refactor was to make the redirect handling logic unit-testable. This PR adds a number of unit tests checking things like proper propagation of credentials on redirects on the same domain (and removal on cross-origin) and HTTP 303 POST-to-GET conversion. The following table illustrates the different behaviors on current `main`, the initial (reverted) redirect handling PR (#12920), the PR that restores #12920 and fixes the 303s bug (#13595), and this PR (#13754). We want to propagate credentials on same-origin but not cross-origin redirects, and we want to look up netrc credentials on redirects. | Behavior | main | reverted #12920 | fix #13595 | update #13754 | |---------------------------------------|------|--------------|-------------|----------------| | Propagate credentials on same-origin redirects | No | Yes | Yes | Yes | | Propagate credentials on cross-origin redirects | No | Yes | Yes | No | | Look up netrc credentials on redirects | No | Yes | Yes | Yes | | Handle 303s without failing | Yes | No | Yes | Yes | Depends on #13595.

This is mostly restoring #13215. It also includes a one-line fix for #13208 (which resulted from that PR). In particular, Azure was returning 303s which were not being correctly handled. I have also opened another PR (#13754) that refactors and improves the redirect handling here. It also supersedes the fix here. There are some tests failing here but they all pass there. This PR depends on #13615, which adds a script for testing against registries. The test fails for Azure when running against the restored #13215 alone and passes with the fix. It also passes for AWS CodeArtifact, GCP Artifact Registry, JFrog Artifactory, GitLab, and Gemfury in both cases. I also plan to test against Cloudsmith and Nexus.

…ble (#13754) This PR factors out and updates the redirect handling logic from #13595. It handles a few new cases: * If a 303 redirect is received for any method other than GET or HEAD, converts it to a GET. Unlike `reqwest`, it does not do this conversion for 301s or 302s (which is not required by RFC 7231 and was not the original intention of the spec). * If the original request did not have a Referer header, does not include a Referer header in the redirect request. * If the redirect is a cross-origin request, removes sensitive headers to avoid leaking credentials to untrusted domains. * * This change had the side effect of breaking mock server tests that redirected from `localhost` to `pypi-proxy.fly.dev`. I have added a `CrossOriginCredentialsPolicy` enum with a `#[cfg(test)]`-only `Insecure` variant. This allows existing tests to continue to work while still making it impossible to propagate credentials on cross-origin requests outside of tests. * * I've updated the main redirect integration test to check if cross-origin requests fail (there is, by design, no way to configure an insecure cross-origin policy from the command line). But critically, netrc credentials for the new location can still be successfully fetched on a cross-origin redirect (tested in `pip_install_redirect_with_netrc_cross_origin`). * One of the goals of the refactor was to make the redirect handling logic unit-testable. This PR adds a number of unit tests checking things like proper propagation of credentials on redirects on the same domain (and removal on cross-origin) and HTTP 303 POST-to-GET conversion. The following table illustrates the different behaviors on current `main`, the initial (reverted) redirect handling PR (#12920), the PR that restores #12920 and fixes the 303s bug (#13595), and this PR (#13754). We want to propagate credentials on same-origin but not cross-origin redirects, and we want to look up netrc credentials on redirects. | Behavior | main | reverted #12920 | fix #13595 | update #13754 | |---------------------------------------|------|--------------|-------------|----------------| | Propagate credentials on same-origin redirects | No | Yes | Yes | Yes | | Propagate credentials on cross-origin redirects | No | Yes | Yes | No | | Look up netrc credentials on redirects | No | Yes | Yes | Yes | | Handle 303s without failing | Yes | No | Yes | Yes | Depends on #13595.

This is mostly restoring #13215. It also includes a one-line fix for #13208 (which resulted from that PR). In particular, Azure was returning 303s which were not being correctly handled. I have also opened another PR (#13754) that refactors and improves the redirect handling here. It also supersedes the fix here. There are some tests failing here but they all pass there. This PR depends on #13615, which adds a script for testing against registries. The test fails for Azure when running against the restored #13215 alone and passes with the fix. It also passes for AWS CodeArtifact, GCP Artifact Registry, JFrog Artifactory, GitLab, and Gemfury in both cases. I also plan to test against Cloudsmith and Nexus.

…ble (#13754) This PR factors out and updates the redirect handling logic from #13595. It handles a few new cases: * If a 303 redirect is received for any method other than GET or HEAD, converts it to a GET. Unlike `reqwest`, it does not do this conversion for 301s or 302s (which is not required by RFC 7231 and was not the original intention of the spec). * If the original request did not have a Referer header, does not include a Referer header in the redirect request. * If the redirect is a cross-origin request, removes sensitive headers to avoid leaking credentials to untrusted domains. * * This change had the side effect of breaking mock server tests that redirected from `localhost` to `pypi-proxy.fly.dev`. I have added a `CrossOriginCredentialsPolicy` enum with a `#[cfg(test)]`-only `Insecure` variant. This allows existing tests to continue to work while still making it impossible to propagate credentials on cross-origin requests outside of tests. * * I've updated the main redirect integration test to check if cross-origin requests fail (there is, by design, no way to configure an insecure cross-origin policy from the command line). But critically, netrc credentials for the new location can still be successfully fetched on a cross-origin redirect (tested in `pip_install_redirect_with_netrc_cross_origin`). * One of the goals of the refactor was to make the redirect handling logic unit-testable. This PR adds a number of unit tests checking things like proper propagation of credentials on redirects on the same domain (and removal on cross-origin) and HTTP 303 POST-to-GET conversion. The following table illustrates the different behaviors on current `main`, the initial (reverted) redirect handling PR (#12920), the PR that restores #12920 and fixes the 303s bug (#13595), and this PR (#13754). We want to propagate credentials on same-origin but not cross-origin redirects, and we want to look up netrc credentials on redirects. | Behavior | main | reverted #12920 | fix #13595 | update #13754 | |---------------------------------------|------|--------------|-------------|----------------| | Propagate credentials on same-origin redirects | No | Yes | Yes | Yes | | Propagate credentials on cross-origin redirects | No | Yes | Yes | No | | Look up netrc credentials on redirects | No | Yes | Yes | Yes | | Handle 303s without failing | Yes | No | Yes | Yes | Depends on #13595.

…ts (#14126) This PR is a combination of #12920 and #13754. Prior to these changes, following a redirect when searching indexes would bypass our authentication middleware. This PR updates uv to support propagating credentials through our middleware on same-origin redirects and to support netrc credentials for both same- and cross-origin redirects. It does not handle the case described in #11097 where the redirect location itself includes credentials (e.g., `https://user:pass@redirect-location.com`). That will be addressed in follow-up work. This includes unit tests for the new redirect logic and integration tests for credential propagation. The automated external registries test is also passing for AWS CodeArtifact, Azure Artifacts, GCP Artifact Registry, JFrog Artifactory, GitLab, Cloudsmith, and Gemfury.

jtfmumm added enhancement New feature or improvement to existing functionality security labels May 31, 2025

jtfmumm mentioned this pull request May 31, 2025

Properly handle authentication when encountering redirects #13595

Merged

jtfmumm commented May 31, 2025

View reviewed changes

jtfmumm force-pushed the jtfm/update-redirect-handling branch 2 times, most recently from 9f19ed9 to e312789 Compare May 31, 2025 14:35

jtfmumm commented May 31, 2025

View reviewed changes

jtfmumm force-pushed the jtfm/update-redirect-handling branch 3 times, most recently from a5e9cdc to 1cc0e69 Compare May 31, 2025 14:39

jtfmumm marked this pull request as ready for review May 31, 2025 14:39

jtfmumm closed this May 31, 2025

jtfmumm reopened this May 31, 2025

jtfmumm changed the title ~~Update redirect implementation~~ Update redirect implementation to handle more cases and be more testable May 31, 2025

jtfmumm force-pushed the jtfm/update-redirect-handling branch 2 times, most recently from 4c08337 to b593d30 Compare June 2, 2025 13:11

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from 8b77039 to edf9395 Compare June 5, 2025 13:39

jtfmumm force-pushed the jtfm/update-redirect-handling branch from b593d30 to fc162d6 Compare June 5, 2025 13:39

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from edf9395 to faca251 Compare June 5, 2025 13:51

jtfmumm force-pushed the jtfm/update-redirect-handling branch from fc162d6 to 416ffe0 Compare June 5, 2025 13:52

konstin approved these changes Jun 5, 2025

View reviewed changes

jtfmumm force-pushed the jtfm/fix-redirect-handling branch 2 times, most recently from 8ac696c to 07df848 Compare June 6, 2025 13:24

jtfmumm force-pushed the jtfm/update-redirect-handling branch from 416ffe0 to 3b11243 Compare June 6, 2025 13:25

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from 07df848 to 0ad2529 Compare June 12, 2025 16:04

jtfmumm force-pushed the jtfm/update-redirect-handling branch from 3b11243 to ea48ef9 Compare June 12, 2025 16:04

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from 0ad2529 to 97183db Compare June 12, 2025 16:08

jtfmumm force-pushed the jtfm/update-redirect-handling branch from ea48ef9 to ae6b69c Compare June 12, 2025 16:09

jtfmumm temporarily deployed to uv-test-registries June 12, 2025 16:13 — with GitHub Actions Inactive

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from a5e0fcc to 8a85667 Compare June 13, 2025 17:03

jtfmumm force-pushed the jtfm/update-redirect-handling branch from 7cded92 to c9c44e3 Compare June 13, 2025 17:04

jtfmumm temporarily deployed to uv-test-registries June 13, 2025 17:06 — with GitHub Actions Inactive

jtfmumm force-pushed the jtfm/fix-redirect-handling branch from 8a85667 to 77960e9 Compare June 18, 2025 08:36

jtfmumm force-pushed the jtfm/update-redirect-handling branch from c9c44e3 to 6852cfd Compare June 18, 2025 08:40

jtfmumm temporarily deployed to uv-test-registries June 18, 2025 08:42 — with GitHub Actions Inactive

Base automatically changed from jtfm/fix-redirect-handling to feature/redirect-authentication June 18, 2025 08:49

Update redirect implementation

9c54824

jtfmumm force-pushed the jtfm/update-redirect-handling branch from 6852cfd to 9c54824 Compare June 18, 2025 08:51

jtfmumm temporarily deployed to uv-test-registries June 18, 2025 08:53 — with GitHub Actions Inactive

jtfmumm merged commit ad789d9 into feature/redirect-authentication Jun 18, 2025
75 checks passed

jtfmumm deleted the jtfm/update-redirect-handling branch June 18, 2025 08:57

jtfmumm mentioned this pull request Jun 18, 2025

Support netrc and same-origin credential propagation on index redirects #14126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update redirect implementation to handle more cases and be more testable #13754

Update redirect implementation to handle more cases and be more testable #13754

Uh oh!

jtfmumm commented May 31, 2025 •

edited

Loading

Uh oh!

jtfmumm May 31, 2025

Uh oh!

jtfmumm May 31, 2025

Uh oh!

konstin left a comment

Uh oh!

jtfmumm commented Jun 6, 2025

Uh oh!

jenshnielsen commented Jun 16, 2025

Uh oh!

jtfmumm commented Jun 16, 2025

Uh oh!

thomasaarholt commented Jun 16, 2025 •

edited

Loading

Uh oh!

jtfmumm commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Update redirect implementation to handle more cases and be more testable #13754

Update redirect implementation to handle more cases and be more testable #13754

Uh oh!

Conversation

jtfmumm commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtfmumm May 31, 2025

Choose a reason for hiding this comment

Uh oh!

jtfmumm May 31, 2025

Choose a reason for hiding this comment

Uh oh!

konstin left a comment

Choose a reason for hiding this comment

Uh oh!

jtfmumm commented Jun 6, 2025

Uh oh!

jenshnielsen commented Jun 16, 2025

Uh oh!

jtfmumm commented Jun 16, 2025

Uh oh!

thomasaarholt commented Jun 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtfmumm commented Jun 16, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtfmumm commented May 31, 2025 •

edited

Loading

thomasaarholt commented Jun 16, 2025 •

edited

Loading