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

fixed userinfo regex for @ character in userinfo #59

Closed
wants to merge 1 commit into from

Conversation

cansarigol
Copy link

@cansarigol cansarigol commented Sep 9, 2019

USERINFO_RE regex pattern separates URL by first @ character. I changed this pattern to evaluate the last @ character so that can use an email address for userinfo.username (e.g. "https://[email protected]:[email protected]")

@cansarigol
Copy link
Author

HI @sethmlarson could you review this pr.

@sigmavirus24
Copy link
Collaborator

There's no description for why this is important. All it says is that it "Fixed" something which is correct per the ABNF.

@cansarigol
Copy link
Author

This fix about using email address for userinfo.username
e.g. "https://[email protected]:[email protected]". Email address is quite used as a username. But using in url I'm not sure. Hence I don't know how important it is.

in Httpx project we had a problem with this (issue328) and as per ticket, @sethmlarson wrote that we can fix rfc3986. Because of that, I wanted to push a commit to fix this problem.

As far as I understood, USERINFO_RE regex pattern separates URL by first @ character. I changed this pattern to evaluate the last @ character.

@sigmavirus24
Copy link
Collaborator

That's all great context that belongs in the commit message and pull request description.

@oczkers
Copy link

oczkers commented Apr 28, 2020

Can we get it merged?

@sigmavirus24
Copy link
Collaborator

This is still not compliant with the RFC. The @ in the email must be encoded for it to be passed correctly

@tomchristie tomchristie closed this May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants