diff --git a/README.md b/README.md index 2b925c6..fc6fb70 100644 --- a/README.md +++ b/README.md @@ -111,10 +111,12 @@ On linked repositories, GitHub webhooks are configured for: - push - pull_request -- check_run Current behavior includes: +- Dropping any event other than `pull_request`/`push` before the repository + lookup, so a hook left subscribed to `check_run` (one delivery per CI job + per state) costs nothing. - Verifying X-Hub-Signature-256 with the stored webhook secret. - Caching/updating pull request state in plugin tables. - Auto-linking PRs when opened/reopened from a branch already linked to a task. diff --git a/backend/integration.go b/backend/integration.go index 1507d36..c4d046b 100644 --- a/backend/integration.go +++ b/backend/integration.go @@ -338,7 +338,7 @@ func (p *githubPlugin) linkRepository(req *plugin.Request, res *plugin.Response) webhookSecret := hex.EncodeToString(secretBytes) webhookID, err := ghc.createWebhook(context.Background(), ghRepo.Owner.Login, ghRepo.Name, webhookURL, webhookSecret, - []string{"push", "pull_request", "check_run"}) + []string{"push", "pull_request"}) if err != nil { var apiErr *ghAPIError if errors.As(err, &apiErr) && isWebhookURLNotPublic(apiErr) { diff --git a/backend/plugin_test.go b/backend/plugin_test.go index cb2a3cd..9891e86 100644 --- a/backend/plugin_test.go +++ b/backend/plugin_test.go @@ -436,9 +436,9 @@ func TestListTaskPRs_SkipsLinkFromAnotherProject(t *testing.T) { // can't distinguish "accepted" from "rejected" the way it can for the // authenticated API routes elsewhere in this file. These tests instead // assert on tc.Log (plugintest.CapturingLogger), using the distinct log -// line each outcome takes — an event that reaches handleWebhookEvent's -// `default:` case only gets there after signature verification succeeds, -// which is what makes "unhandled event type" a reliable positive signal. +// line each outcome takes — "handling pull_request event" is only logged by +// handleWebhookEvent, which runs after signature verification succeeds, so it +// is a reliable positive signal that the delivery was actually dispatched. const testEncryptionKey = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" // 64 hex chars = 32 bytes @@ -449,7 +449,7 @@ func signedWebhookRequest(projectID string, payload []byte, secret string) plugi return plugintest.Request{ PathParams: map[string]string{"projectId": projectID}, Headers: map[string]string{ - "X-Github-Event": "workflow_run", // any type receiveWebhook doesn't specifically handle + "X-Github-Event": "pull_request", "X-Hub-Signature-256": sig, }, Body: payload, @@ -497,7 +497,7 @@ func TestReceiveWebhook_ScopesRepositoryLookupToURLProject(t *testing.T) { if tc.Log.HasMessage("invalid webhook signature") { t.Fatal("signature verified against the wrong project's secret — repository lookup is not scoped to the URL's project") } - if !tc.Log.HasMessage("unhandled event type") { + if !tc.Log.HasMessage("handling pull_request event") { t.Fatalf("expected signature verification to succeed and reach event dispatch; log entries: %+v", tc.Log.Entries()) } } @@ -517,7 +517,7 @@ func TestReceiveWebhook_RejectsMissingSecret(t *testing.T) { payload := []byte(`{"repository":{"full_name":"octocat/hello-world"}}`) req := plugintest.Request{ PathParams: map[string]string{"projectId": testProjectID}, - Headers: map[string]string{"X-Github-Event": "workflow_run", "X-Hub-Signature-256": "sha256=deadbeef"}, + Headers: map[string]string{"X-Github-Event": "pull_request", "X-Hub-Signature-256": "sha256=deadbeef"}, Body: payload, } res := tc.Call("POST", "/webhook", req) @@ -527,7 +527,7 @@ func TestReceiveWebhook_RejectsMissingSecret(t *testing.T) { if !tc.Log.HasMessage("no webhook secret configured") { t.Fatalf("expected the missing-secret delivery to be refused before dispatch; log entries: %+v", tc.Log.Entries()) } - if tc.Log.HasMessage("unhandled event type") { + if tc.Log.HasMessage("handling pull_request event") { t.Fatal("delivery with no configured secret reached event dispatch — HMAC verification was skipped instead of failing closed") } } diff --git a/backend/webhook.go b/backend/webhook.go index 292c446..a2cb136 100644 --- a/backend/webhook.go +++ b/backend/webhook.go @@ -13,6 +13,19 @@ import ( plugin "github.com/Paca-AI/plugin-sdk-go" ) +// handlesWebhookEvent reports whether this plugin processes event at all. +// The set is deliberately closed: anything else is dropped before the +// repository lookup, so a subscription that grows on GitHub's side cannot +// make every delivery pay for a database query. +func handlesWebhookEvent(event string) bool { + switch event { + case "pull_request", "push": + return true + default: + return false + } +} + // branchTaskRefRe matches a task-ID prefix in a branch name (e.g. "PROJ-42"). var branchTaskRefRe = regexp.MustCompile(`(?i)\b([A-Z][A-Z0-9]{1,19})-(\d{1,6})\b`) @@ -34,6 +47,17 @@ func (p *githubPlugin) receiveWebhook(req *plugin.Request, res *plugin.Response) return } + // Only the two events this plugin acts on get any further work. A hook + // created before this gate existed is still subscribed to check_run, and + // GitHub sends one such delivery per CI job per state — each of which + // would otherwise cost a repository lookup and an HMAC verification + // before landing in handleWebhookEvent's default branch. + if !handlesWebhookEvent(event) { + p.log.Info("github: ignoring event this plugin does not handle, event=" + event) + res.NoContent() + return + } + repoFullName := extractRepoFullName(body) if repoFullName == "" { res.NoContent() diff --git a/backend/webhook_test.go b/backend/webhook_test.go new file mode 100644 index 0000000..6a1422c --- /dev/null +++ b/backend/webhook_test.go @@ -0,0 +1,45 @@ +package main + +import "testing" + +// The gate is the reason a repository hook subscribed to check_run — which +// GitHub delivers once per CI job per state — costs this plugin nothing. +func TestHandlesWebhookEvent(t *testing.T) { + cases := map[string]bool{ + "pull_request": true, + "push": true, + "check_run": false, + "check_suite": false, + "workflow_run": false, + "issues": false, + "": false, + } + for event, want := range cases { + if got := handlesWebhookEvent(event); got != want { + t.Errorf("handlesWebhookEvent(%q) = %v, want %v", event, got, want) + } + } +} + +// A check_run delivery — one per CI job per state — must cost this plugin +// nothing: no repository lookup, no HMAC work, no dispatch. Asserted on the +// log lines, since receiveWebhook answers 204 either way. +func TestReceiveWebhook_IgnoresUnhandledEventType(t *testing.T) { + tc := setupPlugin(t) + seedEncryptedRepo(t, tc, "repo-1", testProjectID, "octocat/hello-world", "my-projects-secret") + + payload := []byte(`{"repository":{"full_name":"octocat/hello-world"},"action":"completed"}`) + req := signedWebhookRequest(testProjectID, payload, "my-projects-secret") + req.Headers["X-Github-Event"] = "check_run" + + res := tc.Call("POST", "/webhook", req) + if res.StatusCode != 204 { + t.Fatalf("expected 204, got %d: %s", res.StatusCode, res.BodyString()) + } + if !tc.Log.HasMessage("ignoring event this plugin does not handle") { + t.Fatalf("expected the unhandled event to be dropped before dispatch; log entries: %+v", tc.Log.Entries()) + } + if tc.Log.HasMessage("webhook received") { + t.Fatalf("unhandled event reached handleWebhookEvent (repository lookup + HMAC); log entries: %+v", tc.Log.Entries()) + } +}