Skip to content

Promote develop to main: audit the deploy's environment scope - #23

Merged
ptr727 merged 3 commits into
mainfrom
develop
Aug 4, 2026
Merged

Promote develop to main: audit the deploy's environment scope#23
ptr727 merged 3 commits into
mainfrom
develop

Conversation

@ptr727

@ptr727 ptr727 commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Promotes one commit. Merge commit, per the main ruleset.

What lands

The secrets audit read repository scope only. That is correct for the merge bot's credentials and blind to the deploy's, which are environment-scoped, so an entirely unconfigured deploy passed it. This is the only repo in the fleet where that gap exists.

Both scopes are now asserted, and the third assertion is the one presence-checking misses: a scoped credential must be absent from an environment that does not list it. production holding a Pangolin access token is a finding rather than a harmless extra, because production answers unauthenticated and a token there means a check could pass through a gate production is not supposed to have.

Findings from writing and reviewing it

  • spec/secrets.json declared a deploy root that no environment carries and no workflow reads, since the rsync destination is anchored at the key's confinement root. A declared-but-unread name is drift an audit cannot tell from a missing one.
  • The first version of the check used a jq path that had moved, iterated zero times, and reported clean. Review then caught that the guard was written in a for header, where command substitution discards the exit status, so it never fired. Both lookups are assigned before they are iterated.
  • The bare variable listing prints every value, so the runbook as first written would have put the deploy endpoint into its own output. Names are requested explicitly and the text says why.

Verification

The documented commands run verbatim against the live configuration: staging carries 3 secrets and 4 variables, production 1 and 4, and the staging-only pair is correctly absent from production.

Separately, the deploy pipeline this audit covers has now run for real: a staging deploy dispatched from main built, uploaded, flipped, and verified release 20260804-231050, passing the full 1,245-URL contract against the live site.

🤖 Generated with Claude Code

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

* 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) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 4, 2026 23:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the repository’s audit documentation/spec to explicitly verify GitHub Actions secrets/variables at both repository scope (merge bot) and environment scope (deploy), closing a gap where an unconfigured deploy could pass a repository-only secrets audit.

Changes:

  • Document separate repository-scope vs environment-scope secrets/variables verification in AUDIT.md, including safe gh commands that list names only.
  • Refine spec/secrets.json environment variable/secret declarations (notably removing DEPLOY_ROOT from environment variables and documenting why).
  • Add an additional “empty query reads as clean” pitfall reminder to TODO.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
TODO.md Adds a reminder about empty-result queries falsely appearing as “clean.”
spec/secrets.json Updates environment-scoped declarations/notes (removes DEPLOY_ROOT, adds rationale).
AUDIT.md Documents auditing secrets across both repository and environment scopes with safer CLI patterns.

Comment thread spec/secrets.json
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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 23:42

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

Suppressed comments (1)

AUDIT.md:65

  • The note about gh variable list is factually misleading: by default gh variable list does not print variable values (values are exposed via JSON selection like --json value). Keeping the warning accurate matters because this runbook is meant to be executed verbatim and trusted.

Suggestion: keep the safety guidance but rephrase it as “use --json name for machine-readable output and to guarantee only names are emitted; do not request the value field (or use gh variable view) in audit output.”

`--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.

@ptr727

ptr727 commented Aug 4, 2026

Copy link
Copy Markdown
Owner Author

Declining the premise, taking half the suggestion. Wording sharpened in 2d3aee4.

The bare listing does print values, in both modes. Measured on gh 2.46.0, values redacted here:

piped:   DEPLOY_SSH_HOST<TAB><full value><TAB>2026-08-04T19:04:42Z
terminal: NAME                    VALUE                     UPDATED
          DEPLOY_SSH_HOST         <value>                   about 4 hours ago

The terminal form renders a VALUE column and truncates long entries with an ellipsis. The piped form emits the value in full, untruncated — and piped is the form an audit run actually produces, since its output is captured rather than watched. So the guidance holds, and the case it guards is the more exposed of the two rather than the less.

Your second half is a real improvement and is now in the text: do not request the value field, and do not use gh variable view, which prints one by design. --json name alone would not have stopped someone reaching for --json name,value to make the output "more useful".

I have also made the wording say which mode does what, because the original sentence described the captured form while reading like a claim about the terminal form. That ambiguity is what made the finding reasonable to raise.

Worth stating plainly: this runbook is meant to be executed verbatim and trusted, which is exactly why I measured rather than reasoned about it.

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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings August 4, 2026 23:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@ptr727
ptr727 merged commit 26649fc into main Aug 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants