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

POST and PUT on 307 redirect will re-POST without body #1084

Closed
jaraco opened this Issue Jan 4, 2013 · 13 comments

Comments

Projects
None yet
5 participants
@jaraco
Contributor

jaraco commented Jan 4, 2013

With requests 1.0.4, if one makes a POST or PUT request to a resource which returns a 307 temporary redirect (section 10.3.8 in the RFC), requests will automatically submit the same request to the redirect target, but without any body.

The RFC states:

   If the 307 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.

Therefore, I believe requests should raise an exception in these cases, or otherwise require that the request be configured to "re-post on 307".

I suspect the author has already considered some of these issues, so I post this bug to raise awareness of the issue and document the intended behavior, or to alter the behavior to better suit expectations.

Please let us know if there's anything we can do to help (demonstrate behavior, write tests, supply patch).

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 23, 2013

This has gone a bit without comment it seems. If I understand correctly, the behaviour on redirect should be that we don't follow the redirect but instead just return the 307 response to the user, correct?

Are there any other cases like this?

@piotr-dobrogost

This comment has been minimized.

Contributor

piotr-dobrogost commented Jan 23, 2013

@sigmavirus24

If I understand correctly, the behaviour on redirect should be that we don't follow the redirect but instead just return the 307 response to the user, correct?

Not quite. Section 7.4. Redirection 3xx in the current draft from httpbis working group states

This class of status code indicates that further action needs to be
taken by the user agent in order to fulfill the request. If the
required action involves a subsequent HTTP request, it MAY be carried
out by the user agent without interaction with the user if and only
if the method used in the second request is known to be "safe", as
defined in Section 5.2.1.

So speaking simply in case of 307 status code in reply to request with unsafe method (method other than the GET, HEAD, OPTIONS, and TRACE) we MAY NOT carry the redirect without interaction with the user. Now, the question is how to define interaction with the user for http library. I agree with @jaraco saying that we should require that the request be configured to "re-post on 307".

Are there any other cases like this?

Yes, soon to be accepted 308 status code - look for draft-reschke-http-status-308-07.txt at http://www.rfc-editor.org/cluster_info.php?cid=C160

   +-------------------------------------------+-----------+-----------+
   |                                           | Permanent | Temporary |
   +-------------------------------------------+-----------+-----------+
   | Allows changing the request method from   | 301       | 302       |
   | POST to GET                               |           |           |
   | Does not allow changing the request       | 308       | 307       |
   | method from POST to GET                   |           |           |
   +-------------------------------------------+-----------+-----------+

Side note: using codes.moved, codes.found etc. in the source code instead of codes' numeric values does not help in quickly identifying these codes.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 24, 2013

@piotr-dobrogost my comment was in the context of a POST or PUT request. Otherwise, my opinion is that POST or PUTs should return the 307 as our way of interacting with the user, otherwise follow it. It seems that the decision has been made that allow_redirects should be set to False, otherwise requests understands the interaction to be that it should re-POST/PUT the data, which seems reasonable to me (in the case the user is expecting a 307).

Edit: I'm clearly too busy. I conflated the #975 with this.

@piotr-dobrogost

This comment has been minimized.

Contributor

piotr-dobrogost commented Jan 24, 2013

Otherwise, my opinion is that POST or PUTs should return the 307 as our way of interacting with the user, otherwise follow it.

Returning 307/308 status code as a way of interacting with the user seems like the cleanest solution, indeed. The only problem with this is that in cases people wanted to follow such redirects they would have to implement it themselves. If user could pass a callback called upon receiving 307/308 status codes this callback's return value would represent user's decision on weather or not follow a redirect.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 24, 2013

Returning 307/308 status code as a way of interacting with the user seems
like the cleanest solution, indeed. The only problem with this is that in
cases people wanted to follow such redirects they would have to implement it
themselves. If user could pass a callback called upon receiving 307/308
status codes this callback's return value would represent user's decision
on weather or not follow a redirect.

And by implementing it themselves, they could use hooks. Hooks now are
dispatched before the redirects are detected. One could write a hook that
changes a 307 to a 301 (or 302) on which requests would (and should) happily
redirect. It's far from the best API, but it is security minded, which I
personally would prefer.

@cbare

This comment has been minimized.

Contributor

cbare commented Jan 25, 2013

It would be great if clients could register a call-back or set an option to select the desired redirect behavior. Even though it's a bad idea, there are reasons that people will want to follow redirects including POSTing to the new location.

Writing a hook doesn't work if you want to follow redirected POST requests - and continue POSTing to the new location. The reason is the call to the request method in lines 122-135 of sessions.py doesn't propagate the body of the original request. It doesn't set the data parameter, which instead defaults to None.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 26, 2013

@cbare you're correct about that call to request not being complete.

So let me just walk through the steps of the request before submitting a pull request to fix that.

In a normal case (not chunked encoding), the user calls requests.post(url, data={'key': 'value'}, files={'foo': open('foo', 'rb')}). In this case, the Request object is created and prepared turning into a PreparedRequest which is what we receive as req in resolve_redirects. This is stored in req.body. Since this is prepared, we can do this (in resolve_redirects):

resp = self.request(
    url=url,
    method=method,
    headers=headers,
    data=req.body,
    # ...
)

This works because when data receives a string, it sends that.

The problematic case is when the user is using chunked encoding (I think). The problem is, I'm not entirely sure what happens with chunked encoding at the moment. Maybe @kennethreitz can explain how that works because I haven't presently looked at it at all.

I could be wrong and it could all be handled as one case though.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Jan 26, 2013

So I found the chunked requests commit. Assuming data is a generator, then sending request.body would produce a StopIteration anyway and your data wouldn't be sent. This is an annoyance that needs to be dealt with carefully and I'm not sure of a clean way of dealing with chunked requests besides simply returning the 307 and requiring the user to know that they have to re-POST their data on a 307, which seems very ... awkward.

@piotr-dobrogost

This comment has been minimized.

Contributor

piotr-dobrogost commented Jan 26, 2013

@sigmavirus24

And by implementing it themselves, they could use hooks

Hooks are for custom actions not mandated by any RFC. We should not force users to write/use hooks to accomplish something specified in the RFC. Also there's no collection of commonly needed hooks packaged with Requests which makes any solution based on hooks more problematic for users.

@cbare

the call to the request method in lines 122-135 of sessions.py doesn't propagate the body of the original request

Body will have to be kept when supporting 307/308.

@kennethreitz

This comment has been minimized.

Member

kennethreitz commented Feb 10, 2013

this should only happen if allow_redirects=True, that's why it exists. To allow the user to specify that they want that action to occur.

@piotr-dobrogost

This comment has been minimized.

Contributor

piotr-dobrogost commented Feb 11, 2013

@kennethreitz

allow_redirects is about allowing redirects in general but in case of 307/308 status codes web browser asks user about permission that's why we have to similarly "ask" user of the library.

@sigmavirus24

This comment has been minimized.

Member

sigmavirus24 commented Feb 13, 2013

So with my second to last pull request merged, we now re-post the data on 307. I missed that it should also be on 308, so if @kennethreitz doesn't mind I might just push the one-line fix for that instead of issuing a PR.

If I understand correctly, this issue would then be fixed. If users want to be "asked" they will just have to pass False to allow_redirects which will need to be documented.

@piotr-dobrogost

This comment has been minimized.

Contributor

piotr-dobrogost commented Feb 13, 2013

@sigmavirus24

As I wrote in my last comment

allow_redirects is about allowing redirects in general but in case of 307/308 status codes web browser asks user about permission that's why we have to similarly "ask" user of the library.

We need another param to let users decide if they want to follow 307/308 with unsafe http method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment