Repository navigation
Conversation
elifarley
added a commit
to elifarley/komodo
that referenced
this pull request
Oct 1, 2026
Apply the moghtech#1666 roast (2 critical, 9 major, 4 minor, 5 observations). This round reviewed the PLAN's code against its cited artifacts, with executed receipts (caddy validate rejected the plan's Caddyfile; the census command re-produced 46). Two of this spec's own artifact claims were refuted and are corrected here, both found by re-reading the pinned dists: - Cache listeners are invoked SYNCHRONOUSLY inside dispatch's batch (mutationCache.notify -> listeners.forEach runs in batch's callback; systemSetTimeoutZero schedules only the batched observer queue). The previous "macrotask delivery" claim — mine, from round 2 — was a misreading of notifyManager.batch. The arming conclusion survives STRONGER: a render-phase dispatch notifies zero effect-scoped subscribers, deterministically, not probabilistically. Also: the pending dispatch precedes config.onMutate, not the reverse. - The settled-failed poller stop was INVERTED: remove_all ran in Router's effect (post-commit), so the parent provider's flip render computed hasJwt stale-true and enabled stayed true — one stale-token 401 escaped after settlement; the real stopper was the 401 latch. Fix: hygiene moved into the settlement listener (synchronous, in dispatch's task, before any render). The zero-residual claim is now true by construction instead of by accident of the failure mode that hid the bug (plain hung has no stale token — only seed-intersect- failure exposes it, exactly the comparison Task 5 invites). Plan-level fixes (the critical pair): Task 1 now creates and wires compose/oidc-dev/core-ui.Dockerfile (branch UI built into the digest-pinned core image at /app/ui, spec §6.3) — before, no task produced the artifact Task 11 told the implementer to rebuild, and the fix would have been tested against the stock UI. The latency knob is fixed at both layers: Caddyfile rewritten as a route/handle block routing /auth/login/* to the delay sidecar (the old snippet was adapter-rejected: named matchers are not valid inside reverse_proxy), and the sidecar awaits a real setTimeout before dispatching (ClientRequest.setTimeout is a listener-less no-op). latency/hung now assert observed exchange round-trip >= DELAY and the settlement path actually taken, so a no-op knob fails visibly instead of passing green. Also: m2-forced (route-rewritten sub-less exchange response) and exchange-error (consumed-session replay) scenarios close the §8-row coverage gap; the §6 console shim (localStorage TOKENS transitions) is back in the driver; §11's by-name criterion checks a README row->scenario map instead of trusting output format; scaffold defects fixed (oidc-mock build context, access.log touch-before-mount — Docker pre-creates a directory for a missing bind-mount file); unauthOrFail applies the §6 exclusion list before the status check (a consumed-session 401 on ExchangeForJwt is exempt, not residual); pollLog guards torn writes; §7.2's round-4 orphan useRedeemSettled replaced with the listener-side mechanism it should have described; §7.7's cross-reference to a pre-reload-only check reworded to the implementable any-entry-presence form with the safety rationale stated; late-success-overwrites-user-B recorded in §10. Convergence record: new commit, never an amend. Five rounds in, every critical has lived in the previous round's addition — and every one was found by diffing a sentence against its cited artifact, which is the methodology this document set now encodes.
elifarley
added a commit
to elifarley/komodo
that referenced
this pull request
Oct 1, 2026
Apply the second PR roast of moghtech#1666 (11 findings: the spec's pinned-artifact reading was independently re-verified end to end and every marker checked TRUE — what failed is the plan's own new harness code, on three independent fatal axes plus oracle defects. The three fatals, each verified before fixing: - C-001: the mock's session cookie had no Domain, so it was host-only for portal.oidctest.localhost — the browser would never present it to komodo.oidctest.localhost, so Caddy's forward_auth subrequest would 401 EVERY komodo request including the OIDC callback. No login could ever complete. Fix: provider cookies.long.domain = oidctest.localhost (mirroring Authelia's parent-domain cookie), the no-session AC tightened to redirect-only (a bare 401 now FAILS the gate — it would have been recorded as a reproduced mechanism, a false M3 poisoning the whole discrimination deliverable), and a README caveat for the .localhost Domain-attr PSL edge with the *.test fallback. - C-013: the drive clicked an OIDC button the pinned LoginPage has never rendered (dist-verified by the roast and consistent with my own reads), and the only real entry point (oidc_auto_redirect) is pinned false for M6 discrimination — the two goals were mutually exclusive. Fix: start the flow by direct navigation to /auth/oidc/login?redirect=%2F — the exact URL mogh_auth_client's externalLogin builds — identical core-side flow, pin stays false, portal-reached asserted loudly instead of .catch(() => {}). - C-011: /verify swallowed lookup errors into a bare 401 with zero logging, at the one point whose failure mode masquerades as M3. Fix: log + 503 on lookup error vs 401 on no session, and a documented docs-check step for provider.Session.get's argument shape (oidc-provider documents Koa contexts; the plan now says verify before relying). Oracle defects (each would have shipped silently wrong): - C-017: Caddy ts is epoch-SECONDS; Date.parse of it is NaN and the <=3s check would fail every green run (or, "fixed" naively, pass anything within 50 minutes). Compare in the log's own unit with finite guards. - C-018: the 60s zero-residual window outlived a driver that lived 8-20s; stubs returned true. Failure rows now wait the window out and unimplemented checks throw instead of passing. - C-019: (e.status ?? 101) defaulted missing data to success — the one assertion guarding the riskiest edit could not fail on absent evidence. Positive status match + browser-side liveness, fail-closed until filled. - C-020: per-process logOffset=0 over a persistent file re-read prior runs' entries — checks passed on stale evidence. Baseline the offset at process start; scope checks to post-t0 rows. - C-015: delay service reused the mock Dockerfile with a repo-root context whose COPY paths cannot resolve — watchdog row blocked at build. The sidecar needs no build at all (node:22-alpine + mounted delay.mjs). - O-001: the console shim attached after the drive, missing the exchange document's add_and_change — the M2-critical transition. Now attached pre-navigation. Smaller: one-key-two-writers wording (C-025), Task 7 snippet gains explicit mutations.retry:false to match its own AC (C-026), function-valued enabled composed instead of silently dropped (C-027 — same silent-override class the drive-by exists to fix), corrupt-store import brick + sessionStorage double-loss recorded in §10/§7.3. Six rounds, and the pattern held one more time: this round's failures were all in round 5's harness code, found by executing and fetching (the roast pulled the actual LoginPage dist and ran caddy validate) rather than by reading prose. The Phase-2 gate design survived untouched for the second round running.
…ealthy the harness bootstrap of the moghtech#1665 repro-harness plan (spec 6): compose project `komodo-oidc-dev` boots mongo + komodo-core serving the branch-built UI from /app/ui exactly like production, verified green (both containers Up; GET http://localhost:9120 -> 200) before any fix code exists. Four design-prescribed details turned out wrong or fragile on first boot; each fix carries a WHY comment in the file it lives in: - The image tag is `2.3.3`, NOT `v2.3.3` — the v-prefixed tag does not exist on ghcr (plan's Step 1 command 404s). Pinned via the registry API where buildx is absent: sha256:bca73d0eee143066228fb9b80c38a5a2 726e573ad2758f7aa92b5c91a8aef1c7 (OCI index digest of :2.3.3), stored in compose/oidc-dev/.env (gitignored, never committed) as KOMODO_CORE_IMAGE and consumed by core-ui.Dockerfile's `FROM ${CORE_IMAGE}`. - Core's env is KOMODO_DATABASE_URI (database.uri), not KOMODO_DATABASE_URL. An unrecognized env var is silently ignored and core falls back to database.address = localhost:27017, then dies: "FATAL: Failed to initialize database::Client". Lesson: komodo silently drops unknown KOMODO_* envs — check config/core.config.toml's documented Env names when a container starts but connects to the wrong place. - The mongo healthcheck must be CMD-SHELL, not a CMD array. On the podman-compat path the array's eval expression is re-split into separate argv items ("--eval try" + junk), so the check can never pass — and because rs.initiate() only ever runs inside that healthcheck, the replica set never initialized at all. - `hostname: mongo` pins the replica-set member address. A bare rs.initiate() self-registers under the container's hostname (the ephemeral container id), so the FIRST container recreation poisoned the persisted RS config ("InvalidReplicaSetConfig: not a member of it") and every later healthcheck re-tripped "already initialized". Machine-compat (podman 3.4 rootless) workarounds, kept out of the portable parts: mongod 8 SIGSEGV-loops in tcmallocBg under podman's pre-rseq seccomp profile, so mongo runs seccomp=unconfined (local dev only); and fresh compose networks need ~/.config/cni/net.d/komodo-oidc-dev_default.conflist patched cniVersion 1.0.0 -> 0.4.0 once (README documents both). On a stock docker host none of this is needed — `docker compose -f compose/oidc-dev.compose.yaml --env-file compose/oidc-dev/.env up -d --build` just works. The served UI is this branch's build by construction: stage 1 mirrors ui/Dockerfile's builder verbatim over the worktree context, stage 2 is the digest-pinned core image + COPY --from=builder ... /app/ui (spec 6.3). Later tasks bolt on oidc-mock (node-oidc-provider pinned 11.10.1, documented in README), Caddy, and the Playwright suite.
…des file
Code-first review round. The skeleton pinned oidc_enabled/oidc_auto_redirect
as KOMODO_* env vars alongside the mounted core-config.toml. That fights
komodo's precedence rule: bin/core/src/config.rs:211-213 resolves
`env.komodo_oidc_enabled.unwrap_or(config.oidc_enabled)` (same
env-unwraps-config pattern for oidc_auto_redirect at :238-240 and
jwt_secret at :165-168), so any env pin silently defeats a later flip of
the file value — exactly what the proxy wiring does (set
oidc_enabled = true in the mounted config). Deleted KOMODO_OIDC_ENABLED,
KOMODO_OIDC_AUTO_REDIRECT, and the redundant KOMODO_JWT_SECRET from the
compose env; core-config.toml (mounted at /config/config.toml) is now
the single source of truth for all three, and the compose file carries
the precedence rule as a comment so the next editor doesn't re-add them.
Also, preserving knowledge at the point of failure:
- core-ui.Dockerfile: comment on ARG CORE_IMAGE — it must be a full ref
with digest from compose/oidc-dev/.env; a forgotten --env-file fails
at `FROM ${CORE_IMAGE}` with only this comment to explain why.
- README Run section: points at Step 1 first — the referenced .env does
not exist until the digest is pinned.
- core-config.toml: absorbed the deleted inline comment's spec 6 pin
rationale (oidc_auto_redirect = false — M3/M6 discrimination).
Re-verified after the change (core recreated, mongo healthy):
both containers Up; GET localhost:9120 -> 200; boot log shows
oidc_enabled: false / oidc_auto_redirect: false / jwt_secret set, now
sourced from the file; digest pin unchanged
(sha256:bca73d0e...a8aef1c7) and still resolved as the build arg.
The harness now has a real OIDC provider: oidc-provider serves discovery,
authorization (PKCE required), a dev-interaction login for a static user,
auto-approved consent, token, and userinfo — plus a bare-http /verify
endpoint that Caddy's forward_auth will call.
Every plan snippet needed source-verified adaptation, because the pinned
"oidc-provider 11.10.1" does not exist on npm (no 11.x line was ever
published; latest is 9.12.2). All API notes below are verified against the
9.12.2 source in node_modules and empirically after boot:
- provider.Session.get accepts a Koa ctx OR any { req, res } pair
(lib/models/session.js falls back to provider.createContext(ctx.req,
ctx.res); Provider extends Koa). A bare IncomingMessage has neither
shape, so Session.get(req) would have thrown -> 503 on every verify.
- Session has no userId(); the persisted login field is accountId. The
plan's session.userId() would have TypeError'd every call into the 503
path — the exact failure mode this harness must not mask.
- provider.callback is inherited from Koa: it is a handler FACTORY.
provider.callback(req, res) builds a handler and drops it (requests
hang); the handler must be built once and invoked.
- loadExistingGrant runs with the Koa ctx: account/client live under
ctx.oidc.account/ctx.oidc.client (ctx.account is undefined there ->
500 on the interaction resume). Returning a grant pre-loaded with the
requested scopes is what skips the consent prompt (one POST login,
zero consent round-trips).
- The keys of `claims` ARE the AS-supported scopes: requesting `email`
with no claims.email key fails with "scope must only contain
Authorization Server supported scope values".
Verified end-to-end: discovery 200 from inside the compose network;
/verify 401 without a session (logged as "no session", not 503), 200 with
a real session cookie (also from the Host=oidc-mock:3344 vantage Caddy
uses), 401 for a forged cookie; code -> POST /token with
client_secret_basic + PKCE S256 -> id_token (sub=alice,
aud=komodo-harness); GET /me -> {sub, email} (komodo should read the
profile from userinfo — the id_token carries only sub).
Gotchas recorded in compose/oidc-dev/README.md for the proxy/suite wiring: v9 builds
discovery endpoint URLs from the REQUEST origin (fetch discovery through
the Caddy portal vhost, or every URL comes back as http://oidc-mock:3344);
curl's cookie jar refuses Domain=oidctest.localhost (the same PSL edge as
the browser caveat) — drive logins by copying the Set-Cookie value into a
manual Cookie header.
yarn.lock is committed so the Dockerfile's --frozen-lockfile build fails
loudly on dependency drift instead of silently resolving a newer tree.
…/verify, loopback-only mock Code-review round on cfe44a2: one real build bug and one crash bug, plus hardening/docs found by the same lens. - Dockerfile now COPYs yarn.lock next to package.json and drops the `|| yarn install` fallback. The original never copied the lockfile, so --frozen-lockfile printed "No lockfile found" and the (supposedly redundant) fallback re-resolved the transitive tree on every build — the committed lockfile was decoration. With the fallback gone, a package/lockfile mismatch fails the build loudly instead of silently floating dependencies. - /verify's `new URL(req.url)` is now guarded: `GET //` (or any malformed request line) made it throw, the rejection was unhandled, and Node kills the process (exit 99). 3344 was published on all interfaces, so any LAN scanner could have killed the mock mid-login. Now: 400, keep serving — verified with a raw `GET //` probe followed by a healthy discovery call. - Port binding tightened to 127.0.0.1:3344:3344 and the boundary is stated where it matters: this mock auto-consents ANY caller, so it must never be exposed beyond localhost / the compose network. The client_secret comment marks it a throwaway. - README login recipe: steps 1–3 now share a cookie jar (the _interaction* cookies are host-only; without a jar the login POST dies with SessionNotFound), and the resume step — the response that actually sets _session — is spelled out. The manual Cookie: header trick stays only for _session, whose Domain=oidctest.localhost attribute curl's jar refuses. Re-verified after rebuild: discovery 200 (loopback + in-network), /verify 401 without a session, malformed-request probe 400 with the container staying up.
…C wiring Proxy front for the moghtech#1665 harness: TLS front for komodo + portal vhosts, forward_auth gate to the mock's /verify, JSON access log with request headers, and the DELAY_AUTH_MS latency knob on /auth/login/*. Core runs OIDC-enabled against the mock (config-file only — env pins would override it, config.rs:211-213). All five acceptance criteria verified live; see README "PRE-FIX OBSERVATION" for the pre-fix baseline (first ExchangeForJwt 200 + jwt, replay 401 with the 5-attempt rate-limit counter burning, reload does NOT bounce, no M3 claimed). Deviations from the early sketch (reality won; full list in the README): - ONE published port, 127.0.0.1:8443->443. The host's system Caddy owns :80 and rootless podman cannot publish <1024 at all (rootlessport refuses, and cannot bind [::1] either). Every browser-facing origin therefore carries :8443: MOCK_ISSUER, KOMODO_ORIGIN, KOMODO_HOST/host, oidc_provider, and the mock client's redirect_uris. Loopback-only binding is the security boundary for the portal vhost (the mock behind it auto-consents anyone). - Caddy listens on container ports 443 AND 8443. The network alias fixes NAME resolution from inside the compose network, not the port: core dials the issuer URL verbatim, and with only :443 listeners it got "connection refused". - Core trusts Caddy's internal CA via SSL_CERT_FILE + an exported caddy-root-ca.crt (gitignored, re-export after volume recreation — a stale one fails as rustls "invalid peer certificate: UnknownIssuer"). SSL_CERT_FILE is honored because core's stack resolves roots through rustls-native-certs. - forward_auth lives INSIDE the route block: Caddy's directive order puts route/handle ahead of forward_auth, so a top-level gate + route block would let requests reach core ungated. Verified with `caddy adapt` (gate first). - /verify now 302s unauthenticated NAVIGATIONS to a throwaway authorize URL (AC-2 wants a portal redirect, not a bare 401). Navigation detection is X-Forwarded-Method (caddy always sets it on the subrequest); direct probes without it keep the mock's 401 contract, and non-GET/HEAD (the redeem POST) 401s so XHR sees a status instead of portal HTML. The throwaway authorize lands on a NEW registered redirect_uri, portal /dev — a gateway-initiated authorize carries a fixed PKCE challenge that could never complete komodo's exchange, so the landing must not be komodo's callback. - Mock runs with Koa proxy=true: v9 derives every discovered endpoint from the REQUEST origin (ctx.href in oidc_context.js urlFor), and behind a TLS-terminating proxy it would otherwise return http:// URLs for an https issuer. Only the discovery `issuer` field stays pinned — which is why oidc_provider must be byte-identical to MOCK_ISSUER. - access.log uses roll_disabled: rotation renames the file, impossible on a per-file bind mount; the file must be touched before the first `up` (a missing bind source becomes a directory and breaks the file logger). - oidc_redirect_host left empty — verified komodo builds the redirect from `host` alone. Mock sessions are in-memory: ANY compose reconcile recreates containers and wipes them; re-drive the portal login after any `up`. Knob verified live: 0.019s baseline vs 1.53s with DELAY_AUTH_MS=1500 on /auth/login/* (non-auth path unaffected — the sidecar cannot upgrade websockets, which is why only /auth/login/* routes through it).
…resh-clone CA flow, abort-aware delay Code-quality review on 2d76e56; all four items addressed, re-verified live. 1. CORRECTED a false mechanism claim (Caddyfile + README deviation 4): the previous text said Caddy's directive order sorts route/handle AHEAD of forward_auth, "letting requests reach core ungated" — that is INVERTED. Verified empirically by adapting the same site WITHOUT the route block: `caddy adapt` still compiles the verify proxy FIRST (forward_auth lives in the middleware group, which precedes handle/route/reverse_proxy). The route block stays — not as a bypass fix but as written-order determinism, pinning the evaluation order against directive-order-table drift across Caddy versions. Lesson: state a framework's ordering rule only after adapting the counterfactual, not after checking only the shape you chose. 2. Fresh-clone flow: the compose mounts ./oidc-dev/caddy-root-ca.crt, but the README's Run block only touched access.log — on a fresh clone the first `up` pre-creates the CA bind source as a DIRECTORY and the later export dies with "Is a directory" (the same gotcha the file already documents for the log). Run block now touches BOTH bind files up front, plus an rmdir recovery line for directories that already materialized (rmdir refuses non-empty, so the recovery cannot eat real data). An empty CA file is harmless-by-design: core 500s logins with UnknownIssuer until the export + `up -d core` recreate, which the documented flow performs anyway. 3. delay.mjs: after the delay await, `if (res.destroyed) return;` — a client that gave up during the wait (watchdog fire, Playwright timeout) no longer dispatches to core, so aborted requests stop consuming core's 5-attempt auth rate limit and stop polluting the exchange-counter metric the harness measures. 4. Caddyfile: documented that the komodo vhost's :8443 listener is symmetry only — nothing inside the compose network dials it today (core dials the portal via the issuer URL; the delay sidecar dials core:9120 directly); kept so both hostnames dial identically for a future in-network driver. Re-verification evidence: caddy validate → "Valid configuration"; AC2 unauth GET / → 302 to the portal authorize; DELAY_AUTH_MS=1000 → authed /auth/login/ExchangeForJwt 1.035s vs non-auth / 3.6ms; delay restored to 0.
One driver (compose/oidc-dev/verify.mjs) runs the 7-row scenario matrix of spec 6 against the live stack: it tails Caddy's JSON access log (offset baselined at process start, torn-write-safe by advancing only to the last complete newline) and instruments the browser with a pre-navigation init shim — TOKENS rows on mogh-auth-tokens-v1 transitions, LOADER rows on LoadingScreen visibility, FETCH rows carrying an auth-header tail fingerprint, plus the app's own ws on_login console line. Console rows are collected at the CDP level, which is the only vantage that survives mogh_ui's post-success location.replace; window-scoped page state does not. Fail-closed discipline: a check that cannot find its evidence throws (FAIL), fix-dependent checks are SKIPped unless --post-fix is passed (never vacuous PASS), and every scenario writes out/<scenario>.ndjson (gitignored) before printing `SCENARIO <name> PASS|FAIL` and exiting to match. Reality won over the early sketch in five places (all documented in the README's new "Access-log field notes" + "Deviations" + pre-fix baseline sections): - Caddy 2.11 redacts the authorization header VALUE too, not just Cookie — the earlier task note said values are visible. Presence stays on the wire rows; WHICH token was attached (the M1 discriminator) now comes from the fetch shim's tail-24 fingerprint instead. - Core answers a bad-signature jwt with 500, not the spec's assumed 401; the m1-seeded wire check accepts 401/403/5xx and prints observed statuses. - The seed is once-per-context: an unguarded addInitScript re-poisoned the store on the post-exchange reload and the dashboard's own reads came back stale-token. m1-seeded also self-manages DELAY_AUTH_MS=1500 — 6 says the knob exists to widen the race deterministically, not to rely on lucky timing inside the ~100ms pre-fix window. - hung pre-fix is NOT an eternal spinner at 15s: the LoadingScreen holds for the full delay (asserted: loader-on continuously, 15013ms observed) and the late 200 then converges as a slow success. The "eternal" arm belongs to the post-fix watchdog (--post-fix asserts the gate dropped before the late 200 plus the late-success redirect's landing); the README's the behavior matrix map row records the corrected pre-fix cell. - The /ws/update row is written at connection CLOSE, so success/latency close the context to flush the explicit 101 row; ws row timestamps never anchor time windows. Cookie-domain probe (the README caveat): headless Chromium accepts the mock's parent-domain Domain=oidctest.localhost _session cookie — the full login completes, so the *.oidctest.test + /etc/hosts fallback is NOT needed. Pre-fix baseline (the gate evidence): M1 reproduced (stale-token /user + GetCoreInfo fire inside the pending window, value-proven), M2 reproduced (sub-less jwt silently dropped; reload lands on /login), M5 reproduced (replayed redeem_ready → exchange 401 → no navigation, eternal spinner); isolation already holds pre-fix and stays as the the gate design regression guard. m2-forced installs its route interception in a preDrive hook: the exchange fires DURING the drive's second-entry cascade, so a route added after drive() never saw it (observed: the real jwt landed and the drop became a success row). Scenarios manage the delay knob themselves (printenv-probe, recreate on mismatch, restore 0 in finally) and `all` spaces rows 16s apart because the per-IP auth limiter is 5 attempts / 15s and every row shares Caddy's egress IP. Verified: `all` green end-to-end (7/7 PASS, exit 0), success exit 0, m2-forced --post-fix on stock code fails exactly on the missing drop flag (exit 1) — the the final verification pass flip target.
…safety, M1 regex Quality-second review round on a821d1d, applied as changes (not amend). The headline lesson: `(await fn()) !== false` was FAIL-OPEN for `undefined` — the exchange-error eternal-spinner check returned undefined when zero LOADER rows were captured and PASSed on no evidence. The engine now requires `=== true` (every check fn audited to return a boolean or throw), and the spinner check additionally throws "loader never observed" when the post-replay-goto LOADER window is empty — a missing instrument must look like a missing instrument, not like a green row. The M1 wire regex was silently half-blind: `/^\/(user|read)\//` requires a trailing slash, so `GET /user` never matched and "observed 1" was really only GetCoreInfo. Proven by the re-run: the fixed `(\/|$)` form reports observed 2. The m1 post-fix arms (absence checks) are now guarded by the m2-style instrumentation-liveness rule: an ABSENCE may only pass when the shim provably captured >= 1 FETCH/TOKENS row — a dead shim must fail loudly, not vacuously. The fresh-token check also parses the TOKENS store JSON now and requires `current` present AND != "stale-user" (a null/clear row is not evidence of the transition; the detail string is console text, so it is sliced at the first brace before JSON.parse). Knob crash-safety: SIGINT mid-hung used to leave DELAY_AUTH_MS=15000 set and the next delayMs:null scenario would silently drive with a leftover knob. Every scenario now normalizes its EXPECTED knob (def.delayMs ?? 0) at start — loudly when a non-zero leftover was found — SIGINT/SIGTERM restore 0 via a synchronous recreate before exiting, and a failed restore in the finally block FAILs the scenario verdict (an unverifiable knob state poisons the next run; it must not just log). Also: the eternal-spinner predicate is scoped to LOADER rows after the replay goto (leg 1's spinner could satisfy it), scenario delay values moved onto the defs (runner ternary + hardcoded check-name strings killed), unknown-scenario errors list the valid names, spawn errors include `r.error`, and exits go through `process.exitCode` + natural exit — a hard process.exit can truncate buffered stdout when piped, and the verification suite parses these lines. Spec amendments landed with this commit (three cells only, no other churn): 6's request-bound unit gains "OR an auth-bearing 5xx" and the M1 assumption "the server 401s it" becomes "the server rejects it (observed 500 with a bad-signature token)" — both dated to the harness observation — and the behavior matrix's M1 row pre-fix cell reads "stale-token rejections (observed 500s)". No-regression evidence (all exit 0, knob verified 0 after): success 7/7 PASS, m1-seeded 4/4 PASS (observed 2 stale rows, statuses 500), exchange-error PASS with the scoped spinner check (last state: on).
Reconciliation: the reviewed spec amendment put auth-bearing 5xx into 6's WINDOW-BOUND unit, but the code predicate (unauthOrFail) and the spec's RESIDUAL sentence still carried the 401/403-only unit — two units on paper, one predicate in code. Post-fix, an escaped stale read manifests as an auth-bearing 500 on this core, which the residual window would have read as zero exactly where the design claims zero. unauthOrFail now counts "unauthenticated OR 401/403 OR an auth-bearing 5xx" (and it is the ONE predicate: windowBoundChecks and residualRows both share it, so both windows inherit the extended unit automatically). The /auth/login/* header-less exclusion stays the first disjunct, unchanged. Check-name labels now name the unit they count — a label narrower than its predicate is the same drift in miniature. Spec: the 6 zero-residual sentence and 11's residual bullet both read "...unauthenticated-or-401/403 OR auth-bearing-5xx..." now, with the reviewer's rationale clause — a late exchange failure inside the residual window SHOULD count, because it means something re-triggered the exchange, which the fix forbids. No-regression evidence (exit 0 both, knob 0 after): success window rows 0/0 under the extended unit; exchange-error residual 0/0 — stock code's post-settlement rows are none, so the stricter unit costs nothing today and is armed for the post-fix world where it is the difference between catching and missing an escaped stale read.
The pre-fix gate: run all 7 scenarios against the stock v2.3.3 UI (fresh runs, not the harness suite's cached ndjson) and record which candidate mechanisms actually reproduce. Verdict: M1, M2, M5 reproduced with wire-level evidence -> PROCEED to Phase 2. Why fresh runs: the harness suite's outputs were produced by earlier revisions of the suite; a baseline that quotes cached rows would attest evidence the committed verify.mjs never generated. the harness suite's out/*.ndjson was moved aside before the first run, and every excerpt in BASELINE.md comes from the fresh files (individual runs, >=16s apart, mirroring `all` mode's limiter spacing). Findings worth carrying forward: - M1 (m1-seeded): stale-token /user + /read/GetCoreInfo fire DURING the pending window (1.53s before the exchange 200) with the seeded value proven by the fetch shim, rejected with 500 - not the 401 the spec originally assumed. The shim also shows the stale token attached to the ExchangeForJwt call itself: mogh_ui attaches whichever jwt is current to ALL API calls, which is why the residual/window unit counts auth-bearing 5xx, not just 401/403. 7.5's read-deferral premise is CONFIRMED against this evidence. - M2 (m2-forced): upstream 200, every TOKENS row stays null, sub-less jwt absent from the store; the only user-visible signal is the login chunk loading after the reload. Silent, exactly as the flag contract assumes. - M5 (exchange-error): replayed redeem 401s (one-shot pending state + one limiter attempt burned) -> no navigation, last loader state "on" indefinitely. - hung is a SLOW SUCCESS pre-fix, not an eternal spinner: the spinner holds the full 15037ms round-trip and the late 200 converges. A finite delay sidecar cannot produce the eternal arm; it belongs to the post-fix watchdog run. The README's the behavior matrix row->scenario map already carries the corrected pre-fix cell; BASELINE references it instead of restating. - M3 does NOT reproduce against the mock: its portal session survives token redemption, so the post-exchange reload sails through the gate (zero /verify rows in any ndjson). Recorded as a mock limitation, NOT an exoneration - the reporter's Authelia-class gateway may consume the session at the redemption instant, so M3 stays live for the upstream report (7.4 contingency). Assumption encoded: PASS pre-fix means mechanism-reproduction assertions only; FIX-DEPENDENT checks SKIP (never PASS) without --post-fix, so a green pre-fix baseline cannot silently certify fix behavior that does not exist yet.
…pts pinned to live ndjson Spec review found BASELINE.md quoting the m1-seeded exchange row as "auth":false when out/m1-seeded.ndjson:79 reads "auth":true. Load-bearing: the adjacent narrative (and the previous commit message) claims the stale token was attached to the ExchangeForJwt call itself, and the wire row's auth:true is that claim's only wire-level corroboration — the fetch shim alone carried the value proof. The quote was a transcription slip from the success row's shape (that one really is auth:false); the fix makes the excerpt match the source byte-for-byte and the narrative now names the row's auth:true as the corroboration explicitly. Lesson encoded: never transcribe evidence excerpts by hand — the same error class the suite itself guards against (checks fail closed instead of defaulting to success). A field-by-field excerpt-vs-ndjson verifier now ran over all 26 quoted rows (match on kind + cited ts + every shown key:value): 26/26 OK, which is the acceptance bar for any future edit of this file. Also in this commit: - m1 tokens transition row ts_ms corrected 1790910699009 -> 1790910699010 (the old number is the inner detail timestamp, not the row's ts_ms). - m2 login-chunk row ts un-rounded to the source's 1790910746.5997264. - Header elision note widened: excerpts elide oversized jwt strings AND fields with no evidentiary weight (host, uncited ts/ts_ms, duration_s); url, status, auth, and cited timestamps are never elided — so a reader knows exactly what a "…" is allowed to hide. - exchange-error section re-run and re-quoted: out/ is a rolling cache and the batch-run ndjson (00:12-00:14) was overwritten by an out-of-band re-run at 00:25, so the quoted rows no longer existed on disk. The section now cites a run of the same committed suite performed by this task (00:30:25, stdout captured, same PASS verdict: leg-1 200, replay 401 auth:true, loader "on" indefinitely), with a provenance note explaining why.
First fix-phase code for moghtech#1665 (per the reviewed design; jwt redeem race into state komodo can render against: idle|pending| settled-ok|settled-failed, armed deterministically via MutationCache config hooks (which run inside execute — no subscription race), settled via a filtered cache subscription, bounded by a 12 s watchdog so a black-holing proxy cannot hang the gate pending forever. Module is imported by nothing yet (the main.tsx/router wiring lands next); tsc checks it through tsconfig include. Why the settled-failed hygiene lives in setState, not an effect: query-core dispatches cache events synchronously in the task that triggered them, so by the time the listener flips state, React has not re-rendered — remove_all() inside setState therefore lands before any provider can read the still-present token. Router's effect runs after WebsocketProvider's flip render, whose hasJwt would still read stale-true, letting exactly one stale-token request escape after settlement (the baseline M1 residual). Render phase strictly precedes effects; the listener's synchronous slice also completes before location.replace tears the document down. Why every hook/listener body is exception-free: mutation.cjs awaits the config onSuccess BEFORE mogh_ui's token write and wraps only error-path hooks — one throw from our success hook would convert a 200 exchange into the very silent-drop (M2) this design exists to surface. Likewise a throw inside the cache notify pass breaks that whole notify cycle. All risky operations (storage reads/writes, remove_all, watchdog arming) go through safe(), which logs and swallows; sessionStorage is feature-probed once at module scope and degrades to console-only signals when absent. Why the evidence flag never carries the jwt: sessionStorage outlives remove_all — a persisted token copy would duplicate the credential the fix works to contain. The flag is {t, phase: "ok"|"drop"} only; loop safety rests on the 15 s TTL + clear-on-read, not on page-lifetime assumptions. The settlement listener rewrites it on settled-ok (refreshing t so the late-success TTL measures from settlement) and upgrades to "drop" when the exchanged jwt string-matches no tokens[].jwt entry in localStorage["mogh-auth-tokens-v1"] — decode-free M2 detection. Two tsc-forced deviations from the design's verbatim module (only these; diff-verified against the design's code block): - isExchange's structural type needs mutationKey?: readonly unknown[]: query-core 5.102.4 types MutationKey as ReadonlyArray, and a readonly array is not assignable to a mutable unknown[] — the filter only reads [0], so readonly is faithful. - onSuccess's first param is _data, not data: tsconfig sets noUnusedParameters and the hook intentionally ignores the payload (it records pre-reload evidence, it does not read it). Verified: cd ui && yarn build clean (tsc strict + vite), module unused. Co-Authored-By: Claude Code <noreply@anthropic.com>
…e M2 signal Applies the code-quality review of 8d097c9 as requested; yarn build (tsc strict) clean after. - Narrow the cache event's action instead of casting it. NotifyEventMutationUpdated.action is a REQUIRED discriminated union (ContinueAction | ErrorAction | FailedAction | PendingAction | PauseAction | SuccessAction — query-core 5.102.4 .d.ts); the old `(event.action as { type?: string } | undefined)?.type` was a compile-time lie claiming action could be absent with a string type. `if (event.action.type === "success")` narrows to SuccessAction, whose `data` is the exact success-dispatch payload (identical to mutation.state.data per mutation.cjs's success reducer), so the M2 jwt check now reads event.action.data. No behavior change — honesty gain. Note: the design's reference implementation carries the same cast — plan defect inherited; the plan document is not edited in this commit. - Surface the M2 drop verdict when sessionStorage is unavailable. writeFlag's degraded path was fully silent, but the flag contract + the behavior matrix's M2 row make this module the drop detector: degraded mode now emits a console.error for phase "drop" (the "ok" phase stays silent — it is not a verdict worth alarming on). - M2 corrupt-parse logging now includes the RAW stored value, per the flag contract ("log the raw value to console"). Implemented by passing the raw string into safe()'s failure label (`M2 parse (raw: …)`) after a guarded getItem read: a label only prints on failure, so the happy path builds a short string it never logs — no nested try/catch, no second log call. - Hoisted one module-scope `subscribe` for both useSyncExternalStore calls: kills the duplicated closure and the per-render closure churn (React compares subscribe identity to decide resubscription). - Documented WHY on the constants: WATCHDOG_MS exists because browser fetch has no default timeout (else a black-holing proxy hangs the gate pending forever); FLAG_TTL_MS is "just over the watchdog" AND is measured from the settlement-time rewrite (the flag contract/the late-success redirect), not from the original arm — a subtlety easy to "simplify" into a bug. Co-Authored-By: Claude Code <noreply@anthropic.com>
Komodo owns the cache config; the redeem-gate subscription attaches once
at module scope.
WHY module-scope initRedeemGate(client), before ReactDOM.createRoot:
query-core cache listeners fire synchronously inside the mutation
dispatch's own task. The login flow dispatches the ExchangeForJwt
redeem during render, so a subscriber registered from an effect would
attach only after the commit — it can never observe the render-phase
fired redeem, deterministically. Module scope has no effects, which
also makes it StrictMode-immune by construction: double-invoked effects
can't double-subscribe what never ran as an effect. A module-scope
subscriber lives for the page's lifetime, so the subscription count is
exactly one for exactly as long as the document exists.
WHY mutations: { retry: false } explicit: query-core's pinned default
is retry 0, so behavior is identical today — but this file is a
verbatim-reference for implementers and humans alike, and the gate's
settled-failed path assumes a redeem attempt terminates. Stating the
invariant beats implying it; explicit beats implicit in a document this
load-bearing. queries: { retry: false } preserved verbatim.
The MutationCache is constructed here rather than inside redeem-gate.ts:
the cache belongs to the app, the gate only supplies hooks config, so
Komodo keeps ownership of the wiring and redeem-gate stays importable
without side effects. yarn build (tsc && vite build) clean.
Two review fixes, both cosmetic — no behavior change: - The single-line MutationCache/QueryClient/QueryClientProvider import exceeded prettier's print width; `yarn prettier --check src/main.tsx` failed (no CI gate covers the ui dir, so it shipped). Now split to the canonical 5-line form prettier generates; check passes. - The initRedeemGate comment's second sentence restated the delivery fact (cache listeners fire synchronously in the dispatch task) that redeem-gate.ts:8-10 already documents where it belongs. Duplicated rationale is a drift hazard: one copy gets updated, the other rots. Kept only what is unique at this call site (module scope, once, StrictMode-immune) plus a pointer to the owning doc. yarn prettier --check src/main.tsx and yarn build (tsc && vite build) both clean.
Router now keys its LoadingScreen on the redeem-gate settlement state
instead of mogh_ui's jwt_redeem_ready URL bit, and converges a
settled-failed exchange in-document (param strip + notification) before
falling through to RequireAuth (which routes to /login?backto=... via
the existing flow).
WHY the URL bit is dead as a gate: jwt_redeem_ready is just
search.get("redeem_ready") === "true" read per render, but after the
exchange handler initiates location.replace, location.search is STALE
until the new document commits — a URL-derived gate either flickers or
sticks. The mutation state is the only live truth. Ordering is
load-bearing: mogh_ui's useAuthState fires the redeem synchronously in
its body during THIS render, so calling useAuthState() BEFORE
useRedeemState() means the config onMutate (redeem-gate.ts) has already
flipped the gate to "pending" by the time the snapshot is read — render
moghtech#1 is the LoadingScreen, no login flash. That replaces the old
"Handle exchange token loop" comment, which described the symptom, not
the mechanism.
WHY token hygiene is NOT here (review correction): an effect-ordered
LOGIN_TOKENS.remove_all() would run after the parent provider's flip
render, whose hasJwt still read stale-true — one stale-token request
would escape after settlement. remove_all lives in redeem-gate.ts's
setState, synchronously before any render can observe the flip. This
effect is UI convergence only: strip redeem_ready|totp|passkey (names
verified against mogh_ui's useAuthState: search.get("redeem_ready"/
"totp"/"passkey")) via history.replaceState, then notify.
WHY the notification survives: replaceState is same-document — no
navigation, no reload — so the React tree and the Mantine Notifications
portal stay mounted and the toast actually renders. The router itself
is undisturbed: react-router listens to popstate, not replaceState, so
no spurious re-render loop, and the subsequent fall-through to
RequireAuth navigates client-side. Stripping totp|passkey too matters:
left in the URL they would keep useAuthState reporting
passkey_pending/totp and pin Router on <Login passkeyIsPending> instead
of letting the settled-failed fall-through reach /login.
Formatting-only adaptation: prettier rewrapped the
for-loop header (80-col); prettier --check and yarn build (tsc strict)
both clean. Part of moghtech#1665.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…t gate
ONE mechanism, per the reviewed design: defer the provider's READS while a
redeem is pending. WebsocketProvider holds `gateOpen =
useRedeemGateOpen()` and passes it as `enabled` to useUser + useRead
("GetCoreInfo"); the connect effect is untouched — it keys on
`user && disable_reconnect !== undefined`, both of which the read gating
defers, so a separate gateOpen term there would be dead weight. Early
returns were not an option: a gate flip must never change hook counts
(rules of hooks) — `enabled` composition is the only safe lever.
WHY composition order is the whole fix (enabled-composition drive-by): `enabled`
computed BEFORE `...config` is replaceable, because object spread copies
EXPLICIT keys — including `enabled: undefined`, which React Query
resolves as enabled (queryObserver: `resolveQueryBoolean(enabled) !==
false`). That was not hypothetical: useAllResources passes exactly that
key on all 11 of its list reads, so pre-fix those reads lost the jwt
gate whenever logged out, and usePermissions hands a truthy `enabled`
for non-admins (harmless only because a loaded user implies a jwt). The
old `!== false` check also discarded function-valued predicates
outright. Now the gate is computed AFTER the spread and composes every
caller shape (323 useRead call sites; useUser gained an optional config
param so its two gate consumers compose identically):
boolean/undefined -> `(caller ?? true) && hasJwt`
function-valued -> `(q) => hasJwt && caller(q) !== false`
hasJwt is always ANDed — no caller can switch the gate off, while an
explicit `enabled: false` still wins.
WHY normal loads are byte-identical: the gate predicate is
idle-means-open (only the ExchangeForJwt mutation itself can leave
idle), so gateOpen is true on every non-redeem load and the composed
values reduce to exactly today's `enabled: hasJwt` — audited at each
existing call shape (no config, explicit false, truthy, explicit
undefined).
Typing adaptation (documented per plan): useUser's config mirrors React
Query's own QueryBooleanOption with the EXACT inferred generics of the
GetUser query. The tempting `QueryBooleanOption<Types.User>` (TQueryKey
defaulting to QueryKey = readonly unknown[]) fails tsc: TQueryKey is
invariant through options.queryFn (QueryFunctionContext hands the key
back out), so the widened key is not assignable to the inferred
`string[]`. Restating the inferred generics keeps tsc validating the
composed callback's real parameter type instead of a hand-waved one.
Gates: yarn build (tsc strict + vite) and prettier --check clean on both
files. Part of moghtech#1665.
Co-Authored-By: Claude Code <noreply@anthropic.com>
login.tsx now reads the redeem evidence flag (the flag contract) on mount and closes the two loose ends of the watchdog race (the late-success redirect): - fresh phase:"ok" + a token in the mogh-auth store -> one-shot location.replace(backto ?? "/"), recovering the session UX after the watchdog already showed "Login didn't complete". - phase:"drop" (exchange 200'd but jwt never stored) -> red notification "Login succeeded but the session could not be stored" / "Please log in again." — the M2 case stops being silent. Why decode-free presence + TTL suffices: the flag is written only by an ExchangeForJwt settlement and its TTL restarts AT settlement (the flag contract), so "fresh ok" means a 200 exchange landed within the last 15s. The watchdog path clears the token store (settled-failed hygiene in redeem-gate.ts, run synchronously before the flip) BEFORE the late success re-stores the jwt, so presence at mount cannot be a pre-race stale token. The exchanged jwt itself never survives the reload, so presence is the strongest post-reload signal available. Loop safety: the flag is consumed BEFORE navigating, so the next document finds nothing — including when mogh_ui's own sanitizeQueryInner reload wins the race against our replace (sessionStorage survives a reload, and the settlement-time TTL covers the trip). StrictMode's double effect and a corrupt token store are both safe: the former finds the flag already gone; the latter throws -> caught -> treated as absent -> no redirect. Window APIs instead of router hooks — this is load-bearing, not style: router.tsx renders <Login> for the passkey/totp branch BEFORE <BrowserRouter> mounts (router.tsx:79 return vs :83 provider), where useNavigate/useSearchParams throw for lack of router context. Reading location.search and redirecting via location.replace mirrors mogh_ui's own maybeNavigate, keeping this path identical to the in-form redirect. redeem-gate.ts: TOKENS_KEY becomes exported MOGH_TOKENS_KEY so the jwt-presence check and the M2 storage read share one key constant. Gates: yarn build (tsc strict + vite) and prettier --check clean on login.tsx. Part of moghtech#1665. Co-Authored-By: Claude Code <noreply@anthropic.com>
The @mantine/notifications default autoClose is 4000ms, so "Login
succeeded but the session could not be stored" dismissed itself by
~T+5s after mount. That defeats the point of surfacing M2 at all — the
spec's goal for this message is non-silence, and a must-act prompt
("Please log in again.") that self-destructs is worse than no toast:
the user saw *something* flash and has nothing to act on.
It also deterministically breaks the fix-dependent probe: verify.mjs
(the final verification pass) re-checks at ~T+8s for either the flag or the body text on
screen — the clear-on-read removes the flag and the 4s timer removes
the text, so both probes miss and the m2-forced run reports FAIL
against a correct fix.
Sticky toast (autoClose: false) is the standard remedy: the user can
dismiss it after acting; automated observers get an indefinite window.
Gates: yarn build (tsc strict + vite) and prettier --check clean.
Part of moghtech#1665.
Co-Authored-By: Claude Code <noreply@anthropic.com>
Completes the post-fix half of BASELINE.md (the final verification pass): all seven scenarios
re-run with --post-fix against a core image rebuilt from HEAD (served bundle
hash-verified against the host `yarn build`), fix-dependent arms ENFORCED.
Verdicts: 6/7 green — m2-forced 5/5 (drop flag surfaced in-document),
exchange-error 3/3 (in-document convergence, exactly 1 document load,
zero-residual 60s), hung 5/5 (watchdog dropped the gate 3.0s BEFORE the late
200, then the late-success redirect recovered the late success to /), success/latency/isolation
regression guards hold. m1-seeded FAILS its 7.5 arm — a real result,
recorded with root cause, not papered over:
The two stale-token reads escape ~6ms BEFORE the exchange dispatch. mogh_ui
arms the redeem in Router's render body, but the gated reads live in
WebsocketProvider — Router's PARENT. React renders parent-first, so the
provider's observers commit enabled:true before any render-phase flip inside
a child can exist. The deferral IS in force during actual pending (zero
/user|/read fetches in [dispatch -> 200]); it is structurally one render
phase behind the arm. Lesson: a render-phase gate can only cover components
that render AFTER the arming component — gating an ancestor from a
descendant's mutation callback cannot close the same-commit hole. Minimal
remedy sketched in BASELINE (module-scope pre-arm on location.search,
mirroring mogh_ui's own jwt_redeem_sent guard); NOT attempted here — fix
code is the gate-implementation scope.
Also corrects the baseline's premise reading (the stale reads never fired "during
pending" even pre-fix — the check window simply includes the pre-arm gap), and
adds the README staleness note marking BASELINE.md canonical over the
first-batch PRE-FIX BASELINE SUMMARY numbers.
Five upstream issue drafts appended (mogh_ui add_and_change falsy-sub silent
drop; LOGIN_TOKENS module-init snapshot with no storage-event re-sync; redeem
failure with no in-document convergence + sanitizeQuery full-page-reload
control flow; core per-IP limiter keying behind proxy egress; M3 forward-auth
interplay as an open question, mock-limited).
Build-context gotcha for the rebuild: ui/node_modules/komodo_client (host
`yarn link` state, gitignored) resolves outside the repo, and podman's context
extractor refuses symlinks escaping the extraction root ("invalid symlink").
Moved aside for the image build and restored after — host builds still need
it. A root .dockerignore would fix this properly but is outside this branch's
diff scope.
… read gap
the final verification pass's post-fix m1-seeded run exposed a real gap in redeem-gate.ts. Root
cause (BASELINE m1 deep-dive, reproduced here): mogh_ui arms the redeem in
ROUTER's render body (useAuthState), but the 7.5 gated reads live in
WEBSOCKETPROVIDER — Router's PARENT. React renders parent-first, so the
provider's first render commits enabled = gateOpen(idle→true) &&
hasJwt(stale-true) before any child-render flip can exist; the store change
is only observed after commit, leaking the pre-arm stale-token reads (~6 ms,
2 reads in the final verification pass's run). 7.5's deferral works from the flip onward — this
closes the leak BEFORE the flip.
Fix — module-scope pre-arm, mirroring mogh_ui's own jwt_redeem_sent URL
guard: at import (guaranteed pre-render by main.tsx's import), if
location.search carries redeem_ready=true, initialize state = "pending" AND
arm the watchdog. The provider's FIRST render already sees the gate closed.
The subsequent real onMutate → setState("pending") is idempotent (equality
guard) and its watchdog arm a no-op (watchdog already set). Normal loads (no
param) are untouched — idle, byte-identical. The pre-armed watchdog is also
defensive: if mogh_ui version drift ever stops firing the mutation, the
window still converges to settled-failed (with its remove_all hygiene)
within WATCHDOG_MS instead of hanging pending forever.
Spec: the gate design gains the "Pre-arm at module scope" addendum (mechanism +
parent-first rationale + defensive watchdog).
Harness checker fix included (compose/oidc-dev/verify.mjs, one predicate):
the m1 value arm's shim filter matched ANY fetch row carrying the stale jwt
tail before the wire exchange ts. The redeem request itself legitimately
carries the stale jwt — it IS the credential being exchanged — and the shim
logs that row at DISPATCH (ex−1531 ms), necessarily before the wire
completion ts, so once the real leaks are closed the exchange self-matched
and false-failed the absence arm (my first post-pre-arm run: wire observed
0 — the pre-arm worked — yet the value arm FAILED on the exchange dispatch
row alone). The filter is now scoped to the same ^/(user|read) paths the
wire arm scopes: faithful to the assertion's own contract ("VALUE confirmed
on THOSE requests") — pre-fix behavior unchanged, post-fix absence now
meaningful. verify.mjs's "byte-identical to fa53da3" pledge in BASELINE
describes the final verification pass's batch and is now superseded by this documented fix;
BASELINE's m1 resolution write-up belongs to the final verification pass's owner.
Verified: cd ui && yarn build clean; core-ui image rebuilt (yarn-link
symlink moved out of the build context and restored); node
compose/oidc-dev/verify.mjs m1-seeded --post-fix → SCENARIO m1-seeded PASS
exit 0 — exchange 200; wire stale reads 0; value-absence PASS; fresh token
stored.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…; m1 green the final verification pass completion after 641f966. Batch 1 re-ran the gate-sensitive rows with --post-fix on the final image (28f89a5be8af, HEAD 641f966, container image-sha verified before the runs): success 7/7, m1-seeded 4/4 (first-hand re-verification of a live verification pass — wire arm observed 0; the ONLY fetch in [doc load -> exchange 200] is the exchange's own dispatch, which legitimately carries the stale credential), hung 5/5 (LoadingScreen up from doc+123 ms — the pre-arm visibly engaged; watchdog at doc+12105 ms, 3.0 s before the late 200; the late-success redirect landed /), isolation 3/3. m1-seeded section rewritten FAIL -> resolved: batch-0 failure + parent-first root cause kept as the discovery narrative, the 641f966 pre-arm fix and the companion checker scoping fix (the value arm's shim filter self-matched the exchange's own stale-jwt dispatch row — that row IS the credential being exchanged) documented, batch-1 evidence added, 11 m1 criterion marked met. latency / m2-forced / exchange-error stay on their batch-0 (pre-pre-arm build) runs with a per-row dagger + explicit reasoning: the pre-arm changes only WHEN the gate closes on a redeem_ready document, and none of those rows asserts anything about that window (latency = success + the delay knob, which re-ran green; m2 asserts settlement-time drop detection; the exchange-error convergence exercises the same settled-failed path hung's watchdog re-ran green). Suite-provenance bullets now describe both batches and supersede the byte-identical-to-fa53da3c0 pledge for batch 1. Verdict: 7/7 green — spec 11 "full suite green post-fix" met. Upstream issue drafts untouched, as directed.
The design documents (docs/superpowers) that this branch carried while
the work was in flight are stripped before upstream review. That would
leave every internal pointer in shipped files dangling: `spec 7.x`
anchors, plan-task numbers ("Task 3", "the final verification pass"),
and the internal "M2" mechanism label. The WHY content is preserved
everywhere — only the pointers were replaced with self-contained
wording. Comments and log strings only; no behavior change.
Lesson: a scrub keyed on one grep pattern silently misses hyphenated
and plural spellings ("Task-3", "Tasks 1-2"). Sweep
`grep -E "Task[- ]?[0-9]"` before declaring the job done.
elifarley
force-pushed
the
komodo-oidc-login-jwt-redeem-race-fix-1665
branch
from
October 2, 2026 09:10
c7273d0 to
86348d5
Compare
…r rows on the final build
Round-7 review follow-ups on the redeem-gate work. Two themes: make the
mogh storage-key coupling self-diagnosing, and replace an evidence
argument that did not survive scrutiny with fresh runs.
Dagger rows re-run on the final build (batch 2) — the † transfer
argument is retired
The previous BASELINE claimed the batch-0 rows for `latency`,
`m2-forced`, `exchange-error` could stand because "their asserted
mechanisms do not touch the pre-arm window". That was false for
`exchange-error`, and batch 0's own ndjson refutes it: the replay
document dispatched auth-bearing `GET /user` + `POST /read/GetCoreInfo`
BEFORE the redeem dispatch (fetch shim −2 ms; wire completions
1790918528.2028 vs the 401 at 1790918528.2185 — the −16 ms), both
Referer-pinned by Caddy to `/?redeem_ready=true`. That IS the
parent-first gap, firing in a scenario the text claimed was untouched —
now recorded as PRE-ARM PROOF in the m1 narrative (independent
confirmation from a second scenario).
All three rows were re-run with `--post-fix` on the final image
(28f89a5be8af, id re-verified on the running container), each PASS:
latency 8/8 (round-trip 2058 ms >= 2000 ms), m2-forced 5/5 (drop flag
surfaced in-document), exchange-error 3/3 (in-document convergence +
zero-residual 60 s). Batch 2 is canonical; batch 0's copies stay as
history with corrected framing. The corrected two-part transfer
argument is kept for future re-baselines: (1) the pre-arm defers the
redeem document's own provider reads (batch-0's rows prove the gap was
real; batch-2's exchange-error shows zero fetch rows before the
dispatch); (2) post-settlement, `enabled = gateOpen && hasJwt` keeps
reads off because settlement hygiene empties the store.
Schema-guarded drop verdict (redeem-gate.ts)
The silent-drop check had TWO independent parsers of
`mogh-auth-tokens-v1` (one here, one inline in login.tsx) — drift bait.
One `readMoghStore()` helper now owns the parse (absence and corruption
both -> undefined; parse failures log the RAW stored value), and
`hasStoredJwt()` is the exported schema-shaped presence check login.tsx
uses, deleting the second parser.
The verdict itself now fails safe against upstream drift: `phase:"drop"`
requires the exchanged jwt's absence to be TRUSTWORTHY. Two shapes of
doubt log "mogh token store shape unrecognized — silent-drop detection
unavailable" and write "ok", never a false drop: (a) the pinned key
parsed into bytes that are not the `{ tokens: [...] }` schema, (b)
`LOGIN_TOKENS.jwt()` is truthy (a successful add_and_change updates that
closure in-process) while our read of the pinned key lacks the jwt — an
upstream key/schema rename under ^1.7.1 writes a DIFFERENT key, so our
view is stale by construction. The m2 shape is deliberately NOT doubt:
plain store absence with an EMPTY library view is exactly the silent
drop (add_and_change swallowed the token; nothing was written anywhere),
so it still verdicts "drop". That asymmetry is the whole detector.
hooks.ts: one `composeEnabled(hasJwt, callerEnabled)` helper replaces
the two diverging post-spread ternaries in useUser/useRead (same truth
table — caller `false` wins, `hasJwt` always ANDed; generic in the RQ
query type so each site keeps validating the function-valued form).
router.tsx: the settled-failed notification is latched one-shot per
document (`failureNoticeShown` at module scope, set before showing) —
StrictMode's dev double-mount and a late onMutate re-entering
pending -> settled-failed could each re-show it. The URL strip stays
unlatched (idempotent); a NEW document legitimately re-arms.
compose yaml: core's API port binds `127.0.0.1:9120:9120` — posture
parity with the Caddy/mock loopback-only bindings (harness-internal
surface, never a LAN listener; host health check unaffected).
Provenance honesty: every commit hash cited in BASELINE.md
(`fa53da3c0`, `9d9a38a88`, `49802408e`, `641f966da`) is a pre-rewrite
hash, unreachable from the PR branch's current history. Annotated once
per provenance location, pointing at the fork's
`komodo-oidc-login-jwt-redeem-race-fix-1665-archive` branch.
Upstream drafts: added (f) — mogh_ui's `backto` query param flows
unvalidated into `location.replace` (open redirect; komodo's
late-success redirect mirrors the shape); ask is same-origin validation
upstream.
Declined on purpose: no unit-test framework / `resetForTests()` seam.
The upstream PR must stay within the maintainers' hygiene (the repo has
no UI test infra), and a test seam without a runner is dead code — the
state machine is cheap to verify through the harness suite instead. The
underlying need is recorded where it belongs, as a one-line upstream ask:
mogh_ui should consider exporting the token-store key and redeem
lifecycle seams so hosts can unit-test redeem integrations without a
full harness.
Harness lesson (cost several aborted runs): compose v5 fingerprints the
project environment, so ANY `DELAY_AUTH_MS` change makes `up -d delay`
recreate mongo+core too; and this podman does not schedule healthchecks
for Docker-API-created containers, so core's `service_healthy` gate on
mongo blocks forever in `(starting)`. A manual
`podman healthcheck run komodo-oidc-dev-mongo-1` (0.5 s, exit 0) flips
it and unblocks the gate. If a verify scenario hangs right after
"delay knob active", check for a `(starting)` mongo first.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…quires the tokens array
Two spec-review one-liners on the new store code, both in the same corner:
what readMoghStore is allowed to return.
1. JSON.parse("null") SUCCEEDS — the previous code cast the parse result
straight to MoghTokenStore, so a store holding the literal `null`
came back as a "parsed" store. The drift clause then evaluates
`store.tokens` OUTSIDE safe() (`store !== undefined && !Array.isArray
(store.tokens)`), and `null.tokens` throws in the mutation-cache
subscription — the watchdog still bounds the gate, but the settled-ok
flip is skipped and the toast path never runs. Non-object parses
("5", '"str"') were silently mis-typed the same way. The parse result
is now guarded: `!parsed || typeof parsed !== "object"` folds into the
corruption path (undefined, raw value logged — safe()'s label never
fires here because these parses don't throw, so the log lives at the
guard). Callers can now dereference store.tokens knowing store is a
non-null object.
2. hasStoredJwt counted a token entry via `tokens?.length > 0`, which
returns true for `{"tokens":"junk"}` — less strict than the parser it
replaced in login.tsx (the old inline code checked
Array.isArray(parsed?.tokens)) and divergent from the verdict guard's
Array.isArray. Now `Array.isArray(store?.tokens) && length > 0`, so
the late-success redirect gate and the drop verdict agree on what
"shaped" means.
INFO: the upstream seam ask in BASELINE now carries the store schema
(`{ current, tokens: [{ user_id, jwt }] }`) — an ask that names the key
but not the shape forces every implementer back into node_modules.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…t corruption
Quality-review fixes, both in the same corner: the schema guard ran AFTER
the dereference it guarded, and absence was logged as corruption.
1. The guard ordered wrong. `store?.tokens?.some(...)` short-circuits only
on nullish: for `{"tokens":"junk"}` the expression became
`("junk").some` -> undefined -> CALLED -> TypeError; for
`{"tokens":[null]}` the callback's `t.jwt` threw. Either way the throw
escapes inside the mutation-cache subscription: writeFlag and the
settled-ok flip are skipped, leaving the gate pending until the
watchdog. The shape is now derived first via Array.isArray
(`const tokens = Array.isArray(store?.tokens) ? store.tokens : undefined`),
the scan runs on `tokens` with a nullish-entry guard
(`!!t && t.jwt === jwt`), and drift is `store !== undefined &&
tokens === undefined` — no throw path exists. Both drift clauses from
the previous commit are preserved (parsed-but-unshaped; library's
truthy jwt() contradicting our absence — the key-rename case), so the
verdict semantics are unchanged: drift -> console.error + "ok", never a
false drop; absence + empty library view -> still the m2 drop.
2. readMoghStore logged plain ABSENCE as corruption. raw === null (the
key was never written) hit the `!parsed` guard and emitted "JSON
null/non-object is not a store; treating as corrupt" — crying wolf on
exactly the canonical m2 drop the detector exists to surface, with
`raw: null` indistinguishable from literal "null" bytes. Absence (and
a failed read, already logged by safe()'s label) now returns quietly
BEFORE the parse; only genuinely-parsed garbage gets the corruption
log. Both console.errors name MOGH_TOKENS_KEY so support can inspect
storage without reading the source.
Trace, all shapes (hand-verified against this revision): m2 null store ->
quiet undefined -> drop, no logs; happy -> "ok", no logs; key-rename
drift (stale-shaped or absent old key, lib truthy) -> drift error + "ok";
`{"tokens":"junk"}` -> drift error + "ok", NO throw; `{"tokens":[null]}` ->
no throw (nullish-entry guard). Minor riders: useUser's call-site comment
trimmed to the load-bearing after-the-spread rationale (composeEnabled's
doc comment already carries the semantics); BASELINE batch-2 UI label
reads "(batch 1, re-verified for batch 2)".
Co-Authored-By: Claude Code <noreply@anthropic.com>
…-failure remove_all
The settlement path ran LOGIN_TOKENS.remove_all() on EVERY failure — the
module-scope pre-arm watchdog (armed by ?redeem_ready=true on any path) and
the cache error action alike. mogh_auth_client's remove_all writes
{current: undefined, tokens: []} to the PROFILE-WIDE store, so a failed
redeem silently logged every tab of the browser out of a perfectly valid
pre-existing session: a stale ?redeem_ready=true bookmark, a single-use code
replay, a watchdog on a black-holed proxy — all destroyed a session the
redeem never created.
Why removal (rather than scoping) is the correct contract:
- mogh's add_and_change is all-or-nothing (dist/tokens.js early-returns on a
falsy sub; it writes whole or not at all), so a failed exchange leaves no
partial state to clean — the wipe had no justified population. "Scope it
to the exchanged token" is vacuous on the failure paths: there IS no
exchanged token there.
- Reads that resume post-settlement with a surviving token are the app's
ordinary session behavior — valid -> 200s, expired -> 401 into mogh_ui's
own handling (the pre-gate status quo for any expired-jwt visit). Neither
feeds the per-IP limiter, which counts /auth/login attempts, not rejected
app reads.
- The wipe did not even stop cross-tab traffic: mogh's module-init IIFE has
no storage-event re-sync (upstream draft (b)), so tabs holding the closure
kept serving their in-memory jwt against the emptied store.
Behavior change to note: in exchange-error's shape (live session + failed
redeem) the post-settlement world now holds a VALID jwt, so RequireAuth
renders the app at / instead of bouncing to /login. The harness oracle for
that end state is re-derived in the next commits (session-preserving,
in-document convergence).
Invariant, stated where the next reader will look: failure paths are
store-neutral. mogh_ui's own 401 handling and the explicit user logout
(user-dropdown) remain the only store writers outside successful logins.
Round-8 review (moghtech#1666, C-003).
Co-Authored-By: Claude Code <noreply@anthropic.com>
…vigate The post-exchange late-success redirect read backto straight out of location.search and handed it to window.location.replace unvalidated (login.tsx). backto survives mogh_ui's post-exchange sanitize — utils.js strips only redeem_ready|totp|passkey — so the flag-then-reload chain this PR built guarantees a crafted link like /login?backto=//evil.example&redeem_ready=true navigates the victim off-origin immediately after a SUCCESSFUL login: the gate writes the fresh ok flag pre-navigation, mogh stores the token, and the reload lands back on /login with backto intact and a fresh flag ready. The pattern is inherited — mogh_ui's own maybeNavigate does the same unvalidated replace (receipt: dist/auth/login/index.js; drafted as upstream open-redirect issue (f) in compose/oidc-dev/BASELINE.md) — but this PR added a reachable call site of a flaw it documents itself, in the same PR that compensates for a different mogh_ui gap. Guarding locally, mirroring what upstream should adopt: safeLocalPath: accept "/" exactly, or "/" + a character that is neither "/" (protocol-relative) nor "\" (WHATWG URL parsing folds "\" to "/" for special schemes, so /\evil.example is protocol-relative too — the parser is the spec, not a browser quirk). Everything else falls back to "/". Gotcha tsc caught: `raw === "/" || regex.test(raw)` does NOT narrow raw to string — a falsy first disjunct leaves null reachable, so the typeof guard comes first. Scope note: the in-form path (mogh_ui's maybeNavigate) stays unvalidated until upstream lands draft (f); this only closes the call site this PR owns. Round-8 review (moghtech#1666, C-001). Co-Authored-By: Claude Code <noreply@anthropic.com>
readMoghStore's two failure logs embedded the raw localStorage value — safe()'s
label (`mogh token store parse (raw: ${raw})`) and the explicit
non-object console.error both carried it verbatim. A corrupt-but-parseable
store can still contain every account's jwt tail, and these are exactly the
lines a support ticket pastes: the log designed to help support escalate was
the one carrying credentials into the ticket.
Redaction: log length + an 8-char head fingerprint instead. That identifies
the corruption class (empty, truncated, HTML error page, wrong-type JSON)
without leaking material. The safe() label drops the raw value entirely; the
explicit log now covers EVERY path into `return undefined` — a throwing
parse lands there via safe()'s undefined, JSON.parse("null") via !parsed —
so the fingerprint fires in both cases and safe()'s label only ever carries
the generic failure marker plus V8's position-only SyntaxError text.
Length/head over nothing: without SOME shape signal, support cannot tell a
truncated store from a key written by a different schema — the exact
distinction the drift verdict depends on.
Round-8 review (moghtech#1666, F-002).
Co-Authored-By: Claude Code <noreply@anthropic.com>
The 32d1e1a regex (second char neither "/" nor "\") had a bypass the adversarial vector pass caught: WHATWG URL parsing strips ASCII tab/newline BEFORE parsing, so backto=/%09//evil.example decodes (URLSearchParams) to "/\t//evil.example", passes the regex — second char is a tab — and the parser then welds it into "//evil.example": protocol-relative, off-origin, right after a successful login. Guard-then-strip is the wrong order to reason about; the rule must hold on the POST-strip string. Fix: reject any control character (C0 + DEL) anywhere in the candidate before the shape checks. With no strippable characters, the first two chars are immutable across preprocessing — "/" then a non-slash, non-backslash — so the accepted set provably cannot navigate off-origin after the parser's own normalization. Local paths never legitimately contain control chars, so the accepted set loses nothing real. Vector results (ad-hoc, 13 cases): protocol-relative, backslash-fold, absolute URL, null/empty, tab-weld, CRLF-weld, and control-anywhere all fall back to "/"; plain paths with queries pass through. The two initial vector "failures" that motivated this commit were (1) the real tab-weld bypass and (2) a wrong expectation on "%2F%2Fevil" — that value does not start with "/" so the guard has always fallen back to "/" for it, which is correct: no komodo route is a percent-encoded-slash literal. Lesson: redirect-target guards must reason about the URL PARSER's normalization pipeline (strip C0/space at edges, remove tab/newline everywhere, fold "\" to "/" for special schemes), not about the raw string. A guard that validates the raw string certifies a string the parser never sees. Co-Authored-By: Claude Code <noreply@anthropic.com>
…-bound evidence Three round-8 findings, one commit — they all live in the evidence layer: F-003 (names asserting their inverse): the m1-seeded post-fix predicates demand ZERO stale reads, but the printed names read "stale-token requests fire..." / "VALUE confirmed on those requests" — a future reader grepping PASS lines (or diffing two runs' stdout, which IS the durable artifact) reads the mechanism as the assertion. Names now carry the mode-true expectation: "expect ZERO (fix defers provider reads)" vs "expect >= 1 (pre-fix reproduction)" — honest in both modes a run can execute in. C-003 (the removal of the settled-failure wipe changed the scenario's end state): with failure paths store-neutral, exchange-error's leg-1 session SURVIVES the failed settlement, so RequireAuth now renders the app at "/" where the wipe-era run bounced to /login with an emptied store. The old "converges to /login" arm was pinning an artifact of the wipe, not the contract — replaced with: exactly 1 document load (in-document convergence, unchanged purpose) + final "/" (store-neutral end state). Added the fail-closed preservation oracle: >= 1 authenticated /user 200 strictly after the settlement row — with an emptied store no authenticated request can occur post-settlement (hasJwt gates every read), so zero such rows means the wipe came back. Zero-residual stays valid: it counts unauth/401/403/5xx app rows, and post-settlement 200s were never in its disjunct. F-001 (evidence predating the code it certifies): batch 2's canonical 7/7 ran against the pre-arm build; two later commits reworked the very detector its m2 row certifies, and nothing forced the next run to bind to a tree. verify.mjs now stamps `git rev-parse HEAD` into every scenario banner and ndjson meta row, and REFUSES to run with uncommitted changes under ui/src or compose/oidc-dev — a dirty tree makes the stamped SHA a lie. Dev escape hatch: VERIFY_ALLOW_DIRTY=1 proceeds but stamps the dirt count — evidence can be recorded dirty, never mis-recorded clean. Harness outputs (out/, access.log) are gitignored and do not trip the gate. Run protocol (BASELINE.md): commit -> build -> run -> record. The stamped SHA is now the join key between a PASS line and the code that earned it. Co-Authored-By: Claude Code <noreply@anthropic.com>
…eaks podman
compose/oidc-dev's core-ui build (context = repo root) streamed ~119 MB of
context — mostly .git and host-installed node_modules — and, fatally, the
host's yarn-link symlink ui/node_modules/komodo_client ->
~/.config/yarn/link/komodo_client: it escapes the build context AND cycles
back into it (the link target is this worktree's client/core/ts). Podman's
context sender refuses the whole build on such a symlink ("invalid symlink")
where docker would silently skip it, so the round-8 rebuild failed before
compiling anything.
Every builder stage in this repo yarn-installs its own dependencies (the
core-ui Dockerfile's COPY ./ui is followed by `yarn && yarn build`), so host
dependency trees are never build inputs. Excluding **/node_modules and .git
shrinks the context to actual sources and makes the context send immune to
whatever the host has linked.
Gotcha for the next harness user: if the build dies with "invalid symlink"
on a node_modules path, the fix is the context, not the link — chase it with
.dockerignore, not by removing the host link the local toolchain needs.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…ear-on-read on the drop path Batch 3's first full-suite run at the drift-guard head (e6192cf) failed m2-forced on the fix-dependent surfacing arm: "no komodo-redeem flag and no notification text". The ndjson root-causes it to a consume race BETWEEN DOCUMENTS, not a detector defect: exchange 200 (sub-less jwt dropped) at .843 -> settlement writes the drop flag (same dispatch task) -> the OLD document — session-less, settled — bounces RequireAuth to /login and mounts Login (login chunk fetched at .857) -> mogh's sanitize reload lands at .892, replacing that document -> Login's drop effect ran clear-on-read in the DOOMED document -> the document the user actually lands in finds no flag, no toast Batch 2 passed only because the unload happened to win the passive-effect race by milliseconds. A signal that one document can consume and destroy before the document that must display it exists is not a signal — the consume-on-read contract was correct for the ok branch (it prevents the redirect loop) but wrong for drop, which has no redirect to loop. Contract now: drop surfaces WITHOUT consuming — the TTL (15s from the last confirmed settlement) is the cleanup, freshness gates the toast so a stale flag cannot replay it on later /login visits, and a per-document latch keeps StrictMode/re-mounts from stacking toasts. Whichever document wins the race (and any later /login visit within the window) finds the flag AND shows the toast; verify.mjs's probe reads the flag directly, so the surfacing arm is now race-free by construction. The ok branch keeps consume-BEFORE-navigate: that redirect is the one that would loop. Co-Authored-By: Claude Code <noreply@anthropic.com>
…contract (round 8) C-003 removed the settled-failure wipe; five places in the harness docs still narrated the old contract or the old redirect: - BASELINE "two-part transfer argument" part 2: said post-settlement reads stay off because remove_all empties the store. Superseded in place, with the blast-radius rationale and the new preservation oracle (>=1 authed /user 200 strictly after the settlement row — fail-closed, since an emptied store can produce no authed request). - BASELINE m1 root-cause section: dropped the "(with its remove_all hygiene)" parenthetical on the defensive-watchdog note. - BASELINE upstream draft (c): the failure-path endpoint now depends on what the store holds (/login with no surviving session, the app itself with one) instead of always /login. - BASELINE upstream draft (f): komodo's late-success call site is now locally guarded (safeLocalPath); notes WHY naive second-character guards are bypassable (WHATWG strips tab/newline pre-parse, folds "\" to "/") and that the upstream maybeNavigate fix is still the one that matters. - README scenario map, exchange-error post-fix cell + BASELINE hung narrative: session preservation and the guarded redirect replace the wipe-era "/login bounce" wording. Co-Authored-By: Claude Code <noreply@anthropic.com>
…F-001 closed) Canonical evidence now lives at the code being merged: - Batch 3a (e6192cf): 6/7 — m2-forced FAILED its surfacing arm, the first post-fix execution of the drift-guard detector anywhere (batches 0-2 predate it; that is F-001's point). ndjson root-cause: a consume race BETWEEN documents — the doomed pre-reload document mounted Login and consumed the drop flag (login chunk at …726.857) before mogh's sanitize reload landed (…726.892). Batch 2 had passed only because the unload won the passive-effect race by milliseconds. Fixed by dfa6543 (drop no longer consumes; TTL is the cleanup). - Batch 3b (a861602, image 778aee5742c6, 17:08-17:14 -03:00): 7/7 — success 7/7, latency 8/8, m1-seeded 4/4, m2-forced 5/5, exchange-error 3/3 (+2 pre-fix SKIPs), hung 5/5 (+1 SKIP), isolation 3/3. Exchange-error held the round-8 contract exactly: zero bad rows, zero failed ws, 5 authenticated /user 200s (leg-1 session preserved, fail-closed against the wipe's return), in-document convergence at /. - Every scenario banner and ndjson meta row stamped head=a861602a4279 dirty=0; the SHA-gate refused one launch mid-round (the section's own doc edits were uncommitted) — designed behavior, recorded as such. Verdict table annotated; post-fix metadata extended with batch 3a/3b provenance (image ids, heads, windows). Batch 2's rows remain as history. Co-Authored-By: Claude Code <noreply@anthropic.com>
…not truthiness (round-9 C-003)
The round-7 drift guard's second disjunct — `jwtAbsent && !!jwt()` — read
"the library's closure view holds A jwt while the pinned key lacks THE
exchanged one" as proof of an upstream key rename. That inference only
holds when the closure view was empty before the exchange. In a redeem
document opened by a user with a VALID PRE-EXISTING SESSION, the IIFE
closure holds the OLD token: truthy. A silently-dropped exchange (the exact
upstream bug this detector exists to surface) then classified as drift:
phase stayed "ok", no toast, and the one console error named the wrong
hypothesis — the user authenticated as the OIDC identity while the app
quietly kept the previous session's identity. "M2 closed at the komodo
layer" was true only for session-less documents, which is also the only
shape the harness drove (the harness gap gets its own scenario in the next
commit).
Fix: discriminate the four jwtAbsent populations by what the closure view
holds relative to THE EXCHANGED jwt — a successful add_and_change sets the
closure to the exchanged jwt in-process; a dropped one leaves it untouched:
1. store absent, closure empty -> drop (m2 canonical)
2. store = pre-existing session, closure = OLD token -> drop (was misread
as drift; now surfaces the toast)
3. closure === the EXCHANGED jwt, pinned key lacks it -> key/schema
drift -> loud "ok" (never a false drop — the round-7 intent, now with
a sound premise)
4. pinned key is a non-store schema -> drift -> loud "ok"
EQUALITY is the discriminator; truthiness collapsed populations 2 and 3
into one wrong verdict. No snapshot machinery needed — the closure's
post-exchange value alone separates them.
Also: the settled-failure toast no longer claims "Returned to the login
page" — with a surviving session the app renders in place (store-neutral
failure paths, round-8 C-003), so the message now names no destination
(O-001).
Co-Authored-By: Claude Code <noreply@anthropic.com>
…ery (round-9 C-004) `callerEnabled(q) !== false` enabled a function-valued enabled that returns undefined, where react-query's own resolveEnabled falsy-coerces the result to disabled. Latent today — TS pins the callback's return to boolean and the round-9 caller census found zero function-valued enabled callers — but the helper exists precisely to be composed, and the divergence only bites when someone weakens a predicate's return type (the classic `opt?.flag && ...` shape, which yields undefined for "no"). The function branch now does `!!callerEnabled(q)`: truthy-non-boolean returns (1, non-empty strings) still enable exactly as RQ would, and undefined disables as RQ would. The NON-function branch deliberately keeps `!== false`: an ABSENT enabled is default-enabled in RQ, so undefined must not disable there. The two branches encode two different defaults because react-query does — the comment now says so, since the asymmetry is the trap. Co-Authored-By: Claude Code <noreply@anthropic.com>
…e SHA gate, 429 term, ws flush (round-9)
Five harness dispositions in one commit — they all live in the evidence layer
and each one exists because a guard could not detect its own failure mode:
C-001 — the m2 surfacing arm read the flag with `?? fallback` and accepted
any truthy phase; an unconsumed "ok" on /login is precisely the signature of
a classifier that failed to verdict "drop" (the ok branch consumes its flag
BEFORE navigating), and it printed PASS. The arm now requires phase ===
"drop" (or the toast text when sessionStorage is degraded).
C-003 coverage — new `m2-seeded` scenario: the same route-intercepted
sub-less exchange driven with a pre-existing session seeded into the store.
This is the population the round-7 classifier misread as key-drift (the
production fix landed in the previous commit); until now the suite ran only
the session-less shape and STRUCTURALLY could not see the bug. The seeded
row adds two arms: phase-drop surfaced WITH a session, and the seeded token
preserved through the drop (store neutrality). The m2 drive/checks are
shared by both scenarios — the scenario matrix, not a copy, is what keeps
the two shapes from drifting apart.
C-002 — the SHA-gate pathspec covered ui/src + compose/oidc-dev but the run
exercises more merge surface: the bundle's whole input tree (ui/ —
package.json, yarn.lock, vite config feed core-ui.Dockerfile stage 1), the
yarn-linked client package (client/core/ts), the stack topology
(compose/oidc-dev.compose.yaml sits OUTSIDE compose/oidc-dev/), and
.dockerignore (it decides what enters the build context). Guard widened to
`ui client/core/ts compose .dockerignore`; the acknowledged residual (the
gitignored .env digest pin) is documented in place. Hand-listed pathspecs
are how batch-2 happened — the list now matches the docs' binding claim.
C-005 — `unauthOrFail` had no 429 term although the limiter's 429 is the
suite's own narrative endpoint ("re-fired exchanges burn the budget into
429s"); a 429-only tail passed every disjunct. Added, with the draft-(d)
tie-in comment.
C-006 — `wsFails: 0` was vacuous in exchange-error and hung: the
/ws/update row is written at connection CLOSE and those scenarios never
closed the context, so a healthy open connection produced no row at all.
Both now flush (flushWsRow) after their 60s windows and add a POSITIVE
101-at-close assertion for the recovered/preserved session's ws; hung
captures its final URL before the close (page.url() throws on a closed
page). Check names relabel the term "failed ws rows (flushed at close)".
Co-Authored-By: Claude Code <noreply@anthropic.com>
…loses the context The round-9 C-006 flush was inserted before `page.url()`/loader capture in exchange-error — page.url() on a closed page throws, which would have crashed that scenario's driver (caught per-scenario, but a guaranteed FAIL). hung already captured before closing; exchange-error now does too, and the now-duplicate declarations are removed. Caught by re-reading my own diff before the evidence run — the flushed scenario count in batch 4 is unchanged, just executable. Co-Authored-By: Claude Code <noreply@anthropic.com>
… row Batch 4's new ws-alive check failed deterministically on a healthy exchange-error run. Instrumented diagnostic (one VERIFY_ALLOW_DIRTY probe run): the entries contained exactly ONE ws row — the preserved session's — and its ts was 44 ms BEFORE the settlement row. The predicate assumed ts is the close time and the preserved session's ws upgrades after settlement; both assumptions are wrong: Caddy's ts on a ws row is the UPGRADE (request) time, and leg-2's provider ws upgrades during pending (the ws does not wait for the gated reads), ~44 ms before the 401 lands. hung's identical check passed only because its recovery ordering happened to upgrade after the late 200 — luck, not correctness. Contract fix: flushWsRow returns whether the 101 row was observed, and both scenarios' checks assert THAT result. The invariant C-006 needs is "a ws connection existed through the scenario and closed 101" — the flush itself is the proof; WHERE its upgrade sits on the timeline is over-specification. The zero-residual window keeps bounding failed handshakes; the flush keeps the term from being vacuous. Instrumentation removed; lesson recorded in the flushWsRow doc comment so the next timestamp predicate on a ws row starts from the right model. Co-Authored-By: Claude Code <noreply@anthropic.com>
…lind population covered BASELINE gains the round-9 section: the C-003 drift misclassification (with its four-population equality table), the C-001 strict-phase arm, the C-002 merge-surface gate widening, the three minors, the new m2-seeded scenario, and both batch records — 4a (78739f1, 7/8: the new ws-alive check mis-failed a healthy run because Caddy's ts on a ws row is the UPGRADE time and the preserved session's ws upgrades during pending; root-caused by an instrumented diagnostic, fixed by asserting the flush result) and 4b canonical (1f0f267, 8/8, all eight ndjson meta rows stamped head=1f0f2673b0bb dirty=0, image 925a844ef0a0 built from the c9873f8 tree — ui unchanged by the two later harness-only commits, stated explicitly). README: m2-seeded in the scenario list + the scenario-map row now states the strict-phase contract (an unconsumed ok flag on /login is a classifier regression, not a pass). Verdict-table annotation and post-fix metadata extended with batch 4a/4b provenance (heads, windows, image id). Co-Authored-By: Claude Code <noreply@anthropic.com>
…comments The round-8/9 fixes wrote their review-round ids into the comments where the fixes landed: (round-8 C-003), (round-9 C-004, latent), (batch-3 lesson), (upstream draft (b) cites it). For an upstream maintainer those tokens resolve only through BASELINE.md — a 911-line evidence log no UI reader has reason to open — and the ids collide across rounds (round-8 C-003 and round-9 C-003 are different findings sharing a token), so a grep lands 50/50 on the wrong rationale. An earlier scrub commit replaced spec anchors but missed this hyphenated spelling class — exactly the miss its own commit message warned about. Every marker replaced with the self-contained statement of the constraint it annotated; the WHY content is unchanged (the rationale was already inline — the tags were pointers, and pointers into a private process dialect are worse than none when they collide). Sweep: grep -rnE 'round-[0-9]|batch-[0-9]|C-00[0-9]|F-00[0-9]|draft \([a-z]\)' over ui/src -> zero hits. Co-Authored-By: Claude Code <noreply@anthropic.com>
…ui maybeNavigate path
safeLocalPath guarded exactly one call site — this PR's late-success
redirect — while the same parameter flowed unvalidated through the OTHER
consumer on this very page: mogh_ui's LoginPage success handler
(maybeNavigate) does location.replace(search.get("backto") ?? "/") after a
LOCAL/passkey/totp login, so /login?backto=//evil.example open-redirected
the victim to a lookalike domain immediately after a successful login —
with a fresh session. The PR documented that exact flaw as an upstream
draft and guarded its own call site, which made the coverage LOOK complete
while the most common login path stayed live.
Fix at the input instead of at each consumer: Login's mount effect rewrites
the hostile value out of the URL (history.replaceState — no navigation,
LoginPage unaffected) before anything reads it. mogh_ui reads backto from
location.search at login-success time, so every embedder path on this app
— this file's and mogh_ui's — now navigates only on a value that survived
safeLocalPath. The late-success redirect below stays double-guarded.
Why mount-time rather than an onSuccess wrapper: LoginPage's handlers are
props-we-do-not-control on a component we render; the URL is the one input
we own, and sanitizing it once covers handlers this file never sees.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…ntical mock issuer default (round-10 minors) C-005 — the runner's finally did: write ndjson -> browser.close() -> knob restore. A throw from the write (EACCES/ENOSPC on out/) propagated out of the finally and skipped BOTH cleanups, leaking Chromium and leaving DELAY_AUTH_MS set — the exact leftover the SIGINT handler and knobOk machinery exist to prevent. The write is now guarded and first (flush evidence best-effort -> close browser -> restore the shared knob), and a run whose evidence could not be recorded fails instead of passing. C-004 — hung's "drive observed >= 20s post-callback" check derived its expected value from the scenario's own unconditional 60s sleep: the oracle was self-derived, the assertion unfalsifiable, the PASS line certifying nothing. Deleted — the observation window it gestured at is measured by the zero-residual check, whose name carries the 60s it actually observed. hung is now 5/5 + 1 pre-fix SKIP. C-003 — oidc-mock's baked ENV MOCK_ISSUER lacked the :8443 that the harness's own byte-identical-issuer rule (and the compose override) require; a bare `docker run` of the mock silently booted a port-less issuer and failed komodo's exact-issuer validation 500s away from the cause. Default now carries :8443. Co-Authored-By: Claude Code <noreply@anthropic.com>
…erified byte-level BASELINE gains the round-10 section: the marker scrub (with the sweep pattern, so the next scrub starts from the right grep), the mount-time backto sanitize closing the mogh_ui maybeNavigate path on the shipped app, and the three minors. Batch 5 canonical: HEAD 9bf2f4a, 8/8, all eight ndjson meta rows stamped dirty=0, 44 checks. Notable receipt: the container's podman image-ID field reported an ID matching neither the fresh tag nor the prior build — an unreliable witness. The bundle identity was therefore verified the batch-0 way, byte-level: the served login-*.js md5 equals the host yarn build output at this head. The protocol note now carries that receipt so the next run reaches for the stronger check first. hung's row count drops 6/6 -> 5/5 (the self-derived check is gone — the window it gestured at is measured by the zero-residual check). Co-Authored-By: Claude Code <noreply@anthropic.com>
…ute the backto guard once Two round-11 dispositions in the shipped source: C-001 REFUTED, comment strengthened anyway. The roast claimed the settlement listener is delivered via systemSetTimeoutZero (a later macrotask), contradicting an earlier round's receipt. Re-verified against the pinned query-core 5.102.4: mutationCache.notify invokes this.listeners.forEach DIRECTLY inside notifyManager.batch, and batch runs its callback synchronously (transactions++ / callback() / transactions--). The setTimeoutZero scheduler delivers only the notifyManager's observer-notification queue — filled by schedule()/batchCalls, which cache listeners never touch; flush() with an empty queue is a no-op. The "7-scenario..."-style misread happened because notifyManager.cjs visibly contains a scheduler; the header comment now names WHERE the scheduler applies and where it does not, with the mechanism spelled out so the next reader who greps notifyManager starts from the right model. Hook ordering (mutation.cjs 107-108-111) was never in dispute. F-003 — safeLocalPath ran twice per hostile-backto mount (guard, then URL write). The file's security seam is now computed once and its ANSWER written: one invocation, one approval, no divergence between "what was approved" and "what landed in the URL". Co-Authored-By: Claude Code <noreply@anthropic.com>
…y gets a lockfile too The harness pinned everything that touches evidence EXCEPT its own driver: oidc-provider frozen at 9.12.2 via a committed lockfile, node:22.12-alpine, caddy:2.11, a digest-pinned core image — and then the suite ran on whatever playwright version `npm i playwright` (caret range, gitignored manifest) resolved that day. Chromium/CDP behavior is load-bearing here: console-shim capture, route.fulfill rewriting, addInitScript ordering, the flush dance. A playwright major bump could change that behavior and a future green run would not be comparable to BASELINE's rows even at the same HEAD — the same drift class round-9's SHA-gate widening covered, one layer short. Now committed and pinned: package.json declares playwright 1.63.0 (exact — the resolved version the canonical batches ran on, not a caret), the lockfile is in the repo, `.gitignore` stops ignoring both, and the README install step is `npm ci && npx playwright install chromium` — reproducible by construction, with only the browser binaries staying host-local. `npm ls` confirms 1.63.0 both in the manifest and installed. Co-Authored-By: Claude Code <noreply@anthropic.com>
…source, driver pinned BASELINE gains the round-11 section. The headline is a roast-vs-roast disagreement settled by evidence: round-11 claimed the settlement listener is delivered via systemSetTimeoutZero (which would make the silent-drop verdict's survival a browser-scheduling coincidence); the pinned query-core 5.102.4 says otherwise — mutationCache.notify invokes listeners directly inside notifyManager.batch, which runs its callback synchronously; the setTimeoutZero scheduler serves only the observer-notification queue. The receipt is recorded with the mechanism so the claim is checkable without re-deriving it. Also recorded: the Playwright driver pinned 1.63.0 with a committed lockfile (F-002), the once-computed backto guard (F-003), and batch 6 canonical (7ee624c, 8/8, 44 checks, all meta rows head-stamped dirty=0, served-bundle md5 verified — including the note that COPY layering leaves the prior build's chunk in the image, inert but present, so the index reference is the identity that matters). Co-Authored-By: Claude Code <noreply@anthropic.com>
This branch has not been deployed
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
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.
✅ Fix the OIDC login loop: after a successful
ExchangeForJwtthe UI never sent the returned JWT, so behind a forward-auth proxy every request rode a stale token into 401s and the per-IP rate limiter locked out all proxy users. komodo now owns the redeem lifecycle as renderable state; a docker harness (compose/oidc-dev) reproduces the failure pre-fix and runs all 8 scenarios green post-fix.🟢 Scope:
ui/src(6 files) +compose/oidc-dev*. The branch history was rewritten once before this push to drop in-flight design docs (preserved on thekomodo-oidc-login-jwt-redeem-race-fix-1665-archivebranch in this fork).More context: #1665
What komodo does now
A filtered
MutationCacheconfig + subscription (ui/src/lib/redeem-gate.ts, constructed inmain.tsx— komodo owns the object) turns the redeem lifecycle into renderable state:Router's render — a child ofWebsocketProvider, whose gated reads commit before any child-render flip (React renders parent-first; the harness caught this gap live). The gate initializespendingat module scope when?redeem_ready=trueis present, so the provider's first render already sees it closed.idlemeans open: every page load that never runs a redeem behaves byte-identically to before.fetchhas no timeout; a black-holing proxy would otherwise hang the gate forever.remove_all: it ran on EVERY failure path, so a stale?redeem_ready=truebookmark or a watchdog trip silently logged every tab of the browser out of a valid session — and mogh'sadd_and_changeis all-or-nothing, so there was no partial state for it to clean.) Convergence is: params stripped viahistory.replaceState, a persistent notification, then the existing/login?backto=flow. No reload, so toasts survive. Reads that resume with a surviving token are ordinary session behavior (valid → 200s; expired → mogh_ui's own 401 handling) — never limiter traffic.add_and_changesets the library's in-process view to the exchanged token, so "view holds the exchanged jwt while the pinned key lacks it" is the key-rename signature, while a pre-existing session's OLD token correctly classifies as a drop — the round-7 truthiness check silenced exactly that population./login(mogh_ui has no auto-redirect on token appearance); the login page one-shot-redirects tosafeLocalPath(backto)on a fresh evidence flag — local-path-only with control-character rejection, because WHATWG URL parsing strips tab/newline before parsing and folds\to/for special schemes, so naive//-prefixed guards are bypassable via/%09//evil.exampleAdditionally,backtois sanitized IN THE URL atLoginmount (history.replaceState), so mogh_ui's own unvalidatedmaybeNavigateconsumer — the local/passkey/totp login path — can only navigate on a value that survived the guard; upstream draft (f) remains the proper library-level fix.useRead/useUsernow compose callerenabledwith the jwt gate instead of letting a spread-copiedenabled: undefinedbypass it.Provider reads (
useUser,GetCoreInfo) and the update websocket defer only while the gate is pending — bounded by the watchdog, so nothing can hang closed.Evidence
compose/oidc-dev: digest-pinned core serving the branch UI (the same/app/uiwiring as production), oidc-provider mock with parent-domain session cookie + forward-auth/verify, Caddy with JSON access log + latency knob (ceilinged under the watchdog), and an 8-scenario Playwright suite (driver pinned via a committed lockfile) whose assertions live on the wire (Caddy access-log authorization/401/5xx accounting, no vacuous defaults).BASELINE.md): M1 (stale-token reads fire from the always-mounted provider during the redeem window — rejected with 500s on this core), M2 (silentadd_and_changedrop of a sub-less token), M5 (failed exchange = eternal spinner) reproduced with wire evidence. M3 (forward-auth bouncing the post-exchange reload) is not reproducible with the mock (its session survives redemption) and stays an open question for real Authelia-class topologies. The parent-first render gap the pre-arm closes is independently proven by batch-0's own evidence: the exchange-error run's redeem document dispatched/user+GetCoreInfo16 ms before the exchange completed (referer-pinned in the access log) — zero such reads on the shipped build.m2-seeded: the sub-less exchange driven WITH a pre-existing session — the population the classifier once misclassified — plus strictphase === "drop"surfacing arms, session-preservation, and ws-liveness assertions) — zero stale-token reads during the window, drop surfaced, watchdog converges then the late success lands on the guardedbackto, zero residual over the 60 s windows, update websocket connected, gate isolated from unrelated mutations; a failed redeem additionally asserts session preservation (≥1 authenticated/user200 strictly after settlement — fail-closed against the wipe returning).BASELINE.md§ "Round-11 batch"; batch 5/4b/3 records retained). The harness is self-certifying: every scenario banner and ndjson meta row carries the head SHA, and the runner refuses to execute on a dirty merge-surface tree (ui,client/core/ts,compose,.dockerignore), so each PASS line is bound to the code being merged.BASELINE.md(silent drop, no storage re-sync, redeem-failure fallback, per-IP limiter keying, unvalidatedbacktoopen-redirect, the M3 question) — not filed yet; each cites harness evidence.Why the fix embraces mogh_ui's reload instead of intercepting it
Verified against the pinned
query-core@5.102.4: mogh_ui's mutationonSuccess(token write +sanitizeQueryInner→ full-pagelocation.replace) runs before thesuccessdispatch any host subscriber observes, and none of the success-path hooks are individually guarded. There is no komodo-side hook between the token write and the navigation, so suppressing the reload requires an upstream mogh_ui change. Embracing it deletes a whole class of host-side workarounds (invalidate-on-token-change, storage listeners) and bounds requests by construction — which is also what stops feeding the limiter.🔁 Post-merge cascade
compose/oidc-dev/BASELINE.md.