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

Raise ValueError if BasicAuth login has a ":" #1307

Merged
merged 1 commit into from
Oct 13, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -231,3 +231,5 @@ CHANGES
domains (BACKWARD INCOMPATIBLE) #1125

- Support binary Content-Transfer-Encoding #1169

- Raise ValueError if BasicAuth login has a ":"
1 change: 1 addition & 0 deletions CONTRIBUTORS.txt
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,4 @@ Yusuke Tsutsumi
Семён Марьясин
Pau Freixes
Alexey Firsov
Vikas Kawadia
4 changes: 4 additions & 0 deletions aiohttp/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry so being late for the party, but this is not very correct RFC reference since:

  1. it about HTTP 1/0
  2. there it disallows non-latin usernames, while we don't.
    Here is a better one: https://tools.ietf.org/html/rfc2617#section-2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.
I'l use yarl.quote for both parts separately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is nothing about user-id encoding like percent-quoting etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, translation is base64encode(user+':'+password).
Base64 converts utf8 strings into ascii looselessly.
But colon in user is forbidden.


return super().__new__(cls, login, password, encoding)

@classmethod
Expand Down
5 changes: 5 additions & 0 deletions tests/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ def test_basic_auth2():
helpers.BasicAuth('nkim', None)


def test_basic_with_auth_colon_in_login():
with pytest.raises(ValueError):
helpers.BasicAuth('nkim:1', 'pwd')


def test_basic_auth3():
auth = helpers.BasicAuth('nkim')
assert auth.login == 'nkim'
Expand Down