fix(l4d2-web): reject encoded unsafe redirects
This commit is contained in:
parent
58fb8b2b63
commit
df680f6226
4 changed files with 62 additions and 2 deletions
|
|
@ -44,6 +44,15 @@ def test_login_page_renders_form(client) -> None:
|
||||||
assert "signup" not in text.lower()
|
assert "signup" not in text.lower()
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_page_drops_unsafe_encoded_next(client) -> None:
|
||||||
|
response = client.get("/login?next=/%5Cevil.com")
|
||||||
|
text = response.get_data(as_text=True)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
assert 'name="next"' not in text
|
||||||
|
assert "evil.com" not in text
|
||||||
|
|
||||||
|
|
||||||
def test_signup_routes_are_gone(client) -> None:
|
def test_signup_routes_are_gone(client) -> None:
|
||||||
assert client.get("/signup").status_code == 404
|
assert client.get("/signup").status_code == 404
|
||||||
assert client.post("/signup", data={"username": "alice", "password": "secret"}).status_code == 404
|
assert client.post("/signup", data={"username": "alice", "password": "secret"}).status_code == 404
|
||||||
|
|
@ -86,6 +95,19 @@ def test_login_ignores_backslash_next(client) -> None:
|
||||||
|
|
||||||
assert response.status_code == 302
|
assert response.status_code == 302
|
||||||
assert response.headers["Location"].endswith("/dashboard")
|
assert response.headers["Location"].endswith("/dashboard")
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_ignores_percent_encoded_backslash_next(client) -> None:
|
||||||
|
with session_scope() as session:
|
||||||
|
session.add(User(username="alice", password_digest=hash_password("secret"), admin=False))
|
||||||
|
|
||||||
|
response = client.post(
|
||||||
|
"/login",
|
||||||
|
data={"username": "alice", "password": "secret", "next": "/%5Cevil.com"},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 302
|
||||||
|
assert response.headers["Location"].endswith("/dashboard")
|
||||||
```
|
```
|
||||||
|
|
||||||
- [ ] **Step 2: Add protected-page and root redirect tests**
|
- [ ] **Step 2: Add protected-page and root redirect tests**
|
||||||
|
|
@ -149,7 +171,7 @@ Expected: FAIL because `/login` is still plain text, signup routes still exist,
|
||||||
Update `l4d2web/auth.py` imports and add helpers near the session helpers:
|
Update `l4d2web/auth.py` imports and add helpers near the session helpers:
|
||||||
|
|
||||||
```python
|
```python
|
||||||
from urllib.parse import quote
|
from urllib.parse import quote, unquote
|
||||||
|
|
||||||
from flask import abort, g, redirect, request, session
|
from flask import abort, g, redirect, request, session
|
||||||
```
|
```
|
||||||
|
|
@ -166,6 +188,13 @@ def is_safe_next(target: str | None) -> bool:
|
||||||
return False
|
return False
|
||||||
if "\\" in target:
|
if "\\" in target:
|
||||||
return False
|
return False
|
||||||
|
decoded_target = unquote(target)
|
||||||
|
if decoded_target.startswith("//"):
|
||||||
|
return False
|
||||||
|
if "://" in decoded_target:
|
||||||
|
return False
|
||||||
|
if "\\" in decoded_target:
|
||||||
|
return False
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -47,6 +47,7 @@ A `next` target is safe only when it is a local absolute path:
|
||||||
- It must not start with `//`.
|
- It must not start with `//`.
|
||||||
- It must not contain a URL scheme such as `https://`.
|
- It must not contain a URL scheme such as `https://`.
|
||||||
- It must not contain backslashes, which browsers may normalize into path separators.
|
- It must not contain backslashes, which browsers may normalize into path separators.
|
||||||
|
- The same checks apply after one percent-decoding pass so encoded backslashes and encoded protocol-relative paths are rejected.
|
||||||
|
|
||||||
Unsafe values are ignored and replaced with `/dashboard`. This avoids open redirects while keeping the implementation simple.
|
Unsafe values are ignored and replaced with `/dashboard`. This avoids open redirects while keeping the implementation simple.
|
||||||
|
|
||||||
|
|
@ -87,6 +88,7 @@ Tests should cover:
|
||||||
- A successful login with a safe `next` redirects to that target.
|
- A successful login with a safe `next` redirects to that target.
|
||||||
- A successful login with an unsafe `next` redirects to `/dashboard`.
|
- A successful login with an unsafe `next` redirects to `/dashboard`.
|
||||||
- A successful login with a backslash-containing `next` redirects to `/dashboard`.
|
- A successful login with a backslash-containing `next` redirects to `/dashboard`.
|
||||||
|
- A successful login with a percent-encoded backslash in `next` redirects to `/dashboard`.
|
||||||
- Anonymous `/` redirects to `/login`.
|
- Anonymous `/` redirects to `/login`.
|
||||||
- Logged-in `/` redirects to `/dashboard`.
|
- Logged-in `/` redirects to `/dashboard`.
|
||||||
- Non-admin users still receive `403` for admin pages.
|
- Non-admin users still receive `403` for admin pages.
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,6 @@
|
||||||
from functools import wraps
|
from functools import wraps
|
||||||
from typing import Callable, TypeVar
|
from typing import Callable, TypeVar
|
||||||
from urllib.parse import quote
|
from urllib.parse import quote, unquote
|
||||||
|
|
||||||
from flask import abort, g, redirect, request, session
|
from flask import abort, g, redirect, request, session
|
||||||
from sqlalchemy import select
|
from sqlalchemy import select
|
||||||
|
|
@ -53,6 +53,13 @@ def is_safe_next(target: str | None) -> bool:
|
||||||
return False
|
return False
|
||||||
if "\\" in target:
|
if "\\" in target:
|
||||||
return False
|
return False
|
||||||
|
decoded_target = unquote(target)
|
||||||
|
if decoded_target.startswith("//"):
|
||||||
|
return False
|
||||||
|
if "://" in decoded_target:
|
||||||
|
return False
|
||||||
|
if "\\" in decoded_target:
|
||||||
|
return False
|
||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -41,6 +41,15 @@ def test_login_page_renders_form(client) -> None:
|
||||||
assert "signup" not in text.lower()
|
assert "signup" not in text.lower()
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_page_drops_unsafe_encoded_next(client) -> None:
|
||||||
|
response = client.get("/login?next=/%5Cevil.com")
|
||||||
|
text = response.get_data(as_text=True)
|
||||||
|
|
||||||
|
assert response.status_code == 200
|
||||||
|
assert 'name="next"' not in text
|
||||||
|
assert "evil.com" not in text
|
||||||
|
|
||||||
|
|
||||||
def test_signup_routes_are_gone(client) -> None:
|
def test_signup_routes_are_gone(client) -> None:
|
||||||
assert client.get("/signup").status_code == 404
|
assert client.get("/signup").status_code == 404
|
||||||
assert client.post("/signup", data={"username": "alice", "password": "secret"}).status_code == 404
|
assert client.post("/signup", data={"username": "alice", "password": "secret"}).status_code == 404
|
||||||
|
|
@ -85,6 +94,19 @@ def test_login_ignores_backslash_next(client) -> None:
|
||||||
assert response.headers["Location"].endswith("/dashboard")
|
assert response.headers["Location"].endswith("/dashboard")
|
||||||
|
|
||||||
|
|
||||||
|
def test_login_ignores_percent_encoded_backslash_next(client) -> None:
|
||||||
|
with session_scope() as session:
|
||||||
|
session.add(User(username="alice", password_digest=hash_password("secret"), admin=False))
|
||||||
|
|
||||||
|
response = client.post(
|
||||||
|
"/login",
|
||||||
|
data={"username": "alice", "password": "secret", "next": "/%5Cevil.com"},
|
||||||
|
)
|
||||||
|
|
||||||
|
assert response.status_code == 302
|
||||||
|
assert response.headers["Location"].endswith("/dashboard")
|
||||||
|
|
||||||
|
|
||||||
def test_login_sets_session(client) -> None:
|
def test_login_sets_session(client) -> None:
|
||||||
with session_scope() as session:
|
with session_scope() as session:
|
||||||
session.add(User(username="alice", password_digest=hash_password("secret"), admin=False))
|
session.add(User(username="alice", password_digest=hash_password("secret"), admin=False))
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue