Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-99730: HEAD should remain HEAD request on redirect#99731

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
encukou merged 6 commits intopython:mainfromhaampie:patch-1
May 1, 2024

Conversation

haampie
Copy link
Contributor

@haampiehaampie commentedNov 23, 2022
edited by bedevere-bot
Loading

tgamblin and Moelf reacted with thumbs up emoji
@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@ghost
Copy link

ghost commentedNov 23, 2022
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@haampiehaampie changed the titlegh-99730: HEAD remain HEAD requests on redirectgh-99730: HEAD should remain HEAD request on redirectNov 23, 2022
tgamblin pushed a commit to spack/spack that referenced this pull requestNov 23, 2022
For reasons beyond me Python thinks it's a great idea to upgrade HEADrequests to GET requests when following redirects. So, this PR adds abetter `HTTPRedirectHandler`, and also moves some ad-hoc logic aroundfor dealing with disabling SSL certs verification.Also, I'm stumped by the fact that Spack's `url_exists` does not useHEAD requests at all, so in certain cases Spack awkwardly downloadssomething first to see if it can download it, and then downloads itagain because it knows it can download it. So, this PR ensures that bothurllib and botocore use HEAD requests.Finally, it also removes some things that were there to support currentlyunsupported Python versions.Notice that the HTTP spec [section 10.3.2](https://datatracker.ietf.org/doc/html/rfc2616.html#section-10.3.2) just talks about how to dealwith POST request on redirect (whether to follow or not):>   If the 301 status code is received in response to a request other>   than GET or HEAD, the user agent MUST NOT automatically redirect the>   request unless it can be confirmed by the user, since this might>   change the conditions under which the request was issued.>   Note: When automatically redirecting a POST request after>   receiving a 301 status code, some existing HTTP/1.0 user agents>   will erroneously change it into a GET request.Python has a comment about this, they choose to go with the "erroneous change".But they then mess up the HEAD request while following the redirect, probablybecause they were too busy discussing how to deal with POST.Seepython/cpython#99731
amd-toolchain-support pushed a commit to amd-toolchain-support/spack that referenced this pull requestFeb 16, 2023
For reasons beyond me Python thinks it's a great idea to upgrade HEADrequests to GET requests when following redirects. So, this PR adds abetter `HTTPRedirectHandler`, and also moves some ad-hoc logic aroundfor dealing with disabling SSL certs verification.Also, I'm stumped by the fact that Spack's `url_exists` does not useHEAD requests at all, so in certain cases Spack awkwardly downloadssomething first to see if it can download it, and then downloads itagain because it knows it can download it. So, this PR ensures that bothurllib and botocore use HEAD requests.Finally, it also removes some things that were there to support currentlyunsupported Python versions.Notice that the HTTP spec [section 10.3.2](https://datatracker.ietf.org/doc/html/rfc2616.html#section-10.3.2) just talks about how to dealwith POST request on redirect (whether to follow or not):>   If the 301 status code is received in response to a request other>   than GET or HEAD, the user agent MUST NOT automatically redirect the>   request unless it can be confirmed by the user, since this might>   change the conditions under which the request was issued.>   Note: When automatically redirecting a POST request after>   receiving a 301 status code, some existing HTTP/1.0 user agents>   will erroneously change it into a GET request.Python has a comment about this, they choose to go with the "erroneous change".But they then mess up the HEAD request while following the redirect, probablybecause they were too busy discussing how to deal with POST.Seepython/cpython#99731
Copy link
Member

@encukouencukou left a comment

Choose a reason for hiding this comment

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

This makes sense to me.

@haampie
Copy link
ContributorAuthor

Is there a way to test whether this is breaking in practice?

For example, I've noticed that a Gitlab instance (forgot which) had download URLs for release assets, that returned a redirect to an AWS bucket with a URL that contains a signature. What's signed is the request as a whole, including the HTTP verb. It would only work with GET, HEAD requests would error with 403.

So, following the redirect required a GET request.

Not sure whose bug that is.

@encukou
Copy link
Member

encukou commentedMar 6, 2024
edited
Loading

It seems that the error is with Gitlab/AWS, but easily worked around by not doing HEAD requests to a service that doesn't support them.

@orsenthil, as the urllib expert, do you have an opinion?

@orsenthil
Copy link
Member

do you have an opinion?
This change sounds reasonable and correct to me. I will find out why it was GET prior to this change and check the RFCs before approving and merging.

haampie reacted with thumbs up emoji

@haampie
Copy link
ContributorAuthor

haampie commentedMar 9, 2024
edited
Loading

Looks like it's complicated.

The RFCs suggest "user interaction" in certain cases which is obviously impossible.

And there's different behavior for different types of 3xx status codes.

Looks like they realized that redirect on POST request is somewhat ambiguous: either it means the POST request has to be sent elsewhere, or, the POST request succeeded, you just have to GET the (success) response elsewhere. The 303 message resolves that ambiguity. I have no clue if any modern webserver is following the RFC correctly, they might still reply with 302 on success (?) -- the POST behavior in urllib makes sense to me.

FromRFC 2616 10.3.2:301 Moved Permanently

Note: When automatically redirecting a POST request after
receiving a 301 status code, some existing HTTP/1.0 user agents
willerroneously change it into a GET request.

FromRFC 2616 10.3.3:302 Found

Note:RFC 1945 andRFC 2068 specify that the client is not allowed
to change the method on the redirected request. However, most
existing user agent implementations treat 302 as if it were a 303
response, performing a GET on the Location field-value regardless
of the original request method. The status codes 303 and 307 have
been added for servers that wish to make unambiguously clear which
kind of reaction is expected of the client.

AndRFC 2616 10.3.4:303 See Other

The response to the request can be found under a different URI and
SHOULD be retrieved using a GET method on that resource. This method
exists primarily to allow the output of a POST-activated script to
redirect the user agent to a selected resource.

AndRFC 2626 10.3.8:307 Temporary Redirect

If the 307 status code is received in response to a request other
than GET or HEAD, the user agentMUST NOT automatically redirect the
request unless it can be confirmed by the user, since this might
change the conditions under which the request was issued.

@encukou
Copy link
Member

IMO, while it's not clear whether the redirected request should be made, it's pretty clear that if itis made, HEAD should stay HEAD rather than turn into GET.
As I read it, the standard's wording on303 See Other says what to do to retrieve the resource. But making a HEAD request means youdon't want to retrieve the resource :)

The behaviour on307 Temporary Redirect does not follow the standard, but, that's not what this PR is about.

@encukou
Copy link
Member

@orsenthil,

This change sounds reasonable and correct to me. I will find out why it was GET prior to this change and check the RFCs before approving and merging.

Is there anything I can do to help with this?

@encukou
Copy link
Member

I will find out why it was GET prior to this change and check the RFCs before approving and merging.

@orsenthil Do you think you can find the time before Python 3.13 beta1?

@encukou
Copy link
Member

I plan to merge this around Wednesday if there are no objections.

Copy link
Member

@orsenthilorsenthil left a comment

Choose a reason for hiding this comment

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

LGTM

@orsenthil
Copy link
Member

@encukou -

I apologize, I forgot to get back to this issue and discussion. I spent time reviewing the RFCs, although it surprises me that we have this behavior (of HEAD upgrading to GET) for a long time, I was not able to find a requirement to this behavior in the RFCs. Further, the clients like curl, which I often used as a reference, do no upgrade HEAD to GET. So this change is correct thing to do.

There is a possibility of some client relying on this archaic behavior, but they wont be adversely affected, and they could handle this a GET again.

Thanks for change, and pushing this through.

encukou reacted with heart emoji

@encukou
Copy link
Member

Thank you for checking!

@encukouencukou merged commit759e8e7 intopython:mainMay 1, 2024
33 checks passed
SonicField pushed a commit to SonicField/cpython that referenced this pull requestMay 8, 2024
@haampiehaampie deleted the patch-1 branchFebruary 17, 2025 16:46
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@bcailbcailbcail left review comments

@encukouencukouencukou approved these changes

@orsenthilorsenthilorsenthil approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@haampie@bedevere-bot@encukou@orsenthil@bcail

[8]ページ先頭

©2009-2025 Movatter.jp