From 496c0df26a5d42ca5d46f203d33a3f953a0bd169 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 4 Aug 2026 16:34:07 -0700 Subject: [PATCH 1/3] Audit the environment scope the deploy credentials live in (#22) * Audit the environment scope the deploy credentials live in The secrets audit read repository scope alone, which is where the merge bot's credentials live and not where the deploy's do, so an unconfigured deploy passed it. It now asserts both, including that a scoped credential is absent from the environment that must not carry it. The deploy root is no longer declared, since the rsync destination is anchored at the key's confinement root and a declared-but-unread name is drift an audit cannot tell from a missing one. Co-Authored-By: Claude Opus 5 (1M context) * Guard the lookup that drives the loop, and correct the manifest note A guard written in a for header never fires, because command substitution discards the exit status, so the lookup is assigned before it is iterated. The manifest note described the environment split as carrying a deploy root the manifest no longer declares. Co-Authored-By: Claude Opus 5 (1M context) * List environment credentials by name through the tool that reaches them The section claimed a listing could not reach environment scope, which it can with an environment flag, and the raw API calls read one page. The listing prints every variable's value unless names are requested, so an audit run wrote the deploy endpoint into its own output. Co-Authored-By: Claude Opus 5 (1M context) --------- Co-authored-by: Claude Opus 5 (1M context) --- AUDIT.md | 33 ++++++++++++++++++++++++++++++++- TODO.md | 1 + spec/secrets.json | 6 +++--- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/AUDIT.md b/AUDIT.md index c552e22..ad9286c 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -36,6 +36,10 @@ Two facts specific to this repo: Names only. Never read, print, or log a secret value. +Two scopes, checked separately, because a name present in one is not present in the other. The **repository** scope carries the merge bot's credentials, and the **environment** scope carries the deploy's. This is the only repo in the fleet whose publishing credentials are environment-scoped, so a check written for repository secrets alone reports a clean pass over an unconfigured deploy. + +### Repository scope + ```sh gh secret list --repo ptr727/Blog gh secret list --repo ptr727/Blog --app dependabot @@ -43,7 +47,34 @@ gh secret list --repo ptr727/Blog --app dependabot Assert that every name under `baseline.requires` is present in both stores, and that every name under `baseline.forbids` is absent. `CODEGEN_APP_ID` is forbidden: the App-token action takes `client-id`, and the deprecated `app-id` name silently does nothing. -The deploy credentials live in the `staging` and `production` GitHub Environments rather than in repository secrets, so a staging deploy cannot reach production. They are outside the baseline audit. +### Environment scope + +`configure.sh check` does not reach these and says so, deferring the secrets question to a manual verification. Assert them against `spec/secrets.json`: + +```sh +# Read the lists first, so a moved key fails here rather than emptying the loop below. +envs=$(jq -e -r '.environments.names[]' spec/secrets.json) || exit 1 +jq -e '.environments.environmentSecrets' spec/secrets.json > /dev/null || exit 1 + +for env in $envs; do + gh secret list --repo ptr727/Blog --env "$env" --json name --jq '.[].name' + gh variable list --repo ptr727/Blog --env "$env" --json name --jq '.[].name' +done +``` + +`--json name` is not decoration. The bare `gh variable list` prints every value, so an audit run without it writes the deploy host, the user, and the known-hosts entry into whatever captured its output. + +**Assert the query matched before reading what it returned.** A `jq` path that no longer resolves yields nothing, a loop over nothing runs zero times, and a check that counts failures reports none. Every lookup is `jq -e`, which exits non-zero on a null or missing key, and each is **assigned before it is iterated**: command substitution in a `for` header discards the exit status, so a guard written there is a guard that never fires. + +Three assertions, and the third is the one presence-checking misses: + +- Every name under `environments.secrets` and `environments.variables` is present in **every** environment named in `environments.names`. +- Every name under `environmentSecrets.` is present in that environment. +- A name under `environmentSecrets` is **absent** from an environment that does not list it. `production` holding a Pangolin access token is a finding rather than a harmless extra: production answers unauthenticated, so a token there means a check could pass through a gate production is not supposed to have. + +A **declared but unset** name is drift in the same way an undeclared one is. The deploy root is deliberately not declared, because the rsync destination is anchored at the deploy key's confinement root and the workflow names an environment rather than a host path. + +Never read, print, or log a value. Every command above lists names. ## 3. The URL Contract diff --git a/TODO.md b/TODO.md index 6c87102..bc3d9a2 100644 --- a/TODO.md +++ b/TODO.md @@ -99,6 +99,7 @@ Each of these was hit or nearly hit, and each is cheap to re-trip. - **Do not name any workflow `build-*-task.yml`** while the repo declares `source-only`, since `detect` is literally `["no build-*-task.yml"]`. - **Do not edit `.markdownlint-cli2.jsonc`, `repo-config/configure.sh`, or the two ruleset payloads.** They are carried verbatim and byte-matched against the hub. Scope a glob in the workflow instead. A reviewer finding a real defect in one of them is answered by declining locally and filing it at the hub, never by editing the file to satisfy the review. - **The hub's `main` can promote while a convergence pull request is open**, so ground truth moves underneath work that was correct when it started. It happened twice in one session on 2026-08-03, and the second time added drift the branch could not have known about. Re-run the audit against the hub ref actually carried before claiming convergence, and name that ref in the change, or the claim ages into a false one. +- **A query that matches nothing reads as a clean result.** It has cost three separate false passes: a review-thread poll that could not see suppressed findings, a reviewer filter written in the wrong API's login form, and an audit loop whose `jq` path had moved. Each returned empty, and empty looked like nothing to report. Assert the query matched before reading what it returned, which is what `jq -e` and a non-empty check are for. - **The hub authors `scripts/pr_review.py`, and hand-rolling the review loop re-discovers its bugs.** One `status` call reports rounds, head coverage, unresolved threads, suppressed findings across every round, and whether a request was ever picked up. `wait` runs the backoff in-process, so a review wait costs one turn rather than one per poll. It is read-only by design and the mutations stay explicit, so fetch and run it rather than reimplementing it. Its README documents the traps below as the reason it exists. - **A review request can sit forever without being picked up, which looks exactly like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting; a request that never draws one is not slow, it is inert, and elapsed time cannot tell them apart. The event is REST-only. Recover by clearing the request with `union: false` and an empty `botIds`, then requesting again, after reading the pending set so a human reviewer is not dropped. - **The Copilot reviewer's login differs by API, and a wrong-form filter reads as a clean review.** REST reports `copilot-pull-request-reviewer[bot]`, GraphQL omits the suffix. A filter written in the other form matches nothing, and an empty result is indistinguishable from no findings. Assert the filter matched before trusting what it returned. diff --git a/spec/secrets.json b/spec/secrets.json index af7078c..69d028c 100644 --- a/spec/secrets.json +++ b/spec/secrets.json @@ -18,7 +18,7 @@ "note": "The App-token secrets power the App-signed merge-bot (auto-merge that re-triggers downstream workflows), which every fleet repo runs. Used via actions/create-github-app-token with the client-id input (not the deprecated app-id). The CODEGEN_* name is historical, not codegen-specific." }, "environments": { - "note": "Not part of the baseline audit. One key covers both environments, a deliberate decision recorded in OPERATIONS.md: the per-environment split only pays where the two keys never share a machine, and both sit on one workstation and in one secret store. The environment split still carries the deploy root, the base URL, and the staging-only access token, so it is not decorative.", + "note": "Not part of the baseline audit. One key covers both environments, a deliberate decision recorded in OPERATIONS.md: the per-environment split only pays where the two keys never share a machine, and both sit on one workstation and in one secret store. The split still carries the base URL, the SSH endpoint, and the staging-only access token, so it is not decorative.", "names": [ "staging", "production" @@ -30,7 +30,6 @@ "DEPLOY_SSH_HOST", "DEPLOY_SSH_USER", "DEPLOY_SSH_KNOWN_HOSTS", - "DEPLOY_ROOT", "HUGO_BASEURL" ], "secretsNote": "The 'secrets' and 'variables' lists are required in every environment named above. 'environmentSecrets' names what one environment carries and another does not, so a name audit does not read a staging-only credential as missing from production. Staging keeps its auth gate on and production answers unauthenticated, so the access token exists on staging alone and checks/check-live-urls.sh sends no credential where the pair is absent.", @@ -40,6 +39,7 @@ "PANGOLIN_ACCESS_TOKEN" ], "production": [] - } + }, + "variablesNote": "The deploy root is deliberately absent. The rsync destination is anchored at the deploy key's confinement root, so the workflow names the environment rather than a host path, and a declared-but-unread name is drift an audit cannot distinguish from a missing one." } } From 53fb912aa355232c35e28334ac9671eb6138ca1a Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 4 Aug 2026 16:42:11 -0700 Subject: [PATCH 2/3] Drop the deploy root from the environment variables table (#24) The table listed a name no environment carries and no workflow reads, which left the operator-facing list disagreeing with the manifest about whether it should exist. Co-authored-by: Claude Opus 5 (1M context) --- TODO.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/TODO.md b/TODO.md index bc3d9a2..c78ba00 100644 --- a/TODO.md +++ b/TODO.md @@ -126,11 +126,13 @@ Secrets and variables, per environment. The App-token pair is repository-scoped | --- | --- | | `DEPLOY_SSH_PRIVATE_KEY` | secret | | `DEPLOY_SSH_HOST`, `DEPLOY_SSH_USER`, `DEPLOY_SSH_KNOWN_HOSTS` | variable | -| `DEPLOY_ROOT`, `HUGO_BASEURL` | variable | +| `HUGO_BASEURL` | variable | | `PANGOLIN_ACCESS_TOKEN_ID`, `PANGOLIN_ACCESS_TOKEN` | secret, staging only | | `CODEGEN_APP_CLIENT_ID`, `CODEGEN_APP_PRIVATE_KEY` | secret, both stores | -`DEPLOY_SSH_PRIVATE_KEY` now holds the same key in both environments, per the decision above. The environment split still carries the deploy root, the base URL, and the staging-only token pair, so it is not decorative. +`DEPLOY_SSH_PRIVATE_KEY` holds the same key in both environments, per the decision above. The environment split still carries the base URL, the SSH endpoint, and the staging-only token pair, so it is not decorative. + +The deploy root is deliberately absent from this table. The rsync destination is anchored at the deploy key's confinement root, so the workflow names an environment rather than a host path, and a declared-but-unread name is drift no audit can tell from a missing one. The local `DEPLOY_ROOT` in `secrets/.env` is a different value and is still read. From c51e0253d8fd94a1dac9d47d76fd5307d7da90f9 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Tue, 4 Aug 2026 16:48:17 -0700 Subject: [PATCH 3/3] Name both output modes of the variable listing (#25) The warning described the captured form, where a value prints in full, and read as though it described the terminal form, where the column is truncated. Both emit values, and the captured one is what an audit keeps. Co-authored-by: Claude Opus 5 (1M context) --- AUDIT.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AUDIT.md b/AUDIT.md index ad9286c..cbf0778 100644 --- a/AUDIT.md +++ b/AUDIT.md @@ -62,7 +62,7 @@ for env in $envs; do done ``` -`--json name` is not decoration. The bare `gh variable list` prints every value, so an audit run without it writes the deploy host, the user, and the known-hosts entry into whatever captured its output. +`--json name` is not decoration. The bare `gh variable list` prints a value column, and when its output is captured rather than shown it prints each value in full, so an audit run without it writes the deploy endpoint into its own log. Never request the `value` field here, and never use `gh variable view`, which prints one by design. **Assert the query matched before reading what it returned.** A `jq` path that no longer resolves yields nothing, a loop over nothing runs zero times, and a check that counts failures reports none. Every lookup is `jq -e`, which exits non-zero on a null or missing key, and each is **assigned before it is iterated**: command substitution in a `for` header discards the exit status, so a guard written there is a guard that never fires.