guard: judge command position on the code, not on the text
ci/woodpecker/pr/ci Pipeline was canceled
ci/woodpecker/pr/ci Pipeline was canceled
The position test added an hour ago blocked its own author. The message being sent quoted one of the fixtures, so the quoted text contained an operator followed by a client, and an operator inside a string is not an operator. That is the reported over-blocking defect one level in, and it landed within an hour of shipping the fix for the reported one — which is the argument for pinning both directions as fixtures rather than reasoning about them. Position is now judged against a SKELETON: the command with its data spans (quoted strings, heredoc bodies) removed. Endpoint, URL and body detection keep running against the full text, because real calls quote their URLs and a skeleton would be blind to them. The exception is what makes quotes data in the first place. If something is about to EXECUTE the quoted text — `bash -c`, `sh <<EOF`, `eval` — the quotes hold code, and the skeleton keeps them as command separators so the client inside is still at command position, one interpreter down. Three fixtures: an operator inside a quoted string, a heredoc body, and `bash -c` making the same text code again. 30/30.
This commit is contained in:
@@ -67,6 +67,13 @@ FIXTURES="$TMP/fixtures.tsv"
|
||||
printf '0\t{"tool_input":{"command":"python3 -c '"'"'print(\\"curl -d https://git.example.invalid/api/v1/repos/a/b/issues\\")'"'"'"}}\tprinting an example is not calling it\n'
|
||||
# Command position must still catch the real thing behind operators and env.
|
||||
printf '2\t{"tool_input":{"command":"cd /tmp && GITEA_TOKEN=$T curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge"}}\ta real call behind && and an assignment is still a call\n'
|
||||
# --- and the case the AUTHOR hit, one level in from the reported one: an
|
||||
# operator INSIDE a quoted string is not an operator. This blocked a message
|
||||
# that merely quoted the fixture above. Position is judged on the skeleton.
|
||||
printf '0\t{"tool_input":{"command":"send.sh -m \\"repro was: cd /tmp && curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge\\""}}\tan operator inside a quoted string is not an operator\n'
|
||||
printf '0\t{"tool_input":{"command":"cat >> notes.md <<EOF\\nwe ran: curl -d@b https://git.example.invalid/api/v1/repos/a/b/issues\\nEOF"}}\ta heredoc body is data, not code\n'
|
||||
# ...but quotes stop being data the moment something executes them.
|
||||
printf '2\t{"tool_input":{"command":"bash -c \\"curl -d@b https://git.example.invalid/api/v1/repos/a/b/pulls/1/merge\\""}}\tbash -c makes the quoted text code again\n'
|
||||
} > "$FIXTURES"
|
||||
|
||||
fail=0 n=0
|
||||
|
||||
@@ -85,23 +85,46 @@ fi
|
||||
# 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.
|
||||
# The client must be at COMMAND POSITION — start of the command, or directly
|
||||
# after a shell operator, optionally behind VAR=value assignments. Substring
|
||||
# presence is not enough, and this is the second thing review caught: with a
|
||||
# bare substring test,
|
||||
# The client must be at COMMAND POSITION, and that has to be judged against the
|
||||
# CODE in the command, not against its text. Review caught the text version
|
||||
# blocking ordinary work:
|
||||
# grep -R "curl -d https://host/api/v1/repos/a/b/issues" docs/
|
||||
# echo "curl -d https://host/api/v1/repos/a/b/pulls" > note.txt
|
||||
# were both BLOCKED. Talking about a call is not making one. Over-blocking is
|
||||
# not the safe direction here — a guard that blocks ordinary work gets switched
|
||||
# off, and a guard that is off permits everything.
|
||||
# Talking about a call is not making one, and over-blocking is not the safe
|
||||
# direction: a guard that blocks ordinary work gets switched off, and a guard
|
||||
# that is off permits everything.
|
||||
#
|
||||
# Quoting is what separates the two: in every false positive the client sits
|
||||
# immediately after a quote character, never after an operator. Note the
|
||||
# deliberate absence of quote-stripping: the URL is frequently quoted in REAL
|
||||
# calls, so stripping quotes before matching would blind the endpoint check.
|
||||
# A first fix required the client to follow a shell operator. That lasted until
|
||||
# the author sent a message quoting one of these fixtures — the quoted text
|
||||
# contained `... && GITEA_TOKEN=$T curl -d@b .../merge`, so an operator appeared
|
||||
# INSIDE the quotes and the guard blocked the message. Same defect, one level
|
||||
# in: an operator inside a string is not an operator.
|
||||
#
|
||||
# So the position test runs against a SKELETON — the command with its data spans
|
||||
# (quoted strings, heredoc bodies) removed. Endpoint, URL and body detection all
|
||||
# still run against the FULL text, because real calls quote their URLs and a
|
||||
# skeleton would be blind to them.
|
||||
#
|
||||
# The exception is the reason quotes are data at all: if something is about to
|
||||
# EXECUTE the quoted text (`bash -c`, `sh <<EOF`, `eval`), then the quotes hold
|
||||
# code and the skeleton is the full text again.
|
||||
if printf '%s' "$CMD" | grep -Eq '(^|[[:space:]])((ba|z)?sh[[:space:]]+-c|(ba|z)?sh[[:space:]]*<<|eval[[:space:]])'; then
|
||||
# Quotes become command separators rather than disappearing: in `bash -c
|
||||
# "curl ..."` the client IS at command position, just one interpreter down.
|
||||
SKEL="$(printf '%s' "$CMD" | tr "\"'" ';;')"
|
||||
else
|
||||
# Heredoc bodies first (line-oriented), then quoted spans (span-oriented).
|
||||
SKEL="$(printf '%s' "$CMD" | awk '
|
||||
{ if (hd != "") { if ($0 == hd) hd=""; next }
|
||||
if (match($0, /<<-?[[:space:]]*'"'"'?"?[A-Za-z_][A-Za-z0-9_]*/)) {
|
||||
t = substr($0, RSTART, RLENGTH); sub(/^<<-?[[:space:]]*['"'"'"]?/, "", t); hd = t
|
||||
}
|
||||
print }' | sed "s/'[^']*'//g; s/\"[^\"]*\"//g")"
|
||||
fi
|
||||
|
||||
CLIENT_AT_CMD_POS='(^|[;&|(){}]|`|\$\()[[:space:]]*([A-Za-z_][A-Za-z0-9_]*=[^[:space:]]*[[:space:]]+)*(curl|wget|httpie|http)([[:space:]]|$)'
|
||||
|
||||
if printf '%s' "$CMD" | grep -Eq "$CLIENT_AT_CMD_POS" \
|
||||
if printf '%s' "$SKEL" | grep -Eq "$CLIENT_AT_CMD_POS" \
|
||||
&& printf '%s' "$CMD" | grep -Eq 'https?://'; then
|
||||
|
||||
# Write detection. Every spelling curl accepts, because the guard is defeated
|
||||
|
||||
Reference in New Issue
Block a user