mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-10-05 22:05:50 +01:00
fix(oidc): reject backslash paths in the return_to sanitizer (#1359)
The OIDC `return_to` sanitizer rejects values starting with `//` and then relies on `urlsplit` to catch anything carrying a netloc. A value such as `/\host` has no netloc, so it is stored in the session and used as the post-login redirect target and browsers resolve the backslash as a path separator, which lands the user outside the app after a successful login. `_normalize_return_to` now also rejects values whose path contains a backslash. That matches the frontend sanitizer in `authRedirect.ts`, which parses with `URL` and already discards those forms, so the two ends agree again. The check covers the path only, so query and fragment backslashes still round-trip, and it also catches the script-root case where `/app/\host` strips to `/\host`. ## Verification - New cases in `tests/core/test_oidc_routes.py` cover the rejected forms, including under a script root, and confirm `/`, `/settings` and `/search?q=x#frag` are unaffected. They fail on current main and pass here. - Full suite (3155), ruff, ruff format, basedpyright, vulture green.
This commit is contained in:
@@ -109,7 +109,7 @@ def _normalize_return_to(raw_return_to: object) -> str | None:
|
||||
return None
|
||||
|
||||
parsed = urlsplit(value)
|
||||
if parsed.scheme or parsed.netloc:
|
||||
if parsed.scheme or parsed.netloc or "\\" in parsed.path:
|
||||
return None
|
||||
|
||||
script_root = request.script_root.rstrip("/")
|
||||
|
||||
@@ -182,6 +182,56 @@ class TestOIDCLoginEndpoint:
|
||||
with client.session_transaction() as sess:
|
||||
assert "oidc_return_to" not in sess
|
||||
|
||||
@patch("shelfmark.core.oidc_routes._get_oidc_client")
|
||||
def test_login_ignores_backslash_return_to(self, mock_get_client, client):
|
||||
fake_client = Mock()
|
||||
fake_client.authorize_redirect.return_value = redirect("https://auth.example.com/authorize")
|
||||
mock_get_client.return_value = (fake_client, MOCK_OIDC_CONFIG)
|
||||
|
||||
resp = client.get("/api/auth/oidc/login?return_to=%2F%5Cevil.example.com%2Fphish")
|
||||
|
||||
assert resp.status_code == 302
|
||||
with client.session_transaction() as sess:
|
||||
assert "oidc_return_to" not in sess
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"raw",
|
||||
[
|
||||
"/\\evil.example.com",
|
||||
"/\\evil.example.com/phish",
|
||||
"/\\\\evil.example.com",
|
||||
"/\\/evil.example.com",
|
||||
],
|
||||
)
|
||||
def test_normalize_return_to_rejects_backslash_paths(self, app, raw):
|
||||
from shelfmark.core.oidc_routes import _normalize_return_to
|
||||
|
||||
with app.test_request_context("/api/auth/oidc/login"):
|
||||
assert _normalize_return_to(raw) is None
|
||||
|
||||
def test_normalize_return_to_rejects_backslash_under_script_root(self, app):
|
||||
from shelfmark.core.oidc_routes import _normalize_return_to
|
||||
|
||||
with app.test_request_context(
|
||||
"/shelfmark/api/auth/oidc/login",
|
||||
environ_overrides={"SCRIPT_NAME": "/shelfmark"},
|
||||
):
|
||||
assert _normalize_return_to("/shelfmark/\\evil.example.com") is None
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("raw", "expected"),
|
||||
[
|
||||
("/", "/"),
|
||||
("/settings", "/settings"),
|
||||
("/search?q=x#frag", "/search?q=x#frag"),
|
||||
],
|
||||
)
|
||||
def test_normalize_return_to_keeps_local_paths(self, app, raw, expected):
|
||||
from shelfmark.core.oidc_routes import _normalize_return_to
|
||||
|
||||
with app.test_request_context("/api/auth/oidc/login"):
|
||||
assert _normalize_return_to(raw) == expected
|
||||
|
||||
|
||||
class TestOIDCCallbackEndpoint:
|
||||
def test_normalize_claims_returns_empty_dict_for_invalid_mapping(self):
|
||||
|
||||
Reference in New Issue
Block a user