Raise ValueError if BasicAuth login has a ":"#1307
Raise ValueError if BasicAuth login has a ":"#1307asvetlov merged 1 commit intoaio-libs:masterfrom kawadia:no_colon_in_basicauth
Conversation
Current coverage is 98.50% (diff: 100%)@@ master #1307 diff @@
==========================================
Files 29 29
Lines 6497 6499 +2
Methods 0 0
Messages 0 0
Branches 1090 1091 +1
==========================================
+ Hits 6400 6402 +2
Misses 47 47
Partials 50 50
|
|
Thanks! |
|
|
||
| if ':' in login: | ||
| raise ValueError( | ||
| 'A ":" is not allowed in login (RFC 1945#section-11.1)') |
There was a problem hiding this comment.
Sorry so being late for the party, but this is not very correct RFC reference since:
- it about HTTP 1/0
- there it disallows non-latin usernames, while we don't.
Here is a better one: https://tools.ietf.org/html/rfc2617#section-2
There was a problem hiding this comment.
Mean while newly https://tools.ietf.org/html/rfc7617#page-3 allows any characters for user/pass except control ones. And the colon : is allowed, but needs to be escaped.
There was a problem hiding this comment.
Thanks.
I'l use yarl.quote for both parts separately.
There was a problem hiding this comment.
Hmm, after re-reading RFC 7617 I've found:
The user-id and password MUST NOT contain any control characters (see
"CTL" in Appendix B.1 of [RFC5234]).Furthermore, a user-id containing a colon character is invalid, as the first colon in a user-pass string separates user-id and password from one another; text after the first colon is part of the password.
User-ids containing colons cannot be encoded in user-pass strings.
Looks like the PR is correct
There was a problem hiding this comment.
There is nothing about user-id encoding like percent-quoting etc.
There was a problem hiding this comment.
That's strange, because without percent-quoting you're not able to use non-ascii names and passwords what is quite awkward today. And since you actually can have them, quoting colon character doesn't breaks the parser while it's quoted.
There was a problem hiding this comment.
No, translation is base64encode(user+':'+password).
Base64 converts utf8 strings into ascii looselessly.
But colon in user is forbidden.
4 participants
What do these changes do?
Improves BasicAuth by raising ValueError if BasicAuth login has a ":". A colon is not allowed in login/username per RFC 1945#section-11.1.
Are there changes in behavior for the user?
Yes. Current BasicAuth would silently fail to authenticate if login has a ":". This change would would instead raise a clear error to the user.
Related issue number
Checklist
CONTRIBUTORS.txtCHANGES.rst#isuue_numberformat at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.