From b3a776d5b8f690fb582d095874828d6d548f9eea Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Mon, 10 Aug 2026 19:55:25 +0200 Subject: [PATCH 01/12] feat(#6042): support Prow OWNERS file authorization for slash 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 --- .github/workflows/reusable-dispatch.yml | 56 +++- ...thorization-on-all-agent-dispatch-paths.md | 15 + .../layered-config-reference.md | 24 ++ .../features/dispatch/owners-auth.feature | 65 ++++ pkg/behaviourtest/steps/cleanup.go | 5 + pkg/behaviourtest/steps/owners.go | 289 ++++++++++++++++++ pkg/behaviourtest/steps/registry.go | 1 + pkg/behaviourtest/world/world.go | 5 + 8 files changed, 459 insertions(+), 1 deletion(-) create mode 100644 e2e/behaviour/features/dispatch/owners-auth.feature create mode 100644 pkg/behaviourtest/steps/owners.go diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 9fb229c708..ee66febf2e 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -128,7 +128,10 @@ jobs: ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} - sparse-checkout: .fullsend/config.yaml + sparse-checkout: | + .fullsend/config.yaml + OWNERS + OWNERS_ALIASES sparse-checkout-cone-mode: false - name: Determine stage @@ -159,11 +162,62 @@ jobs: STAGE="" TRIGGER_SOURCE="" + # Check direct membership or alias membership in an OWNERS list. + # Used by has_repo_permission when OWNERS-file auth is enabled. + # yq errors are intentionally suppressed (&>/dev/null): a parse + # failure or missing key means "no match", falling through to + # the collaborator API — not a fail-open gate. + _owners_has_user() { + local key="${1}" user="${2}" + if yq -e ".${key}[] | select(. == \"${user}\")" OWNERS &>/dev/null; then + return 0 + fi + if [[ -f "OWNERS_ALIASES" ]]; then + local entry + while IFS= read -r entry; do + [[ -z "${entry}" ]] && continue + [[ ! "${entry}" =~ ^[a-zA-Z0-9_-]+$ ]] && continue + if yq -e ".aliases.\"${entry}\"[] | select(. == \"${user}\")" OWNERS_ALIASES &>/dev/null; then + return 0 + fi + done < <(yq ".${key}[]" OWNERS 2>/dev/null) + fi + return 1 + } + # 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 [[ -z "${username}" ]] && return 1 + + # OWNERS-file authorization (opt-in via authorization.owners_file in config.yaml). + # Approvers get write-equivalent access; reviewers get triage-equivalent. + # Safe: sparse-checkout pins to base branch SHA, so PR authors cannot + # self-authorize by adding themselves to OWNERS. + if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then + if [[ "$(yq '.authorization.owners_file // false' .fullsend/config.yaml)" == "true" ]]; then + if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then + echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 + else + case "${min}" in + write|triage) + if _owners_has_user approvers "${username}"; then + echo "::notice::User '${username}' authorized via OWNERS file (approver, requested: ${min})" + return 0 + fi + ;;& + triage) + if _owners_has_user reviewers "${username}"; then + echo "::notice::User '${username}' authorized via OWNERS file (reviewer, requested: ${min})" + return 0 + fi + ;; + esac + fi + fi + fi + api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 return 1 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 13a3e59873..0aedf1b10b 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 @@ -189,6 +189,21 @@ 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-08-10, [#6042](https://github.com/fullsend-ai/fullsend/issues/6042)):** +> Prow-based repositories (e.g., OpenShift) use OWNERS files rather than +> GitHub collaborator roles to define contributor authority. +> `has_repo_permission` now supports an opt-in OWNERS-file authorization +> path: when `authorization.owners_file: true` is set in +> `.fullsend/config.yaml`, the function checks the repo-root `OWNERS` +> (and `OWNERS_ALIASES`) before falling back to the collaborator API. +> OWNERS approvers get write-equivalent access; reviewers get +> triage-equivalent. The sparse-checkout pins to the base branch SHA, so +> PR authors cannot self-authorize by modifying OWNERS in their PR. +> This follows the extension path above (extending the allowed permission +> sources in `has_repo_permission`) rather than bypassing the check. +> OWNERS auth applies to built-in stages only; harness agents are +> unaffected. + ## Consequences - All dispatch paths require write-level repository permission, diff --git a/docs/guides/infrastructure/layered-config-reference.md b/docs/guides/infrastructure/layered-config-reference.md index 4231770f3d..446476e1cd 100644 --- a/docs/guides/infrastructure/layered-config-reference.md +++ b/docs/guides/infrastructure/layered-config-reference.md @@ -90,6 +90,7 @@ the overlay → base → code defaults chain. | `models.aliases` | `map[string]string` (nested) | Per-key merge | `nil` (fleet defaults) | | `create_issues` | `*CreateIssuesConfig` | Replace whole object if set | `nil` | | `status_notifications` | `*StatusNotificationConfig` | Replace whole object if set | `nil` | +| `authorization` | `object` | Replace whole object if set | `nil` | ### Per-agent `runtime`, `model`, `effort` on `agents:` entries @@ -366,6 +367,29 @@ The `status_notifications` field uses the same replace-if-set semantics as - Non-nil — replaces the parent value entirely, including nested `comment.start`/`comment.completion` settings. +### `authorization` — replace whole object if set + +The `authorization` field controls alternative authorization backends +for the dispatch workflow's `has_repo_permission` check. Currently +supports one sub-field: + +- `owners_file` (`bool`, default `false`) — when `true`, the dispatch + routing logic checks the repo-root `OWNERS` and `OWNERS_ALIASES` + files before falling back to the GitHub collaborator API. OWNERS + approvers get write-equivalent access; reviewers get + triage-equivalent. See [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md) + for details. + +Example: + +```yaml +authorization: + owners_file: true +``` + +This field is read by `yq` in the workflow bash routing logic, not by +the Go config package. It is only meaningful in per-repo mode. + ## Code defaults reference When neither the overlay nor the base layer sets a field, the following diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature new file mode 100644 index 0000000000..786009a5c0 --- /dev/null +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -0,0 +1,65 @@ +Feature: OWNERS file authorization for bash routing + + Verify that the OWNERS-file authorization path fires when + authorization.owners_file is enabled in config.yaml. The e2e bot + already has collaborator access, so these scenarios confirm the + OWNERS code path is reached (via audit log) rather than testing + the fallback denial path (which requires a restricted identity). + + Background: + Given the enrolled test repository + + Scenario: Triage dispatches via OWNERS approver path when enabled + Given an OWNERS file listing the bot as an approver + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/owners-ok.json, fixtures/dispatch/ok.json | + And an issue + When the issue is labeled "ready-for-triage" + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "authorized via OWNERS file" + + Scenario: OWNERS alias resolves to grant access + Given an OWNERS file with alias "test-team" as approver + And an OWNERS_ALIASES file mapping "test-team" to the bot + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/owners-alias-ok.json, fixtures/dispatch/ok.json | + And an issue + When the issue is labeled "ready-for-triage" + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "authorized via OWNERS file (approver" + + Scenario: OWNERS reviewer can triage but not code + Given an OWNERS file listing the bot as a reviewer only + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/owners-rev-ok.json, fixtures/dispatch/ok.json | + And an issue + When the issue is labeled "ready-for-triage" + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs contain "authorized via OWNERS file (reviewer" + + Scenario: OWNERS reviewer is not granted write-level access via OWNERS + Given an OWNERS file listing the bot as a reviewer only + And OWNERS authorization is enabled + And an issue + When the OWNERS auth test posts "/fs-code" on the issue + Then the dispatch run logs do not contain "authorized via OWNERS file" + + Scenario: Triage dispatches without OWNERS path when not opted in + Given an OWNERS file listing the bot as an approver + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/owners-off-ok.json, fixtures/dispatch/ok.json | + And an issue + When the issue is labeled "ready-for-triage" + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs do not contain "authorized via OWNERS file" diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index cef4bbedd9..f23d6f9ca8 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -244,6 +244,11 @@ func CleanupScenario(w *world.World) { } } + // --- OWNERS auth cleanup --- + if w.OwnersAuthActivated { + cleanupOwnersAuth(w) + } + // --- Reaction notification cleanup --- // Disable reaction notifications so the next scenario on this slot // is not affected by sticky config state. diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go new file mode 100644 index 0000000000..dfe5bae209 --- /dev/null +++ b/pkg/behaviourtest/steps/owners.go @@ -0,0 +1,289 @@ +package steps + +import ( + "context" + "fmt" + "path/filepath" + "strings" + "time" + + "github.com/cucumber/godog" + + "github.com/fullsend-ai/fullsend/internal/forge" + gaci "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci/githubactions" + scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" +) + +func registerOwnersSteps(sc *godog.ScenarioContext) { + sc.Step(`^an OWNERS file listing the bot as an approver$`, func(ctx context.Context) (context.Context, error) { + return ctx, givenOwnersFileWithBot(world.FromContext(ctx)) + }) + sc.Step(`^an OWNERS file with alias "([^"]+)" as approver$`, func(ctx context.Context, alias string) (context.Context, error) { + return ctx, givenOwnersFileWithAlias(world.FromContext(ctx), alias) + }) + sc.Step(`^an OWNERS_ALIASES file mapping "([^"]+)" to the bot$`, func(ctx context.Context, alias string) (context.Context, error) { + return ctx, givenOwnersAliasesFile(world.FromContext(ctx), alias) + }) + sc.Step(`^an OWNERS file listing the bot as a reviewer only$`, func(ctx context.Context) (context.Context, error) { + return ctx, givenOwnersFileWithBotReviewerOnly(world.FromContext(ctx)) + }) + sc.Step(`^OWNERS authorization is enabled$`, func(ctx context.Context) (context.Context, error) { + return ctx, givenOwnersAuthEnabled(world.FromContext(ctx)) + }) + sc.Step(`^the triage workflow logs contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { + return ctx, thenWorkflowLogsContain(world.FromContext(ctx), needle) + }) + sc.Step(`^the triage workflow logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { + return ctx, thenWorkflowLogsDoNotContain(world.FromContext(ctx), needle) + }) + sc.Step(`^the OWNERS auth test posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { + return ctx, whenSlashCommandPosted(world.FromContext(ctx), command) + }) + sc.Step(`^the dispatch run logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { + return ctx, thenDispatchRunLogsDoNotContain(world.FromContext(ctx), needle) + }) +} + +func givenOwnersFileWithBot(w *world.World) error { + ghDriver, ok := w.SCM.(*scmgh.Driver) + if !ok { + return fmt.Errorf("OWNERS test requires GitHub SCM driver") + } + botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) + if err != nil { + return fmt.Errorf("resolving bot login: %w", err) + } + owners := fmt.Sprintf("approvers:\n - %s\nreviewers: []\n", botLogin) + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + "OWNERS", "behaviour: add OWNERS file for auth test", + []byte(owners)); err != nil { + return fmt.Errorf("committing OWNERS file: %w", err) + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersFileWithAlias(w *world.World, alias string) error { + owners := fmt.Sprintf("approvers:\n - %s\n", alias) + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + "OWNERS", "behaviour: add OWNERS file with alias for auth test", + []byte(owners)); err != nil { + return fmt.Errorf("committing OWNERS file: %w", err) + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersAliasesFile(w *world.World, alias string) error { + ghDriver, ok := w.SCM.(*scmgh.Driver) + if !ok { + return fmt.Errorf("OWNERS test requires GitHub SCM driver") + } + botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) + if err != nil { + return fmt.Errorf("resolving bot login: %w", err) + } + aliases := fmt.Sprintf("aliases:\n %s:\n - %s\n", alias, botLogin) + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + "OWNERS_ALIASES", "behaviour: add OWNERS_ALIASES for auth test", + []byte(aliases)); err != nil { + return fmt.Errorf("committing OWNERS_ALIASES file: %w", err) + } + return nil +} + +func givenOwnersFileWithBotReviewerOnly(w *world.World) error { + ghDriver, ok := w.SCM.(*scmgh.Driver) + if !ok { + return fmt.Errorf("OWNERS test requires GitHub SCM driver") + } + botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) + if err != nil { + return fmt.Errorf("resolving bot login: %w", err) + } + owners := fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", botLogin) + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + "OWNERS", "behaviour: add OWNERS file (reviewer only) for auth test", + []byte(owners)); err != nil { + return fmt.Errorf("committing OWNERS file: %w", err) + } + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersAuthEnabled(w *world.World) error { + cfgPath := ".fullsend/config.yaml" + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath) + if err != nil { + return fmt.Errorf("reading config: %w", err) + } + content := string(cfgData) + if strings.Contains(content, "authorization:") { + return fmt.Errorf("config.yaml already contains authorization block") + } + content += "\nauthorization:\n owners_file: true\n" + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + cfgPath, "behaviour: enable OWNERS authorization", + []byte(content)); err != nil { + return fmt.Errorf("updating config: %w", err) + } + w.OwnersAuthActivated = true + return nil +} + +func thenWorkflowLogsContain(w *world.World, needle string) error { + logs, err := getWorkflowLogs(w) + if err != nil { + return err + } + if !strings.Contains(logs, needle) { + return fmt.Errorf("workflow logs do not contain %q", needle) + } + return nil +} + +func thenWorkflowLogsDoNotContain(w *world.World, needle string) error { + logs, err := getWorkflowLogs(w) + if err != nil { + return err + } + if strings.Contains(logs, needle) { + return fmt.Errorf("workflow logs unexpectedly contain %q", needle) + } + return nil +} + +// getWorkflowLogs downloads the full log archive for the workflow run +// via GitHub's API. The result can be megabytes; string matching on it +// is correct for assertion purposes but not a streaming grep. +func getWorkflowLogs(w *world.World) (string, error) { + if err := ensureTriageWorkflowComplete(w); err != nil { + return "", err + } + if w.WorkflowRun == nil { + return "", fmt.Errorf("no workflow run recorded") + } + return w.CI.GetRunLogs(context.Background(), + w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) +} + +func whenSlashCommandPosted(w *world.World, command string) error { + if w.IssueNumber == 0 { + return fmt.Errorf("no issue created") + } + w.ScenarioStart = time.Now() + _, err := w.SCM.AddComment(context.Background(), + w.RepoOwner, w.RepoName, w.IssueNumber, command) + return err +} + +func thenDispatchRunLogsDoNotContain(w *world.World, needle string) error { + run, err := waitForDispatchRunAnyConclusion(w) + if err != nil { + return err + } + gaciDriver, ok := w.CI.(*gaci.Driver) + if !ok { + return fmt.Errorf("dispatch log check requires GitHub Actions CI driver") + } + logs, err := gaciDriver.Client.GetWorkflowRunLogs(context.Background(), + w.RepoOwner, w.Install.TriageWorkflowRepo(), run.ID) + if err != nil { + return fmt.Errorf("fetching dispatch run logs: %w", err) + } + if strings.Contains(logs, needle) { + return fmt.Errorf("dispatch run logs unexpectedly contain %q", needle) + } + return nil +} + +// waitForDispatchRunAnyConclusion polls for a completed fullsend.yaml +// workflow run triggered by issue_comment, accepting any conclusion +// (success or failure). This is needed because a /fs-code dispatch +// where the code job fails still has useful route-job logs to inspect. +func waitForDispatchRunAnyConclusion(w *world.World) (*forge.WorkflowRun, error) { + gaciDriver, ok := w.CI.(*gaci.Driver) + if !ok { + return nil, fmt.Errorf("dispatch run wait requires GitHub Actions CI driver") + } + workflowFile := filepath.Base(w.Install.TriageWorkflowFile()) + ctx := context.Background() + + const poll = 5 * time.Second + deadline := time.Now().Add(12 * time.Minute) + + for time.Now().Before(deadline) { + time.Sleep(poll) + runs, err := gaciDriver.Client.ListWorkflowRuns(ctx, + w.RepoOwner, w.Install.TriageWorkflowRepo(), workflowFile) + if err != nil { + continue + } + for _, run := range runs { + runTime, parseErr := time.Parse(time.RFC3339, run.CreatedAt) + if parseErr != nil || runTime.Before(w.ScenarioStart) { + continue + } + if run.Event != "issue_comment" { + continue + } + if run.Status == "completed" { + return &run, nil + } + } + } + + return nil, fmt.Errorf("dispatch workflow (issue_comment) did not complete within deadline") +} + +// cleanupOwnersAuth removes the OWNERS file and authorization config +// block committed during the scenario so the repo slot is clean for +// the next scenario. +func cleanupOwnersAuth(w *world.World) { + ctx := context.Background() + owner := w.Install.ConfigOwner() + repo := w.Install.ConfigRepo() + + // Remove the authorization block from config.yaml first, so there's + // no window where OWNERS auth is enabled with a stale OWNERS file. + cfgPath := ".fullsend/config.yaml" + cfgData, err := w.SCM.GetFileContent(ctx, owner, repo, cfgPath) + if err == nil { + content := string(cfgData) + if strings.Contains(content, "authorization:") { + cleaned := strings.ReplaceAll(content, "\nauthorization:\n owners_file: true\n", "\n") + if cleaned != content { + if err := w.SCM.CommitFile(ctx, owner, repo, + cfgPath, "behaviour: disable OWNERS authorization", + []byte(cleaned)); err != nil { + worldLogf(w, "behaviour cleanup: disable OWNERS auth: %v", err) + } + } else { + worldLogf(w, "behaviour cleanup: authorization block present but format doesn't match — manual cleanup may be needed") + } + } + } + + // Overwrite OWNERS and OWNERS_ALIASES with empty content rather than + // deleting — the SCM driver's CommitFile doesn't support file deletion. + // The residual files are harmless: has_repo_permission won't match + // anyone in empty lists, and the authorization block was already + // removed above. + empty := []byte("approvers: []\nreviewers: []\n") + if err := w.SCM.CommitFile(ctx, owner, repo, + "OWNERS", "behaviour: clear OWNERS file", empty); err != nil { + worldLogf(w, "behaviour cleanup: remove OWNERS file: %v", err) + } + emptyAliases := []byte("aliases: {}\n") + if err := w.SCM.CommitFile(ctx, owner, repo, + "OWNERS_ALIASES", "behaviour: clear OWNERS_ALIASES file", emptyAliases); err != nil { + worldLogf(w, "behaviour cleanup: remove OWNERS_ALIASES file: %v", err) + } +} diff --git a/pkg/behaviourtest/steps/registry.go b/pkg/behaviourtest/steps/registry.go index 1a2cf20493..82aa843b7c 100644 --- a/pkg/behaviourtest/steps/registry.go +++ b/pkg/behaviourtest/steps/registry.go @@ -18,4 +18,5 @@ func Register(sc *godog.ScenarioContext) { registerJiraPollSteps(sc) registerBranchSteps(sc) registerReactionSteps(sc) + registerOwnersSteps(sc) } diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index 3d351f8473..bab23d6cdf 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -113,6 +113,11 @@ type World struct { AgentsOverridden bool AgentsOriginal []config.AgentEntry + // OwnersAuthActivated records whether this scenario committed an + // OWNERS file and/or enabled authorization.owners_file in config.yaml. + // CleanupScenario removes both. + OwnersAuthActivated bool + // Jira mock state — set by the "Given a mock Jira server" step. JiraMockServer *httptest.Server JiraMockState *jiramock.State From bbe0352bcf61fb753897e946cc20a7c2c2431a80 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Tue, 11 Aug 2026 13:41:33 +0200 Subject: [PATCH 02/12] fix(#6042): address review comments on OWNERS file authorization 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) Signed-off-by: RaphaelBut --- .github/workflows/reusable-dispatch.yml | 14 +- ...thorization-on-all-agent-dispatch-paths.md | 5 +- .../layered-config-reference.md | 9 +- .../features/dispatch/owners-auth.feature | 35 ++-- internal/config/config.go | 13 +- internal/config/interfaces.go | 17 ++ .../.github/workflows/dispatch.yml | 66 +++++++- .../scaffold/workflow_call_alignment_test.go | 17 ++ pkg/behaviourtest/steps/owners.go | 156 +++++++----------- pkg/behaviourtest/steps/owners_test.go | 90 ++++++++++ pkg/behaviourtest/suite/init.go | 1 + 11 files changed, 291 insertions(+), 132 deletions(-) create mode 100644 pkg/behaviourtest/steps/owners_test.go diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index ee66febf2e..435d96a622 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -124,8 +124,9 @@ jobs: # checkout@v7 blocks fork PR checkouts on pull_request_target by # default. Safe here: only .fullsend/ config is read, no fork code # is executed, and credentials are not persisted. Pin to base branch - # so kill-switch / role gating always reads trusted config. - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + # SHA for PR-scoped events so kill-switch / role gating always reads + # trusted config — PR authors cannot self-authorize. + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | @@ -169,7 +170,7 @@ jobs: # the collaborator API — not a fail-open gate. _owners_has_user() { local key="${1}" user="${2}" - if yq -e ".${key}[] | select(. == \"${user}\")" OWNERS &>/dev/null; then + if yq -e ".${key}[] | select((. | downcase) == \"${user}\")" OWNERS &>/dev/null; then return 0 fi if [[ -f "OWNERS_ALIASES" ]]; then @@ -177,7 +178,7 @@ jobs: while IFS= read -r entry; do [[ -z "${entry}" ]] && continue [[ ! "${entry}" =~ ^[a-zA-Z0-9_-]+$ ]] && continue - if yq -e ".aliases.\"${entry}\"[] | select(. == \"${user}\")" OWNERS_ALIASES &>/dev/null; then + if yq -e ".aliases.\"${entry}\"[] | select((. | downcase) == \"${user}\")" OWNERS_ALIASES &>/dev/null; then return 0 fi done < <(yq ".${key}[]" OWNERS 2>/dev/null) @@ -200,15 +201,16 @@ jobs: if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 else + local lc_user="${username,,}" case "${min}" in write|triage) - if _owners_has_user approvers "${username}"; then + if _owners_has_user approvers "${lc_user}"; then echo "::notice::User '${username}' authorized via OWNERS file (approver, requested: ${min})" return 0 fi ;;& triage) - if _owners_has_user reviewers "${username}"; then + if _owners_has_user reviewers "${lc_user}"; then echo "::notice::User '${username}' authorized via OWNERS file (reviewer, requested: ${min})" return 0 fi 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 0aedf1b10b..eae3b315a2 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 @@ -197,8 +197,9 @@ permission list, not by bypassing the check. > `.fullsend/config.yaml`, the function checks the repo-root `OWNERS` > (and `OWNERS_ALIASES`) before falling back to the collaborator API. > OWNERS approvers get write-equivalent access; reviewers get -> triage-equivalent. The sparse-checkout pins to the base branch SHA, so -> PR authors cannot self-authorize by modifying OWNERS in their PR. +> triage-equivalent. The sparse-checkout pins to the base branch SHA for +> PR-scoped events (`pull_request_target`, `pull_request_review`), so PR +> authors cannot self-authorize by modifying OWNERS in their PR. > This follows the extension path above (extending the allowed permission > sources in `has_repo_permission`) rather than bypassing the check. > OWNERS auth applies to built-in stages only; harness agents are diff --git a/docs/guides/infrastructure/layered-config-reference.md b/docs/guides/infrastructure/layered-config-reference.md index 446476e1cd..4395b44c7b 100644 --- a/docs/guides/infrastructure/layered-config-reference.md +++ b/docs/guides/infrastructure/layered-config-reference.md @@ -90,7 +90,14 @@ the overlay → base → code defaults chain. | `models.aliases` | `map[string]string` (nested) | Per-key merge | `nil` (fleet defaults) | | `create_issues` | `*CreateIssuesConfig` | Replace whole object if set | `nil` | | `status_notifications` | `*StatusNotificationConfig` | Replace whole object if set | `nil` | -| `authorization` | `object` | Replace whole object if set | `nil` | +| `authorization`¹ | `object` | Overlay only (not layered) | `nil` | + +> ¹ `authorization` is read by the dispatch workflow's bash/yq directly +> from `.fullsend/config.yaml` (the overlay). It does **not** participate +> in the overlay → base → code-defaults merge chain and is not part of the +> Go config package. Setting it in `config.base.yaml` has no effect. +> See [#6072](https://github.com/fullsend-ai/fullsend/issues/6072) for +> the planned migration to the Go config layer. ### Per-agent `runtime`, `model`, `effort` on `agents:` entries diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature index 786009a5c0..c85e42a8ff 100644 --- a/e2e/behaviour/features/dispatch/owners-auth.feature +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -1,10 +1,14 @@ Feature: OWNERS file authorization for bash routing Verify that the OWNERS-file authorization path fires when - authorization.owners_file is enabled in config.yaml. The e2e bot - already has collaborator access, so these scenarios confirm the - OWNERS code path is reached (via audit log) rather than testing - the fallback denial path (which requires a restricted identity). + authorization.owners_file is enabled in config.yaml. Scenarios + trigger via issues.opened, which unconditionally calls + is_event_actor_authorized and exercises has_repo_permission. + + The e2e bot already has collaborator access, so these scenarios + confirm the OWNERS code path is reached (via audit log) rather + than testing the API-fallback denial path — that requires a + restricted identity without collaborator access (#6072). Background: Given the enrolled test repository @@ -15,11 +19,10 @@ Feature: OWNERS file authorization for bash routing And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-ok.json, fixtures/dispatch/ok.json | - And an issue - When the issue is labeled "ready-for-triage" + When an issue is opened for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "authorized via OWNERS file" + And the triage workflow logs contain "authorized via OWNERS file (approver" Scenario: OWNERS alias resolves to grant access Given an OWNERS file with alias "test-team" as approver @@ -28,38 +31,28 @@ Feature: OWNERS file authorization for bash routing And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-alias-ok.json, fixtures/dispatch/ok.json | - And an issue - When the issue is labeled "ready-for-triage" + When an issue is opened for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution And the triage workflow logs contain "authorized via OWNERS file (approver" - Scenario: OWNERS reviewer can triage but not code + Scenario: OWNERS reviewer can triage Given an OWNERS file listing the bot as a reviewer only And OWNERS authorization is enabled And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-rev-ok.json, fixtures/dispatch/ok.json | - And an issue - When the issue is labeled "ready-for-triage" + When an issue is opened for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution And the triage workflow logs contain "authorized via OWNERS file (reviewer" - Scenario: OWNERS reviewer is not granted write-level access via OWNERS - Given an OWNERS file listing the bot as a reviewer only - And OWNERS authorization is enabled - And an issue - When the OWNERS auth test posts "/fs-code" on the issue - Then the dispatch run logs do not contain "authorized via OWNERS file" - Scenario: Triage dispatches without OWNERS path when not opted in Given an OWNERS file listing the bot as an approver And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-off-ok.json, fixtures/dispatch/ok.json | - And an issue - When the issue is labeled "ready-for-triage" + When an issue is opened for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution And the triage workflow logs do not contain "authorized via OWNERS file" diff --git a/internal/config/config.go b/internal/config/config.go index 5781297a69..97409c705e 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -275,6 +275,12 @@ type CreateIssuesConfig struct { AllowTargets AllowTargets `yaml:"allow_targets"` } +// AuthorizationConfig controls opt-in authorization mechanisms that +// extend the default collaborator-API permission check. +type AuthorizationConfig struct { + OwnersFile bool `yaml:"owners_file,omitempty"` +} + // orgConfig is the top-level configuration for a fullsend organization. // Consumer packages should use the OrgConfigReader or OrgConfigWriter // interfaces rather than referencing this type directly. @@ -780,8 +786,9 @@ type perRepoConfig struct { // resource prefixes. MarshalYAML preserves the nil-vs-empty // distinction: nil (unset) is omitted, empty (deny-all) is // marshaled as `allowed_remote_resources: []`. - AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` - CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` + CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + Authorization *AuthorizationConfig `yaml:"authorization,omitempty"` // Notifications backs the StatusNotifications() accessor. Named // distinctly from the method (unlike CreateIssues/IssueCreationConfig) // because "StatusNotifications" is the established accessor name @@ -982,6 +989,7 @@ type perRepoConfigMarshal struct { Agents []AgentEntry `yaml:"agents,omitempty"` AllowedRemoteResources *[]string `yaml:"allowed_remote_resources,omitempty"` CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + Authorization *AuthorizationConfig `yaml:"authorization,omitempty"` StatusNotifications *StatusNotificationConfig `yaml:"status_notifications,omitempty"` MintURL string `yaml:"mint_url,omitempty"` Inference *PerRepoInferenceConfig `yaml:"inference,omitempty"` @@ -1002,6 +1010,7 @@ func (c *perRepoConfig) MarshalYAML() (interface{}, error) { Runtime: c.Runtime, Agents: c.Agents, CreateIssues: c.CreateIssues, + Authorization: c.Authorization, StatusNotifications: c.Notifications, MintURL: c.MintURL, } diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index afdd5898da..f4523785c6 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -96,6 +96,7 @@ type PerRepoConfigReader interface { type ConfigWriter interface { ConfigReader SetKillSwitch(bool) + SetAuthorizationOwnersFile(bool) SetAgents([]AgentEntry) SetAllowedRemoteResources([]string) SetStatusNotifications(*StatusNotificationConfig) @@ -185,6 +186,10 @@ func (c *orgConfig) StatusNotifications() *StatusNotificationConfig { // SetKillSwitch sets the kill switch state. func (c *orgConfig) SetKillSwitch(v bool) { c.KillSwitch = v } +// SetAuthorizationOwnersFile is a no-op for org configs; OWNERS +// authorization is per-repo only. +func (c *orgConfig) SetAuthorizationOwnersFile(bool) {} + // SetAgents replaces the registered agent entries. func (c *orgConfig) SetAgents(agents []AgentEntry) { c.Agents = agents } @@ -563,6 +568,18 @@ func (c *perRepoConfig) ConfigModelAliases() map[string]string { // an explicit false is distinguishable from unset (nil) across layers. func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = &v } +// SetAuthorizationOwnersFile enables or disables OWNERS-file authorization. +func (c *perRepoConfig) SetAuthorizationOwnersFile(v bool) { + if v { + if c.Authorization == nil { + c.Authorization = &AuthorizationConfig{} + } + c.Authorization.OwnersFile = true + } else { + c.Authorization = nil + } +} + // SetAgents replaces the registered agent entries. func (c *perRepoConfig) SetAgents(agents []AgentEntry) { c.Agents = agents } diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index 80501fe487..e62117876a 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -1,5 +1,5 @@ --- -# lint-workflow-size: max-lines=612 +# lint-workflow-size: max-lines=682 # Dispatcher workflow that routes events to agent workflows based on stage. # Routing logic determines the stage from event context — the shim only # forwards the raw event. Adding a new stage requires only a case branch @@ -30,6 +30,18 @@ jobs: outputs: dispatched_count: ${{ steps.dispatch.outputs.dispatched_count }} steps: + - name: Checkout config and OWNERS files + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} + persist-credentials: false + allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} + sparse-checkout: | + .fullsend/config.yaml + OWNERS + OWNERS_ALIASES + sparse-checkout-cone-mode: false + - name: Determine stage id: route env: @@ -58,11 +70,63 @@ jobs: STAGE="" TRIGGER_SOURCE="" + # Check direct membership or alias membership in an OWNERS list. + # Used by has_repo_permission when OWNERS-file auth is enabled. + # yq errors are intentionally suppressed (&>/dev/null): a parse + # failure or missing key means "no match", falling through to + # the collaborator API — not a fail-open gate. + _owners_has_user() { + local key="${1}" user="${2}" + if yq -e ".${key}[] | select((. | downcase) == \"${user}\")" OWNERS &>/dev/null; then + return 0 + fi + if [[ -f "OWNERS_ALIASES" ]]; then + local entry + while IFS= read -r entry; do + [[ -z "${entry}" ]] && continue + [[ ! "${entry}" =~ ^[a-zA-Z0-9_-]+$ ]] && continue + if yq -e ".aliases.\"${entry}\"[] | select((. | downcase) == \"${user}\")" OWNERS_ALIASES &>/dev/null; then + return 0 + fi + done < <(yq ".${key}[]" OWNERS 2>/dev/null) + fi + return 1 + } + # 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 [[ -z "${username}" ]] && return 1 + + # OWNERS-file authorization (opt-in via authorization.owners_file in config.yaml). + # Approvers get write-equivalent access; reviewers get triage-equivalent. + # Safe: sparse-checkout pins to base branch SHA for PR-scoped events, + # so PR authors cannot self-authorize by adding themselves to OWNERS. + if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then + if [[ "$(yq '.authorization.owners_file // false' .fullsend/config.yaml)" == "true" ]]; then + if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then + echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 + else + local lc_user="${username,,}" + case "${min}" in + write|triage) + if _owners_has_user approvers "${lc_user}"; then + echo "::notice::User '${username}' authorized via OWNERS file (approver, requested: ${min})" + return 0 + fi + ;;& + triage) + if _owners_has_user reviewers "${lc_user}"; then + echo "::notice::User '${username}' authorized via OWNERS file (reviewer, requested: ${min})" + return 0 + fi + ;; + esac + fi + fi + fi + api_err=$(mktemp) || { echo "::warning::Failed to create temp file for permission check of ${username}" >&2 return 1 diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 43bf064e68..d3ab3c5bce 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -645,6 +645,23 @@ func TestDispatchPerStageAuthorization(t *testing.T) { // Retro on PR close remains intentionally ungated (documented) assert.Regexp(t, `(?s)closed\)\s*\n\s+# Intentional ungated:.*\n\s+STAGE="retro"`, s) + + // OWNERS role→permission mapping: approvers in write|triage arm, + // reviewers in triage-only arm, connected by ;;& (pattern-retest). + // A ;& (unconditional fallthrough) would silently give reviewers + // write-level access — this assertion catches that. + assert.Regexp(t, `(?s)write\|triage\).*_owners_has_user approvers`, s, + "OWNERS approvers must be checked in the write|triage case arm") + assert.Regexp(t, `(?s);;&\s*\n\s+triage\).*_owners_has_user reviewers`, s, + "OWNERS reviewers must be in the triage-only arm after ;;& (not ;&)") + assert.Contains(t, s, `lc_user="${username,,}"`, + "OWNERS username comparison must be case-insensitive") + assert.Contains(t, s, `_owners_has_user approvers "${lc_user}"`, + "OWNERS approver check must use lowercased lc_user, not original username") + assert.Contains(t, s, `_owners_has_user reviewers "${lc_user}"`, + "OWNERS reviewer check must use lowercased lc_user, not original username") + assert.Regexp(t, `::notice::User '\$\{username\}' authorized via OWNERS file`, s, + "OWNERS audit log must use original username casing, not lc_user") }) } } diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index dfe5bae209..e0c2cb1ef3 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -9,8 +9,7 @@ import ( "github.com/cucumber/godog" - "github.com/fullsend-ai/fullsend/internal/forge" - gaci "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci/githubactions" + "github.com/fullsend-ai/fullsend/internal/config" scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) @@ -37,11 +36,8 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { sc.Step(`^the triage workflow logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { return ctx, thenWorkflowLogsDoNotContain(world.FromContext(ctx), needle) }) - sc.Step(`^the OWNERS auth test posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { - return ctx, whenSlashCommandPosted(world.FromContext(ctx), command) - }) - sc.Step(`^the dispatch run logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { - return ctx, thenDispatchRunLogsDoNotContain(world.FromContext(ctx), needle) + sc.Step(`^an issue is opened for OWNERS auth testing$`, func(ctx context.Context) (context.Context, error) { + return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx)) }) } @@ -93,6 +89,7 @@ func givenOwnersAliasesFile(w *world.World, alias string) error { []byte(aliases)); err != nil { return fmt.Errorf("committing OWNERS_ALIASES file: %w", err) } + w.OwnersAuthActivated = true return nil } @@ -117,27 +114,54 @@ func givenOwnersFileWithBotReviewerOnly(w *world.World) error { } func givenOwnersAuthEnabled(w *world.World) error { - cfgPath := ".fullsend/config.yaml" + cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath) if err != nil { return fmt.Errorf("reading config: %w", err) } - content := string(cfgData) - if strings.Contains(content, "authorization:") { - return fmt.Errorf("config.yaml already contains authorization block") + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config: %w", err) + } + cfg.SetAuthorizationOwnersFile(true) + merged, err := cfg.Marshal() + if err != nil { + return err } - content += "\nauthorization:\n owners_file: true\n" if err := w.SCM.CommitFile(context.Background(), w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath, "behaviour: enable OWNERS authorization", - []byte(content)); err != nil { + merged); err != nil { return fmt.Errorf("updating config: %w", err) } w.OwnersAuthActivated = true return nil } +// whenIssueOpenedForOwnersAuth creates an issue without draining the +// issues.opened workflow run. The issues.opened path unconditionally +// calls is_event_actor_authorized, which exercises has_repo_permission +// and the OWNERS authorization code path. +func whenIssueOpenedForOwnersAuth(w *world.World) error { + if w.RepoOwner == "" || w.RepoName == "" { + w.RepoOwner = w.Org + w.RepoName = w.Install.TestRepo() + w.RepoFull = w.Org + "/" + w.RepoName + } + w.ScenarioStart = time.Now() + w.TriageTriggerEvent = issueOpenEvent + title := fmt.Sprintf("behaviour-owners-auth-%d", time.Now().UnixNano()) + body := "Behaviour test issue for OWNERS authorization path." + issue, err := w.SCM.CreateIssue(context.Background(), w.RepoOwner, w.RepoName, title, body) + if err != nil { + return err + } + w.IssueNumber = issue.Number + w.IssueTitle = title + return nil +} + func thenWorkflowLogsContain(w *world.World, needle string) error { logs, err := getWorkflowLogs(w) if err != nil { @@ -160,9 +184,6 @@ func thenWorkflowLogsDoNotContain(w *world.World, needle string) error { return nil } -// getWorkflowLogs downloads the full log archive for the workflow run -// via GitHub's API. The result can be megabytes; string matching on it -// is correct for assertion purposes but not a streaming grep. func getWorkflowLogs(w *world.World) (string, error) { if err := ensureTriageWorkflowComplete(w); err != nil { return "", err @@ -174,75 +195,33 @@ func getWorkflowLogs(w *world.World) (string, error) { w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) } -func whenSlashCommandPosted(w *world.World, command string) error { - if w.IssueNumber == 0 { - return fmt.Errorf("no issue created") - } - w.ScenarioStart = time.Now() - _, err := w.SCM.AddComment(context.Background(), - w.RepoOwner, w.RepoName, w.IssueNumber, command) - return err -} - -func thenDispatchRunLogsDoNotContain(w *world.World, needle string) error { - run, err := waitForDispatchRunAnyConclusion(w) +// disableOwnersAuth sets authorization.owners_file: false (removing the +// authorization block) in the enrolled repo's config.yaml. +func disableOwnersAuth(w *world.World) error { + cfgPath := filepath.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath) if err != nil { - return err + return fmt.Errorf("reading config: %w", err) } - gaciDriver, ok := w.CI.(*gaci.Driver) - if !ok { - return fmt.Errorf("dispatch log check requires GitHub Actions CI driver") + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return fmt.Errorf("parsing config: %w", err) } - logs, err := gaciDriver.Client.GetWorkflowRunLogs(context.Background(), - w.RepoOwner, w.Install.TriageWorkflowRepo(), run.ID) + cfg.SetAuthorizationOwnersFile(false) + merged, err := cfg.Marshal() if err != nil { - return fmt.Errorf("fetching dispatch run logs: %w", err) + return err } - if strings.Contains(logs, needle) { - return fmt.Errorf("dispatch run logs unexpectedly contain %q", needle) + if err := w.SCM.CommitFile(context.Background(), + w.Install.ConfigOwner(), w.Install.ConfigRepo(), + cfgPath, "behaviour: disable OWNERS authorization", + merged); err != nil { + return fmt.Errorf("updating config: %w", err) } return nil } -// waitForDispatchRunAnyConclusion polls for a completed fullsend.yaml -// workflow run triggered by issue_comment, accepting any conclusion -// (success or failure). This is needed because a /fs-code dispatch -// where the code job fails still has useful route-job logs to inspect. -func waitForDispatchRunAnyConclusion(w *world.World) (*forge.WorkflowRun, error) { - gaciDriver, ok := w.CI.(*gaci.Driver) - if !ok { - return nil, fmt.Errorf("dispatch run wait requires GitHub Actions CI driver") - } - workflowFile := filepath.Base(w.Install.TriageWorkflowFile()) - ctx := context.Background() - - const poll = 5 * time.Second - deadline := time.Now().Add(12 * time.Minute) - - for time.Now().Before(deadline) { - time.Sleep(poll) - runs, err := gaciDriver.Client.ListWorkflowRuns(ctx, - w.RepoOwner, w.Install.TriageWorkflowRepo(), workflowFile) - if err != nil { - continue - } - for _, run := range runs { - runTime, parseErr := time.Parse(time.RFC3339, run.CreatedAt) - if parseErr != nil || runTime.Before(w.ScenarioStart) { - continue - } - if run.Event != "issue_comment" { - continue - } - if run.Status == "completed" { - return &run, nil - } - } - } - - return nil, fmt.Errorf("dispatch workflow (issue_comment) did not complete within deadline") -} - // cleanupOwnersAuth removes the OWNERS file and authorization config // block committed during the scenario so the repo slot is clean for // the next scenario. @@ -251,31 +230,10 @@ func cleanupOwnersAuth(w *world.World) { owner := w.Install.ConfigOwner() repo := w.Install.ConfigRepo() - // Remove the authorization block from config.yaml first, so there's - // no window where OWNERS auth is enabled with a stale OWNERS file. - cfgPath := ".fullsend/config.yaml" - cfgData, err := w.SCM.GetFileContent(ctx, owner, repo, cfgPath) - if err == nil { - content := string(cfgData) - if strings.Contains(content, "authorization:") { - cleaned := strings.ReplaceAll(content, "\nauthorization:\n owners_file: true\n", "\n") - if cleaned != content { - if err := w.SCM.CommitFile(ctx, owner, repo, - cfgPath, "behaviour: disable OWNERS authorization", - []byte(cleaned)); err != nil { - worldLogf(w, "behaviour cleanup: disable OWNERS auth: %v", err) - } - } else { - worldLogf(w, "behaviour cleanup: authorization block present but format doesn't match — manual cleanup may be needed") - } - } + if err := disableOwnersAuth(w); err != nil { + worldLogf(w, "behaviour cleanup: disable OWNERS auth: %v", err) } - // Overwrite OWNERS and OWNERS_ALIASES with empty content rather than - // deleting — the SCM driver's CommitFile doesn't support file deletion. - // The residual files are harmless: has_repo_permission won't match - // anyone in empty lists, and the authorization block was already - // removed above. empty := []byte("approvers: []\nreviewers: []\n") if err := w.SCM.CommitFile(ctx, owner, repo, "OWNERS", "behaviour: clear OWNERS file", empty); err != nil { diff --git a/pkg/behaviourtest/steps/owners_test.go b/pkg/behaviourtest/steps/owners_test.go new file mode 100644 index 0000000000..803c82d504 --- /dev/null +++ b/pkg/behaviourtest/steps/owners_test.go @@ -0,0 +1,90 @@ +package steps + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/config" +) + +func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { + t.Parallel() + + t.Run("enable then marshal", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "authorization:") + assert.Contains(t, s, "owners_file: true") + assert.Contains(t, s, "runtime: claude") + }) + + t.Run("enable is idempotent", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.Contains(t, string(out), "owners_file: true") + }) + + t.Run("disable removes authorization block", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(false) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.NotContains(t, string(out), "authorization") + }) + + t.Run("parse existing authorization from YAML", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nauthorization:\n owners_file: true\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + out, err := cfg.Marshal() + require.NoError(t, err) + assert.Contains(t, string(out), "owners_file: true") + }) + + t.Run("disable when never enabled is no-op", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(false) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.NotContains(t, s, "authorization") + assert.Contains(t, s, "runtime: claude") + }) + + t.Run("round-trip preserves other fields", func(t *testing.T) { + t.Parallel() + input := []byte("version: \"1\"\nruntime: claude\nkill_switch: false\nroles:\n - coder\n - reviewer\n") + cfg, err := config.ParsePerRepoConfigWriter(input) + require.NoError(t, err) + cfg.SetAuthorizationOwnersFile(true) + out, err := cfg.Marshal() + require.NoError(t, err) + s := string(out) + assert.Contains(t, s, "runtime: claude") + assert.Contains(t, s, "kill_switch: false") + assert.Contains(t, s, "- coder") + assert.Contains(t, s, "owners_file: true") + }) +} diff --git a/pkg/behaviourtest/suite/init.go b/pkg/behaviourtest/suite/init.go index a5be4cb9a2..17186d3a37 100644 --- a/pkg/behaviourtest/suite/init.go +++ b/pkg/behaviourtest/suite/init.go @@ -102,6 +102,7 @@ func resetScenarioWorld(w *world.World) { w.AllowedResourcesOriginal = nil w.AgentsOverridden = false w.AgentsOriginal = nil + w.OwnersAuthActivated = false w.JiraMockServer = nil w.JiraMockState = nil w.JiraConfigDir = "" From 2b4a37f7eb31905a52544991db61fc18f4e1e6d7 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Tue, 11 Aug 2026 15:42:42 +0200 Subject: [PATCH 03/12] fix(#6042): clear only OwnersFile field instead of niling 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) Signed-off-by: RaphaelBut --- internal/config/interfaces.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index f4523785c6..a594985f43 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -569,14 +569,19 @@ func (c *perRepoConfig) ConfigModelAliases() map[string]string { func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = &v } // SetAuthorizationOwnersFile enables or disables OWNERS-file authorization. +// Clears only OwnersFile; the struct is niled only when all fields are zero +// so future sibling fields are not silently wiped. func (c *perRepoConfig) SetAuthorizationOwnersFile(v bool) { if v { if c.Authorization == nil { c.Authorization = &AuthorizationConfig{} } c.Authorization.OwnersFile = true - } else { - c.Authorization = nil + } else if c.Authorization != nil { + c.Authorization.OwnersFile = false + if *c.Authorization == (AuthorizationConfig{}) { + c.Authorization = nil + } } } From 9e611a232830529c8aa737403923947128540404 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Wed, 12 Aug 2026 18:10:49 +0200 Subject: [PATCH 04/12] feat(#6042): add Go OWNERS package, harness dispatch wiring, 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) Signed-off-by: RaphaelBut --- .github/workflows/reusable-dispatch.yml | 4 +- .../features/dispatch/owners-auth.feature | 7 + e2e/behaviour/suite_test.go | 10 + internal/config/defaults.go | 3 + internal/config/interfaces.go | 10 + internal/forge/forge.go | 1 + internal/forge/github/github.go | 4 + internal/harnessdispatch/core.go | 13 ++ internal/owners/owners.go | 104 ++++++++++ internal/owners/owners_test.go | 169 ++++++++++++++++ .../scaffold/workflow_call_alignment_test.go | 39 ++++ pkg/behaviourtest/steps/owners.go | 191 +++++++++++++----- pkg/behaviourtest/world/world.go | 7 + 13 files changed, 505 insertions(+), 57 deletions(-) create mode 100644 internal/owners/owners.go create mode 100644 internal/owners/owners_test.go diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index 435d96a622..ae0f61cd26 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -1518,11 +1518,13 @@ jobs: - name: Checkout caller repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | .fullsend/ + OWNERS + OWNERS_ALIASES .defaults/action.yml .defaults/.github/actions/install-fullsend-cli/ .fullsend/.defaults/action.yml diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature index c85e42a8ff..47ae1ca5d7 100644 --- a/e2e/behaviour/features/dispatch/owners-auth.feature +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -47,6 +47,13 @@ Feature: OWNERS file authorization for bash routing 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 + Given an OWNERS file listing the outsider as a reviewer + And OWNERS authorization is enabled + And an issue + When the outsider posts "/fs-code" on the issue + Then the dispatch run does not authorize via OWNERS + Scenario: Triage dispatches without OWNERS path when not opted in Given an OWNERS file listing the bot as an approver And a dummy agent that would: diff --git a/e2e/behaviour/suite_test.go b/e2e/behaviour/suite_test.go index 037b4ddcd6..c0bb5699d4 100644 --- a/e2e/behaviour/suite_test.go +++ b/e2e/behaviour/suite_test.go @@ -123,6 +123,16 @@ func TestBehaviourSuite(t *testing.T) { RepoOwner: org, } + if outsiderPAT := os.Getenv("TEST_ACTOR_OUTSIDER_PAT"); outsiderPAT != "" { + outsiderClient := e2etest.NewLiveClient(outsiderPAT) + template.OutsiderSCM = scmgh.New(outsiderClient) + login, err := outsiderClient.GetAuthenticatedUser(ctx) + if err != nil { + t.Fatalf("resolving outsider login: %v", err) + } + template.OutsiderLogin = login + } + suiteRunner := godog.TestSuite{ Name: "behaviour", ScenarioInitializer: func(sc *godog.ScenarioContext) { suite.InitScenario(sc, template) }, diff --git a/internal/config/defaults.go b/internal/config/defaults.go index 494984339e..04bd11548f 100644 --- a/internal/config/defaults.go +++ b/internal/config/defaults.go @@ -62,6 +62,9 @@ func (d *perRepoDefaults) StatusNotifications() *StatusNotificationConfig { retu // IsOrgMode returns false — per-repo configs are never org mode. func (d *perRepoDefaults) IsOrgMode() bool { return false } +// AuthorizationOwnersFile returns false — OWNERS auth is off by default. +func (d *perRepoDefaults) AuthorizationOwnersFile() bool { return false } + // ConfigMintURL returns the default mint URL (hosted public mint). func (d *perRepoDefaults) ConfigMintURL() string { return DefaultPerRepoMintURL } diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index a594985f43..dac42aab67 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -52,6 +52,7 @@ type ConfigReader interface { StatusNotificationsReader ConfigVersion() string IsOrgMode() bool + AuthorizationOwnersFile() bool } // --- Mode-specific read interfaces --- @@ -186,6 +187,10 @@ func (c *orgConfig) StatusNotifications() *StatusNotificationConfig { // SetKillSwitch sets the kill switch state. func (c *orgConfig) SetKillSwitch(v bool) { c.KillSwitch = v } +// AuthorizationOwnersFile returns false for org configs; OWNERS +// authorization is per-repo only. +func (c *orgConfig) AuthorizationOwnersFile() bool { return false } + // SetAuthorizationOwnersFile is a no-op for org configs; OWNERS // authorization is per-repo only. func (c *orgConfig) SetAuthorizationOwnersFile(bool) {} @@ -404,6 +409,11 @@ func (c *perRepoConfig) ConfigVersion() string { // IsOrgMode reports that this is a per-repo configuration. func (c *perRepoConfig) IsOrgMode() bool { return false } +// AuthorizationOwnersFile returns whether OWNERS-file authorization is enabled. +func (c *perRepoConfig) AuthorizationOwnersFile() bool { + return c.Authorization != nil && c.Authorization.OwnersFile +} + // ConfigRoles returns the configured agent roles. nil (key omitted) // falls through to parent. Non-nil (including empty) replaces the // parent list entirely. diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 16c39c1e19..687c422131 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -235,6 +235,7 @@ type WorkflowRun struct { Conclusion string // "success", "failure", "cancelled", etc. HTMLURL string CreatedAt string + ActorLogin string // GitHub login of the user who triggered the run } // WorkflowJob represents a job within a workflow run. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 7672a8554b..90d6b88bf4 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -3297,6 +3297,9 @@ func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflow Conclusion string `json:"conclusion"` HTMLURL string `json:"html_url"` CreatedAt string `json:"created_at"` + Actor struct { + Login string `json:"login"` + } `json:"actor"` } `json:"workflow_runs"` } if err := decodeJSON(resp, &result); err != nil { @@ -3312,6 +3315,7 @@ func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflow Conclusion: r.Conclusion, HTMLURL: r.HTMLURL, CreatedAt: r.CreatedAt, + ActorLogin: r.Actor.Login, } } return runs, nil diff --git a/internal/harnessdispatch/core.go b/internal/harnessdispatch/core.go index 42a2ec72ee..df6b8f3596 100644 --- a/internal/harnessdispatch/core.go +++ b/internal/harnessdispatch/core.go @@ -3,10 +3,12 @@ package harnessdispatch import ( "context" "fmt" + "log" "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/fetch" "github.com/fullsend-ai/fullsend/internal/normevent" + "github.com/fullsend-ai/fullsend/internal/owners" ) // Options configures a dispatch run. @@ -38,6 +40,17 @@ func Dispatch(ctx context.Context, opts Options) ([]ExecutionRef, error) { return nil, nil } + // Upgrade Actor.Role in place if OWNERS grants a higher level. + // Bare paths — same working-directory assumption as ConfigDir. + if dirCfg.AuthorizationOwnersFile() && opts.Event.Actor.ID != "" { + role, err := owners.Resolve("OWNERS", "OWNERS_ALIASES", opts.Event.Actor.ID) + if err != nil { + log.Printf("harness dispatch: OWNERS resolution failed for %s: %v", opts.Event.Actor.ID, err) + } else if role != owners.None { + opts.Event.Actor.Role = owners.MapToActorRole(role, opts.Event.Actor.Role) + } + } + if !IsAuthorized(opts.Event) { return nil, nil } diff --git a/internal/owners/owners.go b/internal/owners/owners.go new file mode 100644 index 0000000000..67a2678665 --- /dev/null +++ b/internal/owners/owners.go @@ -0,0 +1,104 @@ +package owners + +import ( + "fmt" + "os" + "regexp" + "strings" + + "gopkg.in/yaml.v3" + + "github.com/fullsend-ai/fullsend/internal/normevent" +) + +var validUsername = regexp.MustCompile(`^[a-zA-Z0-9-]+$`) + +// Role is the OWNERS-file role for a resolved user. +type Role int + +const ( + None Role = iota + Reviewer // triage-equivalent + Approver // write-equivalent +) + +type ownersFile struct { + Approvers []string `yaml:"approvers"` + Reviewers []string `yaml:"reviewers"` +} + +type aliasesFile struct { + Aliases map[string][]string `yaml:"aliases"` +} + +// Resolve checks whether username appears in the OWNERS file at +// ownersPath (directly or via aliases in aliasesPath). Returns +// Approver if the user is an approver, Reviewer if only a reviewer, +// or None if not listed. Matching is case-insensitive. +// +// A missing OWNERS file returns an error. A missing OWNERS_ALIASES +// file is not an error — alias resolution is skipped. +func Resolve(ownersPath, aliasesPath, username string) (Role, error) { + if !validUsername.MatchString(username) { + return None, nil + } + data, err := os.ReadFile(ownersPath) + if err != nil { + return None, fmt.Errorf("reading OWNERS: %w", err) + } + var owners ownersFile + if err := yaml.Unmarshal(data, &owners); err != nil { + return None, fmt.Errorf("parsing OWNERS: %w", err) + } + + var aliases aliasesFile + if aliasData, err := os.ReadFile(aliasesPath); err == nil { + if err := yaml.Unmarshal(aliasData, &aliases); err != nil { + return None, fmt.Errorf("parsing OWNERS_ALIASES: %w", err) + } + } + + if hasMember(owners.Approvers, username, aliases.Aliases) { + return Approver, nil + } + if hasMember(owners.Reviewers, username, aliases.Aliases) { + return Reviewer, nil + } + return None, nil +} + +// hasMember checks if username is in entries, either directly or by +// expanding alias names through the aliases map. +func hasMember(entries []string, username string, aliases map[string][]string) bool { + for _, entry := range entries { + if strings.EqualFold(entry, username) { + return true + } + if members, ok := aliases[entry]; ok { + for _, m := range members { + if strings.EqualFold(m, username) { + return true + } + } + } + } + return false +} + +// MapToActorRole upgrades currentRole based on the OWNERS role. +// Approver grants at least write; reviewer grants at least triage. +// Never downgrades — if the collaborator API already granted a +// higher role, it is preserved. +func MapToActorRole(role Role, currentRole normevent.ActorRole) normevent.ActorRole { + switch role { + case Approver: + if !normevent.IsWriteAuthorized(currentRole) { + return normevent.RoleWrite + } + case Reviewer: + if currentRole == normevent.RoleNone || currentRole == normevent.RoleExternal || currentRole == normevent.RoleRead { + return normevent.RoleTriage + } + } + return currentRole +} diff --git a/internal/owners/owners_test.go b/internal/owners/owners_test.go new file mode 100644 index 0000000000..7125e7e37c --- /dev/null +++ b/internal/owners/owners_test.go @@ -0,0 +1,169 @@ +package owners + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/normevent" +) + +func writeFile(t *testing.T, dir, name, content string) string { + t.Helper() + p := filepath.Join(dir, name) + require.NoError(t, os.WriteFile(p, []byte(content), 0o644)) + return p +} + +func TestResolve(t *testing.T) { + t.Parallel() + + t.Run("direct approver", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers: []\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("direct reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers:\n - bob\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "bob") + require.NoError(t, err) + assert.Equal(t, Reviewer, role) + }) + + t.Run("alias approver", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - team-alpha\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n team-alpha:\n - carol\n - dave\n") + role, err := Resolve(op, ap, "carol") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("alias reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers:\n - team-beta\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n team-beta:\n - eve\n") + role, err := Resolve(op, ap, "eve") + require.NoError(t, err) + assert.Equal(t, Reviewer, role) + }) + + t.Run("not listed", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers:\n - bob\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "mallory") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("case insensitive", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "Alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("case insensitive alias", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - my-team\n") + ap := writeFile(t, dir, "OWNERS_ALIASES", "aliases:\n my-team:\n - Alice\n") + role, err := Resolve(op, ap, "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("missing OWNERS file is error", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + _, err := Resolve(filepath.Join(dir, "OWNERS"), filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.Error(t, err) + }) + + t.Run("missing OWNERS_ALIASES is not error", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "nonexistent"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) + + t.Run("empty lists", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers: []\nreviewers: []\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("malformed OWNERS", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "not: [valid: yaml: {{") + _, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.Error(t, err) + }) + + t.Run("invalid username returns None", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - $(whoami)\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "$(whoami)") + require.NoError(t, err) + assert.Equal(t, None, role) + }) + + t.Run("approver takes precedence over reviewer", func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + op := writeFile(t, dir, "OWNERS", "approvers:\n - alice\nreviewers:\n - alice\n") + role, err := Resolve(op, filepath.Join(dir, "OWNERS_ALIASES"), "alice") + require.NoError(t, err) + assert.Equal(t, Approver, role) + }) +} + +func TestMapToActorRole(t *testing.T) { + t.Parallel() + + t.Run("approver upgrades none to write", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleWrite, MapToActorRole(Approver, normevent.RoleNone)) + }) + + t.Run("approver does not downgrade admin", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleAdmin, MapToActorRole(Approver, normevent.RoleAdmin)) + }) + + t.Run("reviewer upgrades none to triage", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleTriage, MapToActorRole(Reviewer, normevent.RoleNone)) + }) + + t.Run("reviewer does not downgrade write", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleWrite, MapToActorRole(Reviewer, normevent.RoleWrite)) + }) + + t.Run("none does not change role", func(t *testing.T) { + t.Parallel() + assert.Equal(t, normevent.RoleRead, MapToActorRole(None, normevent.RoleRead)) + }) +} diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index d3ab3c5bce..28543c90aa 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -666,6 +666,45 @@ func TestDispatchPerStageAuthorization(t *testing.T) { } } +// TestOwnersCheckoutRefPin validates that every checkout step whose +// sparse-checkout includes OWNERS files pins to base branch SHA for +// pull_request_review events. Without this, a PR author can add +// themselves to OWNERS in their branch and self-authorize on the +// pull_request_review dispatch path. +func TestOwnersCheckoutRefPin(t *testing.T) { + cases := []struct { + name string + content func(t *testing.T) []byte + }{ + {"reusable-dispatch.yml", loadRepoFile(".github/workflows/reusable-dispatch.yml")}, + {"scaffold/dispatch.yml", loadScaffoldFile(".github/workflows/dispatch.yml")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + s := string(tc.content(t)) + // Split into sections by checkout step boundary. Each section + // starting with "uses: actions/checkout@" contains one step's + // with: block up to the next step or job boundary. + sections := regexp.MustCompile(`(?m)^[ \t]*- name:`).Split(s, -1) + + var ownersCheckouts int + for _, section := range sections { + if !strings.Contains(section, "actions/checkout@") { + continue + } + if !strings.Contains(section, "OWNERS") { + continue + } + ownersCheckouts++ + assert.Contains(t, section, "pull_request_review", + "checkout that sparse-checks-out OWNERS must pin ref for pull_request_review events") + } + require.NotZero(t, ownersCheckouts, + "should find at least one checkout step with OWNERS in sparse-checkout") + }) + } +} + // TestShimScaffoldBranchFilter validates that both shim templates skip dispatch // for PRs from the fullsend/scaffold branch. Without this filter, the shim // fires pull_request_target on the scaffold PR, causing dispatch noise (#5470). diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index e0c2cb1ef3..540a437e83 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -8,15 +8,16 @@ import ( "time" "github.com/cucumber/godog" - "github.com/fullsend-ai/fullsend/internal/config" + "github.com/fullsend-ai/fullsend/internal/forge" + gaci "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci/githubactions" scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) func registerOwnersSteps(sc *godog.ScenarioContext) { - sc.Step(`^an OWNERS file listing the bot as an approver$`, func(ctx context.Context) (context.Context, error) { - return ctx, givenOwnersFileWithBot(world.FromContext(ctx)) + sc.Step(`^an OWNERS file listing the bot as (?:an? )?(approver|reviewer)(?: only)?$`, func(ctx context.Context, role string) (context.Context, error) { + return ctx, givenBotInOwners(world.FromContext(ctx), role) }) sc.Step(`^an OWNERS file with alias "([^"]+)" as approver$`, func(ctx context.Context, alias string) (context.Context, error) { return ctx, givenOwnersFileWithAlias(world.FromContext(ctx), alias) @@ -24,9 +25,6 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { sc.Step(`^an OWNERS_ALIASES file mapping "([^"]+)" to the bot$`, func(ctx context.Context, alias string) (context.Context, error) { return ctx, givenOwnersAliasesFile(world.FromContext(ctx), alias) }) - sc.Step(`^an OWNERS file listing the bot as a reviewer only$`, func(ctx context.Context) (context.Context, error) { - return ctx, givenOwnersFileWithBotReviewerOnly(world.FromContext(ctx)) - }) sc.Step(`^OWNERS authorization is enabled$`, func(ctx context.Context) (context.Context, error) { return ctx, givenOwnersAuthEnabled(world.FromContext(ctx)) }) @@ -39,75 +37,72 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { sc.Step(`^an issue is opened for OWNERS auth testing$`, func(ctx context.Context) (context.Context, error) { return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx)) }) + sc.Step(`^an OWNERS file listing the outsider as a reviewer$`, func(ctx context.Context) (context.Context, error) { + return ctx, givenOwnersFileWithOutsiderReviewer(world.FromContext(ctx)) + }) + sc.Step(`^the outsider posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { + return ctx, whenOutsiderPostsCommand(world.FromContext(ctx), command) + }) + sc.Step(`^the dispatch run does not authorize via OWNERS$`, func(ctx context.Context) (context.Context, error) { + return ctx, thenDispatchRunDoesNotAuthorizeViaOwners(world.FromContext(ctx)) + }) } -func givenOwnersFileWithBot(w *world.World) error { +func resolveBotLogin(w *world.World) (string, error) { ghDriver, ok := w.SCM.(*scmgh.Driver) if !ok { - return fmt.Errorf("OWNERS test requires GitHub SCM driver") - } - botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) - if err != nil { - return fmt.Errorf("resolving bot login: %w", err) - } - owners := fmt.Sprintf("approvers:\n - %s\nreviewers: []\n", botLogin) - if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), - "OWNERS", "behaviour: add OWNERS file for auth test", - []byte(owners)); err != nil { - return fmt.Errorf("committing OWNERS file: %w", err) + return "", fmt.Errorf("OWNERS test requires GitHub SCM driver") } - w.OwnersAuthActivated = true - return nil + return ghDriver.Client.GetAuthenticatedUser(context.Background()) } -func givenOwnersFileWithAlias(w *world.World, alias string) error { - owners := fmt.Sprintf("approvers:\n - %s\n", alias) +func commitFile(w *world.World, path, message, content string) error { if err := w.SCM.CommitFile(context.Background(), w.Install.ConfigOwner(), w.Install.ConfigRepo(), - "OWNERS", "behaviour: add OWNERS file with alias for auth test", - []byte(owners)); err != nil { - return fmt.Errorf("committing OWNERS file: %w", err) + path, message, []byte(content)); err != nil { + return fmt.Errorf("committing %s: %w", path, err) } - w.OwnersAuthActivated = true return nil } -func givenOwnersAliasesFile(w *world.World, alias string) error { - ghDriver, ok := w.SCM.(*scmgh.Driver) - if !ok { - return fmt.Errorf("OWNERS test requires GitHub SCM driver") - } - botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) +func givenBotInOwners(w *world.World, role string) error { + login, err := resolveBotLogin(w) if err != nil { return fmt.Errorf("resolving bot login: %w", err) } - aliases := fmt.Sprintf("aliases:\n %s:\n - %s\n", alias, botLogin) - if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), - "OWNERS_ALIASES", "behaviour: add OWNERS_ALIASES for auth test", - []byte(aliases)); err != nil { - return fmt.Errorf("committing OWNERS_ALIASES file: %w", err) + var content string + switch role { + case "approver": + content = fmt.Sprintf("approvers:\n - %s\nreviewers: []\n", login) + case "reviewer": + content = fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", login) + default: + return fmt.Errorf("unknown OWNERS role %q", role) + } + if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", content); err != nil { + return err } w.OwnersAuthActivated = true return nil } -func givenOwnersFileWithBotReviewerOnly(w *world.World) error { - ghDriver, ok := w.SCM.(*scmgh.Driver) - if !ok { - return fmt.Errorf("OWNERS test requires GitHub SCM driver") +func givenOwnersFileWithAlias(w *world.World, alias string) error { + if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", + fmt.Sprintf("approvers:\n - %s\n", alias)); err != nil { + return err } - botLogin, err := ghDriver.Client.GetAuthenticatedUser(context.Background()) + w.OwnersAuthActivated = true + return nil +} + +func givenOwnersAliasesFile(w *world.World, alias string) error { + login, err := resolveBotLogin(w) if err != nil { return fmt.Errorf("resolving bot login: %w", err) } - owners := fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", botLogin) - if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), - "OWNERS", "behaviour: add OWNERS file (reviewer only) for auth test", - []byte(owners)); err != nil { - return fmt.Errorf("committing OWNERS file: %w", err) + if err := commitFile(w, "OWNERS_ALIASES", "behaviour: add OWNERS_ALIASES for auth test", + fmt.Sprintf("aliases:\n %s:\n - %s\n", alias, login)); err != nil { + return err } w.OwnersAuthActivated = true return nil @@ -149,7 +144,7 @@ func whenIssueOpenedForOwnersAuth(w *world.World) error { w.RepoName = w.Install.TestRepo() w.RepoFull = w.Org + "/" + w.RepoName } - w.ScenarioStart = time.Now() + w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) w.TriageTriggerEvent = issueOpenEvent title := fmt.Sprintf("behaviour-owners-auth-%d", time.Now().UnixNano()) body := "Behaviour test issue for OWNERS authorization path." @@ -195,8 +190,95 @@ func getWorkflowLogs(w *world.World) (string, error) { w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) } -// disableOwnersAuth sets authorization.owners_file: false (removing the -// authorization block) in the enrolled repo's config.yaml. +func requireOutsider(w *world.World) error { + if w.OutsiderSCM == nil { + return fmt.Errorf("TEST_ACTOR_OUTSIDER_PAT not set") + } + return nil +} + +func givenOwnersFileWithOutsiderReviewer(w *world.World) error { + if err := requireOutsider(w); err != nil { + return err + } + if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", + fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", w.OutsiderLogin)); err != nil { + return err + } + w.OwnersAuthActivated = true + return nil +} + +func whenOutsiderPostsCommand(w *world.World, command string) error { + if err := requireOutsider(w); err != nil { + return err + } + if w.IssueNumber == 0 { + return fmt.Errorf("no issue created") + } + w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) + _, err := w.OutsiderSCM.AddComment(context.Background(), + w.RepoOwner, w.RepoName, w.IssueNumber, command) + return err +} + +func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { + gaciDriver, ok := w.CI.(*gaci.Driver) + if !ok { + return fmt.Errorf("dispatch run check requires GitHub Actions CI driver") + } + run, err := waitForDispatchRun(gaciDriver, w) + if err != nil { + return err + } + logs, err := gaciDriver.Client.GetWorkflowRunLogs(context.Background(), + w.RepoOwner, w.Install.TriageWorkflowRepo(), run.ID) + if err != nil { + return fmt.Errorf("fetching dispatch run logs: %w", err) + } + if strings.Contains(logs, "authorized via OWNERS file") { + return fmt.Errorf("dispatch run logs unexpectedly contain OWNERS authorization") + } + if !strings.Contains(logs, "No stage matched") { + return fmt.Errorf("dispatch run logs do not contain 'No stage matched' — dispatch may have proceeded via a non-OWNERS path") + } + return nil +} + +func waitForDispatchRun(driver *gaci.Driver, w *world.World) (*forge.WorkflowRun, error) { + workflowFile := filepath.Base(w.Install.TriageWorkflowFile()) + ctx := context.Background() + + const poll = 5 * time.Second + deadline := time.Now().Add(12 * time.Minute) + + for time.Now().Before(deadline) { + time.Sleep(poll) + runs, err := driver.Client.ListWorkflowRuns(ctx, + w.RepoOwner, w.Install.TriageWorkflowRepo(), workflowFile) + if err != nil { + continue + } + for _, run := range runs { + if run.Event != "issue_comment" { + continue + } + if w.OutsiderLogin != "" && run.ActorLogin != w.OutsiderLogin { + continue + } + runTime, parseErr := time.Parse(time.RFC3339, run.CreatedAt) + if parseErr != nil || runTime.Before(w.ScenarioStart) { + continue + } + if run.Status == "completed" { + return &run, nil + } + } + } + + return nil, fmt.Errorf("dispatch workflow (issue_comment, actor=%s) did not complete within deadline", w.OutsiderLogin) +} + func disableOwnersAuth(w *world.World) error { cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), @@ -222,9 +304,6 @@ func disableOwnersAuth(w *world.World) error { return nil } -// cleanupOwnersAuth removes the OWNERS file and authorization config -// block committed during the scenario so the repo slot is clean for -// the next scenario. func cleanupOwnersAuth(w *world.World) { ctx := context.Background() owner := w.Install.ConfigOwner() diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index bab23d6cdf..ecd55ef590 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -85,6 +85,13 @@ type World struct { // Nil when no driver is configured. Driver install.Driver + // OutsiderSCM is an SCM driver authenticated as fstest-outsider, a + // GitHub User with no collaborator access. Used by the OWNERS + // write-denial E2E scenario. Nil when TEST_ACTOR_OUTSIDER_PAT is + // not set. Shared across scenarios, never reset. + OutsiderSCM scm.Driver + OutsiderLogin string + // KillSwitchActivated records whether this scenario activated the // repo-level kill switch. CleanupScenario uses this to deactivate // the switch so the next scenario on this slot is not affected. From f355cf07bb69840cad31f4759561e0c5f3db10b6 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 13 Aug 2026 18:55:36 +0200 Subject: [PATCH 05/12] fix(#6042): address review comments -- security fixes, test 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 #6072. Signed-off-by: RaphaelBut --- .github/workflows/reusable-dispatch.yml | 26 ++-- ...thorization-on-all-agent-dispatch-paths.md | 12 +- docs/contributing/workflow-contracts.md | 2 + .../layered-config-reference.md | 32 +++-- .../features/dispatch/owners-auth.feature | 46 ++++-- internal/config/interfaces.go | 2 + internal/config/interfaces_test.go | 24 ++++ internal/harnessdispatch/core.go | 20 ++- internal/harnessdispatch/core_test.go | 114 ++++++++++++++- internal/owners/owners.go | 14 ++ .../.github/workflows/dispatch.yml | 8 +- .../scaffold/workflow_call_alignment_test.go | 2 +- pkg/behaviourtest/steps/artifacts.go | 1 + pkg/behaviourtest/steps/owners.go | 135 +++++++----------- 14 files changed, 307 insertions(+), 131 deletions(-) diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index ae0f61cd26..c15d406d0f 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -131,8 +131,8 @@ jobs: allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | .fullsend/config.yaml - OWNERS - OWNERS_ALIASES + /OWNERS + /OWNERS_ALIASES sparse-checkout-cone-mode: false - name: Determine stage @@ -205,13 +205,13 @@ jobs: case "${min}" in write|triage) if _owners_has_user approvers "${lc_user}"; then - echo "::notice::User '${username}' authorized via OWNERS file (approver, requested: ${min})" + echo "::notice::OWNERS file resolved user '${username}' as approver (requested: ${min})" return 0 fi ;;& triage) if _owners_has_user reviewers "${lc_user}"; then - echo "::notice::User '${username}' authorized via OWNERS file (reviewer, requested: ${min})" + echo "::notice::OWNERS file resolved user '${username}' as reviewer (requested: ${min})" return 0 fi ;; @@ -656,7 +656,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -767,7 +767,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -905,7 +905,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1034,7 +1034,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1312,7 +1312,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1422,7 +1422,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} @@ -1523,8 +1523,8 @@ jobs: allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | .fullsend/ - OWNERS - OWNERS_ALIASES + /OWNERS + /OWNERS_ALIASES .defaults/action.yml .defaults/.github/actions/install-fullsend-cli/ .fullsend/.defaults/action.yml @@ -1680,7 +1680,7 @@ jobs: - name: Checkout config repository uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: - ref: ${{ github.event_name == 'pull_request_target' && github.event.pull_request.base.sha || github.sha }} + ref: ${{ (github.event_name == 'pull_request_target' || github.event_name == 'pull_request_review') && github.event.pull_request.base.sha || github.sha }} persist-credentials: false allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} 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 eae3b315a2..176b7cbbab 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 @@ -202,8 +202,16 @@ permission list, not by bypassing the check. > authors cannot self-authorize by modifying OWNERS in their PR. > This follows the extension path above (extending the allowed permission > sources in `has_repo_permission`) rather than bypassing the check. -> OWNERS auth applies to built-in stages only; harness agents are -> unaffected. +> OWNERS auth applies to both built-in stages (bash routing) and the +> harness/custom-agent dispatch path (`internal/harnessdispatch`), where +> `owners.Resolve` computes an effective role for the `IsAuthorized` +> gate without mutating the original event. +> +> OWNERS reviewer access (triage-equivalent) applies to built-in +> bash-routed stages only (e.g. `/fs-triage`, `/fs-review`). Custom +> harness dispatch requires write-level access — OWNERS approver or +> GitHub write+ collaborator — because `IsAuthorized` gates all +> harness triggers at the write level. ## Consequences diff --git a/docs/contributing/workflow-contracts.md b/docs/contributing/workflow-contracts.md index 052e6d86f5..58bff78f75 100644 --- a/docs/contributing/workflow-contracts.md +++ b/docs/contributing/workflow-contracts.md @@ -9,6 +9,8 @@ **Silent failures and required-flag consistency:** Omitting a secret that is `required: true` at every hop in the chain fails loudly at workflow-call validation time and self-enforces. However, a secret whose `required` flag is `false` at any upstream hop can still arrive as an empty string at a downstream `required: true` consumer — GitHub Actions' required-secret validation only checks key presence, not that the resolved value is non-empty. For example, `FULLSEND_GCP_WIF_PROVIDER` is `required: false` in `reusable-dispatch.yml` but `required: true` in every downstream `reusable-.yml`, so an installer that never sets it satisfies the key-presence check while the actual value is empty. Treat a missing forwarding hop the same as a missing sync — it is a correctness bug, not a cosmetic issue. Required-flag consistency across the *whole* chain matters, not just the flag at the final consumer. +**OWNERS-file authorization:** `has_repo_permission` in both `reusable-dispatch.yml` and the scaffold `dispatch.yml` supports an opt-in OWNERS-file path gated by `authorization.owners_file: true` in `.fullsend/config.yaml`. When enabled, the function checks the repo-root `OWNERS` (and `OWNERS_ALIASES` if present) before falling back to the collaborator API. Approvers get write-equivalent access; reviewers get triage-equivalent. Changes to `_owners_has_user` or the OWNERS authorization block must be applied to both workflow files — `TestDispatchPerStageAuthorization` checks `has_repo_permission` parity but does not yet cover OWNERS-specific logic. The Go harness-dispatch path (`internal/harnessdispatch/core.go`) has equivalent OWNERS resolution via `internal/owners`; changes to the OWNERS schema or role mapping must be kept in sync across both implementations. + **Security — consuming threaded inputs:** When a newly-threaded entry carries user- or event-controlled data, consume it via `env:` in the final `run:` step — never interpolate `${{ ... }}` directly into a shell block (see the Security note atop `reusable-dispatch.yml`). This prevents the GHA script-injection class of bugs the project defends against elsewhere. **When reviewing PRs:** If a diff adds or renames a `secrets:` or `inputs:` entry in a reusable workflow, check that all callers in both chains have been updated. Flag a missing forwarding hop as a medium-severity or higher finding. New secrets/inputs must be forwarded only to the hop(s)/stage(s) that need them — do not use `secrets: inherit` as a substitute for explicit forwarding (`OTEL_EXPORTER_OTLP_TRACES_HEADERS`, for example, is explicitly forwarded to every inline stage job in `reusable-dispatch.yml` because each stage runs an agent that emits traces). `workflow_call_alignment_test.go` already automates much of this verification — see `TestWorkflowCallInputAlignment` (validates required inputs/secrets are threaded through both chains) and `TestOTELHeadersSecretThreading` (bespoke test for optional secrets). For new optional secrets/inputs, extend those tests or add a similar one rather than relying solely on manual tracing. diff --git a/docs/guides/infrastructure/layered-config-reference.md b/docs/guides/infrastructure/layered-config-reference.md index 4395b44c7b..1c52159fe1 100644 --- a/docs/guides/infrastructure/layered-config-reference.md +++ b/docs/guides/infrastructure/layered-config-reference.md @@ -92,10 +92,12 @@ the overlay → base → code defaults chain. | `status_notifications` | `*StatusNotificationConfig` | Replace whole object if set | `nil` | | `authorization`¹ | `object` | Overlay only (not layered) | `nil` | -> ¹ `authorization` is read by the dispatch workflow's bash/yq directly -> from `.fullsend/config.yaml` (the overlay). It does **not** participate -> in the overlay → base → code-defaults merge chain and is not part of the -> Go config package. Setting it in `config.base.yaml` has no effect. +> ¹ `authorization` is part of the Go config package +> (`AuthorizationConfig` / `AuthorizationOwnersFile()`) and is consumed +> by both the dispatch workflow's bash/yq and `internal/harnessdispatch`. +> However, it is intentionally **overlay-only**: `AuthorizationOwnersFile()` +> does not fall through to the parent config, so setting it in +> `config.base.yaml` has no effect — each repo must opt in explicitly. > See [#6072](https://github.com/fullsend-ai/fullsend/issues/6072) for > the planned migration to the Go config layer. @@ -381,11 +383,21 @@ for the dispatch workflow's `has_repo_permission` check. Currently supports one sub-field: - `owners_file` (`bool`, default `false`) — when `true`, the dispatch - routing logic checks the repo-root `OWNERS` and `OWNERS_ALIASES` - files before falling back to the GitHub collaborator API. OWNERS + routing logic checks the repo-root `OWNERS` file (and `OWNERS_ALIASES` + if present) before falling back to the GitHub collaborator API. OWNERS approvers get write-equivalent access; reviewers get - triage-equivalent. See [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md) - for details. + triage-equivalent. If the user is not listed in OWNERS, authorization + falls through to the collaborator API — OWNERS never blocks a + collaborator who isn't in the file. + +This applies to both the bash routing path (built-in stages) and the +Go harness-dispatch path (custom agents). A missing or malformed OWNERS +file fails closed: the OWNERS check is skipped and authorization falls +through to the collaborator API. + +v1 limitation: only the repo-root flat `approvers`/`reviewers` lists +are read. Prow `filters:` blocks and nested per-directory OWNERS files +are not supported. Example: @@ -394,8 +406,8 @@ authorization: owners_file: true ``` -This field is read by `yq` in the workflow bash routing logic, not by -the Go config package. It is only meaningful in per-repo mode. +See [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md) +for the full design rationale. ## Code defaults reference diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature index 47ae1ca5d7..b22244e79e 100644 --- a/e2e/behaviour/features/dispatch/owners-auth.feature +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -5,47 +5,52 @@ Feature: OWNERS file authorization for bash routing trigger via issues.opened, which unconditionally calls is_event_actor_authorized and exercises has_repo_permission. - The e2e bot already has collaborator access, so these scenarios - confirm the OWNERS code path is reached (via audit log) rather - than testing the API-fallback denial path — that requires a - restricted identity without collaborator access (#6072). + Bot scenarios confirm the OWNERS code path is reached (via audit + log); the bot has collaborator access so the API fallback would + also grant. The outsider identity (TEST_ACTOR_OUTSIDER_PAT) has + no collaborator access, so authorization succeeds only through + OWNERS — the denial scenario verifies a reviewer cannot escalate + to write-level access. Background: Given the enrolled test repository Scenario: Triage dispatches via OWNERS approver path when enabled - Given an OWNERS file listing the bot as an approver + Given an OWNERS file listing the outsider as an approver And OWNERS authorization is enabled And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-ok.json, fixtures/dispatch/ok.json | - When an issue is opened for OWNERS auth testing + When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "authorized via OWNERS file (approver" + And the triage workflow logs contain "OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested:" Scenario: OWNERS alias resolves to grant access Given an OWNERS file with alias "test-team" as approver - And an OWNERS_ALIASES file mapping "test-team" to the bot + And an OWNERS_ALIASES file mapping "test-team" to the outsider And OWNERS authorization is enabled And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-alias-ok.json, fixtures/dispatch/ok.json | - When an issue is opened for OWNERS auth testing + When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "authorized via OWNERS file (approver" + And the triage workflow logs contain "OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested:" Scenario: OWNERS reviewer can triage - Given an OWNERS file listing the bot as a reviewer only + Given an OWNERS file listing the outsider as a reviewer only And OWNERS authorization is enabled And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-rev-ok.json, fixtures/dispatch/ok.json | - When an issue is opened for OWNERS auth testing + When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "authorized via OWNERS file (reviewer" + And the triage workflow logs contain "OWNERS file resolved user" + And the triage workflow logs contain "as reviewer (requested:" Scenario: OWNERS reviewer is denied write-level access Given an OWNERS file listing the outsider as a reviewer @@ -54,12 +59,23 @@ Feature: OWNERS file authorization for bash routing When the outsider posts "/fs-code" on the issue Then the dispatch run does not authorize via OWNERS + Scenario: Unlisted collaborator falls through to API authorization + Given an OWNERS file listing the outsider as a reviewer only + And OWNERS authorization is enabled + And a dummy agent that would: + | description | op | args | + | Prove execution | write_fixture | output/owners-fallback-ok.json, fixtures/dispatch/ok.json | + When the bot opens an issue for OWNERS auth testing + Then the triage workflow completes successfully + And the agent will succeed to Prove execution + And the triage workflow logs do not contain "OWNERS file resolved user" + Scenario: Triage dispatches without OWNERS path when not opted in Given an OWNERS file listing the bot as an approver And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/owners-off-ok.json, fixtures/dispatch/ok.json | - When an issue is opened for OWNERS auth testing + When the bot opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs do not contain "authorized via OWNERS file" + And the triage workflow logs do not contain "OWNERS file resolved user" diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index dac42aab67..4ddbe54823 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -410,6 +410,8 @@ func (c *perRepoConfig) ConfigVersion() string { func (c *perRepoConfig) IsOrgMode() bool { return false } // AuthorizationOwnersFile returns whether OWNERS-file authorization is enabled. +// Intentionally no parent fallback: OWNERS auth is a per-repo opt-in that must +// not be inheritable from config.base.yaml. func (c *perRepoConfig) AuthorizationOwnersFile() bool { return c.Authorization != nil && c.Authorization.OwnersFile } diff --git a/internal/config/interfaces_test.go b/internal/config/interfaces_test.go index 435e1e164b..94c57480ee 100644 --- a/internal/config/interfaces_test.go +++ b/internal/config/interfaces_test.go @@ -891,3 +891,27 @@ func TestPerRepoConfig_InferenceOpenAI_Fallback(t *testing.T) { }) assert.Equal(t, []string{"identity_provider_id", "service_account_id"}, OpenAIWIFConfig{Audience: "a"}.Missing()) } + +// --- AuthorizationOwnersFile: intentionally no parent fallback --- + +func TestPerRepoConfig_AuthorizationOwnersFile_NoFallback(t *testing.T) { + t.Run("returns false when unset", func(t *testing.T) { + cfg := &perRepoConfig{} + assert.False(t, cfg.AuthorizationOwnersFile()) + }) + + t.Run("does not fall through to parent", func(t *testing.T) { + parent := &perRepoConfig{ + Authorization: &AuthorizationConfig{OwnersFile: true}, + } + child := &perRepoConfig{parent: parent} + assert.False(t, child.AuthorizationOwnersFile()) + }) + + t.Run("returns true when set locally", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: &AuthorizationConfig{OwnersFile: true}, + } + assert.True(t, cfg.AuthorizationOwnersFile()) + }) +} diff --git a/internal/harnessdispatch/core.go b/internal/harnessdispatch/core.go index df6b8f3596..66964b1196 100644 --- a/internal/harnessdispatch/core.go +++ b/internal/harnessdispatch/core.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "log" + "path/filepath" "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/fetch" @@ -40,18 +41,27 @@ func Dispatch(ctx context.Context, opts Options) ([]ExecutionRef, error) { return nil, nil } - // Upgrade Actor.Role in place if OWNERS grants a higher level. - // Bare paths — same working-directory assumption as ConfigDir. + // Compute the effective role for the auth gate without mutating the + // caller's event. The OWNERS-upgraded role is used only for + // IsAuthorized; downstream CEL evaluation sees the original + // collaborator-API role. + repoRoot := filepath.Dir(opts.ConfigDir) + effectiveRole := opts.Event.Actor.Role if dirCfg.AuthorizationOwnersFile() && opts.Event.Actor.ID != "" { - role, err := owners.Resolve("OWNERS", "OWNERS_ALIASES", opts.Event.Actor.ID) + ownersPath := filepath.Join(repoRoot, "OWNERS") + aliasesPath := filepath.Join(repoRoot, "OWNERS_ALIASES") + role, err := owners.Resolve(ownersPath, aliasesPath, opts.Event.Actor.ID) if err != nil { log.Printf("harness dispatch: OWNERS resolution failed for %s: %v", opts.Event.Actor.ID, err) } else if role != owners.None { - opts.Event.Actor.Role = owners.MapToActorRole(role, opts.Event.Actor.Role) + effectiveRole = owners.MapToActorRole(role, effectiveRole) + log.Printf("harness dispatch: OWNERS file resolved user %s as %s", opts.Event.Actor.ID, role) } } - if !IsAuthorized(opts.Event) { + authCheck := *opts.Event + authCheck.Actor.Role = effectiveRole + if !IsAuthorized(&authCheck) { return nil, nil } diff --git a/internal/harnessdispatch/core_test.go b/internal/harnessdispatch/core_test.go index da38168652..ab6161736a 100644 --- a/internal/harnessdispatch/core_test.go +++ b/internal/harnessdispatch/core_test.go @@ -87,6 +87,18 @@ func mustEvent(t *testing.T, name string) *normevent.Event { return ev } +func issueOpenedHarnessYAML() string { + return `agent: agents/triage.md +role: triage +slug: fullsend-ai-issue-triage +model: opus +image: ghcr.io/fullsend-ai/fullsend-sandbox:latest +trigger: | + event.entity.kind == "work_item" + && event.transition.kind == "opened" +` +} + func issuePingHarnessYAML() string { return `agent: agents/triage.md role: triage @@ -100,6 +112,82 @@ trigger: | ` } +func TestDispatch_OwnersUpgradesActorRole(t *testing.T) { + // Use issue-opened (not label-added) so IsAuthorized requires + // write-level access. Without the OWNERS approver upgrade the + // actor's RoleNone would be denied — this proves the OWNERS path + // is actually exercised. + ev := mustEvent(t, "issue-opened.json") + + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issueOpenedHarnessYAML(), func(cfg config.PerRepoConfigWriter) { + cfg.SetAuthorizationOwnersFile(true) + }) + ev.Actor.ID = "test-approver" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers:\n - test-approver\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + require.Len(t, refs, 1, "OWNERS approver upgrade should grant write-level access") + // The caller's event must NOT be mutated — the OWNERS upgrade is + // used only for the auth gate, not leaked to downstream CEL or + // the caller. + assert.Equal(t, normevent.RoleNone, ev.Actor.Role) +} + +func TestDispatch_OwnersReviewerDeniedWriteLevel(t *testing.T) { + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issuePingHarnessYAML(), func(cfg config.PerRepoConfigWriter) { + cfg.SetAuthorizationOwnersFile(true) + }) + + ev := mustEvent(t, "issue-opened.json") + ev.Actor.ID = "test-reviewer" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers: []\nreviewers:\n - test-reviewer\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + assert.Empty(t, refs, "OWNERS reviewer should be denied: triage-equivalent does not satisfy write-level harness auth") +} + +func TestDispatch_OwnersDisabledNoUpgrade(t *testing.T) { + // Use issue-opened (not label-added) so IsAuthorized requires + // write-level access. The OWNERS file lists the actor as an + // approver, but with the config flag off, OWNERS should not be + // consulted — the actor stays at RoleNone and is denied. + ev := mustEvent(t, "issue-opened.json") + + dir := t.TempDir() + + configDir := writeHarnessConfigSubdir(t, dir, issueOpenedHarnessYAML()) + ev.Actor.ID = "test-approver" + ev.Actor.Role = normevent.RoleNone + + require.NoError(t, os.WriteFile( + filepath.Join(dir, "OWNERS"), + []byte("approvers:\n - test-approver\n"), + 0o644, + )) + + refs, err := Dispatch(context.Background(), Options{ConfigDir: configDir, Event: ev}) + require.NoError(t, err) + assert.Empty(t, refs, "OWNERS auth is disabled — approver in OWNERS should not grant access") +} + func TestDispatch_NilEvent(t *testing.T) { _, err := Dispatch(context.Background(), Options{ConfigDir: t.TempDir()}) require.Error(t, err) @@ -112,14 +200,38 @@ func TestMergedConfigAgents_InvalidYAML(t *testing.T) { require.Error(t, err) } -func writeHarnessConfig(t *testing.T, dir, harnessYAML string) { +func writeHarnessConfig(t *testing.T, dir, harnessYAML string, opts ...func(config.PerRepoConfigWriter)) { t.Helper() harnessDir := filepath.Join(dir, "harness") require.NoError(t, os.MkdirAll(harnessDir, 0o755)) require.NoError(t, os.WriteFile(filepath.Join(harnessDir, "issue-ping.yaml"), []byte(harnessYAML), 0o644)) cfg := config.NewPerRepoConfig(nil, "fullsend-ai/demo") cfg.SetAgents([]config.AgentEntry{{Name: "issue-ping", Source: "harness/issue-ping.yaml"}}) + for _, opt := range opts { + opt(cfg) + } data, err := yaml.Marshal(cfg) require.NoError(t, err) require.NoError(t, os.WriteFile(filepath.Join(dir, "config.yaml"), data, 0o644)) } + +// writeHarnessConfigSubdir creates a .fullsend/ subdirectory inside +// repoRoot that mirrors the production layout and returns its path for +// use as ConfigDir. This ensures filepath.Dir(configDir) resolves to +// repoRoot, which is required for OWNERS-file resolution. +func writeHarnessConfigSubdir(t *testing.T, repoRoot, harnessYAML string, opts ...func(config.PerRepoConfigWriter)) string { + t.Helper() + configDir := filepath.Join(repoRoot, ".fullsend") + harnessDir := filepath.Join(configDir, "harness") + require.NoError(t, os.MkdirAll(harnessDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(harnessDir, "issue-ping.yaml"), []byte(harnessYAML), 0o644)) + cfg := config.NewPerRepoConfig(nil, "fullsend-ai/demo") + cfg.SetAgents([]config.AgentEntry{{Name: "issue-ping", Source: "harness/issue-ping.yaml"}}) + for _, opt := range opts { + opt(cfg) + } + data, err := yaml.Marshal(cfg) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(configDir, "config.yaml"), data, 0o644)) + return configDir +} diff --git a/internal/owners/owners.go b/internal/owners/owners.go index 67a2678665..4480bd8eca 100644 --- a/internal/owners/owners.go +++ b/internal/owners/owners.go @@ -22,6 +22,20 @@ const ( Approver // write-equivalent ) +func (r Role) String() string { + switch r { + case Reviewer: + return "reviewer" + case Approver: + return "approver" + default: + return "none" + } +} + +// ownersFile represents a root-level Prow OWNERS file with flat +// approvers/reviewers lists. v1 limitation: filters: blocks and +// nested per-directory OWNERS files are not supported. type ownersFile struct { Approvers []string `yaml:"approvers"` Reviewers []string `yaml:"reviewers"` diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index e62117876a..f08ef9ca6d 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -38,8 +38,8 @@ jobs: allow-unsafe-pr-checkout: ${{ github.event_name == 'pull_request_target' }} sparse-checkout: | .fullsend/config.yaml - OWNERS - OWNERS_ALIASES + /OWNERS + /OWNERS_ALIASES sparse-checkout-cone-mode: false - name: Determine stage @@ -112,13 +112,13 @@ jobs: case "${min}" in write|triage) if _owners_has_user approvers "${lc_user}"; then - echo "::notice::User '${username}' authorized via OWNERS file (approver, requested: ${min})" + echo "::notice::OWNERS file resolved user '${username}' as approver (requested: ${min})" return 0 fi ;;& triage) if _owners_has_user reviewers "${lc_user}"; then - echo "::notice::User '${username}' authorized via OWNERS file (reviewer, requested: ${min})" + echo "::notice::OWNERS file resolved user '${username}' as reviewer (requested: ${min})" return 0 fi ;; diff --git a/internal/scaffold/workflow_call_alignment_test.go b/internal/scaffold/workflow_call_alignment_test.go index 28543c90aa..946b9d06da 100644 --- a/internal/scaffold/workflow_call_alignment_test.go +++ b/internal/scaffold/workflow_call_alignment_test.go @@ -660,7 +660,7 @@ func TestDispatchPerStageAuthorization(t *testing.T) { "OWNERS approver check must use lowercased lc_user, not original username") assert.Contains(t, s, `_owners_has_user reviewers "${lc_user}"`, "OWNERS reviewer check must use lowercased lc_user, not original username") - assert.Regexp(t, `::notice::User '\$\{username\}' authorized via OWNERS file`, s, + assert.Regexp(t, `::notice::OWNERS file resolved user '\$\{username\}'`, s, "OWNERS audit log must use original username casing, not lc_user") }) } diff --git a/pkg/behaviourtest/steps/artifacts.go b/pkg/behaviourtest/steps/artifacts.go index 95818482a1..206fa4bac0 100644 --- a/pkg/behaviourtest/steps/artifacts.go +++ b/pkg/behaviourtest/steps/artifacts.go @@ -12,6 +12,7 @@ import ( ) const issueOpenEvent = "issues" +const issueCommentEvent = "issue_comment" func triageWorkflowEvent(w *world.World) string { if w.TriageTriggerEvent != "" { diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index 540a437e83..9e34ad08e7 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -10,20 +10,19 @@ import ( "github.com/cucumber/godog" "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" - gaci "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/ci/githubactions" scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) func registerOwnersSteps(sc *godog.ScenarioContext) { - sc.Step(`^an OWNERS file listing the bot as (?:an? )?(approver|reviewer)(?: only)?$`, func(ctx context.Context, role string) (context.Context, error) { - return ctx, givenBotInOwners(world.FromContext(ctx), role) + sc.Step(`^an OWNERS file listing the (bot|outsider) as (?:an? )?(approver|reviewer)(?: only)?$`, func(ctx context.Context, actor, role string) (context.Context, error) { + return ctx, givenActorInOwners(world.FromContext(ctx), actor, role) }) sc.Step(`^an OWNERS file with alias "([^"]+)" as approver$`, func(ctx context.Context, alias string) (context.Context, error) { return ctx, givenOwnersFileWithAlias(world.FromContext(ctx), alias) }) - sc.Step(`^an OWNERS_ALIASES file mapping "([^"]+)" to the bot$`, func(ctx context.Context, alias string) (context.Context, error) { - return ctx, givenOwnersAliasesFile(world.FromContext(ctx), alias) + sc.Step(`^an OWNERS_ALIASES file mapping "([^"]+)" to the (bot|outsider)$`, func(ctx context.Context, alias, actor string) (context.Context, error) { + return ctx, givenOwnersAliasesFile(world.FromContext(ctx), alias, actor) }) sc.Step(`^OWNERS authorization is enabled$`, func(ctx context.Context) (context.Context, error) { return ctx, givenOwnersAuthEnabled(world.FromContext(ctx)) @@ -34,11 +33,8 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { sc.Step(`^the triage workflow logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { return ctx, thenWorkflowLogsDoNotContain(world.FromContext(ctx), needle) }) - sc.Step(`^an issue is opened for OWNERS auth testing$`, func(ctx context.Context) (context.Context, error) { - return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx)) - }) - sc.Step(`^an OWNERS file listing the outsider as a reviewer$`, func(ctx context.Context) (context.Context, error) { - return ctx, givenOwnersFileWithOutsiderReviewer(world.FromContext(ctx)) + sc.Step(`^the (bot|outsider) opens an issue for OWNERS auth testing$`, func(ctx context.Context, actor string) (context.Context, error) { + return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx), actor) }) sc.Step(`^the outsider posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { return ctx, whenOutsiderPostsCommand(world.FromContext(ctx), command) @@ -48,12 +44,22 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { }) } -func resolveBotLogin(w *world.World) (string, error) { - ghDriver, ok := w.SCM.(*scmgh.Driver) - if !ok { - return "", fmt.Errorf("OWNERS test requires GitHub SCM driver") +func resolveActorLogin(w *world.World, actor string) (string, error) { + switch actor { + case "bot": + ghDriver, ok := w.SCM.(*scmgh.Driver) + if !ok { + return "", fmt.Errorf("OWNERS test requires GitHub SCM driver") + } + return ghDriver.Client.GetAuthenticatedUser(context.Background()) + case "outsider": + if err := requireOutsider(w); err != nil { + return "", err + } + return w.OutsiderLogin, nil + default: + return "", fmt.Errorf("unknown actor %q", actor) } - return ghDriver.Client.GetAuthenticatedUser(context.Background()) } func commitFile(w *world.World, path, message, content string) error { @@ -65,10 +71,10 @@ func commitFile(w *world.World, path, message, content string) error { return nil } -func givenBotInOwners(w *world.World, role string) error { - login, err := resolveBotLogin(w) +func givenActorInOwners(w *world.World, actor, role string) error { + login, err := resolveActorLogin(w, actor) if err != nil { - return fmt.Errorf("resolving bot login: %w", err) + return err } var content string switch role { @@ -95,10 +101,10 @@ func givenOwnersFileWithAlias(w *world.World, alias string) error { return nil } -func givenOwnersAliasesFile(w *world.World, alias string) error { - login, err := resolveBotLogin(w) +func givenOwnersAliasesFile(w *world.World, alias, actor string) error { + login, err := resolveActorLogin(w, actor) if err != nil { - return fmt.Errorf("resolving bot login: %w", err) + return err } if err := commitFile(w, "OWNERS_ALIASES", "behaviour: add OWNERS_ALIASES for auth test", fmt.Sprintf("aliases:\n %s:\n - %s\n", alias, login)); err != nil { @@ -134,21 +140,22 @@ func givenOwnersAuthEnabled(w *world.World) error { return nil } -// whenIssueOpenedForOwnersAuth creates an issue without draining the -// issues.opened workflow run. The issues.opened path unconditionally -// calls is_event_actor_authorized, which exercises has_repo_permission -// and the OWNERS authorization code path. -func whenIssueOpenedForOwnersAuth(w *world.World) error { +func whenIssueOpenedForOwnersAuth(w *world.World, actor string) error { if w.RepoOwner == "" || w.RepoName == "" { - w.RepoOwner = w.Org - w.RepoName = w.Install.TestRepo() - w.RepoFull = w.Org + "/" + w.RepoName + return fmt.Errorf("no repo configured; call 'Given the enrolled test repository' before creating issues") + } + scmDriver := w.SCM + if actor == "outsider" { + if err := requireOutsider(w); err != nil { + return err + } + scmDriver = w.OutsiderSCM } w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) w.TriageTriggerEvent = issueOpenEvent title := fmt.Sprintf("behaviour-owners-auth-%d", time.Now().UnixNano()) body := "Behaviour test issue for OWNERS authorization path." - issue, err := w.SCM.CreateIssue(context.Background(), w.RepoOwner, w.RepoName, title, body) + issue, err := scmDriver.CreateIssue(context.Background(), w.RepoOwner, w.RepoName, title, body) if err != nil { return err } @@ -197,18 +204,6 @@ func requireOutsider(w *world.World) error { return nil } -func givenOwnersFileWithOutsiderReviewer(w *world.World) error { - if err := requireOutsider(w); err != nil { - return err - } - if err := commitFile(w, "OWNERS", "behaviour: add OWNERS file for auth test", - fmt.Sprintf("approvers: []\nreviewers:\n - %s\n", w.OutsiderLogin)); err != nil { - return err - } - w.OwnersAuthActivated = true - return nil -} - func whenOutsiderPostsCommand(w *world.World, command string) error { if err := requireOutsider(w); err != nil { return err @@ -216,67 +211,47 @@ func whenOutsiderPostsCommand(w *world.World, command string) error { if w.IssueNumber == 0 { return fmt.Errorf("no issue created") } - w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) + w.ScenarioStart = time.Now() _, err := w.OutsiderSCM.AddComment(context.Background(), w.RepoOwner, w.RepoName, w.IssueNumber, command) return err } func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { - gaciDriver, ok := w.CI.(*gaci.Driver) - if !ok { - return fmt.Errorf("dispatch run check requires GitHub Actions CI driver") - } - run, err := waitForDispatchRun(gaciDriver, w) + run, err := waitForDispatchRun(w) if err != nil { return err } - logs, err := gaciDriver.Client.GetWorkflowRunLogs(context.Background(), + logs, err := w.CI.GetRunLogs(context.Background(), w.RepoOwner, w.Install.TriageWorkflowRepo(), run.ID) if err != nil { return fmt.Errorf("fetching dispatch run logs: %w", err) } - if strings.Contains(logs, "authorized via OWNERS file") { - return fmt.Errorf("dispatch run logs unexpectedly contain OWNERS authorization") + if strings.Contains(logs, "OWNERS file resolved user") { + return fmt.Errorf("dispatch run %d (%s) logs unexpectedly contain OWNERS authorization", run.ID, run.HTMLURL) } if !strings.Contains(logs, "No stage matched") { - return fmt.Errorf("dispatch run logs do not contain 'No stage matched' — dispatch may have proceeded via a non-OWNERS path") + return fmt.Errorf("dispatch run %d (%s) logs do not contain 'No stage matched' — dispatch may have proceeded via a non-OWNERS path", run.ID, run.HTMLURL) } return nil } -func waitForDispatchRun(driver *gaci.Driver, w *world.World) (*forge.WorkflowRun, error) { - workflowFile := filepath.Base(w.Install.TriageWorkflowFile()) +func waitForDispatchRun(w *world.World) (*forge.WorkflowRun, error) { ctx := context.Background() + repo := w.Install.TriageWorkflowRepo() + file := w.Install.TriageWorkflowFile() - const poll = 5 * time.Second - deadline := time.Now().Add(12 * time.Minute) - - for time.Now().Before(deadline) { - time.Sleep(poll) - runs, err := driver.Client.ListWorkflowRuns(ctx, - w.RepoOwner, w.Install.TriageWorkflowRepo(), workflowFile) - if err != nil { - continue - } - for _, run := range runs { - if run.Event != "issue_comment" { - continue - } - if w.OutsiderLogin != "" && run.ActorLogin != w.OutsiderLogin { - continue - } - runTime, parseErr := time.Parse(time.RFC3339, run.CreatedAt) - if parseErr != nil || runTime.Before(w.ScenarioStart) { - continue - } - if run.Status == "completed" { - return &run, nil - } - } + run, err := w.CI.WaitForWorkflow(ctx, w.RepoOwner, repo, file, w.ScenarioStart, issueCommentEvent) + if err == nil { + return run, nil } - return nil, fmt.Errorf("dispatch workflow (issue_comment, actor=%s) did not complete within deadline", w.OutsiderLogin) + worldLogf(w, "dispatch run: retrying with skew buffer: %v", err) + buffered := w.ScenarioStart.Add(-issueOpenDrainSkewBuffer) + if retryRun, retryErr := w.CI.WaitForWorkflow(ctx, w.RepoOwner, repo, file, buffered, issueCommentEvent); retryErr == nil { + return retryRun, nil + } + return nil, fmt.Errorf("waiting for dispatch workflow (issue_comment): %w", err) } func disableOwnersAuth(w *world.World) error { From f10d187f4c45298c4afdbb124be3538f0ee94c58 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Tue, 18 Aug 2026 13:13:47 +0200 Subject: [PATCH 06/12] fix(#6042): fix OWNERS E2E tests -- wrong fixture path and 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) Signed-off-by: RaphaelBut --- .../features/dispatch/owners-auth.feature | 34 +++++++++---------- pkg/behaviourtest/steps/owners.go | 5 +-- 2 files changed, 18 insertions(+), 21 deletions(-) diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature index b22244e79e..ece2d2f821 100644 --- a/e2e/behaviour/features/dispatch/owners-auth.feature +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -19,38 +19,38 @@ Feature: OWNERS file authorization for bash routing Given an OWNERS file listing the outsider as an approver And OWNERS authorization is enabled And a dummy agent that would: - | description | op | args | - | Prove execution | write_fixture | output/owners-ok.json, fixtures/dispatch/ok.json | + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "OWNERS file resolved user" - And the triage workflow logs contain "as approver (requested:" + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested: triage)" Scenario: OWNERS alias resolves to grant access Given an OWNERS file with alias "test-team" as approver And an OWNERS_ALIASES file mapping "test-team" to the outsider And OWNERS authorization is enabled And a dummy agent that would: - | description | op | args | - | Prove execution | write_fixture | output/owners-alias-ok.json, fixtures/dispatch/ok.json | + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "OWNERS file resolved user" - And the triage workflow logs contain "as approver (requested:" + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as approver (requested: triage)" Scenario: OWNERS reviewer can triage Given an OWNERS file listing the outsider as a reviewer only And OWNERS authorization is enabled And a dummy agent that would: - | description | op | args | - | Prove execution | write_fixture | output/owners-rev-ok.json, fixtures/dispatch/ok.json | + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | When the outsider opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs contain "OWNERS file resolved user" - And the triage workflow logs contain "as reviewer (requested:" + And the triage workflow logs contain "##[notice]OWNERS file resolved user" + And the triage workflow logs contain "as reviewer (requested: triage)" Scenario: OWNERS reviewer is denied write-level access Given an OWNERS file listing the outsider as a reviewer @@ -64,18 +64,18 @@ Feature: OWNERS file authorization for bash routing And OWNERS authorization is enabled And a dummy agent that would: | description | op | args | - | Prove execution | write_fixture | output/owners-fallback-ok.json, fixtures/dispatch/ok.json | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | When the bot opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs do not contain "OWNERS file resolved user" + And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" Scenario: Triage dispatches without OWNERS path when not opted in Given an OWNERS file listing the bot as an approver And a dummy agent that would: - | description | op | args | - | Prove execution | write_fixture | output/owners-off-ok.json, fixtures/dispatch/ok.json | + | description | op | args | + | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | When the bot opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution - And the triage workflow logs do not contain "OWNERS file resolved user" + And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index 9e34ad08e7..d83af5c420 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -227,12 +227,9 @@ func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { if err != nil { return fmt.Errorf("fetching dispatch run logs: %w", err) } - if strings.Contains(logs, "OWNERS file resolved user") { + if strings.Contains(logs, "##[notice]OWNERS file resolved user") { return fmt.Errorf("dispatch run %d (%s) logs unexpectedly contain OWNERS authorization", run.ID, run.HTMLURL) } - if !strings.Contains(logs, "No stage matched") { - return fmt.Errorf("dispatch run %d (%s) logs do not contain 'No stage matched' — dispatch may have proceeded via a non-OWNERS path", run.ID, run.HTMLURL) - } return nil } From 1327d751eeeea630a9933a12f91a258b07addc51 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 20 Aug 2026 16:36:12 +0200 Subject: [PATCH 07/12] fix(#6042): refactor authorization config to provider list, 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) Signed-off-by: RaphaelBut --- .github/workflows/reusable-dispatch.yml | 4 +-- ...thorization-on-all-agent-dispatch-paths.md | 2 +- docs/contributing/workflow-contracts.md | 2 +- .../layered-config-reference.md | 32 +++++++++---------- .../features/dispatch/owners-auth.feature | 20 ++++++------ e2e/behaviour/suite_test.go | 10 ++++++ internal/config/config.go | 32 ++++++++++++++----- internal/config/config_test.go | 28 ++++++++++++++++ internal/config/interfaces.go | 30 +++++++++++------ internal/config/interfaces_test.go | 4 +-- internal/forge/forge.go | 1 - internal/forge/github/github.go | 4 --- .../.github/workflows/dispatch.yml | 4 +-- pkg/behaviourtest/steps/owners.go | 22 +++++++++++-- pkg/behaviourtest/steps/owners_test.go | 11 ++++--- pkg/behaviourtest/world/world.go | 9 +++++- 16 files changed, 151 insertions(+), 64 deletions(-) diff --git a/.github/workflows/reusable-dispatch.yml b/.github/workflows/reusable-dispatch.yml index c15d406d0f..c0abbc52bd 100644 --- a/.github/workflows/reusable-dispatch.yml +++ b/.github/workflows/reusable-dispatch.yml @@ -192,12 +192,12 @@ jobs: local username="${1:-}" min="${2:-write}" role api_err [[ -z "${username}" ]] && return 1 - # OWNERS-file authorization (opt-in via authorization.owners_file in config.yaml). + # OWNERS-file authorization (opt-in via authorization providers list in config.yaml). # Approvers get write-equivalent access; reviewers get triage-equivalent. # Safe: sparse-checkout pins to base branch SHA, so PR authors cannot # self-authorize by adding themselves to OWNERS. if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then - if [[ "$(yq '.authorization.owners_file // false' .fullsend/config.yaml)" == "true" ]]; then + if [[ "$(yq '(.authorization // []) | any_c(.provider == "owners_file")' .fullsend/config.yaml)" == "true" ]]; then if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 else 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 176b7cbbab..3bd054fe23 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 @@ -193,7 +193,7 @@ permission list, not by bypassing the check. > Prow-based repositories (e.g., OpenShift) use OWNERS files rather than > GitHub collaborator roles to define contributor authority. > `has_repo_permission` now supports an opt-in OWNERS-file authorization -> path: when `authorization.owners_file: true` is set in +> path: when `owners_file` is listed in the `authorization` providers in > `.fullsend/config.yaml`, the function checks the repo-root `OWNERS` > (and `OWNERS_ALIASES`) before falling back to the collaborator API. > OWNERS approvers get write-equivalent access; reviewers get diff --git a/docs/contributing/workflow-contracts.md b/docs/contributing/workflow-contracts.md index 58bff78f75..2b28177719 100644 --- a/docs/contributing/workflow-contracts.md +++ b/docs/contributing/workflow-contracts.md @@ -9,7 +9,7 @@ **Silent failures and required-flag consistency:** Omitting a secret that is `required: true` at every hop in the chain fails loudly at workflow-call validation time and self-enforces. However, a secret whose `required` flag is `false` at any upstream hop can still arrive as an empty string at a downstream `required: true` consumer — GitHub Actions' required-secret validation only checks key presence, not that the resolved value is non-empty. For example, `FULLSEND_GCP_WIF_PROVIDER` is `required: false` in `reusable-dispatch.yml` but `required: true` in every downstream `reusable-.yml`, so an installer that never sets it satisfies the key-presence check while the actual value is empty. Treat a missing forwarding hop the same as a missing sync — it is a correctness bug, not a cosmetic issue. Required-flag consistency across the *whole* chain matters, not just the flag at the final consumer. -**OWNERS-file authorization:** `has_repo_permission` in both `reusable-dispatch.yml` and the scaffold `dispatch.yml` supports an opt-in OWNERS-file path gated by `authorization.owners_file: true` in `.fullsend/config.yaml`. When enabled, the function checks the repo-root `OWNERS` (and `OWNERS_ALIASES` if present) before falling back to the collaborator API. Approvers get write-equivalent access; reviewers get triage-equivalent. Changes to `_owners_has_user` or the OWNERS authorization block must be applied to both workflow files — `TestDispatchPerStageAuthorization` checks `has_repo_permission` parity but does not yet cover OWNERS-specific logic. The Go harness-dispatch path (`internal/harnessdispatch/core.go`) has equivalent OWNERS resolution via `internal/owners`; changes to the OWNERS schema or role mapping must be kept in sync across both implementations. +**OWNERS-file authorization:** `has_repo_permission` in both `reusable-dispatch.yml` and the scaffold `dispatch.yml` supports an opt-in OWNERS-file path gated by `owners_file` in the `authorization` providers list in `.fullsend/config.yaml`. When enabled, the function checks the repo-root `OWNERS` (and `OWNERS_ALIASES` if present) before falling back to the collaborator API. Approvers get write-equivalent access; reviewers get triage-equivalent. Changes to `_owners_has_user` or the OWNERS authorization block must be applied to both workflow files — `TestDispatchPerStageAuthorization` checks `has_repo_permission` parity including OWNERS role-mapping invariants. The Go harness-dispatch path (`internal/harnessdispatch/core.go`) has equivalent OWNERS resolution via `internal/owners`; changes to the OWNERS schema or role mapping must be kept in sync across both implementations. **Security — consuming threaded inputs:** When a newly-threaded entry carries user- or event-controlled data, consume it via `env:` in the final `run:` step — never interpolate `${{ ... }}` directly into a shell block (see the Security note atop `reusable-dispatch.yml`). This prevents the GHA script-injection class of bugs the project defends against elsewhere. diff --git a/docs/guides/infrastructure/layered-config-reference.md b/docs/guides/infrastructure/layered-config-reference.md index 1c52159fe1..e8927a7ff1 100644 --- a/docs/guides/infrastructure/layered-config-reference.md +++ b/docs/guides/infrastructure/layered-config-reference.md @@ -90,12 +90,12 @@ the overlay → base → code defaults chain. | `models.aliases` | `map[string]string` (nested) | Per-key merge | `nil` (fleet defaults) | | `create_issues` | `*CreateIssuesConfig` | Replace whole object if set | `nil` | | `status_notifications` | `*StatusNotificationConfig` | Replace whole object if set | `nil` | -| `authorization`¹ | `object` | Overlay only (not layered) | `nil` | +| `authorization`¹ | `[]AuthorizationProvider` | Overlay only (not layered) | `nil` | -> ¹ `authorization` is part of the Go config package -> (`AuthorizationConfig` / `AuthorizationOwnersFile()`) and is consumed +> ¹ `authorization` is a list of authorization providers +> (`AuthorizationProvider` / `AuthorizationOwnersFile()`) consumed > by both the dispatch workflow's bash/yq and `internal/harnessdispatch`. -> However, it is intentionally **overlay-only**: `AuthorizationOwnersFile()` +> It is intentionally **overlay-only**: `AuthorizationOwnersFile()` > does not fall through to the parent config, so setting it in > `config.base.yaml` has no effect — each repo must opt in explicitly. > See [#6072](https://github.com/fullsend-ai/fullsend/issues/6072) for @@ -376,19 +376,19 @@ The `status_notifications` field uses the same replace-if-set semantics as - Non-nil — replaces the parent value entirely, including nested `comment.start`/`comment.completion` settings. -### `authorization` — replace whole object if set +### `authorization` — provider list -The `authorization` field controls alternative authorization backends -for the dispatch workflow's `has_repo_permission` check. Currently -supports one sub-field: +The `authorization` field is a list of authorization providers that +supplement the default collaborator-API permission check. Native GitHub +collaborator-API auth always runs implicitly; the list names additional +backends. Currently one provider is supported: -- `owners_file` (`bool`, default `false`) — when `true`, the dispatch - routing logic checks the repo-root `OWNERS` file (and `OWNERS_ALIASES` - if present) before falling back to the GitHub collaborator API. OWNERS - approvers get write-equivalent access; reviewers get - triage-equivalent. If the user is not listed in OWNERS, authorization - falls through to the collaborator API — OWNERS never blocks a - collaborator who isn't in the file. +- `owners_file` — the dispatch routing logic checks the repo-root + `OWNERS` file (and `OWNERS_ALIASES` if present) before falling back + to the GitHub collaborator API. OWNERS approvers get write-equivalent + access; reviewers get triage-equivalent. If the user is not listed in + OWNERS, authorization falls through to the collaborator API — OWNERS + never blocks a collaborator who isn't in the file. This applies to both the bash routing path (built-in stages) and the Go harness-dispatch path (custom agents). A missing or malformed OWNERS @@ -403,7 +403,7 @@ Example: ```yaml authorization: - owners_file: true + - provider: owners_file ``` See [ADR 0054](../../ADRs/0054-require-authorization-on-all-agent-dispatch-paths.md) diff --git a/e2e/behaviour/features/dispatch/owners-auth.feature b/e2e/behaviour/features/dispatch/owners-auth.feature index ece2d2f821..cb53ca26b9 100644 --- a/e2e/behaviour/features/dispatch/owners-auth.feature +++ b/e2e/behaviour/features/dispatch/owners-auth.feature @@ -1,16 +1,16 @@ Feature: OWNERS file authorization for bash routing Verify that the OWNERS-file authorization path fires when - authorization.owners_file is enabled in config.yaml. Scenarios + owners_file is in the authorization providers list in config.yaml. Scenarios trigger via issues.opened, which unconditionally calls is_event_actor_authorized and exercises has_repo_permission. - Bot scenarios confirm the OWNERS code path is reached (via audit - log); the bot has collaborator access so the API fallback would - also grant. The outsider identity (TEST_ACTOR_OUTSIDER_PAT) has - no collaborator access, so authorization succeeds only through - OWNERS — the denial scenario verifies a reviewer cannot escalate - to write-level access. + The outsider identity (TEST_ACTOR_OUTSIDER_PAT) has no collaborator + access, so authorization succeeds only through OWNERS — the denial + scenario verifies a reviewer cannot escalate to write-level access. + The write actor (TEST_ACTOR_WRITE_PAT) has write-level collaborator + access and is NOT in OWNERS, so fallthrough scenarios verify that + OWNERS misses correctly fall back to the collaborator API. Background: Given the enrolled test repository @@ -65,17 +65,17 @@ Feature: OWNERS file authorization for bash routing And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | - When the bot opens an issue for OWNERS auth testing + When the write actor opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" Scenario: Triage dispatches without OWNERS path when not opted in - Given an OWNERS file listing the bot as an approver + Given an OWNERS file listing the outsider as an approver And a dummy agent that would: | description | op | args | | Prove execution | write_fixture | output/agent-result.json, fixtures/triage/sufficient.json | - When the bot opens an issue for OWNERS auth testing + When the write actor opens an issue for OWNERS auth testing Then the triage workflow completes successfully And the agent will succeed to Prove execution And the triage workflow logs do not contain "##[notice]OWNERS file resolved user" diff --git a/e2e/behaviour/suite_test.go b/e2e/behaviour/suite_test.go index c0bb5699d4..8e3deb0bef 100644 --- a/e2e/behaviour/suite_test.go +++ b/e2e/behaviour/suite_test.go @@ -133,6 +133,16 @@ func TestBehaviourSuite(t *testing.T) { template.OutsiderLogin = login } + if writePAT := os.Getenv("TEST_ACTOR_WRITE_PAT"); writePAT != "" { + writeClient := e2etest.NewLiveClient(writePAT) + template.WriteSCM = scmgh.New(writeClient) + login, err := writeClient.GetAuthenticatedUser(ctx) + if err != nil { + t.Fatalf("resolving write actor login: %v", err) + } + template.WriteLogin = login + } + suiteRunner := godog.TestSuite{ Name: "behaviour", ScenarioInitializer: func(sc *godog.ScenarioContext) { suite.InitScenario(sc, template) }, diff --git a/internal/config/config.go b/internal/config/config.go index 97409c705e..b87c8ea4d4 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -275,10 +275,10 @@ type CreateIssuesConfig struct { AllowTargets AllowTargets `yaml:"allow_targets"` } -// AuthorizationConfig controls opt-in authorization mechanisms that -// extend the default collaborator-API permission check. -type AuthorizationConfig struct { - OwnersFile bool `yaml:"owners_file,omitempty"` +// AuthorizationProvider identifies an opt-in authorization backend +// that supplements the default collaborator-API permission check. +type AuthorizationProvider struct { + Provider string `yaml:"provider"` } // orgConfig is the top-level configuration for a fullsend organization. @@ -381,6 +381,11 @@ func ValidEffortLevels() []string { return slices.Clone(validEffortLevels) } // ValidEffort reports whether level is an accepted effort value. func ValidEffort(level string) bool { return slices.Contains(validEffortLevels, level) } +// ValidAuthorizationProviders returns the set of recognized authorization provider names. +func ValidAuthorizationProviders() []string { + return []string{"owners_file"} +} + // DefaultAgentRoles returns the standard set of agent roles installed // when no custom roles are specified. The fix stage reuses the coder // app (role: coder) so it does not need a separate app or PEM. @@ -786,9 +791,9 @@ type perRepoConfig struct { // resource prefixes. MarshalYAML preserves the nil-vs-empty // distinction: nil (unset) is omitted, empty (deny-all) is // marshaled as `allowed_remote_resources: []`. - AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` - CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` - Authorization *AuthorizationConfig `yaml:"authorization,omitempty"` + AllowedRemoteResources []string `yaml:"allowed_remote_resources,omitempty"` + CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` + Authorization []AuthorizationProvider `yaml:"authorization,omitempty"` // Notifications backs the StatusNotifications() accessor. Named // distinctly from the method (unlike CreateIssues/IssueCreationConfig) // because "StatusNotifications" is the established accessor name @@ -989,7 +994,7 @@ type perRepoConfigMarshal struct { Agents []AgentEntry `yaml:"agents,omitempty"` AllowedRemoteResources *[]string `yaml:"allowed_remote_resources,omitempty"` CreateIssues *CreateIssuesConfig `yaml:"create_issues,omitempty"` - Authorization *AuthorizationConfig `yaml:"authorization,omitempty"` + Authorization []AuthorizationProvider `yaml:"authorization,omitempty"` StatusNotifications *StatusNotificationConfig `yaml:"status_notifications,omitempty"` MintURL string `yaml:"mint_url,omitempty"` Inference *PerRepoInferenceConfig `yaml:"inference,omitempty"` @@ -1080,6 +1085,17 @@ func (c *perRepoConfig) Validate() error { return fmt.Errorf("invalid inference provider %q: must be one of %s", c.Inference.Provider, strings.Join(validProviders, ", ")) } } + validAuthProviders := ValidAuthorizationProviders() + seenProviders := make(map[string]bool, len(c.Authorization)) + for i, p := range c.Authorization { + if !slices.Contains(validAuthProviders, p.Provider) { + return fmt.Errorf("authorization[%d]: invalid provider %q: must be one of %s", i, p.Provider, strings.Join(validAuthProviders, ", ")) + } + if seenProviders[p.Provider] { + return fmt.Errorf("authorization[%d]: duplicate provider %q", i, p.Provider) + } + seenProviders[p.Provider] = true + } // Validate the merged view, as ValidateAgentEntries does above: a bad // key in config.base.yaml must not slip through because the overlay // omits models:. diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 26df5e6440..a29a16676a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -706,6 +706,34 @@ func TestPerRepoConfigValidate_Runtime(t *testing.T) { assert.Contains(t, err.Error(), "invalid runtime") } +func TestPerRepoConfigValidate_AuthorizationValidProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + assert.NoError(t, cfg.Validate()) +} + +func TestPerRepoConfigValidate_AuthorizationInvalidProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "ldap"}}, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "invalid provider") +} + +func TestPerRepoConfigValidate_AuthorizationDuplicateProvider(t *testing.T) { + cfg := &perRepoConfig{ + Version: "1", + Authorization: []AuthorizationProvider{{Provider: "owners_file"}, {Provider: "owners_file"}}, + } + err := cfg.Validate() + assert.Error(t, err) + assert.Contains(t, err.Error(), "duplicate provider") +} + func TestParsePerRepoConfig(t *testing.T) { yamlData := ` version: "1" diff --git a/internal/config/interfaces.go b/internal/config/interfaces.go index 4ddbe54823..41b2e0f2c3 100644 --- a/internal/config/interfaces.go +++ b/internal/config/interfaces.go @@ -413,7 +413,12 @@ func (c *perRepoConfig) IsOrgMode() bool { return false } // Intentionally no parent fallback: OWNERS auth is a per-repo opt-in that must // not be inheritable from config.base.yaml. func (c *perRepoConfig) AuthorizationOwnersFile() bool { - return c.Authorization != nil && c.Authorization.OwnersFile + for _, p := range c.Authorization { + if p.Provider == "owners_file" { + return true + } + } + return false } // ConfigRoles returns the configured agent roles. nil (key omitted) @@ -581,18 +586,25 @@ func (c *perRepoConfig) ConfigModelAliases() map[string]string { func (c *perRepoConfig) SetKillSwitch(v bool) { c.KillSwitch = &v } // SetAuthorizationOwnersFile enables or disables OWNERS-file authorization. -// Clears only OwnersFile; the struct is niled only when all fields are zero -// so future sibling fields are not silently wiped. func (c *perRepoConfig) SetAuthorizationOwnersFile(v bool) { if v { - if c.Authorization == nil { - c.Authorization = &AuthorizationConfig{} + for _, p := range c.Authorization { + if p.Provider == "owners_file" { + return + } + } + c.Authorization = append(c.Authorization, AuthorizationProvider{Provider: "owners_file"}) + } else { + filtered := make([]AuthorizationProvider, 0, len(c.Authorization)) + for _, p := range c.Authorization { + if p.Provider != "owners_file" { + filtered = append(filtered, p) + } } - c.Authorization.OwnersFile = true - } else if c.Authorization != nil { - c.Authorization.OwnersFile = false - if *c.Authorization == (AuthorizationConfig{}) { + if len(filtered) == 0 { c.Authorization = nil + } else { + c.Authorization = filtered } } } diff --git a/internal/config/interfaces_test.go b/internal/config/interfaces_test.go index 94c57480ee..9690954ccc 100644 --- a/internal/config/interfaces_test.go +++ b/internal/config/interfaces_test.go @@ -902,7 +902,7 @@ func TestPerRepoConfig_AuthorizationOwnersFile_NoFallback(t *testing.T) { t.Run("does not fall through to parent", func(t *testing.T) { parent := &perRepoConfig{ - Authorization: &AuthorizationConfig{OwnersFile: true}, + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, } child := &perRepoConfig{parent: parent} assert.False(t, child.AuthorizationOwnersFile()) @@ -910,7 +910,7 @@ func TestPerRepoConfig_AuthorizationOwnersFile_NoFallback(t *testing.T) { t.Run("returns true when set locally", func(t *testing.T) { cfg := &perRepoConfig{ - Authorization: &AuthorizationConfig{OwnersFile: true}, + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, } assert.True(t, cfg.AuthorizationOwnersFile()) }) diff --git a/internal/forge/forge.go b/internal/forge/forge.go index 687c422131..16c39c1e19 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -235,7 +235,6 @@ type WorkflowRun struct { Conclusion string // "success", "failure", "cancelled", etc. HTMLURL string CreatedAt string - ActorLogin string // GitHub login of the user who triggered the run } // WorkflowJob represents a job within a workflow run. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 90d6b88bf4..7672a8554b 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -3297,9 +3297,6 @@ func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflow Conclusion string `json:"conclusion"` HTMLURL string `json:"html_url"` CreatedAt string `json:"created_at"` - Actor struct { - Login string `json:"login"` - } `json:"actor"` } `json:"workflow_runs"` } if err := decodeJSON(resp, &result); err != nil { @@ -3315,7 +3312,6 @@ func (c *LiveClient) ListWorkflowRuns(ctx context.Context, owner, repo, workflow Conclusion: r.Conclusion, HTMLURL: r.HTMLURL, CreatedAt: r.CreatedAt, - ActorLogin: r.Actor.Login, } } return runs, nil diff --git a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml index f08ef9ca6d..591f00dbb1 100644 --- a/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml +++ b/internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml @@ -99,12 +99,12 @@ jobs: local username="${1:-}" min="${2:-write}" role api_err [[ -z "${username}" ]] && return 1 - # OWNERS-file authorization (opt-in via authorization.owners_file in config.yaml). + # OWNERS-file authorization (opt-in via authorization providers list in config.yaml). # Approvers get write-equivalent access; reviewers get triage-equivalent. # Safe: sparse-checkout pins to base branch SHA for PR-scoped events, # so PR authors cannot self-authorize by adding themselves to OWNERS. if [[ -f "OWNERS" && -f ".fullsend/config.yaml" ]]; then - if [[ "$(yq '.authorization.owners_file // false' .fullsend/config.yaml)" == "true" ]]; then + if [[ "$(yq '(.authorization // []) | any_c(.provider == "owners_file")' .fullsend/config.yaml)" == "true" ]]; then if [[ ! "${username}" =~ ^[a-zA-Z0-9-]+$ ]]; then echo "::warning::OWNERS auth skipped: username '${username}' contains unexpected characters" >&2 else diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index d83af5c420..a66f68e24a 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -33,7 +33,7 @@ func registerOwnersSteps(sc *godog.ScenarioContext) { sc.Step(`^the triage workflow logs do not contain "([^"]+)"$`, func(ctx context.Context, needle string) (context.Context, error) { return ctx, thenWorkflowLogsDoNotContain(world.FromContext(ctx), needle) }) - sc.Step(`^the (bot|outsider) opens an issue for OWNERS auth testing$`, func(ctx context.Context, actor string) (context.Context, error) { + sc.Step(`^the (bot|outsider|write actor) opens an issue for OWNERS auth testing$`, func(ctx context.Context, actor string) (context.Context, error) { return ctx, whenIssueOpenedForOwnersAuth(world.FromContext(ctx), actor) }) sc.Step(`^the outsider posts "([^"]+)" on the issue$`, func(ctx context.Context, command string) (context.Context, error) { @@ -57,6 +57,11 @@ func resolveActorLogin(w *world.World, actor string) (string, error) { return "", err } return w.OutsiderLogin, nil + case "write actor": + if err := requireWriteActor(w); err != nil { + return "", err + } + return w.WriteLogin, nil default: return "", fmt.Errorf("unknown actor %q", actor) } @@ -145,11 +150,17 @@ func whenIssueOpenedForOwnersAuth(w *world.World, actor string) error { return fmt.Errorf("no repo configured; call 'Given the enrolled test repository' before creating issues") } scmDriver := w.SCM - if actor == "outsider" { + switch actor { + case "outsider": if err := requireOutsider(w); err != nil { return err } scmDriver = w.OutsiderSCM + case "write actor": + if err := requireWriteActor(w); err != nil { + return err + } + scmDriver = w.WriteSCM } w.ScenarioStart = time.Now().Add(-issueOpenDrainSkewBuffer) w.TriageTriggerEvent = issueOpenEvent @@ -197,6 +208,13 @@ func getWorkflowLogs(w *world.World) (string, error) { w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) } +func requireWriteActor(w *world.World) error { + if w.WriteSCM == nil { + return fmt.Errorf("TEST_ACTOR_WRITE_PAT not set") + } + return nil +} + func requireOutsider(w *world.World) error { if w.OutsiderSCM == nil { return fmt.Errorf("TEST_ACTOR_OUTSIDER_PAT not set") diff --git a/pkg/behaviourtest/steps/owners_test.go b/pkg/behaviourtest/steps/owners_test.go index 803c82d504..1b795f66a6 100644 --- a/pkg/behaviourtest/steps/owners_test.go +++ b/pkg/behaviourtest/steps/owners_test.go @@ -22,7 +22,7 @@ func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { require.NoError(t, err) s := string(out) assert.Contains(t, s, "authorization:") - assert.Contains(t, s, "owners_file: true") + assert.Contains(t, s, "provider: owners_file") assert.Contains(t, s, "runtime: claude") }) @@ -35,7 +35,7 @@ func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { cfg.SetAuthorizationOwnersFile(true) out, err := cfg.Marshal() require.NoError(t, err) - assert.Contains(t, string(out), "owners_file: true") + assert.Contains(t, string(out), "provider: owners_file") }) t.Run("disable removes authorization block", func(t *testing.T) { @@ -52,12 +52,13 @@ func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { t.Run("parse existing authorization from YAML", func(t *testing.T) { t.Parallel() - input := []byte("version: \"1\"\nauthorization:\n owners_file: true\n") + input := []byte("version: \"1\"\nauthorization:\n - provider: owners_file\n") cfg, err := config.ParsePerRepoConfigWriter(input) require.NoError(t, err) + assert.True(t, cfg.AuthorizationOwnersFile()) out, err := cfg.Marshal() require.NoError(t, err) - assert.Contains(t, string(out), "owners_file: true") + assert.Contains(t, string(out), "provider: owners_file") }) t.Run("disable when never enabled is no-op", func(t *testing.T) { @@ -85,6 +86,6 @@ func TestAuthorizationOwnersFileRoundTrip(t *testing.T) { assert.Contains(t, s, "runtime: claude") assert.Contains(t, s, "kill_switch: false") assert.Contains(t, s, "- coder") - assert.Contains(t, s, "owners_file: true") + assert.Contains(t, s, "provider: owners_file") }) } diff --git a/pkg/behaviourtest/world/world.go b/pkg/behaviourtest/world/world.go index ecd55ef590..d3f2ed78f2 100644 --- a/pkg/behaviourtest/world/world.go +++ b/pkg/behaviourtest/world/world.go @@ -92,6 +92,13 @@ type World struct { OutsiderSCM scm.Driver OutsiderLogin string + // WriteSCM is an SCM driver authenticated as a GitHub User with + // write-level collaborator access. Used by OWNERS fallthrough + // scenarios that need an actor passing the collaborator API but + // not listed in OWNERS. Nil when TEST_ACTOR_WRITE_PAT is not set. + WriteSCM scm.Driver + WriteLogin string + // KillSwitchActivated records whether this scenario activated the // repo-level kill switch. CleanupScenario uses this to deactivate // the switch so the next scenario on this slot is not affected. @@ -121,7 +128,7 @@ type World struct { AgentsOriginal []config.AgentEntry // OwnersAuthActivated records whether this scenario committed an - // OWNERS file and/or enabled authorization.owners_file in config.yaml. + // OWNERS file and/or added owners_file to the authorization providers in config.yaml. // CleanupScenario removes both. OwnersAuthActivated bool From 3359018f9f6b7e3544d33299e27480260dced2ac Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 20 Aug 2026 17:06:17 +0200 Subject: [PATCH 08/12] fix(#6042): document OWNERS v1 scope limitation in ADR 0054 Signed-off-by: RaphaelBut --- .../0054-require-authorization-on-all-agent-dispatch-paths.md | 4 ++++ 1 file changed, 4 insertions(+) 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 3bd054fe23..7bff418144 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 @@ -212,6 +212,10 @@ permission list, not by bypassing the check. > harness dispatch requires write-level access — OWNERS approver or > GitHub write+ collaborator — because `IsAuthorized` gates all > harness triggers at the write level. +> +> v1 limitation: only repo-root flat `approvers`/`reviewers` lists are +> read. Prow `filters:` blocks and nested per-directory OWNERS files +> are not supported. ## Consequences From 09d9f373947ee16305d15f7999ac2d9e4c821277 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 20 Aug 2026 17:10:34 +0200 Subject: [PATCH 09/12] fix(#6042): document ConfigDir repo-root assumption in harness dispatch Options Signed-off-by: RaphaelBut --- internal/harnessdispatch/core.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/harnessdispatch/core.go b/internal/harnessdispatch/core.go index 66964b1196..da0aea309c 100644 --- a/internal/harnessdispatch/core.go +++ b/internal/harnessdispatch/core.go @@ -14,6 +14,9 @@ import ( // Options configures a dispatch run. type Options struct { + // ConfigDir is the fullsend config directory (e.g. ".fullsend"). + // Must be a direct child of the repo root; filepath.Dir is used + // to locate OWNERS and OWNERS_ALIASES for authorization. ConfigDir string Event *normevent.Event From 6d0414a27cf561b3785da3fab5a493f088501d0b Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 20 Aug 2026 18:05:13 +0200 Subject: [PATCH 10/12] fix(#6042): add slot-health check before every behaviour scenario 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 --- pkg/behaviourtest/steps/cleanup.go | 34 ++++++++++++++++++++++++++++++ pkg/behaviourtest/steps/owners.go | 23 ++++++++++---------- pkg/behaviourtest/suite/init.go | 4 ++++ 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/pkg/behaviourtest/steps/cleanup.go b/pkg/behaviourtest/steps/cleanup.go index f23d6f9ca8..003782a394 100644 --- a/pkg/behaviourtest/steps/cleanup.go +++ b/pkg/behaviourtest/steps/cleanup.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) @@ -47,6 +48,39 @@ func cleanupRetry(logf func(string, ...any), desc string, fn func() error) error return lastErr } +// ValidateSlotClean reads the repo slot's config and fails if a previous +// scenario left mutable state behind (e.g. kill switch on, OWNERS auth +// enabled). Called before every scenario so stale state is caught +// immediately rather than causing silent false positives downstream. +func ValidateSlotClean(w *world.World) error { + // Unit tests construct bare worlds with no SCM driver; + // there is no remote repo to validate in that case. + if w.SCM == nil || w.Org == "" || w.RepoName == "" { + return nil + } + cfgPath := filepath.Join(".fullsend", "config.yaml") + cfgData, err := w.SCM.GetFileContent(context.Background(), + w.Org, w.RepoName, cfgPath) + if err != nil { + return nil + } + cfg, err := config.ParsePerRepoConfigWriter(cfgData) + if err != nil { + return nil + } + var stale []string + if cfg.IsKillSwitchActive() { + stale = append(stale, "kill_switch is active") + } + if cfg.AuthorizationOwnersFile() { + stale = append(stale, "owners_file authorization is enabled") + } + if len(stale) > 0 { + return fmt.Errorf("repo slot has stale state from a previous scenario (cleanup likely failed): %s", strings.Join(stale, ", ")) + } + return nil +} + func CleanupScenario(w *world.World) { ctx := context.Background() diff --git a/pkg/behaviourtest/steps/owners.go b/pkg/behaviourtest/steps/owners.go index a66f68e24a..d0dd8bf978 100644 --- a/pkg/behaviourtest/steps/owners.go +++ b/pkg/behaviourtest/steps/owners.go @@ -10,6 +10,7 @@ import ( "github.com/cucumber/godog" "github.com/fullsend-ai/fullsend/internal/config" "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/install" scmgh "github.com/fullsend-ai/fullsend/pkg/behaviourtest/drivers/scm/github" "github.com/fullsend-ai/fullsend/pkg/behaviourtest/world" ) @@ -69,7 +70,7 @@ func resolveActorLogin(w *world.World, actor string) (string, error) { func commitFile(w *world.World, path, message, content string) error { if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), + w.Org, w.RepoName, path, message, []byte(content)); err != nil { return fmt.Errorf("committing %s: %w", path, err) } @@ -122,7 +123,7 @@ func givenOwnersAliasesFile(w *world.World, alias, actor string) error { func givenOwnersAuthEnabled(w *world.World) error { cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath) + w.Org, w.RepoName, cfgPath) if err != nil { return fmt.Errorf("reading config: %w", err) } @@ -136,7 +137,7 @@ func givenOwnersAuthEnabled(w *world.World) error { return err } if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), + w.Org, w.RepoName, cfgPath, "behaviour: enable OWNERS authorization", merged); err != nil { return fmt.Errorf("updating config: %w", err) @@ -205,7 +206,7 @@ func getWorkflowLogs(w *world.World) (string, error) { return "", fmt.Errorf("no workflow run recorded") } return w.CI.GetRunLogs(context.Background(), - w.RepoOwner, w.Install.TriageWorkflowRepo(), w.WorkflowRun.ID) + w.RepoOwner, w.RepoName, w.WorkflowRun.ID) } func requireWriteActor(w *world.World) error { @@ -241,7 +242,7 @@ func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { return err } logs, err := w.CI.GetRunLogs(context.Background(), - w.RepoOwner, w.Install.TriageWorkflowRepo(), run.ID) + w.RepoOwner, w.RepoName, run.ID) if err != nil { return fmt.Errorf("fetching dispatch run logs: %w", err) } @@ -253,8 +254,8 @@ func thenDispatchRunDoesNotAuthorizeViaOwners(w *world.World) error { func waitForDispatchRun(w *world.World) (*forge.WorkflowRun, error) { ctx := context.Background() - repo := w.Install.TriageWorkflowRepo() - file := w.Install.TriageWorkflowFile() + repo := w.RepoName + file := install.PerRepoTriageWorkflow run, err := w.CI.WaitForWorkflow(ctx, w.RepoOwner, repo, file, w.ScenarioStart, issueCommentEvent) if err == nil { @@ -272,7 +273,7 @@ func waitForDispatchRun(w *world.World) (*forge.WorkflowRun, error) { func disableOwnersAuth(w *world.World) error { cfgPath := filepath.Join(".fullsend", "config.yaml") cfgData, err := w.SCM.GetFileContent(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), cfgPath) + w.Org, w.RepoName, cfgPath) if err != nil { return fmt.Errorf("reading config: %w", err) } @@ -286,7 +287,7 @@ func disableOwnersAuth(w *world.World) error { return err } if err := w.SCM.CommitFile(context.Background(), - w.Install.ConfigOwner(), w.Install.ConfigRepo(), + w.Org, w.RepoName, cfgPath, "behaviour: disable OWNERS authorization", merged); err != nil { return fmt.Errorf("updating config: %w", err) @@ -296,8 +297,8 @@ func disableOwnersAuth(w *world.World) error { func cleanupOwnersAuth(w *world.World) { ctx := context.Background() - owner := w.Install.ConfigOwner() - repo := w.Install.ConfigRepo() + owner := w.Org + repo := w.RepoName if err := disableOwnersAuth(w); err != nil { worldLogf(w, "behaviour cleanup: disable OWNERS auth: %v", err) diff --git a/pkg/behaviourtest/suite/init.go b/pkg/behaviourtest/suite/init.go index 17186d3a37..a7934b0d4f 100644 --- a/pkg/behaviourtest/suite/init.go +++ b/pkg/behaviourtest/suite/init.go @@ -39,6 +39,10 @@ func beforeScenario(ctx context.Context, tags []string, template *world.World) ( w := template.Clone() resetScenarioWorld(w) + if err := steps.ValidateSlotClean(w); err != nil { + return ctx, err + } + ctx = world.WithWorld(ctx, w) return ctx, nil } From b37b3683817e2c8155bae6bd22de05dab5b1b0b8 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Thu, 20 Aug 2026 19:00:51 +0200 Subject: [PATCH 11/12] fix(#6042): clarify checkout ref fallback in ADR 0054 Signed-off-by: RaphaelBut --- ...0054-require-authorization-on-all-agent-dispatch-paths.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) 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 7bff418144..9b89697648 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 @@ -198,8 +198,9 @@ permission list, not by bypassing the check. > (and `OWNERS_ALIASES`) before falling back to the collaborator API. > OWNERS approvers get write-equivalent access; reviewers get > triage-equivalent. The sparse-checkout pins to the base branch SHA for -> PR-scoped events (`pull_request_target`, `pull_request_review`), so PR -> authors cannot self-authorize by modifying OWNERS in their PR. +> PR-scoped events (`pull_request_target`, `pull_request_review`) and the +> default-branch head otherwise, so PR authors cannot self-authorize by +> modifying OWNERS in their PR. > This follows the extension path above (extending the allowed permission > sources in `has_repo_permission`) rather than bypassing the check. > OWNERS auth applies to both built-in stages (bash routing) and the From d1671952a26aacbd49fa0d9dd5ac246660cec889 Mon Sep 17 00:00:00 2001 From: RaphaelBut Date: Fri, 4 Sep 2026 15:11:09 +0200 Subject: [PATCH 12/12] fix(#6042): add unit tests to meet codecov/patch 80% threshold Cover authorization config accessors (orgConfig, perRepoConfig, perRepoDefaults) and Role.String() that were missing test coverage. Co-Authored-By: Claude Opus 4.6 (1M context) Signed-off-by: RaphaelBut --- internal/config/interfaces_test.go | 55 ++++++++++++++++++++++++++++++ internal/owners/owners_test.go | 7 ++++ 2 files changed, 62 insertions(+) diff --git a/internal/config/interfaces_test.go b/internal/config/interfaces_test.go index 9690954ccc..c025e5e269 100644 --- a/internal/config/interfaces_test.go +++ b/internal/config/interfaces_test.go @@ -915,3 +915,58 @@ func TestPerRepoConfig_AuthorizationOwnersFile_NoFallback(t *testing.T) { assert.True(t, cfg.AuthorizationOwnersFile()) }) } + +func TestOrgConfig_AuthorizationOwnersFile(t *testing.T) { + cfg := &orgConfig{} + assert.False(t, cfg.AuthorizationOwnersFile()) + cfg.SetAuthorizationOwnersFile(true) + assert.False(t, cfg.AuthorizationOwnersFile()) +} + +func TestPerRepoConfig_SetAuthorizationOwnersFile(t *testing.T) { + t.Run("enable adds provider", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(true) + assert.True(t, cfg.AuthorizationOwnersFile()) + assert.Len(t, cfg.Authorization, 1) + }) + + t.Run("enable is idempotent", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(true) + cfg.SetAuthorizationOwnersFile(true) + assert.Len(t, cfg.Authorization, 1) + }) + + t.Run("disable removes provider", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: []AuthorizationProvider{{Provider: "owners_file"}}, + } + cfg.SetAuthorizationOwnersFile(false) + assert.False(t, cfg.AuthorizationOwnersFile()) + assert.Nil(t, cfg.Authorization) + }) + + t.Run("disable preserves other providers", func(t *testing.T) { + cfg := &perRepoConfig{ + Authorization: []AuthorizationProvider{ + {Provider: "owners_file"}, + {Provider: "other"}, + }, + } + cfg.SetAuthorizationOwnersFile(false) + assert.False(t, cfg.AuthorizationOwnersFile()) + assert.Equal(t, []AuthorizationProvider{{Provider: "other"}}, cfg.Authorization) + }) + + t.Run("disable is no-op when not set", func(t *testing.T) { + cfg := &perRepoConfig{} + cfg.SetAuthorizationOwnersFile(false) + assert.Nil(t, cfg.Authorization) + }) +} + +func TestPerRepoDefaults_AuthorizationOwnersFile(t *testing.T) { + d := &perRepoDefaults{} + assert.False(t, d.AuthorizationOwnersFile()) +} diff --git a/internal/owners/owners_test.go b/internal/owners/owners_test.go index 7125e7e37c..8f81320cd4 100644 --- a/internal/owners/owners_test.go +++ b/internal/owners/owners_test.go @@ -139,6 +139,13 @@ func TestResolve(t *testing.T) { }) } +func TestRoleString(t *testing.T) { + t.Parallel() + assert.Equal(t, "approver", Approver.String()) + assert.Equal(t, "reviewer", Reviewer.String()) + assert.Equal(t, "none", None.String()) +} + func TestMapToActorRole(t *testing.T) { t.Parallel()