diff --git a/hermes_cli/dashboard_auth/base.py b/hermes_cli/dashboard_auth/base.py index 8f376f352108..e8b8a7730b1d 100644 --- a/hermes_cli/dashboard_auth/base.py +++ b/hermes_cli/dashboard_auth/base.py @@ -94,9 +94,11 @@ class InvalidCredentialsError(Exception): class RefreshExpiredError(Exception): - """The refresh token is dead. + """This provider rejects the refresh token as dead or invalid. - Middleware clears cookies and forces re-login (302 → ``/login``). + In a multi-provider deployment this does not prove token ownership, so + middleware may try remaining providers. It clears cookies and forces + re-login only after every reachable provider rejects the token. """ @@ -125,9 +127,13 @@ class DashboardAuthProvider(ABC): raises ``ProviderError`` if the IDP is unreachable. Middleware treats expiry and unreachable differently (expiry → refresh; unreachable → 503). - * ``refresh_session`` raises ``RefreshExpiredError`` when the - refresh token is also invalid; middleware then forces re-login. - Raises ``ProviderError`` on network failure. + * ``refresh_session`` raises ``RefreshExpiredError`` when the refresh + token is invalid for that provider. Middleware tries the remaining + providers because an opaque foreign token can be indistinguishable + from an expired one; it forces re-login only after every reachable + provider rejects the token. Raises ``ProviderError`` on network + failure; middleware still tries remaining providers, but returns 503 + without clearing cookies if none succeeds and any was unavailable. * ``revoke_session`` is best-effort and must not raise. Subclasses MUST set ``name`` (lowercase identifier, stable forever) diff --git a/hermes_cli/dashboard_auth/cookies.py b/hermes_cli/dashboard_auth/cookies.py index ef7f79b27d3f..8bcd9db78eb6 100644 --- a/hermes_cli/dashboard_auth/cookies.py +++ b/hermes_cli/dashboard_auth/cookies.py @@ -66,6 +66,10 @@ # request's HTTPS + prefix combination. SESSION_AT_COOKIE = "hermes_session_at" SESSION_RT_COOKIE = "hermes_session_rt" +# Provider that minted the session. This non-secret routing hint prevents a +# refresh token from being handed to the wrong provider when several dashboard +# auth plugins are enabled (for example Basic + Nous OAuth). +SESSION_PROVIDER_COOKIE = "hermes_session_provider" PKCE_COOKIE = "hermes_session_pkce" # One-shot loop-guard marker for the auto-SSO redirect (Phase 1, # cloud-auto-discovery). Set when the gate auto-initiates the portal OAuth @@ -141,6 +145,24 @@ def _common_attrs(*, use_https: bool, prefix: str) -> dict: return attrs +def set_session_provider_cookie( + response: Response, + *, + provider: str, + use_https: bool, + prefix: str = "", +) -> None: + """Persist the non-secret provider routing hint for token refresh.""" + if not provider: + return + response.set_cookie( + _resolved_name(SESSION_PROVIDER_COOKIE, use_https=use_https, prefix=prefix), + provider, + max_age=_RT_MAX_AGE, + **_common_attrs(use_https=use_https, prefix=prefix), + ) + + def set_session_cookies( response: Response, *, @@ -149,6 +171,7 @@ def set_session_cookies( access_token_expires_in: int, use_https: bool, prefix: str = "", + provider: str = "", ) -> None: """Set the session cookies on the response. @@ -181,6 +204,12 @@ def set_session_cookies( max_age=_RT_MAX_AGE, **_common_attrs(use_https=use_https, prefix=prefix), ) + set_session_provider_cookie( + response, + provider=provider, + use_https=use_https, + prefix=prefix, + ) def clear_session_cookies(response: Response, *, prefix: str = "") -> None: @@ -202,6 +231,10 @@ def clear_session_cookies(response: Response, *, prefix: str = "") -> None: f"{variant}{SESSION_RT_COOKIE}", "", max_age=0, path=path, httponly=True, samesite="lax", ) + response.set_cookie( + f"{variant}{SESSION_PROVIDER_COOKIE}", "", max_age=0, + path=path, httponly=True, samesite="lax", + ) def set_pkce_cookie( @@ -248,6 +281,11 @@ def read_session_cookies(request: Request) -> Tuple[Optional[str], Optional[str] return at, rt +def read_session_provider(request: Request) -> Optional[str]: + """Return the provider routing hint associated with the session cookies.""" + return _read_with_fallback(request, SESSION_PROVIDER_COOKIE) + + def read_pkce_cookie(request: Request) -> Optional[str]: return _read_with_fallback(request, PKCE_COOKIE) diff --git a/hermes_cli/dashboard_auth/middleware.py b/hermes_cli/dashboard_auth/middleware.py index 362ed729d65b..caa1b3a6e5dd 100644 --- a/hermes_cli/dashboard_auth/middleware.py +++ b/hermes_cli/dashboard_auth/middleware.py @@ -24,11 +24,17 @@ from hermes_cli.dashboard_auth import list_session_providers from hermes_cli.dashboard_auth.audit import AuditEvent, audit_log -from hermes_cli.dashboard_auth.base import ProviderError, RefreshExpiredError +from hermes_cli.dashboard_auth.base import ( + DashboardAuthProvider, + ProviderError, + RefreshExpiredError, +) from hermes_cli.dashboard_auth.cookies import ( clear_sso_attempt_cookie, read_session_cookies, + read_session_provider, read_sso_attempt_cookie, + set_session_provider_cookie, set_sso_attempt_cookie, ) from hermes_cli.dashboard_auth.public_paths import PUBLIC_API_PATHS @@ -83,6 +89,22 @@ def _client_ip(request: Request) -> str: return request.client.host if request.client else "" +def _ordered_session_providers( + provider_hint: str | None, +) -> list[DashboardAuthProvider]: + """Prefer the hinted provider without making the hint authoritative. + + The cookie can outlive a provider rename/removal or become stale after a + deployment change. A stable sort moves a matching provider to the front + while preserving registration order for every remaining candidate; an + unknown hint therefore leaves the normal scan unchanged. + """ + providers = list_session_providers() + if provider_hint: + providers.sort(key=lambda provider: provider.name != provider_hint) + return providers + + def _unauth_response(request: Request, *, reason: str) -> Response: """API routes → 401 JSON with ``login_url``; HTML routes → 302 → /login. @@ -276,6 +298,7 @@ async def gated_auth_middleware( return await call_next(request) at, _rt = read_session_cookies(request) + provider_hint = read_session_provider(request) if not at and not _rt: # Neither token present — no session at all. Nothing to verify or # refresh. Before falling back to the /login interstitial, try to @@ -321,7 +344,7 @@ async def gated_auth_middleware( # 503 — distinguishing "transient IDP outage" (don't force re-login) # from "token genuinely invalid" (fall through to refresh/relogin). unreachable_provider: str | None = None - for provider in list_session_providers(): + for provider in _ordered_session_providers(provider_hint): try: session = provider.verify_session(access_token=at) except ProviderError as e: @@ -353,9 +376,22 @@ async def gated_auth_middleware( # Access token is expired/invalid. Before forcing re-login, try to # rotate it using the refresh token (if the session cookie carries # one). On success we re-set the rotated cookies on the response and - # serve the request transparently; on RefreshExpiredError (RT dead / - # revoked / reuse-detected) we fall through to clear-and-relogin. - refreshed = _attempt_refresh(request, refresh_token=_rt) + # serve the request transparently; only after every provider rejects + # the RT do we fall through to clear-and-relogin. + try: + refreshed = _attempt_refresh( + request, + refresh_token=_rt, + provider_hint=provider_hint, + ) + except ProviderError as e: + # At least one provider could not confirm or reject the RT, and no + # other provider refreshed it. Preserve the cookies and surface a + # transient outage instead of turning uncertainty into a logout. + return JSONResponse( + {"detail": f"Auth provider {str(e)!r} unreachable"}, + status_code=503, + ) if refreshed is not None: new_session, refreshing_provider = refreshed request.state.session = new_session @@ -378,6 +414,7 @@ async def gated_auth_middleware( access_token_expires_in=_expires_in_seconds(new_session), use_https=detect_https(request), prefix=prefix_from_request(request), + provider=refreshing_provider, ) audit_log( AuditEvent.REFRESH_SUCCESS, @@ -405,7 +442,18 @@ async def gated_auth_middleware( return response request.state.session = session - return await call_next(request) + response = await call_next(request) + if not provider_hint and session.provider: + from hermes_cli.dashboard_auth.cookies import detect_https + from hermes_cli.dashboard_auth.prefix import prefix_from_request + + set_session_provider_cookie( + response, + provider=session.provider, + use_https=detect_https(request), + prefix=prefix_from_request(request), + ) + return response def _expires_in_seconds(session) -> int: @@ -421,33 +469,32 @@ def _expires_in_seconds(session) -> int: return max(60, int(session.expires_at) - int(time.time())) -def _attempt_refresh(request: Request, *, refresh_token): +def _attempt_refresh(request: Request, *, refresh_token, provider_hint: str | None = None): """Try to rotate an expired session via the refresh token. - Returns ``(new_session, provider_name)`` on success, or ``None`` if - there's no RT or every provider's ``refresh_session`` failed with - ``RefreshExpiredError`` (dead/revoked/reuse-detected RT → force re-login). - - A ``ProviderError`` (Portal unreachable) is NOT swallowed into a re-login - here — re-raising would 500 the request; instead we log and return None so - the caller forces a clean re-login, which is the safer UX than a hard - error on a transient network blip during the narrow refresh window. + The provider hint only changes candidate order. ``RefreshExpiredError`` + rejects the token for that candidate, but cannot prove ownership because + providers such as Basic raise it for foreign opaque tokens too. Likewise, + ``ProviderError`` only makes that candidate unavailable. Both are audited + and the remaining providers are tried. Returns ``None`` only when there is + no RT or every reachable provider rejects it. If no provider succeeds and + at least one raised ``ProviderError``, re-raises with that provider's name + so the caller can return 503 without clearing potentially valid cookies. """ if not refresh_token: return None - for provider in list_session_providers(): + unavailable_provider: str | None = None + for provider in _ordered_session_providers(provider_hint): try: new_session = provider.refresh_session(refresh_token=refresh_token) except RefreshExpiredError: - # This provider owns the RT but it's dead — stop trying others - # (an RT belongs to exactly one provider) and force re-login. audit_log( AuditEvent.REFRESH_FAILURE, provider=provider.name, reason="refresh_expired", ip=_client_ip(request), ) - return None + continue except ProviderError as e: _log.warning( "dashboard-auth: provider %r unreachable during refresh: %s", @@ -459,7 +506,11 @@ def _attempt_refresh(request: Request, *, refresh_token): reason="provider_unreachable", ip=_client_ip(request), ) - return None + if unavailable_provider is None: + unavailable_provider = provider.name + continue if new_session is not None: return new_session, provider.name + if unavailable_provider is not None: + raise ProviderError(unavailable_provider) return None diff --git a/hermes_cli/dashboard_auth/routes.py b/hermes_cli/dashboard_auth/routes.py index ee595be7d821..5b833e5df79a 100644 --- a/hermes_cli/dashboard_auth/routes.py +++ b/hermes_cli/dashboard_auth/routes.py @@ -365,6 +365,7 @@ async def auth_callback( access_token_expires_in=expires_in, use_https=detect_https(request), prefix=_prefix(request), + provider=session.provider, ) clear_pkce_cookie(resp, prefix=_prefix(request)) # Clear the one-shot auto-SSO loop-guard marker now that login succeeded, @@ -549,6 +550,7 @@ async def auth_password_login(request: Request, body: _PasswordLoginBody): access_token_expires_in=expires_in, use_https=detect_https(request), prefix=_prefix(request), + provider=session.provider, ) return resp diff --git a/scripts/release.py b/scripts/release.py index d13802599769..86b76b99f508 100755 --- a/scripts/release.py +++ b/scripts/release.py @@ -327,6 +327,7 @@ "290859878+synapsesx@users.noreply.github.com": "synapsesx", "157689911+itsflownium@users.noreply.github.com": "itsflownium", "dirtyren@users.noreply.github.com": "dirtyren", + "theoldwizard123@pm.me": "unsupportedpastels", "johnmlussier@gmail.com": "John-Lussier", "chenkun_lws@126.com": "bytesnail", # PR #60360 salvage (--yolo startup ordering; #60328) "iamgexin@qq.com": "nullptr0807", # PR #60956 salvage (gateway hygiene in-place compaction; #60947) diff --git a/tests/hermes_cli/test_dashboard_auth_401_reauth.py b/tests/hermes_cli/test_dashboard_auth_401_reauth.py index 458be58c7942..63ab76a9a833 100644 --- a/tests/hermes_cli/test_dashboard_auth_401_reauth.py +++ b/tests/hermes_cli/test_dashboard_auth_401_reauth.py @@ -33,8 +33,10 @@ from hermes_cli import web_server from hermes_cli.dashboard_auth import clear_providers, register_provider +from hermes_cli.dashboard_auth.base import ProviderError, RefreshExpiredError from hermes_cli.dashboard_auth.cookies import ( SESSION_AT_COOKIE, + SESSION_PROVIDER_COOKIE, SESSION_RT_COOKIE, clear_session_cookies, set_session_cookies, @@ -252,6 +254,156 @@ def test_at_evicted_rt_present_refreshes_transparently(self, gated_app): for c in set_cookies ), f"no rotated RT cookie in {set_cookies!r}" + def test_provider_hint_routes_refresh_to_token_owner(self, gated_app): + """A Nous-style RT must not be rejected by Basic just because Basic + was registered first. The non-secret provider hint routes directly to + the provider that minted the session.""" + class WrongProvider(StubAuthProvider): + name = "basic" + + def __init__(self): + super().__init__() + self.refresh_calls = 0 + + def refresh_session(self, *, refresh_token: str): + self.refresh_calls += 1 + raise AssertionError("foreign refresh token reached Basic provider") + + wrong = WrongProvider() + _provider, valid_rt = self._build_rt_only_app() + clear_providers() + register_provider(wrong) + register_provider(StubAuthProvider(default_ttl=900)) + gated_app.cookies.clear() + gated_app.cookies.set(SESSION_RT_COOKIE, valid_rt) + gated_app.cookies.set(SESSION_PROVIDER_COOKIE, "stub") + + response = gated_app.get("/api/sessions", follow_redirects=False) + + assert response.status_code == 200 + assert wrong.refresh_calls == 0 + assert any( + SESSION_PROVIDER_COOKIE in cookie and "stub" in cookie + for cookie in response.headers.get_list("set-cookie") + ) + + def test_unknown_provider_hint_retains_verify_fallback(self, gated_app): + """A hint for a removed provider must not suppress the normal scan.""" + import time as _t + from tests.hermes_cli.conftest_dashboard_auth import _sign + + valid_at = _sign({ + "sub": "stub-user-1", + "email": "stub@example.test", + "name": "Stub User", + "org_id": "stub-org-1", + "exp": int(_t.time()) + 900, + }) + gated_app.cookies.clear() + gated_app.cookies.set(SESSION_AT_COOKIE, valid_at) + gated_app.cookies.set(SESSION_PROVIDER_COOKIE, "removed-provider") + + response = gated_app.get("/api/auth/me") + + assert response.status_code == 200 + assert response.json()["provider"] == "stub" + + @pytest.mark.parametrize( + "error_type", + [RefreshExpiredError, ProviderError], + ids=["token-rejected", "provider-unreachable"], + ) + def test_stale_provider_hint_refresh_error_falls_back( + self, + gated_app, + error_type, + ): + """A stale known hint may reject a foreign RT or be unavailable. + + Either failure applies only to that provider candidate; remaining + providers still get a chance to claim the token. + """ + class StaleHintProvider(StubAuthProvider): + name = "basic" + + def __init__(self): + super().__init__() + self.refresh_calls = 0 + + def refresh_session(self, *, refresh_token: str): + self.refresh_calls += 1 + raise error_type("foreign refresh token") + + stale = StaleHintProvider() + _provider, valid_rt = self._build_rt_only_app() + clear_providers() + register_provider(stale) + register_provider(StubAuthProvider(default_ttl=900)) + gated_app.cookies.clear() + gated_app.cookies.set(SESSION_RT_COOKIE, valid_rt) + gated_app.cookies.set(SESSION_PROVIDER_COOKIE, "basic") + + response = gated_app.get("/api/sessions", follow_redirects=False) + + assert response.status_code == 200 + assert stale.refresh_calls == 1 + assert any( + SESSION_PROVIDER_COOKIE in cookie and "stub" in cookie + for cookie in response.headers.get_list("set-cookie") + ) + + def test_refresh_outage_returns_503_without_clearing_cookies(self, gated_app): + """Uncertain ownership during an outage must not log the user out.""" + class UnreachableProvider(StubAuthProvider): + name = "unreachable" + + def refresh_session(self, *, refresh_token: str): + raise ProviderError("simulated provider outage") + + class RejectingProvider(StubAuthProvider): + name = "rejecting" + + def refresh_session(self, *, refresh_token: str): + raise RefreshExpiredError("foreign refresh token") + + clear_providers() + register_provider(UnreachableProvider()) + register_provider(RejectingProvider()) + gated_app.cookies.clear() + gated_app.cookies.set(SESSION_RT_COOKIE, "opaque-refresh-token") + gated_app.cookies.set(SESSION_PROVIDER_COOKIE, "unreachable") + + response = gated_app.get("/api/sessions", follow_redirects=False) + + assert response.status_code == 503 + assert gated_app.cookies.get(SESSION_RT_COOKIE) == "opaque-refresh-token" + assert not any( + SESSION_RT_COOKIE in cookie and "Max-Age=0" in cookie + for cookie in response.headers.get_list("set-cookie") + ) + + def test_valid_legacy_session_is_migrated_with_provider_hint(self, gated_app): + import time as _t + from tests.hermes_cli.conftest_dashboard_auth import _sign + + valid_at = _sign({ + "sub": "stub-user-1", + "email": "stub@example.test", + "name": "Stub User", + "org_id": "stub-org-1", + "exp": int(_t.time()) + 900, + }) + gated_app.cookies.clear() + gated_app.cookies.set(SESSION_AT_COOKIE, valid_at) + + response = gated_app.get("/api/sessions") + + assert response.status_code == 200 + assert any( + SESSION_PROVIDER_COOKIE in cookie and "stub" in cookie + for cookie in response.headers.get_list("set-cookie") + ) + def test_no_cookies_at_all_still_bounces(self, gated_app): """Guard the fix didn't over-reach: a request with NEITHER cookie must still 401 to login (nothing to verify or refresh).""" diff --git a/tests/hermes_cli/test_dashboard_auth_cookies.py b/tests/hermes_cli/test_dashboard_auth_cookies.py index 7109b7b70993..3f0baaa4dbb2 100644 --- a/tests/hermes_cli/test_dashboard_auth_cookies.py +++ b/tests/hermes_cli/test_dashboard_auth_cookies.py @@ -9,11 +9,13 @@ from hermes_cli.dashboard_auth.cookies import ( PKCE_COOKIE, SESSION_AT_COOKIE, + SESSION_PROVIDER_COOKIE, SESSION_RT_COOKIE, clear_pkce_cookie, clear_session_cookies, read_pkce_cookie, read_session_cookies, + read_session_provider, set_pkce_cookie, set_session_cookies, ) @@ -28,7 +30,7 @@ def set_endpoint(): set_session_cookies( r, access_token="AT", refresh_token="RT", access_token_expires_in=3600, use_https=use_https, - prefix=prefix, + prefix=prefix, provider="nous", ) return r @@ -63,7 +65,8 @@ def test_session_cookies_use_host_prefix_on_https_direct(): cookies = r.headers.get_list("set-cookie") at = next(c for c in cookies if c.startswith(f"__Host-{SESSION_AT_COOKIE}=")) rt = next(c for c in cookies if c.startswith(f"__Host-{SESSION_RT_COOKIE}=")) - for c in (at, rt): + provider = next(c for c in cookies if c.startswith(f"__Host-{SESSION_PROVIDER_COOKIE}=nous")) + for c in (at, rt, provider): assert "HttpOnly" in c assert "samesite=lax" in c.lower() assert "Secure" in c @@ -128,6 +131,9 @@ def test_clear_session_cookies_emits_expired_at_and_rt(): assert any( SESSION_RT_COOKIE in c and "Max-Age=0" in c for c in cookies ) + assert any( + SESSION_PROVIDER_COOKIE in c and "Max-Age=0" in c for c in cookies + ) def test_pkce_cookie_short_ttl_and_path_root(): @@ -160,6 +166,19 @@ def test_read_session_cookies_from_request_bare_name(): assert rt == "rt_value" +def test_read_session_provider_from_request(): + scope = { + "type": "http", + "method": "GET", + "path": "/", + "headers": [( + b"cookie", + f"__Host-{SESSION_PROVIDER_COOKIE}=nous".encode(), + )], + } + assert read_session_provider(Request(scope)) == "nous" + + def test_read_session_cookies_from_request_host_prefix(): """Reader also finds cookies set with the __Host- variant (HTTPS direct deploy)."""