Enhanced basic auth decode #3239

dalazx · 2018-09-04T10:15:24Z

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 · 2018-09-04T12:30:42Z

tests/test_helpers.py

-def test_basic_auth_decode():
-    auth = helpers.BasicAuth.decode('Basic bmtpbTpwd2Q=')
+@pytest.mark.parametrize('header', (
+    'Basic bmtpbTpwd2Q=', 'basic bmtpbTpwd2Q='))


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

webknjaz · 2018-09-04T12:43:42Z

aiohttp/helpers.py

-            ).decode(encoding).partition(':')
+            decoded = base64.b64decode(
+                encoded_credentials.encode('ascii'), validate=True
+            ).decode(encoding)


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

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 :)

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

codecov-io · 2018-09-04T18:26:28Z

Codecov Report

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

@@            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 · 2018-09-04T18:31:20Z

setup.cfg

@@ -34,6 +34,8 @@ branch = True
 source = aiohttp, tests
 omit = site-packages

+[mypy]
+incremental = false


Related mypy issue: python/mypy#5534

asvetlov · 2018-09-05T07:28:12Z

Thanks!

lock · 2019-10-28T02:03:57Z

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

dalazx added 3 commits September 4, 2018 13:04

Enhanced BasicAuth.decode

15221e4

Added corresponding tests

399236e

Added a changes file

afd7c0f

asvetlov approved these changes Sep 4, 2018

View reviewed changes

webknjaz reviewed Sep 4, 2018

View reviewed changes

dalazx mentioned this pull request Sep 4, 2018

Implemented Basic Auth neuro-inc/platform-registry-api#6

Merged

dalazx added 2 commits September 4, 2018 17:26

Fixed mypy flakiness

5ffadc2

Fixed mypy errors

dad0d63

asvetlov reviewed Sep 4, 2018

View reviewed changes

asvetlov approved these changes Sep 5, 2018

View reviewed changes

asvetlov merged commit 9c30840 into aio-libs:master Sep 5, 2018

dalazx deleted the enhanced_basic_auth_decode branch September 5, 2018 12:19

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhanced basic auth decode #3239

Enhanced basic auth decode #3239

dalazx commented Sep 4, 2018 •

edited by webknjaz

Loading

webknjaz Sep 4, 2018

webknjaz Sep 4, 2018

asvetlov Sep 4, 2018

dalazx Sep 4, 2018

codecov-io commented Sep 4, 2018 •

edited

Loading

asvetlov Sep 4, 2018

asvetlov commented Sep 5, 2018

lock bot commented Oct 28, 2019

Enhanced basic auth decode #3239

Enhanced basic auth decode #3239

Conversation

dalazx commented Sep 4, 2018 • edited by webknjaz Loading

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

webknjaz Sep 4, 2018

Choose a reason for hiding this comment

webknjaz Sep 4, 2018

Choose a reason for hiding this comment

asvetlov Sep 4, 2018

Choose a reason for hiding this comment

dalazx Sep 4, 2018

Choose a reason for hiding this comment

codecov-io commented Sep 4, 2018 • edited Loading

Codecov Report

asvetlov Sep 4, 2018

Choose a reason for hiding this comment

asvetlov commented Sep 5, 2018

lock bot commented Oct 28, 2019

dalazx commented Sep 4, 2018 •

edited by webknjaz

Loading

codecov-io commented Sep 4, 2018 •

edited

Loading