-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(tests): arm the home guard before the run lock can throw #3507
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
120 changes: 120 additions & 0 deletions
120
devlog/_plan/260905_admin_token_local_ux/000_research.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| <meta name="opencodex-session-token" content="ocx_session_CO-4g0m5B_..."> | ||
| ``` | ||
|
|
||
| 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 <file>` | ||
| 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. |
101 changes: 101 additions & 0 deletions
101
devlog/_plan/260905_admin_token_local_ux/010_suppress_local_prompt.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 \`<meta name="opencodex-runtime-role" content="\${escapeHtmlAttribute(runtimeRole)}">\`; | ||
| } | ||
| ``` | ||
|
|
||
| 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:<port>`), 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. | ||
63 changes: 63 additions & 0 deletions
63
devlog/_plan/260905_admin_token_local_ux/020_dialog_repair.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
82 changes: 82 additions & 0 deletions
82
devlog/_plan/260905_admin_token_local_ux/030_windows_baseline.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Update the plan to match the shipped bind-based gate.
This section still defines prompt eligibility as
runtimeRoleFromDocument() === "hub"and states that everystandalonedashboard must suppress the prompt.devlog/_plan/260905_admin_token_local_ux/040_delivery_record.mdLines 58-64 records that this rule was rejected:standalonewithhostname: "0.0.0.0"is an exposed bind that requires a token, and the shipped predicate isisApiAuthRequiredfromsrc/server/auth-cors.tsLines 285-287.If this plan remains an active implementation contract, a future change can reintroduce an authentication dead end for exposed standalone deployments. Describe the bind-based predicate here, or mark the role-based section as superseded and add the exposed-standalone case to the verification matrix.
Also applies to: 50-52
🤖 Prompt for AI Agents