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.

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

Finish PR 3732: Handle url encoded credentials #4393

Merged
merged 2 commits into from
Apr 1, 2017

Conversation

qdamian
Copy link
Contributor

@qdamian qdamian commented Mar 31, 2017

Please consider this PR that closes #3236.

It's based on mjwillson's fix, but attempts to address Ivoz's comments in PR #3732.

Thanks!

@dstufft
Copy link
Member

dstufft commented Apr 1, 2017

This code looks good to me, though I'd love to get @Lukasa or @sigmavirus24 to take a quick look at it to make sure this makes sense from a HTTP perspective.

@dstufft dstufft added this to the 10.0 milestone Apr 1, 2017
@Lukasa
Copy link
Contributor

Lukasa commented Apr 1, 2017

So this is in principle fine, but bumps into the kind of problem that Requests bumps into a lot: how do you tell the difference between urlencoded credentials and raw credentials that happen to contain valid urlencoded strings? Like...what if the password actually contains the characters %23 in that order?

This is a difficult thing to handle, and consistency is generally valuable, but this change can (and probably will) break someone if we're not careful.

@dstufft
Copy link
Member

dstufft commented Apr 1, 2017

@Lukasa Ok thanks. I think the answer here is probably just, this is an URL so you need to urlquote your stuff, if your password contains %23 you'll need to escape it.

@dstufft dstufft merged commit 2e534b0 into pypa:master Apr 1, 2017
@sigmavirus24
Copy link
Member

Alternate suggestion, @dstufft, you're already using rfc3986. v1.0 will have a URIBuilder object and if you have users specify credentials separately then you can use that to ensure the credentials are properly encoded.

@qdamian qdamian deleted the handle-url-encoded-credentials branch April 2, 2017 01:10
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Failure to authenticate private repository when URL-encoded character in password
4 participants