metricq / aiocouch

🛋 An asynchronous client library for CouchDB 2.x and 3.x
https://aiocouch.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
29 stars 10 forks source link

feat: UTF-8 HTTP Basic authentication (RFC-7617) #47

Closed H--o-l closed 2 years ago

H--o-l commented 2 years ago

Username/password encoding in HTTP Basic authentication is broken for non-latin1 char.

UTF-8 Basic authentication is allowed since RFC-7617. Previously, RFC-2616 only allowed to use ISO-8859-1 text which is basically latin1.

CouchDB correctly encodes and decodes credentials using UTF-8: https://github.com/apache/couchdb/blob/0059b8f90e58e10b199a4b768a06a762d12a30d3/dev/pbkdf2.py#L65

aiohttp still uses latin1 as default: https://github.com/aio-libs/aiohttp/blob/72d3d4b1f68cca5ad15ef50bffb0419b798c7f23/aiohttp/helpers.py#L139

aiocouch is made for CouchDB, and CouchDB follows RFC-7617, so I think aiocouch should follow RFC-7617 too.

codecov[bot] commented 2 years ago

Codecov Report

Merging #47 (745d629) into master (56167f8) will not change coverage. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #47   +/-   ##
=======================================
  Coverage   97.13%   97.13%           
=======================================
  Files          13       13           
  Lines         944      944           
=======================================
  Hits          917      917           
  Misses         27       27           
Impacted Files Coverage Δ
aiocouch/remote.py 96.73% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

adrienverge commented 2 years ago

I completely agree with this proposal.

For reference, I made an equivalent pull request on couchdb-python: https://github.com/djc/couchdb-python/issues/301

H--o-l commented 2 years ago

(don't re-review my PR right now I'm working on adding the test)

H--o-l commented 2 years ago

Hi, @bmario thanks for your review and feedback.

I added the test. I wasn't able to use the CouchDB test admin account directly because iamssen/couchdb-github-action doesn't allow to set a custom admin password (changing the CouchDB test admin password was probably be a bad idea anyway).

Thus I created my own aiocouch_test_user_utf8 following the model of aiocouch_test_user but limited to the scope of my test. This user has a UTF-8 password, passwor§.

The added test fail before this PR and pass after it, I verified it locally on my computer.

Can't wait for your feedback.

H--o-l commented 2 years ago

Thank you for your work, but I'm a bit confused now. Isn't the § character part of latin-1 and hence the test shouldn't fail even without the patch? As such, I'd like to see an obviously non-latin1 character, like emoji's.

@bmario Indeed I should have checked latin-1 table directly and pick a char outside it. The § fails with the code before this PR because his encoding is different in latin-1 than in UTF-8, see the following demonstration which use the same code as aiohttp:

import base64
print(base64.b64encode('§'.encode('latin1')).decode('latin1'))  # pw==
print(base64.b64encode('§'.encode('utf-8')).decode('utf-8'))  # wqc=

I updated the PR to use the sofa emoji's 🛋️ (and with the other change you requested).

H--o-l commented 2 years ago

Thanks @bmario! There is a chance this change ends up in a new release not too far from now?