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.

Sign up for GitHub

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

Restrict URL preparation to HTTP/HTTPS #3713

Merged
merged 1 commit into from
Nov 21, 2016
Merged

Restrict URL preparation to HTTP/HTTPS #3713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions requests/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,9 +347,9 @@ def prepare_url(self, url, params):
url = url.lstrip()

# Don't do any URL preparation for non-HTTP schemes like `mailto`,
# `data` etc to work around exceptions from `url_parse`, which
# handles RFC 3986 only.
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://')):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. This is written as it was introduced in #1717, so I'm clearly thinking of something else.

Copy link
Member

@nateprewitt nateprewitt Nov 21, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove the if ':' in url portion of the conditional now since that's covered in the new startswith tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it but got a test failure in the test case MissingSchema, 'hiwpefhipowhefopw'.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, oversight on my part.

self.url = url
return

Expand Down
17 changes: 17 additions & 0 deletions tests/test_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2138,3 +2138,20 @@ def test_preparing_bad_url(self, url):
r = requests.Request('GET', url=url)
with pytest.raises(requests.exceptions.InvalidURL):
r.prepare()

@pytest.mark.parametrize(
'protocol, url',
(
("http+unix://", b"http+unix://%2Fvar%2Frun%2Fsocket/path"),
("http+unix://", u"http+unix://%2Fvar%2Frun%2Fsocket/path"),
("mailto", b"mailto:[email protected]"),
("mailto", u"mailto:[email protected]"),
("data", b"data:SSDimaUgUHl0aG9uIQ=="),
)
)
def test_url_passthrough(self, protocol, url):
session = requests.Session()
session.mount(protocol, HTTPAdapter())
p = requests.Request('GET', url=url)
p.prepare()
assert p.url == url