Raise ValueError if BasicAuth login has a ":" #1307
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! |
| @@ -47,6 +47,10 @@ def __new__(cls, login, password='', encoding='latin1'): | |||
| if password is None: | |||
| raise ValueError('None is not allowed as password value') | |||
|
|
|||
| 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.
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.