Properly handle authentication when encountering redirects#13595
Properly handle authentication when encountering redirects#13595jtfmumm merged 2 commits intofeature/redirect-authenticationfrom
Conversation
fb122f1 to
c78e13b
Compare
c78e13b to
5b758b8
Compare
5b758b8 to
254db39
Compare
ede06a9 to
c9314f8
Compare
| status, | ||
| StatusCode::MOVED_PERMANENTLY | ||
| | StatusCode::FOUND | ||
| | StatusCode::SEE_OTHER |
There was a problem hiding this comment.
This is the change that fixes the Azure bug
There was a problem hiding this comment.
Do we know what reqwest does here? (i briefly looked but couldn't find it)
There was a problem hiding this comment.
The updates to the redirect implementation in #13754 are closer to what reqwest does
There was a problem hiding this comment.
But reqwest handles this same list of status codes
c9314f8 to
f428e96
Compare
f428e96 to
59a14a3
Compare
|
@jtfmumm I can confirm that this seems to work correctly for me installing packages from an Azure feed build locally from source on windows 11 as of 8a85667 e.g. running a random command such as |
|
Thanks for your help @jenshnielsen! |
|
@jenshnielsen Would you mind also testing #13754? I initially linked this PR (#13595) in the request I cc'd you on, but it would also be helpful to see if #13754 works for you. |
8a85667 to
77960e9
Compare
364403d
into
feature/redirect-authentication
…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.
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.