wrapper-guard: close three fail-opens the last round left, and enumerate the large-repo test
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
Round-two remediation of the four blockers gate-ultron-01 raised onf8d04d1b. All four were confirmed by my own measurement before being touched; none is taken on the reviewer's word. 1. The --config/-K refusal never ran. It sat nested inside `if API_SHAPED`, and API_SHAPED is a test for a provider URL in the command text — which is exactly what a config file removes. The check was guarded by the condition that the capability it guards against defeats, so `curl --config /tmp/write.cfg` walked past it. It now keys on curl itself, ahead of the URL gate, and covers the attached (`-K/tmp/f`) and bundled (`-sK`) spellings a space-separated test cannot see. 2. Percent-encoded endpoints are a live route, not a theoretical one. Measured against the provider: `…/issues/1174` and `…/iss%75es/1174` both return HTTP 200 for the same object. A write carrying any percent-escape is now refused rather than decoded — a decoder has to be exactly right about depth (%2569 -> %69 -> i) and about the provider's own normalisation, and being approximately right there is indistinguishable from not checking. Scoped to writes: a read is never this hook's business and a query string carrying %20 is an ordinary URL. 3. HOME was still expanded unguarded at the `W=` fallback, which runs before any of the new HOME adjudication — so a guard deployed without its siblings still died on an unset HOME, upstream of the fix that was supposed to survive it. Moving a fail-open earlier in the file is not closing it. HOME is now resolved once, above every use, and every later site reads the resolved value. The existing harness could not have caught this: it runs the guard beside its siblings, so `[ -x "$W/pr-review.sh" ]` always succeeded and the fallback was never reached. A test's blind spot can be a property of the harness rather than of the code. The new lone_case() block copies the guard alone into an empty directory and re-asserts the four behaviours there. 4. test-mosaic-worktree-large-repo.sh shipped at mode 100644 and appeared in no CI step, so the enumeration guard (#1017) redded pipeline 2386 — correctly. Committed mode is now 100755 and the test is enumerated in the sanitization step. My own process miss: I verified the CI queue before pushing and never verified terminal CI after. Controls: the 25 fixtures added here all FAIL against029af418(rc 0 or 1 where 2 is required) and all pass at this head, 143/143.
This commit is contained in:
Hermes Agent
@@ -61,6 +61,13 @@ steps:
|
||||
# endpoints and ordinary commands through. Both directions are asserted —
|
||||
# a guard that over-blocks gets routed around, which fails just as hard.
|
||||
- bash packages/mosaic/framework/tools/git/test-wrapper-guard.sh
|
||||
# Hermetic regression for mosaic-worktree.sh at fleet scale: stubs git onto
|
||||
# PATH so `list` faces ~450 KB of porcelain. The defect it pins is invisible
|
||||
# at small size — `git … | awk '…exit'` gives the producer SIGPIPE, which
|
||||
# under `set -euo pipefail` aborts the caller silently with rc=141 and no
|
||||
# output. A repo only reaches that once it has enough worktrees, so the
|
||||
# stub supplies the scale instead of the host's own checkout.
|
||||
- bash packages/mosaic/framework/tools/git/test-mosaic-worktree-large-repo.sh
|
||||
|
||||
# Blocking gate (#791): a framework upgrade must never write or delete an
|
||||
# operator-owned path. The HARD GATE proves an unanticipated operator sentinel
|
||||
|
||||
Regular → Executable
@@ -267,12 +267,34 @@ FIXTURES="$TMP/fixtures.tsv"
|
||||
# and the call went through. An unreadable request is not a cleared one.
|
||||
printf '2\t{"tool_input":{"command":"curl --config /tmp/req https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews"}}\tthe request in a config file is unreadable, so it is refused\t--config/-K\n'
|
||||
printf '2\t{"tool_input":{"command":"curl -K /tmp/req https://git.example.invalid/api/v1/repos/a/b/issues"}}\tthe short spelling, same answer\t--config/-K\n'
|
||||
# Cost, stated: the scope gate admits any https URL, so this refuses a
|
||||
# --config read against a host that has nothing to do with a forge. The
|
||||
# alternative is to require a forge marker in a command whose URL may itself
|
||||
# be in the file, which is the hole again.
|
||||
# Cost, stated: this refuses a --config read against a host that has nothing
|
||||
# to do with a forge. The alternative is to require a forge marker in a
|
||||
# command whose URL may itself be in the file, which is the hole again.
|
||||
printf '2\t{"tool_input":{"command":"curl --config /tmp/req https://example.invalid/anything"}}\tan unrelated https URL with --config is refused too, by decision\t--config/-K\n'
|
||||
printf '0\t{"tool_input":{"command":"eslint --config .eslintrc.json src/"}}\t--config outside an API-shaped command is nobody'"'"'s business\n'
|
||||
printf '0\t{"tool_input":{"command":"eslint --config .eslintrc.json src/"}}\t--config on a command that is not curl is nobody'"'"'s business\n'
|
||||
|
||||
# ROUND TEN. The --config check above was first written INSIDE the API-shape
|
||||
# gate, so it was guarded by a condition that the capability it guards against
|
||||
# removes. A config file can carry the URL; delete the URL from the command and
|
||||
# nothing is API-shaped, the branch is never entered, and the guard reports
|
||||
# clean on precisely the call it exists to refuse. It is now asked of any curl.
|
||||
printf '2\t{"tool_input":{"command":"curl --config /tmp/provider-write.cfg"}}\ta config file can own the URL, so there is nothing API-shaped left to gate on\t--config/-K\n'
|
||||
printf '2\t{"tool_input":{"command":"curl -K/tmp/provider-write.cfg"}}\tcurl accepts the value attached to the short flag\t--config/-K\n'
|
||||
printf '2\t{"tool_input":{"command":"curl -sK /tmp/provider-write.cfg"}}\tand inside a bundle, which a space-separated test does not see\t--config/-K\n'
|
||||
printf '0\t{"tool_input":{"command":"tar -K /tmp/archive.tar"}}\t-K on a command that is not curl is not this hook'"'"'s business\n'
|
||||
|
||||
# Percent-encoded endpoints. Not hypothetical: /issues/1174 and /iss%%75es/1174
|
||||
# both returned HTTP 200 with the same object from the live forge, so the
|
||||
# encoded spelling IS the wrapped endpoint and the literal comparison below it
|
||||
# sees a segment matching nothing. Refused rather than decoded — a decoder has
|
||||
# to be exactly right about depth and normalization, which is the parser
|
||||
# mistake this file declines everywhere else.
|
||||
printf '2\t{"tool_input":{"command":"gh api -X POST repos/a/b/iss%%75es/1/comments -f body=x"}}\tan encoded path segment reaches the wrapped endpoint\tpercent-escape\n'
|
||||
printf '2\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/revi%%65ws"}}\tsame for the review endpoint, which is the one that matters most\tpercent-escape\n'
|
||||
printf '2\t{"tool_input":{"command":"gh api -X POST repos/a/b/iss%%2575es/1/comments -f body=x"}}\tdouble-encoded too, which is why this refuses instead of decoding\tpercent-escape\n'
|
||||
# Scoped to writes, deliberately. Reads are never blocked by this guard and a
|
||||
# query string carrying %%20 is an ordinary URL, not a hazard.
|
||||
printf '0\t{"tool_input":{"command":"curl -s https://git.example.invalid/api/v1/repos/a/b/issues?q=a%%20b"}}\ta percent-escape in a READ is not this hook'"'"'s business\n'
|
||||
} > "$FIXTURES"
|
||||
|
||||
fail=0 n=0
|
||||
@@ -358,6 +380,62 @@ home_case 'HOME unset does not block an ordinary command' \
|
||||
home_case 'HOME unset does not stop the API arms doing their job' \
|
||||
2 '@unset' 'curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews' 'pr-review.sh'
|
||||
|
||||
# ---- the guard standing on its own -----------------------------------------
|
||||
# Every case above runs the guard from the directory holding its siblings, so
|
||||
# `[ -x "$W/pr-review.sh" ]` succeeds and the $HOME fallback beside it never
|
||||
# evaluates. That is a property of the HARNESS, not of the guard, and it hid a
|
||||
# live fail-open: with the guard copied somewhere alone AND no HOME, the
|
||||
# fallback expanded an unset variable under `set -u` and the script died at
|
||||
# rc=1 — on EVERY arm, before any adjudication. A PreToolUse hook exiting
|
||||
# nonzero-but-not-2 is a non-blocking error, so that seat ran with no guard and
|
||||
# nothing reported it.
|
||||
#
|
||||
# The first remediation moved that expansion four lines earlier and called it
|
||||
# closed. It was not closed, because the test could not reach it. So the guard
|
||||
# is copied ALONE here — no siblings, no installed mosaic home — which is the
|
||||
# deployment this file already claims to support ("still works from a repo
|
||||
# checkout with no installed mosaic home").
|
||||
LONE="$TMP/lone"; mkdir -p "$LONE"
|
||||
cp "$GUARD" "$LONE/wrapper-guard.sh"; chmod +x "$LONE/wrapper-guard.sh"
|
||||
|
||||
lone_case() {
|
||||
local why="$1" want="$2" homeval="$3" cmd="$4" needle="${5:-}"
|
||||
local out got
|
||||
n=$((n + 1))
|
||||
if [ "$homeval" = "@unset" ]; then
|
||||
out="$(printf '%s' "{\"tool_input\":{\"command\":\"$cmd\"}}" | env -u HOME "$LONE/wrapper-guard.sh" 2>&1)"
|
||||
else
|
||||
out="$(printf '%s' "{\"tool_input\":{\"command\":\"$cmd\"}}" | env HOME="$homeval" "$LONE/wrapper-guard.sh" 2>&1)"
|
||||
fi
|
||||
got=$?
|
||||
if [ "$got" != "$want" ]; then
|
||||
printf 'FAIL %s [standalone] (want exit %s, got %s)\n' "$why" "$want" "$got"
|
||||
fail=1
|
||||
return
|
||||
fi
|
||||
if [ -n "$needle" ] && ! printf '%s' "$out" | grep -Fq -- "$needle"; then
|
||||
printf 'FAIL %s [standalone] (exit %s, but the message does not say %s)\n' "$why" "$got" "$needle"
|
||||
fail=1
|
||||
return
|
||||
fi
|
||||
printf 'ok %s [standalone]\n' "$why"
|
||||
}
|
||||
|
||||
lone_case 'no siblings and no HOME: an ordinary command still passes, not rc=1' \
|
||||
0 '@unset' 'ls -la /src'
|
||||
lone_case 'no siblings and no HOME: a wrapped write is still refused' \
|
||||
2 '@unset' 'curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/pulls/1/reviews' 'pr-review.sh'
|
||||
lone_case 'no siblings and no HOME: a checkout is refused, not adjudicated' \
|
||||
2 '@unset' 'git clone https://example.invalid/x /src/wt' 'unset or unusable'
|
||||
# Spelled without quotes on purpose. The payload is interpolated into a JSON
|
||||
# string by the helper, so a fixture carrying bare double quotes produces
|
||||
# malformed JSON, jq returns empty, and the guard exits 0 on an empty command —
|
||||
# a PASS that measures nothing. That is what the first version of this case did.
|
||||
lone_case 'no siblings and no HOME: the APPROVE trap still fires' \
|
||||
2 '@unset' 'gh api -X POST repos/a/b/pulls/1/reviews -f event=APPROVE'
|
||||
lone_case 'no siblings, usable HOME: ordinary commands unaffected' \
|
||||
0 '/home/tester' 'ls -la /src'
|
||||
|
||||
printf '\n'
|
||||
if [ "$fail" -eq 0 ]; then
|
||||
printf 'wrapper-guard: %d/%d fixtures behaved as specified.\n' "$n" "$n"
|
||||
|
||||
@@ -100,8 +100,26 @@ override_prefixed && exit 0
|
||||
# still works from a repo checkout with no installed mosaic home (which is how it
|
||||
# is exercised in CI). $HOME remains the fallback for a guard invoked by an
|
||||
# absolute path from somewhere unusual.
|
||||
# $HOME is resolved HERE, once, before anything expands it. The previous attempt
|
||||
# fixed the checkout arm's use of $HOME and left this one, four lines earlier,
|
||||
# reading it raw — so under `set -u` a seat with no HOME still died before
|
||||
# reaching the adjudication that was supposed to handle exactly that. Moving a
|
||||
# fail-open earlier in the file is not closing it. There is now exactly one
|
||||
# expansion of HOME in this script and it is guarded; every later use reads
|
||||
# HOME_DIR / home_known instead, so a new use cannot reintroduce the abort
|
||||
# without going through this block.
|
||||
#
|
||||
# Empty and unset are different values and neither one is a home directory. '/'
|
||||
# is rejected for the same reason as '': every path is under it, so comparing
|
||||
# against it stops discriminating at all.
|
||||
HOME_DIR=""
|
||||
home_known=0
|
||||
case "${HOME-}" in
|
||||
/?*) HOME_DIR="$HOME"; home_known=1 ;;
|
||||
esac
|
||||
|
||||
W="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
|
||||
[ -x "$W/pr-review.sh" ] || W="$HOME/.config/mosaic/tools/git"
|
||||
[ -x "$W/pr-review.sh" ] || W="$HOME_DIR/.config/mosaic/tools/git"
|
||||
|
||||
# ---- 1. checkout into $HOME ------------------------------------------------
|
||||
# $HOME has to be RESOLVED before anything can be compared against it, and the
|
||||
@@ -125,11 +143,9 @@ W="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
|
||||
# was unable to ask. The blast radius of that fail-closed arm is exactly one
|
||||
# command shape (clone / worktree add), not the session.
|
||||
home_re='~|\$HOME'
|
||||
home_known=0
|
||||
case "${HOME-}" in
|
||||
/?*) home_re="$home_re|$(printf '%s' "$HOME" | sed 's/[][\.*^$+?(){}|]/\\&/g')"
|
||||
home_known=1 ;;
|
||||
esac
|
||||
if [ "$home_known" -eq 1 ]; then
|
||||
home_re="$home_re|$(printf '%s' "$HOME_DIR" | sed 's/[][\.*^$+?(){}|]/\\&/g')"
|
||||
fi
|
||||
|
||||
if printf '%s' "$CMD" | grep -Eq 'git[^|;&]*(clone|worktree[[:space:]]+add)'; then
|
||||
if [ "$home_known" -eq 0 ]; then
|
||||
@@ -254,30 +270,35 @@ fi
|
||||
# `gh pr merge`) is NOT covered — catching that means modelling every CLI's verb
|
||||
# grammar, which is the parser mistake again in a new costume. Porcelain is a
|
||||
# gate-7 gap for prose and review to hold, not this hook.
|
||||
API_SHAPED='https?://|/api/v[0-9]|/repos/'
|
||||
API_SHAPED="$API_SHAPED"'|(^|[[:space:]|;&(])(gh|tea|glab|hub)[[:space:]]+api([[:space:]]|$)'
|
||||
if printf '%s' "$CMD" | grep -Eq "$API_SHAPED"; then
|
||||
|
||||
# curl takes its options from a file with -K/--config, and that file may carry
|
||||
# the method, the body, the headers and the URL itself. Every write test below
|
||||
# reads the command TEXT, and none of those things are in it:
|
||||
#
|
||||
# curl --config /tmp/req .../repos/a/b/pulls/1/reviews
|
||||
#
|
||||
# carries no -X, no -d, no -f, so is_write stayed 0 and the call went straight
|
||||
# through. This is not a spelling the write detection was missing — it is the
|
||||
# write question being unaskable, which is the same situation as an endpoint
|
||||
# assembled from expansions, and it gets the same answer. Refusing a READ that
|
||||
# happens to use --config is the acceptable side of that trade, and the message
|
||||
# says how to proceed.
|
||||
if printf '%s' "$CMD" | grep -Eq -- '(^|[[:space:]])(-K|--config)([[:space:]=]|$)'; then
|
||||
# ---- curl whose request lives in a file ------------------------------------
|
||||
# curl takes its options from a file with -K/--config, and that file may carry
|
||||
# the method, the body, the headers AND THE URL. That last one is why this test
|
||||
# cannot live inside the API-shape gate below, which is where it was first put:
|
||||
#
|
||||
# curl --config /tmp/provider-write.cfg
|
||||
#
|
||||
# has no URL, no /repos/, no `gh api` — nothing API-shaped in the text at all —
|
||||
# so it never entered the branch that was supposed to refuse it, and the guard
|
||||
# reported clean on the exact capability the check exists to deny. The check was
|
||||
# guarded by a condition the thing it guards against defeats. It is now asked of
|
||||
# any curl, because "is this a provider call" is not answerable about a command
|
||||
# whose URL is in a file, and a question that cannot be asked is not a question
|
||||
# that came back clean.
|
||||
#
|
||||
# The spelling is deliberately loose. curl accepts the value attached to the
|
||||
# short flag (`-K/tmp/req`, verified) and inside a bundle (`-sK /tmp/req`), and a
|
||||
# guard that recognizes only the space- and equals-separated forms is defeated by
|
||||
# deleting one character. Scoped to curl so that `eslint --config .eslintrc.json`
|
||||
# and every other tool with a --config flag are untouched.
|
||||
if printf '%s' "$CMD" | grep -Eq '(^|[[:space:]|;&(])curl([[:space:]]|$)' \
|
||||
&& printf '%s' "$CMD" | grep -Eq -- '(^|[[:space:]])(-[A-Za-z]*K([[:space:]=]|$|[^[:space:]])|--config([[:space:]=]|$))'; then
|
||||
cat <<EOF
|
||||
BLOCKED: provider API call whose options are supplied from a --config/-K file.
|
||||
BLOCKED: curl invocation whose request is supplied from a --config/-K file.
|
||||
|
||||
curl reads the request method, body, headers and even the URL from that file.
|
||||
None of them appear in the command, so this guard cannot tell whether the call
|
||||
is a read or a write, or what endpoint it reaches. An unreadable request is not
|
||||
a cleared one.
|
||||
is a read or a write, what endpoint it reaches, or whether it is a provider call
|
||||
at all. An unreadable request is not a cleared one.
|
||||
|
||||
$W/ <- the wrappers; use the one for the endpoint you are calling
|
||||
|
||||
@@ -287,7 +308,11 @@ can express, prefix MOSAIC_WRAPPER_OVERRIDE=1 (as the first thing in the
|
||||
command — it is read positionally, not matched as text).
|
||||
EOF
|
||||
exit 2
|
||||
fi
|
||||
fi
|
||||
|
||||
API_SHAPED='https?://|/api/v[0-9]|/repos/'
|
||||
API_SHAPED="$API_SHAPED"'|(^|[[:space:]|;&(])(gh|tea|glab|hub)[[:space:]]+api([[:space:]]|$)'
|
||||
if printf '%s' "$CMD" | grep -Eq "$API_SHAPED"; then
|
||||
|
||||
# Write detection, now client-agnostic. Every spelling curl accepts, because
|
||||
# the guard is defeated by the one spelling it does not know: `-d@body` (no
|
||||
@@ -325,6 +350,42 @@ EOF
|
||||
'\.(post|put|patch|delete)\(' && is_write=1
|
||||
|
||||
if [ "$is_write" -eq 1 ]; then
|
||||
# A percent-escape makes the endpoint unreadable HERE and perfectly readable
|
||||
# to the PROVIDER, which is the whole hazard. Measured against the live
|
||||
# forge, read-only: GET /repos/mosaicstack/stack/issues/1174 and
|
||||
# GET /repos/mosaicstack/stack/iss%75es/1174 both returned HTTP 200 with the
|
||||
# same object. So `iss%75es` IS the wrapped endpoint by the only authority
|
||||
# that gets a vote, while every literal comparison below sees a segment that
|
||||
# matches nothing and clears the write.
|
||||
#
|
||||
# This refuses rather than decodes. A decoder has to be exactly right about
|
||||
# depth (%2569 -> %69 -> i) and about which characters the provider
|
||||
# normalizes, and being exactly right about someone else's parser is the
|
||||
# mistake this file declines everywhere else. Refusing is correct at every
|
||||
# depth at once.
|
||||
#
|
||||
# Scoped to WRITES. Reads are never blocked by this guard, and a query
|
||||
# string carrying %20 is not a hazard — it is an ordinary URL. Putting this
|
||||
# test on the whole API-shaped branch would have refused those too, which is
|
||||
# how a guard earns being routed around.
|
||||
if printf '%s' "$CMD" | grep -Eq '%[0-9A-Fa-f][0-9A-Fa-f]'; then
|
||||
cat <<EOF
|
||||
BLOCKED: provider API WRITE containing a percent-escape.
|
||||
|
||||
The provider decodes the path before routing; this guard compares it literally.
|
||||
An encoded segment therefore reaches a wrapped endpoint while reading, here, as
|
||||
a segment that matches nothing — /iss%75es/ and /issues/ were measured returning
|
||||
the same object from the same repository.
|
||||
|
||||
$W/ <- the wrappers; use the one for the endpoint you are calling
|
||||
|
||||
Spell the path literally and re-run. If the escape is genuinely required and no
|
||||
wrapper can express the call, prefix MOSAIC_WRAPPER_OVERRIDE=1 (as the first
|
||||
thing in the command — it is read positionally, not matched as text).
|
||||
EOF
|
||||
exit 2
|
||||
fi
|
||||
|
||||
# The endpoint map, and the rule that keeps it honest: an arm exists here
|
||||
# ONLY because a wrapper in this directory owns that call. It is an
|
||||
# inventory, not a model — read off `ls tools/git/*.sh` and the flags each
|
||||
|
||||
Reference in New Issue
Block a user