Skip to content

fix(#5188): recognize org bot actors in dispatch routing #5415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ggallen wants to merge 1 commit into from
Closed

fix(#5188): recognize org bot actors in dispatch routing #5415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 46 additions & 21 deletions .github/workflows/reusable-dispatch.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,15 +128,16 @@ 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
return 1
}
Comment thread
ggallen marked this conversation as resolved.
Comment thread
ggallen marked this conversation as resolved.
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

@fullsend-ai-review fullsend-ai-review Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] workflow-command-injection

Pre-existing (improved): ${username} is interpolated unsanitized into ::warning:: workflow commands in has_repo_permission. GitHub login names are restricted to [a-zA-Z0-9-] plus [bot], preventing injection in practice. The PR improves the situation by adding sed sanitization to the API error output.

rm -f "${api_err}"
return 1
}
Expand All @@ -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-<role>[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}-<role>[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

@ifireball ifireball Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assumes the defaults set of GH apps - people can still use other apps.

@ggallen ggallen Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point — verified this is real: --app-set accepts any string (ValidateAppSet has no format requirement tying it to the org), and docs/guides/infrastructure/advanced-setup.md explicitly documents registering "multiple app sets on the same mint," which by definition can't all equal a single ${ORG_NAME}. Neither this check nor the collaborator-permission-API check it replaces has ever been able to recognize a non-conventional app-set bot (GitHub Apps aren't collaborators regardless of name), so this narrows an existing gap rather than introducing one. Properly closing it means threading the actual configured app-set prefix through as a repo variable at install time (nothing analogous to the existing FULLSEND_MINT_URL/FULLSEND_GCP_REGION variables exists for this yet) — that's installer + scaffold plumbing beyond this PR's routing-bug-fix scope, so I filed #5480 to track it and added a short note to the ADR (pending push, commit ae9930ed) acknowledging the gap where the ${ORG_NAME} assumption is stated.

@ifireball ifireball Jul 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need to recognise bots by name evaporates if we require then to prove their access by having them proactively set labels instead of trying to sniff it via the GH API. This had been our approach for a long time, and I'm not sure why are we diverging from it now. (I wrote this on my main review comment as well, but those seem to always get lost in the noise)

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

@ifireball ifireball Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this still makes string-based assumptions on the bot name, in practice bots can have strange names if people so desire.

@ggallen ggallen Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same root concern as your comment on line 176 above — this pattern is string/name-based by necessity (GitHub Apps aren't identifiable any other way in these event payloads, and performed_via_github_app.id pinning is tracked separately as #5463 for the review-bot-specific gate). The specific gap you're pointing at here — that the name doesn't have to match ${ORG_NAME} at all — is now tracked as #5480, with an ADR note added (commit ae9930ed, pending push) acknowledging that a self-hosted operator can register an app set under any name.

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
Expand Down Expand Up @@ -243,18 +263,21 @@ 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
if [[ "${TRIGGERING_LABEL}" == "ready-for-triage" ]]; then
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"
Comment thread
ggallen marked this conversation as resolved.
fi
Comment thread
ggallen marked this conversation as resolved.
Expand All @@ -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
;;
Expand All @@ -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
Expand Down Expand Up @@ -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 // \"\"" \

@fullsend-ai-review fullsend-ai-review Bot Jul 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] prompt-injection

Pre-existing: the Pre-fetch review body steps match review comments by .user.login string, which on the self-managed org path could be spoofed by a third party squatting the GitHub App slug. This path feeds potentially attacker-authored text into the fix agent prompt. Tracked as #5463 and marked out of scope for #5188/#2636. pre-fetch-prior-review.sh already performs performed_via_github_app.client_id provenance validation downstream.

> "${REVIEW_FILE}" 2>/dev/null || echo "" > "${REVIEW_FILE}"
BYTE_COUNT="$(wc -c < "${REVIEW_FILE}")"
echo "Pre-fetched review body: ${BYTE_COUNT} bytes"
Expand Down
7 changes: 5 additions & 2 deletions .github/workflows/reusable-fix.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 // \"\"" \
Comment thread
ggallen marked this conversation as resolved.
> "${REVIEW_FILE}" 2>/dev/null || echo "" > "${REVIEW_FILE}"
BYTE_COUNT="$(wc -c < "${REVIEW_FILE}")"
echo "Pre-fetched review body: ${BYTE_COUNT} bytes"
Expand Down
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
108 changes: 103 additions & 5 deletions docs/ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

@ifireball ifireball Jul 22, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we (massively) updating an accepted ADR instead of sending-in a new one when we want to make drastic design changes?

@ggallen ggallen Jul 22, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair question. The convention here is append-only dated notes ("Note (YYYY-MM-DD, #issue):" blockquotes) rather than editing the original decision text — this predates this PR: the #5223 note above (2026-07-17) already used this pattern to extend the authorization mechanism (adding the triage permission tier) without touching the accepted body text. This PR's notes follow the same convention to document a platform-constraint exception (bot identities aren't resolvable via the collaborator-permission API this ADR originally specified), not to reverse or rewrite the original decision — repository permissions are still the authorization source in both cases, just resolved two different ways.

That said, I think your broader point stands as a real design question: if the is_org_bot() pattern keeps needing amendment (as this thread and #5480 show), at what point does it stop being a footnote on a permission-check ADR and become its own decision worth a fresh ADR with its own context/consequences section? I don't have a strong view on where that line is, but if #5480's fix ends up being substantial (new install-time plumbing, a repo variable, etc.), that's a reasonable candidate for its own ADR rather than another note here.

(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

Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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`
Expand All @@ -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-<role>[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}-<role>[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}-<anything>[bot]` or
> `fullsend-ai-<anything>[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
Comment thread
ggallen marked this conversation as resolved.
> 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,
Expand Down
Loading
Loading