~bigbes/lethe

cfa98be8d2b52ff51bad2be4209a149c69f4ef07 — Eugene Blikh a month ago e920ae8
docs(lethe-web-ui-login): record review pass + conclusion
1 files changed, 68 insertions(+), 1 deletions(-)

M docs/tasks/lethe-web-ui-login.md
M docs/tasks/lethe-web-ui-login.md => docs/tasks/lethe-web-ui-login.md +68 -1
@@ 364,7 364,74 @@ Notes:
- **Browser smoke deferred** — the new `/login` and `/auth/callback` routes plus the dev-stub `/authorize`+`/token` round-trip were not exercised end-to-end in a real browser. tsc + vitest + Go tests are clean and the dev stub is now RFC-compliant for auth-code+PKCE per the test suite, but a full Chrome walk requires standing up the lethe server with OIDC enabled, the dev stub, AND removing the `Remote-User` injection from `web/vite.config.ts` (RK2: otherwise dev fetches succeed via forward-auth and AuthGate never triggers). Out of reach in autonomous mode without user setup. Recommended walk: (a) start the lethe server with `auth.oidc.enabled=true` and `auth.oidc.dev_stub.enabled=true`, (b) comment out the `Remote-User` injection in `web/vite.config.ts`, (c) `npm run dev`, (d) hit `/`, see the AuthGate "Sign in with OIDC" card, click it, watch the round-trip, land back authenticated.

## Conclusion
<empty — filled by up:ureview>

Outcome: shipped — `ac34df3..HEAD` (HEAD = `e920ae8`); 7 execute commits + 3 review-fix commits + 2 doc commits.

Invariants:
- IV1 — `oidcstub.go:340-346` rejects `code_challenge_method != S256` with 400 invalid_request; `oidcstub.go:405-410` verifies `base64url(SHA-256(verifier)) == code_challenge` before mint. Tests: `TestToken_BadVerifier_Returns400`, `TestAuthorize_NonS256Challenge_Returns400`.
- IV2 — `codestore.go:71` `delete(s.entries, code)` runs unconditionally inside `Consume` (single-use). Tests: `TestCodeStore_Consume_SingleUse`, `TestToken_CodeReuse_Returns400`.
- IV3 — `oidcstub.go:350` issues codes with `5*time.Minute` TTL; `codestore.go:62-72` rejects expired entries. Tests: `TestCodeStore_Consume_Expired` covers the unit-layer expiry; `TestToken_ExpiredCode_Returns400` is `t.Skip`'d (handler clock not injectable — unit-test layer is the test boundary; recorded as deviation).
- IV4 — `grep -n "localStorage.setItem" web/src/lib/auth.ts` empty; tokenStore writes are closure-only. Anti-loop counter and PKCE state in localStorage are timestamps + verifiers + state nonces, never tokens. Cache-invalidation test in `auth.test.ts` enforces.
- IV5 — `web/src/api/client.ts:25-28` returns `{Authorization: \`Bearer ${token}\`}` only when `token !== null`; null returns empty spread (no header). Tests in `client.test.ts` cover both branches.
- IV6 — `AuthGate.tsx:32-58` `auth_error` branch renders manual "Try again" button; no `useEffect` auto-retry. Anti-loop counter in `auth.callback.tsx` blocks at >3 failures within 5 min and renders "please reload" instead of redirecting.
- IV7 — `AuthGate.tsx:22-26` `useEffect` auto-redirect fires only when `status === 'unauthenticated' && hasBeenAuthenticated === true`; cold render falls through to manual button at line 65+. `hasBeenAuthenticated` flag flows from `AuthProvider` to `AuthGate` via `AuthState`.
- IV8 — `oidcstub.go:125` `mux.HandleFunc("/dev/token", s.handleDevToken)` unchanged; `TestDevToken_StillWorks` regression-passes; `TestEndToEnd_MultiUserIsolation` in `cmd/lethe` passes.

### Assumptions check

- AS1 — held — `web/src/test-setup.ts:3-11` polyfills `globalThis.crypto` from Node `webcrypto` for vitest; production browsers carry `crypto.subtle` natively per modern-browser baseline. No conditional exists in production code that depends on the polyfill.
- AS2 — held — `internal/server/server.go` keeps `validateLoopbackBind`; dev-stub binds to `127.0.0.1:<port>`; SPA's `redirect_uri = ${origin}/auth/callback` resolves to a localhost URL the stub accepts. `TestAuthorize_RedirectsWithCodeAndState` exercises a localhost redirect.
- AS3 — unverifiable at this layer — same SPA code talking to a real OP unchanged is UK1's territory. The stub is RFC 6749 §4.1 + RFC 7636 §4.6 compliant (per the test suite + the percent-encoding fix below); divergence with a real OP would be a stub bug, not a SPA bug.

### Unknowns outcome

- UK1 — still-open — real-OP smoke (Authelia, Keycloak, etc.) is out of scope per Design "Out of scope". The dev-stub flow is RFC-conformant per the test suite; the user's real-OP rollout is the next gate.
- UK2 — resolved — `authContext.tsx` parses the `id_token` payload for the `name` claim only; falls back to `user = null` on parse failure (token still valid for middleware checks; name is cosmetic). Stored in `AuthState.user.name`; consumed nowhere yet (TopBar greeting not wired in this task — additive follow-up).

### Plan adherence

- **Plan 1.7 unimplemented** — required passing `Cfg.OIDC.DevStub.User` to `oidcstub.Options{}`, but `OIDCDevStubConfig` has no `User` field in `internal/config/config.go`. PH1 implementer disclosed this; the stub falls back to its `"bigbes"` constant default. Behaviour matches the project's single-developer convention. Adding the config field is scope creep; logged here for completeness. Reviewer flagged separately at confidence 80; same call.
- **`Stub.handleAuthorize` `redirect_uri` validation** — RFC 6749 §4.1.3 says the `redirect_uri` sent on `/token` must match the one stored at `/authorize`. `oidcstub.go:401-403` (in `handleToken`) verifies this; `TestToken_RedirectURIMismatch_Returns400` regression-tests it. Plan called this out under PC1; verified.

### Review findings

- Critical: none.
- Important (4 — three fixed, one recorded):
  1. **`embed.go` silent no-op when `</head>` absent** — fixed in `f1926dc` by comparing pre/post `bytes.Replace` and writing 500 on no-op (fail-loud per GPC6).
  2. **`oidcstub.go` `handleAuthorize` redirect query unencoded** — fixed in `be6e43e` by switching to `url.Values{}.Encode()` for RFC 6749 §4.1.2 compliance (PC1). Operationally inert in today's SPA (state was URL-safe), but real-OP-parity invariant required the fix.
  3. **`auth.callback.tsx` `lethe_auth_failures` array unbounded** — fixed in `e920ae8` by pruning entries outside the 5-min window before each push. Plan explicitly asked "is the array bounded; are timestamps pruned at insert" — answer is now yes.
  4. **`devstub.go` plan 1.7 no-op** — recorded only (see Plan adherence). Behavior is correct; only loss is configurability that was never in the config struct.

### Future work

- **Wire `AuthState.user.name` into the TopBar greeting** — UK2 resolved at the data layer; UI hookup deferred (out of scope per Design "TopBar greeting only" comment; not load-bearing for this task's gating-flow purpose). Single sibling-task call when desired.
- **Real-OP smoke (UK1)** — user-driven real-OP integration test once Authelia/Keycloak deployment exists. Stub is RFC-conformant; divergence would be a stub bug.
- **Add `User` field to `OIDCDevStubConfig`** if a future contributor needs to dev as someone other than `bigbes`. Below the rule-of-three threshold today.

### Verified by

- Browser smoke deferred — see Verify → Notes → "Browser smoke deferred". The new flow requires standing up the lethe server with OIDC enabled, the dev stub, AND temporarily commenting out the `Remote-User` injection in `web/vite.config.ts` (RK2).

### Hands-off decisions

- size: Medium — covers backend dev-stub upgrade + SPA login flow + three call-site swaps. Full Design + Plan + Execute flow.
- mode: hands-off enabled mid-design at user request. The /up:make invocation did not prefix `handsoff`; flipped after the user said "so handoffed design is not applicable?".
- udesign — Path A (dev-stub upgrade to full auth-code+PKCE OP) — single code path beats divergent ones.
- udesign — token model: SPA-direct (Bearer); middleware already accepts Bearer.
- udesign — token storage: in-memory React context; reload = re-login.
- udesign — refresh: re-login on expiry; 8h stub default.
- udesign — trigger: manual click first, auto-redirect on mid-session 401.
- udesign — anti-loop: PKCE state + return_to in localStorage with 5-minute TTL; max 3 callback failures in 5 minutes.
- udesign — bundle dev-stub upgrade in this task (not a #10 follow-up).
- branch/worktree: Branch=master, Worktree=none — deviates from hands-off default but matches every prior `lethe-*` task.
- uplan: plan auto-approved (hands-off) — four phases; IF1/IF2/IF3 cross-phase; Wave 1 = PH1‖PH2, Wave 2 = PH3, Wave 3 = PH4.
- uplan: surfaced deviation — SPA needs to learn issuer/client_id at runtime; design did not specify the mechanism. Pick: inject `window.__LETHE_CONFIG__` via `internal/server/web/embed.go`.
- uplan: surfaced deviation — PH3.4 (`auth.callback.tsx`) uses raw `fetch` to POST to cross-origin `/token`; the foundation invariant "no raw `fetch` outside `client.ts`" does not extend to non-`/api/v1` cross-origin endpoints. Allowed.
- uexecute: wave-1 IF1 contract fix — PH1's `Config{Issuer, ClientID}` had no json tags so JSON would have emitted PascalCase, breaking PH3's `client_id` reader. Fixed inline at the wave-1/wave-2 boundary in commit `5d910e8` before dispatching PH3.
- ureview: fixed embed.go silent no-op — added pre/post `bytes.Equal` check; 500 on missing `</head>` (commit `f1926dc`).
- ureview: fixed oidcstub redirect encoding — switched to `url.Values.Encode()` for RFC 6749 §4.1.2 compliance (commit `be6e43e`).
- ureview: fixed auth.callback unbounded log — prune-then-push within 5-min window (commit `e920ae8`).
- ureview: recorded deviation only for plan 1.7 (DevStubUser config field doesn't exist; stub default is correct behavior).

### Hands-off decisions