Handle github repository transfer events. (mindersec#4130)

Github webhook events having type `repository` and action
`transferred` are handled by producing an internal event that triggers
an evaluation. Despite being safe, it cannot lead to any useful action
since the github app installation lost permissions to operate on the
repository.

This change makes it so that `repository.transferred` events are
handled as deletions. In order for such deletion to be successful, 403
Forbidden errors on webhook deletions must be handled gracefully, so
that repository deletion can terminate without errors.

This is safe since (b) there's nothing Minder can do about this,
having lost the necessary permissions, and (b) repository registration
removes stale webhooks. The latter point is relevant to fix repository
transfers when auto registration is enabled: in these cases,
repository ownership is migrated before the event is delivered to
Minder, making it impossible to cleanup webhooks, which leads to an
SQL deletion failure, which eventually makes it impossibile to
re-register the repository under the destination project.

Fixes mindersec#3274

Co-authored-by: Don Browne <[email protected]>

internal/controlplane/handlers_githubwebhooks.go

@@ -74,11 +74,12 @@ func newErrNotHandled(smft string, args ...any) error {
 }
 
 const (
-	webhookActionEventDeleted   = "deleted"
-	webhookActionEventOpened    = "opened"
-	webhookActionEventReopened  = "reopened"
-	webhookActionEventClosed    = "closed"
-	webhookActionEventPublished = "published"
+	webhookActionEventDeleted     = "deleted"
+	webhookActionEventOpened      = "opened"
+	webhookActionEventReopened    = "reopened"
+	webhookActionEventClosed      = "closed"
+	webhookActionEventPublished   = "published"
+	webhookActionEventTransferred = "transferred"
 )
 
 // pingEvent are messages sent from GitHub to check the status of a
@@ -803,6 +804,9 @@ func (s *Server) processRelevantRepositoryEvent(
 	if event.GetAction() == webhookActionEventDeleted {
 		topic = events.TopicQueueReconcileEntityDelete
 	}
+	if event.GetAction() == webhookActionEventTransferred {
+		topic = events.TopicQueueReconcileEntityDelete
+	}
 
 	return &processingResult{topic: topic, wrapper: eiw}, nil
 }

internal/controlplane/handlers_githubwebhooks_test.go

@@ -343,10 +343,6 @@ func (s *UnitTestSuite) TestHandleWebHookUnexistentRepoPackage() {
 
 	<-evt.Running()
 
-	// mockStore.EXPECT().
-	// 	GetRepositoryByRepoID(gomock.Any(), gomock.Any()).
-	// 	Return(db.Repository{}, sql.ErrNoRows)
-
 	ts := httptest.NewServer(srv.HandleGitHubWebHook())
 
 	event := github.PackageEvent{
@@ -1759,7 +1755,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
 					},
 				),
 			),
-			topic:      events.TopicQueueEntityEvaluate,
+			topic:      events.TopicQueueReconcileEntityDelete,
 			statusCode: http.StatusOK,
 			//nolint:thelper
 			queued: func(t *testing.T, event string, ch <-chan *message.Message) {
@@ -1851,7 +1847,7 @@ func (s *UnitTestSuite) TestHandleGitHubWebHook() {
 			event: "repository",
 			// https://pkg.go.dev/github.com/google/go-github/[email protected]/github#RepositoryEvent
 			payload: &github.RepositoryEvent{
-				Action: github.String("transferred"),
+				Action: github.String("created"),
 				Repo: &github.Repository{
 					ID:       github.Int64(12345),
 					Name:     github.String("minder"),

internal/repositories/github/clients/mock/fixtures/client.go

@@ -94,6 +94,15 @@ func WithNotFoundDeletion(mock ClientMock) {
 	stubDelete(mock, githubResp, ErrClientTest)
 }
 
+func WithForbiddenDeletion(mock ClientMock) {
+	githubResp := &github.Response{
+		Response: &http.Response{
+			StatusCode: http.StatusForbidden,
+		},
+	}
+	stubDelete(mock, githubResp, ErrClientTest)
+}
+
 func stubDelete(mock ClientMock, resp *github.Response, err error) {
 	mock.EXPECT().
 		DeleteHook(gomock.Any(), gomock.Eq(RepoOwner), gomock.Eq(RepoName), gomock.Eq(HookID)).

internal/repositories/github/webhooks/manager.go

@@ -134,7 +134,14 @@ func (_ *webhookManager) DeleteWebhook(
 	resp, err := client.DeleteHook(ctx, repoOwner, repoName, hookID)
 	if err != nil {
 		if resp != nil && resp.StatusCode == http.StatusNotFound {
-			// if the hook is not found, we can ignore the error, user might have deleted it manually
+			// If the hook is not found, we can ignore the
+			// error, user might have deleted it manually.
+			return nil
+		}
+		if resp != nil && resp.StatusCode == http.StatusForbidden {
+			// We ignore deleting webhooks that we're not
+			// allowed to touch. This is usually the case
+			// with repository transfer.
 			return nil
 		}
 		return fmt.Errorf("error deleting hook: %w", err)

internal/repositories/github/webhooks/manager_test.go

@@ -109,6 +109,11 @@ func TestWebhookManager_DeleteWebhook(t *testing.T) {
 			ClientSetup:   cf.NewClientMock(cf.WithSuccessfulDeletion),
 			ShouldSucceed: true,
 		},
+		{
+			Name:          "DeleteWebhook ignores forbidden",
+			ClientSetup:   cf.NewClientMock(cf.WithForbiddenDeletion),
+			ShouldSucceed: true,
+		},
 	}
 
 	for _, scenario := range deletionScenarios {