Skip to content

Conversation

@sethmlarson
Copy link
Member

This will fix our issues with the source address error tests and Python 3.7.4 exploding when we include , \r, or \n in the request target.

cc @pquentin

@sethmlarson sethmlarson force-pushed the percent-encode-targets branch 3 times, most recently from 77134dd to e7d53c1 Compare August 29, 2019 01:22
@sethmlarson sethmlarson force-pushed the percent-encode-targets branch from e7d53c1 to fd8a95a Compare August 29, 2019 02:45
@pquentin
Copy link
Member

Thanks! Do you know why the new test is failing?

@codecov-io
Copy link

codecov-io commented Sep 19, 2019

Codecov Report

Merging #1673 into master will decrease coverage by 0.29%.
The diff coverage is 87.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1673     +/-   ##
=========================================
- Coverage   99.49%   99.19%   -0.3%     
=========================================
  Files          22       22             
  Lines        1974     1990     +16     
=========================================
+ Hits         1964     1974     +10     
- Misses         10       16      +6
Impacted Files Coverage Δ
src/urllib3/connectionpool.py 100% <100%> (ø) ⬆️
src/urllib3/util/url.py 97.93% <84.61%> (-1.51%) ⬇️
src/urllib3/connection.py 92.36% <0%> (-1.39%) ⬇️
src/urllib3/response.py 99.72% <0%> (-0.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67715fd...c9d40f5. Read the comment docs.

@sethmlarson sethmlarson force-pushed the percent-encode-targets branch 3 times, most recently from bf3b39d to 888fd6b Compare September 19, 2019 12:08
@sethmlarson sethmlarson force-pushed the percent-encode-targets branch from 888fd6b to c9d40f5 Compare September 19, 2019 12:12
@sethmlarson sethmlarson merged commit 26cfb9a into urllib3:master Sep 19, 2019
jeblair pushed a commit to jeblair/urllib3 that referenced this pull request Sep 19, 2019
Fixes urllib3#1683.

Recently merged urllib3#1673 ensured that URLs are percent-encoded.
This is a behavior change in that URLs with tilde characters in
them would not previously have been percent-encoded, but are now.

RFC 3986 says[1]:

   For consistency, percent-encoded octets in the ranges of ALPHA
   (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E),
   underscore (%5F), or tilde (%7E) should not be created by URI
   producers and, when found in a URI, should be decoded to their
   corresponding unreserved characters by URI normalizers.

This suggests that urllib3 should not escape tilde characters
in URLs.  The RFC describes tilde as an "unreserved" character.
Among the character classes at the top of url.py, the closest
match to the "unreserved" set seems to be ZONE_ID_CHARS.  RFC6874
says:

    A <zone_id> SHOULD contain only ASCII characters classified as
    "unreserved" for use in URIs [RFC3986].

Which suggests that it should be safe to add to that set.

[1] https://tools.ietf.org/html/rfc3986#section-2.3
[2] https://tools.ietf.org/html/rfc6874#section-2
@sethmlarson sethmlarson deleted the percent-encode-targets branch November 10, 2019 04:24
Dobatymo pushed a commit to Dobatymo/urllib3 that referenced this pull request Mar 16, 2022
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.

3 participants