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

fix #2247 #2249

Merged
merged 2 commits into from Oct 5, 2014

Conversation

Projects
None yet
5 participants
@t-8ch
Contributor

t-8ch commented Sep 25, 2014

We have to pass urllib3 the url without the authentication information,
else it will be parsed by httplib as a netloc and included in the request line
and Host header

fix #2247
We have to pass urllib3 the url without the authentication information,
else it will be parsed by httplib as a netloc and included in the request line
and Host header
@Lukasa

This comment has been minimized.

Show comment
Hide comment
@Lukasa

Lukasa Sep 25, 2014

Member

This looks right, but it's hard for me to know because URLs are a crazy mess. I'll let @sigmavirus24 review this, he knows URLs better than I do.

Member

Lukasa commented Sep 25, 2014

This looks right, but it's hard for me to know because URLs are a crazy mess. I'll let @sigmavirus24 review this, he knows URLs better than I do.

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Sep 26, 2014

Member

So if we used a better (read: less forgiving) URL parser library we wouldn't need to split on @ (like we do here). In principle this works fine. In reality, we should get something like rfc3986 into acceptable shape for @shazow or just make the Url object in urllib3 more RFC compliant (whichever works better) and use that for parsing the URL and reconstructing it. In short, you have 5 major components of the URL:

{scheme}://{authority}{/path}{?query}{#fragment}

And authority which we're dealing with right now has 3 sub-components so a URL would look like:

{scheme}://{userinfo@}{hostname}{:port}{/path}{?query}{#fragment}

rfc3986 would split this up and allow you to replace userinfo with None and then reconstruct the URL. For a quick fix, this is great. I'd rather not have so much URL/URI parsing logic in requests though, this is an HTTP library not an HTTP + URL + ... library.

Member

sigmavirus24 commented Sep 26, 2014

So if we used a better (read: less forgiving) URL parser library we wouldn't need to split on @ (like we do here). In principle this works fine. In reality, we should get something like rfc3986 into acceptable shape for @shazow or just make the Url object in urllib3 more RFC compliant (whichever works better) and use that for parsing the URL and reconstructing it. In short, you have 5 major components of the URL:

{scheme}://{authority}{/path}{?query}{#fragment}

And authority which we're dealing with right now has 3 sub-components so a URL would look like:

{scheme}://{userinfo@}{hostname}{:port}{/path}{?query}{#fragment}

rfc3986 would split this up and allow you to replace userinfo with None and then reconstruct the URL. For a quick fix, this is great. I'd rather not have so much URL/URI parsing logic in requests though, this is an HTTP library not an HTTP + URL + ... library.

Show outdated Hide outdated requests/adapters.py
@@ -17,7 +17,7 @@
from .packages.urllib3.util import Timeout as TimeoutSauce
from .compat import urlparse, basestring, urldefrag

This comment has been minimized.

@sigmavirus24

sigmavirus24 Sep 26, 2014

Member

Please remove the import of urldefrag here.

@sigmavirus24

sigmavirus24 Sep 26, 2014

Member

Please remove the import of urldefrag here.

This comment has been minimized.

@t-8ch

t-8ch Sep 26, 2014

Contributor
@t-8ch
@t-8ch

This comment has been minimized.

Show comment
Hide comment
@t-8ch

t-8ch Sep 26, 2014

Contributor

+∞. I think it would be best to have URL objects that are immutable and an API like URL.replace(userinfo=None, fragment=None)

Contributor

t-8ch commented Sep 26, 2014

+∞. I think it would be best to have URL objects that are immutable and an API like URL.replace(userinfo=None, fragment=None)

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Sep 26, 2014

Member

@t-8ch that's roughly rfc3986's API, except that I don't think I call it replace but maybe I do.... I'll double check later.

Member

sigmavirus24 commented Sep 26, 2014

@t-8ch that's roughly rfc3986's API, except that I don't think I call it replace but maybe I do.... I'll double check later.

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Sep 26, 2014

Contributor

@sigmavirus24 Please open the specific issue with urllib3's url parser, no need to be backhanded. :)

Contributor

shazow commented Sep 26, 2014

@sigmavirus24 Please open the specific issue with urllib3's url parser, no need to be backhanded. :)

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24

sigmavirus24 Sep 26, 2014

Member

Sorry @shazow, it wasn't meant to be back handed. I'll pull together the list of things the object is missing and make an issue with it tonight.

Member

sigmavirus24 commented Sep 26, 2014

Sorry @shazow, it wasn't meant to be back handed. I'll pull together the list of things the object is missing and make an issue with it tonight.

@shazow

This comment has been minimized.

Show comment
Hide comment
@shazow

shazow Sep 26, 2014

Contributor

Thanks. :)

Contributor

shazow commented Sep 26, 2014

Thanks. :)

@kennethreitz

This comment has been minimized.

Show comment
Hide comment
@kennethreitz

kennethreitz Oct 5, 2014

Member

What's the status of this?

Member

kennethreitz commented Oct 5, 2014

What's the status of this?

@sigmavirus24

This comment has been minimized.

Show comment
Hide comment
@sigmavirus24
Member

sigmavirus24 commented Oct 5, 2014

kennethreitz added a commit that referenced this pull request Oct 5, 2014

@kennethreitz kennethreitz merged commit 5850b1f into requests:master Oct 5, 2014

1 check passed

default Merged build finished.
Details

@t-8ch t-8ch deleted the t-8ch:fix_2247 branch Oct 5, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment