From 153ac86a5a2d322749a12c34a9f59ddca60b2a36 Mon Sep 17 00:00:00 2001 From: Zak Bearman Date: Sat, 19 Sep 2026 12:25:43 +1200 Subject: [PATCH] fix(auth): accept configured public origin for sign-in --- PRODUCTION.md | 8 + backend/app/main.py | 6 +- backend/app/services/request_origins.py | 41 ++++++ backend/tests/test_request_origins.py | 187 ++++++++++++++++++++++++ frontend/app/lib/login-errors.test.ts | 51 +++++++ frontend/app/lib/login-errors.ts | 17 +++ frontend/app/login/page.tsx | 13 +- scripts/ci_container_smoke.sh | 25 ++++ 8 files changed, 335 insertions(+), 13 deletions(-) create mode 100644 backend/app/services/request_origins.py create mode 100644 backend/tests/test_request_origins.py create mode 100644 frontend/app/lib/login-errors.test.ts create mode 100644 frontend/app/lib/login-errors.ts diff --git a/PRODUCTION.md b/PRODUCTION.md index f94901b..4ffe33a 100644 --- a/PRODUCTION.md +++ b/PRODUCTION.md @@ -35,6 +35,14 @@ from `main`; use `prod-` tags to identify an exact release. feature, database integrity and account counts. Do not trigger bulk permission changes, email sends or user imports as a deployment smoke test. + Include browser-origin POST checks for both `/api/auth/login` and + `/api/auth/jellyfin/login`: an empty form with `Origin` set to the public URL + must reach input validation (422), while an unrelated origin must return 403. + GET-only login/health checks do not detect origin-policy lockouts. Set + `CORS_ALLOW_ORIGIN` to the exact public origin; the state-change guard also + accepts the explicitly configured Hosting & proxy public URL, never a URL + inferred from request Host or forwarded headers. + For rollback, select the saved image and recreate only Magent. Restore data only if needed; doing so can discard activity since the backup. Never restore a whole shared Compose or Caddy file without checking for unrelated changes first. diff --git a/backend/app/main.py b/backend/app/main.py index c2926c6..c109d3c 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -60,6 +60,7 @@ from .runtime import get_runtime_settings from .metrics import record_api, start_metrics from .request_limits import InstallationBodyLimitMiddleware from .secret_storage import validate_secret_storage_configuration +from .services.request_origins import is_allowed_request_origin logger = logging.getLogger(__name__) _background_tasks: list[asyncio.Task[None]] = [] @@ -112,9 +113,8 @@ async def log_requests_and_add_security_headers(request: Request, call_next): ) request.state.request_id = request_id if request.method.upper() not in {"GET", "HEAD", "OPTIONS"}: - origin = str(request.headers.get("origin") or "").rstrip("/") - allowed_origin = str(settings.cors_allow_origin or "").rstrip("/") - if origin and origin != allowed_origin: + origin = str(request.headers.get("origin") or "") + if origin and not is_allowed_request_origin(origin): record_api(request, 403, 0.0) if operation_id and operation_token is not None: finish_operation(operation_id, success=False, status_code=403) diff --git a/backend/app/services/request_origins.py b/backend/app/services/request_origins.py new file mode 100644 index 0000000..52ad3c8 --- /dev/null +++ b/backend/app/services/request_origins.py @@ -0,0 +1,41 @@ +"""State-changing requests may originate only from explicitly configured sites. + +The public Hosting & proxy URL can be stored in the database, while the CORS +environment setting still has its localhost default on an upgraded install. +Never infer a trusted origin from request Host or forwarded headers. +""" + +from urllib.parse import urlsplit + +from ..config import settings +from .public_urls import magent_public_url, valid_public_url + + +def _origin(value: str, *, configured_url: bool = False) -> tuple[str, str, int] | None: + value = str(value or "") + if any(character.isspace() or ord(character) < 33 or ord(character) == 127 for character in value): + return None + if "?" in value or "#" in value: + return None + validated = valid_public_url(value) + if not validated: + return None + parsed = urlsplit(value) + if parsed.username is not None or parsed.password is not None: + return None + if not configured_url and parsed.path: + return None + return ( + parsed.scheme.lower(), + parsed.hostname.lower(), + parsed.port or (443 if parsed.scheme == "https" else 80), + ) + + +def is_allowed_request_origin(origin: str) -> bool: + candidate = _origin(origin) + if candidate is None: + return False + if candidate == _origin(str(settings.cors_allow_origin or "").rstrip("/")): + return True + return candidate == _origin(magent_public_url(), configured_url=True) diff --git a/backend/tests/test_request_origins.py b/backend/tests/test_request_origins.py new file mode 100644 index 0000000..8711618 --- /dev/null +++ b/backend/tests/test_request_origins.py @@ -0,0 +1,187 @@ +"""Origin checks use operator configuration, never caller-controlled routing headers.""" + +from pathlib import Path +import tempfile +from types import SimpleNamespace +import unittest +from unittest.mock import AsyncMock, patch + +from fastapi.testclient import TestClient + +from backend.app import db, main +from backend.app.config import settings +from backend.app.routers import auth as auth_router +from backend.app.services import public_urls +from backend.app.services.request_origins import is_allowed_request_origin + + +PUBLIC_ORIGIN = "https://watch.example.test" +LOCAL_ORIGIN = "http://localhost:3000" + + +class RequestOriginTests(unittest.TestCase): + def setUp(self): + self.runtime = SimpleNamespace( + magent_proxy_enabled=False, + magent_proxy_base_url=None, + magent_application_url=PUBLIC_ORIGIN, + ) + self.enterContext(patch.object(settings, "cors_allow_origin", LOCAL_ORIGIN)) + self.enterContext(patch.object(public_urls, "get_runtime_settings", return_value=self.runtime)) + + def test_explicit_cors_and_configured_public_url_are_both_allowed(self): + self.assertTrue(is_allowed_request_origin(LOCAL_ORIGIN)) + self.assertTrue(is_allowed_request_origin(PUBLIC_ORIGIN)) + self.assertFalse(is_allowed_request_origin("https://unrelated.example.test")) + + def test_scheme_hostname_case_and_default_ports_are_canonicalized(self): + for origin in (PUBLIC_ORIGIN, "HTTPS://WATCH.EXAMPLE.TEST", "https://watch.example.test:443"): + with self.subTest(origin=origin): + self.assertTrue(is_allowed_request_origin(origin)) + self.runtime.magent_application_url = "http://watch.example.test:80" + self.assertTrue(is_allowed_request_origin("http://WATCH.example.test")) + self.assertFalse(is_allowed_request_origin("https://watch.example.test")) + self.assertFalse(is_allowed_request_origin("http://watch.example.test:8080")) + + def test_nondefault_ports_must_match(self): + self.runtime.magent_application_url = "https://watch.example.test:8443/magent" + self.assertTrue(is_allowed_request_origin("https://watch.example.test:8443")) + self.assertFalse(is_allowed_request_origin("https://watch.example.test")) + self.assertFalse(is_allowed_request_origin("https://watch.example.test:443")) + + def test_configured_subpath_does_not_become_part_of_origin(self): + self.runtime.magent_application_url = PUBLIC_ORIGIN + "/magent/" + self.assertTrue(is_allowed_request_origin(PUBLIC_ORIGIN)) + self.assertFalse(is_allowed_request_origin(PUBLIC_ORIGIN + "/magent")) + + def test_enabled_proxy_uses_configured_proxy_public_url(self): + self.runtime.magent_proxy_enabled = True + self.runtime.magent_proxy_base_url = "https://proxy.example.test/magent" + self.assertTrue(is_allowed_request_origin("https://proxy.example.test")) + self.assertTrue(is_allowed_request_origin(LOCAL_ORIGIN)) + self.assertFalse(is_allowed_request_origin(PUBLIC_ORIGIN)) + + def test_unconfigured_public_url_only_allows_explicit_cors(self): + self.runtime.magent_application_url = None + self.assertTrue(is_allowed_request_origin(LOCAL_ORIGIN)) + self.assertFalse(is_allowed_request_origin(PUBLIC_ORIGIN)) + + def test_invalid_or_non_origin_inputs_are_rejected(self): + for origin in ( + "", "null", "*", "watch.example.test", "//watch.example.test", + "ftp://watch.example.test", "javascript:alert(1)", + PUBLIC_ORIGIN + "/", PUBLIC_ORIGIN + "/path", + PUBLIC_ORIGIN + "?query=true", PUBLIC_ORIGIN + "#fragment", + PUBLIC_ORIGIN + "?", PUBLIC_ORIGIN + "#", + "https://user@watch.example.test", "https://user:password@watch.example.test", + "https://watch.example.test@evil.example.test", "https://watch.example.test.evil.example.test", + "https://watch.example.test:0", "https://watch.example.test:65536", + "https://watch.example.test:invalid", "https://[invalid", + PUBLIC_ORIGIN + " https://evil.example.test", PUBLIC_ORIGIN + ",https://evil.example.test", + "https://watch.example.test\\@evil.example.test", PUBLIC_ORIGIN + "\n", + ): + with self.subTest(origin=repr(origin)): + self.assertFalse(is_allowed_request_origin(origin)) + + def test_invalid_configured_public_url_does_not_authorize_an_origin(self): + for configured in ( + "https://user:password@watch.example.test", PUBLIC_ORIGIN + "?token=private", + PUBLIC_ORIGIN + "#fragment", "javascript:alert(1)", "https://watch.example.test:65536", + ): + with self.subTest(configured=configured): + self.runtime.magent_application_url = configured + self.assertFalse(is_allowed_request_origin(PUBLIC_ORIGIN)) + self.assertTrue(is_allowed_request_origin(LOCAL_ORIGIN)) + + +class RequestOriginHttpTests(unittest.TestCase): + def setUp(self): + temporary = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + self.addCleanup(temporary.cleanup) + self.runtime = SimpleNamespace( + magent_proxy_enabled=False, + magent_proxy_base_url=None, + magent_application_url=PUBLIC_ORIGIN, + ) + for name, value in { + "sqlite_path": str(Path(temporary.name) / "origin-tests.db"), + "sqlite_journal_mode": "DELETE", + "jwt_secret": "request-origin-tests-jwt-secret-at-least-32-characters", + "settings_encryption_key": None, + "admin_username": "unused-environment-admin", + "admin_password": "", + "cors_allow_origin": LOCAL_ORIGIN, + "auth_cookie_domain": None, + "auth_cookie_secure": True, + }.items(): + self.enterContext(patch.object(settings, name, value)) + self.enterContext(patch.object(public_urls, "get_runtime_settings", return_value=self.runtime)) + # Constructing without a context deliberately skips production startup: + # no migrations/workers/listeners/log files outside this temporary DB. + db.init_db() + self.client = TestClient(main.app, base_url=PUBLIC_ORIGIN) + self.addCleanup(self.client.close) + + def test_public_origin_reaches_both_auth_handlers_with_localhost_cors_default(self): + for path in ("/auth/login", "/auth/jellyfin/login"): + with self.subTest(path=path): + response = self.client.post(path, data={}, headers={"Origin": PUBLIC_ORIGIN}) + self.assertEqual(response.status_code, 422, response.text) + self.assertNotEqual(response.json().get("detail"), "Cross-origin state change rejected") + + def test_explicit_cors_origin_remains_allowed(self): + response = self.client.post("/auth/login", data={}, headers={"Origin": LOCAL_ORIGIN}) + self.assertEqual(response.status_code, 422, response.text) + + def test_no_origin_keeps_existing_nonbrowser_behavior(self): + response = self.client.post("/auth/login", data={}) + self.assertEqual(response.status_code, 422, response.text) + + def test_caller_controlled_host_forwarding_and_fetch_headers_cannot_authorize_evil_origin(self): + for path in ("/auth/login", "/auth/jellyfin/login"): + for routing_headers in ( + {}, + {"Host": "evil.example.test"}, + {"X-Forwarded-Host": "evil.example.test", "X-Forwarded-Proto": "https"}, + {"Host": "evil.example.test", "X-Forwarded-Host": "evil.example.test", "Sec-Fetch-Site": "same-origin"}, + {"Host": "watch.example.test", "X-Forwarded-Host": "watch.example.test", "Sec-Fetch-Site": "same-origin"}, + ): + with self.subTest(path=path, routing_headers=routing_headers): + response = self.client.post(path, data={}, headers={"Origin": "https://evil.example.test", **routing_headers}) + self.assertEqual(response.status_code, 403, response.text) + self.assertEqual(response.json()["detail"], "Cross-origin state change rejected") + + def test_null_path_query_and_userinfo_origins_are_rejected_before_login(self): + for origin in ("null", PUBLIC_ORIGIN + "/", PUBLIC_ORIGIN + "/path", PUBLIC_ORIGIN + "?query=1", "https://user@watch.example.test"): + with self.subTest(origin=origin): + response = self.client.post("/auth/login", data={}, headers={"Origin": origin}) + self.assertEqual(response.status_code, 403, response.text) + + def test_valid_local_login_works_from_configured_public_origin(self): + password = "origin-tests-valid-local-password" + db.create_user("origin-owner", password, role="admin") + response = self.client.post("/auth/login", data={"username": "origin-owner", "password": password}, headers={"Origin": PUBLIC_ORIGIN}) + self.assertEqual(response.status_code, 200, response.text) + self.assertIn(settings.auth_cookie_name, self.client.cookies) + profile = self.client.get("/auth/profile") + self.assertEqual(profile.status_code, 200, profile.text) + self.assertEqual(profile.json()["user"]["username"], "origin-owner") + + def test_valid_mocked_jellyfin_login_works_from_configured_public_origin(self): + jellyfin_runtime = SimpleNamespace(jellyfin_base_url="http://jellyfin.test:8096", jellyfin_api_key="test-api-key") + upstream = SimpleNamespace( + configured=lambda: True, + authenticate_by_name=AsyncMock(return_value={"User": {"Id": "test-jellyfin-id", "Name": "origin-viewer"}}), + get_users=AsyncMock(return_value=[]), + _extract_user_id=lambda _response: "test-jellyfin-id", + ) + with patch.object(auth_router, "get_runtime_settings", return_value=jellyfin_runtime), patch.object(auth_router, "JellyfinClient", return_value=upstream), patch.object(auth_router, "get_cached_jellyseerr_users", return_value=[]): + response = self.client.post("/auth/jellyfin/login", data={"username": "origin-viewer", "password": "origin-tests-jellyfin-password"}, headers={"Origin": PUBLIC_ORIGIN}) + self.assertEqual(response.status_code, 200, response.text) + upstream.authenticate_by_name.assert_awaited_once_with("origin-viewer", "origin-tests-jellyfin-password") + self.assertIn(settings.auth_cookie_name, self.client.cookies) + self.assertEqual(db.get_user_by_username("origin-viewer")["auth_provider"], "jellyfin") + + +if __name__ == "__main__": + unittest.main() diff --git a/frontend/app/lib/login-errors.test.ts b/frontend/app/lib/login-errors.test.ts new file mode 100644 index 0000000..4beafea --- /dev/null +++ b/frontend/app/lib/login-errors.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, it } from "vitest"; +import { loginErrorMessage } from "./login-errors"; + +const errorResponse = (status: number, payload: unknown) => new Response(JSON.stringify(payload), { status }); + +describe("login error messages", () => { + it("identifies a site security rejection without blaming the account", async () => { + expect(await loginErrorMessage(errorResponse(403, { detail: "Cross-origin state change rejected" }))).toBe( + "Sign-in was blocked by the site's security configuration. Please contact an administrator.", + ); + }); + + it.each(["User is blocked", "User access has expired", "Unknown upstream error"])( + "keeps a generic account message for %s", + async (detail) => { + expect(await loginErrorMessage(errorResponse(403, { detail }))).toBe( + "This account cannot sign in. Please contact an administrator.", + ); + }, + ); + + it.each([ + null, + [], + { detail: ["Cross-origin state change rejected"] }, + { detail: "Cross-origin state change rejected: private upstream detail" }, + { detail: "" }, + ])("does not render or loosely match unexpected response bodies: %j", async (payload) => { + expect(await loginErrorMessage(errorResponse(403, payload))).toBe( + "This account cannot sign in. Please contact an administrator.", + ); + }); + + it("handles a non-JSON proxy denial safely", async () => { + expect(await loginErrorMessage(new Response("Forbidden", { status: 403 }))).toBe( + "This account cannot sign in. Please contact an administrator.", + ); + }); + + it.each([ + [401, "Check your username and password, then try again."], + [400, "Check your username and password, then try again."], + [429, "Too many attempts. Please wait a moment and try again."], + [500, "Sign-in is temporarily unavailable. Please try again shortly."], + [502, "Sign-in is temporarily unavailable. Please try again shortly."], + ])("preserves the existing message for HTTP %s", async (status, expected) => { + expect( + await loginErrorMessage(errorResponse(status as number, { detail: "Cross-origin state change rejected" })), + ).toBe(expected); + }); +}); diff --git a/frontend/app/lib/login-errors.ts b/frontend/app/lib/login-errors.ts new file mode 100644 index 0000000..7f38d27 --- /dev/null +++ b/frontend/app/lib/login-errors.ts @@ -0,0 +1,17 @@ +export async function loginErrorMessage(response: Response): Promise { + if (response.status === 429) return "Too many attempts. Please wait a moment and try again."; + if (response.status >= 500) return "Sign-in is temporarily unavailable. Please try again shortly."; + if (response.status === 403) { + const payload: unknown = await response.json().catch(() => null); + if ( + payload !== null && + typeof payload === "object" && + "detail" in payload && + payload.detail === "Cross-origin state change rejected" + ) { + return "Sign-in was blocked by the site's security configuration. Please contact an administrator."; + } + return "This account cannot sign in. Please contact an administrator."; + } + return "Check your username and password, then try again."; +} diff --git a/frontend/app/login/page.tsx b/frontend/app/login/page.tsx index d3da499..05ff57e 100644 --- a/frontend/app/login/page.tsx +++ b/frontend/app/login/page.tsx @@ -1,7 +1,8 @@ "use client"; -import { useEffect, useState, type FormEvent } from "react"; +import { type FormEvent, useEffect, useState } from "react"; import { getApiBase, setToken } from "../lib/auth"; +import { loginErrorMessage } from "../lib/login-errors"; import AuthLayout from "../ui/AuthLayout"; type LoginMode = "jellyfin" | "local"; @@ -73,15 +74,7 @@ export default function LoginPage() { }, ); if (!response.ok) { - setError( - response.status === 429 - ? "Too many attempts. Please wait a moment and try again." - : response.status >= 500 - ? "Sign-in is temporarily unavailable. Please try again shortly." - : response.status === 403 - ? "This account cannot sign in. Please contact an administrator." - : "Check your username and password, then try again.", - ); + setError(await loginErrorMessage(response)); return; } const data = await response.json(); diff --git a/scripts/ci_container_smoke.sh b/scripts/ci_container_smoke.sh index 1f11d62..e8933a8 100644 --- a/scripts/ci_container_smoke.sh +++ b/scripts/ci_container_smoke.sh @@ -23,6 +23,7 @@ docker run --detach --name "$container_name" \ --env JWT_SECRET=ci-only-secret-with-at-least-32-characters \ --env SETTINGS_ENCRYPTION_KEY=AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA= \ --env ADMIN_PASSWORD=ci-only-bootstrap-password-123 \ + --env MAGENT_APPLICATION_URL=https://magent-ci.example.test \ magent:ci >/dev/null deadline=$((SECONDS + 120)) @@ -37,3 +38,27 @@ done docker exec "$container_name" curl --fail --silent --show-error http://127.0.0.1:8000/health >/dev/null docker exec "$container_name" curl --fail --silent --show-error http://127.0.0.1:3000/login >/dev/null + +# Exercise browser-origin requests, not only GET health checks. A configured +# public address must work even when CORS_ALLOW_ORIGIN has its localhost default. +docker exec -i "$container_name" python - <<'PY' +from urllib import error, request + +for path in ("/auth/login", "/auth/jellyfin/login"): + for origin, expected in ( + ("https://magent-ci.example.test", 422), + ("https://untrusted.example.test", 403), + ): + probe = request.Request( + "http://127.0.0.1:8000" + path, + data=b"", + headers={"Origin": origin, "Content-Type": "application/x-www-form-urlencoded"}, + ) + try: + response = request.urlopen(probe, timeout=10) + except error.HTTPError as exc: + response = exc + with response: + assert response.status == expected, (path, origin, response.status, expected) + print(f"Browser-origin login smoke: {path} {origin} -> {expected}") +PY