Skip to content

Commit

Permalink
Merge branch 'GHSA-v6wp-4m6f-gcjg' into master
Browse files Browse the repository at this point in the history
This patch fixes an open redirect vulnerability bug in
`aiohttp.web_middlewares.normalize_path_middleware` by
making sure that there's at most one slash at the
beginning of the `Location` header value.

Refs:
* https://cheatsheetseries.owasp.org/cheatsheets/Unvalidated_Redirects_and_Forwards_Cheat_Sheet.html
* GHSA-v6wp-4m6f-gcjg

(cherry picked from commit 76c1fa1315faf48d44b061a1433d0d0c3e4dc12f)
  • Loading branch information
webknjaz committed Feb 25, 2021
1 parent 4ed7c25 commit 021c416
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 0 deletions.
9 changes: 9 additions & 0 deletions CHANGES/5497.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
**(SECURITY BUG)** Started preventing open redirects in the
``aiohttp.web.normalize_path_middleware`` middleware. For
more details, see
https://github.com/aio-libs/aiohttp/security/advisories/GHSA-v6wp-4m6f-gcjg.

Thanks to `Beast Glatisant <https://github.com/g147>`__ for
finding the first instance of this issue and `Jelmer Vernooij
<https://jelmer.uk/>`__ for reporting and tracking it down
in aiohttp.
1 change: 1 addition & 0 deletions aiohttp/web_middlewares.py
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ async def impl(request: Request, handler: _Handler) -> StreamResponse:
paths_to_check.append(merged_slashes[:-1])

for path in paths_to_check:
path = re.sub("^//+", "/", path) # SECURITY: GHSA-v6wp-4m6f-gcjg
resolves, request = await _check_request_resolves(request, path)
if resolves:
raise redirect_class(request.raw_path + query)
Expand Down
33 changes: 33 additions & 0 deletions tests/test_web_middleware.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import re
from typing import Any

import pytest
from yarl import URL
Expand Down Expand Up @@ -352,6 +353,38 @@ async def test_cannot_remove_and_add_slash(self) -> None:
with pytest.raises(AssertionError):
web.normalize_path_middleware(append_slash=True, remove_slash=True)

@pytest.mark.parametrize(
["append_slash", "remove_slash"],
[
(True, False),
(False, True),
(False, False),
],
)
async def test_open_redirects(
self, append_slash: bool, remove_slash: bool, aiohttp_client: Any
) -> None:
async def handle(request: web.Request) -> web.StreamResponse:
pytest.fail(
msg="Security advisory 'GHSA-v6wp-4m6f-gcjg' test handler "
"matched unexpectedly",
pytrace=False,
)

app = web.Application(
middlewares=[
web.normalize_path_middleware(
append_slash=append_slash, remove_slash=remove_slash
)
]
)
app.add_routes([web.get("/", handle), web.get("/google.com", handle)])
client = await aiohttp_client(app, server_kwargs={"skip_url_asserts": True})
resp = await client.get("//google.com", allow_redirects=False)
assert resp.status == 308
assert resp.headers["Location"] == "/google.com"
assert resp.url.query == URL("//google.com").query


async def test_old_style_middleware(loop, aiohttp_client) -> None:
async def handler(request):
Expand Down

0 comments on commit 021c416

Please sign in to comment.