wrapper-guard: fail closed on an unreadable endpoint in every shape the scope gate admits
ci/woodpecker/pr/ci Pipeline was successful
ci/woodpecker/pr/ci Pipeline was successful
Round-9 review found the fail-closed rule narrower than the block it guards.
The scope gate admits three shapes — a scheme URL, a schemeless /api/vN path,
and a provider CLI's `api` subcommand — but the unreadable-endpoint test asked
only for https?://. So a split endpoint token in the other two shapes was in
scope to be blocked, produced no readable endpoint, and fell through to ALLOW,
while the identical split behind a literal scheme blocked.
Four writes reaching the provider unexamined, one of them a review verdict:
p=repos/a/b/iss; q=ues; gh api -X POST ${p}${q} -f title=x
p=repos/a/b/issues/1/comm; q=ents; gh api -X POST ${p}${q} -f body=x
p=repos/a/b/pulls/1/rev; q=iews; gh api -X POST ${p}${q} -f event=APPROVED
p=/api/v1/repos/a/b/iss; q=ues; curl -X POST -d x host${p}${q}
This is the same defect class as the milestone arm one round earlier, one layer
up: there the map claimed a span its wrapper did not cover, here a control
claimed a surface it did not measure. A control is only as wide as its narrowest
arm, and widening the scope gate without widening the fail-closed rule left the
gap exactly where the gate had just been extended.
Three arms now, one per admitted shape: a scheme URL token carrying an
expansion; a schemeless token carrying both a forge fragment and an expansion,
in either order; and the endpoint argument of a provider-CLI api call, read
positionally. Limits are stated in the source rather than implied — a caller who
splits the hostname as well, and an endpoint pushed past an option whose value
contains whitespace, are both outside what this measures.
An expansion in a BODY is explicitly not unreadable. Passing a payload in a
variable is the safe practice and leaves the endpoint fully legible; blocking it
would have been a control punishing the behaviour it wants.
101/101 fixtures, up from 92. Each arm is negative-controlled separately:
removing the schemeless arm fails exactly the two schemeless fixtures, removing
the provider-CLI arm fails exactly the four CLI fixtures, and neither disturbs
any pre-existing fixture. One added fixture was rewritten after it passed for
the wrong reason — its endpoint was readable, so it blocked on the endpoint map
and never exercised the arm it was written for.
Evidence: 101/101 locally and in ci-base; shellcheck clean at warning+; a
12-command sweep of ordinary forge work — reads with split endpoints, bodies in
variables, unwrapped endpoints, an artifact PUT — blocks none of them; the
18-command sweep still blocks the same three round-six flips and nothing new.
This commit is contained in:
Hermes Agent
@@ -180,6 +180,23 @@ FIXTURES="$TMP/fixtures.tsv"
|
||||
# Requesting a reviewer is not submitting one; pr-review.sh files verdicts and
|
||||
# nothing in the tree adds a requested reviewer.
|
||||
printf '0\t{"tool_input":{"command":"gh api -X POST repos/a/b/pulls/1/requested_reviewers -f reviewers[]=u"}}\tno wrapper requests a reviewer, so it is not refused with pr-review.sh\n'
|
||||
# SPAN, applied to the guard's OWN fail-closed rule rather than to a wrapper.
|
||||
# The scope gate admits three shapes; the unreadable-endpoint rule asked only
|
||||
# for `https?://`, so a split endpoint in the other two was in scope to block,
|
||||
# produced no readable endpoint, and fell through to allow. Same defect class
|
||||
# as the milestone arm, one layer up. Each shape gets its own fixture, because
|
||||
# a single one would have passed on the arm that already worked.
|
||||
printf '2\t{"tool_input":{"command":"p=repos/a/b/iss; q=ues; gh api -X POST ${p}${q} -f title=x"}}\ta split endpoint in a provider-CLI api call is unreadable, not absent\n'
|
||||
printf '2\t{"tool_input":{"command":"p=repos/a/b/issues/1/comm; q=ents; gh api -X POST ${p}${q} -f body=x"}}\tsame, comments\n'
|
||||
printf '2\t{"tool_input":{"command":"p=repos/a/b/pulls/1/rev; q=iews; gh api -X POST ${p}${q} -f event=APPROVED"}}\tsame, and a verdict is the costliest one to lose\n'
|
||||
printf '2\t{"tool_input":{"command":"p=repos/a/b/iss; q=ues; tea api -X POST ${p}${q} -f title=x"}}\tevery CLI the scope gate admits, not just gh\n'
|
||||
printf '2\t{"tool_input":{"command":"p=/api/v1/repos/a/b/iss; q=ues; curl -X POST -d x git.example.invalid${p}${q}"}}\ta schemeless forge host with a split path is unreadable too\n'
|
||||
printf '2\t{"tool_input":{"command":"h=git.example.invalid; q=ues; curl -X POST -d x ${h}/api/v1/repos/a/b/iss${q}"}}\tthe expansion may come first; the token is what matters\n'
|
||||
# And the reason this is not "any variable blocks a write": a payload in a
|
||||
# variable is the SAFE way to pass one and leaves the endpoint fully legible.
|
||||
printf '0\t{"tool_input":{"command":"gh api repos/a/b/git/refs -f sha=$SHA"}}\tan expansion in a body value leaves the endpoint readable\n'
|
||||
printf '0\t{"tool_input":{"command":"curl -X POST -d \\"$BODY\\" https://git.example.invalid/api/v1/repos/a/b/git/refs"}}\tsame for a quoted body on an unwrapped endpoint\n'
|
||||
printf '0\t{"tool_input":{"command":"gh api repos/${OWNER}/${REPO}/git/refs"}}\ta read with a split endpoint is still a read\n'
|
||||
printf '0\t{"tool_input":{"command":"curl -X PATCH -d @b https://git.example.invalid/api/v1/repos/a/b/issues/comments/5"}}\tediting a comment has no wrapper; only creating one does\n'
|
||||
printf '0\t{"tool_input":{"command":"curl -X POST -d @b https://git.example.invalid/api/v1/repos/a/b/issues/1/stopwatch/start"}}\tno wrapper owns a stopwatch, and none is invented for it\n'
|
||||
printf '0\t{"tool_input":{"command":"gh api -X POST repos/a/b/issues/1/times -f time=60"}}\tnor time tracking\n'
|
||||
|
||||
@@ -308,7 +308,7 @@ edit — that one is a real wrapper gap, and the override exists for it." ;;
|
||||
fi ;;
|
||||
esac
|
||||
|
||||
# A URL the guard cannot READ is a URL the guard must not CLEAR.
|
||||
# An endpoint the guard cannot READ is an endpoint the guard must not CLEAR.
|
||||
#
|
||||
# Round one fixed one spelling of this and review immediately produced the
|
||||
# general form: split the endpoint token itself across two variables —
|
||||
@@ -318,25 +318,58 @@ edit — that one is a real wrapper gap, and the override exists for it." ;;
|
||||
# fragments is unwinnable: the endpoint does not exist until the shell
|
||||
# expands it, and this hook runs before that.
|
||||
#
|
||||
# So stop pretending to read it. If a write's URL contains an expansion,
|
||||
# So stop pretending to read it. If a write's endpoint contains an expansion,
|
||||
# the guard has no endpoint to judge, and "no endpoint" must not mean
|
||||
# "allowed" — that is the same absence-driven allow as the missing-wrapper
|
||||
# case, wearing different clothes.
|
||||
#
|
||||
# Scoped to commands that are visibly forge-shaped, so an opaque webhook or
|
||||
# artifact POST is untouched. A caller who splits `/api/` and the hostname
|
||||
# as well does get through; that is no longer a mistake anyone makes by
|
||||
# accident, and this hook stops mistakes. It is not a sandbox, and pretending
|
||||
# otherwise is how you get a control nobody can trust the boundaries of.
|
||||
if [ -z "$endpoint" ] \
|
||||
&& printf '%s' "$CMD" | grep -Eq 'https?://[^[:space:]"'"'"'|;&)]*[$`]' \
|
||||
&& printf '%s' "$CMD" | grep -Eq '/api/v[0-9]|/repos/|git\.|gitea|github\.com|gitlab|forgejo'; then
|
||||
cat <<EOF
|
||||
BLOCKED: raw provider API write whose URL this guard cannot read.
|
||||
# SPAN, and the defect review found here: a fail-closed rule must cover the
|
||||
# same surface as the block it guards. This test asked only for `https?://`
|
||||
# while the scope gate above had already been widened to three shapes, so
|
||||
# p=repos/a/b/iss; q=ues; gh api -X POST ${p}${q} -f title=x
|
||||
# p=/api/v1/repos/a/b/iss; q=ues; curl -X POST -d x git.example.invalid${p}${q}
|
||||
# were in scope to be blocked, produced no readable endpoint, and then fell
|
||||
# through to ALLOW — while the identical split behind a literal `https://`
|
||||
# blocked. Same shape as the milestone arm one round earlier: the correct
|
||||
# treatment already existed and was applied to one of the surfaces it
|
||||
# covered. A control is only as wide as its narrowest arm.
|
||||
#
|
||||
# Three arms, one per shape the scope gate admits:
|
||||
# A a scheme-bearing URL token carrying an expansion
|
||||
# B a schemeless token carrying BOTH a forge fragment and an expansion
|
||||
# C the endpoint argument of a provider-CLI `api` call carrying one
|
||||
#
|
||||
# Stated limits, because a control may not claim more than it measures. B
|
||||
# requires the fragment and the expansion in the SAME shell token, so a
|
||||
# caller who splits the hostname and `/api/` as well gets through. C reads
|
||||
# the endpoint positionally — the first bare token after `api` and its option
|
||||
# run — so an endpoint pushed past an option whose value itself contains
|
||||
# whitespace is not seen. Both are deliberate: this hook stops mistakes, it
|
||||
# is not a sandbox, and pretending otherwise is how you get a control nobody
|
||||
# can trust the boundaries of.
|
||||
#
|
||||
# Note what is NOT unreadable: an expansion in a BODY (`-d "$BODY"`,
|
||||
# `-f sha=$SHA`) leaves the endpoint perfectly legible, and blocking it would
|
||||
# punish the safest way to pass a payload. Only the endpoint region counts.
|
||||
URLTOK='[^[:space:]"'"'"'|;&)]*'
|
||||
FORGE='(/api/v[0-9]|/repos/|git\.|gitea|github\.com|gitlab|forgejo)'
|
||||
unreadable=0
|
||||
if printf '%s' "$CMD" | grep -Eq "https?://$URLTOK"'[$`]' \
|
||||
&& printf '%s' "$CMD" | grep -Eq "$FORGE"; then unreadable=1; fi
|
||||
printf '%s' "$CMD" | grep -Eq \
|
||||
"$URLTOK($FORGE$URLTOK"'[$`]'"|"'[$`]'"$URLTOK$FORGE)" && unreadable=1
|
||||
printf '%s' "$CMD" | grep -Eq \
|
||||
'(^|[[:space:]|;&(])(gh|tea|glab|hub)[[:space:]]+api([[:space:]]+--?[A-Za-z][A-Za-z-]*([[:space:]]+[^-[:space:]][^[:space:]]*)?)*[[:space:]]+[^-[:space:]][^[:space:]]*[$`]' \
|
||||
&& unreadable=1
|
||||
|
||||
The URL is assembled from shell expansions, so the endpoint it names does not
|
||||
if [ -z "$endpoint" ] && [ "$unreadable" -eq 1 ]; then
|
||||
cat <<EOF
|
||||
BLOCKED: raw provider API write whose endpoint this guard cannot read.
|
||||
|
||||
The endpoint is assembled from shell expansions, so the path it names does not
|
||||
exist until the shell builds it — after this check runs. The guard cannot tell
|
||||
whether it is a wrapped endpoint, and an unreadable URL is not a cleared one.
|
||||
whether it is a wrapped endpoint, and an unreadable endpoint is not a cleared
|
||||
one.
|
||||
|
||||
$W/ <- the wrappers; use the one for the endpoint you are calling
|
||||
|
||||
@@ -344,8 +377,10 @@ If you are calling a wrapped endpoint (reviews, merges, comments, pulls,
|
||||
issues, milestones), use the wrapper — it also resolves identity explicitly,
|
||||
which matters on a host whose default provider login is an admin account.
|
||||
|
||||
If this is genuinely not a provider endpoint, either write the URL literally so
|
||||
the guard can see what it is, or prefix MOSAIC_WRAPPER_OVERRIDE=1.
|
||||
If this is genuinely not a provider endpoint, either write the endpoint
|
||||
literally so the guard can see what it is, or prefix MOSAIC_WRAPPER_OVERRIDE=1.
|
||||
A variable in the BODY is fine and does not trigger this; only the endpoint
|
||||
itself has to be legible.
|
||||
EOF
|
||||
exit 2
|
||||
fi
|
||||
|
||||
Reference in New Issue
Block a user