diff --git a/.github/dependabot.yml b/.github/dependabot.yml index a1ed2c7..27b2d8c 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -28,3 +28,21 @@ updates: directory: "/" schedule: interval: "weekly" + + # NO LEFTHOOK ENTRY, AND THAT IS AN EXCLUSION RATHER THAN AN OVERSIGHT. + # + # A lefthook consumer pins exactly one version -- the `ref:` under `remotes:` + # in their lefthook.yml, which README.md's install block tells them to write. + # Dependabot has no ecosystem that reads that file, so no updater will ever + # raise a pull request for it: the `pre-commit` entry above covers + # `.pre-commit-config.yaml` and nothing else. Adding a `github-actions` or + # `gomod` entry would not reach it either, and listing one here to look + # covered is the failure this repository exists to catch. + # + # What does watch it is `no-stale-hook-pins`, which reads lefthook `remotes:` + # entries as pins alongside pre-commit `repo:`/`rev:` pairs and refuses one + # that has fallen behind its upstream or names no `ref:` at all. So the pin is + # watched, by a guard rather than by an updater -- meaning a lefthook consumer + # is told their pin is stale and is never handed the bump. That difference is + # stated in README.md so a consumer reads it before they need it, and it is + # the reason to prefer a manager Dependabot can see where there is a choice. diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 33ee33f..250422b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -283,8 +283,17 @@ jobs: - if: matrix.tool == 'pre-commit' || matrix.tool == 'prek' run: pipx install "${{ matrix.tool }}" - - if: matrix.tool == 'lefthook' - uses: actions/setup-go@v7 + # Go on every leg, and it is not the lefthook binary's toolchain -- that + # is the step below. The consumer harness's ninth question drives the four + # published Go ids, which are `language: system` and use whatever `go` the + # consumer has, so all three runners need one to drive them with. + # + # This does not weaken the gate above. What that gate protects is the "no + # Rust toolchain needed" claim, and Rust is still absent from the two legs + # whose runners are supposed to bootstrap it themselves. A Go toolchain + # proves nothing about a Rust bootstrap in either direction, and the four + # ids it is here for run no uphold code at all. + - uses: actions/setup-go@v7 with: go-version: '>=1.26' diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index 49c9640..7601318 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -7,12 +7,20 @@ # the claim true. lefthook has no equivalent format; it consumes `lefthook.yml` # from a remote repository instead, which is what `hooks/lefthook.yml` is for. # -# `language: rust` throughout, and it is not a toolchain requirement being -# pushed onto the consumer: both runners bootstrap Rust themselves when it is -# absent. All of these hooks build ONE environment, because pre-commit and prek -# key an environment on (repo, language, version) rather than on the hook, so -# the compile is paid once for the whole file however many ids a repository -# pins. +# `language: rust` for every id that runs the binary, and it is not a toolchain +# requirement being pushed onto the consumer: both runners bootstrap Rust +# themselves when it is absent. Those hooks build ONE environment between them, +# because pre-commit and prek key an environment on (repo, language, version) +# rather than on the hook, so the compile is paid once for the whole file +# however many ids a repository pins. +# +# The four Go ids at the bottom are the exception, and they are `language: +# system`. They run a consumer's own toolchain over a consumer's own module, so +# there is no uphold code in them and nothing here for a runner to build: +# `language: golang` would have pre-commit install a Go package out of THIS +# repository, which contains none. A repository with Go in it has `go` on PATH +# by definition, and one that does not is told so by the command rather than by +# a build of the wrong thing. # The declaration check. This fires when the declaration changes or when any # configuration it reads changes -- the files that can turn a true enforcement @@ -163,3 +171,98 @@ stages: [manual] pass_filenames: false always_run: true + +# ── the Go toolchain ───────────────────────────────────────────────── +# +# Four ids that run no uphold code at all, published here for one reason: a +# fleet audit read 24 sibling repositories side by side and found these four +# declared as `repo: local` entries and hand-copied -- go-test in 24, go-vet in +# 24, gofmt in 24, go-build in 22. Byte-identical apart from one variant, and +# the variant is the whole argument: two of the gofmt copies could never exit +# nonzero, because `gofmt -l` PRINTS the files it would reformat and exits 0 +# either way. Twenty-two enforced; two reported "Passed" over unformatted code +# for as long as they existed, and nothing compared a copy against its siblings. +# +# A pinned id can drift in exactly one dimension, the rev, and +# `no-stale-hook-pins` already watches that one. A copied `entry:` line can +# drift in every dimension and nothing watches any of them. +# +# The ids carry the `uphold-` prefix every id in this file carries, even though +# they name no uphold command. hooks/lefthook.yml publishes the same four, and a +# lefthook remote config is MERGED into the consumer's own: two commands sharing +# a name under one hook is one command silently replacing the other, and `gofmt` +# is a name a Go repository has very likely used already. The two files have to +# publish one id list, so the prefix is on both. +# +# `files:` rather than `always_run:`, and that is the whole of "fires in no +# repository without Go in it". The same list is spelled as a `glob:` in +# hooks/lefthook.yml, and the two are meant to be read side by side: +# +# files: '(\.go|go\.mod|go\.sum)$' <-> glob: "{*.go,*go.mod,*go.sum}" +# +# They select the same paths, nested ones included, because lefthook's `*` +# crosses a path separator -- and they OVER-select the same way, since a file +# named `cargo.mod` satisfies both. Identical over-selection is the property +# worth having here: two triggers that disagree are the forked copy this whole +# section exists to end, one level up. +# +# `pass_filenames: false` on all four, because not one of these commands takes a +# file list. `./...` is a package pattern and `gofmt -l .` is a walk; handing +# either the staged subset would ask a narrower question than the id names. +# +# `stages: [pre-commit]` and not the `[pre-commit, manual]` every other id here +# carries, which is the one place these four are deliberately narrower than they +# look. Measured against lefthook v2.1.9: `glob` is applied to the files a GIT +# HOOK is running over, and `lefthook run ` -- the only manual stage that +# runner has -- has no such set, so every job in the group runs whatever its +# glob says. A Go job reachable that way fires in a repository with no Go in it +# and fails for want of a module. Declaring `manual` here anyway would buy a +# sweep on two runners out of three and make the two published id lists disagree +# about what a pinned id does, which is the failure these ids exist to end. + +# The broken copy, written once. `gofmt -l` is a REPORT and not a gate, so the +# EMPTINESS of its output is the verdict and emptiness is what is tested. The +# list is printed before the refusal, because a gate that fails without naming +# what failed is the next one somebody deletes. +- id: uphold-gofmt + name: gofmt + description: refuse a tree gofmt would reformat -- the check `gofmt -l` alone cannot make + entry: sh -c 'unformatted="$(gofmt -l .)"; [ -z "$unformatted" ] || { echo "gofmt would reformat these files; run gofmt -w ."; echo "$unformatted"; exit 1; }' + language: system + stages: [pre-commit] + pass_filenames: false + files: '(\.go|go\.mod|go\.sum)$' + +- id: uphold-go-vet + name: go vet + description: run go vet over every package in the module + entry: go vet ./... + language: system + stages: [pre-commit] + pass_filenames: false + files: '(\.go|go\.mod|go\.sum)$' + +# `-o` into a throwaway directory rather than a bare `go build ./...`, and it is +# not tidiness. Where `./...` resolves to several packages go discards what it +# built, but where it resolves to a SINGLE main package go writes the executable +# into the working directory -- so the hook that checks the tree compiles leaves +# an untracked binary in the tree it just checked, named after the module, in +# exactly the repositories small enough to have one package. The status is +# carried across the cleanup because `rm` succeeding must not become the answer. +- id: uphold-go-build + name: go build + description: refuse a module that does not compile, without leaving a binary behind + entry: sh -c 'out="$(mktemp -d)"; go build -o "$out" ./...; status=$?; rm -rf "$out"; exit $status' + language: system + stages: [pre-commit] + pass_filenames: false + files: '(\.go|go\.mod|go\.sum)$' + +- id: uphold-go-test + name: go test + description: run the module's tests + entry: go test ./... + language: system + stages: [pre-commit] + pass_filenames: false + files: '(\.go|go\.mod|go\.sum)$' diff --git a/README.md b/README.md index 489bafa..4abe42f 100644 --- a/README.md +++ b/README.md @@ -57,6 +57,43 @@ remotes: cargo install --git https://github.com/HackingGate/uphold --tag v1.1.1 ``` +That `ref:` is the one version a lefthook consumer pins, and **Dependabot does +not watch it**: there is no ecosystem that reads a lefthook config, so no +updater will raise a pull request when a newer tag lands. What watches it is +`no-stale-hook-pins`, which reads lefthook `remotes:` as pins alongside +pre-commit `repo:`/`rev:` pairs and refuses one that has fallen behind its +upstream or names no `ref:` at all — so the pin is watched by a guard rather +than by an updater, and you are told it is stale rather than handed the bump. It +reads `lefthook.yml`, `lefthook.yaml`, `.lefthook.yml` and `.lefthook.yaml` at +any depth; it does not read `lefthook.toml`, `lefthook.json` or the `-local` +overlay files, so a pin written in one of those is watched by nothing. + +**Go repositories** — four toolchain ids ship here too. They run no uphold code +and need no uphold binary; pin them instead of transcribing them. + +```yaml + - id: uphold-gofmt # a tree gofmt would reformat + - id: uphold-go-vet + - id: uphold-go-build + - id: uphold-go-test +``` + +`language: system`, so they use the `go` a Go repository already has on PATH and +add no toolchain and no build, and a `files:` regex keeps all four silent in a +repository with no Go in it. A lefthook consumer gets the same four ids from +`hooks/lefthook.yml` with nothing extra to write. They are `pre-commit` only, +and not `manual` as the uphold ids are: lefthook applies a job's `glob` to what +a git hook is running over, and a named group has no such set, so a Go job +reachable that way would fire in a repository with no Go in it. + +They exist because 24 sibling repositories declared these four by hand, and two +of the `gofmt` copies could never fail — `gofmt -l` prints the files it would +reformat and exits `0` regardless, so twenty-two enforced and two reported +"Passed" over unformatted code until someone read all 24 side by side. +`uphold-gofmt` tests the *emptiness* of that output, which is where the verdict +actually is. A pinned id can drift in one dimension, the rev, and that dimension +has a check; a copied `entry:` line can drift in every dimension and has none. + ## Declare what enforces what ```toml diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index e8603aa..b2b117d 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -496,14 +496,27 @@ that will not decode is a surface this run did not examine, and saying so is the whole contract. A submodule is enumerated by path and never read as a blob: its content is another repository's. -`no-stale-hook-pins` reaches every `.pre-commit-config.yaml` and every -`lefthook.yml` in the tree, not just the ones at the root, and reads lefthook -`remotes:` entries as pins alongside pre-commit `repo:`/`rev:` pairs. A tree -with no pin file at all is a pass that says why — the lefthook-only install path -is documented and pins nothing. A pin whose remote could not be reached is exit -`2`: a runner with no network fails this guard where it used to pass it, and -`UPHOLD_ALLOW=no-stale-hook-pins` is the deliberate bypass, named in the -refusal. +`no-stale-hook-pins` reaches every `.pre-commit-config.yaml` and every lefthook +config in the tree — `lefthook.yml`, `lefthook.yaml`, `.lefthook.yml`, +`.lefthook.yaml`, at any depth, gitignored files and submodules excluded — and +reads lefthook `remotes:` entries as pins alongside pre-commit `repo:`/`rev:` +pairs. A `remotes:` entry with no `ref:` is refused as unpinned, because it +follows the upstream's default branch. `lefthook.toml`, `lefthook.json` and the +`-local` overlay files are **not** read, so a pin written in one of those is +watched by nothing here. + +Three trees that look alike from the outside and are three different answers: + +| the tree | the answer | +|---|---| +| a lefthook config and no `.pre-commit-config.yaml` | `0` with a note. That is the documented lefthook-only install path, and any `remotes:` the lefthook config pins *were* read | +| a hook config naming no remote pin — every entry `repo: local` or `repo: meta`, or a lefthook config with no `remotes:` | `0` with a note. These files were read, and what they say is that this repository pins nothing remote | +| no hook configuration of **either** manager, anywhere under the root | `2`. Zero pins found is not zero pins to find: a config renamed, moved above this root, or added to `.gitignore` — ignored files are not walked — arrives here as an empty tree, and used to read as clean | + +A pin whose remote could not be reached is exit `2` for the same reason: a +runner with no network fails this guard where it used to pass it. +`UPHOLD_ALLOW=no-stale-hook-pins` is the deliberate bypass in each of those +cases, and every refusal names it. ### Overriding one diff --git a/hooks/lefthook.yml b/hooks/lefthook.yml index 514e16d..f89ec17 100644 --- a/hooks/lefthook.yml +++ b/hooks/lefthook.yml @@ -54,6 +54,56 @@ pre-commit: run: uphold check glob: "{policy/upheld.toml,policy/principles.toml,.pre-commit-config.yaml,lefthook.yml}" + # ── the Go toolchain ───────────────────────────────────────────── + # + # Four jobs that run no uphold code and need no uphold binary. They are + # published because a fleet audit read 24 sibling repositories side by side + # and found these four hand-copied as local declarations -- and found that + # two of the 24 gofmt copies could never exit nonzero, because `gofmt -l` + # prints the files it would reformat and exits 0 either way. The reasoning + # is in .pre-commit-hooks.yaml, which publishes the same four ids under the + # same names; this file is the lefthook half of that one list. + # + # `glob:` is what makes them fire in no repository without Go in it, and it + # is the same list .pre-commit-hooks.yaml spells as a `files:` regex: + # + # glob: "{*.go,*go.mod,*go.sum}" <-> files: '(\.go|go\.mod|go\.sum)$' + # + # The two select the same paths. lefthook's `*` crosses a path separator, so + # `*.go` reaches `internal/x/y.go` and `*go.mod` reaches a nested module's + # `go.mod`; the regex is anchored only at the end, so it reaches both as + # well -- and both spellings over-select a file named `cargo.mod`, in the + # same way. Two triggers that disagreed would be the forked copy this + # section exists to end, one level up. + # + # Jobs rather than commands for the reason uphold-check is one: `glob` is a + # job key, and a Go hook with no condition on it is the full test suite in + # front of every commit in the repository, Go or not. + - name: uphold-gofmt + # `gofmt -l` is a REPORT and not a gate, so the EMPTINESS of its output is + # the verdict and emptiness is what is tested here. The list is printed + # before the refusal, because a gate that fails without naming what failed + # is the next one somebody deletes. + run: unformatted="$(gofmt -l .)"; [ -z "$unformatted" ] || { echo "gofmt would reformat these files; run gofmt -w ."; echo "$unformatted"; exit 1; } + glob: "{*.go,*go.mod,*go.sum}" + - name: uphold-go-vet + run: go vet ./... + glob: "{*.go,*go.mod,*go.sum}" + - name: uphold-go-build + # `-o` into a throwaway directory rather than a bare `go build ./...`, and + # it is not tidiness. Where `./...` resolves to several packages go + # discards what it built; where it resolves to a SINGLE main package go + # writes the executable into the working directory -- so the job that + # checks the tree compiles leaves an untracked binary in the tree it just + # checked, in exactly the repositories small enough to have one package. + # The status is carried across the cleanup because `rm` succeeding must + # not become the answer. + run: out="$(mktemp -d)"; go build -o "$out" ./...; status=$?; rm -rf "$out"; exit $status + glob: "{*.go,*go.mod,*go.sum}" + - name: uphold-go-test + run: go test ./... + glob: "{*.go,*go.mod,*go.sum}" + commit-msg: commands: uphold-scan-text: @@ -89,3 +139,14 @@ uphold-manual: run: uphold scan uphold-guard: run: uphold guard --stage manual + +# The four Go jobs are NOT repeated here, and .pre-commit-hooks.yaml declares +# them `stages: [pre-commit]` for the same reason rather than the +# `[pre-commit, manual]` every other id there carries. Measured against lefthook +# v2.1.9: `glob` is applied to the files a GIT HOOK is running over, and a named +# group like this one has no such set -- every job in it runs, glob or no glob. +# So a Go job here fires in a repository with no Go in it and fails for want of +# a module, which is the one thing these four are published not to do. Making +# the two files disagree about which stages the ids reach, to keep a manual +# sweep this runner cannot condition, would be the forked declaration the ids +# exist to end. diff --git a/policy/principles.toml b/policy/principles.toml index 71d5615..ce981a2 100644 --- a/policy/principles.toml +++ b/policy/principles.toml @@ -258,7 +258,16 @@ exec = "uphold scan --text -" # place through a pull-request body, through a branch name in a push, and # through a package's metadata, and none of those is more forgiving than the # others. -command.before = ["gh", "glab", "git push", "npm publish"] +# +# `npm` bare, not `npm publish`: the `[[shim]]` below matches `pack:*` as well, +# because the tarball's metadata is built by `pack` and merely uploaded by +# `publish` -- the same subject, one step earlier. A checker named for `publish` +# alone stands in front of only half of what the shim says it checks, and the +# shim now refuses an invocation it matches with no checker in front of it +# rather than passing it in silence, so `npm pack` was exit 2. Widening the +# checker keeps both subcommands checked; narrowing the shim would have bought +# the same green by looking at less. +command.before = ["gh", "glab", "git push", "npm"] [rule.no-published-markers] message = """ diff --git a/policy/upheld.toml b/policy/upheld.toml index dc3006f..de123b3 100644 --- a/policy/upheld.toml +++ b/policy/upheld.toml @@ -41,14 +41,27 @@ rule = "catalog-tests" [[enforce]] principle = "explicit-unknown" rule = "no-stale-hook-pins" -# A pin that could not be checked -- unreachable remote, unreadable config, a -# bare sha with no ref to look up -- exits 2, which is not the 0 it would exit -# if the pin resolved. The claim used to name `hook-pins-resolve`, a separate -# script asking that half of the question while this guard asked the other and -# counted a pin it could not reach as passed; one rule asks both now, so the -# claim moves to the rule that carries it. src/pins.rs::stale returns the -# "Could not look is not a pass" refusal, and tests/hook_pins_cli.rs asserts -# that an unreachable remote is exit 2 rather than the exit 0 it used to be. +# A pin that could not be checked -- a remote this run could not reach, a +# directory it could not read, or a tree holding no hook configuration of either +# manager at all -- exits 2, which is not the 0 it would exit if the pin +# resolved. That third case is the could-not-look one step earlier than the +# others: zero pins found is not zero pins to find, and a config renamed, moved +# above the root, or added to .gitignore looks exactly like a repository that +# never had one. +# +# A bare sha naming no tag is deliberately NOT one of them, and must not be +# listed here as though it were: the sha arm of `src/pins.rs::stale` passes it, +# on the ground that a sha is neither behind a tag nor missing. This claim +# records that decision rather than papering over it -- what would make the +# claim false is the guard exiting 0 over a pin it could not establish, not over +# one it established by a different route. +# +# The claim used to name `hook-pins-resolve`, a separate script asking that half +# of the question while this guard asked the other and counted a pin it could +# not reach as passed; one rule asks both now, so the claim moves to the rule +# that carries it. src/pins.rs::stale returns the "Could not look is not a pass" +# refusal, and tests/hook_pins_cli.rs asserts that an unreachable remote is exit +# 2 rather than the exit 0 it used to be. [[enforce]] principle = "single-authoritative-source" diff --git a/scripts/consumer_check.sh b/scripts/consumer_check.sh index 1b4a136..4695437 100755 --- a/scripts/consumer_check.sh +++ b/scripts/consumer_check.sh @@ -11,7 +11,7 @@ # that read git's stdin under a runner that does not forward it. Each of those # passed every test here and failed on first contact with a consumer. # -# Each runner is asked the same eight questions, because "supports lefthook" has +# Each runner is asked the same nine questions, because "supports lefthook" has # to mean the same thing as "supports pre-commit" or it is a listing rather than # a claim: # @@ -28,6 +28,7 @@ # 7. an ordinary merge commit is made and passes, and a merge that would bring # in a zero-width space is refused # 8. the manual-stage entry point runs and passes +# 9. each of the four published Go ids runs, and each one can refuse # # Question 4 is the one that matters for the runners. A guard that cannot see # the push does not fail loudly by default; it falls back to some other tree and @@ -45,6 +46,13 @@ # invocation that nothing here made. A pinned id that never runs is this # script's own failure mode, one level up -- it passes here, in a config that # looks complete, and does nothing in the consumer that copies it. +# +# Question 9 is the same lesson applied before it can be learned twice. The four +# Go ids are published for repositories with Go in them and are triggered by a +# `files:`/`glob:` on Go paths, so in a consumer like this one -- which has none +# until question 9 makes some -- all four skip, silently and correctly, through +# every question above. Four more ids pinned and never driven is exactly the +# hole questions 6 to 8 were added to close. set -euo pipefail @@ -75,6 +83,23 @@ commit() { commit -q -m "$1" } +# Stage what is in the tree, commit it, and require that ONE named id refused +# and that the refusal carries the tool's own words. Used by question 9, where a +# passing commit would be no evidence at all: a hook that never fired and a hook +# that fired and found nothing look identical from out here, and the two runners +# spell their progress output differently enough that grepping for a hook's name +# would be a third thing to keep in step. +refuses() { + local id=$1 needle=$2 subject=$3 + git -C "$CONSUMER" add -A + if git -C "$CONSUMER" -c user.email=demo@example.test -c user.name=Demo \ + commit -q -m "$subject" >"$WORK/$id.log" 2>&1; then + fail "$id did not refuse: \"$subject\" was accepted" + fi + grep -q "$needle" "$WORK/$id.log" || + fail "refused, but not by $id: $(cat "$WORK/$id.log")" +} + say "consumer: $CONSUMER runner: $RUNNER hooks: $HOOKS_REPO@$HOOKS_REF" # The hooks repository is cloned to a neutral path and pinned by a branch name, @@ -156,6 +181,10 @@ repos: - id: uphold-guard-merge - id: uphold-guard-push - id: uphold-guard-manual + - id: uphold-gofmt + - id: uphold-go-vet + - id: uphold-go-build + - id: uphold-go-test CONFIG raw_commit "seed" (cd "$CONSUMER" && "$RUNNER" install --install-hooks >/dev/null) @@ -354,4 +383,79 @@ lefthook) ;; esac -say "$RUNNER: all eight passed" +say "9. each of the four published Go ids runs, and each one can refuse" +# Every question above ran in a consumer with no Go in it, which is where these +# four are supposed to be silent -- and silence is also what a broken id sounds +# like. So Go arrives here, and each id is then driven by a fault that ONLY it +# can see: a build that does not compile fails all four at once and would prove +# nothing about which of them ran. +cat > "$CONSUMER/go.mod" <<'GOMOD' +module example.test/consumerapp + +go 1.22 +GOMOD +cat > "$CONSUMER/main.go" <<'GO' +package main + +func main() {} +GO +commit "Add a Go module" || fail "a clean Go tree was refused" + +# `go build ./...` writes an executable into the working directory when `./...` +# resolves to a single main package -- which is the shape a small consumer has, +# and the shape of this fixture. The published id builds into a throwaway +# directory for exactly that reason; if it ever stops, the binary lands here, +# untracked, in the tree the hook had just finished pronouncing clean. +if [ -e "$CONSUMER/consumerapp" ]; then + fail "uphold-go-build left an executable behind in the consumer's tree" +fi + +# gofmt, which is why these four ids exist at all. `gofmt -l` PRINTS the files +# it would reformat and exits 0 whatever it printed, so a hand-copied entry +# running it bare reports a pass over unformatted code for as long as it lives +# -- two of the 24 copies audited did precisely that. The published id tests the +# EMPTINESS of that output, and this refusal is the whole of the difference. +printf 'package main\n\nfunc main( ){}\n' > "$CONSUMER/main.go" +refuses uphold-gofmt "gofmt would reformat" "Unformatted Go" +printf 'package main\n\nfunc main() {}\n' > "$CONSUMER/main.go" + +# go vet, on a finding the other three accept: an unused `fmt.Sprintf` result +# compiles, and `unusedresult` is outside the vet subset `go test` runs itself. +printf 'package main\n\nimport "fmt"\n\nfunc main() { fmt.Sprintf("nothing reads this") }\n' \ + > "$CONSUMER/main.go" +refuses uphold-go-vet "result of fmt.Sprintf call not used" "Go that only vet objects to" +printf 'package main\n\nfunc main() {}\n' > "$CONSUMER/main.go" + +# go test, on a test that builds and vets clean, so nothing else can be what +# refused it. +cat > "$CONSUMER/main_test.go" <<'GO' +package main + +import "testing" + +func TestConsumer(t *testing.T) { t.Fatal("this test fails on purpose") } +GO +refuses uphold-go-test "this test fails on purpose" "A failing Go test" +rm -f "$CONSUMER/main_test.go" + +# go build last, because a tree that does not compile refuses under all four and +# can only be attributed once the other three have already answered. +printf 'package main\n\nfunc main() { nope() }\n' > "$CONSUMER/main.go" +refuses uphold-go-build "undefined: nope" "Go that does not compile" +printf 'package main\n\nfunc main() {}\n' > "$CONSUMER/main.go" + +# And the accepting direction, which is the half a consumer lives in: four ids +# that only ever refuse would pass this question by never letting anything +# through. It carries a NEW file rather than only the restored one, for the +# reason question 6 replaces a claim instead of deleting it -- main.go is back +# to bytes already committed and main_test.go was never committed at all, so +# there would be nothing staged, git would refuse the empty commit, and the +# question would have passed on a commit that never happened. +cat > "$CONSUMER/greet.go" <<'GO' +package main + +func greet() string { return "hello" } +GO +commit "Restore the Go module" || fail "a clean Go tree was refused after the faults" + +say "$RUNNER: all nine passed" diff --git a/src/guard/names.rs b/src/guard/names.rs index e43d817..0c9fe11 100644 --- a/src/guard/names.rs +++ b/src/guard/names.rs @@ -156,11 +156,7 @@ fn foreign_forge_names(text: &str) -> BTreeSet<(String, String)> { /// A private repository on another forge is still reachable here: declare its /// owner in `private_owners`, which is matched in the bare form and needs no /// network. -fn candidates( - text: &str, - private_owners: &[String], - own_owner: Option<&str>, -) -> BTreeSet<(String, String)> { +fn candidates(text: &str, owners: &OwnerMatchers) -> BTreeSet<(String, String)> { let mut found: BTreeSet<(String, String)> = BTreeSet::new(); for capture in url_pattern().captures_iter(text) { if !is_github_host(&capture[1]) { @@ -174,63 +170,91 @@ fn candidates( found.insert((owner, repo)); } - // The repository's OWN owner, treated as though it had been declared. - // - // `acme/widget` written with no host is the spelling a README uses for a - // sibling -- "now maintained in acme/widget" -- and the URL forms above all - // miss it. It is not caught for owners in general because a bare - // `owner/repo` is indistinguishable from a relative path, and every path in - // every document would become a lookup. It is caught for THIS owner because - // a segment equal to the login that owns the repository, followed by a - // name, is a sibling reference and not a directory: nobody writes - // `acme/main.rs`. - // - // Found by trying to write the deprecation note that would close #29 and - // watching the guard pass it. - let mut owners: Vec = private_owners.to_vec(); - if let Some(own_owner) = own_owner { - if !owners - .iter() - .any(|owner| owner.eq_ignore_ascii_case(own_owner)) - { - owners.push(own_owner.to_owned()); - } - } - - for owner in &owners { - // Anchored at the owner so a declared-private owner is found in the - // bare form too. Escaped, because an owner may legitimately contain a - // dot and an unescaped one would match any character. - let pattern = format!( - r"(?i)\b{}/([A-Za-z0-9][A-Za-z0-9._-]*)", - regex::escape(owner) - ); - let Ok(matcher) = Regex::new(&pattern) else { - continue; - }; + for (owner, matcher) in &owners.named { for capture in matcher.captures_iter(text) { found.insert((owner.clone(), clean_repo(&capture[1]))); } + } + for (owner, matcher) in &owners.bare { + if matcher.is_match(text) { + found.insert((owner.clone(), String::new())); + } + } + found +} - // The owner ON ITS OWN, with no repository after it. Every form above - // needs an `owner/repo`, and this is the one that got past a hand - // audit: a sentence naming a private organisation discloses that it - // exists and who owns it without ever naming one of its repositories. - // Only for a DECLARED owner -- a bare word is not otherwise a name, and - // treating any capitalised token as one would fire on ordinary prose. - // The repository's own owner is deliberately not in this half: its - // name is published by the repository existing. - if !private_owners.iter().any(|declared| declared == owner) { - continue; +/// The patterns that depend only on the OWNER, compiled once per judgement. +/// +/// Built here rather than inside `candidates` because `candidates` is asked +/// about one source at a time, and the staged scan now hands it one ADDED LINE +/// at a time: a pattern built inside the search is rebuilt once per line per +/// declared owner. The tree-wide scan was already rebuilding it twice per blob, +/// which is thousands of compilations of a pattern that cannot vary with the +/// text it is run against. +struct OwnerMatchers { + /// `owner/repo`, anchored at each owner this rule looks for. + named: Vec<(String, Regex)>, + /// A DECLARED private owner written on its own, with no repository after it. + bare: Vec<(String, Regex)>, +} + +impl OwnerMatchers { + fn new(private_owners: &[String], own_owner: Option<&str>) -> Self { + // The repository's OWN owner, treated as though it had been declared. + // + // `acme/widget` written with no host is the spelling a README uses for + // a sibling -- "now maintained in acme/widget" -- and the URL forms in + // `candidates` all miss it. It is not caught for owners in general + // because a bare `owner/repo` is indistinguishable from a relative + // path, and every path in every document would become a lookup. It is + // caught for THIS owner because a segment equal to the login that owns + // the repository, followed by a name, is a sibling reference and not a + // directory: nobody writes `acme/main.rs`. + // + // Found by trying to write the deprecation note that would close #29 + // and watching the guard pass it. + let mut owners: Vec = private_owners.to_vec(); + if let Some(own_owner) = own_owner { + if !owners + .iter() + .any(|owner| owner.eq_ignore_ascii_case(own_owner)) + { + owners.push(own_owner.to_owned()); + } } - let bare = format!(r"(?i)\b{}\b", regex::escape(owner)); - if let Ok(bare_matcher) = Regex::new(&bare) { - if bare_matcher.is_match(text) { - found.insert((owner.clone(), String::new())); + + let mut named: Vec<(String, Regex)> = Vec::new(); + let mut bare: Vec<(String, Regex)> = Vec::new(); + for owner in owners { + // Anchored at the owner so a declared-private owner is found in the + // bare form too. Escaped, because an owner may legitimately contain + // a dot and an unescaped one would match any character. + let pattern = format!( + r"(?i)\b{}/([A-Za-z0-9][A-Za-z0-9._-]*)", + regex::escape(&owner) + ); + let Ok(matcher) = Regex::new(&pattern) else { + continue; + }; + + // The owner ON ITS OWN, with no repository after it. Every form + // above needs an `owner/repo`, and this is the one that got past a + // hand audit: a sentence naming a private organisation discloses + // that it exists and who owns it without ever naming one of its + // repositories. Only for a DECLARED owner -- a bare word is not + // otherwise a name, and treating any capitalised token as one would + // fire on ordinary prose. The repository's own owner is + // deliberately not in this half: its name is published by the + // repository existing. + if private_owners.contains(&owner) { + if let Ok(alone) = Regex::new(&format!(r"(?i)\b{}\b", regex::escape(&owner))) { + bare.push((owner.clone(), alone)); + } } + named.push((owner, matcher)); } + Self { named, bare } } - found } /// Ask the forge, once per name per run. @@ -412,9 +436,10 @@ fn judge(root: &Path, rule: &Rule, owners: &[String], sources: &[(String, String let mut refused = Vec::new(); let mut unresolved = Vec::new(); let mut seen: BTreeSet = BTreeSet::new(); + let matchers = OwnerMatchers::new(owners, our_owner.as_deref()); for (where_found, text) in sources { - for (owner, repo) in candidates(text, owners, our_owner.as_deref()) { + for (owner, repo) in candidates(text, &matchers) { let bare_owner = repo.is_empty(); let name = if bare_owner { owner.clone() @@ -565,6 +590,14 @@ struct Staged { /// prints a path verbatim -- no quoting, no escaping -- and a path read out of /// a `+++ b/...` header is a path this reader would have to unquote correctly /// to attribute a finding to the right file. +/// +/// `--no-ext-diff` and `--no-textconv` for the reason `added_lines` sets out at +/// length. git counts these itself and does not put them through a diff driver, +/// so on the git in front of me the flags change nothing here -- they are +/// written anyway so that the three `git diff` calls in this file cannot be +/// read as three different decisions about whose config gets a say. The one +/// that was missing them was blind, and it looked exactly like its twins until +/// somebody put them side by side. fn staged_paths(root: &Path) -> Result> { let records = git::run_z( root, @@ -573,6 +606,8 @@ fn staged_paths(root: &Path) -> Result> { "core.quotepath=false", "diff", "--cached", + "--no-ext-diff", + "--no-textconv", "--numstat", "-z", ], @@ -605,7 +640,35 @@ fn staged_paths(root: &Path) -> Result> { Ok(staged) } -/// The lines one staged path ADDS. +/// Which line of the NEW file a hunk opens at, from the `+c,d` half of its +/// `@@ -a,b +c,d @@` header. +/// +/// The count is optional and `-U0` is where that shows: a one-line hunk is +/// spelled `@@ -7 +7 @@`, with no comma anywhere in it, so a reader that split +/// on one found no number and every finding in the commit lost its line. +fn hunk_start(header: &str) -> Option { + header + .split_once('+')? + .1 + .split(|character: char| character == ',' || character.is_whitespace()) + .next() + .filter(|number| !number.is_empty()) + .and_then(|number| number.parse().ok()) +} + +/// The lines one staged path ADDS, each with the line it will be at. +/// +/// Read hunk by hunk rather than by keeping every line that starts with `+` and +/// excepting `+++`. That exception cannot tell a header from content: an added +/// line whose own first two characters are `++` is spelled `+++...` in the diff +/// exactly like the `+++ b/path` above it, so every such line was dropped from +/// the scan -- `++ github.com/acme/secret` in a changelog was a name this guard +/// never looked at. Inside a hunk a leading `+` is always the marker, and the +/// file header cannot appear inside one. +/// +/// It is also the only place the LINE NUMBER exists. A finding that names the +/// file and not the line is one a reader has to go searching for, and the +/// sibling guard over the same text has named both since it was written. /// /// Every flag here closes a way this diff was reported as empty over a file /// that was not: @@ -623,7 +686,7 @@ fn staged_paths(root: &Path) -> Result> { /// other listing in this file spells it. /// * `--text` on the second pass, which is what makes a `diff` attribute stop /// deciding whether the bytes get read. -fn added_lines(root: &Path, path: &str, force_text: bool) -> Result { +fn added_lines(root: &Path, path: &str, force_text: bool) -> Result, String)>> { let mut argv: Vec<&str> = vec![ "-c", "core.quotepath=false", @@ -641,11 +704,58 @@ fn added_lines(root: &Path, path: &str, force_text: bool) -> Result { argv.push("--"); argv.push(&spec); let diff = git::run(root, &argv)?; - Ok(diff - .lines() - .filter(|line| line.starts_with('+') && !line.starts_with("+++")) - .collect::>() - .join("\n")) + + let mut added: Vec<(Option, String)> = Vec::new(); + let mut in_hunk = false; + // `None` inside a hunk means a header this reader could not parse. The line + // is still SCANNED -- it is only reported without a number. A guard that + // dropped the line because it could not number it would be answering "where + // is it" by deciding there is nothing there. + let mut line: Option = None; + for record in diff.lines() { + if let Some(header) = record.strip_prefix("@@") { + in_hunk = true; + line = hunk_start(header); + continue; + } + // One path per call, so this is belt and braces -- but the counter has + // to be wrong before a finding can name the wrong line, and starting it + // over at each file is what makes that impossible. + if record.starts_with("diff --git ") { + in_hunk = false; + line = None; + continue; + } + if !in_hunk { + // The preamble: the mode and index lines, `--- /dev/null`, + // `+++ b/path`, and "Binary files a/x and b/x differ" -- which is + // the whole of the output for a path the first pass cannot read, + // and the reason there is a second one. + continue; + } + if let Some(text) = record.strip_prefix('+') { + added.push((line, text.to_owned())); + line = line.map(|number| number.saturating_add(1)); + } else if record.starts_with(' ') { + // Context, which `-U0` does not ask for. Counted all the same, + // because what the numbers here mean must not depend on a + // `diff.context` in somebody's personal config being overridden. + line = line.map(|number| number.saturating_add(1)); + } + // A `-` line is text the commit removes and does not carry, and + // `\ No newline at end of file` is a note about the line above it. + // Neither is a line of the new file, and neither moves the counter. + } + Ok(added) +} + +/// Where a finding was found: the path, and the line when the diff said which. +/// +/// Dropped rather than guessed at when a hunk header could not be read. A wrong +/// line number sends a reader to the wrong place and is worse than none, and the +/// line was scanned either way. +fn located(path: &str, line: Option) -> String { + line.map_or_else(|| path.to_owned(), |number| format!("{path}:{number}")) } /// The paths this commit INTRODUCES, whatever is inside them. @@ -656,6 +766,11 @@ fn added_lines(root: &Path, path: &str, force_text: bool) -> Result { /// only -- a path that was already there is the tree-wide guard's business, /// and reporting it at every commit that touches the file would be a wall /// somebody bypasses by reflex rather than a finding they act on. +/// +/// Spelled with the same flags as its two neighbours, for the reason +/// `staged_paths` gives: whose config gets a say in what this guard can see is +/// one decision, and three call sites that answer it three ways is the fork +/// this tool exists to catch. fn introduced_paths(root: &Path) -> Result> { git::run_z( root, @@ -664,6 +779,8 @@ fn introduced_paths(root: &Path) -> Result> { "core.quotepath=false", "diff", "--cached", + "--no-ext-diff", + "--no-textconv", "--name-only", "--diff-filter=ACR", "-z", @@ -673,10 +790,11 @@ fn introduced_paths(root: &Path) -> Result> { /// The lines this commit ADDS, and the paths it introduces. /// -/// One source per path rather than one blob for the whole commit: the rule's -/// `[rule.files]` scope is a question about a PATH, so a single blob labelled -/// "staged changes" could not be scoped at all -- and the finding it produced -/// named neither the file it came from nor anything a reader could open. +/// One source per ADDED LINE rather than one blob for the whole commit: the +/// rule's `[rule.files]` scope is a question about a PATH, so a single blob +/// labelled "staged changes" could not be scoped at all -- and the finding it +/// produced named neither the file it came from nor anything a reader could +/// open. A path answers the scope; the line is what a reader opens. pub(crate) fn in_staged(request: &Request<'_>) -> Result> { let mut sources: Vec<(String, String)> = Vec::new(); @@ -693,10 +811,9 @@ pub(crate) fn in_staged(request: &Request<'_>) -> Result> { } if staged.as_text { if staged.added { - sources.push(( - staged.path.clone(), - added_lines(request.root, &staged.path, false)?, - )); + for (line, text) in added_lines(request.root, &staged.path, false)? { + sources.push((located(&staged.path, line), text)); + } } continue; } @@ -739,10 +856,9 @@ pub(crate) fn in_staged(request: &Request<'_>) -> Result> { if bytes.iter().take(8000).any(|byte| *byte == 0) { continue; } - sources.push(( - staged.path.clone(), - added_lines(request.root, &staged.path, true)?, - )); + for (line, text) in added_lines(request.root, &staged.path, true)? { + sources.push((located(&staged.path, line), text)); + } } decide(request, &sources) @@ -831,6 +947,16 @@ pub(crate) fn in_text( mod tests { use super::*; + /// The owner patterns are compiled once per judgement now, so a test that + /// asks what a text names builds them the way `judge` does. + fn named( + text: &str, + private_owners: &[String], + own_owner: Option<&str>, + ) -> BTreeSet<(String, String)> { + candidates(text, &OwnerMatchers::new(private_owners, own_owner)) + } + fn resolved(visibility: Visibility, canonical: Option<&str>) -> Resolved { Resolved { visibility, @@ -881,7 +1007,7 @@ mod tests { "https://datatracker.ietf.org/rfc/rfc9110", ] { assert!( - candidates(citation, &[], None).is_empty(), + named(citation, &[], None).is_empty(), "{citation} was read as a repository name" ); } @@ -892,7 +1018,7 @@ mod tests { // The dangerous direction: `github.acme.com/acme/widget` is a different // forge. Asking github.com about it answers about somebody else's // repository, and a public answer there passes a private one here. - assert!(candidates("https://github.acme.com/acme/widget", &[], None).is_empty()); + assert!(named("https://github.acme.com/acme/widget", &[], None).is_empty()); } #[test] @@ -909,7 +1035,7 @@ mod tests { #[test] fn a_raw_content_url_is_still_a_github_name() { - let found = candidates( + let found = named( "https://raw.githubusercontent.com/acme/widget/main/README.md", &[], None, @@ -919,13 +1045,13 @@ mod tests { #[test] fn a_forge_url_is_a_candidate() { - let found = candidates("see https://github.com/acme/widget for details", &[], None); + let found = named("see https://github.com/acme/widget for details", &[], None); assert!(found.contains(&("acme".to_owned(), "widget".to_owned()))); } #[test] fn a_sentence_full_stop_is_not_part_of_the_name() { - let found = candidates("moved to acme/widget.", &[], Some("acme")); + let found = named("moved to acme/widget.", &[], Some("acme")); assert!( found.contains(&("acme".to_owned(), "widget".to_owned())), "{found:?}" @@ -934,16 +1060,16 @@ mod tests { #[test] fn a_dot_git_suffix_is_not_part_of_the_name() { - let found = candidates("git@github.com:acme/widget.git", &[], None); + let found = named("git@github.com:acme/widget.git", &[], None); assert!(found.contains(&("acme".to_owned(), "widget".to_owned()))); } #[test] fn a_bare_path_is_not_a_candidate_unless_its_owner_was_declared() { // Otherwise every relative path in every document is a lookup. - assert!(candidates("see src/main.rs", &[], None).is_empty()); + assert!(named("see src/main.rs", &[], None).is_empty()); let declared = vec!["src".to_owned()]; - assert!(candidates("see src/main.rs", &declared, None) + assert!(named("see src/main.rs", &declared, None) .contains(&("src".to_owned(), "main.rs".to_owned()))); } @@ -952,7 +1078,7 @@ mod tests { // `acme/widget` with no host is what a README writes -- "now maintained // in acme/widget" -- and every URL form misses it. Found by trying to // write a deprecation note and watching the guard pass it. - let found = candidates("now maintained in acme/widget", &[], Some("acme")); + let found = named("now maintained in acme/widget", &[], Some("acme")); assert!(found.contains(&("acme".to_owned(), "widget".to_owned()))); } @@ -961,7 +1087,7 @@ mod tests { // A bare `owner/repo` is indistinguishable from a relative path, so // this stays off for owners in general: every path in every document // would otherwise become a forge lookup. - let found = candidates("see src/main.rs", &[], Some("acme")); + let found = named("see src/main.rs", &[], Some("acme")); assert!(found.is_empty(), "{found:?}"); } @@ -969,17 +1095,38 @@ mod tests { fn the_repositorys_own_owner_alone_is_not_a_finding() { // Its name is published by the repository existing. Only a DECLARED // private owner is caught on its own. - let found = candidates("maintained by acme", &[], Some("acme")); + let found = named("maintained by acme", &[], Some("acme")); assert!(found.is_empty(), "{found:?}"); } #[test] fn a_declared_owner_with_a_dot_is_escaped_not_interpreted() { let declared = vec!["acme.corp".to_owned()]; - let found = candidates("acme.corp/thing and acmeXcorp/other", &declared, None); + let found = named("acme.corp/thing and acmeXcorp/other", &declared, None); assert!(found.contains(&("acme.corp".to_owned(), "thing".to_owned()))); assert!(!found .iter() .any(|(owner, _)| owner.eq_ignore_ascii_case("acmeXcorp"))); } + + #[test] + fn a_hunk_with_no_count_still_says_which_line_it_opens_at() { + // `-U0` spells a one-line hunk without a comma anywhere in it, and it is + // the spelling the staged scan asks for -- so a reader that needed the + // comma numbered nothing this guard ever sees. + assert_eq!(hunk_start(" -7 +7 @@ fn f()"), Some(7)); + assert_eq!(hunk_start(" -0,0 +1,3 @@"), Some(1)); + assert_eq!(hunk_start(" -1 +0,0 @@"), Some(0)); + // Nothing to read rather than a number invented from one: a wrong line + // sends a reader to the wrong place. + assert_eq!(hunk_start(" not a hunk header"), None); + } + + #[test] + fn a_finding_with_no_line_still_names_the_file() { + // The line is dropped when a header could not be parsed, and the finding + // is not -- the line was scanned either way. + assert_eq!(located("docs/note.md", Some(12)), "docs/note.md:12"); + assert_eq!(located("docs/note.md", None), "docs/note.md"); + } } diff --git a/src/pins.rs b/src/pins.rs index a66a9f1..e6f08cc 100644 --- a/src/pins.rs +++ b/src/pins.rs @@ -23,6 +23,14 @@ //! the alternative -- what this did -- is to print the pin to stderr and exit 0 //! with the guard counted among the ones that passed. //! +//! A tree holding NO hook configuration at all is that same state one step +//! earlier: no pin was read, so no pin was established, and it exits 2 too. The +//! two mistakes here are opposite and both were made -- opening +//! `.pre-commit-config.yaml` unconditionally killed every lefthook consumer at +//! every push, and the repair passed any tree where the file was simply not +//! found, which is what a config that was renamed, moved above the root, or +//! added to `.gitignore` also looks like from here. +//! //! Both managers are read. pre-commit writes `repos:` with a `rev:`; lefthook //! writes `remotes:` with a `ref:`, and that entry is the single version a //! lefthook consumer pins. It was read by nothing here and there is no @@ -97,13 +105,26 @@ pub(crate) struct Pin { pub source: String, } -/// Every pin in the tree, and what could not be read while collecting them. +/// One read of a tree: every pin in it, and how much of it was read at all. +/// +/// Named for the act rather than for the pins, because the count below is a +/// fact about the read and not about the pins: an empty `pins` means one thing +/// after two configurations were opened and another after none were. #[derive(Debug)] -pub(crate) struct Pins { +pub(crate) struct Reading { pub pins: Vec, /// Facts about coverage rather than about pins: which of the two hook /// managers this tree even uses. Said aloud, never counted as a pass. pub notes: Vec, + /// How many hook configurations this walk actually opened. + /// + /// Zero pins and no file to read a pin out of are different answers, and + /// only the first of them is a measurement. Without this count they were + /// the same empty `pins` vector, so a tree whose configuration had been + /// renamed, moved out from under the root, or added to `.gitignore` -- the + /// walk skips ignored files -- reported the same clean answer as a tree + /// with a current pin in it. + pub configs: usize, } const PRE_COMMIT_CONFIG: &str = ".pre-commit-config.yaml"; @@ -279,25 +300,30 @@ fn lefthook_pins(path: &Path, source: &str, text: &str) -> Result> { Ok(pins) } -pub(crate) fn read_pins(root: &Path) -> Result { +pub(crate) fn read_pins(root: &Path) -> Result { let mut pins = Vec::new(); let mut notes = Vec::new(); let mut saw_pre_commit = false; - for path in hook_configs(root)? { + let mut saw_lefthook = false; + let mut sources: Vec = Vec::new(); + let configs = hook_configs(root)?; + for path in &configs { let source = path .strip_prefix(root) - .unwrap_or(&path) + .unwrap_or(path) .display() .to_string(); - let text = read_to_string(&path)?; + sources.push(source.clone()); + let text = read_to_string(path)?; if path .file_name() .is_some_and(|name| name == PRE_COMMIT_CONFIG) { saw_pre_commit = true; - pins.extend(pre_commit_pins(&path, &source, &text)?); + pins.extend(pre_commit_pins(path, &source, &text)?); } else { - pins.extend(lefthook_pins(&path, &source, &text)?); + saw_lefthook = true; + pins.extend(lefthook_pins(path, &source, &text)?); } } // An absent file, reported as a fact rather than as an io error. This @@ -306,14 +332,45 @@ pub(crate) fn read_pins(root: &Path) -> Result { // on every consumer who followed the documented lefthook-only install path. // They have no pre-commit config because they were told not to make one, and // that is an answer, not a failure to obtain one. - if !saw_pre_commit { + // + // Conditional on a lefthook config having been READ, because the sentence + // ends by promising that a lefthook config's `remotes:` were. Said over a + // tree holding neither manager's file, that promise names a file that does + // not exist -- and it was the whole justification for the pass the caller + // then reported. That case is the caller's to refuse; this one is a + // coverage fact beside a real answer. + if saw_lefthook && !saw_pre_commit { notes.push(format!( "no `{PRE_COMMIT_CONFIG}` anywhere in this tree, so there are no pre-commit pins \ to check. That is the documented lefthook-only install path, not a hole in the \ answer; any `remotes:` a lefthook config pins were read." )); } - Ok(Pins { pins, notes }) + // A configuration that pins nothing is not a configuration whose pins are + // current, and this said nothing at all about the difference. The reader + // this module replaced counted `local` and `meta` entries on exactly that + // ground -- they pin nothing, so a config made only of them has verified no + // version -- and a lefthook config with no `remotes:` is the same state in + // the other manager. Silence here is the silence a fully-current tree + // prints, which leaves the two indistinguishable at the one moment the + // distinction matters. + // + // A note and not a refusal: these files were read, and what they say is + // that this repository pins nothing remote. That is an answer. + if pins.is_empty() && !sources.is_empty() { + notes.push(format!( + "read {} hook configuration(s) -- {} -- and none of them names a remote pin. \ + `repo: local`, `repo: meta` and a lefthook config with no `remotes:` pin \ + nothing by design, so no version was verified here.", + sources.len(), + sources.join(", ") + )); + } + Ok(Reading { + pins, + notes, + configs: configs.len(), + }) } /// Compare two tags the way a person reads them. @@ -406,12 +463,60 @@ fn remote_refs(repo: &str) -> Result> { })) } +/// What was not established, in the one wording both exits owe a reader. +/// +/// Both callers below say this: the one that found a violation as well, and the +/// one that found nothing else at all. Written out twice they drifted -- the +/// violation arm said it on stderr in its own words while the other said it in +/// the refusal -- and the arm that mattered more to a reader was the terser one. +fn unestablished(unchecked: &[String]) -> String { + format!( + "{} pin(s) could not be checked, so this guard established nothing about them:\n{}", + unchecked.len(), + unchecked.join("\n") + ) +} + /// Both questions, over every pin. pub(crate) fn stale(request: &Request<'_>) -> Result> { - let Pins { pins, notes } = read_pins(request.root)?; + let Reading { + pins, + notes, + configs, + } = read_pins(request.root)?; for note in ¬es { println!("{}: {note}", request.rule.id); } + // NO FILE TO READ IS THE COULD-NOT-LOOK ONE STEP EARLIER THAN AN + // UNREACHABLE REMOTE, and it exited 0. + // + // Making an absent `.pre-commit-config.yaml` a fact rather than an io error + // fixed a guard that killed every lefthook consumer at every push. Left + // there, it swapped one defect for its opposite: a tree holding NEITHER + // manager's configuration produced zero pins, a note explaining that the + // lefthook-only path is documented, and a pass -- with the note citing a + // lefthook config that was not there either. + // + // Zero pins found is not zero pins to find. The walk skips gitignored + // files, so a `.pre-commit-config.yaml` someone added to `.gitignore` + // arrives here as an empty tree; so does one renamed, moved above the root, + // or lost in a merge. Each of those is a repository whose hooks still run + // pinned code that nothing is watching, and each of them read as clean. + if configs == 0 { + return Err(Fatal::new(format!( + "{}: no `{PRE_COMMIT_CONFIG}` and no lefthook configuration ({}) anywhere under \ + {}, so this guard read no pins and established nothing about the versions this \ + repository's hooks run. Zero pins found and no file to find one in are different \ + answers, and a config that was renamed, moved above this root, or added to \ + `.gitignore` -- ignored files are not walked -- looks exactly like this one.\n\n\ + Could not look is not a pass. Point this run at the tree that holds the hook \ + configuration, or bypass it deliberately with UPHOLD_ALLOW={}.", + request.rule.id, + LEFTHOOK_CONFIGS.join(", "), + request.root.display(), + request.rule.id + ))); + } let mut behind: Vec = Vec::new(); let mut missing: Vec = Vec::new(); let mut unchecked: Vec = Vec::new(); @@ -484,15 +589,25 @@ pub(crate) fn stale(request: &Request<'_>) -> Result> { report.push_str("\n\nThe upstream tag owns the version; a `rev:` here is a copy of it."); } if !report.is_empty() { - // Said aloud beside the violation. The refusal below exits 1 on what was - // checked, and a reader has to know that number was measured over fewer - // pins than the file holds. + // IN the refusal, not on a stream beside it. `Refusal` says it carries + // the whole report because a guard that says only "refused" sends the + // reader looking; this caveat was the one part that did not travel with + // it. It went to stderr from here, before `guard::run` printed the + // finding it qualifies, so the two arrived out of order and only for a + // caller watching that stream -- and the exit code is 1, which reads as + // "checked, and here is what is wrong" over a set of pins smaller than + // the tree holds. + // + // Exit 1 and not 2, deliberately, on the rule `audit::verdict` states: + // a violation outranks an unread surface, because something WAS found + // and the reader has a fix to make either way. What they must not have + // to guess is that the finding was measured over fewer pins. if !unchecked.is_empty() { - eprintln!( - "{}: {} pin(s) could not be checked, on top of the finding(s) below:\n{}", - request.rule.id, - unchecked.len(), - unchecked.join("\n") + report.push_str("\n\n"); + report.push_str(&unestablished(&unchecked)); + report.push_str( + "\n\nThose are on top of the finding(s) above, and the finding(s) above were \ + measured over the pins that remain.", ); } return Ok(Some(Refusal { @@ -516,12 +631,10 @@ pub(crate) fn stale(request: &Request<'_>) -> Result> { // `Exit::Broken` over a surface it could not read. if !unchecked.is_empty() { return Err(Fatal::new(format!( - "{}: {} pin(s) could not be checked, so this guard established nothing about \ - them:\n{}\n\nCould not look is not a pass. Restore the remote's reachability, \ + "{}: {}\n\nCould not look is not a pass. Restore the remote's reachability, \ or bypass this run deliberately with UPHOLD_ALLOW={}.", request.rule.id, - unchecked.len(), - unchecked.join("\n"), + unestablished(&unchecked), request.rule.id ))); } @@ -642,6 +755,12 @@ mod tests { assert!(error.to_string().contains("could not be read"), "{error}"); } + /// And a config that is nothing but local hooks says so. + /// + /// The pin reader this module replaced counted `local` and `meta` entries + /// because a config made only of them has verified no version at all, and + /// this printed the same nothing a tree of current pins prints -- so the + /// two states were indistinguishable to the reader of the output. #[test] fn a_local_repo_has_no_pin_to_check() { let dir = tree("local"); @@ -650,7 +769,16 @@ mod tests { ".pre-commit-config.yaml", "repos:\n - repo: local\n hooks:\n - id: x\n", ); - assert!(read_pins(&dir).unwrap().pins.is_empty()); + let read = read_pins(&dir).unwrap(); + assert!(read.pins.is_empty()); + assert_eq!(read.configs, 1); + assert!( + read.notes + .iter() + .any(|note| note.contains("no version was verified here")), + "{:?}", + read.notes + ); } /// The documented lefthook-only install path is not a broken repository. @@ -658,11 +786,22 @@ mod tests { /// `read_pins` opened `root/.pre-commit-config.yaml` unconditionally and /// `read_to_string` turns ENOENT into a `Fatal`, so this guard exited 2 for /// every consumer who installed the way the documentation tells them to. + /// + /// Over a tree that HOLDS a lefthook config, which is what that install + /// path leaves behind and what the note this asserts on promises was read. + /// Asserted over an empty directory, this test passed while describing a + /// consumer it was not standing in for. #[test] fn an_absent_pre_commit_config_is_an_answer_and_not_an_error() { let dir = tree("absent"); + write( + &dir, + "lefthook.yml", + "remotes:\n - git_url: https://example.test/hooks\n ref: v1.2.3\n", + ); let read = read_pins(&dir).unwrap(); - assert!(read.pins.is_empty()); + assert_eq!(read.pins.len(), 1, "{:?}", read.pins); + assert_eq!(read.configs, 1); assert_eq!(read.notes.len(), 1, "{:?}", read.notes); assert!( read.notes @@ -673,6 +812,25 @@ mod tests { ); } + /// No file to read a pin out of is not zero pins. + /// + /// The repair above stopped an absent `.pre-commit-config.yaml` being an + /// error and went one step too far: a tree with neither manager's config in + /// it produced an empty pin list, the lefthook note -- naming a file that + /// was not there either -- and a pass. `configs` is what tells the caller + /// apart from a tree that was actually read, and the caller refuses on it. + #[test] + fn no_hook_configuration_at_all_is_not_a_measurement() { + let dir = tree("nothing"); + let read = read_pins(&dir).unwrap(); + assert_eq!(read.configs, 0); + assert!(read.pins.is_empty(), "{:?}", read.pins); + // No note either: the one sentence available here ends by promising a + // lefthook config's `remotes:` were read, and there was no lefthook + // config. It is the caller's refusal that says what happened. + assert!(read.notes.is_empty(), "{:?}", read.notes); + } + /// A pin in `sub/` is a pin a run touches. /// /// The retired upstream read every `.pre-commit-config.yaml` in the work diff --git a/src/shim.rs b/src/shim.rs index dea08c8..e845023 100644 --- a/src/shim.rs +++ b/src/shim.rs @@ -152,6 +152,123 @@ fn in_list(list: &[String], needle: &str) -> bool { list.iter().any(|item| item == needle) } +/// Global options that take the word AFTER them, per command. +/// +/// Grammar rather than policy, which is why it lives here and not in a +/// `[[shim]]` table: a table names the flags whose values this shim publishes, +/// and nothing in one says what `git -c` does. Skipping only the table's own +/// flags left `git -c user.name=x push origin topic` reading `user.name=x` as +/// the verb and `push` as the noun -- a pair no `match` list contains, so the +/// shim decided a push to a public forge was none of its business and exec'd it +/// unexamined, printing nothing and exiting 0. +/// +/// `--git-dir`, `--work-tree`, `--namespace` and `--config-env` are documented +/// with an `=` and accepted both ways by `git.c`, so both spellings are here: +/// the `=` form is split off before this table is consulted. +const VALUE_OPTIONS: &[(&str, &[&str])] = &[ + ( + "git", + &[ + "-c", + "-C", + "--git-dir", + "--work-tree", + "--namespace", + "--config-env", + "--super-prefix", + ], + ), + ("gh", &["-R", "--repo"]), + ("glab", &["-R", "--repo"]), +]; + +/// Global options that take nothing, per command. +/// +/// Listed for the sake of the ones that are NOT listed. An option neither table +/// knows leaves the word after it ambiguous, and an ambiguity is reported out +/// loud -- so `git --no-pager status` would warn about a line with no +/// subcommand this shim wants, every time it is typed, if the harmless half of +/// git's grammar were left out. +/// +/// `--exec-path` with no `=` prints a path and exits rather than taking a +/// value, which is why it is on this side. +const BARE_OPTIONS: &[(&str, &[&str])] = &[ + ( + "git", + &[ + "-v", + "--version", + "-h", + "--help", + "-p", + "--paginate", + "-P", + "--no-pager", + "--bare", + "--exec-path", + "--html-path", + "--man-path", + "--info-path", + "--no-replace-objects", + "--no-lazy-fetch", + "--no-optional-locks", + "--no-advice", + "--literal-pathspecs", + "--no-literal-pathspecs", + "--glob-pathspecs", + "--noglob-pathspecs", + "--icase-pathspecs", + "--no-icase-pathspecs", + ], + ), + ("gh", &["--help", "--version"]), + ("glab", &["--help", "--version"]), +]; + +fn listed(table: &[(&str, &[&str])], command: &str, flag: &str) -> bool { + table + .iter() + .any(|(name, flags)| *name == command && flags.contains(&flag)) +} + +/// Whether an option before the subcommand takes the word after it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Arity { + /// It takes none, so the next word is the next word. + Bare, + /// It takes the word after it, which is therefore not a subcommand. + Value, + /// Nothing here can say, so both readings are possible and the shim has to + /// answer for both. + Unknown, +} + +/// One reading of argv: the first two words that are neither an option nor an +/// option's value, and the first option that could have been read either way. +#[derive(Debug)] +struct Words { + verb: String, + noun: String, + unclear: Option, +} + +/// What a shim can say about an invocation from argv alone. +#[derive(Debug)] +enum Reading { + /// A `match` entry names it, under a reading of the options this shim can + /// defend. + Named, + /// No entry names it, and every word before the subcommand was accounted + /// for. + Absent, + /// The subcommand could not be located. This option sits before it, nothing + /// here says whether the word after it is its value, and the two readings + /// disagree about which word the subcommand even is -- neither of them + /// matching. Not the same answer as `Absent`, and folding it into one is + /// how a guard reports a pass over an invocation it never identified. + Unclear(String), +} + impl Shim { /// Whether a flag this table names takes the word after it as its value. fn takes_value(&self, flag: &str) -> bool { @@ -161,8 +278,22 @@ impl Shim { || in_list(&self.path_flags, flag) } - /// The verb and the noun of an invocation: the first two words that are - /// neither an option nor an option's value. + /// What this option does to the word after it. + fn arity(&self, flag: &str) -> Arity { + if self.takes_value(flag) || listed(VALUE_OPTIONS, &self.command, flag) { + Arity::Value + } else if in_list(&self.skip_flags, flag) + || in_list(&self.web_flags, flag) + || listed(BARE_OPTIONS, &self.command, flag) + { + Arity::Bare + } else { + Arity::Unknown + } + } + + /// The verb and the noun of an invocation, under one reading of the options + /// nothing here can classify. /// /// Reading `argv[0]` and `argv[1]` is not the same question. Every one of /// these CLIs takes options before the subcommand, and `gh --repo @@ -171,15 +302,23 @@ impl Shim { /// decides the invocation is none of its business and execs a publishing /// command unexamined. Nothing is printed and the exit code is 0, which is /// the shape of failure this tool exists to refuse. - fn verb_noun(&self, argv: &[String]) -> (String, String) { - let mut words: Vec<&str> = Vec::new(); + /// + /// `unknown_takes_value` is the reading applied to an option neither this + /// table nor the grammar above names, and it is a parameter because neither + /// answer is safe alone: assume it takes nothing and `git -c user.name=x + /// push` loses `push`; assume it takes the next word and `gh --draft pr + /// create` loses `pr`. Both readings are tried, and where they disagree the + /// caller hears that rather than a verdict. + fn words(&self, argv: &[String], unknown_takes_value: bool) -> Words { + let mut found: Vec<&str> = Vec::new(); + let mut unclear: Option = None; let mut index = 0; while let Some(argument) = argv.get(index) { index += 1; // `--` ends the options. Everything after it is positional however // it is spelt. if argument == "--" { - words.extend( + found.extend( argv.get(index..) .unwrap_or_default() .iter() @@ -189,11 +328,7 @@ impl Shim { } if argument.starts_with('-') && argument != "-" { // `--flag=value` carries its value in the same word; `--flag - // value` takes the next one, and only this table knows which - // flags do. A flag it does not name is assumed to take none, - // which is the safe way to be wrong: the worst case is reading - // a value as a subcommand and checking an invocation that - // needed no checking. + // value` takes the next one. let inline = argument.starts_with("--") && argument.contains('='); let flag = if inline { argument @@ -202,29 +337,80 @@ impl Shim { } else { argument.as_str() }; - if !inline && self.takes_value(flag) { - index += 1; + if !inline { + match self.arity(flag) { + Arity::Value => index += 1, + Arity::Bare => {} + Arity::Unknown => { + // Only where there IS a word after it. An option at + // the end of argv took nothing whatever its grammar + // says, and `gh --version` is not an invocation + // whose subcommand went missing. + if argv.get(index).is_some() { + if unclear.is_none() { + unclear = Some(flag.to_owned()); + } + if unknown_takes_value { + index += 1; + } + } + } + } } continue; } - words.push(argument); - if words.len() == 2 { + found.push(argument); + if found.len() == 2 { break; } } - let mut words = words.into_iter(); - ( - words.next().unwrap_or_default().to_owned(), - words.next().unwrap_or_default().to_owned(), - ) + let mut found = found.into_iter(); + Words { + verb: found.next().unwrap_or_default().to_owned(), + noun: found.next().unwrap_or_default().to_owned(), + unclear, + } } - /// Whether this invocation is one the shim has anything to say about. - pub(crate) fn matches(&self, argv: &[String]) -> bool { - let (verb, noun) = self.verb_noun(argv); - in_list(&self.match_, "*") - || in_list(&self.match_, &format!("{verb}:{noun}")) - || in_list(&self.match_, &format!("{verb}:*")) + /// Whether a `match` entry names the pair one reading found. + fn names(&self, words: &Words) -> bool { + in_list(&self.match_, &format!("{}:{}", words.verb, words.noun)) + || in_list(&self.match_, &format!("{}:*", words.verb)) + } + + /// Whether this invocation is one the shim has anything to say about, and + /// where it cannot tell, that it cannot tell. + fn reading(&self, argv: &[String]) -> Reading { + if in_list(&self.match_, "*") { + return Reading::Named; + } + let bare = self.words(argv, false); + if self.names(&bare) { + return Reading::Named; + } + // One reading found nothing; the other is what an option that DOES take + // a value would have left, and a `match` hit under it is a hit. Matching + // under either reading errs towards checking, which is the direction + // this whole seam exists to err in. + let valued = self.words(argv, true); + if self.names(&valued) { + return Reading::Named; + } + // Nothing was read either way, so the answer is the answer. + let Some(flag) = bare.unclear else { + return Reading::Absent; + }; + // An option neither reading can classify leaves the SUBCOMMAND in doubt + // only where the two readings disagree about which word it is. `git log + // -1 --oneline` is `log` whether `-1` swallows the word after it or not, + // and `-1` sits after the subcommand besides. Reporting that as a + // could-not-look prints the refusal line over every ordinary command + // this shim exists to stay out of the way of -- which trains the reader + // to ignore the one invocation where the doubt is real. + if bare.verb == valued.verb && bare.noun == valued.noun { + return Reading::Absent; + } + Reading::Unclear(flag) } /// Walk argv once, reading the flags this table names. @@ -1025,14 +1211,42 @@ fn edit_and_check(root: &Path, policy: &Policy, name: &str, argv: &[String]) -> kind: "text", value: text, }; - let mut refusals: Vec = Vec::new(); - for rule in policy + // The same two kinds `run` consults, and for the reason the dispatch there + // gives: a guard cannot judge a body typed into an editor one way and the + // same body given with `--body` another under one id. Reading only `exec` + // here meant a policy whose checker for this command is a BUILT-IN had a + // checkpoint that opened an editor, read the file back, consulted nobody and + // exited 0. + let checkers: Vec<&Rule> = policy .before_command(name, &opened_for) - .filter(|rule| rule.is(Check::Exec)) - { + .filter(|rule| rule.is(Check::Exec) || rule.is(Check::Builtin)) + .collect(); + if checkers.is_empty() { + // Nothing to consult, and the text exists now: this is a checkpoint + // with nobody standing at it. Exit 2 rather than 0, because the command + // abandons what it was doing on any non-zero -- and a body that reached + // an editor installed by this shim and was then read by nothing must not + // leave here looking like a body that passed. + return Err(Fatal::new(format!( + "{name}: the editor closed on a body to publish, and no rule stands in front of \ + `{}` -- no `command.before` names it. Nothing was published, because nothing \ + would have been checked", + opened_for.join(" ") + ))); + } + let mut refusals: Vec = Vec::new(); + for rule in checkers { if crate::guard::bypassed(&rule.id) { continue; } + if rule.is(Check::Builtin) { + if let Some(refusal) = + crate::guard::text_refusal(root, rule, subject.kind, &subject.value)? + { + refusals.push(refusal.report); + } + continue; + } if let Some(refusal) = consult(root, rule, &subject)? { refusals.push(format!("{refusal}\n{}", rule.message())); } @@ -1200,7 +1414,22 @@ pub(crate) fn run( let mut collected = Collected::default(); let mut in_scope = false; - if shim.matches(&words) { + let reading = shim.reading(&words); + if let Reading::Unclear(flag) = &reading { + // Said out loud, and for the same reason the unresolvable-target arm in + // `in_scope` says its piece: the decision to run the command anyway is + // deliberate, and making it in silence is not available. Refusing here + // would stop every invocation carrying an option a release added to a + // command this shim stands in front of machine-wide; running one whose + // subcommand was never identified without a word about it is the shape + // of failure this tool refuses. + eprintln!( + "uphold shim: {name}: {flag} sits before the subcommand and nothing here says \ + whether it takes the word after it, so which subcommand this is could not be \ + established and no checker ran. This is not a pass." + ); + } + if matches!(reading, Reading::Named) { // The one place the bytes have to be text. This invocation is one the // shim reads values out of, and a value that is not UTF-8 cannot be // read as text -- checking the lossy copy would report a pass over @@ -1218,6 +1447,27 @@ pub(crate) fn run( } collected = shim.collect(root, &words)?; in_scope = shim.in_scope(root, &collected, &words)?; + // A `[[shim]]` that named this invocation and a policy with no rule + // standing in front of it: the shim collects the body, consults nobody, + // execs the command and exits 0 -- which is indistinguishable from a + // body every checker approved, and is the one outcome this tool exists + // to make impossible. `[[shim]]` says which command lines are checked + // before they are published; `command.before` says who checks them, and + // neither implies the other. The load refuses a `[[shim]]` whose command + // no rule names at all; this is the same reading one invocation later, + // where the rules that name the command do not name THIS command line. + // + // Out of scope is not this case: there the policy answered, and the + // answer was that these checks do not apply to this destination. + if in_scope && checkers.is_empty() { + return Err(Fatal::new(format!( + "{name}: `{}` is an invocation this repository's `[[shim]]` says is checked \ + before it is published, and no rule stands in front of it -- no \ + `command.before` names it. Nothing was published, because nothing would have \ + been checked", + words.join(" ") + ))); + } if in_scope { for subject in &collected.subjects { if subject.value.trim().is_empty() { @@ -1299,19 +1549,119 @@ mod tests { } } + /// A `git` shim as the shipped policy declares it: positional text, and + /// none of git's global grammar written into the table. + fn git_push() -> Shim { + Shim { + command: String::from("git"), + match_: vec!["push:*".into()], + text_flags: Vec::new(), + file_flags: Vec::new(), + path_flags: Vec::new(), + target_flags: Vec::new(), + skip_flags: Vec::new(), + web_flags: Vec::new(), + argv_subject: false, + editor_env: None, + target: Target::GitRemote, + scope: Scope::PublicTarget, + collect: Collect::GitRefs, + } + } + fn argv(line: &str) -> Vec { line.split_whitespace().map(str::to_owned).collect() } + fn named(shim: &Shim, line: &str) -> bool { + matches!(shim.reading(&argv(line)), Reading::Named) + } + #[test] fn a_named_subcommand_matches_and_an_unnamed_one_does_not() { // Named rather than pattern-matched: a shim that guesses which // subcommands carry text is one release away from missing a new one in // silence. - assert!(gh().matches(&argv("pr create"))); - assert!(gh().matches(&argv("issue comment"))); - assert!(!gh().matches(&argv("pr checkout"))); - assert!(!gh().matches(&argv("repo clone"))); + assert!(named(&gh(), "pr create")); + assert!(named(&gh(), "issue comment")); + assert!(!named(&gh(), "pr checkout")); + assert!(!named(&gh(), "repo clone")); + } + + #[test] + fn a_git_global_option_does_not_switch_the_push_shim_off() { + // The grammar a `[[shim]]` table does not carry and should not have to. + // Skipping only the flags the table names, `git -c user.name=x push + // origin topic` reads `user.name=x` as the verb -- no `match` entry + // contains that, so a push to a public forge exec'd unexamined, silently + // and with an exit code of 0. + for line in [ + "-c user.name=x push origin topic", + "-C /somewhere/else push origin topic", + "--git-dir /elsewhere/.git push", + "--git-dir=/elsewhere/.git push", + "--no-pager push origin topic", + "-c a=b -C /elsewhere --no-pager push", + ] { + assert!(named(&git_push(), line), "{line}"); + } + // And the half a looser matcher would lose. `Absent` rather than merely + // unnamed: knowing git's grammar is what makes this a decision instead + // of an ambiguity, so `-c` swallowing a value spelled like a subcommand + // is answered rather than warned about. + for line in ["-c user.name=push status", "-C /elsewhere log"] { + assert!( + matches!(git_push().reading(&argv(line)), Reading::Absent), + "{line}" + ); + } + } + + #[test] + fn an_option_nothing_can_classify_is_unclear_rather_than_absent() { + // A guard that could not tell which subcommand it was looking at has + // not established that this is none of its business. Both readings are + // tried first -- one of them matching IS an answer -- and only a line + // no reading names lands here. + assert!(matches!( + git_push().reading(&argv("--fictional-option value status")), + Reading::Unclear(flag) if flag == "--fictional-option" + )); + // A word that follows nothing took nothing, whatever its grammar says: + // `gh --version` is not an invocation whose subcommand went missing. + assert!(matches!( + gh().reading(&argv("--fictional-option")), + Reading::Absent + )); + // And an option a reading DOES resolve into a match is a match, not an + // ambiguity: erring towards checking is the direction this seam exists + // to err in. + assert!(named( + &git_push(), + "--fictional-option value push origin topic" + )); + } + + #[test] + fn an_option_after_the_subcommand_does_not_make_the_subcommand_unclear() { + // The doubt an unclassifiable option raises is doubt about WHICH WORD + // the subcommand is. An option that cannot move it -- because the + // subcommand was already read, or because swallowing the next word + // leaves the same pair -- raises none, and saying otherwise puts a + // could-not-look line on the terminal for `git log -1 --oneline`. A + // refusal a reader sees on every ordinary command is a refusal they + // stop reading, which costs the one invocation where it was true. + for line in [ + "log -1 --oneline", + "status --short --branch", + "diff --stat --cached", + "log --format=%H -5", + ] { + assert!( + matches!(git_push().reading(&argv(line)), Reading::Absent), + "{line}" + ); + } } #[test] @@ -1452,7 +1802,7 @@ mod tests { "-w issue comment", "-- pr create", ] { - assert!(gh().matches(&argv(line)), "{line}"); + assert!(named(&gh(), line), "{line}"); } // And it still says no to what it has nothing to say about, which is // the half a looser matcher would lose. @@ -1460,7 +1810,7 @@ mod tests { "--repo acme/widget pr checkout", "-R acme/widget repo clone", ] { - assert!(!gh().matches(&argv(line)), "{line}"); + assert!(!named(&gh(), line), "{line}"); } } @@ -1469,8 +1819,11 @@ mod tests { // `--title pr` puts the word `pr` in argv without the invocation being // about a pull request, and only this table knows that `--title` took // it. - let (verb, noun) = gh().verb_noun(&argv("--title pr create issue")); - assert_eq!((verb.as_str(), noun.as_str()), ("create", "issue")); + let words = gh().words(&argv("--title pr create issue"), false); + assert_eq!( + (words.verb.as_str(), words.noun.as_str()), + ("create", "issue") + ); } #[test] diff --git a/tests/config_cli.rs b/tests/config_cli.rs index 1be1b1c..d0857c7 100644 --- a/tests/config_cli.rs +++ b/tests/config_cli.rs @@ -110,22 +110,6 @@ text_flags = ["-t", "--title"] scope = "always" "#; -/// The rule under both cases below: a built-in that reads a push range, whose -/// only declared place is one no shim consults. -const PUSH_GUARD_IN_FRONT_OF_A_COMMAND: &str = r#" -[rule.push] -builtin = "prevent-public-push" - -[rule.push.command] -before = ["faux"] - -[[shim]] -command = "faux" -match = ["pr:create"] -text_flags = ["-t"] -scope = "always" -"#; - /// `command.before` on a check no shim consults, refused by the real binary. /// /// `shim::run` filters the rules it consults to `exec` checkers and @@ -136,7 +120,21 @@ scope = "always" /// is the one this rule walks past. #[test] fn a_command_place_no_seam_reads_stops_the_binary() { - let root = workspace(PUSH_GUARD_IN_FRONT_OF_A_COMMAND); + let root = workspace( + r#" + [rule.push] + builtin = "prevent-public-push" + + [rule.push.command] + before = ["faux"] + + [[shim]] + command = "faux" + match = ["pr:create"] + text_flags = ["-t"] + scope = "always" + "#, + ); // Asked of an entry point that only loads and prints, so what fails is the // load and not a check downstream of it. let output = uphold(&root, &["rules", "--effective"]); @@ -159,7 +157,21 @@ fn a_command_place_no_seam_reads_stops_the_binary() { /// running the command. #[test] fn the_shim_refuses_rather_than_running_a_command_that_rule_could_not_guard() { - let root = workspace(PUSH_GUARD_IN_FRONT_OF_A_COMMAND); + let root = workspace( + r#" + [rule.push] + builtin = "prevent-public-push" + + [rule.push.command] + before = ["faux"] + + [[shim]] + command = "faux" + match = ["pr:create"] + text_flags = ["-t"] + scope = "always" + "#, + ); let output = uphold(&root, &["shim", "faux", "pr", "create", "-t", "A title"]); assert_eq!(code(&output), 2, "{}", stderr(&output)); assert!( diff --git a/tests/guard_cli.rs b/tests/guard_cli.rs index 1e94304..bc7a4fa 100644 --- a/tests/guard_cli.rs +++ b/tests/guard_cli.rs @@ -868,3 +868,144 @@ fn no_global_identity_at_all_says_the_guard_did_not_run() { stderr(&output) ); } + +// ── the staged name scan: where a finding says it is ───────────────── + +/// A declared private owner needs no network and cannot be contradicted by one, +/// which is what lets these judge a real refusal without asking a forge. +const STAGED_NAMES: &str = r#" +[rule.no-private-repo-names-staged] +builtin = "no-private-repo-names-staged" +visibility = "public" +private_owners = ["acme-private"] + +[rule.no-private-repo-names-staged.git] +hooks = ["pre-commit"] +"#; + +#[test] +fn a_staged_finding_names_the_line_the_name_arrived_on() { + // The file alone leaves a reader searching a file they did not write for a + // name they have not seen -- and the sibling guard over the same text has + // named `path:line:column` since it was written. + let root = repository(STAGED_NAMES); + write( + &root, + "docs/note.md", + "one\ntwo\nwe hit this in acme-private/secret\nfour\n", + ); + git(&root, &["add", "docs/note.md"]); + + let output = guard(&root, &["--stage", "pre-commit"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("docs/note.md:3"), + "{}", + stderr(&output) + ); +} + +#[test] +fn an_added_line_that_looks_like_a_diff_header_is_still_read() { + // Every added line is spelled with ONE leading `+`, so a line whose own + // first two characters are `++` reaches the reader as `+++...` -- exactly + // like the `+++ b/path` header. The reader excepted `+++` to skip that + // header and skipped the content with it, so a name written after `++` in a + // changelog or a diff quoted in a document was never looked at, and the + // guard exited 0 over it. + let root = repository(STAGED_NAMES); + write( + &root, + "CHANGELOG.md", + "notes\n++ ported from acme-private/secret\n", + ); + git(&root, &["add", "CHANGELOG.md"]); + + let output = guard(&root, &["--stage", "pre-commit"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("CHANGELOG.md:2"), + "{}", + stderr(&output) + ); +} + +#[test] +fn a_one_line_hunk_in_a_file_that_was_already_there_is_numbered() { + // `-U0` spells a one-line change `@@ -7 +7 @@`, with no count and no comma + // anywhere in it, and every hunk this guard reads is asked for that way. The + // number also has to come from the hunk rather than from counting the diff: + // the name below is on line 7 of the file and on the sixth line of the diff. + let root = repository(STAGED_NAMES); + write(&root, "notes.md", "a\nb\nc\nd\ne\nf\ng\nh\n"); + git(&root, &["add", "notes.md"]); + git(&root, &["commit", "-qm", "one", "--no-verify"]); + + write( + &root, + "notes.md", + "a\nb\nc\nd\ne\nf\nsee acme-private/secret\nh\n", + ); + git(&root, &["add", "notes.md"]); + + let output = guard(&root, &["--stage", "pre-commit"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("notes.md:7"), + "{}", + stderr(&output) + ); +} + +#[test] +fn a_diff_attribute_does_not_take_the_line_number_away() { + // The second pass, which forces `--text` over a `diff` attribute, reads its + // hunks through the same reader as the first. It used to hand back one blob + // per file, so the half of the scan that exists FOR a suppressed diff was + // also the half whose findings said least about where they were. + let root = repository(STAGED_NAMES); + write(&root, ".gitattributes", "* -diff\n"); + git(&root, &["add", ".gitattributes"]); + git(&root, &["commit", "-qm", "attributes", "--no-verify"]); + + write( + &root, + "docs/note.md", + "one\ntwo\nwe hit this in acme-private/secret\n", + ); + git(&root, &["add", "docs/note.md"]); + + let output = guard(&root, &["--stage", "pre-commit"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("docs/note.md:3"), + "{}", + stderr(&output) + ); +} + +#[test] +fn a_personal_textconv_cannot_blind_the_listing_the_scan_starts_from() { + // The twin of the external-diff case, one call earlier: the path listing and + // the numstat verdict decide WHICH files the reader is ever pointed at, so a + // diff driver that reached them would take a file out of the scan before the + // flags on the patch call could matter. + let root = repository(STAGED_NAMES); + git(&root, &["config", "diff.external", "true"]); + git(&root, &["config", "diff.render.textconv", "true"]); + write(&root, ".gitattributes", "* diff=render\n"); + write( + &root, + "docs/note.md", + "one\nwe hit this in acme-private/secret\n", + ); + git(&root, &["add", "-A"]); + + let output = guard(&root, &["--stage", "pre-commit"]); + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("docs/note.md:2"), + "{}", + stderr(&output) + ); +} diff --git a/tests/hook_pins_cli.rs b/tests/hook_pins_cli.rs index d6a94a2..8b294cb 100644 --- a/tests/hook_pins_cli.rs +++ b/tests/hook_pins_cli.rs @@ -130,9 +130,22 @@ fn a_pin_whose_remote_cannot_be_reached_is_could_not_look_and_not_a_pass() { /// `read_pins` opened `root/.pre-commit-config.yaml` unconditionally and /// `read_to_string` turns ENOENT into a `Fatal`, so this guard exited 2 for /// every consumer who installed the way the documentation tells them to. +/// +/// The tree carries the lefthook config that install path leaves behind, and a +/// current pin inside it. Written over an EMPTY tree, this test asserted the +/// pass belonging to a consumer it was not standing in for -- see below. #[test] -fn a_tree_with_no_pre_commit_config_passes_and_says_why() { +fn a_lefthook_only_tree_passes_and_says_why_there_are_no_pre_commit_pins() { let root = repository(); + let url = upstream(&root, &["v1.0.0"]); + write( + &root, + "lefthook.yml", + &format!( + "remotes:\n - git_url: {url}\n ref: v1.0.0\n configs:\n - lefthook.yml\n" + ), + ); + let output = guard(&root); let report = text(&output); assert_eq!(output.status.code().unwrap(), 0, "{report}"); @@ -140,6 +153,73 @@ fn a_tree_with_no_pre_commit_config_passes_and_says_why() { assert!(report.contains("lefthook-only"), "{report}"); } +/// No file to read a pin out of is not a repository whose pins are current. +/// +/// This is the opposite half of the finding above, and the repair for that one +/// created it: with the unconditional open gone, a tree holding NEITHER +/// manager's configuration produced zero pins, printed the note about the +/// documented lefthook-only path -- citing a lefthook config that was not there +/// either -- and exited 0. The walk skips gitignored files, so a +/// `.pre-commit-config.yaml` added to `.gitignore` arrives here as this exact +/// state, as does one renamed, moved above the root, or dropped in a merge: +/// every hook in the repository still runs pinned code, and nothing is now +/// watching the pin. +#[test] +fn a_tree_with_no_hook_configuration_at_all_is_could_not_look_and_not_a_pass() { + let root = repository(); + + let output = guard(&root); + let report = text(&output); + assert_eq!( + output.status.code().unwrap(), + 2, + "no file to read a pin out of is not a pass:\n{report}" + ); + assert!(report.contains("established nothing"), "{report}"); + assert!(report.contains("Could not look is not a pass"), "{report}"); + // Both spellings, because a reader who has neither file has to be told + // which two files would have been read. + assert!(report.contains(".pre-commit-config.yaml"), "{report}"); + assert!(report.contains("lefthook.yml"), "{report}"); +} + +/// A finding does not cancel the pins the run never reached. +/// +/// `unchecked` went to stderr from inside the guard, before `guard::run` +/// printed the refusal it qualifies, and never entered `Refusal::report` -- +/// which is documented as carrying the whole report precisely so a reader does +/// not have to go and find the rest. So the caveat arrived detached from the +/// finding, out of order, and only for a caller watching that stream, while the +/// exit code said 1: checked, and here is what is wrong. It was measured over +/// fewer pins than the tree holds. +#[test] +fn a_finding_beside_an_unreachable_remote_carries_the_pin_it_could_not_check() { + let root = repository(); + let url = upstream(&root, &["v1.0.0", "v2.0.0"]); + let nowhere = root.join("no-such-upstream"); + write( + &root, + ".pre-commit-config.yaml", + &format!( + "repos:\n - repo: {url}\n rev: v1.0.0\n hooks:\n - id: x\n \ + - repo: {}\n rev: v1.0.0\n hooks:\n - id: y\n", + nowhere.display() + ), + ); + + let output = guard(&root); + let report = text(&output); + // A violation outranks an unread surface, which is the rule `audit::verdict` + // states and the reason this is 1 rather than 2: something WAS found. + assert_eq!(output.status.code().unwrap(), 1, "{report}"); + assert!(report.contains("v2.0.0 is newer"), "{report}"); + assert!( + report.contains("established nothing about them"), + "the pin nobody could reach has to travel with the finding:\n{report}" + ); + assert!(report.contains("no-such-upstream"), "{report}"); +} + /// The one version a lefthook consumer pins, which nothing was reading. /// /// A `remotes:` entry is a pin in every sense this guard means: it names diff --git a/tests/root_cli.rs b/tests/root_cli.rs new file mode 100644 index 0000000..484f858 --- /dev/null +++ b/tests/root_cli.rs @@ -0,0 +1,415 @@ +//! CLI-level tests for the two questions every subcommand answers before it +//! looks at anything: WHICH repository is this about, and can argv even be read. +//! +//! At the CLI and not at `discover`/`root_of`, because both failures are only +//! visible from outside. A root taken from the enclosing superproject is a +//! function returning a perfectly ordinary `PathBuf`; what makes it a bug is +//! that the process then prints findings about files that are not in the +//! repository the command was run in. And an argv that is not UTF-8 does not +//! return anything at all -- it panics, and the only place exit 101 exists is in +//! the process's status. + +#![expect( + clippy::let_underscore_must_use, + clippy::tests_outside_test_module, + clippy::unwrap_used, + reason = "A CLI test asserts on the outcome; a panic in the harness that builds the fixture IS the failure report, and there is no caller to hand a Result to" +)] + +use std::ffi::{OsStr, OsString}; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A rule that names a file, so a report about the WRONG tree is legible as +/// one: an exit code alone cannot say which repository was walked. +/// +/// The policy file writes the pattern it matches on, so it excludes itself -- +/// the same exclusion every bundled rule carries, for the same reason. +const POLICY: &str = r#" +[rule.no-todo] +message = "no TODO" +regexp = 'TODO' + +[rule.no-todo.files] +exclude = ["policy/**"] +"#; + +/// One directory per case, under a name no other test file claims. The suite +/// runs in parallel threads of one process, so a path keyed on the process id +/// alone is the SAME path for every case and one case reads the tree another is +/// still building. +fn workspace() -> PathBuf { + static NEXT: AtomicUsize = AtomicUsize::new(0); + let root = std::env::temp_dir().join(format!( + "uphold-root-cli-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(&root).unwrap(); + root +} + +fn write(directory: &Path, relative: &str, contents: &str) { + let path = directory.join(relative); + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent).unwrap(); + } + std::fs::write(path, contents).unwrap(); +} + +fn write_policy(directory: &Path) { + write(directory, "policy/principles.toml", POLICY); +} + +/// Where a repository begins, as an ordinary clone spells it. +fn mark_repository(directory: &Path) { + std::fs::create_dir_all(directory.join(".git")).unwrap(); +} + +fn uphold>(working: &Path, arguments: &[S]) -> Output { + Command::new(env!("CARGO_BIN_EXE_uphold")) + .args(arguments) + .current_dir(working) + // A guard bypass leaking in from the developer's shell would turn a + // refusal these cases assert on into a pass. + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap() +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +/// `Option`, not `unwrap`: a process killed by a signal has no code, and the +/// panic-vs-exit cases below are exactly where that distinction matters. +fn code(output: &Output) -> Option { + output.status.code() +} + +// --- the repository boundary ------------------------------------------------ + +/// The live failure: a repository with no policy of its own, inside one that +/// has a policy. +/// +/// The walk used to climb past the inner repository's root, load the +/// superproject's policy and adopt the SUPERPROJECT'S directory as root -- so +/// the run scanned a different tree and the report named files outside the +/// repository the command was run in, under this repository's name. Asserting +/// on the exit code alone would not catch it: the give-away is the file name. +#[test] +fn a_repository_with_no_policy_is_not_checked_against_the_superprojects() { + let superproject = workspace(); + write_policy(&superproject); + write( + &superproject, + "outside.txt", + "TODO: the superproject's own\n", + ); + + let inner = superproject.join("inner"); + mark_repository(&inner); + write(&inner, "inside.txt", "nothing to find here\n"); + + // From the repository root, and from a directory below it -- the climb is + // the same walk either way, and the boundary has to stop both. + for working in [inner.clone(), inner.join("src")] { + std::fs::create_dir_all(&working).unwrap(); + let output = uphold(&working, &["scan"]); + assert_eq!(code(&output), Some(2), "{}", stderr(&output)); + assert!( + stderr(&output).contains("no policy in this repository"), + "{}", + stderr(&output) + ); + assert!( + !stderr(&output).contains("outside.txt"), + "reported on the superproject's tree: {}", + stderr(&output) + ); + assert!( + !stdout(&output).contains("policy checks passed"), + "a repository with nothing to check against is not a pass: {}", + stdout(&output) + ); + } +} + +/// A `.git` FILE is the boundary a `.git` directory is. +/// +/// That is what a linked worktree and a submodule have where a clone has a +/// directory, and a check that asked `is_dir` would walk straight out of both +/// -- which is precisely the shape the superproject case is made of. +#[test] +fn a_git_file_stops_the_walk_the_way_a_git_directory_does() { + let superproject = workspace(); + write_policy(&superproject); + write( + &superproject, + "outside.txt", + "TODO: the superproject's own\n", + ); + + let submodule = superproject.join("submodule"); + std::fs::create_dir_all(&submodule).unwrap(); + std::fs::write( + submodule.join(".git"), + "gitdir: ../.git/modules/submodule\n", + ) + .unwrap(); + + let output = uphold(&submodule, &["scan"]); + assert_eq!(code(&output), Some(2), "{}", stderr(&output)); + assert!( + stderr(&output).contains("no policy in this repository"), + "{}", + stderr(&output) + ); + assert!( + !stderr(&output).contains("outside.txt"), + "a submodule borrowed the superproject's policy: {}", + stderr(&output) + ); +} + +/// Every entry point that resolves a root, not just `scan`. +/// +/// They each call the same walk, and a fix applied at one call site and not the +/// others would leave `guard` refusing a commit on the strength of a policy +/// belonging to a tree the committer has never opened. +#[test] +fn every_subcommand_that_resolves_a_root_stops_at_the_boundary() { + let superproject = workspace(); + write_policy(&superproject); + let inner = superproject.join("inner"); + mark_repository(&inner); + + for arguments in [ + vec!["scan"], + vec!["guard", "--stage", "manual"], + vec!["audit", "--for-publication"], + vec!["check"], + vec!["check", "--coverage"], + vec!["rules", "--effective"], + // Asked for BY NAME, an absent policy is an error rather than a + // passthrough: the caller asked this repository for a shim, and the + // answer is that this repository declares none -- not the + // superproject's answer to the same question. + vec!["shim", "faux", "--version"], + ] { + let output = uphold(&inner, &arguments); + assert_eq!(code(&output), Some(2), "{arguments:?}: {}", stderr(&output)); + assert!( + stderr(&output).contains("no policy in this repository"), + "{arguments:?}: {}", + stderr(&output) + ); + } +} + +/// The boundary is where the walk STOPS, not a refusal to look: a repository +/// carrying its own policy is the ordinary case, and it is checked against its +/// own -- reporting its own files and none of the superproject's. +#[test] +fn a_repository_with_its_own_policy_reports_its_own_files_and_no_others() { + let superproject = workspace(); + write_policy(&superproject); + write( + &superproject, + "outside.txt", + "TODO: the superproject's own\n", + ); + + let inner = superproject.join("inner"); + mark_repository(&inner); + write_policy(&inner); + write(&inner, "inside.txt", "TODO: this repository's own\n"); + + let below = inner.join("src"); + std::fs::create_dir_all(&below).unwrap(); + + let output = uphold(&below, &["scan"]); + assert_eq!(code(&output), Some(1), "{}", stderr(&output)); + assert!( + stderr(&output).contains("inside.txt"), + "{}", + stderr(&output) + ); + assert!( + !stderr(&output).contains("outside.txt"), + "named a file outside the repository it was run in: {}", + stderr(&output) + ); +} + +// --- --policy, and the root it implies -------------------------------------- + +/// `--policy` used to take the file's grandparent as the root and check +/// nothing, so `--policy principles.toml` rooted the scan at the repository's +/// PARENT and the default include of `["."]` then walked that. A root that +/// cannot be established is exit 2: scanning the wrong tree and reporting on it +/// is worse than saying the layout was not understood. +#[test] +fn an_explicit_policy_off_the_layout_is_refused_rather_than_rooted_elsewhere() { + let root = workspace(); + // Deliberately NOT under `policy/`: this is the layout that used to make + // the temporary directory's parent the tree under test. + write(&root, "principles.toml", POLICY); + write(&root, "a.txt", "TODO: here\n"); + + for given in ["principles.toml", "./principles.toml"] { + let output = uphold(&root, &["scan", "--policy", given]); + assert_eq!(code(&output), Some(2), "{given}: {}", stderr(&output)); + assert!( + stderr(&output).contains("/policy/.toml"), + "{given}: {}", + stderr(&output) + ); + assert!( + !stdout(&output).contains("policy checks passed"), + "{given}: a layout that was not understood is not a pass: {}", + stdout(&output) + ); + } +} + +/// The layout it does accept, and the root it names -- asked from a +/// subdirectory, because the root has to come from where the policy file sits +/// and not from where the command was typed. +#[test] +fn an_explicit_policy_in_the_layout_roots_at_the_repository() { + let root = workspace(); + write_policy(&root); + write(&root, "a.txt", "TODO: here\n"); + let below = root.join("src"); + std::fs::create_dir_all(&below).unwrap(); + + let output = uphold(&below, &["scan", "--policy", "../policy/principles.toml"]); + assert_eq!(code(&output), Some(1), "{}", stderr(&output)); + assert!( + stderr(&output).contains("a.txt"), + "the root is where the policy file says it is: {}", + stderr(&output) + ); +} + +// --- argv that is not text -------------------------------------------------- + +/// `std::env::args()` PANICS on an argument that is not Unicode. +/// +/// Exit 101, out of a binary that promises three exit codes and is installed in +/// front of `git`, `gh` and `npm` -- where a path spelled in latin-1 is an +/// ordinary thing to be handed. Every assertion here is on the CODE and not +/// only on the message, because 101 is the whole finding: a panic also writes +/// to stderr, and a test that only read stderr would pass on one. +#[test] +fn an_argument_that_is_not_text_is_exit_two_and_never_a_panic() { + use std::os::unix::ffi::OsStringExt; + + let root = workspace(); + write_policy(&root); + // `caf\xe9`, a perfectly good file name that is not UTF-8. + let latin1 = OsString::from_vec(b"caf\xe9".to_vec()); + + for arguments in [ + // A subcommand name. + vec![latin1.clone()], + // An option name, past a subcommand that parses its own arguments. + vec![OsString::from("scan"), latin1.clone()], + vec![OsString::from("guard"), latin1.clone()], + // A value that has to be text to mean anything: a rule-set name is + // matched against literals, so bytes that spell none of them name + // nothing this binary has. + vec![ + OsString::from("rules"), + OsString::from("--set"), + latin1.clone(), + ], + // The command a shim stands in front of. + vec![OsString::from("shim"), latin1], + ] { + let output = uphold(&root, &arguments); + assert_eq!(code(&output), Some(2), "{arguments:?}: {}", stderr(&output)); + assert!( + !stderr(&output).contains("panicked"), + "{arguments:?}: {}", + stderr(&output) + ); + } +} + +/// A link named in bytes that are not text names no command, and says so. +/// +/// argv[0] decides which shim this binary is, and it is read before anything +/// else -- so it was the first thing `std::env::args()` panicked on, in the one +/// invocation shape this binary is installed as. +#[test] +fn a_link_whose_name_is_not_text_is_refused_rather_than_a_panic() { + use std::os::unix::ffi::OsStringExt; + + let root = workspace(); + write_policy(&root); + let mut name = OsString::from_vec(b"caf\xe9".to_vec()); + let link = { + let mut path = root.clone().into_os_string(); + path.push("/"); + path.push(&mut name); + PathBuf::from(path) + }; + std::os::unix::fs::symlink(env!("CARGO_BIN_EXE_uphold"), &link).unwrap(); + + let output = Command::new(&link) + .arg("--version") + .current_dir(&root) + .env_remove("UPHOLD_ALLOW") + .output() + .unwrap(); + assert_eq!(code(&output), Some(2), "{}", stderr(&output)); + assert!( + stderr(&output).contains("not valid UTF-8"), + "{}", + stderr(&output) + ); + assert!(!stderr(&output).contains("panicked"), "{}", stderr(&output)); +} + +/// The other half of the same promise: where the bytes are a PATH rather than a +/// name, they are not converted at all. +/// +/// A lossy conversion here would leave the binary opening `caf\u{FFFD}.toml`, +/// which does not exist -- so the run would fail with the wrong reason, or +/// worse, be read as "no policy" and treated as nothing to check. The policy +/// file opens because it kept the bytes it was given, and the proof is that the +/// scan it drives finds the violation. +#[test] +fn a_policy_path_that_is_not_text_still_opens_the_file_it_names() { + use std::os::unix::ffi::OsStringExt; + + let root = workspace(); + std::fs::create_dir_all(root.join("policy")).unwrap(); + let policy = { + let mut path = root.join("policy").into_os_string(); + path.push("/"); + path.push(OsString::from_vec(b"caf\xe9.toml".to_vec())); + PathBuf::from(path) + }; + std::fs::write(&policy, POLICY).unwrap(); + write(&root, "a.txt", "TODO: here\n"); + + let output = uphold( + &root, + &[ + OsString::from("scan"), + OsString::from("--policy"), + policy.into_os_string(), + ], + ); + assert_eq!(code(&output), Some(1), "{}", stderr(&output)); + assert!(stderr(&output).contains("a.txt"), "{}", stderr(&output)); +} diff --git a/tests/shim_cli.rs b/tests/shim_cli.rs index cb0ad20..62f19e1 100644 --- a/tests/shim_cli.rs +++ b/tests/shim_cli.rs @@ -542,6 +542,162 @@ fn a_clean_body_reaches_the_real_command_through_a_builtin_checker() { assert!(stdout(&output).contains("faux ran"), "{}", stdout(&output)); } +/// A `git` shim as the shipped policy declares it, with git's own global +/// grammar written nowhere in it. +const GIT_POLICY: &str = r#" +[rule.no-published-markers] +message = "remove the marker" +exec = "uphold guard --text -" + +[rule.no-published-markers.command] +before = ["git"] + +[rule.prevent-ai-author] +builtin = "prevent-ai-author" + +[rule.prevent-ai-author.git] +hooks = ["commit-msg"] + +[[shim]] +command = "git" +match = ["push:*"] +text_flags = ["-m"] +scope = "always" +"#; + +/// A stub for a command the workspace does not install by default. +fn stub(root: &Path, name: &str, script: &str) { + let path = root.join("bin").join(name); + std::fs::write(&path, script).unwrap(); + let mut permissions = std::fs::metadata(&path).unwrap().permissions(); + std::os::unix::fs::PermissionsExt::set_mode(&mut permissions, 0o755); + std::fs::set_permissions(&path, permissions).unwrap(); +} + +#[test] +fn a_git_global_option_before_the_subcommand_does_not_switch_the_shim_off() { + // A `[[shim]]` table names the flags whose values it publishes; nothing in + // one says what `git -c` does, and no repository should have to write git's + // grammar into its policy to be guarded. Skipping only the table's flags, + // `git -c user.name=x push ...` reads `user.name=x` as the verb -- no + // `match` entry contains that, so a push to a public forge exec'd + // unexamined, silently and with an exit code of 0. + let root = workspace(GIT_POLICY); + stub(&root, "git", "#!/bin/sh\necho \"git ran: $*\"\n"); + for form in [ + vec![ + "git", + "-c", + "user.name=x", + "push", + "-m", + "Generated with Claude Code", + ], + vec![ + "git", + "-C", + "/somewhere/else", + "push", + "-m", + "Generated with Claude Code", + ], + vec![ + "git", + "--git-dir", + "/elsewhere/.git", + "push", + "-m", + "Generated with Claude Code", + ], + ] { + let output = shim(&root, &form); + assert_eq!(code(&output), 1, "{form:?}: {}", stderr(&output)); + assert!(!stdout(&output).contains("git ran:"), "{form:?}"); + } + + // And the half a looser matcher would lose: `-c` takes the word after it, + // so `status` there is a value and not a subcommand this shim checks. Read + // as a decision rather than as an ambiguity -- knowing git's grammar is + // what keeps every `git -c ... status` on the machine quiet. + let output = shim(&root, &["git", "-c", "user.name=push", "status"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("git ran:"), "{}", stdout(&output)); + assert!( + !stderr(&output).contains("This is not a pass."), + "{}", + stderr(&output) + ); +} + +#[test] +fn an_option_nothing_can_classify_is_said_out_loud_rather_than_passed_in_silence() { + // Neither reading of `--fictional value` names an invocation this shim + // checks, so which subcommand this even is was never established. Running + // it anyway is deliberate -- the link is on PATH for the whole machine, and + // refusing every command that grows an option would make the guard the + // reason work stops -- but running it in silence is the shape of failure + // this tool refuses. + let root = workspace(POLICY); + let output = shim(&root, &["faux", "--fictional", "value", "repo", "clone"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("faux ran:"), "{}", stdout(&output)); + assert!( + stderr(&output).contains("This is not a pass."), + "{}", + stderr(&output) + ); +} + +/// A shim that names this invocation, and a checker that names another one. +/// +/// The load refuses a `[[shim]]` whose command no rule names at all. This is +/// the same reading one invocation later: `faux pr create` is checked, and +/// `faux issue create` -- which the same `match` list names -- is collected, +/// consulted by nobody, and exec'd. +const NARROWED_CHECKER: &str = r#" +[rule.no-published-markers] +message = "remove the marker" +exec = "uphold guard --text -" + +[rule.no-published-markers.command] +before = ["faux pr create"] + +[[shim]] +command = "faux" +match = ["pr:create", "issue:*"] +text_flags = ["-t", "--title", "-b", "--body"] +scope = "always" +"#; + +#[test] +fn an_invocation_no_checker_stands_in_front_of_is_not_a_pass() { + let root = workspace(NARROWED_CHECKER); + + // The invocation a rule does name is checked, and runs. + let output = shim(&root, &["faux", "pr", "create", "-t", "An ordinary title"]); + assert_eq!(code(&output), 0, "{}", stderr(&output)); + assert!(stdout(&output).contains("faux ran:"), "{}", stdout(&output)); + + // The one it does not is exit 2 and no command at all. A body collected and + // consulted by nobody exits 0 otherwise, which is indistinguishable from a + // body every checker approved. + let output = shim( + &root, + &["faux", "issue", "create", "-t", "An ordinary title"], + ); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux ran:"), + "{}", + stdout(&output) + ); + assert!( + stderr(&output).contains("nothing would have been checked"), + "{}", + stderr(&output) + ); +} + #[test] fn a_builtin_checker_satisfies_the_shim_that_would_otherwise_check_nothing() { // The load refuses a shim no checker names, because a command collected and diff --git a/tests/shim_handoff_cli.rs b/tests/shim_handoff_cli.rs index 3ed7737..1a353ae 100644 --- a/tests/shim_handoff_cli.rs +++ b/tests/shim_handoff_cli.rs @@ -452,6 +452,103 @@ fn an_editor_that_writes_something_ordinary_is_left_alone() { ); } +/// A checker that is a BUILT-IN rather than a program this repository names. +const BUILTIN_RULE: &str = r#" +[rule.no-private-repo-names] +builtin = "no-private-repo-names" +visibility = "public" +private_owners = ["acme-private"] + +[rule.no-private-repo-names.command] +before = ["faux"] +"#; + +#[test] +fn a_builtin_checker_stands_at_the_editor_checkpoint_as_well() { + // The editor round trip consulted `exec` rules and nothing else, so a + // repository whose checker for this command is a built-in got a checkpoint + // with nobody standing at it: the shim installed itself as the editor, ran + // it, read the file back, consulted zero rules and exited 0. That is the + // same guard judging a body one way through `--body` and another through the + // editor, under one id. + let root = workspace( + &format!("{BUILTIN_RULE}{EDITOR_POLICY}"), + &[ + ("faux", EDITING_COMMAND), + ( + "private-editor", + "#!/bin/sh\nprintf 'this fixes acme-private/thing\\n' > \"$1\"\n", + ), + ], + ); + let editor = root.join("bin/private-editor"); + let output = Run { + args: &["faux", "pr", "create"], + envs: &[("EDITOR", &editor.to_string_lossy())], + ..Run::default() + } + .go(&root); + + assert_ne!(code(&output), 0, "{}", stderr(&output)); + assert!( + !stdout(&output).contains("faux published:"), + "{}", + stdout(&output) + ); + assert!( + stderr(&output).contains("acme-private"), + "{}", + stderr(&output) + ); +} + +/// A checker that names one command line, standing in front of a shim that +/// names more than one. +const NARROWED_RULE: &str = r#" +[rule.no-published-markers] +message = "remove the marker" +exec = "uphold guard --text -" + +[rule.no-published-markers.command] +before = ["faux pr create"] +"#; + +#[test] +fn an_editor_checkpoint_with_nothing_to_consult_is_not_a_pass() { + // Re-entered as the editor for a command line no rule stands in front of -- + // here because the argv the editor was opened for did not survive into this + // process, which is how the checkers that ran on the way in are chosen. The + // body exists by now and nobody is left to read it, so exit 2: the command + // abandons what it was doing on any non-zero, and a body read by nothing + // must not leave here looking like a body that passed. + let root = workspace( + &format!("{NARROWED_RULE}{EDITOR_POLICY}"), + &[ + ("faux", EDITING_COMMAND), + ( + "clean-editor", + "#!/bin/sh\nprintf 'An ordinary body\\n' > \"$1\"\n", + ), + ], + ); + let editor = root.join("bin/clean-editor"); + let output = Run { + args: &["faux", "body.md"], + envs: &[ + ("UPHOLD_SHIM_EDITOR", "faux"), + ("UPHOLD_SHIM_EDITOR_REAL", &editor.to_string_lossy()), + ], + ..Run::default() + } + .go(&root); + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!( + stderr(&output).contains("nothing would have been checked"), + "{}", + stderr(&output) + ); +} + #[test] fn a_body_given_on_the_command_line_does_not_open_an_editor_at_all() { // The editor is installed for the one case that needs it. A body already in diff --git a/tests/test_uphold_check.py b/tests/test_uphold_check.py index 6bd1445..242e86c 100644 --- a/tests/test_uphold_check.py +++ b/tests/test_uphold_check.py @@ -7,6 +7,8 @@ from __future__ import annotations +import contextlib +import io import json import subprocess import sys @@ -14,6 +16,7 @@ import textwrap import unittest from pathlib import Path +from unittest import mock ROOT = Path(__file__).resolve().parents[1] SCRIPT = ROOT / "uphold_check.py" @@ -113,6 +116,20 @@ def needs_the_engine(test): """ +# A repository both readers can be asked about at once: one guard whose stage a +# pinned id installs, and one content rule the pinned scan runs. Two seams, so an +# export that lost either would still look plausible on its own. +DIFFERENTIAL_DECLARATION = """ +[[enforce]] +principle = "fail-safe-defaults" +rule = "prevent-public-push" + +[[enforce]] +principle = "explicit-unknown" +rule = "no-merge-conflict-markers" +""" + + def run(cwd: Path, *args: str) -> subprocess.CompletedProcess: return subprocess.run( [sys.executable, str(SCRIPT), *args], @@ -268,3 +285,151 @@ def test_a_refused_declaration_exports_nothing(self): result = run(Path(tmp), "--oscal") self.assertEqual(result.returncode, 1, result.stdout) self.assertEqual(result.stdout.strip(), "") + + +@needs_the_engine +class TheTwoReadersOfOneReport(unittest.TestCase): + """The binary decides which rules run; the export must carry that answer whole. + + Since the reconcile moved into the loader, this script no longer derives a + rule set of its own -- it PARSES the binary's report. That closes the old + disagreement and opens a quieter one: a parser that stops recognising the + report it reads produces an empty answer rather than an error, and an empty + answer here is a component definition with no components, published at exit + 0 over a repository whose reconcile had just succeeded on every claim. + """ + + def setUp(self): + self._directory = tempfile.TemporaryDirectory() + self.tmp = Path(self._directory.name) + self.addCleanup(self._directory.cleanup) + build( + self.tmp, + DIFFERENTIAL_DECLARATION, + **{ + ".pre-commit-config.yaml": PRE_COMMIT_WITH_PRINCIPLES, + "policy__principles.toml": GUARD_POLICY + HYGIENE_BASE, + }, + ) + + def test_the_binary_and_the_export_name_the_same_rules(self): + """The differential: one fixture, both readers, one rule set. + + The binary's half is read from its own header count and not from the + evidence lines, so this does not check the parser against itself. + """ + check = uphold_check.engine(self.tmp, "check") + self.assertEqual(check.returncode, 0, check.stderr) + self.assertIn("reconciled 2 enforcement claims:", check.stdout) + + exported = run(self.tmp, "--oscal") + self.assertEqual(exported.returncode, 0, exported.stderr) + requirements = [ + requirement + for component in json.loads(exported.stdout)["component-definition"][ + "components" + ] + for implementation in component["control-implementations"] + for requirement in implementation["implemented-requirements"] + ] + self.assertEqual(len(requirements), 2, exported.stdout) + rules = { + prop["value"] + for requirement in requirements + for prop in requirement["props"] + if prop["name"] == "rule-id" + } + self.assertEqual(rules, {"prevent-public-push", "no-merge-conflict-markers"}) + + def test_a_report_this_reader_cannot_parse_is_could_not_look(self): + """One word of the report changes, and the reader recovers nothing. + + Before the count check this returned `{}`, which is indistinguishable + from a repository where no claim is supplied by anything. + """ + real = uphold_check.engine + + def drifted(root, *args): + answered = real(root, *args) + if args[:1] == ("check",): + answered.stdout = answered.stdout.replace("enforced by", "supplied by") + return answered + + with ( + mock.patch.object(uphold_check, "engine", drifted), + self.assertRaises(uphold_check.CouldNotLook) as raised, + ): + uphold_check.engine_suppliers(self.tmp) + self.assertIn("2 reconciled claim(s)", str(raised.exception)) + self.assertIn("0 evidence line(s)", str(raised.exception)) + + def test_a_drifted_report_publishes_no_component_definition(self): + """Could not look is exit 2 and no document, not exit 0 and an empty one.""" + real = uphold_check.engine + + def drifted(root, *args): + answered = real(root, *args) + if args[:1] == ("check",): + answered.stdout = answered.stdout.replace("enforced by", "supplied by") + return answered + + stdout = io.StringIO() + with ( + contextlib.chdir(self.tmp), + contextlib.redirect_stdout(stdout), + contextlib.redirect_stderr(io.StringIO()), + mock.patch.object(uphold_check, "engine", drifted), + ): + code = uphold_check.main(["--oscal"]) + self.assertEqual(code, 2) + self.assertEqual(stdout.getvalue().strip(), "") + + +class AnUnreadableDeclaration(unittest.TestCase): + """`explicit-unknown`, claimed by `catalog-tests` in policy/upheld.toml. + + The claim names this file for this assertion: a declaration this tool could + not read exits 2. Not 0, which would be a repository reported as complying + on a file nobody could open, and not 1 -- exit 1 in this tool means a claim + is FALSE, and one stray byte is not an untrue claim. + + No engine is needed: the declaration is read before anything is asked of + the binary, which is the point. The catalog job runs on an image with no + Rust toolchain, and this assertion has to hold there too. + """ + + def setUp(self): + self._directory = tempfile.TemporaryDirectory() + self.tmp = Path(self._directory.name) + self.addCleanup(self._directory.cleanup) + (self.tmp / "policy").mkdir() + + def write_declaration(self, body: bytes) -> None: + (self.tmp / "policy" / "upheld.toml").write_bytes(body) + + def test_a_declaration_that_is_not_utf_8_is_two_and_not_a_traceback(self): + # `tomllib.load` takes a binary handle and decodes the bytes itself, so + # a stray 0xff raises `UnicodeDecodeError` out of the DECODE and never + # reaches `TOMLDecodeError`. It derives from `ValueError` and not from + # `OSError`, which is how it escaped the handler entirely and left the + # process on a traceback and exit 1. + self.write_declaration(b'[[enforce]]\nprinciple = "\xff"\nrule = "x"\n') + result = run(self.tmp, "--oscal") + self.assertEqual(result.returncode, 2, result.stdout) + self.assertIn("could not look", result.stderr) + self.assertNotIn("Traceback", result.stderr) + self.assertEqual(result.stdout.strip(), "") + + def test_the_review_mode_reads_the_same_declaration_the_same_way(self): + """`--review` reaches `read_toml` by its own route and owes the same answer.""" + self.write_declaration(b"[review]\nmax_lines = 900 # \xff\n") + result = run(self.tmp, "--review") + self.assertEqual(result.returncode, 2, result.stdout) + self.assertIn("could not look", result.stderr) + self.assertNotIn("Traceback", result.stderr) + + def test_a_declaration_that_is_not_there_is_two(self): + """The other unreadable: nothing to read at all, from a directory with none.""" + result = run(self.tmp, "--oscal") + self.assertEqual(result.returncode, 2, result.stdout) + self.assertEqual(result.stdout.strip(), "") diff --git a/tests/text_cli.rs b/tests/text_cli.rs new file mode 100644 index 0000000..836d783 --- /dev/null +++ b/tests/text_cli.rs @@ -0,0 +1,241 @@ +//! CLI-level tests for `uphold scan --text`. +//! +//! Text mode is the one entry point whose subject never becomes a file: a +//! commit message, a pull-request body, a release note, handed in on stdin and +//! published the moment it is accepted. Every test here is driven through the +//! binary rather than through `text::check`, because what is being preserved is +//! what the CALLER sees -- the exit code, and whether a refusal was printed at +//! all. A test calling `dedent` directly cannot tell the difference between a +//! violation report and a process that died at exit 101 before printing one, +//! and that difference is the whole subject of the first test below. + +#![expect( + clippy::let_underscore_must_use, + clippy::tests_outside_test_module, + clippy::unwrap_used, + reason = "A CLI test asserts on the outcome; a panic in the harness that builds the fixture IS the failure report, and there is no caller to hand a Result to" +)] + +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process::{Command, Output, Stdio}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// A policy directory and nothing else. No `git init`: `discover` stops at a +/// repository root but does not require one, and text mode runs wherever the +/// author happens to be standing. +fn workspace(policy: &str) -> PathBuf { + static NEXT: AtomicUsize = AtomicUsize::new(0); + let root = std::env::temp_dir().join(format!( + "uphold-text-{}-{}", + std::process::id(), + NEXT.fetch_add(1, Ordering::Relaxed) + )); + let _ = std::fs::remove_dir_all(&root); + std::fs::create_dir_all(root.join("policy")).unwrap(); + std::fs::write(root.join("policy/principles.toml"), policy).unwrap(); + root +} + +/// `HOME` is set on every run, never inherited. The identity needles are read +/// from the environment the scan runs in, so a test that let the real one +/// through would assert on this machine's username and home path and mean +/// something different on the next machine. +fn scan_text(root: &Path, stdin: &[u8], home: &str) -> Output { + let mut child = Command::new(env!("CARGO_BIN_EXE_uphold")) + .args(["scan", "--text", "-"]) + .current_dir(root) + .env_remove("UPHOLD_ALLOW") + .env("HOME", home) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + child.stdin.as_mut().unwrap().write_all(stdin).unwrap(); + child.wait_with_output().unwrap() +} + +fn stderr(output: &Output) -> String { + String::from_utf8_lossy(&output.stderr).into_owned() +} + +fn stdout(output: &Output) -> String { + String::from_utf8_lossy(&output.stdout).into_owned() +} + +fn code(output: &Output) -> i32 { + output.status.code().unwrap() +} + +// ── the report survives the message it has to print ────────────────── + +/// A message no policy author is forbidden to write used to be a crash. +/// +/// `report::dedent` minimised a BYTE count over the non-blank lines while +/// `trim_start` stripped UNICODE whitespace, so the minimum taken from the +/// two-space ASCII line landed inside the first U+3000 of the wide-indented +/// line and `&line[indent..]` panicked. The two whitespace-only lines are the +/// second way in -- excluded from the minimum and sliced by it anyway -- and +/// the single ASCII space is shorter than the minimum in BYTES as well, so it +/// panicked on the range rather than on the boundary. Both are in one message +/// because both reach the same function from the same field. +/// +/// The consequence is why this is a CLI test and not a unit test. The panic +/// happened INSIDE the function whose only job is printing a violation, so the +/// run that found the violation exited 101 having said nothing about it -- a +/// check that looked, found, and then reported neither a pass nor a failure. +/// Only the process boundary can tell that apart from a report. +#[test] +fn a_message_indented_with_unicode_whitespace_is_printed_not_panicked() { + let root = workspace( + "[rule.no-example-needle]\n\ + message = \"\"\"\n \ + ASCII indent, and a multi-byte character: caf\u{e9} \u{2014}\n\ + \u{3000}\u{3000}Wide-space indent on this line.\n\ + \u{a0}\n \n \ + Use neutral placeholders such as example-user instead.\n\ + \"\"\"\n\ + forbidden_literals_from = \"printf 'example-needle\\n'\"\n\ + \n\ + [rule.no-example-needle.files]\n", + ); + + let output = scan_text( + &root, + b"deployed with example-needle today\n", + "/srv/example", + ); + + assert_eq!(code(&output), 1, "{}", stderr(&output)); + // Named explicitly: 101 is the abort the fix exists to remove, and it is + // the one wrong answer that also looks like "some other failure". + assert_ne!(code(&output), 101, "{}", stderr(&output)); + assert!(!stderr(&output).contains("panicked"), "{}", stderr(&output)); + + // The three non-blank lines arrive with the shared two-CHARACTER indent + // gone and their characters intact. Asserting on the text and not just on + // the exit code is what distinguishes a report from a rule that happened to + // fire while printing nothing usable. + let printed = stderr(&output); + assert!( + printed.contains("ASCII indent, and a multi-byte character: caf\u{e9} \u{2014}"), + "{printed}" + ); + assert!( + printed.contains("Wide-space indent on this line."), + "{printed}" + ); + assert!( + printed.contains("Use neutral placeholders such as example-user instead."), + "{printed}" + ); + assert!(printed.contains("no-example-needle"), "{printed}"); +} + +// ── the bytes that could not be read ───────────────────────────────── + +/// stdin that is not UTF-8 is exit 2, and the wording is `scan`'s wording. +/// +/// `tests/guard_recovered_halves.rs` pins the exit code for the same input; +/// this pins the ANSWER, because the failure being guarded against was never a +/// wrong exit code on its own -- it was "policy checks passed (text)" printed +/// over bytes `from_utf8_lossy` had already replaced. So the pass line is +/// asserted absent, and the phrase `scan` uses about a non-UTF-8 file is +/// asserted present, since two readers giving the same bytes two different +/// answers is how one of them stops being believed. +#[test] +fn non_utf8_stdin_says_unexamined_and_never_prints_the_pass_line() { + let root = workspace( + "[rule.no-example-needle]\n\ + message = \"do not publish the needle\"\n\ + forbidden_literals_from = \"printf 'example-needle\\n'\"\n\ + \n\ + [rule.no-example-needle.files]\n", + ); + + let output = scan_text(&root, b"caf\xe9 latin1 bytes\n", "/srv/example"); + + assert_eq!(code(&output), 2, "{}", stderr(&output)); + assert!(stderr(&output).contains("not UTF-8"), "{}", stderr(&output)); + assert!( + stderr(&output).contains("unexamined"), + "{}", + stderr(&output) + ); + assert!( + !stdout(&output).contains("policy checks passed"), + "{}", + stdout(&output) + ); +} + +// ── the host-identity fallback ─────────────────────────────────────── + +/// A distinctive home path, so that only the `home-path` needle can match and +/// the assertion does not depend on this machine's username or hostname. +const EXAMPLE_HOME: &str = "/srv/example-home-7c3e91"; + +/// A repository that declares a literal rule about something ELSE keeps the +/// fallback. +/// +/// The test used to be for the check KIND, so any `forbidden_literals` rule at +/// all -- a repository's own literal list, a command source, a rule about the +/// default route -- silently deleted the one rule that stops the running host's +/// identity being published. Declaring a rule about the default route is not a +/// decision to stop checking identity. +/// +/// The fallback's own id is asserted rather than only the exit code: the +/// declared rule is present in this policy too, and an exit of 1 alone cannot +/// say which of the two produced it. +#[test] +fn an_unrelated_literal_rule_leaves_the_identity_fallback_in_place() { + let root = workspace( + "[rule.no-default-route-in-text]\n\ + message = \"Do not publish this machine's default route.\"\n\ + forbidden_literals = \"running-default-route\"\n\ + files.include = [\".\"]\n", + ); + + let subject = format!("the log said {EXAMPLE_HOME}/work/output.txt\n"); + let output = scan_text(&root, subject.as_bytes(), EXAMPLE_HOME); + + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("no-running-os-identity-metadata"), + "{}", + stderr(&output) + ); +} + +/// A repository that declares the identity rule ITSELF is answered once. +/// +/// The other half of the same test. Matching on the rule's literal source and +/// not on its check kind has to suppress the fallback exactly when the +/// repository has already said this -- otherwise the same home path is reported +/// twice under two rule names, and a reader who fixes the one they were shown +/// still has a finding open. +#[test] +fn a_declared_identity_rule_is_not_reported_twice() { + let root = workspace( + "[rule.house-identity-rule]\n\ + message = \"Do not publish host identity.\"\n\ + forbidden_literals = \"running-os-identity\"\n\ + files.include = [\".\"]\n", + ); + + let subject = format!("the log said {EXAMPLE_HOME}/work/output.txt\n"); + let output = scan_text(&root, subject.as_bytes(), EXAMPLE_HOME); + + assert_eq!(code(&output), 1, "{}", stderr(&output)); + assert!( + stderr(&output).contains("house-identity-rule"), + "{}", + stderr(&output) + ); + assert!( + !stderr(&output).contains("no-running-os-identity-metadata"), + "{}", + stderr(&output) + ); +} diff --git a/uphold_check.py b/uphold_check.py index c87ed49..f35674b 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -237,6 +237,25 @@ def declared_claims(declaration: dict) -> list[tuple[str, str]]: return claims +# The report `uphold check` prints, as the two fragments this reader keys on. +# Written down here rather than inline because the count check below is the only +# thing that notices when the binary's format and this reader drift apart, and a +# reader that has to be read alongside the format it parses should say what it +# expects in one place. +RECONCILED_PREFIX = "reconciled " +RECONCILED_SUFFIX = " enforcement claims:" +ENFORCED_BY = " enforced by " + + +def reconciled_count(line: str) -> int | None: + """The N of `reconciled N enforcement claims:`, or None for any other line.""" + line = line.strip() + if not (line.startswith(RECONCILED_PREFIX) and line.endswith(RECONCILED_SUFFIX)): + return None + count = line[len(RECONCILED_PREFIX) : -len(RECONCILED_SUFFIX)] + return int(count) if count.isdigit() else None + + def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]]: """Which seams supply each rule, as the reconcile in the binary sees it. @@ -247,6 +266,15 @@ def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]] supplies is exactly the state it exists to help with, and refusing there would take the review document away at the moment it is most wanted. The evidence lines for the claims that DID hold are on stdout either way. + + The evidence lines are counted against the binary's own header, because this + is a PARSER of another program's report and a parser that stops recognising + it fails silently: every line skipped by the `continue` below, an empty + mapping returned, and `--oscal` publishing a component definition with no + components at all -- exit 0, to an outside reader, over a repository whose + reconcile had just succeeded on every claim. Nothing in the pipeline could + tell that apart from a repository that enforces nothing. A report this + reader could not read is could-not-look; see the `explicit-unknown` record. """ answered = engine(root, "check") if answered.returncode == 2: @@ -257,11 +285,16 @@ def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]] f"honest to export:\n{answered.stderr.strip()}" ) suppliers: dict[str, list[str]] = {} + reconciled: int | None = None + read = 0 for line in answered.stdout.splitlines(): - if " <- " not in line or "enforced by" not in line: + if reconciled is None: + reconciled = reconciled_count(line) + if " <- " not in line or ENFORCED_BY not in line: continue + read += 1 _, rest = line.split(" <- ", 1) - rule, by = rest.split(" enforced by ", 1) + rule, by = rest.split(ENFORCED_BY, 1) # Folded back to the SEAM, because an OSCAL component is a thing that # implements a control and the seam is that thing. `uphold check` names # the evidence -- which stage, which scan -- and a component per stage @@ -275,6 +308,19 @@ def engine_suppliers(root: Path, *, strict: bool = True) -> dict[str, list[str]] for seam in seams: if seam not in suppliers.setdefault(rule.strip(), []): suppliers[rule.strip()].append(seam) + + # Only where the binary said the reconcile HELD. A refused one prints its + # failures on stderr and nothing on stdout, and `--review` is meant to keep + # going over exactly that; there is no count there to check against, and + # requiring one would turn the state this mode exists for into exit 2. + if answered.returncode == 0 and read != reconciled: + raise CouldNotLook( + f"`uphold check` reported {reconciled if reconciled is not None else 'no'} " + f"reconciled claim(s) and this reader recovered {read} evidence line(s) " + "from the same report, so which seam supplies which rule is unknown. " + "The binary's report and this reader have drifted apart; the binary " + "is the authority, so fix the reader." + ) return suppliers