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

do not count original request for RedirectLoop protection #801

Closed
wants to merge 1 commit into from

Conversation

oczkers
Copy link
Contributor

@oczkers oczkers commented Feb 2, 2020

There are many situations when servers redirects to other resource and back so we cannot be so strict here. For example:
https://some.url/protected -> 302 your login token has expired so you have to refresh it
https://some.url/refreshToken -> 302 new token is set in cookies, redirecting back to original url
https://some.url/protected -> 200, request complete

I know this isn't good approach and servers probably should be doing this without asking client/browser but this is life and currently there is no way to use httpx for this kind of sites.

@tomchristie
Copy link
Member

Okay, so the first thing to check with this kind of change is prior art...

  • How does requests handle these cases?
  • How does urllib3 handle these cases (potentially different since requests doesn’t use urllib3’s built-in redirect behaviour)
  • How does curl handle these cases?

@oczkers
Copy link
Contributor Author

oczkers commented Feb 2, 2020

urllib3: By default max 3 retries, max 3 redirects, no url tracking.
requests: By default max 0 retries, max 30 redirects, no url tracking.
curl: By default 0 retries, 0 redirects

None of those (not sure about curl, haven't analyzed code) implemented this kind of protection, they simply rely on number of redirects like browsers do.

@tomchristie
Copy link
Member

In that case I'd probably suggest we instead just drop our RedirectLoop checking completely, and adopt the same behaviour.

@oczkers
Copy link
Contributor Author

oczkers commented Feb 4, 2020

You're probably right and this can be problematic with really wrong designed websites but considering that no one yet reported any we might keep it until someone finds one. I really like this idea, might be helpful tracking mistakes :-).

@oczkers
Copy link
Contributor Author

oczkers commented Feb 19, 2020

I force pushed because forgot about Client, now it's changed in both async and sync client.

@tomchristie
Copy link
Member

Given that neither urllib3 nor requests implement redirect loop checking I don't think we should either, at least at this point in time. The change here looks reasonable, but I reckon we should instead just drop the RedirectLoop exception and check altogether.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants