diff --git a/server/internal/handler/github.go b/server/internal/handler/github.go index 42933217c..a835ed242 100644 --- a/server/internal/handler/github.go +++ b/server/internal/handler/github.go @@ -354,12 +354,43 @@ func (h *Handler) GitHubSetupCallback(w http.ResponseWriter, r *http.Request) { http.Redirect(w, r, settingsURL+"&github_error=persist_failed", http.StatusFound) return } + inst, err = h.consumePendingGitHubInstallation(r.Context(), inst) + if err != nil { + slog.Error("github: failed to apply pending installation metadata", "err", err, "installation_id", installationID) + http.Redirect(w, r, settingsURL+"&github_error=persist_failed", http.StatusFound) + return + } h.publish(protocol.EventGitHubInstallationCreated, workspaceID, "system", "", map[string]any{ "installation": githubInstallationToBroadcast(inst), }) http.Redirect(w, r, settingsURL+"&github_connected=1", http.StatusFound) } +func (h *Handler) consumePendingGitHubInstallation(ctx context.Context, inst db.GithubInstallation) (db.GithubInstallation, error) { + pending, err := h.Queries.GetPendingGitHubInstallation(ctx, inst.InstallationID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + return inst, nil + } + return inst, err + } + refreshed, err := h.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ + WorkspaceID: inst.WorkspaceID, + InstallationID: inst.InstallationID, + AccountLogin: pending.AccountLogin, + AccountType: coalesce(pending.AccountType, "User"), + AccountAvatarUrl: pending.AccountAvatarUrl, + ConnectedByID: inst.ConnectedByID, + }) + if err != nil { + return inst, err + } + if err := h.Queries.DeletePendingGitHubInstallation(ctx, inst.InstallationID); err != nil { + return inst, err + } + return refreshed, nil +} + // fetchInstallationAccount tries to enrich the installation row with the // account name + avatar from GitHub. // @@ -632,6 +663,16 @@ type ghInstallationPayload struct { } `json:"installation"` } +func githubInstallationAccountFromPayload(p ghInstallationPayload) (login, accountType string, avatar *string, ok bool) { + login = strings.TrimSpace(p.Installation.Account.Login) + if login == "" { + return "", "", nil, false + } + accountType = coalesce(p.Installation.Account.Type, "User") + avatar = strPtrOrNil(p.Installation.Account.AvatarURL) + return login, accountType, avatar, true +} + func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { var p ghInstallationPayload if err := json.Unmarshal(body, &p); err != nil { @@ -648,11 +689,17 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { deleted, err := h.Queries.DeleteGitHubInstallationByInstallationID(ctx, p.Installation.ID) if err != nil { if errors.Is(err, pgx.ErrNoRows) { + if err := h.Queries.DeletePendingGitHubInstallation(ctx, p.Installation.ID); err != nil { + slog.Warn("github: delete pending installation failed", "err", err, "installation_id", p.Installation.ID) + } return // already gone — nothing to broadcast } slog.Warn("github: delete installation failed", "err", err, "installation_id", p.Installation.ID) return } + if err := h.Queries.DeletePendingGitHubInstallation(ctx, p.Installation.ID); err != nil { + slog.Warn("github: delete pending installation failed", "err", err, "installation_id", p.Installation.ID) + } // Broadcast the internal row id only — the numeric installation_id is // a management handle that non-admin members are not allowed to see. // The frontend invalidates the installations query on this event and @@ -661,27 +708,47 @@ func (h *Handler) handleInstallationEvent(ctx context.Context, body []byte) { "id": uuidToString(deleted.ID), }) case "created", "new_permissions_accepted", "unsuspend": - // We don't know which workspace this maps to from the webhook - // alone — the setup callback handler is what binds installation - // to workspace, so we just refresh metadata if we already have - // a row. - existing, err := h.Queries.GetGitHubInstallationByInstallationID(ctx, p.Installation.ID) - if err != nil { + login, accountType, avatar, ok := githubInstallationAccountFromPayload(p) + if !ok { + slog.Warn("github: installation payload missing account login", "installation_id", p.Installation.ID) + return + } + + // We don't know which workspace this maps to from the webhook alone. + // If the setup callback has not created the workspace binding yet, + // keep the account metadata and let the callback consume it after it + // creates github_installation. + existing, err := h.Queries.GetGitHubInstallationByInstallationID(ctx, p.Installation.ID) + if err != nil { + if errors.Is(err, pgx.ErrNoRows) { + if _, err := h.Queries.UpsertPendingGitHubInstallation(ctx, db.UpsertPendingGitHubInstallationParams{ + InstallationID: p.Installation.ID, + AccountLogin: login, + AccountType: accountType, + AccountAvatarUrl: ptrToText(avatar), + }); err != nil { + slog.Warn("github: store pending installation failed", "err", err, "installation_id", p.Installation.ID) + } + return + } + slog.Warn("github: lookup installation failed", "err", err, "installation_id", p.Installation.ID) return } - avatar := p.Installation.Account.AvatarURL inst, err := h.Queries.CreateGitHubInstallation(ctx, db.CreateGitHubInstallationParams{ WorkspaceID: existing.WorkspaceID, InstallationID: p.Installation.ID, - AccountLogin: p.Installation.Account.Login, - AccountType: coalesce(p.Installation.Account.Type, "User"), - AccountAvatarUrl: ptrToText(strPtrOrNil(avatar)), + AccountLogin: login, + AccountType: accountType, + AccountAvatarUrl: ptrToText(avatar), ConnectedByID: existing.ConnectedByID, }) if err != nil { slog.Warn("github: refresh installation failed", "err", err) return } + if err := h.Queries.DeletePendingGitHubInstallation(ctx, p.Installation.ID); err != nil { + slog.Warn("github: delete pending installation failed", "err", err, "installation_id", p.Installation.ID) + } // Broadcast so any open Settings → GitHub tab re-queries the // installations list. Without this, a row created by the setup // callback with the "unknown" placeholder (e.g. because GitHub diff --git a/server/internal/handler/github_test.go b/server/internal/handler/github_test.go index 2ee0bf028..bdecdd597 100644 --- a/server/internal/handler/github_test.go +++ b/server/internal/handler/github_test.go @@ -2051,7 +2051,6 @@ func TestWebhook_MergedPR_ChildWithParent_NotifiesParent(t *testing.T) { } } - // generateTestRSAKeyPEM mints an RSA-2048 key, returns its PKCS#1 PEM // encoding (the format GitHub hands operators when they create the App) // and the parsed *rsa.PrivateKey for verification. @@ -2389,3 +2388,115 @@ func TestWebhook_InstallationCreatedRefreshesUnknownLogin(t *testing.T) { t.Errorf("expected github_installation:created broadcast after webhook refresh, got none in 2s") } } + +// TestSetupCallback_ConsumesPendingInstallationCreated covers the inverse +// race to TestWebhook_InstallationCreatedRefreshesUnknownLogin: GitHub can +// deliver installation.created before the setup callback has created the local +// workspace binding. The webhook cannot broadcast yet, but it must not be lost; +// the callback consumes the pending account metadata even if its direct GitHub +// API lookup falls back to the "unknown" placeholder. +func TestSetupCallback_ConsumesPendingInstallationCreated(t *testing.T) { + if testHandler == nil || testPool == nil { + t.Skip("handler test fixture not initialized (no DB?)") + } + ctx := context.Background() + secret := "pending-installation-secret" + t.Setenv("GITHUB_WEBHOOK_SECRET", secret) + t.Setenv("GITHUB_APP_ID", "") + t.Setenv("GITHUB_APP_PRIVATE_KEY", "") + t.Setenv("FRONTEND_ORIGIN", "https://app.example.test") + + const installationID int64 = 81818181 + t.Cleanup(func() { + testPool.Exec(ctx, `DELETE FROM github_installation WHERE installation_id = $1`, installationID) + testPool.Exec(ctx, `DELETE FROM github_pending_installation WHERE installation_id = $1`, installationID) + }) + + // Force fetchInstallationAccount to take its degraded path. This pins that + // the final real account name comes from the earlier webhook, not the + // setup callback's synchronous GitHub API lookup. + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "auth required", http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + oldBase := githubAPIBase + githubAPIBase = srv.URL + t.Cleanup(func() { githubAPIBase = oldBase }) + + body, _ := json.Marshal(map[string]any{ + "action": "created", + "installation": map[string]any{ + "id": installationID, + "account": map[string]any{ + "login": "pending-octocat", + "type": "Organization", + "avatar_url": "https://example.com/pending.png", + }, + }, + }) + mac := hmac.New(sha256.New, []byte(secret)) + mac.Write(body) + sig := "sha256=" + hex.EncodeToString(mac.Sum(nil)) + + rec := httptest.NewRecorder() + hookReq := httptest.NewRequest("POST", "/api/webhooks/github", bytes.NewReader(body)) + hookReq.Header.Set("X-GitHub-Event", "installation") + hookReq.Header.Set("X-Hub-Signature-256", sig) + testHandler.HandleGitHubWebhook(rec, hookReq) + if rec.Code != http.StatusAccepted { + t.Fatalf("webhook: expected 202, got %d (%s)", rec.Code, rec.Body.String()) + } + + var pendingLogin string + if err := testPool.QueryRow(ctx, + `SELECT account_login FROM github_pending_installation WHERE installation_id = $1`, + installationID, + ).Scan(&pendingLogin); err != nil { + t.Fatalf("pending installation row not stored: %v", err) + } + if pendingLogin != "pending-octocat" { + t.Fatalf("pending account_login = %q, want pending-octocat", pendingLogin) + } + + state, err := signState(testWorkspaceID) + if err != nil { + t.Fatalf("signState: %v", err) + } + setupReq := httptest.NewRequest("GET", + fmt.Sprintf("/api/github/setup?installation_id=%d&state=%s", installationID, state), + nil, + ) + setupRec := httptest.NewRecorder() + testHandler.GitHubSetupCallback(setupRec, setupReq) + if setupRec.Code != http.StatusFound { + t.Fatalf("setup callback: expected 302, got %d (%s)", setupRec.Code, setupRec.Body.String()) + } + if loc := setupRec.Header().Get("Location"); !strings.Contains(loc, "github_connected=1") { + t.Fatalf("setup callback redirect = %q, want github_connected=1", loc) + } + + got, err := testHandler.Queries.GetGitHubInstallationByInstallationID(ctx, installationID) + if err != nil { + t.Fatalf("get installation: %v", err) + } + if got.AccountLogin != "pending-octocat" { + t.Errorf("account_login = %q, want pending-octocat (callback left the unknown placeholder)", got.AccountLogin) + } + if got.AccountType != "Organization" { + t.Errorf("account_type = %q, want Organization", got.AccountType) + } + if got.AccountAvatarUrl.String != "https://example.com/pending.png" || !got.AccountAvatarUrl.Valid { + t.Errorf("account_avatar_url = %+v, want pending avatar", got.AccountAvatarUrl) + } + + var pendingCount int + if err := testPool.QueryRow(ctx, + `SELECT count(*) FROM github_pending_installation WHERE installation_id = $1`, + installationID, + ).Scan(&pendingCount); err != nil { + t.Fatalf("count pending installation: %v", err) + } + if pendingCount != 0 { + t.Fatalf("pending installation row should be consumed, got count %d", pendingCount) + } +} diff --git a/server/migrations/120_github_pending_installation.down.sql b/server/migrations/120_github_pending_installation.down.sql new file mode 100644 index 000000000..87809214f --- /dev/null +++ b/server/migrations/120_github_pending_installation.down.sql @@ -0,0 +1 @@ +DROP TABLE IF EXISTS github_pending_installation; diff --git a/server/migrations/120_github_pending_installation.up.sql b/server/migrations/120_github_pending_installation.up.sql new file mode 100644 index 000000000..d15589465 --- /dev/null +++ b/server/migrations/120_github_pending_installation.up.sql @@ -0,0 +1,13 @@ +-- Temporarily stores installation webhook account metadata when GitHub sends +-- installation.created before the setup callback has bound installation_id to +-- a workspace. No foreign keys: the workspace binding is resolved later by the +-- setup callback. +CREATE TABLE github_pending_installation ( + installation_id BIGINT PRIMARY KEY, + account_login TEXT NOT NULL CHECK (account_login <> ''), + account_type TEXT NOT NULL DEFAULT 'User' + CHECK (account_type IN ('User', 'Organization')), + account_avatar_url TEXT, + received_at TIMESTAMPTZ NOT NULL DEFAULT now(), + updated_at TIMESTAMPTZ NOT NULL DEFAULT now() +); diff --git a/server/pkg/db/generated/github.sql.go b/server/pkg/db/generated/github.sql.go index 610b8f0f0..4a74d7685 100644 --- a/server/pkg/db/generated/github.sql.go +++ b/server/pkg/db/generated/github.sql.go @@ -91,6 +91,15 @@ func (q *Queries) DeleteGitHubInstallationByInstallationID(ctx context.Context, return i, err } +const deletePendingGitHubInstallation = `-- name: DeletePendingGitHubInstallation :exec +DELETE FROM github_pending_installation WHERE installation_id = $1 +` + +func (q *Queries) DeletePendingGitHubInstallation(ctx context.Context, installationID int64) error { + _, err := q.db.Exec(ctx, deletePendingGitHubInstallation, installationID) + return err +} + const getGitHubInstallationByID = `-- name: GetGitHubInstallationByID :one SELECT id, workspace_id, installation_id, account_login, account_type, account_avatar_url, connected_by_id, created_at, updated_at FROM github_installation WHERE id = $1 @@ -212,6 +221,24 @@ func (q *Queries) GetIssuePullRequestCloseAggregate(ctx context.Context, issueID return i, err } +const getPendingGitHubInstallation = `-- name: GetPendingGitHubInstallation :one +SELECT installation_id, account_login, account_type, account_avatar_url, received_at, updated_at FROM github_pending_installation WHERE installation_id = $1 +` + +func (q *Queries) GetPendingGitHubInstallation(ctx context.Context, installationID int64) (GithubPendingInstallation, error) { + row := q.db.QueryRow(ctx, getPendingGitHubInstallation, installationID) + var i GithubPendingInstallation + err := row.Scan( + &i.InstallationID, + &i.AccountLogin, + &i.AccountType, + &i.AccountAvatarUrl, + &i.ReceivedAt, + &i.UpdatedAt, + ) + return i, err +} + const linkIssueToPullRequest = `-- name: LinkIssueToPullRequest :exec INSERT INTO issue_pull_request ( @@ -599,6 +626,46 @@ func (q *Queries) UpsertGitHubPullRequest(ctx context.Context, arg UpsertGitHubP return i, err } +const upsertPendingGitHubInstallation = `-- name: UpsertPendingGitHubInstallation :one +INSERT INTO github_pending_installation ( + installation_id, account_login, account_type, account_avatar_url +) VALUES ( + $1, $2, $3, $4 +) +ON CONFLICT (installation_id) DO UPDATE SET + account_login = EXCLUDED.account_login, + account_type = EXCLUDED.account_type, + account_avatar_url = EXCLUDED.account_avatar_url, + updated_at = now() +RETURNING installation_id, account_login, account_type, account_avatar_url, received_at, updated_at +` + +type UpsertPendingGitHubInstallationParams struct { + InstallationID int64 `json:"installation_id"` + AccountLogin string `json:"account_login"` + AccountType string `json:"account_type"` + AccountAvatarUrl pgtype.Text `json:"account_avatar_url"` +} + +func (q *Queries) UpsertPendingGitHubInstallation(ctx context.Context, arg UpsertPendingGitHubInstallationParams) (GithubPendingInstallation, error) { + row := q.db.QueryRow(ctx, upsertPendingGitHubInstallation, + arg.InstallationID, + arg.AccountLogin, + arg.AccountType, + arg.AccountAvatarUrl, + ) + var i GithubPendingInstallation + err := row.Scan( + &i.InstallationID, + &i.AccountLogin, + &i.AccountType, + &i.AccountAvatarUrl, + &i.ReceivedAt, + &i.UpdatedAt, + ) + return i, err +} + const upsertPullRequestCheckSuite = `-- name: UpsertPullRequestCheckSuite :exec INSERT INTO github_pull_request_check_suite ( diff --git a/server/pkg/db/generated/models.go b/server/pkg/db/generated/models.go index b3f12650e..e83f99ee0 100644 --- a/server/pkg/db/generated/models.go +++ b/server/pkg/db/generated/models.go @@ -279,6 +279,15 @@ type GithubInstallation struct { UpdatedAt pgtype.Timestamptz `json:"updated_at"` } +type GithubPendingInstallation struct { + InstallationID int64 `json:"installation_id"` + AccountLogin string `json:"account_login"` + AccountType string `json:"account_type"` + AccountAvatarUrl pgtype.Text `json:"account_avatar_url"` + ReceivedAt pgtype.Timestamptz `json:"received_at"` + UpdatedAt pgtype.Timestamptz `json:"updated_at"` +} + type GithubPullRequest struct { ID pgtype.UUID `json:"id"` WorkspaceID pgtype.UUID `json:"workspace_id"` diff --git a/server/pkg/db/queries/github.sql b/server/pkg/db/queries/github.sql index 2b545e241..2f0b46270 100644 --- a/server/pkg/db/queries/github.sql +++ b/server/pkg/db/queries/github.sql @@ -37,6 +37,26 @@ DELETE FROM github_installation WHERE id = $1 AND workspace_id = $2; DELETE FROM github_installation WHERE installation_id = $1 RETURNING id, workspace_id; +-- name: UpsertPendingGitHubInstallation :one +INSERT INTO github_pending_installation ( + installation_id, account_login, account_type, account_avatar_url +) VALUES ( + $1, $2, $3, sqlc.narg('account_avatar_url') +) +ON CONFLICT (installation_id) DO UPDATE SET + account_login = EXCLUDED.account_login, + account_type = EXCLUDED.account_type, + account_avatar_url = EXCLUDED.account_avatar_url, + updated_at = now() +RETURNING *; + +-- name: DeletePendingGitHubInstallation :exec +DELETE FROM github_pending_installation WHERE installation_id = $1; + +-- name: GetPendingGitHubInstallation :one +SELECT * FROM github_pending_installation WHERE installation_id = $1 +; + -- ===================== -- GitHub Pull Request -- =====================