fix(#5421): authorize stop-fix via collaborator permission API

[waynesun09]

Summary

The stop-fix job in both scaffold shim templates gated the /fs-fix-stop command on author_association (including CONTRIBUTOR). GitHub grants CONTRIBUTOR to anyone with a single merged PR, so any external contributor could comment /fs-fix-stop on another user's PR and disable the fix agent without holding write access — a denial-of-service vector inconsistent with the authorization model established by ADR 0054 (#1688).

Related Issue

Fixes #5421

Changes

• Add a collaborator permission API check to the stop-fix label step in both shim-workflow-call.yaml and shim-per-repo.yaml. The command now requires an admin/maintain/write role, mirroring the has_repo_permission helper in dispatch.yml.
• Preserve the PR-author escape hatch: the PR author may always stop the fix agent on their own PR regardless of permission level.
• Remove CONTRIBUTOR from the if: condition as defense in depth (the runtime API check is authoritative).
• Add TestShimStopFixAuthorization covering both shim templates: no CONTRIBUTOR gate, permission API call present, admin|maintain|write required, author escape hatch preserved, and labeling gated on the authorization result.

Existing repos using the shim will need a reconciliation pass to pick up the fixed templates.

Testing

• make lint passes (staged changes first, then ran)
• Tests added/updated for new or modified logic (go test ./internal/scaffold/ passes)

Checklist

• PR title follows Conventional Commits (correct type, ! for breaking changes)
• Commits are signed off (DCO) — human and human-directed agent sessions only
• I wrote this contribution myself and can explain all changes in it

[qodo-code-review]

PR Summary by Qodo

Harden stop-fix authorization using collaborator permission API

🐞 Bug fix 🧪 Tests 🕐 20-40 Minutes

AI Description

• Prevent unauthorized users from disabling fix agent via /fs-fix-stop comments.
• Authorize stop-fix using GitHub collaborator role_name (admin|maintain|write), per ADR 0054.
• Add tests ensuring both shim templates enforce the new authorization model.

Diagram

graph TD
  A(("/fs-fix-stop comment")) --> B["stop-fix job"] --> C{"PR author?"}
  C -- "yes" --> F["Apply fullsend-no-fix label"]
  C -- "no" --> D[("Collaborator permission API")] --> E{"role in admin|maintain|write?"}
  E -- "yes" --> F
  E -- "no" --> G["Notice + exit"]

  subgraph Legend
    direction LR
    _evt(("Event")) ~~~ _step["Workflow step"] ~~~ _dec{"Decision"} ~~~ _api[("API")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Rely solely on runtime permission check (remove association prefilter)

• ➕ Single authoritative authorization path (API + PR-author check)
• ➕ Avoids any confusion between author_association and actual repo permissions
• ➖ More job invocations for unauthorized commenters (runner cost/noise)
• ➖ Slightly slower feedback due to always starting the job

2. Factor authorization into a shared composite action/snippet

• ➕ One source of truth reused by both shim templates
• ➕ Reduces risk of the two templates drifting over time
• ➖ Adds another artifact/versioning concern for scaffolded repos
• ➖ Still requires careful pinning and reconciliation for existing installs

Recommendation: Current approach is good: keep a lightweight if: prefilter to avoid unnecessary runner usage, but make the collaborator permission API check authoritative before labeling. Consider extracting the bash auth logic into a shared action only if more templates/jobs need the same policy.

Files changed (3) +82 / -2

Bug fix (2) +43 / -2

shim-per-repo.yamlAuthorize stop-fix via collaborator permission API before labeling +21/-1

Authorize stop-fix via collaborator permission API before labeling

• Removes the 'CONTRIBUTOR' author_association gate and adds a runtime authorization check using the GitHub collaborator permission API. Preserves the PR-author escape hatch and exits early with a notice when the commenter lacks admin/maintain/write access.

internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml

shim-workflow-call.yamlMirror stop-fix permission API authorization in workflow_call shim +22/-1

Mirror stop-fix permission API authorization in workflow_call shim

• Applies the same defense-in-depth authorization model as the per-repo shim: no 'CONTRIBUTOR' gating, explicit collaborator permission API check, and PR-author override. Prevents unauthorized users from applying the disabling label.

internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml

Tests (1) +39 / -0

scaffold_test.goAdd regression test ensuring stop-fix auth uses permission API +39/-0

Add regression test ensuring stop-fix auth uses permission API

• Introduces 'TestShimStopFixAuthorization' to validate both shim templates: no 'CONTRIBUTOR' usage, collaborator permission API call present, required roles include admin/maintain/write, PR-author escape hatch exists, and labeling is gated on authorization.

internal/scaffold/scaffold_test.go

[rh-hemartin]

LGTM, makes sense.

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 3:07 PM UTC · Ended 3:16 PM UTC
Commit: 0d7836b · View workflow run →

[github-actions]

Site preview

Preview: https://96f013e8-site.fullsend-ai.workers.dev

Commit: 497dc71f10a101b1d508d20277940106c38b1fa5

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 61 rules

✅ Skills: writing-user-docs, writing-adrs

1. Managed shim still vulnerable ✓ Resolved 🐞 Bug ⛨ Security

Description

The PR updates only the scaffold templates/tests, but the repository’s active managed shim workflow
(.github/workflows/fullsend.yaml) still authorizes stop-fix via author_association (including
CONTRIBUTOR) and does not perform the collaborator permission API check, so the original stop-fix
DoS vector remains in this repo until reconciliation updates that file.

Code

internal/scaffold/scaffold_test.go[R159-194]

+// TestShimStopFixAuthorization verifies the stop-fix job authorizes the
+// /fs-fix-stop command via the collaborator permission API (ADR 0054) rather
+// than author_association. See issue #5421: author_association grants
+// CONTRIBUTOR to anyone with a single merged PR, which let an unauthorized
+// external contributor disable the fix agent on another user's PR.
+func TestShimStopFixAuthorization(t *testing.T) {
+	for _, tmpl := range []string{
+		"templates/shim-workflow-call.yaml",
+		"templates/shim-per-repo.yaml",
+	} {
+		t.Run(tmpl, func(t *testing.T) {
+			content, err := FullsendRepoFile(tmpl)
+			require.NoError(t, err)
+			s := string(content)
+
+			// CONTRIBUTOR must not gate any command — it is granted to anyone
+			// with a single merged PR (the DoS vector from #5421).
+			assert.NotContains(t, s, "CONTRIBUTOR",
+				"stop-fix must not authorize based on the CONTRIBUTOR association")
+
+			// The label step must call the collaborator permission API and
+			// require an admin|maintain|write role (ADR 0054).
+			assert.Contains(t, s, "collaborators/$COMMENT_USER_LOGIN/permission",
+				"stop-fix must check permission via the collaborator API")
+			assert.Contains(t, s, "admin | maintain | write",
+				"stop-fix must require admin/maintain/write access")
+
+			// The PR author retains an escape hatch on their own PR regardless
+			// of their permission level.
+			assert.Contains(t, s, `"$COMMENT_USER_LOGIN" == "$ISSUE_USER_LOGIN"`,
+				"stop-fix must allow the PR author")
+
+			// Unauthorized callers exit before the label is applied.
+			assert.Contains(t, s, `if [[ "$authorized" != "true" ]]; then`,
+				"stop-fix must gate labeling on the authorization result")
+		})

Relevance

⭐⭐ Medium

Repo often updates managed .github/workflows/fullsend.yaml via separate “update shim workflow” PRs;
sync-in-same-PR is inconsistent.

PR-#5198
PR-#3820

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The templates now include the collaborator permission API check, but the repo’s actual managed
workflow still contains the old CONTRIBUTOR gate and lacks any gh api .../permission
authorization, so the fix is not applied to the running workflow here.

internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml[50-102]
.github/workflows/fullsend.yaml[52-83]
internal/scaffold/scaffold_test.go[159-196]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The templates now implement collaborator-permission authorization for `/fs-fix-stop`, but `.github/workflows/fullsend.yaml` (the managed workflow that actually runs in this repo) is still on the old logic and remains vulnerable.

## Issue Context
- Templates were updated, and a new test asserts on embedded templates only.
- The managed workflow file in the repo is explicitly marked as generated from the shim template, but it was not updated in this PR.

## Fix Focus Areas
- internal/scaffold/scaffold_test.go[159-196]
- .github/workflows/fullsend.yaml[52-83]
- internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml[50-102]

## What to change
1. Update/regenerate `.github/workflows/fullsend.yaml` so its `stop-fix` job matches the updated template logic (remove `CONTRIBUTOR` from the `if:` gate and add the collaborator permission API check + PR-author escape hatch).
2. Extend tests to catch this drift, e.g. in `TestShimStopFixAuthorization` also read `.github/workflows/fullsend.yaml` from disk and assert:
  - it does not contain `CONTRIBUTOR`
  - it contains the collaborator permission API call
  - it preserves the PR-author escape hatch

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Permission API errors hidden ✓ Resolved 🐞 Bug ◔ Observability

Description

The new stop-fix authorization suppresses collaborator permission API stderr and converts failures
to an empty role, so transient GitHub API/auth failures will deny even authorized users without any
diagnostic output explaining the failure.

Code

internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml[R82-95]

+          authorized=false
+          if [[ "$COMMENT_USER_LOGIN" == "$ISSUE_USER_LOGIN" ]]; then
+            authorized=true
+          else
+            role=$(gh api "repos/$REPO/collaborators/$COMMENT_USER_LOGIN/permission" \
+              --jq '.role_name' 2>/dev/null) || role=""
+            case "$role" in
+              admin | maintain | write) authorized=true ;;
+            esac
+          fi
+          if [[ "$authorized" != "true" ]]; then
+            echo "::notice::User $COMMENT_USER_LOGIN is not authorized to stop the fix agent (requires write access or PR authorship)"
+            exit 0
+          fi

Relevance

⭐⭐⭐ High

Team previously accepted emitting ::warning:: on collaborator permission API failures
(mktemp+api_err) in dispatch helper.

PR-#1688
PR-#191

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The shim templates discard permission API errors entirely, while the existing dispatch helper
captures and reports permission API failures as warnings, which avoids silent/opaque authorization
failures.

internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml[76-95]
internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml[81-99]
internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[56-70]
PR-#1688

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The stop-fix authorization code uses `2>/dev/null` on the collaborator permission API call and then falls back to `role=""` on errors. This fail-closed behavior is OK for security, but it hides the actual failure reason, making it hard to debug when an authorized user is unexpectedly denied.

## Issue Context
A similar helper (`has_repo_permission`) already exists in the dispatch workflow and captures stderr into a temp file and prints a `::warning::` on failure.

## Fix Focus Areas
- internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml[82-95]
- internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml[87-99]
- internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml[56-77]

## What to change
1. In both shim templates, replace the `2>/dev/null` suppression with captured stderr (e.g., `api_err=$(mktemp) || { echo "::warning::..."; ... }`).
2. If the `gh api` call fails, emit a `::warning::Permission API call failed for $COMMENT_USER_LOGIN: ...` (sanitize/limit output if needed), then continue to fail-closed (treat as unauthorized).
3. Keep the PR-author escape hatch behavior unchanged.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:17 PM UTC · Completed 3:34 PM UTC
Commit: 02dc6bb · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/workflows/fullsend.yaml — This file is under the .github/ protected path. The PR links to issue dispatch: stop-fix shim job still uses author_association instead of collaborator permission API #5421 and explains the rationale (fixing an authorization bypass per ADR 0054), providing sufficient context. Human approval is always required for protected-path changes regardless of context.

Low

• [incomplete-doc] docs/agents/fix.md:107 — States /fs-fix-stop "Requires write-level repository permission" but does not mention the PR author escape hatch (PR author can always stop the fix agent on their own PR regardless of permission level). This staleness pre-dates the PR.
  Remediation: Add a note that the PR author may always use this command on their own PR.

• [incomplete-doc] docs/guides/user/bugfix-workflow.md:71 — Same pre-existing omission of the PR author escape hatch in authorization documentation.
  Remediation: Update to mention the PR author escape hatch.

• [injection] .github/workflows/fullsend.yaml — The ::warning:: message interpolates $(cat "$api_err") without sanitizing for :: sequences. Same pattern exists in all three workflow files and in dispatch.yml's has_repo_permission(). Practical risk is low: ::warning:: can only annotate logs, gh CLI stderr is structured text, and $COMMENT_USER_LOGIN is GitHub-restricted to alphanumeric + hyphens.
  Remediation: For defense-in-depth, sanitize $(cat "$api_err") for :: sequences before interpolation.

• [bash-variable-style] .github/workflows/fullsend.yaml, internal/scaffold/fullsend-repo/templates/shim-workflow-call.yaml — The new bash scripts use unbraced variables ($REPO, $PR_NUMBER) while has_repo_permission() in dispatch.yml uses braced syntax (${username}, ${api_err}). The code comment says "keep the two in sync" — style divergence makes visual comparison harder. Note: the pre-PR code in these files also uses unbraced style, so the new code is internally consistent within each file.

Previous run

Review

Findings

Medium

• [stale-reference] .github/workflows/fullsend.yaml:58 — The fullsend repo's own workflow still uses the vulnerable author_association gating (including CONTRIBUTOR) that this PR removes from the scaffold templates. Lines 59–62 contain the old pattern. The vulnerability from dispatch: stop-fix shim job still uses author_association instead of collaborator permission API #5421 remains exploitable on this repository until reconciliation deploys the fixed template.
  Remediation: Apply the same collaborator permission API authorization to .github/workflows/fullsend.yaml's stop-fix job, or document that reconciliation will auto-fix this after merge.

• [edge-case] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:93 — When mktemp fails, the script emits ::warning:: and does exit 0, skipping authorization entirely. This diverges from has_repo_permission() in dispatch.yml which uses return 1 (deny) on mktemp failure, letting the caller decide. The exit 0 is fail-closed (label not applied), but provides no user feedback about the denial — unlike the normal unauthorized path which emits a ::notice::.
  Remediation: Consider falling through to the authorized != true check instead of exit 0, so the user gets the standard 'not authorized' notice.

Low

• [bash-variable-style] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The new bash script uses unbraced variables ($COMMENT_USER_LOGIN, $REPO) while has_repo_permission() in dispatch.yml consistently uses braced syntax (${username}, ${GITHUB_REPOSITORY}). The code comment says 'keep the two in sync' — style divergence makes visual comparison harder.

• [bash-case-pattern-spacing] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml:99 — The case pattern admin | maintain | write) has spaces around pipe operators, while dispatch.yml uses admin|maintain|write) without spaces.

• [incomplete-doc] docs/agents/fix.md:107 — States /fs-fix-stop 'Requires write-level repository permission' but does not mention the PR author escape hatch. This staleness pre-dates the PR (the escape hatch was already in the old job-level if:).

• [incomplete-doc] docs/guides/user/bugfix-workflow.md:71 — Same pre-existing omission of the PR author escape hatch in authorization documentation.

• [injection] internal/scaffold/fullsend-repo/templates/shim-per-repo.yaml — The ::warning:: message interpolates $(cat "$api_err") without sanitizing for :: sequences. Practical risk is low (gh CLI stderr is structured, ::warning:: has limited blast radius). Same pattern exists in both shim templates.

• [missing-test] internal/scaffold/scaffold_test.go — TestShimStopFixAuthorizationRuntime covers 4 scenarios but not the mktemp failure path.

---

Labels: PR fixes authorization logic in dispatch shim templates (component/dispatch) and is a bug fix (bug)

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:43 PM UTC · Completed 3:58 PM UTC
Commit: 497dc71 · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 2:22 PM UTC · Completed 2:36 PM UTC
Commit: 497dc71 · View workflow run →

[fullsend-ai-retro]

Retro: PR #5467 — authorize stop-fix via collaborator permission API

Timeline: Issue #5421 was filed 2026-07-21 reporting that the stop-fix shim job authorized /fs-fix-stop via author_association (including CONTRIBUTOR), allowing any external contributor with a single merged PR to disable the fix agent on another user's PR — a DoS vector violating ADR 0054. The triage agent classified it as Medium/Security and applied the triaged label (not ready-to-code), correctly routing it to a human for implementation. waynesun09 authored the fix manually, opening PR #5467 on 2026-07-22. The review agent ran three times: the first was cancelled (~10 min) when a new commit superseded it (expected concurrency behavior), the second found 2 Medium + 6 Low findings, and the third confirmed the Mediums were resolved. The PR merged 2026-07-23.

What went well:

• Triage agent correctly classified the security issue and routed to human implementation (security issues should not be auto-coded).
• Review agent caught two substantive Medium findings: (1) the repo's own managed workflow .github/workflows/fullsend.yaml was not updated alongside the templates, leaving the vulnerability exploitable on this repo; (2) the mktemp failure path did exit 0 (silently skipping authorization) instead of the fail-closed deny pattern used in dispatch.yml. Both were real security-adjacent bugs.
• qodo-code-review independently flagged the managed workflow gap, providing defense-in-depth.
• Author was responsive — fixed all Medium findings within ~8 minutes and added TestManagedShimStopFixNotStale as a regression guard against template-to-managed-workflow drift.
• Re-review tracking worked correctly: the sticky comment system preserved the first run's findings in a collapsed "Previous run" section, and the second run's findings accurately reflected which issues were resolved.
• Zero false positives — all findings were substantive and actionable.

Concerns identified (covered by existing issues):

1. Human approval preceded agent review. rh-hemartin approved 92 seconds after PR creation, before the review agent ran. The agent subsequently found 2 Medium issues. After the author pushed fixes, the PR was author-merged without a fresh human re-review. Existing issues cover this concern: #3273 (dismiss prior approval when re-review finds Medium+ issues) and #2099 (alert when human approves with unresolved Medium+ findings). This PR provides evidence that the timing gap between human approval and agent review can result in Medium+ findings being merged without human re-examination — relevant context for both issues.

2. Cancelled first review run (~10 min token cost). The first review was nearly complete when cancelled by a new commit push. Existing issues #5139 (same-commit review dedup) and #1014 (debounce review dispatch on rapid synchronize events) cover this optimization.

No new proposals. The workflow executed well across all stages. The review agent outperformed the initial human review on this PR, and the concerns identified are already tracked in existing issues.