-
Notifications
You must be signed in to change notification settings - Fork 637
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retain authentication attached to URLs when making requests to the same host #1874
Conversation
/// <https://datatracker.ietf.org/doc/html/rfc7235#section-2.2> | ||
/// <https://datatracker.ietf.org/doc/html/rfc7230#section-5.5> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for linking to the relevant sections
if request_url.port() != response_url.port() { | ||
if (request_url.port() == default_port || request_url.port().is_none()) | ||
&& (response_url.port() == default_port || response_url.port().is_none()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the check the port
comment not that useful. I can see that we check something with the port but I'm having difficulty understanding what's being checked. Maybe we can explain that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment just matches all the previous ones saying which part we're checking, the inner comment is supposed to explain this. I can try to do better though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay 477e18b should be a lot better
Fyi I tested this against AWS CodeArtifact. |
…les (#1886) Closes #1709 Closes #1371 Tested with the reproduction provided in #1709 which gets past the HTTP 401. Reuses the same copying logic we introduced in #1874 to ensure authentication is attached to file URLs with a realm that matches that of the index. I had to move the authentication logic into a new crate so it could be used in `distribution-types`. We will want to something more robust in the future, like track all realms with authentication in a central store and perform lookups there. That's what `pip` does and it allows consolidation of logic like netrc lookups. That refactor feels significant though, and I'd like to get this fixed ASAP so this is a minimal fix.
Closes #1860
In #1816, we started using the URL attached to a response instead of the request URL for subsequent requests — this fixes various bugs but has the side-effect of dropping credentials from the URL. Here, we transfer credentials from the request URL to the response URL. We perform RFC compliant checks for safety.