diff --git a/.github/audit/application-security.md b/.github/audit/application-security.md index c63853406..15c1c2923 100644 --- a/.github/audit/application-security.md +++ b/.github/audit/application-security.md @@ -4,42 +4,12 @@ - `docs/specs/security-local.md` - `docs/specs/security-remote.md` -- `docs/specs/security-hosted.md` **Output file:** `audit-application.md` This is a code-and-specs audit of the product's own boundaries — the remote -control stack, and the local application. You need no PAT — do not use one. -The two pgstencil provenance checks below do read the GitHub API, but only a -public repository, which the workflow's default `GITHUB_TOKEN` and the -operator's own `gh` login both reach; if that API is unreachable, report those -two checks as `UNVERIFIABLE`. - -For Hosted accounts, read `docs/specs/hosted.md`, `hosted/server/`, -`hosted/src/`, `hosted/scripts/`, `hosted/wrangler.jsonc`, and -`.github/workflows/hosted-preview.yml` and -`.github/workflows/hosted-production.yml` — `docs/specs/security-hosted.md`'s -Deployment boundary quantifies over the preview and production paths, which -live in those scripts and workflows rather than in the Worker. Verify the -vendored packages by their provenance rather than by reading them: hash each -archive in `vendor/` against `vendor/build.json`; read each archive's own claim -with `tar -xOf vendor/.tgz package/dist/provenance.json` and check -that it names `build.json`'s commit and does not record `dirty`; then check -that commit against pgstencil `main` and its audit: - -```sh -gh api repos/diffplug/pgstencil/compare/...main --jq .status -gh api repos/diffplug/pgstencil/commits//check-runs \ - --jq '.check_runs[] | select(.name=="security-audit") | .conclusion' -``` - -The first must be `ahead` or `identical`, the second `success`. The packed code -itself is audited in `diffplug/pgstencil` by that repository's own -`security-audit` workflow against its `SECURITY.md`; do not audit the tarballs' -contents here — audit how `hosted/` configures the adapter. Distinguish tested -code from pending production configuration; do not treat local provider -simulations as live OAuth acceptance, and treat a checked-in placeholder as no -evidence about an external control. +control stack, and the local application. You need no GitHub API access and no +PAT — do not use one. Read, at minimum: `docs/specs/remote-security-model.md` **and its paired `docs/specs/remote-security-model.rationale.md`**, `docs/specs/relay.md`, @@ -170,23 +140,6 @@ Be adversarial, and go past the `FAIL IF` list. Ask specifically: reconstruct and relay only the registered pane-level shapes, never relay a nested document's location, and target only that origin plus the validated app origin—never a wildcard or foreign origin. -- **Is the Hosted origin the only one that can drive Hosted?** Trace a request - from `hosted/server/worker.ts` through `workerApp`'s origin gate and - `secureHeaders`: a foreign `Host`, a preview hostname, a misconfigured - deployment's error path, and the SPA fallback must each answer without - credentialed CORS, without a cacheable shell, and without inline script. - Check that authentication cookies stay `__Host-`, Secure, HttpOnly, `Path=/` - and Domain-less, and that no session token reaches browser JSON or storage. -- **Can a Hosted login become terminal access, or an account become someone - else's?** `authPolicy` must keep explicit linking and independent logins; a - callback whose initiating login was revoked must fail; an unused or unknown - provider credential must enable nothing. No Hosted endpoint may mint a Burrow - ACL grant or stand in for the encrypted pairing and presence proof. -- **Does anything from the test or preview build reach production?** The - production Worker must not export the captured-email inbox, the deterministic - clock, or the testing injection module; preview must not copy production - routes, bindings, or credentials, must not call real mail or OAuth, and its - cleanup must check out the base branch rather than the closed PR's. - Does the shipped code still match what the specs and this section claim? Spec drift is a finding; say which side is wrong. The newest sections are the ones most likely to have drifted: `remote-security-model.md`'s Presence proofs, @@ -199,11 +152,11 @@ Be adversarial, and go past the `FAIL IF` list. Ask specifically: look for what a *textual* lint cannot see. 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. +by a list: you own everything in the repository that `supply-chain.md`, +`ci-and-secrets.md`, and `hosted.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 @@ -215,10 +168,9 @@ 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/`, `relay/`, `remote-lib-common/`, `standalone/`, -`vscode-ext/`, `dor/`, `dor-lib-common/`, `hosted/`, `vendor/`, `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. +`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 diff --git a/.github/audit/hosted.md b/.github/audit/hosted.md new file mode 100644 index 000000000..64588cc57 --- /dev/null +++ b/.github/audit/hosted.md @@ -0,0 +1,72 @@ +# Domain: hosted + +**Scope — these specs, and no others:** + +- `docs/specs/security-hosted.md` + +**Output file:** `audit-hosted.md` + +This is a code-and-specs audit of the Hosted account application. You need no +PAT — do not use one. The two pgstencil provenance checks below do read the +GitHub API, but only a public repository, which the workflow's default +`GITHUB_TOKEN` and the operator's own `gh` login both reach; if that API is +unreachable, report those two checks as `UNVERIFIABLE`. + +Read `docs/specs/hosted.md`, `hosted/server/`, `hosted/src/`, `hosted/scripts/`, +`hosted/wrangler.jsonc`, and `.github/workflows/hosted-preview.yml` and +`.github/workflows/hosted-production.yml` — `docs/specs/security-hosted.md`'s +Deployment boundary quantifies over the preview and production paths, which +live in those scripts and workflows rather than in the Worker. + +Verify the vendored packages by their provenance rather than by reading them: +hash each archive in `vendor/` against `vendor/build.json`; read each archive's +own claim with `tar -xOf vendor/.tgz package/dist/provenance.json` and +check that it names `build.json`'s commit and does not record `dirty`; then +check that commit against pgstencil `main` and its audit: + +```sh +gh api repos/diffplug/pgstencil/compare/...main --jq .status +gh api repos/diffplug/pgstencil/commits//check-runs \ + --jq '.check_runs[] | select(.name=="security-audit") | .conclusion' +``` + +The first must be `ahead` or `identical`, the second `success`. The packed code +itself is audited in `diffplug/pgstencil` by that repository's own +`security-audit` workflow against its `SECURITY.md`; do not audit the tarballs' +contents here — audit how `hosted/` configures the adapter. Distinguish tested +code from pending production configuration; do not treat local provider +simulations as live OAuth acceptance, and treat a checked-in placeholder as no +evidence about an external control. + +## Qualitative pass + +You own `hosted/` and `vendor/`. You **read** `.github/workflows/hosted-preview.yml` +and `.github/workflows/hosted-production.yml` for the Deployment boundary above, +but you do not own them: `ci-and-secrets` owns those workflows' credentials, +environments, reviewers, and token placement +(`docs/specs/security-ci.md` -> "Hosted Deployments"). Report what the +deployment *path* does and leave that half to it, so the two domains do not +report the same finding twice. + +Be adversarial, and go past the `FAIL IF` list. Ask specifically: + +- **Is the Hosted origin the only one that can drive Hosted?** Trace a request + from `hosted/server/worker.ts` through `workerApp`'s origin gate and + `secureHeaders`: a foreign `Host`, a preview hostname, a misconfigured + deployment's error path, and the SPA fallback must each answer without + credentialed CORS, without a cacheable shell, and without inline script. + Check that authentication cookies stay `__Host-`, Secure, HttpOnly, `Path=/` + and Domain-less, and that no session token reaches browser JSON or storage. +- **Can a Hosted login become terminal access, or an account become someone + else's?** `authPolicy` must keep explicit linking and independent logins; a + callback whose initiating login was revoked must fail; an unused or unknown + provider credential must enable nothing. No Hosted endpoint may mint a Burrow + ACL grant or stand in for the encrypted pairing and presence proof. +- **Does anything from the test or preview build reach production?** The + production Worker must not export the captured-email inbox, the deterministic + clock, or the testing injection module; preview must not copy production + routes, bindings, or credentials, must not call real mail or OAuth, and its + cleanup must check out the base branch rather than the closed PR's. + +Does the shipped code still match what the spec and this section claim? Spec +drift is a finding; say which side is wrong. diff --git a/.github/audit/orchestrator.md b/.github/audit/orchestrator.md index 4a63691a9..7a585ea40 100644 --- a/.github/audit/orchestrator.md +++ b/.github/audit/orchestrator.md @@ -5,30 +5,32 @@ The security specs (`docs/specs/security*.md`) are what you audit against: their `FAIL IF` lines are concrete mechanical checks, and `docs/specs/security-audit.md` says that list is not exhaustive, so each domain gets a qualitative pass too. -**Audit nothing yourself.** Fan the work out to three subagents with disjoint +**Audit nothing yourself.** Fan the work out to four subagents with disjoint scopes, then merge what they return. The domains are genuinely different subject matters with different evidence — dependency provenance is lockfiles, CI is `gh api` output, application security is reading the pairing code -adversarially — and one context holding all three degrades the third. +adversarially, Hosted accounts are a Worker's origin gate and its deployment +path — and one context holding them all degrades the ones that read code. -## 1. Spawn all three +## 1. Spawn all four Spawn them with the Task tool **in a single message** so they run -concurrently, using these three `subagent_type` values: +concurrently, using these four `subagent_type` values: - `supply-chain` - `ci-and-secrets` - `application-security` +- `hosted` 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. +should run on. `application-security` and `hosted` are deliberately on a +stronger model than the other two; do not override them, 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. +and holding all four in your context is the thing this split exists to avoid. ## 2. Wait without ending your turn @@ -74,13 +76,13 @@ finished() { [ -s "$1" ] && [ "$(sed -e '/^[[:space:]]*$/d' "$1" | tail -n1)" = # 540 leaves a minute of margin under the cap. CALL_END=$(( $(date +%s) + 540 )) ANSWER="ALL FINISHED" -until finished audit-supply-chain.md && finished audit-ci-secrets.md && finished audit-application.md; do +until finished audit-supply-chain.md && finished audit-ci-secrets.md && finished audit-application.md && finished audit-hosted.md; do NOW=$(date +%s) [ "$NOW" -ge "$DEADLINE" ] && { ANSWER="DEADLINE"; break; } [ "$NOW" -ge "$CALL_END" ] && { ANSWER="STILL WAITING"; break; } sleep 10 done -for f in audit-supply-chain.md audit-ci-secrets.md audit-application.md; do +for f in audit-supply-chain.md audit-ci-secrets.md audit-application.md audit-hosted.md; do if finished "$f"; then echo "$f: finished" elif [ -s "$f" ]; then echo "$f: still writing" else echo "$f: not started"; fi @@ -151,6 +153,7 @@ emit() { emit "Supply chain" audit-supply-chain.md emit "CI and secrets" audit-ci-secrets.md emit "Application security" audit-application.md + emit "Hosted accounts" audit-hosted.md } > audit-report.md ``` @@ -175,7 +178,7 @@ unaudited ones in the one paragraph a reader starts from. ## 4. The verdict Write `PASS` or `FAIL` — no other text — to `audit-status.txt` according to -the precedence below. PASS requires all three domains to pass. +the precedence below. PASS requires all four domains to pass. FAIL if any subagent returned FAIL. That is a finding, and it stays a finding whether or not the other domains reported. diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 35149f43a..701aa0a6f 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -21,7 +21,7 @@ jobs: audit: runs-on: ubuntu-latest env: - # The three per-domain fragment filenames, in one place. They are + # The four per-domain fragment filenames, in one place. They are # named by the redactor's argv, its fail-closed `rm -f`, and the # reporting step's guard loop, and drift between two of those lists # is precisely what the `FAIL IF` on the redactor exists to catch — @@ -30,7 +30,7 @@ jobs: # space-separated and shell-safe. The artifact `path:` needs one # entry per line and so cannot consume it; that list is spelled out # and carries a pointer back here. - AUDIT_FRAGMENTS: audit-supply-chain.md audit-ci-secrets.md audit-application.md + AUDIT_FRAGMENTS: audit-supply-chain.md audit-ci-secrets.md audit-application.md audit-hosted.md # Must stay well above the orchestrator's own wait deadline in # `.github/audit/orchestrator.md` (32 minutes, counted from the first # time its wait loop runs — i.e. after checkout, setup-node, the @@ -86,7 +86,7 @@ jobs: # calls with `GH_TOKEN=$AUDIT_PAT`. AUDIT_PAT: ${{ secrets.AUDIT_PAT }} # Process-wide: this raises the cap on every Bash call in the - # session, the three domain subagents' as well as the + # session, the four domain subagents' as well as the # orchestrator's. What it is for is the orchestrator's wait loop, # which breaks itself at 540s so a capped call still prints # `STILL WAITING`; at the two-minute default that call is @@ -125,21 +125,22 @@ jobs: # release gate for $5 and no verdict. # # The cause was ending the turn, not delegating. `Task` is - # therefore allowed on purpose — the audit fans out to three + # therefore allowed on purpose — the audit fans out to four # scoped domain agents — and the orchestrator prompt carries # 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 + # Two of the four 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. + # does them well. `application-security` and `hosted` are the + # ones that read code adversarially, and their 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. They run on + # Opus; the orchestrator and the two mechanical domains 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 @@ -148,7 +149,7 @@ jobs: --model sonnet --allowed-tools "Read,Write,Edit,Bash,Grep,Glob,Task,Agent" --disallowed-tools "Workflow" - --agents '{"supply-chain":{"description":"Audits docs/specs/security-supply-chain.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/supply-chain.md`, and follow them exactly."},"ci-and-secrets":{"description":"Audits docs/specs/security.md, docs/specs/security-ci.md, and docs/specs/security-audit.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/ci-and-secrets.md`, and follow them exactly."},"application-security":{"description":"Audits docs/specs/security-local.md and docs/specs/security-remote.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"}}' + --agents '{"supply-chain":{"description":"Audits docs/specs/security-supply-chain.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/supply-chain.md`, and follow them exactly."},"ci-and-secrets":{"description":"Audits docs/specs/security.md, docs/specs/security-ci.md, and docs/specs/security-audit.md.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/ci-and-secrets.md`, and follow them exactly."},"application-security":{"description":"Audits docs/specs/security-local.md and docs/specs/security-remote.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"},"hosted":{"description":"Audits docs/specs/security-hosted.md: Hosted accounts and the pgstencil provenance link.","prompt":"Read `.github/audit/_preamble.md` and then `.github/audit/hosted.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 @@ -216,7 +217,7 @@ jobs: TRANSCRIPT="$RUNNER_TEMP/claude-execution-output.json" # Every sink that the archive step below publishes has to be in # the argument list, and the fail-closed `rm -f` has to delete - # the same set — docs/specs/security-audit.md names `audit-report.md`, the three + # the same set — docs/specs/security-audit.md names `audit-report.md`, the four # per-domain fragments, and the transcript. The fragments are # agent output like any other: a subagent that shells out and # prints an environment lands the value in its own fragment, @@ -276,6 +277,7 @@ jobs: audit-supply-chain.md audit-ci-secrets.md audit-application.md + audit-hosted.md retention-days: 14 if-no-files-found: warn @@ -349,7 +351,7 @@ jobs: # 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 + # then lost — a mis-summarised verdict, or four 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 diff --git a/docs/specs/security-audit.md b/docs/specs/security-audit.md index e29b4bf92..996b04dc1 100644 --- a/docs/specs/security-audit.md +++ b/docs/specs/security-audit.md @@ -1,6 +1,6 @@ # Security Audit -> Owns how the security specs are audited: the schedule and the release gate, the three domains and their prompts, the orchestration, the three outcomes, the reporting step, and the environment that holds `AUDIT_PAT`. Defers what is audited to `docs/specs/security.md` and the specs it names. +> Owns how the security specs are audited: the schedule and the release gate, the four domains and their prompts, the orchestration, the three outcomes, the reporting step, and the environment that holds `AUDIT_PAT`. Defers what is audited to `docs/specs/security.md` and the specs it names. > Read `docs/specs/security.md` first. ## Schedule and gate @@ -15,7 +15,7 @@ ## Domains -**Must fan the CI audit out to three subagents, each owning a disjoint share of the tree**; a domain reads outside its share — `application-security` runs the repo's own lints — but owns nothing there. The orchestrator audits nothing itself, spawning them concurrently and merging what they return (rationale). +**Must fan the CI audit out to four subagents, each owning a disjoint share of the tree**; a domain reads outside its share — `application-security` runs the repo's own lints — but owns nothing there. The orchestrator audits nothing itself, spawning them concurrently and merging what they return (rationale). **Ownership is by file: every `docs/specs/security*.md` spec is in exactly one domain's scope**, declared as backticked repo paths in the bullet list under the `**Scope` line of its domain file in `.github/audit/`, and enforced by `scripts/spec-lint.mjs`. @@ -23,11 +23,12 @@ |---|---| | `supply-chain` | `docs/specs/security-supply-chain.md` | | `ci-and-secrets` | `docs/specs/security-ci.md`, `docs/specs/security-audit.md`, `docs/specs/security.md` | -| `application-security` | `docs/specs/security-local.md`, `docs/specs/security-remote.md`, `docs/specs/security-hosted.md` | +| `application-security` | `docs/specs/security-local.md`, `docs/specs/security-remote.md` | +| `hosted` | `docs/specs/security-hosted.md` | -**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, and only the prompt tells `application-security` not to use it. A prompt is not a control: three contexts each *read* less, none *holds* less. A known gap, staged as `## Future` -> Credential separation. +**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, and only the prompt tells `application-security` and `hosted` not to use it. A prompt is not a control: four contexts each *read* less, none *holds* less. A known gap, staged as `## Future` -> Credential separation. -**Must pin the mechanical domains to Sonnet and `application-security` to Opus in CI and locally** (rationale). +**Must pin the mechanical domains to Sonnet and the two code-reading domains — `application-security` and `hosted` — to Opus in CI and locally** (rationale). **Must keep shared CI/local prompts and their scopes in `.github/audit/`** (rationale). @@ -39,8 +40,8 @@ - **The subtraction is recursive**: where a domain claims a subdirectory rather than a whole tree — as `supply-chain` does inside `website/` — the remainder of that tree belongs to `application-security`. - **FAIL IF** a `docs/specs/security*.md` spec is in no domain's scope, or in two, or a scope names a file that does not exist (rationale). -- **FAIL IF** the audit stops fanning out to a dedicated `application-security` subagent scoped to the application specs in the Domains table, or that scope is merged back into a context that also carries the supply-chain or CI domains (rationale). -- **FAIL IF** `application-security` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `claude_args` — its `--model` sets the floor and its `--agents` raises that one domain — and `scripts/security-audit-local.sh` (rationale). +- **FAIL IF** the audit stops fanning out to a dedicated `application-security` subagent scoped to the application specs in the Domains table, or to a dedicated `hosted` subagent scoped to `docs/specs/security-hosted.md`, or either scope is merged back into a context that also carries another domain (rationale). +- **FAIL IF** `application-security` or `hosted` does not run on a stronger model than the mechanical domains, in **both** `.github/workflows/security-audit.yaml`'s `claude_args` — its `--model` sets the floor and its `--agents` raises those two domains — and `scripts/security-audit-local.sh` (rationale). - **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 (rationale). - **FAIL IF** the union of the subagents' qualitative scopes does not cover every top-level path in the repository (rationale). - **FAIL IF** `.github/audit/` or `.vscode/` is outside **any** consumer of `.github/workflows/workflow-audit.yaml`'s diff window — the commit list, `own_changes`, and both classifiers' refusals, whose half is *derived* from the single `WINDOW` array (`"${WINDOW[@]:1}"`). Widening one consumer without the others is the failure. The security specs are deliberately *not* watched there (rationale). @@ -53,7 +54,7 @@ Source of truth: the `**Scope` and `## Qualitative pass` sections of each domain - **The job's `timeout-minutes: 40` stays above the orchestrator's 32-minute wait deadline** (rationale). - **`--allowed-tools` enforces none of this**, only auto-approving; `Task`/`Agent` are allowed on purpose and only `Workflow` is denied (rationale). -- **Each subagent appends to its own fragment as it determines each result**, never holding findings for a write-up at the end, and the orchestrator concatenates them rather than retyping; `AUDIT_FRAGMENTS` in `.github/workflows/security-audit.yaml` names the three. Fragments upload with the transcript, so an orchestrator that dies mid-merge still ships what the domains found. +- **Each subagent appends to its own fragment as it determines each result**, never holding findings for a write-up at the end, and the orchestrator concatenates them rather than retyping; `AUDIT_FRAGMENTS` in `.github/workflows/security-audit.yaml` names the four. Fragments upload with the transcript, so an orchestrator that dies mid-merge still ships what the domains found. - **A fragment opens `VERDICT: INCONCLUSIVE` and closes with the literal ``**, its verdict rewritten once at the end. **The sentinel, not existence, is what a reader treats as finished** (rationale). - **FAIL IF** the orchestrator prompt stops requiring a non-turn-ending wait — a Bash `until` loop over the fragments' sentinels, **breaking on its own sub-cap under the Bash cap the workflow sets** so every call ends by printing its answer, re-issued under a bounded 32-minute deadline **persisted to a file** (`$RUNNER_TEMP/audit-deadline`) rather than recomputed from `now`. That cap is `BASH_DEFAULT_TIMEOUT_MS` in `.github/workflows/security-audit.yaml`, set above the loop's 540-second break; the harness default is two minutes, under it (rationale). @@ -82,7 +83,7 @@ Source of truth: `2. Wait without ending your turn`, `3. Merge`, and `4. The ver - **The report is truncated to 32,000 characters before posting**, head kept, by `scripts/clamp-issue-body.mjs` (self-tested by `scripts/clamp-issue-body-selftest.mjs`). The call is non-fatal; the `audit-transcript` artifact holds the report in full; `.github/workflows/workflow-audit.yaml` truncates its commit list the same way (rationale). - **Every run uploads the `audit-transcript` artifact, which is world-readable and not secret-masked** — 14-day retention, deep-linked from failure issues (rationale). -- **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 (rationale). +- **FAIL IF** the `Redact secrets from agent output` step is removed, stops covering any sink that is later published (`audit-report.md`, the four per-domain fragments, and the transcript), or stops failing closed by deleting those files when the redactor itself throws (rationale). - **FAIL IF** the reporting step writes issue prose per *combination* of conditions rather than one note per condition that holds (rationale). - **FAIL IF** either fragment guard is gated on the status at all (rationale). - **FAIL IF** the reporting step accepts any domain verdict other than exact `VERDICT: PASS` as passing, fails to recognize a `VERDICT: FAIL` prefix as dissent, ignores an inconclusive domain, accepts a fragment with no completion sentinel as finished, or accepts status text other than literal `PASS`/`FAIL` (rationale). @@ -109,4 +110,4 @@ Source of truth: `Verify AUDIT_PAT is provisioned` in `.github/workflows/securit A second job outside the `security-audit` environment, running the domains that need no PAT and passing their fragments back as artifacts, would leave -`application-security` unable to hold `AUDIT_PAT` at all. +`application-security` and `hosted` unable to hold `AUDIT_PAT` at all. diff --git a/docs/specs/security-audit.rationale.md b/docs/specs/security-audit.rationale.md index bc0fae7be..ce88d36f7 100644 --- a/docs/specs/security-audit.rationale.md +++ b/docs/specs/security-audit.rationale.md @@ -6,7 +6,9 @@ The three release-gate pieces are named separately because they break independen ## Domains -One context holding all three subject matters degrades application security — the newest domain, with the most code behind it, and the easiest to crowd out with API responses. +One context holding every subject matter degrades application security — the domain with the most code behind it, and the easiest to crowd out with API responses. + +Hosted accounts were split out of `application-security` on 2026-09-21. That domain already carried remote control (where the depth goes), the local boundaries, Hosted, and the catch-all sweep, and it had overrun the 32-minute deadline more than once, so a remote-control pass that ran out of time took the Hosted results down with it. Hosted is a disjoint tree — `hosted/`, `vendor/`, and the two `hosted-*.yml` workflows it reads for the Deployment boundary — with its own spec, so it splits cleanly and now writes its own fragment. It also gives the pgstencil provenance checks a prompt that is about them rather than a paragraph inside one about pairing code. It runs on Opus for the same reason `application-security` does: the Worker's origin gate and the deployment path are read, not enumerated. Folding the application-security scope back into a shared context is how that spec stops being audited without anyone deciding to stop auditing it. @@ -36,7 +38,7 @@ The deadline is persisted because one longer than the ten-minute Bash cap cannot A call that reaches the cap is *moved to the background*, not returned: it prints nothing back, so re-issuing becomes a judgement call rather than a step. Run 34457954349 happened to re-issue a third time and its deadline fell inside that call, so it merged and published two PASS domains; run 34581574869 spent one extra call checking the fragments, which shifted the phase enough that a third wait would have been needed, ended its turn instead, and published no report at all — the same two domains' PASS fragments survived only in the artifact. A loop that ends itself under the cap turns both nights into the same printed answer. -The answer is the call's last line, below a per-domain status, rather than an `ls` listing: once domains append as they go, all three fragments exist within minutes, so a listing of three files no longer means three reports. +The answer is the call's last line, below a per-domain status, rather than an `ls` listing: once domains append as they go, every fragment exists within minutes, so a listing of them no longer means that many reports. The 25-minute deadline was raised to 32 after `application-security` failed to report inside it two nights running — the deadline expired on it on 2026-09-10, and on 2026-09-11 it was still sweeping when the run ended at 21 minutes — while roughly 13 of the job's 40 minutes went unused on both nights. On 2026-09-15 every domain reported in an agent step that ran 25.5 minutes, so the old deadline had little slack even on a night that finished. diff --git a/docs/specs/security.md b/docs/specs/security.md index 8346fb8e2..9095ddfcf 100644 --- a/docs/specs/security.md +++ b/docs/specs/security.md @@ -118,7 +118,7 @@ Gaps rather than accepted risks: we intend to close them. writes ([same](./security-remote.md#revocation-and-the-audit-trail)). - **The workflow audit's window has two evasions**, both in how the window is computed ([Automated Maintainer](./security-ci.md#automated-maintainer-tend)). -- **The audit's three subagents share one credential.** Their contexts are +- **The audit's four subagents share one credential.** Their contexts are separate; `AUDIT_PAT` is not ([Domains](./security-audit.md#domains)). - **The notarization password sits on a command line for up to half an hour** per architecture; the remedy is known and not yet done @@ -140,7 +140,7 @@ lints can only read. **Every night at 04:21 UTC, and before every VS Code release**, `.github/workflows/security-audit.yaml` audits the repository against these -specs. Three subagents, each owning the specs below, run every `FAIL IF` as a +specs. Four subagents, each owning the specs below, run every `FAIL IF` as a mechanical check with evidence, then read their domain adversarially for what no check names. A failure, or a run reaching no verdict, files a public issue labeled @@ -149,11 +149,12 @@ and holds the release; a later pass closes it. Open issues are live; closed ones record what tripped and changed. `scripts/security-audit-local.sh` runs the same prompts locally. [security-audit.md](./security-audit.md) is the contract. pgstencil audits the -packages Hosted consumes in its own repository; these checks verify that link. +packages Hosted consumes in its own repository. | Domain | Specs | Covers | | --- | --- | --- | -| `application-security` | [security-local.md](./security-local.md), [security-remote.md](./security-remote.md), [security-hosted.md](./security-hosted.md) | local boundaries, remote control, Hosted accounts, and every path no other domain claims | +| `application-security` | [security-local.md](./security-local.md), [security-remote.md](./security-remote.md) | local boundaries, remote control, and everything no other domain claims | +| `hosted` | [security-hosted.md](./security-hosted.md) | Hosted accounts and the pgstencil provenance link | | `supply-chain` | [security-supply-chain.md](./security-supply-chain.md) | the dependency graph, the lockfile, the disclosure and its generator | | `ci-and-secrets` | [security-ci.md](./security-ci.md), [security-audit.md](./security-audit.md), this spec | GitHub Actions, the bot, releases, secrets, and the audit itself | diff --git a/scripts/security-audit-local.sh b/scripts/security-audit-local.sh index 77a165f96..2da4019b6 100755 --- a/scripts/security-audit-local.sh +++ b/scripts/security-audit-local.sh @@ -11,7 +11,7 @@ # repo or the ci-and-secrets domain will report FAILs that are really 403s. # # Usage: -# scripts/security-audit-local.sh # all three domains +# scripts/security-audit-local.sh # all four domains # scripts/security-audit-local.sh application-security # one domain # # Reports land in ./audit-*.md, which .gitignore covers. @@ -34,7 +34,7 @@ if ! command -v claude >/dev/null 2>&1; then exit 1 fi -for f in _preamble orchestrator supply-chain ci-and-secrets application-security; do +for f in _preamble orchestrator supply-chain ci-and-secrets application-security hosted; do [ -f "$AUDIT_DIR/$f.md" ] || { echo "error: missing $AUDIT_DIR/$f.md" >&2; exit 1; } done @@ -47,26 +47,27 @@ run_domain() { supply-chain) out=audit-supply-chain.md ;; ci-and-secrets) out=audit-ci-secrets.md ;; application-security) out=audit-application.md ;; - *) echo "error: unknown domain '$domain' (supply-chain|ci-and-secrets|application-security)" >&2; return 64 ;; + hosted) out=audit-hosted.md ;; + *) echo "error: unknown domain '$domain' (supply-chain|ci-and-secrets|application-security|hosted)" >&2; return 64 ;; esac # 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. + # `--agents`): the two mechanical domains run on the default, and the two + # code-reading domains — application-security and hosted — run on Opus. + # Local and CI must agree here, or the domains where the model matters most + # are the ones 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 docs/specs/security-audit.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. + # only the code-reading domains carry 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" + case "$domain" in application-security|hosted) model_args="--model opus" ;; esac echo "==> $domain -> $out${model_args:+ ($model_args)}" rm -f "$out" @@ -111,11 +112,11 @@ if [ $# -gt 0 ]; then exit $? fi -# All three, sequentially rather than fanned out. CI parallelises because it is +# All four, sequentially rather than fanned out. CI parallelises because it is # paying wall-clock for a nightly; locally, serial output is readable and a # each domain's failure is recorded while the remaining domains still run. status=0 -for domain in supply-chain ci-and-secrets application-security; do +for domain in supply-chain ci-and-secrets application-security hosted; do run_domain "$domain" || status=1 done diff --git a/scripts/security-audit.test.mjs b/scripts/security-audit.test.mjs index 9064e37f0..50deac2c6 100644 --- a/scripts/security-audit.test.mjs +++ b/scripts/security-audit.test.mjs @@ -35,28 +35,28 @@ const SENTINEL = ''; const reporting = runBlock('Surface result, file or close issue'); const cases = [ - { name: 'all checks pass', status: 'PASS\n', verdicts: ['PASS', 'PASS', 'PASS'], expected: 'PASS' }, - { name: 'missing merged verdict', verdicts: ['PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, - { name: 'embedded whitespace is not PASS', status: 'P A\nSS\n', verdicts: ['PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, - { name: 'PASS prefix with a suffix is unreadable', status: 'PASS', verdicts: ['PASS but unfinished', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, - { name: 'missing fragment', status: 'PASS', verdicts: [null, 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, - { name: 'unverifiable checks override merged PASS', status: 'PASS', verdicts: ['INCONCLUSIVE', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, - { name: 'dissent overrides missing merged verdict', verdicts: ['FAIL', 'PASS', 'PASS'], expected: 'FAIL' }, - { name: 'FAIL with explanation overrides merged PASS', status: 'PASS', verdicts: ['FAIL — credential leaked', 'PASS', 'PASS'], expected: 'FAIL' }, - { name: 'FAIL with explanation overrides missing merged verdict', verdicts: ['FAIL — credential leaked', 'PASS', 'PASS'], expected: 'FAIL' }, - { name: 'FAIL records every incomplete condition', status: 'FAIL', verdicts: [null, 'garbled', 'INCONCLUSIVE'], expected: 'FAIL', notes: ['left no report', 'could not be read', 'could not determine every check'] }, - { name: 'dissent and incomplete domains coexist', status: 'PASS', verdicts: ['FAIL', null, 'INCONCLUSIVE'], expected: 'FAIL', notes: ['returned `FAIL`', 'left no report', 'could not determine every check'] }, + { name: 'all checks pass', status: 'PASS\n', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], expected: 'PASS' }, + { name: 'missing merged verdict', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, + { name: 'embedded whitespace is not PASS', status: 'P A\nSS\n', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, + { name: 'PASS prefix with a suffix is unreadable', status: 'PASS', verdicts: ['PASS but unfinished', 'PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, + { name: 'missing fragment', status: 'PASS', verdicts: [null, 'PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, + { name: 'unverifiable checks override merged PASS', status: 'PASS', verdicts: ['INCONCLUSIVE', 'PASS', 'PASS', 'PASS'], expected: 'INCONCLUSIVE' }, + { name: 'dissent overrides missing merged verdict', verdicts: ['FAIL', 'PASS', 'PASS', 'PASS'], expected: 'FAIL' }, + { name: 'FAIL with explanation overrides merged PASS', status: 'PASS', verdicts: ['FAIL — credential leaked', 'PASS', 'PASS', 'PASS'], expected: 'FAIL' }, + { name: 'FAIL with explanation overrides missing merged verdict', verdicts: ['FAIL — credential leaked', 'PASS', 'PASS', 'PASS'], expected: 'FAIL' }, + { name: 'FAIL records every incomplete condition', status: 'FAIL', verdicts: [null, 'garbled', 'INCONCLUSIVE', 'PASS'], expected: 'FAIL', notes: ['left no report', 'could not be read', 'could not determine every check'] }, + { name: 'dissent and incomplete domains coexist', status: 'PASS', verdicts: ['FAIL', null, 'INCONCLUSIVE', 'PASS'], expected: 'FAIL', notes: ['returned `FAIL`', 'left no report', 'could not determine every check'] }, // A domain cut off between rewriting its verdict line and writing its // sentinel reads as a clean PASS on line 1. Without the sentinel guard that // is a merged PASS over a report that stopped early, and PASS opens the // release gate. - { name: 'PASS without a sentinel is a cut-off domain', status: 'PASS', verdicts: ['PASS', 'PASS', 'PASS'], unfinished: [2], expected: 'INCONCLUSIVE', notes: ['cut off mid-report'] }, - { name: 'a cut-off FAIL is still a finding', status: 'PASS', verdicts: ['PASS', 'PASS', 'FAIL'], unfinished: [2], expected: 'FAIL', notes: ['returned `FAIL`', 'cut off mid-report'] }, - { name: 'a trailing blank line still ends a report', status: 'PASS', verdicts: ['PASS', 'PASS', 'PASS'], trailingBlank: true, expected: 'PASS' }, + { name: 'PASS without a sentinel is a cut-off domain', status: 'PASS', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], unfinished: [2], expected: 'INCONCLUSIVE', notes: ['cut off mid-report'] }, + { name: 'a cut-off FAIL is still a finding', status: 'PASS', verdicts: ['PASS', 'PASS', 'FAIL', 'PASS'], unfinished: [2], expected: 'FAIL', notes: ['returned `FAIL`', 'cut off mid-report'] }, + { name: 'a trailing blank line still ends a report', status: 'PASS', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], trailingBlank: true, expected: 'PASS' }, // The no-verdict note is the reader's index into the merged report, so it // names every marker the merge can leave there. Drop one and the reader is // told to look for two shapes in a report that has three. - { name: 'the no-verdict note names every report marker', verdicts: ['PASS', 'PASS', 'PASS'], unfinished: [2], expected: 'INCONCLUSIVE', notes: ['`UNVERIFIABLE`', '`_Incomplete …_`', '`_No report …_`'] }, + { name: 'the no-verdict note names every report marker', verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], unfinished: [2], expected: 'INCONCLUSIVE', notes: ['`UNVERIFIABLE`', '`_Incomplete …_`', '`_No report …_`'] }, // Run 34581574869 ended its turn before §3, so no merged report existed and // this arm published a single line — while two domains' finished `VERDICT: // PASS` fragments sat in the working directory and reached a human only @@ -71,7 +71,7 @@ const cases = [ // satisfied by the marker appearing anywhere: dropping the sentinel test // marks every fragment and inverting it marks the finished one, and both // read as a pass. Those are the inverse of the bug this arm fixes. - { name: 'no merged report publishes the fragments, marking cut-off and absent domains', report: null, verdicts: ['PASS', 'PASS', null], unfinished: [1], expected: 'INCONCLUSIVE', + { name: 'no merged report publishes the fragments, marking cut-off and absent domains', report: null, verdicts: ['PASS', 'PASS', null, 'PASS'], unfinished: [1], expected: 'INCONCLUSIVE', notes: ['the merge never ran', '## audit-supply-chain.md', 'VERDICT: PASS', '## audit-application.md', '_No report — this domain produced no fragment._', '## audit-ci-secrets.md\n\n_Incomplete — this domain never closed its report'], counts: { '_Incomplete — this domain never closed its report': 1 } }, @@ -139,7 +139,7 @@ for (const [verdict, cliExit, expected, sentinel = true] of [['PASS', 0, 0], ['F const { dir, env } = fixture(t); copyFileSync(join(repo, 'scripts/security-audit-local.sh'), join(dir, 'scripts/security-audit-local.sh')); mkdirSync(join(dir, '.github/audit'), { recursive: true }); - for (const name of ['_preamble', 'orchestrator', 'supply-chain', 'ci-and-secrets', 'application-security']) { + for (const name of ['_preamble', 'orchestrator', 'supply-chain', 'ci-and-secrets', 'application-security', 'hosted']) { copyFileSync(join(repo, `.github/audit/${name}.md`), join(dir, `.github/audit/${name}.md`)); } stub(dir, 'claude', ` @@ -297,6 +297,17 @@ test('orchestrator wait: fragments still being written keep the wait going', (t) assert.deepEqual(lines.slice(0, -1), [`${first}: finished`, ...rest.map((f) => `${f}: still writing`)]); }); +// One domain still writing keeps the wait going, whichever one it is. The +// wait block's per-domain status lines are pinned to `AUDIT_FRAGMENTS`; its +// `until` predicate is not, so a fragment dropped from that predicate would +// let the orchestrator merge and publish while that domain was still writing. +for (const held of fragments) { + test(`orchestrator wait: ${held} alone unfinished keeps the wait going`, (t) => { + const { answer } = runWait(t, { finished: fragments.filter((f) => f !== held), writing: [held] }); + assert.equal(answer, 'STILL WAITING'); + }); +} + test('orchestrator wait: a re-issued call reads back the persisted deadline', (t) => { const deadline = now() + 600; const { answer, persisted } = runWait(t, { deadline }); @@ -323,6 +334,7 @@ test('merge distinguishes finished, cut-off, and absent domains', (t) => { // exact `tail -n1` this finished domain is published under the cut-off caveat. writeFileSync(join(dir, 'audit-supply-chain.md'), `VERDICT: PASS\nsupply evidence\n\n${SENTINEL}\n\n`); writeFileSync(join(dir, 'audit-ci-secrets.md'), 'VERDICT: INCONCLUSIVE\nci evidence\n'); + writeFileSync(join(dir, 'audit-hosted.md'), `VERDICT: PASS\nhosted evidence\n${SENTINEL}\n`); const result = spawnSync('bash', ['-c', merge], { cwd: dir, encoding: 'utf8' }); assert.equal(result.status, 0, result.stderr); const report = readFileSync(join(dir, 'audit-report.md'), 'utf8'); @@ -335,4 +347,7 @@ test('merge distinguishes finished, cut-off, and absent domains', (t) => { assert.equal(report.match(/_Incomplete —/g).length, 1); // A domain that never wrote anything is neither. assert.match(report, /## Application security\n\n_No report —/); + // The fourth domain is emitted too — a heading dropped from `emit` would + // silently publish a report missing a domain that did report. + assert.match(report, /## Hosted accounts\n\nVERDICT: PASS\nhosted evidence/); });