From cfa98be8d2b52ff51bad2be4209a149c69f4ef07 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sun, 26 Apr 2026 18:17:28 +0300 Subject: [PATCH] docs(lethe-web-ui-login): record review pass + conclusion --- docs/tasks/lethe-web-ui-login.md | 69 +++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 1 deletion(-) diff --git a/docs/tasks/lethe-web-ui-login.md b/docs/tasks/lethe-web-ui-login.md index 9830daddabf483dcb8a98b9cdd6294421e1d59be..8b4f8a66521abe77365361b83ac0ec42627fc8e0 100644 --- a/docs/tasks/lethe-web-ui-login.md +++ b/docs/tasks/lethe-web-ui-login.md @@ -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 - + +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:`; 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 `` 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 `` (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