Skip to content

Add device flow verification page and DCR support - #6682

Merged
reyortiz3 merged 15 commits into
mainfrom
device-flow-verification-page
Sep 18, 2026
Merged

reyortiz3 merged 15 commits into
mainfrom
device-flow-verification-page

Conversation

@reyortiz3

@reyortiz3 reyortiz3 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #6647 (RFC 8628 Device Authorization Grant), which added storage,
the /oauth/device_authorization endpoint, and the token-endpoint grant
handler but explicitly left two gaps open, both called out in that PR's
description as required before DeviceFlowEnabled is safe to turn on for
real traffic:

  1. No verification page. POST /oauth/device_authorization mints a
    device_code/user_code pair whose verification_uri points at
    GET /oauth/device — but that route didn't exist.
    storage.MarkDeviceRequestAuthorized/MarkDeviceRequestDenied were only
    ever called by the integration test, directly against storage.
  2. No way to register a device-flow client. DCR's grant-type allowlist
    only permitted authorization_code/refresh_token.

This PR closes both gaps, so the device flow is now usable end to end by a
real client and a real human.

What changed:

  • StoragePendingDeviceLoginStorage / PendingDeviceConfirmationStorage
    (memory + Redis), modeled on the existing PendingAuthorizationStorage.
    The confirmation record is addressed only by an opaque, unguessable token
    handed to the browser as a hidden form field — the resolved identity is
    never round-tripped through the browser as editable form data.
  • DCR — a device_code-only carve-out (parallel to the existing
    private_key_jwt one): redirect_uris/response_types are not required
    for a registration whose grant types are exactly {device_code} or
    {device_code, refresh_token}.
  • Verification pageGET/POST /oauth/device (enter/confirm the
    user_code, redirect to the first configured upstream IDP) and
    POST /oauth/device/confirm (explicit Approve/Deny), rendered via
    html/template — the first HTML surface in pkg/authserver (every other
    handler writes JSON). The three pages (verify, confirm, result) use a
    ToolHive/Stacklok-branded palette with light/dark support via
    prefers-color-scheme, no external font/CDN dependency (air-gapped/
    enterprise deployments).
  • /oauth/callback is now shared between the OAuth-client
    authorization_code flow and the device flow's login. An upstream IDP's
    redirect_uri is fixed per upstream at construction
    (upstream.OAuth2Config.RedirectURI / AuthCodeURL), so there is no way to
    register a second callback path for the verification page's login — see
    "Special notes" below.
  • Hardening from review: the deny action now surfaces a storage failure
    instead of always reporting success; the shared /oauth/callback dispatch
    and the verification page's user_code re-checks distinguish a genuine
    storage error (500) from an expired/not-found request (400) instead of
    collapsing both into the same client error; POST /oauth/device is now
    rate-limited like /oauth/device_authorization; the rendered pages send
    anti-framing/no-sniff headers.
  • Integration testTestIntegration_DeviceFlow_FullHappyPath now
    drives the real HTTP flow through a mock upstream IDP (mockoidc) instead of
    simulating the verification page by calling storage directly.

Related: stacklok/stacklok-enterprise-platform#4127 (tracked in #6647).

Type of change

  • New feature

Test plan

  • Unit tests (task test)

  • Linting (task lint-fix)

  • pkg/authserver/storage: store/load/delete/expiry for both new storage
    types, in both memory_test.go and redis_test.go.

  • pkg/authserver/server/registration: device_code-only registration
    succeeds without redirect_uris/response_types; device_code +
    refresh_token succeeds; device_code with response_types set is rejected;
    device_code mixed with authorization_code still requires redirect_uris
    (the carve-out only applies to an exact device-code-only grant set).
    Updated two pre-existing CIMD tests whose fixtures relied on device_code
    being an unsupported (filtered-out) grant type — it's now supported.

  • pkg/authserver/server/handlers: full happy path (submit code → redirect →
    callback → confirm page → approve), deny path (including a storage failure
    on deny, and deny-then-replay of the confirm token), invalid/expired
    user_code, unknown callback state, unknown/reused confirm token
    (single-use, including a concurrent-replay race test).

  • pkg/authserver/integration_test.go:
    TestIntegration_DeviceFlow_FullHappyPath rewritten to drive the real
    HTTP flow end to end through a mock OIDC upstream (submit → upstream login
    → shared callback → confirm → poll /oauth/token → success → replay
    fails).

  • Manual verification: ran the embedded auth server locally (mock upstream,
    DCR-registered device-code client) and drove the full flow through a real
    browser against the new verification/confirm/result pages.

  • Verified independently: full-repo go build ./... clean; go test ./pkg/authserver/... green across every package; task lint-fix clean.

Changes

File Change
pkg/authserver/storage/types.go, memory.go, redis.go, redis_keys.go PendingDeviceLoginStorage / PendingDeviceConfirmationStorage + both backend implementations
pkg/authserver/server/registration/dcr.go device_code-only DCR carve-out
pkg/authserver/server/handlers/device_verification.go New: verification-page handlers, branded HTML templates, error-handling/security hardening
pkg/authserver/server/handlers/callback.go /oauth/callback now dispatches between the OAuth-client and device-flow logins, distinguishing storage errors from not-found
pkg/authserver/server/handlers/handler.go Route registration + rate limiting for the verification page
pkg/authserver/integration_test.go Device flow test now drives the real HTTP page through a mock upstream

Does this introduce a user-facing change?

Yes: with DeviceFlowEnabled on, a device-flow client can now be fully
authorized by a real human via the verification page, and a device-flow
client can be dynamically registered via DCR. Both were previously
non-functional stubs (the flag existed but nothing could actually complete
the flow).

Implementation plan

Approved implementation plan

Device Flow Verification Page + DCR Support

Context

PR #6647 (merged, 59ba54dbd) added RFC 8628 Device Authorization Grant support to the embedded OAuth
authorization server (pkg/authserver/), gated behind DeviceFlowEnabled (off by default). Code review on
that PR (jhrozek) flagged that the feature is not actually usable end-to-end yet:

  1. POST /oauth/device_authorization mints a device_code/user_code pair whose verification_uri points
    at GET /oauth/device — but that route doesn't exist. storage.MarkDeviceRequestAuthorized/
    MarkDeviceRequestDenied are only ever called by the integration test, directly against storage.
  2. pkg/authserver/server/registration/dcr.go's DCR grant-type allowlist only permits
    authorization_code/refresh_token, so there is also no way to register a client allowed to use the
    device_code grant.

Both gaps were called out in the same PR description update ("DeviceFlowEnabled must stay off ... until both
follow-ups above land") and are being addressed together here, in one PR, per that plan.

Part A: Device verification page

Flow

  1. GET /oauth/device[?user_code=XXXX-XXXX]DeviceVerificationHandler renders an HTML form asking
    the user to enter/confirm the user_code (pre-filled from the query param when the client sent
    verification_uri_complete).
  2. POST /oauth/deviceDeviceVerificationSubmitHandler normalizes the submitted code, loads the
    DeviceRequest via storage.LoadDeviceRequestByUserCode (already exists), and validates it is
    DeviceRequestStatusPending and unexpired. On success, generates upstream login secrets (reusing
    newUpstreamAuthSecrets from authorize.go), stores a PendingDeviceLogin row keyed by that state, and
    redirects (302) to h.upstreams[0].Provider.AuthorizationURL(...).
  3. Upstream callback — the upstream IDP's redirect_uri is fixed per upstream at construction, so this
    reuses the existing /oauth/callback endpoint (discovered during implementation; see PR description).
    CallbackHandler resolves the identity and stores it server-side behind a fresh opaque token, then
    renders the Approve/Deny confirmation page.
  4. POST /oauth/device/confirm — loads+deletes the PendingDeviceConfirmation by token (single-use),
    and calls storage.MarkDeviceRequestAuthorized (approve) or MarkDeviceRequestDenied (deny).

Only h.upstreams[0] (the first configured upstream) is used — matching AuthorizeHandler's first-leg
behavior — with no multi-upstream authorization chain support for device flow.

New storage types

PendingDeviceLoginStorage and PendingDeviceConfirmationStorage, modeled on PendingAuthorizationStorage,
implemented for both MemoryStorage and RedisStorage, embedded into Storage.

Part B: DCR support for device_code

Add oauthproto.GrantTypeDeviceCode to allowedGrantTypes; a device-code-only carve-out (parallel to the
existing private_key_jwt one) skips the redirect_uris/response_types requirements when the grant types
are exactly {device_code} or {device_code, refresh_token}.

Testing

Storage round-trip tests for both new types (memory + Redis); handler tests for the full happy path, deny,
and error paths; DCR validation test cases for device-code-only registration; integration test rewritten to
drive the real HTTP flow through a mock upstream IDP instead of simulating the verification page via direct
storage calls.

Special notes for reviewers

  • Architectural correction made mid-implementation: the original plan called for a distinct
    GET /oauth/device/callback route. While implementing it, I found that upstream.OAuth2Provider's
    redirect_uri is fixed per upstream at construction time (upstream.OAuth2Config.RedirectURI, baked into
    the golang.org/x/oauth2 config used by AuthCodeURL) — there's no way to pass a different redirect_uri
    per authorization attempt. So the upstream IDP will always redirect back to whatever /oauth/callback was
    registered with it, never a second device-flow-specific path. I unified this by having CallbackHandler
    try LoadPendingAuthorization first (the existing OAuth-client flow), and fall back to
    LoadPendingDeviceLogin when the state doesn't match — see callback.go's
    loadPendingOrCompleteDeviceLogin/tryCompleteDeviceLogin. This avoids requiring operators to register a
    second redirect_uri with their upstream IdP, which many IdPs require to be pre-registered exactly.
  • Consent step: the verification page shows an explicit Approve/Deny confirmation screen after upstream
    login, rather than auto-approving immediately — matches how GitHub/Google device flows behave.
  • No multi-upstream chain support for the verification page — it always uses the first configured
    upstream, matching AuthorizeHandler's first-leg behavior. Chained upstream authorization for device flow
    is out of scope here.
  • Page styling: the three rendered pages are cosmetic only — no change to the underlying OAuth
    semantics. Kept deliberately simple (a single card, no heavy branding chrome) to stay in the same visual
    family as the existing plain OAuth-adjacent pages elsewhere in the codebase (e.g. the CLI's local
    loopback callback page in pkg/auth/oauth/flow.go).
  • PR size: this exceeds the usual 400-line/10-file guideline, for the same reason Add RFC 8628 device authorization grant support #6647 did — splitting
    storage from the handlers that use it would leave an intermediate PR with dead code and no caller.

🤖 Generated with Claude Code

reyortiz3 and others added 4 commits September 17, 2026 12:26
The RFC 8628 verification page needs to correlate two short-lived,
single-use pieces of state: the upstream IDP login attempt in flight
(state -> device_code/user_code plus PKCE/nonce) and, after that login
resolves, the identity awaiting an explicit Approve/Deny decision.

Add PendingDeviceLoginStorage and PendingDeviceConfirmationStorage,
modeled directly on the existing PendingAuthorizationStorage (same
shape of problem: short-lived, state-keyed correlation data with no
new architectural pattern needed), implemented for both MemoryStorage
and RedisStorage and embedded into Storage alongside
PendingAuthorizationStorage.

The confirmation record is deliberately addressed only by an opaque,
unguessable token handed to the browser as a hidden form field -- the
resolved identity itself is never round-tripped through the browser as
editable form data, closing an obvious tamper vector.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pkg/authserver/server/registration/dcr.go's grant-type allowlist only
ever permitted authorization_code/refresh_token, so there was no way
to dynamically register a client allowed to use the RFC 8628
device_code grant -- a real client could never be provisioned to use
the device flow added in #6647.

Add a device-code-only carve-out (grant_types exactly {device_code} or
{device_code, refresh_token}) parallel to the existing private_key_jwt
carve-out: redirect_uris and response_types are not required, since a
device-code client never receives a redirect. Mixing device_code into
an authorization_code registration is intentionally not covered --
such a request still needs a redirect_uri/response_type as before.

FilterPublicGrantTypes/FilterPublicResponseTypes (used by the CIMD
client-metadata-document path) share the same allowlist and now start
admitting device_code too; existing tests that asserted device_code
was silently filtered out are updated to reflect it being kept.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Closes the gap left by #6647: /oauth/device_authorization mints a
device_code/user_code pair whose verification_uri points at
GET /oauth/device, but that route didn't exist -- storage.
MarkDeviceRequestAuthorized/MarkDeviceRequestDenied were only ever
called by the integration test, directly against storage.

Add DeviceVerificationHandler (GET/POST /oauth/device) and
DeviceVerificationConfirmHandler (POST /oauth/device/confirm), backed
by html/template pages (the first HTML surface in this package -- every
other handler writes JSON). The flow: enter/confirm the user_code,
redirect to the first configured upstream IDP to authenticate, land on
an explicit Approve/Deny confirmation screen, then call
MarkDeviceRequestAuthorized or MarkDeviceRequestDenied.

The upstream IDP's redirect_uri is fixed per upstream at construction
(see upstream.OAuth2Config.RedirectURI / AuthCodeURL), so there is no
way to register a distinct callback path for this second login flow.
CallbackHandler now dispatches between the OAuth-client
authorization_code flow and a device-flow login based on which pending
record the callback's state parameter matches, falling back to
completeDeviceLogin (device_verification.go) when it doesn't match a
PendingAuthorization.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TestIntegration_DeviceFlow_FullHappyPath previously simulated the
not-yet-built verification page by calling storage.
MarkDeviceRequestAuthorized directly. Now that the page exists, wire
the test through a real mock upstream IDP (mockoidc) and the actual
HTTP flow: submit user_code, follow the upstream login redirect, land
on the shared /oauth/callback endpoint, extract the confirm_token from
the rendered confirmation page, and POST the approval -- exercising
the exact path a real device-flow client and human now go through end
to end, closing the loop the original PR's integration test left open.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added the size/XL Extra large PR: 1000+ lines changed label Sep 17, 2026
@codecov

codecov Bot commented Sep 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.66298% with 105 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.19%. Comparing base (97b81d8) to head (f3d64ab).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
.../authserver/server/handlers/device_verification.go 67.91% 60 Missing ⚠️
pkg/authserver/storage/redis.go 86.60% 15 Missing ⚠️
pkg/authserver/storage/memory.go 91.80% 10 Missing ⚠️
pkg/authserver/server/handlers/callback.go 72.00% 7 Missing ⚠️
...rver/server/handlers/device_verification_render.go 87.50% 5 Missing ⚠️
pkg/authserver/server/registration/dcr.go 84.00% 4 Missing ⚠️
pkg/authserver/server/handlers/handler.go 70.00% 3 Missing ⚠️
...r/server/handlers/device_verification_ratelimit.go 95.45% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #6682    +/-   ##
========================================
  Coverage   79.18%   79.19%            
========================================
  Files         791      794     +3     
  Lines       79518    80005   +487     
========================================
+ Hits        62965    63358   +393     
- Misses      16548    16642    +94     
  Partials        5        5            

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 17, 2026
The deny action on the confirmation page always reported success even
when the underlying storage write failed, so a user believed they had
denied a device whose request could remain authorized. The shared
/oauth/callback dispatch also collapsed a genuine storage outage into
the same "not found" path used for an expired state, masking backend
incidents as client errors. Neither the verification page's user_code
guesses nor its confirm_token replay had rate limiting or clickjacking
protection, unlike the existing device_authorization endpoint.

- Mirror the approve branch's error handling on deny; log at Warn.
- Distinguish not-found/expired from real storage errors in the
  callback dispatch and the user_code re-checks, returning 500 on a
  genuine backend failure instead of a generic 400.
- Rate-limit POST /oauth/device, matching /oauth/device_authorization.
- Add anti-framing/no-sniff headers to the verification page renders.
- Add tests for deny-then-replay and a concurrent confirm_token race.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 17, 2026
Give the three device-flow HTML pages (verify, confirm, result) a
proper visual identity instead of unstyled markup: ToolHive/Stacklok
brand colors, light/dark support via prefers-color-scheme, pill
buttons, and a card layout, with no external font/CDN dependency so
the pages still work in air-gapped deployments.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 17, 2026
authorization_code and refresh_token were hardcoded as string
literals in dcr.go even though oauthproto already exported constants
for both, unlike the device_code entry added alongside them. Use the
constants everywhere for consistency.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 17, 2026
Split the HTML templates, page-data structs, and render* helpers out
of device_verification.go into device_verification_render.go, so the
request-handling logic isn't interleaved with ~290 lines of markup
and CSS.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 17, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review of the device-flow verification page (RFC 8628). Overall the design is solid — PKCE/S256, OIDC nonce handling, IDP mix-up defense, explicit opaque-token-gated consent, and the DCR device_code-only carve-out all check out against spec. Two issues below should block merge: upstream tokens are never persisted for device-flow-authorized sessions, and an upstream access_denied during device-flow login isn't routed to the device-flow cleanup path. The rest are lower-severity/non-blocking.

Comment thread pkg/authserver/server/handlers/device_verification.go
Comment thread pkg/authserver/server/handlers/callback.go
Comment thread pkg/authserver/server/handlers/handler.go Outdated
Comment thread pkg/authserver/server/handlers/device_verification.go
Comment thread pkg/authserver/server/handlers/device_verification_render.go Outdated
Comment thread pkg/authserver/server/handlers/device_verification.go
Device-flow sessions have no session id until confirm time, so the
tokens exchanged at login had nowhere to be written yet and were
being dropped entirely -- any downstream lookup by session id (token
injection, refresh) found nothing for a device-flow session.

- Add UpstreamTokens/Synthetic fields to PendingDeviceConfirmation so
  the handler layer can persist them once a session id exists.
- Extract newStoredUpstreamTokens from marshalUpstreamTokensWithTTL so
  both call sites share the same epoch-time conversion.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reyortiz3 and others added 4 commits September 18, 2026 09:20
- Persist the upstream tokens captured at login under the device
  grant's final session id on approval (see the storage commit just
  before this one) -- previously nothing ever called
  StoreUpstreamTokens for a device-flow session.
- Give handleUpstreamError a device-flow fallback: an upstream IDP
  error (e.g. the human denies at the upstream login screen) left the
  DeviceRequest Pending until the device_code itself expired, since
  nothing marked it denied on that path.
- Require exactly one configured upstream at submit time instead of
  silently defaulting to h.upstreams[0] -- device flow has no
  client_id/redirect_uri of its own to route per-client, so a
  multi-upstream deployment would otherwise authenticate against
  whichever upstream is configured first.
- Fix a garbled doc comment on DeviceVerificationHandler.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fields[0][:1] sliced the first byte of a display name, splitting
multi-byte UTF-8 sequences and rendering invalid UTF-8/replacement
characters in the avatar badge for non-ASCII names.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
deviceVerificationLimiter was a single process-wide bucket, unlike
the machine-driven, one-shot endpoints it was modeled on
(registerLimiter et al). /oauth/device is a human-facing login page a
legitimate user retries against, so one caller burning the shared
burst starved every other concurrent device-flow login on the
server. Add a small per-IP limiter (with inline idle-entry eviction,
no background goroutine to manage) and key the gate on RemoteAddr --
deliberately not X-Forwarded-For, which is attacker-controlled
without a trusted proxy list.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Covers: upstream tokens retrievable by session id after approval,
denial on upstream IDP error, the single-upstream requirement,
non-ASCII initials, and per-IP rate-limiter scoping.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 18, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 6 findings from the previous review round were addressed with dedicated commits and accompanying tests: upstream tokens are now persisted under the device grant's final session id, upstream IDP denial now routes to the device-flow cleanup path, the verification-page rate limiter is now per-IP, the confirm-page initials handle multibyte names correctly, the upstream-selection ambiguity is now an explicit fail-fast, and the garbled doc comment is fixed. LGTM pending CI.

@github-actions github-actions Bot added size/XL Extra large PR: 1000+ lines changed and removed size/XL Extra large PR: 1000+ lines changed labels Sep 18, 2026
@reyortiz3
reyortiz3 merged commit ef8096c into main Sep 18, 2026
46 checks passed
@reyortiz3
reyortiz3 deleted the device-flow-verification-page branch September 18, 2026 13:58
@github-actions github-actions Bot mentioned this pull request Sep 18, 2026
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XL Extra large PR: 1000+ lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants