-
-
Notifications
You must be signed in to change notification settings - Fork 844
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
Pre encoded path/params/fragments should be kept #2976
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense to me. I stumbled upon this behavior in the /
encoding implementation and was confused.
I don't think this PR fixes #2883. That was about encodes forward slash. |
Hmm. Well not the initial report there indeed. But the the receiver of that url should url decode. Not doing that would be a bug in its parser. It might only fix the followup comments with similar issues. |
@@ -150,6 +150,26 @@ def test_param_with_existing_escape_requires_encoding(): | |||
assert str(url) == "http://webservice?u=http%3A%2F%2Fexample.com%3Fq%3Dfoo%252Fa" | |||
|
|||
|
|||
def test_param_with_existing_escape(): | |||
url = httpx.URL("https://webservice/?u=/%3D%26&v=1%202") | |||
assert str(url) == "https://webservice/?u=%2F%3D%26&v=1%202" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this will work, it's still corrupting the parameter by encoding /
to %2F
which is something not all servers can handle.
Moving to draft n favor off #2980 |
While I think we may want to revert the behavior that encodes |
Superseeded by #2990 |
Summary
URL with pre-encoded segments would end up double encoded when parsed by the URL parser. We must consider the % as a safe value, to avoid encoding it here.
Related to comments in #2883
Related #2723
Duplicate #2929
Checklist