From e300d81175174cbb2780f42a04d69fd4d8350c91 Mon Sep 17 00:00:00 2001 From: Andrew Peltekci Date: Thu, 6 Aug 2026 02:09:36 -0700 Subject: [PATCH 1/5] feat(auth): mount the device grant under OIDC, and keep its re-auth gate honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The device-authorization grant refused to mount unless auth mode was `accounts`. OIDC deployments were excluded on the reasoning that they delegate login to the IdP via the cli-ticket flow and so never need it. That reasoning covers a CLI logging a human in. It does not cover a third-party application asking to act as a user: the cli-ticket flow hands back the server's own session JWT — full account authority, no scope claim, no grant id, no revocation handle, no `act` provenance. Everything that makes a delegated token safe to give away is exactly what it lacks. An OIDC deployment therefore had no way to authorize an application at all, and the device grant needs nothing from `accounts` that `oidc` does not also provide: both mint the same HS256 session cookie, and `_check_cookie` already picks between the two configs the same way this now does. Header mode stays excluded, and for a real reason rather than symmetry: identity there is asserted by an upstream proxy, so there is no session to delegate FROM and no login to bounce a consenting browser through. ## The part that is not a gate flip Consent requires a login performed AFTER the grant began (session `iat` >= grant `created_at`). A stale session is bounced to the login page with `reauth=1`. That gate is the anti-phishing control: a victim handed a one-click link with the code prefilled must deliberately re-enter their credentials against a screen naming the identity and the client, rather than approving by reflex. `reauth=1` was implemented entirely in the accounts SPA login form, which holds back its auto-redirect and demands a password. `/auth/login` never read the parameter. So mounting the router under OIDC and stopping there would have produced this: bounce to the IdP, IdP recognises its own session, silent redirect back, callback mints a session with a fresh `iat`, gate satisfied. The user proves nothing. Nothing errors, no test fails, and the control is gone while still appearing to be there. `/auth/login` now forwards `reauth=1` to the IdP as `prompt=login` (OIDC Core 3.1.2.1) — the standard way to ask an IdP to re-prompt a user it has already authenticated. Sent only on that path; unconditional re-prompting would cost a password on every ordinary sign-in, which is how a control like this ends up switched off by whoever finds it irritating. Matched strictly against `"1"`. The consent page is the only caller and sends exactly that, so accepting loose truthy spellings would only widen the surface for an unrelated query param to trigger a re-prompt. ## Verified - 220 auth-suite tests pass (device_auth, oidc, callback, invites, open-redirect, accounts, and the new file) - `test_oidc_reauth_prompt.py` pins the parameter both ways: present on `reauth=1`, absent otherwise, and `return_to` still round-trips through the bounce so the pending grant is not abandoned - Removing the `prompt=login` block fails that suite; the tests are not vacuous - ruff, ruff format and mypy clean on every touched file The e2e browser proof (`tests/e2e_ui/auth/test_device_grant_reauth.py`) remains accounts-only — it drives a real password form, which OIDC does not have. The OIDC half is covered at the route level above. Signed-off-by: Andrew Peltekci --- designs/DEVICE_AUTH.md | 30 +++-- omnigent/server/app.py | 15 ++- omnigent/server/routes/auth.py | 22 ++++ omnigent/server/routes/device_auth.py | 57 +++++++--- tests/server/test_device_auth.py | 46 ++++++-- tests/server/test_oidc_reauth_prompt.py | 140 ++++++++++++++++++++++++ 6 files changed, 272 insertions(+), 38 deletions(-) create mode 100644 tests/server/test_oidc_reauth_prompt.py diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index d55ca60403..9fa3a1e718 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -18,8 +18,8 @@ > `set_grant_revocation_check`). Wired in `omnigent/server/app.py`, > **opt-in and default-off** via `OMNIGENT_DEVICE_GRANT_ENABLED` (the > `/oauth/*` routes are unmounted unless it is truthy), and then only in -> **accounts** mode (OIDC delegates login to the IdP via the cli-ticket -> flow and never mounts these routes). +> **accounts** and **oidc** modes — the two that own a server-minted +> session cookie. Header mode has no server-mintable identity. > Slack: `integrations/slack/src/omnigent_slack/oauth.py`, > `tokens.py` (Fernet-encrypted `oauth_tokens`), `auth_manager.py`, plus > the bearer/refresh wiring in `omnigent.py` (`ClientAuth`, @@ -91,9 +91,11 @@ The device grant builds on existing server primitives: - **Bearer validation** — `UnifiedAuthProvider._check_cookie` accepts `Authorization: Bearer ` and validates the same claim shape (`auth.py`). Delegated access tokens validate through this path unchanged. -- **Browser consent under accounts mode** — the `accounts` provider - establishes the browser identity via its session cookie; the consent page - runs behind it. (This is why the grant mounts in accounts mode only — see +- **Browser consent** — both `accounts` and `oidc` establish the browser + identity through the same HS256 session cookie, and the consent page runs + behind it. Under OIDC the bounce hands off to the IdP, so *how* the user + proves themselves — password, Google, SAML — is never this module's + business. (Header mode mints no session, which is why it is excluded — see the mount restriction below.) - **Open-redirect hardening** — `_sanitize_return_to` (`routes/auth.py`) guards the post-login bounce back to the consent page. @@ -184,10 +186,20 @@ approved it. Mounted in `app.py` only when **`OMNIGENT_DEVICE_GRANT_ENABLED` is truthy** (opt-in, **default-off** — the `/oauth/*` routes are absent otherwise), and -then **only in `accounts` mode** (OIDC delegates login to the IdP via the -cli-ticket flow and never mounts these routes; header mode has no -server-mintable identity — see `create_device_auth_router`, which raises if -constructed for any other source). The `device_grants` table is created +then **only in `accounts` and `oidc` modes**. Header mode has no +server-mintable identity — there is nothing to delegate from and no login to +bounce a consenting browser through — see `create_device_auth_router`, which +raises if constructed for any other source. + +**Forced re-authentication across the two modes.** The consent gate (session +`iat` ≥ grant `created_at`) bounces with `reauth=1`, and each login path has +to honour it in its own way. Accounts holds back the SPA's auto-redirect and +demands a password. OIDC has no form to hold back, so `/auth/login` forwards +`reauth=1` to the IdP as `prompt=login` (OIDC Core 3.1.2.1). Dropping that +forwarding does not break the flow — the IdP satisfies the bounce from its own +session and the callback mints a fresh `iat` that clears the gate — it just +silently removes the deliberate-credential-entry property the gate exists for. +Pinned by `tests/server/test_oidc_reauth_prompt.py`. The `device_grants` table is created unconditionally by the migration regardless of the flag; only the router mount is gated. This router **owns** `mint_delegated_token` and `DELEGATED_SCOPE`. diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 722315c611..ebc51f1155 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2479,17 +2479,22 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: ) # Device Authorization Grant (RFC 8628): opt-in, default-off via - # OMNIGENT_DEVICE_GRANT_ENABLED, and accounts-mode only. OIDC delegates - # login to the IdP (cli-ticket flow), so it neither needs nor mounts - # these routes. Wires the revocation lookup into the auth provider so - # revoking a grant immediately rejects its delegated access tokens. + # OMNIGENT_DEVICE_GRANT_ENABLED, for the two modes that own a + # server-minted session cookie: accounts and OIDC. Under OIDC the + # consent page bounces the browser through /auth/login and the IdP + # decides how the user proves themselves, so the grant works without + # this module knowing anything about credentials. Header mode is + # excluded — identity is asserted by an upstream proxy, so there is no + # session to delegate from and no login to bounce through. Wires the + # revocation lookup into the auth provider so revoking a grant + # immediately rejects its delegated access tokens. # See designs/DEVICE_AUTH.md. from omnigent.server.auth import env_var_is_truthy if ( env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) and isinstance(auth_provider, UnifiedAuthProvider) - and auth_provider._source == "accounts" + and auth_provider._source in ("accounts", "oidc") and permission_store is not None ): from omnigent.server.device_grant_store import DeviceGrantStore diff --git a/omnigent/server/routes/auth.py b/omnigent/server/routes/auth.py index 73aeda7b7c..50c1d9d414 100644 --- a/omnigent/server/routes/auth.py +++ b/omnigent/server/routes/auth.py @@ -143,6 +143,11 @@ async def login(request: Request) -> Response: ``state`` parameter. Stores them in a short-lived signed cookie so the callback can verify the response. + ``?reauth=1`` adds ``prompt=login``, forcing the IdP to re-prompt + for credentials even when it already has a session. The + device-grant consent page sets it; see + :mod:`omnigent.server.routes.device_auth`. + :param request: The incoming FastAPI request. :returns: 302 redirect to the IdP with PKCE and state params. @@ -163,6 +168,17 @@ async def login(request: Request) -> Response: # before the callback redeems it. Only meaningful when invites # are enabled; ignored otherwise. invite = request.query_params.get("invite") if _invites_enabled else None + # Forced re-authentication, set by the device-grant consent page when + # the caller's session predates the grant it is being asked to approve. + # + # Accounts mode implements this in the SPA login form (it skips its + # auto-bounce and demands a password). OIDC has no form to hold back — + # the IdP owns the credential — so it has to be asked, and `prompt` is + # the OIDC parameter for asking. Without it the bounce is satisfied by + # the IdP's own session: the user is redirected out and straight back + # with a fresh `iat`, having proven nothing. The consent gate would + # still pass, which is precisely why this cannot be left implicit. + reauth = request.query_params.get("reauth") == "1" # Store state + code_verifier in a short-lived signed cookie. state_payload: dict[str, str | int] = { @@ -187,6 +203,12 @@ async def login(request: Request) -> Response: "code_challenge": code_challenge, "code_challenge_method": "S256", } + if reauth: + # OIDC Core 3.1.2.1: re-prompt for credentials even when the IdP + # already has a session. Sent only on this path — making it the + # default would re-prompt on every ordinary login, which is how a + # security control gets switched off. + params["prompt"] = "login" auth_url = config.authorization_endpoint + "?" + urlencode(params) response = RedirectResponse(url=auth_url, status_code=302) diff --git a/omnigent/server/routes/device_auth.py b/omnigent/server/routes/device_auth.py index 5793d9afc4..141ea1f95e 100644 --- a/omnigent/server/routes/device_auth.py +++ b/omnigent/server/routes/device_auth.py @@ -21,10 +21,17 @@ returns delegated access + refresh tokens. - ``POST /oauth/revoke`` — revoke a grant (backs client logout). -Mounted only in ``accounts`` auth mode (and only when -``OMNIGENT_DEVICE_GRANT_ENABLED`` is set). OIDC deployments delegate login -to the IdP via the cli-ticket flow (``/auth/cli-login``) and never use this -grant; header mode has no server-mintable identity. +Mounted in ``accounts`` and ``oidc`` auth modes (and only when +``OMNIGENT_DEVICE_GRANT_ENABLED`` is set). Header mode has no +server-mintable identity, so there is nothing to delegate from. + +Under OIDC the consent page bounces an unauthenticated (or stale) browser +through ``/auth/login``, which hands off to the IdP — so whether the user +proves themselves with a password, Google, or anything else is the IdP's +business and never this module's. The forced-re-authentication gate below +relies on that bounce carrying ``reauth=1`` through to ``prompt=login``; +without it the IdP would satisfy the bounce from its own session and the +gate would pass without the user having proven anything. See ``designs/DEVICE_AUTH.md`` for the full design + threat model. @@ -263,17 +270,30 @@ def create_device_auth_router( ) -> APIRouter: """Build the ``/oauth/*`` device-grant router. - :param auth_provider: The active provider. Must be ``accounts`` mode; - its cookie config supplies the HMAC signing key and public base URL. + :param auth_provider: The active provider. Must be ``accounts`` or + ``oidc`` mode; its cookie config supplies the HMAC signing key and + public base URL. :param device_grant_store: Persistence for device grants. :returns: APIRouter to mount at the app root. """ - if auth_provider._source != "accounts": + # Header mode stays out: identity there is asserted by an upstream proxy + # and there is no server-mintable session, so there is nothing to delegate + # FROM and no login to bounce a consenting browser through. + if auth_provider._source not in ("accounts", "oidc"): raise RuntimeError( - f"create_device_auth_router requires accounts auth (got {auth_provider._source!r})" + "create_device_auth_router requires accounts or oidc auth " + f"(got {auth_provider._source!r})" ) - cookie_config = auth_provider._accounts_config - assert cookie_config is not None, "accounts mode must have an accounts config" + # Both modes sign the same HS256 session cookie and both configs expose + # `cookie_secret`, `session_cookie_name` and `base_url` — see the + # AccountsConfig docstring and UnifiedAuthProvider._check_cookie, which + # picks between them exactly this way. + cookie_config = ( + auth_provider._oidc_config + if auth_provider._source == "oidc" + else auth_provider._accounts_config + ) + assert cookie_config is not None, f"{auth_provider._source} mode must have a cookie config" cookie_secret = cookie_config.cookie_secret base_url = cookie_config.base_url provider_name = auth_provider._source @@ -392,9 +412,11 @@ async def device_authorize(request: Request) -> Response: def _bounce_to_login(user_code: str, *, reauth: bool) -> RedirectResponse: """302 to the login page, returning to this consent URL afterward. - ``reauth`` adds ``&reauth=1`` so the login page forces a fresh - password submit instead of auto-bouncing an already-signed-in user - (which would loop back here with the same stale session). + ``reauth`` adds ``&reauth=1``, which forces a fresh credential entry + instead of auto-bouncing an already-signed-in user (which would loop + back here with the same stale session). Both login paths honour it: + the accounts SPA holds back its auto-redirect and shows the password + form, and ``/auth/login`` forwards it to the IdP as ``prompt=login``. """ login_url = auth_provider.login_url or "/login" return_to = f"/oauth/device?user_code={user_code}" if user_code else "/oauth/device" @@ -406,10 +428,11 @@ def _bounce_to_login(user_code: str, *, reauth: bool) -> RedirectResponse: def _session_iat(request: Request) -> int | None: """Return the ``iat`` (issue time) of the caller's session JWT. - Read from the session cookie (accounts mode mints a fresh ``iat`` - on every ``/auth/login``, so this is effectively the last-login - time). ``None`` when absent/invalid. Used to enforce that consent - follows a login started FOR this device flow. + Read from the session cookie. Both modes mint a fresh ``iat`` on + every completed login — accounts on the ``/auth/login`` POST, OIDC + in the ``/auth/callback`` handler — so this is effectively the + last-login time. ``None`` when absent/invalid. Used to enforce that + consent follows a login started FOR this device flow. """ token = request.cookies.get(cookie_config.session_cookie_name) if not token: diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 21e6aac239..07082a279c 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -31,21 +31,53 @@ # ── Router mount guard (unit) ───────────────────────────────────── -@pytest.mark.parametrize("source", ["oidc", "header"]) -def test_router_factory_rejects_non_accounts_mode(source: str, tmp_path: Path) -> None: - """The device grant is accounts-mode only. OIDC delegates login to the IdP - (cli-ticket flow) and never uses these routes; header can't mint identity. - ``create_device_auth_router`` must refuse to build for either.""" +def test_router_factory_rejects_header_mode(tmp_path: Path) -> None: + """Header mode has no server-mintable session: identity is asserted by an + upstream proxy, so there is nothing to delegate FROM and no login to bounce + a consenting browser through. The factory must refuse to build.""" from types import SimpleNamespace from omnigent.server.routes.device_auth import create_device_auth_router - provider = SimpleNamespace(_source=source) + provider = SimpleNamespace(_source="header") store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") - with pytest.raises(RuntimeError, match="accounts"): + with pytest.raises(RuntimeError, match="accounts or oidc"): create_device_auth_router(provider, store) # type: ignore[arg-type] +def test_router_factory_builds_in_oidc_mode_from_the_oidc_config(tmp_path: Path) -> None: + """OIDC owns the same HS256 session cookie accounts does, so the grant + works there — the IdP simply decides how the user proves themselves. + + The config must come from ``_oidc_config``: reading ``_accounts_config`` + (None under OIDC) would trip the assert, and reading the wrong secret would + sign device codes and refresh tokens with a key nothing else validates. + """ + from types import SimpleNamespace + + from omnigent.server.routes.device_auth import create_device_auth_router + + oidc_secret = b"o" * 32 + provider = SimpleNamespace( + _source="oidc", + _oidc_config=SimpleNamespace( + cookie_secret=oidc_secret, + base_url="https://omnigent.example.com", + session_cookie_name="__Host-ap_session", + ), + _accounts_config=None, + login_url="/auth/login", + ) + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + + router = create_device_auth_router(provider, store) # type: ignore[arg-type] + + paths = {route.path for route in router.routes} # type: ignore[attr-defined] + assert "/oauth/device/authorize" in paths + assert "/oauth/token" in paths + assert "/oauth/revoke" in paths + + # ── Store invariants (unit) ─────────────────────────────────────── diff --git a/tests/server/test_oidc_reauth_prompt.py b/tests/server/test_oidc_reauth_prompt.py new file mode 100644 index 0000000000..a3a9655396 --- /dev/null +++ b/tests/server/test_oidc_reauth_prompt.py @@ -0,0 +1,140 @@ +"""Forced re-authentication survives the handoff to an OIDC IdP. + +The device-grant consent page refuses to render for a session that predates +the grant being approved, and bounces through the login page with +``?reauth=1``. That is the anti-phishing gate: approving a device grant must +cost a deliberate credential entry, so a victim handed a one-click link cannot +bind an attacker's grant by reflex. + +In accounts mode the SPA login form enforces it — it holds back its +auto-redirect and demands a password. OIDC has no form to hold back; the IdP +owns the credential. The only way to ask is the OIDC ``prompt`` parameter, so +``/auth/login`` must forward ``reauth=1`` as ``prompt=login``. + +Without that forwarding the flow still *works* — the IdP satisfies the bounce +from its own session, returns a token, and the callback mints a session whose +fresh ``iat`` clears the consent gate. Nothing errors, no test fails, and the +gate silently approves a user who proved nothing. These tests pin the +parameter so that regression cannot pass quietly. +""" + +from __future__ import annotations + +from collections.abc import Iterator +from pathlib import Path +from urllib.parse import parse_qs, urlparse + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from omnigent.server.admin_list import AdminList +from omnigent.server.auth import UnifiedAuthProvider +from omnigent.server.oidc import OIDCConfig +from omnigent.server.routes.auth import create_auth_router +from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore + +_TEST_SECRET = bytes.fromhex("aa" * 32) + + +def _oidc_config() -> OIDCConfig: + """An OIDC config over plain HTTP so TestClient handles cookies.""" + return OIDCConfig( + issuer="https://accounts.google.com", + client_id="cid", + client_secret="secret", + redirect_uri="http://localhost:8000/auth/callback", + cookie_secret=_TEST_SECRET, + scopes="openid email profile", + session_ttl_hours=8, + logout_redirect_uri=None, + allowed_domains=None, + provider_type="oidc", + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + jwks_uri="https://www.googleapis.com/oauth2/v3/certs", + userinfo_endpoint=None, + allow_invites=False, + ) + + +@pytest.fixture +def oidc_client(tmp_path: Path, db_uri: str) -> Iterator[TestClient]: + """An OIDC auth router mounted on a TestClient, redirects not followed.""" + perm_store = SqlAlchemyPermissionStore(db_uri) + admins = tmp_path / "admins" + admins.write_text("") + provider = UnifiedAuthProvider(source="oidc", oidc_config=_oidc_config()) + + app = FastAPI() + app.include_router( + create_auth_router(provider, perm_store, AdminList(admins)), + prefix="/auth", + ) + with TestClient(app) as client: + yield client + + +def _authorize_params(client: TestClient, query: str) -> dict[str, list[str]]: + """Follow ``/auth/login`` one hop and return the IdP authorize params.""" + res = client.get(f"/auth/login{query}", follow_redirects=False) + assert res.status_code == 302, res.status_code + return parse_qs(urlparse(res.headers["location"]).query) + + +def test_reauth_forwards_prompt_login_to_the_idp(oidc_client: TestClient) -> None: + """``?reauth=1`` must reach the IdP as ``prompt=login``. + + This is the whole gate under OIDC. Drop the parameter and the IdP + re-authenticates the user silently from its own session. + """ + params = _authorize_params(oidc_client, "?reauth=1&return_to=/oauth/device") + + assert params.get("prompt") == ["login"] + # The rest of the request must be unchanged — PKCE and state still apply. + assert params["code_challenge_method"] == ["S256"] + assert params["response_type"] == ["code"] + assert params["code_challenge"] and params["state"] + + +def test_an_ordinary_login_does_not_re_prompt(oidc_client: TestClient) -> None: + """No ``reauth`` ⇒ no ``prompt``. + + Sending ``prompt=login`` unconditionally would force a password entry on + every single sign-in, which is how a security control ends up switched + off by whoever finds it annoying. + """ + assert "prompt" not in _authorize_params(oidc_client, "") + assert "prompt" not in _authorize_params(oidc_client, "?return_to=/sessions") + + +@pytest.mark.parametrize("raw", ["0", "true", "yes", "", "1 ", "TRUE"]) +def test_only_an_exact_1_forces_re_authentication(oidc_client: TestClient, raw: str) -> None: + """Anything other than exactly ``1`` is not a re-auth request. + + Matched strictly because the consent page is the only caller and it sends + exactly ``1``; accepting loose truthy spellings would let an unrelated + query param turn on a re-prompt nobody asked for. + """ + assert "prompt" not in _authorize_params(oidc_client, f"?reauth={raw}") + + +def test_re_authentication_still_round_trips_the_return_to(oidc_client: TestClient) -> None: + """The consent URL must survive the re-auth bounce. + + Losing it would land the user on the dashboard after re-entering their + password, with the pending grant abandoned and no way back to it. + """ + from urllib.parse import unquote + + import jwt + + oidc_client.get( + "/auth/login?reauth=1&return_to=/oauth/device%3Fuser_code%3DK7M2-QP9X", + follow_redirects=False, + ) + cookie = oidc_client.cookies.get("ap_auth_state") + assert cookie is not None + claims = jwt.decode(cookie, _TEST_SECRET, algorithms=["HS256"]) + + assert unquote(claims["return_to"]) == "/oauth/device?user_code=K7M2-QP9X" From f3bb95f756a7badaebb94542f4cbe9d7789807ef Mon Sep 17 00:00:00 2001 From: Andrew Peltekci Date: Thu, 6 Aug 2026 02:33:20 -0700 Subject: [PATCH 2/5] fix(auth): force re-auth on every consent bounce, and refuse GitHub OAuth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses three review findings on the device-grant-under-OIDC change. Both P1s are the same mistake as the one that change was written to fix: a control that still looks present after it has stopped working. ## The unforced first bounce `device_consent_page` bounced an unauthenticated caller with `reauth=False`, forcing re-authentication only when an existing Omnigent cookie was stale. Under accounts that distinction is sound: no session means no credential, so the SPA shows the password form either way. Under OIDC it is not. "No Omnigent session" says nothing about the IdP's session, which is separate and may well be live — so the unforced bounce is satisfied silently, the callback mints a cookie with an `iat` newer than the grant, and the consent gate passes without the user having proven anything. The same silent-pass the `prompt=login` work closed, reached by the sibling path. The consent page cannot tell the two cases apart, so every bounce now forces it. The parameter is gone rather than defaulted, because there is no caller that wants an unforced bounce. ## GitHub OAuth cannot honour the gate `OIDCConfig.from_env` accepts GitHub as an `oidc` source but points it at `https://github.com/login/oauth/authorize` — plain OAuth 2.0, with no `prompt` parameter in the specification. `prompt=login` is ignored, GitHub reuses its session, and the fresh `iat` clears the gate. The security property the whole change rests on is absent for that provider, while every test and every log line reads as though it holds. `unsupported_reason` now owns the rule and refuses the grant there. A grant issued behind a gate that cannot hold is worse than no grant, because it looks protected. `app.py` consults the same predicate and logs the refusal, so an operator who set OMNIGENT_DEVICE_GRANT_ENABLED learns why `/oauth/*` is missing rather than concluding the flag did not take — and the server still boots. Folding the header-mode check into the same predicate keeps one answer to "can this provider carry a grant", rather than two that can drift. ## Comment length The forced-reauth rationale in routes/auth.py ran ten inline lines against AGENTS.md's three-line guidance. Condensed to three; the full reasoning already lives in the route docstring and DEVICE_AUTH.md. ## Verified - 231 tests pass across the auth suite and app integration - Both P1 fixes mutation-checked: restoring the unforced bounce fails 1 test, allowing GitHub fails 2 - New coverage: forced bounce with no session at all, GitHub refused, and `unsupported_reason` admitting standard OIDC and accounts so the predicate cannot over-refuse - ruff, ruff format, mypy clean Signed-off-by: Andrew Peltekci --- designs/DEVICE_AUTH.md | 17 +++++- omnigent/server/app.py | 34 ++++++++---- omnigent/server/routes/auth.py | 20 +++---- omnigent/server/routes/device_auth.py | 74 ++++++++++++++++++-------- tests/server/test_device_auth.py | 75 ++++++++++++++++++++++----- 5 files changed, 158 insertions(+), 62 deletions(-) diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index 9fa3a1e718..197381552e 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -199,7 +199,22 @@ demands a password. OIDC has no form to hold back, so `/auth/login` forwards forwarding does not break the flow — the IdP satisfies the bounce from its own session and the callback mints a fresh `iat` that clears the gate — it just silently removes the deliberate-credential-entry property the gate exists for. -Pinned by `tests/server/test_oidc_reauth_prompt.py`. The `device_grants` table is created +Pinned by `tests/server/test_oidc_reauth_prompt.py`. + +*Every* bounce is forced, including the one for a caller with no Omnigent +session. Under accounts that changes nothing — no session means no credential, +so the form is shown regardless — but under OIDC the caller may still hold a +live IdP session, and the consent page cannot tell the two cases apart. + +**Why GitHub is excluded.** `OIDCConfig.from_env` accepts GitHub as an `oidc` +source, but points it at `https://github.com/login/oauth/authorize` — plain +OAuth 2.0, which has no `prompt` parameter. `prompt=login` would be ignored, +GitHub would reuse its session, and the callback's fresh `iat` would clear the +gate. `unsupported_reason` therefore refuses the grant for that provider +outright: a grant issued behind a gate that cannot hold is worse than no grant, +because it looks protected. `app.py` logs the refusal so an operator who set +`OMNIGENT_DEVICE_GRANT_ENABLED` is told why `/oauth/*` is absent instead of +assuming the flag did not take. The `device_grants` table is created unconditionally by the migration regardless of the flag; only the router mount is gated. This router **owns** `mint_delegated_token` and `DELEGATED_SCOPE`. diff --git a/omnigent/server/app.py b/omnigent/server/app.py index ebc51f1155..9892f611fe 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2479,22 +2479,34 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: ) # Device Authorization Grant (RFC 8628): opt-in, default-off via - # OMNIGENT_DEVICE_GRANT_ENABLED, for the two modes that own a - # server-minted session cookie: accounts and OIDC. Under OIDC the - # consent page bounces the browser through /auth/login and the IdP - # decides how the user proves themselves, so the grant works without - # this module knowing anything about credentials. Header mode is - # excluded — identity is asserted by an upstream proxy, so there is no - # session to delegate from and no login to bounce through. Wires the - # revocation lookup into the auth provider so revoking a grant - # immediately rejects its delegated access tokens. + # OMNIGENT_DEVICE_GRANT_ENABLED, and only for providers that can be + # made to re-prompt an already signed-in user — see + # `unsupported_reason`, which owns that rule. Wires the revocation + # lookup into the auth provider so revoking a grant immediately + # rejects its delegated access tokens. # See designs/DEVICE_AUTH.md. from omnigent.server.auth import env_var_is_truthy + from omnigent.server.routes.device_auth import unsupported_reason + + _device_grant_wanted = env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) + _device_grant_blocked = ( + unsupported_reason(auth_provider) + if isinstance(auth_provider, UnifiedAuthProvider) + else "a custom auth provider cannot mint the session the grant delegates from" + ) + if _device_grant_wanted and _device_grant_blocked is not None: + # Asked for and refused: say so, or the operator sees only the + # absence of /oauth/* and assumes the flag did not take. + _logger.warning( + "device-grant: OMNIGENT_DEVICE_GRANT_ENABLED is set but the " + "/oauth/* routes are NOT mounted — %s. See designs/DEVICE_AUTH.md.", + _device_grant_blocked, + ) if ( - env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) + _device_grant_wanted and isinstance(auth_provider, UnifiedAuthProvider) - and auth_provider._source in ("accounts", "oidc") + and _device_grant_blocked is None and permission_store is not None ): from omnigent.server.device_grant_store import DeviceGrantStore diff --git a/omnigent/server/routes/auth.py b/omnigent/server/routes/auth.py index 50c1d9d414..821b3e1cf4 100644 --- a/omnigent/server/routes/auth.py +++ b/omnigent/server/routes/auth.py @@ -168,16 +168,9 @@ async def login(request: Request) -> Response: # before the callback redeems it. Only meaningful when invites # are enabled; ignored otherwise. invite = request.query_params.get("invite") if _invites_enabled else None - # Forced re-authentication, set by the device-grant consent page when - # the caller's session predates the grant it is being asked to approve. - # - # Accounts mode implements this in the SPA login form (it skips its - # auto-bounce and demands a password). OIDC has no form to hold back — - # the IdP owns the credential — so it has to be asked, and `prompt` is - # the OIDC parameter for asking. Without it the bounce is satisfied by - # the IdP's own session: the user is redirected out and straight back - # with a fresh `iat`, having proven nothing. The consent gate would - # still pass, which is precisely why this cannot be left implicit. + # Forced re-authentication, requested by the device-grant consent page. + # Without it the IdP satisfies the bounce from its own session and the + # consent gate passes on a user who proved nothing. reauth = request.query_params.get("reauth") == "1" # Store state + code_verifier in a short-lived signed cookie. @@ -204,10 +197,9 @@ async def login(request: Request) -> Response: "code_challenge_method": "S256", } if reauth: - # OIDC Core 3.1.2.1: re-prompt for credentials even when the IdP - # already has a session. Sent only on this path — making it the - # default would re-prompt on every ordinary login, which is how a - # security control gets switched off. + # OIDC Core 3.1.2.1: re-prompt even when the IdP has a session. + # Only on this path — as a default it would cost a password on + # every sign-in, and get switched off. params["prompt"] = "login" auth_url = config.authorization_endpoint + "?" + urlencode(params) diff --git a/omnigent/server/routes/device_auth.py b/omnigent/server/routes/device_auth.py index 141ea1f95e..08b0b820dd 100644 --- a/omnigent/server/routes/device_auth.py +++ b/omnigent/server/routes/device_auth.py @@ -264,6 +264,38 @@ def allow(self, key: str, now: float) -> bool: return True +def unsupported_reason(auth_provider: UnifiedAuthProvider) -> str | None: + """Why this provider cannot carry a device grant, or ``None`` if it can. + + The consent gate is only as strong as the login path's ability to + re-prompt a user the provider has already authenticated. Where that + cannot be demanded the grant is refused outright rather than issued + behind a gate that silently passes. + + - ``header`` — identity comes from an upstream proxy: no session to + delegate from, and no login to bounce a consenting browser through. + - ``github`` — configured as an OIDC source, but GitHub's OAuth + authorization endpoint is not OIDC and has no ``prompt`` parameter. + It would ignore ``prompt=login``, reuse its session, and return a + callback whose fresh ``iat`` clears the gate. + + :param auth_provider: The active provider. + :returns: A human-readable reason, or ``None`` when supported. + """ + source = auth_provider._source + if source not in ("accounts", "oidc"): + return f"{source!r} auth has no server-minted session to delegate from" + if source == "oidc": + config = auth_provider._oidc_config + if config is not None and config.provider_type == "github": + return ( + "GitHub OAuth cannot be asked to re-authenticate an already " + "signed-in user (no OIDC 'prompt' parameter), so the consent " + "page's forced-re-authentication gate would not hold" + ) + return None + + def create_device_auth_router( auth_provider: UnifiedAuthProvider, device_grant_store: DeviceGrantStore, @@ -276,14 +308,9 @@ def create_device_auth_router( :param device_grant_store: Persistence for device grants. :returns: APIRouter to mount at the app root. """ - # Header mode stays out: identity there is asserted by an upstream proxy - # and there is no server-mintable session, so there is nothing to delegate - # FROM and no login to bounce a consenting browser through. - if auth_provider._source not in ("accounts", "oidc"): - raise RuntimeError( - "create_device_auth_router requires accounts or oidc auth " - f"(got {auth_provider._source!r})" - ) + reason = unsupported_reason(auth_provider) + if reason is not None: + raise RuntimeError(f"create_device_auth_router cannot build: {reason}") # Both modes sign the same HS256 session cookie and both configs expose # `cookie_secret`, `session_cookie_name` and `base_url` — see the # AccountsConfig docstring and UnifiedAuthProvider._check_cookie, which @@ -409,20 +436,18 @@ async def device_authorize(request: Request) -> Response: # ── Browser consent page ────────────────────────────────────── - def _bounce_to_login(user_code: str, *, reauth: bool) -> RedirectResponse: - """302 to the login page, returning to this consent URL afterward. + def _bounce_to_login(user_code: str) -> RedirectResponse: + """302 to the login page with ``reauth=1``, returning here afterward. - ``reauth`` adds ``&reauth=1``, which forces a fresh credential entry - instead of auto-bouncing an already-signed-in user (which would loop - back here with the same stale session). Both login paths honour it: - the accounts SPA holds back its auto-redirect and shows the password - form, and ``/auth/login`` forwards it to the IdP as ``prompt=login``. + Always forced, on every bounce. "No Omnigent session" does not mean + "no credential" under OIDC — the IdP holds its own, and would satisfy + an unforced bounce silently. The accounts SPA holds back its + auto-redirect and shows the password form; ``/auth/login`` forwards + this to the IdP as ``prompt=login``. """ login_url = auth_provider.login_url or "/login" return_to = f"/oauth/device?user_code={user_code}" if user_code else "/oauth/device" - query = f"return_to={html.escape(return_to, quote=True)}" - if reauth: - query += "&reauth=1" + query = f"return_to={html.escape(return_to, quote=True)}&reauth=1" return RedirectResponse(url=f"{login_url}?{query}", status_code=302) def _session_iat(request: Request) -> int | None: @@ -458,15 +483,20 @@ async def device_consent_page(request: Request) -> Response: device flow began (session ``iat`` ≥ the grant's ``created_at``). A pre-existing session — however recent — is bounced back through the login page with ``reauth=1``, so approving a device grant always - costs a deliberate, fresh password entry. This closes the + costs a deliberate, fresh credential entry. This closes the reflex-approve phishing case: a victim already signed in can't bind - an attacker's grant with one click; they must re-enter their password + an attacker's grant with one click; they must re-authenticate against a screen naming the exact identity and client. + + Every bounce forces it, including the one for a caller with no + Omnigent session at all. Under OIDC that caller may still hold a + live IdP session, which would satisfy an unforced bounce without + their involvement. """ user_id = auth_provider.get_user_id(request) user_code = (request.query_params.get("user_code") or "").strip() if user_id is None: - return _bounce_to_login(user_code, reauth=False) + return _bounce_to_login(user_code) if not user_code: return HTMLResponse(_consent_html(prompt_for_code=True), status_code=200) @@ -485,7 +515,7 @@ async def device_consent_page(request: Request) -> Response: # rather than auto-returning the stale session (which would loop). session_iat = _session_iat(request) if session_iat is None or session_iat < grant.created_at: - return _bounce_to_login(user_code, reauth=True) + return _bounce_to_login(user_code) return HTMLResponse( _consent_html( diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 07082a279c..5e5d4a6bea 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -41,33 +41,67 @@ def test_router_factory_rejects_header_mode(tmp_path: Path) -> None: provider = SimpleNamespace(_source="header") store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") - with pytest.raises(RuntimeError, match="accounts or oidc"): + with pytest.raises(RuntimeError, match="no server-minted session"): create_device_auth_router(provider, store) # type: ignore[arg-type] -def test_router_factory_builds_in_oidc_mode_from_the_oidc_config(tmp_path: Path) -> None: - """OIDC owns the same HS256 session cookie accounts does, so the grant - works there — the IdP simply decides how the user proves themselves. - - The config must come from ``_oidc_config``: reading ``_accounts_config`` - (None under OIDC) would trip the assert, and reading the wrong secret would - sign device codes and refresh tokens with a key nothing else validates. - """ +def _oidc_provider(provider_type: str) -> object: + """A minimal OIDC-shaped provider stub for the mount-guard tests.""" from types import SimpleNamespace - from omnigent.server.routes.device_auth import create_device_auth_router - - oidc_secret = b"o" * 32 - provider = SimpleNamespace( + return SimpleNamespace( _source="oidc", _oidc_config=SimpleNamespace( - cookie_secret=oidc_secret, + cookie_secret=b"o" * 32, base_url="https://omnigent.example.com", session_cookie_name="__Host-ap_session", + provider_type=provider_type, ), _accounts_config=None, login_url="/auth/login", ) + + +def test_router_factory_rejects_github_oauth(tmp_path: Path) -> None: + """GitHub is an ``oidc`` source pointed at a NON-OIDC endpoint. + + ``OIDCConfig.from_env`` sets ``provider_type="github"`` and + ``authorization_endpoint=https://github.com/login/oauth/authorize`` — + plain OAuth 2.0, which has no ``prompt`` parameter. It would ignore + ``prompt=login``, reuse its session, and hand back a callback whose fresh + ``iat`` clears the consent gate. Refused outright: a grant issued behind a + gate that cannot hold is worse than no grant, because it looks protected. + """ + from omnigent.server.routes.device_auth import create_device_auth_router + + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + with pytest.raises(RuntimeError, match="GitHub OAuth"): + create_device_auth_router(_oidc_provider("github"), store) # type: ignore[arg-type] + + +def test_unsupported_reason_admits_standard_oidc_and_accounts() -> None: + """The predicate must not over-refuse: real OIDC and accounts both pass.""" + from types import SimpleNamespace + + from omnigent.server.routes.device_auth import unsupported_reason + + assert unsupported_reason(_oidc_provider("oidc")) is None # type: ignore[arg-type] + assert unsupported_reason(SimpleNamespace(_source="accounts")) is None # type: ignore[arg-type] + assert unsupported_reason(_oidc_provider("github")) is not None # type: ignore[arg-type] + assert unsupported_reason(SimpleNamespace(_source="header")) is not None # type: ignore[arg-type] + + +def test_router_factory_builds_in_oidc_mode_from_the_oidc_config(tmp_path: Path) -> None: + """OIDC owns the same HS256 session cookie accounts does, so the grant + works there — the IdP simply decides how the user proves themselves. + + The config must come from ``_oidc_config``: reading ``_accounts_config`` + (None under OIDC) would trip the assert, and reading the wrong secret would + sign device codes and refresh tokens with a key nothing else validates. + """ + from omnigent.server.routes.device_auth import create_device_auth_router + + provider = _oidc_provider("oidc") store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") router = create_device_auth_router(provider, store) # type: ignore[arg-type] @@ -411,6 +445,19 @@ def test_consent_page_requires_login(app: TestClient) -> None: assert "/login" in r.headers["location"] +def test_consent_forces_reauth_even_with_no_session_at_all(app: TestClient) -> None: + """The bounce for a caller with NO session must force re-authentication too. + + Under accounts, "no session" means no credential and the SPA shows the + password form either way. Under OIDC it does not: the caller may still + hold a live IdP session, which would satisfy an unforced bounce silently + and return a fresh ``iat`` that clears the consent gate. The consent page + cannot tell the two apart, so every bounce is forced. + """ + r = app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) + assert "reauth=1" in r.headers["location"] + + def test_consent_forces_reauth_for_stale_session(app: TestClient) -> None: """A session that predates the grant is bounced to a FORCED re-login. From 8faa7f97bae77ca92b9e8503740352bfba893489 Mon Sep 17 00:00:00 2001 From: Andrew Peltekci Date: Thu, 6 Aug 2026 02:41:21 -0700 Subject: [PATCH 3/5] fix(auth): verify the forced re-authentication instead of only asking for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the remaining review findings on this branch. All three come back to the same gap: the gate was requested but never checked. ## prompt=login was a request with no verification `prompt=login` asks an IdP to re-authenticate. Nothing confirmed it obeyed, and `auth_time` appeared nowhere in the codebase. A non-conforming or misconfigured IdP satisfies the bounce from its own session, the callback mints a cookie with a fresh `iat`, and the consent gate passes — the same silent failure this branch exists to prevent, one level further out. The bounce now sends `max_age=0` alongside `prompt=login`, which obliges a conforming IdP to return `auth_time`. `/auth/login` signs the bounce time into the state cookie as `reauth_at`, so the requirement cannot be stripped by editing the URL, and `/auth/callback` refuses with 403 — minting no session — unless the returned `auth_time` postdates it. A missing `auth_time` is refused as well. Silence is indistinguishable from a reused session, and this is the only gate between a phished consent link and a delegated grant. Ordinary logins carry no `reauth_at` and are untouched; most IdPs omit the claim, and requiring it everywhere would break every sign-in. Verifying it meant reading the id_token twice, so the validated decode is now `_verified_id_token_claims`, shared by the email resolver and the freshness check. One validated path, so no caller can read a claim out of an unverified token. GitHub short-circuits `reauth` at the login route as well as being refused the grant: it can neither be asked to re-authenticate nor report that it did, and setting `reauth_at` for it would fail every such login at the callback instead. ## The mount decision was never exercised through create_app The factory tests prove the router builds; they never proved the app calls it. A typo in the mount condition would leave `/oauth/*` absent under OIDC with the whole suite green — which is exactly the failure the condition was widened to avoid. `create_app` is now driven directly with a real OIDC provider (constructed in-process, so no IdP discovery request), asserting the grant is reachable, and that it is absent for GitHub. The GitHub case asserts against the route table rather than a status code: the SPA catch-all answers unmounted paths, so "not 200" would also pass with the routes mounted and merely erroring. ## A SimpleNamespace config proved nothing about the real cookie `_session_iat` reads `cookie_config.session_cookie_name` and verifies with `cookie_config.cookie_secret`. If either diverged from what `/auth/callback` sets, it would return None on every request and the consent page would bounce forever — a login loop with no error and no failing test. A hand-built stub cannot catch that. Added a test that mints a session through the real `mint_session_cookie` from a real `OIDCConfig` and asserts the consent page renders, naming the identity and the client. ## Verified - 280 tests pass across the auth suite, integration and e2e - Mutation-checked: dropping the callback verification fails 2 tests, treating a missing auth_time as a pass fails 1 - ruff, ruff format, mypy clean Signed-off-by: Andrew Peltekci --- designs/DEVICE_AUTH.md | 10 ++ omnigent/server/routes/auth.py | 127 +++++++++++++++++++----- tests/server/test_device_auth.py | 120 ++++++++++++++++++++++ tests/server/test_oidc_callback.py | 84 +++++++++++++++- tests/server/test_oidc_reauth_prompt.py | 46 +++++++++ 5 files changed, 363 insertions(+), 24 deletions(-) diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index 197381552e..31b5959cfe 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -206,6 +206,16 @@ session. Under accounts that changes nothing — no session means no credential, so the form is shown regardless — but under OIDC the caller may still hold a live IdP session, and the consent page cannot tell the two cases apart. +**Requested, then verified.** `prompt=login` is only a request, so the bounce +also sends `max_age=0`, which obliges a conforming IdP to report the moment it +authenticated the user in the `auth_time` claim. `/auth/login` signs the bounce +time into the state cookie as `reauth_at`, and `/auth/callback` refuses (403, +no session minted) unless the returned `auth_time` postdates it. A missing +`auth_time` is refused too: silence is indistinguishable from a reused session, +and this gate is the only thing between a phished consent link and a delegated +grant. Ordinary logins carry no `reauth_at` and are unaffected — most IdPs omit +the claim, and requiring it everywhere would break every sign-in. + **Why GitHub is excluded.** `OIDCConfig.from_env` accepts GitHub as an `oidc` source, but points it at `https://github.com/login/oauth/authorize` — plain OAuth 2.0, which has no `prompt` parameter. `prompt=login` would be ignored, diff --git a/omnigent/server/routes/auth.py b/omnigent/server/routes/auth.py index 821b3e1cf4..9a81305d60 100644 --- a/omnigent/server/routes/auth.py +++ b/omnigent/server/routes/auth.py @@ -46,6 +46,8 @@ _AUTH_STATE_COOKIE_PLAIN = "ap_auth_state" _AUTH_STATE_TTL_SECONDS = 300 # 5 minutes _CLI_TICKET_TTL_SECONDS = 300 # 5 minutes +# Tolerance when comparing the IdP's `auth_time` against our own clock. +_REAUTH_CLOCK_SKEW_SECONDS = 60 # How long an OIDC invite URL stays redeemable. Matches the accounts # provider's default invite window (72h) — long enough to share # out-of-band, short enough to bound exposure of an unused link. @@ -170,8 +172,10 @@ async def login(request: Request) -> Response: invite = request.query_params.get("invite") if _invites_enabled else None # Forced re-authentication, requested by the device-grant consent page. # Without it the IdP satisfies the bounce from its own session and the - # consent gate passes on a user who proved nothing. - reauth = request.query_params.get("reauth") == "1" + # consent gate passes on a user who proved nothing. GitHub OAuth has + # no way to demand or report it, so it never gets here — see + # `device_auth.unsupported_reason`. + reauth = request.query_params.get("reauth") == "1" and config.provider_type != "github" # Store state + code_verifier in a short-lived signed cookie. state_payload: dict[str, str | int] = { @@ -184,6 +188,10 @@ async def login(request: Request) -> Response: state_payload["ticket"] = ticket if invite: state_payload["invite"] = invite + if reauth: + # Signed, so the callback's freshness check cannot be removed by + # editing the URL. + state_payload["reauth_at"] = int(time.time()) state_jwt = jwt.encode(state_payload, config.cookie_secret, algorithm="HS256") # Build the authorization URL. @@ -198,9 +206,10 @@ async def login(request: Request) -> Response: } if reauth: # OIDC Core 3.1.2.1: re-prompt even when the IdP has a session. - # Only on this path — as a default it would cost a password on - # every sign-in, and get switched off. + # `max_age=0` makes it enforceable — it obliges a conforming IdP + # to return `auth_time`, which the callback then verifies. params["prompt"] = "login" + params["max_age"] = "0" auth_url = config.authorization_endpoint + "?" + urlencode(params) response = RedirectResponse(url=auth_url, status_code=302) @@ -318,6 +327,17 @@ async def callback(request: Request) -> Response: else: email = _resolve_oidc_email(token_json, config) + # This login was demanded by a device-grant consent bounce, so a + # session the IdP simply reused is not good enough. + reauth_at = state_payload.get("reauth_at") + if isinstance(reauth_at, int) and not _reauthenticated_after( + token_json, config, reauth_at + ): + return JSONResponse( + status_code=403, + content={"error": "Re-authentication was required but did not occur"}, + ) + if not email: return JSONResponse( status_code=400, @@ -781,6 +801,84 @@ def _claim_is_verified_true(value: object) -> bool: return isinstance(value, str) and value.strip().lower() == "true" +def _verified_id_token_claims( + token_json: dict[str, object], + config: OIDCConfig, +) -> dict[str, object] | None: + """Validate the ``id_token`` and return its claims. + + Checks the JWT signature against the IdP's JWKS and verifies ``iss`` + and ``aud``. Shared by every consumer so no caller can read a claim out + of an unverified token. + + :param token_json: The token endpoint response JSON. + :param config: The OIDC configuration with JWKS URI and expected + issuer/audience. + :returns: The verified claims, or ``None`` when the token is missing, + unverifiable, or the config has no JWKS URI. + """ + id_token = token_json.get("id_token") + if not isinstance(id_token, str) or not id_token: + return None + if config.jwks_uri is None: + _logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI") + return None + + try: + jwks_client = jwt.PyJWKClient(config.jwks_uri) + signing_key = jwks_client.get_signing_key_from_jwt(id_token) + claims: dict[str, object] = jwt.decode( + id_token, + signing_key.key, + algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"], + audience=config.client_id, + issuer=config.issuer, + ) + except jwt.InvalidTokenError as exc: + _logger.warning("id_token validation failed: %s", exc) + return None + + return claims + + +def _reauthenticated_after( + token_json: dict[str, object], + config: OIDCConfig, + not_before: int, +) -> bool: + """Did the IdP actually re-authenticate the user for this login? + + ``prompt=login`` and ``max_age=0`` are requests. This is the check that + they were honoured: a conforming IdP that re-authenticates must report + when it did, via the ``auth_time`` claim, and that moment has to fall + after the bounce that demanded it. + + Fails closed. A missing ``auth_time`` means the IdP did not answer the + question, which is indistinguishable from it having reused an existing + session — and this is the only gate standing between a phished consent + link and a delegated grant. + + :param token_json: The token endpoint response JSON. + :param config: The OIDC configuration. + :param not_before: Epoch seconds the re-authentication must postdate. + :returns: True only on a proven fresh authentication. + """ + claims = _verified_id_token_claims(token_json, config) + if claims is None: + return False + + auth_time = claims.get("auth_time") + if not isinstance(auth_time, int): + _logger.warning( + "Forced re-authentication could not be verified: the id_token has no " + "integer auth_time claim, so the IdP may have reused an existing session" + ) + return False + + # Small tolerance for clock skew between us and the IdP. + return auth_time >= not_before - _REAUTH_CLOCK_SKEW_SECONDS + + def _resolve_oidc_email( token_json: dict[str, object], config: OIDCConfig, @@ -819,25 +917,8 @@ def _resolve_oidc_email( ``email_verified`` is not truthy (and verification is not skipped via config). """ - id_token = token_json.get("id_token") - if not isinstance(id_token, str) or not id_token: - return None - if config.jwks_uri is None: - _logger.warning("Rejecting id_token: OIDC configuration has no JWKS URI") - return None - - try: - jwks_client = jwt.PyJWKClient(config.jwks_uri) - signing_key = jwks_client.get_signing_key_from_jwt(id_token) - claims = jwt.decode( - id_token, - signing_key.key, - algorithms=["RS256", "RS384", "RS512", "ES256", "ES384", "ES512"], - audience=config.client_id, - issuer=config.issuer, - ) - except jwt.InvalidTokenError as exc: - _logger.warning("id_token validation failed: %s", exc) + claims = _verified_id_token_claims(token_json, config) + if claims is None: return None email = claims.get(config.email_claim) diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 5e5d4a6bea..72e40dfd07 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -341,6 +341,126 @@ def _build_accounts_app( yield client +def _build_oidc_app( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + *, + provider_type: str = "oidc", +) -> Iterator[TestClient]: + """Build a real app through ``create_app`` with an OIDC auth provider. + + The provider is constructed directly rather than through + ``create_auth_provider`` so no IdP discovery request is made; everything + downstream — including the device-grant mount decision — is the + production path. + """ + monkeypatch.setenv("OMNIGENT_DEVICE_GRANT_ENABLED", "1") + monkeypatch.delenv("OMNIGENT_AUTH_PROVIDER", raising=False) + + db_url = f"sqlite:///{tmp_path}/test.db" + from omnigent.db.utils import get_or_create_engine + from omnigent.runtime import init as init_runtime + from omnigent.runtime import telemetry + from omnigent.runtime.agent_cache import AgentCache + from omnigent.runtime.caps import RuntimeCaps + from omnigent.server.app import create_app + from omnigent.server.auth import UnifiedAuthProvider + from omnigent.server.oidc import OIDCConfig + from omnigent.stores.agent_store.sqlalchemy_store import SqlAlchemyAgentStore + from omnigent.stores.artifact_store.local import LocalArtifactStore + from omnigent.stores.comment_store.sqlalchemy_store import SqlAlchemyCommentStore + from omnigent.stores.conversation_store.sqlalchemy_store import SqlAlchemyConversationStore + from omnigent.stores.file_store.sqlalchemy_store import SqlAlchemyFileStore + from omnigent.stores.host_store import HostStore + from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore + + get_or_create_engine(db_url) + telemetry.init() + permission_store = SqlAlchemyPermissionStore(db_url) + agent_store = SqlAlchemyAgentStore(db_url) + conversation_store = SqlAlchemyConversationStore(db_url) + file_store = SqlAlchemyFileStore(db_url) + comment_store = SqlAlchemyCommentStore(db_url) + host_store = HostStore(db_url) + artifact_store = LocalArtifactStore(str(tmp_path / "artifacts")) + agent_cache = AgentCache(artifact_store=artifact_store, cache_dir=tmp_path / "cache") + init_runtime( + agent_cache=agent_cache, + caps=RuntimeCaps(), + agent_store=agent_store, + file_store=file_store, + conversation_store=conversation_store, + artifact_store=artifact_store, + comment_store=comment_store, + ) + + config = OIDCConfig( + issuer="https://accounts.google.com", + client_id="cid", + client_secret="secret", + redirect_uri="http://localhost:8000/auth/callback", + cookie_secret=bytes.fromhex("bb" * 32), + scopes="openid email profile", + session_ttl_hours=8, + logout_redirect_uri=None, + allowed_domains=None, + provider_type=provider_type, + authorization_endpoint="https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint="https://oauth2.googleapis.com/token", + jwks_uri="https://www.googleapis.com/oauth2/v3/certs", + userinfo_endpoint=None, + allow_invites=False, + ) + app = create_app( + agent_store=agent_store, + file_store=file_store, + conversation_store=conversation_store, + artifact_store=artifact_store, + agent_cache=agent_cache, + comment_store=comment_store, + permission_store=permission_store, + host_store=host_store, + auth_provider=UnifiedAuthProvider(source="oidc", oidc_config=config), + ) + with TestClient(app) as client: + yield client + + +def test_create_app_mounts_the_grant_under_oidc( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The mount decision must be exercised through ``create_app`` itself. + + Testing the factory directly proves the router builds; it does not prove + the app ever calls it. A typo in the mount condition would silently leave + ``/oauth/*`` absent under OIDC with every other test still green. + """ + for client in _build_oidc_app(tmp_path, monkeypatch): + res = client.post("/oauth/device/authorize", json={"client_id": "polly"}) + assert res.status_code == 200, f"/oauth/device/authorize returned {res.status_code}" + assert res.json()["user_code"] + + +def test_create_app_refuses_the_grant_for_github_oauth( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """GitHub is an ``oidc`` source that cannot honour the re-auth gate. + + The app must boot without it rather than mount routes whose security + property does not hold. + """ + for client in _build_oidc_app(tmp_path, monkeypatch, provider_type="github"): + # Asserted against the route table, not a status code: the SPA + # catch-all answers unmounted paths, so "not 200" would also pass if + # the routes were mounted and merely erroring. + oauth_routes = [ + route.path + for route in client.app.routes # type: ignore[attr-defined] + if getattr(route, "path", "").startswith("/oauth/") + ] + assert oauth_routes == [], f"the grant must not mount for GitHub OAuth: {oauth_routes}" + + @pytest.fixture def app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]: yield from _build_accounts_app(tmp_path, monkeypatch) diff --git a/tests/server/test_oidc_callback.py b/tests/server/test_oidc_callback.py index e92933b2df..79991c0431 100644 --- a/tests/server/test_oidc_callback.py +++ b/tests/server/test_oidc_callback.py @@ -186,7 +186,11 @@ async def _fake_post( yield client, keys -def _do_callback(client: TestClient, id_token: str) -> httpx.Response: +def _do_callback( + client: TestClient, + id_token: str, + extra_state: dict[str, object] | None = None, +) -> httpx.Response: """Drive a full ``/auth/callback`` with a valid state cookie. Crafts the signed state cookie the way ``/auth/login`` would, sets @@ -206,6 +210,7 @@ def _do_callback(client: TestClient, id_token: str) -> httpx.Response: "code_verifier": "verifier", "return_to": "/", "exp": int(time.time()) + 300, + **(extra_state or {}), }, _TEST_SECRET, algorithm="HS256", @@ -478,3 +483,80 @@ def test_callback_accepts_boolean_and_string_true( # Accepted as a verified identity → redirect + session. assert resp.status_code == 302, resp.text assert resp.cookies.get("ap_session") is not None + + +# ── Forced re-authentication is verified, not merely requested ──── + + +def test_reauth_login_accepts_a_proven_fresh_authentication( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """An ``auth_time`` after the bounce is what the gate is looking for.""" + client, keys = callback_client + bounced_at = int(time.time()) + token = keys.sign_id_token( + {"email": "alice@example.com", "email_verified": True, "auth_time": bounced_at + 1} + ) + + res = _do_callback(client, token, extra_state={"reauth_at": bounced_at}) + + assert res.status_code == 302, res.text + + +def test_reauth_login_refuses_a_session_the_idp_reused( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The whole point: ``prompt=login`` is a request, this is the check. + + An IdP that ignores it returns a token whose ``auth_time`` predates the + bounce. Accepting that would mint a session with a fresh ``iat``, clear + the device-grant consent gate, and delegate authority to a client the + user never re-authenticated for. + """ + client, keys = callback_client + bounced_at = int(time.time()) + token = keys.sign_id_token( + { + "email": "alice@example.com", + "email_verified": True, + # Signed in an hour ago and never re-prompted. + "auth_time": bounced_at - 3600, + } + ) + + res = _do_callback(client, token, extra_state={"reauth_at": bounced_at}) + + assert res.status_code == 403 + assert "Re-authentication" in res.json()["error"] + assert "Set-Cookie" not in res.headers, "a refused re-auth must not mint a session" + + +def test_reauth_login_fails_closed_without_auth_time( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """No ``auth_time`` is not a pass. + + ``max_age=0`` obliges a conforming IdP to report when it authenticated + the user. Silence is indistinguishable from a reused session, and this + gate is the only thing between a phished consent link and a grant. + """ + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + res = _do_callback(client, token, extra_state={"reauth_at": int(time.time())}) + + assert res.status_code == 403 + + +def test_an_ordinary_login_is_unaffected_by_the_reauth_check( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """No ``reauth_at`` in the state ⇒ no ``auth_time`` requirement. + + The check must not leak into normal sign-in, where most IdPs omit the + claim entirely and every login would start failing. + """ + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + assert _do_callback(client, token).status_code == 302 diff --git a/tests/server/test_oidc_reauth_prompt.py b/tests/server/test_oidc_reauth_prompt.py index a3a9655396..f1ffe21295 100644 --- a/tests/server/test_oidc_reauth_prompt.py +++ b/tests/server/test_oidc_reauth_prompt.py @@ -138,3 +138,49 @@ def test_re_authentication_still_round_trips_the_return_to(oidc_client: TestClie claims = jwt.decode(cookie, _TEST_SECRET, algorithms=["HS256"]) assert unquote(claims["return_to"]) == "/oauth/device?user_code=K7M2-QP9X" + + +# ── The consent page against a REAL OIDCConfig ──────────────────── + + +def test_consent_page_renders_for_a_real_oidc_session_cookie(tmp_path: Path) -> None: + """A genuine OIDC session must satisfy the consent page's own cookie read. + + ``_session_iat`` reads ``cookie_config.session_cookie_name`` and verifies + with ``cookie_config.cookie_secret``. If either diverged from what + ``/auth/callback`` actually sets, it would return ``None`` on every + request and the consent page would bounce forever — a login loop with no + error and no failing test. A hand-built ``SimpleNamespace`` cannot catch + that; this drives the real config and the real minting helper. + """ + import time + + from fastapi import FastAPI + + from omnigent.server.device_grant_store import DeviceGrantStore + from omnigent.server.oidc import mint_session_cookie + from omnigent.server.routes.device_auth import create_device_auth_router + + config = _oidc_config() + provider = UnifiedAuthProvider(source="oidc", oidc_config=config) + store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") + + app = FastAPI() + app.include_router(create_device_auth_router(provider, store)) + + with TestClient(app) as client: + res = client.post("/oauth/device/authorize", json={"client_id": "polly"}) + assert res.status_code == 200, res.text + user_code = res.json()["user_code"] + + # A login that happens AFTER the grant, exactly as the forced bounce + # would produce. + time.sleep(1) + session = mint_session_cookie("alice@example.com", config.cookie_secret, 8, "oidc") + client.cookies.set(config.session_cookie_name, session) + + page = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) + + assert page.status_code == 200, f"consent bounced instead of rendering: {page.headers}" + assert "alice@example.com" in page.text, "the consent screen must name the identity" + assert "polly" in page.text, "and the client asking for access" From 0ae12a9c823c49c021753f36ec40f0060c72e03e Mon Sep 17 00:00:00 2001 From: Andrew Peltekci Date: Thu, 6 Aug 2026 10:05:29 -0700 Subject: [PATCH 4/5] fix(auth): prove the re-authentication on the session, not with iat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second review round. The High is the same class of hole as the previous two: a control that reads as present while a path around it stays open. ## The gate could be skipped rather than beaten Consent required the session's `iat` to postdate the grant, on the reasoning that only a login started for this flow could produce a fresh one. It could not: `/auth/login` is a public GET accepting any same-origin `return_to`, so an attacker who starts a grant sends the victim /auth/login?return_to=%2Foauth%2Fdevice%3Fuser_code%3DXXXX with no `reauth=1`. Nothing is demanded, the IdP satisfies it from its own session, the callback mints a cookie with `iat` of now, and consent renders. The forced-re-authentication path was never entered, so hardening it changed nothing. The victim clicked one link. `iat` cannot carry this property: every completed callback has a fresh one, including the ones with no user involvement at all. So the proof is now recorded on the session as an `auth_time` claim, written only where a credential was actually presented — an accounts password submit, or an IdP-attested re-authentication — and consent requires `auth_time >= grant.created_at`. A login that skipped the bounce carries no claim, so it bounces and is made to prove itself; the demand no longer depends on the attacker's link having asked for it. `reauth_at` stays as the marker that makes the callback 403 outright rather than merely decline to stamp the proof. Both the consent GET and the approve POST read the new claim, so neither can be reached by posting directly. An IdP that never emits `auth_time` now cannot carry a grant: one bounce, then a 403 with a clear error. That terminates instead of looping, and it is the correct answer for a provider that cannot support the control. ## Freshness is compared on one clock `auth_time >= reauth_at - 60` mixed the IdP's clock with ours, so a 60s window admitted an authentication performed *before* the bounce that demanded it. It now compares `auth_time` against the id_token's own `iat` — both the IdP's — so skew between the two servers cancels out and a reused session fails regardless of whose clock is ahead. The remaining allowance covers the IdP's processing between authenticating the user and signing the token, which is what it should have been measuring. ## Allowlist, not denylist `unsupported_reason` denied `provider_type == "github"` and admitted every other string, including a `None` config. `from_env` yields only `github` or `oidc` today so nothing was broken, but the next OAuth dialect modelled under this source would have been admitted by default, behind a gate that cannot hold for it. Now only `"oidc"` qualifies, and the refusal names the value it rejected. ## The refusal was invisible where it mattered most The warning sat inside the auth-router mount, which is gated on `login_url` being truthy — and header mode's is `None`. Header mode is one of the two cases the predicate exists to explain, so the operator least able to work out why `/oauth/*` was missing was the only one who got no explanation. Hoisted above that gate. ## The chain was tested at both ends and joined nowhere `max_age=0` and the `reauth_at` marker are what make the gate enforceable, and neither was asserted: the authorize tests checked `prompt` only, and the callback tests injected `reauth_at` by hand instead of letting `/auth/login` write it. Either line could be deleted with the suite green. Both ends are now pinned, and the state cookie is decoded to assert the marker is present with `reauth=1` and absent without it. ## Verified - 290 tests pass across the auth suite, integration and e2e - Every fix mutation-checked: gating on `iat` fails 1, denylisting instead of allowlisting fails 1, dropping `max_age=0` fails 1, renaming the `reauth_at` marker fails 1, stamping `auth_time` without the IdP's attestation fails 4 - ruff, ruff format, mypy clean Signed-off-by: Andrew Peltekci --- designs/DEVICE_AUTH.md | 59 +++++++++---- omnigent/server/app.py | 42 +++++---- omnigent/server/oidc.py | 24 ++++- omnigent/server/routes/accounts_auth.py | 12 +++ omnigent/server/routes/auth.py | 60 +++++++++---- omnigent/server/routes/device_auth.py | 65 +++++++++----- tests/server/test_device_auth.py | 111 ++++++++++++++++++++++-- tests/server/test_oidc_callback.py | 78 ++++++++++++++++- tests/server/test_oidc_reauth_prompt.py | 57 ++++++++++-- 9 files changed, 412 insertions(+), 96 deletions(-) diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index 31b5959cfe..e9e6280725 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -208,23 +208,48 @@ live IdP session, and the consent page cannot tell the two cases apart. **Requested, then verified.** `prompt=login` is only a request, so the bounce also sends `max_age=0`, which obliges a conforming IdP to report the moment it -authenticated the user in the `auth_time` claim. `/auth/login` signs the bounce -time into the state cookie as `reauth_at`, and `/auth/callback` refuses (403, -no session minted) unless the returned `auth_time` postdates it. A missing -`auth_time` is refused too: silence is indistinguishable from a reused session, -and this gate is the only thing between a phished consent link and a delegated -grant. Ordinary logins carry no `reauth_at` and are unaffected — most IdPs omit -the claim, and requiring it everywhere would break every sign-in. - -**Why GitHub is excluded.** `OIDCConfig.from_env` accepts GitHub as an `oidc` -source, but points it at `https://github.com/login/oauth/authorize` — plain -OAuth 2.0, which has no `prompt` parameter. `prompt=login` would be ignored, -GitHub would reuse its session, and the callback's fresh `iat` would clear the -gate. `unsupported_reason` therefore refuses the grant for that provider -outright: a grant issued behind a gate that cannot hold is worse than no grant, -because it looks protected. `app.py` logs the refusal so an operator who set -`OMNIGENT_DEVICE_GRANT_ENABLED` is told why `/oauth/*` is absent instead of -assuming the flag did not take. The `device_grants` table is created +authenticated the user in the `auth_time` claim. The callback compares that +claim against the id_token's own `iat` — both the IdP's clock, so skew between +the two servers cancels out and a session established minutes earlier fails the +comparison regardless of whose clock is ahead. A missing `auth_time` fails too: +silence is indistinguishable from a reused session. + +**The proof rides on the session, not on `iat`.** Gating consent on the session +cookie's `iat` was bypassable without ever attacking the gate. `/auth/login` is +a public GET accepting any same-origin `return_to`, so an attacker who starts a +grant can send the victim `/auth/login?return_to=/oauth/device?user_code=…` +with **no** `reauth=1` — no `reauth_at` is signed, nothing is demanded, the IdP +satisfies it from its own session, and the resulting cookie carries an `iat` of +*now* that clears the gate. The forced path was never entered. + +So a successful re-authentication is recorded on the session itself, as an +`auth_time` claim (`mint_session_token`), written **only** where a credential +was actually presented: an accounts password submit, or an IdP-attested +re-authentication. Consent requires `auth_time ≥ grant.created_at`. A login +that skipped the bounce carries no claim, so it bounces and is made to prove +itself — the demand no longer depends on the attacker's link having asked for +it. `reauth_at` remains, signed into the state cookie, as the marker that makes +the callback *refuse* (403, no session minted) rather than merely decline to +stamp the proof. + +An IdP that never emits `auth_time` cannot carry a device grant: consent +bounces once, the forced callback 403s with a clear error, and the loop +terminates rather than spinning. + +**Which providers qualify.** `unsupported_reason` **allowlists** +`provider_type == "oidc"` rather than denying known-bad values. `from_env` +yields only `github` or `oidc` today, so the two are equivalent right now — but +the next OAuth 2.0 dialect modelled under the `oidc` source would otherwise be +admitted by default, behind a gate that cannot hold for it. GitHub is the +present example: `from_env` points it at +`https://github.com/login/oauth/authorize`, which has no `prompt` parameter, no +id_token and no `auth_time`, so there is no way to demand a re-authentication +nor to detect that none happened. + +`app.py` logs every refusal, and computes it **before** the auth-router mount — +that mount is gated on `login_url` being truthy, and header mode's is `None`, +so an operator in the one mode where the grant can never work was previously +the only one who never saw the explanation. The `device_grants` table is created unconditionally by the migration regardless of the flag; only the router mount is gated. This router **owns** `mint_delegated_token` and `DELEGATED_SCOPE`. diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 9892f611fe..e128471408 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2419,6 +2419,30 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: # /magic/redeem, /users, /users/{id}/reset, /users/me/password). # Must be registered BEFORE the SPA static mount because the SPA's # HTML5-history fallback catches all unmatched extensionless paths. + # Device-grant support is decided BEFORE the auth-router mount below, + # which is gated on `login_url` being truthy — and header mode's is None. + # Computing it inside that block meant the one operator most likely to be + # confused by a missing /oauth/* (header mode, where the grant can never + # work) was the one who never saw the explanation. + from omnigent.server.auth import UnifiedAuthProvider as _UnifiedAuthProvider + from omnigent.server.auth import env_var_is_truthy + from omnigent.server.routes.device_auth import unsupported_reason + + _device_grant_wanted = env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) + _device_grant_blocked = ( + unsupported_reason(auth_provider) + if isinstance(auth_provider, _UnifiedAuthProvider) + else "a custom auth provider cannot mint the session the grant delegates from" + ) + if _device_grant_wanted and _device_grant_blocked is not None: + # Asked for and refused: say so, or the operator sees only the absence + # of /oauth/* and assumes the flag did not take. + _logger.warning( + "device-grant: OMNIGENT_DEVICE_GRANT_ENABLED is set but the " + "/oauth/* routes are NOT mounted — %s. See designs/DEVICE_AUTH.md.", + _device_grant_blocked, + ) + if auth_provider is not None and getattr(auth_provider, "login_url", None): from omnigent.server.auth import UnifiedAuthProvider @@ -2485,24 +2509,6 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: # lookup into the auth provider so revoking a grant immediately # rejects its delegated access tokens. # See designs/DEVICE_AUTH.md. - from omnigent.server.auth import env_var_is_truthy - from omnigent.server.routes.device_auth import unsupported_reason - - _device_grant_wanted = env_var_is_truthy("OMNIGENT_DEVICE_GRANT_ENABLED", default=False) - _device_grant_blocked = ( - unsupported_reason(auth_provider) - if isinstance(auth_provider, UnifiedAuthProvider) - else "a custom auth provider cannot mint the session the grant delegates from" - ) - if _device_grant_wanted and _device_grant_blocked is not None: - # Asked for and refused: say so, or the operator sees only the - # absence of /oauth/* and assumes the flag did not take. - _logger.warning( - "device-grant: OMNIGENT_DEVICE_GRANT_ENABLED is set but the " - "/oauth/* routes are NOT mounted — %s. See designs/DEVICE_AUTH.md.", - _device_grant_blocked, - ) - if ( _device_grant_wanted and isinstance(auth_provider, UnifiedAuthProvider) diff --git a/omnigent/server/oidc.py b/omnigent/server/oidc.py index 9b676182eb..d142f253f6 100644 --- a/omnigent/server/oidc.py +++ b/omnigent/server/oidc.py @@ -55,6 +55,8 @@ def mint_session_token( cookie_secret: bytes, ttl_seconds: int, provider: str, + *, + auth_time: int | None = None, ) -> str: """ Mint a signed session JWT with a second-granularity lifetime. @@ -65,21 +67,34 @@ def mint_session_token( same validator (:meth:`UnifiedAuthProvider._check_cookie`) accepts either. + ``auth_time`` records **when this server last saw the user prove who + they are**, on this server's clock. It is deliberately not the same as + ``iat``: minting a session proves only that a login flow completed, + which under OIDC can happen with no user involvement at all when the + IdP reuses its own session. Callers pass it only where a credential + was actually presented — a password submit, or an IdP-attested + re-authentication. Absent otherwise, and consumers must fail closed on + its absence. See :mod:`omnigent.server.routes.device_auth`. + :param user_id: The authenticated user's email, e.g. ``"alice@example.com"``. :param cookie_secret: HMAC key for HS256 signing. :param ttl_seconds: Token lifetime in seconds. :param provider: Identity provider name, e.g. ``"google"`` or ``"accounts"``. Stored as an informational claim. + :param auth_time: Epoch seconds (this server's clock) at which the + user proved their identity, or ``None`` when unproven. :returns: An HS256-signed JWT string. """ now = int(time.time()) - payload = { + payload: dict[str, object] = { "sub": user_id, "iat": now, "exp": now + ttl_seconds, "provider": provider, } + if auth_time is not None: + payload["auth_time"] = auth_time return jwt.encode(payload, cookie_secret, algorithm="HS256") @@ -88,6 +103,8 @@ def mint_session_cookie( cookie_secret: bytes, ttl_hours: int, provider: str, + *, + auth_time: int | None = None, ) -> str: """Mint a signed session cookie JWT. @@ -97,9 +114,12 @@ def mint_session_cookie( :param ttl_hours: Session lifetime in hours. :param provider: Identity provider name, e.g. ``"google"`` or ``"github"``. Stored as an informational claim. + :param auth_time: See :func:`mint_session_token`. :returns: An HS256-signed JWT string. """ - return mint_session_token(user_id, cookie_secret, ttl_hours * 3600, provider) + return mint_session_token( + user_id, cookie_secret, ttl_hours * 3600, provider, auth_time=auth_time + ) def hmac_digest(token: str, secret: bytes) -> str: diff --git a/omnigent/server/routes/accounts_auth.py b/omnigent/server/routes/accounts_auth.py index 75b9834501..224465e7b9 100644 --- a/omnigent/server/routes/accounts_auth.py +++ b/omnigent/server/routes/accounts_auth.py @@ -299,6 +299,9 @@ async def login(body: LoginRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) user = account_store.get_user(username) @@ -446,6 +449,9 @@ async def register(body: RegisterRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = JSONResponse( status_code=200, @@ -545,6 +551,9 @@ async def setup(body: SetupRequest) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = JSONResponse( status_code=200, @@ -631,6 +640,9 @@ async def magic_redeem(request: Request) -> Response: cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider="accounts", + # The server verified the credential itself just now, so this is + # a local fact rather than a third party's claim about one. + auth_time=int(time.time()), ) resp = RedirectResponse(url="/", status_code=302) _set_session_cookie( diff --git a/omnigent/server/routes/auth.py b/omnigent/server/routes/auth.py index 9a81305d60..9bb160b081 100644 --- a/omnigent/server/routes/auth.py +++ b/omnigent/server/routes/auth.py @@ -46,8 +46,9 @@ _AUTH_STATE_COOKIE_PLAIN = "ap_auth_state" _AUTH_STATE_TTL_SECONDS = 300 # 5 minutes _CLI_TICKET_TTL_SECONDS = 300 # 5 minutes -# Tolerance when comparing the IdP's `auth_time` against our own clock. -_REAUTH_CLOCK_SKEW_SECONDS = 60 +# How long the IdP may take between authenticating the user and signing the +# id_token. Both timestamps are the IdP's own, so this is not clock skew. +_REAUTH_PROCESSING_ALLOWANCE_SECONDS = 120 # How long an OIDC invite URL stays redeemable. Matches the accounts # provider's default invite window (72h) — long enough to share # out-of-band, short enough to bound exposure of an unused link. @@ -287,6 +288,11 @@ async def callback(request: Request) -> Response: "code_verifier": code_verifier, } + # When (on our clock) the user proved their identity to the IdP for + # THIS login. Stays None unless the IdP attests to it — GitHub OAuth + # never can, which is why it cannot carry a device grant. + proven_at: int | None = None + async with httpx.AsyncClient() as client: # GitHub requires Accept: application/json to get JSON # response from the token endpoint. @@ -327,12 +333,17 @@ async def callback(request: Request) -> Response: else: email = _resolve_oidc_email(token_json, config) + # Record whether the user actually proved themselves here, rather + # than the IdP silently reusing its own session. Checked on EVERY + # login, not only the forced ones: the device-grant consent gate + # reads this, and a login that skipped the bounce would otherwise + # look identical to one that honoured it. + if _idp_reauthenticated(token_json, config): + proven_at = int(time.time()) + # This login was demanded by a device-grant consent bounce, so a # session the IdP simply reused is not good enough. - reauth_at = state_payload.get("reauth_at") - if isinstance(reauth_at, int) and not _reauthenticated_after( - token_json, config, reauth_at - ): + if state_payload.get("reauth_at") is not None and proven_at is None: return JSONResponse( status_code=403, content={"error": "Re-authentication was required but did not occur"}, @@ -387,12 +398,16 @@ async def callback(request: Request) -> Response: permission_store.ensure_user(email) promote_if_listed(admin_list, permission_store, email) - # Mint session cookie. + # Mint session cookie. `auth_time` carries the proof forward: under + # OIDC a completed callback says nothing about user involvement, so + # consumers that need a deliberate authentication must read this + # rather than `iat`. session_jwt = mint_session_cookie( user_id=email, cookie_secret=config.cookie_secret, ttl_hours=config.session_ttl_hours, provider=config.provider_type, + auth_time=proven_at, ) # Check if this callback fulfills a CLI login ticket. @@ -841,26 +856,30 @@ def _verified_id_token_claims( return claims -def _reauthenticated_after( +def _idp_reauthenticated( token_json: dict[str, object], config: OIDCConfig, - not_before: int, ) -> bool: - """Did the IdP actually re-authenticate the user for this login? + """Did the IdP authenticate the user *for this login*, or reuse a session? ``prompt=login`` and ``max_age=0`` are requests. This is the check that - they were honoured: a conforming IdP that re-authenticates must report - when it did, via the ``auth_time`` claim, and that moment has to fall - after the bounce that demanded it. + they were honoured: a conforming IdP reports when it authenticated the + user via ``auth_time``, and for a genuine re-authentication that moment + coincides with this token's own issuance. + + Both values come from the IdP's clock, so they are compared against each + other rather than against ours — a skew between the two servers cancels + out, and a session the IdP established minutes or hours ago fails the + comparison no matter whose clock is ahead. The allowance covers the + IdP's own processing between authenticating the user and signing the + token, not clock drift. Fails closed. A missing ``auth_time`` means the IdP did not answer the question, which is indistinguishable from it having reused an existing - session — and this is the only gate standing between a phished consent - link and a delegated grant. + session. :param token_json: The token endpoint response JSON. :param config: The OIDC configuration. - :param not_before: Epoch seconds the re-authentication must postdate. :returns: True only on a proven fresh authentication. """ claims = _verified_id_token_claims(token_json, config) @@ -870,13 +889,16 @@ def _reauthenticated_after( auth_time = claims.get("auth_time") if not isinstance(auth_time, int): _logger.warning( - "Forced re-authentication could not be verified: the id_token has no " + "Re-authentication could not be verified: the id_token has no " "integer auth_time claim, so the IdP may have reused an existing session" ) return False - # Small tolerance for clock skew between us and the IdP. - return auth_time >= not_before - _REAUTH_CLOCK_SKEW_SECONDS + issued_at = claims.get("iat") + if not isinstance(issued_at, int): + return False + + return auth_time >= issued_at - _REAUTH_PROCESSING_ALLOWANCE_SECONDS def _resolve_oidc_email( diff --git a/omnigent/server/routes/device_auth.py b/omnigent/server/routes/device_auth.py index 08b0b820dd..3646a72a5f 100644 --- a/omnigent/server/routes/device_auth.py +++ b/omnigent/server/routes/device_auth.py @@ -287,11 +287,20 @@ def unsupported_reason(auth_provider: UnifiedAuthProvider) -> str | None: return f"{source!r} auth has no server-minted session to delegate from" if source == "oidc": config = auth_provider._oidc_config - if config is not None and config.provider_type == "github": + if config is None: + return "oidc auth is selected but no OIDC config is present" + # Allowlisted, not denylisted. `provider_type` distinguishes real + # OIDC from OAuth 2.0 dialects modelled under the same source, and + # only the former can be asked to re-authenticate (`prompt=login`) + # and made to attest that it did (`auth_time`). Admitting an + # unaudited value by default would silently issue grants behind a + # gate that cannot hold — which is the failure this predicate exists + # to prevent. + if config.provider_type != "oidc": return ( - "GitHub OAuth cannot be asked to re-authenticate an already " - "signed-in user (no OIDC 'prompt' parameter), so the consent " - "page's forced-re-authentication gate would not hold" + f"{config.provider_type!r} is not full OIDC, so it cannot be asked to " + "re-authenticate an already signed-in user ('prompt=login') nor attest " + "that it did ('auth_time'), and the consent page's gate would not hold" ) return None @@ -450,14 +459,24 @@ def _bounce_to_login(user_code: str) -> RedirectResponse: query = f"return_to={html.escape(return_to, quote=True)}&reauth=1" return RedirectResponse(url=f"{login_url}?{query}", status_code=302) - def _session_iat(request: Request) -> int | None: - """Return the ``iat`` (issue time) of the caller's session JWT. + def _session_auth_time(request: Request) -> int | None: + """When did this session's owner last *prove* who they are? - Read from the session cookie. Both modes mint a fresh ``iat`` on - every completed login — accounts on the ``/auth/login`` POST, OIDC - in the ``/auth/callback`` handler — so this is effectively the - last-login time. ``None`` when absent/invalid. Used to enforce that - consent follows a login started FOR this device flow. + Deliberately not ``iat``. Minting a session proves only that a login + flow completed, and under OIDC that can happen with no user + involvement: an attacker who sends a victim a plain + ``/auth/login?return_to=/oauth/device?user_code=…`` link — no + ``reauth=1`` — gets the IdP to satisfy it from its own session, and + the resulting cookie carries an ``iat`` of *now*. Gating on ``iat`` + therefore let a crafted link walk straight past the consent check by + simply never asking for the re-authentication. + + ``auth_time`` is written only where a credential was actually + presented (see :func:`omnigent.server.oidc.mint_session_token`), so + it cannot be manufactured by starting a login flow. + + ``None`` when absent or invalid, which callers must treat as + unproven. """ token = request.cookies.get(cookie_config.session_cookie_name) if not token: @@ -466,8 +485,8 @@ def _session_iat(request: Request) -> int | None: payload = jwt.decode(token, cookie_secret, algorithms=["HS256"]) except jwt.InvalidTokenError: return None - iat = payload.get("iat") - return iat if isinstance(iat, int) else None + auth_time = payload.get("auth_time") + return auth_time if isinstance(auth_time, int) else None @router.get("/oauth/device") async def device_consent_page(request: Request) -> Response: @@ -509,12 +528,12 @@ async def device_consent_page(request: Request) -> Response: status_code=200, ) - # Force a fresh login when the current session predates this grant: - # only a login started for THIS flow (iat ≥ the grant's created_at) - # may approve. Bounce with reauth=1 so the login page re-prompts - # rather than auto-returning the stale session (which would loop). - session_iat = _session_iat(request) - if session_iat is None or session_iat < grant.created_at: + # Only a credential presented for THIS flow may approve it: the + # proven-authentication time must postdate the grant. Bounce with + # reauth=1 so the login page re-prompts rather than auto-returning + # the existing session (which would loop). + proven_at = _session_auth_time(request) + if proven_at is None or proven_at < grant.created_at: return _bounce_to_login(user_code) return HTMLResponse( @@ -550,10 +569,10 @@ async def device_approve(request: Request) -> Response: ) # Re-auth gate, enforced here too (not just on the consent GET): a - # stale session must not approve by POSTing directly. Only a login - # started for THIS flow (session iat ≥ the grant's created_at) passes. - session_iat = _session_iat(request) - if session_iat is None or session_iat < grant.created_at: + # stale session must not approve by POSTing directly. Only a + # credential proven for THIS flow passes. + proven_at = _session_auth_time(request) + if proven_at is None or proven_at < grant.created_at: return HTMLResponse( _consent_html( error="Your session is too old to approve this login. " diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 72e40dfd07..6befdde191 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -17,7 +17,7 @@ import logging import secrets -from collections.abc import Iterator +from collections.abc import Callable, Iterator from pathlib import Path import pytest @@ -75,7 +75,7 @@ def test_router_factory_rejects_github_oauth(tmp_path: Path) -> None: from omnigent.server.routes.device_auth import create_device_auth_router store = DeviceGrantStore(f"sqlite:///{tmp_path}/dg.db") - with pytest.raises(RuntimeError, match="GitHub OAuth"): + with pytest.raises(RuntimeError, match="not full OIDC"): create_device_auth_router(_oidc_provider("github"), store) # type: ignore[arg-type] @@ -346,16 +346,19 @@ def _build_oidc_app( monkeypatch: pytest.MonkeyPatch, *, provider_type: str = "oidc", + provider: object | None = None, ) -> Iterator[TestClient]: """Build a real app through ``create_app`` with an OIDC auth provider. The provider is constructed directly rather than through ``create_auth_provider`` so no IdP discovery request is made; everything downstream — including the device-grant mount decision — is the - production path. + production path. ``provider`` overrides it outright, for modes that need + no OIDC config at all. """ monkeypatch.setenv("OMNIGENT_DEVICE_GRANT_ENABLED", "1") - monkeypatch.delenv("OMNIGENT_AUTH_PROVIDER", raising=False) + if provider is None: + monkeypatch.delenv("OMNIGENT_AUTH_PROVIDER", raising=False) db_url = f"sqlite:///{tmp_path}/test.db" from omnigent.db.utils import get_or_create_engine @@ -420,7 +423,7 @@ def _build_oidc_app( comment_store=comment_store, permission_store=permission_store, host_store=host_store, - auth_provider=UnifiedAuthProvider(source="oidc", oidc_config=config), + auth_provider=provider or UnifiedAuthProvider(source="oidc", oidc_config=config), ) with TestClient(app) as client: yield client @@ -741,8 +744,12 @@ def test_no_secret_configured_stays_public(app: TestClient) -> None: assert r.status_code == 200, r.text -def _capture_app_warnings(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> list[str]: - """Build the accounts app and return WARNING messages from ``server.app``. +def _capture_app_warnings( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + builder: Callable[[], Iterator[TestClient]] | None = None, +) -> list[str]: + """Build an app and return WARNING messages from ``server.app``. Attaches a handler directly to the ``omnigent.server.app`` logger rather than using ``caplog``: the server's telemetry/logging init runs during the @@ -760,7 +767,7 @@ def emit(self, record: logging.LogRecord) -> None: handler = _Collector() logger.addHandler(handler) try: - for _ in _build_accounts_app(tmp_path, monkeypatch): + for _ in builder() if builder else _build_accounts_app(tmp_path, monkeypatch): break # build (and immediately tear down) so the mount runs finally: logger.removeHandler(handler) @@ -787,3 +794,91 @@ def test_startup_silent_when_client_secret_set( monkeypatch.setenv("OMNIGENT_DEVICE_CLIENT_SECRET", _SECRET) warnings = _capture_app_warnings(tmp_path, monkeypatch) assert not any("OMNIGENT_DEVICE_CLIENT_SECRET is not set" in m for m in warnings) + + +def _build_header_app(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]: + """A header/proxy-mode app with the device grant requested.""" + monkeypatch.delenv("OMNIGENT_OIDC_ISSUER", raising=False) + monkeypatch.setenv("OMNIGENT_AUTH_PROVIDER", "header") + monkeypatch.setenv("OMNIGENT_AUTH_ENABLED", "1") + from omnigent.server.auth import UnifiedAuthProvider + + yield from _build_oidc_app( + tmp_path, monkeypatch, provider=UnifiedAuthProvider(source="header") + ) + + +def test_refusal_is_explained_in_header_mode( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Header mode must still learn WHY /oauth/* is missing. + + It is one of the two cases ``unsupported_reason`` exists to explain, and + also the mode where the auth router itself never mounts (``login_url`` is + None). Deciding device-grant support inside that mount block meant the + operator most likely to be confused saw only silence. + """ + warnings = _capture_app_warnings( + tmp_path, monkeypatch, builder=lambda: _build_header_app(tmp_path, monkeypatch) + ) + + assert any( + "OMNIGENT_DEVICE_GRANT_ENABLED is set" in message and "no server-minted session" in message + for message in warnings + ), warnings + + +def test_refusal_is_explained_for_github_oauth( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Same for a GitHub-backed OIDC deployment.""" + warnings = _capture_app_warnings( + tmp_path, + monkeypatch, + builder=lambda: _build_oidc_app(tmp_path, monkeypatch, provider_type="github"), + ) + + assert any( + "OMNIGENT_DEVICE_GRANT_ENABLED is set" in message and "not full OIDC" in message + for message in warnings + ), warnings + + +def test_a_supported_deployment_logs_no_refusal( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The predicate must not over-refuse: real OIDC mounts silently.""" + warnings = _capture_app_warnings( + tmp_path, monkeypatch, builder=lambda: _build_oidc_app(tmp_path, monkeypatch) + ) + + assert not any("routes are NOT mounted" in message for message in warnings), warnings + + +def test_unsupported_reason_refuses_an_unaudited_provider_type() -> None: + """Allowlisted, not denylisted. + + ``from_env`` yields only ``github`` or ``oidc`` today, so denying the one + known-bad value happened to be equivalent. The next OAuth dialect modelled + under this source — or a directly-constructed config — would have been + admitted by default, behind a gate that cannot hold for it. + """ + + from omnigent.server.routes.device_auth import unsupported_reason + + for provider_type in ("github", "gitlab", "oauth2", ""): + reason = unsupported_reason(_oidc_provider(provider_type)) # type: ignore[arg-type] + assert reason is not None, provider_type + assert repr(provider_type) in reason, "the reason must name the value it refused" + + assert unsupported_reason(_oidc_provider("oidc")) is None # type: ignore[arg-type] + + +def test_unsupported_reason_refuses_oidc_with_no_config() -> None: + """A missing config is not a supported deployment.""" + from types import SimpleNamespace + + from omnigent.server.routes.device_auth import unsupported_reason + + provider = SimpleNamespace(_source="oidc", _oidc_config=None, _accounts_config=None) + assert unsupported_reason(provider) is not None # type: ignore[arg-type] diff --git a/tests/server/test_oidc_callback.py b/tests/server/test_oidc_callback.py index 79991c0431..54c0a0d345 100644 --- a/tests/server/test_oidc_callback.py +++ b/tests/server/test_oidc_callback.py @@ -495,7 +495,7 @@ def test_reauth_login_accepts_a_proven_fresh_authentication( client, keys = callback_client bounced_at = int(time.time()) token = keys.sign_id_token( - {"email": "alice@example.com", "email_verified": True, "auth_time": bounced_at + 1} + {"email": "alice@example.com", "email_verified": True, "auth_time": bounced_at} ) res = _do_callback(client, token, extra_state={"reauth_at": bounced_at}) @@ -519,7 +519,9 @@ def test_reauth_login_refuses_a_session_the_idp_reused( { "email": "alice@example.com", "email_verified": True, - # Signed in an hour ago and never re-prompted. + # Signed in an hour before this token was issued, and never + # re-prompted. Both stamps are the IdP's own clock, so the gap is + # real regardless of whose clock is ahead. "auth_time": bounced_at - 3600, } ) @@ -560,3 +562,75 @@ def test_an_ordinary_login_is_unaffected_by_the_reauth_check( token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) assert _do_callback(client, token).status_code == 302 + + +def _session_claims(res: httpx.Response) -> dict[str, object]: + """Decode the session cookie a successful callback set.""" + cookie = res.headers["set-cookie"] + token = cookie.split("ap_session=", 1)[1].split(";", 1)[0] + return jwt.decode(token, _TEST_SECRET, algorithms=["HS256"]) + + +def test_a_proven_login_stamps_auth_time_on_the_session( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The proof has to outlive the callback, or nothing downstream can see it. + + ``iat`` cannot carry it: every completed callback has a fresh one, + including the ones where the IdP reused its own session. + """ + client, keys = callback_client + now = int(time.time()) + token = keys.sign_id_token( + {"email": "alice@example.com", "email_verified": True, "auth_time": now} + ) + + claims = _session_claims(_do_callback(client, token)) + + assert isinstance(claims["auth_time"], int) + assert claims["auth_time"] >= now + + +def test_an_unmarked_login_link_cannot_manufacture_proof( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """The bypass: reach /auth/login WITHOUT reauth=1 and the gate is skipped. + + ``/auth/login`` is a public GET that accepts any same-origin + ``return_to``, so an attacker who starts a grant can send the victim + ``/auth/login?return_to=/oauth/device?user_code=XXXX`` — no ``reauth=1``, + therefore no ``reauth_at``, therefore no forced re-authentication. The IdP + satisfies it from its own session and the callback completes normally. + + That must not produce a session the consent gate accepts. No proof, no + ``auth_time`` — the gate then bounces and demands one. + """ + client, keys = callback_client + token = keys.sign_id_token( + { + "email": "alice@example.com", + "email_verified": True, + # The IdP reused a session from an hour before this token. + "auth_time": int(time.time()) - 3600, + } + ) + + res = _do_callback(client, token) # no reauth_at: the crafted-link case + + assert res.status_code == 302, "an ordinary login must still succeed" + assert "auth_time" not in _session_claims(res), ( + "a reused IdP session must not be recorded as proof of identity" + ) + + +def test_an_idp_that_omits_auth_time_proves_nothing( + callback_client: tuple[TestClient, _IdpKeys], +) -> None: + """Silence is not proof, even on an ordinary login.""" + client, keys = callback_client + token = keys.sign_id_token({"email": "alice@example.com", "email_verified": True}) + + res = _do_callback(client, token) + + assert res.status_code == 302 + assert "auth_time" not in _session_claims(res) diff --git a/tests/server/test_oidc_reauth_prompt.py b/tests/server/test_oidc_reauth_prompt.py index f1ffe21295..b63487048e 100644 --- a/tests/server/test_oidc_reauth_prompt.py +++ b/tests/server/test_oidc_reauth_prompt.py @@ -91,6 +91,10 @@ def test_reauth_forwards_prompt_login_to_the_idp(oidc_client: TestClient) -> Non params = _authorize_params(oidc_client, "?reauth=1&return_to=/oauth/device") assert params.get("prompt") == ["login"] + # `prompt` alone is only a request. `max_age=0` is what obliges a + # conforming IdP to return `auth_time`, which is what the callback then + # verifies — drop it and the gate degrades to asking politely. + assert params.get("max_age") == ["0"] # The rest of the request must be unchanged — PKCE and state still apply. assert params["code_challenge_method"] == ["S256"] assert params["response_type"] == ["code"] @@ -104,8 +108,10 @@ def test_an_ordinary_login_does_not_re_prompt(oidc_client: TestClient) -> None: every single sign-in, which is how a security control ends up switched off by whoever finds it annoying. """ - assert "prompt" not in _authorize_params(oidc_client, "") - assert "prompt" not in _authorize_params(oidc_client, "?return_to=/sessions") + for query in ("", "?return_to=/sessions"): + params = _authorize_params(oidc_client, query) + assert "prompt" not in params + assert "max_age" not in params @pytest.mark.parametrize("raw", ["0", "true", "yes", "", "1 ", "TRUE"]) @@ -173,14 +179,51 @@ def test_consent_page_renders_for_a_real_oidc_session_cookie(tmp_path: Path) -> assert res.status_code == 200, res.text user_code = res.json()["user_code"] - # A login that happens AFTER the grant, exactly as the forced bounce - # would produce. - time.sleep(1) - session = mint_session_cookie("alice@example.com", config.cookie_secret, 8, "oidc") - client.cookies.set(config.session_cookie_name, session) + # A session with no proven authentication — the shape an ordinary + # OIDC login produces when the IdP reuses its own session. + unproven = mint_session_cookie("alice@example.com", config.cookie_secret, 8, "oidc") + client.cookies.set(config.session_cookie_name, unproven) + bounced = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) + # An authentication proven AFTER the grant began, exactly as the + # forced bounce produces. + time.sleep(1) + proven = mint_session_cookie( + "alice@example.com", + config.cookie_secret, + 8, + "oidc", + auth_time=int(time.time()), + ) + client.cookies.set(config.session_cookie_name, proven) page = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) + assert bounced.status_code == 302, "an unproven session must not reach consent" + assert "reauth=1" in bounced.headers["location"] + assert page.status_code == 200, f"consent bounced instead of rendering: {page.headers}" assert "alice@example.com" in page.text, "the consent screen must name the identity" assert "polly" in page.text, "and the client asking for access" + + +def test_reauth_is_signed_into_the_state_cookie(oidc_client: TestClient) -> None: + """`/auth/login` must record the demand where the callback can trust it. + + The callback refuses a login that failed to re-authenticate only when + the state carries ``reauth_at``. Signed into the cookie rather than read + back off the URL, so it cannot be stripped by editing the redirect. + """ + import jwt + + oidc_client.get("/auth/login?reauth=1&return_to=/oauth/device", follow_redirects=False) + claims = jwt.decode(oidc_client.cookies["ap_auth_state"], _TEST_SECRET, algorithms=["HS256"]) + assert isinstance(claims.get("reauth_at"), int) + + +def test_an_ordinary_login_signs_no_reauth_marker(oidc_client: TestClient) -> None: + """Without it the callback must not demand proof of every sign-in.""" + import jwt + + oidc_client.get("/auth/login?return_to=/sessions", follow_redirects=False) + claims = jwt.decode(oidc_client.cookies["ap_auth_state"], _TEST_SECRET, algorithms=["HS256"]) + assert "reauth_at" not in claims From 575338735e0d050460f7dead740d5d2ce8a7d626 Mon Sep 17 00:00:00 2001 From: Andrew Peltekci Date: Thu, 6 Aug 2026 10:27:22 -0700 Subject: [PATCH 5/5] fix(auth): percent-encode the bounce return_to, and close three test gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four items raised in earlier review bodies rather than as inline comments, which is why they were missed while the anchored findings were being fixed. None is exploitable; all four are places where a test or an invariant reads as stronger than it is. ## The bounce built a URL with an HTML escaper `_bounce_to_login` ran the consent URL through `html.escape` before appending `&reauth=1`. That is an HTML escaper, not a URL one: it leaves `?`, `=`, `#` and `+` untouched, so a `#` in the code ended the URL and turned the rest into a fragment — silently truncating the `return_to` the user is supposed to come back to. The review that raised this described a different failure: a crafted `user_code` injecting `&reauth=0` ahead of the real parameter, letting the first value win. That does not reproduce — `html.escape` turns `&` into `&`, so the injected parameter arrives as `amp;reauth` and `reauth=1` is still the only `reauth`. The conclusion holds anyway, and for a better reason than the one given: the forced-re-auth invariant should not depend on an escaper's incidental handling of one character. `quote(..., safe="/")` is the correct tool and makes the question moot. A legitimate `user_code` draws from an alphabet with none of these characters, so nothing was reachable in practice. ## `"/login" in location` could not fail Three bounce assertions matched a substring that `/auth/login` also satisfies, so they passed under either provider and proved nothing about `login_url` in either direction. Now anchored: accounts must bounce to `/login?`, and the OIDC consent test asserts `/auth/login?`. ## An `unquote` hid what the state cookie actually stores The `return_to` round-trip test decoded the stored value before comparing, so it passed whether the query survived the bounce or not — the exact property it exists to check. Asserts the stored form directly now, with the input percent-encoded as `_bounce_to_login` emits it. ## The encoding had no test at all Confirmed by mutation: reverting to `html.escape` left the suite green. Added a test driving a `user_code` containing the characters `html.escape` ignores, asserting the consent URL round-trips intact and that `reauth=1` is the only `reauth` present. Reverting now fails it. ## Documentation Mounting the grant under OIDC changes neither first-party client: the CLI and Slack both still probe the mode and still take the cli-ticket flow there. Noted, along with what the OIDC device-grant routes are actually for — external clients that need a scoped, revocable, refreshable credential rather than the server's own session JWT. ## Verified - 291 tests pass across the auth suite, integration and e2e - The encoding fix is mutation-checked (reverting it fails 1) - ruff, ruff format, mypy clean Signed-off-by: Andrew Peltekci --- designs/DEVICE_AUTH.md | 8 +++++++ omnigent/server/routes/device_auth.py | 8 ++++++- tests/server/test_device_auth.py | 32 ++++++++++++++++++++++--- tests/server/test_oidc_reauth_prompt.py | 12 ++++++---- 4 files changed, 52 insertions(+), 8 deletions(-) diff --git a/designs/DEVICE_AUTH.md b/designs/DEVICE_AUTH.md index e9e6280725..2006895af7 100644 --- a/designs/DEVICE_AUTH.md +++ b/designs/DEVICE_AUTH.md @@ -38,6 +38,14 @@ > (`app.py`: device auth is `oidc`/`accounts` only), so `start_login` > raises a clear error rather than firing a request the server would 404. > +> **Note.** Mounting the grant under OIDC does not change either +> first-party client: the CLI (`cli.py`) and Slack (`oauth.py`) still +> probe the mode and still take the cli-ticket flow there. The OIDC +> device-grant routes exist for *external* clients — ones that must hold a +> scoped, revocable, refreshable credential rather than the server's own +> session JWT, and that cannot be trusted with account-wide authority. +> Migrating the first-party clients onto it is separate work. +> > Tests: `tests/server/test_device_auth.py`, and the Slack > `test_oauth.py` / `test_tokens.py` / `test_client_auth.py` / > `test_auth_manager.py`. diff --git a/omnigent/server/routes/device_auth.py b/omnigent/server/routes/device_auth.py index 3646a72a5f..6317b4c5cc 100644 --- a/omnigent/server/routes/device_auth.py +++ b/omnigent/server/routes/device_auth.py @@ -60,6 +60,7 @@ import os import secrets import time +from urllib.parse import quote import jwt from fastapi import APIRouter, HTTPException, Request @@ -456,7 +457,12 @@ def _bounce_to_login(user_code: str) -> RedirectResponse: """ login_url = auth_provider.login_url or "/login" return_to = f"/oauth/device?user_code={user_code}" if user_code else "/oauth/device" - query = f"return_to={html.escape(return_to, quote=True)}&reauth=1" + # Percent-encoded, not HTML-escaped: this is a URL query value, and + # `html.escape` leaves `?`, `=`, `#` and `+` untouched, so a user_code + # carrying one would truncate or corrupt the round-trip. `&` survives + # as `&` today, which is why `reauth=1` still wins — an accident + # of the escaper, not a property worth depending on. + query = f"return_to={quote(return_to, safe='/')}&reauth=1" return RedirectResponse(url=f"{login_url}?{query}", status_code=302) def _session_auth_time(request: Request) -> int | None: diff --git a/tests/server/test_device_auth.py b/tests/server/test_device_auth.py index 6befdde191..062e882df2 100644 --- a/tests/server/test_device_auth.py +++ b/tests/server/test_device_auth.py @@ -565,7 +565,7 @@ def test_consent_page_requires_login(app: TestClient) -> None: """The consent page bounces an unauthenticated visitor to login.""" r = app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) assert r.status_code == 302 - assert "/login" in r.headers["location"] + assert r.headers["location"].startswith("/login?"), r.headers["location"] def test_consent_forces_reauth_even_with_no_session_at_all(app: TestClient) -> None: @@ -604,7 +604,8 @@ def test_consent_forces_reauth_for_stale_session(app: TestClient) -> None: r = app.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) assert r.status_code == 302, r.text loc = r.headers["location"] - assert "/login" in loc and "reauth=1" in loc + assert loc.startswith("/login?"), loc + assert "reauth=1" in loc # Approve POST refuses the stale session too (defense in depth — a direct # POST must not bypass the GET's gate). @@ -735,7 +736,7 @@ def test_browser_consent_not_gated_by_client_secret(secret_app: TestClient) -> N r = secret_app.get("/oauth/device?user_code=ABCD-2345", follow_redirects=False) # Bounces to login (unauthenticated), NOT a 401 invalid_client. assert r.status_code == 302 - assert "/login" in r.headers["location"] + assert r.headers["location"].startswith("/login?"), r.headers["location"] def test_no_secret_configured_stays_public(app: TestClient) -> None: @@ -882,3 +883,28 @@ def test_unsupported_reason_refuses_oidc_with_no_config() -> None: provider = SimpleNamespace(_source="oidc", _oidc_config=None, _accounts_config=None) assert unsupported_reason(provider) is not None # type: ignore[arg-type] + + +def test_bounce_percent_encodes_the_return_to(app: TestClient) -> None: + """The consent URL must survive the bounce whatever the code contains. + + ``html.escape`` is an HTML escaper, not a URL one: it leaves ``?``, + ``=``, ``#`` and ``+`` untouched. A ``#`` therefore ended the URL and + turned everything after it into a fragment, silently truncating the + ``return_to`` the user is meant to come back to. Percent-encoding is the + correct tool for a query value. + + The ``&reauth=1`` that follows survived only because ``html.escape`` + happens to turn ``&`` into ``&`` — an accident of the escaper, not a + property the forced-re-auth invariant should rest on. + """ + from urllib.parse import parse_qs, urlparse + + r = app.get("/oauth/device?user_code=AB%23CD%26reauth=0", follow_redirects=False) + + assert r.status_code == 302 + query = parse_qs(urlparse(r.headers["location"]).query) + assert query["return_to"] == ["/oauth/device?user_code=AB#CD&reauth=0"], ( + "the consent URL must round-trip intact" + ) + assert query["reauth"] == ["1"], "and the forced re-auth must be the only one" diff --git a/tests/server/test_oidc_reauth_prompt.py b/tests/server/test_oidc_reauth_prompt.py index b63487048e..aee7e9d2d9 100644 --- a/tests/server/test_oidc_reauth_prompt.py +++ b/tests/server/test_oidc_reauth_prompt.py @@ -131,19 +131,20 @@ def test_re_authentication_still_round_trips_the_return_to(oidc_client: TestClie Losing it would land the user on the dashboard after re-entering their password, with the pending grant abandoned and no way back to it. """ - from urllib.parse import unquote - import jwt + # Percent-encoded exactly as `_bounce_to_login` emits it. Decoding the + # stored value before asserting would have passed under either encoding + # and hidden whether the query survived at all. oidc_client.get( - "/auth/login?reauth=1&return_to=/oauth/device%3Fuser_code%3DK7M2-QP9X", + "/auth/login?reauth=1&return_to=%2Foauth%2Fdevice%3Fuser_code%3DK7M2-QP9X", follow_redirects=False, ) cookie = oidc_client.cookies.get("ap_auth_state") assert cookie is not None claims = jwt.decode(cookie, _TEST_SECRET, algorithms=["HS256"]) - assert unquote(claims["return_to"]) == "/oauth/device?user_code=K7M2-QP9X" + assert claims["return_to"] == "/oauth/device?user_code=K7M2-QP9X" # ── The consent page against a REAL OIDCConfig ──────────────────── @@ -199,6 +200,9 @@ def test_consent_page_renders_for_a_real_oidc_session_cookie(tmp_path: Path) -> page = client.get(f"/oauth/device?user_code={user_code}", follow_redirects=False) assert bounced.status_code == 302, "an unproven session must not reach consent" + # `/auth/login`, not the accounts SPA's `/login` — the other half of + # `login_url`, which a `"/login" in location` substring cannot tell apart. + assert bounced.headers["location"].startswith("/auth/login?"), bounced.headers["location"] assert "reauth=1" in bounced.headers["location"] assert page.status_code == 200, f"consent bounced instead of rendering: {page.headers}"