Skip to content

remote/oidc: discriminated credential record + session-aware resolve (T4) - #199

Closed
platypii wants to merge 3 commits into
chunk2-identity-oidc-loginfrom
chunk3-credentials-kind
Closed

remote/oidc: discriminated credential record + session-aware resolve (T4)#199
platypii wants to merge 3 commits into
chunk2-identity-oidc-loginfrom
chunk3-credentials-kind

Conversation

@platypii

Copy link
Copy Markdown
Contributor

Chunk 3 of the OIDC client login (LLP 0046-0048). Stacked on #198 (base chunk2-identity-oidc-login); review/merge after #197, #198.

What

  • Per-target records gain a kind discriminator (LLP 0046 D4). readCredentials normalizes; a legacy token-only record reads as static (read-implicit migration). writeToken now stamps kind: 'static'.
  • writeSession(...) writes a kind: 'oidc' record through the same atomic 0600 path.
  • resolveAccessJwt({...}) (LLP 0046 D5): env override wins; static returns its token; oidc returns a fresh JWT, refreshing + persisting within a 60s skew of expiry and propagating an invalid_grant failure. resolveToken stays for the stdio proxy.
  • removeToken drops either kind. types.d.ts gains the record interfaces.

Tests

remote-credentials-oidc.test.js (10 tests) + updated static tests; full npm test green (1515 pass); typecheck clean.

🤖 Generated with Claude Code

Two dependency-free local primitives for the browser login flow:
- pkce.js: createPkcePair() -> { verifier, challenge }, S256 over stdlib
crypto. The client's downstream PKCE leg (LLP 0046 D3).
- loopback.js: startLoopbackReceiver({ state, timeoutMs }) binds a
single-shot 127.0.0.1:0 HTTP listener serving /callback, returns its
redirectUri up front, and resolves { code } on a state-matched
callback (rejecting on mismatch, error=, or timeout). RFC 8252
ephemeral redirect (LLP 0046 D2).
Unit tests cover the SHA-256 challenge derivation, fresh randomness,
and the loopback success / state-mismatch / error / timeout paths.
… T5)
Completes milestone-1 local primitives, composing chunk 1's PKCE +
loopback:
- identity_client.js: exchangeCode / refreshSession over an injectable
fetch against <origin>/v1/identity/token; a 401 invalid_grant surfaces
a typed InvalidGrantError. Response field is access_jwt, expires_at is
ISO. No external JWKS on the client.
- open_browser.js: platform opener (open / xdg-open / cmd start),
detached; returns whether an opener was found (LLP 0046 D8).
- oidc_login.js: loginWithBrowser() orchestrates PKCE + a random state,
starts the loopback receiver, builds the /login/start URL, opens the
browser (or prints the URL), awaits the code, and exchanges it for the
session. No persistence; the caller stores it (LLP 0046 D2/D3).
- types.d.ts: OidcSession / RefreshedAccess shared interfaces.
Unit tests cover the token request bodies + response mapping, the
invalid_grant typing, each platform opener, and the full
PKCE->loopback->exchange orchestration including --no-browser and
loopback cleanup on failure.
…(T4)
Extends the 0600 credential store to carry OIDC sessions alongside the
existing static tokens (LLP 0046 D4), and adds the attach-path resolver
(LLP 0046 D5):
- Records gain a kind discriminator. readCredentials normalizes each
record; a legacy token-only record (no kind) reads as static, so
existing files keep working without a rewrite. writeToken now stamps
kind: 'static'.
- writeSession(stateDir, target, { refreshToken, accessJwt, expiresAt,
org }) writes a kind: 'oidc' record through the same atomic 0600 path.
- resolveAccessJwt({ target, env, stateDir, identityBase, now, fetchImpl
}): env override wins; a static record returns its token; an oidc
record returns a fresh access JWT, refreshing + persisting when the
cached one is within a 60s skew of expiry, and propagating a refresh
failure (typed invalid_grant). resolveToken stays for the stdio proxy.
- removeToken drops either kind (whole-record delete, unchanged).
- types.d.ts: RemoteStaticRecord / RemoteOidcRecord / the union.
Unit tests cover the round-trip, legacy normalization, env override,
fresh vs stale refresh + persistence, and failure propagation. Full
suite green.
@platypii
platypiiforce-pushed the chunk2-identity-oidc-login branch from d0ea83e to a60d2d6CompareJune 29, 2026 18:36
@platypii
platypii deleted the branch chunk2-identity-oidc-loginJune 29, 2026 18:36
@platypii

Copy link
Copy Markdown
ContributorAuthor

Review (independent agent review)

Verdict: approve-with-nits. Correctness is sound for realistic inputs: read-implicit migration (legacy {token} reads as static, writes stamp kind), env-override precedence for both kinds, isFresh skew direction + NaN/empty expiresAt handling (unparseable forces refresh), atomic tmp+rename 0600 with chmod re-assert, and invalid_grant propagation all check out.

Findings

  • minor — normalizeRecord could drop a usable static token. A corrupt/hand-edited record with both token and refreshToken but no accessJwt and no kind matched the oidc guard, failed the accessJwt check, and returned null — silently losing a working credential. (Only reachable via a hand-edited file; no code path writes that shape.)
  • minor — mixed error channel in resolveAccessJwt: most failures return { ok: false } but a refresh failure throws. This is intentional per D5 (so the attach path can map invalid_grant to re-login guidance), documented.
  • minor/follow-up — resolveToken hands an oidc record's cached JWT to the stdio proxy with no freshness check. Deliberate split (refresh lives on the attach path per D5); tracked as a proxy follow-up.
  • nits — boundary test gaps: skew-window edge, malformed-oidc drop, resolveToken on an oidc record.

Addressed in this PR

  • normalizeRecord now falls through to a usable static token when the oidc shape is incomplete.
  • Added tests: incomplete-oidc-with-static-token fallthrough, malformed-oidc drop, resolveToken returning an oidc cached JWT, and a skew-window-boundary refresh (expiry 30s out, inside the 60s window).

The mixed error channel is intentional (D5) and the proxy-freshness item is an out-of-scope follow-up. 21 tests green, typecheck clean.

platypii added a commit that referenced this pull request Jun 29, 2026
… record
Review follow-up (PR #199): an incomplete oidc shape (refreshToken but no
accessJwt) on a hand-edited record no longer returns null and drop a
working static `token`; it falls through to the static branch. Added
boundary tests: the static fallthrough, a malformed-oidc drop, resolveToken
on an oidc record, and a skew-window-boundary refresh.
platypii added a commit that referenced this pull request Jun 29, 2026
…(T4) (#203)
* remote/oidc: discriminated credential record + session-aware resolve (T4)
Extends the 0600 credential store to carry OIDC sessions alongside the
existing static tokens (LLP 0046 D4), and adds the attach-path resolver
(LLP 0046 D5):
- Records gain a kind discriminator. readCredentials normalizes each
record; a legacy token-only record (no kind) reads as static, so
existing files keep working without a rewrite. writeToken now stamps
kind: 'static'.
- writeSession(stateDir, target, { refreshToken, accessJwt, expiresAt,
org }) writes a kind: 'oidc' record through the same atomic 0600 path.
- resolveAccessJwt({ target, env, stateDir, identityBase, now, fetchImpl
}): env override wins; a static record returns its token; an oidc
record returns a fresh access JWT, refreshing + persisting when the
cached one is within a 60s skew of expiry, and propagating a refresh
failure (typed invalid_grant). resolveToken stays for the stdio proxy.
- removeToken drops either kind (whole-record delete, unchanged).
- types.d.ts: RemoteStaticRecord / RemoteOidcRecord / the union.
Unit tests cover the round-trip, legacy normalization, env override,
fresh vs stale refresh + persistence, and failure propagation. Full
suite green.
* remote/oidc: normalizeRecord keeps a usable static token on a corrupt record
Review follow-up (PR #199): an incomplete oidc shape (refreshToken but no
accessJwt) on a hand-edited record no longer returns null and drop a
working static `token`; it falls through to the static branch. Added
boundary tests: the static fallthrough, a malformed-oidc drop, resolveToken
on an oidc record, and a skew-window-boundary refresh.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@platypii