Support credentials in URL with empty user, http://:password@host (#6494)

[shuckc]

What do these changes do?

Per issue #6494 this brings our handling of credentials in the URL where the username is blank in line with the behavior of curl, requests and perhaps other client libraries.

Are there changes in behavior for the user?

Potentially - although vanishingly remote. One could imagine a client to be providing a password in this style, and for aiohttp to be ignoring it, and for the service to allow anonymous access, but not with a Basic authentication header provided. However this is a hidden misconfiguration, so perhaps we are helping them out.

Related issue number

Fixes #6494

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."

[shuckc]

For the test case added to test_basic_auth_no_user_from_url I ran the same fictitious URL with curl and verified the Basic auth header value matched.

[webknjaz]

Blocking until we can decide if this change is correct. What cURL does is not indicative as it relies on the user/invoker constructing parts of the request properly. And the RFC seems to indicate that having an empty username is incorrect.

This needs more research and we must remain RFC-compliant.

Ref: #6494 (comment)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.25%. Comparing base (1d21dce) to head (6bacea7).
Report is 1050 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6495   +/-   ##
=======================================
  Coverage   98.25%   98.25%           
=======================================
  Files         107      107           
  Lines       34125    34136   +11     
  Branches     4048     4048           
=======================================
+ Hits        33530    33541   +11     
  Misses        421      421           
  Partials      174      174           

Flag	Coverage Δ
CI-GHA	98.15% <100.00%> (+<0.01%)	⬆️
OS-Linux	97.81% <100.00%> (+<0.01%)	⬆️
OS-Windows	96.21% <100.00%> (+<0.01%)	⬆️
OS-macOS	97.48% <100.00%> (+<0.01%)	⬆️
Py-3.10.11	97.58% <100.00%> (+<0.01%)	⬆️
Py-3.10.14	97.51% <100.00%> (+<0.01%)	⬆️
Py-3.11.9	97.74% <100.00%> (+<0.01%)	⬆️
Py-3.12.4	96.01% <100.00%> (+<0.01%)	⬆️
Py-3.12.5	97.54% <100.00%> (+<0.01%)	⬆️
Py-3.9.13	97.47% <100.00%> (+<0.01%)	⬆️
Py-3.9.19	97.41% <100.00%> (+<0.01%)	⬆️
Py-pypy7.3.16	97.02% <100.00%> (+<0.01%)	⬆️
VM-macos	97.48% <100.00%> (+<0.01%)	⬆️
VM-ubuntu	97.81% <100.00%> (+<0.01%)	⬆️
VM-windows	96.21% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[shuckc]

If we look at the collected ABNF for a URL from Appendix-A of the RFC3986 in particular:

authority     = [ userinfo "@" ] host [ ":" port ]
userinfo      = *( unreserved / pct-encoded / sub-delims / ":" )

You can see that :password is permitted as a userinfo value, indeed the use of repeated colons is allowed. However the RFC mentions nothing about how to actually parse the extracted userinfo into what we commonly call user/password, for which reference to other implementations is probably helpful.

The regexp given in Appendex-B of the same document also permits the authority to contain an empty username.

Looking at other relevant RFCs, Basic Auth has to join back together the user/password (recreating the userinfo field) and here they are more specific about that process:

RFC2617 section 2 (Basic Auth) specifies an ABNF that allows zero length usernames (and passwords)

2 Basic Authentication Scheme

The "basic" authentication scheme is based on the model that the
client must authenticate itself with a user-ID and a password for
each realm (...)
...
To receive authorization, the client sends the userid and password,
separated by a single colon (":") character, within a base64 [7]
encoded string in the 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

RFC7617 (Basic Auth) does not mandate the username be of any particular length.

I cannot see anything in relevant RFCs that prohibit a zero-length username.

[Dreamsorcerer]

I don't really mind either way. Looks fine.

As mentioned, the whole basic auth thing is a rather old deprecated thing, but that's more a concern for the server-side implementations still making the mistake of using it, rather than the client-side code which is forced to use it.

[shuckc]

Updated commit and CHANGES message - @webknjaz anything outstanding?

[webknjaz]

Updated commit and CHANGES message - @webknjaz anything outstanding?

I'd like to get feedback from @asvetlov too and dig into the standards a little bit more myself before making this change permanent.

[webknjaz]

WHATWG seems to think that this corner case is valid: whatwg/url#139. OTOH I haven't found any explicit tests in the requests repo that would indicate that they intentionally support this.

[shuckc]

Any chance of re-review and/or merge?

[Dreamsorcerer]

To summarise:
https://datatracker.ietf.org/doc/html/rfc3986#section-3.2.1
This does not say anything about not allowing empty usernames and the ABNF seems to allow it (the "optional password" I would interpret as the : being optional to specific a password, which again can be empty).

WHATWG also seems to allow it: https://url.spec.whatwg.org/#authority-state

RFC7617 doesn't appear to disallow it.

So, I think we can just proceed with this.

[patchback]

Backport to 3.10: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply ce9c4eb on top of patchback/backports/3.10/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495

Backporting merged PR #6495 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.10/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495 upstream/3.10

4. Now, cherry-pick PR Support credentials in URL with empty user, http://:password@host (#6494) #6495 contents into that branch:

   $ git cherry-pick -x ce9c4eb0f895f356e775ca268d7ccef908f4c936

   If it'll yell at you with something like fatal: Commit ce9c4eb0f895f356e775ca268d7ccef908f4c936 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x ce9c4eb0f895f356e775ca268d7ccef908f4c936

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Support credentials in URL with empty user, http://:password@host (#6494) #6495 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.10/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

[patchback]

Backport to 3.11: 💔 cherry-picking failed — conflicts found

❌ Failed to cleanly apply ce9c4eb on top of patchback/backports/3.11/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495

Backporting merged PR #6495 into master

1. Ensure you have a local repo clone of your fork. Unless you cloned it
   from the upstream, this would be your origin remote.
2. Make sure you have an upstream repo added as a remote too. In these
   instructions you'll refer to it by the name upstream. If you don't
   have it, here's how you can add it:

   $ git remote add upstream https://github.com/aio-libs/aiohttp.git

3. Ensure you have the latest copy of upstream and prepare a branch
   that will hold the backported code:

   $ git fetch upstream
   $ git checkout -b patchback/backports/3.11/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495 upstream/3.11

4. Now, cherry-pick PR Support credentials in URL with empty user, http://:password@host (#6494) #6495 contents into that branch:

   $ git cherry-pick -x ce9c4eb0f895f356e775ca268d7ccef908f4c936

   If it'll yell at you with something like fatal: Commit ce9c4eb0f895f356e775ca268d7ccef908f4c936 is a merge but no -m option was given., add -m 1 as follows instead:

   $ git cherry-pick -m1 -x ce9c4eb0f895f356e775ca268d7ccef908f4c936

5. At this point, you'll probably encounter some merge conflicts. You must
   resolve them in to preserve the patch from PR Support credentials in URL with empty user, http://:password@host (#6494) #6495 as close to the
   original as possible.
6. Push this branch to your fork on GitHub:

   $ git push origin patchback/backports/3.11/ce9c4eb0f895f356e775ca268d7ccef908f4c936/pr-6495

7. Create a PR, ensure that the CI is green. If it's not — update it so that
   the tests and any other checks pass. This is it!
   Now relax and wait for the maintainers to process your pull request
   when they have some cycles to do reviews. Don't worry — they'll tell you if
   any improvements are necessary when the time comes!

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.