fix(git-tools): env-robust token parse, host-bound creds, real Gitea URL verify (#865 round-5)
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
All checks were successful
ci/woodpecker/pr/ci Pipeline was successful
CI-red root cause (classification a: my round-4 change fails in the clean/cold CI env): get_gitea_token_for_login hard-required PyYAML (`import yaml`), which is absent on CI's node:24-alpine (python3 without py3-yaml). Round-4's --login override cases were the first to exercise that path, turning the mosaic package test (test:framework-shell -> test-pr-review-gitea-comment.sh) RED. Fix: add an indentation-aware line-parser fallback that resolves the SAME per-name token PyYAML would from tea's flat `logins:` list; PyYAML stays the fast path. This also repairs a latent production defect (--login overrides were silently unusable on any PyYAML-less host). Auditor blockers folded into the same round-5: 1. issue_url vs pull_request_url shape (correctness): Gitea populates WEB (html) URLs in issue_url/pull_request_url, not API paths, and a PR-conversation comment carries pull_request_url (issue_url empty). Verification now accepts either web shape scoped to the repo slug + number, so a durable write is never rejected for URL shape. Test stubs now emit the REAL Gitea web shapes. 2. Cross-host credential binding (security): get_gitea_token_for_login now takes the repo host and requires the matched login's configured URL host to equal it; an override login configured for a different host FAILS CLOSED instead of sending a cross-host credential. Regression tests added to both suites. 3. Non-exhaustive enumeration (false-fail): removed the redundant, non-exhaustive post-verification list enumeration (gitea_fetch_all + confirm_*_enumerable) from both wrappers; the exact-id GET is authoritative. Pagination cases dropped; a guard asserts no list enumeration is performed. 4. Trap clobbering / temp-file leak (security/hygiene): removing the nested enumeration eliminates the RETURN-trap nesting that clobbered caller cleanup; remaining RETURN traps are single/non-nested and clean up on all exit paths. Temp-file leak regression tests (success + failure paths) added to both suites. 5. README: corrected the exhaustive-pagination claim and documented host-bound --login selection. Preserves every round-2/3/4 fix (explicit --login fail-closed at all write sites, token->identity attribution seam). Gates: cold `pnpm turbo run test --filter=@mosaicstack/mosaic` green (14/14); full test-*.sh suite green with AND without PyYAML; bash -n, shellcheck -x -S warning, prettier --check README clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -26,6 +26,13 @@
|
||||
# CLOSED (unresolvable case) — never silently downgrade to the host-default
|
||||
# identity. The host-default best-effort fallback is reserved for the
|
||||
# no-override default path.
|
||||
#
|
||||
# #865 Round-5: the exact-id read-back is the SOLE authority — the wrapper does
|
||||
# NO follow-up list enumeration (the stub exposes no review/comment list
|
||||
# endpoint, so a residual enumeration would fail the run). A --login override is
|
||||
# host-bound: an override configured for a DIFFERENT host than the repo remote
|
||||
# FAILS CLOSED rather than leaking a cross-host credential. And every run leaves
|
||||
# no scratch temp files behind on any exit path (POST/GET bodies + metadata).
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
@@ -43,6 +50,9 @@ CURL_LOG="$WORK_DIR/curl.log"
|
||||
AUTH_LOG="$WORK_DIR/auth.log"
|
||||
OUTPUT_FILE="$WORK_DIR/output.log"
|
||||
CREDENTIALS_FILE="$WORK_DIR/credentials.json"
|
||||
# A dedicated scratch dir the wrapper is pointed at via TMPDIR, so the leak
|
||||
# check can assert every POST/GET body + metadata temp file is cleaned up.
|
||||
TMP_SCRATCH="$WORK_DIR/scratch"
|
||||
|
||||
cleanup() {
|
||||
rm -rf "$WORK_DIR"
|
||||
@@ -57,17 +67,25 @@ HEAD_SHA="HEADSHA_FEEDFACE"
|
||||
OVERRIDE_LOGIN="primary-reviewer"
|
||||
DEFAULT_TOKEN="test-only-placeholder"
|
||||
OVERRIDE_TOKEN="override-token-placeholder"
|
||||
# A --login override whose tea config URL points at a DIFFERENT Gitea host than
|
||||
# the repo remote (git.mosaicstack.dev). Host-bound selection must reject it
|
||||
# rather than send its token cross-host.
|
||||
CROSS_HOST_LOGIN="foreign-host-reviewer"
|
||||
CROSS_HOST_TOKEN="cross-host-token-placeholder"
|
||||
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR"
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR" "$TMP_SCRATCH"
|
||||
git -C "$REPO_DIR" init -q
|
||||
git -C "$REPO_DIR" remote add origin https://git.mosaicstack.dev/mosaicstack/stack.git
|
||||
|
||||
# tea config: the override login carries its own token here. The default login
|
||||
# name ("mosaicstack") is deliberately absent, so the no-override default path
|
||||
# resolves via the host credential fallback while an explicit --login must
|
||||
# resolve from this file or fail closed.
|
||||
# resolve from this file or fail closed. A second login is configured for a
|
||||
# DIFFERENT host to exercise host-bound rejection.
|
||||
mkdir -p "$XDG_DIR/tea"
|
||||
OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" python3 - "$XDG_DIR/tea/config.yml" <<'PY'
|
||||
OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \
|
||||
CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \
|
||||
python3 - "$XDG_DIR/tea/config.yml" <<'PY'
|
||||
import os
|
||||
import sys
|
||||
|
||||
@@ -76,6 +94,9 @@ with open(sys.argv[1], "w", encoding="utf-8") as handle:
|
||||
handle.write(f" - name: {os.environ['OVERRIDE_LOGIN']}\n")
|
||||
handle.write(" url: https://git.mosaicstack.dev\n")
|
||||
handle.write(f" token: {os.environ['OVERRIDE_TOKEN']}\n")
|
||||
handle.write(f" - name: {os.environ['CROSS_HOST_LOGIN']}\n")
|
||||
handle.write(" url: https://git.uscllc.com\n")
|
||||
handle.write(f" token: {os.environ['CROSS_HOST_TOKEN']}\n")
|
||||
PY
|
||||
|
||||
write_credentials() {
|
||||
@@ -155,6 +176,7 @@ acting_identity=""
|
||||
case "$auth_token" in
|
||||
"$PR_REVIEW_DEFAULT_TOKEN") acting_identity="$PR_REVIEW_ACTING_LOGIN" ;;
|
||||
"$PR_REVIEW_OVERRIDE_TOKEN") acting_identity="$PR_REVIEW_OVERRIDE_LOGIN" ;;
|
||||
"$PR_REVIEW_CROSS_HOST_TOKEN") acting_identity="$PR_REVIEW_CROSS_HOST_LOGIN" ;;
|
||||
esac
|
||||
printf '%s %s %s\n' "$method" "$path" "${acting_identity:-<unauthenticated>}" >> "$PR_REVIEW_AUTH_LOG"
|
||||
|
||||
@@ -245,23 +267,6 @@ else:
|
||||
print(json.dumps(match))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "GET" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
emit "$(PR_REVIEW_QUERY="$query" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
from urllib.parse import parse_qs
|
||||
|
||||
state_path = os.environ["PR_REVIEW_REVIEWS"]
|
||||
params = parse_qs(os.environ["PR_REVIEW_QUERY"])
|
||||
limit = int(params.get("limit", ["50"])[0])
|
||||
page = int(params.get("page", ["1"])[0])
|
||||
with open(state_path, encoding="utf-8") as handle:
|
||||
reviews = json.load(handle)
|
||||
start = (page - 1) * limit
|
||||
print("200")
|
||||
print(json.dumps(reviews[start:start + limit]))
|
||||
PY
|
||||
)"
|
||||
elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/issues/123/comments" ]]; then
|
||||
case "$mode" in
|
||||
write-transport-failure)
|
||||
@@ -278,13 +283,18 @@ import os
|
||||
|
||||
state_path = os.environ["PR_REVIEW_COMMENTS"]
|
||||
acting = os.environ["PR_REVIEW_ACTING_LOGIN"]
|
||||
base = os.environ["PR_REVIEW_EXPECTED_API_BASE"]
|
||||
web_base = os.environ["PR_REVIEW_WEB_BASE"]
|
||||
body = json.loads(os.environ["PR_REVIEW_PAYLOAD"]).get("body")
|
||||
# REAL Gitea shape for a comment posted to a PR's conversation
|
||||
# (/issues/{n}/comments on a PR): pull_request_url is the WEB pulls path and
|
||||
# issue_url is left empty. This is what the wrapper must tolerate — it must NOT
|
||||
# require an API-shaped issue_url.
|
||||
record = {
|
||||
"id": 456,
|
||||
"body": body,
|
||||
"user": {"login": acting},
|
||||
"issue_url": f"{base}/issues/123",
|
||||
"issue_url": "",
|
||||
"pull_request_url": f"{web_base}/pulls/123",
|
||||
}
|
||||
with open(state_path, "w", encoding="utf-8") as handle:
|
||||
json.dump([record], handle)
|
||||
@@ -344,10 +354,10 @@ def review(rid, state, commit, login):
|
||||
return {"id": rid, "state": state, "commit_id": commit, "user": {"login": login}}
|
||||
|
||||
|
||||
if mode == "paginated-approve":
|
||||
# 50 pre-existing reviews fill page 1 (limit 50); the review this run submits
|
||||
# becomes id 51 and lands ALONE on page 2, exercising >page-1 pagination in
|
||||
# the enumeration check.
|
||||
if mode == "many-prior-approve":
|
||||
# 50 pre-existing reviews already exist; the review this run submits becomes
|
||||
# id 51, proving exact-id read-back works regardless of how many reviews
|
||||
# precede it (no list enumeration is involved).
|
||||
reviews = [review(i, "COMMENT", "oldsha0000", acting) for i in range(1, 51)]
|
||||
elif mode == "no-op-concurrent-review":
|
||||
# A concurrent SAME-IDENTITY APPROVED review at the CURRENT head already
|
||||
@@ -370,6 +380,7 @@ run_review() {
|
||||
local login_override="${7:-}"
|
||||
local expected_api_base="${configured_url%/}/api/v1/repos/$expected_repo"
|
||||
local expected_api_root="${configured_url%/}/api/v1"
|
||||
local expected_web_base="${configured_url%/}/$expected_repo"
|
||||
git -C "$REPO_DIR" remote set-url origin "$remote_url"
|
||||
write_credentials "$configured_url"
|
||||
: > "$TEA_LOG"
|
||||
@@ -380,6 +391,7 @@ run_review() {
|
||||
(
|
||||
cd "$REPO_DIR"
|
||||
PATH="$BIN_DIR:$PATH" \
|
||||
TMPDIR="$TMP_SCRATCH" \
|
||||
XDG_CONFIG_HOME="$XDG_DIR" \
|
||||
MOSAIC_CREDENTIALS_FILE="$CREDENTIALS_FILE" \
|
||||
PR_REVIEW_TEA_LOG="$TEA_LOG" \
|
||||
@@ -393,16 +405,32 @@ run_review() {
|
||||
PR_REVIEW_EXPECTED_BODY="$comment" \
|
||||
PR_REVIEW_EXPECTED_API_BASE="$expected_api_base" \
|
||||
PR_REVIEW_API_ROOT="$expected_api_root" \
|
||||
PR_REVIEW_WEB_BASE="$expected_web_base" \
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" \
|
||||
PR_REVIEW_ACTING_LOGIN="$ACTING_LOGIN" \
|
||||
PR_REVIEW_FOREIGN_LOGIN="$FOREIGN_LOGIN" \
|
||||
PR_REVIEW_OVERRIDE_LOGIN="$OVERRIDE_LOGIN" \
|
||||
PR_REVIEW_CROSS_HOST_LOGIN="$CROSS_HOST_LOGIN" \
|
||||
PR_REVIEW_DEFAULT_TOKEN="$DEFAULT_TOKEN" \
|
||||
PR_REVIEW_OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \
|
||||
PR_REVIEW_CROSS_HOST_TOKEN="$CROSS_HOST_TOKEN" \
|
||||
"$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"} ${login_override:+--login "$login_override"}
|
||||
) > "$OUTPUT_FILE" 2>&1
|
||||
}
|
||||
|
||||
# Assert the wrapper left no scratch temp files behind in TMPDIR (POST/GET
|
||||
# request bodies + metadata). Called after both success and failure paths so a
|
||||
# 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)
|
||||
if [[ -n "$leaked" ]]; then
|
||||
echo "FAIL: pr-review temp files leaked ($context):" >&2
|
||||
printf '%s\n' "$leaked" >&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
|
||||
@@ -421,12 +449,17 @@ grep -q '^GET https://git.mosaicstack.dev/api/v1/user$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123$' "$CURL_LOG"
|
||||
grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/101$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=1$' "$CURL_LOG"
|
||||
# No review-list enumeration is performed — the exact-id GET is authoritative.
|
||||
if grep -Eq '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews(\?|$)' "$CURL_LOG"; then
|
||||
echo "FAIL: wrapper performed a redundant review-list enumeration" >&2
|
||||
exit 1
|
||||
fi
|
||||
# No-override default path: the write, /user lookup, and read-back all resolve
|
||||
# via the host-default credential and authenticate as the acting identity.
|
||||
grep -q "^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews $ACTING_LOGIN\$" "$AUTH_LOG"
|
||||
grep -q "^GET https://git.mosaicstack.dev/api/v1/user $ACTING_LOGIN\$" "$AUTH_LOG"
|
||||
assert_no_tea_write
|
||||
assert_no_temp_leak "approve"
|
||||
# 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
|
||||
@@ -454,6 +487,8 @@ if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: read-back did not enforce acting-identity authorship" >&2
|
||||
exit 1
|
||||
fi
|
||||
# Failure-after-read-back path must ALSO leave no scratch temp files behind.
|
||||
assert_no_temp_leak "author-mismatch-review"
|
||||
|
||||
# Case 3: a no-op submit with a concurrent SAME-IDENTITY, same-state review at
|
||||
# the current head already present must FAIL CLOSED — the closed concurrency
|
||||
@@ -472,12 +507,16 @@ if grep -q '/pulls/123/reviews/77$' "$CURL_LOG"; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 4: a genuine matching review that lands beyond page 1 of the reviews list
|
||||
# must still be found by the fully-paginating enumeration check.
|
||||
run_review paginated-approve approve
|
||||
# Case 4: a genuine matching review (id 51) created after 50 pre-existing reviews
|
||||
# is still verified by its EXACT provider-returned id — no list enumeration is
|
||||
# needed regardless of how many reviews precede it.
|
||||
run_review many-prior-approve approve
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 51)' "$OUTPUT_FILE"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/51$' "$CURL_LOG"
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews?limit=[0-9]*&page=2$' "$CURL_LOG"
|
||||
if grep -Eq '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews(\?|$)' "$CURL_LOG"; then
|
||||
echo "FAIL: wrapper performed a redundant review-list enumeration" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 5: an approve WITH a body carries that body in the review submit itself —
|
||||
# there is no separate detached comment POST.
|
||||
@@ -522,6 +561,7 @@ grep -q '^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues
|
||||
grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/comments/456$' "$CURL_LOG"
|
||||
grep -q 'Added and verified comment on Gitea PR #123' "$OUTPUT_FILE"
|
||||
assert_no_tea_write
|
||||
assert_no_temp_leak "comment-success"
|
||||
|
||||
run_review http-success comment durable-body http://git.mosaicstack.dev
|
||||
grep -q '^POST http://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/issues/123/comments$' "$CURL_LOG"
|
||||
@@ -620,4 +660,37 @@ if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 10 (#865 Round-5): a --login override that IS present in tea config but
|
||||
# whose URL is a DIFFERENT host than the repo remote must FAIL CLOSED (host-bound
|
||||
# selection). The cross-host token must NEVER be sent to the repo host, and no
|
||||
# review POST occurs.
|
||||
if run_review cross-host approve "" https://git.mosaicstack.dev \
|
||||
https://git.mosaicstack.dev/mosaicstack/stack.git mosaicstack/stack "$CROSS_HOST_LOGIN"; then
|
||||
echo "FAIL: cross-host --login override did not fail closed" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: cross-host --login override reported success" >&2
|
||||
exit 1
|
||||
fi
|
||||
# The cross-host credential must not have performed ANY request against the repo
|
||||
# host — no request may be attributed to the cross-host identity.
|
||||
if grep -q " $CROSS_HOST_LOGIN\$" "$AUTH_LOG"; then
|
||||
echo "FAIL: cross-host credential was sent to the repo host (cross-host leak)" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -qE '^POST .*/pulls/123/reviews ' "$AUTH_LOG"; then
|
||||
echo "FAIL: cross-host --login override performed a review POST" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
echo "FAIL: cross-host --login override fell back to the host-default identity" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_temp_leak "cross-host"
|
||||
|
||||
echo "pr-review.sh REST review + comment create/read-back regression passed"
|
||||
|
||||
Reference in New Issue
Block a user