security: close the four audit blockers, verify what the release signs - #16
Merged
Conversation
jsonValue.raw copied each value's source span as the parser descended, so the cost was quadratic in nesting depth: a 60 KB policy file expanded to 962 MB of retained slices before anything validated it. A policy file is operator-supplied but it is also the artifact a fleet job reads on every run, so "malformed input exhausts the host" is reachable without an attacker. Values are now sliced lazily, with a depth cap of 64 and a 1 MB size cap. The same 60 KB file peaks at 5.8 MB. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four changes, in the same files because they touch the same two commands.
Containment (blocker 1). apply.Apply resolves symlinks and rewrites the target,
so a committed `.github/CODEOWNERS -> ../../outside/f` made `sync` and `apply`
edit a file outside the clone and report "applied", exit 0. In the fleet model
these verbs exist for, that symlink travels IN the repository: anyone who can
push to one repo chooses a path the central runner writes to. It is also wrong
with no attacker — GitHub does not follow a symlinked CODEOWNERS, so the run
edited a file that governs nothing while reporting success. Contained in the CLI
layer, which is the layer that knows --repo; in-repo symlinks still work.
`plan` is contained too. It writes no CODEOWNERS, so this is not an escape — but
it emitted a reviewable artifact whose codeowners_path pointed outside the clone,
and every downstream refusal fires after that review. It now refuses and writes
no artifact.
Token redaction (blocker 2). `fs.String("token", os.Getenv("GITHUB_TOKEN"), ...)`
made the live PAT the flag's default, and Go's flag package prints non-empty
defaults in its usage text — so `audit --help`, and far worse any mistyped flag,
wrote the token to stderr where CI captures it (CWE-532). $GITHUB_TOKEN is still
the documented fallback, just resolved after Parse.
Ref guard. --branch reaches git as a positional argument, so a dash-leading
value was parsed as an option: `--branch '--format=…'` answers "--format can't be
combined with other format-altering options", which is ls-tree's option parser.
A refname cannot begin with a dash (git-check-ref-format) and no revision
expression does either, so rejecting it costs nothing legitimate.
Note on --end-of-options: it is used on cat-file and rev-parse, but NOT on
ls-tree, where it makes the trailing `--` a pathspec rather than a separator —
the tree then comes back empty and every scope resolves against a repository
that appears to have no files. A silent wrong answer is worse than the problem.
Also documents --out/--summary-out as trusted operator paths in their flag help:
unlike --file and the discovered CODEOWNERS path, no repository can influence
them, and containing them would break the fleet uses they exist for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Paths were built by concatenation from values that came out of a CODEOWNERS file or off the command line. The impact is wrong answers rather than compromise — the client never writes and the token is the caller's own — but "the audit said it was fine" is the entire product. The owner grammar permits a dot, so `@org/..` is a LEGAL owner and reached TeamExists as the slug "..". That produced GET /orgs/org/teams/.., which any normalizing proxy — Go's own ServeMux included — rewrites to GET /orgs/org. That answers 200 for any visible org, so the client concluded the team exists and A-1 raised nothing about an owner that can never be granted review. url.PathEscape closes the slash and space cases but leaves "." and ".." alone, since both are legal path characters; a dot-only segment is therefore encoded by hand, which turns the traversal into a plain 404 — the correct definitive negative for a team that does not exist. The ref was interpolated raw into the query string, so a ref containing `&` appended parameters the caller never wrote to an endpoint whose parameters govern what comes back, and an ordinary ref containing a space failed to parse as a URL at all — an inconclusive result for a branch that exists. Now url.Values. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…p job Supply chain (blocker 4). All nine `uses:` are pinned to 40-character commit SHAs — a moved tag runs new code inside a job that publishes artifacts and holds a tap token. The release now produces a build-provenance attestation signed with a short-lived OIDC identity (id-token: write), which answers "was this built HERE by THIS workflow" — a question no hash on the release can answer, because checksums.txt ships on the same release from the same host. Attestation runs BEFORE the release is created: a failure should mean no release exists, not a published release nobody can verify. Builds also stamp -X so `version` reports the build it is rather than "dev". Least privilege. The homebrew job inherited the workflow-level contents: write while doing nothing to this repository but reading tools/gen-formula.sh — it pushes to the tap with its own PAT. A job-level permissions block REPLACES the workflow-level one rather than adding to it, which is why declaring one is the only way to drop a permission, and why the release job restates contents: write. The tap token no longer goes in the clone URL. It was written in cleartext to .git/config on the runner and would appear in any git error echoing the remote; Actions masks secrets in its own log lines, but the value has been through a shell and neither git's config nor a log shipper is masked. It now goes through gh's credential helper, and the checkout uses persist-credentials: false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The release signs an attestation and nothing read it, which buys very little. install.sh checked only the SHA-256 against checksums.txt — and checksums.txt lives on the same release, fetched over the same channel from the same host as the archive it covers. That is integrity in transit and nothing about ORIGIN: whoever can write the release rewrites both in one step and "Checksum OK." still prints. Homebrew inherits it, since gen-formula.sh reads its sha256 values from that same file. gh is deliberately NOT made a prerequisite. The reason this script exists is `curl | sh` on a machine with nothing on it — CI images, minimal containers, a fresh laptop — and gh is absent from most of them. Requiring it would not make those installs verified, it would make them fail, and the realistic response to a failing install is a hand-downloaded tarball, which is verified less than before. PROVENANCE=auto verify when gh can; warn loudly when it cannot (default) PROVENANCE=require no verification, no install — for CI and managed fleets PROVENANCE=skip do not attempt it A verification that RUNS and FAILS is fatal in every mode. Only the inability to run one degrades to a warning, and the two are kept apart by checking for gh and for an authenticated session up front rather than by reading intent out of gh's exit code — it exits nonzero for a forged attestation, a missing one, and an unknown flag alike. The one branch taken on the error text is for a gh too old to know --signer-workflow, which is a missing client feature rather than a statement about the artifact, and degrades to the check that still pins the repository. README documents the command for the direct-download path, which bypasses this script entirely, and is explicit that Homebrew inherits the weaker guarantee. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 500k-case differential fuzz against the vendored, unmodified hmarr/codeowners oracle is the strongest correctness evidence in the repository — the matcher is a port of that implementation and this is what catches porting drift byte-for-byte — and it was reachable only as `make diff-test` on someone's laptop, which means in practice it ran when somebody remembered. It runs the FULL 500k on every PR, not a reduced count: measured at 35 seconds, in its own parallel job, so there was no case for weakening the gate. Same count and same seed as `make diff-test`, so a green run locally is a green gate here. difftest takes an optional seed. The seed stays fixed in CI on purpose — a gate that draws different inputs each run fails on PRs that changed nothing relevant, and a gate people re-run until it goes green is not a gate. The cost is that it covers one fixed region of the input space forever, and raising the case count only extends the same sequence; a different seed is the only way to reach input the gate structurally cannot, which is worth doing when editing the matcher. The seed prints with the result so anything found replays exactly. CI's own actions are SHA-pinned to match release.yml. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…endabot The procedural gaps that stall enterprise intake reviews. SECURITY.md routes reports to GitHub private vulnerability reporting rather than a shared mailbox, states best-effort response times as best-effort, and is explicit about what is in scope — a write that leaves the repository, a "wrong but confident" answer, a credential in an output — and what is deliberate, namely that --out/--summary-out/--plan are trusted operator paths. CODEOWNERS: a CODEOWNERS tool whose own repository has none fails its own A-11 check. One rule, deliberately — narrower rules would name the same single maintainer, and each would risk an A-4 (matches zero files) or A-6 (fully shadowed) finding against its own audit. dependabot.yml covers github-actions only, and says why there is no gomod entry: this module has zero dependencies and no go.sum by policy, and an entry whose only possible action is to touch go.mod is a standing invitation to land the first dependency without anyone deciding to. It exists to keep the SHA pins from going stale — a pin never picks up an upstream fix on its own. LICENSE names the copyright holder in full; OSS legal review flags a first name alone as ambiguous. NOTICE, which carries the hmarr/codeowners attribution, is untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Generated by `make docs` from the test doc comments, now covering the containment, token-redaction, ref-guard, URL-escaping and parser-bound tests added here. CI gates this file with `git diff --exit-code`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses the enterprise security audit’s four blockers and related follow-ups by hardening the CLI’s filesystem containment, preventing token leakage via flag defaults, bounding policy JSON parsing to avoid pathological resource usage, and tightening release/provenance integrity (workflow pinning, attestations, and verification).
Changes:
- Add containment/ref-guard rails to prevent repo-controlled inputs (symlinks, dash-leading refs) from causing writes outside
--repoor reaching git’s option parser. - Prevent credential leakage by deferring
$GITHUB_TOKENresolution until after flag parsing; introduce aversioncommand with release-time-Xstamping. - Strengthen supply-chain integrity by SHA-pinning GitHub Actions, producing/using build provenance attestations, and verifying provenance in
install.sh(with a definedPROVENANCE=policy).
Reviewed changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/supplychain/supplychain_test.go | Adds workflow-level supply-chain gates (action pinning, attestation markers, OIDC permission, version stamping). |
| tools/supplychain/permissions_test.go | Adds least-privilege and credential-in-URL workflow gates, plus workflow job parsing. |
| tools/supplychain/install_verify_test.go | Adds tests ensuring install/docs verify provenance (and handle missing gh). |
| tools/difftest/main.go | Adds optional seed argument and prints it for reproducible fuzz runs. |
| SECURITY.md | Adds a repository security policy and provenance verification guidance. |
| README.md | Documents provenance verification and `PROVENANCE=auto |
| Makefile | Clarifies difftest reproducibility and how to vary seed. |
| LICENSE | Updates copyright holder name. |
| internal/policy/policy.go | Adds size cap for policy loads to prevent large-file OOMs. |
| internal/policy/jsonsrc.go | Removes quadratic raw-span copying; adds depth cap and located “too deep” error. |
| internal/policy/bounds_test.go | Adds tests guarding depth/size/linear allocation, and positive controls. |
| internal/gittree/refguard_test.go | Adds tests for rejecting dash-leading refs and ensuring normal refs still work. |
| internal/gittree/gittree.go | Adds ValidateRef and applies it to git calls; uses --end-of-options where safe. |
| internal/ghapi/urlescape_test.go | Adds tests ensuring path/query construction can’t traverse/expand segments or inject query params. |
| internal/ghapi/ghapi.go | Escapes path segments and query params to prevent normalization-based false positives. |
| internal/cli/token_redaction_test.go | Adds regression tests ensuring tokens never print but still authenticate. |
| internal/cli/sync.go | Documents trusted operator paths; validates refs early; adds extra containment pre-check. |
| internal/cli/containment_test.go | Adds end-to-end tests preventing CODEOWNERS symlink escapes and artifact lies. |
| internal/cli/cli.go | Adds Version stamping and version/--version command; adds write-path containment helpers. |
| install.sh | Verifies build provenance via gh attestation verify with PROVENANCE= modes and clear failure semantics. |
| docs/BEHAVIOR.md | Regenerates behavior docs to reflect new/updated tests. |
| CONTRIBUTING.md | Adds contribution constraints and workflow/test guidance. |
| CHANGELOG.md | Adds changelog entries describing the security/supply-chain changes. |
| .github/workflows/release.yml | SHA-pins actions, adds provenance attestation + OIDC perms, stamps version, reduces homebrew job privileges, removes token-in-URL. |
| .github/workflows/ci.yml | SHA-pins actions; adds difftest job running full 500k cases. |
| .github/dependabot.yml | Enables Dependabot for SHA-pinned GitHub Actions updates only. |
| .github/CODEOWNERS | Adds repo ownership to satisfy the tool’s own audit expectations. |
Suppressed comments (2)
tools/supplychain/permissions_test.go:95
- This test reads workflowFiles(t)["release.yml"] inline without checking that the file exists. If release.yml is renamed/missing, splitJobs will parse an empty string and the resulting error will be misleading. Resolve release.yml once with an ok-check, then pass the body to splitJobs.
func TestSupplyChain_HomebrewJobCannotWriteThisRepository(t *testing.T) {
jobs := splitJobs(t, workflowFiles(t)["release.yml"])
block, ok := jobs["homebrew"]
if !ok {
t.Skipf("no `homebrew` job in release.yml; jobs present: %v", jobNames(jobs))
tools/supplychain/supplychain_test.go:146
- This test indexes workflowFiles(t)["release.yml"] directly, which can lead to misleading results if the workflow file is renamed/missing (the empty string will fail the assertion, but without a clear diagnostic about the missing file). Mirror the explicit presence check used earlier in this file so a missing/renamed workflow is reported clearly.
func TestSupplyChain_ReleaseStampsAVersionIntoTheBinary(t *testing.T) {
body := workflowFiles(t)["release.yml"]
if !strings.Contains(body, "-X ") && !strings.Contains(body, "-X\t") {
t.Errorf("release.yml builds with -ldflags but never stamps a version via -X.\nWithout it `codeowners-tool version` cannot report the build, and an operator holding a binary cannot tell whether it is the one with a given fix.")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+134
to
+135
| body := workflowFiles(t)["release.yml"] | ||
| if !strings.Contains(body, "id-token") { |
| // Least privilege has to be stated per job, because inheritance here defaults to | ||
| // the most privileged thing in the file. | ||
| func TestSupplyChain_EveryReleaseJobDeclaresItsOwnPermissions(t *testing.T) { | ||
| body := workflowFiles(t)["release.yml"] |
Raised in review. Several gates indexed workflowFiles(t)["release.yml"] directly,
so a renamed or missing workflow left them asserting against "" — each failing
with a message about the property it could not find ("release.yml declares no
id-token: write") rather than the file it never read.
No false pass and no panic: a string-map miss yields "", and every affected
assertion still fails. It is a diagnostics problem, and one this file had already
solved once — TestSupplyChain_ReleaseIsSignedOrAttested did the ok-check inline.
Lifted that into releaseWorkflow(t) and routed the rest through it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
No behavior change; comments, docs and test prose only. The explanations added with this branch had drifted well past the point of being read — the rule in CONTRIBUTING.md is a named scenario and its consequence, not an essay on it. Roughly a third shorter across the board (-173 lines outside the generated docs). Each block keeps the concrete failure it exists to record and loses the restatement around it: ValidateRef no longer spends a paragraph on what git's option parser is, install.sh states the gh trade in one paragraph instead of three, and SECURITY.md/CONTRIBUTING.md/CHANGELOG.md drop the throat-clearing. docs/BEHAVIOR.md is regenerated, since it is built from the test doc comments. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last pass shortened the prose and kept its shape, which missed the point: .github/CODEOWNERS carried seven lines of comment above one line of content, and most files were the same trade. The default was wrong, not the length. Default is now no comment. One stays only where a reader would otherwise get it wrong — that ls-tree's trailing `--` makes --end-of-options unusable, that url.PathEscape leaves ".." alone, that a permissions check on the homebrew job passes vacuously because the job declares nothing. The rationale that belongs in a commit message or CHANGELOG has been removed from the source. Comments, docs and test prose only; no behavior change. BEHAVIOR.md regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the four blockers from the enterprise security audit, plus the second-tier
findings and the procedural gaps that stall intake review.
Eight commits, each self-contained and separately reviewable. Two of them
(
security(cli)andsecurity(release)) carry both an original blocker fix and arelated follow-up, because they land in the same files — the commit messages say
which is which.
The four blockers
.github/CODEOWNERS -> ../../outside/fmadesync/applywrite outside the clone and reportapplied, exit 0--repo. In-repo symlinks still workos.Getenv("GITHUB_TOKEN")as a flag default meant Go's flag package printed the live PAT on--helpand on any flag error (CWE-532)Parse;--tokenstill winsjsonValue.rawcopied each value's source span — quadratic in nesting, 60 KB → 962 MBcontents: writeand a tap token; nothing signeduses:SHA-pinned, build-provenance attestation,-Xversion stamp,versionsubcommandBeyond the blockers
little —
install.shonly checked SHA-256 againstchecksums.txt, which shipson the same release from the same host, so it proved integrity in transit and
nothing about origin.
planis contained. It writes nothing, so this was not an escape — but itproduced a reviewable artifact whose
codeowners_pathpointed outside therepo, and every downstream refusal fires after that review.
contents: write(it inherited it by declaringnothing) and no longer embeds the tap token in a clone URL.
--branch '--format=…'reached git's option parser.@org/..is a legal owner andproduced
GET /orgs/org/teams/.., which normalizes toGET /orgs/org,answering 200 and reporting a nonexistent team as valid.
Two decisions worth your attention
ghis not an install prerequisite. The point ofcurl | shis a machinewith nothing on it, and
ghis absent from most base images. Requiring it wouldnot make those installs verified — it would make them fail, and the realistic
response is a hand-downloaded tarball, verified less than before. So
PROVENANCE=autoverifies when it can and warns loudly when it cannot;requirerefuses to install without verification;skipopts out. Averification that runs and fails is fatal in every mode.
--end-of-optionsis deliberately NOT used onls-tree. Verified againstgit 2.39.5:
It turns the trailing
--into a pathspec, so the tree returns empty and everyscope silently resolves against a repo that appears to have no files. A silent
wrong answer is worse than the problem. The ref guard closes it instead;
--end-of-optionsis used oncat-fileandrev-parse, where the grammarleaves the separator alone.
Verification
make vetgo test -race ./...actionlint/shellcheckmake docsgo.modis still dependency-free and nogo.sumexists. No test was weakened,skipped, or deleted.
Three of the new tests initially passed vacuously and were tightened rather
than trusted: the homebrew-permissions test (the job held
contents: writeprecisely because it declared nothing, so the literal was not there to grep
for), the
ReadFileAtRefref-guard test (git errored on the bogus objectanyway), and the credential-URL detector (which failed on its own remediation
comment).
Not fixed — flagged for a decision
auditandsnapshotread a symlink's target string as CODEOWNERScontent. They read via
git cat-file blob <ref>:<path>, and for a symlinktree entry the blob content is the link text. No escape — nothing outside is
opened — but where
plan/sync/applynow refuse, these two silentlyanalyze garbage. They arguably deserve the same refusal.
gh attestation verifyaborts the install inautomode, since a verification that runs and fails is fatal. Deliberate andconsistent with the fail-closed R-12 posture, but it is a behavior change on a
flaky network.
gh auth login --with-tokenwrites the PAT to~/.config/gh/hosts.ymlonthe runner. Ephemeral, standard, and strictly better than
.git/config— butstill a token at rest.
and setting one up is your call, not something to do unasked.
Monorepo performance (O(rules × files), 9.3s on 20k files × 500 rules) was left
alone as out of scope — a scalability finding, not a security one.
🤖 Generated with Claude Code