feat(auth): mount the device grant under OIDC, and keep its re-auth gate honest - #4304
Open
appletechie wants to merge 5 commits into
Open
feat(auth): mount the device grant under OIDC, and keep its re-auth gate honest#4304appletechie wants to merge 5 commits into
appletechie wants to merge 5 commits into
Conversation
…ate honest 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 <andrew@peltekci.com>
…Auth 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 <andrew@peltekci.com>
… for it 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 <andrew@peltekci.com>
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 <andrew@peltekci.com>
…gaps 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 <andrew@peltekci.com>
Contributor
|
@appletechie Thanks for the PR! It doesn't reference an issue yet. We require an issue for every PR, so the work can be prioritized before it's reviewed. Add one to the description:
No issue exists for this yet? Open one first, then reference it. That's how we track what's worth doing, and it's usually quicker than it sounds. Note a reference has to point at an issue: naming another PR doesn't count. The only exceptions are changes with no user-visible behaviour: pure Refactor / chore, Docs, or Test / CI work. If that's genuinely what this is, check that box under Type of change. Anything that fixes a bug, adds a feature, or changes the UI needs an issue, even when it also touches docs or tests. See CONTRIBUTING.md for the full policy. No action is taken beyond this comment. |
Contributor
Author
|
CI is red on The finding The secret scan itself passed ("no secrets in added lines"). What fired is the co-occurrence heuristic in
Both "sinks" are type annotations, not calls. The "secret" is the test HS256 key a helper uses to decode a JWT inside an assertion. Nothing is sent anywhere; there is no network call in the file at all. Why I did not just rename it
The load-bearing point: The ask Per the scan's own message, Happy to do it differently if you would rather harden the heuristic instead — e.g. requiring the network match to look like a call ( |
13 tasks
Contributor
Author
|
Opened the heuristic fix as its own PR: #4316. It narrows the exfil scan's Verified against this PR specifically: with #4316 applied, the scanner exits 0 on this diff, where So there are two ways to unblock this PR, whichever you prefer:
No action needed from me on either path; this PR is otherwise green and |
Contributor
|
@appletechie This PR is a Bug fix, Feature, or UI / frontend change but the Demo section is missing or only contains a placeholder. These change types require a screenshot or screen recording so reviewers can see the new behaviour without checking out the branch. Please update the Demo section with:
Use |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related issue
N/A — found while connecting a delegated third-party client to a self-hosted
OIDC deployment.
Summary
The device-authorization grant (RFC 8628) refuses to mount unless auth mode is
accounts. OIDC deployments are excluded on the reasoning that they delegatelogin 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
scopeclaim, nogrant_id, no revocation handle, noactprovenance. 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.
The grant needs nothing from
accountsthatoidcdoes not also provide —both mint the same HS256 session cookie, and
_check_cookiealready picksbetween the two configs the same way this now does.
headermode stays excluded for a real reason rather than symmetry: identitythere 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 withreauth=1.That gate is the anti-phishing control — a victim handed a one-click link must
re-authenticate before they can approve anything.
Under OIDC that bounce was silently a no-op:
The user proves nothing. Nothing errors, no test fails, and the control is gone
while still appearing to be there. It would have shipped green — which is the
part worth flagging.
/auth/loginnow forwardsreauth=1to the IdP asprompt=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:
prompt=loginwould cost a password onevery ordinary sign-in, which is how a control like this ends up switched off
by whoever finds it irritating.
"1". The consent page is the only caller and sends exactlythat; accepting loose truthy spellings would widen the surface for an
unrelated query param to trigger a re-prompt.
Changes
server/app.pyoidc; warns when the flag is set but the mode can't support itserver/routes/device_auth.pyoidc, selects_oidc_configserver/routes/auth.pyreauth=1→prompt=loginserver/oidc.py,routes/accounts_auth.pydesigns/DEVICE_AUTH.mdtests/server/test_device_auth.pyheaderrejected,oidcbuildstests/server/test_oidc_reauth_prompt.pytests/server/test_oidc_callback.pyiatpathTest Plan
pytest tests/server/test_device_auth.py tests/server/test_oidc_callback.py tests/server/test_oidc_reauth_prompt.py— 73 passed.
pytest tests/server -k "auth or oidc or device"— 321 passed, coveringdevice_auth, oidc, callback, invites, open-redirect and accounts alongside the
new file.
prompt=loginblock fails the newsuite.
return_tostill round-trips the bounce, so a re-auth does not abandon thepending grant.
pre-commit run --filesover every touched file.Demo
N/A — no visual surface. The observable change is that an OIDC deployment can
authorize a third-party application at all, and that the consent bounce actually
forces a re-authentication instead of silently round-tripping.
Type of change
Test coverage
Coverage notes
The existing e2e browser proof (
tests/e2e_ui/auth/test_device_grant_reauth.py)stays accounts-only: it drives a real password form, which OIDC does not have.
The OIDC half is covered at the route level instead — the new suite asserts the
exact query parameter sent to the IdP, which is the only thing the server
controls on that path. Flagging that asymmetry explicitly rather than leaving it
to be discovered.
Changelog
OIDC deployments can authorize third-party applications through the device grant, and the consent re-authentication is actually enforced.