diff --git a/.gitea/scripts/review-check.sh b/.gitea/scripts/review-check.sh index 86b78febd..ba2f7153a 100755 --- a/.gitea/scripts/review-check.sh +++ b/.gitea/scripts/review-check.sh @@ -197,19 +197,15 @@ if [ "$HTTP_CODE" != "200" ]; then exit 1 fi -# Filter: state=APPROVED, not-dismissed, non-author. Optionally strict-mode -# adds commit_id==head.sha (off by default; see header). +# Filter: state=APPROVED, official=true, not-dismissed, non-author, +# commit_id matches current PR head. All conditions are mandatory. JQ_FILTER='.[] | select(.state == "APPROVED") + | select(.official == true) | select(.dismissed != true) - | select(.official != false) - | select(.user.login != $author)' -if [ "${REVIEW_CHECK_STRICT:-}" = "1" ]; then - JQ_FILTER="${JQ_FILTER} - | select(.commit_id == \$head)" -fi -JQ_FILTER="${JQ_FILTER} - | .user.login" + | select(.user.login != $author) + | select(.commit_id == $head) + | .user.login' REVIEW_CANDIDATES=$(jq -r --arg author "$PR_AUTHOR" --arg head "$PR_HEAD_SHA" "$JQ_FILTER" "$REVIEWS_JSON" | sort -u) debug "candidate non-author approvers: $(echo "$REVIEW_CANDIDATES" | tr '\n' ' ')" @@ -241,49 +237,14 @@ if [ -z "$REVIEW_CANDIDATES" ]; then fi -# --- Fallback/extension (internal#348): check issue comments for agent-approval --- -# core-qa-agent and core-security-agent can approve via issue comments. Always -# include comment candidates, even if the reviews API returned approvals for a -# different team; team membership below is the authoritative filter. -COMMENT_CANDIDATES="" -AGENT_PATTERN="" -case "$TEAM" in - qa) AGENT_PATTERN="\\[core-qa-agent\\]" ;; - security) AGENT_PATTERN="\\[core-security-agent\\]" ;; -esac -HTTP_CODE=$(curl -sS -o "$COMMENTS_JSON" -w '%{http_code}' \ - -K "$CURL_AUTH_FILE" "${API}/repos/${OWNER}/${NAME}/issues/${PR_NUMBER}/comments") -debug "GET /issues/${PR_NUMBER}/comments → HTTP ${HTTP_CODE}" -if [ "$HTTP_CODE" = "200" ]; then - # JQ expression: select non-author comments that match either the - # agent-prefix pattern (case-insensitive) OR a generic approval keyword. - JQ_APPROVALS=' - .[] | - select(.user.login != $author) | - . as $cmt | - if ($agent_pattern | length) > 0 and ($cmt.body // "" | test($agent_pattern; "i")) then - $cmt.user.login - elif ($cmt.body // "" | test("\\b(APPROVED|LGTM|ACCEPTED)\\b"; "i")) then - $cmt.user.login - else - empty - end - ' - COMMENT_CANDIDATES=$(jq -r \ - --arg author "$PR_AUTHOR" \ - --arg agent_pattern "$AGENT_PATTERN" \ - "$JQ_APPROVALS" \ - "$COMMENTS_JSON" 2>/dev/null | sort -u) - debug "comment-based approval candidates: $(echo "$COMMENT_CANDIDATES" | tr '\n' ' ')" - - if [ -n "$COMMENT_CANDIDATES" ]; then - echo "::notice::${TEAM}-review: found $(echo "$COMMENT_CANDIDATES" | wc -w | xargs) comment-based approval candidate(s) — verifying team membership..." - fi -else - debug "could not fetch issue comments (HTTP ${HTTP_CODE})" -fi - -CANDIDATES=$(printf '%s\n%s\n' "$REVIEW_CANDIDATES" "$COMMENT_CANDIDATES" | sed '/^$/d' | sort -u) +# --- COMMENT APPROVAL REMOVED (security hardening) --- +# Previous versions accepted issue comments containing generic approval +# keywords (APPROVED/LGTM/ACCEPTED) or agent prefixes ([core-qa-agent], +# [core-security-agent]) as satisfying the gate. Both paths are bypasses: +# a comment lacks the audit trail, dismissal, stale-review invalidation, +# and commit_id binding that an official Gitea review provides. +# Only APPROVED reviews from the Gitea reviews API count. +CANDIDATES="$REVIEW_CANDIDATES" if [ -z "${CANDIDATES:-}" ]; then echo "::error::${TEAM}-review awaiting non-author APPROVE from ${TEAM} team (no candidates from reviews API or issue comments)" diff --git a/.gitea/scripts/tests/_review_check_fixture.py b/.gitea/scripts/tests/_review_check_fixture.py index e31e92b86..c6b7efbf3 100644 --- a/.gitea/scripts/tests/_review_check_fixture.py +++ b/.gitea/scripts/tests/_review_check_fixture.py @@ -109,23 +109,34 @@ class Handler(http.server.BaseHTTPRequestHandler): return self._json(200, [{ "state": "APPROVED", "dismissed": True, + "official": True, "user": {"login": "core-devops"}, - "commit_id": "abc1234", + "commit_id": "deadbeef0000111122223333444455556666", }]) if sc == "T3_reviews_approved_non_author": return self._json(200, [ - {"state": "CHANGES_REQUESTED", "dismissed": False, "user": {"login": "bob"}, "commit_id": "abc1234"}, - {"state": "APPROVED", "dismissed": False, "user": {"login": "core-devops"}, "commit_id": "abc1234"}, + {"state": "CHANGES_REQUESTED", "dismissed": False, "official": True, "user": {"login": "bob"}, "commit_id": "deadbeef0000111122223333444455556666"}, + {"state": "APPROVED", "dismissed": False, "official": True, "user": {"login": "core-devops"}, "commit_id": "deadbeef0000111122223333444455556666"}, ]) if sc == "T19_ai_sop_ack_approved": # ai-sop-ack member submitted APPROVED review — must NOT count # toward qa-review (team_id=20) or security-review (team_id=21). return self._json(200, [ - {"state": "APPROVED", "dismissed": False, "user": {"login": "ai-reviewer"}, "commit_id": "abc1234"}, + {"state": "APPROVED", "dismissed": False, "official": True, "user": {"login": "ai-reviewer"}, "commit_id": "deadbeef0000111122223333444455556666"}, ]) - # Default: one non-author APPROVED + if sc == "T21_stale_head_approved": + # APPROVED review but on an old commit (stale head) → must be rejected + return self._json(200, [ + {"state": "APPROVED", "dismissed": False, "official": True, "user": {"login": "core-devops"}, "commit_id": "oldsha0000000000000000000000000000"}, + ]) + if sc == "T22_missing_official": + # APPROVED review with no official field → must be rejected + return self._json(200, [ + {"state": "APPROVED", "dismissed": False, "user": {"login": "core-devops"}, "commit_id": "deadbeef0000111122223333444455556666"}, + ]) + # Default: one non-author APPROVED (current head, official) return self._json(200, [ - {"state": "APPROVED", "dismissed": False, "user": {"login": "core-devops"}, "commit_id": "abc1234"}, + {"state": "APPROVED", "dismissed": False, "official": True, "user": {"login": "core-devops"}, "commit_id": "deadbeef0000111122223333444455556666"}, ]) # GET /repos/{owner}/{name}/issues/{pr_number}/comments diff --git a/.gitea/scripts/tests/test_review_check.sh b/.gitea/scripts/tests/test_review_check.sh index 683b41dc0..dd1cf4cbd 100755 --- a/.gitea/scripts/tests/test_review_check.sh +++ b/.gitea/scripts/tests/test_review_check.sh @@ -14,10 +14,17 @@ # T9 — team membership probe → 403 (token not in team) → script exits 1 (fail closed) # T10 — CURL_AUTH_FILE created with mode 600 and correct header content # T11 — bash syntax check (bash -n passes) -# T12 — jq filter: non-author APPROVED → in candidate list; dismissed → excluded +# T12 — jq filter: non-author APPROVED official current-head → in candidate list; dismissed → excluded # T13 — missing required env GITEA_TOKEN → exits 1 with error # T14 — non-default-base PR exits 0 without requiring review -# T18 — wrong-team review candidate does not block right-team comment approval +# T15 — comment agent-prefix approval → exit 1 +# T16 — comment generic keyword approval → exit 1 +# T17 — comments with no approval keywords → exit 1 +# T18 — wrong-team review + right-team comment → exit 1 +# T19 — ai-sop-ack APPROVED review excluded from qa-review gate +# T20 — ai-sop-ack APPROVED review excluded from security-review gate +# T21 — stale-head APPROVED review → exit 1 (commit_id mismatch) +# T22 — missing/non-official APPROVED review → exit 1 (official != true) # # Hostile-self-review (per feedback_assert_exact_not_substring): # this test MUST FAIL if the script is absent. Verified by running @@ -319,41 +326,50 @@ assert_file_contains "T10b printf header format (CURL_AUTH_FILE content)" "$T10_ assert_file_contains "T10c 'header =' curl-config syntax" "$T10_AUTHFILE" 'header = "Authorization: token ' rm -f "$T10_AUTHFILE" -# T12 — jq filter: non-author APPROVED included, dismissed excluded +# T12 — jq filter: non-author APPROVED official current-head included; dismissed/stale/missing-official excluded echo echo "== T12 jq filter ==" # These are tested indirectly via T3 and T6 above, but let's also test # the jq expression directly. JQ_FILTER='.[] | select(.state == "APPROVED") + | select(.official == true) | select(.dismissed != true) | select(.user.login != "alice") + | select(.commit_id == $head) | .user.login' -T12_INPUT='[{"state":"APPROVED","dismissed":false,"user":{"login":"core-devops"}},{"state":"CHANGES_REQUESTED","dismissed":false,"user":{"login":"bob"}},{"state":"APPROVED","dismissed":false,"user":{"login":"alice"}},{"state":"APPROVED","dismissed":true,"user":{"login":"carol"}}]' +T12_INPUT='[{"state":"APPROVED","official":true,"dismissed":false,"commit_id":"deadbeef0000111122223333444455556666","user":{"login":"core-devops"}},{"state":"CHANGES_REQUESTED","official":true,"dismissed":false,"commit_id":"deadbeef0000111122223333444455556666","user":{"login":"bob"}},{"state":"APPROVED","official":true,"dismissed":false,"commit_id":"deadbeef0000111122223333444455556666","user":{"login":"alice"}},{"state":"APPROVED","official":true,"dismissed":true,"commit_id":"deadbeef0000111122223333444455556666","user":{"login":"carol"}},{"state":"APPROVED","official":false,"dismissed":false,"commit_id":"deadbeef0000111122223333444455556666","user":{"login":"dave"}},{"state":"APPROVED","official":true,"dismissed":false,"commit_id":"oldsha0000000000000000000000000000","user":{"login":"eve"}}]' JQ_CMD=$(command -v jq 2>/dev/null || echo /tmp/jq) -T12_CANDIDATES=$(echo "$T12_INPUT" | "$JQ_CMD" -r "$JQ_FILTER" 2>/dev/null | sort -u) -assert_contains "T12 jq: core-devops (non-author APPROVED) in candidates" "core-devops" "$T12_CANDIDATES" +T12_CANDIDATES=$(echo "$T12_INPUT" | "$JQ_CMD" -r --arg head "deadbeef0000111122223333444455556666" "$JQ_FILTER" 2>/dev/null | sort -u) +assert_contains "T12 jq: core-devops (non-author APPROVED official current-head) in candidates" "core-devops" "$T12_CANDIDATES" assert_eq "T12 jq: alice (author) NOT in candidates" "" "$(echo "$T12_CANDIDATES" | grep '^alice$' || true)" assert_eq "T12 jq: carol (dismissed) NOT in candidates" "" "$(echo "$T12_CANDIDATES" | grep '^carol$' || true)" +assert_eq "T12 jq: dave (official=false) NOT in candidates" "" "$(echo "$T12_CANDIDATES" | grep '^dave$' || true)" +assert_eq "T12 jq: eve (stale head) NOT in candidates" "" "$(echo "$T12_CANDIDATES" | grep '^eve$' || true)" -# T15 — comment-based approval via agent prefix pattern → exit 0 +# T15 — comment-based approval via agent prefix pattern → exit 1 +# SECURITY: agent-prefix comments are also removed. A text prefix in an +# issue comment is spoofable (any team member can type "[core-qa-agent]") +# and lacks the audit trail of an official Gitea review. echo echo "== T15 comment agent-prefix approval ==" T15_OUT=$(run_review_check "T15_comments_agent_approval") T15_RC=$(cat "$FIX_STATE_DIR/last_rc") -assert_eq "T15 exit code 0 (agent-comment approval + team member)" "0" "$T15_RC" -assert_contains "T15 comment fallback notice" "comment-based approval" "$T15_OUT" -assert_contains "T15 core-qa-agent APPROVED" "APPROVED by core-qa-agent" "$T15_OUT" +assert_eq "T15 exit code 1 (agent-prefix comment rejected — not an official review)" "1" "$T15_RC" +assert_contains "T15 no candidates error" "no candidates from reviews API or issue comments" "$T15_OUT" -# T16 — comment-based approval via generic APPROVED keyword → exit 0 +# T16 — comment-based approval via generic APPROVED keyword → exit 1 +# SECURITY: generic keywords (APPROVED/LGTM/ACCEPTED) must NOT satisfy the +# gate — only official Gitea reviews or agent-prefix comments count. A plain +# comment from a team member is a bypass if it skips the review UI. echo echo "== T16 comment generic keyword approval ==" T16_OUT=$(run_review_check "T16_comments_generic_approval") T16_RC=$(cat "$FIX_STATE_DIR/last_rc") -assert_eq "T16 exit code 0 (generic-approval comment + team member)" "0" "$T16_RC" -assert_contains "T16 comment fallback notice" "comment-based approval" "$T16_OUT" +assert_eq "T16 exit code 1 (generic-approval comment rejected — not an official review)" "1" "$T16_RC" +assert_contains "T16 no candidates error" "no candidates from reviews API or issue comments" "$T16_OUT" # T17 — no approval keywords in comments → exit 1 echo @@ -363,16 +379,16 @@ T17_RC=$(cat "$FIX_STATE_DIR/last_rc") assert_eq "T17 exit code 1 (no candidates from comments)" "1" "$T17_RC" assert_contains "T17 no candidates error" "no candidates from reviews API or issue comments" "$T17_OUT" -# T18 — a wrong-team PR review candidate must not suppress a right-team -# comment approval. This matches PR #1790, where QA had an APPROVED review -# and security approved via the agent comment convention. +# T18 — wrong-team review + right-team comment → exit 1 +# SECURITY: with comment approval fully removed, a wrong-team review plus +# a right-team comment yields NO valid candidates. Only official reviews +# from the target team count. echo echo "== T18 review candidate wrong team, comment candidate right team ==" T18_OUT=$(run_review_check "T18_review_wrong_team_comment_right_team") T18_RC=$(cat "$FIX_STATE_DIR/last_rc") -assert_eq "T18 exit code 0 (comment approval still considered)" "0" "$T18_RC" -assert_contains "T18 comment candidate notice" "comment-based approval" "$T18_OUT" -assert_contains "T18 comment approver accepted" "APPROVED by core-qa-agent" "$T18_OUT" +assert_eq "T18 exit code 1 (comment approval removed — no valid candidates)" "1" "$T18_RC" +assert_contains "T18 none are in team" "none are in team" "$T18_OUT" # T19 — ai-sop-ack member APPROVED review must NOT count toward qa-review # or security-review (R1 hardening refinement, msg 1388c76f). @@ -393,6 +409,24 @@ assert_eq "T20 exit code 1 (ai-sop-ack not in security team)" "1" "$T20_RC" assert_contains "T20 ai-reviewer excluded from security" "candidates: ai-reviewer" "$T20_OUT" assert_contains "T20 none are in security team" "none are in team" "$T20_OUT" +# T21 — stale-head APPROVED review must be rejected (commit_id mismatch). +# SECURITY: an approval on an old commit does not cover the current head. +echo +echo "== T21 stale-head APPROVED review rejected ==" +T21_OUT=$(run_review_check "T21_stale_head_approved") +T21_RC=$(cat "$FIX_STATE_DIR/last_rc") +assert_eq "T21 exit code 1 (stale-head approval rejected)" "1" "$T21_RC" +assert_contains "T21 no candidates error" "no candidates from reviews API or issue comments" "$T21_OUT" + +# T22 — missing/non-official APPROVED review must be rejected. +# SECURITY: only official Gitea reviews count; comments and non-official reviews lack audit trail. +echo +echo "== T22 missing official flag APPROVED review rejected ==" +T22_OUT=$(run_review_check "T22_missing_official") +T22_RC=$(cat "$FIX_STATE_DIR/last_rc") +assert_eq "T22 exit code 1 (missing official rejected)" "1" "$T22_RC" +assert_contains "T22 no candidates error" "no candidates from reviews API or issue comments" "$T22_OUT" + echo echo "------" echo "PASS=$PASS FAIL=$FAIL"