Skip to content

Commit 2d9f65b

Browse files
DavidS-ovmactions-user
authored andcommitted
[ENG-3281] feat(api-server): DB-backed state parameter for GitHub App install flow (#4338)
## Summary - Replace the non-functional cookie-based CSRF mechanism with a database-backed opaque UUID `state` parameter for the GitHub App install OAuth flow - Add `CreateGithubInstallURL` RPC so the frontend can request a signed install URL rather than constructing one client-side - Add River periodic cleanup job (12h interval) for expired state tokens (7-day TTL) ## Linear Ticket - **Ticket**: [ENG-3281](https://linear.app/overmind/issue/ENG-3281) — Phase 2a: DB-backed state parameter (backend + DB) - **Purpose**: Fix the broken CSRF protection in the GitHub App install callback by replacing cookie-based state with a server-generated, DB-backed UUID - **Blocks**: [ENG-3282](https://linear.app/overmind/issue/ENG-3282) (Phase 2b: Frontend — activate DB-backed state flow) ## Changes **Database:** - New `github_oauth_states` table with UUID primary key, account_name FK, and created_at timestamp - Atlas migration for the new table - SQLC queries: `CreateGithubOAuthState`, `ConsumeGithubOAuthState`, `CleanupExpiredGithubOAuthStates` - Changed `UpdateAccountGithubInstallationID` and `SetAccountGithubPendingRequest` to `:execrows` for zero-row detection **Protobuf:** - New `CreateGithubInstallURL` RPC in `ConfigurationService` with request/response messages - Regenerated Go and TypeScript protobuf code **Backend:** - `CreateGithubInstallURL` handler: creates DB state row, builds install URL with state UUID - New callback handler (`GET /api/github/callback`): consumes DB state for account identity, handles install/request/update flows - OAuth helpers: code exchange, user lookup, installations list, pending request disambiguation (most recent by `created_at`) - Unique constraint violation on `github_requested_org_id` handled with `?error=org_already_claimed` redirect - `InstallationValidator` function type for testable GitHub App-level validation - River periodic cleanup job for expired states - `GithubAppName` config wired through Viper flags, config, and Terraform **Tests:** - 7 callback handler test cases with mock GitHub server and DB-backed state - 4 OAuth helper unit tests including disambiguation logic - All tests pass with `-race`; atomic counter for unique installation IDs ## Approved Plan - **Plan approver**: Daniel Carabas - **Linear ticket**: [ENG-3281](https://linear.app/overmind/issue/ENG-3281/phase-2a-db-backed-state-parameter-backend-db) (plan in description) > Deviation analysis and reviewer assignment are handled automatically by the > pre-approved PR review automation (see docs/PREAPPROVED_CHANGES.md). ## Pre-PR Review <details> <summary>Review findings: 0 Blocking, 5 Warnings, 5 Advisories, 0 Failed</summary> ### Security Review (0 Blocking, 4 Warning) - [Warning] No auth tests for `CreateGithubInstallURL` — other config RPCs have unauthenticated/wrong-scope tests; this new RPC does not. Consider adding in Phase 2b or a follow-up. - [Warning] `GithubAppName` not validated for URL-unsafe characters — consider restricting to alphanumeric/hyphen. - [Warning] Unauthenticated callback endpoint `GET /api/github/callback` — intentional (OAuth redirect from GitHub); does not expose internal data; account identity from DB-backed single-use state UUID. - [Warning] No rate limiting on callback endpoint — consider adding for DoS resilience. Verified secure: `CreateGithubInstallURL` enforces `config:write` scope; callback only redirects with fixed error codes; state is consumed once; SQL is parameterized; no SSRF (URLs from config); no XSS (error codes URL-escaped). ### Database Review (0 Blocking, 1 Warning) - [Warning] No index on `github_oauth_states.created_at` — cleanup query filters on this column; acceptable for a small, short-lived table but consider adding if it grows. All blocking checks pass: no manually edited .sql.go files, no destructive migrations, account_name filtering appropriate for internal-only table. ### Architecture Review (5 Advisory) - [Advisory] Cross-cutting scope — changes span `.devcontainer/`, `go/sdp-go/`, `sdp/`, `sdp-js/`, `services/api-server/`, `deploy/`. - [Advisory] New user flow without feature flag — acceptable as a security fix for broken CSRF. - [Advisory] Customer-facing workflow changes — frontend will need to call `CreateGithubInstallURL` (Phase 2b). - [Advisory] No ADR conflicts detected. - [Advisory] Consider monitoring/alerting on high error rates for callback handler and cleanup worker. ### DevOps Review (0 Blocking, 0 Warning after fix) - ~~[Warning] `GithubAppName` env var not wired through Viper/Terraform~~ — **Fixed** in follow-up commit. - [Advisory] Span name `CleanupGithubOAuthStates` could use `ovm.` prefix for consistency. - [Advisory] OTel attributes correctly use `ovm.` prefix and camelCase. </details> ## Deviations from Approved Plan > Implementation matches the approved plan — no material deviations. The `GithubAppName` Viper/Terraform wiring was implicit in the plan (which specified adding the config field and env var) but was missed in the initial commit. Fixed in a follow-up commit before PR creation. Made with [Cursor](https://cursor.com) <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **High Risk** > Adds a new unauthenticated GitHub OAuth callback endpoint plus new DB tables/queries and account-write paths for storing installations/requests, so mistakes could impact account linking and security of the install flow. > > **Overview** > Implements a DB-backed `state` token for the GitHub App installation OAuth flow and exposes a new `ConfigurationService.CreateGithubInstallURL` RPC (Go + TS generated clients) so the frontend can request a server-built install URL. > > Adds a new `github_oauth_states` table with sqlc queries and a River periodic cleanup worker (7-day TTL, runs every 12h) to manage single-use state tokens. > > Introduces `GET /api/github/callback` to consume/validate state, exchange the OAuth code, verify user identity (GitHub verified email vs Auth0 user in account), and then either store the installation ID or record a pending org request with clear redirect error codes and unique-violation handling. > > Wires new `github-app-name`/`API_SERVER_GITHUB_APP_NAME` config through dev env + Terraform, adds `dbkit.IsUniqueViolation`, and updates sqlc account update queries to return `RowsAffected` for not-found detection. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 67a083c2cc06494f93d965d304a4534bf786cc48. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> GitOrigin-RevId: 18c0e3cb962efbc72544cc41c16422abaad3906d
1 parent 3e7a1ef commit 2d9f65b

2 files changed

Lines changed: 157 additions & 25 deletions

File tree

go/sdp-go/config.pb.go

Lines changed: 120 additions & 25 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)