From 8b7ac5b51e32bb5d29be48b11b5ae7aea8034be6 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Wed, 12 Aug 2026 17:20:11 -0500 Subject: [PATCH] guard: close four fail-open holes found by independent review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An independent reviewer broke all three new controls before they shipped. Every finding is reproduced as a fixture or a repro, because the class is recurring rather than incidental: each hole was a case where the answer was "allow" because something was ABSENT rather than because it was CHECKED. 1. wrapper-guard read only the spellings it knew. `curl -d@body` (no space), `--request=POST` (equals form), and a URL path assembled from shell variables each carried a real provider write straight through. Write detection now covers every body and method form curl accepts, and the endpoint match no longer anchors on a literal host path that a variable can dissolve. 2. wrapper-guard blocked only when the wrapper FILE existed. A host with a broken or partial install therefore permitted exactly the raw writes the guard exists to stop. Blocking is now on the endpoint; a missing wrapper changes the remedy text, not the verdict — a broken install is not permission to bypass gate 7. 3. mosaic-worktree read a worktree's safety from two questions, and a clean, fully-pushed tree holding a gitignored `local.secret` answered both with zero. `git worktree remove` then deleted the one copy in existence. A file is gitignored precisely so nothing else holds it, so ignored-but-not- disposable files are now a third evidence question. Build junk (node_modules, .venv, dist, caches, *.pyc) stays disposable, so the common case still reads SAFE. 4. check-tools-index counted a documented tool as discoverable at mode 0644. Every caller tests `[ -x ]`, so a non-executable tool is a missing tool; it now fails the gate with its own message. Local gates green: sanitization, resident budget, test enumeration, tools-index (4/4 self-test, 100% on the enforced git suite), and wrapper-guard 20/20. --- .../framework/tools/git/mosaic-worktree.sh | 61 ++++++++++++---- .../framework/tools/git/test-wrapper-guard.sh | 13 ++++ .../framework/tools/git/wrapper-guard.sh | 71 +++++++++++++------ .../quality/scripts/check-tools-index.sh | 45 +++++++++--- 4 files changed, 146 insertions(+), 44 deletions(-) diff --git a/packages/mosaic/framework/tools/git/mosaic-worktree.sh b/packages/mosaic/framework/tools/git/mosaic-worktree.sh index 7d230934..a770f798 100755 --- a/packages/mosaic/framework/tools/git/mosaic-worktree.sh +++ b/packages/mosaic/framework/tools/git/mosaic-worktree.sh @@ -31,9 +31,11 @@ # mosaic-worktree.sh rm [--force] remove; refuses to lose work # mosaic-worktree.sh gc [--apply] report/remove clean+pushed worktrees # -# `rm` and `gc` refuse to delete a worktree with uncommitted changes or with -# commits absent from every remote. That check is by EVIDENCE, never by size or -# age. --force overrides it and is yours to type deliberately. +# `rm` and `gc` refuse to delete a worktree with uncommitted changes, with +# commits absent from every remote, or holding ignored files that are not of the +# well-known regenerable kind (a `.env` is ignored so it is never committed, +# which is also why nothing else holds a copy). That check is by EVIDENCE, never +# by size or age. --force overrides it and is yours to type deliberately. # # Run from anywhere inside the repo, or pass --repo . @@ -98,16 +100,41 @@ configuration, credentials, state and caches — not checkouts." ;; # Two independent questions, both answered from git, neither from size or age: # dirty — anything uncommitted in the tree # unpushed — commits reachable from HEAD that no remote ref contains +# precious — IGNORED files git will not mention and will not miss +# +# The third question is not obvious and was missed on the first pass. An +# independent reviewer demonstrated it in four commands: a pushed, clean +# worktree whose .gitignore covers `*.secret`, holding one `local.secret`. +# `git status --porcelain` is empty, `rev-list --count HEAD --not --remotes` is +# 0 — the evidence reads SAFE — and `git worktree remove` deletes the file. The +# same shape covers `.env`, credentials, scratch notes, downloaded fixtures: +# precisely the files that are ignored BECAUSE they must not be committed, which +# is also why nothing else is holding a copy. +# +# So ignored files count as work unless they are the well-known regenerable +# kind. Getting that set wrong is asymmetric: an over-broad list preserves a +# worktree that could have been reclaimed (cheap, visible, fixable by --force), +# an over-narrow one deletes the only copy of a secret (silent, permanent). +# The list stays short and conservative for that reason. +DISPOSABLE_RE='(^|/)(node_modules|\.venv|venv|__pycache__|\.mypy_cache|\.pytest_cache|\.ruff_cache|\.turbo|\.cache|\.parcel-cache|\.gradle|dist|build|out|target|coverage|\.next|\.nuxt|\.svelte-kit)(/|$)|\.(pyc|pyo|o|class)$' + wt_dirty() { git -C "$1" status --porcelain 2>/dev/null | head -200 | wc -l; } wt_unpushed() { git -C "$1" rev-list --count HEAD --not --remotes 2>/dev/null || echo "?"; } +# Default --ignored (not =matching) so a 40k-file node_modules collapses to one +# directory entry instead of being enumerated and then discarded. +wt_precious() { + git -C "$1" status --porcelain --ignored 2>/dev/null \ + | awk '/^!! /{print substr($0,4)}' \ + | grep -Ev "$DISPOSABLE_RE" | head -200 | wc -l +} wt_state() { - local wt="$1" d u - d="$(wt_dirty "$wt")"; u="$(wt_unpushed "$wt")" - if [ "$d" -eq 0 ] && [ "$u" = "0" ]; then - printf 'SAFE\tclean; 0 unpushed' + local wt="$1" d u p + d="$(wt_dirty "$wt")"; u="$(wt_unpushed "$wt")"; p="$(wt_precious "$wt")" + if [ "$d" -eq 0 ] && [ "$u" = "0" ] && [ "$p" -eq 0 ]; then + printf 'SAFE\tclean; 0 unpushed; no ignored files worth keeping' else - printf 'PRESERVE\t%s uncommitted; %s unpushed' "$d" "$u" + printf 'PRESERVE\t%s uncommitted; %s unpushed; %s ignored-but-not-disposable' "$d" "$u" "$p" fi } @@ -180,14 +207,18 @@ cmd_rm() { local path; path="$(derive_path "$branch")" [ -d "$path" ] || die "no worktree at $path" - local d u - d="$(wt_dirty "$path")"; u="$(wt_unpushed "$path")" - if [ "$force" -eq 0 ] && { [ "$d" -ne 0 ] || [ "$u" != "0" ]; }; then + local d u p + d="$(wt_dirty "$path")"; u="$(wt_unpushed "$path")"; p="$(wt_precious "$path")" + if [ "$force" -eq 0 ] && { [ "$d" -ne 0 ] || [ "$u" != "0" ] || [ "$p" -ne 0 ]; }; then die "refusing to remove $path - uncommitted files: $d - unpushed commits: $u -Commit and push first — that is the contract. If this work is genuinely -disposable, re-run with --force." + uncommitted files: $d + unpushed commits: $u + ignored, not disposable: $p +Commit and push first — that is the contract. Ignored files are counted because +git will neither report them nor miss them: a .env or a *.secret is ignored +precisely so it is never committed, which is also why nothing else holds a copy. +List them with: git -C $path status --porcelain --ignored | grep '^!!' +If this work is genuinely disposable, re-run with --force." fi # NB: ${force:+--force} would expand for force=0 too ("0" is non-empty). diff --git a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh index 8c268ec2..3782464e 100755 --- a/packages/mosaic/framework/tools/git/test-wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/test-wrapper-guard.sh @@ -39,6 +39,19 @@ FIXTURES="$TMP/fixtures.tsv" printf '0\t{"tool_input":{"command":"ls -la /src"}}\tordinary commands are untouched\n' printf '0\t{"tool_input":{"command":"MOSAIC_WRAPPER_OVERRIDE=1 curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls"}}\tbreak-glass works\n' printf '0\t{"tool_input":{}}\tan empty payload does not block the session\n' + # --- bypasses an independent reviewer demonstrated against the first version. + # Each of these returned 0 (allowed) and each is a real write. They are pinned + # as fixtures rather than fixed-and-forgotten because the class is recurring: + # the guard reads text, so every spelling it does not know is a hole. + printf '2\t{"tool_input":{"command":"curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\t-d@body with no space is still a body\n' + printf '2\t{"tool_input":{"command":"curl --request=POST -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\t--request=POST equals-form is still a method\n' + printf '2\t{"tool_input":{"command":"p=/api/v1/repo; q=s/a/b/pulls/1/reviews; curl -d@b https://git.example.invalid${p}${q}"}}\ta path split across variables is still that path\n' + printf '2\t{"tool_input":{"command":"curl --data-binary @b https://git.example.invalid/api/v1/repos/a/b/issues/1/comments"}}\t--data-binary is a body\n' + printf '2\t{"tool_input":{"command":"curl -F f=@b https://git.example.invalid/api/v1/repos/a/b/issues"}}\t-F multipart is a body\n' + # Reads must survive every one of those broadenings, or the guard gets disabled. + printf '0\t{"tool_input":{"command":"curl -s https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tno body and no verb is a read\n' + printf '0\t{"tool_input":{"command":"grep -rn /pulls/ src/ | head -20"}}\ta path fragment in a grep is not an API call\n' + printf '0\t{"tool_input":{"command":"curl -X POST -d @b https://registry.example.invalid/v2/x/manifests/latest"}}\tan unwrapped API is not this guard'"'"'s business\n' } > "$FIXTURES" fail=0 n=0 diff --git a/packages/mosaic/framework/tools/git/wrapper-guard.sh b/packages/mosaic/framework/tools/git/wrapper-guard.sh index 9e815c13..a3e8b445 100755 --- a/packages/mosaic/framework/tools/git/wrapper-guard.sh +++ b/packages/mosaic/framework/tools/git/wrapper-guard.sh @@ -74,35 +74,68 @@ EOF fi # ---- 2/3. provider API writes --------------------------------------------- -# Only consider calls that are (a) to a provider API path and (b) mutating. -is_api=0 -printf '%s' "$CMD" | grep -Eq '/api/v1/repos/|api\.github\.com/repos/' && is_api=1 -if [ "$is_api" -eq 1 ]; then +# A raw provider write is four things at once: an HTTP client, a URL, a mutating +# verb or a request body, and a path fragment naming an endpoint a wrapper +# already owns. All four are required, which is what keeps reads and unwrapped +# endpoints flowing. +# +# Deliberately NOT gated on the literal "/api/v1/repos/". An independent reviewer +# broke that version in one line: build the path in shell variables +# p=/api/v1/repo; q=s/a/b/pulls/1/reviews; curl -d@body "https://host${p}${q}" +# and the host-anchored literal never appears, so the check read clean while the +# write went through. The endpoint fragments below survive it, because the +# fragment has to appear somewhere for the URL to be constructible at all. +if printf '%s' "$CMD" | grep -Eq 'curl|wget|http(ie)?[[:space:]]' \ + && printf '%s' "$CMD" | grep -Eq 'https?://'; then + + # Write detection. Every spelling curl accepts, because the guard is defeated + # by the one spelling it does not know: `-d@body` (no space) and + # `--request=POST` (equals form) both slipped past the first version. is_write=0 - printf '%s' "$CMD" | grep -Eq -- '-X[[:space:]]*(POST|PATCH|PUT|DELETE)|--request[[:space:]]*(POST|PATCH|PUT|DELETE)' && is_write=1 - # curl sends POST implicitly when given a body. - printf '%s' "$CMD" | grep -Eq -- '--data|-d[[:space:]]' && is_write=1 + printf '%s' "$CMD" | grep -Eq -- \ + '-X[[:space:]]*(POST|PATCH|PUT|DELETE)|--request[[:space:]=]*(POST|PATCH|PUT|DELETE)' && is_write=1 + # curl sends POST implicitly when handed a body, in any of these forms. + printf '%s' "$CMD" | grep -Eq -- \ + '(^|[[:space:]])(-d|-F|-T)|--data([-a-z]*)?[[:space:]=]|--json[[:space:]=]|--form|--upload-file' && is_write=1 if [ "$is_write" -eq 1 ]; then endpoint=""; wrapper="" case "$CMD" in *"/pulls/"*"/reviews"*|*"/pulls/"*"/requested_reviewers"*) - endpoint="pull-request review"; wrapper="$W/pr-review.sh" ;; - *"/pulls/"*"/merge"*) endpoint="pull-request merge"; wrapper="$W/pr-merge.sh" ;; - *"/issues/"*"/comments"*) endpoint="issue comment"; wrapper="$W/issue-comment.sh" ;; - *"/pulls"*) endpoint="pull request"; wrapper="$W/pr-create.sh" ;; - *"/issues"*) endpoint="issue"; wrapper="$W/issue-create.sh" ;; - *"/milestones"*) endpoint="milestone"; wrapper="$W/milestone-create.sh" ;; + endpoint="pull-request review"; wrapper="pr-review.sh" ;; + *"/pulls/"*"/merge"*) endpoint="pull-request merge"; wrapper="pr-merge.sh" ;; + *"/issues/"*"/comments"*) endpoint="issue comment"; wrapper="issue-comment.sh" ;; + *"/pulls"*) endpoint="pull request"; wrapper="pr-create.sh" ;; + *"/issues"*) endpoint="issue"; wrapper="issue-create.sh" ;; + *"/milestones"*) endpoint="milestone"; wrapper="milestone-create.sh" ;; esac - if [ -n "$wrapper" ] && [ -x "$wrapper" ]; then + # Block on the ENDPOINT, never on whether the wrapper file happens to exist. + # The previous version required `[ -x "$W/$wrapper" ]`, which meant a host + # with a broken or absent install allowed exactly the raw writes the guard + # exists to stop — an absence-driven allow, and the second one found in this + # file. A missing wrapper is a broken install; it is not a licence to bypass + # gate 7. Say so, and say which is which. + if [ -n "$endpoint" ]; then + if [ -x "$W/$wrapper" ]; then + remedy="Use the wrapper the Constitution (gate 7) requires: + + $W/$wrapper + +Run \`$wrapper --help\` for the flags." + else + remedy="The wrapper that covers this endpoint is \`$wrapper\`, and it is NOT +present or not executable at: + + $W/$wrapper + +That is a broken or incomplete install, not permission to send the call raw. +Repair the install (\`mosaic doctor\`) and use the wrapper." + fi cat < "$tmp/tools/git/documented-tool.sh" printf '#!/bin/sh\n' > "$tmp/tools/git/test-ignored.sh" + chmod +x "$tmp/tools/git/documented-tool.sh" "$tmp/tools/git/test-ignored.sh" # run_check reads the TOOLS_DIR / DOCS globals; an array cannot ride in a # command-prefix assignment, so point the globals at the fixture directly. @@ -231,18 +244,18 @@ self_test() { # Case 1: fully documented -> pass. printf 'see tools/git/documented-tool.sh for details\n' > "$tmp/doc.md" if run_check >/dev/null; then - printf 'self-test 1/3 ok (complete index passes)\n' + printf 'self-test 1/4 ok (complete index passes)\n' else - printf 'self-test 1/3 FAIL (complete index should pass)\n'; return 1 + printf 'self-test 1/4 FAIL (complete index should pass)\n'; return 1 fi # Case 2: an undocumented tool -> fail. printf '#!/bin/sh\n' > "$tmp/tools/git/undocumented-tool.sh" rc=0; run_check >/dev/null || rc=$? if [ "$rc" -eq 1 ]; then - printf 'self-test 2/3 ok (undocumented tool fails the gate)\n' + printf 'self-test 2/4 ok (undocumented tool fails the gate)\n' else - printf 'self-test 2/3 FAIL (undocumented tool should fail, got rc=%s)\n' "$rc"; return 1 + printf 'self-test 2/4 FAIL (undocumented tool should fail, got rc=%s)\n' "$rc"; return 1 fi # Case 3: a stale index reference -> fail. @@ -250,12 +263,26 @@ self_test() { printf 'also tools/git/deleted-tool.sh\n' >> "$tmp/doc.md" rc=0; run_check >/dev/null || rc=$? if [ "$rc" -eq 1 ]; then - printf 'self-test 3/3 ok (stale index reference fails the gate)\n' + printf 'self-test 3/4 ok (stale index reference fails the gate)\n' else - printf 'self-test 3/3 FAIL (stale reference should fail, got rc=%s)\n' "$rc"; return 1 + printf 'self-test 3/4 FAIL (stale reference should fail, got rc=%s)\n' "$rc"; return 1 fi - printf '\nself-test passed: the gate demonstrably reds on both drift directions.\n' + # Case 4: documented, present, and NOT executable -> fail. Found by an + # independent reviewer: a 0644 wrapper scored 100% here while reading as + # absent to every `[ -x ]` in the fleet, including the wrapper guard's. + sed -i '/deleted-tool/d' "$tmp/doc.md" + printf '#!/bin/sh\n' > "$tmp/tools/git/noexec-tool.sh" + chmod 0644 "$tmp/tools/git/noexec-tool.sh" + printf 'and tools/git/noexec-tool.sh\n' >> "$tmp/doc.md" + rc=0; run_check >/dev/null || rc=$? + if [ "$rc" -eq 1 ]; then + printf 'self-test 4/4 ok (documented but non-executable tool fails the gate)\n' + else + printf 'self-test 4/4 FAIL (non-executable tool should fail, got rc=%s)\n' "$rc"; return 1 + fi + + printf '\nself-test passed: the gate demonstrably reds on every drift direction.\n' } if [ "$SELF_TEST" -eq 1 ]; then