Skip to content

Enhanced basic auth decode#3239

Merged
asvetlov merged 5 commits intoaio-libs:masterfrom
dalazx:enhanced_basic_auth_decode
Sep 5, 2018
Merged

Enhanced basic auth decode#3239
asvetlov merged 5 commits intoaio-libs:masterfrom
dalazx:enhanced_basic_auth_decode

Conversation

@dalazx
Copy link
Copy Markdown
Contributor

@dalazx dalazx commented Sep 4, 2018 •
edited by webknjaz
Loading

What do these changes do?

These changes prevent passing illegal chars in the base64 payload.
It was possible to use Authorization: Basic ??? to get BasicAuth(login='', password='') without exceptions.
Also, the related RFC https://www.ietf.org/rfc/rfc2617.txt allows the username and password to be blank, but the colon must be present.

      credentials = "Basic" basic-credentials
      basic-credentials = base64-user-pass
      base64-user-pass  = <base64 [4] encoding of user-pass, except not limited to 76 char/line>
      user-pass   = userid ":" password
      userid      = *<TEXT excluding ":">
      password    = *TEXT

and

curl -vv -u '' example.com
Authorization: Basic Og==  # which is just ":"

Are there changes in behavior for the user?

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

webknjaz
webknjaz reviewed Sep 4, 2018
View reviewed changes
tests/test_helpers.py Outdated
def test_basic_auth_decode():
auth = helpers.BasicAuth.decode('Basic bmtpbTpwd2Q=')
@pytest.mark.parametrize('header', (
'Basic bmtpbTpwd2Q=', 'basic bmtpbTpwd2Q='))
Copy link
Copy Markdown
Member

@webknjaz webknjaz Sep 4, 2018

Choose a reason for hiding this comment

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

Nitpick: if placed one under the other, sequence items are better readable.

webknjaz
webknjaz reviewed Sep 4, 2018
View reviewed changes
aiohttp/helpers.py
).decode(encoding).partition(':')
decoded = base64.b64decode(
encoded_credentials.encode('ascii'), validate=True
).decode(encoding)
Copy link
Copy Markdown
Member

@webknjaz webknjaz Sep 4, 2018

Choose a reason for hiding this comment

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

Not sure whether it's applicable to current PR, but JFYI some browsers (Firefox) don't encode some of UTF-8 bytes correctly, assuming ISO-8859-1 for unicode input and loosely encodes that input, which cuts some bytes in two-byte encoded chars (try entering €öäü in browser and see what you receive in server), which results in error during encoding since it cannot understand byte sequence when reaches those characters. Ref cherrypy/cherrypy#1680

Copy link
Copy Markdown
Member

@asvetlov asvetlov Sep 4, 2018

Choose a reason for hiding this comment

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

Thanks for the reminder.
We discussed it 2 or 3 years ago and decided to do nothing until users report.
There is no blame yet :)

Copy link
Copy Markdown
Contributor Author

@dalazx dalazx Sep 4, 2018

Choose a reason for hiding this comment

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

thanks @webknjaz. noted, but it seems to be out of scope of this PR though.

@dalazx dalazx mentioned this pull request Sep 4, 2018
Merged
@codecov-io
Copy link
Copy Markdown

codecov-io commented Sep 4, 2018 •
edited
Loading

Codecov Report

Merging #3239 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3239      +/-   ##
==========================================
- Coverage   98.09%   98.05%   -0.04%     
==========================================
  Files          43       43              
  Lines        7856     7871      +15     
  Branches     1353     1354       +1     
==========================================
+ Hits         7706     7718      +12     
- Misses         58       60       +2     
- Partials       92       93       +1
Impacted Files Coverage Δ
aiohttp/helpers.py 97.51% <100%> (+0.02%) ⬆️
aiohttp/tcp_helpers.py 90% <0%> (-6.67%) ⬇️
aiohttp/client_reqrep.py 97.47% <0%> (-0.17%) ⬇️
aiohttp/web_app.py 98.39% <0%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a0359f...dad0d63. Read the comment docs.

asvetlov
asvetlov reviewed Sep 4, 2018
View reviewed changes
setup.cfg
omit = site-packages

[mypy]
incremental = false
Copy link
Copy Markdown
Member

@asvetlov asvetlov Sep 4, 2018

Choose a reason for hiding this comment

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

Related mypy issue: python/mypy#5534

@asvetlov asvetlov merged commit 9c30840 into aio-libs:master Sep 5, 2018
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Sep 5, 2018

Thanks!

@dalazx dalazx deleted the enhanced_basic_auth_decode branch September 5, 2018 12:19
@lock
Copy link
Copy Markdown

lock bot commented Oct 28, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@webknjaz webknjaz webknjaz left review comments

@asvetlov asvetlov asvetlov approved these changes

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@dalazx @codecov-io @asvetlov @webknjaz