-
-
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
Always encode forward slashes as %2F
in query parameters
#2723
Conversation
This is expected to fail tests due to double escaping
Okay, so we also need to change this... def urlencode(items: typing.List[typing.Tuple[str, str]]) -> str:
# We can use a much simpler version of the stdlib urlencode here because
# we don't need to handle a bunch of different typing cases, such as bytes vs str.
#
# https://github.com/python/cpython/blob/b2f7b2ef0b5421e01efb8c7bee2ef95d3bab77eb/Lib/urllib/parse.py#L926
#
# Note that we use '%20' encoding for spaces, and treat '/' as a safe
# character. This means our query params have the same escaping as other
# characters in the URL path. This is slightly different to `requests`,
# but is the behaviour that browsers use.
#
# See https://github.com/encode/httpx/issues/2536 and
# https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlencode
return "&".join([quote(k) + "=" + quote(v) for k, v in items]) To not use the default def urlencode(items: typing.List[typing.Tuple[str, str]]) -> str:
# We can use a much simpler version of the stdlib urlencode here because
# we don't need to handle a bunch of different typing cases, such as bytes vs str.
#
# Note that we use '%20' encoding for spaces, instead of "+".
return "&".join([quote(k, safe="") + "=" + quote(v, safe="") for k, v in items]) |
Thanks for the hint! I'm pretty confused how it manifested that way still :D |
This needs to be reverted, see this comment and #2883. |
Hi @ttys0dev — I appreciate the sentiment but that's not a great way to collaborate with us. We're considering reverting this and the existing discussion is the place to weigh in with details of why this matters to you. |
Oh, I had looked for existing open issues and didn't originally find one tracking this issue yet, I hadn't realized this project often puts issues under discussions(I've never really used the discussions feature before) which is a bit different from other projects. |
parsed_query: typing.Optional[str] = ( | ||
None if query is None else quote(query, safe=SUB_DELIMS + ":/?[]@") | ||
None if query is None else quote(query, safe=SUB_DELIMS + ":?[]@") |
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 change was unnecessary. The fix was the changes below. We dont need to change an already encoded query to encode slashes. We do need to handle it when passed in as separate parameter.
Summary
Closes #2721
Encodes
/
as%2F
in query parameters for more robust behavior matching browser implementations. See issue for discussion.Checklist