Skip to content
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

Infinite Redirect detection doesn't work well #67

Closed
myronmarston opened this issue Apr 14, 2010 · 8 comments
Closed

Infinite Redirect detection doesn't work well #67

myronmarston opened this issue Apr 14, 2010 · 8 comments

Comments

@myronmarston
Copy link
Contributor

The rack test driver's infinite redirect detection is not working well for me. The current implementation uses a simple 4 second timeout. However, sometimes I set breakpoints in my controllers or models while running cucumber to troubleshoot something. In this case, the 4 second timeout occurs and I get an infinite redirect error even though it wasn't actually an infinite redirect.

Maybe this would be a better approach? Track all the redirect URLs since the last user-initiated request, and if a redirect occurs to the same URL more than twice, consider it an infinite redirect.

@jnicklas
Copy link
Collaborator

Yeah, the infinite redirect detection is extremely primitive. I don't actually know how browsers do this, but just going by URL seems dangerous, what if the session state has changed?

@myronmarston
Copy link
Contributor Author

Hmmm, this is definitely a thornier problem than I first thought. I did some research and ran across RFC 2068, which, in section 10.3, states:

A user agent SHOULD NOT automatically redirect a request more than 5 times,
since such redirections usually indicate an  infinite loop.

So you'd have warrant for raising an infinite redirect error after 5 redirects. You could probably beef it up a bit and make it 5 redirects to the same URL, or you could make it configurable, so that apps that really need capybara to redirect 15 times (for whatever reason) could configure it to allow that.

@daveyeu
Copy link

daveyeu commented Jun 24, 2010

We've also been bitten by this, exceeding the 4 second timeout on requests that simply timed out in there. Swapping out the 4 with Capybara.default_wait_time was a small improvement.

@jnicklas
Copy link
Collaborator

Redirect a maximum of 5 times instead of time limit, closed by 2e6745e

This is as per http://www.ietf.org/rfc/rfc2068.txt

"A user agent SHOULD NOT automatically redirect a request
more than 5 times, since such redirections usually indicate an
infinite loop."

@myronmarston
Copy link
Contributor Author

FWIW, Chrome, Safari and Firefox all allow more than 5 redirects, in spite of the RFC. I realized that the /redirect/:times/times route can easily be used to test the browser limits just by visiting /redirect/100/times.

  • Chrome follows 21 redirects.
  • Safari follows 16 redirects.
  • Firefox follows 20 redirects.

I haven't tested it on other browsers since I don't have any others installed.

I think the limit of 5 is fine (IMHO, you're building your app wrong if it ever requires more than 5 redirects to work properly), but I thought it might be useful to document the browser behavior for if/when this issue is discussed further in the future.

@jnicklas
Copy link
Collaborator

How do you think we should document this?

Maybe a more descriptive error message, as in "Capybara only allows five consecutive redirects" or something?

@myronmarston
Copy link
Contributor Author

I don't know...I just put it here since I figured anyone having issues with the infinite redirect detection would probably search and find this ticket. If you think it's appropriate to add comments to the code containing this info, feel free.

@myronmarston
Copy link
Contributor Author

Yeah, the error message would be nice.

mcmire pushed a commit that referenced this issue Apr 18, 2011
This is as per http://www.ietf.org/rfc/rfc2068.txt

"A user agent SHOULD NOT automatically redirect a request
more than 5 times, since such redirections usually indicate an
infinite loop."
@lock lock bot locked and limited conversation to collaborators Aug 18, 2019
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants