feat(auth): label-gated workflow-change elevation#2548
Conversation
Implement ADR 0054 so collaborators can authorize workflow file edits via labels, with fullsend auth check enforcing gates and mint granting workflows:write only through requested elevations. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Site previewPreview: https://725afbf1-site.fullsend-ai.workers.dev Commit: |
|
🤖 Review · |
The mint-token action used jq's empty filter in object construction, which produced no JSON body when elevations were omitted and broke all agent token mints (including triage e2e). Also apply gofmt and fix shellcheck issues in auth harness tests. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ❌ Failure · Started 10:44 AM UTC · Completed 11:00 AM UTC |
ReviewFindingsMedium
Previous runReviewFindingsMedium
Labels: PR modifies mint token service, authorization gates, CLI commands, and agent workflow/script infrastructure for code/fix/triage workflows. Labels: PR modifies dispatch workflows and GCF provisioner embed files alongside mint and authorization components. Previous run (2)ReviewFindingsMedium
Labels: PR modifies mint token service, authorization gates, and agent scripts for code/fix/triage workflows Previous run (3)ReviewFindingsMedium
Labels: PR modifies mint token service, authorization gates, and agent scripts for code/fix/triage workflows Previous run (4)ReviewFindingsHigh
Medium
Low
Labels: PR introduces authorization gate framework for elevated agent permissions, modifies mint service and GitHub App manifests |
Signed-off-by: Barak Korren <bkorren@redhat.com>
Merge upstream main, fix gofmt on appsetup_test.go, use MergeRoleElevations for mint grant logging, adopt resolveToken() in auth/labels CLI commands, and document new commands in cli-internals. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
PR Summary by Qodofeat(auth): label-gated workflow-change elevation Description
Diagram
High-Level Assessment
Files changed (48)
|
|
🤖 Review · |
Update TestBundleEmbeddedMintSource count after adding mintcore/elevations.go to the embedded mint source. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Update validate-output-schema harness after adding authorizations_required to triage-result.schema.json. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
pre-code.sh now runs auth check when GH_TOKEN is set; stub fullsend to pass authorization so PR-search tests keep exercising their path. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Add tests for registry, comment templates, ApplyMutations, pre-push allowed path, and auth exit codes to improve patch coverage. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
ifireball
force-pushed
the
cursor/c4c62cea
branch
from
900ecab to
ca0eeec
Compare
|
🤖 Review · |
Cover apply/sticky posting, mint text output, pre-push denial, stale evaluation paths, and multi-page label timeline lookup. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Review · |
Add pattern matching, stale-check edge case, fake client label, CLI apply, and mint elevation handler tests to improve codecov patch coverage. Signed-off-by: Barak Korren <bkorren@redhat.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
🤖 Finished Review · ✅ Success · Started 9:27 PM UTC · Completed 9:43 PM UTC |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review Squad Report — 6-agent review (round 2)
Agents: 2x claude-coder, 2x claude-researcher, 1x gemini-code-review, 1x cursor-code-review
Models: Claude, Gemini, Codex
3 unique MEDIUM findings posted inline (not covered by prior 10-agent review). See inline comments.
| Contents: "write", | ||
| PullRequests: "write", | ||
| Checks: "read", | ||
| Workflows: "write", |
There was a problem hiding this comment.
[medium] blast-radius (1/6 agents flagged)
Adding Workflows: "write" to the coder app manifest means every org's GitHub App installation for the coder role now has workflows: write as a ceiling permission. While mint only requests this at token-mint time when elevations are passed, the app installation itself has the capability at the org level.
Impact:
- Increases blast radius of a coder app PEM key compromise — attacker gains
workflows: writefor all token mints, not just elevated ones - Existing orgs must re-approve the coder app installation in GitHub org settings after this change
Suggested fix: No code change needed — this is inherent to how GitHub App permissions work (manifest = ceiling, token = scoped). Recommend:
- Document in ADR 0055 that this increases PEM compromise blast radius
- Add to enrollment/upgrade docs that orgs must re-approve the coder app after this rollout
- Consider an admin CLI warning when installed app permissions don't match the manifest
| // New creates a new GitHub client with the given personal access token. | ||
| func New(token string) *LiveClient { | ||
| baseURL := "https://api.github.com" | ||
| if override := strings.TrimSpace(os.Getenv("GITHUB_API_URL")); override != "" { |
There was a problem hiding this comment.
[medium] scope-creep (1/6 agents flagged)
New() now reads GITHUB_API_URL from the environment, overriding the base URL for all ~20 production call sites that use New(). The existing WithBaseURL() was the opt-in mechanism for custom URLs.
If a CI environment sets GITHUB_API_URL for other tools (e.g., GitHub CLI, actions/github-script), all forge API calls silently redirect to that URL. This is a behavioral change with no explicit opt-in at each call site.
Suggested fix: Limit the env override to the call sites that need it (the new auth.go and labels.go CLI commands):
client := gh.New(token)
if u := os.Getenv("GITHUB_API_URL"); u != "" {
client = client.WithBaseURL(u)
}Or, if this is an intentional global knob, add a code comment documenting that it affects all New() callers.
| cmd.Flags().StringVar(&repo, "repo", "", "repository in owner/repo format (required)") | ||
| cmd.Flags().IntVar(&fromNumber, "from", 0, "source issue or PR number (required)") | ||
| cmd.Flags().IntVar(&toNumber, "to", 0, "destination issue or PR number (required)") | ||
| cmd.Flags().StringVar(&prefix, "prefix", "workflow-change-", "copy labels with this prefix") |
There was a problem hiding this comment.
[medium] auth-scope (2/6 agents flagged)
labels copy defaults to prefix workflow-change-, copying both workflow-change-needed AND workflow-change-allowed from the source issue to the destination PR. The copy is mechanical — no staleness check, no collaborator verification on the destination.
This means the PR's fix agent inherits the authorization without separate collaborator approval on the PR itself. If this is intentional (collaborator approved on issue → applies to PR), the scope expansion should be documented.
Additionally, post-code.sh lines 419-423 and 493-496 use || true, silently swallowing copy failures.
Suggested fix:
- If authorization should be issue-scoped only, filter to copy only
workflow-change-needed(notallowed) - If cross-scope is intentional, document this in ADR 0055 under "Authorization Scope"
- Replace
|| truewith explicit error handling that at least logs failures
fullsend-ai-review
Bot
added
requires-manual-review
Signed-off-by: Barak Korren <bkorren@redhat.com>
|
🤖 Finished Review · ❌ Failure · Started 6:09 AM UTC · Completed 6:22 AM UTC |
|
Since PR #2389 moved token minting into the binary, the approach in this PR needs to change so drastically to the point where the code here is irrelevant. We also tried to do too many things here in the same PR which resulted in messy code with many issues that were flagged by the reviewers. Here is a phased plan for fixing #470:
|
|
🤖 Finished Retro · ✅ Success · Started 7:17 AM UTC · Completed 7:25 AM UTC |
Retro: PR #2548 — label-gated workflow-change elevationOutcome: Closed without merging. The author (ifireball) abandoned the PR after recognizing that upstream changes in PR #2389 (moving token minting into the binary) made the approach obsolete, and that the PR tried to do too many things at once. Timeline:
What went well:
Existing issues already cover most improvement opportunities:
One proposal filed for the security review gap between automated and human reviewers. Proposals filed |
2 participants
Closes #470
Summary
workflow-change-needed/workflow-change-allowed.fullsend auth checkin workflows and agent pre/post scripts; mint mechanically mergeselevations(e.g.workflows: write) only when the CLI approves.ready-to-codewhen workflow edits are anticipated; code and fix agents are gated at pre-run and pre-push.Test plan
make go-test(authorization, cli, forge, mintcore, appsetup, dispatch embed sync)bash internal/scaffold/fullsend-repo/scripts/post-triage-test.shbash internal/scaffold/fullsend-repo/scripts/pre-code-auth-test.shbash internal/scaffold/fullsend-repo/scripts/post-code-auth-test.shworkflow-change-neededwithoutready-to-codeworkflow-change-allowed,/fs-code, verify mint elevation and successful workflow push