From a6204a318e602e0ed203597d42e166ab80731c1a Mon Sep 17 00:00:00 2001 From: splitsec2 <35583321+splitsec2@users.noreply.github.com> Date: Sun, 20 Sep 2026 10:51:21 -0600 Subject: [PATCH] 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. --- shelfmark/core/oidc_routes.py | 2 +- tests/core/test_oidc_routes.py | 50 ++++++++++++++++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/shelfmark/core/oidc_routes.py b/shelfmark/core/oidc_routes.py index 42ec8a6a..0fd19400 100644 --- a/shelfmark/core/oidc_routes.py +++ b/shelfmark/core/oidc_routes.py @@ -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("/") diff --git a/tests/core/test_oidc_routes.py b/tests/core/test_oidc_routes.py index 2fe8a91e..8f716950 100644 --- a/tests/core/test_oidc_routes.py +++ b/tests/core/test_oidc_routes.py @@ -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):