Improve error when redirecting with custom schemes #1002
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jcugat
commented
May 27, 2020
| @@ -322,6 +322,10 @@ def redirect_url(self, request: Request, response: Response) -> URL: | |||
|
|
|||
| url = URL(location, allow_relative=True) | |||
|
|
|||
| # Check that we can handle the scheme | |||
| if url.scheme and url.scheme not in ("http", "https"): | |||
There was a problem hiding this comment.
This pattern ("http", "https") (or something similar) is repeated in a few places, like:
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_client.py#L609
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_models.py#L105
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_models.py#L132
It would probably make sense to unify it in a single place.
There was a problem hiding this comment.
In these kinds of cases, if it's a single-liner I'd often prefer not to introduce indirection. It's as clear as it can possibly be, and it's not a complicated expression.
jcugat
force-pushed
the
redirect-custom-scheme
branch
from
9160631
to
c57f992
Compare
yeraydiazdiaz
approved these changes
May 28, 2020
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #822
I used
Scheme "blah" not supportedas the error message instead ofURL scheme must be "http" or "https".used in other parts of the code. It makes more sense to show the actual invalid scheme since it's coming from the server and not something that the user specified.I also tried the same test with
allow_redirects=Falsebut unfortunately when the response object is built, it needs a port which we can't derive from the custom scheme. Not sure how to solve this without changing the port definition ofUrlto allow aNoneport, ideas?