Handle 404 from UpdateBranchProtection in branch policy Fix - #864
Open
pujitha24 wants to merge 1 commit into
Open
Handle 404 from UpdateBranchProtection in branch policy Fix#864pujitha24 wants to merge 1 commit into
pujitha24 wants to merge 1 commit into
Conversation
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>
Contributor
There was a problem hiding this comment.
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 Foundresponses fromUpdateBranchProtectionsimilarly to existing403 Forbiddenhandling: log a warning and returnnilinstead of propagating the error. - Add regression subtests covering the “create from scratch” and “update existing protection” paths when
UpdateBranchProtectionreturns404.
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
continuehere 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 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 stashon 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