diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index 3400a0cd9..05330bc15 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/hermes_cli/dashboard_auth/middleware.py @@ -150,6 +150,16 @@ def _safe_next_target(request: Request) -> str: for p in ("/login", "/auth/", "/api/auth/") ): return "" + # Reject ALL ``/api/*`` paths. The 401-envelope code path fires for + # any unauthenticated SPA fetch (e.g. ``GET /api/analytics/models`` + # from ModelsPage), and the SPA's global 401 handler full-page + # navigates to ``login_url``. After the OAuth round trip the user + # would land on the API URL and see raw JSON instead of the + # dashboard. SPA routes survive (they don't start with ``/api/``); + # the SPA's own ``sessionStorage["hermes.lastLocation"]`` fallback + # in ``web/src/lib/api.ts`` covers the deep-link case. + if path == "/api" or path.startswith("/api/"): + return "" # Preserve query string if present (e.g. /sessions?page=2). query = request.url.query target = f"{path}?{query}" if query else path diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index 50d464599..637cb7048 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -365,6 +365,15 @@ def _validate_post_login_target(raw: str) -> str: for p in ("/login", "/auth/", "/api/auth/") ): return "" + # Reject any ``/api/*`` target. The gate's ``_safe_next_target`` + # already filters these out before they reach the cookie, but a + # malicious or stale ``next=`` value that re-enters via the + # callback URL must not be honoured: a successful redirect to an + # API endpoint renders raw JSON in the browser address bar — never + # a useful post-login destination, and indistinguishable from an + # attacker trying to weaponise the redirect. + if decoded == "/api" or decoded.startswith("/api/"): + return "" return decoded diff --git a/tests/hermes_cli/test_dashboard_auth_401_reauth.py b/tests/hermes_cli/test_dashboard_auth_401_reauth.py index 28d410dc3..e4b1a0440 100644 --- a/tests/hermes_cli/test_dashboard_auth_401_reauth.py +++ b/tests/hermes_cli/test_dashboard_auth_401_reauth.py @@ -163,12 +163,33 @@ class TestApi401Envelope: for c in set_cookies ) - def test_login_url_carries_next_for_deep_api_path(self, gated_app): + def test_login_url_drops_next_for_deep_api_path(self, gated_app): + """Bug fix: ``/api/*`` paths must NOT round-trip into ``next=``. + + Before the fix, an unauthenticated SPA fetch like ``GET + /api/analytics/models?days=30`` from ModelsPage round-tripped + through the OAuth dance and landed the user on the raw JSON + endpoint instead of the dashboard. The gate now drops API paths + from ``next=`` entirely; the SPA's own ``hermes.lastLocation`` + fallback in ``web/src/lib/api.ts`` covers the deep-link case. + """ r = gated_app.get("/api/sessions?page=2") body = r.json() - # next= is URL-encoded. - assert "next=" in body["login_url"] - assert quote("/api/sessions?page=2", safe="") in body["login_url"] + # ``login_url`` is the bare ``/login`` (no ``next=``) — the + # post-callback landing falls back to "/" rather than the API + # URL. + assert body["login_url"] == "/login" + assert "next=" not in body["login_url"] + + def test_login_url_drops_next_for_analytics_path(self, gated_app): + """Specific repro for the ``/api/analytics/models?days=30`` + case Ben reported: page on /models, session expires, SPA fires + getModelsAnalytics(), 401 envelope carries ``next=``, user ends + up staring at JSON post-callback.""" + r = gated_app.get("/api/analytics/models?days=30") + body = r.json() + assert body["login_url"] == "/login" + assert "next=" not in body["login_url"] class TestHtmlRedirectNext: @@ -253,6 +274,46 @@ class TestNextSameOriginValidation: assert _safe_next_target(FakeRequest("/auth/login")) == "" assert _safe_next_target(FakeRequest("/api/auth/me")) == "" + def test_safe_next_validator_rejects_api_paths(self): + """``/api/*`` paths must not round-trip through ``next=``. + + Any API URL is a JSON endpoint; landing the browser there after + OAuth shows raw JSON instead of the dashboard. This is the bug + fix that closes the analytics-page redirect mishap. + """ + from hermes_cli.dashboard_auth.middleware import _safe_next_target + + class FakeRequest: + def __init__(self, path, query=""): + self.url = type("URL", (), {"path": path, "query": query})() + + assert _safe_next_target(FakeRequest("/api/analytics/models")) == "" + assert ( + _safe_next_target(FakeRequest("/api/analytics/models", "days=30")) + == "" + ) + assert _safe_next_target(FakeRequest("/api/sessions")) == "" + assert _safe_next_target(FakeRequest("/api/config")) == "" + assert _safe_next_target(FakeRequest("/api/status")) == "" + # Exact ``/api`` (no trailing slash) also rejected — the dashboard + # has no such SPA route, but pinning the boundary keeps the rule + # crisp. + assert _safe_next_target(FakeRequest("/api")) == "" + + def test_safe_next_validator_does_not_reject_api_prefix_lookalikes(self): + """Negative guard: ``/api-docs`` or ``/apis`` aren't ``/api/*`` + and must remain valid landing targets.""" + from hermes_cli.dashboard_auth.middleware import _safe_next_target + + class FakeRequest: + def __init__(self, path): + self.url = type("URL", (), {"path": path, "query": ""})() + + # ``/apidocs`` or ``/api-keys`` lookalike SPA routes — we must + # only match the ``/api/`` prefix or exact ``/api``. + assert _safe_next_target(FakeRequest("/apidocs")) == "%2Fapidocs" + assert _safe_next_target(FakeRequest("/api-keys")) == "%2Fapi-keys" + # --------------------------------------------------------------------------- # /auth/callback honours next= and validates it @@ -395,6 +456,91 @@ class TestAuthCallbackNext: # NOT /internal-admin. assert r.headers["location"] == "/" + def test_callback_with_api_next_lands_at_root(self, gated_app): + """End-to-end repro of the analytics-redirect bug. + + Drive ``/auth/login?next=/api/analytics/models?days=30`` — + exactly what the pre-fix gate would have stamped after a + ModelsPage 401. The validator at /auth/login MUST now drop + ``/api/*`` so the PKCE cookie never carries the API path, AND + the callback's ``_validate_post_login_target`` MUST drop it as + second-line defence. Either layer alone is enough; both means + a regression in one is caught by the other. + + Discrimination: under the pre-fix code, both validators + accepted ``/api/*`` and the callback redirected to the raw + JSON endpoint. With the fix, the callback redirects to "/". + """ + api_next = "/api/analytics/models?days=30" + r_to_idp = gated_app.get( + f"/auth/login?provider=stub&next={quote(api_next, safe='')}", + follow_redirects=False, + ) + state = r_to_idp.headers["location"].split("state=")[1] + r = gated_app.get( + f"/auth/callback?code=stub_code&state={state}", + follow_redirects=False, + ) + assert r.status_code == 302 + # Landing falls back to "/" — NOT the API URL. + assert r.headers["location"] == "/" + + +# --------------------------------------------------------------------------- +# Unit-level coverage: _validate_post_login_target on the callback boundary +# --------------------------------------------------------------------------- + + +class TestValidatePostLoginTarget: + """Cover ``_validate_post_login_target`` directly — it's the second + half of the next= validator pair (the callback boundary). The gate + side has matching coverage in ``TestNextSameOriginValidation``. + """ + + def test_accepts_same_origin_paths(self): + from hermes_cli.dashboard_auth.routes import _validate_post_login_target + assert _validate_post_login_target("/sessions") == "/sessions" + # URL-encoded form (as the cookie carries it) round-trips through + # the validator's unquote step. + assert ( + _validate_post_login_target("%2Fsessions%3Fpage%3D2") + == "/sessions?page=2" + ) + + def test_rejects_protocol_relative(self): + from hermes_cli.dashboard_auth.routes import _validate_post_login_target + assert _validate_post_login_target("//evil.com") == "" + assert _validate_post_login_target("%2F%2Fevil.com") == "" + + def test_rejects_login_loop(self): + from hermes_cli.dashboard_auth.routes import _validate_post_login_target + assert _validate_post_login_target("/login") == "" + assert _validate_post_login_target("/auth/login") == "" + assert _validate_post_login_target("/api/auth/me") == "" + + def test_rejects_api_paths(self): + """Bug fix: any ``/api/*`` target is dropped at the callback + boundary. Pin both the exact match and the trailing-slash forms + plus a few realistic SPA-API endpoints.""" + from hermes_cli.dashboard_auth.routes import _validate_post_login_target + assert _validate_post_login_target("/api") == "" + assert _validate_post_login_target("/api/analytics/models") == "" + assert _validate_post_login_target("/api/analytics/models?days=30") == "" + assert _validate_post_login_target("/api/sessions") == "" + assert _validate_post_login_target("/api/config") == "" + # URL-encoded form — what the cookie actually carries. + assert ( + _validate_post_login_target( + "%2Fapi%2Fanalytics%2Fmodels%3Fdays%3D30" + ) == "" + ) + + def test_does_not_reject_api_prefix_lookalikes(self): + from hermes_cli.dashboard_auth.routes import _validate_post_login_target + # SPA route lookalikes — must NOT be dropped. + assert _validate_post_login_target("/apidocs") == "/apidocs" + assert _validate_post_login_target("/api-keys") == "/api-keys" + # --------------------------------------------------------------------------- # Unit-level coverage: render_login_html threads next= into provider buttons