feat(#6042): support Prow OWNERS file authorization for slash commands - #6051
feat(#6042): support Prow OWNERS file authorization for slash commands#6051RaphaelBut wants to merge 11 commits into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoSupport opt-in Prow OWNERS authorization for slash-command dispatch
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Adding more bash is always technical debt (we're on a journey to move much of it into the go binary), but - that said, the cli entrypoint you'd need to use here (imagine - a new I think we should file a backlog issue to centralize that in a new subcommand and replace this - meaning that, I think I'm ok with moving forwards with adding more bash here for now. I'd love to hear from others in @fullsend-ai/core before moving forwards though. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Multi-model review (Claude ×2 + Grok, with maintainer verification). Review-only — no approve/request-changes action. Posting the unique MEDIUM-and-above findings inline; qodo's #4 (givenOwnersAliasesFile missing OwnersAuthActivated) and #5 (resetScenarioWorld doesn't reset it) are confirmed and deepened — a stale/true flag makes later scenarios commit empty OWNERS files to their leased slot — so they're not re-posted here. qodo #1 (guide placement) is a false positive: there is no docs/guides/admin/ and the file pre-dates this PR.
The gate itself is sound
The has_repo_permission / _owners_has_user bash was traced by all three reviewers and executed against yq v4 with injection probes: username (^[a-zA-Z0-9-]+$) and alias-entry (^[a-zA-Z0-9_-]+$) are validated before any yq interpolation, key is always a literal, it fails closed on every malformed input, and the ;;& fallthrough correctly gives approvers write+triage while reviewers can never reach write. The inline-bash choice is not itself a defect — @RaphaelBut, your instinct to ask was right, and the answer is that the logic is safe as written.
Must-fix before merge
- H1 — the
pull_request_reviewcheckout ref makes the "cannot self-authorize" invariant false on that path (workflow-level; not fixed by any refactor). One-line pin fix inline. - H2 — the OWNERS-auth e2e scenarios exercise the auth path in zero cases and have never run on this fork PR. Inline.
- M1 — case-sensitive matching silently denies the non-collaborator Prow users this feature targets. Inline.
Direction — bash vs. Go package vs. CEL
This is the real decision, and it needs a core-team call (see @ifireball below). Summary of the analysis:
- CEL is not reachable for this feature yet. CEL authorization runs host-side in Go, but only in the
harness-dispatchjob for custom agents, and its env exposes onlyevent(no OWNERS data / API). The built-in stages this PR targets (triage/code/fix/review) are routed by the bashRoutejob, which is checkout + one bash step and has nofullsendbinary. Routing OWNERS auth through CEL would first require migrating built-in stages onto harness-dispatch — the "default agents not on CEL" gap. - The Go package is the right home for the logic (parsing, alias resolution, actor→role, policy): typed, unit-testable, and it makes M1 and the M5/typo gap correct by construction. Its durable consumer is the
IsAuthorized/ normalized-event layer that already runs Go — after the built-in-stages migration. - But wiring a
fullsend owners-checksubcommand into theRoutejob now would add a binary install/build step to the highest-frequency path (every comment/label/review across all repos), and it's throwaway once the migration lands — the exact cost @RaphaelBut flagged in the PR description.
Recommended sequencing: land this as the reviewed bash interim after H1 + H2 + M1; extract internal/owners/ (logic + tests) as the immediate follow-up so the resolver exists and is reusable; let the built-in-stages→harness-dispatch/CEL migration — not this PR — be what moves the call site into Go. That path fixes the review's real concerns without gating a first contribution behind a core dispatch-path migration.
@ifireball — flagging you as fullsend core / CEL owner
The substantive open question here isn't the bash correctness (verified) but the direction above: whether OWNERS authorization should be built toward the CEL / harness-dispatch authorization path — and whether/when built-in stages migrate onto it — or ship as the interim bash gate and be revisited during that migration. That's your call as CEL feature owner; the recommendation above is a proposal, not a decision. H1 (the security-invariant/checkout-ref fix) also warrants a core-team eye since it touches the dispatch trust boundary.
7621199 to
4c93fe3
Compare
|
Thanks @waynesun09 and @ralphbean for the thorough review! Highlights beyond what's visible in the diff:
|
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep — no approve/request-changes action taken. Posting one additional finding not already covered by existing review threads; the OWNERS-auth CI-coverage gap I'd flagged is already tracked in more depth by the existing HIGH comment on e2e/behaviour/features/dispatch/owners-auth.feature (skipped as a substantive duplicate).
|
im working on the OWNERS-auth CI-coverage gap |
Site previewPreview: https://a843ed08-site.fullsend-ai.workers.dev Commit: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Approving. Re-reviewed at 62efb214: H1 (pull_request_review checkout ref) is fixed in both workflow files, and M1/M3/M4/M5 plus the config-setter finding are all resolved — with scaffold parity and unit tests added on top.
I also verified the OWNERS grant/deny logic executes correctly against the shipped bash locally: approver→write, reviewer→triage-only (no escalation), alias resolution, case-insensitive matching, and injection blocked. The ;;& privilege boundary is now pinned by a running test across both workflow files.
Only remaining item is live behaviour/e2e coverage, which is fail-closed and non-blocking — fine as a follow-up alongside the CI-coverage work you're already on. Nice work for a first contribution.
ifireball
left a comment
ifireball
left a comment
There was a problem hiding this comment.
Needs to add an implementation of this to the custom agent dispatch path as well (inside the Go binary)
To @waynesun09 about wither CEL expressions should come into play - wither the hard-wired auth will be replaced by the expressions is an open question (has to do with wither we ever want to allow agent use by less privileged actors). I do not think it is in scope for this.
| // AuthorizationConfig controls opt-in authorization mechanisms that | ||
| // extend the default collaborator-API permission check. | ||
| type AuthorizationConfig struct { | ||
| OwnersFile bool `yaml:"owners_file,omitempty"` |
There was a problem hiding this comment.
Perhaps define this in a more flexible way with:
authorization:
- provider: native # what we do now
- provider: owners_file
- provider: LDAP # future speculation
ldap-url: ...
Where the checks AND all providers, and the default is to use the native.
There was a problem hiding this comment.
I looked a bit at this part and the provider list is definitely the right way to go.
Right now the native collaborator API always runs as a fallback. The owners path is an additional check on top, not a replacement. The boolean flag does match the behavior: "also check owners" rather than "switch to owners instead of native."
It's not pretty but it's honest about what the code does. Happy to rework it to the list schema in this PR if you want, or we can land as-is and move this to #6072 when the Go migration removes the bash side and a second provider makes sense to be added. Let me know which you'd prefer.
There was a problem hiding this comment.
I would be happy if we can do the list now, to save us user migration and deprecation headache later. We can define that whatever is in the list is always on top of the native auth if that makes more sense. (I was under the impression that since Prow can do stuff on behalf of the users it can effectively bypass the native auth)
There was a problem hiding this comment.
Thanks! That makes a lot of sense, I did not think about the migration :D
Added the provider list in 58aa53c and updated documentation accordingly.
For now, native auth is the implicit fallback, not a provider in the list, meaning owners is checked first, if it authorizes the user, the collaborator API is not consulted.
If the user isn't in owners, it falls through to native auth.
authorization:
- provider: owners_file
And the default (no list / empty list) is just collaborator API:
# No authorization list — collaborator API only
authorization: []
Let me know what you think!
|
Thank you so much for all of your reviews <3 Pushed 659e568 to adress review comments and add a denial test, although I am not quite sure if we should go a bit further still with adding more tests around this.
If you decide this PR is far enough to let the CI run, we can take a look at how my tests break :D |
ralphbean
added
the
ok-to-test
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only sweep (multi-agent, verified against the live PR head and CI). No approve/request-changes action taken. Findings below are all newly introduced or newly-relevant as of the final commit (659e568c) and were checked for overlap with existing review threads before posting; the write-denial CI failure was reproduced live against the exact head SHA.
| And the agent will succeed to Prove execution | ||
| And the triage workflow logs contain "authorized via OWNERS file (reviewer" | ||
|
|
||
| Scenario: OWNERS reviewer is denied write-level access |
There was a problem hiding this comment.
CRITICAL — CI is red on the current head; the write-denial security-invariant test is actually failing
Verified live: the behaviour check on head 659e568c (run 31616854589) has conclusion=failure. The raw job log shows this exact scenario failing with after scenario hook failed: dispatch run logs unexpectedly contain OWNERS authorization — i.e. the E2E run observed the outsider's reviewer-only OWNERS entry being treated as authorized for a write-level command, which is exactly the escalation this scenario exists to disprove.
Three other new OWNERS scenarios also fail (artifact fullsend-triage not found), though those look like they may share a common upstream flake (a harness-agent workflow run failing before producing an artifact) rather than being a second independent bug. This has not been fixed/re-run since; it is the current, unresolved state of the exact head SHA of the open PR.
Suggestion: Do not merge until the behaviour suite is green on this exact head. Root-cause the reviewer-denial failure specifically (check for state leakage between godog scenarios in a shared repo slot) with a clean, isolated repro before re-requesting review.
There was a problem hiding this comment.
The root cause was that dummy agents wrote to custom output paths (e.g. output/owners-ok.json)
but the triage validation script expects output/agent-result.json with triage-schema content. In addition the log assertion was matching to widely.
Commit 08c93cc fixed this "fix OWNERS E2E tests -- wrong fixture path and log assertions matching source display"
The CI run after that commit were successful for this failing test.
State leakage is of critical concern, so I added a check for stale config before any scenario starts. See ValidateSlotClean added in commit 8670676
Needs a behaviour CI run on the current head to confirm green.
| w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) | ||
| } | ||
|
|
||
| func requireOutsider(w *world.World) error { |
There was a problem hiding this comment.
MEDIUM — OWNERS write-denial E2E scenario has no capability-skip guard; will hard-fail rather than skip when the outsider PAT is unavailable
Verified: requireOutsider() (lines 194-198) returns a hard fmt.Errorf("TEST_ACTOR_OUTSIDER_PAT not set") when the PAT is empty, and the "OWNERS reviewer is denied write-level access" scenario (owners-auth.feature:50-55) that depends on it carries no tag at all. This repo has an established mechanism for exactly this situation — scenarios tagged @requires:capability:<name> are skipped via godog.ErrSkip in SkipErrorForTagNames (pkg/behaviourtest/suite/init.go) when the capability is not declared via BEHAVIOUR_CAPABILITIES (confirmed precedent: @requires:capability:applier-branch-namespace in branch-namespace.feature) — but no outsider-pat capability was ever wired into env.RunnerConfig/HasCapability, and this scenario does not use the tag. If TEST_ACTOR_OUTSIDER_PAT is ever unset in a given CI context, this scenario fails the whole suite instead of skipping cleanly.
Suggestion: Add a @requires:capability:outsider-pat tag to the scenario and extend env.LoadRunnerConfig/HasCapability to report that capability based on TEST_ACTOR_OUTSIDER_PAT presence, consistent with the existing skip pattern.
There was a problem hiding this comment.
Tagging this with requires:capability:outsider-pat would allow the write-denial test to potentially be silently skipped.
IIUC hard-fail is preferred here unless there is a CI guardrail that catches silently-skipped capability scenarios?
|
@RaphaelBut rebase is needed to resolve conflicts |
ralphbean
left a comment
ralphbean
left a comment
There was a problem hiding this comment.
Nice work carrying this through so many rounds of review — the OWNERS/alias resolution logic and the new Go package are solid, and the earlier H1/H2/M1 findings all look genuinely fixed.
One thing I noticed poking around outside the diff: route and harness-dispatch now pin their config checkout to the base SHA for pull_request_review events, but fix (and triage/code/review/retro/prioritize/harness-run) still only pin for pull_request_target. fix is the one stage pull_request_review actually dispatches, so its kill-switch/config checks can still read from the PR head there. It's not part of this diff so I can't point at a line directly, but it'd be worth fixing.
A couple more notes inline.
659e568 to
75cb622
Compare
github-actions
Bot
removed
the
ok-to-test
|
Thanks again! Rebased on main and addressed all review comments in a single commit. I humbly request another CI run :D Summary of changes: Security fixes:
Test hardening:
Code:
Docs:
Deferred: Provider-list config schema tracked in #6072. |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (no approve/request-changes action) — one additional MEDIUM finding not covered by the existing threads on this PR.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review-only pass (no approve/request-changes action taken) — 2 findings not covered by existing threads on this PR.
ralphbean
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
|
Re-assigning to author to deal with comments. |
rh-hemartin
added
the
ok-to-test
github-actions
Bot
removed
the
ok-to-test
…ash commands Add an opt-in OWNERS-file authorization path to has_repo_permission in reusable-dispatch.yml. When authorization.owners_file is set to true in .fullsend/config.yaml, the dispatch routing checks OWNERS and OWNERS_ALIASES before falling back to the GitHub collaborator API. Approvers get write-equivalent access; reviewers get triage-equivalent. Sparse-checkout pins to the base branch SHA to prevent PR-based self-authorization. Username and alias entry names are validated before yq interpolation. Audit notices are emitted on every OWNERS-granted authorization. Includes five e2e behaviour scenarios (direct approver, alias resolution, reviewer triage, reviewer write denial, opt-in gate) and documentation updates to ADR 0054 and the layered config reference. Limitations: - OWNERS auth applies to built-in stages only; harness agents are unaffected (they resolve roles via Go code in ghaevent.go). - OWNERS-authorized users without GitHub write access will use the fork PR path via commit.go, requiring /ok-to-test for CI. - Authorization logic is implemented in bash/yq rather than Go. See PR description for discussion and migration path. Signed-off-by: RaphaelBut <rbut@redhat.com>
…zation Pin checkout ref to base SHA for pull_request_review events, closing a self-authorization gap. Switch E2E scenarios to issues.opened trigger so they exercise has_repo_permission. Add case-insensitive OWNERS matching via lc_user without leaking lowercase into the API fallback. Mirror OWNERS auth into scaffold dispatch.yml for parity. Replace yaml.Node config manipulation with SetAuthorizationOwnersFile on the config writer, matching the SetKillSwitch pattern. Add workflow alignment assertions for the role-mapping security invariant. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: RaphaelBut <rbut@redhat.com>
…Authorization struct SetAuthorizationOwnersFile(false) was niling the entire Authorization pointer, which would silently wipe future sibling fields. Now clears only OwnersFile and nils the struct only when all fields are zero. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: RaphaelBut <rbut@redhat.com>
…g, and E2E denial test Add internal/owners/ package with OWNERS file parser, alias resolver, case-insensitive role mapping, and username validation matching the bash regex guard (18 unit tests). Wire into harnessdispatch.Dispatch to upgrade actor role before authorization when owners_file is enabled. Pin harness-dispatch checkout ref to base SHA for pull_request_review events and add OWNERS to its sparse-checkout. Add TestOwnersCheckoutRefPin to catch future checkout-without-pin bugs. Add AuthorizationOwnersFile() accessor to ConfigReader. Simplify behaviour step definitions with shared helpers and parameterized role step. Apply clock-skew buffer to issue-open trigger timestamp. Add outsider-driven E2E scenario proving OWNERS reviewer cannot escalate to write-level access, with ActorLogin filtering and dual log-line assertions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: RaphaelBut <rbut@redhat.com>
…t hardening, docs Pin all 9 checkout refs for pull_request_review events to base SHA, closing a pre-existing gap where downstream stage jobs read config from the PR merge commit instead of the trusted base branch. Harden E2E tests: switch scenarios 1-3 from the bot to the outsider identity so authorization succeeds only through OWNERS (not the API fallback). Add a collaborator-fallthrough scenario proving unlisted collaborators are still authorized via the API. Rewrite the denial test to use WaitForWorkflow with the standard retry pattern instead of a custom polling loop. Parameterize step definitions by actor (bot|outsider) to eliminate duplicated functions. Resolve OWNERS relative to filepath.Dir(ConfigDir) instead of bare working-directory paths. Avoid mutating the callers event -- the OWNERS-upgraded role is used only for the IsAuthorized gate, not leaked to downstream CEL evaluation. Anchor sparse-checkout to /OWNERS and /OWNERS_ALIASES. Update ADR 0054, layered-config reference, and workflow contracts. Add Dispatch integration tests and a no-parent-fallback unit test. Document the v1 flat-schema limitation. Provider-list config schema deferred to fullsend-ai#6072. Signed-off-by: RaphaelBut <rbut@redhat.com>
… log assertions matching source display Dummy agents wrote to custom output paths (e.g. output/owners-ok.json) but the triage validation script expects output/agent-result.json with triage-schema content. Log assertions matched "OWNERS file resolved user" in GitHub Actions' bash source code display, not just runtime output -- use ##[notice] prefix and expanded parameter values to distinguish. Drop the "No stage matched" echo-filtering heuristic from the denial scenario. The heuristic was brittle and inconsistent with the ##[notice] approach. The assertion tested a test-environment precondition (outsider has no collaborator access), not the feature under test (OWNERS reviewer cannot escalate to write). The ##[notice] negative check alone is sufficient. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: RaphaelBut <rbut@redhat.com>
…, fix E2E actor
Refactor the authorization config from a boolean field to a provider
list to avoid future migrations:
authorization:
- provider: owners_file
The config interfaces (AuthorizationOwnersFile/SetAuthorizationOwnersFile)
keep the same signatures -- only the storage format changes. Native
collaborator-API auth remains implicit and always runs; the list names
additional providers. Includes validation for unknown/duplicate providers.
Fix the two failing OWNERS E2E scenarios ("Unlisted collaborator falls
through to API authorization" and "Triage dispatches without OWNERS path
when not opted in") by switching from the bot actor to the write actor
(TEST_ACTOR_WRITE_PAT). The bot cannot pass has_repo_permission because
GitHub App bots are not collaborators and their [bot] username fails the
OWNERS regex. The write actor has write-level collaborator access but is
not in OWNERS, correctly testing the API fallthrough path.
Also removes the unused ActorLogin field from forge.WorkflowRun and
fixes a stale claim in workflow-contracts.md.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: RaphaelBut <rbut@redhat.com>
Signed-off-by: RaphaelBut <rbut@redhat.com>
…ness dispatch Options Signed-off-by: RaphaelBut <rbut@redhat.com>
…cenario OWNERS auth cleanup failure can silently grant authorization in unrelated scenarios, causing false positives. Validate repo slot config is clean before every scenario so stale state fails loudly. Signed-off-by: RaphaelBut <rbut@redhat.com>
Signed-off-by: RaphaelBut <rbut@redhat.com>
8670676 to
55666c3
Compare
5 participants
Summary
Adds opt-in Prow OWNERS file authorization for slash command dispatch. Repos that use Prow/OWNERS instead of GitHub collaborator roles can add
owners_fileto theauthorizationproviders list in.fullsend/config.yamlto letOWNERS-listed users trigger fullsend agents without needing GitHub write/triage collaborator access. Approvers get write-equivalent access; reviewers get triage-equivalent only. OWNERS is checked first; if it authorizes the user,
the collaborator API is not consulted. If the user isn't in OWNERS, it falls through to the collaborator API.
Changes
fallthrough mapping the role hierarchy
filepath.Dir(ConfigDir), not bare working-directory paths
workflow contracts documents the OWNERS sync contract and parity test coverage
Known limitations
Related
Testing
Checklist