diff --git a/internal/config/config.go b/internal/config/config.go index 1431f070..eac48ac9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -479,6 +479,22 @@ func Load() *Config { cfg.GitHubAppWebhookSecret = os.Getenv("GITHUB_APP_WEBHOOK_SECRET") cfg.GitHubAppClientID = os.Getenv("GITHUB_APP_CLIENT_ID") cfg.GitHubAppClientSecret = os.Getenv("GITHUB_APP_CLIENT_SECRET") + // Fail-closed when the App is enabled: an empty GITHUB_APP_WEBHOOK_SECRET + // makes the webhook HMAC verifiable with a publicly-known (empty) key — any + // attacker could forge a valid X-Hub-Signature-256. Likewise an empty + // private key / App ID can't mint tokens. Panic at Load (like JWT_SECRET) + // rather than silently serve an auth-bypassing webhook. (Review HIGH-1.) + if cfg.GitHubAppEnabled { + if len(strings.TrimSpace(cfg.GitHubAppWebhookSecret)) < 16 { + panic(&ErrMissingConfig{Key: "GITHUB_APP_WEBHOOK_SECRET (>=16 chars required when GITHUB_APP_ENABLED=true)"}) + } + if strings.TrimSpace(cfg.GitHubAppPrivateKey) == "" { + panic(&ErrMissingConfig{Key: "GITHUB_APP_PRIVATE_KEY (required when GITHUB_APP_ENABLED=true)"}) + } + if strings.TrimSpace(cfg.GitHubAppID) == "" { + panic(&ErrMissingConfig{Key: "GITHUB_APP_ID (required when GITHUB_APP_ENABLED=true)"}) + } + } if len(cfg.JWTSecret) < 32 { panic("JWT_SECRET must be at least 32 bytes") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 49fc244c..197387ef 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -374,8 +374,18 @@ func TestLoad_DeploySourceGitEnabled(t *testing.T) { } func TestLoad_GitHubAppEnabled(t *testing.T) { + // When enabling the App, Load() fails closed unless the webhook secret + + // private key + app id are present (review HIGH-1), so set them here. + appSecrets := func(enabled string) map[string]string { + return map[string]string{ + "GITHUB_APP_ENABLED": enabled, + "GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret", + "GITHUB_APP_PRIVATE_KEY": "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----", + "GITHUB_APP_ID": "12345", + } + } for _, val := range []string{"true", "1", "yes", "TRUE", " Yes "} { - applyBaselineEnv(t, map[string]string{"GITHUB_APP_ENABLED": val}) + applyBaselineEnv(t, appSecrets(val)) if !Load().GitHubAppEnabled { t.Errorf("GITHUB_APP_ENABLED=%q should enable", val) } @@ -386,6 +396,28 @@ func TestLoad_GitHubAppEnabled(t *testing.T) { t.Errorf("GITHUB_APP_ENABLED=%q should stay disabled", val) } } + + // Fail-closed: enabling without each required secret must panic, not + // silently serve an HMAC-bypassable / token-less App. + mustPanic := func(name string, env map[string]string) { + defer func() { + if recover() == nil { + t.Errorf("%s: Load() must panic", name) + } + }() + applyBaselineEnv(t, env) + _ = Load() + } + mustPanic("no webhook secret", map[string]string{"GITHUB_APP_ENABLED": "true"}) + mustPanic("no private key", map[string]string{ + "GITHUB_APP_ENABLED": "true", + "GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret", + }) + mustPanic("no app id", map[string]string{ + "GITHUB_APP_ENABLED": "true", + "GITHUB_APP_WEBHOOK_SECRET": "a-sufficiently-long-webhook-secret", + "GITHUB_APP_PRIVATE_KEY": "-----BEGIN RSA PRIVATE KEY-----\nx\n-----END RSA PRIVATE KEY-----", + }) // the GITHUB_APP_* values are plumbed verbatim. applyBaselineEnv(t, map[string]string{ "GITHUB_APP_ID": "12345", diff --git a/internal/github/webhook.go b/internal/github/webhook.go new file mode 100644 index 00000000..e32fb625 --- /dev/null +++ b/internal/github/webhook.go @@ -0,0 +1,129 @@ +// Webhook verification and event parsing helpers for the InstaNode GitHub App +// (P4.2). These functions are called by the HTTP handler that receives +// X-GitHub-Event deliveries; they must be called before any payload processing +// so an unverified payload is never acted on. +package github + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "strings" +) + +// VerifyWebhookSignature checks the X-Hub-Signature-256 header GitHub sends +// with every delivery. It returns nil only when the HMAC-SHA256 computed from +// body and secret matches the signature in signatureHeader; any other condition +// (missing header, bad prefix, non-hex value, or mismatch) returns an error. +// Comparison is constant-time (hmac.Equal) so this is safe against timing +// attacks. Error messages never echo the secret or the expected signature. +func VerifyWebhookSignature(body []byte, signatureHeader, secret string) error { + if signatureHeader == "" { + return fmt.Errorf("github webhook: missing X-Hub-Signature-256 header") + } + const prefix = "sha256=" + if !strings.HasPrefix(signatureHeader, prefix) { + return fmt.Errorf("github webhook: signature header has wrong prefix (expected sha256=)") + } + hexSig := signatureHeader[len(prefix):] + gotSig, err := hex.DecodeString(hexSig) + if err != nil { + return fmt.Errorf("github webhook: signature is not valid hex") + } + + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + wantSig := mac.Sum(nil) + + if !hmac.Equal(wantSig, gotSig) { + return fmt.Errorf("github webhook: signature mismatch") + } + return nil +} + +// pushEventWire is the minimal shape of a GitHub push event we need. +type pushEventWire struct { + Ref string `json:"ref"` + After string `json:"after"` + Repository struct { + FullName string `json:"full_name"` + } `json:"repository"` + Installation struct { + ID int64 `json:"id"` + } `json:"installation"` +} + +// PushEvent holds the normalised fields from a GitHub push webhook payload. +type PushEvent struct { + // Repo is repository.full_name, e.g. "acme/my-app". + Repo string + // Ref is the full ref string, e.g. "refs/heads/main" or "refs/tags/v1.0". + Ref string + // HeadCommitSHA is the "after" field — the SHA of the head commit after the push. + HeadCommitSHA string + // InstallationID is installation.id from the delivery envelope. + InstallationID int64 +} + +// Branch returns the short branch name extracted from Ref (e.g. "main" from +// "refs/heads/main"). It returns "" when Ref is not a branch ref (e.g. a tag +// or an empty string). +func (e *PushEvent) Branch() string { + const branchPrefix = "refs/heads/" + if strings.HasPrefix(e.Ref, branchPrefix) { + return e.Ref[len(branchPrefix):] + } + return "" +} + +// ParsePushEvent decodes a GitHub push event payload. Only the fields InstaNode +// needs for a build dispatch are populated; every other field is ignored. +func ParsePushEvent(body []byte) (*PushEvent, error) { + var w pushEventWire + if err := json.Unmarshal(body, &w); err != nil { + return nil, fmt.Errorf("github webhook: parse push event: %w", err) + } + return &PushEvent{ + Repo: w.Repository.FullName, + Ref: w.Ref, + HeadCommitSHA: w.After, + InstallationID: w.Installation.ID, + }, nil +} + +// installationEventWire is the minimal shape of a GitHub installation event. +type installationEventWire struct { + Action string `json:"action"` + Installation struct { + ID int64 `json:"id"` + Account struct { + Login string `json:"login"` + } `json:"account"` + } `json:"installation"` +} + +// InstallationEvent holds the normalised fields from a GitHub installation +// webhook payload (created / deleted / suspend / unsuspend). +type InstallationEvent struct { + // Action is the lifecycle verb: "created", "deleted", "suspend", or "unsuspend". + Action string + // InstallationID is installation.id. + InstallationID int64 + // AccountLogin is installation.account.login — the GitHub user or org that owns the install. + AccountLogin string +} + +// ParseInstallationEvent decodes a GitHub installation event payload. +func ParseInstallationEvent(body []byte) (*InstallationEvent, error) { + var w installationEventWire + if err := json.Unmarshal(body, &w); err != nil { + return nil, fmt.Errorf("github webhook: parse installation event: %w", err) + } + return &InstallationEvent{ + Action: w.Action, + InstallationID: w.Installation.ID, + AccountLogin: w.Installation.Account.Login, + }, nil +} diff --git a/internal/github/webhook_test.go b/internal/github/webhook_test.go new file mode 100644 index 00000000..d45845f7 --- /dev/null +++ b/internal/github/webhook_test.go @@ -0,0 +1,203 @@ +package github + +import ( + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "testing" +) + +// makeHMAC computes the sha256= signature the same way GitHub does. +func makeHMAC(secret string, body []byte) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + return "sha256=" + hex.EncodeToString(mac.Sum(nil)) +} + +func TestVerifyWebhookSignature(t *testing.T) { + body := []byte(`{"action":"created"}`) + secret := "super-secret" + goodSig := makeHMAC(secret, body) + + tests := []struct { + name string + body []byte + header string + secret string + wantErr bool + errSubstr string + }{ + { + name: "valid signature", + body: body, + header: goodSig, + secret: secret, + wantErr: false, + }, + { + name: "empty header", + body: body, + header: "", + secret: secret, + wantErr: true, + errSubstr: "missing", + }, + { + name: "wrong prefix", + body: body, + header: "md5=" + hex.EncodeToString([]byte("whatever")), + secret: secret, + wantErr: true, + errSubstr: "prefix", + }, + { + name: "non-hex value after prefix", + body: body, + header: "sha256=notvalidhex!!", + secret: secret, + wantErr: true, + errSubstr: "not valid hex", + }, + { + name: "signature mismatch — wrong secret", + body: body, + header: makeHMAC("wrong-secret", body), + secret: secret, + wantErr: true, + errSubstr: "mismatch", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := VerifyWebhookSignature(tc.body, tc.header, tc.secret) + if tc.wantErr { + if err == nil { + t.Fatalf("expected error containing %q, got nil", tc.errSubstr) + } + if tc.errSubstr != "" && !containsStr(err.Error(), tc.errSubstr) { + t.Fatalf("error %q does not contain %q", err.Error(), tc.errSubstr) + } + } else { + if err != nil { + t.Fatalf("expected nil error, got: %v", err) + } + } + }) + } +} + +func TestParsePushEvent(t *testing.T) { + t.Run("valid push payload", func(t *testing.T) { + payload := map[string]interface{}{ + "ref": "refs/heads/main", + "after": "abc123def456", + "repository": map[string]interface{}{ + "full_name": "acme/my-app", + }, + "installation": map[string]interface{}{ + "id": float64(42), + }, + } + body, _ := json.Marshal(payload) + + ev, err := ParsePushEvent(body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ev.Repo != "acme/my-app" { + t.Errorf("Repo: got %q, want %q", ev.Repo, "acme/my-app") + } + if ev.Ref != "refs/heads/main" { + t.Errorf("Ref: got %q, want %q", ev.Ref, "refs/heads/main") + } + if ev.HeadCommitSHA != "abc123def456" { + t.Errorf("HeadCommitSHA: got %q, want %q", ev.HeadCommitSHA, "abc123def456") + } + if ev.InstallationID != 42 { + t.Errorf("InstallationID: got %d, want %d", ev.InstallationID, 42) + } + if got := ev.Branch(); got != "main" { + t.Errorf("Branch(): got %q, want %q", got, "main") + } + }) + + t.Run("tag ref → Branch returns empty string", func(t *testing.T) { + payload := map[string]interface{}{ + "ref": "refs/tags/v1.0", + "after": "deadbeef", + "repository": map[string]interface{}{ + "full_name": "acme/my-app", + }, + "installation": map[string]interface{}{ + "id": float64(1), + }, + } + body, _ := json.Marshal(payload) + ev, err := ParsePushEvent(body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := ev.Branch(); got != "" { + t.Errorf("Branch(): got %q, want empty string for tag ref", got) + } + }) + + t.Run("invalid JSON", func(t *testing.T) { + _, err := ParsePushEvent([]byte(`{not json`)) + if err == nil { + t.Fatal("expected error for invalid JSON, got nil") + } + }) +} + +func TestParseInstallationEvent(t *testing.T) { + t.Run("valid installation payload", func(t *testing.T) { + payload := map[string]interface{}{ + "action": "created", + "installation": map[string]interface{}{ + "id": float64(99), + "account": map[string]interface{}{ + "login": "acme-org", + }, + }, + } + body, _ := json.Marshal(payload) + + ev, err := ParseInstallationEvent(body) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if ev.Action != "created" { + t.Errorf("Action: got %q, want %q", ev.Action, "created") + } + if ev.InstallationID != 99 { + t.Errorf("InstallationID: got %d, want %d", ev.InstallationID, 99) + } + if ev.AccountLogin != "acme-org" { + t.Errorf("AccountLogin: got %q, want %q", ev.AccountLogin, "acme-org") + } + }) + + t.Run("invalid JSON", func(t *testing.T) { + _, err := ParseInstallationEvent([]byte(`[bad`)) + if err == nil { + t.Fatal("expected error for invalid JSON, got nil") + } + }) +} + +// containsStr is a simple substring helper kept here to avoid importing +// strings in the test for just one call. +func containsStr(s, sub string) bool { + return len(s) >= len(sub) && (s == sub || len(sub) == 0 || + func() bool { + for i := 0; i <= len(s)-len(sub); i++ { + if s[i:i+len(sub)] == sub { + return true + } + } + return false + }()) +} diff --git a/internal/handlers/github_app.go b/internal/handlers/github_app.go index 0debf2d0..5f8a239a 100644 --- a/internal/handlers/github_app.go +++ b/internal/handlers/github_app.go @@ -18,6 +18,7 @@ package handlers import ( "database/sql" + "errors" "fmt" "log/slog" "net/url" @@ -117,6 +118,14 @@ func (h *GitHubAppHandler) Callback(c *fiber.Ctx) error { } // account_login is enriched by the installation webhook (P4.2); empty here. if _, err := models.UpsertGitHubInstallation(c.Context(), h.db, instID, teamUUID, ""); err != nil { + var conflict *models.ErrGitHubInstallationTeamConflict + if errors.As(err, &conflict) { + // The installation is already linked to a different team — refuse + // rather than rebind (anti-hijack, review HIGH-2). + slog.Warn("github_app.callback.team_conflict", "installation_id", instID, "team_id", teamID) + return respondError(c, fiber.StatusConflict, "install_conflict", + "This GitHub installation is already linked to another InstaNode team") + } slog.Error("github_app.callback.persist_failed", "error", err, "installation_id", instID) return respondError(c, fiber.StatusServiceUnavailable, "install_persist_failed", "Could not save the GitHub installation") diff --git a/internal/handlers/github_app_integration_test.go b/internal/handlers/github_app_integration_test.go index df6c9d17..6955ffe7 100644 --- a/internal/handlers/github_app_integration_test.go +++ b/internal/handlers/github_app_integration_test.go @@ -6,6 +6,7 @@ package handlers_test // DB-gated; skips locally without a DB. import ( + "context" "net/http" "net/http/httptest" "strings" @@ -18,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "instant.dev/internal/config" + "instant.dev/internal/models" "instant.dev/internal/testhelpers" ) @@ -179,6 +181,41 @@ func TestGitHubAppCallback_PersistError_503(t *testing.T) { assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) } +// An installation already linked to team A cannot be rebound by team B's +// callback → 409 install_conflict (review HIGH-2 anti-hijack). +func TestGitHubAppCallback_InstallConflict_409(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + teamA := testhelpers.MustCreateTeamDB(t, db, "pro") + teamB := testhelpers.MustCreateTeamDB(t, db, "pro") + const instID int64 = 556677 + // team A owns the installation first. + _, err := models.UpsertGitHubInstallation(context.Background(), db, instID, uuid.MustParse(teamA), "acme") + require.NoError(t, err) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", appEnabled) + defer clean() + + // team B tries to bind the same installation via a state legitimately signed + // for team B → must be refused. + state := signInstallState(t, teamB) + req := httptest.NewRequest(http.MethodGet, + "/integrations/github/callback?installation_id=556677&state="+state, nil) + resp, err := app.Test(req, 5000) + require.NoError(t, err) + defer resp.Body.Close() + assert.Equal(t, http.StatusConflict, resp.StatusCode) + + // team A's row is untouched. + inst, gerr := models.GetGitHubInstallation(context.Background(), db, instID) + require.NoError(t, gerr) + assert.Equal(t, teamA, inst.TeamID.String(), "victim installation must remain bound to team A") +} + func TestGitHubAppCallback_PersistsAndRedirects(t *testing.T) { daDeployNeedsDB(t) db, cleanDB := testhelpers.SetupTestDB(t) diff --git a/internal/handlers/github_app_webhook.go b/internal/handlers/github_app_webhook.go new file mode 100644 index 00000000..32925754 --- /dev/null +++ b/internal/handlers/github_app_webhook.go @@ -0,0 +1,221 @@ +package handlers + +// github_app_webhook.go — the single App-level GitHub webhook (P4.2). Distinct +// from the manual per-connection receiver (github_deploy.go Receive, keyed by +// :webhook_id with a per-connection secret): GitHub delivers ALL events for the +// InstaNode App here, signed with the one App webhook secret. On `push` we match +// the repo+branch to connection(s) linked to that installation and enqueue the +// same redeploy the manual path uses; on `installation` lifecycle events we keep +// github_installations in sync (so a deleted/suspended install stops deploying). +// +// Responses are plain statuses for GitHub's consumption (no agent_action +// envelope — GitHub is the only caller). Gated by cfg.GitHubAppEnabled. + +import ( + "database/sql" + "errors" + "log/slog" + "regexp" + "time" + + "github.com/gofiber/fiber/v2" + "github.com/redis/go-redis/v9" + + "instant.dev/internal/config" + "instant.dev/internal/github" + "instant.dev/internal/metrics" + "instant.dev/internal/middleware" + "instant.dev/internal/models" + "instant.dev/internal/plans" +) + +// githubAppDeliveryDedupTTL is how long a processed X-GitHub-Delivery id is +// remembered so a GitHub redelivery is a no-op (well over GitHub's retry window). +const githubAppDeliveryDedupTTL = 24 * time.Hour + +// knownWebhookEvents bounds the {event} metric label. The X-GitHub-Event header +// is attacker-controlled and the label is written BEFORE auth (on a bad +// signature), so an unbounded value would let an unauthenticated caller blow up +// Prometheus series cardinality (review MED-1). Anything unrecognised → "other". +var knownWebhookEvents = map[string]bool{ + "push": true, "installation": true, "installation_repositories": true, + "ping": true, "pull_request": true, "check_run": true, "check_suite": true, + "create": true, "delete": true, "release": true, +} + +func canonicalizeWebhookEvent(event string) string { + if knownWebhookEvents[event] { + return event + } + return "other" +} + +// commitSHARE matches a full 40-hex git SHA. A push payload's `after` is +// attacker-shaped; anything that isn't a real SHA is not deployable (review LOW-2). +var commitSHARE = regexp.MustCompile(`^[0-9a-f]{40}$`) + +// GitHubAppWebhookHandler serves POST /webhooks/github. +type GitHubAppWebhookHandler struct { + db *sql.DB + rdb *redis.Client + cfg *config.Config + plan *plans.Registry +} + +// NewGitHubAppWebhookHandler constructs the handler. +func NewGitHubAppWebhookHandler(db *sql.DB, rdb *redis.Client, cfg *config.Config, planRegistry *plans.Registry) *GitHubAppWebhookHandler { + return &GitHubAppWebhookHandler{db: db, rdb: rdb, cfg: cfg, plan: planRegistry} +} + +// Receive verifies the App-level HMAC, dedupes on delivery id, and dispatches. +func (h *GitHubAppWebhookHandler) Receive(c *fiber.Ctx) error { + if !h.cfg.GitHubAppEnabled { + metrics.GitHubWebhookReceivedTotal.WithLabelValues("", "disabled").Inc() + return c.SendStatus(fiber.StatusNotImplemented) + } + + body := c.Body() + if len(body) > githubMaxWebhookBodyBytes { + metrics.GitHubWebhookReceivedTotal.WithLabelValues("", "error").Inc() + return c.SendStatus(fiber.StatusRequestEntityTooLarge) + } + + event := c.Get("X-GitHub-Event") + // metricEvent is the bounded label (raw event drives dispatch only). + metricEvent := canonicalizeWebhookEvent(event) + if err := github.VerifyWebhookSignature(body, c.Get("X-Hub-Signature-256"), h.cfg.GitHubAppWebhookSecret); err != nil { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(metricEvent, "bad_signature").Inc() + slog.Warn("github_app.webhook.signature_failed", + "event", metricEvent, "ip", c.IP(), "request_id", middleware.GetRequestID(c)) + return c.SendStatus(fiber.StatusUnauthorized) + } + + // Idempotency: a redelivered X-GitHub-Delivery is a no-op. SETNX; fail open + // on a Redis error (a rare double-deploy beats a missed deploy). The header + // is GitHub-guaranteed; an absent one (or nil rdb) skips dedup by design. + if delivery := c.Get("X-GitHub-Delivery"); delivery != "" && h.rdb != nil { + ok, err := h.rdb.SetNX(c.Context(), "ghapp:delivery:"+delivery, "1", githubAppDeliveryDedupTTL).Result() + if err == nil && !ok { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(metricEvent, "replay").Inc() + return c.JSON(fiber.Map{"ok": true, "duplicate": true}) + } + } + + switch event { + case "ping": + metrics.GitHubWebhookReceivedTotal.WithLabelValues(metricEvent, "ok").Inc() + return c.JSON(fiber.Map{"ok": true, "pong": true}) + case "installation": + return h.handleInstallation(c, body, metricEvent) + case "push": + return h.handlePush(c, body, metricEvent) + default: + // pull_request, check_run, etc. — accept (2xx) so GitHub shows green. + metrics.GitHubWebhookReceivedTotal.WithLabelValues(metricEvent, "ok").Inc() + return c.JSON(fiber.Map{"ok": true, "ignored": true, "event": event}) + } +} + +// handleInstallation keeps github_installations in sync with the App lifecycle. +// created is a no-op here (the install callback already persisted the team +// link; the webhook has no team_id of ours to bind). delete/suspend/unsuspend +// are keyed on installation_id alone and stop/resume deploys. +func (h *GitHubAppWebhookHandler) handleInstallation(c *fiber.Ctx, body []byte, event string) error { + ev, err := github.ParseInstallationEvent(body) + if err != nil { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "error").Inc() + return c.SendStatus(fiber.StatusBadRequest) + } + switch ev.Action { + case "deleted": + if _, derr := models.DeleteGitHubInstallation(c.Context(), h.db, ev.InstallationID); derr != nil { + slog.Warn("github_app.webhook.installation_delete_failed", "error", derr, "installation_id", ev.InstallationID) + } + case "suspend": + if serr := models.SetGitHubInstallationSuspended(c.Context(), h.db, ev.InstallationID, true); serr != nil { + slog.Warn("github_app.webhook.installation_suspend_failed", "error", serr, "installation_id", ev.InstallationID) + } + case "unsuspend": + if serr := models.SetGitHubInstallationSuspended(c.Context(), h.db, ev.InstallationID, false); serr != nil { + slog.Warn("github_app.webhook.installation_unsuspend_failed", "error", serr, "installation_id", ev.InstallationID) + } + } + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "ok").Inc() + return c.JSON(fiber.Map{"ok": true, "action": ev.Action}) +} + +// handlePush matches the push to connection(s) for that repo+branch+installation +// and enqueues a redeploy for each (rate-limited, same path as the manual webhook). +func (h *GitHubAppWebhookHandler) handlePush(c *fiber.Ctx, body []byte, event string) error { + ev, err := github.ParsePushEvent(body) + if err != nil { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "error").Inc() + return c.SendStatus(fiber.StatusBadRequest) + } + branch := ev.Branch() + // Non-branch ref (tag), branch delete (all-zero SHA), or a non-SHA `after` + // (hostile payload) → nothing to deploy. commitSHARE rejects the all-zeros + // delete SHA too (not 40 lowercase hex... it is hex — but a delete carries + // branch="" only when ref isn't a head; guard both). + if branch == "" || !commitSHARE.MatchString(ev.HeadCommitSHA) || ev.HeadCommitSHA == "0000000000000000000000000000000000000000" { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "ok").Inc() + return c.JSON(fiber.Map{"ok": true, "ignored": true, "reason": "no_deployable_ref"}) + } + + // Security: the installation must exist for us and not be suspended. We never + // act on a push whose installation we don't own / has been revoked. + inst, ierr := models.GetGitHubInstallation(c.Context(), h.db, ev.InstallationID) + if ierr != nil || inst.SuspendedAt.Valid { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "no_match").Inc() + return c.JSON(fiber.Map{"ok": true, "ignored": true, "reason": "no_active_installation"}) + } + + conns, cerr := models.FindConnectionsByRepoBranch(c.Context(), h.db, ev.Repo, branch) + if cerr != nil { + slog.Error("github_app.webhook.connection_lookup_failed", "error", cerr, "repo", ev.Repo, "branch", branch) + metrics.GitHubPushDeployTotal.WithLabelValues("error").Inc() + return c.SendStatus(fiber.StatusServiceUnavailable) + } + + since := time.Now().Add(-githubRateLimitWindow) + matched, enqueued := 0, 0 + for _, conn := range conns { + // The connection MUST belong to the same installation+team the push came + // from — never deploy a repo for an installation the team doesn't own. + if !conn.InstallationID.Valid || conn.InstallationID.Int64 != ev.InstallationID || conn.TeamID != inst.TeamID { + continue + } + matched++ + _, enqErr := models.CountAndEnqueueGitHubDeployLocked(c.Context(), h.db, models.EnqueueGitHubDeployParams{ + ConnectionID: conn.ID, + AppID: conn.AppID, + CommitSHA: ev.HeadCommitSHA, + PusherLogin: "", + }, since, githubMaxDeploysPerHour) + if enqErr != nil { + var rl *models.ErrGitHubDeployRateLimited + if errors.As(enqErr, &rl) { + metrics.GitHubPushDeployTotal.WithLabelValues("rate_limited").Inc() + continue + } + slog.Error("github_app.webhook.enqueue_failed", "error", enqErr, "connection_id", conn.ID) + metrics.GitHubPushDeployTotal.WithLabelValues("error").Inc() + continue + } + if uerr := models.UpdateGitHubConnectionLastDeploy(c.Context(), h.db, conn.ID, ev.HeadCommitSHA); uerr != nil { + slog.Warn("github_app.webhook.last_deploy_update_failed", "error", uerr, "connection_id", conn.ID) + } + enqueued++ + metrics.GitHubPushDeployTotal.WithLabelValues("enqueued").Inc() + } + + if matched == 0 { + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "no_match").Inc() + metrics.GitHubPushDeployTotal.WithLabelValues("no_connection").Inc() + return c.JSON(fiber.Map{"ok": true, "ignored": true, "reason": "no_connection"}) + } + // `enqueued` reports successful enqueues only (≤ matched; the rest were + // rate-limited/errored and counted in their own metric). + metrics.GitHubWebhookReceivedTotal.WithLabelValues(event, "ok").Inc() + return c.Status(fiber.StatusAccepted).JSON(fiber.Map{"ok": true, "matched": matched, "enqueued": enqueued}) +} diff --git a/internal/handlers/github_app_webhook_503_test.go b/internal/handlers/github_app_webhook_503_test.go new file mode 100644 index 00000000..c2175eb1 --- /dev/null +++ b/internal/handlers/github_app_webhook_503_test.go @@ -0,0 +1,50 @@ +package handlers + +// github_app_webhook_503_test.go — covers handlePush's connection-lookup error +// arm (FindConnectionsByRepoBranch fails after the installation lookup +// succeeds → 503). Driven with sqlmock so the two sequential queries can return +// row-then-error deterministically (impossible to stage with a single real DB). +// Reuses whSignBody / whPost from github_app_webhook_whitebox_test.go. + +import ( + "errors" + "net/http" + "testing" + "time" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + + "instant.dev/internal/config" +) + +func TestGitHubAppWebhook_Push_ConnectionLookupError_503(t *testing.T) { + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + defer func() { _ = db.Close() }() + + // 1) installation lookup succeeds (not suspended). + mock.ExpectQuery(`FROM github_installations WHERE installation_id`). + WillReturnRows(sqlmock.NewRows([]string{ + "installation_id", "team_id", "account_login", "suspended_at", "created_at", "updated_at", + }).AddRow(int64(42), uuid.New(), "acme", nil, time.Now(), time.Now())) + // 2) connection lookup errors → handler returns 503. + mock.ExpectQuery(`FROM app_github_connections WHERE github_repo`). + WillReturnError(errors.New("connection lookup boom")) + + h := NewGitHubAppWebhookHandler(db, nil, &config.Config{ + GitHubAppEnabled: true, + GitHubAppWebhookSecret: "whsec", + }, nil) + app := fiber.New() + app.Post("/wh", h.Receive) + + body := []byte(`{"repository":{"full_name":"owner/repo"},"ref":"refs/heads/main","after":"a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0","installation":{"id":42}}`) + resp := whPost(t, app, body, "push", whSignBody("whsec", body), "") + if resp.StatusCode != http.StatusServiceUnavailable { + t.Errorf("connection lookup error must 503, got %d", resp.StatusCode) + } +} diff --git a/internal/handlers/github_app_webhook_errarms_test.go b/internal/handlers/github_app_webhook_errarms_test.go new file mode 100644 index 00000000..66d2ada9 --- /dev/null +++ b/internal/handlers/github_app_webhook_errarms_test.go @@ -0,0 +1,126 @@ +package handlers + +// github_app_webhook_errarms_test.go — sqlmock white-box coverage for the +// defensive error arms in handleInstallation/handlePush that a real-DB +// integration test can't reach (the DB op succeeds normally there): the +// installation delete/suspend/unsuspend slog.Warn arms, the generic +// enqueue-error arm, and the last-deploy-update slog.Warn arm. Reuses +// whSignBody/whPost from github_app_webhook_whitebox_test.go. + +import ( + "errors" + "net/http" + "testing" + "time" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/gofiber/fiber/v2" + "github.com/google/uuid" + + "instant.dev/internal/config" +) + +func newErrArmApp(t *testing.T) (*fiber.App, sqlmock.Sqlmock, func()) { + t.Helper() + db, mock, err := sqlmock.New() + if err != nil { + t.Fatal(err) + } + h := NewGitHubAppWebhookHandler(db, nil, &config.Config{ + GitHubAppEnabled: true, + GitHubAppWebhookSecret: "whsec", + }, nil) + app := fiber.New() + app.Post("/wh", h.Receive) + return app, mock, func() { _ = db.Close() } +} + +// installation op errors → slog.Warn arm, still 200 (covers delete/suspend/unsuspend). +func TestGitHubAppWebhook_InstallationOpErrors(t *testing.T) { + cases := []struct { + action string + expectFn func(sqlmock.Sqlmock) + }{ + {"deleted", func(m sqlmock.Sqlmock) { + m.ExpectExec(`DELETE FROM github_installations`).WillReturnError(errors.New("boom")) + }}, + {"suspend", func(m sqlmock.Sqlmock) { + m.ExpectExec(`UPDATE github_installations SET suspended_at`).WillReturnError(errors.New("boom")) + }}, + {"unsuspend", func(m sqlmock.Sqlmock) { + m.ExpectExec(`UPDATE github_installations SET suspended_at`).WillReturnError(errors.New("boom")) + }}, + } + for _, tc := range cases { + t.Run(tc.action, func(t *testing.T) { + app, mock, done := newErrArmApp(t) + defer done() + tc.expectFn(mock) + body := []byte(`{"action":"` + tc.action + `","installation":{"id":42,"account":{"login":"acme"}}}`) + resp := whPost(t, app, body, "installation", whSignBody("whsec", body), "") + if resp.StatusCode != http.StatusOK { + t.Errorf("%s op error must still 200, got %d", tc.action, resp.StatusCode) + } + }) + } +} + +// pushMatchExpectations stages the installation + connection lookups so a push +// reaches the enqueue step with exactly one matching connection. +func pushMatchExpectations(mock sqlmock.Sqlmock, team uuid.UUID, connID uuid.UUID, installID int64) { + mock.ExpectQuery(`FROM github_installations WHERE installation_id`). + WillReturnRows(sqlmock.NewRows([]string{ + "installation_id", "team_id", "account_login", "suspended_at", "created_at", "updated_at", + }).AddRow(installID, team, "acme", nil, time.Now(), time.Now())) + mock.ExpectQuery(`FROM app_github_connections WHERE github_repo`). + WillReturnRows(sqlmock.NewRows([]string{ + "id", "app_id", "team_id", "github_repo", "branch", "webhook_secret", + "installation_id", "created_at", "last_deploy_at", "last_commit_sha", + }).AddRow(connID, uuid.New(), team, "owner/repo", "main", "sec", installID, time.Now(), nil, nil)) +} + +// pushBody42 is a valid push payload for installation_id 42 with a 40-hex SHA. +func pushBody42() []byte { + return []byte(`{"repository":{"full_name":"owner/repo"},"ref":"refs/heads/main","after":"a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0","installation":{"id":42}}`) +} + +// enqueue returns a generic (non-rate-limit) error → error arm, 202 enqueued:0. +func TestGitHubAppWebhook_Push_EnqueueError(t *testing.T) { + app, mock, done := newErrArmApp(t) + defer done() + team := uuid.New() + pushMatchExpectations(mock, team, uuid.New(), 42) + // CountAndEnqueueGitHubDeployLocked: BeginTx fails → generic error (not rate-limited). + mock.ExpectBegin().WillReturnError(errors.New("begin boom")) + + resp := whPost(t, app, pushBody42(), "push", whSignBody("whsec", pushBody42()), "") + if resp.StatusCode != http.StatusAccepted { + t.Fatalf("want 202 (matched but enqueue errored), got %d", resp.StatusCode) + } +} + +// enqueue succeeds but UpdateGitHubConnectionLastDeploy errors → slog.Warn arm, +// still counts as enqueued, 202. +func TestGitHubAppWebhook_Push_LastDeployUpdateError(t *testing.T) { + app, mock, done := newErrArmApp(t) + defer done() + team := uuid.New() + connID := uuid.New() + pushMatchExpectations(mock, team, connID, 42) + // Full successful enqueue tx. + mock.ExpectBegin() + mock.ExpectQuery(`SELECT id FROM app_github_connections WHERE id = .* FOR UPDATE`). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(connID)) + mock.ExpectQuery(`SELECT COUNT\(\*\) FROM pending_github_deploys`). + WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + mock.ExpectQuery(`INSERT INTO pending_github_deploys`). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(uuid.New())) + mock.ExpectCommit() + // last-deploy bump fails → warn arm. + mock.ExpectExec(`UPDATE app_github_connections`).WillReturnError(errors.New("update boom")) + + resp := whPost(t, app, pushBody42(), "push", whSignBody("whsec", pushBody42()), "") + if resp.StatusCode != http.StatusAccepted { + t.Fatalf("want 202, got %d", resp.StatusCode) + } +} diff --git a/internal/handlers/github_app_webhook_integration_test.go b/internal/handlers/github_app_webhook_integration_test.go new file mode 100644 index 00000000..bfe7ab3a --- /dev/null +++ b/internal/handlers/github_app_webhook_integration_test.go @@ -0,0 +1,476 @@ +package handlers_test + +// github_app_webhook_integration_test.go — DB+Redis-gated coverage for the +// GitHub App webhook handler (POST /webhooks/github, P4.2). These tests cover +// every branch that requires a real Postgres row: installation lifecycle events +// (deleted/suspend/unsuspend), the push happy-path (matched connection → +// pending_github_deploys), push no_connection, push no_active_installation, +// and the delivery-dedup (replay) path. +// +// They skip cleanly when TEST_DATABASE_URL is not set (same guard as the +// existing daDeployNeedsDB helper). + +import ( + "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "instant.dev/internal/config" + "instant.dev/internal/models" + "instant.dev/internal/testhelpers" +) + +// ── test-local helpers ──────────────────────────────────────────────────────── + +const whibWebhookSecret = "whsectest" + +// whibSign computes the X-Hub-Signature-256 value for body + secret. +func whibSign(secret string, body []byte) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + return "sha256=" + hex.EncodeToString(mac.Sum(nil)) +} + +// whibEnableWebhook is a config mutator that turns on the GitHub App webhook +// feature with the shared test secret. +func whibEnableWebhook(c *config.Config) { + c.GitHubAppEnabled = true + c.GitHubAppWebhookSecret = whibWebhookSecret +} + +// whibPost fires POST /webhooks/github on the test Fiber app. +func whibPost(t *testing.T, app interface { + Test(*http.Request, ...int) (*http.Response, error) +}, body []byte, event, delivery string) *http.Response { + t.Helper() + sig := whibSign(whibWebhookSecret, body) + req := httptest.NewRequest(http.MethodPost, "/webhooks/github", strings.NewReader(string(body))) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-GitHub-Event", event) + req.Header.Set("X-Hub-Signature-256", sig) + if delivery != "" { + req.Header.Set("X-GitHub-Delivery", delivery) + } + resp, err := app.Test(req, 10000) + require.NoError(t, err) + return resp +} + +// whibDecodeJSON reads and decodes the entire response body into v. +func whibDecodeJSON(t *testing.T, resp *http.Response, v interface{}) { + t.Helper() + defer resp.Body.Close() + data, err := io.ReadAll(resp.Body) + require.NoError(t, err) + require.NoError(t, json.Unmarshal(data, v)) +} + +// installationBody builds a minimal GitHub installation event JSON payload. +func installationBody(action string, installID int64, login string) []byte { + return []byte(fmt.Sprintf( + `{"action":%q,"installation":{"id":%d,"account":{"login":%q}}}`, + action, installID, login, + )) +} + +// pushEventBody builds a minimal GitHub push event JSON payload. +func pushEventBody(repo, branch, sha string, installID int64) []byte { + return []byte(fmt.Sprintf( + `{"ref":"refs/heads/%s","after":%q,"repository":{"full_name":%q},"installation":{"id":%d}}`, + branch, sha, repo, installID, + )) +} + +// ── installation deleted ────────────────────────────────────────────────────── + +func TestGitHubAppWebhook_Installation_Deleted(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 100101 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + body := installationBody("deleted", installID, "acme-org") + resp := whibPost(t, app, body, "installation", "del-001") + defer resp.Body.Close() + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + // Verify the row is gone. + _, gerr := models.GetGitHubInstallation(ctx, db, installID) + require.Error(t, gerr, "installation row should be deleted") + var nf *models.ErrGitHubInstallationNotFound + assert.ErrorAs(t, gerr, &nf) +} + +// ── installation suspend ────────────────────────────────────────────────────── + +func TestGitHubAppWebhook_Installation_Suspend(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 100102 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + body := installationBody("suspend", installID, "acme-org") + resp := whibPost(t, app, body, "installation", "sus-001") + defer resp.Body.Close() + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + inst, gerr := models.GetGitHubInstallation(ctx, db, installID) + require.NoError(t, gerr) + assert.True(t, inst.SuspendedAt.Valid, "SuspendedAt should be set after suspend event") +} + +// ── installation unsuspend ──────────────────────────────────────────────────── + +func TestGitHubAppWebhook_Installation_Unsuspend(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 100103 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + // Suspend first so there is something to clear. + require.NoError(t, models.SetGitHubInstallationSuspended(ctx, db, installID, true)) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + body := installationBody("unsuspend", installID, "acme-org") + resp := whibPost(t, app, body, "installation", "unsu-001") + defer resp.Body.Close() + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + inst, gerr := models.GetGitHubInstallation(ctx, db, installID) + require.NoError(t, gerr) + assert.False(t, inst.SuspendedAt.Valid, "SuspendedAt should be NULL after unsuspend event") +} + +// ── push happy path → 202 enqueued ─────────────────────────────────────────── + +func TestGitHubAppWebhook_Push_HappyPath_202(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 200201 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + // Seed a deployments row with an explicit UUID for the FK. + deploymentUUID := uuid.New() + _, err = db.Exec(` + INSERT INTO deployments (id, team_id, app_id, port, tier, status) + VALUES ($1, $2, $3, 8080, 'pro', 'healthy')`, + deploymentUUID, teamID, "ghwh-"+deploymentUUID.String()[:8]) + require.NoError(t, err) + + // Wire a github connection: repo="owner/testrepo", branch="main". + instID64 := installID + conn, cerr := models.CreateGitHubConnection(ctx, db, models.CreateGitHubConnectionParams{ + AppID: deploymentUUID, + TeamID: teamID, + GitHubRepo: "owner/testrepo", + Branch: "main", + WebhookSecret: "placeholder_encrypted_secret", + InstallationID: &instID64, + }) + require.NoError(t, cerr) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + const commitSHA = "aabbccdd11223344556677889900aabbccdd1122" + body := pushEventBody("owner/testrepo", "main", commitSHA, installID) + resp := whibPost(t, app, body, "push", "push-happy-001") + defer resp.Body.Close() + + assert.Equal(t, http.StatusAccepted, resp.StatusCode, "happy push must return 202") + + // Verify a pending_github_deploys row was inserted. + var count int + err = db.QueryRowContext(ctx, + `SELECT COUNT(*) FROM pending_github_deploys WHERE connection_id = $1 AND commit_sha = $2`, + conn.ID, commitSHA, + ).Scan(&count) + require.NoError(t, err) + assert.GreaterOrEqual(t, count, 1, "a pending_github_deploys row must exist after a happy push") + + // Verify last_commit_sha was bumped. + var lastSHA string + err = db.QueryRowContext(ctx, + `SELECT COALESCE(last_commit_sha,'') FROM app_github_connections WHERE id = $1`, conn.ID, + ).Scan(&lastSHA) + require.NoError(t, err) + assert.Equal(t, commitSHA, lastSHA) +} + +// ── push rate-limited (cap saturated) → 202 with enqueued:0 ─────────────────── + +func TestGitHubAppWebhook_Push_RateLimited(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 200299 + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + deploymentUUID := uuid.New() + _, err = db.Exec(`INSERT INTO deployments (id, team_id, app_id, port, tier, status) + VALUES ($1, $2, $3, 8080, 'pro', 'healthy')`, + deploymentUUID, teamID, "ghwh-rl-"+deploymentUUID.String()[:8]) + require.NoError(t, err) + + instID64 := installID + conn, cerr := models.CreateGitHubConnection(ctx, db, models.CreateGitHubConnectionParams{ + AppID: deploymentUUID, TeamID: teamID, GitHubRepo: "owner/rl-repo", Branch: "main", + WebhookSecret: "placeholder", InstallationID: &instID64, + }) + require.NoError(t, cerr) + + // Saturate the per-connection hourly cap (githubMaxDeploysPerHour = 10). + for i := 0; i < 10; i++ { + _, eerr := models.EnqueueGitHubDeploy(ctx, db, models.EnqueueGitHubDeployParams{ + ConnectionID: conn.ID, AppID: deploymentUUID, CommitSHA: fmt.Sprintf("%040d", i), PusherLogin: "", + }) + require.NoError(t, eerr) + } + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + body := pushEventBody("owner/rl-repo", "main", "aabbccddeeff00112233445566778899aabbccdd", installID) + resp := whibPost(t, app, body, "push", "push-rl-001") + var out struct { + Matched int `json:"matched"` + Enqueued int `json:"enqueued"` + } + whibDecodeJSON(t, resp, &out) + assert.Equal(t, http.StatusAccepted, resp.StatusCode) + assert.Equal(t, 1, out.Matched, "the connection matched the push") + assert.Equal(t, 0, out.Enqueued, "the matched connection was rate-limited → 0 enqueued") +} + +// ── push no_connection → 200 ignored ───────────────────────────────────────── + +func TestGitHubAppWebhook_Push_NoConnection_Ignored(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 200202 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + // Push for a repo+branch with no matching app_github_connections row. + body := pushEventBody("owner/no-such-repo", "main", "deadbeef0000000000000000000000000000dead", installID) + resp := whibPost(t, app, body, "push", "push-noconn-001") + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + var out map[string]interface{} + whibDecodeJSON(t, resp, &out) + assert.Equal(t, "no_connection", out["reason"], "reason must be no_connection") +} + +// ── push no_active_installation → 200 ignored ──────────────────────────────── + +func TestGitHubAppWebhook_Push_NoActiveInstallation_Ignored(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + // Push whose installation_id has NO github_installations row at all. + const missingInstallID int64 = 999999999 + body := pushEventBody("owner/whatever", "main", "cafebabe0000000000000000000000000000cafe", missingInstallID) + resp := whibPost(t, app, body, "push", "push-noinst-001") + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + var out map[string]interface{} + whibDecodeJSON(t, resp, &out) + assert.Equal(t, "no_active_installation", out["reason"]) +} + +// ── push when installation is suspended → 200 no_active_installation ───────── + +func TestGitHubAppWebhook_Push_SuspendedInstallation_Ignored(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamID := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + const installID int64 = 200203 + + _, err := models.UpsertGitHubInstallation(ctx, db, installID, teamID, "acme-org") + require.NoError(t, err) + require.NoError(t, models.SetGitHubInstallationSuspended(ctx, db, installID, true)) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + body := pushEventBody("owner/repo-suspended", "main", "face00000000000000000000000000000000face", installID) + resp := whibPost(t, app, body, "push", "push-suspended-001") + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + var out map[string]interface{} + whibDecodeJSON(t, resp, &out) + assert.Equal(t, "no_active_installation", out["reason"]) +} + +// ── delivery dedup (replay) ─────────────────────────────────────────────────── + +func TestGitHubAppWebhook_DeliveryDedup_Replay(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + // Use a ping event (no DB access) to isolate the dedup logic. + body := []byte(`{"zen":"Keep it logically awesome.","hook_id":1}`) + const deliveryID = "dedup-delivery-abc123" + + // First delivery — should process normally (200 pong). + resp1 := whibPost(t, app, body, "ping", deliveryID) + assert.Equal(t, http.StatusOK, resp1.StatusCode) + var out1 map[string]interface{} + whibDecodeJSON(t, resp1, &out1) + _, isDuplicate1 := out1["duplicate"] + assert.False(t, isDuplicate1, "first delivery must not be marked duplicate") + + // Second delivery with the same X-GitHub-Delivery → replay. + resp2 := whibPost(t, app, body, "ping", deliveryID) + assert.Equal(t, http.StatusOK, resp2.StatusCode) + var out2 map[string]interface{} + whibDecodeJSON(t, resp2, &out2) + isDup2, _ := out2["duplicate"].(bool) + assert.True(t, isDup2, "second delivery with same id must be marked duplicate:true") +} + +// ── push: connection belongs to a different installation → matched==0 ───────── + +func TestGitHubAppWebhook_Push_InstallationMismatch_NoConnection(t *testing.T) { + daDeployNeedsDB(t) + db, cleanDB := testhelpers.SetupTestDB(t) + defer cleanDB() + rdb, cleanRedis := testhelpers.SetupTestRedis(t) + defer cleanRedis() + + ctx := context.Background() + teamA := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + teamB := uuid.MustParse(testhelpers.MustCreateTeamDB(t, db, "pro")) + + // Installation A — the one that owns the connection. + const installA int64 = 300301 + _, err := models.UpsertGitHubInstallation(ctx, db, installA, teamA, "acme-org") + require.NoError(t, err) + + // Installation B — the one that will send the push (different installation). + const installB int64 = 300302 + _, err = models.UpsertGitHubInstallation(ctx, db, installB, teamB, "other-org") + require.NoError(t, err) + + // Deploy + connection owned by installA/teamA. + deployUUID := uuid.New() + _, err = db.Exec(` + INSERT INTO deployments (id, team_id, app_id, port, tier, status) + VALUES ($1, $2, $3, 8080, 'pro', 'healthy')`, + deployUUID, teamA, "ghwh-mm-"+deployUUID.String()[:8]) + require.NoError(t, err) + + instA64 := installA + _, cerr := models.CreateGitHubConnection(ctx, db, models.CreateGitHubConnectionParams{ + AppID: deployUUID, + TeamID: teamA, + GitHubRepo: "owner/mismatch-repo", + Branch: "main", + WebhookSecret: "placeholder", + InstallationID: &instA64, + }) + require.NoError(t, cerr) + + app, clean := testhelpers.NewTestAppWithServices(t, db, rdb, "deploy", whibEnableWebhook) + defer clean() + + // Push comes from installB — the connection's InstallationID (installA) won't match. + body := pushEventBody("owner/mismatch-repo", "main", "1234567890abcdef1234567890abcdef12345678", installB) + resp := whibPost(t, app, body, "push", "push-mismatch-001") + + assert.Equal(t, http.StatusOK, resp.StatusCode) + var out map[string]interface{} + whibDecodeJSON(t, resp, &out) + assert.Equal(t, "no_connection", out["reason"]) +} diff --git a/internal/handlers/github_app_webhook_whitebox_test.go b/internal/handlers/github_app_webhook_whitebox_test.go new file mode 100644 index 00000000..24a6509f --- /dev/null +++ b/internal/handlers/github_app_webhook_whitebox_test.go @@ -0,0 +1,349 @@ +package handlers + +// github_app_webhook_whitebox_test.go — white-box (package handlers) coverage +// for GitHubAppWebhookHandler.Receive and the two sub-handlers. These tests +// run without a DB; every branch that touches h.db is either excluded here +// (handled in the integration file) or triggered before any DB call. +// +// Branches covered: +// - flag OFF → 501 +// - body > 25 MiB → 413 +// - bad/missing signature → 401 +// - ping → 200 pong +// - unknown event → 200 ignored +// - installation invalid JSON → 400 (no DB touched) +// - push invalid JSON → 400 (no DB touched) +// - push non-branch ref (refs/tags/v1) → 200 ignored no_deployable_ref +// - push branch-delete (after == 0*40) → 200 ignored no_deployable_ref + +import ( + "bytes" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/gofiber/fiber/v2" + + "instant.dev/internal/config" +) + +// whSignBody computes a valid X-Hub-Signature-256 header value for body+secret. +func whSignBody(secret string, body []byte) string { + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + return "sha256=" + hex.EncodeToString(mac.Sum(nil)) +} + +// whNewHandler builds a GitHubAppWebhookHandler with the given flag + secret; +// db, rdb, and planRegistry are all nil (no DB-touching path is exercised). +func whNewHandler(enabled bool, secret string) *GitHubAppWebhookHandler { + return NewGitHubAppWebhookHandler(nil, nil, &config.Config{ + GitHubAppEnabled: enabled, + GitHubAppWebhookSecret: secret, + }, nil) +} + +// whApp builds a bare fiber.App with no special ErrorHandler (the webhook +// handler uses plain c.SendStatus / c.JSON, not respondError, so the default +// ErrorHandler is fine). +func whApp(h *GitHubAppWebhookHandler) *fiber.App { + app := fiber.New() + app.Post("/wh", h.Receive) + return app +} + +// whPost fires a POST /wh with the given body, event header, and signature. +func whPost(t *testing.T, app *fiber.App, body []byte, event, sig, delivery string) *http.Response { + t.Helper() + req := httptest.NewRequest(http.MethodPost, "/wh", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + if event != "" { + req.Header.Set("X-GitHub-Event", event) + } + if sig != "" { + req.Header.Set("X-Hub-Signature-256", sig) + } + if delivery != "" { + req.Header.Set("X-GitHub-Delivery", delivery) + } + resp, err := app.Test(req, 5000) + if err != nil { + t.Fatalf("whPost: app.Test: %v", err) + } + return resp +} + +// ── flag OFF → 501 ──────────────────────────────────────────────────────────── + +func TestGitHubAppWebhook_FlagOff_501(t *testing.T) { + h := whNewHandler(false, "secret") + app := whApp(h) + + resp := whPost(t, app, []byte(`{}`), "ping", "sha256=irrelevant", "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusNotImplemented { + t.Errorf("flag OFF must return 501, got %d", resp.StatusCode) + } +} + +// ── body > 25 MiB → 413 ────────────────────────────────────────────────────── + +func TestGitHubAppWebhook_BodyTooLarge_413(t *testing.T) { + const secret = "whsec_test" + h := whNewHandler(true, secret) + + // Override Fiber's global BodyLimit so the giant body actually reaches the + // handler (not Fiber's middleware layer). The handler's own check fires first. + app := fiber.New(fiber.Config{BodyLimit: 50 * 1024 * 1024}) + app.Post("/wh", h.Receive) + + oversized := bytes.Repeat([]byte("x"), (25<<20)+1) + sig := whSignBody(secret, oversized) + req := httptest.NewRequest(http.MethodPost, "/wh", bytes.NewReader(oversized)) + req.Header.Set("Content-Type", "application/octet-stream") + req.Header.Set("X-GitHub-Event", "push") + req.Header.Set("X-Hub-Signature-256", sig) + + resp, err := app.Test(req, 10000) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusRequestEntityTooLarge { + t.Errorf("body > 25 MiB must return 413, got %d", resp.StatusCode) + } +} + +// ── missing/wrong signature → 401 ──────────────────────────────────────────── + +func TestGitHubAppWebhook_MissingSignature_401(t *testing.T) { + h := whNewHandler(true, "whsec_test") + app := whApp(h) + + body := []byte(`{"action":"ping"}`) + // Send no X-Hub-Signature-256 header at all. + req := httptest.NewRequest(http.MethodPost, "/wh", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-GitHub-Event", "ping") + + resp, err := app.Test(req, 5000) + if err != nil { + t.Fatalf("app.Test: %v", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("missing signature must return 401, got %d", resp.StatusCode) + } +} + +func TestGitHubAppWebhook_WrongSignature_401(t *testing.T) { + h := whNewHandler(true, "correct_secret") + app := whApp(h) + + body := []byte(`{"action":"ping"}`) + wrongSig := whSignBody("wrong_secret", body) + + resp := whPost(t, app, body, "ping", wrongSig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusUnauthorized { + t.Errorf("wrong signature must return 401, got %d", resp.StatusCode) + } +} + +// ── ping → 200 pong ────────────────────────────────────────────────────────── + +func TestGitHubAppWebhook_Ping_200(t *testing.T) { + const secret = "whsec_ping" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{"zen":"Keep it logically awesome.","hook_id":42}`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "ping", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("ping must return 200, got %d", resp.StatusCode) + } + var out map[string]interface{} + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + t.Fatalf("decode ping response: %v", err) + } + if ok, _ := out["ok"].(bool); !ok { + t.Error("ping response must carry ok:true") + } + if pong, _ := out["pong"].(bool); !pong { + t.Error("ping response must carry pong:true") + } +} + +// ── unknown event → 200 ignored ────────────────────────────────────────────── + +func TestGitHubAppWebhook_UnknownEvent_200Ignored(t *testing.T) { + const secret = "whsec_unknown" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{"action":"opened"}`) + sig := whSignBody(secret, body) + + // Use an event NOT in knownWebhookEvents so the metric label canonicalizes + // to "other" (covers canonicalizeWebhookEvent's fallback + the default arm). + resp := whPost(t, app, body, "workflow_run", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("unknown event must return 200, got %d", resp.StatusCode) + } + var out map[string]interface{} + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + t.Fatalf("decode unknown-event response: %v", err) + } + if ignored, _ := out["ignored"].(bool); !ignored { + t.Error("unknown event response must carry ignored:true") + } +} + +// ── installation with invalid JSON → 400 ───────────────────────────────────── +// ParseInstallationEvent fails; the handler returns 400 without touching h.db. + +func TestGitHubAppWebhook_Installation_BadJSON_400(t *testing.T) { + const secret = "whsec_instbad" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`not-json`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "installation", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("installation bad JSON must return 400, got %d", resp.StatusCode) + } +} + +// ── installation action "created" with valid JSON ───────────────────────────── +// action "created" is a no-op in handleInstallation (falls through the switch +// without touching h.db), so it returns 200 without needing a DB. + +func TestGitHubAppWebhook_Installation_Created_200(t *testing.T) { + const secret = "whsec_created" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{"action":"created","installation":{"id":99,"account":{"login":"acme"}}}`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "installation", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("installation created must return 200, got %d", resp.StatusCode) + } +} + +// ── push with invalid JSON → 400 ───────────────────────────────────────────── + +func TestGitHubAppWebhook_Push_BadJSON_400(t *testing.T) { + const secret = "whsec_pushbad" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{broken`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "push", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("push bad JSON must return 400, got %d", resp.StatusCode) + } +} + +// ── push with a tag ref (non-branch) → 200 ignored no_deployable_ref ───────── + +func TestGitHubAppWebhook_Push_TagRef_Ignored(t *testing.T) { + const secret = "whsec_tagref" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{"ref":"refs/tags/v1.2.3","after":"abc123deadbeef0000000000000000000000000a","repository":{"full_name":"owner/repo"},"installation":{"id":1}}`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "push", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("push tag ref must return 200, got %d", resp.StatusCode) + } + var out map[string]interface{} + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + t.Fatalf("decode: %v", err) + } + if reason, _ := out["reason"].(string); reason != "no_deployable_ref" { + t.Errorf("expected reason=no_deployable_ref, got %q", reason) + } +} + +// ── push branch-delete (after == 0*40) → 200 ignored no_deployable_ref ─────── + +func TestGitHubAppWebhook_Push_BranchDelete_Ignored(t *testing.T) { + const secret = "whsec_branchdel" + h := whNewHandler(true, secret) + app := whApp(h) + + zeroSHA := strings.Repeat("0", 40) + bodyStr := `{"ref":"refs/heads/main","after":"` + zeroSHA + `","repository":{"full_name":"owner/repo"},"installation":{"id":1}}` + body := []byte(bodyStr) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "push", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("branch-delete push must return 200, got %d", resp.StatusCode) + } + var out map[string]interface{} + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + t.Fatalf("decode: %v", err) + } + if reason, _ := out["reason"].(string); reason != "no_deployable_ref" { + t.Errorf("expected reason=no_deployable_ref, got %q", reason) + } +} + +// ── push with empty HeadCommitSHA → 200 ignored no_deployable_ref ───────────── + +func TestGitHubAppWebhook_Push_EmptySHA_Ignored(t *testing.T) { + const secret = "whsec_emptysha" + h := whNewHandler(true, secret) + app := whApp(h) + + body := []byte(`{"ref":"refs/heads/main","after":"","repository":{"full_name":"owner/repo"},"installation":{"id":1}}`) + sig := whSignBody(secret, body) + + resp := whPost(t, app, body, "push", sig, "") + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Errorf("push with empty SHA must return 200, got %d", resp.StatusCode) + } + var out map[string]interface{} + if err := json.NewDecoder(resp.Body).Decode(&out); err != nil { + t.Fatalf("decode: %v", err) + } + if reason, _ := out["reason"].(string); reason != "no_deployable_ref" { + t.Errorf("expected reason=no_deployable_ref, got %q", reason) + } +} diff --git a/internal/handlers/helpers.go b/internal/handlers/helpers.go index 19b03326..40aae9f5 100644 --- a/internal/handlers/helpers.go +++ b/internal/handlers/helpers.go @@ -513,6 +513,9 @@ var codeToAgentAction = map[string]errorCodeMeta{ "install_persist_failed": { AgentAction: "Tell the user the GitHub installation could not be saved due to a transient backend error. Retry the install shortly — see https://instanode.dev/status.", }, + "install_conflict": { + AgentAction: "Tell the user this GitHub installation is already linked to a different InstaNode team. Uninstall the InstaNode app from GitHub and re-install under the intended team, or contact support to move it — see https://instanode.dev/docs/deploy.", + }, "state_unavailable": { AgentAction: "Tell the user the GitHub install could not be started due to a transient backend error. Retry shortly — see https://instanode.dev/status.", }, diff --git a/internal/handlers/openapi_test.go b/internal/handlers/openapi_test.go index 5111c955..2d3ddab0 100644 --- a/internal/handlers/openapi_test.go +++ b/internal/handlers/openapi_test.go @@ -668,8 +668,8 @@ func TestOpenAPI_CoversAllRegisteredRoutes(t *testing.T) { // webhook URL — returns 405 + Allow: POST so a dashboard sees // "URL exists, method wrong" rather than silently abandoning a // 401. Not an agent-facing surface; stays out of OpenAPI. - "GET /api/v1/email/webhook/brevo": true, - "GET /api/v1/email/webhook/ses": true, + "GET /api/v1/email/webhook/brevo": true, + "GET /api/v1/email/webhook/ses": true, "POST /internal/teams/{id}/terminate": true, // Worker-only resend driver. Auth is the shared // WORKER_INTERNAL_JWT_SECRET HS256 token; agents must never call @@ -709,6 +709,11 @@ func TestOpenAPI_CoversAllRegisteredRoutes(t *testing.T) { // in P4.3 alongside the flag flip. See SCOPE-P4-github-app + github_app.go. "GET /integrations/github/install": true, "GET /integrations/github/callback": true, + // App-level GitHub webhook (P4.2) — GitHub-facing (HMAC-authed), not an + // agent-facing API; flag-gated OFF until P4.3. Documenting it in the + // agent OpenAPI would mislead agents (same rationale as the manual + // /webhooks/github/{webhook_id} receiver, which is also not in the spec). + "POST /webhooks/github": true, } var missing []string diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 6f1b336b..0e352837 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -308,6 +308,23 @@ var ( Help: "Inbound Brevo transactional-webhook events by normalized class (delivered/bounced_hard/bounced_soft/rejected/complaint/deferred/unsubscribed/error/unhandled/missing_message_id/unauthorized/invalid_payload/oversized)", }, []string{"event"}) + // GitHubWebhookReceivedTotal counts inbound GitHub App webhook deliveries to + // POST /webhooks/github by event type and outcome. (P4.2 — push-to-deploy.) + // event = "push" | "installation" | "ping" | other X-GitHub-Event value + // result = "ok" | "bad_signature" | "replay" | "no_match" | "disabled" | "error" + // Lazy *Vec — no series at /metrics until the first delivery of each label set. + GitHubWebhookReceivedTotal = promauto.NewCounterVec(prometheus.CounterOpts{ + Name: "instant_github_webhook_received_total", + Help: "Inbound GitHub App webhook deliveries by event and result", + }, []string{"event", "result"}) + + // GitHubPushDeployTotal counts push→auto-redeploy outcomes (P4.2). + // result = "enqueued" | "rate_limited" | "no_connection" | "error" + GitHubPushDeployTotal = promauto.NewCounterVec(prometheus.CounterOpts{ + Name: "instant_github_pushdeploy_total", + Help: "GitHub App push-to-deploy outcomes by result", + }, []string{"result"}) + // WebhookAuthFailuresTotal counts inbound webhook auth failures across // every email-provider webhook surface (Brevo HMAC + SES/SNS RSA). // Distinguishes: diff --git a/internal/models/github_installation.go b/internal/models/github_installation.go index bb3dd0c3..daff13f9 100644 --- a/internal/models/github_installation.go +++ b/internal/models/github_installation.go @@ -34,6 +34,17 @@ func (e *ErrGitHubInstallationNotFound) Error() string { return fmt.Sprintf("github installation not found: %d", e.InstallationID) } +// ErrGitHubInstallationTeamConflict is returned when an upsert would rebind an +// installation that already belongs to a DIFFERENT team — the install-callback +// hijack guard (review HIGH-2). A re-install by the same team is allowed. +type ErrGitHubInstallationTeamConflict struct { + InstallationID int64 +} + +func (e *ErrGitHubInstallationTeamConflict) Error() string { + return fmt.Sprintf("github installation %d is already linked to another team", e.InstallationID) +} + const githubInstallationColumns = `installation_id, team_id, account_login, suspended_at, created_at, updated_at` @@ -54,17 +65,25 @@ func scanGitHubInstallation(row interface { // installation_id is GitHub's primary key; a re-install (or a callback replay) // updates team_id/account_login and clears any prior suspension. func UpsertGitHubInstallation(ctx context.Context, db *sql.DB, installationID int64, teamID uuid.UUID, accountLogin string) (*GitHubInstallation, error) { + // The DO UPDATE is guarded by `WHERE team_id = EXCLUDED.team_id` so a + // re-install by the SAME team refreshes the row, but a request carrying a + // different team cannot rebind an installation it doesn't own — the update + // matches no row, RETURNING is empty, and we surface a team conflict instead + // of silently hijacking the victim's installation (review HIGH-2). row := db.QueryRowContext(ctx, ` INSERT INTO github_installations (installation_id, team_id, account_login) VALUES ($1, $2, $3) ON CONFLICT (installation_id) DO UPDATE - SET team_id = EXCLUDED.team_id, - account_login = EXCLUDED.account_login, + SET account_login = EXCLUDED.account_login, suspended_at = NULL, updated_at = now() + WHERE github_installations.team_id = EXCLUDED.team_id RETURNING `+githubInstallationColumns, installationID, teamID, accountLogin) i, err := scanGitHubInstallation(row) + if err == sql.ErrNoRows { + return nil, &ErrGitHubInstallationTeamConflict{InstallationID: installationID} + } if err != nil { return nil, fmt.Errorf("models.UpsertGitHubInstallation: %w", err) } diff --git a/internal/models/github_installation_test.go b/internal/models/github_installation_test.go index 00f0fb98..8e43a4f9 100644 --- a/internal/models/github_installation_test.go +++ b/internal/models/github_installation_test.go @@ -30,6 +30,19 @@ func TestUpsertGitHubInstallation(t *testing.T) { mock2.ExpectQuery(`INSERT INTO github_installations`).WillReturnError(errors.New("boom")) _, err = UpsertGitHubInstallation(ctx, db2, 42, uuid.New(), "acme") require.ErrorContains(t, err, "boom") + + // team conflict: the WHERE-guarded DO UPDATE matches no row (installation + // already owned by another team), RETURNING is empty → ErrNoRows → + // ErrGitHubInstallationTeamConflict (review HIGH-2, anti-hijack). + db3, mock3 := newMock(t) + mock3.ExpectQuery(`INSERT INTO github_installations`). + WillReturnRows(sqlmock.NewRows([]string{ + "installation_id", "team_id", "account_login", "suspended_at", "created_at", "updated_at", + })) // no rows → ErrNoRows + _, err = UpsertGitHubInstallation(ctx, db3, 42, uuid.New(), "acme") + var conflict *ErrGitHubInstallationTeamConflict + require.ErrorAs(t, err, &conflict) + require.Contains(t, conflict.Error(), "another team") } func TestGetGitHubInstallation(t *testing.T) { diff --git a/internal/models/github_push_match.go b/internal/models/github_push_match.go new file mode 100644 index 00000000..1ecf85ac --- /dev/null +++ b/internal/models/github_push_match.go @@ -0,0 +1,74 @@ +package models + +// github_push_match.go — query helpers for the GitHub push-to-deploy webhook +// (P4.2). A single push event carries a repo + branch; we need to find all +// AppGitHubConnections that are wired to that repo+branch so each linked +// deployment can be redeployed. We also expose a lookup by installation_id so +// the installation.deleted / installation.suspended events can locate every +// affected connection in one query. + +import ( + "context" + "database/sql" + "fmt" +) + +// FindConnectionsByRepoBranch returns every AppGitHubConnection whose +// github_repo and branch match the supplied values. A single repo+branch pair +// can be wired to multiple deployments (e.g. a monorepo with two apps), so the +// result is always a slice. An empty slice (not an error) is returned when +// there are no matching rows. +func FindConnectionsByRepoBranch(ctx context.Context, db *sql.DB, githubRepo, branch string) ([]*AppGitHubConnection, error) { + rows, err := db.QueryContext(ctx, + `SELECT `+githubConnectionColumns+` + FROM app_github_connections + WHERE github_repo = $1 + AND branch = $2`, + githubRepo, branch) + if err != nil { + return nil, fmt.Errorf("models.FindConnectionsByRepoBranch: %w", err) + } + defer func() { _ = rows.Close() }() + + var out []*AppGitHubConnection + for rows.Next() { + c, err := scanGitHubConnection(rows) + if err != nil { + return nil, fmt.Errorf("models.FindConnectionsByRepoBranch scan: %w", err) + } + out = append(out, c) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("models.FindConnectionsByRepoBranch rows: %w", err) + } + return out, nil +} + +// FindConnectionsByInstallationID returns every AppGitHubConnection whose +// installation_id matches the supplied value. Used when a GitHub App +// installation is deleted or suspended to locate all affected connections. +// An empty slice (not an error) is returned when there are no matching rows. +func FindConnectionsByInstallationID(ctx context.Context, db *sql.DB, installationID int64) ([]*AppGitHubConnection, error) { + rows, err := db.QueryContext(ctx, + `SELECT `+githubConnectionColumns+` + FROM app_github_connections + WHERE installation_id = $1`, + installationID) + if err != nil { + return nil, fmt.Errorf("models.FindConnectionsByInstallationID: %w", err) + } + defer func() { _ = rows.Close() }() + + var out []*AppGitHubConnection + for rows.Next() { + c, err := scanGitHubConnection(rows) + if err != nil { + return nil, fmt.Errorf("models.FindConnectionsByInstallationID scan: %w", err) + } + out = append(out, c) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("models.FindConnectionsByInstallationID rows: %w", err) + } + return out, nil +} diff --git a/internal/models/github_push_match_test.go b/internal/models/github_push_match_test.go new file mode 100644 index 00000000..62582b43 --- /dev/null +++ b/internal/models/github_push_match_test.go @@ -0,0 +1,158 @@ +package models + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/DATA-DOG/go-sqlmock" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// ghConnRow returns a one-row Rows fixture with all columns that +// scanGitHubConnection expects (mirrors githubConnectionColumns order): +// +// id, app_id, team_id, github_repo, branch, +// webhook_secret, installation_id, created_at, last_deploy_at, last_commit_sha +func ghConnRow() *sqlmock.Rows { + return sqlmock.NewRows([]string{ + "id", "app_id", "team_id", "github_repo", "branch", + "webhook_secret", "installation_id", "created_at", "last_deploy_at", "last_commit_sha", + }).AddRow( + uuid.New(), uuid.New(), uuid.New(), + "acme/api", "main", + "secret-cipher", int64(99), time.Now(), nil, nil, + ) +} + +// ── FindConnectionsByRepoBranch ─────────────────────────────────────────────── + +func TestFindConnectionsByRepoBranch(t *testing.T) { + ctx := context.Background() + + // happy path — two matching rows + t.Run("two rows", func(t *testing.T) { + db, mock := newMock(t) + rows := sqlmock.NewRows([]string{ + "id", "app_id", "team_id", "github_repo", "branch", + "webhook_secret", "installation_id", "created_at", "last_deploy_at", "last_commit_sha", + }). + AddRow(uuid.New(), uuid.New(), uuid.New(), "acme/api", "main", "s1", int64(1), time.Now(), nil, nil). + AddRow(uuid.New(), uuid.New(), uuid.New(), "acme/api", "main", "s2", int64(2), time.Now(), nil, nil) + mock.ExpectQuery(`FROM app_github_connections`). + WithArgs("acme/api", "main"). + WillReturnRows(rows) + got, err := FindConnectionsByRepoBranch(ctx, db, "acme/api", "main") + require.NoError(t, err) + require.Len(t, got, 2) + require.Equal(t, "acme/api", got[0].GitHubRepo) + require.Equal(t, "acme/api", got[1].GitHubRepo) + }) + + // happy path — no matching rows → empty slice, not error + t.Run("no rows", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WithArgs("ghost/repo", "main"). + WillReturnRows(sqlmock.NewRows([]string{ + "id", "app_id", "team_id", "github_repo", "branch", + "webhook_secret", "installation_id", "created_at", "last_deploy_at", "last_commit_sha", + })) + got, err := FindConnectionsByRepoBranch(ctx, db, "ghost/repo", "main") + require.NoError(t, err) + require.Empty(t, got) + }) + + // query error + t.Run("query error", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnError(errors.New("boom")) + _, err := FindConnectionsByRepoBranch(ctx, db, "acme/api", "main") + require.ErrorContains(t, err, "boom") + require.ErrorContains(t, err, "FindConnectionsByRepoBranch") + }) + + // scan error (wrong column count) + t.Run("scan error", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(uuid.New())) + _, err := FindConnectionsByRepoBranch(ctx, db, "acme/api", "main") + require.ErrorContains(t, err, "scan") + }) + + // rows.Err() + t.Run("rows err", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnRows(ghConnRow().RowError(0, errors.New("rowboom"))) + _, err := FindConnectionsByRepoBranch(ctx, db, "acme/api", "main") + require.ErrorContains(t, err, "rowboom") + require.ErrorContains(t, err, "rows") + }) +} + +// ── FindConnectionsByInstallationID ────────────────────────────────────────── + +func TestFindConnectionsByInstallationID(t *testing.T) { + ctx := context.Background() + + // happy path — one row + t.Run("one row", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WithArgs(int64(99)). + WillReturnRows(ghConnRow()) + got, err := FindConnectionsByInstallationID(ctx, db, 99) + require.NoError(t, err) + require.Len(t, got, 1) + require.True(t, got[0].InstallationID.Valid) + require.Equal(t, int64(99), got[0].InstallationID.Int64) + }) + + // happy path — no rows → empty slice + t.Run("no rows", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WithArgs(int64(404)). + WillReturnRows(sqlmock.NewRows([]string{ + "id", "app_id", "team_id", "github_repo", "branch", + "webhook_secret", "installation_id", "created_at", "last_deploy_at", "last_commit_sha", + })) + got, err := FindConnectionsByInstallationID(ctx, db, 404) + require.NoError(t, err) + require.Empty(t, got) + }) + + // query error + t.Run("query error", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnError(errors.New("boom")) + _, err := FindConnectionsByInstallationID(ctx, db, 99) + require.ErrorContains(t, err, "boom") + require.ErrorContains(t, err, "FindConnectionsByInstallationID") + }) + + // scan error (wrong column count) + t.Run("scan error", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(uuid.New())) + _, err := FindConnectionsByInstallationID(ctx, db, 99) + require.ErrorContains(t, err, "scan") + }) + + // rows.Err() + t.Run("rows err", func(t *testing.T) { + db, mock := newMock(t) + mock.ExpectQuery(`FROM app_github_connections`). + WillReturnRows(ghConnRow().RowError(0, errors.New("rowboom"))) + _, err := FindConnectionsByInstallationID(ctx, db, 99) + require.ErrorContains(t, err, "rowboom") + require.ErrorContains(t, err, "rows") + }) +} diff --git a/internal/router/router.go b/internal/router/router.go index 39c7c340..ffe98f9f 100644 --- a/internal/router/router.go +++ b/internal/router/router.go @@ -694,6 +694,12 @@ func NewWithHooks(cfg *config.Config, db *sql.DB, rdb *redis.Client, geoDbs *mid app.Get("/integrations/github/install", middleware.RequireAuth(cfg), githubAppH.Install) app.Get("/integrations/github/callback", githubAppH.Callback) + // App-level GitHub webhook (P4.2) — one endpoint for ALL installations, + // signed with the App webhook secret. No auth/RequireAuth: the HMAC is the + // boundary (mirrors the manual receiver above). + githubAppWebhookH := handlers.NewGitHubAppWebhookHandler(db, rdb, cfg, planRegistry) + app.Post("/webhooks/github", githubAppWebhookH.Receive) + // Deploy — Phase 6 (auth required on all endpoints). // POST /deploy/new is gated by RequireEnvAccess(ActionDeploy) — the // env scope arrives as a multipart form field (not JSON or query), so diff --git a/internal/testhelpers/testhelpers.go b/internal/testhelpers/testhelpers.go index 689e15c1..d2d0275c 100644 --- a/internal/testhelpers/testhelpers.go +++ b/internal/testhelpers/testhelpers.go @@ -1232,6 +1232,8 @@ func NewTestAppWithServices(t *testing.T, db *sql.DB, rdb *redis.Client, service githubAppH := handlers.NewGitHubAppHandler(db, cfg, planReg) app.Get("/integrations/github/install", middleware.RequireAuth(cfg), githubAppH.Install) app.Get("/integrations/github/callback", githubAppH.Callback) + githubAppWebhookH := handlers.NewGitHubAppWebhookHandler(db, rdb, cfg, planReg) + app.Post("/webhooks/github", githubAppWebhookH.Receive) // A/B-experiment conversion sink — wired into the test app so // handler tests can exercise the full route stack (router +