feat(auth): mount the device grant under OIDC, and keep its re-auth gate honest

[appletechie]

The device-authorization grant (RFC 8628) refuses to mount unless auth mode is accounts. OIDC deployments are 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 — which is what the grant is for. 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 hand to an application is exactly what it lacks.

So an OIDC deployment has no way to authorize an application at all. And the 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 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 isn't a gate flip

Consent requires a login performed after the grant began (session iat ≥ grant created_at); a stale session bounces 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, instead of 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 flipping the mount gate and stopping there would have produced:

consent → stale iat → bounce /auth/login?reauth=1
        → IdP recognises its own session
        → silent redirect back
        → callback mints session with fresh iat
        → gate satisfied

The user proves nothing. Nothing errors, no test fails, and the control is gone while still appearing to be there. That is the failure mode worth flagging: it would have shipped green.

/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.

Two deliberate narrownesses:

• Only on that path. Unconditional prompt=login 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.
• Only exact "1". The consent page is the only caller and sends exactly that; accepting loose truthy spellings would just widen the surface for an unrelated query param to trigger a re-prompt.

Changes

File
app.py	mount gate accepts oidc
device_auth.py	factory accepts oidc, selects _oidc_config
routes/auth.py	reauth=1 → prompt=login
designs/DEVICE_AUTH.md	mode claims + the cross-mode re-auth note
test_device_auth.py	mode test split: header rejected, oidc builds
test_oidc_reauth_prompt.py	new — pins the parameter

Verified

• 220 auth-suite tests pass — device_auth, oidc, callback, invites, open-redirect, accounts, plus the new file
• Removing the prompt=login block fails the new suite — the tests aren't vacuous
• return_to still round-trips the bounce, so a re-auth doesn't abandon the pending grant
• ruff, ruff format, mypy clean on every touched file

The e2e browser proof (tests/e2e_ui/auth/test_device_grant_reauth.py) stays accounts-only — it drives a real password form, which OIDC doesn't have. The OIDC half is covered at the route level.

Why now

Unblocks polly-webhook connecting to a self-hosted Omnigent as a delegated client. It needs /v1/sessions, /v1/agents, /v1/hosts — already exactly the _DELEGATED_ALLOWED_PREFIXES allowlist — and the deployment it must reach first runs OIDC.

[appletechie]

All eight findings addressed across two commits (204f54f, ca94fad). Both reviewers landed on the same two P1s independently, and both were right — they're the same mistake this PR was written to fix, reached by paths I didn't check.

P1 — unforced first bounce

device_consent_page bounced an unauthenticated caller with reauth=False, forcing re-auth only when an existing cookie was stale.

Under accounts that's sound: no session means no credential, so the SPA shows the form regardless. Under OIDC it isn't — "no Omnigent session" says nothing about the IdP's, which is separate and may be live. Silent bounce, fresh iat, gate passes.

Every bounce is forced now. The parameter is removed rather than defaulted, since no caller wants an unforced bounce.

P1 — GitHub OAuth can't honour the gate

Confirmed: provider_type="github" points at https://github.com/login/oauth/authorize — plain OAuth 2.0, no prompt parameter, no id_token, no auth_time. The gate degrades to "redirect out and back" with no way to detect it.

Refused outright via a new unsupported_reason predicate, which also absorbs the header-mode check so there's one answer to "can this provider carry a grant" instead of two that can drift. app.py consults the same predicate and logs the refusal — an operator who set the flag learns why /oauth/* is missing rather than concluding it didn't take — and the server still boots.

Medium — requested but never verified

This one I'd have missed, and it's the sharpest of the eight. prompt=login is a request; nothing checked it was honoured, and auth_time appeared nowhere in the tree.

Now: max_age=0 rides alongside (obliging a conforming IdP to report auth_time), /auth/login signs the bounce time into the state cookie as reauth_at so it can't be stripped via the URL, and /auth/callback 403s without minting a session unless auth_time postdates it.

Missing auth_time is refused too — silence is indistinguishable from a reused session. Ordinary logins carry no reauth_at and are untouched, since most IdPs omit the claim and requiring it everywhere would break every sign-in.

Verification meant decoding the id_token twice, so the validated decode is now _verified_id_token_claims, shared with the email resolver — one validated path, so no caller can read a claim from an unverified token.

Medium — mount never exercised through create_app

Correct: the factory tests proved the router builds, never that the app calls it. create_app is now driven with a real OIDC provider (constructed in-process, so no discovery request), asserting the grant is reachable — and absent for GitHub.

The GitHub assertion is against the route table, not a status code: the SPA catch-all answers unmounted paths with 405, so "not 200" would also pass with the routes mounted and merely erroring. Found that while writing it.

Medium — SimpleNamespace proved nothing about the real cookie

Also correct, and the failure mode is nasty: if OIDCConfig.session_cookie_name/cookie_secret diverged from what the callback sets, _session_iat returns None forever and consent becomes an infinite bounce — no error, no failing test. Added a test that mints through the real mint_session_cookie from a real OIDCConfig and asserts the consent page renders, naming the identity and the client.

P2 — comment length

Ten inline lines against AGENTS.md's three. Condensed; the reasoning already lives in the docstring and DEVICE_AUTH.md.

Verified

• 280 tests pass — auth suite, integration, e2e
• Mutation-checked every fix: unforced bounce fails 1, allowing GitHub fails 2, dropping the callback verification fails 2, treating missing auth_time as a pass fails 1
• ruff, ruff format, mypy clean
• DEVICE_AUTH.md updated for all three: the always-forced bounce, the GitHub exclusion, and requested-then-verified

One thing I did not do: verify auth_time for the accounts path. It doesn't need it — the server checks the password itself, so the re-authentication is a local fact rather than a claim from a third party.

[polly-review]

🤖 Polly Review

Approach: Mostly sound

Both prior High findings are fixed. What remains is fail-open classification, an unverified re-auth, and test gaps.

Findings

• Medium — device_auth.py:290: unsupported_reason denies only provider_type == "github". from_env yields exactly github or oidc today, so nothing is broken now — but any future plain-OAuth provider, or a directly-constructed config, defaults to allowed and silently reopens the bypass this PR closes. Invert to an allowlist: permit "oidc", refuse anything else until audited.
• Medium — routes/auth.py:203 (prior d7286f4818d1, still open): prompt=login is requested, never verified. grep -rn 'auth_time' omnigent/ returns zero hits. Send max_age=0, record the re-auth request time in the signed state, and reject a missing or stale auth_time in the callback before minting the session.
• Medium — app.py:2422: the refusal warning sits inside the login_url truthiness gate at app.py:2344, and UnifiedAuthProvider.login_url is None for header mode (auth.py:443). So the header-mode refusal — one of the two cases unsupported_reason exists to explain — is never logged, and the custom-provider reason string at :2417 is likewise mostly unreachable. Hoist the warning above that gate. No caplog assertion anywhere covers it.
• Medium — tests/server/test_device_auth.py:458 (prior 1e6d9245dae6, still open): the app fixture is _build_accounts_app (:273) — the one mode where always-on reauth provably changes nothing. No test in tests/ exercises the consent gate under OIDC, and all three bounce assertions are "/login" in location, a substring /auth/login also satisfies, so login_url is untested in both directions.
• Medium — tests/server/test_device_auth.py:94 (prior 60f80474d531, still open): still a SimpleNamespace _oidc_config. Nothing proves the real OIDCConfig cookie name/secret match what /auth/callback writes; a mismatch is an infinite consent bounce with a green suite.
• Low — device_auth.py:449: return_to is html.escaped, not URL-encoded, and &reauth=1 is appended after it. A crafted user_code=X%26reauth=0 yields &reauth=0&reauth=1, and URLSearchParams.get returns the first. Not exploitable (that path has no session to auto-bounce), but the new always-on invariant rests on param ordering. Use quote(return_to, safe="/").
• Low — device_auth.py:288: _source == "oidc" with _oidc_config is None classifies as supported, then trips an assert. Classify it as unsupported and raise deliberately.

Checked

GitHub refused on both the factory and create_app paths; every _bounce_to_login caller now forced; _logger binding and the custom-provider type guard; accounts SPA + e2e behaviour under always-on reauth (inert, cannot loop); ordinary OIDC login unchanged unless reauth is exactly 1; no existing test breaks or becomes vacuous.

✅ Previously reported, now fixed: 2
⚠️ Previously reported, still open: 3

5 finding(s) without a valid diff anchor

• Medium · Provider classification fails open — omnigent/server/routes/device_auth.py:290 (diff unavailable for validation)
  This denies one known-bad value and admits every other string. OIDCConfig.from_env produces exactly github or oidc today, so the gate holds now — but the next plain-OAuth provider added to from_env, or any directly-constructed config, is admitted by default and silently reopens the bypass this PR was written to close. The safe shape for a security predicate is an allowlist: return None only for provider_type == "oidc" and refuse anything else with a reason naming the unaudited value.
• Medium · prompt=login is requested but never verified — omnigent/server/routes/auth.py:203 (diff unavailable for validation)
  Carried over from the previous review and unchanged at this head: grep -rn 'auth_time' omnigent/ returns zero hits, and no max_age is sent on the authorization request. A misconfigured or non-conforming IdP satisfies the bounce from its own session, and the callback mints a fresh iat that clears the gate with no error anywhere. Send max_age=0 alongside prompt=login (OIDC Core then requires the IdP to return auth_time), persist the re-auth request time in the signed state cookie, and reject a missing or older auth_time in the callback before minting the session.
• Medium · Refusal warning never fires for header mode — omnigent/server/app.py:2422 (diff unavailable for validation)
  This whole block is nested under the auth_provider is not None and getattr(auth_provider, "login_url", None) gate at app.py:2344, and UnifiedAuthProvider.login_url returns None for header mode (auth.py:443). Header mode is one of the two cases unsupported_reason exists to explain, so an operator who sets OMNIGENT_DEVICE_GRANT_ENABLED there gets exactly the silent absence of /oauth/* this warning was added to eliminate — as does a custom provider, whose dedicated reason string just above is likewise unreachable. Hoist the wanted/blocked computation and the warning above the login_url gate, and add a caplog test (there is none).
• Medium · The forced bounce is only tested where it does nothing — tests/server/test_device_auth.py:458 (diff unavailable for validation)
  The app fixture is _build_accounts_app (:273), and under accounts reauth=1 on a no-session bounce is inert — LoginPage.tsx:90 only suppresses a getMe() probe that would not have redirected anyway. So this test cannot fail if _bounce_to_login regressed to reauth=False; the OIDC case that motivates the change is untested. Also, this and the two sibling assertions check "/login" in location, which /auth/login also satisfies, leaving auth_provider.login_url unverified in both directions. Add an OIDC-mode app fixture and assert the location starts with /auth/login and carries reauth=1, for both the no-session and stale-session bounces.
• Medium · SimpleNamespace config still proves nothing about the real cookie — tests/server/test_device_auth.py:94 (diff unavailable for validation)
  Carried over from the previous review. This builds the router from a hand-written _oidc_config, so the one thing that can actually break — whether the real OIDCConfig.session_cookie_name and cookie_secret match what /auth/callback writes — is asserted nowhere. tests/server/test_oidc_reauth_prompt.py has a real OIDCConfig but never mints a session cookie or touches the device router. If they diverge, _session_iat returns None on every request and the OIDC consent page becomes an infinite bounce with a green suite. Mint via mint_session_cookie from a real OIDCConfig, set it under cfg.session_cookie_name, and assert the consent page renders.

---

Reviewed at 204f54f.

[polly-review]

🤖 Polly Review

Approach: Needs changes

Three prior findings remain open, and the new verification machinery is bypassable.

Findings

• High — routes/auth.py:332: freshness is enforced on the login request, not the consent decision, and the marker is opt-in on a public GET. Attacker starts a grant, then sends the victim /auth/login?return_to=%2Foauth%2Fdevice%3Fuser_code%3DXXXX with no reauth=1 — no prompt, no max_age, no reauth_at, so the callback skips the check, mints a session with iat = now, and the consent gate (iat >= created_at) passes. The victim approves having re-authenticated nothing. _sanitize_return_to permits that path (auth.py:681), and mint_session_token emits only sub/iat/exp/provider (oidc.py:53-102), so nothing downstream can distinguish a forced login from a silent SSO. Fix: carry the verified auth_time into the session (and set an equivalent only after accounts password verification), and have both consent gates compare that against grant.created_at, failing closed when absent — or issue a short-lived server-signed marker bound to grant.id and require it on the consent GET and POST.
• Medium — device_auth.py:290 (4c77469dc408, unchanged): still a denylist of "github" only; _oidc_config is None also classifies as supported.
• Medium — app.py:2422 (ac6cbdc72665, unchanged): still nested under the login_url truthiness gate at app.py:2337, so header mode never logs the refusal.
• Medium — routes/auth.py:212: max_age=0 and reauth_at are the two lines the whole gate rests on, and neither is asserted anywhere. They appear in tests/ only in the callback tests that inject reauth_at by hand. Delete either line and the suite stays green.
• Medium — routes/auth.py:50: auth_time >= not_before - 60 contradicts the doc's "postdates it" — a session authenticated 59s before the bounce passes. Compare auth_time against the id_token's own iat (same clock) with a small processing allowance.
• Medium — tests/server/test_device_auth.py:565 (13c9947e2476, unchanged): still no OIDC consent-bounce test; all three assertions are "/login" in location, which /auth/login also satisfies, so login_url stays untested in both directions.
• Low — two JWKS fetches per re-auth callback (auth.py:866 and :920 each build a fresh PyJWKClient); isinstance(auth_time, int) 403s a spec-conforming float; the 403 is raw JSON with no retry path after the code is already exchanged; _build_oidc_app never clears OMNIGENT_DEVICE_CLIENT_SECRET, so an ambient value makes the mount test 401 instead of 200; the time.sleep(1) at test_oidc_reauth_prompt.py:178 is not load-bearing.

Checked

The _resolve_oidc_email refactor is behaviour-identical line for line; the reauth_at check sits after the github/oidc email branch so both converge through it before the sole session mint; marked flows fail closed on missing or stale auth_time; both create_app tests will really exercise the mount decision; the real-OIDCConfig consent test genuinely exercises session_cookie_name and cookie_secret against _session_iat.

✅ Previously reported, now fixed: 5
⚠️ Previously reported, still open: 3

---

Reviewed at ca94fad.

[polly-review]

High · Re-auth gate is bypassed by an unmarked login link

Only runs when the state carries reauth_at, which only /auth/login?reauth=1 writes — and /auth/login is a public GET accepting any same-origin return_to. An attacker starts a grant, then sends the victim /auth/login?return_to=%2Foauth%2Fdevice%3Fuser_code%3DXXXX with no reauth=1: no reauth_at is signed, so this is skipped, the callback mints a session with iat = now, and the consent gate at device_auth.py:517 passes on iat alone. Fix: persist the verified auth_time into the session (mint_session_token carries only sub/iat/exp/provider) and gate consent on that, or bind a signed re-auth marker to grant.id.

[polly-review]

Medium · Provider classification still fails open

Unchanged since the last review. This denies one known-bad value and admits every other string, and _oidc_config is None also returns supported. from_env yields only github or oidc today, so nothing is broken now — but the next plain-OAuth provider, or a directly-constructed config, is admitted by default. Require a present config and allow only provider_type == "oidc", refusing anything else with a reason naming the unaudited value.

[polly-review]

Medium · Refusal warning still never fires for header mode

Unchanged since the last review. This block is nested under the login_url truthiness gate at app.py:2337, and UnifiedAuthProvider.login_url returns None for header mode (auth.py:443). Header mode is one of the two cases unsupported_reason exists to explain, so an operator who sets OMNIGENT_DEVICE_GRANT_ENABLED there still gets only the silent absence of /oauth/*. Hoist the wanted/blocked computation and the warning above that gate, and add a caplog assertion.

[polly-review]

Medium · max_age=0 and reauth_at are untested

These two lines are what makes the gate enforceable, and neither is asserted anywhere. test_reauth_forwards_prompt_login_to_the_idp checks prompt and PKCE but not max_age; the callback tests inject reauth_at by hand rather than letting /auth/login write it. The chain is tested at each end and joined nowhere — delete either line and the suite stays green while the gate switches off. Assert params["max_age"] == ["0"] in the authorize-params tests, and decode ap_auth_state to assert reauth_at is present with reauth=1 and absent without it.

[polly-review]

Medium · 60s skew window admits a pre-bounce authentication

_reauthenticated_after compares auth_time >= not_before - 60, where not_before is our own clock at the bounce. An authentication performed up to 59 seconds before the consent bounce therefore satisfies a gate the design doc describes as requiring the re-auth to postdate it, so a non-conforming IdP reusing a very recent session passes. Since auth_time and the id_token's iat both come from the IdP's clock, compare those two directly with a small processing allowance, and keep the server-clock value only for binding the marker to this request.

[appletechie]

Second round addressed — b96ae1a. All five hold up; the High is the sharpest finding on this PR so far.

High — the gate could be skipped rather than beaten

Correct, and it makes the previous two rounds partly beside the point. Hardening the forced path doesn't matter if the forced path can simply not be entered.

/auth/login is a public GET accepting any same-origin return_to. An attacker who starts a grant sends the victim:

/auth/login?return_to=%2Foauth%2Fdevice%3Fuser_code%3DXXXX

No reauth=1 → no reauth_at → nothing demanded → IdP satisfies it from its own session → cookie with iat of now → consent renders. One click.

The root cause is that iat cannot carry the property: every completed callback has a fresh one, including the ones with zero user involvement. So the proof now lives 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, bounces, and is made to prove itself. The demand no longer depends on the attacker's link having asked for it.

Both the consent GET and the approve POST read the new claim, so the render check can't be sidestepped by posting directly.

Took your first suggested fix (persist into the session) over binding to grant.id — it generalises to any future consumer needing "did this human actually authenticate", rather than solving it once for device grants.

One consequence worth stating: an IdP that never emits auth_time can no longer carry a grant — one bounce, then a 403 with a clear error. That terminates rather than looping, and it's the right answer for a provider that can't support the control.

Medium — 60s skew admitted a pre-bounce authentication

Right, and the diagnosis was exact: I was comparing the IdP's clock against ours. Now compares auth_time against the id_token's own iat — both the IdP's — so skew between the two servers cancels and a reused session fails regardless of whose clock is ahead. The remaining allowance covers the IdP's processing between authenticating and signing, which is what it should have been measuring.

Medium — provider classification failed open

Fixed as suggested: allowlist provider_type == "oidc", require a present config, and name the refused value in the reason. Agreed on the reasoning — from_env yields only two values today, so this is about the next one, not a live bug.

Medium — refusal warning never fired for header mode

Verified: the block sat under the login_url truthiness gate at app.py:2344, and header mode's login_url is None. So the operator least able to work out why /oauth/* was missing was the only one who got no explanation. Hoisted above that gate, with caplog-style assertions for header mode, GitHub, and a negative case asserting a supported deployment logs nothing.

Medium — max_age=0 and reauth_at untested

Also correct — tested at each end, joined nowhere. Both pinned now, and the state cookie is decoded to assert the marker is present with reauth=1 and absent without it.

Verified

• 290 tests pass — auth suite, integration, e2e
• Mutation-checked each fix: 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
• DEVICE_AUTH.md rewritten for the session-carried proof, the same-clock comparison, and the allowlist

Note for anyone reading the diff: mint_session_token/mint_session_cookie gained an optional auth_time, and the four accounts login paths pass it since the server verifies the credential itself. mint_runner_token deliberately does not — a runner token should never satisfy a consent gate.

[polly-review]

🤖 Polly Review

Approach: Mostly sound

All eight prior findings are fixed. Moving the proof onto the session as auth_time is the right design — but the stamp is unconditional, which lets the bypass back in through a different door.

Findings

• High — routes/auth.py:341: _idp_reauthenticated runs on every login, so proof is stamped whenever the id_token reports auth_time >= iat - 120, including on a login the attacker's unmarked link triggered. Two ways through: an IdP that sets auth_time to token-issuance time on silent SSO (a widespread misimplementation) always passes; and even a conforming IdP passes inside the 120s window after a genuine sign-in. Fix: stamp proven_at only when the signed state carries reauth_at.
• Medium — accounts_auth.py:645: magic_redeem stamps auth_time on the strength of a token /auth/magic minted for whatever session asked, so a session whose proof predates the grant can self-upgrade to "freshly proven" with no password entry. Carried forward rather than introduced here, and not cross-origin drivable — but it contradicts the invariant oidc.py:78-81 and the design doc both rest on.
• Medium — device_auth.py:536: the gate proves an authentication after the grant began, not one for this grant. Attacker authorizes at T0, victim signs in at T1 for unrelated reasons, click at T2 renders consent with no friction. Not a regression, but the docstring's "always costs a deliberate, fresh credential entry" overstates it.
• Medium — routes/auth.py:341: _resolve_oidc_email and _idp_reauthenticated each build a fresh PyJWKClient, so its instance cache can't be reused and every standard OIDC callback now makes two sequential JWKS fetches — doubled latency and rate-limit exposure, synchronously, inside an async handler. Verify once, pass the claims to both.
• Low — routes/auth.py:889: isinstance(auth_time, int) rejects a fractional NumericDate, which RFC 7519 permits, and accepts True. Same at :897.
• Low — routes/auth.py:346: the 403 fires after the code is exchanged, so it can't be retried, and returns raw JSON with no route back to the pending grant.
• Low — doc/comment drift: the retired iat gate and _session_iat (which no longer exists) survive in device_auth.py:280,502, three spots in test_device_auth.py, two in test_oidc_reauth_prompt.py, and six in DEVICE_AUTH.md — where :194-201 now contradicts :217-233.
• Low — test hygiene: neither _build_oidc_app nor test_consent_page_renders_for_a_real_oidc_session_cookie clears OMNIGENT_DEVICE_CLIENT_SECRET, so an ambient value turns a mount test into a 401 that reads like a mount failure; three bounce assertions still use "/login" in location.
• Note (pre-existing, untouched by this diff) — anyone can create a CLI ticket, get a victim to open /auth/login?ticket=…, and poll the victim's session JWT out unauthenticated. This PR doesn't cause it, but auth_time now travels on that JWT, so the exfiltrated session also carries device-approval power. Worth its own issue.

Checked

import time is present in accounts_auth.py (four new callers, no NameError); every mint_session_* call site audited — no refresh/sliding-renewal mint exists, and the bootstrap and runner tokens correctly omit the claim; pre-existing sessions fail closed and self-heal in one login; an IdP that never emits auth_time terminates in one bounce plus one 403 on both the OIDC and accounts SPA paths; nothing else decodes the session claim set; the new tests are collectable, tear down cleanly, and fail on inversion.

✅ Previously reported, now fixed: 8

---

Reviewed at b96ae1a.

[polly-review]

High · Proof is stamped on every login, not only forced ones

_idp_reauthenticated runs unconditionally, so any login whose id_token reports auth_time >= iat - 120 gets proof stamped — including one the attacker's unmarked /auth/login?return_to=/oauth/device?user_code=… link triggered. Two ways through: an IdP that sets auth_time to token-issuance time on silent SSO (a common misimplementation) always passes; and even a conforming IdP passes if the victim authenticated within the last 120s. Either reopens the bypass this redesign closed. Fix: stamp proven_at only when the signed state carries reauth_at. Ordinary logins then get an unproven session and are bounced at consent — which the page already handles.

[polly-review]

Medium · magic_redeem lets an unproven session mint its own proof

The comment says the server verified a credential just now, but /auth/magic mints this token for whatever session called it, carrying no evidence that session ever presented one. So a session whose auth_time predates the grant can call /auth/magic then /auth/magic/redeem and come back stamped fresh, clearing the approve gate at device_auth.py:574 with no password entry. Not cross-origin drivable (no CORS middleware, the redeem_url sits in a JSON body), and carried forward rather than introduced here — but it violates the invariant oidc.py:78-81 and DEVICE_AUTH.md both state. Fix: propagate the caller's auth_time through the magic token instead of restamping.

[polly-review]

Medium · Proof is temporal, not bound to this grant

proven_at >= grant.created_at accepts any credential entry inside the grant's pending window, whatever it was for. Attacker authorizes at T0 and mails the link; the victim signs in at T1 for an unrelated reason; clicking at T2 renders consent and one click binds the grant — no bounce, no grant-specific friction. Not a regression (the old iat check behaved the same), but the docstring at :505 and DEVICE_AUTH.md claim approval "always costs a deliberate, fresh credential entry", and it costs a credential entry. Fix: carry the grant id on the proof and compare it on both the consent GET and the approve POST, or soften both claims.

[polly-review]

Low · auth_time type check accepts bool, rejects float

isinstance(auth_time, int) is True for True, so an IdP sending "auth_time": true reads as 1 — harmless here, but it passes the type gate for the wrong reason. The real problem is the other direction: a fractional NumericDate is valid per RFC 7519 and is rejected outright, so a conforming IdP emitting auth_time: 1767225600.5 can never carry a device grant and every forced login 403s. Same for the issued_at check at :897. Fix: accept finite int | float and exclude bool explicitly.

[polly-review]

Low · Docstring still describes the retired iat gate

This paragraph describes the gate as session iat >= the grant's created_at, but the code below reads auth_time (:535). The same stale iat / _session_iat wording survives at :280, in test_device_auth.py:72,577,589, test_oidc_reauth_prompt.py:16,155, and across DEVICE_AUTH.md:147,195,200,349,372,377 — _session_iat no longer exists under that name. The module docstring at :31 is inverted too: it says dropping prompt=login makes the gate "pass without the user having proven anything", when at this head it makes the gate bounce and then 403. Worth fixing together, since the doc is the design record.

[appletechie]

Went back through the review bodies — I had only been reading the inline comments, and four items never got a diff anchor, so they were never addressed. Fixed in 6e19b47. None was exploitable; all four are places where something read as stronger than it was.

Low — the bounce built a URL with an HTML escaper

_bounce_to_login ran the consent URL through html.escape before appending &reauth=1.

The described attack doesn't reproduce. html.escape turns & into &, so a crafted user_code=X%26reauth=0 arrives as amp;reauth=0 and reauth=1 remains the only reauth:

today : return_to=/oauth/device?user_code=K7M2-QP9X&reauth=0&reauth=1
parsed: {'return_to': [...], 'amp;reauth': ['0'], 'reauth': ['1']}

But the conclusion is right, for a better reason. html.escape is an HTML escaper, not a URL one — it leaves ?, =, # and + untouched. A # in the code ends the URL and turns the rest into a fragment, silently truncating the return_to the user is meant to return to. And the forced-re-auth invariant shouldn't rest on an escaper's incidental handling of one character. quote(..., safe="/") makes the whole question moot.

Not reachable in practice either way — the user_code alphabet contains none of these characters.

Low — "/login" in location could not fail

Correct. /auth/login also contains /login, so all three bounce assertions 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?.

Low — an unquote hid what the state cookie stores

Also correct, and the sharper version of the point: the test decoded the stored value before comparing, so it passed whether or not the query survived the bounce — the one property it exists to check. Asserts the stored form directly now.

The encoding had no test at all

Worth stating plainly: I confirmed by mutation that reverting to html.escape left the entire suite green. Added a test driving a user_code containing exactly the characters html.escape ignores, asserting the consent URL round-trips intact and that reauth=1 is the only reauth. Reverting now fails it.

Doc note

Fair, and worth being explicit about: 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. Added that, plus what these routes are actually for: external clients needing a scoped, revocable, refreshable credential rather than the server's own session JWT. Migrating the first-party clients is separate work.

Verified

• 291 tests pass — auth suite, integration, e2e
• Encoding fix mutation-checked (reverting fails 1)
• ruff, ruff format, mypy clean

Also closed from the same body, in the previous commit: _source == "oidc" with a None config used to classify as supported and then trip an assert — it is now refused deliberately with a reason.

[polly-review]

🤖 Polly Review

Approach: Mostly sound

This push is one commit over four files. The only runtime change is _bounce_to_login switching to quote(); the rest is docs and test assertions. All five listed findings are untouched.

Findings

• High, still open — routes/auth.py:341: _idp_reauthenticated still runs on every callback and still writes proven_at into the session when the state carries no reauth_at, so an unmarked login can mint device-approval proof.
• Medium, still open — accounts_auth.py:645: /auth/magic lets any valid session mint a token, and redemption stamps a brand-new auth_time without carrying or checking the originating session's proof — a stale session refreshes its own proof with no password.
• Medium, still open — device_auth.py:541 (and the identical predicate at :581): re-traced at this head — grant at T0, victim's unrelated sign-in at T1, click at T2 renders consent and one click binds it. _session_auth_time reads only auth_time; nothing reads grant.id or the user_code.
• Medium — routes/auth.py:843: _resolve_oidc_email and _idp_reauthenticated each build a fresh PyJWKClient, so every ordinary OIDC callback now makes two sequential synchronous JWKS fetches. Verify once, pass the claims to both.
• Low — device_auth.py:465: the new encoding is single-layer. user_code is interpolated raw into return_to before the outer quote(), so a # survives decoding and Starlette keeps it in the callback's Location; the consent server then receives a truncated user_code. No open redirect and forced re-auth still holds, but the round trip the fix claims doesn't complete. The new test only parses the first bounce and misses it.
• Low, still open — routes/auth.py:889: still isinstance(auth_time, int) for both claims — admits bool, rejects the fractional NumericDate RFC 7519 permits.
• Low, still open — doc drift, now self-contradictory: DEVICE_AUTH.md:225 carries the correct "proof rides on the session, not on iat" text while :155,203,208,357,380 still describe the retired gate, and :385 plus test_oidc_reauth_prompt.py:156 name _session_iat, which no longer exists (it is _session_auth_time). Same in device_auth.py:31,281,508 and four spots in test_device_auth.py. Two of these claim the gate "silently passes" without prompt=login; at this head it 403s.
• Low — neither _build_oidc_app, _build_accounts_app, nor test_consent_page_renders_for_a_real_oidc_session_cookie clears OMNIGENT_DEVICE_CLIENT_SECRET; an ambient value 401s the unheadered authorize call and reads like a mount failure.

Checked

test_bounce_percent_encodes_the_return_to genuinely fails against the old html.escape — # is the discriminator, since urlparse split it into a fragment and truncated return_to; %26 alone would not have been. The three tightened startswith("/login?") assertions match accounts-mode login_url, and the new startswith("/auth/login?") is the case the old substring could never fail. Dropping unquote() is genuinely stricter — the stored state value is decoded at that point. safe='/' is fine for a query value. html is still used at device_auth.py:814, so the import is not dead. The DEVICE_AUTH.md:41-48 note about first-party clients is accurate. All new tests are collectable and tear down cleanly.

⚠️ Previously reported, still open: 5

---

Reviewed at 6e19b47.

[polly-review]

High · Proof is still stamped on every login, not only forced ones

Unchanged since the last review. _idp_reauthenticated runs on every callback and proven_at reaches mint_session_cookie at :405-410 even when the signed state carries no reauth_at, so a login the attacker's unmarked /auth/login?return_to=/oauth/device?user_code=… link triggered can still mint device-approval proof — either against an IdP that reports auth_time as token-issuance time, or against a conforming one within the 120s allowance. Fix: validate and stamp only when state_payload contains reauth_at. Ordinary logins then carry no proof and are bounced at consent, which the page already handles.

[polly-review]

Medium · magic_redeem still refreshes proof with no credential

Unchanged. /auth/magic (:578-590) mints a token for any valid session, and redemption stamps a brand-new auth_time here without carrying or checking whether the originating session ever presented a credential. A session whose proof predates the grant can therefore call /auth/magic then /auth/magic/redeem and come back stamped fresh, clearing the approve gate at device_auth.py:581 with no password entry. Fix: omit auth_time on redemption, or propagate the originating session's existing timestamp without advancing it.

[polly-review]

Medium · Consent proof is still a timestamp, not bound to this grant

Unchanged, and the identical predicate sits at :581. _session_auth_time (:487-495) reads only auth_time; nothing reads grant.id, the user_code, or a per-grant nonce. Re-traced at this head: attacker authorizes at T0, victim signs in for an unrelated reason at T1 (accounts stamps on every password submit), clicks the link at T2 — consent renders and one click binds the grant. DEVICE_AUTH.md:357 still claims the victim "must deliberately re-enter their password". Fix: sign grant_id into the state alongside reauth_at, mint it onto the session, and match it here.

[polly-review]

Low · Percent-encoding fix is single-layer

The outer quote() correctly stops user_code from injecting a second parameter onto /auth/login, but the inner value is still interpolated raw into return_to at :459. After /auth/login decodes it, _sanitize_return_to and the signed state both preserve delimiters, and Starlette keeps # in the callback's Location — so AB#CD comes back to the consent page as user_code=AB. Not a security hole (no open redirect, forced re-auth still holds), but the round trip this change is meant to guarantee doesn't complete. The new test parses only the first bounce. Fix: urlencode both layers.

[polly-review]

Medium · Two synchronous JWKS fetches per OIDC callback

_resolve_oidc_email (:334) and _idp_reauthenticated (:341) each call _verified_id_token_claims, and each call constructs a fresh PyJWKClient here — whose cache is per-instance, so it can never be reused. Every ordinary OIDC callback therefore makes two sequential blocking JWKS requests inside an async handler, doubling login latency and the exposure to a JWKS outage or rate limit. Fix: verify the id_token once per callback and pass the claims to both consumers, ideally holding one router-scoped PyJWKClient.

[polly-review]

🤖 Polly Review

Review could not be completed: Diff fetch failed: 406 {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","f

[appletechie]

Superseded by omnigent-ai#4304 — same branch, same commits, opened against upstream where it belongs.

This was targeting the fork's main, which now tracks upstream/main. Merging 1125 lines into it would have re-diverged the fork — the exact state that made #14 explode into a 39-file conflict — and the fix would have reached no Omnigent user.

Before opening upstream I rebased onto current main (the branch was 103 behind) and signed off all 5 commits (none had a Signed-off-by, so DCO would have failed). Re-verified after the rebase: 73 passed across the three auth suites, 321 passed across tests/server -k "auth or oidc or device".

Closing here. Follow it at omnigent-ai#4304.

[polly-review]

🤖 Polly Review

Review could not be completed: Diff fetch failed: 406 {"message":"Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead.","errors":[{"resource":"PullRequest","f

[appletechie]

Superseded by omnigent-ai#4304, which carries the same five commits (head db6f6894). This fork only stages branches for upstream PRs.

Branch feat/device-grant-oidc is intentionally left in place — omnigent-ai#4304 points at it.