Skip to content

Handle 404 from UpdateBranchProtection in branch policy Fix - #864

Open
pujitha24 wants to merge 1 commit into
ossf:mainfrom
pujitha24:auto/issue-562
Open

Handle 404 from UpdateBranchProtection in branch policy Fix#864
pujitha24 wants to merge 1 commit into
ossf:mainfrom
pujitha24:auto/issue-562

Conversation

@pujitha24

Copy link
Copy Markdown

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

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 ossf#562

Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24
pujitha24 requested a review from a team as a code owner July 26, 2026 20:12
Copilot AI review requested due to automatic review settings July 26, 2026 20:12
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Jul 26, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves Allstar’s Branch Protection policy enforcement robustness by preventing a GitHub API 404 from aborting enforcement across other repositories in the same GitHub App installation (notably when branch protection is disabled due to rulesets migration, per #562).

Changes:

  • Treat 404 Not Found responses from UpdateBranchProtection similarly to existing 403 Forbidden handling: log a warning and return nil instead of propagating the error.
  • Add regression subtests covering the “create from scratch” and “update existing protection” paths when UpdateBranchProtection returns 404.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
pkg/policies/branch/branch.go Adds 404 handling around UpdateBranchProtection to avoid breaking enforcement loops across repos.
pkg/policies/branch/branch_test.go Adds regression coverage for UpdateBranchProtection returning 404 on both create/update protection paths.
Comments suppressed due to low confidence (1)

pkg/policies/branch/branch.go:655

  • This 404 handling returns from fix(), which stops processing any remaining branches configured for enforcement in the same repository. Using continue here avoids skipping other branches, and the warning message should avoid asserting a specific 404 cause unless it is explicitly detected.
						Str("repo", repo).
						Str("area", polName).
						Msg("Fix action selected, but branch protection has been disabled on this repository.")
					return nil
				}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +497 to +505
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
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S This PR changes 10-29 lines, ignoring generated files.

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Issue with Allstar Branch Protection Enforcement (404 errors upon action: fix)

2 participants