fix(git-wrappers): #865 round-7 addendum — conservative YAML recognizer + review/comment hardening
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
Fold the remaining round-7 audit items into the tea-CLI comment-invocation fix. Fallback token parser (detect-platform.sh, test-gitea-login-resolution.sh): Replace the line-by-line scalar fallback with a strict CONSERVATIVE block-YAML recognizer that reconstructs the same object PyYAML would or fails closed the instant it meets anything outside the tea-config subset. Closes 5 structural fail-open classes the old parser missed (nested-shadow logins, block-scalar shadow, duplicate root key / login name / token field, malformed-after-valid, extra-document / end-marker). Validated by a 360k-check differential fuzz vs real PyYAML (0 fail-open) plus explicit forced-PyYAML-absence fixtures. ITEM 1 (pr-review.sh) current-head TOCTOU: after the exact review-id read-back succeeds, re-read the live PR head and fail closed if it advanced past the submitted commit_id, so a review is never reported as covering a superseded tip. ITEM 2 (pr-review.sh comment action): require the returned resource be a pull_request (populated pull_request_url); reject a bare issue_url so a plain issue #N cannot masquerade as a verified PR comment. issue-comment.sh keeps its broader issue-or-PR acceptance. ITEM 3a (both wrappers): move the Authorization bearer OUT of curl argv into a private mode-0600 curl --config file (gitea_write_auth_config), removed on every exit path, so the token never appears in the process table. ITEM 3b (pr-review.sh): bind the review body with presence + string-type + exact equality instead of `(body or "")`, so a non-empty submitted body persisted as null/missing fails closed. Tests: add race, plain-issue, argv-capture (no token printed), and null-body fixtures; broaden temp-leak checks to the auth-config files. Full gate set green (bash -n, shellcheck -x -S warning, prettier, all 3 REST/resolution suites with PyYAML and forced-absent, cold TURBO_FORCE turbo 14/14). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -45,8 +45,13 @@ STATE_DIR="$WORK_DIR/state"
|
||||
REVIEWS_FILE="$STATE_DIR/reviews.json"
|
||||
COMMENTS_FILE="$STATE_DIR/comments.json"
|
||||
SUBMIT_PAYLOAD_FILE="$STATE_DIR/review_payload.json"
|
||||
# Counts GET /pulls/{n} calls within a single run so a race mode can advance the
|
||||
# reported head between the pre-submit read and the post-verify re-read.
|
||||
HEAD_CALLS_FILE="$STATE_DIR/head_calls"
|
||||
TEA_LOG="$WORK_DIR/tea.log"
|
||||
CURL_LOG="$WORK_DIR/curl.log"
|
||||
# Full curl argv per invocation — proves the bearer token never rides in argv.
|
||||
CURL_ARGV_LOG="$WORK_DIR/curl-argv.log"
|
||||
AUTH_LOG="$WORK_DIR/auth.log"
|
||||
OUTPUT_FILE="$WORK_DIR/output.log"
|
||||
CREDENTIALS_FILE="$WORK_DIR/credentials.json"
|
||||
@@ -143,17 +148,24 @@ cat > "$BIN_DIR/curl" <<'SH'
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
|
||||
# Record the FULL argv exactly as spawned, BEFORE any consumption. The bearer
|
||||
# token must NOT appear here — it is delivered via a curl --config file, so only
|
||||
# the config file PATH may show up. (#865 ITEM 3a credential-in-argv exposure.)
|
||||
printf '%s\n' "$*" >> "$PR_REVIEW_CURL_ARGV_LOG"
|
||||
|
||||
output_file=""
|
||||
method="GET"
|
||||
payload=""
|
||||
url=""
|
||||
auth_token=""
|
||||
config_file=""
|
||||
while [[ $# -gt 0 ]]; do
|
||||
case "$1" in
|
||||
-o) output_file="$2"; shift 2 ;;
|
||||
-H)
|
||||
[[ "$2" == Authorization:* ]] && auth_token="${2##* }"
|
||||
shift 2 ;;
|
||||
-K|--config) config_file="$2"; shift 2 ;;
|
||||
-w) shift 2 ;;
|
||||
-X) method="$2"; shift 2 ;;
|
||||
-d|--data) payload="$2"; shift 2 ;;
|
||||
@@ -163,6 +175,17 @@ while [[ $# -gt 0 ]]; do
|
||||
esac
|
||||
done
|
||||
|
||||
# Resolve the bearer token from the curl --config file (its real, secure source);
|
||||
# only fall back to an -H header for defense in depth. The config line is
|
||||
# `header = "Authorization: token <value>"`.
|
||||
if [[ -z "$auth_token" && -n "$config_file" && -f "$config_file" ]]; then
|
||||
config_hdr="$(grep -i 'Authorization' "$config_file" 2>/dev/null || true)"
|
||||
if [[ "$config_hdr" == *"token "* ]]; then
|
||||
auth_token="${config_hdr##*token }"
|
||||
auth_token="${auth_token%\"}"
|
||||
fi
|
||||
fi
|
||||
|
||||
path="${url%%\?*}"
|
||||
query="${url#*\?}"
|
||||
[[ "$query" == "$url" ]] && query=""
|
||||
@@ -207,7 +230,25 @@ elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123
|
||||
write_response 200 "$(PR_REVIEW_HEAD_SHA="$PR_REVIEW_HEAD_SHA" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
print(json.dumps({"head": {"sha": os.environ["PR_REVIEW_HEAD_SHA"]}}))
|
||||
|
||||
head = os.environ["PR_REVIEW_HEAD_SHA"]
|
||||
mode = os.environ.get("PR_REVIEW_TEST_MODE", "")
|
||||
calls_path = os.environ.get("PR_REVIEW_HEAD_CALLS", "")
|
||||
# Count GET /pulls/{n} calls within this run: call 1 is the pre-submit head read
|
||||
# that pins the review; call 2+ is the post-verify re-read (current-head TOCTOU
|
||||
# close-out). In the race mode the branch "advances" after the pin.
|
||||
n = 1
|
||||
if calls_path:
|
||||
try:
|
||||
with open(calls_path, encoding="utf-8") as handle:
|
||||
n = int(handle.read() or "0") + 1
|
||||
except (OSError, ValueError):
|
||||
n = 1
|
||||
with open(calls_path, "w", encoding="utf-8") as handle:
|
||||
handle.write(str(n))
|
||||
if mode == "head-advanced-race" and n >= 2:
|
||||
head = "HEADSHA_ADVANCED_DEADBEEF"
|
||||
print(json.dumps({"head": {"sha": head}}))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
@@ -254,6 +295,26 @@ if mode == "review-body-reuse":
|
||||
print(json.dumps(record))
|
||||
raise SystemExit(0)
|
||||
|
||||
# review-body-null (#865 ITEM 3b): a non-empty body was submitted but the
|
||||
# persisted review carries body == null. id/author/state/head all line up; only
|
||||
# strict presence + string-type body verification catches the lost body. The old
|
||||
# `(body or "")` coalesce would have treated null as an empty string and passed.
|
||||
if mode == "review-body-null":
|
||||
new_id = (max((r["id"] for r in reviews), default=0)) + 1
|
||||
record = {
|
||||
"id": new_id,
|
||||
"state": submitted.get("event"),
|
||||
"commit_id": submitted.get("commit_id"),
|
||||
"body": None,
|
||||
"user": {"login": acting},
|
||||
}
|
||||
reviews.append(record)
|
||||
with open(state_path, "w", encoding="utf-8") as handle:
|
||||
json.dump(reviews, handle)
|
||||
print("201")
|
||||
print(json.dumps(record))
|
||||
raise SystemExit(0)
|
||||
|
||||
author = foreign if mode == "author-mismatch-review" else acting
|
||||
new_id = (max((r["id"] for r in reviews), default=0)) + 1
|
||||
record = {
|
||||
@@ -313,6 +374,23 @@ body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body")
|
||||
# issue_url is left empty. This is what the wrapper must tolerate — it must NOT
|
||||
# require an API-shaped issue_url.
|
||||
pr_url = f"{web_base}/pulls/123"
|
||||
# comment-plain-issue (#865 ITEM 2): #123 is a plain ISSUE, not a PR. POST
|
||||
# /issues/123/comments lands an issue comment whose issue_url is set and
|
||||
# pull_request_url is empty. The pr-review `comment` action MUST reject this — it
|
||||
# claimed a PR comment, so a bare issue_url is not acceptable proof.
|
||||
if mode == "comment-plain-issue":
|
||||
record = {
|
||||
"id": 456,
|
||||
"body": body,
|
||||
"user": {"login": acting},
|
||||
"issue_url": f"{web_base}/issues/123",
|
||||
"pull_request_url": "",
|
||||
}
|
||||
with open(state_path, "w", encoding="utf-8") as handle:
|
||||
json.dump([record], handle)
|
||||
print("201")
|
||||
print(json.dumps(record))
|
||||
raise SystemExit(0)
|
||||
# URL-injection modes (#865 Blocker 3): id/author/body are all correct but the
|
||||
# provider-returned pull_request_url is forged, so ONLY origin+full-path
|
||||
# verification can catch them.
|
||||
@@ -377,7 +455,7 @@ chmod +x "$BIN_DIR/curl"
|
||||
seed_state() {
|
||||
local mode="$1"
|
||||
printf '[]' > "$COMMENTS_FILE"
|
||||
rm -f "$SUBMIT_PAYLOAD_FILE"
|
||||
rm -f "$SUBMIT_PAYLOAD_FILE" "$HEAD_CALLS_FILE"
|
||||
PR_REVIEW_SEED_MODE="$mode" PR_REVIEW_SEED_ACTING="$ACTING_LOGIN" \
|
||||
PR_REVIEW_SEED_HEAD="$HEAD_SHA" python3 - "$REVIEWS_FILE" <<'PY'
|
||||
import json
|
||||
@@ -424,6 +502,7 @@ run_review() {
|
||||
write_credentials "$configured_url"
|
||||
: > "$TEA_LOG"
|
||||
: > "$CURL_LOG"
|
||||
: > "$CURL_ARGV_LOG"
|
||||
: > "$AUTH_LOG"
|
||||
: > "$OUTPUT_FILE"
|
||||
seed_state "$mode"
|
||||
@@ -436,10 +515,12 @@ run_review() {
|
||||
PR_REVIEW_TEA_LOG="$TEA_LOG" \
|
||||
PR_REVIEW_LOGIN_URL="${configured_url%/}" \
|
||||
PR_REVIEW_CURL_LOG="$CURL_LOG" \
|
||||
PR_REVIEW_CURL_ARGV_LOG="$CURL_ARGV_LOG" \
|
||||
PR_REVIEW_AUTH_LOG="$AUTH_LOG" \
|
||||
PR_REVIEW_REVIEWS="$REVIEWS_FILE" \
|
||||
PR_REVIEW_COMMENTS="$COMMENTS_FILE" \
|
||||
PR_REVIEW_SUBMIT_PAYLOAD="$SUBMIT_PAYLOAD_FILE" \
|
||||
PR_REVIEW_HEAD_CALLS="$HEAD_CALLS_FILE" \
|
||||
PR_REVIEW_TEST_MODE="$mode" \
|
||||
PR_REVIEW_EXPECTED_BODY="$comment" \
|
||||
PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \
|
||||
@@ -462,7 +543,9 @@ run_review() {
|
||||
# clobbered/leaked RETURN trap is caught on every exit route.
|
||||
assert_no_temp_leak() {
|
||||
local context="$1" leaked
|
||||
leaked=$(find "$TMP_SCRATCH" -type f -name 'mosaic-pr-review-*' 2>/dev/null || true)
|
||||
# Includes the curl auth-config files (mosaic-gitea-auth-*), which carry the
|
||||
# bearer token and must be unlinked on every exit path.
|
||||
leaked=$(find "$TMP_SCRATCH" -type f \( -name 'mosaic-pr-review-*' -o -name 'mosaic-gitea-auth-*' \) 2>/dev/null || true)
|
||||
if [[ -n "$leaked" ]]; then
|
||||
echo "FAIL: pr-review temp files leaked ($context):" >&2
|
||||
printf '%s\n' "$leaked" >&2
|
||||
@@ -470,6 +553,21 @@ assert_no_temp_leak() {
|
||||
fi
|
||||
}
|
||||
|
||||
# Assert the presented bearer token NEVER appeared in curl's argv (it must travel
|
||||
# via a curl --config file), and that --config auth was actually used. On the
|
||||
# expected path grep matches nothing, so no token value is ever printed.
|
||||
assert_token_not_in_argv() {
|
||||
local context="$1"
|
||||
if grep -qF -e "$DEFAULT_TOKEN" -e "$OVERRIDE_TOKEN" -e "$CROSS_HOST_TOKEN" "$CURL_ARGV_LOG"; then
|
||||
echo "FAIL: a Gitea bearer token leaked into curl argv ($context)" >&2
|
||||
exit 1
|
||||
fi
|
||||
if ! grep -q -- '--config' "$CURL_ARGV_LOG"; then
|
||||
echo "FAIL: curl was not invoked with --config file auth ($context)" >&2
|
||||
exit 1
|
||||
fi
|
||||
}
|
||||
|
||||
assert_no_tea_write() {
|
||||
# tea must only ever be used for the login list, never to write.
|
||||
if grep -qvE '^login list --output json$' "$TEA_LOG"; then
|
||||
@@ -499,6 +597,9 @@ grep -q "^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/
|
||||
grep -q "^GET https://git.mosaicstack.dev/api/v1/user $ACTING_LOGIN\$" "$AUTH_LOG"
|
||||
assert_no_tea_write
|
||||
assert_no_temp_leak "approve"
|
||||
# ITEM 3a: the host-default token drove this whole chain, yet never appeared in
|
||||
# any curl argv — it was passed via a curl --config file.
|
||||
assert_token_not_in_argv "approve default-token"
|
||||
# The submitted review payload carries the event and the PR head commit_id.
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY'
|
||||
import json
|
||||
@@ -673,6 +774,8 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
exit 1
|
||||
fi
|
||||
assert_no_tea_write
|
||||
# ITEM 3a: the override token likewise never leaked into curl argv.
|
||||
assert_token_not_in_argv "override-success override-token"
|
||||
|
||||
# Case 9 (#865 Round-4): an UNRESOLVABLE explicit --login override (a name absent
|
||||
# from the tea config) must FAIL CLOSED — nonzero exit, no success line, no review
|
||||
@@ -765,4 +868,56 @@ for bad_mode in comment-url-wrong-host comment-url-wrong-owner comment-url-wrong
|
||||
assert_no_temp_leak "$bad_mode"
|
||||
done
|
||||
|
||||
# Case 16 (#865 ITEM 1, current-head TOCTOU): the PR head advances between the
|
||||
# pre-submit head read (which pins the review) and the post-verify re-read. The
|
||||
# review is genuinely created and verified as pinned to the OLD head, but the
|
||||
# live tip has moved on, so the wrapper must FAIL CLOSED rather than report a
|
||||
# review that no longer covers the PR's current commit.
|
||||
if run_review head-advanced-race approve; then
|
||||
echo "FAIL: approve reported success though the PR head advanced after submit" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: current-head TOCTOU close-out did not fail closed on an advanced head" >&2
|
||||
exit 1
|
||||
fi
|
||||
# The head was re-read after the submit/verify (2nd GET /pulls/123).
|
||||
if [[ "$(grep -c '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG")" -lt 2 ]]; then
|
||||
echo "FAIL: wrapper did not re-read the PR head after review verification" >&2
|
||||
cat "$CURL_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_temp_leak "head-advanced-race"
|
||||
|
||||
# Case 17 (#865 ITEM 2): a claimed PR comment that actually lands as a plain
|
||||
# ISSUE comment (issue #123 exists, PR #123 does not — issue_url set,
|
||||
# pull_request_url empty) must FAIL CLOSED. The pr-review `comment` verifier
|
||||
# requires a pull_request_url (kind=pulls) and rejects a bare issue_url.
|
||||
if run_review comment-plain-issue comment durable-body; then
|
||||
echo "FAIL: pr-review accepted a plain issue comment as a verified PR comment" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: a plain issue_url satisfied the PR comment verifier" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_temp_leak "comment-plain-issue"
|
||||
|
||||
# Case 18 (#865 ITEM 3b): a non-empty review body submitted but persisted as null
|
||||
# must FAIL CLOSED. id/author/state/head all match; only strict presence +
|
||||
# string-type + exact body equality (not the old `(body or "")` coalesce) catches
|
||||
# the lost body.
|
||||
if run_review review-body-null approve real-submitted-review-body; then
|
||||
echo "FAIL: review whose non-empty body persisted as null was accepted" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: a null persisted body passed strict review-body verification" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_temp_leak "review-body-null"
|
||||
|
||||
echo "pr-review.sh REST review + comment create/read-back regression passed"
|
||||
|
||||
Reference in New Issue
Block a user