Fix router matching pre-encoded URLs by Dreamsorcerer · Pull Request #8898 · aio-libs/aiohttp

Dreamsorcerer · 2024-08-26T15:52:53Z

Fixes #5621.
Fixes #6619.

codecov · 2024-08-26T16:02:53Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.28%. Comparing base (9738426) to head (92e9ab5).
Report is 1069 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #8898   +/-   ##
=======================================
  Coverage   98.27%   98.28%           
=======================================
  Files         107      107           
  Lines       34226    34221    -5     
  Branches     4058     4058           
=======================================
- Hits        33637    33633    -4     
+ Misses        416      415    -1     
  Partials      173      173

Flag	Coverage Δ
CI-GHA	`98.17% <100.00%> (+<0.01%)`	⬆️
OS-Linux	`97.83% <100.00%> (+<0.01%)`	⬆️
OS-Windows	`96.24% <100.00%> (+<0.01%)`	⬆️
OS-macOS	`97.51% <100.00%> (+<0.01%)`	⬆️
Py-3.10.11	`97.61% <100.00%> (+<0.01%)`	⬆️
Py-3.10.14	`97.54% <100.00%> (+<0.01%)`	⬆️
Py-3.11.9	`97.77% <100.00%> (+<0.01%)`	⬆️
Py-3.12.5	`97.88% <100.00%> (+<0.01%)`	⬆️
Py-3.9.13	`97.49% <100.00%> (-0.01%)`	⬇️
Py-3.9.19	`97.43% <100.00%> (+<0.01%)`	⬆️
Py-pypy7.3.16	`97.04% <100.00%> (+<0.01%)`	⬆️
VM-macos	`97.51% <100.00%> (+<0.01%)`	⬆️
VM-ubuntu	`97.83% <100.00%> (+<0.01%)`	⬆️
VM-windows	`96.24% <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.

Dreamsorcerer · 2024-08-26T18:47:41Z

I'm not too sure about this, but I think everything is working and passing if I make this change to yarl:

diff --git a/yarl/_quoting_py.py b/yarl/_quoting_py.py
index 585a1da..fc20b37 100644
--- a/yarl/_quoting_py.py
+++ b/yarl/_quoting_py.py
@@ -158,7 +158,7 @@ class _Unquoter:
                         if to_add is None:  # pragma: no cover
                             raise RuntimeError("Cannot quote None")
                         ret.append(to_add)
-                    elif unquoted in self._unsafe:
+                    elif unquoted in self._unsafe or unquoted == "/":
                         to_add = self._quoter(unquoted)
                         if to_add is None:  # pragma: no cover
                             raise RuntimeError("Cannot quote None")

Essentially, we shouldn't unquote %2F in the path, as this could lead to incorrectly treating it as a path separator.

bdraco · 2024-08-26T19:14:00Z

I'm not too sure about this, but I think everything is working and passing if I make this change to yarl:

diff --git a/yarl/_quoting_py.py b/yarl/_quoting_py.py
index 585a1da..fc20b37 100644
--- a/yarl/_quoting_py.py
+++ b/yarl/_quoting_py.py
@@ -158,7 +158,7 @@ class _Unquoter:
                         if to_add is None:  # pragma: no cover
                             raise RuntimeError("Cannot quote None")
                         ret.append(to_add)
-                    elif unquoted in self._unsafe:
+                    elif unquoted in self._unsafe or unquoted == "/":
                         to_add = self._quoter(unquoted)
                         if to_add is None:  # pragma: no cover
                             raise RuntimeError("Cannot quote None")

Essentially, we shouldn't unquote %2F in the path, as this could lead to incorrectly treating it as a path separator.

https://datatracker.ietf.org/doc/html/rfc3986#section-2.4

When a URI is dereferenced, the components and subcomponents
significant to the scheme-specific dereferencing process (if any)
must be parsed and separated before the percent-encoded octets within
those components can be safely decoded, as otherwise the data may be
mistaken for component delimiters.

I read that as the URL needs to be separated by delimiters before its decoded

Dreamsorcerer · 2024-08-26T19:28:06Z

I read that as the URL needs to be separated by delimiters before its decoded

Hmm, I'm not sure how best to accommodate that...

bdraco · 2024-08-26T19:48:30Z

https://www.w3.org/Addressing/URL/4_URI_Recommentations.html

Example 2
The URIs
http://www.w3.org/albert/bertram/marie-claude

and
http://www.w3.org/albert/bertram%2Fmarie-claude

are NOT identical, as in the second case the encoded slash does not have hierarchical significance.

bdraco · 2024-08-26T19:50:45Z

This gets messy quickly.. https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2007-0450

bdraco · 2024-08-26T19:53:15Z

The safest course seems to never decode %2F to / when decoding a path

bdraco · 2024-08-26T19:54:49Z

yarl treats + as unsafe in the unquoter but not /
https://github.com/aio-libs/yarl/blob/24073cec26aade02aa18f959e429a46e90cee001/yarl/_url.py#L165

but for the quoter protected="/+"
https://github.com/aio-libs/yarl/blob/24073cec26aade02aa18f959e429a46e90cee001/yarl/_url.py#L156

bdraco · 2024-08-26T20:02:11Z

I think making / unsafe in yarl makes sense.... but its probably a breaking change

Dreamsorcerer · 2024-08-26T20:34:18Z

I think making / unsafe in yarl makes sense.... but its probably a breaking change

unsafe has a slightly different meaning, I tried that first.
/my/path%2Ffoo will become %2Fmy%2Fpath%2Ffoo

So, we'd probably want to add an ignore option or something separately, which would go where I hardcoded the value in the diff.

Dreamsorcerer · 2024-08-26T20:35:21Z

Given the security implications, I suspect the change should be made regardless of backwards compatibility.

Dreamsorcerer · 2024-08-26T22:25:17Z

aio-libs/yarl#1057

bdraco · 2024-08-28T07:30:59Z

Still haven't found a definitive answer on this one.....maybe we should be testing this against:
https://url.spec.whatwg.org/
https://github.com/web-platform-tests/wpt/tree/master/url

https://github.com/web-platform-tests/wpt/blob/master/url/resources/percent-encoding.json
https://github.com/web-platform-tests/wpt/blob/master/url/resources/urltestdata.json

Dreamsorcerer · 2024-08-28T12:00:26Z

Still haven't found a definitive answer on this one.....maybe we should be testing this against: https://url.spec.whatwg.org/ https://github.com/web-platform-tests/wpt/tree/master/url

Sounds like it could be a lot of work. Feel free to take that on, but I think this current solution is probably reasonable until that is done.

bdraco · 2024-08-28T15:56:42Z

Still haven't found a definitive answer on this one.....maybe we should be testing this against: url.spec.whatwg.org web-platform-tests/wpt@master/url

Sounds like it could be a lot of work. Feel free to take that on, but I think this current solution is probably reasonable until that is done.

I got a bit of a chuckle out of that. Its definitely a lot of work, and I wasn't intending for it to be done here. While I think it would be great to test long term, I was thinking we could look this specific case is handled there as a reference point.

I plan on digging through it a bit more and provide some better analysis. Its Home Assistant beta week so I haven't had time to do that yet.

for more information, see https://pre-commit.ci

changelog: aio-libs/yarl@v1.9.4...v1.9.5

bdraco · 2024-08-31T03:16:45Z

Will test this shortly

for more information, see https://pre-commit.ci

bdraco · 2024-08-31T16:54:53Z

I'm hoping to find time to test this today

bdraco · 2024-08-31T18:42:29Z

Everything looks like its working as expected. Calling path is a bit heavier than raw_path but not much we can do about that.

The cached_property implementation in yarl is quite a bit slower than the reify implementation in aiohttp. I noted that in aio-libs/yarl#1065 .. maybe I need to do that sooner rather than later.

patchback · 2024-08-31T18:56:22Z

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

❌ Failed to cleanly apply 6be9452 on top of patchback/backports/3.10/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898

Backporting merged PR #8898 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
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
```

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/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898 upstream/3.10

Now, cherry-pick PR Fix router matching pre-encoded URLs #8898 contents into that branch:
```
$ git cherry-pick -x 6be94520ea46fe1829e6c9d986e7fc9f7db50cad
```
If it'll yell at you with something like fatal: Commit 6be94520ea46fe1829e6c9d986e7fc9f7db50cad is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 6be94520ea46fe1829e6c9d986e7fc9f7db50cad
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix router matching pre-encoded URLs #8898 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.10/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898

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 · 2024-08-31T18:56:28Z

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

❌ Failed to cleanly apply 6be9452 on top of patchback/backports/3.11/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898

Backporting merged PR #8898 into master

Ensure you have a local repo clone of your fork. Unless you cloned it
from the upstream, this would be your origin remote.
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
```

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/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898 upstream/3.11

Now, cherry-pick PR Fix router matching pre-encoded URLs #8898 contents into that branch:
```
$ git cherry-pick -x 6be94520ea46fe1829e6c9d986e7fc9f7db50cad
```
If it'll yell at you with something like fatal: Commit 6be94520ea46fe1829e6c9d986e7fc9f7db50cad is a merge but no -m option was given., add -m 1 as follows instead:
```
$ git cherry-pick -m1 -x 6be94520ea46fe1829e6c9d986e7fc9f7db50cad
```
At this point, you'll probably encounter some merge conflicts. You must
resolve them in to preserve the patch from PR Fix router matching pre-encoded URLs #8898 as close to the
original as possible.

Push this branch to your fork on GitHub:

$ git push origin patchback/backports/3.11/6be94520ea46fe1829e6c9d986e7fc9f7db50cad/pr-8898

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.

Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 6be9452)

#8898 now passes the unquoted path and we would unquote it again

The minimum version must increase because we need ``URL.path_safe`` to be able to fix #9267 and the original PR #8898

Fix router matching pre-encoded URLs

c7f74bb

Dreamsorcerer added backport-3.10 labels Aug 26, 2024

Dreamsorcerer requested a review from asvetlov as a code owner August 26, 2024 15:52

Create 8898.bugfix.rst

559bc93

Dreamsorcerer requested a review from webknjaz as a code owner August 26, 2024 15:54

psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Aug 26, 2024

Fix

ce40103

Dreamsorcerer marked this pull request as draft August 26, 2024 18:45

Dreamsorcerer mentioned this pull request Aug 29, 2024

let custom regex of route can match special and non-ascii characters without urlencoding the pattern too #6622

Closed

5 tasks

Dreamsorcerer and others added 3 commits August 29, 2024 18:19

Add test for #6619

a1d19cb

[pre-commit.ci] auto fixes from pre-commit.com hooks

73aa7a6

for more information, see https://pre-commit.ci

Bump yarl to 1.9.5

5cd5de2

changelog: aio-libs/yarl@v1.9.4...v1.9.5

bdraco mentioned this pull request Aug 31, 2024

Bump yarl to 1.9.6 #8952

Closed

Merge branch 'bump_yarl' into fix-decoded-url-matching

616cf6b

bdraco and others added 4 commits August 30, 2024 22:26

1.9.6

ebddebd

Merge branch 'bump_yarl' into fix-decoded-url-matching

6a10b36

typing

282b3e2

[pre-commit.ci] auto fixes from pre-commit.com hooks

9d6774f

for more information, see https://pre-commit.ci

Dreamsorcerer marked this pull request as ready for review August 31, 2024 14:24

Update 8898.bugfix.rst

92e51a2

Merge branch 'master' into fix-decoded-url-matching

92e9ab5

bdraco approved these changes Aug 31, 2024

View reviewed changes

Dreamsorcerer merged commit 6be9452 into master Aug 31, 2024

Dreamsorcerer deleted the fix-decoded-url-matching branch August 31, 2024 18:56

Dreamsorcerer added a commit that referenced this pull request Aug 31, 2024

Fix router matching pre-encoded URLs (#8898)

2a40ba4

Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 6be9452)

Dreamsorcerer added a commit that referenced this pull request Aug 31, 2024

Fix router matching pre-encoded URLs (#8898)

51abb0a

Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 6be9452)

Dreamsorcerer added a commit that referenced this pull request Aug 31, 2024

Fix router matching pre-encoded URLs (#8898) (#8960)

9ac6853

Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 6be9452)

Dreamsorcerer added a commit that referenced this pull request Aug 31, 2024

Fix router matching pre-encoded URLs (#8898) (#8961)

b8f1b12

Co-authored-by: J. Nick Koston <nick@koston.org> (cherry picked from commit 6be9452)

booniepepper mentioned this pull request Sep 3, 2024

patchback/backports/3.11/1d170d37f476df705a9dcc3588e192e8ccb871c0/pr 7731 #9005

Closed

5 tasks

dopplershift mentioned this pull request Sep 4, 2024

Rebuild for python 3.13 conda-forge/aiohttp-feedstock#94

Merged

dotlambda mentioned this pull request Sep 8, 2024

python312Packages.yarl: 1.9.4 -> 1.13.1, python312Packages.aiohttp: 3.10.5 -> 3.10.7, python312Packages.aiohappyeyeballs: 2.3.6 -> 2.4.2 NixOS/nixpkgs#339112

Merged

13 tasks

This was referenced Sep 23, 2024

Bump aiohttp to 3.10.6rc2 home-assistant/core#126468

Merged

Highlight fixing router matching pre-encoded URLs can be a breaking change #9266

Closed

bdraco added a commit that referenced this pull request Sep 23, 2024

Fix double unquoting in url dispatcher

d8af609

#8898 now passes the unquoted path and we would unquote it again

bdraco mentioned this pull request Sep 23, 2024

Fix double unquoting in url dispatcher #9267

Merged

bdraco added a commit that referenced this pull request Sep 23, 2024

Fix double unquoting in url dispatcher

2731a1a

#8898 now passes the unquoted path and we would unquote it again

bdraco added a commit that referenced this pull request Sep 23, 2024

Increase minimum yarl version to 1.12.0

544a476

The minimum version must increase because we need ``URL.path_safe`` to be able to fix #9267 and the original PR #8898

This was referenced Sep 23, 2024

Increase minimum yarl version to 1.12.0 #9268

Closed

Release 3.10.6rc1 #9271

Merged

Uh oh!

Conversation

Dreamsorcerer commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Dreamsorcerer commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

Dreamsorcerer commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

bdraco commented Aug 26, 2024

Uh oh!

Dreamsorcerer commented Aug 26, 2024

Uh oh!

Dreamsorcerer commented Aug 26, 2024

Uh oh!

Dreamsorcerer commented Aug 26, 2024

Uh oh!

bdraco commented Aug 28, 2024

Uh oh!

Dreamsorcerer commented Aug 28, 2024

Uh oh!

bdraco commented Aug 28, 2024

Uh oh!

bdraco commented Aug 31, 2024

Uh oh!

bdraco commented Aug 31, 2024

Uh oh!

bdraco commented Aug 31, 2024

Uh oh!

patchback bot commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Backporting merged PR #8898 into master

Uh oh!

patchback bot commented Aug 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Backporting merged PR #8898 into master

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dreamsorcerer commented Aug 26, 2024 •

edited

Loading

codecov bot commented Aug 26, 2024 •

edited

Loading

patchback bot commented Aug 31, 2024 •

edited

Loading

patchback bot commented Aug 31, 2024 •

edited

Loading