Skip to content
This repository was archived by the owner on Jan 13, 2021. It is now read-only.

Conversation

@KostyaEsmukov
Copy link
Contributor

As per https://tools.ietf.org/html/rfc3986#section-3.2.1

userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )

Where, according to https://tools.ietf.org/html/rfc3986#section-2.2 ,

sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

The sub-delims set of chars was added in this PR.

--

(a bit unrelated)

Hyper doesn't handle userinfo to automatically add Basic Auth header to requests. If host (or proxy_host) of connection contains userinfo, it's getting stripped out in the to_host_port_tuple function.

I feel it's not good to quietly eat up auth part without actually using it. Maybe at least we should issue a warning in this case?

As per https://tools.ietf.org/html/rfc3986#section-3.2.1
userinfo    = *( unreserved / pct-encoded / sub-delims / ":" )

Where, according to https://tools.ietf.org/html/rfc3986#section-2.2 ,
sub-delims  = "!" / "$" / "&" / "'" / "(" / ")"
                  / "*" / "+" / "," / ";" / "="

The sub-delims set of chars was added in this commit
@KostyaEsmukov
Copy link
Contributor Author

KostyaEsmukov commented Jul 2, 2017

How come first Travis build haven't failed? 😄 Maybe because I've pushed a fix too fast.

That test fails for me locally:

    def test_connections_can_parse_proxy_hosts_with_userinfo(self):
        c = HTTP20Connection('www.google.com',
>                            proxy_host='azAz09!==:fakepaswd@localhost:8443')

test/test_hyper.py:72: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
hyper/http20/connection.py:127: in __init__
    proxy_host, default_port=8080
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

host_port_str = 'azAz09!==:fakepaswd@localhost:8443', default_port = 8080

    def to_host_port_tuple(host_port_str, default_port=80):
        """
        Converts the given string containing a host and possibly a port
        to a tuple.
        """
        uri = URIReference(
            scheme=None,
            authority=host_port_str,
            path=None,
            query=None,
            fragment=None
        )
    
>       host = uri.host.strip('[]')
E       AttributeError: 'NoneType' object has no attribute 'strip'

hyper/common/util.py:48: AttributeError

It happens because userinfo part in this test contains ! and = chars, which weren't present in the SUBAUTHORITY_MATCHER regex, thus producing None as match

@Lukasa
Copy link
Member

Lukasa commented Jul 2, 2017

Thanks for this! However, RFC3986 is a third-party library, not part of this code, so we'd need to fix it in that repo. Sorry!

@Lukasa Lukasa closed this Jul 2, 2017
@KostyaEsmukov
Copy link
Contributor Author

Oh, right, I guess it's this one: https://github.com/sigmavirus24/rfc3986
No problem, I'll open a PR there then.

BTW, what do you think about issuing a warning when stripping out the userinfo part?

@Lukasa
Copy link
Member

Lukasa commented Jul 2, 2017

That's the one. 😀

As to the warning, I'm not sure. Try pushing a branch that does it and I'll see what it looks like.

@KostyaEsmukov
Copy link
Contributor Author

This PR has been merged in python-hyper/rfc3986#26

What are the following steps? Should I make a PR updating the vendored library?
Or maybe we could wait until this fix is published to the PyPI and unvendor this library? (It was vendored in #172 , but I don't understand why, instead of requiring)

@Lukasa
Copy link
Member

Lukasa commented Jul 9, 2017

I'm open to going either direction. The easiest thing to do, probably, is ask for a PyPI release and then unvendor.

@KostyaEsmukov
Copy link
Contributor Author

Great, let's go with unvendoring then. I'll open a PR after PyPI release

@KostyaEsmukov KostyaEsmukov mentioned this pull request Jul 21, 2017
@KostyaEsmukov KostyaEsmukov deleted the pr/userinfo_allowed_chars branch July 24, 2017 07:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants