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

Use percent-encoding for / in query params, for better robustness. #2721

Closed
marban opened this issue May 25, 2023 Discussed in #2661 · 4 comments · Fixed by #2723
Closed

Use percent-encoding for / in query params, for better robustness. #2721

marban opened this issue May 25, 2023 Discussed in #2661 · 4 comments · Fixed by #2723
Labels
enhancement New feature or request

Comments

@marban
Copy link

marban commented May 25, 2023

Discussed in #2661

Originally posted by marban April 15, 2023
Haven't gotten into the details but by upgrading to 0.24 and using a regular dict as GET params that contains e.g. an url like {"q": "http://example.com"} will somehow break the request to, in this case, Twitter's API. If if urllib.parse.quote_plus the "http://example.com", everything works fine (The slash is the culprit).
If I urlencode the whole dict manually before passing it to httpx and call the r.url after the request the / is again not encoded, although it previously was.
https://api.twitter.com/1.1/search/tweets.json?q=url%3Ahttp%3A//google.com

I assume it has to do with

  • Query parameter encoding switches from using + for spaces and %2F for forward slash, to instead using %20 for spaces and treating forward slash as a safe, unescaped character. This differs from requests, but is in line with browser behavior in Chrome, Safari, and Firefox. Both options are RFC valid. (Use '%20' for encoding spaces in query parameters. #2543)
@tomchristie
Copy link
Member

tomchristie commented May 26, 2023

Let me nudge you in the right direction here...

None if query is None else quote(query, safe=SUB_DELIMS + ":/?[]@")

Also we don't want to make it optional, we just want to change the behaviour.

Evidently percent-encoding / in query params might not be spec-neccessary, but it is more robust behaviour.

@tomchristie tomchristie changed the title Make forward slash encoding optional (to make it compatible with Twitter's API) Use percent-encoding for / in query params, for better robustness. May 26, 2023
@tomchristie tomchristie added the enhancement New feature or request label May 26, 2023
@BjoernPetersen
Copy link

@tomchristie just so you know: this broke more than the Twitter API. I have a project in which I want to request a signed URL to an image (from the OpenAI API) using httpx. Some signature query params contain forward slashes, and the server evidently doesn't accept it if those are encoded as %2F.

I want to note that the URL works in Firefox, Chrome, and using requests.get(image_url), but httpx's encoding breaks it.

It might be true that encoding the slashes is technically "more correct", but that doesn't really help if the server just won't accept it.

@ttys0dev
Copy link

ttys0dev commented Oct 7, 2023

Some signature query params contain forward slashes, and the server evidently doesn't accept it if those are encoded as %2F.

Yeah, hit this as well in #2883 when migrating from requests to httpx, unconditionally precent-encoding /'s is clearly not the correct behavior IMO if servers treat it differently.

@alexmaurizio
Copy link

Jumping in to signal we also had to revert httpx from 0.25.0 to 0.24.1 due to this PR.
This change is causing issues with double encoding and a whole lot of services.

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

Successfully merging a pull request may close this issue.

5 participants