From f0f7f55f76a058bf8d5fa27baadfadde04ec47e4 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Wed, 2 Sep 2026 09:20:55 -0700 Subject: [PATCH] =?UTF-8?q?refactor(web):=20dashboard=5Fauth/web=5Frouters?= =?UTF-8?q?/kanban=20dedupe=20+=20=5Fcommon=20helpers=20(resume=20?= =?UTF-8?q?=E2=80=94=20verified=20partial=20work)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- hermes_cli/dashboard_auth/base.py | 274 +-- hermes_cli/dashboard_auth/cookies.py | 433 ++-- hermes_cli/dashboard_auth/login_page.py | 85 +- hermes_cli/dashboard_auth/middleware.py | 538 ++--- hermes_cli/dashboard_auth/native_flow.py | 196 +- hermes_cli/dashboard_auth/prefix.py | 148 +- hermes_cli/dashboard_auth/request_utils.py | 54 + hermes_cli/dashboard_auth/routes.py | 902 +++------ hermes_cli/dashboard_auth/token_auth.py | 115 +- hermes_cli/web_deps.py | 71 +- hermes_cli/web_routers/_common.py | 81 + hermes_cli/web_routers/cron.py | 147 +- hermes_cli/web_routers/git.py | 32 +- hermes_cli/web_routers/mcp.py | 225 +-- hermes_cli/web_routers/sessions.py | 544 ++--- hermes_cli/web_routers/skills.py | 309 +-- hermes_cli/web_routers/tools.py | 508 ++--- plugins/kanban/dashboard/plugin_api.py | 2116 ++++++-------------- 18 files changed, 2172 insertions(+), 4606 deletions(-) create mode 100644 hermes_cli/dashboard_auth/request_utils.py create mode 100644 hermes_cli/web_routers/_common.py diff --git a/hermes_cli/dashboard_auth/base.py b/hermes_cli/dashboard_auth/base.py index 02db55f65e..9559ded817 100644 --- a/hermes_cli/dashboard_auth/base.py +++ b/hermes_cli/dashboard_auth/base.py @@ -8,11 +8,10 @@ from typing import Optional @dataclass(frozen=True) class Session: - """A verified identity. Returned by ``complete_login`` and ``verify_session``. + """A verified interactive identity (from ``complete_login`` / ``verify_session``). - All fields are mandatory. Providers that don't have a concept of orgs - should set ``org_id`` to an empty string. ``access_token`` and - ``refresh_token`` are opaque to Hermes — provider-specific. + All fields mandatory; providers without orgs set ``org_id=""``. The tokens + are opaque to Hermes. """ user_id: str @@ -27,25 +26,11 @@ class Session: @dataclass(frozen=True) class TokenPrincipal: - """A verified non-interactive (service-to-service) caller. + """A verified non-interactive (service-to-service) caller — the token analog + of :class:`Session`: one bearer token on one request, no login/cookie/refresh. - The token analog of :class:`Session`. Where a ``Session`` represents an - interactive human identity behind a session cookie, a ``TokenPrincipal`` - represents a machine/service caller that authenticated by presenting a - bearer token in the ``Authorization`` request header on a single - request — no login, no cookie, no refresh. - - Returned by :meth:`DashboardAuthProvider.verify_token` and attached to - ``request.state.token_principal`` by the token-auth middleware seam so a - route handler can see *who* called it. - - Fields: - * ``principal`` — stable identifier for the caller (e.g. the provider - name, a service account id, or an agent id). Opaque to the seam. - * ``provider`` — the ``name`` of the provider that verified the token. - * ``scopes`` — capability strings this principal is authorised for. - Empty tuple means "unscoped" (the provider vouches for the caller but - attaches no capability list); a route MAY enforce a required scope. + ``principal`` is an opaque stable caller id; ``scopes`` empty means + "unscoped" (a route MAY enforce a required scope). """ principal: str @@ -57,20 +42,11 @@ class TokenPrincipal: class LoginStart: """First leg of the OAuth round trip. - ``redirect_url`` is the URL the browser must navigate to (e.g. the - Portal's ``/oauth/authorize``). ``cookie_payload`` is a dict of cookie - name → serialised value that the auth route will ``Set-Cookie`` on the - response. Used for PKCE state, CSRF nonces, etc. Cookies set here MUST - be HttpOnly + Secure (when over HTTPS) with a TTL ≤ 10 minutes (the - login lifetime). - - SameSite: use ``Lax`` by default. The one exception is the PKCE state - cookie, which is ``SameSite=None; Secure`` over HTTPS — it is set on - the ``/auth/login`` 302 and has to survive the cross-site redirect - chain back from the IDP, which Chromium drops intermittently under - ``Lax`` (crbug 40508226). Over plain HTTP it stays ``Lax``, since - ``SameSite=None`` requires ``Secure``. See - :func:`hermes_cli.dashboard_auth.cookies.set_pkce_cookie`. + ``redirect_url`` is where the browser goes (the IDP's authorize endpoint); + ``cookie_payload`` maps cookie name -> serialised PKCE/CSRF state that the + auth route sets (HttpOnly, Secure over HTTPS, TTL <= 10 min). The PKCE + cookie is ``SameSite=None; Secure`` over HTTPS so it survives the cross-site + redirect chain — see :func:`hermes_cli.dashboard_auth.cookies.set_pkce_cookie`. """ redirect_url: str @@ -78,61 +54,35 @@ class LoginStart: class ProviderError(Exception): - """IDP unreachable, network error, or other transient failure. - - Middleware translates this to HTTP 503. - """ + """IDP unreachable / transient failure. Middleware -> HTTP 503.""" class InvalidCodeError(Exception): - """The OAuth callback ``code`` / ``state`` failed validation. - - Middleware translates this to HTTP 400. - """ + """OAuth callback ``code``/``state`` failed validation. Middleware -> HTTP 400.""" class InvalidCredentialsError(Exception): - """A username/password pair was rejected by a password provider. - - Raised by :meth:`DashboardAuthProvider.complete_password_login`. The - ``/auth/password-login`` route translates this to HTTP 401 with a - deliberately generic detail (never distinguishing "unknown user" from - "wrong password") so the endpoint can't be used as a username oracle. - """ + """Username/password rejected. The route answers a generic 401 (never + distinguishing unknown user from wrong password — no username oracle).""" class RefreshExpiredError(Exception): - """This provider rejects the refresh token as dead or invalid. - - 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. - """ + """This provider rejects the refresh token. Not proof of ownership in a + multi-provider deployment, so middleware tries the remaining providers and + forces re-login only after every reachable one rejects it.""" def classify_jwks_lookup_error(exc: BaseException) -> Exception: """Map a ``PyJWKClient.get_signing_key_from_jwt`` failure to the protocol. - Only a genuine transport failure (the IDP's JWKS endpoint could not be - fetched) is a :class:`ProviderError` — middleware turns that into 503 - "auth provider unreachable" so a flaky IDP never forces a logout. - - Everything else means the token itself cannot be verified by this - provider and is an :class:`InvalidCodeError` (``verify_session`` returns - ``None``, the middleware tries the next provider / refresh / 401): - - * ``jwt.DecodeError`` — the bearer is not a JWT at all (an opaque peer - key, a legacy session token, garbage). #94558: hosted agents answered - every non-JWT bearer with a fast 503 ``Auth provider 'nous' - unreachable`` even though Portal was healthy, because "cannot parse" - and "cannot reach" were folded into one branch. - * ``jwt.PyJWKSetError`` — the JWKS was fetched fine but holds no key for - this token's ``kid`` (rotated/foreign key). The provider was reached; - the token is simply not one of ours. - - ``PyJWKClientConnectionError`` is the only ``PyJWKClientError`` subclass - that denotes unreachability; a bare ``PyJWKClientError`` (unexpected - JWKS shape) is kept as a provider fault since the IDP misbehaved. + Only a genuine transport failure (``PyJWKClientConnectionError``, or an + unexpected JWKS shape = IDP misbehaving) is a :class:`ProviderError` (503, + never forces logout). A non-JWT bearer (``DecodeError``), a JWKS with no key + for this ``kid`` (``PyJWKSetError``) or any other invalid token means the + token is simply not verifiable by this provider -> :class:`InvalidCodeError` + (``verify_session`` returns ``None``; next provider / refresh / 401). + Folding "cannot parse" into "cannot reach" once made every opaque bearer a + fast 503 against a healthy Portal. """ try: import jwt @@ -152,83 +102,37 @@ def classify_jwks_lookup_error(exc: BaseException) -> Exception: class DashboardAuthProvider(ABC): """Protocol every dashboard-auth provider plugin implements. - Lifecycle: - 1. ``start_login`` — user clicks "Log in with X" on the login page. - Provider returns a redirect URL and any PKCE/CSRF state to stash - in short-lived cookies. - 2. Browser bounces through the OAuth IDP and lands at /auth/callback. - 3. ``complete_login`` — exchange the code + verifier for a Session. - 4. ``verify_session`` — called on every request to validate the - access token in the cookie. Returns ``None`` if the token is - expired or invalid (middleware then triggers refresh or logout). - 5. ``refresh_session`` — called when the access token is near expiry. - Returns a new Session with rotated tokens. - 6. ``revoke_session`` — called on /auth/logout. Best-effort. + Lifecycle: ``start_login`` (redirect URL + PKCE state for cookies) -> + IDP -> ``complete_login`` (code + verifier -> Session) -> + ``verify_session`` on every request -> ``refresh_session`` near expiry -> + ``revoke_session`` on logout (best-effort, must not raise). Failure semantics: - * ``start_login`` may raise ``ProviderError`` if the IDP is - unreachable. - * ``complete_login`` raises ``InvalidCodeError`` on bad code/state; - ``ProviderError`` if the IDP is unreachable. - * ``verify_session`` returns ``None`` on expiry / unknown token; - 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 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. + * ``start_login`` / ``complete_login`` raise ``ProviderError`` when the + IDP is unreachable; ``complete_login`` raises ``InvalidCodeError`` on + a bad code/state. + * ``verify_session`` returns ``None`` for expired/unknown tokens and + raises ``ProviderError`` when unreachable — middleware refreshes on + the former and answers 503 on the latter. + * ``refresh_session`` raises ``RefreshExpiredError`` when the token is + invalid for that provider (an opaque foreign token is + indistinguishable from an expired one, so middleware tries the rest) + and ``ProviderError`` on network failure (503 without clearing cookies + if none succeeds). - Subclasses MUST set ``name`` (lowercase identifier, stable forever) - and ``display_name`` (user-facing label on the login page). - - Password (non-redirect) providers: - A provider that authenticates with a username + password instead of - an OAuth redirect sets ``supports_password = True`` and implements - ``complete_password_login``. The login page then renders a - credential form (POSTing to ``/auth/password-login``) instead of a - "Log in with X" redirect button. Everything downstream of login — - ``verify_session`` / ``refresh_session`` / ``revoke_session``, the - session cookies, the WS-ticket mint — is identical to the OAuth - path, because a password session is just a :class:`Session` with - provider-minted opaque tokens. The OAuth methods (``start_login`` / - ``complete_login``) remain abstract; a pure-password provider that - will never be reached via the redirect flow may implement them as - stubs that raise ``NotImplementedError``. + Subclasses MUST set ``name`` (stable lowercase id) and ``display_name``. + Capability flags: ``supports_password`` (renders a credential form and + implements ``complete_password_login``; ``start_login``/``complete_login`` + may be ``NotImplementedError`` stubs), ``supports_token`` (implements + ``verify_token`` for the token-auth seam), ``supports_session`` (False for + token-only credentials such as drain, which are never offered a login). + Everything downstream of login is identical for every kind of session. """ name: str = "" display_name: str = "" - - # When True, this provider authenticates via username + password - # (``complete_password_login``) rather than (or in addition to) the - # OAuth redirect flow. The login page renders a credential form for - # such providers; the ``/auth/password-login`` route dispatches to - # ``complete_password_login``. OAuth-only providers leave this False - # and are completely unaffected. supports_password: bool = False - - # When True, this provider can verify a non-interactive bearer token - # (``verify_token``) presented on a single request by a service-to-service - # caller — no login, no cookie, no refresh. This is the generic - # API-token capability flag, mirroring ``supports_password``: a route - # opts into token auth (see ``token_auth`` middleware seam) and the - # gate consults every ``supports_token`` provider in turn until one - # recognises the token. OAuth/password providers leave this False and - # are completely unaffected. The drain bearer-secret plugin is the - # first consumer, but the capability is deliberately generic so any - # future machine-credential provider drops in without core changes. supports_token: bool = False - - # When True, this provider does the interactive cookie-session flow (login, - # verify, refresh). The login page, /auth/login, and the gate's - # verify/refresh loops consult only supports_session providers, so a - # token-only credential (e.g. drain) is never offered a login. Mirrors - # supports_token. supports_session: bool = True @abstractmethod @@ -258,24 +162,11 @@ class DashboardAuthProvider(ABC): ) -> "Session": """Verify a username/password pair and mint a :class:`Session`. - Only called when ``supports_password`` is True (the - ``/auth/password-login`` route guards on the flag). The default - raises ``NotImplementedError`` so an OAuth-only provider that - forgets to set the flag fails loudly rather than silently - accepting credentials. - - The returned ``Session`` carries provider-minted opaque - ``access_token`` / ``refresh_token`` exactly like the OAuth path, - so all downstream session handling (cookies, verify, refresh, - ws-tickets, logout) is identical. - - Failure semantics: - * ``InvalidCredentialsError`` — username/password rejected. The - route surfaces a generic 401 (no user-vs-password - distinction). Implementations SHOULD spend constant time on - unknown users (dummy hash verify) to avoid a timing oracle. - * ``ProviderError`` — the backing credential store is - unreachable (LDAP/DB down); the route surfaces 503. + Only called when ``supports_password`` is True. Raise + ``InvalidCredentialsError`` on rejection (SHOULD spend constant time + on unknown users to avoid a timing oracle) and ``ProviderError`` when + the credential store is unreachable. The default raises so a provider + that forgets the flag fails loudly rather than accepting credentials. """ raise NotImplementedError( f"{type(self).__name__} does not support password login " @@ -286,30 +177,12 @@ class DashboardAuthProvider(ABC): def verify_token(self, *, token: str) -> "Optional[TokenPrincipal]": """Verify a non-interactive bearer token; return its principal. - The token analog of ``verify_session``. Only consulted when - ``supports_token`` is True. Called by the ``token_auth`` middleware - seam for every request to a token-authable route, in registration - order, until one provider returns a non-None principal. - - Contract (mirrors ``verify_session`` stacking semantics): - * Return a :class:`TokenPrincipal` if this provider recognises and - accepts the token. - * Return ``None`` for a token this provider does NOT recognise — - never raise, so the seam can fall through to the next provider. - A malformed/expired/wrong token is "not recognised" → ``None``. - * Raise ``ProviderError`` ONLY for a genuine backing-store outage - (the provider can neither confirm nor deny). The seam treats this - like ``verify_session``: remember it, keep trying other providers, - and surface 503 only if NO provider accepts the token AND at least - one was unreachable. - - Implementations MUST use a constant-time comparison - (``hmac.compare_digest``) when matching a shared secret so the - endpoint isn't a timing oracle. - - The default raises ``NotImplementedError`` so a provider that sets - ``supports_token`` but forgets to implement this fails loudly rather - than silently accepting every caller. + Stacking mirrors ``verify_session``: return ``None`` (never raise) for + a token this provider does not recognise so the seam falls through; + raise ``ProviderError`` ONLY for a genuine backing-store outage (the + seam surfaces 503 only if no provider accepts the token). Shared + secrets MUST be compared with ``hmac.compare_digest``. The default + raises so a mis-flagged provider fails loudly. """ raise NotImplementedError( f"{type(self).__name__} does not support token auth " @@ -320,32 +193,19 @@ class DashboardAuthProvider(ABC): def assert_protocol_compliance(cls: type) -> None: """Raise ``TypeError`` if ``cls`` doesn't fully implement the provider protocol. - Call this in every provider plugin's unit tests:: - - def test_protocol_compliance(): - assert_protocol_compliance(MyProvider) - - Returns ``None`` on success so callers can assert it explicitly. + Call it from every provider plugin's unit tests. """ - required_methods = ( - "start_login", - "complete_login", - "verify_session", - "refresh_session", - "revoke_session", - ) - required_attrs = ("name", "display_name") - - for attr in required_attrs: - val = getattr(cls, attr, "") - if not val: + for attr in ("name", "display_name"): + if not getattr(cls, attr, ""): raise TypeError( f"{cls.__name__} missing or empty attribute: {attr!r}" ) - for method in required_methods: + for method in ( + "start_login", "complete_login", "verify_session", + "refresh_session", "revoke_session", + ): if not callable(getattr(cls, method, None)): raise TypeError(f"{cls.__name__} missing method: {method}") - # Also catch the ABC-not-overridden case. if getattr(cls, "__abstractmethods__", None): raise TypeError( f"{cls.__name__} has unimplemented abstract methods: " diff --git a/hermes_cli/dashboard_auth/cookies.py b/hermes_cli/dashboard_auth/cookies.py index 0c26d3ac0a..439b15347c 100644 --- a/hermes_cli/dashboard_auth/cookies.py +++ b/hermes_cli/dashboard_auth/cookies.py @@ -1,68 +1,28 @@ """Cookie helpers for dashboard auth. -Three cookies in play: - - hermes_session_at: the OAuth access token - (HttpOnly, lifetime = token TTL, ~15 min) - - hermes_session_rt: the OAuth refresh token - (HttpOnly, lifetime = 24h, ROTATING + reuse-detected) - Nous Portal issues a rotating refresh token for the - dashboard auth-code grant (Portal NAS #293 / hermes - #37247). ``set_session_cookies`` writes this cookie - whenever the provider returns a non-empty - ``refresh_token``; the middleware uses it to rotate a - fresh access token transparently on AT expiry. A - provider that omits the refresh token (empty string) - degrades gracefully to access-token-only sessions — - the RT cookie is simply not written. - - hermes_session_pkce: short-lived PKCE state + CSRF nonce + provider - hint (HttpOnly, lifetime = 10 minutes) +Cookies (all HttpOnly, ``SameSite=Lax`` unless noted, Path = proxy prefix or /): + - hermes_session_at access token; Max-Age = token TTL (~15 min) + - hermes_session_rt rotating refresh token; written only when the + provider returned one (AT-only sessions degrade + gracefully), always cleared on logout/expiry + - hermes_session_provider non-secret routing hint so a refresh token is + not handed to the wrong provider when several + auth plugins are enabled + - hermes_session_pkce PKCE state + CSRF nonce + provider hint, 10 min. + ``SameSite=None; Secure`` over HTTPS because it is + set on the /auth/login 302 and must survive the + cross-site redirect chain out to the IDP and back + (Chromium drops Lax cookies set on such a 302, + crbug 40508226); plain HTTP falls back to Lax. + - hermes_sso_attempt one-shot auto-SSO loop-guard marker (60 s) -The two session cookies are ``SameSite=Lax`` and live under the prefix's -Path. The PKCE cookie is the exception: ``SameSite=None`` over HTTPS, -falling back to ``Lax`` on plain HTTP (where ``SameSite=None`` is invalid -without ``Secure``). It is set on the ``/auth/login`` 302 and must survive -the cross-site redirect chain out to the IDP and back to -``/auth/callback``; Chromium intermittently drops ``Lax`` cookies set on a -302 in such a chain (crbug 40508226), which surfaces as "Missing PKCE -state cookie". ``Secure`` is set ONLY when the dashboard was reached over -HTTPS — detected via the request URL scheme, which honours -``X-Forwarded-Proto`` upstream of Fly's TLS terminator when uvicorn is -configured with ``proxy_headers=True``. Loopback dev traffic is always -HTTP so ``Secure`` would lock the cookies out of the browser. - -NOTE: uvicorn only honours ``X-Forwarded-Proto`` from a peer inside its -``forwarded_allow_ips`` (default: ``127.0.0.1``). A TLS terminator that -reaches the dashboard from a non-loopback address — e.g. a reverse proxy -in its own container — is not trusted, so the request still looks like -HTTP here and these cookies are written in their HTTP shape. - -Cookie prefix selection (browser hardening per -https://datatracker.ietf.org/doc/html/draft-west-cookie-prefixes): - - * Loopback HTTP — bare name. ``__Host-`` / ``__Secure-`` require - ``Secure``, which is incompatible with HTTP. - * Gated HTTPS, direct deploy (Path=/) — ``__Host-`` prefix. Binds the - cookie to the exact origin (no Domain attribute) — strongest spec - guarantee. - * Gated HTTPS, behind a reverse-proxy prefix (Path=/hermes) — - ``__Secure-`` prefix. ``__Host-`` is disallowed when Path != "/"; - ``__Secure-`` keeps the Secure-required hardening without the - Path constraint, and the explicit ``Path=/hermes`` covers - same-origin app isolation. - -The setters and readers BOTH consult the active prefix because the -cookie *name* changes — a reader that looked up the bare name when the -setter wrote ``__Secure-hermes_session_at`` would never find the value. - -Refresh-token handling: - ``set_session_cookies`` accepts ``refresh_token=""`` (provider omitted - it) and silently skips writing the RT cookie in that case, so a - refresh-token-less provider degrades to access-token-only sessions. - ``clear_session_cookies`` always emits a Max-Age=0 deletion for the RT - cookie on logout / session expiry so a stale cookie from an earlier - deployment gets cleared. The transparent rotation flow ("expired AT + - live RT → rotate server-side, else 401 → /login") lives in - ``middleware._attempt_refresh``. +``Secure`` is set only when the request arrived over HTTPS +(``request.url.scheme``, which honours ``X-Forwarded-Proto`` only from a peer +in uvicorn's ``forwarded_allow_ips``). Cookie-prefix hardening follows +draft-west-cookie-prefixes: bare name over HTTP; ``__Host-`` on gated HTTPS +with Path=/; ``__Secure-`` behind a proxy prefix (``__Host-`` forbids +Path != /). Setters and readers BOTH resolve the name through +:func:`_resolved_name` — a mismatch silently breaks sessions. """ from __future__ import annotations @@ -76,76 +36,36 @@ from urllib.parse import unquote from fastapi import Request from fastapi.responses import Response -# Bare cookie names — the request-scoped ``_resolved_name`` helper -# decides whether to prepend ``__Host-`` / ``__Secure-`` based on the -# 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 -# redirect on an unauthenticated document load; its mere PRESENCE on the next -# unauthenticated load tells the gate "we already bounced once" so a genuinely -# absent portal session degrades to the /login page instead of ping-ponging. -# Carries no secret — it's a boolean breadcrumb — but is set HttpOnly/Lax/Secure -# like the others for consistency. Short TTL so a user who returns later gets a -# fresh silent attempt rather than a permanently-disabled one. SSO_ATTEMPT_COOKIE = "hermes_sso_attempt" -# Possible name variants we may have to read back. Sorted so most-strict -# wins on iteration when both happen to be present (shouldn't happen in -# practice — a single request emits exactly one variant). +# Name variants a reader may have to try; most strict first. _NAME_VARIANTS = ("__Host-", "__Secure-", "") -# RT cookie Max-Age. Kept at 30 days as a generous upper bound on the cookie's -# browser lifetime; Portal's actual refresh-token TTL (24h, rotating) is the -# real authority — once the RT itself expires/rotates out, a refresh attempt -# returns 400 → RefreshExpiredError → clean re-login, regardless of how long -# the cookie lingers. (Not tightened to 24h here to avoid coupling the cookie -# lifetime to a server-side TTL that can change independently; revisit if the -# stale-cookie refresh churn ever matters.) +# RT cookie lifetime is a generous browser-side upper bound; the provider's own +# RT TTL is the real authority (an expired RT -> RefreshExpiredError -> re-login). _RT_MAX_AGE = 30 * 24 * 60 * 60 _PKCE_MAX_AGE = 10 * 60 -# Auto-SSO loop-guard marker TTL. Just long enough to cover one redirect -# round trip to the portal and back (a few seconds in practice); kept at 60s -# so a slow portal hop or a manual back-button still trips the guard, while a -# user returning minutes later gets a fresh silent attempt rather than being -# stuck on /login forever. The marker is also cleared explicitly on a -# successful callback and whenever the gate falls back to /login. +# Long enough for one portal round trip / back-button; short enough that a +# user returning later gets a fresh silent attempt rather than a stuck /login. _SSO_ATTEMPT_MAX_AGE = 60 def _resolved_name(bare: str, *, use_https: bool, prefix: str) -> str: - """Pick the cookie-prefix variant for the active request shape. - - See module docstring for the prefix selection rules. Mismatch - between setter and reader would silently break sessions, so this - function is the single source of truth for naming. - """ + """Cookie-prefix variant for the request shape (see module docstring).""" if not use_https: return bare if prefix: - # Path != "/" forbids __Host-; fall back to __Secure-. return f"__Secure-{bare}" return f"__Host-{bare}" def _cookie_path(prefix: str) -> str: - """Cookie ``Path`` attribute for the active deploy shape. - - Under ``X-Forwarded-Prefix: /hermes`` we want ``Path=/hermes`` so: - a) the browser sends the cookie back on requests under the prefix - (browsers omit the cookie if request path doesn't start with - Path); - b) the cookie doesn't leak to other apps on the same origin - (``mission-control.tilos.com/billing/...``). - - Direct-deploy (no proxy prefix) gets ``Path=/``. - """ + """``Path=/hermes`` under a proxy prefix (so the browser sends the cookie + back under the prefix and it does not leak to sibling apps), else ``/``.""" return prefix if prefix else "/" @@ -160,6 +80,25 @@ def _common_attrs(*, use_https: bool, prefix: str) -> dict: return attrs +def _pkce_attrs(*, use_https: bool, prefix: str) -> dict: + """Attributes shared by the PKCE set AND clear paths (a shape mismatch + means the browser silently keeps the stale cookie).""" + attrs = _common_attrs(use_https=use_https, prefix=prefix) + if use_https: + attrs["samesite"] = "none" + return attrs + + +def _set(response: Response, bare: str, value: str, *, max_age: int, + use_https: bool, prefix: str, attrs: dict | None = None) -> None: + response.set_cookie( + _resolved_name(bare, use_https=use_https, prefix=prefix), + value, + max_age=max_age, + **(attrs if attrs is not None else _common_attrs(use_https=use_https, prefix=prefix)), + ) + + def set_session_provider_cookie( response: Response, *, @@ -170,12 +109,8 @@ def set_session_provider_cookie( """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), - ) + _set(response, SESSION_PROVIDER_COOKIE, provider, max_age=_RT_MAX_AGE, + use_https=use_https, prefix=prefix) def set_session_cookies( @@ -188,42 +123,20 @@ def set_session_cookies( prefix: str = "", provider: str = "", ) -> None: - """Set the session cookies on the response. + """Set the session cookies. - ``access_token_expires_in`` is in seconds. Use the provider's reported - TTL for the access token. - - ``refresh_token`` is written as the RT cookie when non-empty. Nous Portal - issues a 24h rotating refresh token (hermes #37247); a provider that - omits it returns ``Session.refresh_token == ""`` and we simply don't - persist the RT cookie — the session then behaves as access-token-only - until the AT expires. No other branch changes between the two cases. - - ``prefix`` is the normalised X-Forwarded-Prefix value (e.g. ``/hermes``) - or ``""`` for a direct deploy. It influences both the cookie name - (``__Host-`` vs ``__Secure-`` vs bare) and the ``Path`` attribute. + ``access_token_expires_in`` is seconds (the provider's reported TTL). An + empty ``refresh_token`` means "don't persist the RT cookie" — a literal + empty cookie would be dead state at best, attack surface at worst. + ``prefix`` is the normalised X-Forwarded-Prefix or ``""``. """ - response.set_cookie( - _resolved_name(SESSION_AT_COOKIE, use_https=use_https, prefix=prefix), - access_token, - max_age=access_token_expires_in, - **_common_attrs(use_https=use_https, prefix=prefix), - ) - # Contract v1: empty refresh token means "don't persist RT cookie". - # Keeping a literal empty-value cookie around would be dead state at - # best, attack surface at worst. + _set(response, SESSION_AT_COOKIE, access_token, max_age=access_token_expires_in, + use_https=use_https, prefix=prefix) if refresh_token: - response.set_cookie( - _resolved_name(SESSION_RT_COOKIE, use_https=use_https, prefix=prefix), - refresh_token, - max_age=_RT_MAX_AGE, - **_common_attrs(use_https=use_https, prefix=prefix), - ) + _set(response, SESSION_RT_COOKIE, refresh_token, max_age=_RT_MAX_AGE, + use_https=use_https, prefix=prefix) set_session_provider_cookie( - response, - provider=provider, - use_https=use_https, - prefix=prefix, + response, provider=provider, use_https=use_https, prefix=prefix, ) @@ -237,19 +150,14 @@ def _clear_cookie_variants( ) -> None: """Emit Max-Age=0 deletions for every plausible name variant of a cookie. - Cookie-prefix rules make the deletion shape load-bearing: a Set-Cookie - for a ``__Host-``/``__Secure-`` name is rejected outright by the - browser unless it carries ``Secure`` (and ``__Host-`` additionally - requires ``Path=/``), so those deletions always carry the attributes - their name demands. The bare-name deletion mirrors the shape the - setter uses (``bare_attrs``) — under RFC 6265bis a deletion sent from - a secure origin may omit ``Secure`` and still delete a Secure cookie, - while a ``Secure`` deletion on a plain-HTTP origin can be ignored, so - matching the setter is the shape that works on both origins. + We don't know which variant the setter used (it depends on the setting + request's shape). Prefixed names are rejected by the browser unless they + carry ``Secure`` (``__Host-`` additionally needs ``Path=/``), so those + deletions always carry them; the bare deletion mirrors the setter's shape + (``bare_attrs``), which works on both HTTP and HTTPS origins. """ for variant in _NAME_VARIANTS: if variant == "__Host-": - # __Host- demands Secure AND Path=/ or the header is invalid. response.set_cookie( f"{variant}{bare_name}", "", max_age=0, path="/", httponly=True, samesite=https_samesite, @@ -267,18 +175,13 @@ def _clear_cookie_variants( ) -def clear_session_cookies(response: Response, *, prefix: str = "") -> None: - """Emit Max-Age=0 deletions for both session cookies. +def _lax_bare_attrs(prefix: str) -> dict: + return {"path": _cookie_path(prefix), "httponly": True, "samesite": "lax"} - To delete a cookie reliably the deletion's ``Path`` must match the - set path AND the cookie name must match the variant the setter used. - We don't know which variant was originally set (cookie prefix - depends on the request that set it), so we emit deletions for every - plausible variant under the active path. - """ - bare_attrs = { - "path": _cookie_path(prefix), "httponly": True, "samesite": "lax", - } + +def clear_session_cookies(response: Response, *, prefix: str = "") -> None: + """Delete the AT, RT and provider cookies (every name variant, active path).""" + bare_attrs = _lax_bare_attrs(prefix) for name in (SESSION_AT_COOKIE, SESSION_RT_COOKIE, SESSION_PROVIDER_COOKIE): _clear_cookie_variants( response, name, @@ -286,33 +189,14 @@ def clear_session_cookies(response: Response, *, prefix: str = "") -> None: ) -def _pkce_attrs(*, use_https: bool, prefix: str) -> dict: - """Cookie attributes for the PKCE cookie's set AND clear paths. - - Single source of truth so a deletion always matches the shape the - setter emitted for the same origin — a shape mismatch means the - browser silently keeps the stale cookie. - """ - attrs = _common_attrs(use_https=use_https, prefix=prefix) - if use_https: - attrs["samesite"] = "none" - return attrs - - def encode_pkce_payload(parts: dict[str, str]) -> str: - """Serialise PKCE segments to the wire value: ``base64url(JSON)``. + """Serialise PKCE segments to the wire value ``base64url(JSON)``, no padding. - The urlsafe base64 alphabet (``A-Za-z0-9-_``, padding stripped) is a - strict subset of the RFC 6265 cookie-octet set — no ``;`` (attribute - terminator), no ``"`` and no ``\\`` (the chars that make Python's - http.cookies emit the quoted ``\\073`` form, which strict cookie-aware - proxy hops such as Go's net/http reject outright). The ``=`` padding - is stripped because http.cookies treats ``=`` as outside its legal - unquoted set and would re-wrap the value in the quoted form this - codec exists to avoid; the parser restores the padding. JSON carries - the segments, so no delimiter can ever collide with segment values — - the delimiter/quoting bug class this codec replaces (see - :func:`parse_pkce_payload` for the two legacy formats it superseded). + The urlsafe alphabet is a strict subset of RFC 6265 cookie-octets (no + ``;``, ``"``, ``\\``), so http.cookies never quotes it (strict proxy hops + such as Go net/http reject the quoted form) and no segment value can + collide with a delimiter. Padding ``=`` would trigger quoting; the parser + restores it. """ raw = json.dumps(parts, separators=(",", ":"), sort_keys=True) return ( @@ -329,47 +213,18 @@ def set_pkce_cookie( use_https: bool, prefix: str = "", ) -> None: - # SameSite=None when HTTPS: the PKCE cookie is set on the /auth/login - # 302 response (redirecting to the IDP) and must survive the cross-site - # redirect chain (same-site → IDP → same-site callback). Chromium has a - # long-standing bug (crbug 40508226) where SameSite=Lax cookies set on a - # 302 in a cross-site redirect chain are intermittently dropped, causing - # "Missing PKCE state cookie" on the callback. SameSite=None + Secure - # sidesteps the bug — these cookies are explicitly designed for cross-site - # delivery and Chromium processes them reliably during redirects. - # Loopback HTTP degrades to Lax (SameSite=None requires Secure). - # - # Value encoding: ``payload`` is the segment dict - # (``{"provider": …, "state": …, "verifier": …, "next": …}``) and goes - # on the wire as base64url(JSON) via encode_pkce_payload() — plain - # RFC 6265 cookie-octets end to end, so every cookie-aware hop - # (browsers, Go net/http proxies, Python parsers) passes the value - # through untouched. Readers decode via parse_pkce_payload(), which - # also keeps a compatibility ladder for cookies minted by the two - # earlier wire formats during a rolling upgrade. - response.set_cookie( - _resolved_name(PKCE_COOKIE, use_https=use_https, prefix=prefix), - encode_pkce_payload(payload), - max_age=_PKCE_MAX_AGE, - **_pkce_attrs(use_https=use_https, prefix=prefix), - ) + """Set the PKCE cookie (``payload`` is the segment dict; see module docstring + for the SameSite=None rationale).""" + _set(response, PKCE_COOKIE, encode_pkce_payload(payload), max_age=_PKCE_MAX_AGE, + use_https=use_https, prefix=prefix, + attrs=_pkce_attrs(use_https=use_https, prefix=prefix)) def clear_pkce_cookie( response: Response, *, use_https: bool, prefix: str = "", ) -> None: - """Emit Max-Age=0 deletions for every plausible PKCE cookie variant. - - A deletion is only honoured when its shape is acceptable to the - browser on the current origin: a ``Secure`` deletion can be dropped - on a plain-HTTP origin, while the ``__Host-``/``__Secure-`` name - variants REQUIRE ``Secure`` to be valid at all. So the bare-name - deletion mirrors the setter's shape for the active origin (Lax - without ``Secure`` over HTTP; ``SameSite=None; Secure`` over HTTPS, - matching :func:`set_pkce_cookie`), and the prefixed variants — which - can only ever have been set on an HTTPS origin — always carry - ``Secure; SameSite=None``. - """ + """Delete every PKCE cookie variant; the bare deletion mirrors the setter's + shape for the active origin, the prefixed ones carry ``Secure; SameSite=None``.""" _clear_cookie_variants( response, PKCE_COOKIE, prefix=prefix, https_samesite="none", @@ -380,13 +235,8 @@ def clear_pkce_cookie( def _read_with_fallback( request: Request, bare_name: str, ) -> Optional[str]: - """Read a cookie by checking every prefix variant in order. - - The setter chooses one variant based on the active request shape; - the reader doesn't know which one fired (the request that READS - the cookie may not be the same shape as the request that SET it - in pathological cases). Trying all three guarantees we find it. - """ + """Read a cookie trying every prefix variant (the reading request may not + have the same shape as the one that set it).""" for variant in _NAME_VARIANTS: value = request.cookies.get(f"{variant}{bare_name}") if value is not None: @@ -396,9 +246,10 @@ def _read_with_fallback( def read_session_cookies(request: Request) -> Tuple[Optional[str], Optional[str]]: """Returns (access_token, refresh_token), either may be None.""" - at = _read_with_fallback(request, SESSION_AT_COOKIE) - rt = _read_with_fallback(request, SESSION_RT_COOKIE) - return at, rt + return ( + _read_with_fallback(request, SESSION_AT_COOKIE), + _read_with_fallback(request, SESSION_RT_COOKIE), + ) def read_session_provider(request: Request) -> Optional[str]: @@ -410,50 +261,26 @@ def read_pkce_cookie(request: Request) -> Optional[str]: return _read_with_fallback(request, PKCE_COOKIE) -# base64url wire values are exactly the urlsafe alphabet (padding is -# stripped by the encoder; the decoder restores it). Used as a cheap -# pre-filter before attempting the JSON decode so legacy wire forms -# (which always contain ``%`` or ``;``) never even reach the base64 -# decoder. +# Cheap pre-filter: legacy wire forms always contain ``%`` or ``;``, which are +# outside the base64url alphabet, so they never reach the base64 decoder. _B64URL_RE = re.compile(r"^[A-Za-z0-9_-]+={0,2}$") def parse_pkce_payload(raw: str) -> dict[str, str]: - """Decode + parse a PKCE cookie value into its segment dict. + """Decode a PKCE cookie value into its segment dict (inverse of + :func:`encode_pkce_payload`). EVERY reader must go through this — a reader + that interprets the raw wire value parses zero segments and silently + disables the check it feeds (provider dispatch, CSRF state, broker binding). - Single inverse of :func:`set_pkce_cookie` / - :func:`encode_pkce_payload`. EVERY reader of the PKCE cookie must go - through this helper — a reader that interprets the raw wire value - itself parses zero segments and silently disables whatever check it - was feeding (provider dispatch, CSRF state, native-flow broker - binding). - - Compatibility ladder — the PKCE cookie has a 10-minute TTL and is - opaque + server-set, so during a rolling upgrade a cookie minted by - one server version can arrive at another. Three formats, tried in - order; each rung is unambiguous: - - 1. **base64url(JSON)** (current): the wire value is pure urlsafe - base64 that decodes to a JSON object. Legacy forms can never - match — they always contain ``%`` (URL-encoded, #99176) or a raw - ``;`` (oldest flat form), both outside the base64url alphabet. - 2. **Oldest flat form** (pre-#99176): raw ``;`` between segments - (``provider=…;state=…;verifier=…``). Split as-is WITHOUT - unquoting the payload — the ``next`` segment carries its own - single URL-encoding, and unquoting here would turn a ``%3B`` - inside it into a bogus delimiter and truncate the post-login - target. Neither newer format can contain a raw ``;``. - 3. **URL-encoded flat form** (#99176): the whole flat payload passed - through ``quote(payload, safe="")`` — no raw ``;`` possible - (it is ``%3B``); unquote once, then split. - - Rollout directions: OLD cookie → NEW server is handled here (rungs - 2 and 3 parse both legacy forms correctly). NEW cookie → OLD server - (a rollback, or a mixed fleet routing the callback to a not-yet- - upgraded instance) fails the OAuth state check — the old reader - can't find a ``state`` segment in the base64url blob — and the user - simply retries login against the now-consistent fleet; no data loss, - nothing minted. + Compatibility ladder for cookies minted by an older server during a rolling + upgrade (10-minute TTL), each rung unambiguous: + 1. base64url(JSON) — current. + 2. Oldest flat form with raw ``;`` delimiters — split WITHOUT unquoting + (the ``next`` segment carries its own URL-encoding; unquoting first + would turn a ``%3B`` inside it into a bogus delimiter). + 3. URL-encoded flat form (``quote(payload, safe="")``) — unquote once, split. + A NEW cookie hitting an OLD server fails the state check and the user + simply retries; nothing is minted. """ if _B64URL_RE.match(raw): try: @@ -465,33 +292,16 @@ def parse_pkce_payload(raw: str) -> dict[str, str]: decoded = None if isinstance(decoded, dict): return {str(k): str(v) for k, v in decoded.items()} - if ";" in raw: - # Oldest flat form: already flat, split as-is (no unquote). - return dict( - seg.split("=", 1) for seg in raw.split(";") if "=" in seg - ) - # #99176 URL-encoded flat form: unquote once, then split. - return dict( - seg.split("=", 1) for seg in unquote(raw).split(";") if "=" in seg - ) + flat = raw if ";" in raw else unquote(raw) + return dict(seg.split("=", 1) for seg in flat.split(";") if "=" in seg) def set_sso_attempt_cookie( response: Response, *, use_https: bool, prefix: str = "", ) -> None: - """Set the one-shot auto-SSO loop-guard marker (Phase 1). - - Written by the gate the moment it auto-initiates the portal OAuth - redirect on an unauthenticated document load. The value is a constant - (``"1"``) — only its presence matters. Short Max-Age so a stale marker - can't permanently suppress a future silent attempt. - """ - response.set_cookie( - _resolved_name(SSO_ATTEMPT_COOKIE, use_https=use_https, prefix=prefix), - "1", - max_age=_SSO_ATTEMPT_MAX_AGE, - **_common_attrs(use_https=use_https, prefix=prefix), - ) + """Set the auto-SSO loop-guard marker; only its presence matters.""" + _set(response, SSO_ATTEMPT_COOKIE, "1", max_age=_SSO_ATTEMPT_MAX_AGE, + use_https=use_https, prefix=prefix) def read_sso_attempt_cookie(request: Request) -> Optional[str]: @@ -500,26 +310,15 @@ def read_sso_attempt_cookie(request: Request) -> Optional[str]: def clear_sso_attempt_cookie(response: Response, *, prefix: str = "") -> None: - """Emit Max-Age=0 deletions for the auto-SSO marker, every name variant. - - Called on a successful callback and whenever the gate falls back to - /login, so the marker never lingers to suppress a later silent attempt. - """ + """Delete the auto-SSO marker (every variant) so it never suppresses a + later silent attempt.""" _clear_cookie_variants( response, SSO_ATTEMPT_COOKIE, - prefix=prefix, https_samesite="lax", - bare_attrs={ - "path": _cookie_path(prefix), "httponly": True, "samesite": "lax", - }, + prefix=prefix, https_samesite="lax", bare_attrs=_lax_bare_attrs(prefix), ) def detect_https(request: Request) -> bool: - """Decide whether to set the ``Secure`` cookie flag. - - Reads ``request.url.scheme`` — under uvicorn's ``proxy_headers=True`` - (which start_server enables when the gate is active), this honours - ``X-Forwarded-Proto`` from Fly's TLS terminator. Loopback traffic is - always HTTP so this returns False there. - """ + """``Secure`` flag decision: ``request.url.scheme == "https"`` (honours + ``X-Forwarded-Proto`` under uvicorn ``proxy_headers=True``).""" return request.url.scheme == "https" diff --git a/hermes_cli/dashboard_auth/login_page.py b/hermes_cli/dashboard_auth/login_page.py index d440eee992..201ae3ab05 100644 --- a/hermes_cli/dashboard_auth/login_page.py +++ b/hermes_cli/dashboard_auth/login_page.py @@ -1,37 +1,22 @@ -"""Server-rendered /login page. +"""Server-rendered /login page (no React, no SPA bundle, no injected token). -No React, no JavaScript dependency. Listed providers come from the -registry; clicking a provider sends a GET to -``/auth/login?provider=``. +Providers come from the registry; an OAuth provider renders an anchor to +``/auth/login?provider=``, a ``supports_password`` provider renders a +credential form wired by :data:`_PASSWORD_FORM_SCRIPT`. Styling mirrors the +``@nous-research/ui`` design system; fonts load from the SPA's ``/fonts/`` +mount, which the gate allowlists pre-auth. -Visual styling mirrors the Nous Research design system (the -``@nous-research/ui`` package the React dashboard uses): the same -``Collapse`` / ``Rules Compressed`` typeface, amber-on-dark colour -tokens (``#170d02`` / ``#ffac02`` / ``#fff``), uppercase + wide-tracking -brand chrome, and the inset-bevel button shadow. Fonts are served -out of the SPA's ``/fonts/`` directory which the dashboard-auth gate -already allowlists pre-auth (see ``_GATE_PUBLIC_PREFIXES`` in -``middleware.py``), so the page renders without needing the React -bundle loaded. - -Test-stable class names: the existing test suite extracts the -``class="provider-btn"`` anchor href to walk the OAuth flow. That -class name MUST NOT change without updating -``tests/hermes_cli/test_dashboard_auth_401_reauth.py``. +The ``class="provider-btn"`` anchor is test-stable: the suite extracts its +href to walk the OAuth flow. """ from __future__ import annotations import html +from urllib.parse import quote from hermes_cli.dashboard_auth import list_session_providers -# Inline minimal CSS. The dashboard's full skin lives in the React -# bundle, which we deliberately do NOT load here — the login page must -# not depend on the SPA build being present or on the injected session -# token. -# -# Single curly braces are placeholders for ``str.format``; CSS curlies -# are doubled (``{{`` / ``}}``). +# Single curly braces are ``str.format`` placeholders; CSS curlies are doubled. _LOGIN_HTML_TEMPLATE = """\ @@ -404,14 +389,10 @@ an SSH tunnel or Tailscale.

""" -# Inline script that wires every password provider form to POST JSON to -# ``/auth/password-login`` and navigate on success. Emitted ONLY when at -# least one ``supports_password`` provider is listed (OAuth-only login -# pages stay script-free, preserving the no-JS contract for that case). -# -# Plain string (NOT run through ``str.format``), so braces are literal — -# do not double them. A single delegated submit handler covers all forms; -# the provider name is read from the form's ``data-provider`` attribute. +# Emitted ONLY when a ``supports_password`` provider is listed, so OAuth-only +# login pages stay script-free. Plain string (not ``str.format``): braces are +# literal. One delegated submit handler covers every form; the provider name +# comes from the form's ``data-provider`` attribute. _PASSWORD_FORM_SCRIPT = """\