fix: abort repository deletion if upstream webhook deregistration fails#6370
fix: abort repository deletion if upstream webhook deregistration fails#6370AftAb-25 wants to merge 1 commit intomindersec:mainfrom
Conversation
evankanderson
left a comment
There was a problem hiding this comment.
We need to be able to delete the repository if the user uses the GitHub interface to remove the repository from the application installation. In this case, we have to leak the webhook, as we don't have permissions to update the repository (including to remove the webhook) anymore. The best we can do is to clean up the Minder state around the repository -- the dangling webhook has become unreachable from Minder's cleanup perspective.
It's possible that all this could be avoided by registering an app-level webhook, but that would be a larger refactor of both webhook and provider logic.
evankanderson
left a comment
There was a problem hiding this comment.
The most likely cause of being unable to delete a webhook is that the app's permissions on the repository have been removed. In that case, keeping a repository object in Minder will simply produce more failed attempts to act on a resource that we don't have permissions on.
We could potentially introduce some sort of "tombstone" process to mark these entities in the database as needing future cleanup, but preventing any policy application (e.g. filtering them from all the current RPC outputs, etc), but I'm not convinced that the payoff is worth the costs.
I'm marking this PR as "request changes" (needs to address the above issue) to help track which outstanding PRs need maintainer action vs contributor action.
Fixes #6369
Description
This fixes a critical resource leak where deleting a repository from Minder could leave a permanently orphaned "zombie" webhook in the user's GitHub repository.
Previously, when a user deleted a repo, deleteRepository in internal/repositories/service.go would attempt to deregister the webhook upstream. If that GitHub API call failed (e.g., due to a temporary network issue, rate limit, or revoked token), the code simply logged the error and blithely continued to delete the repository from the Minder database.
Once deleted from Minder, the user had absolutely no way to retry the deletion, leaving the active webhook permanently installed on their GitHub repo. This webhook would continuously spray ingress traffic against Minder, which would just
404orsql.ErrNoRowssince the repo no longer existed in the database.Changes
client.DeregisterEntityfails, allowing users to safely retry the deletion once credentials or upstream connectivity are restored.Checklist