From 1e3c9522fc11e13622b6ed0b3e947a3a99c95c53 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 11:52:49 -0700 Subject: [PATCH 01/19] security(audit): own the repository by subtraction, not by a list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The permissions FAIL from #447 is cleared — `default_workflow_permissions` is `read` and `can_approve_pull_request_reviews` is `false`. Re-running the ci-and-secrets domain confirmed it, and turned up the next layer. The scope-union FAIL IF was violated by its own fix. Naming the paths each domain owns meant the list went stale the moment a top-level directory appeared: `.vscode/` and `.impeccable/` were owned by nobody, in the prompt files and in SECURITY.md alike. `application-security` is now the catch-all defined by subtraction — everything `supply-chain.md` and `ci-and-secrets.md` do not explicitly claim, worked out from `ls -A` rather than trusted from an enumeration. The lists that remain are descriptions of today's tree, labelled as such. Two paths get explicit owners on the way: - `.vscode/` to ci-and-secrets. It is configuration that executes: a `tasks.json` entry with `"runOn": "folderOpen"` runs on checkout when a maintainer opens the folder — the persistence shape `workflow-audit` watches workflows for, reached without touching `.github/`. There is no such task today; the point is that adding one should be a finding. - `website/scripts/` to supply-chain, which already owns the output of the generator living there. Owning the output without the generator is half a check. `workflow-audit.yaml`'s window grows to cover `.vscode/` for that reason, and `SECURITY.md` itself — weakening a `FAIL IF` and weakening the workflow that runs it are the same act one file apart, and only one of them was being watched. Also corrects a statement that had gone stale: max-sixty/tend#1005 merged upstream on 2026-08-22, where this document still said "still open". The gap is unchanged in practice — the latest release is 0.1.18, cut 2026-08-16, so the fix is merged but unreleased — but the text claimed a longer path than the one that remains. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/audit/application-security.md | 23 ++++++++++++++++------- .github/audit/ci-and-secrets.md | 14 +++++++++++--- .github/audit/supply-chain.md | 4 +++- .github/workflows/workflow-audit.yaml | 13 +++++++++++-- SECURITY.md | 12 ++++++------ 5 files changed, 47 insertions(+), 19 deletions(-) diff --git a/.github/audit/application-security.md b/.github/audit/application-security.md index 530baca0..de2b4fd3 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -40,10 +40,19 @@ gap is known (revocation, the audit trail, the two `workflow-audit` window evasions), do not re-report it as a finding — report only if the situation has changed or is worse than described. -You also own the **rest of the repository** qualitatively, so that no top-level -path is outside every domain: `lib/`, `server/`, `server-lib-common/`, -`standalone/`, `vscode-ext/`, `dor/`, `dor-lib-common/`, `canopy/`, `deploy/`, -`docs/`, and the root files. Remote control is where the depth goes; the rest -is a sweep for anything that would be a security hole in a terminal that runs -local shells — command construction, path handling, deserialization of -persisted state, IPC that crosses a trust boundary. +You are also the **catch-all** domain, and this is defined by subtraction, not +by a list: you own every top-level path in the repository that +`supply-chain.md` and `ci-and-secrets.md` do not explicitly claim. Run `ls -A` +and work out the remainder rather than trusting any enumeration — an +enumeration goes stale the moment someone adds a directory, which is exactly +how `.vscode/` and `.impeccable/` ended up owned by nobody. + +Today the remainder is `lib/`, `server/`, `server-lib-common/`, `standalone/`, +`vscode-ext/`, `dor/`, `dor-lib-common/`, `canopy/`, `deploy/`, `docs/`, +`.impeccable/`, and the root files — but treat that as a description of the +current tree, not as your scope. Your scope is the remainder. + +Remote control is where the depth goes; the rest is a sweep for anything that +would be a security hole in a terminal that runs local shells — command +construction, path handling, deserialization of persisted state, IPC that +crosses a trust boundary. diff --git a/.github/audit/ci-and-secrets.md b/.github/audit/ci-and-secrets.md index 20572a80..9d37f0d4 100644 --- a/.github/audit/ci-and-secrets.md +++ b/.github/audit/ci-and-secrets.md @@ -35,6 +35,14 @@ write scopes. ## Qualitative pass You own `.github/` (including `.github/audit/`, which holds this audit's own -prompts), `.config/`, `.claude/`, `scripts/`, and `website/public/` — the Tauri -updater manifest shipped apps fetch lives there, so it is a release artifact -rather than marketing. You also own any code anywhere that touches a secret. +prompts), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and +`website/public/` — the Tauri updater manifest shipped apps fetch lives there, +so it is a release artifact rather than marketing. You also own any code +anywhere that touches a secret. + +`.vscode/` is here rather than with the product code because it is +configuration that can execute: a `tasks.json` entry with +`"runOn": "folderOpen"` runs on checkout when a maintainer opens the folder, +which is the same shape of persistence `workflow-audit.yaml` watches workflows +for. There is no such task today; the point is that adding one should be a +finding, not a quiet config change. diff --git a/.github/audit/supply-chain.md b/.github/audit/supply-chain.md index 4f95d297..cf117567 100644 --- a/.github/audit/supply-chain.md +++ b/.github/audit/supply-chain.md @@ -25,7 +25,9 @@ enumeration is the shortcut that goes stale. ## Qualitative pass -You own the dependency graph, the lockfile, and `website/src/`. Look at: +You own the dependency graph, the lockfile, `website/src/`, and +`website/scripts/` — the generator whose output the first `FAIL IF` checks +lives there, and owning the output without the generator is half a check. - newly added or upgraded runtime dependencies since the last audit - anything in the lockfile that resolves outside the registry diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index 89c02516..3d2d4de0 100644 --- a/.github/workflows/workflow-audit.yaml +++ b/.github/workflows/workflow-audit.yaml @@ -1,7 +1,7 @@ name: workflow-audit # Nightly audit of every commit touching .github/workflows/, -# .config/tend.yaml, or .github/audit/. Surfaces changes from feature branches and direct +# .config/tend.yaml, .github/audit/, .vscode/, or SECURITY.md. Surfaces changes from feature branches and direct # pushes, not just the main branch — so a bot push that adds a new # workflow file gets a visible issue even if it never opens a PR. # @@ -95,6 +95,14 @@ jobs: # does not generate this directory), so anything landing here is # reported on its own content, which is the intent. # + # .vscode/ is in the window because `tasks.json` can carry + # `"runOn": "folderOpen"`, which executes when a maintainer opens the + # folder — the same persistence-on-checkout shape a workflow gives, + # reached without touching .github/. SECURITY.md is in it because it + # is the document the nightly security audit enforces: weakening a + # `FAIL IF` and weakening the workflow that runs it are the same act + # one file apart, and only one of them was watched. + # # .config/tend.yaml is in the window, not just .github/workflows/. # Without it, a commit editing only the config never enters the audit, # and a later commit regenerating from it leaves the config untouched, @@ -105,7 +113,8 @@ jobs: # so nothing in the widened window can be swallowed by an arm that # doesn't inspect it — that pairing is the invariant, not either half. COMMITS=$(git log --all --since="$SINCE" --pretty=format:'%H' \ - -- .github/workflows/ .config/tend.yaml .github/audit/ | sort -u) + -- .github/workflows/ .config/tend.yaml .github/audit/ \ + .vscode/ SECURITY.md | sort -u) if [ -z "$COMMITS" ]; then echo "No workflow or tend-config changes since $SINCE." diff --git a/SECURITY.md b/SECURITY.md index 50d0a0fe..5d0d6b3f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -170,7 +170,7 @@ An attacker who lands a prompt injection in tend's harness can reach three secre **Prompt-injection through user-supplied content.** tend's harness reads PR descriptions, code diffs, issue text, comments, and CI logs — all attacker-influenceable surfaces. A malicious prompt could direct the harness to push a workflow that references a repo-level secret to an external URL. The bot cannot merge to `main` or push tags, so admin-gated release paths stay sealed, but a workflow on a bot-pushed feature branch will still execute with repo-level secrets in scope. -**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. The control is only as complete as its path list: this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so a list naming only `CLAUDE.md` reverts a pointer and leaves the content it points at attacker-controlled. `AGENTS.md` is absent from that list at the pinned `0.1.18`, and every entry there is root-relative. Reported from this audit; a fix with a regression test is committed on [max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005), which is **still open**. It reaches us only once upstream merges it, cuts a release, and the nightly regen bumps the pin — three steps outside this repo's control, so treat the gap as live rather than as closing on a schedule. There *is* a local remedy, and it is the reason to state this precisely: the regen overwrites the *workflow*, not this repository's instruction files. `CLAUDE.md` **is** on the restored path list, so moving the instruction body into `CLAUDE.md` and dropping the `@AGENTS.md` pointer closes the gap today, permanently, with no upstream dependency — at the cost of the filename convention that other agent harnesses read. Until that trade is made or the upstream fix lands, the gap is live. Note that the control's completeness is a property of the pinned upstream version, not of anything in this repo: if the instructions ever move to a filename that list does not name, the revert silently stops covering them. +**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. The control is only as complete as its path list: this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so a list naming only `CLAUDE.md` reverts a pointer and leaves the content it points at attacker-controlled. `AGENTS.md` is absent from that list at the pinned `0.1.18`, and every entry there is root-relative. Reported from this audit; a fix with a regression test **merged upstream** as [max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005) on 2026-08-22. It has not reached us: the latest release is still `0.1.18`, cut 2026-08-16, so the fix is merged but unreleased. Two steps remain outside this repo's control — upstream cutting a release, and the nightly regen bumping the pin — so the gap is live, just shorter than it was. There *is* a local remedy, and it is the reason to state this precisely: the regen overwrites the *workflow*, not this repository's instruction files. `CLAUDE.md` **is** on the restored path list, so moving the instruction body into `CLAUDE.md` and dropping the `@AGENTS.md` pointer closes the gap today, permanently, with no upstream dependency — at the cost of the filename convention that other agent harnesses read. Until that trade is made or the upstream fix lands, the gap is live. Note that the control's completeness is a property of the pinned upstream version, not of anything in this repo: if the instructions ever move to a filename that list does not name, the revert silently stops covering them. **Credential isolation bounds an injection.** The agent runs as a separate, non-sudo sandbox user behind a local credential-injecting proxy: `TEND_BOT_TOKEN` and the Anthropic credential live only in the proxy and never enter the agent's environment, its disk, or `.git/config` (the setup strips the credential `actions/checkout` persists there). An injected instruction can therefore make the bot *act* within its permissions — comment, push a feature branch — but cannot read the token value out and exfiltrate it. The worst-case analysis above is about what the bot's identity can do, not about the secret escaping. @@ -264,12 +264,12 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the audit stops fanning out to a dedicated application-security subagent scoped to **Remote Control**, or that subagent's scope is merged back into a context that also carries the supply-chain or CI domains. Folding it back in is how that section stops being audited without anyone deciding to stop auditing it. - FAIL IF the orchestrator prompt stops requiring a non-turn-ending wait — a Bash `until` loop over the fragment files, re-issued past the ten-minute Bash cap, under a bounded deadline that is **persisted to a file** rather than recomputed from `now`. A deadline longer than the ten-minute cap cannot fire inside one call, so a re-issued loop that recomputes it never reaches it: the bound is then written down but never binds, and the only thing ending the wait is the runner's cancellation. Delegating is safe; ending the turn to wait is what kills the run, and no tool allowlist prevents it. - FAIL IF `.github/audit/` is missing a prompt file the workflow names, or `scripts/security-audit-local.sh` stops running the audit from those same files. A local runner with its own copy of the prompts is worse than no local runner: it drifts, and the drift is invisible until a nightly disagrees with a local pass. -- FAIL IF `.github/audit/` is outside `workflow-audit.yaml`'s diff window. The prompts decide what gets audited and by whom, so a change to them is a change to the security automation — the same reason `.config/tend.yaml` is in that window. +- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside `workflow-audit.yaml`'s diff window. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. - FAIL IF a `## ` section of this document is in no subagent's scope, or is in two. Every section is owned by exactly one domain: a section owned by none is unaudited, and one owned by two produces contradictory verdicts. Each domain file in `.github/audit/` names its sections as exact `## ` headings on their own lines, so this is a real grep over four markdown files rather than a reading of prose embedded in YAML. -- FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division: - - `application-security` — `lib/`, `server/`, `server-lib-common/`, `standalone/`, `vscode-ext/`, `dor/`, `dor-lib-common/`, `canopy/`, `deploy/`, `docs/`, and the repository root files. - - `ci-and-secrets` — `.github/`, `.config/`, `.claude/`, `scripts/`, and `website/public/` (the updater manifest is a release artifact, not marketing). - - `supply-chain` — the dependency graph, the lockfile, and `website/src/`. +- FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: + - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. + - `supply-chain` — the dependency graph, the lockfile, `website/src/`, and `website/scripts/` (owning the generator's output without the generator is half a check). + - `application-security` — **everything else**, worked out from `ls -A` rather than from a list. An enumeration here goes stale the moment a top-level path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. From 0badaadc6dac6e72bc706ed511e1be09442ea0f9 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 12:02:16 -0700 Subject: [PATCH 02/19] security(release): keep the Tauri updater key off argv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `tauri signer sign` was given `--private-key "$TAURI_SIGNING_PRIVATE_KEY"` while the same line already exported that variable into its environment. The flag documents itself as falling back to `[env: TAURI_SIGNING_PRIVATE_KEY]`, so it bought nothing and cost a production signing key on a command line — readable via `ps` by anything on the machine for the life of the call, and `pnpm exec` means every dependency's lifecycle scripts share that session. `jsign --storepass` stays. Checked its `--help`: the password is only accepted as a literal option value, with no environment or file indirection, so there is nowhere else to put it. Documented as accepted rather than left to be rediscovered — the residual is local `ps` for the duration of the call, for a PIN that is inert without the physical YubiKey. Both facts are now pinned by a FAIL IF, with jsign named as the one exception so the rule does not read as violated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- SECURITY.md | 3 +++ scripts/sign-and-deploy.sh | 13 ++++++++++++- 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 5d0d6b3f..b93c2aeb 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -218,7 +218,10 @@ The VS Code extension is published by GitHub Actions. The secrets which allow th Desktop releases are not fully automated. GitHub Actions builds unsigned artifacts, publishes attestations and hash manifests, and uploads those unsigned artifacts for local release signing. Final desktop deployment is manual through `scripts/sign-and-deploy.sh`. Before signing, the script verifies the CI artifact attestations and the recorded SHA-256 hashes. The local machine then performs platform signing and uploads the final release assets. Windows Authenticode signing requires a physical YubiKey and the signing PIN. macOS signing and notarization also happen locally, outside GitHub Actions. CI must not have the production Tauri updater private key; CI uses only an ephemeral updater key so Tauri emits updater-shaped unsigned artifacts. Tauri updater signing is applied locally after OS signing so the updater signs the final release bundles that users will download. +**Signing credentials and argv.** Both signing secrets are supplied to the script through the environment, and neither should reach a command line: argv is readable via `ps` by any process on the machine for the lifetime of the call, and `pnpm exec` means a dependency's lifecycle scripts share that session. The Tauri updater key is env-only — `tauri signer sign` documents `--private-key` as falling back to `TAURI_SIGNING_PRIVATE_KEY`, so passing both was redundant. The Windows PIV PIN is the exception and is **accepted rather than solved**: `jsign` reads `--storepass` only as a literal option value, with no environment or file indirection, so there is nowhere else to put it. The residual is bounded — local `ps` for the duration, on a single-user machine, for a PIN that is inert without the physical YubiKey it unlocks. + - FAIL IF `scripts/sign-and-deploy.sh` stops doing any of three things: verifying GitHub artifact attestations, verifying artifact SHA-256 manifests, or using PIV-backed Windows signing. +- FAIL IF `TAURI_SIGNING_PRIVATE_KEY` is passed on a command line anywhere in `scripts/sign-and-deploy.sh` rather than through the environment. `jsign --storepass` is the one documented exception, for the reason above. ## Reporting a Vulnerability diff --git a/scripts/sign-and-deploy.sh b/scripts/sign-and-deploy.sh index ff6d38b9..e95a1019 100755 --- a/scripts/sign-and-deploy.sh +++ b/scripts/sign-and-deploy.sh @@ -691,6 +691,12 @@ sign_windows() { local exe_path exe_path=$(windows_exe_path) + # `--storepass` stays on argv because jsign offers no alternative: it reads + # the password only as a literal option value, with no environment or + # file indirection (checked against jsign's own `--help`). The exposure is + # `ps` on this machine for the duration of the call, and the PIN alone is + # inert without the physical YubiKey it unlocks. Accepted, not overlooked — + # see SECURITY.md, "Desktop Releases". log "Signing inner executable: $exe_path" jsign \ --storetype PIV \ @@ -760,10 +766,15 @@ sign_updates() { if [[ -f "$bundle" ]]; then log "Tauri-signing: $(basename "$bundle")" # Use tauri signer to sign the bundle + # The key goes in the environment and NOT on argv. `tauri signer + # sign` documents `--private-key` as falling back to + # `[env: TAURI_SIGNING_PRIVATE_KEY]`, so the flag was redundant — + # and argv is world-readable through `ps` for the life of the + # process, which matters more here than usual: `pnpm exec` means + # every dependency's lifecycle scripts share this session. TAURI_SIGNING_PRIVATE_KEY="$TAURI_SIGNING_PRIVATE_KEY" \ TAURI_SIGNING_PRIVATE_KEY_PASSWORD="${TAURI_SIGNING_PRIVATE_KEY_PASSWORD:-}" \ pnpm --dir "$REPO_ROOT/standalone" exec tauri signer sign \ - --private-key "$TAURI_SIGNING_PRIVATE_KEY" \ "$bundle" fi done From a855460a24d8c84abdac256b4385bf453884b62f Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 12:14:05 -0700 Subject: [PATCH 03/19] security: enable secret scanning and push protection, and let Renovate act on CVEs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Repository settings, applied and verified: - secret scanning: enabled. The history backfill finished with **0 alerts**, so nothing has ever been committed that GitHub recognizes as a credential. - push protection: enabled. This is the one control that acts before a secret lands rather than after — it blocks a push whose diff carries a recognized provider token, and it applies to `dormouse-bot` as much as to a human, which is the point: an injected agent pasting a token into a file is exactly the shape it stops. Bypass exists and is logged, so it cannot wedge a release. - Dependabot alerts: enabled. Alerts only — not `dependabot_security_updates`, which opens PRs and would duplicate Renovate. Left off deliberately: `non_provider_patterns` (entropy-based, high false positive rate — worth adding once the provider-pattern signal is understood, not on the same day) and `validity_checks` (transmits candidate secrets to third-party validation endpoints; a real choice, not a free win). Renovate gets a `vulnerabilityAlerts` block so a known-vulnerable dependency does not wait for the Monday batch window. The judgement worth recording is what it does NOT override: `minimumReleaseAge` stays in force. That cooldown guards the opposite threat — a compromised release yanked within a day — and dropping it would trade a supply-chain control for patch latency we do not need, since nothing auto-merges, a human reviews every Renovate PR, and the Dependabot alert already makes the vulnerability visible the moment it is published. The PR is the remediation, not the notification. All three settings are now pinned by a FAIL IF, so turning any of them back off is a finding rather than a quiet change. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/renovate.json | 10 ++++++++++ SECURITY.md | 2 ++ 2 files changed, 12 insertions(+) diff --git a/.github/renovate.json b/.github/renovate.json index 03c1b079..ce9f2cca 100644 --- a/.github/renovate.json +++ b/.github/renovate.json @@ -4,6 +4,16 @@ "dependencyDashboard": true, "timezone": "America/Los_Angeles", "schedule": ["* * * * 1"], + "vulnerabilityAlerts": { + "description": [ + "Security fixes must not wait for the Monday window. Everything else here is batched to one day a week, which is right for routine bumps and wrong for a known-vulnerable dependency, so this overrides the schedule only.", + "The `minimumReleaseAge` cooldown is deliberately NOT overridden. It exists for the opposite threat — a compromised release that gets yanked within a day — and dropping it for security PRs would trade a supply-chain control for patch latency we do not actually need: nothing here auto-merges, a human reviews every Renovate PR, and Dependabot alerts already make the vulnerability visible the moment it is published. The PR is the remediation, not the notification.", + "See SECURITY.md, Dependency Supply Chain." + ], + "enabled": true, + "schedule": [], + "labels": ["security", "dependencies"] + }, "packageRules": [ { "matchManagers": ["npm", "cargo"], diff --git a/SECURITY.md b/SECURITY.md index b93c2aeb..d8c96a25 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -146,6 +146,8 @@ The standalone app ships a Node.js runtime binary (`standalone/src-tauri/build.r - FAIL IF the `build-standalone` job in `.github/workflows/release.yml` does not install the pinned runtime via `node-version-file: package.json`, **or** the root `package.json` gains a `volta.node` or `engines.node` field. `setup-node` resolves that file by precedence (`volta.node` -> `devEngines.runtime` -> `engines.node`), so the pin this document relies on is the one it reads only while the higher-precedence fields are absent — adding one would silently change the bundled runtime with no diff to the workflow. Other jobs may pin `node-version` inline since their interpreter is never bundled. - FAIL IF `pnpm-workspace.yaml` is missing `minimumReleaseAge: 1440`. - FAIL IF `.github/renovate.json` is missing `npm` or `cargo` from `enabledManagers` (npm covers `/`; cargo covers `/standalone/src-tauri`), or is missing `minimumReleaseAge` package rules for those managers (the Renovate equivalent of dependency cooldown windows). +- FAIL IF `.github/renovate.json` has no `vulnerabilityAlerts` block, or that block overrides `minimumReleaseAge`. Routine bumps are batched to a weekly window, which is wrong for a known-vulnerable dependency — hence the block. But the cooldown stays: it guards the *opposite* threat, a compromised release yanked within a day, and nothing here auto-merges, so the delay it costs is not delay in fixing anything. Dependabot alerts are what make a vulnerability visible immediately; the Renovate PR is the remediation. +- FAIL IF secret scanning or its push protection is disabled on the repository (`gh api repos/diffplug/dormouse --jq .security_and_analysis`), or Dependabot alerts are off (`GET /repos/diffplug/dormouse/vulnerability-alerts` must answer 204, not 404). Push protection is the one control that acts *before* a credential lands: it blocks a push whose diff carries a recognized provider token, and it applies to `dormouse-bot` too — which is the point, since an injected agent pasting a token into a file is exactly the shape it stops. ## GitHub Actions Policies From e66feeec1120163fad017fd67a4e55a6f6115fc7 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 13:16:55 -0700 Subject: [PATCH 04/19] security: fix three claims the audit proved wrong MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three domains passed, and each returned a WARNING that this document was asserting something untrue. Every one is verified against source before the fix. **The Renovate cooldown did the opposite of what I documented.** The previous commit omitted `minimumReleaseAge` from `vulnerabilityAlerts` on the reasoning that omission preserves the `packageRules` cooldown. It does not: Renovate's built-in default for that block is `minimumReleaseAge: null`, force-applied before lookup (confirmed in `lib/config/options` in renovate's own source), so omitting the key DROPS the cooldown. It is now set explicitly to `1 day`, and the FAIL IF requires it explicitly rather than forbidding an override — the inverted rule that let this pass. **"Both signing secrets are env-only" was wrong twice over.** There are three, and two are on argv. `APPLE_SIGN_PASS` goes to `xcrun notarytool --password` and, with `--wait --timeout 30m`, sits on the command line for up to half an hour per architecture — a standalone credential, unlike the PIV PIN that is inert without its YubiKey. Each of the three is now listed with its actual state. The notarytool remedy (`store-credentials` + `--keychain-profile`) is named but not applied: it changes the release runbook and cannot be exercised without live Apple credentials. **"Server compromise — no Host access" read stronger than what holds.** It cannot create an authorized Client, which is what the sentence meant. But `RemoteHost.#onMsg` gates `msg` frames on `state.established` for a `clientId` the relay itself minted, with no per-frame authentication, so a compromised Server can fabricate frames on an open session and reach `terminal.write` — keystroke injection into a live PTY — as well as suppress or rewrite in either direction. Architectural, following from the relay being a dumb pipe, and the staged PRF session key is what closes it. The bound is that compromise cannot create an authorized Client, not that it cannot act through one. Also: the catch-all subtraction was top-level only, so `website/`'s own config files fell through the gap between three partial claims on that tree. It is recursive now. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/audit/application-security.md | 18 +++++++++++++----- .github/renovate.json | 4 +++- SECURITY.md | 14 ++++++++++---- 3 files changed, 26 insertions(+), 10 deletions(-) diff --git a/.github/audit/application-security.md b/.github/audit/application-security.md index de2b4fd3..e0cda383 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -41,11 +41,19 @@ evasions), do not re-report it as a finding — report only if the situation has changed or is worse than described. You are also the **catch-all** domain, and this is defined by subtraction, not -by a list: you own every top-level path in the repository that -`supply-chain.md` and `ci-and-secrets.md` do not explicitly claim. Run `ls -A` -and work out the remainder rather than trusting any enumeration — an -enumeration goes stale the moment someone adds a directory, which is exactly -how `.vscode/` and `.impeccable/` ended up owned by nobody. +by a list: you own everything in the repository that `supply-chain.md` and +`ci-and-secrets.md` do not explicitly claim. Run `ls -A` and work out the +remainder rather than trusting any enumeration — an enumeration goes stale the +moment someone adds a directory, which is exactly how `.vscode/` and +`.impeccable/` ended up owned by nobody. + +Subtraction is **recursive, not top-level**. Where another domain claims a +subdirectory rather than a whole tree, the rest of that tree is yours: the +other two claim `website/public/`, `website/src/`, and `website/scripts/`, so +`website/`'s remaining files — `vite.config.ts`, `react-router.config.ts`, +`package.json`, `tsconfig.json` — are yours. Check one level down wherever a +claim is partial, or the same orphaning happens again inside a directory +instead of beside it. Today the remainder is `lib/`, `server/`, `server-lib-common/`, `standalone/`, `vscode-ext/`, `dor/`, `dor-lib-common/`, `canopy/`, `deploy/`, `docs/`, diff --git a/.github/renovate.json b/.github/renovate.json index ce9f2cca..6a30d24b 100644 --- a/.github/renovate.json +++ b/.github/renovate.json @@ -7,11 +7,13 @@ "vulnerabilityAlerts": { "description": [ "Security fixes must not wait for the Monday window. Everything else here is batched to one day a week, which is right for routine bumps and wrong for a known-vulnerable dependency, so this overrides the schedule only.", - "The `minimumReleaseAge` cooldown is deliberately NOT overridden. It exists for the opposite threat — a compromised release that gets yanked within a day — and dropping it for security PRs would trade a supply-chain control for patch latency we do not actually need: nothing here auto-merges, a human reviews every Renovate PR, and Dependabot alerts already make the vulnerability visible the moment it is published. The PR is the remediation, not the notification.", + "`minimumReleaseAge` is set here EXPLICITLY, and must stay that way. Renovate\u0027s own default for this block is `minimumReleaseAge: null`, force-applied before lookup — so omitting it does not inherit the cooldown from packageRules, it DROPS the cooldown entirely. Stating it is the only way to keep it.", + "Keeping it is the deliberate choice: the cooldown guards the opposite threat — a compromised release that gets yanked within a day — and a reviewer reading a Renovate diff cannot detect a supply-chain compromise the way the ecosystem\u0027s own yank process can. Nothing here auto-merges, and the Dependabot alert already makes the vulnerability visible the moment it is published, so what the cooldown costs is a day before the remediation PR appears, not a day before anyone knows.", "See SECURITY.md, Dependency Supply Chain." ], "enabled": true, "schedule": [], + "minimumReleaseAge": "1 day", "labels": ["security", "dependencies"] }, "packageRules": [ diff --git a/SECURITY.md b/SECURITY.md index d8c96a25..b6af2580 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -14,7 +14,9 @@ The design lives in [`docs/specs/remote-security-model.md`](docs/specs/remote-se Four layers, none sufficient alone: a passkey proves fresh user presence, a non-extractable per-browser device key is long-lived Client identity, the Host's local ACL authorizes the *pair* of those two, and the Host makes the final access decision. What each compromise actually buys: -- **Server compromise** — relay traffic and account state, but **no Host access**. A forged account, a forged presence stamp, and an injected `ConnectionRequest` all still arrive in front of `authorizeConnection` on the Host, which re-verifies the passkey assertion and the device-key signature against its own ACL and its own `ConnectionPolicy` (the origin/rpId recorded at enrollment) regardless of what the Server claims to have checked. +- **Server compromise** — relay traffic and account state, and **no new authorization**: a forged account, a forged presence stamp, and an injected `ConnectionRequest` all still arrive in front of `authorizeConnection` on the Host, which re-verifies the passkey assertion and the device-key signature against its own ACL and its own `ConnectionPolicy` (the origin/rpId recorded at enrollment) regardless of what the Server claims to have checked. It cannot make the Host trust a device the user never approved. + + What it *does* buy, on any session an authorized Client already has open, is **read and write** — not read alone. After a decision the Host gates `msg` frames on `state.established` for a `clientId` the relay itself minted (`RemoteHost.#onMsg`), and there is no per-frame authentication, so a compromised Server can fabricate frames for an established session and reach `terminal.write` — keystroke injection into a live PTY — as well as suppress or rewrite frames in either direction. This follows directly from the relay being a dumb pipe with no end-to-end authentication (see Accepted limitations; the staged PRF-derived session key would close it). It is architectural, not a defect, but "no Host access" read stronger than what holds: the bound is that compromise cannot *create* an authorized Client, not that it cannot act through one. - **Setup-password compromise** — full account takeover: `/api/setup/*` is gated by the password alone, so re-presenting it registers another passkey, and `/api/host/enroll` mints Host credentials. Still **no Host access**: reaching an already-enrolled Host requires a pairing ceremony that a human approves in a modal on that laptop. - **A synced or stolen passkey** — sign-in, and the ability to *ask*. The paired device key is missing, so `HostAcl` answers `device-not-paired` and the Client reaches nothing. - **Device-key compromise** — requires a compromised browser or OS, or XSS in the Pocket origin. The key is usable in place but not extractable, and connecting still needs a fresh passkey assertion. @@ -146,7 +148,7 @@ The standalone app ships a Node.js runtime binary (`standalone/src-tauri/build.r - FAIL IF the `build-standalone` job in `.github/workflows/release.yml` does not install the pinned runtime via `node-version-file: package.json`, **or** the root `package.json` gains a `volta.node` or `engines.node` field. `setup-node` resolves that file by precedence (`volta.node` -> `devEngines.runtime` -> `engines.node`), so the pin this document relies on is the one it reads only while the higher-precedence fields are absent — adding one would silently change the bundled runtime with no diff to the workflow. Other jobs may pin `node-version` inline since their interpreter is never bundled. - FAIL IF `pnpm-workspace.yaml` is missing `minimumReleaseAge: 1440`. - FAIL IF `.github/renovate.json` is missing `npm` or `cargo` from `enabledManagers` (npm covers `/`; cargo covers `/standalone/src-tauri`), or is missing `minimumReleaseAge` package rules for those managers (the Renovate equivalent of dependency cooldown windows). -- FAIL IF `.github/renovate.json` has no `vulnerabilityAlerts` block, or that block overrides `minimumReleaseAge`. Routine bumps are batched to a weekly window, which is wrong for a known-vulnerable dependency — hence the block. But the cooldown stays: it guards the *opposite* threat, a compromised release yanked within a day, and nothing here auto-merges, so the delay it costs is not delay in fixing anything. Dependabot alerts are what make a vulnerability visible immediately; the Renovate PR is the remediation. +- FAIL IF `.github/renovate.json` has no `vulnerabilityAlerts` block, or that block does not set `minimumReleaseAge` **explicitly**. This reads backwards and is the whole point: Renovate's built-in default for `vulnerabilityAlerts` is `minimumReleaseAge: null`, force-applied before lookup, so *omitting* the key drops the cooldown rather than inheriting it from `packageRules`. Stating it is the only way to keep it. Keeping it is deliberate — the cooldown guards the opposite threat, a compromised release yanked within a day, which a reviewer reading a dependency diff cannot detect the way the ecosystem's own yank process can. Nothing auto-merges, so what it costs is a day before the remediation PR appears, not a day before anyone knows. - FAIL IF secret scanning or its push protection is disabled on the repository (`gh api repos/diffplug/dormouse --jq .security_and_analysis`), or Dependabot alerts are off (`GET /repos/diffplug/dormouse/vulnerability-alerts` must answer 204, not 404). Push protection is the one control that acts *before* a credential lands: it blocks a push whose diff carries a recognized provider token, and it applies to `dormouse-bot` too — which is the point, since an injected agent pasting a token into a file is exactly the shape it stops. ## GitHub Actions Policies @@ -220,7 +222,11 @@ The VS Code extension is published by GitHub Actions. The secrets which allow th Desktop releases are not fully automated. GitHub Actions builds unsigned artifacts, publishes attestations and hash manifests, and uploads those unsigned artifacts for local release signing. Final desktop deployment is manual through `scripts/sign-and-deploy.sh`. Before signing, the script verifies the CI artifact attestations and the recorded SHA-256 hashes. The local machine then performs platform signing and uploads the final release assets. Windows Authenticode signing requires a physical YubiKey and the signing PIN. macOS signing and notarization also happen locally, outside GitHub Actions. CI must not have the production Tauri updater private key; CI uses only an ephemeral updater key so Tauri emits updater-shaped unsigned artifacts. Tauri updater signing is applied locally after OS signing so the updater signs the final release bundles that users will download. -**Signing credentials and argv.** Both signing secrets are supplied to the script through the environment, and neither should reach a command line: argv is readable via `ps` by any process on the machine for the lifetime of the call, and `pnpm exec` means a dependency's lifecycle scripts share that session. The Tauri updater key is env-only — `tauri signer sign` documents `--private-key` as falling back to `TAURI_SIGNING_PRIVATE_KEY`, so passing both was redundant. The Windows PIV PIN is the exception and is **accepted rather than solved**: `jsign` reads `--storepass` only as a literal option value, with no environment or file indirection, so there is nowhere else to put it. The residual is bounded — local `ps` for the duration, on a single-user machine, for a PIN that is inert without the physical YubiKey it unlocks. +**Signing credentials and argv.** Three secrets reach `scripts/sign-and-deploy.sh` through the environment, and argv is readable via `ps` by any process on the machine for the lifetime of a call — which matters more than usual here, since `pnpm exec` means a dependency's lifecycle scripts share that session. One of the three is now env-only; two are on a command line because their tools offer nowhere else to put them: + +- `TAURI_SIGNING_PRIVATE_KEY` — **env-only**. `tauri signer sign` documents `--private-key` as falling back to that variable, so passing both was redundant exposure. +- `EV_SIGN_PIN` — on argv. `jsign` reads `--storepass` only as a literal option value, with no environment or file indirection. Bounded: local `ps` for the duration of one call, for a PIN inert without the physical YubiKey it unlocks. +- `APPLE_SIGN_PASS` — on argv, and the weakest of the three. `xcrun notarytool` offers no environment form either, but unlike the PIN this is a standalone credential, and `--wait --timeout 30m` holds it on the command line for up to half an hour per architecture. The documented remedy is `notarytool store-credentials` plus `--keychain-profile`, which moves the exposure to one short call instead of every submission. Not yet done — it changes the release runbook and cannot be exercised without live Apple credentials. - FAIL IF `scripts/sign-and-deploy.sh` stops doing any of three things: verifying GitHub artifact attestations, verifying artifact SHA-256 manifests, or using PIV-backed Windows signing. - FAIL IF `TAURI_SIGNING_PRIVATE_KEY` is passed on a command line anywhere in `scripts/sign-and-deploy.sh` rather than through the environment. `jsign --storepass` is the one documented exception, for the reason above. @@ -274,7 +280,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. - `supply-chain` — the dependency graph, the lockfile, `website/src/`, and `website/scripts/` (owning the generator's output without the generator is half a check). - - `application-security` — **everything else**, worked out from `ls -A` rather than from a list. An enumeration here goes stale the moment a top-level path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. + - `application-security` — **everything else**, worked out from `ls -A` rather than from a list. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. From e7dc78340dd8cb98c56cf9d1486f7ef1d85ae6ec Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 13:48:38 -0700 Subject: [PATCH 05/19] security(workflow-audit): make the widened window reach every consumer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit added `.github/audit/`, `.vscode/`, and `SECURITY.md` to `workflow-audit.yaml`'s `git log` pathspec and nowhere else. The audit found what that actually did, and it was worse than leaving them out. `own_changes` still listed files with the old two-path spec, and the caller drops any commit whose list comes back empty — a `continue` written for merge commits. So a commit touching only one of the three new paths was matched by `git log`, returned nothing from `own_changes`, and was silently dropped: never classified, never counted, never in the issue or the step summary. The run stayed green, the 48-hour liveness check stayed green, and SECURITY.md claimed the path was watched. A bot deleting a `FAIL IF`, or adding a `runOn: folderOpen` task, would have passed unremarked. Both classifiers had the mirror of the same hole: each refuses a commit that touches `.config/tend.yaml`, because neither inspects it — but that refusal was never extended to the new paths. A nightly tend regen that also rewrote `.github/audit/` still reproduces byte-for-byte from `uvx tend init`, so it would be classified "explained" and the audit's own scope edited under cover of the one commit shape it is designed to wave through. There is now one `WINDOW` array, and every consumer reads it: the commit list, both `own_changes` branches, and both classifier refusals (via `WINDOW_NON_WORKFLOW`, the non-workflow half they must refuse rather than inspect). Arrays rather than a space-joined scalar, because the unquoted form is correct only under a word-splitting shell — it silently matches nothing otherwise, which is the same class of failure one layer down. That is not hypothetical: my first attempt tested clean under bash and returned empty under zsh. Verified by running the real functions against real commits: a SECURITY.md-only commit is reported rather than dropped, commits touching the new paths are refused by both classifiers, and a pure workflow commit still reaches them as before. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/workflow-audit.yaml | 38 ++++++++++++++++++++++----- SECURITY.md | 2 +- 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index 3d2d4de0..aaaf436a 100644 --- a/.github/workflows/workflow-audit.yaml +++ b/.github/workflows/workflow-audit.yaml @@ -112,9 +112,23 @@ jobs: # content. Both classifiers refuse any commit that touches the config, # so nothing in the widened window can be swallowed by an arm that # doesn't inspect it — that pairing is the invariant, not either half. + # ONE definition of the window. Every consumer below — the commit + # list, `own_changes`, and both classifiers' refusals — must use this + # same set, or a path that is in the window for one and out of it for + # another goes silently unreported: `git log` matches the commit, + # `own_changes` returns nothing for it, and the empty-list `continue` + # swallows it. Widening one without the others is worse than not + # widening at all, because the FAIL IF then claims coverage that does + # not exist. + # Arrays, not a space-joined string: a pathspec list expanded from an + # unquoted scalar is correct here only because this step runs bash, + # and silently matches nothing under a shell that does not word-split. + # `"${WINDOW[@]}"` means the same thing everywhere and needs no + # shellcheck exemption. + WINDOW=(.github/workflows/ .config/tend.yaml .github/audit/ .vscode/ SECURITY.md) + WINDOW_NON_WORKFLOW=(.config/tend.yaml .github/audit/ .vscode/ SECURITY.md) COMMITS=$(git log --all --since="$SINCE" --pretty=format:'%H' \ - -- .github/workflows/ .config/tend.yaml .github/audit/ \ - .vscode/ SECURITY.md | sort -u) + -- "${WINDOW[@]}" | sort -u) if [ -z "$COMMITS" ]; then echo "No workflow or tend-config changes since $SINCE." @@ -165,7 +179,12 @@ jobs: # arms refusing it, any commit touching the config must be reported # by one of them — the window covers two paths and neither classifier # can silently swallow the one it doesn't inspect. - [ -z "$(git show --name-only --pretty='' "$sha" -- .config/tend.yaml)" ] || return 1 + # Refuse the whole non-workflow half of the window, not just the + # config: this arm's content test only ever inspects + # `.github/workflows/`, so anything else riding along on the same + # commit would be waved through unexamined. A regen that also + # rewrites the audit's own prompts is exactly that shape. + [ -z "$(git show --name-only --pretty='' "$sha" -- "${WINDOW_NON_WORKFLOW[@]}")" ] || return 1 diff=$(git show --format='' -U0 "$sha" -- .github/workflows/) || return 1 # Content lines only — drop diff headers and hunk markers. @@ -214,7 +233,12 @@ jobs: # holding up a control that exists because the bot can author # workflows. Real regen commits are version bumps that leave the # config untouched, so this costs nothing. - [ -z "$(git show --name-only --pretty='' "$sha" -- .config/tend.yaml)" ] || return 1 + # Refuse the whole non-workflow half of the window, not just the + # config: this arm's content test only ever inspects + # `.github/workflows/`, so anything else riding along on the same + # commit would be waved through unexamined. A regen that also + # rewrites the audit's own prompts is exactly that shape. + [ -z "$(git show --name-only --pretty='' "$sha" -- "${WINDOW_NON_WORKFLOW[@]}")" ] || return 1 version=$(git show "$sha:.github/workflows/tend-review.yaml" 2>/dev/null \ | sed -nE '1s/^# Generated by tend ([0-9]+\.[0-9]+\.[0-9]+)\..*/\1/p') @@ -251,11 +275,11 @@ jobs: parents=$(git rev-list --parents -n1 "$sha" | cut -d' ' -f2-) nparents=$(printf '%s\n' "$parents" | wc -w | tr -d ' ') if [ "$nparents" -le 1 ]; then - git show --name-only --pretty='' "$sha" -- .github/workflows/ .config/tend.yaml + git show --name-only --pretty='' "$sha" -- "${WINDOW[@]}" return fi for p in $parents; do - cur=$(git diff --name-only "$p" "$sha" -- .github/workflows/ .config/tend.yaml | sort -u) + cur=$(git diff --name-only "$p" "$sha" -- "${WINDOW[@]}" | sort -u) if [ "$first" -eq 1 ]; then acc="$cur"; first=0 else @@ -324,7 +348,7 @@ jobs: BODY=$(mktemp) { - echo "$COUNT unexplained commit(s) touching \`.github/workflows/\` or \`.config/tend.yaml\` since \`$SINCE\`." + echo "$COUNT unexplained commit(s) in the audit window (\`.github/workflows/\`, \`.config/tend.yaml\`, \`.github/audit/\`, \`.vscode/\`, \`SECURITY.md\`) since \`$SINCE\`." echo "" echo "Routine Renovate pin bumps and reproducible tend regenerations are" echo "classified and omitted — see the run summary for what was skipped." diff --git a/SECURITY.md b/SECURITY.md index b6af2580..6e352c91 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -275,7 +275,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the audit stops fanning out to a dedicated application-security subagent scoped to **Remote Control**, or that subagent's scope is merged back into a context that also carries the supply-chain or CI domains. Folding it back in is how that section stops being audited without anyone deciding to stop auditing it. - FAIL IF the orchestrator prompt stops requiring a non-turn-ending wait — a Bash `until` loop over the fragment files, re-issued past the ten-minute Bash cap, under a bounded deadline that is **persisted to a file** rather than recomputed from `now`. A deadline longer than the ten-minute cap cannot fire inside one call, so a re-issued loop that recomputes it never reaches it: the bound is then written down but never binds, and the only thing ending the wait is the runner's cancellation. Delegating is safe; ending the turn to wait is what kills the run, and no tool allowlist prevents it. - FAIL IF `.github/audit/` is missing a prompt file the workflow names, or `scripts/security-audit-local.sh` stops running the audit from those same files. A local runner with its own copy of the prompts is worse than no local runner: it drifts, and the drift is invisible until a nightly disagrees with a local pass. -- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside `workflow-audit.yaml`'s diff window. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. +- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside **every** consumer of `workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals. Widening one without the others is worse than not widening at all: `git log` matches the commit, `own_changes` returns nothing for it, the empty-list `continue` swallows it, and this bullet then claims a coverage that does not exist. One `WINDOW` array is the reason they cannot drift. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. - FAIL IF a `## ` section of this document is in no subagent's scope, or is in two. Every section is owned by exactly one domain: a section owned by none is unaudited, and one owned by two produces contradictory verdicts. Each domain file in `.github/audit/` names its sections as exact `## ` headings on their own lines, so this is a real grep over four markdown files rather than a reading of prose embedded in YAML. - FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. From 47b1779b6b9a4f09826fad7e21345b9c6c1a4475 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 14:05:49 -0700 Subject: [PATCH 06/19] security: derive the classifier window, and record that tend shipped the fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two WARNINGs from the ci-and-secrets re-run, both cases of this document claiming more than the code delivers. All 28 FAIL IF checks passed. **The "one WINDOW array" claim was overstated.** There were two literal arrays, independently maintained. Consistent today, but held by convention rather than construction — and the drift it invites is exactly the failure the sentence was written to rule out: add a path to `WINDOW` alone and a commit that regenerates `tend-*.yaml` byte-for-byte while also editing that path passes the classifiers' refusal, reproduces from `uvx tend init`, and is classified "explained". Reachable with `TEND_BOT_TOKEN`, which is the credential this audit exists to watch. `WINDOW_NON_WORKFLOW` is now derived as `"${WINDOW[@]:1}"`, so the complement cannot fall behind the window. **tend 0.1.19 shipped today**, 2026-08-26T20:51Z — minutes before the audit ran, which is why the previous commit's "merged but unreleased" was true when written and false by the time it was read. Verified the content, not the tag: `shared/steps/lib/pin-instruction-paths.sh` pins `':(glob)**/AGENTS.md'` at any depth, where 0.1.18's flat `SENSITIVE` list named no `AGENTS.md` at all. The gap is still live here — all eight `tend-*.yaml` still pin `@0.1.18`, so a fork PR still hands the agent an attacker-controlled `AGENTS.md` as authoritative instructions. What changed is that the last remaining step is now forceable rather than something to wait on. Bumping the pin is a separate change: it regenerates eight workflow files and deserves its own review, not a footnote at the end of this one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/workflow-audit.yaml | 9 ++++++++- SECURITY.md | 4 ++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index aaaf436a..800e169e 100644 --- a/.github/workflows/workflow-audit.yaml +++ b/.github/workflows/workflow-audit.yaml @@ -126,7 +126,14 @@ jobs: # `"${WINDOW[@]}"` means the same thing everywhere and needs no # shellcheck exemption. WINDOW=(.github/workflows/ .config/tend.yaml .github/audit/ .vscode/ SECURITY.md) - WINDOW_NON_WORKFLOW=(.config/tend.yaml .github/audit/ .vscode/ SECURITY.md) + # DERIVED, never hand-maintained: element 0 is the workflows tree, and + # this is everything else — the half both classifiers must refuse + # rather than inspect, since each only ever content-checks + # `.github/workflows/`. Writing it out as a second literal list would + # put the drift back one level: add a path to WINDOW alone and a + # regen-shaped commit could carry an edit to it past the refusal, + # reproduce byte-for-byte, and be classified "explained". + WINDOW_NON_WORKFLOW=("${WINDOW[@]:1}") COMMITS=$(git log --all --since="$SINCE" --pretty=format:'%H' \ -- "${WINDOW[@]}" | sort -u) diff --git a/SECURITY.md b/SECURITY.md index 6e352c91..60135255 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -174,7 +174,7 @@ An attacker who lands a prompt injection in tend's harness can reach three secre **Prompt-injection through user-supplied content.** tend's harness reads PR descriptions, code diffs, issue text, comments, and CI logs — all attacker-influenceable surfaces. A malicious prompt could direct the harness to push a workflow that references a repo-level secret to an external URL. The bot cannot merge to `main` or push tags, so admin-gated release paths stay sealed, but a workflow on a bot-pushed feature branch will still execute with repo-level secrets in scope. -**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. The control is only as complete as its path list: this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so a list naming only `CLAUDE.md` reverts a pointer and leaves the content it points at attacker-controlled. `AGENTS.md` is absent from that list at the pinned `0.1.18`, and every entry there is root-relative. Reported from this audit; a fix with a regression test **merged upstream** as [max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005) on 2026-08-22. It has not reached us: the latest release is still `0.1.18`, cut 2026-08-16, so the fix is merged but unreleased. Two steps remain outside this repo's control — upstream cutting a release, and the nightly regen bumping the pin — so the gap is live, just shorter than it was. There *is* a local remedy, and it is the reason to state this precisely: the regen overwrites the *workflow*, not this repository's instruction files. `CLAUDE.md` **is** on the restored path list, so moving the instruction body into `CLAUDE.md` and dropping the `@AGENTS.md` pointer closes the gap today, permanently, with no upstream dependency — at the cost of the filename convention that other agent harnesses read. Until that trade is made or the upstream fix lands, the gap is live. Note that the control's completeness is a property of the pinned upstream version, not of anything in this repo: if the instructions ever move to a filename that list does not name, the revert silently stops covering them. +**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. The control is only as complete as its path list: this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so a list naming only `CLAUDE.md` reverts a pointer and leaves the content it points at attacker-controlled. `AGENTS.md` is absent from that list at the pinned `0.1.18`, and every entry there is root-relative. Reported from this audit; a fix with a regression test **merged upstream** as [max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005) on 2026-08-22. **Upstream released it as `0.1.19` on 2026-08-26**, and the fix is real: `shared/steps/lib/pin-instruction-paths.sh` now pins `':(glob)**/AGENTS.md'` (and `**/CLAUDE.md`, `**/.claude/**`) at any depth, where `0.1.18`'s flat `SENSITIVE` list named no `AGENTS.md` at all. The gap is nevertheless **still live here**: all eight `tend-*.yaml` files still pin `max-sixty/tend/claude@0.1.18`, so a fork PR still hands the agent an attacker-controlled `AGENTS.md` as authoritative instructions. What changed is that the remaining step — the nightly regen bumping the pin — is now something that can be forced rather than waited on. There *is* a local remedy, and it is the reason to state this precisely: the regen overwrites the *workflow*, not this repository's instruction files. `CLAUDE.md` **is** on the restored path list, so moving the instruction body into `CLAUDE.md` and dropping the `@AGENTS.md` pointer closes the gap today, permanently, with no upstream dependency — at the cost of the filename convention that other agent harnesses read. Until that trade is made or the upstream fix lands, the gap is live. Note that the control's completeness is a property of the pinned upstream version, not of anything in this repo: if the instructions ever move to a filename that list does not name, the revert silently stops covering them. **Credential isolation bounds an injection.** The agent runs as a separate, non-sudo sandbox user behind a local credential-injecting proxy: `TEND_BOT_TOKEN` and the Anthropic credential live only in the proxy and never enter the agent's environment, its disk, or `.git/config` (the setup strips the credential `actions/checkout` persists there). An injected instruction can therefore make the bot *act* within its permissions — comment, push a feature branch — but cannot read the token value out and exfiltrate it. The worst-case analysis above is about what the bot's identity can do, not about the secret escaping. @@ -275,7 +275,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the audit stops fanning out to a dedicated application-security subagent scoped to **Remote Control**, or that subagent's scope is merged back into a context that also carries the supply-chain or CI domains. Folding it back in is how that section stops being audited without anyone deciding to stop auditing it. - FAIL IF the orchestrator prompt stops requiring a non-turn-ending wait — a Bash `until` loop over the fragment files, re-issued past the ten-minute Bash cap, under a bounded deadline that is **persisted to a file** rather than recomputed from `now`. A deadline longer than the ten-minute cap cannot fire inside one call, so a re-issued loop that recomputes it never reaches it: the bound is then written down but never binds, and the only thing ending the wait is the runner's cancellation. Delegating is safe; ending the turn to wait is what kills the run, and no tool allowlist prevents it. - FAIL IF `.github/audit/` is missing a prompt file the workflow names, or `scripts/security-audit-local.sh` stops running the audit from those same files. A local runner with its own copy of the prompts is worse than no local runner: it drifts, and the drift is invisible until a nightly disagrees with a local pass. -- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside **every** consumer of `workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals. Widening one without the others is worse than not widening at all: `git log` matches the commit, `own_changes` returns nothing for it, the empty-list `continue` swallows it, and this bullet then claims a coverage that does not exist. One `WINDOW` array is the reason they cannot drift. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. +- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside **every** consumer of `workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals. Widening one without the others is worse than not widening at all: `git log` matches the commit, `own_changes` returns nothing for it, the empty-list `continue` swallows it, and this bullet then claims a coverage that does not exist. One `WINDOW` array is the reason they cannot drift: the classifiers' half is *derived* from it (`"${WINDOW[@]:1}"`), not written out a second time, so adding a path cannot reach the commit list while missing the refusal. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. - FAIL IF a `## ` section of this document is in no subagent's scope, or is in two. Every section is owned by exactly one domain: a section owned by none is unaudited, and one owned by two produces contradictory verdicts. Each domain file in `.github/audit/` names its sections as exact `## ` headings on their own lines, so this is a real grep over four markdown files rather than a reading of prose embedded in YAML. - FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. From 38f4e20757c8089e36a62f4676d0e8ee45976793 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 17:47:14 -0700 Subject: [PATCH 07/19] chore(tend): regenerate at 0.1.19 Picks up max-sixty/tend#1005, which pins instruction files at any depth rather than by a flat root-relative list. `pin-instruction-paths.sh` globs `**/AGENTS.md` alongside `**/CLAUDE.md` and `**/.claude/**`, and `restore-sensitive-config.sh` passes them to `pin_to_base`, so on a fork PR the agent now reads instructions from the reviewed base branch rather than from the PR's own tree. That closes the gap SECURITY.md has been carrying since it was first reported from this audit: this repo keeps its instructions in AGENTS.md behind a one-line CLAUDE.md pointer, so 0.1.18's list reverted the pointer and left the content it points at attacker-controlled. Regenerated with `uvx tend@0.1.19 init`. The diff is the version header and the action pin, two lines per file, in all eight. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/tend-ci-fix.yaml | 4 ++-- .github/workflows/tend-mention.yaml | 4 ++-- .github/workflows/tend-nightly.yaml | 4 ++-- .github/workflows/tend-notifications.yaml | 4 ++-- .github/workflows/tend-review-runs.yaml | 4 ++-- .github/workflows/tend-review.yaml | 4 ++-- .github/workflows/tend-triage.yaml | 4 ++-- .github/workflows/tend-weekly.yaml | 4 ++-- 8 files changed, 16 insertions(+), 16 deletions(-) diff --git a/.github/workflows/tend-ci-fix.yaml b/.github/workflows/tend-ci-fix.yaml index 8f89ebcd..cf066786 100644 --- a/.github/workflows/tend-ci-fix.yaml +++ b/.github/workflows/tend-ci-fix.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -32,7 +32,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/tend-mention.yaml b/.github/workflows/tend-mention.yaml index 8f6ca61c..db45e795 100644 --- a/.github/workflows/tend-mention.yaml +++ b/.github/workflows/tend-mention.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -418,7 +418,7 @@ jobs: # the API record — the dispatch payload never carries one to spoof. EVENT_TS: ${{ github.event.comment.updated_at || needs.verify.outputs.ts || github.event.issue.updated_at }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/tend-nightly.yaml b/.github/workflows/tend-nightly.yaml index 11bee848..4f9034a0 100644 --- a/.github/workflows/tend-nightly.yaml +++ b/.github/workflows/tend-nightly.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -32,7 +32,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/tend-notifications.yaml b/.github/workflows/tend-notifications.yaml index 70e8cdf2..adae46d4 100644 --- a/.github/workflows/tend-notifications.yaml +++ b/.github/workflows/tend-notifications.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -145,7 +145,7 @@ jobs: fetch-depth: 0 fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 if: steps.check.outputs.count != '0' || github.event_name == 'workflow_dispatch' with: github_token: ${{ secrets.TEND_BOT_TOKEN }} diff --git a/.github/workflows/tend-review-runs.yaml b/.github/workflows/tend-review-runs.yaml index b0aef480..ff643eb9 100644 --- a/.github/workflows/tend-review-runs.yaml +++ b/.github/workflows/tend-review-runs.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -32,7 +32,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/tend-review.yaml b/.github/workflows/tend-review.yaml index beef6851..b29605de 100644 --- a/.github/workflows/tend-review.yaml +++ b/.github/workflows/tend-review.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -134,7 +134,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 if: steps.gate.outputs.should_run == 'true' with: github_token: ${{ secrets.TEND_BOT_TOKEN }} diff --git a/.github/workflows/tend-triage.yaml b/.github/workflows/tend-triage.yaml index 9e8452bb..7315c9e6 100644 --- a/.github/workflows/tend-triage.yaml +++ b/.github/workflows/tend-triage.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -44,7 +44,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} diff --git a/.github/workflows/tend-weekly.yaml b/.github/workflows/tend-weekly.yaml index f0c95d2c..5c0d9a63 100644 --- a/.github/workflows/tend-weekly.yaml +++ b/.github/workflows/tend-weekly.yaml @@ -1,4 +1,4 @@ -# Generated by tend 0.1.18. Regenerate with: uvx tend@latest init +# Generated by tend 0.1.19. Regenerate with: uvx tend@latest init # # Do not edit this file directly — it will be overwritten on regeneration. # To customize behavior, edit the relevant skill (for example, @@ -32,7 +32,7 @@ jobs: fetch-tags: true token: ${{ secrets.TEND_BOT_TOKEN }} - - uses: max-sixty/tend/claude@0.1.18 + - uses: max-sixty/tend/claude@0.1.19 with: github_token: ${{ secrets.TEND_BOT_TOKEN }} claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} From b7393d92e93ca65fa4968d083cdedfa92aebe870 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 17:47:52 -0700 Subject: [PATCH 08/19] security(tend): record the instruction-file gap as closed, and pin the floor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The regen onto 0.1.19 closes the fork-PR instruction-injection path this audit reported. SECURITY.md described it as live across four commits, each time accurately at the moment of writing — merged-but-unreleased, then released-but-unpinned — so it now says what actually holds: the revert list is pathspec globs covering `**/AGENTS.md` at any depth, and this repo runs that version. Two things survive the fix and are kept: - The control lives in upstream code, so what protects this repo is whatever the pinned version implements. A pin that moves backwards reopens the path with no visible change here except a version number — so there is now a FAIL IF on the floor, not just on the pin being present. - The local remedy still exists if it ever regresses: the regen overwrites the workflow, not this repo's instruction files, so moving the body into CLAUDE.md would close it with no upstream dependency. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- SECURITY.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 60135255..fdc27104 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -174,7 +174,11 @@ An attacker who lands a prompt injection in tend's harness can reach three secre **Prompt-injection through user-supplied content.** tend's harness reads PR descriptions, code diffs, issue text, comments, and CI logs — all attacker-influenceable surfaces. A malicious prompt could direct the harness to push a workflow that references a repo-level secret to an external URL. The bot cannot merge to `main` or push tags, so admin-gated release paths stay sealed, but a workflow on a bot-pushed feature branch will still execute with repo-level secrets in scope. -**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. The control is only as complete as its path list: this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so a list naming only `CLAUDE.md` reverts a pointer and leaves the content it points at attacker-controlled. `AGENTS.md` is absent from that list at the pinned `0.1.18`, and every entry there is root-relative. Reported from this audit; a fix with a regression test **merged upstream** as [max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005) on 2026-08-22. **Upstream released it as `0.1.19` on 2026-08-26**, and the fix is real: `shared/steps/lib/pin-instruction-paths.sh` now pins `':(glob)**/AGENTS.md'` (and `**/CLAUDE.md`, `**/.claude/**`) at any depth, where `0.1.18`'s flat `SENSITIVE` list named no `AGENTS.md` at all. The gap is nevertheless **still live here**: all eight `tend-*.yaml` files still pin `max-sixty/tend/claude@0.1.18`, so a fork PR still hands the agent an attacker-controlled `AGENTS.md` as authoritative instructions. What changed is that the remaining step — the nightly regen bumping the pin — is now something that can be forced rather than waited on. There *is* a local remedy, and it is the reason to state this precisely: the regen overwrites the *workflow*, not this repository's instruction files. `CLAUDE.md` **is** on the restored path list, so moving the instruction body into `CLAUDE.md` and dropping the `@AGENTS.md` pointer closes the gap today, permanently, with no upstream dependency — at the cost of the filename convention that other agent harnesses read. Until that trade is made or the upstream fix lands, the gap is live. Note that the control's completeness is a property of the pinned upstream version, not of anything in this repo: if the instructions ever move to a filename that list does not name, the revert silently stops covering them. +**Instruction files are part of that surface.** `tend-review.yaml` runs on `pull_request_target` and checks out the PR merge ref, so on a fork PR the working tree the agent reads is attacker-controlled — including the files Claude Code loads as *project instructions* (`CLAUDE.md`, `AGENTS.md`, `.claude/`, `.mcp.json`). Those are not read as data the way a diff is; they are read as authoritative guidance. tend closes this by reverting those paths from the reviewed base branch before the agent starts (`shared/steps/restore-sensitive-config.sh`), so instructions come from code a maintainer merged. + +This was **reported from this audit and is now fixed**. At the previously pinned `0.1.18` the revert list was a flat, root-relative `SENSITIVE` array naming `CLAUDE.md` but no `AGENTS.md` at all — and this repo keeps its instructions in `AGENTS.md` with `CLAUDE.md` as a one-line `@AGENTS.md` pointer, so the control reverted a pointer and left the content it pointed at attacker-controlled. The fix ([max-sixty/tend#1005](https://github.com/max-sixty/tend/pull/1005), merged 2026-08-22, released in `0.1.19` on 2026-08-26) replaces that list with pathspec globs — `':(glob)**/AGENTS.md'`, `':(glob)**/CLAUDE.md'`, `':(glob)**/.claude/**'` — which `restore-sensitive-config.sh` passes to `pin_to_base`, covering every depth rather than a hand-enumerated set of root paths. This repo regenerated onto `0.1.19`, so the gap is closed here rather than merely closable. + +Two things stay true regardless of the fix. The control's completeness is a property of the *pinned upstream version*, not of anything in this repo, so a pin that moves backwards silently reopens it — hence the `FAIL IF` below. And there remains a local remedy if it ever regresses: the regen overwrites the *workflow*, not this repository's instruction files, so moving the instruction body into `CLAUDE.md` and dropping the pointer would close it without any upstream dependency, at the cost of the filename convention other agent harnesses read. **Credential isolation bounds an injection.** The agent runs as a separate, non-sudo sandbox user behind a local credential-injecting proxy: `TEND_BOT_TOKEN` and the Anthropic credential live only in the proxy and never enter the agent's environment, its disk, or `.git/config` (the setup strips the credential `actions/checkout` persists there). An injected instruction can therefore make the bot *act* within its permissions — comment, push a feature branch — but cannot read the token value out and exfiltrate it. The worst-case analysis above is about what the bot's identity can do, not about the secret escaping. @@ -207,6 +211,7 @@ An attacker who lands a prompt injection in tend's harness can reach three secre - No org-level secret visible to this repository at all (see "Org-level secrets" above). - FAIL IF `CHROMATIC_PROJECT_TOKEN` is missing from `secrets.allowed` in `.config/tend.yaml`. The allowlist entry is an explicit acknowledgment that the bot can read this token. - FAIL IF `.github/workflows/workflow-audit.yaml` is missing, disabled, or has not produced a successful run in the last 48 hours. The margin is thinner than it reads: `workflow-audit` runs at 07:13 UTC and this audit at 04:21, so the steady state is ~21.5h and a single skipped run lands at ~45.5h — inside tolerance by under three hours, which is a reason to treat one skipped run as a signal rather than noise. +- FAIL IF any `tend-*.yaml` pins `max-sixty/tend` below `0.1.19`, the release that pins instruction files by glob at any depth. The revert list is upstream code, so the protection this repo gets is whatever the pinned version implements — a downgrade reopens the fork-PR instruction-injection path with no visible change to any file here except a version number. - FAIL IF any `tend-*.yaml` workflow uses an unpinned action reference (e.g. `@main`, no version). Tag pins are accepted inside `tend-*.yaml` because the file is owned by the upstream generator; every other workflow — agent-managed or not — must SHA-pin per the rule above. - FAIL IF any job in an agent-managed workflow has **effective** `GITHUB_TOKEN` permissions beyond `contents: write`, `pull-requests: write`, `issues: write`, `id-token: write`, `actions: read`, or any `read` permission. Effective, not declared: a job with no `permissions:` block inherits the repository default, so this check is only meaningful together with the next one. A job that declares nothing textually "grants" nothing while its token carries nine write scopes. - FAIL IF `default_workflow_permissions` for this repository is not `read`, or `can_approve_pull_request_reviews` is not `false` (`gh api repos/diffplug/dormouse/actions/permissions/workflow`). This is the backstop for every permission bullet in this document: with the default at `write`, one regenerated workflow that omits a `permissions:` block silently reopens what those bullets close, and the repository setting is the only place to fix it durably — a YAML edit does not survive the nightly regen. From 905b7b54eb22432901194d7aba1d21d90b22c5e2 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 18:27:21 -0700 Subject: [PATCH 09/19] security(audit): run application-security on Opus, and make a domain's verdict binding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three changes from the clean run, plus the model split. **Model split.** The nightly ran everything on Sonnet — the action's default, since no model was set. That is right for two of three domains: `supply-chain` and `ci-and-secrets` are mechanical, and Sonnet did them well, including catching both window BLOCKERs and the stale `hono` disclosure. `application-security` is the one that reads code adversarially, and its findings are the ones that needed real reasoning — tracing a relay-minted `clientId` to keystroke injection, or working out that an eight-character fingerprint carried ~40 bits because a P-256 point's leading byte is constant. It now runs on Opus via a per-agent `model` in `--agents`, verified to take effect: a session pinned to `--model sonnet` spawned a probe agent that came back on Opus. The orchestrator and the other two domains stay on the default, so the cost lands where the depth is. `scripts/security-audit-local.sh` applies the same split — local and CI disagreeing would be worst on exactly the domain where the model matters. **A domain's verdict is now binding.** The reporting step checked that fragments exist and never read them, so a subagent could write `VERDICT: FAIL`, return FAIL, and an orchestrator that mis-merged into `PASS` would pass unremarked — worse than a missing fragment, because `PASS` closes the open failure issue and opens the release gate, leaving the finding only in a 14-day artifact. Line 1 of each fragment is now the literal verdict, and the step downgrades a `PASS` that contradicts one. FAIL rather than MISSING: a dissenting domain finished and found something. **Remote resize is bounded at the top.** `clampTerminalDimension` clamped only the minimum, and `terminal.resize` carries a peer-supplied number straight to `term.resize` in the webview that owns the pane, where xterm allocates rows × cols. One frame asking for a million by a million wedges every terminal in that window. Reachable by an authorized Client, or by a compromised Server forging `msg` on an established session. Bounded at 2000 in the shared helper, so Host, client, and harness all get it. **SECURITY.md leaves the workflow-audit window**, reversing my own addition. #450 argues it belongs out, and it is right for a reason I missed: that job watches code that executes from *any* branch — a workflow on a bot-pushed branch, a folderOpen task on checkout — whereas a `FAIL IF` is inert until merged to main, which is admin-gated. So the branch-wide watch adds nothing over PR review, while reporting a commit on nearly every security PR. A control that cries wolf on routine work is one people learn to skim. `.vscode/` stays, because it does execute from a branch. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/audit/_preamble.md | 6 ++- .github/audit/orchestrator.md | 18 ++++++--- .github/workflows/security-audit.yaml | 53 +++++++++++++++++++++++++++ .github/workflows/workflow-audit.yaml | 20 +++++++--- SECURITY.md | 8 +++- scripts/security-audit-local.sh | 11 +++++- server-lib-common/src/remote/wire.ts | 20 +++++++++- server-lib-common/test/wire.test.mjs | 29 +++++++++++++++ 8 files changed, 149 insertions(+), 16 deletions(-) create mode 100644 server-lib-common/test/wire.test.mjs diff --git a/.github/audit/_preamble.md b/.github/audit/_preamble.md index c25e1e24..646b099b 100644 --- a/.github/audit/_preamble.md +++ b/.github/audit/_preamble.md @@ -16,7 +16,11 @@ for a check you could not determine — a transient network error, or an area yo ran out of room to reach — and say which it was. It is never a substitute for a check you could have run. -Write your findings to the file named in your own prompt, with two sections: +Write your findings to the file named in your own prompt. **Its very first +line must be literally `VERDICT: PASS` or `VERDICT: FAIL`** — nothing else on +that line. The reporting step greps for it, so it is the one part of your +report a machine reads: a `FAIL` there cannot be lost in a merge, and it is +what stops an optimistic summary from overriding you. Then two sections: `### FAIL IF results` (one line per check) and `### Qualitative findings` (severity-tagged). **Write that file before you return** — your caller reads the file, not your reply, and a fragment that does not exist fails the whole diff --git a/.github/audit/orchestrator.md b/.github/audit/orchestrator.md index c794af9a..5278af26 100644 --- a/.github/audit/orchestrator.md +++ b/.github/audit/orchestrator.md @@ -14,13 +14,21 @@ adversarially — and one context holding all three degrades the third. ## 1. Spawn all three Spawn them with the Task tool **in a single message** so they run -concurrently. Give each subagent, verbatim: +concurrently, using these three `subagent_type` values: -- the shared preamble in `.github/audit/_preamble.md`, then -- its own file: `.github/audit/supply-chain.md`, - `.github/audit/ci-and-secrets.md`, `.github/audit/application-security.md`. +- `supply-chain` +- `ci-and-secrets` +- `application-security` -Read all four files before you spawn anything. +Each is already defined with the prompt it needs — pointing at +`.github/audit/_preamble.md` plus its own domain file — and with the model it +should run on. `application-security` is deliberately on a stronger model than +the other two; do not override it, and do not paste prompt text into the Task +call. A one-line instruction such as "begin your audit" is enough, because the +agent definition carries the rest. + +Do not read the domain files yourself. They are long, you are not auditing, +and holding all three in your context is the thing this split exists to avoid. ## 2. Wait without ending your turn diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 122751ce..2c2f4d18 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -116,9 +116,24 @@ jobs: # the rule that makes it safe: block in a Bash `until` loop on # the fragment files, never end the turn to wait. `Workflow` # stays denied because nothing here should be spawning one. + # Two of the three domains are mechanical — run a generator, read + # `gh api`, compare a pin — and the session default (Sonnet) + # does them well. `application-security` is the one that has to + # read code adversarially, and its findings have been the ones + # that needed real reasoning: tracing a relay-minted `clientId` + # to a keystroke-injection path, or working out that an 8-char + # fingerprint carries ~40 bits because a P-256 point's leading + # byte is constant. It runs on Opus; the orchestrator and the + # other two stay on the cheaper default, so the cost lands only + # where the depth does. + # + # Each agent's prompt is a pointer, not a copy — the content + # stays in `.github/audit/` so CI and + # `scripts/security-audit-local.sh` cannot drift apart. claude_args: >- --allowed-tools "Read,Write,Edit,Bash,Grep,Glob,Task,Agent" --disallowed-tools "Workflow" + --agents '{"supply-chain":{"description":"Audits the Dependency Supply Chain section of SECURITY.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/supply-chain.md`, and follow them exactly."},"ci-and-secrets":{"description":"Audits the CI, tend, release, and audit-contract sections of SECURITY.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/ci-and-secrets.md`, and follow them exactly."},"application-security":{"description":"Audits the Remote Control section of SECURITY.md, and sweeps the rest of the product code.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/application-security.md`, and follow them exactly.","model":"opus"}}' # The prompts live in `.github/audit/`, not inline here: they # are long enough to need real diffs in review, they must be # runnable locally against the same text CI uses @@ -302,6 +317,44 @@ jobs: done fi + # Existence is not agreement. The loop above catches a domain that + # produced nothing; this catches one that produced a FAIL the merge + # then lost — a mis-summarised verdict, or three domains flattened + # into an optimistic paragraph. That case is worse than a missing + # fragment, because PASS *closes* the open failure issue below and + # opens the release gate, leaving the finding only in a 14-day + # artifact nobody is looking at. + # + # FAIL, not MISSING: a domain that returned FAIL did finish, and it + # found something. Each subagent is told to make line 1 of its + # fragment the literal verdict so this grep can exist at all — + # otherwise the verdict rests on the orchestrator faithfully + # propagating it, which is the same "a prompt is not a control" + # shape the missing-fragment guard exists to close. + if [ "$STATUS" = "PASS" ]; then + # shellcheck disable=SC2086 + for f in $AUDIT_FRAGMENTS; do + [ -s "$f" ] || continue + if head -n1 "$f" | grep -q '^VERDICT: FAIL'; then + echo "::error::Audit reported PASS but $f opens with VERDICT: FAIL; downgrading to FAIL." + DISSENTING="${DISSENTING:+$DISSENTING, }$f" + STATUS=FAIL + fi + done + if [ -n "${DISSENTING:-}" ]; then + { + echo + echo "---" + echo + echo "**Downgraded to FAIL by the reporting step.** The merged verdict was" + echo "\`PASS\`, but these domain reports open with \`VERDICT: FAIL\`:" + echo "$DISSENTING." + echo + echo "A domain's own verdict outranks the merge." + } >> audit-report.md + fi + fi + DATE=$(date -u +%Y-%m-%dT%H:%MZ) RUN_URL="https://github.com/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID" diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index 800e169e..dd017555 100644 --- a/.github/workflows/workflow-audit.yaml +++ b/.github/workflows/workflow-audit.yaml @@ -1,7 +1,7 @@ name: workflow-audit # Nightly audit of every commit touching .github/workflows/, -# .config/tend.yaml, .github/audit/, .vscode/, or SECURITY.md. Surfaces changes from feature branches and direct +# .config/tend.yaml, .github/audit/, or .vscode/. Surfaces changes from feature branches and direct # pushes, not just the main branch — so a bot push that adds a new # workflow file gets a visible issue even if it never opens a PR. # @@ -98,10 +98,18 @@ jobs: # .vscode/ is in the window because `tasks.json` can carry # `"runOn": "folderOpen"`, which executes when a maintainer opens the # folder — the same persistence-on-checkout shape a workflow gives, - # reached without touching .github/. SECURITY.md is in it because it - # is the document the nightly security audit enforces: weakening a - # `FAIL IF` and weakening the workflow that runs it are the same act - # one file apart, and only one of them was watched. + # reached without touching .github/. + # + # SECURITY.md is deliberately NOT in the window, though it was + # briefly. Two reasons, and the second is why the first is not enough + # on its own. What this job watches is code that *executes from any + # branch*: a workflow runs on a bot-pushed branch, and a folderOpen + # task runs when someone checks one out. SECURITY.md is inert until + # it is merged to main, which is admin-gated — so the branch-wide + # watch buys nothing PR review does not already give. And it changes + # in nearly every security commit, so including it would report a + # commit on almost every such PR; a control that cries wolf on + # routine work is one people learn to skim. # # .config/tend.yaml is in the window, not just .github/workflows/. # Without it, a commit editing only the config never enters the audit, @@ -125,7 +133,7 @@ jobs: # and silently matches nothing under a shell that does not word-split. # `"${WINDOW[@]}"` means the same thing everywhere and needs no # shellcheck exemption. - WINDOW=(.github/workflows/ .config/tend.yaml .github/audit/ .vscode/ SECURITY.md) + WINDOW=(.github/workflows/ .config/tend.yaml .github/audit/ .vscode/) # DERIVED, never hand-maintained: element 0 is the workflows tree, and # this is everything else — the half both classifiers must refuse # rather than inspect, since each only ever content-checks diff --git a/SECURITY.md b/SECURITY.md index fdc27104..42bc9214 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -190,7 +190,7 @@ Two things stay true regardless of the fix. The control's completeness is a prop **Org-level secrets.** Secrets shared with this repo from the `diffplug` org would be reachable by any workflow the bot can author, exactly like repo-level ones, and they do not appear in this repo's own secret listing (`gh api repos/diffplug/dormouse/actions/organization-secrets` is the check). None are visible here today. `BUILDCACHE_USER` and `NEXUS_USER` were org-wide shares — visible to every `diffplug` repository, not grants made to this one — and were previously accepted on the grounds that they are usernames rather than the paired credentials. They have since been narrowed to `selected` visibility over the repositories that actually consume them, which excludes this one, so the acceptance no longer has to be made. Every `diffplug` org secret is now `selected` and none lists `diffplug/dormouse`. Any org secret becoming visible here is an exposure that must be re-evaluated and named before it is accepted — hence the FAIL IF below admits none. -**Upstream compromise.** Tend's action is referenced as `max-sixty/tend/claude@0.1.18` in every generated workflow — a **tag**, not a commit SHA. A tag is mutable by whoever owns that repository, so upstream can change what our workflows execute without any commit landing here, and `workflow-audit.yaml` would see nothing: the file is byte-identical. This is a real residual, not a solved problem. It is accepted because the file is generated (a hand-edited SHA is overwritten by the next nightly regen, so pinning locally is not durable) and because the trust it represents is the same trust the harness already has — tend runs the agent that holds `TEND_BOT_TOKEN` either way. What it means concretely is that the version pin bounds *deliberate* upgrades, not a hostile upstream. `uvx tend@latest` runs only at install and during nightly regen; a compromise of that path affects the next re-run, not the in-flight workflows. +**Upstream compromise.** Tend's action is referenced as `max-sixty/tend/claude@0.1.19` in every generated workflow — a **tag**, not a commit SHA. A tag is mutable by whoever owns that repository, so upstream can change what our workflows execute without any commit landing here, and `workflow-audit.yaml` would see nothing: the file is byte-identical. This is a real residual, not a solved problem. It is accepted because the file is generated (a hand-edited SHA is overwritten by the next nightly regen, so pinning locally is not durable) and because the trust it represents is the same trust the harness already has — tend runs the agent that holds `TEND_BOT_TOKEN` either way. What it means concretely is that the version pin bounds *deliberate* upgrades, not a hostile upstream. `uvx tend@latest` runs only at install and during nightly regen; a compromise of that path affects the next re-run, not the in-flight workflows. **Audit visibility.** `workflow-audit.yaml` is a nightly job that walks every commit touching `.github/workflows/` or `.config/tend.yaml` since its previous successful run — across all branches, not just `main`, so a workflow pushed to a feature branch is seen even though it never opens a PR. The config is in the window because its values are inputs to the generated workflows, making an edit to it a workflow change made one step earlier; keeping it out would let a config edit and a regeneration be split across two commits, the first invisible to the audit and the second reproducing byte-for-byte against it. It reports the *unexplained*: two routine sources are classified and omitted on independently checked provenance and content. A Renovate pin bump must be a valid GitHub-signed commit with `author.login == "renovate[bot]"` and `committer.login == "web-flow"`, must be associated only with Renovate-authored PRs, and must change nothing but the ref of an already-referenced action. The signed author/committer pair is the provenance control: GitHub's automatically signed `createCommitOnBranch` mutation binds the author to the authenticating credential and does not permit the caller to supply the author or committer, while REST paths that permit those fields require the caller to supply the signature; requiring `web-flow` therefore rejects both a caller-supplied Renovate author and a commit signed by another identity. PR authorship is independent server-side corroboration. The content test adds a separate bound by requiring the diff to express nothing but a new ref for an action already referenced by name — the residual being a ref selected by Renovate inside that action's own repo, which is the same trust every Renovate bump already rests on. A tend regeneration must reproduce byte-for-byte from `uvx tend@ init` at the version in the files' own header, and must not touch `.config/tend.yaml` in the same commit — the config's values land verbatim in the generated YAML, so a commit that edits it and regenerates would reproduce by construction, making "reproducible" contingent on the upstream generator escaping its inputs. Identity is not evidence here at all: `TEND_BOT_TOKEN` is precisely the credential in question. Both classifiers fail open: any error or ambiguity reports the commit. Commits already merged to `main` are still reported, because review is not proof — the social-engineering path above ends in an admin merge. Deliberately not deduplicated by branch or file set: that would let a benign change be reported once and a later force-push of malicious content to the same files pass unremarked. A silent run is the healthy steady state; the liveness check below keys on a successful run, not on an issue existing. A bot push that disables or modifies the audit itself is caught in the next successful run's diff window. @@ -252,6 +252,8 @@ The `security-audit` workflow at `.github/workflows/security-audit.yaml` enforce **The audit is fanned out to three subagents with disjoint scopes**, and the orchestrator audits nothing itself — it spawns them concurrently and merges what they return. The domains are `supply-chain` (**Dependency Supply Chain**), `ci-and-secrets` (**GitHub Actions Policies**, **Automated Maintainer (tend)**, both release sections, **Reporting a Vulnerability**, and this one), and `application-security` (**Remote Control**). The split is not about parallelism. These are different subject matters with different evidence — dependency provenance is lockfiles, CI is `gh api` output, and application security is reading the pairing code adversarially — and one context holding all three degrades the third, which is the newest, has the most code behind it, and is the easiest to crowd out with API responses. The separation is one of **context, not of credential**: `AUDIT_PAT` is a step-level `env:` on the one job, so every subagent inherits it in its process environment, and only the prompt tells the application-security agent not to use it. A prompt is not a control. Making that separation real would take a second job without the `security-audit` environment, passing fragments between jobs as artifacts — worth doing, not done. Until then the honest claim is that three contexts each *read* less, not that any of them *holds* less. +**The domains do not all run on the same model.** `supply-chain` and `ci-and-secrets` are mechanical — run a generator, read an API response, compare a pin — and the session default handles them. `application-security` reads code adversarially, and it is where the findings that needed real reasoning have come from: tracing a relay-minted `clientId` to a keystroke-injection path, or working out that an eight-character device fingerprint carried ~40 bits rather than ~48 because a P-256 point's leading byte is constant. It runs on Opus, declared per-agent in `--agents`, so the cost lands on the one domain that has depth to find rather than on all three. `scripts/security-audit-local.sh` applies the same split; local and CI disagreeing about the model would be worst on exactly the domain where it matters. + Each subagent writes its own report fragment (`audit-supply-chain.md`, `audit-ci-secrets.md`, `audit-application.md`) before returning its verdict, and the orchestrator concatenates those files rather than retyping them. Fragments are uploaded with the transcript, so an orchestrator that dies mid-merge still ships whatever the domains found — the INCONCLUSIVE shape below, which the archive exists to explain. **The prompts live in `.github/audit/`, not inline in the workflow**, and that placement is load-bearing three times over. `scripts/security-audit-local.sh` runs the audit against the same files CI uses, so the loop that catches problems in this document is a local one and cannot drift from the nightly. Prompt changes get reviewed as ordinary markdown diffs rather than as YAML block-scalar churn. And the section-ownership rule below is only a grep because the `## ` headings sit in markdown — inline, block-scalar wrapping split `## Automated Maintainer (tend)` across two lines and the check silently matched nothing. @@ -280,12 +282,14 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the audit stops fanning out to a dedicated application-security subagent scoped to **Remote Control**, or that subagent's scope is merged back into a context that also carries the supply-chain or CI domains. Folding it back in is how that section stops being audited without anyone deciding to stop auditing it. - FAIL IF the orchestrator prompt stops requiring a non-turn-ending wait — a Bash `until` loop over the fragment files, re-issued past the ten-minute Bash cap, under a bounded deadline that is **persisted to a file** rather than recomputed from `now`. A deadline longer than the ten-minute cap cannot fire inside one call, so a re-issued loop that recomputes it never reaches it: the bound is then written down but never binds, and the only thing ending the wait is the runner's cancellation. Delegating is safe; ending the turn to wait is what kills the run, and no tool allowlist prevents it. - FAIL IF `.github/audit/` is missing a prompt file the workflow names, or `scripts/security-audit-local.sh` stops running the audit from those same files. A local runner with its own copy of the prompts is worse than no local runner: it drifts, and the drift is invisible until a nightly disagrees with a local pass. -- FAIL IF `.github/audit/`, `.vscode/`, or this file is outside **every** consumer of `workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals. Widening one without the others is worse than not widening at all: `git log` matches the commit, `own_changes` returns nothing for it, the empty-list `continue` swallows it, and this bullet then claims a coverage that does not exist. One `WINDOW` array is the reason they cannot drift: the classifiers' half is *derived* from it (`"${WINDOW[@]:1}"`), not written out a second time, so adding a path cannot reach the commit list while missing the refusal. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open; and weakening a `FAIL IF` here is the same act as weakening the workflow that runs it, one file apart. All three are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. +- FAIL IF `.github/audit/` or `.vscode/` is outside **every** consumer of `workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals. Widening one without the others is worse than not widening at all: `git log` matches the commit, `own_changes` returns nothing for it, the empty-list `continue` swallows it, and this bullet then claims a coverage that does not exist. One `WINDOW` array is the reason they cannot drift: the classifiers' half is *derived* from it (`"${WINDOW[@]:1}"`), not written out a second time, so adding a path cannot reach the commit list while missing the refusal. The prompts decide what gets audited and by whom; `.vscode/tasks.json` can execute on folder open. Both are changes to the security automation, which is the reason `.config/tend.yaml` is in that window. This document is deliberately *not* watched there: what that job catches is code executing from a branch nobody reviewed, and a `FAIL IF` is inert until it is merged to `main`, which is admin-gated — so the watch would add no coverage over PR review while reporting a commit on nearly every security PR. - FAIL IF a `## ` section of this document is in no subagent's scope, or is in two. Every section is owned by exactly one domain: a section owned by none is unaudited, and one owned by two produces contradictory verdicts. Each domain file in `.github/audit/` names its sections as exact `## ` headings on their own lines, so this is a real grep over four markdown files rather than a reading of prose embedded in YAML. - FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. - `supply-chain` — the dependency graph, the lockfile, `website/src/`, and `website/scripts/` (owning the generator's output without the generator is half a check). - `application-security` — **everything else**, worked out from `ls -A` rather than from a list. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. +- FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. +- FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. diff --git a/scripts/security-audit-local.sh b/scripts/security-audit-local.sh index ea9f52dc..2f36267a 100755 --- a/scripts/security-audit-local.sh +++ b/scripts/security-audit-local.sh @@ -41,9 +41,18 @@ run_domain() { application-security) out=audit-application.md ;; *) echo "error: unknown domain '$domain' (supply-chain|ci-and-secrets|application-security)" >&2; return 64 ;; esac - echo "==> $domain -> $out" + # Same model split as CI (`.github/workflows/security-audit.yaml` -> + # `--agents`): the two mechanical domains run on the default, and + # application-security — the one that reads code adversarially — runs on + # Opus. Local and CI must agree here, or the domain where the model matters + # most is the one they disagree about. + local model_args=() + [ "$domain" = "application-security" ] && model_args=(--model opus) + + echo "==> $domain -> $out${model_args[*]:+ (${model_args[*]})}" rm -f "$out" claude -p "$(cat "$AUDIT_DIR/_preamble.md"; echo; cat "$AUDIT_DIR/$domain.md")" \ + "${model_args[@]}" \ --allowed-tools "Read,Write,Edit,Bash,Grep,Glob" \ --disallowed-tools "Task,Agent,Workflow" if [ -s "$out" ]; then diff --git a/server-lib-common/src/remote/wire.ts b/server-lib-common/src/remote/wire.ts index a0cd4389..55b2301c 100644 --- a/server-lib-common/src/remote/wire.ts +++ b/server-lib-common/src/remote/wire.ts @@ -407,13 +407,31 @@ export interface TerminalResizeParams { rows: number; } +/** + * The largest terminal dimension a remote peer may ask for. + * + * Far past any real display — a 4K screen at an unreadably small font is on + * the order of 800 columns — and small enough that the worst case a peer can + * request is a few million cells rather than an arbitrary number of them. + */ +export const MAX_TERMINAL_DIMENSION = 2000; + /** * Coerce a requested terminal dimension (cols or rows) to a positive integer, * falling back to `fallback` when the value is absent or not finite. Shared so * the Host api, the client adapter, and the test harness all sanitize sizes the * same way. + * + * Clamped at **both** ends, and the upper bound is the security-relevant half: + * a local resize is derived from element geometry and cannot be large, but + * `terminal.resize` carries a peer-supplied number straight to `term.resize` + * in the webview that owns the pane, and xterm bounds only the minimum before + * allocating `rows × cols` cells. Unbounded, one frame asking for a million by + * a million wedges every terminal in that window — reachable by an authorized + * Client, or by a compromised Server forging `msg` on an established session + * (`SECURITY.md` -> Remote Control, Trust boundary). */ export function clampTerminalDimension(value: number | undefined, fallback: number): number { if (value === undefined || !Number.isFinite(value)) return fallback; - return Math.max(1, Math.floor(value)); + return Math.min(MAX_TERMINAL_DIMENSION, Math.max(1, Math.floor(value))); } diff --git a/server-lib-common/test/wire.test.mjs b/server-lib-common/test/wire.test.mjs new file mode 100644 index 00000000..ac2e6b99 --- /dev/null +++ b/server-lib-common/test/wire.test.mjs @@ -0,0 +1,29 @@ +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { MAX_TERMINAL_DIMENSION, clampTerminalDimension } from '../dist/index.js'; + +test('clampTerminalDimension falls back on absent or non-finite values', () => { + assert.equal(clampTerminalDimension(undefined, 80), 80); + assert.equal(clampTerminalDimension(Number.NaN, 24), 24); + assert.equal(clampTerminalDimension(Number.POSITIVE_INFINITY, 24), 24); +}); + +test('clampTerminalDimension floors to a positive integer', () => { + assert.equal(clampTerminalDimension(80.9, 24), 80); + assert.equal(clampTerminalDimension(0, 24), 1); + assert.equal(clampTerminalDimension(-5, 24), 1); +}); + +test('clampTerminalDimension bounds the top, not just the bottom', () => { + // The security-relevant half: `terminal.resize` carries a peer-supplied + // number to `term.resize` in the webview that owns the pane, and xterm + // bounds only the minimum before allocating rows × cols cells. Unbounded, + // one frame wedges every terminal in that window. + assert.equal(clampTerminalDimension(1_000_000, 24), MAX_TERMINAL_DIMENSION); + assert.equal(clampTerminalDimension(Number.MAX_SAFE_INTEGER, 24), MAX_TERMINAL_DIMENSION); + assert.equal(clampTerminalDimension(MAX_TERMINAL_DIMENSION, 24), MAX_TERMINAL_DIMENSION); + assert.equal(clampTerminalDimension(MAX_TERMINAL_DIMENSION + 1, 24), MAX_TERMINAL_DIMENSION); + // A realistic large terminal is untouched. + assert.equal(clampTerminalDimension(400, 24), 400); +}); From 0dfa1e3272359b6f6ca9302dccabd26970ddce55 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 18:29:53 -0700 Subject: [PATCH 10/19] security(audit): fold in what #449, #450 and #451 found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three bot PRs converged on the same problems this branch was fixing locally. Two of them found things I did not, so this adopts their substance rather than closing them as pure duplicates. **From #449 — `dormouse-lib` is a disclosure root for a specific reason, and my prose had it wrong.** I wrote that it is "reached as a workspace edge". It is not: `vscode-ext` declares only `node-pty` and `ws` and reaches the lib through relative imports into `../lib/src/` from fifteen files, so the extension's dependency walk never arrives at it. Only `dormouse-standalone`'s edge does — which puts the disclosure of lib's whole subtree one refactor away from vanishing silently. Verified both halves. Naming it a root is what makes that not matter, and the paragraph now says so. Also from #449: `website/` goes to supply-chain as a subtraction — all of it except `public/` — instead of two named subdirectories. Naming `src/` and `scripts/` left the build config owned by nobody, which is the same orphaning shape one level down that the catch-all was rewritten to stop. And dotfile directories are now named explicitly wherever they land, because a catch-all has twice been read as covering them. **From #451 — the Audit visibility paragraph named two paths where the window has four.** It is the prose spec of that pathspec, so a reader checking the `FAIL IF` against it got a contradiction. Both enumerations now agree, with the reason they belong together stated: each path holds something that executes from a branch nobody reviewed. **From #450 — the same BLOCKER this branch already fixed**, found independently. Its fix is equivalent; the version here derives the classifiers' half from the window (`"${WINDOW[@]:1}"`) rather than maintaining a second list, so the two cannot drift. #450 also argued `SECURITY.md` does not belong in the window, which is why the previous commit took it back out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/audit/supply-chain.md | 10 +++++++--- SECURITY.md | 8 ++++---- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/.github/audit/supply-chain.md b/.github/audit/supply-chain.md index cf117567..6ed9de67 100644 --- a/.github/audit/supply-chain.md +++ b/.github/audit/supply-chain.md @@ -25,9 +25,13 @@ enumeration is the shortcut that goes stale. ## Qualitative pass -You own the dependency graph, the lockfile, `website/src/`, and -`website/scripts/` — the generator whose output the first `FAIL IF` checks -lives there, and owning the output without the generator is half a check. +You own the dependency graph, the lockfile, and **all of `website/` except +`website/public/`**, which is `ci-and-secrets`' because the Tauri updater +manifest lives there. So `website/src/`, `website/scripts/`, and the build +config (`package.json`, `vite.config.ts`, `react-router.config.ts`, +`tsconfig.json`) are all yours. `generate-deps.js` is in that set: audit the +whole generator, not just the `productDependencyFilters` array the +root-completeness bullet names. - newly added or upgraded runtime dependencies since the last audit - anything in the lockfile that resolves outside the registry diff --git a/SECURITY.md b/SECURITY.md index 42bc9214..39676634 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -130,7 +130,7 @@ Every dependency Dormouse **puts on a user's machine** is listed at init` at the version in the files' own header, and must not touch `.config/tend.yaml` in the same commit — the config's values land verbatim in the generated YAML, so a commit that edits it and regenerates would reproduce by construction, making "reproducible" contingent on the upstream generator escaping its inputs. Identity is not evidence here at all: `TEND_BOT_TOKEN` is precisely the credential in question. Both classifiers fail open: any error or ambiguity reports the commit. Commits already merged to `main` are still reported, because review is not proof — the social-engineering path above ends in an admin merge. Deliberately not deduplicated by branch or file set: that would let a benign change be reported once and a later force-push of malicious content to the same files pass unremarked. A silent run is the healthy steady state; the liveness check below keys on a successful run, not on an issue existing. A bot push that disables or modifies the audit itself is caught in the next successful run's diff window. +**Audit visibility.** `workflow-audit.yaml` is a nightly job that walks every commit touching `.github/workflows/`, `.config/tend.yaml`, `.github/audit/`, or `.vscode/` since its previous successful run — across all branches, not just `main`, so a workflow pushed to a feature branch is seen even though it never opens a PR. This paragraph is the prose spec of that job's `WINDOW`, so the two enumerations name the same paths: a path added to one without the other leaves a reader checking the `FAIL IF` below against a paragraph that contradicts it. What unites them is that each executes from a branch nobody reviewed — a workflow on a bot push, a `folderOpen` task on checkout, a prompt that decides what the nightly audit even looks at. The config is in the window because its values are inputs to the generated workflows, making an edit to it a workflow change made one step earlier; keeping it out would let a config edit and a regeneration be split across two commits, the first invisible to the audit and the second reproducing byte-for-byte against it. It reports the *unexplained*: two routine sources are classified and omitted on independently checked provenance and content. A Renovate pin bump must be a valid GitHub-signed commit with `author.login == "renovate[bot]"` and `committer.login == "web-flow"`, must be associated only with Renovate-authored PRs, and must change nothing but the ref of an already-referenced action. The signed author/committer pair is the provenance control: GitHub's automatically signed `createCommitOnBranch` mutation binds the author to the authenticating credential and does not permit the caller to supply the author or committer, while REST paths that permit those fields require the caller to supply the signature; requiring `web-flow` therefore rejects both a caller-supplied Renovate author and a commit signed by another identity. PR authorship is independent server-side corroboration. The content test adds a separate bound by requiring the diff to express nothing but a new ref for an action already referenced by name — the residual being a ref selected by Renovate inside that action's own repo, which is the same trust every Renovate bump already rests on. A tend regeneration must reproduce byte-for-byte from `uvx tend@ init` at the version in the files' own header, and must not touch `.config/tend.yaml` in the same commit — the config's values land verbatim in the generated YAML, so a commit that edits it and regenerates would reproduce by construction, making "reproducible" contingent on the upstream generator escaping its inputs. Identity is not evidence here at all: `TEND_BOT_TOKEN` is precisely the credential in question. Both classifiers fail open: any error or ambiguity reports the commit. Commits already merged to `main` are still reported, because review is not proof — the social-engineering path above ends in an admin merge. Deliberately not deduplicated by branch or file set: that would let a benign change be reported once and a later force-push of malicious content to the same files pass unremarked. A silent run is the healthy steady state; the liveness check below keys on a successful run, not on an issue existing. A bot push that disables or modifies the audit itself is caught in the next successful run's diff window. **Two known evasions of that diff window**, both from how the window is computed rather than from what it classifies. The lower bound is server-set (the previous successful run's `created_at`), but the filter that applies it is `git log --all --since`, which compares against the **committer date** — a field the pusher sets freely, so `GIT_COMMITTER_DATE=2020-01-01` on a commit adding a workflow makes it invisible to every future window. And a branch pushed, run with repo-level secrets in scope, and deleted before the nightly fetch is never in any window at all, because the audit only ever sees refs that still exist. Closing both means keying on server-observed ref changes (the repository activity API records pushes, force-pushes, and deletions with server timestamps and before/after SHAs) rather than on the commit graph as the client presents it. Neither is closed today; they are stated here so the control is not read as stronger than it is. @@ -286,8 +286,8 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF a `## ` section of this document is in no subagent's scope, or is in two. Every section is owned by exactly one domain: a section owned by none is unaudited, and one owned by two produces contradictory verdicts. Each domain file in `.github/audit/` names its sections as exact `## ` headings on their own lines, so this is a real grep over four markdown files rather than a reading of prose embedded in YAML. - FAIL IF the union of the subagents' qualitative scopes does not cover every top-level path in the repository. The per-domain scopes replaced a single roving "flag any other security hole you find", so anything no domain names is now nobody's job — and the first version of this split silently orphaned `canopy/`, `.claude/` (named as prompt-injection surface two sections above), `docs/`, the root files, and all of `website/` outside `src/data/`, which includes the Tauri updater manifest that shipped apps fetch. The division is by **subtraction**, so that adding a directory cannot orphan it: - `ci-and-secrets` — `.github/` (including `.github/audit/`), `.config/`, `.claude/`, `.vscode/`, `scripts/`, and `website/public/`. The updater manifest is a release artifact, not marketing; `.vscode/` is here because `tasks.json` can carry `"runOn": "folderOpen"`, which executes on checkout. - - `supply-chain` — the dependency graph, the lockfile, `website/src/`, and `website/scripts/` (owning the generator's output without the generator is half a check). - - `application-security` — **everything else**, worked out from `ls -A` rather than from a list. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. + - `supply-chain` — the dependency graph, the lockfile, and all of `website/` except `website/public/`. Stated as a subtraction rather than as named subdirectories, because naming `src/` and `scripts/` left `website/`'s own build config owned by nobody — the same orphaning shape one level down. `generate-deps.js` is in there, so the generator behind the disclosed snapshot is audited, not just the `productDependencyFilters` array a bullet above names. + - `application-security` — **everything else**, worked out from `ls -A` rather than from a list, including `.impeccable/` (the design-token snapshot behind `DESIGN.md`). Dotfile directories are named explicitly wherever they land, in this list and in the prompt files, because a catch-all has twice now been read as covering them when no reader could tell which domain owned one. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. - FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. From b150870402bdda07b8c2059395094904c8c56ff7 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 18:54:11 -0700 Subject: [PATCH 11/19] security(audit): pin both sides of the local model split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `FAIL IF` added one commit ago failed on its own implementation, which is the system working. It requires application-security to run on a stronger model than the mechanical domains "in both" CI and the local runner. CI satisfies it: the session default is Sonnet and only that one agent carries an override. The local runner did not — it pinned Opus for application-security and left the other two unpinned, inheriting whatever `~/.claude/settings.json` names. On this machine that default is `opus[1m]`, so locally the two mechanical domains were running on a *larger-context Opus* while application-security ran on plain Opus. The relation was not merely unestablished, it was inverted. The consequence is worth recording rather than just fixing: every local audit run in this session executed the mechanical domains on `opus[1m]`, not Sonnet. Those runs were therefore more capable than the nightly will be, and a local PASS from them is not evidence that CI will pass. The first faithful rehearsal is the run after this commit. Both sides are pinned now — `--model sonnet` for the mechanical domains, `--model opus` for application-security. Also carries the bash 3.2 fix from the run before this one: macOS ships bash 3.2, where expanding an empty array under `set -u` is an unbound variable error, which killed the runner on the first domain. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- SECURITY.md | 2 +- scripts/security-audit-local.sh | 20 ++++++++++++++++---- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 39676634..670c517d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -252,7 +252,7 @@ The `security-audit` workflow at `.github/workflows/security-audit.yaml` enforce **The audit is fanned out to three subagents with disjoint scopes**, and the orchestrator audits nothing itself — it spawns them concurrently and merges what they return. The domains are `supply-chain` (**Dependency Supply Chain**), `ci-and-secrets` (**GitHub Actions Policies**, **Automated Maintainer (tend)**, both release sections, **Reporting a Vulnerability**, and this one), and `application-security` (**Remote Control**). The split is not about parallelism. These are different subject matters with different evidence — dependency provenance is lockfiles, CI is `gh api` output, and application security is reading the pairing code adversarially — and one context holding all three degrades the third, which is the newest, has the most code behind it, and is the easiest to crowd out with API responses. The separation is one of **context, not of credential**: `AUDIT_PAT` is a step-level `env:` on the one job, so every subagent inherits it in its process environment, and only the prompt tells the application-security agent not to use it. A prompt is not a control. Making that separation real would take a second job without the `security-audit` environment, passing fragments between jobs as artifacts — worth doing, not done. Until then the honest claim is that three contexts each *read* less, not that any of them *holds* less. -**The domains do not all run on the same model.** `supply-chain` and `ci-and-secrets` are mechanical — run a generator, read an API response, compare a pin — and the session default handles them. `application-security` reads code adversarially, and it is where the findings that needed real reasoning have come from: tracing a relay-minted `clientId` to a keystroke-injection path, or working out that an eight-character device fingerprint carried ~40 bits rather than ~48 because a P-256 point's leading byte is constant. It runs on Opus, declared per-agent in `--agents`, so the cost lands on the one domain that has depth to find rather than on all three. `scripts/security-audit-local.sh` applies the same split; local and CI disagreeing about the model would be worst on exactly the domain where it matters. +**The domains do not all run on the same model.** `supply-chain` and `ci-and-secrets` are mechanical — run a generator, read an API response, compare a pin — and the session default handles them. `application-security` reads code adversarially, and it is where the findings that needed real reasoning have come from: tracing a relay-minted `clientId` to a keystroke-injection path, or working out that an eight-character device fingerprint carried ~40 bits rather than ~48 because a P-256 point's leading byte is constant. It runs on Opus, declared per-agent in `--agents`, so the cost lands on the one domain that has depth to find rather than on all three. `scripts/security-audit-local.sh` applies the same split, and pins **both** sides rather than only the strong one. Leaving the mechanical domains unpinned inherits whatever the operator's own default is, which is not necessarily weaker — on a machine defaulting to `opus[1m]` it is *stronger*, which inverts the relation and quietly turns the local loop into something other than a rehearsal of the nightly. CI gets this for free, since its session default is Sonnet and only one agent carries an override. Each subagent writes its own report fragment (`audit-supply-chain.md`, `audit-ci-secrets.md`, `audit-application.md`) before returning its verdict, and the orchestrator concatenates those files rather than retyping them. Fragments are uploaded with the transcript, so an orchestrator that dies mid-merge still ships whatever the domains found — the INCONCLUSIVE shape below, which the archive exists to explain. diff --git a/scripts/security-audit-local.sh b/scripts/security-audit-local.sh index 2f36267a..11d842ca 100755 --- a/scripts/security-audit-local.sh +++ b/scripts/security-audit-local.sh @@ -46,13 +46,25 @@ run_domain() { # application-security — the one that reads code adversarially — runs on # Opus. Local and CI must agree here, or the domain where the model matters # most is the one they disagree about. - local model_args=() - [ "$domain" = "application-security" ] && model_args=(--model opus) + # BOTH sides are pinned, not just the strong one. Leaving the mechanical + # domains unpinned inherits whatever the operator's `~/.claude/settings.json` + # names, which is not necessarily weaker than Opus — on a machine defaulting + # to `opus[1m]` it is *stronger* (same family, larger context), inverting the + # relation SECURITY.md requires and making a local run no longer a rehearsal + # of the nightly. CI gets this for free: its session default is Sonnet and + # only application-security carries an override. + # + # A plain string, not an array: macOS ships bash 3.2, where `"${arr[@]}"` on + # an EMPTY array is an unbound-variable error under `set -u`. These are fixed + # literals with no whitespace, so the unquoted expansion below is safe. + local model_args="--model sonnet" + [ "$domain" = "application-security" ] && model_args="--model opus" - echo "==> $domain -> $out${model_args[*]:+ (${model_args[*]})}" + echo "==> $domain -> $out${model_args:+ ($model_args)}" rm -f "$out" + # shellcheck disable=SC2086 claude -p "$(cat "$AUDIT_DIR/_preamble.md"; echo; cat "$AUDIT_DIR/$domain.md")" \ - "${model_args[@]}" \ + $model_args \ --allowed-tools "Read,Write,Edit,Bash,Grep,Glob" \ --disallowed-tools "Task,Agent,Workflow" if [ -s "$out" ]; then From 6a208e3b61e3d5be6ae3393961bc3a88cc15777d Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 19:13:43 -0700 Subject: [PATCH 12/19] security(standalone): make session snapshots owner-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `write_session_to` created `sessions/` and its files under the bare umask, so terminal transcripts landed `0644` in a `0755` directory — readable by every other account on the machine. The blob is a `PersistedWindow` carrying scrollback: whatever the user's shells printed, which is a superset of every other secret this document tracks. The Host's own state file has been `0600` in a `0700` directory all along, for a strictly smaller secret, so this closes an inconsistency rather than inventing a rule. The mode goes on the temp file *before* any bytes are written, because the atomic rename preserves it — tightening after the rename would leave a window where the transcript is world-readable. Best-effort and unix-only: Windows ACLs are not unix modes, and a filesystem without POSIX permissions must not fail a session save. Type-checked with `cargo check` against the pinned self-contained Node (`build.rs` rejects the Homebrew one, which is not relocatable). Also fixes an overlap I introduced one commit ago: `application-security` still named `website/`'s build config as its remainder after `supply-chain` was widened to claim all of `website/` except `public/`. Overlap is the safe direction — audited twice rather than never — but it is the drift the recursive-subtraction rule exists to prevent, and the audit caught it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/audit/application-security.md | 13 +++++++------ SECURITY.md | 2 ++ docs/specs/standalone.md | 11 +++++++++-- standalone/src-tauri/src/lib.rs | 26 ++++++++++++++++++++++++++ 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/.github/audit/application-security.md b/.github/audit/application-security.md index e0cda383..0b928eef 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -48,12 +48,13 @@ moment someone adds a directory, which is exactly how `.vscode/` and `.impeccable/` ended up owned by nobody. Subtraction is **recursive, not top-level**. Where another domain claims a -subdirectory rather than a whole tree, the rest of that tree is yours: the -other two claim `website/public/`, `website/src/`, and `website/scripts/`, so -`website/`'s remaining files — `vite.config.ts`, `react-router.config.ts`, -`package.json`, `tsconfig.json` — are yours. Check one level down wherever a -claim is partial, or the same orphaning happens again inside a directory -instead of beside it. +subdirectory rather than a whole tree, the rest of that tree is yours — so +check one level down wherever a claim is partial, or the same orphaning +happens inside a directory instead of beside it. `website/` is *not* an +example of this any more: `supply-chain` claims all of it except +`website/public/`, so none of it is yours. That was fixed by stating the claim +as a subtraction rather than as two named subdirectories, which is the shape +to prefer when you find the next one. Today the remainder is `lib/`, `server/`, `server-lib-common/`, `standalone/`, `vscode-ext/`, `dor/`, `dor-lib-common/`, `canopy/`, `deploy/`, `docs/`, diff --git a/SECURITY.md b/SECURITY.md index 670c517d..8324c3dc 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -50,11 +50,13 @@ Four credentials outlive a process, and each one is a full bypass of some layer | Setup password | `config/server.env` in the install root | mode `0600`, generated locally, never printed by a routine install and never in the LaunchAgent plist | | `hostToken` (the `/ws/host` bearer) | server `hosts.json`; Host side in the enrollment record | server state dir `0700` + every file `0600`; Host side a `0600` file in standalone, `SecretStorage` (the OS keychain) in VS Code — never a webview realm | | VAPID private key | server `vapid.json` | same `0700`/`0600` treatment | +| Session snapshots | `sessions/` under the standalone app-data dir | `0700` dir, `0600` file, best-effort on unix. These are `PersistedWindow` blobs carrying terminal **transcripts** — whatever the user's shells printed, which is a superset of every other secret here. They inherited the umask as `0644` until this was tightened | | Host ACL | `HostStateStore`, keyed per `hostId` | a `0600` file in standalone; VS Code `globalState`. The records are public keys, so confidentiality is not the concern — but neither store provides *integrity* against a process running as the same user, and nothing here claims otherwise. What the mode buys is that another local **account** cannot add a record; a same-user compromise already reads the terminals. Deliberately never on the Server | Without explicit modes these files inherit the umask and end up world-readable, which hands live host tokens to any other local account on a shared machine. The Client's device key is the exception that needs no file protection: it is a non-extractable `CryptoKey` in IndexedDB and is never exported. - FAIL IF `server/src/state.ts` stops creating `$DORMOUSE_STATE_DIR` mode `0o700`, or stops writing every file through `writeAtomic` at mode `0o600`. The "every file" clause is a negative search over `server/src/`: no `writeFile`, `appendFile`, or `createWriteStream` may target the state directory outside `writeAtomic`. +- FAIL IF `write_session_to` in `standalone/src-tauri/src/lib.rs` stops restricting the `sessions/` directory to `0700` and the snapshot to `0600` on unix. The mode is applied to the temp file *before* any bytes are written, because the atomic rename preserves it — tightening after the rename would leave a window where the transcript is world-readable. - FAIL IF `FileHostStateStore` (`lib/src/host/remote/host-state-store.ts`) stops creating its directory `0o700` and writing `0o600` on non-Windows platforms, or if `VsCodeHostStateStore` stops keeping the **enrollment** in `SecretStorage`. The ACL's home in `globalState` is deliberate and is not a finding; the enrollment's is what carries `hostToken`. - FAIL IF `deploy/local/install-macos.sh` stops generating the setup password locally from at least 32 bytes of `/dev/urandom`. Its own length guard is in hex characters, so it must require 64, not 32 — a guard reading `-ge 32` passes a regression to half the entropy. - FAIL IF the installer stops writing `config/server.env` at mode `0600` under `umask 077`, or stops keeping `config/` and `state/` at `0700`. diff --git a/docs/specs/standalone.md b/docs/specs/standalone.md index 11ec2e82..044f2b99 100644 --- a/docs/specs/standalone.md +++ b/docs/specs/standalone.md @@ -368,8 +368,15 @@ reader). A days-long session made this pathological. cannot truncate the previous snapshot. The temp file is fsynced before the rename, and on unix the sessions directory is fsynced *after* the rename (a directory-entry fsync is what makes the rename itself durable; Windows has no -equivalent concept, so that step is unix-only). There is no WAL to grow, and -overwriting in place bounds the on-disk size to one blob. **Window identity is implicit**: +equivalent concept, so that step is unix-only). On unix the directory is +`0700` and the file `0600`, applied to the temp file *before* any bytes are +written because the rename preserves its mode — the blob carries terminal +transcripts, so under the bare umask it landed `0644` and any other local +account could read the user's scrollback (`SECURITY.md` -> Remote Control, +Credentials at rest). Best-effort and unix-only: Windows ACLs are not unix +modes, and a filesystem without POSIX permissions must not fail a save. There +is no WAL to grow, and overwriting in place bounds the on-disk size to one +blob. **Window identity is implicit**: each command keys by the invoking `tauri::Window`'s `label()`, so the frontend stays window-agnostic and a second window (`win-2`, …) persists to its own file without ever rewriting the first window's blob — the store is multi-window even diff --git a/standalone/src-tauri/src/lib.rs b/standalone/src-tauri/src/lib.rs index bbcef313..4bb33589 100644 --- a/standalone/src-tauri/src/lib.rs +++ b/standalone/src-tauri/src/lib.rs @@ -774,8 +774,31 @@ fn read_session_from(dir: &Path, label: &str) -> Result, String> } } +/// Tighten a path to owner-only, best-effort. +/// +/// Session snapshots are `PersistedWindow` blobs carrying terminal +/// *transcripts* — scrollback, so whatever the user's shells printed: +/// tokens echoed by a failing curl, a pasted connection string, the contents +/// of a `.env` someone `cat`ed. Written under the umask they land `0644` in a +/// `0755` directory, readable by every other account on the machine. The +/// Host's own state file is already `0600` in a `0700` directory for a +/// strictly *smaller* secret (`lib/src/host/remote/host-state-store.ts`), so +/// this is closing an inconsistency, not inventing a rule. +/// +/// Best-effort on purpose, and unix-only: Windows ACLs are not unix modes, +/// and a filesystem without POSIX permissions must not fail a session save. +#[cfg(unix)] +fn restrict_to_owner(path: &Path, mode: u32) { + use std::os::unix::fs::PermissionsExt; + let _ = std::fs::set_permissions(path, std::fs::Permissions::from_mode(mode)); +} + +#[cfg(not(unix))] +fn restrict_to_owner(_path: &Path, _mode: u32) {} + fn write_session_to(dir: &Path, label: &str, state: &str) -> Result<(), String> { create_dir_all(dir).map_err(|e| format!("create sessions dir: {e}"))?; + restrict_to_owner(dir, 0o700); let file_name = session_file_name(label); let path = dir.join(&file_name); let tmp = dir.join(format!("{file_name}.tmp")); @@ -783,6 +806,9 @@ fn write_session_to(dir: &Path, label: &str, state: &str) -> Result<(), String> // target so a crash mid-write can never truncate the previous good snapshot. { let mut f = File::create(&tmp).map_err(|e| format!("open temp: {e}"))?; + // Before any bytes land: the rename below preserves the temp file's + // mode, so tightening here is what makes the final snapshot 0600. + restrict_to_owner(&tmp, 0o600); f.write_all(state.as_bytes()) .map_err(|e| format!("write temp: {e}"))?; f.sync_all().map_err(|e| format!("fsync temp: {e}"))?; From f6530bcc2c9d6a044106e755b7ea6a576a1a8a17 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 19:23:21 -0700 Subject: [PATCH 13/19] security(remote): bound pending pairings, and make user verification reachable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two WARNINGs from the application-security domain, both fixed. The third is documented rather than fixed, for a stated reason. **Pending pairings were unbounded in two places the ceremony's own cap did not cover.** `MAX_PENDING_TICKETS` claims to stop a hostile relay turning `pair` frames into unbounded memory "in the process that owns the PTYs" — but each frame also allocates in `RemoteHost.#clients` and in the service's mirrored queue, both keyed by a relay-chosen `clientId` and removed only by a `client-gone` a hostile relay never sends. Measured by the audit: 5000 frames retained 5000 requests holding ~16 MB of relay-chosen strings while `#tickets` sat at 64, and because the service re-serializes its whole queue to the webview per frame the bridge traffic is quadratic. What makes it worth fixing rather than accepting is who can reach it. SECURITY.md documents a synced or stolen passkey as buying "sign-in, and the ability to *ask*", with the Client reaching "nothing" — but such a session passes `checkPair` on every attempt, gets a fresh `clientId` per socket, and can refresh presence indefinitely. Asking was a denial of service. Both maps are now capped at `MAX_PENDING_PAIRINGS`, oldest evicted first and answered with a denial so the evicted client learns its request is gone. **`requireUserVerification` was unreachable on both sides** while two comments asserted the halves "must mirror" or they "silently disagree on what a valid assertion is". The server-side enforcement exists and is tested; only the config path was missing, and the Host had no mechanism at all. Rather than delete a tested control, it is wired through: `DORMOUSE_REQUIRE_USER_VERIFICATION=true` on the Server, mirrored into `HostEnrollResponse` and thence the Host's `ConnectionPolicy`. Optional and additive — absent means off, so an older Host reading a newer Server, or the reverse, degrades to presence rather than failing. Only the exact string `true` enables it: a misspelling must read as off, because turning UV on without UV-capable authenticators locks the account out of its own server. **`hostToken` in the `/ws/host` query string is documented, not changed.** The rationale in the wire contract — browsers cannot set WebSocket headers — is true of `/ws/client` and not of `/ws/host`, which only a Node process opens. But closing it means an `Authorization` header, a `createWebSocket` signature that can carry one, and matching relay support, across both hosts; it cannot be integration-tested here, and the audit rates it INFO because TLS covers it in transit and nothing in this repo logs the URL. Named in SECURITY.md as a hardening gap with the remedy spelled out. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- SECURITY.md | 4 ++- docs/specs/remote-security-model.md | 12 ++++++- docs/specs/server.md | 1 + lib/src/host/remote/service.ts | 11 ++++++ lib/src/remote/host/enrollment.ts | 22 +++++++++++- lib/src/remote/host/remote-host.test.ts | 34 ++++++++++++++++++ lib/src/remote/host/remote-host.ts | 43 ++++++++++++++++++++++- server-lib-common/src/remote/wire.ts | 12 +++++++ server-lib-common/src/security/pairing.ts | 25 ++++++++++++- server/src/app.ts | 6 +++- server/src/config.ts | 12 +++++++ server/test/hosts.test.mjs | 14 ++++++++ 12 files changed, 190 insertions(+), 6 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 8324c3dc..bab2c174 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,7 +12,7 @@ The design lives in [`docs/specs/remote-security-model.md`](docs/specs/remote-se ### Trust boundary -Four layers, none sufficient alone: a passkey proves fresh user presence, a non-extractable per-browser device key is long-lived Client identity, the Host's local ACL authorizes the *pair* of those two, and the Host makes the final access decision. What each compromise actually buys: +Four layers, none sufficient alone: a passkey proves fresh user presence, a non-extractable per-browser device key is long-lived Client identity, the Host's local ACL authorizes the *pair* of those two, and the Host makes the final access decision. A deployment can raise the first layer from presence to *user verification* — biometric or PIN rather than a touch — with `DORMOUSE_REQUIRE_USER_VERIFICATION=true`; the flag is mirrored to every Host in its enrollment response, because the Host is the final authority and a Server demanding UV while the Host did not would leave the weaker verifier deciding. What each compromise actually buys: - **Server compromise** — relay traffic and account state, and **no new authorization**: a forged account, a forged presence stamp, and an injected `ConnectionRequest` all still arrive in front of `authorizeConnection` on the Host, which re-verifies the passkey assertion and the device-key signature against its own ACL and its own `ConnectionPolicy` (the origin/rpId recorded at enrollment) regardless of what the Server claims to have checked. It cannot make the Host trust a device the user never approved. @@ -25,6 +25,8 @@ The property to hold on to: **the only path into a Host's ACL is a human clickin - FAIL IF the Host stops being the final authority: `authorizeConnection` in `server-lib-common/src/security/connection.ts` must verify the passkey assertion, the device-key signature, and the ACL against the Host's own `ConnectionPolicy` before any session is established, and no code path may let a Server-supplied claim stand in for any of the three. - FAIL IF local approval stops being the only thing that **mints** an ACL record: `HostAcl.approve` must have no caller other than `PairingCeremony.approve`, and an approval must be matched against the immutable `pairingId` of the request that was displayed, never against a mutable `clientId` alone. (Records can also be *carried* rather than minted — see the `adopt` bullet below — which is a different act and is bounded separately.) +- FAIL IF the pending-pairing queues are unbounded. Every `pair` frame allocates under a `clientId` the relay chooses, in **both** `RemoteHost`'s client map and the service's mirrored queue, and the only thing that removes one is a `client-gone` a hostile relay simply never sends. Both are capped at `MAX_PENDING_PAIRINGS`, oldest evicted first, because either can be fed independently and a cap only one honors is not a cap. Capping the ceremony's ticket map alone was not enough: it left 5000 frames retaining megabytes of relay-chosen strings in the process that owns every PTY, with the whole queue re-serialized to the webview per frame, so the cost was quadratic. Reachable by anything that can sign in — a synced or stolen passkey buys "the ability to ask", and this is what stops asking from being a denial of service. +- FAIL IF `requireUserVerification` is reachable on one side without being mirrored to the other: the Server reads `DORMOUSE_REQUIRE_USER_VERIFICATION`, and `HostEnrollResponse` must carry it into the Host's `ConnectionPolicy`. - FAIL IF the Host accepts a `pair` frame it has not shape-validated itself. `isPairingRequest` runs on the Server too, and that is exactly why the Host cannot rely on it: a relay-supplied object reaching `PairingCeremony.begin` puts unvalidated fields into the approval UI and, on approval, into a persisted record. The `requestedLabel` must likewise be reduced with `boundedPairingLabel` before any consumer sees it — it is attacker-chosen text rendered in the one dialog the ACL rests on. - FAIL IF any **service→webview** message can carry `hostToken`. Check the direction, not just the identifier: `RemoteHostResult`, `HostStatusEvent`, `PairingQueueEvent`, and `RemoteHostConsoleStatus` in `lib/src/host/remote/service-protocol.ts` are the outbound shapes, and none may expose it. (Inbound is a different matter — `EnrollParams` carries the setup password and `AdoptParams.enrollment` structurally carries a `hostToken`, both by design: enrolling is initiated from the webview, whether from the Settings dialog or the `window.dormouseRemoteHost` console hook.) - FAIL IF `adopt` — the migration hand-off from builds that persisted the Host in webview `localStorage`, and the one command that carries ACL records inbound — stops being bounded on all three of: the service's own store holding **no** enrollment, a `serverUrl` inside the baked allowlist below, and every record passing the full `isHostAclRecord` shape guard as well as the `hostId` match. It is not a second authorization path: it cannot touch a machine that already has a Host, and it authorizes nothing that whoever could already write the webview's storage could not authorize by other means. It is bounded because it converts *local* compromise into *persistent remote* access, which outlives the local compromise being fixed. diff --git a/docs/specs/remote-security-model.md b/docs/specs/remote-security-model.md index 8901858b..f683c08a 100644 --- a/docs/specs/remote-security-model.md +++ b/docs/specs/remote-security-model.md @@ -229,8 +229,18 @@ lacks (there `authorizeConnection` verifies presence itself). Under Server compromise, a forged freshness stamp gets an attacker no further than the human staring at the approval modal. +**Pending requests are bounded.** A `pair` frame allocates in three places — +the ceremony's ticket map, the Host's per-`clientId` client map, and the +service's queue mirrored to the webview — under a `clientId` the relay chooses, +and only a `client-gone` removes one. All three are capped +(`MAX_PENDING_TICKETS`, `MAX_PENDING_PAIRINGS`), oldest evicted first and +answered with a denial rather than dropped silently, because anything that can +sign in can send these and a queue that only grows wedges the process that owns +every PTY. + Source of truth: `PairingRequest` / `PairingTicket` / `PairingCeremony` / -`PAIRING_PRESENCE_WINDOW_MS` in `server-lib-common/src/security/pairing.ts` +`PAIRING_PRESENCE_WINDOW_MS` / `MAX_PENDING_PAIRINGS` in +`server-lib-common/src/security/pairing.ts` (tickets are single-use with a `DEFAULT_PAIRING_TTL_MS` = 5-minute TTL; approval after expiry fails without touching the ACL — the presence window gates the *request*, not the approver's deliberation). The wire sequence — diff --git a/docs/specs/server.md b/docs/specs/server.md index ea7670b7..398c0bfd 100644 --- a/docs/specs/server.md +++ b/docs/specs/server.md @@ -39,6 +39,7 @@ UI lives in `lib`/`standalone`. | `DORMOUSE_ORIGIN` | External origin, e.g. `https://dormouse.tailnet.ts.net`. Source of the WebAuthn `rpId`/`origin` and the Host's `ConnectionPolicy`. Defaults to `http://localhost:` for dev. | | `DORMOUSE_STATE_DIR` | Where the JSON state files live. Default `./data`. | | `PORT` | Default 3000. Blank is unset — `Number('')` is 0, which would ask the OS for an ephemeral port and move the server out from under whatever proxy is pointed at it. An explicit `PORT=0` is a `ConfigError` for the same reason: nothing can be pointed at a port that changes every restart. | +| `DORMOUSE_REQUIRE_USER_VERIFICATION` | `true` demands a *user-verified* passkey assertion (biometric/PIN), not merely user presence. Off by default, and only the exact string `true` enables it — a misspelling must read as off, because turning this on without UV-capable authenticators locks the account out of its own server. Mirrored to every Host in its `HostEnrollResponse` so both sides demand the same thing (`SECURITY.md` -> Remote Control). | | `DORMOUSE_BIND_HOST` | Interface to listen on. Unset binds every interface (what a container wants); set `127.0.0.1` when a TLS proxy on the same machine is the front door. | | `DORMOUSE_VAPID_PUBLIC_KEY` / `DORMOUSE_VAPID_PRIVATE_KEY` | Web Push signing keypair. Set both or neither. At startup the Server decodes both, derives the P-256 public point from the private key, and exits on a missing, malformed, or mismatched pair. Unset, the server mints a pair on first boot and persists it to `vapid.json`. | | `DORMOUSE_VAPID_SUBJECT` | `mailto:`/`https:` contact for push-service operators (RFC 8292). Defaults to `DORMOUSE_ORIGIN` when that origin is https and not loopback; otherwise there is no default and push stays off. The Server parses and validates it at startup and exits on an invalid value — including a loopback contact, which Apple rejects. | diff --git a/lib/src/host/remote/service.ts b/lib/src/host/remote/service.ts index 1a3450a1..e0c2f63a 100644 --- a/lib/src/host/remote/service.ts +++ b/lib/src/host/remote/service.ts @@ -18,6 +18,7 @@ * are settled there (`sidecar-entry.ts`), so they never reach this dispatch. */ +import { MAX_PENDING_PAIRINGS } from 'server-lib-common'; import { filterAclRecords } from '../../remote/host/acl'; import { isEnrollment, performEnrollment, type HostEnrollment } from '../../remote/host/enrollment'; import type { HostSurfaceProvider } from '../../remote/host/host-surface-provider'; @@ -434,6 +435,16 @@ export class RemoteHostService { // --- Pairing queue --- #enqueuePairing(pending: PendingPairing): void { + // Bounded, like the controller's own map: this one is mirrored to the + // webview in full on every change, so an unbounded queue costs quadratic + // bridge traffic on top of the memory. `RemoteHost` evicts on its side too; + // both are capped because either can be fed independently, and a cap that + // only one of them honors is not a cap. + while (this.#pairings.size >= MAX_PENDING_PAIRINGS) { + const oldest = this.#pairings.keys().next(); + if (oldest.done) break; + this.#pairings.delete(oldest.value); + } // Coalesce by clientId: a re-sent pair for the same client replaces the old. this.#pairings.set(pending.clientId, pending); this.#emitQueue(); diff --git a/lib/src/remote/host/enrollment.ts b/lib/src/remote/host/enrollment.ts index 828c7417..694647e9 100644 --- a/lib/src/remote/host/enrollment.ts +++ b/lib/src/remote/host/enrollment.ts @@ -28,6 +28,16 @@ export interface HostEnrollment { origin: string; /** The Host's `ConnectionPolicy.rpId`. */ rpId: string; + /** + * The Host's `ConnectionPolicy.requireUserVerification`, mirrored from the + * Server at enrollment so the two cannot disagree about what a valid + * assertion is. + * + * Optional, and absent means `false`: an enrollment persisted by an older + * build has no such field, and it must keep loading rather than being + * rejected as malformed. + */ + requireUserVerification?: boolean; } /** @@ -44,7 +54,11 @@ export function isEnrollment(value: unknown): value is HostEnrollment { typeof v.hostId === 'string' && typeof v.hostToken === 'string' && typeof v.origin === 'string' && - typeof v.rpId === 'string' + typeof v.rpId === 'string' && + // Optional — absent is the documented default. Present-but-wrong-typed is + // still a rejection: a store that round-trips `"false"` as truthy would be + // the silent disagreement this field exists to prevent. + (v.requireUserVerification === undefined || typeof v.requireUserVerification === 'boolean') ); } @@ -119,6 +133,12 @@ export async function performEnrollment( hostToken: enrolled?.hostToken, origin: enrolled?.origin, rpId: enrolled?.rpId, + // Only when the server actually sent a boolean: spreading `undefined` in + // would make the key present-and-undefined, which the guard treats the + // same but a store round-trip would not. + ...(typeof enrolled?.requireUserVerification === 'boolean' + ? { requireUserVerification: enrolled.requireUserVerification } + : {}), }; if (!isEnrollment(enrollment)) { throw new Error( diff --git a/lib/src/remote/host/remote-host.test.ts b/lib/src/remote/host/remote-host.test.ts index fc093019..c49d2a77 100644 --- a/lib/src/remote/host/remote-host.test.ts +++ b/lib/src/remote/host/remote-host.test.ts @@ -32,6 +32,7 @@ import { type ConnectionRequest, type HostAclRecord, type PairingRequest, + MAX_PENDING_PAIRINGS, } from 'server-lib-common'; import { RemoteHost } from './remote-host'; import type { HostEnrollment } from './enrollment'; @@ -239,6 +240,39 @@ describe('RemoteHost frame handling', () => { expect(Array.from(savedRecords[0]!.label).length).toBeLessThanOrEqual(64); }); + it('bounds pending pairings so pair frames cannot grow the host unbounded', () => { + makeHost(); + // Every `pair` frame allocates under a relay-chosen clientId, and + // `client-gone` — the only thing that removes one — is what a hostile relay + // simply never sends. Unbounded, 5000 frames retained 5000 requests holding + // megabytes of relay-chosen strings in the process that owns every PTY. + const sent = 200; + for (let i = 0; i < sent; i++) { + socket.receive({ + t: 'pair', + clientId: `c${i}`, + request: { + accountId: 'owner', + passkeyCredentialId: `cred-${i}`, + passkeyPublicKeyHash: `hash-${i}`, + devicePublicKey: `device-${i}`, + requestedLabel: `iPhone ${i}`, + }, + }); + } + + // `approvals` is the harness's cumulative call log, so it counts every + // request ever shown — the live queue is what is bounded. Evictions are + // observable as denials on the wire, which is also the point: an evicted + // client is told, rather than left waiting on a modal that no longer + // exists. + const denials = socket.frames('pair-result').filter((f) => f.approved === false); + expect(denials).toHaveLength(sent - MAX_PENDING_PAIRINGS); + expect(denials.every((f) => f.error === 'superseded')).toBe(true); + // Nothing reached the ACL without a human. + expect(savedRecords).toHaveLength(0); + }); + it('deny → pair-result approved:false, ACL untouched', () => { makeHost(); socket.receive({ diff --git a/lib/src/remote/host/remote-host.ts b/lib/src/remote/host/remote-host.ts index b0b11136..1fa936d2 100644 --- a/lib/src/remote/host/remote-host.ts +++ b/lib/src/remote/host/remote-host.ts @@ -32,6 +32,7 @@ import { WS_ROUTES, WS_TOKEN_PARAM, authorizeConnection, + MAX_PENDING_PAIRINGS, boundedPairingAccount, boundedPairingLabel, isPairingRequest, @@ -148,7 +149,14 @@ export class RemoteHost { constructor(options: RemoteHostOptions) { this.#enrollment = options.enrollment; - this.#policy = { rpId: options.enrollment.rpId, origin: options.enrollment.origin }; + this.#policy = { + rpId: options.enrollment.rpId, + origin: options.enrollment.origin, + // Mirrored from the Server at enrollment. Both sides must demand the + // same thing: the Host is the final authority, so a Server enforcing UV + // while the Host does not would leave the weaker verifier deciding. + requireUserVerification: options.enrollment.requireUserVerification ?? false, + }; this.#now = options.now ?? (() => Date.now()); this.#acl = loadHostAcl(options.enrollment.hostId, options.loadAcl); this.#challenges = new HostChallengeIssuer({ now: this.#now }); @@ -276,6 +284,29 @@ export class RemoteHost { this.#clients.clear(); } + /** + * Drop the oldest pending pairing when the queue is full, so a new request + * displaces one rather than growing the map. Oldest first: whoever initiated + * it is the least likely to still be waiting on the modal. + */ + #evictOldestPairingIfFull(): void { + let pendingCount = 0; + for (const state of this.#clients.values()) if (state.pending) pendingCount++; + while (pendingCount >= MAX_PENDING_PAIRINGS) { + let oldestId: string | null = null; + let oldestAt = Number.POSITIVE_INFINITY; + for (const [id, state] of this.#clients) { + if (state.pending && state.pending.requestedAt < oldestAt) { + oldestAt = state.pending.requestedAt; + oldestId = id; + } + } + if (oldestId === null) return; + this.#denyPairing(oldestId, this.#clients.get(oldestId)!.pending!.pairingId, 'superseded'); + pendingCount--; + } + } + /** Get or create the per-client state record for `clientId`. */ #clientState(clientId: string): ClientState { let state = this.#clients.get(clientId); @@ -359,6 +390,16 @@ export class RemoteHost { approve: (label) => this.#approvePairing(clientId, ticket.pairingId, label), deny: (error) => this.#denyPairing(clientId, ticket.pairingId, error), }; + // Bound the queue before adding to it. Every `pair` frame allocates a + // `#clients` entry under a relay-chosen `clientId`, and those are removed + // only by `client-gone` — which a hostile relay simply never sends — or by + // the socket dropping. Unbounded, 5000 frames retain 5000 pending requests + // holding megabytes of relay-chosen strings in the process that owns every + // PTY, and the service re-serializes the whole queue to the webview on each + // one, so the traffic is quadratic. Reachable by anything that can sign in: + // a synced or stolen passkey is documented as buying only "the ability to + // ask", and this is what stops asking from being a denial of service. + this.#evictOldestPairingIfFull(); this.#clientState(clientId).pending = pending; this.#requestApproval(pending); } diff --git a/server-lib-common/src/remote/wire.ts b/server-lib-common/src/remote/wire.ts index 55b2301c..ecb90863 100644 --- a/server-lib-common/src/remote/wire.ts +++ b/server-lib-common/src/remote/wire.ts @@ -148,6 +148,18 @@ export interface HostEnrollResponse { /** What the Host must enforce as its ConnectionPolicy. */ origin: string; rpId: string; + /** + * Whether the Host must demand a user-verified assertion (biometric/PIN, + * not merely presence). + * + * Optional and additive: an older Host reading a newer server's response + * ignores it, and a newer Host reading an older server's sees `undefined`, + * which is the same as `false`. It travels here rather than being + * configured on the Host because the invariant is that the two sides + * *mirror* — a Server demanding UV while the Host does not means the Host is + * the weaker verifier, and the Host is the one that decides access. + */ + requireUserVerification?: boolean; } export interface HostsResponse { diff --git a/server-lib-common/src/security/pairing.ts b/server-lib-common/src/security/pairing.ts index 5cc49f95..54384bc1 100644 --- a/server-lib-common/src/security/pairing.ts +++ b/server-lib-common/src/security/pairing.ts @@ -27,10 +27,33 @@ export const DEFAULT_PAIRING_TTL_MS = 5 * 60 * 1000; /** * How many tickets one ceremony will hold. Far above any real use — a human * approves one at a time — and low enough that a hostile relay cannot turn - * `pair` frames into unbounded memory in the process that owns the PTYs. + * `pair` frames into unbounded memory here. + * + * This bounds the ceremony's own map and nothing else. The Host keeps its own + * per-`clientId` records of a pending pairing, and the service mirrors a queue + * of them to the webview; both are bounded separately by + * {@link MAX_PENDING_PAIRINGS}. Capping only this map is what let 5000 `pair` + * frames retain ~16 MB of relay-chosen strings while `#tickets` sat happily at + * 64. */ const MAX_PENDING_TICKETS = 64; +/** + * How many pairing requests may await local approval at once, across the + * Host's own client map and the service's mirrored queue. + * + * Much smaller than {@link MAX_PENDING_TICKETS}, because this is the number a + * *human* is being asked to look at: past a handful the modal is not a + * decision any more. Oldest is evicted first — the person who initiated the + * oldest request is the least likely to still be watching for it. + * + * Every `pair` frame allocates in both structures keyed by a `clientId` the + * relay chooses, and the service re-serializes its whole queue to the webview + * on each change, so the cost of leaving them unbounded is quadratic rather + * than linear. + */ +export const MAX_PENDING_PAIRINGS = 8; + /** * How recent the session's last server-verified passkey assertion must be for * the Server to relay a pairing request. Tight on purpose: it covers diff --git a/server/src/app.ts b/server/src/app.ts index 12392f96..dace5872 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -398,7 +398,11 @@ export function createApp(config: AppConfig): CreatedApp { hostToken: host.hostToken, origin, rpId, - }; + // Mirrored to the Host so both sides demand the same thing. The Host + // is the final authority, so a Server that demands UV while the Host + // does not would leave the weaker verifier deciding access. + ...(config.requireUserVerification ? { requireUserVerification: true } : {}), +}; return c.json(res); }); diff --git a/server/src/config.ts b/server/src/config.ts index bb3b9b7e..3a84704b 100644 --- a/server/src/config.ts +++ b/server/src/config.ts @@ -34,6 +34,13 @@ export interface ServerConfig { * origin is unusable as one on a loopback dev server. */ vapidSubject: string | null; + /** + * Demand a user-verified passkey assertion (biometric/PIN), not merely user + * presence. Off by default because a deployment whose authenticators cannot + * do UV would lock itself out; it is mirrored to every Host at enrollment so + * the two sides cannot disagree about what a valid assertion is. + */ + readonly requireUserVerification: boolean; } /** Thrown for a missing or unusable environment; the entrypoint exits on it. */ @@ -62,6 +69,10 @@ export function readConfig(env: Env = process.env): ServerConfig { } const bindHost = env.DORMOUSE_BIND_HOST?.trim() || undefined; + // Opt-in, and only the exact string: an unset or misspelled value must read + // as "off" rather than as "on", because turning this on without + // UV-capable authenticators locks the account out of its own server. + const requireUserVerification = env.DORMOUSE_REQUIRE_USER_VERIFICATION?.trim() === 'true'; const origin = env.DORMOUSE_ORIGIN ?? `http://localhost:${port}`; const stateDir = env.DORMOUSE_STATE_DIR ?? './data'; @@ -94,6 +105,7 @@ export function readConfig(env: Env = process.env): ServerConfig { return { port, bindHost, + requireUserVerification, setupPassword, origin, stateDir, diff --git a/server/test/hosts.test.mjs b/server/test/hosts.test.mjs index d35de4a8..997ef76c 100644 --- a/server/test/hosts.test.mjs +++ b/server/test/hosts.test.mjs @@ -133,3 +133,17 @@ test('a host socket opens with a real enrollment token', async () => { await server.close(); } }); + +test('enrollment mirrors requireUserVerification to the Host, and omits it when off', async () => { + // The flag has to travel: the Host is the final authority on an assertion, + // so a Server that demands UV while the Host does not leaves the weaker + // verifier deciding access. Absent means false, which is what an older Host + // reading a newer server — or either reading an older one — must see. + const on = await freshApp({ requireUserVerification: true }); + const { body: uvOn } = await enrollHost(on.app, { label: 'uv-on' }); + assert.equal(uvOn.requireUserVerification, true); + + const off = await freshApp(); + const { body: uvOff } = await enrollHost(off.app, { label: 'uv-off' }); + assert.equal('requireUserVerification' in uvOff, false); +}); From c0af61778dc4e53dbcb64380886008aa75b0e43c Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 19:35:43 -0700 Subject: [PATCH 14/19] security(remote): evict the pairing record, not just its payload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from tend-review on #452, all correct, all fixed. **The pairing bound closed the bounded half.** `#evictOldestPairingIfFull` called `#denyPairing`, which clears `state.pending` and leaves the `#clients` entry in place — so the map still grew by one slot per distinct `pair` frame. Worse, the residual is the *unbounded* part: every `PairingRequest` field is capped at `PAIRING_FIELD_LIMIT`, but `clientId` was only type-checked, so a relay could make each retained key arbitrarily large. Eviction now drops the record (guarded on the client being neither established nor session-holding, since `#clientState` recreates it if that client is heard from again), and `clientId` is length-bounded at the frame boundary. My test would not have caught it: it asserted denial counts and `savedRecords`, neither of which observes map size. `trackedClientCount` exists now so the bound is observable, and I verified the assertion fails against the unfixed code (200 vs 8) before restoring it. A bound nothing can observe is exactly how the first version passed its own test. **The verdict-dissent guard had two cases where it needed three.** It recognized only `^VERDICT: FAIL`, so a fragment whose first line is anything else — a subagent that ignored the brand-new preamble, a stray heading — fell through to an unchallenged `PASS`. That puts the verdict back on a prompt having been followed, which is the thing the guard exists to stop being the control, and it is the *likely* first state since the instruction ships in this PR. A verdictless fragment now reads MISSING rather than FAIL — a report we cannot read is an audit that did not finish, not a finding — and deliberately not via `MISSING_FRAGMENTS`, whose message says no fragment was left at all. Exercised all four paths. **Two smaller ones:** the spec attributed "answered with a denial" to all three eviction sites when only the controller does that (the ceremony and the service's queue both bare-`delete`), and my `app.ts` insertion had mangled indentation with the closing brace at column 0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/security-audit.yaml | 24 +++++++++++++++---- SECURITY.md | 4 ++-- docs/specs/remote-security-model.md | 10 ++++---- lib/src/remote/host/remote-host.test.ts | 7 +++++- lib/src/remote/host/remote-host.ts | 32 ++++++++++++++++++++++++- server/src/app.ts | 4 ++-- 6 files changed, 66 insertions(+), 15 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 2c2f4d18..6d9d05b2 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -335,11 +335,25 @@ jobs: # shellcheck disable=SC2086 for f in $AUDIT_FRAGMENTS; do [ -s "$f" ] || continue - if head -n1 "$f" | grep -q '^VERDICT: FAIL'; then - echo "::error::Audit reported PASS but $f opens with VERDICT: FAIL; downgrading to FAIL." - DISSENTING="${DISSENTING:+$DISSENTING, }$f" - STATUS=FAIL - fi + # Three cases, not two. A fragment whose first line is neither + # verdict — a subagent that ignored the preamble, a stray heading + # or blank line — must not fall through to an unchallenged PASS: + # that would put the verdict back on a prompt having been + # followed, which is the thing this guard exists to stop being + # the control. MISSING rather than FAIL, since a report we cannot + # read is an audit that did not finish rather than a finding, and + # deliberately not folded into MISSING_FRAGMENTS, whose message + # says no fragment was left at all. + case "$(head -n1 "$f")" in + 'VERDICT: FAIL'*) + echo "::error::Audit reported PASS but $f opens with VERDICT: FAIL; downgrading to FAIL." + DISSENTING="${DISSENTING:+$DISSENTING, }$f" + STATUS=FAIL ;; + 'VERDICT: PASS'*) ;; + *) + echo "::warning::$f has no VERDICT line; the dissent check cannot read it." + [ "$STATUS" = FAIL ] || STATUS=MISSING ;; + esac done if [ -n "${DISSENTING:-}" ]; then { diff --git a/SECURITY.md b/SECURITY.md index bab2c174..5e3f02c2 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -25,7 +25,7 @@ The property to hold on to: **the only path into a Host's ACL is a human clickin - FAIL IF the Host stops being the final authority: `authorizeConnection` in `server-lib-common/src/security/connection.ts` must verify the passkey assertion, the device-key signature, and the ACL against the Host's own `ConnectionPolicy` before any session is established, and no code path may let a Server-supplied claim stand in for any of the three. - FAIL IF local approval stops being the only thing that **mints** an ACL record: `HostAcl.approve` must have no caller other than `PairingCeremony.approve`, and an approval must be matched against the immutable `pairingId` of the request that was displayed, never against a mutable `clientId` alone. (Records can also be *carried* rather than minted — see the `adopt` bullet below — which is a different act and is bounded separately.) -- FAIL IF the pending-pairing queues are unbounded. Every `pair` frame allocates under a `clientId` the relay chooses, in **both** `RemoteHost`'s client map and the service's mirrored queue, and the only thing that removes one is a `client-gone` a hostile relay simply never sends. Both are capped at `MAX_PENDING_PAIRINGS`, oldest evicted first, because either can be fed independently and a cap only one honors is not a cap. Capping the ceremony's ticket map alone was not enough: it left 5000 frames retaining megabytes of relay-chosen strings in the process that owns every PTY, with the whole queue re-serialized to the webview per frame, so the cost was quadratic. Reachable by anything that can sign in — a synced or stolen passkey buys "the ability to ask", and this is what stops asking from being a denial of service. +- FAIL IF the pending-pairing queues are unbounded. Every `pair` frame allocates under a `clientId` the relay chooses, in **both** `RemoteHost`'s client map and the service's mirrored queue, and the only thing that removes one is a `client-gone` a hostile relay simply never sends. Both are capped at `MAX_PENDING_PAIRINGS`, oldest evicted first, because either can be fed independently and a cap only one honors is not a cap. The controller's eviction drops the **record**, not merely the request it holds: clearing the payload while keeping the map slot bounds the part that `PAIRING_FIELD_LIMIT` already bounded and leaves the relay-chosen key — which is why `clientId` is length-bounded at the frame boundary too. Capping the ceremony's ticket map alone was not enough: it left 5000 frames retaining megabytes of relay-chosen strings in the process that owns every PTY, with the whole queue re-serialized to the webview per frame, so the cost was quadratic. Reachable by anything that can sign in — a synced or stolen passkey buys "the ability to ask", and this is what stops asking from being a denial of service. - FAIL IF `requireUserVerification` is reachable on one side without being mirrored to the other: the Server reads `DORMOUSE_REQUIRE_USER_VERIFICATION`, and `HostEnrollResponse` must carry it into the Host's `ConnectionPolicy`. - FAIL IF the Host accepts a `pair` frame it has not shape-validated itself. `isPairingRequest` runs on the Server too, and that is exactly why the Host cannot rely on it: a relay-supplied object reaching `PairingCeremony.begin` puts unvalidated fields into the approval UI and, on approval, into a persisted record. The `requestedLabel` must likewise be reduced with `boundedPairingLabel` before any consumer sees it — it is attacker-chosen text rendered in the one dialog the ACL rests on. - FAIL IF any **service→webview** message can carry `hostToken`. Check the direction, not just the identifier: `RemoteHostResult`, `HostStatusEvent`, `PairingQueueEvent`, and `RemoteHostConsoleStatus` in `lib/src/host/remote/service-protocol.ts` are the outbound shapes, and none may expose it. (Inbound is a different matter — `EnrollParams` carries the setup password and `AdoptParams.enrollment` structurally carries a `hostToken`, both by design: enrolling is initiated from the webview, whether from the Settings dialog or the `window.dormouseRemoteHost` console hook.) @@ -294,6 +294,6 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - `application-security` — **everything else**, worked out from `ls -A` rather than from a list, including `.impeccable/` (the design-token snapshot behind `DESIGN.md`). Dotfile directories are named explicitly wherever they land, in this list and in the prompt files, because a catch-all has twice now been read as covering them when no reader could tell which domain owned one. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. -- FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. +- FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one — **or** stops treating a fragment with no readable verdict as inconclusive. Three cases, not two: a fragment the check cannot read must not fall through to an unchallenged `PASS`, because that puts the verdict back on a prompt having been followed, which is the thing this guard exists to stop being the control. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. diff --git a/docs/specs/remote-security-model.md b/docs/specs/remote-security-model.md index f683c08a..ae1e0cc9 100644 --- a/docs/specs/remote-security-model.md +++ b/docs/specs/remote-security-model.md @@ -233,10 +233,12 @@ human staring at the approval modal. the ceremony's ticket map, the Host's per-`clientId` client map, and the service's queue mirrored to the webview — under a `clientId` the relay chooses, and only a `client-gone` removes one. All three are capped -(`MAX_PENDING_TICKETS`, `MAX_PENDING_PAIRINGS`), oldest evicted first and -answered with a denial rather than dropped silently, because anything that can -sign in can send these and a queue that only grows wedges the process that owns -every PTY. +(`MAX_PENDING_TICKETS`, `MAX_PENDING_PAIRINGS`), oldest evicted first — the +controller answers its eviction with a `pair-result` denial and drops the +client record rather than leaving someone on a modal that no longer exists, +while the ceremony and the service's mirrored queue simply delete theirs — +because anything that can sign in can send these and a queue that only grows +wedges the process that owns every PTY. Source of truth: `PairingRequest` / `PairingTicket` / `PairingCeremony` / `PAIRING_PRESENCE_WINDOW_MS` / `MAX_PENDING_PAIRINGS` in diff --git a/lib/src/remote/host/remote-host.test.ts b/lib/src/remote/host/remote-host.test.ts index c49d2a77..007e6932 100644 --- a/lib/src/remote/host/remote-host.test.ts +++ b/lib/src/remote/host/remote-host.test.ts @@ -241,7 +241,7 @@ describe('RemoteHost frame handling', () => { }); it('bounds pending pairings so pair frames cannot grow the host unbounded', () => { - makeHost(); + const host = makeHost(); // Every `pair` frame allocates under a relay-chosen clientId, and // `client-gone` — the only thing that removes one — is what a hostile relay // simply never sends. Unbounded, 5000 frames retained 5000 requests holding @@ -269,6 +269,11 @@ describe('RemoteHost frame handling', () => { const denials = socket.frames('pair-result').filter((f) => f.approved === false); expect(denials).toHaveLength(sent - MAX_PENDING_PAIRINGS); expect(denials.every((f) => f.error === 'superseded')).toBe(true); + // The map itself is bounded, not just the payload it holds: evicting only + // `pending` would free the capped request and keep the slot plus its + // relay-chosen key forever, which is the unbounded half. + expect(host.trackedClientCount).toBeLessThanOrEqual(MAX_PENDING_PAIRINGS); + // Nothing reached the ACL without a human. expect(savedRecords).toHaveLength(0); }); diff --git a/lib/src/remote/host/remote-host.ts b/lib/src/remote/host/remote-host.ts index 1fa936d2..4003e864 100644 --- a/lib/src/remote/host/remote-host.ts +++ b/lib/src/remote/host/remote-host.ts @@ -113,6 +113,17 @@ export interface RemoteHostOptions { reconnect?: boolean; } +/** + * The longest `clientId` this Host will act on. + * + * The relay mints these as base64url of 16 random bytes (~22 characters), so + * this is an order of magnitude of headroom. It exists because the id is a + * *map key* on a hostile-relay path: every other field of a `pair` frame is + * capped by `PAIRING_FIELD_LIMIT`, and bounding those while leaving the key + * free would bound only the part that was already bounded. + */ +const MAX_CLIENT_ID_LENGTH = 256; + const INITIAL_BACKOFF_MS = 1_000; const MAX_BACKOFF_MS = 30_000; @@ -289,6 +300,16 @@ export class RemoteHost { * displaces one rather than growing the map. Oldest first: whoever initiated * it is the least likely to still be waiting on the modal. */ + /** + * How many clients this Host is tracking. Exists for the pending-pairing + * bound's test: the growth it guards against is in a private map, and a + * bound nothing can observe is how the first version of that cap passed its + * own test while the map kept growing. + */ + get trackedClientCount(): number { + return this.#clients.size; + } + #evictOldestPairingIfFull(): void { let pendingCount = 0; for (const state of this.#clients.values()) if (state.pending) pendingCount++; @@ -303,6 +324,14 @@ export class RemoteHost { } if (oldestId === null) return; this.#denyPairing(oldestId, this.#clients.get(oldestId)!.pending!.pairingId, 'superseded'); + // Drop the record too, not just its payload. `#denyPairing` only clears + // `pending`, so without this the map keeps one entry per `pair` frame + // forever under a relay-chosen key — bounding the capped payload while + // leaving the unbounded part. `#clientState` recreates it if that client + // is ever heard from again, and an established or session-holding client + // is left alone. + const evicted = this.#clients.get(oldestId); + if (evicted && !evicted.established && !evicted.session) this.#clients.delete(oldestId); pendingCount--; } } @@ -337,7 +366,8 @@ export class RemoteHost { if ( !frame || typeof (frame as { t?: unknown }).t !== 'string' || - typeof (frame as { clientId?: unknown }).clientId !== 'string' + typeof (frame as { clientId?: unknown }).clientId !== 'string' || + (frame as { clientId: string }).clientId.length > MAX_CLIENT_ID_LENGTH ) { return; } diff --git a/server/src/app.ts b/server/src/app.ts index dace5872..5af27327 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -398,11 +398,11 @@ export function createApp(config: AppConfig): CreatedApp { hostToken: host.hostToken, origin, rpId, - // Mirrored to the Host so both sides demand the same thing. The Host + // Mirrored to the Host so both sides demand the same thing. The Host // is the final authority, so a Server that demands UV while the Host // does not would leave the weaker verifier deciding access. ...(config.requireUserVerification ? { requireUserVerification: true } : {}), -}; + }; return c.json(res); }); From 62848c7a05b0d265fc26b7f0f5151ed6c42b7221 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 19:53:41 -0700 Subject: [PATCH 15/19] security(audit): give the unreadable-verdict case its own issue prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the second review round, all correct. **The verdictless branch landed on prose describing a different failure.** The reviewer's own prior suggestion said plain `MISSING` "lands on the generic Audit produced no verdict branch, which fits" — and it does not. That branch is selected on `MISSING_FRAGMENTS` being non-empty, which the verdictless arm deliberately leaves alone, so it fell through to an `else` opening "The audit step exited without writing audit-status.txt." In this path the status file was written and said `PASS`, every domain reported, and the `UNVERIFIABLE` / `_No report …_` markers it directs the reader to would not exist. Three claims, all false for this cause. There is now a third arm, keyed on its own `UNREADABLE_VERDICTS`, saying what actually happened: the merged verdict could not be trusted because a fragment's first line was not a verdict, the report is complete, and the likely fix is a subagent that ignored `_preamble.md`. Verified every cause selects its own arm, with the missing-fragment case winning when both apply since it is the graver. **"The map itself is bounded" overstated what the cap does.** `#evictOldestPairingIfFull` counts only entries carrying a `pending`, and `#onConnect` creates entries through `#resetAuthorization` that carry none — so a pure-`connect` workload still grows the map until the socket drops. Not a regression, and not something to fix here: evicting an entry that may be `established` is a different act from denying a pending request, and belongs to a change scoped to the connect path. What was wrong was the claim, in the test comment and in two specs. All three now say the pairing path specifically, and name what keeps the other entries cheap — `MAX_CLIENT_ID_LENGTH`, which was also missing from the spec's `Source of truth:` list despite SECURITY.md leaning on it. **An orphaned doc comment**, from inserting `trackedClientCount` between `#evictOldestPairingIfFull`'s JSDoc and the method it documented. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/security-audit.yaml | 20 ++++++++++++++++++++ SECURITY.md | 2 +- docs/specs/remote-security-model.md | 24 ++++++++++++++++++------ lib/src/remote/host/remote-host.test.ts | 6 ++++-- lib/src/remote/host/remote-host.ts | 18 +++++++++++++----- 5 files changed, 56 insertions(+), 14 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 6d9d05b2..cfc9d502 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -352,6 +352,7 @@ jobs: 'VERDICT: PASS'*) ;; *) echo "::warning::$f has no VERDICT line; the dissent check cannot read it." + UNREADABLE_VERDICTS="${UNREADABLE_VERDICTS:+$UNREADABLE_VERDICTS, }$f" [ "$STATUS" = FAIL ] || STATUS=MISSING ;; esac done @@ -435,6 +436,25 @@ jobs: echo "suspect \`Redact secrets from agent output\` — it deletes every" echo "fragment and the report when it throws, and that means a secret" echo "reached a file in cleartext." + elif [ -n "${UNREADABLE_VERDICTS:-}" ]; then + # A third cause, and it must not borrow either arm's prose: the + # status file WAS written and said `PASS`, and every domain DID + # report — so "exited without writing audit-status.txt" and the + # `UNVERIFIABLE` / `_No report …_` markers below are both wrong + # here. What actually happened is that a fragment's first line + # was not a verdict, so the dissent check could not read it and + # the merged PASS could not be trusted. + echo "Audit verdict withheld at $DATE. [Run]($RUN_URL)" + echo + echo "The audit reported \`PASS\`, but it was downgraded: the first" + echo "line of $UNREADABLE_VERDICTS is neither \`VERDICT: PASS\` nor" + echo "\`VERDICT: FAIL\`, so the reporting step could not confirm that" + echo "domain agreed with the merged verdict. Every domain did report," + echo "and the report below is complete — read the named domain's" + echo "section for its own conclusion. This is a defect in the audit's" + echo "output format, not necessarily a security finding: the fix is" + echo "usually that the subagent did not follow" + echo "\`.github/audit/_preamble.md\`'s rule that line 1 is the verdict." else echo "Audit produced no verdict at $DATE. [Run]($RUN_URL)" echo diff --git a/SECURITY.md b/SECURITY.md index 5e3f02c2..d2d60176 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -25,7 +25,7 @@ The property to hold on to: **the only path into a Host's ACL is a human clickin - FAIL IF the Host stops being the final authority: `authorizeConnection` in `server-lib-common/src/security/connection.ts` must verify the passkey assertion, the device-key signature, and the ACL against the Host's own `ConnectionPolicy` before any session is established, and no code path may let a Server-supplied claim stand in for any of the three. - FAIL IF local approval stops being the only thing that **mints** an ACL record: `HostAcl.approve` must have no caller other than `PairingCeremony.approve`, and an approval must be matched against the immutable `pairingId` of the request that was displayed, never against a mutable `clientId` alone. (Records can also be *carried* rather than minted — see the `adopt` bullet below — which is a different act and is bounded separately.) -- FAIL IF the pending-pairing queues are unbounded. Every `pair` frame allocates under a `clientId` the relay chooses, in **both** `RemoteHost`'s client map and the service's mirrored queue, and the only thing that removes one is a `client-gone` a hostile relay simply never sends. Both are capped at `MAX_PENDING_PAIRINGS`, oldest evicted first, because either can be fed independently and a cap only one honors is not a cap. The controller's eviction drops the **record**, not merely the request it holds: clearing the payload while keeping the map slot bounds the part that `PAIRING_FIELD_LIMIT` already bounded and leaves the relay-chosen key — which is why `clientId` is length-bounded at the frame boundary too. Capping the ceremony's ticket map alone was not enough: it left 5000 frames retaining megabytes of relay-chosen strings in the process that owns every PTY, with the whole queue re-serialized to the webview per frame, so the cost was quadratic. Reachable by anything that can sign in — a synced or stolen passkey buys "the ability to ask", and this is what stops asking from being a denial of service. +- FAIL IF the pending-pairing queues are unbounded. Every `pair` frame allocates under a `clientId` the relay chooses, in **both** `RemoteHost`'s client map and the service's mirrored queue, and the only thing that removes one is a `client-gone` a hostile relay simply never sends. Both are capped at `MAX_PENDING_PAIRINGS`, oldest evicted first, because either can be fed independently and a cap only one honors is not a cap. The controller's eviction drops the **record**, not merely the request it holds: clearing the payload while keeping the map slot bounds the part that `PAIRING_FIELD_LIMIT` already bounded and leaves the relay-chosen key — which is why `MAX_CLIENT_ID_LENGTH` bounds `clientId` at the frame boundary too, before any map is touched. This bounds the *pairing* path; `connect` allocates a client entry by another route that the pairing counter deliberately does not evict, since dropping an entry that may be `established` is a different act. Those carry no request and are cleared when the socket drops. Capping the ceremony's ticket map alone was not enough: it left 5000 frames retaining megabytes of relay-chosen strings in the process that owns every PTY, with the whole queue re-serialized to the webview per frame, so the cost was quadratic. Reachable by anything that can sign in — a synced or stolen passkey buys "the ability to ask", and this is what stops asking from being a denial of service. - FAIL IF `requireUserVerification` is reachable on one side without being mirrored to the other: the Server reads `DORMOUSE_REQUIRE_USER_VERIFICATION`, and `HostEnrollResponse` must carry it into the Host's `ConnectionPolicy`. - FAIL IF the Host accepts a `pair` frame it has not shape-validated itself. `isPairingRequest` runs on the Server too, and that is exactly why the Host cannot rely on it: a relay-supplied object reaching `PairingCeremony.begin` puts unvalidated fields into the approval UI and, on approval, into a persisted record. The `requestedLabel` must likewise be reduced with `boundedPairingLabel` before any consumer sees it — it is attacker-chosen text rendered in the one dialog the ACL rests on. - FAIL IF any **service→webview** message can carry `hostToken`. Check the direction, not just the identifier: `RemoteHostResult`, `HostStatusEvent`, `PairingQueueEvent`, and `RemoteHostConsoleStatus` in `lib/src/host/remote/service-protocol.ts` are the outbound shapes, and none may expose it. (Inbound is a different matter — `EnrollParams` carries the setup password and `AdoptParams.enrollment` structurally carries a `hostToken`, both by design: enrolling is initiated from the webview, whether from the Settings dialog or the `window.dormouseRemoteHost` console hook.) diff --git a/docs/specs/remote-security-model.md b/docs/specs/remote-security-model.md index ae1e0cc9..0e732327 100644 --- a/docs/specs/remote-security-model.md +++ b/docs/specs/remote-security-model.md @@ -229,20 +229,32 @@ lacks (there `authorizeConnection` verifies presence itself). Under Server compromise, a forged freshness stamp gets an attacker no further than the human staring at the approval modal. -**Pending requests are bounded.** A `pair` frame allocates in three places — +**Pending pairings are bounded.** A `pair` frame allocates in three places — the ceremony's ticket map, the Host's per-`clientId` client map, and the service's queue mirrored to the webview — under a `clientId` the relay chooses, -and only a `client-gone` removes one. All three are capped -(`MAX_PENDING_TICKETS`, `MAX_PENDING_PAIRINGS`), oldest evicted first — the -controller answers its eviction with a `pair-result` denial and drops the +and only a `client-gone` removes one. All three cap what a *pairing* can +retain (`MAX_PENDING_TICKETS`, `MAX_PENDING_PAIRINGS`), oldest evicted first — +the controller answers its eviction with a `pair-result` denial and drops the client record rather than leaving someone on a modal that no longer exists, while the ceremony and the service's mirrored queue simply delete theirs — because anything that can sign in can send these and a queue that only grows wedges the process that owns every PTY. +The client map is bounded against *that* path and not in general: `connect` +also creates an entry (through `#resetAuthorization`), those carry no pending +request, and the pairing counter neither sees nor evicts them — evicting an +entry that may be `established` is a different act from denying a pending +request. What keeps those cheap is `MAX_CLIENT_ID_LENGTH`, which bounds the +frame's `clientId` before any map is touched: every other field of a `pair` +frame is capped by `PAIRING_FIELD_LIMIT`, so leaving the key free would bound +only the half that was already bounded. They are cleared wholesale when the +relay socket drops. + Source of truth: `PairingRequest` / `PairingTicket` / `PairingCeremony` / -`PAIRING_PRESENCE_WINDOW_MS` / `MAX_PENDING_PAIRINGS` in -`server-lib-common/src/security/pairing.ts` +`PAIRING_PRESENCE_WINDOW_MS` / `PAIRING_FIELD_LIMIT` / `MAX_PENDING_PAIRINGS` +in `server-lib-common/src/security/pairing.ts`, and `MAX_CLIENT_ID_LENGTH` / +`RemoteHost.#evictOldestPairingIfFull` in +`lib/src/remote/host/remote-host.ts` (tickets are single-use with a `DEFAULT_PAIRING_TTL_MS` = 5-minute TTL; approval after expiry fails without touching the ACL — the presence window gates the *request*, not the approver's deliberation). The wire sequence — diff --git a/lib/src/remote/host/remote-host.test.ts b/lib/src/remote/host/remote-host.test.ts index 007e6932..b3b5c275 100644 --- a/lib/src/remote/host/remote-host.test.ts +++ b/lib/src/remote/host/remote-host.test.ts @@ -269,9 +269,11 @@ describe('RemoteHost frame handling', () => { const denials = socket.frames('pair-result').filter((f) => f.approved === false); expect(denials).toHaveLength(sent - MAX_PENDING_PAIRINGS); expect(denials.every((f) => f.error === 'superseded')).toBe(true); - // The map itself is bounded, not just the payload it holds: evicting only + // The record is dropped, not just the payload it holds: evicting only // `pending` would free the capped request and keep the slot plus its - // relay-chosen key forever, which is the unbounded half. + // relay-chosen key forever, which is the unbounded half. This bounds the + // pairing path — `connect` frames allocate through a different route that + // this counter deliberately does not evict. expect(host.trackedClientCount).toBeLessThanOrEqual(MAX_PENDING_PAIRINGS); // Nothing reached the ACL without a human. diff --git a/lib/src/remote/host/remote-host.ts b/lib/src/remote/host/remote-host.ts index 4003e864..1a2f96de 100644 --- a/lib/src/remote/host/remote-host.ts +++ b/lib/src/remote/host/remote-host.ts @@ -295,11 +295,6 @@ export class RemoteHost { this.#clients.clear(); } - /** - * Drop the oldest pending pairing when the queue is full, so a new request - * displaces one rather than growing the map. Oldest first: whoever initiated - * it is the least likely to still be waiting on the modal. - */ /** * How many clients this Host is tracking. Exists for the pending-pairing * bound's test: the growth it guards against is in a private map, and a @@ -310,6 +305,19 @@ export class RemoteHost { return this.#clients.size; } + /** + * Drop the oldest pending pairing when the queue is full, so a new request + * displaces one rather than growing the map. Oldest first: whoever initiated + * it is the least likely to still be waiting on the modal. + * + * Bounds the *pairing* path specifically. `#onConnect` also creates a + * `#clients` entry through `#resetAuthorization`, and those carry no + * `pending`, so this counter does not see them and does not evict them — + * deliberately, since evicting an entry that may be `established` is a + * different act from denying a pending request. Those entries are cheap (a + * length-bounded key and two fields, no `PairingRequest`) and are cleared + * wholesale when the socket drops. + */ #evictOldestPairingIfFull(): void { let pendingCount = 0; for (const state of this.#clients.values()) if (state.pending) pendingCount++; From 47d01c98c6d92c5b72b8f0131d52572136794f5d Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 21:44:54 -0700 Subject: [PATCH 16/19] security(audit): let a dissent outrank a missing fragment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One finding from the third review round, and it is an interaction rather than a line: the dissent loop this PR added was gated on `[ "$STATUS" = "PASS" ]`, and the pre-existing missing-fragment loop twenty lines above writes that same variable. So a single empty fragment switched the dissent check off for every *other* fragment. A domain that returned `VERDICT: FAIL` beside a missing sibling was never noticed, and the issue then pointed the maintainer at "the rest" precisely when a domain in the rest had failed. The gate is now "not already FAIL" rather than "still PASS". The three outcomes are ordered — FAIL (a domain found something) outranks MISSING (the audit did not finish) outranks PASS — so a dissent can raise MISSING to FAIL and never the reverse. Simulated all five combinations: clean, empty-sibling-plus-FAIL (the bug, now FAIL with both sets populated), empty-sibling-only, unreadable-plus-FAIL, and an orchestrator that already said FAIL. That combination is newly reachable, so the FAIL issue body now names any missing or unreadable fragments alongside the report. Without it the report reads as the whole story while a domain in it went unaudited — which is the same failure the missing-fragment guard exists to prevent, one branch over. Also closes the stale enumeration the review flagged twice: `workflow-audit.yaml`'s issue body listed five paths where `WINDOW` has four, left behind when `SECURITY.md` came out of the window. It now interpolates `${WINDOW[*]}` rather than restating it, so the two cannot disagree again. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/security-audit.yaml | 28 ++++++++++++++++++++++++++- .github/workflows/workflow-audit.yaml | 2 +- SECURITY.md | 1 + 3 files changed, 29 insertions(+), 2 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index cfc9d502..33f129a3 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -331,7 +331,17 @@ jobs: # otherwise the verdict rests on the orchestrator faithfully # propagating it, which is the same "a prompt is not a control" # shape the missing-fragment guard exists to close. - if [ "$STATUS" = "PASS" ]; then + # + # Gated on "not already FAIL", NOT on "still PASS". The loop above + # writes the same variable, so a `PASS` gate meant one empty fragment + # switched this check off for every *other* fragment — and a domain + # that returned VERDICT: FAIL beside a missing sibling would go + # unnoticed, while the issue told the maintainer the rest was fine. + # The three outcomes are ordered: FAIL (a domain found something) + # outranks MISSING (the audit did not finish), which outranks PASS, + # so a dissent must be able to raise MISSING to FAIL and never the + # reverse. + if [ "$STATUS" != "FAIL" ]; then # shellcheck disable=SC2086 for f in $AUDIT_FRAGMENTS; do [ -s "$f" ] || continue @@ -353,6 +363,9 @@ jobs: *) echo "::warning::$f has no VERDICT line; the dissent check cannot read it." UNREADABLE_VERDICTS="${UNREADABLE_VERDICTS:+$UNREADABLE_VERDICTS, }$f" + # Never downgrade: a FAIL from an earlier fragment in this + # same loop, or a MISSING from the missing-fragment loop + # above, both outrank "unreadable". [ "$STATUS" = FAIL ] || STATUS=MISSING ;; esac done @@ -497,6 +510,19 @@ jobs: [ -n "$TRANSCRIPT_URL" ] && LINKS="$LINKS · [Transcript]($TRANSCRIPT_URL)" echo "Audit failed at $DATE. $LINKS" echo + # A FAIL can now arrive with fragments missing or unreadable + # alongside it: the dissent loop is gated on "not already FAIL" + # rather than "still PASS", so a domain that returned FAIL beside + # a broken sibling reaches this branch instead of being masked as + # MISSING. Say so, or the report below reads as the whole story + # while a domain in it was never audited. + if [ -n "${MISSING_FRAGMENTS:-}" ] || [ -n "${UNREADABLE_VERDICTS:-}" ]; then + [ -n "${MISSING_FRAGMENTS:-}" ] && + echo "> **Incomplete besides.** No report fragment was left for: $MISSING_FRAGMENTS — those domains are unaudited, not passing." + [ -n "${UNREADABLE_VERDICTS:-}" ] && + echo "> **Unreadable verdict.** The first line of $UNREADABLE_VERDICTS is neither \`VERDICT: PASS\` nor \`VERDICT: FAIL\`, so that domain's own conclusion could not be confirmed." + echo + fi cat audit-report.md } > audit-comment.md fi diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index dd017555..fe219161 100644 --- a/.github/workflows/workflow-audit.yaml +++ b/.github/workflows/workflow-audit.yaml @@ -363,7 +363,7 @@ jobs: BODY=$(mktemp) { - echo "$COUNT unexplained commit(s) in the audit window (\`.github/workflows/\`, \`.config/tend.yaml\`, \`.github/audit/\`, \`.vscode/\`, \`SECURITY.md\`) since \`$SINCE\`." + echo "$COUNT unexplained commit(s) in the audit window (\`${WINDOW[*]}\`) since \`$SINCE\`." echo "" echo "Routine Renovate pin bumps and reproducible tend regenerations are" echo "classified and omitted — see the run summary for what was skipped." diff --git a/SECURITY.md b/SECURITY.md index d2d60176..ab235d60 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -294,6 +294,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - `application-security` — **everything else**, worked out from `ls -A` rather than from a list, including `.impeccable/` (the design-token snapshot behind `DESIGN.md`). Dotfile directories are named explicitly wherever they land, in this list and in the prompt files, because a catch-all has twice now been read as covering them when no reader could tell which domain owned one. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. +- FAIL IF the dissent check is gated on the status still being `PASS` rather than on it not already being `FAIL`. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. - FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one — **or** stops treating a fragment with no readable verdict as inconclusive. Three cases, not two: a fragment the check cannot read must not fall through to an unchallenged `PASS`, because that puts the verdict back on a prompt having been followed, which is the thing this guard exists to stop being the control. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. From 60ebf67f79f02292e8931f956566f562f66904bf Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 22:00:53 -0700 Subject: [PATCH 17/19] security(audit): report conditions, not combinations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four review rounds in a row found the same defect wearing different clothes: an issue-body arm whose prose was true of the only state that could select it, and became false when the next gate widened. Round 2 found it in the verdictless arm, round 3 in the dissent gate, round 4 in two more states that round 3's fix made reachable. Each fix was correct and each created the next finding. That is a structural problem, not four coincidences. The reporting step wrote prose per *combination* of `(STATUS, MISSING_FRAGMENTS, UNREADABLE_VERDICTS, DISSENTING)`, so widening any gate made new combinations reachable and every one needed its text re-audited. Patching combinations cannot converge — the combinations outnumber the patches. So it emits one note per condition that *holds*, and no note claims anything about the others. A fifth condition later means adding one note; it cannot make an existing note wrong. This is the same fix-by-derivation that ended the drift twice already in this PR: `WINDOW_NON_WORKFLOW` derived from `WINDOW`, and the audit header interpolating `${WINDOW[*]}` instead of restating it. Both of round 4's findings fall out rather than being patched: - `MISSING` alongside an unreadable verdict now reports both, because both conditions hold and each gets its line. - The dissent path entered with no merged verdict now says so: `STATUS_FILE_VERDICT` records what the file itself said, separately from the `STATUS` the guards mutate, since a dissent can raise the same MISSING to FAIL and the original cause is otherwise unrecoverable. The second half of that finding needed a real fix: the missing-fragment loop was still gated on `PASS`, so a run that lost a fragment *and* never wrote a verdict recorded nothing about the fragment. Both guards are now gated on "not already FAIL" — they record conditions the issue reports on, so they must run on any path that reaches it. Verified by extracting the real step and running nine combinations, including every one the previous four rounds patched individually. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/security-audit.yaml | 183 +++++++++++--------------- SECURITY.md | 3 +- 2 files changed, 76 insertions(+), 110 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 33f129a3..3c3d005e 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -291,6 +291,14 @@ jobs: *) STATUS=MISSING ;; esac + # What the *file* said, kept separate from `STATUS`, which the guards + # below mutate. Every note in the issue body is written about a + # condition rather than about a branch, and "the audit never wrote a + # verdict" is one of those conditions — it cannot be recovered from + # `STATUS` afterwards, because a dissent can raise that same MISSING + # to FAIL. + if [ "$STATUS" = "MISSING" ]; then STATUS_FILE_VERDICT=none; else STATUS_FILE_VERDICT="$STATUS"; fi + # A PASS is only as good as the evidence behind it. Each domain # writes its own fragment before returning a verdict, so a # missing or empty fragment means that domain never reported — @@ -305,12 +313,19 @@ jobs: # reason the three outcomes exist above — this is an audit that # did not finish, not a security finding — and MISSING already # exits non-zero and holds the release gate shut. - if [ "$STATUS" = "PASS" ]; then + # Gated the same way as the dissent loop below, and for the same + # reason: this records a *condition* the issue body reports on, so it + # has to run on any path that can reach that body. Gated on `PASS` it + # was skipped whenever the audit wrote no verdict at all — so a run + # that lost a fragment *and* never decided filed with no sign the + # fragment was missing. Only a settled FAIL skips it, and only + # because FAIL is already the ceiling. + if [ "$STATUS" != "FAIL" ]; then # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. # shellcheck disable=SC2086 for f in $AUDIT_FRAGMENTS; do if [ ! -s "$f" ]; then - echo "::warning::Audit reported PASS but $f is missing or empty; downgrading to INCONCLUSIVE." + echo "::warning::$f is missing or empty; the audit cannot be considered complete." MISSING_FRAGMENTS="${MISSING_FRAGMENTS:+$MISSING_FRAGMENTS, }$f" STATUS=MISSING fi @@ -356,7 +371,7 @@ jobs: # says no fragment was left at all. case "$(head -n1 "$f")" in 'VERDICT: FAIL'*) - echo "::error::Audit reported PASS but $f opens with VERDICT: FAIL; downgrading to FAIL." + echo "::error::$f opens with VERDICT: FAIL; the merged verdict cannot stand." DISSENTING="${DISSENTING:+$DISSENTING, }$f" STATUS=FAIL ;; 'VERDICT: PASS'*) ;; @@ -417,115 +432,65 @@ jobs: TRANSCRIPT_URL="" fi - if [ "$STATUS" = "MISSING" ]; then - TITLE="[security-audit] INCONCLUSIVE on $(date -u +%Y-%m-%d)" - { - # Two different causes reach MISSING, and they want different - # first moves from the reader: no status file at all means - # start at the report's own markers — the prose below names - # both, because that branch itself has two causes (a - # turn-ending failure mid-audit, and an expired wait deadline) - # which leave different markers. A downgraded PASS means the - # named domains are the whole story — the rest reported, and - # the "ran short, no verdict" prose below would be wrong - # about a run that did reach one. The named set can be one, - # two, or all three: the fail-closed `rm -f` in `Redact - # secrets from agent output` deletes every fragment *and* - # `audit-report.md` while leaving `audit-status.txt` alone, - # so a PASS reaches this branch with all three named, no - # report body, and no transcript link. That path is a - # cleartext-secret incident, not a flaky domain, so the - # all-three case says so rather than leaving the reader to - # infer it from an empty issue. - if [ -n "${MISSING_FRAGMENTS:-}" ]; then - echo "Audit verdict withheld at $DATE. [Run]($RUN_URL)" - echo - echo "The audit reported \`PASS\`, but it was downgraded: no report" - echo "fragment was left for: $MISSING_FRAGMENTS. A domain that leaves" - echo "no fragment did not pass — it did not finish, and a missing" - echo "fragment is indistinguishable in the merged report from a domain" - echo "that found nothing. The named domains are unaudited; any report" - echo "reproduced below covers only the rest. If all three are named," - echo "suspect \`Redact secrets from agent output\` — it deletes every" - echo "fragment and the report when it throws, and that means a secret" - echo "reached a file in cleartext." - elif [ -n "${UNREADABLE_VERDICTS:-}" ]; then - # A third cause, and it must not borrow either arm's prose: the - # status file WAS written and said `PASS`, and every domain DID - # report — so "exited without writing audit-status.txt" and the - # `UNVERIFIABLE` / `_No report …_` markers below are both wrong - # here. What actually happened is that a fragment's first line - # was not a verdict, so the dissent check could not read it and - # the merged PASS could not be trusted. - echo "Audit verdict withheld at $DATE. [Run]($RUN_URL)" - echo - echo "The audit reported \`PASS\`, but it was downgraded: the first" - echo "line of $UNREADABLE_VERDICTS is neither \`VERDICT: PASS\` nor" - echo "\`VERDICT: FAIL\`, so the reporting step could not confirm that" - echo "domain agreed with the merged verdict. Every domain did report," - echo "and the report below is complete — read the named domain's" - echo "section for its own conclusion. This is a defect in the audit's" - echo "output format, not necessarily a security finding: the fix is" - echo "usually that the subagent did not follow" - echo "\`.github/audit/_preamble.md\`'s rule that line 1 is the verdict." - else - echo "Audit produced no verdict at $DATE. [Run]($RUN_URL)" - echo - echo "The audit step exited without writing \`audit-status.txt\`." - if [ -s audit-report.md ]; then - echo "It did leave a partial \`audit-report.md\`, reproduced below," - echo "which may itself contain a real finding — read it first. A" - echo "report without a verdict is the expected shape of an audit that" - echo "ran short: the prompt asks for the report either way, and for" - echo "the status file only once the verdict covers every check. Start" - echo "at the markers rather than the transcript: \`UNVERIFIABLE\` is a" - echo "check the agent itself could not determine, while a domain" - echo "section reading \`_No report …_\` is a domain that never reported" - echo "at all — which is what an expired wait deadline looks like." - else - echo "**This is not a security finding** — the audit did not reach" - echo "one. Treat it as a failed audit run, not a failed audit." - fi - fi - if [ -n "$TRANSCRIPT_URL" ]; then - echo - echo "[Download the transcript]($TRANSCRIPT_URL) to see where it stopped." - fi - if [ -s audit-report.md ]; then - echo - echo "---" - echo - cat audit-report.md - fi - } > audit-comment.md - else + # One note per condition that HOLDS, not one block per combination + # of conditions. + # + # This used to be an if/elif chain with hand-written prose per arm, + # and every arm was true only of the combination that could select + # it. Each time a gate widened, new combinations became reachable and + # the arm that caught them described a different failure — four review + # rounds in a row found exactly that, each one created by the previous + # round's fix. Prose proportional to combinations cannot be kept + # correct by patching combinations. + # + # So: collect the conditions, emit a line for each. Adding a fourth + # condition later means adding one note, and it cannot make any + # existing note wrong, because no note claims anything about the + # others. + NOTES=$(mktemp) + if [ -n "${DISSENTING:-}" ]; then + echo "- **A domain returned \`FAIL\`.** $DISSENTING opened with \`VERDICT: FAIL\` — read that domain's section first. A domain's own verdict outranks the merged one." >> "$NOTES" + fi + if [ -n "${MISSING_FRAGMENTS:-}" ]; then + echo "- **A domain left no report.** Nothing was written for: $MISSING_FRAGMENTS. Those domains are unaudited, not passing — a missing fragment is indistinguishable in the merged report from a domain that found nothing. If all three are named, suspect \`Redact secrets from agent output\`: it deletes every fragment and the report when it throws, which means a secret reached a file in cleartext." >> "$NOTES" + fi + if [ -n "${UNREADABLE_VERDICTS:-}" ]; then + echo "- **A domain's verdict could not be read.** The first line of $UNREADABLE_VERDICTS is neither \`VERDICT: PASS\` nor \`VERDICT: FAIL\`, so its agreement with the merged verdict is unconfirmed. Usually a subagent that did not follow \`.github/audit/_preamble.md\`; the domain did report, so its section below still stands on its own." >> "$NOTES" + fi + if [ "$STATUS_FILE_VERDICT" = "none" ]; then + echo "- **The audit wrote no verdict.** \`audit-status.txt\` was absent, empty, or not \`PASS\`/\`FAIL\`, so the run ended without deciding. Start at the report's own markers: \`UNVERIFIABLE\` is a check the agent could not determine, while a domain section reading \`_No report …_\` is one that never reported — which is what an expired wait deadline looks like." >> "$NOTES" + fi + + if [ "$STATUS" = "FAIL" ]; then TITLE="[security-audit] FAIL on $(date -u +%Y-%m-%d)" - if [ ! -s audit-report.md ]; then - printf '%s\n' \ - "Audit reported FAIL but wrote no \`audit-report.md\`." \ - > audit-report.md - fi - { - LINKS="[Run]($RUN_URL)" - [ -n "$TRANSCRIPT_URL" ] && LINKS="$LINKS · [Transcript]($TRANSCRIPT_URL)" - echo "Audit failed at $DATE. $LINKS" + HEADLINE="Audit failed at $DATE." + else + TITLE="[security-audit] INCONCLUSIVE on $(date -u +%Y-%m-%d)" + HEADLINE="Audit reached no usable verdict at $DATE." + fi + + { + LINKS="[Run]($RUN_URL)" + [ -n "$TRANSCRIPT_URL" ] && LINKS="$LINKS · [Transcript]($TRANSCRIPT_URL)" + echo "$HEADLINE $LINKS" + echo + if [ -s "$NOTES" ]; then + cat "$NOTES" echo - # A FAIL can now arrive with fragments missing or unreadable - # alongside it: the dissent loop is gated on "not already FAIL" - # rather than "still PASS", so a domain that returned FAIL beside - # a broken sibling reaches this branch instead of being masked as - # MISSING. Say so, or the report below reads as the whole story - # while a domain in it was never audited. - if [ -n "${MISSING_FRAGMENTS:-}" ] || [ -n "${UNREADABLE_VERDICTS:-}" ]; then - [ -n "${MISSING_FRAGMENTS:-}" ] && - echo "> **Incomplete besides.** No report fragment was left for: $MISSING_FRAGMENTS — those domains are unaudited, not passing." - [ -n "${UNREADABLE_VERDICTS:-}" ] && - echo "> **Unreadable verdict.** The first line of $UNREADABLE_VERDICTS is neither \`VERDICT: PASS\` nor \`VERDICT: FAIL\`, so that domain's own conclusion could not be confirmed." - echo - fi + elif [ "$STATUS" = "MISSING" ]; then + # Belt and braces: MISSING with no condition set should be + # unreachable, since every path that sets it also records why. + # Say so rather than filing a blank issue. + echo "- **Inconclusive for a reason this step could not name.** That is itself a bug in the reporting step." + echo + fi + if [ -s audit-report.md ]; then cat audit-report.md - } > audit-comment.md - fi + else + echo "_No \`audit-report.md\` was produced._" + fi + } > audit-comment.md + rm -f "$NOTES" EXISTING=$(gh issue list --label security-audit-failure \ --state open --json number --jq '.[0].number' || true) diff --git a/SECURITY.md b/SECURITY.md index ab235d60..b05d2e68 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -294,7 +294,8 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - `application-security` — **everything else**, worked out from `ls -A` rather than from a list, including `.impeccable/` (the design-token snapshot behind `DESIGN.md`). Dotfile directories are named explicitly wherever they land, in this list and in the prompt files, because a catch-all has twice now been read as covering them when no reader could tell which domain owned one. An enumeration here goes stale the moment a path is added, which is exactly how `.vscode/` and `.impeccable/` came to be owned by nobody after the first version of this split named paths explicitly. The subtraction is **recursive**: where another domain claims a subdirectory rather than a whole tree — as both do inside `website/` — the remainder of that tree belongs here, or the same orphaning recurs one level down. - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. -- FAIL IF the dissent check is gated on the status still being `PASS` rather than on it not already being `FAIL`. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. +- FAIL IF the reporting step writes issue prose per *combination* of conditions rather than one note per condition that holds. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. Prose proportional to combinations cannot be kept correct by fixing combinations; a note that claims nothing about the other conditions cannot be invalidated by a new one. +- FAIL IF either fragment guard is gated on the status still being `PASS` rather than on it not already being `FAIL`. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. - FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one — **or** stops treating a fragment with no readable verdict as inconclusive. Three cases, not two: a fragment the check cannot read must not fall through to an unchallenged `PASS`, because that puts the verdict back on a prompt having been followed, which is the thing this guard exists to stop being the control. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. From 4353ec75e45d0245f152b0ac6723d6ce326d784d Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 22:11:58 -0700 Subject: [PATCH 18/19] security(audit): separate recording a condition from deciding the verdict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 5 found the last two instances, and both are this restructure not having gone far enough rather than the class returning. **Both fragment guards were still gated on the status.** Which meant that when the orchestrator itself wrote `FAIL`, neither loop ran — so a domain that left no report beside a real finding produced no annotation, no note, and nothing in the issue, while `SECURITY.md` requires exactly that case to say so. Same defect in a third costume: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, a real `FAIL` silenced both. The root is that the loops were doing two jobs. Recording what is true of a run and deciding its verdict are separate concerns, and gating the first on the second is what kept producing findings. Both loops now run unconditionally and only record; escalation happens once, afterwards, from what they recorded. The `FAIL IF` says so, rather than naming the particular gate that was wrong this time. **The last per-combination prose is gone.** The addendum appended to `audit-report.md` still opened "Downgraded to FAIL — the merged verdict was `PASS`", which is a claim about a combination (dissent set AND the merge said `PASS`) and became false as soon as a dissent could arrive on a run that said nothing. It sat one `cat` below a note that contradicted it. The notes block already carries the same fact in condition form, so the addendum is deleted rather than reworded — which is what the `FAIL IF` added last commit actually requires. Verified by running the extracted step across eight combinations, including the one round 5 reported and every one earlier rounds fixed individually. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- .github/workflows/security-audit.yaml | 125 ++++++++++++-------------- SECURITY.md | 2 +- 2 files changed, 59 insertions(+), 68 deletions(-) diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 3c3d005e..f3fe037d 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -313,24 +313,24 @@ jobs: # reason the three outcomes exist above — this is an audit that # did not finish, not a security finding — and MISSING already # exits non-zero and holds the release gate shut. - # Gated the same way as the dissent loop below, and for the same - # reason: this records a *condition* the issue body reports on, so it - # has to run on any path that can reach that body. Gated on `PASS` it - # was skipped whenever the audit wrote no verdict at all — so a run - # that lost a fragment *and* never decided filed with no sign the - # fragment was missing. Only a settled FAIL skips it, and only - # because FAIL is already the ceiling. - if [ "$STATUS" != "FAIL" ]; then - # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. - # shellcheck disable=SC2086 - for f in $AUDIT_FRAGMENTS; do - if [ ! -s "$f" ]; then - echo "::warning::$f is missing or empty; the audit cannot be considered complete." - MISSING_FRAGMENTS="${MISSING_FRAGMENTS:+$MISSING_FRAGMENTS, }$f" - STATUS=MISSING - fi - done - fi + # Recording a condition and escalating the status are two jobs, and + # gating the first on the second is what kept producing findings. + # Both loops now run UNCONDITIONALLY — they answer "what is true of + # this run", which the issue body reports on regardless of verdict. + # Only the escalation below is conditional. Gated on `!= FAIL` this + # loop was skipped whenever the orchestrator itself wrote FAIL, so a + # domain that left no fragment beside a real finding appeared + # nowhere: no annotation, no note, and a report that read as the + # whole story. + # + # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. + # shellcheck disable=SC2086 + for f in $AUDIT_FRAGMENTS; do + if [ ! -s "$f" ]; then + echo "::warning::$f is missing or empty; the audit cannot be considered complete." + MISSING_FRAGMENTS="${MISSING_FRAGMENTS:+$MISSING_FRAGMENTS, }$f" + fi + done # Existence is not agreement. The loop above catches a domain that # produced nothing; this catches one that produced a FAIL the merge @@ -347,55 +347,46 @@ jobs: # propagating it, which is the same "a prompt is not a control" # shape the missing-fragment guard exists to close. # - # Gated on "not already FAIL", NOT on "still PASS". The loop above - # writes the same variable, so a `PASS` gate meant one empty fragment - # switched this check off for every *other* fragment — and a domain - # that returned VERDICT: FAIL beside a missing sibling would go - # unnoticed, while the issue told the maintainer the rest was fine. - # The three outcomes are ordered: FAIL (a domain found something) - # outranks MISSING (the audit did not finish), which outranks PASS, - # so a dissent must be able to raise MISSING to FAIL and never the - # reverse. - if [ "$STATUS" != "FAIL" ]; then - # shellcheck disable=SC2086 - for f in $AUDIT_FRAGMENTS; do - [ -s "$f" ] || continue - # Three cases, not two. A fragment whose first line is neither - # verdict — a subagent that ignored the preamble, a stray heading - # or blank line — must not fall through to an unchallenged PASS: - # that would put the verdict back on a prompt having been - # followed, which is the thing this guard exists to stop being - # the control. MISSING rather than FAIL, since a report we cannot - # read is an audit that did not finish rather than a finding, and - # deliberately not folded into MISSING_FRAGMENTS, whose message - # says no fragment was left at all. - case "$(head -n1 "$f")" in - 'VERDICT: FAIL'*) - echo "::error::$f opens with VERDICT: FAIL; the merged verdict cannot stand." - DISSENTING="${DISSENTING:+$DISSENTING, }$f" - STATUS=FAIL ;; - 'VERDICT: PASS'*) ;; - *) - echo "::warning::$f has no VERDICT line; the dissent check cannot read it." - UNREADABLE_VERDICTS="${UNREADABLE_VERDICTS:+$UNREADABLE_VERDICTS, }$f" - # Never downgrade: a FAIL from an earlier fragment in this - # same loop, or a MISSING from the missing-fragment loop - # above, both outrank "unreadable". - [ "$STATUS" = FAIL ] || STATUS=MISSING ;; - esac - done - if [ -n "${DISSENTING:-}" ]; then - { - echo - echo "---" - echo - echo "**Downgraded to FAIL by the reporting step.** The merged verdict was" - echo "\`PASS\`, but these domain reports open with \`VERDICT: FAIL\`:" - echo "$DISSENTING." - echo - echo "A domain's own verdict outranks the merge." - } >> audit-report.md - fi + # Also unconditional, for the same reason. + # shellcheck disable=SC2086 + for f in $AUDIT_FRAGMENTS; do + [ -s "$f" ] || continue + # Three cases, not two. A fragment whose first line is neither + # verdict — a subagent that ignored the preamble, a stray heading + # or blank line — must not fall through to an unchallenged PASS: + # that would put the verdict back on a prompt having been + # followed, which is the thing this guard exists to stop being + # the control. MISSING rather than FAIL, since a report we cannot + # read is an audit that did not finish rather than a finding, and + # deliberately not folded into MISSING_FRAGMENTS, whose message + # says no fragment was left at all. + case "$(head -n1 "$f")" in + 'VERDICT: FAIL'*) + echo "::error::$f opens with VERDICT: FAIL; the merged verdict cannot stand." + DISSENTING="${DISSENTING:+$DISSENTING, }$f" ;; + 'VERDICT: PASS'*) ;; + *) + echo "::warning::$f has no VERDICT line; the dissent check cannot read it." + UNREADABLE_VERDICTS="${UNREADABLE_VERDICTS:+$UNREADABLE_VERDICTS, }$f" ;; + esac + done + # Escalation, in ONE place, from what the loops recorded. Ordered: + # FAIL (a domain found something) outranks MISSING (the audit did + # not finish) outranks PASS. A dissent can raise MISSING to FAIL and + # never the reverse, and an orchestrator that already said FAIL stays + # there. + # + # There is deliberately no prose emitted here. The old code appended + # a "Downgraded to FAIL — the merged verdict was PASS" block to the + # report, which was a claim about a *combination* (dissent set AND + # the merge said PASS) and became false the moment a dissent could + # arrive on a run that said nothing. The notes block below carries + # the same fact in condition form. + if [ -n "${DISSENTING:-}" ]; then + STATUS=FAIL + elif [ "$STATUS" != "FAIL" ] && + { [ -n "${MISSING_FRAGMENTS:-}" ] || [ -n "${UNREADABLE_VERDICTS:-}" ]; }; then + STATUS=MISSING fi DATE=$(date -u +%Y-%m-%dT%H:%MZ) diff --git a/SECURITY.md b/SECURITY.md index b05d2e68..01c44596 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -295,7 +295,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. - FAIL IF the reporting step writes issue prose per *combination* of conditions rather than one note per condition that holds. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. Prose proportional to combinations cannot be kept correct by fixing combinations; a note that claims nothing about the other conditions cannot be invalidated by a new one. -- FAIL IF either fragment guard is gated on the status still being `PASS` rather than on it not already being `FAIL`. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. +- FAIL IF either fragment guard is gated on the status at all. Recording what is true of a run and deciding its verdict are separate jobs, and gating the first on the second is what produced this defect three times in different clothes: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, an orchestrator that wrote `FAIL` itself silenced both, so a domain that left no report beside a real finding appeared nowhere at all. Both loops run unconditionally and only record; escalation happens once, afterwards, from what they recorded. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. - FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one — **or** stops treating a fragment with no readable verdict as inconclusive. Three cases, not two: a fragment the check cannot read must not fall through to an unchallenged `PASS`, because that puts the verdict back on a prompt having been followed, which is the thing this guard exists to stop being the control. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above. From e040281342f3cb34559a20ba0548b740933e1ab5 Mon Sep 17 00:00:00 2001 From: Ned Twigg Date: Wed, 26 Aug 2026 22:20:43 -0700 Subject: [PATCH 19/19] security(audit): drop a FAIL IF sentence describing code that no longer exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bullet claiming "both loops run unconditionally and only record" still carried, two sentences later, "the missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one" — present tense, contradicting the sentence before it, and describing code this branch deleted. `STATUS` is assigned in exactly two places now: where the status file is parsed, and in the single escalation block. This is my own careless edit, and of the exact class this PR exists to end. I replaced the bullet's first sentence and left the old tail appended, without re-reading the whole bullet — the document equivalent of an if-arm whose prose was true of the shape it was written against. It matters more here than in a comment: SECURITY.md is what the audit reads as the control, so an auditor checking this bullet goes looking for a loop that writes `STATUS` and finds a rule whose rationale does not match the code. A rule justified by absent code is worse than no rule, because it reads as verified. Rewrote the bullet whole rather than excising the sentence, and then checked the rest of the section against the workflow mechanically — the prompt pointer, the fragment list, the Opus override, the redaction step and its fail-closed `rm`, and `release.yml`'s `gh run watch --exit-status` all still match what the document claims. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FxF3HqmmoddTbmof7fTxWu --- SECURITY.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/SECURITY.md b/SECURITY.md index 01c44596..4485747e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -295,7 +295,7 @@ gh secret set AUDIT_PAT --env security-audit --repo diffplug/dormouse --body 'gi - FAIL IF the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the three per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws. It is the only thing between an accidental `printenv` and a world-readable artifact, and until this bullet existed nothing would have tripped on its deletion. - FAIL IF `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `--agents` and `scripts/security-audit-local.sh`. A local run that silently uses a weaker model than the nightly makes the local loop — the one that catches problems before they merge — worse than the thing it is standing in for. - FAIL IF the reporting step writes issue prose per *combination* of conditions rather than one note per condition that holds. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. Prose proportional to combinations cannot be kept correct by fixing combinations; a note that claims nothing about the other conditions cannot be invalidated by a new one. -- FAIL IF either fragment guard is gated on the status at all. Recording what is true of a run and deciding its verdict are separate jobs, and gating the first on the second is what produced this defect three times in different clothes: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, an orchestrator that wrote `FAIL` itself silenced both, so a domain that left no report beside a real finding appeared nowhere at all. Both loops run unconditionally and only record; escalation happens once, afterwards, from what they recorded. The missing-fragment loop writes the same variable, so a `PASS` gate lets one empty fragment switch the dissent check off for every other one — and a domain that returned `FAIL` beside a broken sibling is then never noticed, while the issue tells the reader the rest is fine. The three outcomes are ordered: `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS`, so a dissent must be able to raise `MISSING` to `FAIL` and never the reverse. A `FAIL` arriving alongside missing or unreadable fragments must say so in the issue, or the report reads as the whole story. +- FAIL IF either fragment guard is gated on the status at all. Recording what is true of a run and deciding its verdict are separate jobs, and gating the first on the second produced this defect three times in different clothes: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, an orchestrator that wrote `FAIL` itself silenced both, so a domain that left no report beside a real finding appeared nowhere at all. Both loops run unconditionally and only record; `STATUS` is assigned in exactly two places, where the status file is parsed and in the single escalation block. That block encodes the ordering — `FAIL` (a domain found something) outranks `MISSING` (the audit did not finish) outranks `PASS` — so a dissent can raise `MISSING` to `FAIL` and never the reverse, and a `FAIL` arriving alongside missing or unreadable fragments still reports them. - FAIL IF a fragment's first line is not `VERDICT: PASS` or `VERDICT: FAIL`, or the reporting step stops downgrading a merged `PASS` that contradicts one — **or** stops treating a fragment with no readable verdict as inconclusive. Three cases, not two: a fragment the check cannot read must not fall through to an unchallenged `PASS`, because that puts the verdict back on a prompt having been followed, which is the thing this guard exists to stop being the control. Existence is not agreement: the missing-fragment guard catches a domain that produced nothing, and this catches one whose `FAIL` the merge lost — which is worse, because `PASS` closes the open failure issue and opens the release gate. - FAIL IF the orchestrator can report `PASS` while a subagent left no report fragment. A domain that dies silently must not pass the audit — a missing fragment is indistinguishable from a domain that found nothing, and only one of those is safe to publish a release on. It must not be published as `FAIL` either, unless some domain actually returned one: the prompt writes no status file when a fragment is missing and no domain failed, which routes an audit that ran out of time to the INCONCLUSIVE issue rather than filing it as a security finding and relabelling an open issue up to `FAIL`. Both outcomes exit non-zero and hold the release gate shut, so the distinction costs nothing and is the whole reason there are three of them. - FAIL IF the audit has been weakened in any other way — e.g. the prompt no longer requires the qualitative pass, a `FAIL IF` can be ignored, the failure-reporting step that opens a `security-audit-failure` issue and exits non-zero has been removed, or the `AUDIT_PAT` pre-check is removed or bypassed. This bullet is a judgement item, not a checklist: the examples are the ones that have come up, not the ones that exist. Two weakenings found by the audit's own first run were not covered by any example here, and both became their own bullets above.