diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index e4d2c7f07..e85278e76 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -128,7 +128,7 @@ jobs: # Collaborator role_name vs min (write|triage). See #5223 / ADR 0054. # API resolves org membership regardless of visibility (gh-aw-mcpg#2862). has_repo_permission() { - local username="${1:-}" min="${2:-write}" role api_err + local username="${1:-}" min="${2:-write}" role api_err sanitized_err [[ -z "${username}" ]] && return 1 api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 @@ -136,7 +136,8 @@ jobs: } role=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${username}/permission" \ --jq '.role_name' 2>"${api_err}") || { - echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2 + sanitized_err=$(tr '\n' ' ' < "${api_err}" | sed 's/::/: /g') + echo "::warning::Permission API call failed for ${username}: ${sanitized_err}" >&2 rm -f "${api_err}" return 1 } @@ -148,25 +149,44 @@ jobs: esac } - has_write_permission() { has_repo_permission "${1:-}" write; } - # Slash-command auth; optional $1 = write|triage (default write). - is_authorized() { - has_repo_permission "${COMMENT_USER_LOGIN}" "${1:-write}" - } + is_authorized() { has_repo_permission "${COMMENT_USER_LOGIN:-}" "${1:-write}"; } # Event-actor auth; $1=user, optional $2 = write|triage (default write). - is_event_actor_authorized() { - has_repo_permission "${1:-}" "${2:-write}" - } - - is_issue_author() { - [[ "${COMMENT_USER_LOGIN}" == "${ISSUE_USER_LOGIN}" ]] + is_event_actor_authorized() { has_repo_permission "${1:-}" "${2:-write}"; } + + is_issue_author() { [[ "${COMMENT_USER_LOGIN:-}" == "${ISSUE_USER_LOGIN:-}" ]]; } + + FULLSEND_AGENT_ROLES="coder review triage retro prioritize" + + # fullsend's own agent bots bypass the collaborator permission API + # (#5188, #2636). Two exact-match trust paths, never a wildcard: + # 1. Shared, vendor-owned fullsend-ai-[bot] Apps — the + # default deployment model (ADR 0029/0059/0068); every + # adopting org installs the same public Apps regardless of + # its own org name. + # 2. A self-managed org's own exact ${ORG_NAME}-[bot] + # identity (ADR 0029/0033), for orgs running private + # per-role Apps named after themselves. + is_org_bot() { + local username="${1:-}" want_role="${2:-}" role + [[ -z "${username}" ]] && return 1 + for role in ${FULLSEND_AGENT_ROLES}; do + [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue + [[ "${username}" == "fullsend-ai-${role}[bot]" ]] && return 0 + done + if [[ -n "${ORG_NAME:-}" ]]; then + for role in ${FULLSEND_AGENT_ROLES}; do + [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue + [[ "${username}" == "${ORG_NAME}-${role}[bot]" ]] && return 0 + done + fi + return 1 } has_label() { local needle="$1" - local csv="${2:-${ISSUE_LABELS}}" + local csv="${2:-${ISSUE_LABELS:-}}" IFS=',' read -ra labels <<< "${csv}" for l in "${labels[@]}"; do [[ "$l" == "$needle" ]] && return 0 @@ -243,11 +263,11 @@ jobs: issues) if [[ "${EVENT_ACTION}" == "opened" ]]; then - if is_event_actor_authorized "${ISSUE_USER_LOGIN}" triage; then + if is_org_bot "${ISSUE_USER_LOGIN}" || is_event_actor_authorized "${ISSUE_USER_LOGIN}" triage; then STAGE="triage" fi elif [[ "${EVENT_ACTION}" == "edited" ]]; then - if is_event_actor_authorized "${EVENT_SENDER_LOGIN}" triage; then + if is_org_bot "${EVENT_SENDER_LOGIN}" || is_event_actor_authorized "${EVENT_SENDER_LOGIN}" triage; then STAGE="triage" fi elif [[ "${EVENT_ACTION}" == "labeled" ]]; then @@ -255,6 +275,9 @@ jobs: STAGE="triage" elif [[ "${TRIGGERING_LABEL}" == "ready-to-code" ]]; then # Mutation stage: write+ labeler, or bots (agent handoff). + # Broad [bot]$ here (not exact-match is_org_bot) because + # labeling already requires write access — the request was + # already authorized before this bot-to-bot continuation. if [[ "${EVENT_SENDER_LOGIN}" =~ \[bot\]$ ]] || is_event_actor_authorized "${EVENT_SENDER_LOGIN}"; then STAGE="code" fi @@ -269,7 +292,7 @@ jobs: pull_request_target) case "${EVENT_ACTION}" in opened|synchronize|ready_for_review) - if is_event_actor_authorized "${PR_USER_LOGIN}" triage; then + if is_org_bot "${PR_USER_LOGIN}" || is_event_actor_authorized "${PR_USER_LOGIN}" triage; then STAGE="review" fi ;; @@ -288,8 +311,7 @@ jobs: pull_request_review) if [[ "${EVENT_ACTION}" == "submitted" && "${REVIEW_STATE}" == "changes_requested" ]]; then - REVIEW_BOT="${ORG_NAME}-review[bot]" - if [[ "${REVIEW_USER_LOGIN}" == "${REVIEW_BOT}" ]]; then + if is_org_bot "${REVIEW_USER_LOGIN}" review; then # Human PRs need fullsend-fix + write+ author (not triage-only). if [[ -n "${PR_HEAD_REPO}" && -n "${PR_BASE_REPO}" ]]; then if [[ "${PR_HEAD_REPO}" == "${PR_BASE_REPO}" ]]; then @@ -1085,9 +1107,12 @@ jobs: ORG_NAME: ${{ github.repository_owner }} run: | REVIEW_FILE="${GITHUB_WORKSPACE}/review-body.txt" - REVIEW_BOT="${ORG_NAME}-review[bot]" + # Same two identities as is_org_bot (#5188, #2636, ADR 0054): the + # shared fullsend-ai-review[bot] App, or a self-managed org's own + # exact ${ORG_NAME}-review[bot]. jq can't call the bash helper, so + # both exact logins are inlined here directly. gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/reviews" \ - --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and .user.login == \"${REVIEW_BOT}\")] | last | .body // \"\"" \ + --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and (.user.login == \"fullsend-ai-review[bot]\" or .user.login == \"${ORG_NAME}-review[bot]\"))] | last | .body // \"\"" \ > "${REVIEW_FILE}" 2>/dev/null || echo "" > "${REVIEW_FILE}" BYTE_COUNT="$(wc -c < "${REVIEW_FILE}")" echo "Pre-fetched review body: ${BYTE_COUNT} bytes" diff --git a/.github/workflows/reusable-fix.yml b/.github/workflows/reusable-fix.yml index 1f480751a..c283770e5 100644 --- a/.github/workflows/reusable-fix.yml +++ b/.github/workflows/reusable-fix.yml @@ -349,9 +349,12 @@ jobs: ORG_NAME: ${{ github.repository_owner }} run: | REVIEW_FILE="${GITHUB_WORKSPACE}/review-body.txt" - REVIEW_BOT="${ORG_NAME}-review[bot]" + # Same two identities as is_org_bot (#5188, #2636, ADR 0054): the + # shared fullsend-ai-review[bot] App, or a self-managed org's own + # exact ${ORG_NAME}-review[bot]. jq can't call the bash helper, so + # both exact logins are inlined here directly. gh api "repos/${SOURCE_REPO}/pulls/${PR_NUM}/reviews" \ - --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and .user.login == \"${REVIEW_BOT}\")] | last | .body // \"\"" \ + --jq "[.[] | select(.state == \"CHANGES_REQUESTED\" and (.user.login == \"fullsend-ai-review[bot]\" or .user.login == \"${ORG_NAME}-review[bot]\"))] | last | .body // \"\"" \ > "${REVIEW_FILE}" 2>/dev/null || echo "" > "${REVIEW_FILE}" BYTE_COUNT="$(wc -c < "${REVIEW_FILE}")" echo "Pre-fetched review body: ${BYTE_COUNT} bytes" diff --git a/Makefile b/Makefile index ddc1a858a..5c2fcd1c2 100644 --- a/Makefile +++ b/Makefile @@ -161,6 +161,7 @@ script-test: $(call run-timed,bash internal/scaffold/fullsend-repo/scripts/validate-output-schema-test.sh) $(call run-timed,bash internal/scaffold/fullsend-repo/scripts/pre-code-test.sh) $(call run-timed,bash internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh) + $(call run-timed,bash internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers-test.sh) $(call run-timed,python3 internal/scaffold/fullsend-repo/scripts/process-fix-result-test.py) $(call run-timed,python3 skills/topissues/scripts/topissues_test.py) $(call run-timed,python3 -m pytest gitlint_rules_test.py -v) diff --git a/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md b/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md index c387d4d0a..b81798af2 100644 --- a/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md +++ b/docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md @@ -70,9 +70,10 @@ The check applies universally — to slash commands and to automatic event triggers where the acting user may be external. Both `is_authorized` (slash commands) and `is_event_actor_authorized` -(event triggers) delegate to a shared `has_write_permission` helper that -calls the collaborator permission API. This ensures consistent behavior -across all paths. +(event triggers) delegate to a shared `has_write_permission` helper +(renamed `has_repo_permission` — see note below) that calls the +collaborator permission API. This ensures consistent behavior across +all paths. ### Slash commands @@ -102,6 +103,7 @@ regardless of membership visibility. The implementation uses a three-function layering: - `has_write_permission(username)` — calls the API, checks `.role_name` + (renamed `has_repo_permission` — see note below) - `is_authorized()` — delegates to `has_write_permission` for the comment author - `is_event_actor_authorized(username)` — delegates for event actors @@ -176,8 +178,9 @@ unauthorized users. If a future per-repo configuration system needs to customize authorization rules (e.g., allowing `triage` or `read` permission), it -should do so by extending the `has_write_permission` function's allowed -permission list, not by bypassing the check. +should do so by extending the `has_write_permission` function's +(renamed `has_repo_permission` — see note below) allowed permission +list, not by bypassing the check. > **Note (2026-07-17, [#5223](https://github.com/fullsend-ai/fullsend/issues/5223)):** > Observation stages (triage, review) now accept the GitHub `triage` @@ -189,6 +192,101 @@ permission list, not by bypassing the check. > any closer can trigger read-only lifecycle accounting. This follows > the extension path above rather than bypassing the check. +> **Note (2026-07-21, [#5188](https://github.com/fullsend-ai/fullsend/issues/5188), [#2636](https://github.com/fullsend-ai/fullsend/issues/2636)):** +> `has_write_permission` referenced above was folded into the +> parameterized `has_repo_permission` helper introduced by #5223 and no +> longer exists as a separate function. +> +> GitHub App bot accounts (e.g. the org's own code, review, triage, and +> retro agents) are not resolvable via the collaborator permission API — +> `role_name` comes back empty even though the bot has legitimate +> push/comment access via its app installation grant. This silently +> broke two independent paths: `pull_request_target.opened/synchronize/ +> ready_for_review` (PRs authored by the org's own agents never received +> automatic review, including after every fix-agent iteration — #5188), +> and `issues.opened/edited` (issues filed or edited by the org's own +> agents, e.g. the retro agent's proposal issues, never received +> automatic triage — #2636, previously worked around by having the +> filer also apply a routing label rather than fixing the actor check). +> +> Both paths now also accept a recognized fullsend agent bot actor +> (`is_org_bot`) as authorized, bypassing the collaborator permission API +> check. This follows the extension path above rather than bypassing the +> check, via two exact-match trust paths — never a prefix/suffix +> wildcard: +> +> 1. `fullsend-ai-[bot]` (coder, review, triage, retro, +> prioritize), unconditionally, regardless of the installing repo's +> own org name. fullsend's default deployment model is a shared, +> vendor-owned App per role (ADR 0029/0059/0068) — every adopting org +> installs the *same* public Apps, so the bot identity is fixed and +> does not vary with `ORG_NAME` (`github.repository_owner`). An +> earlier version of this check instead matched `${ORG_NAME}-*[bot]`, +> which only worked by coincidence on fullsend-ai/fullsend itself +> (where org name and vendor-App prefix happen to collide) and was a +> no-op for every other adopting org. +> 2. `${ORG_NAME}-[bot]`, for self-managed orgs that run their own +> private per-role Apps named after themselves (ADR 0029/0033) instead +> of the shared vendor Apps. +> +> Both paths match one of a fixed, known set of role suffixes — never a +> bare prefix wildcard — so a third party can't forge the bypass by +> registering a GitHub App named `${ORG_NAME}-[bot]` or +> `fullsend-ai-[bot]`. `is_org_bot` also takes an optional +> second argument to restrict the match to one specific role (e.g. +> `is_org_bot "${REVIEW_USER_LOGIN}" review`) — used by the +> `pull_request_review` → fix auto-dispatch gate, which previously +> checked `REVIEW_USER_LOGIN == "${ORG_NAME}-review[bot]"` directly and +> had the identical ORG_NAME-vs-shared-App bug as #5188/#2636 (also only +> "worked" by coincidence on fullsend-ai/fullsend itself). The same +> single-identity `REVIEW_BOT="${ORG_NAME}-review[bot]"` mistake also +> existed in three more places that fetch a prior review comment/review +> by login for context rather than for dispatch authorization — the +> `reusable-dispatch.yml` and `reusable-fix.yml` "Pre-fetch review body" +> steps, and `pre-fetch-prior-review.sh` — all now check both identities +> (`fullsend-ai-review[bot]` and `${ORG_NAME}-review[bot]`) directly, +> since `jq` can't call the bash `is_org_bot` helper. The +> separate `PR_USER_LOGIN =~ \[bot\]$` check further down that same gate +> is untouched — it decides whether to auto-fix without requiring an +> explicit `fullsend-fix` label, and stays intentionally broad. The +> `issues.labeled` `ready-to-code` path is also unaffected by this note +> — it already allows a generic `[bot]$` actor for agent handoff, per +> the table above, which is a deliberate, broader trust decision for +> that specific bot-to-bot continuation path (see #2679): a request +> already reached that point via an authorized trigger, so trusting any +> bot to continue the chain doesn't extend trust to a new actor. +> +> This is a platform-constraint exception, not a relaxation of this +> ADR's core principle: authorization here still derives from +> repository permissions (either fullsend's own vendor-App identity or +> the installing org's own exact identity), not an arbitrary identity +> claim. The `pull_request_target`/`issues.opened`/`issues.edited` uses +> reach only read-only observation stages (triage, review); the +> `pull_request_review` use reaches the mutation `fix` stage, but only as +> a continuation of a request already authorized earlier in the same +> chain — a genuine `changes_requested` verdict from fullsend's own +> review bot — not a fresh grant of trust to a new actor, consistent +> with the `ready-to-code` reasoning above. +> +> **Note (2026-07-22):** the "not a fresh grant of trust" framing above +> assumes the `changes_requested` review genuinely came from fullsend's +> own review bot. That assumption is qualitatively weaker on the +> self-managed path than on the shared-vendor-App path: if a third party +> squats `${ORG_NAME}-review[bot]` at a self-managed org (the residual +> risk this ADR already accepts elsewhere), `is_org_bot` still passes — +> by design, since the identity itself is what's untrustworthy in that +> scenario — and the same login match is what the "Pre-fetch review +> body" steps in `reusable-fix.yml`/`reusable-dispatch.yml` use to pull +> that review's body text into the autonomous fix agent's prompt. Unlike +> the `ready-to-code` bot-to-bot continuation (which only ever advances +> a *stage*, not attacker-controlled *content*), this path feeds +> attacker-authored text into a privileged, auto-pushing agent — a +> prompt-injection channel, not benign observation. Pinning this +> specific check to `performed_via_github_app.id` (the numeric, +> non-reusable app ID) instead of login string would close this gap, but +> is out of scope for #5188/#2636 — tracked as +> [#5463](https://github.com/fullsend-ai/fullsend/issues/5463). + ## Consequences - All dispatch paths require write-level repository permission, diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index 8f357f0d0..f582aff8c 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -56,7 +56,7 @@ jobs: # Collaborator role_name vs min (write|triage). See #5223 / ADR 0054. # API resolves org membership regardless of visibility (gh-aw-mcpg#2862). has_repo_permission() { - local username="${1:-}" min="${2:-write}" role api_err + local username="${1:-}" min="${2:-write}" role api_err sanitized_err [[ -z "${username}" ]] && return 1 api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 @@ -64,7 +64,8 @@ jobs: } role=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${username}/permission" \ --jq '.role_name' 2>"${api_err}") || { - echo "::warning::Permission API call failed for ${username}: $(cat "${api_err}")" >&2 + sanitized_err=$(tr '\n' ' ' < "${api_err}" | sed 's/::/: /g') + echo "::warning::Permission API call failed for ${username}: ${sanitized_err}" >&2 rm -f "${api_err}" return 1 } @@ -76,28 +77,26 @@ jobs: esac } - has_write_permission() { has_repo_permission "${1:-}" write; } + is_authorized() { has_repo_permission "${COMMENT_USER_LOGIN:-}" "${1:-write}"; } + is_event_actor_authorized() { has_repo_permission "${1:-}" "${2:-write}"; } + is_issue_author() { [[ "${COMMENT_USER_LOGIN:-}" == "${ISSUE_USER_LOGIN:-}" ]]; } - # Slash-command auth; optional $1 = write|triage (default write). - is_authorized() { - has_repo_permission "${COMMENT_USER_LOGIN}" "${1:-write}" - } - - # Event-actor auth; $1=user, optional $2 = write|triage (default write). - is_event_actor_authorized() { - has_repo_permission "${1:-}" "${2:-write}" - } - - # Helper: check if user is the PR/issue author - is_issue_author() { - [[ "${COMMENT_USER_LOGIN}" == "${ISSUE_USER_LOGIN}" ]] + FULLSEND_AGENT_ROLES="coder review triage retro prioritize" + # fullsend bots bypass the permission API (#5188, #2636, ADR 0054): shared fullsend-ai-[bot] (default, ADR 0029/0059/0068) or exact org-managed ${ORG_NAME}-[bot]. + is_org_bot() { + local username="${1:-}" want_role="${2:-}" role + [[ -z "${username}" ]] && return 1 + for role in ${FULLSEND_AGENT_ROLES}; do [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue; [[ "${username}" == "fullsend-ai-${role}[bot]" ]] && return 0; done + [[ -z "${ORG_NAME:-}" ]] && return 1 + for role in ${FULLSEND_AGENT_ROLES}; do [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue; [[ "${username}" == "${ORG_NAME}-${role}[bot]" ]] && return 0; done + return 1 } # Helper: check if a label is present in a comma-separated list. # Usage: has_label [label_csv] (defaults to ISSUE_LABELS) has_label() { local needle="$1" - local csv="${2:-${ISSUE_LABELS}}" + local csv="${2:-${ISSUE_LABELS:-}}" IFS=',' read -ra labels <<< "${csv}" for l in "${labels[@]}"; do [[ "$l" == "$needle" ]] && return 0 @@ -175,11 +174,11 @@ jobs: issues) if [[ "${EVENT_ACTION}" == "opened" ]]; then - if is_event_actor_authorized "${ISSUE_USER_LOGIN}" triage; then + if is_org_bot "${ISSUE_USER_LOGIN}" || is_event_actor_authorized "${ISSUE_USER_LOGIN}" triage; then STAGE="triage" fi elif [[ "${EVENT_ACTION}" == "edited" ]]; then - if is_event_actor_authorized "${EVENT_SENDER_LOGIN}" triage; then + if is_org_bot "${EVENT_SENDER_LOGIN}" || is_event_actor_authorized "${EVENT_SENDER_LOGIN}" triage; then STAGE="triage" fi elif [[ "${EVENT_ACTION}" == "labeled" ]]; then @@ -187,6 +186,8 @@ jobs: STAGE="triage" elif [[ "${TRIGGERING_LABEL}" == "ready-to-code" ]]; then # Mutation stage: write+ labeler, or bots (agent handoff). + # Broad [bot]$ (not is_org_bot): labeling already requires + # write access, so this bot-to-bot handoff was pre-authorized. if [[ "${EVENT_SENDER_LOGIN}" =~ \[bot\]$ ]] || is_event_actor_authorized "${EVENT_SENDER_LOGIN}"; then STAGE="code" fi @@ -201,7 +202,7 @@ jobs: pull_request_target) case "${EVENT_ACTION}" in opened|synchronize|ready_for_review) - if is_event_actor_authorized "${PR_USER_LOGIN}" triage; then + if is_org_bot "${PR_USER_LOGIN}" || is_event_actor_authorized "${PR_USER_LOGIN}" triage; then STAGE="review" fi ;; @@ -220,8 +221,7 @@ jobs: pull_request_review) if [[ "${EVENT_ACTION}" == "submitted" && "${REVIEW_STATE}" == "changes_requested" ]]; then - REVIEW_BOT="${ORG_NAME}-review[bot]" - if [[ "${REVIEW_USER_LOGIN}" == "${REVIEW_BOT}" ]]; then + if is_org_bot "${REVIEW_USER_LOGIN}" review; then # Block fork PRs — fail closed if we cannot determine. # Human PRs need fullsend-fix + write+ author (not triage-only). if [[ -n "${PR_HEAD_REPO}" && -n "${PR_BASE_REPO}" ]]; then diff --git a/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers-test.sh b/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers-test.sh new file mode 100644 index 000000000..ff60974ac --- /dev/null +++ b/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers-test.sh @@ -0,0 +1,205 @@ +#!/usr/bin/env bash +# dispatch-route-helpers-test.sh — Tests for dispatch-route-helpers.sh +# +# Run from the repo root: +# bash internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers-test.sh + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +HELPERS="${SCRIPT_DIR}/dispatch-route-helpers.sh" +FAILURES=0 + +# --- Helpers --- + +run_test() { + local test_name="$1" + local expected_rc="$2" # expected return code + local func_call="$3" # function name + args + local env_overrides="${4:-}" + + # Build env array + local env_cmd=( + env + GITHUB_REPOSITORY="test-org/test-repo" + ORG_NAME="test-org" + GH_TOKEN="fake-token" + COMMENT_USER_LOGIN="" + ISSUE_USER_LOGIN="" + ISSUE_LABELS="" + ) + + if [[ -n "${env_overrides}" ]]; then + while IFS= read -r kv; do + [[ -n "${kv}" ]] && env_cmd+=("${kv}") + done <<< "${env_overrides}" + fi + + local actual_rc=0 + # Source the helpers and call the function in a subshell + "${env_cmd[@]}" bash -c " + source '${HELPERS}' + ${func_call} + " > /dev/null 2>&1 || actual_rc=$? + + if [[ "${actual_rc}" -ne "${expected_rc}" ]]; then + echo "FAIL: ${test_name} — expected rc=${expected_rc}, got rc=${actual_rc}" + FAILURES=$((FAILURES + 1)) + return + fi + + echo "PASS: ${test_name}" +} + +# --- is_org_bot tests --- + +# Path 1: fullsend's own shared, vendor-owned Apps match unconditionally, +# regardless of ORG_NAME — this is the default deployment model +# (ADR 0029/0059/0068), where every adopting org installs the same +# fullsend-ai-[bot] Apps rather than org-named ones. +run_test "is_org_bot: shared fullsend-ai coder bot matches regardless of ORG_NAME" 0 \ + 'is_org_bot "fullsend-ai-coder[bot]"' \ + "ORG_NAME=some-other-customer-org" + +run_test "is_org_bot: shared fullsend-ai review bot matches" 0 \ + 'is_org_bot "fullsend-ai-review[bot]"' + +run_test "is_org_bot: shared fullsend-ai triage bot matches" 0 \ + 'is_org_bot "fullsend-ai-triage[bot]"' + +run_test "is_org_bot: shared fullsend-ai retro bot matches" 0 \ + 'is_org_bot "fullsend-ai-retro[bot]"' + +run_test "is_org_bot: shared fullsend-ai prioritize bot matches" 0 \ + 'is_org_bot "fullsend-ai-prioritize[bot]"' + +# Path 1 doesn't need ORG_NAME at all +run_test "is_org_bot: shared fullsend-ai bot matches with ORG_NAME unset" 0 \ + 'is_org_bot "fullsend-ai-coder[bot]"' \ + "ORG_NAME=" + +# Path 1 is exact-role-match only — not a wildcard on the fullsend-ai- prefix +run_test "is_org_bot: fullsend-ai prefix with unknown role rejected" 1 \ + 'is_org_bot "fullsend-ai-exploit[bot]"' + +# Path 2: a self-managed org's own exact ${ORG_NAME}-[bot] identity +# (ADR 0029/0033) — for orgs running their own private Apps. +run_test "is_org_bot: self-managed org coder bot matches" 0 \ + 'is_org_bot "test-org-coder[bot]"' + +run_test "is_org_bot: self-managed org review bot matches" 0 \ + 'is_org_bot "test-org-review[bot]"' + +run_test "is_org_bot: self-managed org triage bot matches" 0 \ + 'is_org_bot "test-org-triage[bot]"' + +run_test "is_org_bot: self-managed org retro bot matches" 0 \ + 'is_org_bot "test-org-retro[bot]"' + +run_test "is_org_bot: self-managed org prioritize bot matches" 0 \ + 'is_org_bot "test-org-prioritize[bot]"' + +# Path 2 requires ORG_NAME +run_test "is_org_bot: self-managed match rejected with empty ORG_NAME" 1 \ + 'is_org_bot "test-org-coder[bot]"' \ + "ORG_NAME=" + +# Path 2 is exact-role-match only — not a wildcard on the ${ORG_NAME}- prefix. +# This is the spoofing vector the wildcard design allowed: a third party +# registering "${ORG_NAME}-[bot]" must NOT match. +run_test "is_org_bot: org-prefixed non-role bot rejected (no wildcard)" 1 \ + 'is_org_bot "test-org-exploit[bot]"' + +# A different self-managed org's bot does not match this org's identity +run_test "is_org_bot: different org's self-managed bot rejected" 1 \ + 'is_org_bot "other-org-coder[bot]"' + +# Third-party bots matching neither path are rejected +run_test "is_org_bot: third-party bot rejected" 1 \ + 'is_org_bot "renovate[bot]"' + +# Human user does not match +run_test "is_org_bot: human user rejected" 1 \ + 'is_org_bot "human-dev"' + +# Empty username / no argument returns 1 +run_test "is_org_bot: empty username rejected" 1 \ + 'is_org_bot ""' + +run_test "is_org_bot: no argument rejected" 1 \ + 'is_org_bot' + +# ORG_NAME is matched literally (plain string equality in the role loop, +# not a glob/regex), so special characters in an org name can't be +# misinterpreted as a pattern +run_test "is_org_bot: org name with special characters matches literally" 0 \ + 'is_org_bot "a+b.c-coder[bot]"' \ + "ORG_NAME=a+b.c" + +# --- is_org_bot role-filter tests (used by the pull_request_review -> +# fix dispatch gate, which must match specifically the review bot, not +# any fullsend agent bot) --- + +run_test "is_org_bot: role filter matches shared bot with matching role" 0 \ + 'is_org_bot "fullsend-ai-review[bot]" review' + +run_test "is_org_bot: role filter rejects shared bot with different role" 1 \ + 'is_org_bot "fullsend-ai-coder[bot]" review' + +run_test "is_org_bot: role filter matches self-managed bot with matching role" 0 \ + 'is_org_bot "test-org-review[bot]" review' + +run_test "is_org_bot: role filter rejects self-managed bot with different role" 1 \ + 'is_org_bot "test-org-coder[bot]" review' + +# --- has_label tests --- + +run_test "has_label: label present" 0 \ + 'has_label "bug"' \ + "ISSUE_LABELS=bug,enhancement,ready-to-code" + +run_test "has_label: label absent" 1 \ + 'has_label "feature"' \ + "ISSUE_LABELS=bug,enhancement" + +run_test "has_label: empty labels" 1 \ + 'has_label "bug"' \ + "ISSUE_LABELS=" + +run_test "has_label: custom csv" 0 \ + 'has_label "ready-for-review" "ready-for-review,bug"' + +run_test "has_label: custom csv miss" 1 \ + 'has_label "ready-for-review" "bug,enhancement"' + +# has_label must not hard-fail under `set -u` when ISSUE_LABELS is truly +# unset (not just empty) — regression test, since the caller documents +# ISSUE_LABELS as optional but workflows source this under set -euo pipefail. +actual_rc=0 +stderr_output=$(env -u ISSUE_LABELS GITHUB_REPOSITORY="test-org/test-repo" ORG_NAME="test-org" GH_TOKEN="fake-token" \ + bash -c "set -u; source '${HELPERS}'; has_label 'bug'" 2>&1 >/dev/null) || actual_rc=$? +if [[ "${actual_rc}" -eq 1 && "${stderr_output}" != *"unbound variable"* ]]; then + echo "PASS: has_label: unset ISSUE_LABELS under set -u returns 1, not unbound-variable error" +else + echo "FAIL: has_label: unset ISSUE_LABELS under set -u — expected clean rc=1, got rc=${actual_rc} stderr=${stderr_output}" + FAILURES=$((FAILURES + 1)) +fi + +# --- is_issue_author tests --- + +run_test "is_issue_author: matching" 0 \ + 'is_issue_author' \ + "$(printf '%s\n%s' 'COMMENT_USER_LOGIN=alice' 'ISSUE_USER_LOGIN=alice')" + +run_test "is_issue_author: not matching" 1 \ + 'is_issue_author' \ + "$(printf '%s\n%s' 'COMMENT_USER_LOGIN=alice' 'ISSUE_USER_LOGIN=bob')" + +# --- Summary --- + +echo "" +if [[ ${FAILURES} -gt 0 ]]; then + echo "${FAILURES} test(s) failed" + exit 1 +fi +echo "All tests passed" diff --git a/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers.sh b/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers.sh new file mode 100644 index 000000000..d614abf74 --- /dev/null +++ b/internal/scaffold/fullsend-repo/scripts/dispatch-route-helpers.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# dispatch-route-helpers.sh — Reference implementation of the dispatch +# routing helpers, kept in sync by hand with the inline copies in +# dispatch.yml (per-org) and reusable-dispatch.yml (per-repo). +# +# The workflows define these functions inline rather than sourcing this +# file, since sourcing would require an extra checkout step in every +# dispatch run. This file exists so the routing logic is unit-testable +# — see dispatch-route-helpers-test.sh. +# +# Required env vars (set by the caller before sourcing): +# GITHUB_REPOSITORY — full repo name (org/repo) +# ORG_NAME — repository owner / org name +# GH_TOKEN — GitHub token for API calls +# +# Optional env vars (used by routing callers): +# COMMENT_USER_LOGIN, COMMENT_BODY, COMMENT_USER_TYPE, +# COMMENT_AUTHOR_ASSOC, ISSUE_LABELS, PR_LABELS, +# ISSUE_USER_LOGIN, PR_USER_LOGIN, EVENT_SENDER_LOGIN, +# REVIEW_USER_LOGIN, REVIEW_STATE, TRIGGERING_LABEL, +# PR_HEAD_REPO, PR_BASE_REPO + +# Collaborator role_name vs min (write|triage). See #5223 / ADR 0054. +# API resolves org membership regardless of visibility (gh-aw-mcpg#2862). +has_repo_permission() { + local username="${1:-}" min="${2:-write}" role api_err sanitized_err + [[ -z "${username}" ]] && return 1 + api_err=$(mktemp) || { + echo "::warning::Failed to create temp file for permission check of ${username}" >&2 + return 1 + } + role=$(gh api "repos/${GITHUB_REPOSITORY}/collaborators/${username}/permission" \ + --jq '.role_name' 2>"${api_err}") || { + # Sanitize before logging: a crafted API/gh-cli error containing "::" + # could otherwise be interpreted as a GitHub Actions workflow command. + sanitized_err=$(tr '\n' ' ' < "${api_err}" | sed 's/::/: /g') + echo "::warning::Permission API call failed for ${username}: ${sanitized_err}" >&2 + rm -f "${api_err}" + return 1 + } + rm -f "${api_err}" + case "${role}" in + admin|maintain|write) return 0 ;; + triage) [[ "${min}" == "triage" ]] && return 0 || return 1 ;; + *) return 1 ;; + esac +} + +# Slash-command auth; optional $1 = write|triage (default write). +is_authorized() { + has_repo_permission "${COMMENT_USER_LOGIN:-}" "${1:-write}" +} + +# Event-actor auth; $1=user, optional $2 = write|triage (default write). +is_event_actor_authorized() { + has_repo_permission "${1:-}" "${2:-write}" +} + +# Known role suffixes for fullsend's own agent bots. Used by is_org_bot +# below for both trust paths (shared and self-managed) — see there. +FULLSEND_AGENT_ROLES="coder review triage retro prioritize" + +# Check whether a username is one of fullsend's own first-party agent bots, +# optionally restricted to one specific role (e.g. "review") via $2. +# See #5188, #2636 / ADR 0054. GitHub App bot accounts are not resolvable +# via the collaborator permission API, so they always fail +# is_event_actor_authorized/has_repo_permission even though they have +# legitimate push/comment access via their app installation grant. +# +# Two exact-match trust paths, never a prefix/suffix wildcard: +# 1. fullsend's own shared, vendor-owned role Apps (ADR 0029/0059/0068): +# every adopting org installs the SAME public fullsend-ai-[bot] +# Apps, regardless of the installing repo's own org name — this is +# the default/normative deployment model, so these identities are +# trusted unconditionally, independent of ORG_NAME. +# 2. A self-managed org's own exact ${ORG_NAME}-[bot] identity, for +# orgs that run their own private per-role Apps (ADR 0029/0033) named +# after themselves. Exact role match only — not a suffix wildcard — +# so a third party can't forge the bypass by registering an app named +# "${ORG_NAME}-[bot]". +is_org_bot() { + local username="${1:-}" want_role="${2:-}" role + [[ -z "${username}" ]] && return 1 + + for role in ${FULLSEND_AGENT_ROLES}; do + [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue + [[ "${username}" == "fullsend-ai-${role}[bot]" ]] && return 0 + done + + if [[ -n "${ORG_NAME:-}" ]]; then + for role in ${FULLSEND_AGENT_ROLES}; do + [[ -n "${want_role}" && "${role}" != "${want_role}" ]] && continue + [[ "${username}" == "${ORG_NAME}-${role}[bot]" ]] && return 0 + done + fi + + return 1 +} + +# Helper: check if user is the PR/issue author +is_issue_author() { + [[ "${COMMENT_USER_LOGIN:-}" == "${ISSUE_USER_LOGIN:-}" ]] +} + +# Helper: check if a label is present in a comma-separated list. +# Usage: has_label [label_csv] (defaults to ISSUE_LABELS) +has_label() { + local needle="$1" + local csv="${2:-${ISSUE_LABELS:-}}" + IFS=',' read -ra labels <<< "${csv}" + for l in "${labels[@]}"; do + [[ "$l" == "$needle" ]] && return 0 + done + return 1 +} diff --git a/internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh b/internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh index 7b5adbd71..46a8cc047 100644 --- a/internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/pre-fetch-prior-review-test.sh @@ -51,16 +51,19 @@ MOCKEOF } # make_comment_json builds the JSON that gh api would return after -# jq filtering. The body field is set to $1. +# jq filtering. The body field is set to $1; the comment author login +# defaults to the self-managed org identity but can be overridden by $2 +# (e.g. to test the shared fullsend-ai-review[bot] identity). make_comment_json() { local body="$1" + local login="${2:-test-org-review[bot]}" # Escape the body for JSON embedding. local escaped_body escaped_body="$(printf '%s' "${body}" | python3 -c 'import json,sys; print(json.dumps(sys.stdin.read()))')" cat <")))] | last // empty' \ 2>/dev/null || echo "") diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 69ef40853..f9b45c1a0 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -219,9 +219,23 @@ func TestDispatchWorkflowContent(t *testing.T) { // Authorization checks (collaborator permission API using .role_name) assert.Contains(t, s, "is_authorized") assert.Contains(t, s, "has_repo_permission") - assert.Contains(t, s, "has_write_permission") assert.Contains(t, s, ".role_name") assert.Contains(t, s, "admin|maintain|write") + // Org bot actors bypass the collaborator permission check on both + // review dispatch (#5188) and triage dispatch (#2636). Anchored on + // the "||" operator (not just the is_org_bot substring) so flipping + // it to "&&" — which would silently require org bots to also pass + // the collaborator-permission check they're known to fail — fails + // these assertions instead of passing CI green. + assert.Contains(t, s, "is_org_bot") + assert.Regexp(t, `is_org_bot "\$\{ISSUE_USER_LOGIN\}" \|\| is_event_actor_authorized "\$\{ISSUE_USER_LOGIN\}" triage; then`, s) + assert.Regexp(t, `is_org_bot "\$\{EVENT_SENDER_LOGIN\}" \|\| is_event_actor_authorized "\$\{EVENT_SENDER_LOGIN\}" triage; then`, s) + assert.Regexp(t, `is_org_bot "\$\{PR_USER_LOGIN\}" \|\| is_event_actor_authorized "\$\{PR_USER_LOGIN\}" triage; then`, s) + // Third bypass site: pull_request_review -> fix auto-dispatch must + // require specifically the review bot, not any fullsend agent bot. + // Regression test for reverting to the old single-identity + // REVIEW_USER_LOGIN == "${ORG_NAME}-review[bot]" check. + assert.Regexp(t, `is_org_bot "\$\{REVIEW_USER_LOGIN\}" review; then`, s) // Observation stages accept triage; mutation stages stay write+ (#5223) assert.Contains(t, s, `is_authorized triage`) assert.Regexp(t, `is_authorized; then\s*\n\s+STAGE="code"`, s) diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 8071aaabc..ae85e0ec6 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -432,6 +432,141 @@ func TestReusableDispatchWorkflowContent(t *testing.T) { s := string(content) assert.Regexp(t, `(?s)/fs-review\)\s*\n\s+if \[\[ "\$\{ISSUE_IS_PR\}"`, s) assert.Regexp(t, `(?s)ready-for-review"\s*\]\];\s*then\s*\n\s+if \[\[ "\$\{ISSUE_IS_PR\}"`, s) + // Org bot actors bypass the collaborator permission check (#5188, #2636). + // Anchored on the "||" operator (not just the is_org_bot substring) so + // flipping it to "&&" — which would silently require org bots to also + // pass the collaborator-permission check they're known to fail — fails + // these assertions instead of passing CI green. + assert.Contains(t, s, "is_org_bot") + assert.Regexp(t, `is_org_bot "\$\{ISSUE_USER_LOGIN\}" \|\| is_event_actor_authorized "\$\{ISSUE_USER_LOGIN\}" triage; then`, s) + assert.Regexp(t, `is_org_bot "\$\{EVENT_SENDER_LOGIN\}" \|\| is_event_actor_authorized "\$\{EVENT_SENDER_LOGIN\}" triage; then`, s) + assert.Regexp(t, `is_org_bot "\$\{PR_USER_LOGIN\}" \|\| is_event_actor_authorized "\$\{PR_USER_LOGIN\}" triage; then`, s) + // Third bypass site: pull_request_review -> fix auto-dispatch must + // require specifically the review bot, not any fullsend agent bot. + // Regression test for reverting to the old single-identity + // REVIEW_USER_LOGIN == "${ORG_NAME}-review[bot]" check. + assert.Regexp(t, `is_org_bot "\$\{REVIEW_USER_LOGIN\}" review; then`, s) + // The fix job's own "Pre-fetch review body" step (#5188, #2636) must + // also match both review-bot identities — see reviewBotJQFilter. + assert.Contains(t, s, reviewBotJQFilter) +} + +// reviewBotJQFilter is the dual-identity jq select filter for the shared +// vendor App and self-managed-org review bot, hand-duplicated in +// reusable-fix.yml and reusable-dispatch.yml's "Pre-fetch review body" +// steps because jq can't call the bash is_org_bot helper (#5188, #2636). +const reviewBotJQFilter = `.user.login == \"fullsend-ai-review[bot]\" or .user.login == \"${ORG_NAME}-review[bot]\"` + +// TestReusableFixWorkflowContent validates the "Pre-fetch review body" step +// in reusable-fix.yml checks both review-bot identities. This step had zero +// dedicated test coverage before #5188/#2636 despite its near-identical +// sibling in reusable-dispatch.yml being covered by +// TestReusableDispatchWorkflowContent — a single-identity regression here +// would silently make the fix agent treat every shared-App review as "no +// prior review", with no test catching it. +func TestReusableFixWorkflowContent(t *testing.T) { + content := loadRepoFile(".github/workflows/reusable-fix.yml")(t) + s := string(content) + assert.Contains(t, s, reviewBotJQFilter, + "reusable-fix.yml: Pre-fetch review body step must match both review-bot identities") +} + +// TestIsOrgBotConstantsStayInSync guards against the three hand-copied +// is_org_bot implementations (dispatch-route-helpers.sh, reusable-dispatch.yml, +// and the scaffold dispatch.yml) silently drifting apart. Full byte-equality +// isn't feasible — dispatch.yml intentionally uses a compressed single-line +// style to stay under its workflow-size cap while the other two use an +// expanded style — but the security-relevant constants (the role list and +// the two identity match patterns) must be identical everywhere: a change +// applied to only one or two copies would leave dispatch-route-helpers-test.sh +// green while production silently diverges from what's tested. +func TestIsOrgBotConstantsStayInSync(t *testing.T) { + referenceScript, err := FullsendRepoFile("scripts/dispatch-route-helpers.sh") + require.NoError(t, err) + scaffoldDispatch, err := FullsendRepoFile(".github/workflows/dispatch.yml") + require.NoError(t, err) + reusableDispatch := loadRepoFile(".github/workflows/reusable-dispatch.yml")(t) + + sources := map[string]string{ + "dispatch-route-helpers.sh": string(referenceScript), + "dispatch.yml": string(scaffoldDispatch), + "reusable-dispatch.yml": string(reusableDispatch), + } + + const rolesConst = `FULLSEND_AGENT_ROLES="coder review triage retro prioritize"` + for name, content := range sources { + assert.Contains(t, content, rolesConst, + "%s: FULLSEND_AGENT_ROLES must match the other two copies exactly", name) + } + + // Both identity match patterns must appear in all three copies, in + // whichever quoting style that source uses (compressed single-line + // forms use double-quoted brace expansion the same as the expanded + // forms, so a single pattern per identity covers both styles). + identityPatterns := []string{ + `"fullsend-ai-${role}[bot]"`, + `"${ORG_NAME}-${role}[bot]"`, + } + for name, content := range sources { + for _, pattern := range identityPatterns { + assert.Contains(t, content, pattern, + "%s: missing is_org_bot identity pattern %q", name, pattern) + } + } + + // The want_role guard must reject non-matching roles ("!=") — flipping + // it to "==" (an independent copy/paste slip that wouldn't touch the + // role list or identity patterns above) would make the optional + // role filter match every role EXCEPT the one requested, silently + // inverting `is_org_bot "${REVIEW_USER_LOGIN}" review`'s meaning at + // the pull_request_review -> fix gate. + const wantRoleGuard = `"${role}" != "${want_role}"` + for name, content := range sources { + assert.Contains(t, content, wantRoleGuard, + "%s: is_org_bot's want_role guard must reject non-matching roles, not accept them", name) + } +} + +// TestReviewBotIdentitiesStayInSync guards against the two hand-duplicated +// review-bot login literals (`fullsend-ai-review[bot]` and +// `${ORG_NAME}-review[bot]`) drifting apart across the five places they're +// checked: the three is_org_bot copies covered above (as an authorization +// gate), plus the "Pre-fetch review body" jq filters in reusable-fix.yml +// and reusable-dispatch.yml, plus pre-fetch-prior-review.sh — none of which +// can call the shared is_org_bot helper because jq can't invoke bash +// functions. A change to the vendor App slug or role name could update the +// three is_org_bot copies (caught above) while silently leaving these two +// jq-based lookups stale, reintroducing the single-identity bug fixed by +// #5188/#2636 in exactly the places that fetch review context for the fix +// agent's prompt rather than for dispatch authorization. +func TestReviewBotIdentitiesStayInSync(t *testing.T) { + reusableFix := loadRepoFile(".github/workflows/reusable-fix.yml")(t) + reusableDispatch := loadRepoFile(".github/workflows/reusable-dispatch.yml")(t) + preFetchPriorReview, err := FullsendRepoFile("scripts/pre-fetch-prior-review.sh") + require.NoError(t, err) + + sources := map[string]string{ + "reusable-fix.yml": string(reusableFix), + "reusable-dispatch.yml": string(reusableDispatch), + "pre-fetch-prior-review.sh": string(preFetchPriorReview), + } + for name, content := range sources { + assert.Contains(t, content, "fullsend-ai-review[bot]", + "%s: missing shared vendor-App review bot identity", name) + assert.Contains(t, content, `${ORG_NAME}-review[bot]`, + "%s: missing self-managed-org review bot identity", name) + } + + // Bare identity-literal presence isn't enough for pre-fetch-prior-review.sh: + // unlike the other two sources (guarded by the full "or"-joined + // reviewBotJQFilter literal in TestReusableFixWorkflowContent / + // TestReusableDispatchWorkflowContent), this file passes both identities + // to jq via separate --arg bindings, so the two Contains checks above + // would keep passing even if the filter's "or" were mutated to an + // impossible-to-satisfy "and". Anchor the actual boolean-joined + // comparison so that mutation is caught here too. + assert.Contains(t, string(preFetchPriorReview), `.user.login == $bot1 or .user.login == $bot2`, + "pre-fetch-prior-review.sh: must match either review-bot identity, not require both") } // TestDispatchPerStageAuthorization ensures triage-role users can trigger