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/application-security.md b/.github/audit/application-security.md index 530baca0..0b928eef 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -40,10 +40,28 @@ 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 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 — 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/`, +`.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/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/audit/supply-chain.md b/.github/audit/supply-chain.md index 4f95d297..6ed9de67 100644 --- a/.github/audit/supply-chain.md +++ b/.github/audit/supply-chain.md @@ -25,7 +25,13 @@ 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, 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/.github/renovate.json b/.github/renovate.json index 03c1b079..6a30d24b 100644 --- a/.github/renovate.json +++ b/.github/renovate.json @@ -4,6 +4,18 @@ "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.", + "`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": [ { "matchManagers": ["npm", "cargo"], diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 122751ce..f3fe037d 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 @@ -276,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 — @@ -290,16 +313,80 @@ 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 - # 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." - MISSING_FRAGMENTS="${MISSING_FRAGMENTS:+$MISSING_FRAGMENTS, }$f" - STATUS=MISSING - fi - done + # 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 + # 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. + # + # 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) @@ -336,83 +423,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." - 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 + 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/.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 }} diff --git a/.github/workflows/workflow-audit.yaml b/.github/workflows/workflow-audit.yaml index 89c02516..fe219161 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/, 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. # @@ -95,6 +95,22 @@ 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 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, # and a later commit regenerating from it leaves the config untouched, @@ -104,8 +120,30 @@ 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/) + # 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' \ - -- .github/workflows/ .config/tend.yaml .github/audit/ | sort -u) + -- "${WINDOW[@]}" | sort -u) if [ -z "$COMMITS" ]; then echo "No workflow or tend-config changes since $SINCE." @@ -156,7 +194,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. @@ -205,7 +248,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') @@ -242,11 +290,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 @@ -315,7 +363,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 (\`${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 50d0a0fe..4485747e 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -12,9 +12,11 @@ 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, 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. @@ -23,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. 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.) - 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. @@ -48,11 +52,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`. @@ -128,7 +134,7 @@ Every dependency Dormouse **puts on a user's machine** is listed at `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 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 @@ -170,7 +178,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 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. + +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. @@ -182,9 +194,9 @@ An attacker who lands a prompt injection in tend's harness can reach three secre **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. +**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. @@ -203,6 +215,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. @@ -218,7 +231,14 @@ 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.** 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. ## Reporting a Vulnerability @@ -236,6 +256,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, 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. **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. @@ -264,12 +286,16 @@ 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/` 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: - - `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, 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 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 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. diff --git a/docs/specs/remote-security-model.md b/docs/specs/remote-security-model.md index 8901858b..0e732327 100644 --- a/docs/specs/remote-security-model.md +++ b/docs/specs/remote-security-model.md @@ -229,8 +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 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 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` 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/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/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/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..b3b5c275 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,46 @@ 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', () => { + 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 + // 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); + // 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. 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. + 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..1a2f96de 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, @@ -112,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; @@ -148,7 +160,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 +295,55 @@ export class RemoteHost { this.#clients.clear(); } + /** + * 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; + } + + /** + * 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++; + 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'); + // 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--; + } + } + /** Get or create the per-client state record for `clientId`. */ #clientState(clientId: string): ClientState { let state = this.#clients.get(clientId); @@ -306,7 +374,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; } @@ -359,6 +428,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/scripts/security-audit-local.sh b/scripts/security-audit-local.sh index ea9f52dc..11d842ca 100755 --- a/scripts/security-audit-local.sh +++ b/scripts/security-audit-local.sh @@ -41,9 +41,30 @@ 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. + # 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)}" rm -f "$out" + # shellcheck disable=SC2086 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/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 diff --git a/server-lib-common/src/remote/wire.ts b/server-lib-common/src/remote/wire.ts index a0cd4389..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 { @@ -407,13 +419,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/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-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); +}); diff --git a/server/src/app.ts b/server/src/app.ts index 12392f96..5af27327 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -398,6 +398,10 @@ 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); +}); 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}"))?;