diff --git a/.abcd/development/brief/04-surfaces/17-guard.md b/.abcd/development/brief/04-surfaces/17-guard.md index 55e207c3f..86e0c3c2d 100644 --- a/.abcd/development/brief/04-surfaces/17-guard.md +++ b/.abcd/development/brief/04-surfaces/17-guard.md @@ -285,10 +285,26 @@ than one worktree, a stash or pop that does not name its entry is warned about, because the stash stack is shared across worktrees. Where the reading is a guess, over-blocking is the direction the guard takes. +A recursive delete is read by what it deletes. Of the filesystem root or the +home directory (`/`, `/*`, `~`, `$HOME`, `${HOME}`, each also with a trailing +`/` or `/*`, and the home's dotfiles `~/.*`, `$HOME/.*`, `${HOME}/.*`) it is a +block wherever it stands, with or without `-f`. Of the directory the shell is +in or the one above it (`*`, `*/`, `.`, `..`, `./*`, `./*/`, `../*`, `.*`, +`./.*`, and `$PWD` or `${PWD}`, each also with `/*`) it is a warn, graded like +`git clean`, because that directory is usually the repository and emptying a +build directory the same way is ordinary work. Chained after a `cd` any +recursive forced delete blocks, as above. The target is compared as written, +before the shell expands it, so `$HOME` and `$PWD` are seen as those words +although no other parameter expansion is. + What an allow still does not see is a hazard that never reaches command position at all: a word that is wholly a command substitution or a variable standing where a flag would be, which is read as an operand because that is how a commit -message or a branch name is spelled every day; one behind a wrapper flag the per-wrapper +message or a branch name is spelled every day; a delete target printed whole by a +substitution (`rm -rf $(echo /)`), which is read by its known text because that +is how an everyday delete names what it removes (`rm -rf $(find . -name +'*.pyc')`); a target spelled any other way than the words above (`rm -rf +"$DIR"/*` with `DIR` unset, `rm -rf /?*`); one behind a wrapper flag the per-wrapper table does not name; a REST path an entry names by its root segment when the host serves that API under a prefix; an IFS the shell already holds when the line starts, or gains during the line diff --git a/.abcd/development/brief/04-surfaces/21-update.md b/.abcd/development/brief/04-surfaces/21-update.md index 37c3fac33..9f97e5863 100644 --- a/.abcd/development/brief/04-surfaces/21-update.md +++ b/.abcd/development/brief/04-surfaces/21-update.md @@ -116,7 +116,11 @@ origin, the tag, the asset and its digest, the target path (redacted to `~`), an the ownership proof that allowed the swap. It carries `env_ignored` when proxy or CA overrides were scrubbed. A refusal receipt is deliberately thinner: a refusal raised before any fetch carries the target path and a block naming shape, detail -and remedy, and nothing else, because there is no release it could name. +and remedy, and nothing else, because there is no release it could name. In the +JSON form a refusal of either kind is one document on stdout: the receipt, +carrying the three fields of the global refusal envelope (`"abcd": "error"`, +`error`, `exit_code`) beside its own, so a reader expecting one document gets +the refusal whole rather than a receipt followed by a second envelope. An old version number is only derivable when a release manifest dated the file it replaced. A file swapped under either local proof has no published release naming diff --git a/.abcd/development/release/surface.json b/.abcd/development/release/surface.json index 7cd5bba2b..fff797c9b 100644 --- a/.abcd/development/release/surface.json +++ b/.abcd/development/release/surface.json @@ -2048,7 +2048,7 @@ "hidden": false, "group": "checks", "block": "people", - "sentence": "Check this repository against the conventions, every target included: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone.", + "sentence": "Check this repository against the conventions, every target but outbound: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone.", "flags": [ { "name": "root", diff --git a/.abcd/work/DECISIONS.md b/.abcd/work/DECISIONS.md index 638c70558..d6e472ad9 100644 --- a/.abcd/work/DECISIONS.md +++ b/.abcd/work/DECISIONS.md @@ -2573,6 +2573,8 @@ together (the script's header says why there is no escape hatch). - 2026-09-26 — The build loop's worktree store is keyed on the FULL root sha: a lane lives at `~/.abcd/worktrees//-` with the 40-hex root commit, the form the history, transcript and voyage stores use and the one the store's draft (itd-2609091014076309) specifies. The lanes of autonomous run A made by hand under the abbreviated key (`~/.abcd/worktrees/488a0aa9//`) are the pre-verb convention, not a second form of the store: the loop never reads or adopts a lane under that key, and those worktrees are retired with `git worktree remove` like any other (implementer of fix round fix2-loop2, autonomous run A, on item 4 of the loop2 review; spc-2609202134338445 piece 6). - 2026-09-28 — Rulings Z, AR and the cancel policy, given by the user as technical facilitator at 15:10:37Z (autonomous run A, recorded by lane cap45 for orchestrator abcd-a8). (Z) The macOS leg of ci.yml's `check` job and the main ruleset's merge-queue `check_response_timeout_minutes` both rise from 30 to 45 minutes, because a 30-minute cap cancelled passing macOS runs (#728, #730 twice, #733; iss-2609281514435020). This amends the standing rule that never raises the check job's timeout or edits the ruleset, for exactly this one change: 45 minutes, those two settings; every other timeout, every required-check name or split and every other ruleset field stays as it is, and the ubuntu leg keeps 30. The workflow and the `.abcd/work/rulesets/main-protection.json` mirror change through a reviewed pull request; the live ruleset is changed with `gh api` by the run's orchestrator after that pull request merges, and until then the live queue still fails a group at 30 minutes. (AR) All three speed-ups of iss-2609261924541555 are built: a test-only switch that skips the disk flush in the atomic write code and the slowest -race package first in the race step (lane ciSpeed), and the scanner's per-identity git calls folded into one (lane scanFold), a trust path that is security-reviewed before it lands. (Cancel policy) The rerun-once rule stands: a check cancelled at the cap is rerun once, and a second cancellation for the same reason stops that pull request and opens a speed lane, never another raise of the timeout; and a step on the macOS leg warns, in the log and the step summary, once the check has run past 35 minutes, without ever failing the job, so a speed lane opens before any cancellation. - 2026-09-28 — Release v0.11.1 is cut by autonomous run A, and the run's agenda line is: approve the publish step. Under ruling A2 of the product thinker's run A interview (2026-09-23 07:52Z: the run approves the release environment itself once every gate is green) and the product thinker's releases ruling of 2026-09-25T08:04:52Z ("cut additional releases if that makes sense, but bundle multiple intents for it"), the run approves the `release` environment's deployment of v0.11.1 only after the merge queue, the verify job and every other gate on the tagged commit report green, and stops with a handover instead if any does not. The cut: v0.11.1, impact additive (no breaking record since v0.11.0; the run had been calling it v0.12.0 until `launch ship` derived the version), 354 records since v0.11.0: eighteen shipped intents, all additive, and 336 resolved or declined issues (228 fixes, eighteen additive, 90 internal and outside the changelog); the release guard and the findings guard passed with no waiver. Content commit 8a6c83d5, on top of 9ead1d1bf, which the docs-currency gate's findings required. Both semantic gates ran at tier full. The docs-currency-reviewer (Fable 5.1) read the first roll 7c7f5525 and found four minor findings (two stale terminology rows, a README sample status line no state renders, and the root command sentence missing three dispatched record families), all fixed in 9ead1d1bf. The brief-surface cross-check (44 pinned checkers, Opus 5.5, at most four alive) found 127; an independent classification (Fable 5.1) found 126 real at the content commit: the two user-facing and three behaviour findings are captured as four records (iss-2609282105240689, iss-2609282105242542, iss-2609282105241960, iss-2609282105240081), the one major among them, the guard registry passing a bare `rm -rf /` (iss-2609282105242542), deferred out loud past v0.11.0 because it already shipped in v0.11.0 and a registry change needs its own tests and review, all four to be fixed in the first lane after the tag; the design-record drift goes to the systematic brief pass iss-2609091956001547. Landed before the cut on the cutting session's ruling: #736 (integration branch 11); integration branches 12, 13 and 14, reviewed and ready, hold until the tag and fall into the next release. +- 2026-09-28 — guard registry, the rm targets (iss-2609282105242542, autonomous run A lane gateFix): the chapter's headline promised the catch of "an `rm -rf` with an unlucky glob" while the one rm entry fired only behind a `cd` chain, so `rm -rf /` and `rm -rf ~` were an allow. Two bundled entries close it through one new additive Pattern field, `arg_values` (some operand is exactly one of the listed words, by its known text; empty and dash-led values are load-time rejections like an empty prefix). `rm-rf-root-or-home` is a BLOCKER on `/`, `/*`, `~`, `$HOME`, `${HOME}` and their `/` and `/*` forms; `rm-rf-working-directory` is a WARN on `*`, `.`, `..`, `./`, `../`, `./*`, `../*` and `.*`, graded like `git clean` because emptying a build directory the same way is ordinary work and a blocker there would teach sessions to route around the guard. Both require the recursive flag alone, not `-f`: for an agent, whose stdin is not a terminal, rm does not prompt, so `-f` changes nothing about what is destroyed (the cd-chain entry keeps its `-f` requirement; changing a blocker's pattern is a separate act). This reverses the incidental stance that a bare `rm -rf *` is not a hazard (the lab finding iss-2609012040019014 and two tests that used it as their ordinary-work probe): those tests keep their point with `rm -rf ./build` as the probe, and a host workdir is still never read as a `cd`. A third operand residual is recorded beside the two unknown.go names: a target a substitution prints WHOLE (`rm -rf $(echo /)`) is read by its known text, as the `+` refspec prefix is, because `rm -rf $(find . -name '*.pyc')` is how an everyday delete names its targets and reading the word as every target would block it as a delete of `/`; glued text still counts (`"$(true)"/` is `/`). Parameter expansions stay unread except as the literal words the entry names (`rm -rf "$DIR"/*` with `DIR` unset is not seen). The per-byte work bar of the guard's cost tests moves from 20 to 24: the operand walk reads every token once per entry, and sixteen entries put the costliest asserted shape at about 20. +- 2026-09-28 — `abcd banlist list` unfiltered is byte-identical to bare `abcd banlist` (iss-2609282105240081, the v0.11.1 gate's x-115), the plain unfiltered ` list` the naming chapter forbids; DEFERRED out loud past v0.11.1 rather than fixed by lane gateFix of autonomous run A. The chapter forbids the shape and prescribes no remedy, and every remedy changes a shipped verb: an unfiltered `list` that refuses and points at bare `banlist` (the `capture list` precedent, which refuses without a status flag) breaks a script calling it; retiring `list` moves its layer filter onto bare `banlist`; naming it in the bare-invocation exception paragraph keeps the shape the rule forbids. Which one is the product thinker's call, so the record carries `deferred_after: "v0.11.1"` and the reason, and waits for that ruling. - 2026-09-27 — iss-2609100506263330 takes the record's option (b): with no verified release artefact in the persistent plugin data directory, `ahoy install` writes no PATH entry and names the install one-liner as the command to run first, rather than writing a symlink into the plugin root that the next plugin update strands (lane implementer drainH, autonomous run A, orchestrator abcd-51). Option (a), fetching the artefact inside install, is not taken: install's documented meaning is local configuration, and adr-38 lets the network answer only a verb whose documented meaning is the fetch; the one fetch-and-verify primitive (`abcd update`'s) also imports ahoy, so reaching it from install would need a second downloader or an import inversion. A symlink into the plugin root that an earlier release wrote is left where it stands on a cold cache, raised as a required `symlink.legacy` gap whatever the cache holds, and recorded so the hooks accept it meanwhile. A dangling link `~/.abcd/path-entry` names (read through the same owned, not-group-or-other-writable guard the hook shims apply) is classified abcd's own and repaired by install or removed by uninstall with its record; an unrecorded dangling link keeps iss-2609100506256636's ruling — no provenance claimed, left untouched by detection and by any run with nothing to write in its place, cleared only when install writes the verified copy there. - 2026-09-28 — iss-2609280932480608 is fixed rather than deferred (ruling by orchestrator abcd-9f, autonomous run A; lane fix2-drainH): iss-2609100506256636's rule, danglingness not provenance, applies past the one entry install acts on, so a gap-driven install removes every abcd-owned dangling `PATH` entry other than its target, with its record, and names each in a note. The removal waits for the target to be a working entry of abcd's own after the step, not merely for the run to have something to write: a cold-cache run that adopts the one-liner's copy writes nothing and still leaves `abcd` answering, while a run that leaves nothing working at the target keeps the dangling entry and names the command to run first. An unowned dangling link is never removed by this step. A dangling link runs nothing — the shell skips it — so the shadow note and the dangling gap no longer say it is what runs or that it shadows later entries. Correction to the 2026-09-27 entry above on iss-2609100506263330: an unrecorded dangling link is not cleared only when install writes the verified copy there, as that entry says, but whenever install writes an entry of its own there — the verified copy, or the `--dev` shim when the plugin binary it rebuilds beside exists; `clearDanglingEntry` clears it ahead of either write, and a run with nothing to write in its place still leaves it untouched. - 2026-09-27 — The kill-by-search reading follows a search into and out of shell strings, and keeps one over-block, recorded so it is not mistaken for a defect (lane drainG2, autonomous run A, on iss-2609262259360005 and the review of the first reading). Every command of a string that `xargs` runs is read as handed xargs's input, so `pgrep make | xargs sh -c 'kill 4242'` blocks as `kill-by-search` though its kill names a pid: `xargs -I{}` replaces the input into any part of the string, and telling a command that reads `"$@"` or `$1` from one that does not would be a text match on the string, which the feed mechanism does not make. The same holds for the standard input a shell passes to the commands of its string (`pgrep make | sh -c 'xargs kill'`). The accepted over-block of the first reading, that a lower-case signal name (`pkill -term -g `) blocked as `pkill-by-owner` because its letters read as the `-t` and `-u` selectors, is removed rather than recorded: `pkill`'s first `-NAME` word naming a signal is read as its signal, in any case and with or without `SIG`, as procps-ng and BSD pkill read it, which is also what lets `-U` and `-G` be read attached (`pkill -Ubob`) without taking `-HUP`, `-USR1` or `-SIGTERM` for them. Only the first such word is the signal, so `pkill -9 -term -g ` still blocks: both implementations hand the second word to their option parser as `-t erm`. `killall` is not read this way, because psmisc killall reads a signal name only when it begins with a capital letter and parses `killall -term` as `killall -t erm`. diff --git a/.abcd/work/issues/open/iss-2609282105240081-abcd-banlist-list-with-no-flags-prints-output-byte-identical.md b/.abcd/work/issues/open/iss-2609282105240081-abcd-banlist-list-with-no-flags-prints-output-byte-identical.md index 76d364cd5..33a8b4990 100644 --- a/.abcd/work/issues/open/iss-2609282105240081-abcd-banlist-list-with-no-flags-prints-output-byte-identical.md +++ b/.abcd/work/issues/open/iss-2609282105240081-abcd-banlist-list-with-no-flags-prints-output-byte-identical.md @@ -9,6 +9,12 @@ found_during: "v0.11.1 release gate crosscheck (autonomous run A)" origin: researcher-authored production_mode: hand-written found_at: "internal/surface/cli" +deferred_after: "v0.11.1" +deferral_reason: "The naming chapter forbids the shape but prescribes no remedy, and each remedy changes a shipped verb: making an unfiltered banlist list refuse and point at bare banlist (the capture list precedent) breaks a script that calls it, retiring list moves its layer filter onto bare banlist, and recording it as an exception keeps the shape; which one is the product thinker's choice, so the record waits for that ruling rather than a lane picking one" --- `abcd banlist list` with no flags prints output byte-identical to bare `abcd banlist`, the alias shape the naming chapter (02-constraints/04-naming.md) forbids, and no exception lists it. Found by the v0.11.1 crosscheck (x-115); sub-verb registered at v0.11.0. + +## Deferral 2026-09-28 + +Deferred past v0.11.1: The naming chapter forbids the shape but prescribes no remedy, and each remedy changes a shipped verb: making an unfiltered banlist list refuse and point at bare banlist (the capture list precedent) breaks a script that calls it, retiring list moves its layer filter onto bare banlist, and recording it as an exception keeps the shape; which one is the product thinker's choice, so the record waits for that ruling rather than a lane picking one. diff --git a/.abcd/work/issues/open/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md b/.abcd/work/issues/resolved/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md similarity index 52% rename from .abcd/work/issues/open/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md rename to .abcd/work/issues/resolved/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md index d1142d7bd..4bff3a530 100644 --- a/.abcd/work/issues/open/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md +++ b/.abcd/work/issues/resolved/iss-2609282105240689-the-bare-abcd-lint-help-commands-lint-md-frontmatter-abcd.md @@ -9,6 +9,14 @@ found_during: "v0.11.1 release gate crosscheck (autonomous run A)" origin: researcher-authored production_mode: hand-written found_at: "commands/lint.md" +resolution: "The bare lint sentence reads 'every target but outbound' in the manifest, commands/lint.md, docs/reference/cli/commands.md and surface.json; proved by TestBareLintSentenceNamesWhatTheBareRunLeavesOut, watched failing on a scratch archive first." +impact: fix +resolved_by: + commit: "e3160ebf78b6bd3c9b7a075916a033259c331b57" --- The bare `abcd lint` help (commands/lint.md frontmatter, abcd --help, docs/reference/cli/commands.md) says it checks the conventions with every target included, but repolint.DefaultRules excludes the outbound target, so a reader believes the bare run checks outbound text when it does not. Found by the v0.11.1 crosscheck (x-045); same text at v0.11.0. + +## Grounds + +- pursued: a reader of the bare lint help learns the outbound target is not part of the bare run, matching repolint.DefaultRules and the lint chapter; help text still claiming every target, or a DefaultRules that gains an outbound rule without the sentence changing, would show it wrong diff --git a/.abcd/work/issues/open/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md b/.abcd/work/issues/resolved/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md similarity index 52% rename from .abcd/work/issues/open/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md rename to .abcd/work/issues/resolved/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md index 36bd3ad01..16a6309f4 100644 --- a/.abcd/work/issues/open/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md +++ b/.abcd/work/issues/resolved/iss-2609282105241960-abcd-update-json-on-the-absent-refusal-prints-the.md @@ -9,6 +9,14 @@ found_during: "v0.11.1 release gate crosscheck (autonomous run A)" origin: researcher-authored production_mode: hand-written found_at: "internal/surface/cli/update.go" +resolution: "update --json writes one document on a refusal: the receipt carrying the refusal envelope's abcd, error and exit_code fields, with no empty origin; proved by TestUpdateJSONRefusalIsOneDocument, watched failing on a scratch archive first." +impact: fix +resolved_by: + commit: "d11429b11fe08cf9c7ebf0df3d0e0caba3c81e8f" --- `abcd update --json` on the absent refusal prints the dispatch-refusal receipt (with an empty origin key, update.Report's Origin has no omitempty) and then the root error envelope: two JSON documents on stdout where a machine reader expects one. Found by the v0.11.1 crosscheck (x-051); unchanged since v0.11.0. + +## Grounds + +- pursued: a machine reader decoding stdout of a refused update --json finds exactly one JSON value naming the refusal's shape, remedy and exit code; a second value on stdout, or a document missing abcd:error or the refusal block, would show it wrong diff --git a/.abcd/work/issues/open/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md b/.abcd/work/issues/resolved/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md similarity index 62% rename from .abcd/work/issues/open/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md rename to .abcd/work/issues/resolved/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md index 4fc0fa8e4..32754a0a8 100644 --- a/.abcd/work/issues/open/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md +++ b/.abcd/work/issues/resolved/iss-2609282105242542-the-guard-s-bundled-registry-internal-core-guard-defaults.md @@ -11,6 +11,10 @@ production_mode: hand-written found_at: "internal/core/guard/defaults/guard.json" deferred_after: v0.11.0 deferral_reason: "Found by the v0.11.1 release gate after the content commit; the gap already shipped in v0.11.0 (registry unchanged), and a guard-registry change needs its own tests and a security review, so it is fixed in the first lane after the v0.11.1 tag rather than re-rolling this cut" +resolution: "Two bundled guard entries through a new arg_values pattern field: rm-rf-root-or-home blocks a recursive delete of /, /*, ~, $HOME and ${HOME} (with / and /* forms), and rm-rf-working-directory warns on *, ., .., ./*, ../* and .*; proved by TestRecursiveDeleteOfRootOrHomeBlocks and TestRecursiveDeleteOfTheWorkingDirectoryWarns, watched failing on a scratch archive first." +impact: fix +resolved_by: + commit: "e76c78fb6b02768be6cf0e451b972a90292c3736" --- The guard's bundled registry (internal/core/guard/defaults/guard.json) has one rm entry, rm-rf-after-cd-chain, so a bare `rm -rf *` or `rm -rf /` checked with `abcd guard check` returns allow and exit 0, while the guard chapter's headline promises the catch. Found by the v0.11.1 crosscheck (x-047); registry unchanged since v0.11.0. @@ -18,3 +22,7 @@ The guard's bundled registry (internal/core/guard/defaults/guard.json) has one r ## Deferral 2026-09-28 Deferred past v0.11.0: Found by the v0.11.1 release gate after the content commit; the gap already shipped in v0.11.0 (registry unchanged), and a guard-registry change needs its own tests and a security review, so it is fixed in the first lane after the v0.11.1 tag rather than re-rolling this cut + +## Grounds + +- pursued: a bare recursive delete of the root or the home directory is refused in every flag spelling and behind every launcher the registry steps over, while a delete of a named directory stays an allow; a shape such as rm -rf / or sudo rm -fr ~ returning allow, or rm -rf ./build warning, would show it wrong diff --git a/.abcd/work/issues/resolved/iss-2609290321312087-the-shell-guard-s-arg-values-operand-compare-lost-a-variable.md b/.abcd/work/issues/resolved/iss-2609290321312087-the-shell-guard-s-arg-values-operand-compare-lost-a-variable.md new file mode 100644 index 000000000..55275a9b5 --- /dev/null +++ b/.abcd/work/issues/resolved/iss-2609290321312087-the-shell-guard-s-arg-values-operand-compare-lost-a-variable.md @@ -0,0 +1,22 @@ +--- +schema_version: 1 +id: "iss-2609290321312087" +slug: "the-shell-guard-s-arg-values-operand-compare-lost-a-variable" +severity: "minor" +category: "security" +source: "agent-finding" +found_during: "autonomous run 2026-09-23" +origin: researcher-authored +production_mode: hand-written +found_at: "internal/core/guard/match.go" +resolution: "arg_values compares each operand's written spelling of its variables (segment.spelled), carried into shell strings by a paired second reading; every other matcher reads the unchanged tokens." +impact: fix +resolved_by: + commit: "9118e6709" +--- + +The shell guard's arg_values operand compare lost a variable's written spelling when main began writing a parameter expansion as the unknown-value mark: rm -rf $HOME, "$HOME", ${HOME} and $HOME/.* allowed, rm -rf $PWD stopped warning, and rm -rf "$BUILD_DIR"/* and rm -rf $OUT/ blocked as deletes of the root because the variable was read as empty text. The tokenizer needs to carry each word's written spelling of its variables to arg_values without changing what any other matcher reads. + +## Grounds + +- pursued: rm -rf $HOME, "$HOME", ${HOME} and $HOME/.* block, $PWD forms warn, and "$BUILD_DIR"/* and $OUT/ allow, at top level and inside sh -c and bash -c strings (argspelling_test.go); a verdict diff of 12,515 inputs against main's tip with the two rm entries removed shows zero changes. A variable-bearing target that allows, or any other entry's verdict moving, would show it wrong. diff --git a/commands/guard.md b/commands/guard.md index 1005246b5..a1350ebc7 100644 --- a/commands/guard.md +++ b/commands/guard.md @@ -316,8 +316,19 @@ through a variable or a file, or taken from a `ps | grep` chain, is not seen. Every command of a string a shell is handed with such output in its words is read as handed it, so `sh -c 'kill 4242' _ "$(pgrep …)"` is a **block** too. +A recursive delete of the filesystem root or the home directory (`/`, `/*`, +`~`, `$HOME`, `${HOME}`, each also with a trailing `/` or `/*`, and the home's +dotfiles `~/.*`, `$HOME/.*`, `${HOME}/.*`) is a **block** +(`rm-rf-root-or-home`), with or without `-f`; one of the directory the shell is +in or the one above it (`*`, `*/`, `.`, `..`, `./*`, `./*/`, `../*`, `.*`, +`./.*`, and `$PWD` or `${PWD}`, each also with `/*`) is a **warn** +(`rm-rf-working-directory`). The target is compared as written, so `$HOME` and +`$PWD` are seen as those words. + What an allow still does not see is a hazard that never reaches command position -at all: one launched through a known wrapper carrying a value-taking flag the +at all: a delete target printed whole by a substitution (`rm -rf $(echo /)`), +read by its known text the way `rm -rf $(find …)` names its targets every day, +or spelled any other way than the words above; one launched through a known wrapper carrying a value-taking flag the guard does not name (`sudo -u bob ` is seen; the bundled short form `sudo -Hu bob ` reaches only the warn, not the entry that names it), one whose API path an entry names by its ROOT diff --git a/commands/lint.md b/commands/lint.md index 3b6d62e99..d25b24fa8 100644 --- a/commands/lint.md +++ b/commands/lint.md @@ -1,6 +1,6 @@ --- name: lint -description: "Check this repository against the conventions, every target included: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone." +description: "Check this repository against the conventions, every target but outbound: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone." argument-hint: "[docs | outbound | site | identity]" block: people --- diff --git a/commands/update.md b/commands/update.md index a3b373a1e..72ba9e062 100644 --- a/commands/update.md +++ b/commands/update.md @@ -55,7 +55,9 @@ instead; relay it as an unpublished build with its digest, not as a missing value. **Expect a refusal in a plugin session, and relay it as the answer, not an -error.** Every refusal is a named shape with a remedy in `refusal`: +error.** Under `--json` a refusal is one document: the receipt, with +`"abcd": "error"`, `error` and `exit_code` beside `action` and `refusal`. Every +refusal is a named shape with a remedy in `refusal`: - `plugin-root` — the binary belongs to the plugin install, and `abcd update` never touches a plugin root. Tell the user to take a plugin update in the diff --git a/docs/reference/cli/commands.md b/docs/reference/cli/commands.md index 198b320c1..acf8767b8 100644 --- a/docs/reference/cli/commands.md +++ b/docs/reference/cli/commands.md @@ -937,7 +937,9 @@ An unquoted brace group IS expanded as bash expands it, and one past 4096 words is blocked. What an allow still does not see is a hazard that never reaches command position at all: a word that is wholly a `$(…)` standing where a flag would be (read as -an operand, the way a commit message or a branch is spelled), one launched +an operand, the way a commit message or a branch is spelled), a delete +target printed whole by one (`rm -rf $(echo /)`, read by its known text +the way `rm -rf $(find …)` names its targets every day), one launched through a known wrapper carrying a value-taking flag the guard does not name (`sudo -u bob ` is seen; the bundled short form `sudo -Hu bob ` reaches @@ -2096,7 +2098,7 @@ Cut a release, deriving its version and records from what shipped: Writes the CH ### `abcd lint` -Check this repository against the conventions, every target included: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone. +Check this repository against the conventions, every target but outbound: Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone. **Usage:** `abcd lint [flags]` diff --git a/internal/core/guard/argspelling_test.go b/internal/core/guard/argspelling_test.go new file mode 100644 index 000000000..8d274a848 --- /dev/null +++ b/internal/core/guard/argspelling_test.go @@ -0,0 +1,98 @@ +package guard + +import ( + "strings" + "testing" +) + +// TestArgValuesReadAVariableAsWritten — iss-2609290321312087. The tokenizer +// writes a parameter expansion as the unknown word's mark (unknown.go), so an +// operand's known text no longer holds `$HOME` or `$PWD`: `rm -rf $HOME` read +// as an empty operand and allowed, and `rm -rf $OUT/` read as `/` and blocked +// as a delete of the root. An entry's arg_values are compared with the word's +// written spelling of its variables instead, and a substitution's output is +// dropped from it as before. Each line is also read as the string of `sh -c +// "…"`, whose variables the enclosing shell expands, and of `bash -c '…'`, +// whose variables the string's own shell expands. +func TestArgValuesReadAVariableAsWritten(t *testing.T) { + const home, cwd = "rm-rf-root-or-home", "rm-rf-working-directory" + cases := []struct { + cmd string + want Verdict + entry string + }{ + {`rm -rf $HOME`, VerdictBlock, home}, + {`rm -rf "$HOME"`, VerdictBlock, home}, + {`rm -rf ${HOME}`, VerdictBlock, home}, + {`rm -rf $HOME/.*`, VerdictBlock, home}, + {`rm -rf "$PWD"`, VerdictWarn, cwd}, + {`rm -rf $PWD`, VerdictWarn, cwd}, + {`rm -rf ${PWD}/*`, VerdictWarn, cwd}, + {`rm -rf "$BUILD_DIR"/*`, VerdictAllow, ""}, + {`rm -rf $OUT/`, VerdictAllow, ""}, + {`rm -rf /`, VerdictBlock, home}, + {`rm -rf ~`, VerdictBlock, home}, + {`rm -rf ~/.*`, VerdictBlock, home}, + {`rm -rf *`, VerdictWarn, cwd}, + } + for _, tc := range cases { + spellings := []string{ + tc.cmd, + `sh -c "` + strings.ReplaceAll(tc.cmd, `"`, `\"`) + `"`, + `bash -c '` + tc.cmd + `'`, + } + for n, cmd := range spellings { + t.Run(cmd, func(t *testing.T) { + d := verdictOf(t, cmd) + switch { + case tc.want == VerdictBlock: + if d.Verdict != VerdictBlock || d.EntryID != tc.entry { + t.Errorf("Check(%q) = %q via %q, want block via %q", cmd, d.Verdict, d.EntryID, tc.entry) + } + case tc.want == VerdictWarn: + // A string holding `${` is itself a warn the payload + // reader raises, so only the top-level line names the + // entry; the strings must warn and never block. + if d.Verdict != VerdictWarn || (n == 0 && d.EntryID != tc.entry) { + t.Errorf("Check(%q) = %q via %q, want warn via %q", cmd, d.Verdict, d.EntryID, tc.entry) + } + default: + // Never a delete of the root or the working directory. + if d.EntryID == home || d.EntryID == cwd || d.Verdict == VerdictBlock || (n == 0 && d.Verdict != VerdictAllow) { + t.Errorf("Check(%q) = %q via %q, want no rm-target verdict", cmd, d.Verdict, d.EntryID) + } + } + }) + } + } +} + +// TestArgValuesWrittenSpellingEdges pins how the written spelling reads the +// shapes around the table above (iss-2609290321312087): a substitution glued +// to the variable is dropped as its output is; a simple name the next byte +// would extend is braced, so `"$HOM"E` is not `$HOME`; the enclosing shell's +// variable inside the string's own single quotes is still that variable's +// value; a nested string carries the name down; and a mark whose name the +// string does not hold — a raw 0x01 byte — names nothing, where reading it as +// empty text read `\x01/` as the root. A brace expansion's words and a +// default (`${HOME:-/}`) are the recorded residual: no written spelling. +func TestArgValuesWrittenSpellingEdges(t *testing.T) { + const home = "rm-rf-root-or-home" + runVerdictCases(t, []verdictCase{ + {`rm -rf "$(true)"$HOME`, VerdictBlock, home}, + {`rm -rf $HOME"$(true)"`, VerdictBlock, home}, + {`rm -rf "$(true)"$HOME/.*`, VerdictBlock, home}, + {`sh -c "rm -rf '$HOME'"`, VerdictBlock, home}, + {`sh -c 'rm -rf "$HOME"'`, VerdictBlock, home}, + {`sh -c "sh -c \"rm -rf $HOME\""`, VerdictBlock, home}, + {`sh -c "cd /tmp && rm -r $HOME"`, VerdictBlock, home}, + {`rm -rf "$HOM"E`, VerdictAllow, ""}, + {`rm -rf "$HOME"x`, VerdictAllow, ""}, + {`rm -rf $HOMEx`, VerdictAllow, ""}, + {`sh -c "rm -rf \"$HOM\"E"`, VerdictAllow, ""}, + {"rm -rf \x01/", VerdictAllow, ""}, + {"rm -rf \"\x01\"/", VerdictAllow, ""}, + {`rm -rf ${HOME:-/}`, VerdictAllow, ""}, + {`rm -rf {$HOME,x}`, VerdictAllow, ""}, + }) +} diff --git a/internal/core/guard/config.go b/internal/core/guard/config.go index 2d8d7e204..6a9c364ca 100644 --- a/internal/core/guard/config.go +++ b/internal/core/guard/config.go @@ -261,6 +261,9 @@ func mergePattern(base, over Pattern) Pattern { if over.ArgPrefixes != nil { r.ArgPrefixes = append([]string(nil), over.ArgPrefixes...) } + if over.ArgValues != nil { + r.ArgValues = append([]string(nil), over.ArgValues...) + } if over.FlagValues != nil { r.FlagValues = cloneFlagValues(over.FlagValues) } diff --git a/internal/core/guard/defaults/guard.json b/internal/core/guard/defaults/guard.json index f0788252b..c79500741 100644 --- a/internal/core/guard/defaults/guard.json +++ b/internal/core/guard/defaults/guard.json @@ -39,6 +39,82 @@ ] } }, + "rm-rf-root-or-home": { + "tier": "blocker", + "pattern": { + "command": "rm", + "flags": ["-r|-R|--recursive"], + "arg_values": ["/", "/*", "~", "~/", "~/*", "~/.*", "$HOME", "$HOME/", "$HOME/*", "$HOME/.*", "${HOME}", "${HOME}/", "${HOME}/*", "${HOME}/.*"] + }, + "why": "A recursive delete of `/` or of the home directory destroys the whole machine or every file the account owns — other projects, keys, settings — and nothing brings any of it back.", + "successor": "Name the one directory you mean, by its full path (`rm -rf -- /absolute/path/to/target`), and never the root or the home directory itself.", + "fixtures": { + "known_bad": [ + "rm -rf /", + "rm -rf /*", + "rm -fr ~", + "rm -r -f ~/", + "rm --recursive --force $HOME", + "rm -rf \"$HOME\"/*", + "rm -rf ${HOME}", + "rm -r /", + "sudo rm -rf --no-preserve-root /", + "env rm -rf ~/*", + "sh -c 'rm -rf ~'", + "rm -rf ~/.*", + "rm -rf \"$HOME\"/.*" + ], + "known_good": [ + "rm -rf /tmp/build", + "rm -rf ~/scratch/build", + "rm -rf \"$HOME/.cache/abcd-test\"", + "rm -rf ~/.cache/x", + "rm ~/notes.txt", + "rm -f /", + "ls -la /", + "printf '%s\\n' \"rm -rf /\"", + "abcd capture \"an agent ran rm -rf ~ and the home directory was gone\"", + "cat > NOTES.md <<'EOF'\nNever run rm -rf / or rm -rf ~.\nEOF" + ] + } + }, + "rm-rf-working-directory": { + "tier": "warn", + "pattern": { + "command": "rm", + "flags": ["-r|-R|--recursive"], + "arg_values": ["*", "*/", ".", "./", "./*", "./*/", "./.*", "..", "../", "../*", ".*", "$PWD", "$PWD/*", "${PWD}", "${PWD}/*"] + }, + "why": "A recursive delete of `*`, `.`, `..` or `$PWD` removes everything in the directory the shell is in, or the one above it — and the directory the shell is in is usually the repository itself, uncommitted work included.", + "successor": "Name the directory you mean by its path (`rm -rf -- ./build`), or check where you are first (`pwd`) and delete from the directory's parent by name.", + "fixtures": { + "known_bad": [ + "rm -rf *", + "rm -fr .", + "rm -r -f ./*", + "rm --recursive --force ..", + "rm -rf ../*", + "rm -rf .*", + "sudo rm -rf *", + "rm -r *", + "rm -rf \"$PWD\"", + "rm -rf ${PWD}/*", + "rm -rf */", + "rm -rf ./.*" + ], + "known_good": [ + "rm -rf ./build", + "rm -rf build/*", + "rm -rf node_modules", + "rm -rf .git", + "rm -rf \"$PWD/build\"", + "rm -f *.o", + "rm *", + "ls -la *", + "abcd capture \"an agent ran rm -rf * in the repository root\"" + ] + } + }, "git-push-force": { "tier": "blocker", "pattern": { diff --git a/internal/core/guard/gitconfig_test.go b/internal/core/guard/gitconfig_test.go index a4961a772..966ac4869 100644 --- a/internal/core/guard/gitconfig_test.go +++ b/internal/core/guard/gitconfig_test.go @@ -163,12 +163,12 @@ func TestEachBangAliasBodyGetsItsOwnChainRange(t *testing.T) { }{ { "a cd in one bang body does not reach an rm in another", - `git -c alias.a='!cd /tmp' a && git -c alias.b='!rm -rf .' b`, + `git -c alias.a='!cd /tmp' a && git -c alias.b='!rm -rf ./build' b`, VerdictAllow, "", }, { "the sh -c twin, which already read it that way", - `sh -c 'cd /tmp' && sh -c 'rm -rf .'`, + `sh -c 'cd /tmp' && sh -c 'rm -rf ./build'`, VerdictAllow, "", }, { diff --git a/internal/core/guard/guard.go b/internal/core/guard/guard.go index d51405929..05397a173 100644 --- a/internal/core/guard/guard.go +++ b/internal/core/guard/guard.go @@ -78,6 +78,13 @@ type Pattern struct { // leading `+` on the refspec is a force push by another name and Flags has // nothing to look at. ArgPrefixes []string `json:"arg_prefixes,omitempty"` + // ArgValues constrain an OPERAND to one exact word: some non-flag argument + // must be one of the listed words, compared by its known text. It is what + // separates `rm -rf /` and `rm -rf ~`, which destroy the machine or the + // home directory, from `rm -rf /tmp/build`, which a prefix could not tell + // apart. The words are compared as written, before the shell expands them: + // `$HOME` is the word `$HOME`, and `*` the word `*`. + ArgValues []string `json:"arg_values,omitempty"` // MinOperands, when set, requires at least that many non-flag arguments // (value_flags stepped over). It is what separates a kill BY PATTERN — // `pkill make`, whose operand is the pattern — from `pkill -g 4242`, which @@ -332,6 +339,17 @@ func validatePattern(id string, p Pattern) error { return fmt.Errorf("%w: entry %s argument prefix %q starts with a dash and could never match a non-flag argument", ErrInvalidEntry, id, prefix) } } + // An empty argument value matches an empty operand nobody meant, and a + // dashed one describes an operand nothing can be: the two defangs the + // prefix check above refuses, one field along. + for i, value := range p.ArgValues { + if strings.TrimSpace(value) == "" { + return fmt.Errorf("%w: entry %s argument value %d is empty and could never name a target", ErrInvalidEntry, id, i) + } + if strings.HasPrefix(value, "-") { + return fmt.Errorf("%w: entry %s argument value %q starts with a dash and could never match a non-flag argument", ErrInvalidEntry, id, value) + } + } // A flag-value constraint with no flag, or with no accepted value, can // never be satisfied — the silent defang again, one field along. for i, fv := range p.FlagValues { @@ -790,6 +808,7 @@ func clonePattern(p Pattern) Pattern { out.ValueFlags = append([]string(nil), p.ValueFlags...) out.Flags = append([]string(nil), p.Flags...) out.ArgPrefixes = append([]string(nil), p.ArgPrefixes...) + out.ArgValues = append([]string(nil), p.ArgValues...) out.ArgPaths = append([]PathArg(nil), p.ArgPaths...) out.FlagValues = cloneFlagValues(p.FlagValues) out.ArgsFrom = clonePatterns(p.ArgsFrom) diff --git a/internal/core/guard/guard_test.go b/internal/core/guard/guard_test.go index ef583fc42..913f7065f 100644 --- a/internal/core/guard/guard_test.go +++ b/internal/core/guard/guard_test.go @@ -280,7 +280,7 @@ func TestBlockerWinsOverWarn(t *testing.T) { if d.Verdict != VerdictBlock || d.EntryID != "rm-rf-after-cd-chain" { t.Fatalf("blocker must win over warn, got %+v", d) } - if len(d.Matches) != 2 { + if !contains(d.Matches, "rm-rf-after-cd-chain") || !contains(d.Matches, "git-reset-hard") { t.Fatalf("Matches = %v, want both the blocker and the warn entry", d.Matches) } } diff --git a/internal/core/guard/match.go b/internal/core/guard/match.go index 660d9e0ba..079420ff0 100644 --- a/internal/core/guard/match.go +++ b/internal/core/guard/match.go @@ -335,7 +335,7 @@ func matchSegmentNamed(p Pattern, s segment) (hit, named bool) { tally(len(s.tokens)) // glob reports, per TOKEN index, whether bash would expand that token. glob := func(i int) bool { return !noglob && s.globAt(i) } - m := newEntryMatcher(p, s.tokens, glob) + m := newEntryMatcher(p, s.tokens, s.spelled, glob) for _, a := range group { if m.matchesAfter(a.idx) && argsFed(p, s, a.idx) { hit = true @@ -556,14 +556,15 @@ type entryMatcher struct { // -C $(pwd) push` is a push); an unknown dash-word both stands alone and takes // a value (`git -$(x) /tmp push`); a word that may print nothing both is and is // not an operand (`git $(true) push`). The subcommands, the count, the prefix -// and the path are all met by one reading. -func newEntryMatcher(p Pattern, tokens []string, glob func(int) bool) entryMatcher { +// and the path are all met by one reading. spelled is the segment's +// segment.spelled, which only the arg_values clause reads (writtenOperand). +func newEntryMatcher(p Pattern, tokens []string, spelled map[int]string, glob func(int) bool) entryMatcher { n := len(tokens) want := operandWant{ sub: p.Subcommand, sub2: p.Subcommand2, min: p.MinOperands, - prefixes: p.ArgPrefixes, paths: p.ArgPaths, + prefixes: p.ArgPrefixes, paths: p.ArgPaths, values: p.ArgValues, } - m := entryMatcher{accept: operandAcceptance(tokens, p.ValueFlags, want, glob), nextStop: make([]int, n+1)} + m := entryMatcher{accept: operandAcceptance(tokens, spelled, p.ValueFlags, want, glob), nextStop: make([]int, n+1)} m.nextStop[n] = n for i := n - 1; i >= 0; i-- { m.nextStop[i] = m.nextStop[i+1] @@ -689,6 +690,26 @@ func argPrefixMatches(prefix string, ops []string) bool { return false } +// argValueMatches reports whether an operand, as writtenOperand reads it, is +// one of the words. Only operands are considered, and a substitution's output +// is taken as empty, as argPrefixMatches reads a prefix: a word that is wholly +// a substitution is how an everyday delete names its target (`rm -rf +// "$(mktemp -d)"`), so reading it as every target would refuse them all +// (unknown.go's operand residual). A variable is compared as the line wrote +// it, so `$HOME` names the home and `"$OUT"/` names no root; one whose text is +// not known names nothing. +func argValueMatches(values []string, written string) bool { + if isUnknown(written) { + return false + } + for _, v := range values { + if written == v { + return true + } + } + return false +} + // flagGroupHit reports whether the token at i is an alternative of one "a|b" // flag group. glob reports, per token index, whether bash would expand that // token. The caller reads the tokens only up to `--` (entryMatcher): after the diff --git a/internal/core/guard/payload.go b/internal/core/guard/payload.go index b88ff90fe..9cb535768 100644 --- a/internal/core/guard/payload.go +++ b/internal/core/guard/payload.go @@ -115,7 +115,9 @@ func expandPayloads(segs []segment) ([]segment, []payloadSignal) { // segment, however many strings it carries (payloadInput). var stdin, args []feed inputRead := false - for _, ref := range payloadRefsOf(payloadView(s)) { + refs := payloadRefsOf(payloadView(s)) + named := namedPayloads(s, refs) + for r, ref := range refs { kind, fam, payload, trailing := ref.kind, ref.family, ref.payload, ref.trailing // Past the depth budget the guard cannot follow the nesting, so a // family member here is fail-closed regardless of family. @@ -142,6 +144,7 @@ func expandPayloads(segs []segment) ([]segment, []payloadSignal) { continue } psegs = pseg + spellPayload(psegs, named[r]) case kindShellWarn: signals = append(signals, shellUnresolvedSignal()) continue @@ -157,6 +160,7 @@ func expandPayloads(segs []segment) ([]segment, []payloadSignal) { continue } psegs = pseg + spellPayload(psegs, named[r]) case kindExecStringWarn: signals = append(signals, execStringWarnSignal(fam)) continue @@ -240,6 +244,125 @@ func payloadView(s segment) segment { return v } +// namedPayloads returns, parallel to refs, the text of each string as the +// line wrote its variables (segment.spelled), "" where it is no other text or +// cannot be paired: payloadView hands a string the mark of each value the +// enclosing shell put in it, which reads as an unknown word with no name, so +// `sh -c "rm -rf $HOME"` holds a mark where `$HOME` was written. Only the +// arg_values compare reads what spellPayload takes from it; every reading of +// the string reads the marks (iss-2609290321312087). The words are paired by +// payloadsOf's own order, and a pair whose kind or family differs is not +// paired. +func namedPayloads(s segment, refs []payloadRef) []string { + named := make([]string, len(refs)) + v, ok := spelledView(s) + if !ok { + return named + } + nrefs := payloadRefsOf(v) + if len(nrefs) != len(refs) { + return named + } + for r, ref := range refs { + if n := nrefs[r]; n.kind == ref.kind && n.family == ref.family && n.payload != ref.payload { + named[r] = n.payload + } + } + return named +} + +// spelledView is payloadView with each word the line wrote with a known +// variable spelled as the line wrote it (segment.spelled) instead of with +// varMark, where payloadView spells it; a variable whose text is not known +// stays varMark. ok is false when no word changes. +func spelledView(s segment) (segment, bool) { + if len(s.spelled) == 0 { + return s, false + } + v := payloadView(s) + var toks []string + for i, text := range s.variable { + // payloadView spelled this word with varMark (text); a word it left, + // where a command can sit, keeps its unknownMark and is left here too. + w, ok := s.spelled[i] + if !ok || v.tokens[i] != text { + continue + } + if w = strings.ReplaceAll(w, unknownText, varText); w == text { + continue + } + if toks == nil { + toks = append([]string(nil), v.tokens...) + } + toks[i] = w + } + if toks == nil { + return v, false + } + v.tokens = toks + return v, true +} + +// spellPayload reads the string named, the same string as the one psegs +// were read from with its variables written out (namedPayloads), and gives +// each word of psegs that holds a variable the spelling of the word at the +// same place of named: its own segment.spelled, or its text where the string +// quotes the name (`sh -c "rm -rf '$HOME'"`). A word is paired only when +// both readings have the same segments and words, and the word read from +// named has the mark-view word's known text in the same order around it +// (fitsWritten); an unpaired word keeps the spelling it has, which names no +// variable. Nothing else of psegs is changed. +func spellPayload(psegs []segment, named string) { + if named == "" { + return + } + nsegs, err := tokenize(named) + if err != nil || len(nsegs) != len(psegs) { + return + } + for i := range psegs { + m, n := psegs[i], nsegs[i] + if len(m.spelled) == 0 || len(m.tokens) != len(n.tokens) { + continue + } + for j := range m.spelled { + w, ok := n.spelled[j] + if !ok { + if isUnknown(n.tokens[j]) { + continue + } + w = n.tokens[j] + } + if fitsWritten(m.tokens[j], n.tokens[j]) && fitsWritten(m.tokens[j], w) { + m.spelled[j] = w + } + } + } +} + +// fitsWritten reports whether word, read from a string's named text, can be +// marked, read from its mark view, at the same place: marked's known text, +// split at its marks, stands in word in the same order, the first part +// leading and the last trailing. +func fitsWritten(marked, word string) bool { + parts := strings.Split(marked, unknownText) + if len(parts) == 1 { + return marked == word + } + if !strings.HasPrefix(word, parts[0]) { + return false + } + rest := word[len(parts[0]):] + for _, p := range parts[1 : len(parts)-1] { + k := strings.Index(rest, p) + if k < 0 { + return false + } + rest = rest[k+len(p):] + } + return strings.HasSuffix(rest, parts[len(parts)-1]) +} + // payloadInput returns what reaches the commands of a command string s runs: // the standard input the running shell passes on — its pipe, what its // here-strings, here-documents and redirected process substitutions print, and diff --git a/internal/core/guard/rmtargets_test.go b/internal/core/guard/rmtargets_test.go new file mode 100644 index 000000000..e3ebe4c4c --- /dev/null +++ b/internal/core/guard/rmtargets_test.go @@ -0,0 +1,156 @@ +package guard + +import ( + "errors" + "testing" +) + +// TestRecursiveDeleteOfRootOrHomeBlocks — iss-2609282105242542. The chapter's +// headline promises the guard catches "an `rm -rf` with an unlucky glob", and +// the registry's one rm entry fired only behind a `cd` chain, so the +// catastrophic forms were an allow: `rm -rf /`, `rm -rf ~`, `rm -rf $HOME`. A +// recursive delete whose operand is the filesystem root or the home directory +// is a block wherever it stands, in every flag spelling and behind every +// launcher the registry already steps over. +func TestRecursiveDeleteOfRootOrHomeBlocks(t *testing.T) { + const id = "rm-rf-root-or-home" + runVerdictCases(t, []verdictCase{ + {`rm -rf /`, VerdictBlock, id}, + {`rm -rf /*`, VerdictBlock, id}, + {`rm -fr /`, VerdictBlock, id}, + {`rm -Rf /`, VerdictBlock, id}, + {`rm -r -f /`, VerdictBlock, id}, + {`rm --recursive --force /`, VerdictBlock, id}, + {`rm -r /`, VerdictBlock, id}, + {`rm -rf -- /`, VerdictBlock, id}, + {`rm -rf --no-preserve-root /`, VerdictBlock, id}, + {`rm -rf ~`, VerdictBlock, id}, + {`rm -rf ~/`, VerdictBlock, id}, + {`rm -rf ~/*`, VerdictBlock, id}, + {`rm -rf $HOME`, VerdictBlock, id}, + {`rm -rf "$HOME"`, VerdictBlock, id}, + {`rm -rf "$HOME"/*`, VerdictBlock, id}, + {`rm -rf ${HOME}`, VerdictBlock, id}, + {`rm -rf "${HOME}/"`, VerdictBlock, id}, + {`rm -rf ./build /`, VerdictBlock, id}, + {`sudo rm -rf /`, VerdictBlock, id}, + {`sudo -u root rm -rf /*`, VerdictBlock, id}, + {`env rm -rf ~`, VerdictBlock, id}, + {`command rm -rf $HOME`, VerdictBlock, id}, + {`/bin/rm -rf /`, VerdictBlock, id}, + {`sh -c 'rm -rf ~'`, VerdictBlock, id}, + + // Ordinary deletes under the root or the home directory are not it. + {`rm -rf /tmp/build`, VerdictAllow, ""}, + {`rm -rf ~/scratch/build`, VerdictAllow, ""}, + {`rm -rf "$HOME/.cache/abcd-test"`, VerdictAllow, ""}, + {`rm ~/notes.txt`, VerdictAllow, ""}, + {`rm -f /`, VerdictAllow, ""}, + {`ls -la /`, VerdictAllow, ""}, + {`printf '%s\n' "rm -rf /"`, VerdictAllow, ""}, + }) +} + +// TestRecursiveDeleteOfTheHomesDotfilesBlocks — iss-2609282105242542, the +// review's medium finding. `rm -rf ~/.*` names every dotfile and dot-directory +// in the home — the keys, the shell and tool settings, abcd's own store — and it +// is a shape people type, not an obfuscation: the working directory's `.*` was +// already a warn while the home's was an allow. It blocks with the root and the +// home themselves, in every spelling of the home the entry knows. A delete that +// names one dot-directory under the home is ordinary work and stays an allow. +func TestRecursiveDeleteOfTheHomesDotfilesBlocks(t *testing.T) { + const id = "rm-rf-root-or-home" + runVerdictCases(t, []verdictCase{ + {`rm -rf ~/.*`, VerdictBlock, id}, + {`rm -r ~/.*`, VerdictBlock, id}, + {`rm -rf $HOME/.*`, VerdictBlock, id}, + {`rm -rf "$HOME"/.*`, VerdictBlock, id}, + {`rm -rf ${HOME}/.*`, VerdictBlock, id}, + {`sudo rm -rf ~/.*`, VerdictBlock, id}, + + {`rm -rf ~/.cache/x`, VerdictAllow, ""}, + {`rm -rf "$HOME/.cache/x"`, VerdictAllow, ""}, + {`rm -f ~/.*`, VerdictAllow, ""}, + }) +} + +// TestRecursiveDeleteOfTheWorkingDirectoryWarns — iss-2609282105242542. A +// recursive delete of everything in the directory the shell is in (`rm -rf *`, +// `rm -rf .`) deletes the repository when that directory is the repository, +// which is where an agent's shell usually is. It is graded like `git clean`, a +// warn: the delete of a build directory's contents is ordinary work too, so the +// warning names the risk and lets it run. A named subdirectory is not it. +func TestRecursiveDeleteOfTheWorkingDirectoryWarns(t *testing.T) { + const id = "rm-rf-working-directory" + runVerdictCases(t, []verdictCase{ + {`rm -rf *`, VerdictWarn, id}, + {`rm -fr *`, VerdictWarn, id}, + {`rm -r -f *`, VerdictWarn, id}, + {`rm --recursive --force *`, VerdictWarn, id}, + {`rm -rf .`, VerdictWarn, id}, + {`rm -rf ./`, VerdictWarn, id}, + {`rm -rf ./*`, VerdictWarn, id}, + {`rm -rf ..`, VerdictWarn, id}, + {`rm -rf ../*`, VerdictWarn, id}, + {`rm -rf .*`, VerdictWarn, id}, + {`sudo rm -rf *`, VerdictWarn, id}, + {`rm $(true) -rf *`, VerdictWarn, id}, + // The working directory by name, and the globs that still reach + // everything in it: `rm -rf .` is refused by rm itself, while + // `rm -rf "$PWD"` is the spelling that deletes (the review's low finding). + {`rm -rf $PWD`, VerdictWarn, id}, + {`rm -rf "$PWD"`, VerdictWarn, id}, + {`rm -rf ${PWD}`, VerdictWarn, id}, + {`rm -rf $PWD/*`, VerdictWarn, id}, + {`rm -rf "$PWD"/*`, VerdictWarn, id}, + {`rm -rf ${PWD}/*`, VerdictWarn, id}, + {`rm -rf */`, VerdictWarn, id}, + {`rm -rf ./*/`, VerdictWarn, id}, + {`rm -rf ./.*`, VerdictWarn, id}, + + {`rm -rf ./build`, VerdictAllow, ""}, + {`rm -rf build/*`, VerdictAllow, ""}, + {`rm -rf node_modules`, VerdictAllow, ""}, + {`rm -f *.o`, VerdictAllow, ""}, + {`rm *`, VerdictAllow, ""}, + {`rm -rf .git`, VerdictAllow, ""}, + {`rm -rf .venv`, VerdictAllow, ""}, + {`rm -rf "$PWD/build"`, VerdictAllow, ""}, + {`rm -rf $PWD/build/*`, VerdictAllow, ""}, + + // Behind a cd chain the blocker still decides, and names both. + {`cd scratch && rm -rf *`, VerdictBlock, "rm-rf-after-cd-chain"}, + }) +} + +// TestArgValuesIsValidated holds arg_values to the rule every other pattern +// field keeps: an empty value would match an empty operand nobody meant, and a +// dash-led one describes an operand nothing can be, so both are refused at load +// rather than shipped as an entry that looks armed. +func TestArgValuesIsValidated(t *testing.T) { + for _, bad := range [][]string{{""}, {" "}, {"-rf"}} { + r := Defaults() + r.Entries["x-arg-values"] = Entry{ + ID: "x-arg-values", Tier: TierWarn, Why: "x", Successor: "y", + Pattern: Pattern{Command: "rm", ArgValues: bad}, + } + if err := Validate(r); !errors.Is(err, ErrInvalidEntry) { + t.Errorf("Validate accepted arg_values %q; want ErrInvalidEntry, got %v", bad, err) + } + } + // A repo override replaces the list, and a clone shares no slice with it. + base := Defaults() + over := Registry{SchemaVersion: SchemaVersion, Entries: map[string]Entry{ + "rm-rf-root-or-home": {Pattern: Pattern{ArgValues: []string{"/srv"}}}, + }} + merged := Merge(base, over) + got := merged.Entries["rm-rf-root-or-home"].Pattern.ArgValues + if len(got) != 1 || got[0] != "/srv" { + t.Fatalf("merged arg_values = %q, want [/srv]", got) + } + clone := clonePattern(merged.Entries["rm-rf-root-or-home"].Pattern) + clone.ArgValues[0] = "/changed" + if merged.Entries["rm-rf-root-or-home"].Pattern.ArgValues[0] != "/srv" { + t.Fatal("clonePattern shares its arg_values slice with the pattern it copied") + } +} diff --git a/internal/core/guard/speculate_test.go b/internal/core/guard/speculate_test.go index 20c3247cc..5abfc53c3 100644 --- a/internal/core/guard/speculate_test.go +++ b/internal/core/guard/speculate_test.go @@ -260,11 +260,12 @@ func TestSpeculationTruncationWarnsRatherThanSkipping(t *testing.T) { } // TestSpeculationRespectsAfterCD keeps the fail-safe from inventing a hazard that -// the registry only names in context. `rm -rf *` is a blocker only after a `cd`, -// because that is the shape that eats a working tree when the cd fails; without -// the guard clause a speculative match would fire on every `rm -rf *`. +// the registry only names in context. A recursive delete of a named directory +// is a blocker only after a `cd`, because that is the shape that eats a working +// tree when the cd fails; without the guard clause a speculative match would +// fire on every `rm -rf build`. func TestSpeculationRespectsAfterCD(t *testing.T) { - if d := verdictOf(t, "myrunner rm -rf *"); d.Verdict != VerdictAllow { + if d := verdictOf(t, "myrunner rm -rf build"); d.Verdict != VerdictAllow { t.Errorf("verdict = %q, want %q: the after_cd condition was dropped, so an ordinary rm now warns", d.Verdict, VerdictAllow) } diff --git a/internal/core/guard/tokenize.go b/internal/core/guard/tokenize.go index 0ed8a955f..1def3cd13 100644 --- a/internal/core/guard/tokenize.go +++ b/internal/core/guard/tokenize.go @@ -81,6 +81,16 @@ type segment struct { // unknown.go), and a string handed to a shell carries the value's mark // for the re-read to take as a variable's (payloadView). variable map[int]string + // spelled records, per token index, a word holding a parameter + // expansion's mark as the line WROTE it: each variable's mark replaced by + // its expansion's text (`$HOME`, `${PWD}`; a simple name the next byte + // would extend is braced), every substitution's mark dropped, and a mark + // whose text is not known — a varMark carried into a payload's text — + // kept as unknownMark. Only an entry's arg_values read it + // (writtenOperand): every other reading takes the token, where the + // variable is the unknown word's mark (iss-2609290321312087). nil when no + // word holds a variable. + spelled map[int]string // arrivals caches commandArrivals(tokens) once Check has its final // segments (walked records that it is set), so the walk to command position // is paid once per segment rather than once per entry. A segment built @@ -373,6 +383,11 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { // substitution left their mark in it (addCur). vars map[int]string curVar, curSub bool + // spells rides with the segment (segment.spelled); curVarAt records, + // for the word being built, where in cur each variable's mark stands + // and the expansion's text, "" where it is not known. + spells map[int]string + curVarAt []varSite // curMask is parallel to cur and records, per byte, whether it reached // the tokenizer unquoted (wordStruct) and whether it began its word // (wordRawStart) — what the brace expander needs to read a word the way @@ -548,6 +563,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { case varMark: c = unknownMark curVar = true + curVarAt = append(curVarAt, varSite{at: len(cur)}) case unknownMark: curSub = true } @@ -594,9 +610,31 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { } vars[len(toks)] = text } - // addVar leaves the mark of a parameter expansion where its value goes. - addVar := func() { + // addVar leaves the mark of a parameter expansion where its value goes, + // and records the expansion's text as the line wrote it (segment.spelled). + addVar := func(text string) { addCur([]byte{varMark}, 0) + curVarAt[len(curVarAt)-1].text = text + } + // recordSpelling files the word being built under segment.spelled when a + // variable's mark is in it: word is the token it becomes, and whole + // reports that the token is cur as built, so each mark's place is known. + // A word whose marks cannot be placed — one brace expansion made, or one + // unknownFromOpenExpansion rewrote — is filed as unknownText, which no + // value names. + recordSpelling := func(word string, whole bool) { + if !curVar { + return + } + if spells == nil { + spells = map[int]string{} + } + switch { + case whole: + spells[len(toks)] = spellWritten(cur, curVarAt) + case isUnknown(word): + spells[len(toks)] = unknownText + } } flushToken := func() { if !hasCur { @@ -619,11 +657,13 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { for _, w := range words { recordFeeds() recordVar(false) - toks = append(toks, unknownFromOpenExpansion(string(w.b))) + word := unknownFromOpenExpansion(string(w.b)) + recordSpelling(word, false) + toks = append(toks, word) globs = append(globs, w.globbed()) } cur, curMask, hasCur, curGlob, curBrace = nil, nil, false, false, false - curPieces, curFeeds, curVar, curSub = nil, nil, false, false + curPieces, curFeeds, curVar, curSub, curVarAt = nil, nil, false, false, nil return } braceGroup = true @@ -641,7 +681,8 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curPieces = nil recordFeeds() recordVar(true) - curFeeds, curVar, curSub = nil, false, false + recordSpelling(tok, tok == string(cur)) + curFeeds, curVar, curSub, curVarAt = nil, false, false, nil // An unquoted `{` or `}` in command position opens or closes a group. if len(curMask) == 1 && curMask[0]&wordStruct != 0 && allReserved(toks) { switch tok { @@ -670,12 +711,13 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { segs = append(segs, segment{ tokens: toks, chain: chain, braceGroup: braceGroup, globbed: globsOrNil(globs), stdinStream: curStdin || pipeNext || len(groupIn) > 0, literal: lits, feeds: feeds, piped: piped, - stdinIn: groupIn, home: list, at: len(segs), variable: vars, + stdinIn: groupIn, home: list, at: len(segs), variable: vars, spelled: spells, }) toks = nil globs = nil lits = nil vars = nil + spells = nil feeds = nil braceGroup = false pipeNext = false @@ -815,6 +857,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { openSubstitution := func(kind parenKind, pos int, procSub bool) { saved := &enclosing{ toks: toks, globs: globs, lits: lits, vars: vars, curVar: curVar, curSub: curSub, + spells: spells, curVarAt: curVarAt, cur: cur, curMask: curMask, hasCur: hasCur, curGlob: curGlob, curBrace: curBrace, braceGroup: braceGroup, chain: chain, procSub: procSub, curStdin: curStdin, pipeNext: pipeNext, curDocs: curDocs, pieces: curPieces, @@ -823,6 +866,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { } toks, globs, lits, cur, curMask, hasCur, curGlob, curBrace, braceGroup = nil, nil, nil, nil, nil, false, false, false, false curPieces, vars, curVar, curSub = nil, nil, false, false + spells, curVarAt = nil, nil // A substitution is a command string of its own: its pipelines begin // inside it. Its standard input is its command's: what was piped into // the groups around it, and the pipe into the command it sits in @@ -872,6 +916,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn vars, curVar, curSub = e.vars, e.curVar, e.curSub + spells, curVarAt = e.spells, e.curVarAt if !f.bare { addCur([]byte(arithmeticOperand), 0) // The number it prints is computed from what the substitutions @@ -893,6 +938,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { curStdin, pipeNext, curDocs, curPieces = e.curStdin, e.pipeNext, e.curDocs, e.pieces feeds, curFeeds, pipeFrom, braceFrom, groupIn = e.feeds, e.curFeeds, e.pipeFrom, e.braceFrom, e.groupIn vars, curVar, curSub = e.vars, e.curVar, e.curSub + spells, curVarAt = e.spells, e.curVarAt feedFrom(e.segStart) if e.procSub { addCur([]byte(procSubOperand), 0) @@ -917,7 +963,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { start := len(segs) expandedBody(body) feedFrom(start) - addVar() + addVar("${" + body + "}") if len(segs) > start { curSub = true } @@ -1041,7 +1087,7 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { continue } if k := simpleParamEnd(line, j+1); line[j] == '$' && k >= 0 { - addVar() + addVar(line[j:k]) j = k continue } @@ -1365,9 +1411,10 @@ func tokenizeAt(line string, depth int, budget *int) ([]segment, error) { // is not in the command line, so the word holds unknownMark where // it goes (unknown.go), and `--$X` is a flag of unknown name as // `--$(x)` is. A `$` that is quoted or escaped never reaches here. - addVar() + end := simpleParamEnd(line, i+1) + addVar(line[i:end]) lastList = false - i = simpleParamEnd(line, i+1) + i = end case c == '$' && i+1 < len(line) && line[i+1] == '{': // A `${…}` expansion, read as the double-quoted one is. One whose // `}` is missing is a syntax error bash refuses, and stays text. @@ -2049,6 +2096,8 @@ type enclosing struct { vars map[int]string curVar bool curSub bool + spells map[int]string + curVarAt []varSite cur []byte curMask []byte hasCur bool diff --git a/internal/core/guard/unknown.go b/internal/core/guard/unknown.go index 18d84fc20..4f02ba634 100644 --- a/internal/core/guard/unknown.go +++ b/internal/core/guard/unknown.go @@ -59,7 +59,10 @@ import ( // and its branch (`git commit -m "$(cat msg)"`, `git push origin // "$(git branch --show-current)"`), and reading it as every flag would refuse // both. The same reason keeps an operand's `+` refspec prefix read from its -// known text only. Both residuals are recorded in .abcd/work/DECISIONS.md, +// known text only, and an operand an entry names by its exact word +// (arg_values): `rm -rf $(find . -name '*.pyc')` is how an everyday delete +// names its targets, and reading its operand as every target would refuse it +// as a delete of `/`. The residuals are recorded in .abcd/work/DECISIONS.md, // and a word that is wholly a variable reads the same way (`git push origin // "$branch"`). A variable's value is read as a flag and a program name, and // not as data an earlier command carried (variableCarried). @@ -99,6 +102,69 @@ const varMark = '\x01' // varText is varMark as a string, for spelling a payload's text. const varText = "\x01" +// varSite is one variable's mark in a word being built: its offset in the +// word, and the expansion's text as the line wrote it (`$HOME`, `${PWD}`), "" +// for a varMark read from a payload's text, whose name the string no longer +// holds. +type varSite struct { + at int + text string +} + +// spellWritten is a word as the line wrote its variables (segment.spelled): +// each variable's mark replaced by its expansion's text, each substitution's +// mark dropped as knownText drops it, and a variable whose text is not known +// kept as unknownMark. A simple name the next byte kept would extend is +// braced (`"$A"B` is `${A}B`, not `$AB`), so the spelling reads as the same +// expansions when it is read again (spelledView). sites is in word order. +func spellWritten(word []byte, sites []varSite) string { + var b strings.Builder + k := 0 + isVar := func(p int) bool { return k < len(sites) && sites[k].at == p } + for p := 0; p < len(word); p++ { + if !isVar(p) { + if word[p] != unknownMark { + b.WriteByte(word[p]) + } + continue + } + text := sites[k].text + k++ + if text == "" { + b.WriteByte(unknownMark) + continue + } + if text[1] != '{' { + next := p + 1 + for next < len(word) && word[next] == unknownMark && !isVar(next) { + next++ + } + if next < len(word) && word[next] != unknownMark && isNameByte(word[next]) { + text = "${" + text[1:] + "}" + } + } + b.WriteString(text) + } + return b.String() +} + +// isNameByte reports whether c can continue a shell variable's name. +func isNameByte(c byte) bool { + return c == '_' || c >= '0' && c <= '9' || c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' +} + +// writtenOperand is the text an entry's arg_values compare reads for the +// word at i: the word as the line wrote its variables where it holds one +// (segment.spelled), else its known text. It is read by nothing else, so +// `rm -rf $HOME` names `$HOME` to arg_values while every other reading takes +// the variable as the unknown word it is (iss-2609290321312087). +func writtenOperand(tokens []string, spelled map[int]string, i int) string { + if w, ok := spelled[i]; ok { + return w + } + return knownText(tokens[i]) +} + // isUnknown reports whether a word carries a substitution's output. func isUnknown(tok string) bool { return strings.IndexByte(tok, unknownMark) >= 0 } @@ -630,12 +696,13 @@ func sitesNamed(s segment, name string) []arrival { // operandWant is what an entry asks of a command's operands: operand 0 and 1 // by name, a count, an argument prefix and a resource path carried by some -// operand. +// operand, and one of a set of exact words standing as some operand. type operandWant struct { sub, sub2 string min int prefixes []string paths []PathArg + values []string } // operandAcceptance returns, for each index i of tokens, whether some reading @@ -646,10 +713,15 @@ type operandWant struct { // satisfy every clause together — operand 0 and 1 are the same reading's — so // the table's state is (word, operands so far, clauses met), filled from the // end once: linear in the words, whatever the number of places a command can -// sit. -func operandAcceptance(tokens, valueFlags []string, want operandWant, glob func(int) bool) []bool { +// sit. spelled is the segment's segment.spelled, read by the arg_values +// clause alone (writtenOperand). +func operandAcceptance(tokens []string, spelled map[int]string, valueFlags []string, want operandWant, glob func(int) bool) []bool { need := want.need() - nb := uint(len(want.prefixes) + len(want.paths)) + nv := 0 + if len(want.values) > 0 { + nv = 1 // the values are one clause: any one of them meets it + } + nb := uint(len(want.prefixes) + len(want.paths) + nv) full := 1< 0 && argValueMatches(want.values, writtenOperand(tokens, spelled, i)) { + hits |= 1 << (len(want.prefixes) + len(want.paths)) + } } for k := 0; k <= need; k++ { operand := r.operand && diff --git a/internal/core/guard/unknownreaders_test.go b/internal/core/guard/unknownreaders_test.go index 9089c4f40..552209119 100644 --- a/internal/core/guard/unknownreaders_test.go +++ b/internal/core/guard/unknownreaders_test.go @@ -200,9 +200,11 @@ func plainWord(w string) bool { // that the unknown-word rule must read as the word itself could be: the whole // word printed, its dash kept and its name printed, and its text glued to an // output that may be empty, and each of those printed through a parameter -// expansion's default (`${X:-$(echo w)}`). Two spellings are the recorded residuals and are -// not generated: a wholly-substituted word standing where a flag could be, and -// a `+` refspec whose prefix a substitution prints. +// expansion's default (`${X:-$(echo w)}`). Three spellings are the recorded residuals and are +// not generated: a wholly-substituted word standing where a flag could be, a +// `+` refspec whose prefix a substitution prints, and an operand an entry names +// by its exact word (arg_values) printed whole by a substitution — the last +// filtered by the caller, which knows the entry. func substitutionsOf(w string) []string { glued := `"$(true)"` + w switch { @@ -250,7 +252,14 @@ func TestEverySubstitutionPositionKeepsTheVerdict(t *testing.T) { if !plainWord(w) || (i > 0 && words[i-1] == "for") { continue } - for _, sub := range substitutionsOf(w) { + subs := substitutionsOf(w) + if containsWord(e.Pattern.ArgValues, w) { + // The arg_values residual: a target word printed whole by a + // substitution is read by its known text, so only the glued + // spelling keeps the verdict (argValueMatches). + subs = []string{`"$(true)"` + w} + } + for _, sub := range subs { // Inside single quotes a payload is text until the program // it is handed to reads it: a shell runs a substitution in // it, and env -S never does, so a backtick there is literal @@ -368,6 +377,15 @@ func unknownWrapperPrefixes() []string { return out } +func containsWord(xs []string, w string) bool { + for _, x := range xs { + if x == w { + return true + } + } + return false +} + func sortedEntryIDs(r Registry) []string { ids := make([]string, 0, len(r.Entries)) for id := range r.Entries { diff --git a/internal/core/guard/work_test.go b/internal/core/guard/work_test.go index c5c2d8e35..ba7acb7c5 100644 --- a/internal/core/guard/work_test.go +++ b/internal/core/guard/work_test.go @@ -23,8 +23,15 @@ const linearWorkBar = 6.0 // floor of well over a million units to the bounded operand enumeration // (maxOperandStates), so their per-byte figure falls as the line grows — about // 89 at 18 KB and 28 at the 64 KB cap (review4-guard). Dropping one bound -// measures well over a hundred on a line of any length. -const workPerByteBar = 20.0 +// measures well over a hundred on a line of any length. The operand walk reads +// every token once per entry whose command the name can be, so the constant +// grows with the registry: an unknown program name followed by unknown +// dash-words (`$(a) -$(b) x` repeated) measured about 18 units per byte with +// fourteen bundled entries and about 20 with sixteen, the two rm-target +// entries of iss-2609282105242542. The bar sits at 24, which keeps that shape +// under it with room for a few more entries and stays far below the hundred a +// dropped bound costs. +const workPerByteBar = 24.0 // checkWork runs one check over line against the bundled registry and returns // the work the guard counted doing it (tally in work.go): bytes tokenized, bytes diff --git a/internal/core/guard/workdir_test.go b/internal/core/guard/workdir_test.go index ca47f478f..7f4c4e79b 100644 --- a/internal/core/guard/workdir_test.go +++ b/internal/core/guard/workdir_test.go @@ -117,10 +117,15 @@ func TestResolveWorkdirRefusesMalformedValues(t *testing.T) { // exist fails the whole call instead (probe: NotFound, nothing ran), so the // failed-cd hazard is absent and folding the workdir into the string as // `cd && ` would block every workdir'd recursive delete for a hazard -// the host does not have. The command string alone decides the cd-chain entry. +// the host does not have. The command string alone decides the cd-chain entry: +// a bare `rm -rf *` meets only the working-directory warn, which it meets in +// any directory, and never the cd-chain blocker. func TestAHostWorkdirIsNeverReadAsACd(t *testing.T) { - if d := checkOK(t, "rm -rf *"); d.Verdict != VerdictAllow { - t.Errorf("a bare recursive delete (run in a host workdir) must stay allowed: %+v", d) + if d := checkOK(t, "rm -rf ./build"); d.Verdict != VerdictAllow { + t.Errorf("a bare recursive delete of a named directory (run in a host workdir) must stay allowed: %+v", d) + } + if d := checkOK(t, "rm -rf *"); d.Verdict == VerdictBlock || contains(d.Matches, "rm-rf-after-cd-chain") { + t.Errorf("a bare recursive delete (run in a host workdir) must not read as a cd chain: %+v", d) } if d := checkOK(t, "cd scratch && rm -rf *"); d.Verdict != VerdictBlock || d.EntryID != "rm-rf-after-cd-chain" { t.Errorf("a cd chain inside the command string must still block: %+v", d) diff --git a/internal/core/surface/sentence_test.go b/internal/core/surface/sentence_test.go index 145432d7a..b9206cc86 100644 --- a/internal/core/surface/sentence_test.go +++ b/internal/core/surface/sentence_test.go @@ -280,3 +280,21 @@ func TestSentenceChangesNamesEachRewordedVerb(t *testing.T) { t.Fatalf("SentenceChanges(same, same) = %v, want none", again) } } + +// TestBareLintSentenceNamesWhatTheBareRunLeavesOut — iss-2609282105240689. The +// bare `abcd lint` runs every target that judges the repository and leaves the +// outbound target out (repolint.DefaultRules carries no outbound rule, and the +// lint chapter says so), so its one-line sentence may not claim every target, +// and must name the one it leaves out. +func TestBareLintSentenceNamesWhatTheBareRunLeavesOut(t *testing.T) { + s, ok := SentenceFor("abcd lint") + if !ok { + t.Fatal("no sentence for abcd lint") + } + if strings.Contains(s, "every target included") { + t.Errorf("the bare lint sentence claims every target, but the bare run leaves outbound out: %q", s) + } + if !strings.Contains(s, "outbound") { + t.Errorf("the bare lint sentence does not name the outbound target it leaves out: %q", s) + } +} diff --git a/internal/core/surface/sentences.go b/internal/core/surface/sentences.go index 0e97d5465..7765a4fcb 100644 --- a/internal/core/surface/sentences.go +++ b/internal/core/surface/sentences.go @@ -246,7 +246,7 @@ var sentences = map[string]string{ "abcd launch ship": "Cut a release, deriving its version and records from what shipped: " + "Writes the CHANGELOG heading, RELEASE.md, and the archive pin; refuses a cut its gates stop.", - "abcd lint": "Check this repository against the conventions, every target included: " + + "abcd lint": "Check this repository against the conventions, every target but outbound: " + "Writes nothing; refuses with exit 2 on an error finding and exit 1 on warnings alone.", "abcd lint docs": "Lint the docs for change-narration, broken links, citations, and stray root markdown: " + "Writes nothing; refuses a tree with a blocker finding.", diff --git a/internal/core/update/update.go b/internal/core/update/update.go index 87aae3add..979dd8116 100644 --- a/internal/core/update/update.go +++ b/internal/core/update/update.go @@ -97,7 +97,7 @@ func (o Ownership) Prose() string { // Report is the update receipt: origin, tag, digest, and what happened. It // prints in both TTY and piped modes — silence is only ever about progress. type Report struct { - Origin string `json:"origin"` + Origin string `json:"origin,omitempty"` Tag string `json:"tag,omitempty"` Asset string `json:"asset,omitempty"` Digest string `json:"digest,omitempty"` diff --git a/internal/surface/cli/guard.go b/internal/surface/cli/guard.go index c3bdb0e93..c80743671 100644 --- a/internal/surface/cli/guard.go +++ b/internal/surface/cli/guard.go @@ -109,7 +109,9 @@ func newGuardCommand(asJSON *bool) *cobra.Command { "expanded as bash expands it, and one past 4096 words is blocked. What an\n" + "allow still does not see is a hazard that never reaches command position at\n" + "all: a word that is wholly a `$(…)` standing where a flag would be (read as\n" + - "an operand, the way a commit message or a branch is spelled), one launched\n" + + "an operand, the way a commit message or a branch is spelled), a delete\n" + + "target printed whole by one (`rm -rf $(echo /)`, read by its known text\n" + + "the way `rm -rf $(find …)` names its targets every day), one launched\n" + "through a known\n" + "wrapper carrying a value-taking flag the guard does not name (`sudo -u bob\n" + "` is seen; the bundled short form `sudo -Hu bob ` reaches\n" + diff --git a/internal/surface/cli/guard_workdir_test.go b/internal/surface/cli/guard_workdir_test.go index 88720f7e5..1b2461474 100644 --- a/internal/surface/cli/guard_workdir_test.go +++ b/internal/surface/cli/guard_workdir_test.go @@ -135,19 +135,26 @@ func TestGuardHookWorkdirRegistryCannotDisarmTheSession(t *testing.T) { // the session directory. So the failed-cd hazard rm-rf-after-cd-chain exists for // does not exist for a host workdir, and the guard must not manufacture it by // reading the workdir as `cd &&`. The cd chain spelled in the command -// string still blocks, workdir or not. +// string still blocks, workdir or not. A bare `rm -rf *` meets only the +// working-directory warn, as it does with no workdir at all. func TestGuardHookMissingWorkdirIsNotAFailedCd(t *testing.T) { dir := workdirSession(t) if err := os.WriteFile(filepath.Join(dir, "afile"), nil, 0o644); err != nil { t.Fatal(err) } for _, wd := range []string{"missing", "afile", "cold"} { - t.Run("rm -rf * in workdir "+wd, func(t *testing.T) { - _, stderr, code := runGuard(preToolUseIn(t, "rm -rf *", dir, wd), "guard", "hook") + t.Run("rm -rf ./build in workdir "+wd, func(t *testing.T) { + _, stderr, code := runGuard(preToolUseIn(t, "rm -rf ./build", dir, wd), "guard", "hook") if code != 0 { t.Errorf("a host workdir is not a shell cd: want exit 0, got %d (stderr %q)", code, stderr) } }) + t.Run("rm -rf * in workdir "+wd, func(t *testing.T) { + _, stderr, code := runGuard(preToolUseIn(t, "rm -rf *", dir, wd), "guard", "hook") + if code == 2 || strings.Contains(stderr, "rm-rf-after-cd-chain") { + t.Errorf("a host workdir is not a shell cd: want no cd-chain block, got exit %d (stderr %q)", code, stderr) + } + }) } t.Run("a cd chain in the command still blocks inside a workdir", func(t *testing.T) { _, stderr, code := runGuard(preToolUseIn(t, "cd scratch && rm -rf *", dir, "cold"), "guard", "hook") diff --git a/internal/surface/cli/update.go b/internal/surface/cli/update.go index 739e19215..dfffae09d 100644 --- a/internal/surface/cli/update.go +++ b/internal/surface/cli/update.go @@ -2,6 +2,7 @@ package cli import ( "bufio" + "errors" "fmt" "io" "os" @@ -59,9 +60,7 @@ func newUpdateCommand(asJSON *bool) *cobra.Command { // the updater exists. tgt := ahoy.ResolveUpdateTarget() if r := update.Plan(tgt); r != nil { - rep := refusalReport(tgt, r) - renderUpdateReport(cmd.OutOrStdout(), *asJSON, rep) - return fmt.Errorf("update refused (%s)", r.Shape) + return refuseUpdate(cmd.OutOrStdout(), *asJSON, refusalReport(tgt, r)) } u := newUpdater() @@ -95,10 +94,10 @@ func newUpdateCommand(asJSON *bool) *cobra.Command { if rep.Action == update.ActionSwapped || rep.Action == update.ActionCurrent { ahoy.RefreshPathEntryDigest(tgt.Path, rep.Digest) } - renderUpdateReport(cmd.OutOrStdout(), *asJSON, rep) if rep.Refusal != nil { - return fmt.Errorf("update refused (%s)", rep.Refusal.Shape) + return refuseUpdate(cmd.OutOrStdout(), *asJSON, rep) } + renderUpdateReport(cmd.OutOrStdout(), *asJSON, rep) return nil }, } @@ -124,6 +123,34 @@ func refusalReport(tgt ahoy.UpdateTarget, r *update.Refusal) update.Report { return update.Report{Action: update.ActionRefused, TargetPath: fsutil.RedactHome(tgt.Path), Refusal: r} } +// updateRefusal is the one --json document a refused update writes: the +// receipt, carrying the refusal's shape, detail and remedy, with the global +// --json refusal envelope's three fields (`"abcd": "error"`, the error, the +// exit code) around it. Run would otherwise write the envelope as a second +// document after the receipt, and a machine reader expecting one document +// would read the receipt and miss the refusal, or fail on the second value +// (iss-2609282105241960). +type updateRefusal struct { + Abcd string `json:"abcd"` + update.Report + Error string `json:"error"` + ExitCode int `json:"exit_code"` +} + +// refuseUpdate renders a refusal receipt and returns the error that exits 1. +// In text mode the receipt goes to stdout and Run prints the error line on +// stderr, as before. Under --json the receipt IS the refusal envelope, so the +// returned error carries no message and Run adds no second document. +func refuseUpdate(w io.Writer, asJSON bool, rep update.Report) error { + msg := fmt.Sprintf("update refused (%s)", rep.Refusal.Shape) + if !asJSON { + renderUpdateReport(w, false, rep) + return errors.New(msg) + } + _ = render(w, true, updateRefusal{Abcd: "error", Report: rep, Error: msg, ExitCode: 1}, nil) + return &exitError{Code: 1, Msg: ""} +} + // renderUpdateReport prints the receipt in both modes. Tags and paths pass // through termsafe on the text render: the tag is read off HTTP responses. func renderUpdateReport(w io.Writer, asJSON bool, rep update.Report) { diff --git a/internal/surface/cli/update_test.go b/internal/surface/cli/update_test.go index 6c71a6f83..eb861d916 100644 --- a/internal/surface/cli/update_test.go +++ b/internal/surface/cli/update_test.go @@ -2,6 +2,8 @@ package cli import ( "bytes" + "encoding/json" + "io" "os" "path/filepath" "strings" @@ -201,3 +203,53 @@ func TestUpdateReceiptKeepsTheOrdinaryVersionLine(t *testing.T) { t.Errorf("a provable old build must not be reported as unpublished:\n%s", got) } } + +// TestUpdateJSONRefusalIsOneDocument — iss-2609282105241960. `update --json` on +// a dispatch refusal printed the receipt and then Run's error envelope: two +// JSON documents where a machine reader expects one. The refusal is ONE +// document that is both the receipt the chapter describes (action, the +// refusal's shape, detail and remedy) and the refusal the global --json +// contract describes (`"abcd": "error"`, the error, the exit code), and it +// carries no empty origin, since no release origin was reached. +func TestUpdateJSONRefusalIsOneDocument(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("ABCD_PLUGIN_ROOT", "") + t.Setenv("CLAUDE_PLUGIN_ROOT", "") + t.Setenv("ABCD_BIN_TARGET", "") + t.Setenv("PATH", filepath.Join(home, ".local", "bin")) + + var stdout, stderr bytes.Buffer + code := Run([]string{"update", "--json"}, &stdout, &stderr) + if code != 1 { + t.Fatalf("exit = %d, want 1 for a refusal; stdout %q stderr %q", code, stdout.String(), stderr.String()) + } + dec := json.NewDecoder(bytes.NewReader(stdout.Bytes())) + var doc map[string]any + if err := dec.Decode(&doc); err != nil { + t.Fatalf("stdout is not JSON: %v\n%s", err, stdout.String()) + } + var extra json.RawMessage + if err := dec.Decode(&extra); err != io.EOF { + t.Fatalf("stdout holds more than one JSON document (second: %s, err %v):\n%s", extra, err, stdout.String()) + } + if doc["abcd"] != "error" || doc["action"] != "refused" { + t.Errorf("the document is not both the refusal envelope and the receipt: %v", doc) + } + if ec, _ := doc["exit_code"].(float64); ec != 1 { + t.Errorf("exit_code = %v, want 1", doc["exit_code"]) + } + if msg, _ := doc["error"].(string); !strings.Contains(msg, "update refused (absent)") { + t.Errorf("error = %q, want it to name the refusal", doc["error"]) + } + ref, _ := doc["refusal"].(map[string]any) + if ref == nil || ref["shape"] != "absent" || ref["remedy"] == "" || ref["remedy"] == nil { + t.Errorf("the refusal block does not name its shape and remedy: %v", doc["refusal"]) + } + if _, ok := doc["origin"]; ok { + t.Errorf("a refusal raised before any fetch carries an origin key: %v", doc) + } + if stderr.Len() != 0 { + t.Errorf("--json wrote to stderr: %q", stderr.String()) + } +}