fix(dashboard-auth): drop /api/* paths from OAuth next= round trip (#36244)
When an unauthenticated SPA fetch hit a gated /api/* endpoint (e.g.
GET /api/analytics/models?days=30 fired from ModelsPage on mount or
after a session expiry), the gated middleware stamped the request's
own path into next= on the 401 envelope's login_url. The SPA's global
401 handler in web/src/lib/api.ts full-page-navigated to that URL,
the PKCE cookie carried the encoded /api/* value through the OAuth
round trip to Portal, and /auth/callback's _validate_post_login_target
accepted it as same-origin and redirected the user to the raw JSON
endpoint instead of the dashboard.
Symptom Ben reported: after the OAuth screen he kept landing on
$DOMAIN/api/analytics/models?days=30 (raw JSON) rather than /models.
The bug was deterministic per page — whichever /api/* call ModelsPage,
AnalyticsPage, or SessionsPage fired first owned the redirect race.
Fix: both validators now reject /api/* targets in addition to the
existing /login, /auth/, /api/auth/ exclusions:
- _safe_next_target in middleware.py drops the value before it ever
enters login_url, so the SPA's 401 handler navigates to a bare
/login (which the SPA itself can return-from via its own
sessionStorage["hermes.lastLocation"] fallback that was already
saving the actual browser location).
- _validate_post_login_target in routes.py drops it as second-line
defence at the callback boundary, so a legacy cookie, a regressed
middleware, or an attacker-crafted /auth/login?next=/api/... value
can't smuggle the redirect through. Either layer alone is enough;
pairing them means a regression in one is caught by the other.
The match is anchored: ``decoded == "/api"`` or
``decoded.startswith("/api/")``. SPA route lookalikes like /apidocs
or /api-keys remain valid landing targets — tests pin that.
Test additions in test_dashboard_auth_401_reauth.py:
- TestApi401Envelope: rewrote test_login_url_carries_next_for_deep_
api_path (which asserted the pre-fix behaviour) as
test_login_url_drops_next_for_deep_api_path, plus added the
specific analytics-models repro case from Ben's report.
- TestNextSameOriginValidation: rejects-api-paths + does-not-reject-
api-prefix-lookalikes (covers /apidocs, /api-keys).
- TestAuthCallbackNext: end-to-end test_callback_with_api_next_
lands_at_root drives /auth/login?next=/api/... through to the
callback and asserts the user lands at "/", not the API URL.
- TestValidatePostLoginTarget: new class covering the callback-side
validator directly, including the URL-encoded ``%2Fapi%2F...``
form the PKCE cookie actually carries.
Mutation-tested: reverting both validators causes exactly the 5 new
or rewritten /api/*-related assertions to fail (each fix layer is
independently tested), while the 31 other assertions in the file
remain green. Full tests/hermes_cli/ suite (288 files, 5,938 tests)
passes with the fix applied.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user