Restrict URL preparation to HTTP/HTTPS #3713
Conversation
|
http+unix is used in Docker, Custodia and SSSD, e.g. https://github.com/latchset/custodia/blob/master/custodia/client.py#L60 with URI like |
|
This looks entirely reasonable to me, thanks @tiran! |
|
One note though: can we get a test for this? |
| if ':' in url and not url.lower().startswith('http'): | ||
| # `data`, `http+unix` etc to work around exceptions from `url_parse`, | ||
| # which handles RFC 3986 only. | ||
| if ':' in url and not url.lower().startswith(('http://', 'https://')): |
There was a problem hiding this comment.
Didn't we have this previously? Can we check the history and see if we can find why we switched to what we have today?
There was a problem hiding this comment.
Nope. This is written as it was introduced in #1717, so I'm clearly thinking of something else.
tiran
force-pushed
the
strict_http_protocol_check
branch
from
40d7cfa to
f5b1264
Compare
| ) | ||
| ) | ||
|
|
||
| def test_prepare_http_unix(self, url): |
There was a problem hiding this comment.
So this is good, let's just make this a bit more tempting for people to extend in future by changing the name: test_url_passthrough.
| if ':' in url and not url.lower().startswith('http'): | ||
| # `data`, `http+unix` etc to work around exceptions from `url_parse`, | ||
| # which handles RFC 3986 only. | ||
| if ':' in url and not url.lower().startswith(('http://', 'https://')): |
There was a problem hiding this comment.
I think we can remove the if ':' in url portion of the conditional now since that's covered in the new startswith tuple.
There was a problem hiding this comment.
I tried it but got a test failure in the test case MissingSchema, 'hiwpefhipowhefopw'.
There was a problem hiding this comment.
Yup, it's needed: it exists to allow for things that don't look to us like URLs at all but still might be handle-able by something down in the stack.
Requests treats all URLs starting with the string 'http' as HTTP URLs. Preparation with IDNA breaks non-standard URIs like http+unix. Requests now prepares only URLs with prefix http:// and https://. Signed-off-by: Christian Heimes <[email protected]>
tiran
force-pushed
the
strict_http_protocol_check
branch
from
f5b1264 to
34af72c
Compare
|
Cool, thanks @tiran! All tests pass, so it seems like it's good to go. Thanks so much! ✨ 🍰 ✨ |
Requests treats all URLs starting with the string 'http' as HTTP URLs.
Preparation with IDNA breaks non-standard URIs like http+unix. Requests
now prepares only URLs with prefix http:// and https://.
Signed-off-by: Christian Heimes [email protected]