From c958b93bb2f214e3a6f1a6dc7b4e5013b87e6cc1 Mon Sep 17 00:00:00 2001 From: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Date: Sun, 26 Jul 2026 13:12:44 -0700 Subject: [PATCH] Handle 404 from UpdateBranchProtection in branch policy Fix Motivation: Allstar's Branch Protection policy fails to enforce branch protection on repos where branch protection has been disabled (e.g. via GitHub's rulesets migration). GitHub's PUT /repos/{owner}/{repo}/branches/{branch}/protection endpoint returns a 404 ("Branch protection has been disabled on this repository") in this case, which fix() previously propagated as an error. That error bubbles up through runPolicies() to runPoliciesOnInstRepos(), which breaks out of its loop over repos on the first error. Net effect: once one repo in a GitHub App installation hits this 404, every other repo in that installation is skipped for the rest of that enforcement cycle, every cycle, until the affected repo is fixed or removed. This does not crash the process (EnforceAll logs the error and returns nil from the errgroup), so no outage occurs, but enforcement silently stops for unrelated repos in the same installation. Approach: Mirror the existing http.StatusForbidden handling already present at both call sites of rep.UpdateBranchProtection() in fix(): when the response status is 404, log a Warning (matching the existing log style/fields) and return nil instead of propagating the error, so the enforcement loop for other repos/installations is unaffected. This matches the short-term fix suggested by maintainer jeffmendoza on the issue: "update the code to expect the 404 on some repos, and just log a Warning and continue without exiting the enforcement loop." Validation: Added two new TestFix subtests in pkg/policies/branch/branch_test.go that simulate a 404 from UpdateBranchProtection on both the create-from-scratch path and the update-existing-protection path, and assert fix() returns nil without issuing any protection request. Verified both new subtests fail with the pre-fix code (via `git stash` on branch.go) and pass with the fix applied. go build ./... go test ./pkg/policies/branch/... ok github.com/ossf/allstar/pkg/policies/branch 0.251s Fixes #562 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> --- pkg/policies/branch/branch.go | 17 ++++++++ pkg/policies/branch/branch_test.go | 62 ++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+) diff --git a/pkg/policies/branch/branch.go b/pkg/policies/branch/branch.go index 8897124b..8ec44f5f 100644 --- a/pkg/policies/branch/branch.go +++ b/pkg/policies/branch/branch.go @@ -494,6 +494,15 @@ func fix(ctx context.Context, rep repositories, c *github.Client, // no sense to continue, just return return nil } + if rsp != nil && rsp.StatusCode == http.StatusNotFound { + log.Warn(). + Str("org", owner). + Str("repo", repo). + Str("area", polName). + Msg("Fix action selected, but branch protection has been disabled on this repository.") + // no sense to continue, just return + return nil + } return err } continue @@ -636,6 +645,14 @@ func fix(ctx context.Context, rep repositories, c *github.Client, Msg("Action set to fix, but did not accept admin:write permissions update.") return nil } + if rsp != nil && rsp.StatusCode == http.StatusNotFound { + log.Warn(). + Str("org", owner). + Str("repo", repo). + Str("area", polName). + Msg("Fix action selected, but branch protection has been disabled on this repository.") + return nil + } return err } log.Info(). diff --git a/pkg/policies/branch/branch_test.go b/pkg/policies/branch/branch_test.go index a39a755f..9d1504ff 100644 --- a/pkg/policies/branch/branch_test.go +++ b/pkg/policies/branch/branch_test.go @@ -1371,6 +1371,7 @@ func TestFix(t *testing.T) { Prot map[string]github.Protection SignatureProt map[string]github.SignaturesProtectedBranch cofigEnabled bool + FailUpdateWith int Exp map[string]github.ProtectionRequest ExpSignatureRequests map[string]bool }{ @@ -1490,6 +1491,60 @@ func TestFix(t *testing.T) { }, ExpSignatureRequests: map[string]bool{}, }, + { + // Regression test for https://github.com/ossf/allstar/issues/562: + // GitHub returns 404 when creating branch protection on a repo + // that has protection disabled via rulesets migration. Fix + // should warn and return nil, not error out. + Name: "AddProtectionFromScratchDisabled", + Org: OrgConfig{ + EnforceDefault: true, + RequireApproval: true, + ApprovalCount: 2, + DismissStale: true, + BlockForce: true, + EnforceOnAdmins: true, + }, + Repo: RepoConfig{}, + Prot: map[string]github.Protection{}, + cofigEnabled: true, + FailUpdateWith: http.StatusNotFound, + Exp: map[string]github.ProtectionRequest{}, + SignatureProt: map[string]github.SignaturesProtectedBranch{}, + ExpSignatureRequests: map[string]bool{}, + }, + { + // Same regression, but on the path that updates existing + // protection rather than creating it from scratch. + Name: "EnforceAdminsDisabled", + Org: OrgConfig{ + EnforceDefault: true, + EnforceOnAdmins: true, + }, + Repo: RepoConfig{}, + Prot: map[string]github.Protection{ + "main": { + AllowForcePushes: &github.AllowForcePushes{ + Enabled: false, + }, + EnforceAdmins: &github.AdminEnforcement{ + Enabled: false, + }, + RequiredPullRequestReviews: &github.PullRequestReviewsEnforcement{ + RequiredApprovingReviewCount: 0, + }, + }, + }, + cofigEnabled: true, + FailUpdateWith: http.StatusNotFound, + Exp: map[string]github.ProtectionRequest{}, + SignatureProt: map[string]github.SignaturesProtectedBranch{ + "main": { + Enabled: github.Ptr(false), + }, + }, + ExpSignatureRequests: map[string]bool{}, + }, { Name: "NotEnabled", Org: OrgConfig{ @@ -2065,6 +2120,13 @@ func TestFix(t *testing.T) { branch string, preq *github.ProtectionRequest) (*github.Protection, *github.Response, error, ) { + if test.FailUpdateWith != 0 { + return nil, &github.Response{ + Response: &http.Response{ + StatusCode: test.FailUpdateWith, + }, + }, errors.New("update failed") + } got[branch] = *preq return nil, nil, nil }