security: dashboard auth token + brutal-honesty remediation pass - #68
Merged
Conversation
…/quality/tests) The dashboard served full transcript text behind only a browser-oriented guard (Host/Sec-Fetch-Site/Origin) — no defense against a non-browser local process. It now requires a per-session token, minted and delivered exactly like admin's (ADR-0007), including a query-param fallback for the two SSE routes since EventSource cannot set headers. Also fixes a cluster of correctness/hygiene issues found by the same multi-reviewer audit (security, code-quality, test-quality): - SSE client-cap TOCTOU + permanent leak on early client abort; shared transport extracted to src/lib/dashboard/sse.mjs so the fix lands once - shell:true on Windows in exec.mjs, reachable by provider/model strings from kit.json; replaced with resolved-argv + shell:false always - writeJsonWithBackup was truncate-then-write; now atomic tmp+rename - build-check.mjs could report "0 shipped files" as a pass when git ls-files failed; now throws + asserts a floor - secret-masker missed quoted-JSON and YAML/TOML credential shapes - /api/session/:id reparsed the whole usage-index cache per request; now memoized on mtime with an id->file index, plus age-based cache pruning - readIndex's memo key omitted `force`, so a forced refresh could return a stale cached aggregate - admin's DNS-rebinding guard was Host-only despite a comment claiming parity with dashboard's three-layer guard; now shares the real one - dashboard had no CSP; nosniff was inconsistent across JSON routes - CLI dispatch tables were plain objects (`ak toString` threw a raw stack trace via Object.prototype); now Object.create(null) - --dry-run in `ak setup` unconditionally wrote kit.json to disk anyway - coverage floors enforced via node --test --experimental-test-coverage - flaky fixed-sleep waits in live-transcript/live-service tests replaced with condition polling; hand-rolled .cjs harnesses gained a test-count floor so a silently deleted test block can no longer pass - new test coverage for setup/status/sync/x-verify/uninstall/exec, the previously untested code that actually writes into $HOME See docs/adr/0014-dashboard-auth-and-remediation.md for full rationale.
…e writes under --dry-run
Two bugs found by the test-coverage agent's work on setup/status, verified
and fixed here:
- status.mjs's collect({ cwd }) passed process.cwd() into dbPathPinStatus()
instead of the cwd it was actually given, so a caller that collects for a
different directory than the CLI's own cwd (the dashboard does exactly
this) got a memory-pin row describing the wrong project.
- bin/agentic-kit.mjs's post-command drift nudge ran unconditionally,
including under --dry-run. It shells `npm view`, which writes to npm's own
cache (~/.npm/_cacache, ~/.npm/_logs) as a side effect of the network
call — contradicting "--dry-run: prints the plan, changes nothing" even
though no ak-managed path was touched. Verified live: the old code added a
new npm log file on `ak setup --dry-run`; the fix adds none.
Both regressions are covered: a status-command.test.mjs case that fails
under the old process.cwd() code (proven by reverting the fix locally and
re-running), and a new dry-run-nudge.test.mjs that spawns the real CLI with
a fake npm shim on PATH to prove npm is never invoked under --dry-run.
…oken in LIVE-SESSIONS.md - README's top-of-file command summary listed every other command's full flag surface (setup: 7 flags, uninstall: 5) but silently dropped ak dashboard's --live-source, even though it's real, documented in the command's own --help, and covered in the detailed feature table below. Not deliberate — just inconsistent with the rest of the block. - LIVE-SESSIONS.md's Privacy section predated the per-session token (this branch's ADR-0014) and still read as if Host/Origin/Fetch-Metadata checks were the whole access-control story. Those stop a hostile browser tab, not another local non-browser process — which is exactly what the token closes. Now stated explicitly, plus a 401 row in the troubleshooting table.
… is shown The fact that Claude/Codex are auto-discovered while ruflo/agentic-qe/ dual-run are not was already documented correctly, but only ~160 lines in (Evidence and limitations) and in the troubleshooting table — both past the point a reader asking "what happens if I don't pass these flags" would naturally stop reading. Added as a GitHub-flavored NOTE directly after the usage example, matching this repo's existing alert-callout convention.
pacphi
added a commit
that referenced
this pull request
Jul 28, 2026
…dashboard gate, qe-court self-heal (#69) * fix(dashboard): gate overlay never hides due to missing [hidden] CSS rule .gate had display:flex with no [hidden] override, so the auth gate rendered on top of the dashboard permanently regardless of token validity — every sibling gated element in this file (.menu, .badge, .panel, .segbadge, .view, .live-health, .live-empty, .sdetail) already follows this pattern; .gate was the one omission (from PR #68). Verified with Playwright: before the fix, computed display was "flex" even with the hidden attribute set (token accepted); after, "none". * fix: aqe fallback-chain credential checks, seeded-routing divergence detection, ruvector drift management Closes #54 — ak now checks every aqe fallback rung against an actual credential before writing/reporting it: applyAqeRouter warns (never refuses) on a keyless rung, ak x provider pick warns interactively, ak status reports a WARN providers row when any rung lacks a credential, and openrouter is now visible in ak x provider status's provider table. Closes #55 — seeded per-activity routing pins that diverge from current DEFAULT_ROUTES are now surfaced (info severity, never "stale"/"outdated" per the issue's explicit framing) via divergedRoutes(), with a new `ak x provider refresh` to re-seed per-activity on demand. ak sync never auto-refreshes. MODEL_CATALOG notes now distinguish per-token price from per-task cost. aqeFallback entries carry source provenance. Also adds standalone ruvector (global npm CLI) drift detection/upgrade, opt-in and separate from the nested ruflo/aqe copies already managed indirectly, wired into ak status/sync and the dashboard. 824+ tests added/passing; full suite green. * fix(qe-court): temporary self-heal for writerIsNeverJuror collision agentic-qe's own shipped default qe-court config.json violates its own writerIsNeverJuror invariant (defense: cognitum-low, jury: cognitum-high resolve to the same vendor), so a brand-new project fails validation before any user touches the file. Filed upstream: proffesor-for-testing/agentic-qe#576. Until that's fixed, ak status flags a fixable violation and ak sync reassigns `jury` to an already-configured distinct-vendor seat (preferring deeperReviewer), backing up the original first. Never invents a vendor the project hasn't configured, never touches other keys. qeCourtShipped() consolidated into qeCourt.mjs to avoid a second copy of the version gate. * fix(test): status-viability's withoutEnv raced its own restore against collect() withoutEnv() wasn't async and didn't await fn(), so its `finally` fired the instant fn() returned a still-pending promise — before collect()'s own awaited work (driftReport() et al.) had reached the credential check — clobbering the env vars the test had just set before credentialGaps ever read them. This stayed invisible on any machine exporting a real credential env var (e.g. a personal OPENROUTER_API_KEY): the premature restore landed on that real value instead of `undefined`, and a real key is just as truthy as the test's injected 'sk-test', so the assertion passed for the wrong reason. CI has no such variable, so the race was exposed: 9/9 test matrix jobs failed identically on 'warn' !== 'ok'. Verified locally both with and without a personal OPENROUTER_API_KEY present. Root-caused via a targeted debug run (not guessed) — see run https://github.com/pacphi/agentic-kit/actions/runs/30401871783.
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
1 participant
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.
Summary
A multi-reviewer brutal-honesty audit (independent security, code-quality, and test-quality passes) found that
ak dashboardserved full local transcript text behind only a browser-oriented guard (Host/Sec-Fetch-Site/Origin) — no defense against another local, non-browser process reaching the port directly. Meanwhileak admin, serving a strictly less sensitive payload, already required a per-session token. This PR closes that gap and fixes the cluster of correctness/hygiene issues the same audit surfaced, concentrated in the parts of the codebase that shipped fastest (live-sessions dashboard, usage-index).Full rationale:
docs/adr/0014-dashboard-auth-and-remediation.md.Commits
21ee1d9shell:trueon Windows, non-atomic settings writes, a build-gate that could pass having checked nothing, secret-masker gaps, an O(n) session lookup, a stale-cache bug onforce, prototype-pollution-shaped CLI dispatch, enforced coverage floors, flaky-sleep tests, silent test-count erosion, and new test coverage for the previously-untestedsetup/status/sync/verify/uninstall/execmodules.2c840c3status.mjs's memory-pin check readprocess.cwd()instead of thecwdit was actually given (wrong project when a caller like the dashboard collects for a different directory);ak setup --dry-runwas still writing to~/.npm/**via the drift-nudge'snpm viewcall, contradicting "--dry-run: changes nothing". Both covered by regression tests, both verified to fail against the pre-fix code.20defe5ak dashboard(silently dropped--live-source);LIVE-SESSIONS.md's Privacy section predated this PR's token and still read as if Host/Origin checks were the whole access-control story.1648509[!NOTE]directly afterLIVE-SESSIONS.md's usage example, stating plainly that--live-sourceis opt-in — Claude/Codex are auto-discovered either way, only ruflo/agentic-qe/dual-run require it. The fact was already documented correctly but ~160 lines further into the file.Impact
/api/statusor/api/usagedirectly now needs the per-session token (header, or?token=for the two SSE routes, sinceEventSourcecan't set headers). The browser UI itself handles this transparently — paste-once, thenlocalStorage-persisted, identical toak admin's existing flow.ak admin's behavior beyond reusing its exportedtokenMatchesand porting its full 3-layerrequestRejectionguard (it previously only had the Host-header layer).pnpm test): 70% lines/branches/functions ontests/kit/*.test.mjs, set at the measured level at merge time — not an aspirational number.ak setup/ak syncoutside of--dry-runnow being genuinely side-effect-free.Test plan
pnpm run check(typecheck, lint, markdownlint, build, coverage-enforced test suite) — green on every commit in this branch.ak dashboardprocess, curl'd with missing/wrong/right token (401/401/200), SSE query-param token auth, CSP/nosniff headers present.~/.claudeand~/.config/agentic-kitconfirmed untouched (mtime + file-count checks) before and after the full suite run, including the new sandboxed command-module tests.