fix(git-tools): fail closed on unresolvable explicit --login override (#865)
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
Some checks failed
ci/woodpecker/pr/ci Pipeline failed
gitea_resolve_api_for_login silently fell back to the host-default identity whenever the named login could not be resolved for ANY reason -- including when the name came from an EXPLICIT --login override. A caller passing a dedicated per-role credential could thus have its write attributed to the shared default identity while being told it succeeded as requested. Thread an "override was explicit" signal into gitea_resolve_api_for_login (second param, "explicit" when LOGIN_OVERRIDE is non-empty). When the override is explicit and that login's token cannot be resolved, FAIL CLOSED (return 1, clear error naming the login, no host-default fallback). The best-effort host-default fallback now applies ONLY on the no-override default path. Applied symmetrically to issue-comment.sh and all three pr-review.sh dispatch sites (approve / request-changes / comment). Tests: both scripts' write flows now assert credential attribution via a token->identity seam in the curl stub -- (a) resolvable --login override drives the entire write/read-back chain under THAT login's token, nothing under the default; (b) unresolvable --login override fails closed (nonzero, no success line, no write, no default-identity request); (c) no-override default path still succeeds under the host-default best-effort credential. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
Hermes Agent
@@ -18,6 +18,14 @@
|
||||
# the read-back reads that same state. There is no independently fabricated
|
||||
# record for the wrapper to "find" — verification passes only when the POST
|
||||
# genuinely created the record the read-back retrieves.
|
||||
#
|
||||
# #865 Round-4: the curl stub also maps the presented bearer token to the
|
||||
# identity it authenticates as and logs it per request, so tests can prove
|
||||
# credential attribution. An explicit --login override must drive the entire
|
||||
# write→read-back chain under THAT login's token (resolvable case) or FAIL
|
||||
# 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.
|
||||
|
||||
set -euo pipefail
|
||||
|
||||
@@ -32,6 +40,7 @@ COMMENTS_FILE="$STATE_DIR/comments.json"
|
||||
SUBMIT_PAYLOAD_FILE="$STATE_DIR/review_payload.json"
|
||||
TEA_LOG="$WORK_DIR/tea.log"
|
||||
CURL_LOG="$WORK_DIR/curl.log"
|
||||
AUTH_LOG="$WORK_DIR/auth.log"
|
||||
OUTPUT_FILE="$WORK_DIR/output.log"
|
||||
CREDENTIALS_FILE="$WORK_DIR/credentials.json"
|
||||
|
||||
@@ -43,11 +52,32 @@ trap cleanup EXIT
|
||||
ACTING_LOGIN="review-bot"
|
||||
FOREIGN_LOGIN="other-writer"
|
||||
HEAD_SHA="HEADSHA_FEEDFACE"
|
||||
# A dedicated per-role --login override identity with its own token in tea's
|
||||
# config (the author-not-equal-reviewer hardening path).
|
||||
OVERRIDE_LOGIN="primary-reviewer"
|
||||
DEFAULT_TOKEN="test-only-placeholder"
|
||||
OVERRIDE_TOKEN="override-token-placeholder"
|
||||
|
||||
mkdir -p "$REPO_DIR" "$BIN_DIR" "$XDG_DIR" "$STATE_DIR"
|
||||
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.
|
||||
mkdir -p "$XDG_DIR/tea"
|
||||
OVERRIDE_LOGIN="$OVERRIDE_LOGIN" OVERRIDE_TOKEN="$OVERRIDE_TOKEN" python3 - "$XDG_DIR/tea/config.yml" <<'PY'
|
||||
import os
|
||||
import sys
|
||||
|
||||
with open(sys.argv[1], "w", encoding="utf-8") as handle:
|
||||
handle.write("logins:\n")
|
||||
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")
|
||||
PY
|
||||
|
||||
write_credentials() {
|
||||
local configured_url="$1"
|
||||
CONFIGURED_GITEA_URL="$configured_url" python3 - "$CREDENTIALS_FILE" <<'PY'
|
||||
@@ -96,10 +126,14 @@ output_file=""
|
||||
method="GET"
|
||||
payload=""
|
||||
url=""
|
||||
auth_token=""
|
||||
while [[ $# -gt 0 ]]; do
|
||||
case "$1" in
|
||||
-o) output_file="$2"; shift 2 ;;
|
||||
-w|-H) shift 2 ;;
|
||||
-H)
|
||||
[[ "$2" == Authorization:* ]] && auth_token="${2##* }"
|
||||
shift 2 ;;
|
||||
-w) shift 2 ;;
|
||||
-X) method="$2"; shift 2 ;;
|
||||
-d|--data) payload="$2"; shift 2 ;;
|
||||
-s|-S|-sS) shift ;;
|
||||
@@ -113,6 +147,17 @@ query="${url#*\?}"
|
||||
[[ "$query" == "$url" ]] && query=""
|
||||
printf '%s %s\n' "$method" "$url" >> "$PR_REVIEW_CURL_LOG"
|
||||
|
||||
# Map the presented bearer token to the identity it authenticates as (as Gitea's
|
||||
# /user does). The write, /user lookup, and read-back must all carry the SAME
|
||||
# token, so the identity logged here reveals which credential performed each
|
||||
# request — proving an explicit --login override is honored, not downgraded.
|
||||
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" ;;
|
||||
esac
|
||||
printf '%s %s %s\n' "$method" "$path" "${acting_identity:-<unauthenticated>}" >> "$PR_REVIEW_AUTH_LOG"
|
||||
|
||||
write_response() {
|
||||
local status="$1" body="$2"
|
||||
[[ -n "$output_file" ]] || exit 96
|
||||
@@ -129,7 +174,8 @@ emit() {
|
||||
mode="${PR_REVIEW_TEST_MODE:-}"
|
||||
|
||||
if [[ "$method" == "GET" && "$path" == "$PR_REVIEW_API_ROOT/user" ]]; then
|
||||
write_response 200 "$(PR_REVIEW_LOGIN="$PR_REVIEW_ACTING_LOGIN" python3 - <<'PY'
|
||||
[[ -n "$acting_identity" ]] || { write_response 401 '{"message":"unauthenticated"}'; exit 0; }
|
||||
write_response 200 "$(PR_REVIEW_LOGIN="$acting_identity" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
print(json.dumps({"login": os.environ["PR_REVIEW_LOGIN"]}))
|
||||
@@ -144,7 +190,7 @@ PY
|
||||
)"
|
||||
elif [[ "$method" == "POST" && "$path" == "$PR_REVIEW_EXPECTED_API_BASE/pulls/123/reviews" ]]; then
|
||||
printf '%s' "$payload" > "$PR_REVIEW_SUBMIT_PAYLOAD"
|
||||
emit "$(PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY'
|
||||
emit "$(PR_REVIEW_ACTING_LOGIN="${acting_identity:-$PR_REVIEW_ACTING_LOGIN}" PR_REVIEW_PAYLOAD="$payload" python3 - <<'PY'
|
||||
import json
|
||||
import os
|
||||
|
||||
@@ -321,12 +367,14 @@ run_review() {
|
||||
local configured_url="${4:-https://git.mosaicstack.dev}"
|
||||
local remote_url="${5:-https://git.mosaicstack.dev/mosaicstack/stack.git}"
|
||||
local expected_repo="${6:-mosaicstack/stack}"
|
||||
local login_override="${7:-}"
|
||||
local expected_api_base="${configured_url%/}/api/v1/repos/$expected_repo"
|
||||
local expected_api_root="${configured_url%/}/api/v1"
|
||||
git -C "$REPO_DIR" remote set-url origin "$remote_url"
|
||||
write_credentials "$configured_url"
|
||||
: > "$TEA_LOG"
|
||||
: > "$CURL_LOG"
|
||||
: > "$AUTH_LOG"
|
||||
: > "$OUTPUT_FILE"
|
||||
seed_state "$mode"
|
||||
(
|
||||
@@ -337,6 +385,7 @@ run_review() {
|
||||
PR_REVIEW_TEA_LOG="$TEA_LOG" \
|
||||
PR_REVIEW_LOGIN_URL="${configured_url%/}" \
|
||||
PR_REVIEW_CURL_LOG="$CURL_LOG" \
|
||||
PR_REVIEW_AUTH_LOG="$AUTH_LOG" \
|
||||
PR_REVIEW_REVIEWS="$REVIEWS_FILE" \
|
||||
PR_REVIEW_COMMENTS="$COMMENTS_FILE" \
|
||||
PR_REVIEW_SUBMIT_PAYLOAD="$SUBMIT_PAYLOAD_FILE" \
|
||||
@@ -347,7 +396,10 @@ run_review() {
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" \
|
||||
PR_REVIEW_ACTING_LOGIN="$ACTING_LOGIN" \
|
||||
PR_REVIEW_FOREIGN_LOGIN="$FOREIGN_LOGIN" \
|
||||
"$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"}
|
||||
PR_REVIEW_OVERRIDE_LOGIN="$OVERRIDE_LOGIN" \
|
||||
PR_REVIEW_DEFAULT_TOKEN="$DEFAULT_TOKEN" \
|
||||
PR_REVIEW_OVERRIDE_TOKEN="$OVERRIDE_TOKEN" \
|
||||
"$SCRIPT_DIR/pr-review.sh" -n 123 -a "$action" ${comment:+-c "$comment"} ${login_override:+--login "$login_override"}
|
||||
) > "$OUTPUT_FILE" 2>&1
|
||||
}
|
||||
|
||||
@@ -370,6 +422,10 @@ grep -q '^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/1
|
||||
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-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
|
||||
# The submitted review payload carries the event and the PR head commit_id.
|
||||
PR_REVIEW_HEAD_SHA="$HEAD_SHA" python3 - "$SUBMIT_PAYLOAD_FILE" <<'PY'
|
||||
@@ -521,4 +577,47 @@ if grep -q 'Added and verified comment' "$OUTPUT_FILE"; then
|
||||
exit 1
|
||||
fi
|
||||
|
||||
# Case 8 (#865 Round-4): a RESOLVABLE explicit --login override must attribute
|
||||
# the entire write→read-back chain to THAT login's token/identity, never the
|
||||
# host-default identity. The override login carries its own token in the tea
|
||||
# config, so /user, the review POST, and the exact-id read-back all authenticate
|
||||
# as the override identity — and NOTHING is performed under the default identity.
|
||||
run_review override-success approve "" https://git.mosaicstack.dev \
|
||||
https://git.mosaicstack.dev/mosaicstack/stack.git mosaicstack/stack "$OVERRIDE_LOGIN"
|
||||
grep -q 'Approved and verified Gitea PR #123 (review ID 101)' "$OUTPUT_FILE"
|
||||
grep -q "^GET https://git.mosaicstack.dev/api/v1/user $OVERRIDE_LOGIN\$" "$AUTH_LOG"
|
||||
grep -q "^POST https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews $OVERRIDE_LOGIN\$" "$AUTH_LOG"
|
||||
grep -q "^GET https://git.mosaicstack.dev/api/v1/repos/mosaicstack/stack/pulls/123/reviews/101 $OVERRIDE_LOGIN\$" "$AUTH_LOG"
|
||||
if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
echo "FAIL: an explicit --login override was silently downgraded to the host-default identity" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
assert_no_tea_write
|
||||
|
||||
# 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
|
||||
# POST, and above all NO request performed under the host-default identity. The
|
||||
# host-default best-effort fallback is reserved for the no-override path only.
|
||||
if run_review override-unresolvable approve "" https://git.mosaicstack.dev \
|
||||
https://git.mosaicstack.dev/mosaicstack/stack.git mosaicstack/stack "nonexistent-typo-login"; then
|
||||
echo "FAIL: an unresolvable --login override was not rejected (silently used the host default)" >&2
|
||||
cat "$OUTPUT_FILE" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q 'Approved and verified' "$OUTPUT_FILE"; then
|
||||
echo "FAIL: unresolvable --login override reported success" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q '/pulls/123/reviews ' "$AUTH_LOG" && grep -qE '^POST .*/pulls/123/reviews ' "$AUTH_LOG"; then
|
||||
echo "FAIL: unresolvable --login override performed a review POST" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
if grep -q " $ACTING_LOGIN\$" "$AUTH_LOG"; then
|
||||
echo "FAIL: unresolvable --login override fell back to the host-default identity" >&2
|
||||
cat "$AUTH_LOG" >&2
|
||||
exit 1
|
||||
fi
|
||||
|
||||
echo "pr-review.sh REST review + comment create/read-back regression passed"
|
||||
|
||||
Reference in New Issue
Block a user