diff --git a/devlog/_plan/260905_admin_token_local_ux/000_research.md b/devlog/_plan/260905_admin_token_local_ux/000_research.md new file mode 100644 index 0000000000..ae938bf82e --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/000_research.md @@ -0,0 +1,120 @@ +# 000 — Research: why a plain local user is shown an admin token box + +Two open issues describe the same wound from opposite ends. + +- **#3483** (juzijia, Windows 10, 2.42.0) — the admin token dialog paints an + empty red error notice the moment it opens, before anything is submitted. +- **#3353** (Tao-Yida) — after upgrading to 2.40.0 a user was locked out of the + dashboard by a bare password box, assumed a config-loss bug, and had to have + an LLM read the source to learn the token was a new security feature. + +The user's framing is stronger than either issue, and it is the one this unit +adopts: **a plain local user should never see that dialog at all.** When it does +appear it is a symptom, and the box asks the user to solve a problem they did +not cause and cannot diagnose. + +## How the dashboard is supposed to authenticate + +A loopback install never needs a typed credential. The server mints a session +and injects it into the served document: + +``` +GET /opencodex-session + -> src/server/index.ts:2074 issueGuiSession(...) + -> src/server/gui-session.ts:166 + -> meta opencodex-session-token / -csrf / -origin / -server-origin + (src/server/gui-static.ts:71-74) +``` + +`gui/src/api.ts:loadInjectedSession()` reads those tags on boot. Verified live +against the running 2.43.0 proxy on port 10100: + +```text +curl -i -H 'Host: 127.0.0.1:10100' http://127.0.0.1:10100/opencodex-session +HTTP/1.1 200 OK + +``` + +So on the happy path the prompt is unreachable. The interesting question is what +happens when that mint fails. + +## The fallback that should not be a fallback + +`gui/src/api.ts:resolveTokenAfter401()` (around line 247) handles a 401 like this: + +```ts +const renewed = await Promise.race([reBootstrapSessionToken(plane), watchdog]); +if (renewed.kind === "minted") return renewed.token; +if (renewed.kind === "failed") return null; +const prompted = await requestAdminToken(token => verifyAdminToken(plane, token)); +``` + +`reBootstrapSessionToken` maps **any 4xx** to `"unavailable"`: + +```ts +if (!response.ok) return response.status >= 400 && response.status < 500 + ? { kind: "unavailable" } : { kind: "failed" }; +``` + +And `"unavailable"` is precisely the branch that raises the password box. + +Now read the mint conditions (`src/server/gui-session.ts:172-183`): + +```ts +if (!isApiAuthRequired(config)) { + if (!isLoopbackHostname(host.hostname) || !isAllowedManagementOrigin(req, config)) return null; + ... +} +``` + +with `isApiAuthRequired(config) === !isLoopbackHostname(config.hostname)` +(`src/server/auth-cors.ts:285`). + +That yields the defect in one sentence: **on a loopback install the only ways to +get a 401 from the bootstrap are a Host or Origin mismatch — a misconfiguration +the admin token cannot fix.** Typing a token there is not a recovery path; it is +a dead end wearing a login form. + +And when the bind genuinely is non-loopback, the token is real and required — +but the dialog explains none of that, which is exactly #3353. + +## Why the notice is already red and empty (#3483) + +`gui/src/admin-token-dialog.ts:76-79` builds the error element up front: + +```ts +validationError.className = "notice notice-err"; +validationError.hidden = true; +``` + +`hidden` works only because the UA stylesheet says `[hidden] { display: none }`, +and that rule is the weakest one in the cascade. `gui/src/styles.css:1307` then +says: + +```css +.notice { ... display: flex; ... } +``` + +An author rule with an explicit `display` beats the UA `[hidden]` rule, so the +element stays laid out. It has `notice-err` borders and padding +(`styles.css:1355-1359`) and no text, which renders as the empty red box in the +screenshot. The bug is a CSS cascade defect, not a logic error — which is why no +existing test caught it: happy-dom asserts `hidden === true` happily while a real +browser paints the box. + +## What this unit changes + +1. Never prompt a standalone/loopback dashboard. Tell the user what is actually + wrong instead. (`010`) +2. When the prompt is legitimate, make it self-explanatory and link to a real + setup guide. Fix the empty notice while in there. (`020`) +3. Triage the Windows baseline and #3320 and land what is provable. (`030`) +4. Deliver as a stacked PR chain, admin-merged to `dev`. (`040`) + +## Constraints carried from the request + +- No repository-wide local suite on this workstation. Focused `bun test ` + plus `bun run typecheck`; heavy probes go to SSH hosts. +- A Windows baseline is already in flight on `desktop-c795oh4` under + `/c/ocxwin` (lock `/c/ocxwin/.suite.lock`, shards `base-1..4`). It is read-only + evidence for this unit and must not be disturbed. diff --git a/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md b/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md new file mode 100644 index 0000000000..e887fa3219 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md @@ -0,0 +1,101 @@ +# 010 — Never prompt a standalone dashboard for an admin token + +Work-phase `wp1`. Depends on 000. Criterion `c-1`. + +## Problem + +`gui/src/api.ts:resolveTokenAfter401()` treats "the server would not mint me a +session" as "ask the human for a token". On a loopback install those are not the +same thing. `issueGuiSession` mints unconditionally for a loopback host with an +allowed origin (`src/server/gui-session.ts:172-183`), so a 401 there means the +request did not look loopback to the server — a Host/Origin/bind problem. No +token the user can type changes that verdict, because the token is not what was +refused. + +## The signal + +The server already states its topology on every served document: + +```ts +// src/server/gui-static.ts:95 +function runtimeRoleMeta(runtimeRole: string): string { + return \`\`; +} +``` + +and the GUI already reads it (`gui/src/api-targets.ts:10-14`). That comment block +is explicit that a missing tag means "standalone / older server / Vite dev", i.e. +the safe default. This unit reuses that exact reader rather than inventing a +second topology signal. + +The rule: **the admin-token prompt is for a deployment that actually requires a +typed credential.** That is the non-loopback bind, which is the `hub` role. Any +other role — `standalone`, `client`, or an absent tag — must not prompt. + +## Change + +In `gui/src/api-targets.ts`, add a sibling to `isConnectedRuntime()`: + +```ts +/** + * May this dashboard ask the user to type an admin token? + * + * Only a hub does. A standalone loopback install mints its own session + * (src/server/gui-session.ts), so a refusal there is a Host/Origin + * misconfiguration that no typed token can repair — prompting for one asks the + * user to answer a question they did not cause and cannot diagnose (#3353). + * A missing tag reads as standalone, matching runtimeRoleFromDocument's + * existing safe default. + */ +export function adminTokenPromptAllowed(): boolean { + return runtimeRoleFromDocument() === "hub"; +} +``` + +In `gui/src/api.ts:resolveTokenAfter401()`, gate the prompt and record why it was +skipped: + +```ts +if (renewed.kind === "failed") return null; +if (!adminTokenPromptAllowed()) { + state.promptCancelled = true; // do not re-ask on every subsequent 401 + reportSessionUnavailable(plane); // surface an actionable notice instead + return null; +} +const prompted = await requestAdminToken(...); +``` + +`promptCancelled = true` matters: without it every failing request re-enters the +resolution path. The existing `storeSession` already resets that flag when a +session is later minted (`gui/src/api.ts:81`), so recovery is automatic once the +misconfiguration is fixed. + +## What the user sees instead + +A dismissible notice, not a form. Copy names the real cause and the real fix: + +> **The dashboard could not start a session.** OpenCodex is running, but this +> page's address is not one it recognises as local. Open the dashboard at the +> address the proxy prints on startup (usually `http://127.0.0.1:`), or see +> the dashboard access guide. + +`reportSessionUnavailable` is a thin, testable seam: it dispatches a +`CustomEvent` the shell renders. It must not be a `alert()` and must not block. + +## Verification + +`bun test gui/tests/api-auth-deadline.test.ts` plus a new case: + +- role `standalone` (and absent tag): after 401 + `unavailable` rebootstrap the + injected `adminTokenPrompt` spy is **not** called, the request resolves, and a + second failing request does not call it either. +- role `hub`: the spy **is** called (the legitimate path stays intact). + +The role must be settable per test — the tests build their own `happy-dom` +document, so the case writes the meta tag before `installApiAuthFetch()`. + +## Out of scope + +No server change. `issueGuiSession`, `requireManagementAuth`, and the CORS +resolvers keep their current semantics; this phase only stops the GUI from +asking a question the server never wanted asked. diff --git a/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md b/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md new file mode 100644 index 0000000000..3e4743988a --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md @@ -0,0 +1,63 @@ +# 020 — Repair the dialog itself + +Work-phase `wp2`. Criteria `c-2`, `c-3`, `c-4`. Landed in #3491 and #3493. + +Two defects in one surface, deliberately split across two PRs because they have +nothing to do with each other beyond sharing a file. + +## #3483 — the empty red notice + +`gui/src/admin-token-dialog.ts` builds the error element up front and hides it +with the `hidden` attribute. That works only because the UA stylesheet says +`[hidden] { display: none }`, and `gui/src/styles.css` overrode it: + +```css +.notice { ... display: flex; ... } +``` + +Author origin beats user-agent origin. **Specificity never enters the +comparison** — which is why this looked like a validation false-positive and why +a `.notice[hidden]` fix would have been the wrong shape (it would still lose to +the three-class `.startup-runtime-notice` rule at `0,3,0`). + +The repository had already solved this exact problem once, in +`gui/src/styles-combos-workspace.css`, where a bare `display: flex` left both +tab panels painted at once. That comment block is the precedent this fix +follows: move the display onto `:not([hidden])`. + +Applied to `.notice`, `.notice-warn` (used without `.notice` in several places), +and `.notice.notice-warn.startup-runtime-notice`. + +### Testing this required two tests, not one + +happy-dom applies no author stylesheet and performs no layout, so +`expect(alert.hidden).toBe(true)` **passes today, unfixed**, while a real browser +paints the box. A DOM assertion cannot see this class of bug. + +So the DOM test asserts the notice is hidden AND empty on open (keeping the two +halves of "no error" from drifting), and a second test reads `styles.css` and +fails any `.notice` rule that sets `display` without the guard. The second was +driven red by reverting the CSS before being accepted. + +## #3353 — the box that explained nothing + +The dialog's only text was a title naming an environment variable. A user who +had never set one had no way in. + +Ground truth, verified in source rather than assumed: + +- the proxy writes the token to `getConfigDir()/admin-api-token` on first start + (`src/lib/admin-secrets.ts`), `0600`, matching `/^ocx_admin_[A-Za-z0-9_-]{43}$/` +- `OPENCODEX_ADMIN_AUTH_TOKEN` replaces it entirely and is not regex-checked +- **no CLI prints it.** `ocx doctor` deliberately reports presence without ever + returning a value + +That last point is worth stating in the docs explicitly rather than omitting: +hunting for `ocx token` is the obvious next move, and silence about it wastes +the user's time. + +The dialog now carries a help paragraph plus a link to a new +"Finding the admin token" anchor, styled like the existing in-app docs links +(`target="_blank"`, `rel="noreferrer"`, accent colour) per +`gui/src/pages/dashboard-dialogs.tsx`. Copy landed in all nine locales; +`gui/tests/i18n-locales.test.ts` enforces key parity. diff --git a/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md b/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md new file mode 100644 index 0000000000..a0371ea812 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md @@ -0,0 +1,82 @@ +# 030 — Windows baseline: what the failures actually were + +Work-phase `wp3`. Criterion `c-5`. Evidence, not a landed fix. + +A full Windows suite was already running on `desktop-c795oh4` when this unit +started (`/c/ocxwin/repo` at `00834d710`, shards `base-1..4`). It was read-only +evidence for this unit and was not disturbed. A second run on Bun `1.4.0` +followed (`v140-1..4`). + +## The headline + +| run | Bun | fail | +|---|---|---:| +| `base-1..4` | 1.3.14 | 179 | +| `v140-1..4` | 1.4.0 | 25 | + +Shard 1 alone went from 100 `(fail)` lines to 2. **The Windows suite was not +telling us about 179 product defects; it was mostly reporting one harness +failure over and over.** + +## Why 161 of them were one bug + +`tests/preload.ts` calls `acquireTestRunLock` (line 42) BEFORE it arms the guard +with `OCX_TEST_HOME_GUARD=1` (line 64). On Windows the lock path needs the +effective account SID, and under 4-shard load that lookup timed out: + +```text +CodexUserIdentityRefusal: Windows effective-account lookup timed out. + at powershellValue (src/codex/user-identity.ts:252) + at resolveWindowsSid (src/codex/user-identity.ts:261) + at resolveDefaultTestRunLockPath (scripts/test-run-lock.ts:227) + at tests/preload.ts:42 +``` + +The worker then ran with the guard permanently off, and every test that asserts +"this helper is only available under the repository test preload" failed as a +cascade: 44 in `codex-reset-credit-operation-ledger`, 68 in +`codex-reset-credit-recovery`, 49 in `lab-fabric-task`. + +**That cascade had teeth.** With the guard down, two suites reached live +machine state instead of being refused: + +- `windows-elevation-spawn.test.ts:89` expected `launcherPid: null` and got + `18144` — a real PowerShell process was launched. +- `service.test.ts:1283`/`:1307` expected "refusing to mutate the + machine-global Windows Task Scheduler from an armed test process" and instead + got real scheduler-registration outcomes. + +So the ordering in `preload.ts` is not only noisy, it is the difference between +a refused test and one that touches the developer's own Task Scheduler. Worth +fixing on its own merits, independently of the Bun version that exposed it. + +## What survives on Bun 1.4.0 + +25 failures in three groups: + +- `multi-account auth store` — 22 of the 25, one file. Not yet diagnosed. +- `ocx v2 keep-native-v1` — 2. Dirac classified the `base` occurrence as a test + artifact: the product CLI exited 0 and the V2 disable took effect; only the + spy compares raw argv, and Windows `.cmd` invocation goes through the ComSpec + wrapper (`src/lib/win-exec.ts:79`), which is correct behaviour. +- `cli wiring > interactiveGuardOk ... when cwd is unlinked` — 1. + +## Not fixed here, and why + +This unit's authority is the admin-token UX. None of the surviving failures are +in that surface, and each needs its own reproduction on Windows before a fix is +more than a guess — the `base` run's evidence is contaminated by the guard +cascade, so a fix written against it would be written against an artifact. + +The honest carry-forward is three separate units: + +1. Arm `OCX_TEST_HOME_GUARD` before `acquireTestRunLock`, or make the SID lookup + fail closed instead of proceeding unguarded. Highest value: it is a safety + defect, not just a flake. +2. Diagnose `multi-account auth store` on Windows. +3. Decide whether `keep-native-v1` should assert on parsed argv rather than the + raw ComSpec string. + +Issue #3320 (non-ASCII account names misclassifying a valid scheduler task) is +adjacent to (1) — both are Windows identity handling — but it is `needs-info` +and was not reproduced here, so it stays open. diff --git a/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md b/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md new file mode 100644 index 0000000000..7c9e486298 --- /dev/null +++ b/devlog/_plan/260905_admin_token_local_ux/040_delivery_record.md @@ -0,0 +1,64 @@ +# 040 — Delivery record + +Work-phase `wp4`. All three PRs merged to `dev` with admin authority. + +| PR | merge SHA | what | +|---|---|---| +| [#3491](https://github.com/lidge-jun/opencodex/pull/3491) | `85e42117c` | #3483 empty red notice (CSS cascade) | +| [#3496](https://github.com/lidge-jun/opencodex/pull/3496) | `3e65218ba` | never prompt a local dashboard | +| [#3493](https://github.com/lidge-jun/opencodex/pull/3493) | `8b961b198` | #3353 dialog guidance + docs anchor | + +Ancestry proven for each: `git fetch origin dev` then +`git merge-base --is-ancestor FETCH_HEAD` exit 0. + +Issues #3483 and #3353 closed with landing comments. + +## The stack-parent close race + +#3492 was the original middle PR. Merging #3491 with `--delete-branch` removed +`codex/admin-token-empty-notice`, which was #3492's base, and GitHub auto-closed +it. A closed PR cannot be retargeted, and it could not be reopened because its +base branch no longer existed. #3496 carries the identical commits rebased onto +`dev`; #3492 records the supersession. + +**For the next stack: retarget children to `dev` BEFORE merging the parent, or +merge without `--delete-branch` and delete the branch afterwards.** The order +that looks natural — merge, then restack — is the one that loses the PR. + +## Trailing CI, classified + +Two failures on green-otherwise heads, both proven flakes rather than assumed: + +- `test 3/4` on `6da1269f8`: `tests/responses-state.test.ts:1464` through + `fallbackPendingResponseSpills` (`src/responses/state.ts:714`), which is the + `remaining <= 0` deadline-budget branch in a 2 MB shutdown-spill test — a + loaded-runner timeout. The diff touches no `responses/` file; the file passes + 141/141 locally twice; it passed on rerun. +- `macos` on `87abc9152`: `tests/shutdown-launcher.test.ts:147`. That PR changes + only GUI copy, i18n, and docs — no runtime code at all. Passes 3/3 locally; + passed on rerun. + +Both were rerun to green rather than merged over. + +## Verification actually run + +No repository-wide local suite (prohibited for this task). Instead: + +- `bun run typecheck` — clean +- `cd gui && bun test` — 1366 pass / 0 fail across 220 files +- `bun test tests/gui-static.test.ts tests/server-management-auth.test.ts tests/server-auth.test.ts` — 143 pass / 0 fail +- `bun run lint:gui`, `bun run privacy:scan` — clean +- exact-head CI green on `d9a1afc71`, `6da1269f8`, `87abc9152` before each merge + +The full GUI suite earned its place: it caught 7 tests in `api-auth-memory` +that assumed the prompt always fires, which the focused files did not cover. + +## One correction worth recording + +The first implementation gated the prompt on `runtimeRole === "hub"`. A review +lane checking "would this lock anyone out?" found that it would: `standalone` + +`hostname: "0.0.0.0"` is an operator who deliberately exposed the dashboard and +must type the token, and `tests/server-management-auth.test.ts` already proves +that bind mints no session. The role is topology; the question is the bind. The +shipped predicate is `isApiAuthRequired`, the same one the server gates the mint +with, and a regression now covers the exposed-standalone case explicitly.