Wrong behavior if delete request receives redirect response #281

Closed
hading opened this Issue Dec 6, 2012 · 12 comments

Projects

None yet

5 participants

@hading

If one makes a DELETE request and receives a redirect as a response then Mechanize follows the redirect, but as a GET.

Per Eric Hodel:

According to RFC 2616 section 10.3 only GET and HEAD requests may follow redirects.

So it should not follow at all.

I raise the issue as I'm using a commercial product that may issue a redirect in response to a DELETE and expects me to issue a new DELETE to the location specified by the redirect. So I'd love if Mechanize provided some sort of hook to enable that to happen, whether it be raising an exception or setting a flag in some way that I could check to see if I got a redirect and providing a way to extract the new location and reissue the request, or (I'm sure less optimal from your point of view) a way to configure the non-standard behavior for anyone who wanted it.

@leejarvis
Sparkle Motion member

Adding some info here

tests to update: test_mechanize.rb lines 283 (DELETE), 952 (PUT), 938 (POST)

@drbrain Thoughts on returns for a Net::HTTPRedirection on everything but Net::HTTP::Get or Net::HTTP::Head?

@drbrain
Sparkle Motion member

We should follow RFC 2616 in this manner, if we get a redirect response when we didn't issue a GET or HEAD we should return some special type of response for the user to follow manually.

@leejarvis leejarvis added a commit that referenced this issue Feb 4, 2013
@leejarvis leejarvis ensure only GET and HEAD requests follow redirects
ref #281

RFC 2616 section 10.3

example:

agent = Mechanize.new
page = agent.delete('http://example.com/')
page.body        #=> ""
page['Location'] #=> "http://google.com"
71602a5
@leejarvis
Sparkle Motion member

@drbrain Can you review that commit, am I missing anything?

@drbrain
Sparkle Motion member

I thought I could CC this issue from the commit, but I can't. Looks great!

@drbrain drbrain closed this Feb 4, 2013
@leejarvis leejarvis added a commit that referenced this issue Feb 4, 2013
@leejarvis leejarvis updated changelog for #281 5ec694f
@bradleybuda

This change breaks many real websites (I can provide examples - I have a script that logs into Campfire that breaks with this change). While you're correctly following the RFC, all of the major browsers follow a 302 redirect after a POST as well. The RFC defines codes 303 and 307 to disambiguate this case, and further notes that the standard is different than most actual implementations: http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html (see the note under "302 Found").

You're absolutely right that this follows the standard more closely, but if the goal of Mechanize is to act like a real browser, this is a step in the wrong direction.

@drbrain drbrain reopened this Feb 8, 2013
@drbrain
Sparkle Motion member

Oops, I did not read the change close enough, you are right.

We're only allowed to GET or HEAD after redirect on POST, DELETE, etc., not POST or DELETE after redirect so we should change the method to GET when redirecting (unless the request was HEAD).

@leejarvis
Sparkle Motion member
@muescha

i think we should behave like more browsers.

and for the special use case of the issue opener (no redirects at posts) can be solved with the original unchanged code with the option {{@redirect_ok}} set to false

see:
https://github.com/sparklemotion/mechanize/blob/71602a5a9106cd64525429f4894121673608a71b/lib/mechanize/http/agent.rb#L936
this option let also return the page like its done in the patch 71602a5

@leejarvis
Sparkle Motion member

Yeah, @drbrain should we just revert 71602a5 (the behaviour that can still be achieved using redirect_ok) and continue with the existing code (which follows redirects with a GET)

@drbrain
Sparkle Motion member

Just for clarification:

> DELETE /foo
< 303 See Other, Location: /
> GET /
< 200 OK

is OK, but:

> DELETE /foo
< 303 See Other, Location: /
> DELETE /
< …

is NOT OK

@leejarvis
Sparkle Motion member

@drbrain Yeah from what I can see, this is exactly what happened before. Reverting my changes should resolve this which discards the original issues request.

@leejarvis leejarvis added a commit that referenced this issue Mar 5, 2013
@leejarvis leejarvis Revert "ensure only GET and HEAD requests follow redirects"
This reverts commit 71602a5.

ref #281
e1e0209
@leejarvis leejarvis added a commit that referenced this issue Mar 5, 2013
@leejarvis leejarvis Revert "updated changelog for #281"
This reverts commit 5ec694f.

Conflicts:
	CHANGELOG.rdoc

ref #281
62b1460
@leejarvis
Sparkle Motion member

Can we close this? I reverted the altered code, as it appears Mechanize already behaves as we're discussing it should.

@leejarvis leejarvis closed this Mar 21, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment