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

Always encode forward slashes as %2F in query parameters #2723

Merged
merged 4 commits into from Jun 9, 2023

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented May 26, 2023

Summary

Closes #2721

Encodes / as %2F in query parameters for more robust behavior matching browser implementations. See issue for discussion.

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

httpx/_urlparse.py Outdated Show resolved Hide resolved
This is expected to fail tests due to double escaping
@tomchristie
Copy link
Member

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 safe="/" value...

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])

@zanieb
Copy link
Contributor Author

zanieb commented Jun 8, 2023

Thanks for the hint! I'm pretty confused how it manifested that way still :D

@zanieb zanieb marked this pull request as ready for review June 8, 2023 23:57
@tomchristie tomchristie merged commit 920333e into master Jun 9, 2023
5 checks passed
@tomchristie tomchristie deleted the encode-forslash branch June 9, 2023 09:06
@trim21 trim21 mentioned this pull request Aug 1, 2023
@ttys0dev
Copy link

ttys0dev commented Oct 7, 2023

This needs to be reverted, see this comment and #2883.

@zanieb
Copy link
Contributor Author

zanieb commented Oct 7, 2023

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.

@ttys0dev
Copy link

ttys0dev commented Oct 7, 2023

existing discussion

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 + ":?[]@")
Copy link

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.

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.

Use percent-encoding for / in query params, for better robustness.
4 participants