diff --git a/server/src/oauth/__tests__/refresh.test.ts b/server/src/oauth/__tests__/refresh.test.ts index 8121bac7d61..87d1359666d 100644 --- a/server/src/oauth/__tests__/refresh.test.ts +++ b/server/src/oauth/__tests__/refresh.test.ts @@ -105,6 +105,7 @@ describe("refreshConnection", () => { expect(result.outcome).toBe("success"); if (result.outcome === "success") { expect(result.accessToken).toBe("NEW_ACCESS"); + expect(result.accessTokenSecretId).toBe("new-access"); } expect(upsertSecretByName).toHaveBeenCalledWith( "c1", diff --git a/server/src/oauth/refresh.ts b/server/src/oauth/refresh.ts index 550bf56e9c3..e24c5ad00ad 100644 --- a/server/src/oauth/refresh.ts +++ b/server/src/oauth/refresh.ts @@ -30,7 +30,7 @@ export interface RefreshDeps { } export type RefreshResult = - | { outcome: "success"; accessToken: string } + | { outcome: "success"; accessToken: string; accessTokenSecretId?: string } | { outcome: "revoked" } | { outcome: "transient"; error: string } | { outcome: "skipped"; reason: string }; @@ -273,6 +273,7 @@ export async function refreshConnection(deps: RefreshDeps): Promise ({ + exchangeToken: vi.fn().mockResolvedValue({}), + fetchAccountInfo: vi.fn().mockResolvedValue({}), +})); + +describe("OAuth callback cleanup", () => { + it("removes newly written token secrets when the connection transaction fails", async () => { + const stateRow = { + id: "state-1", + providerId: "github", + consumedAt: new Date(), + expiresAt: new Date(Date.now() + 60_000), + returnUrl: "/settings/connections", + companyId: "c1", + codeVerifier: "verifier", + redirectUri: "https://app.paperclip.test/api/oauth/callback/github", + scopesRequested: ["repo"], + }; + + const returning = vi.fn().mockResolvedValue([stateRow]); + const where = vi.fn().mockReturnValue({ returning }); + const set = vi.fn().mockReturnValue({ where }); + const update = vi.fn().mockReturnValue({ set }); + const tx = { + query: { + oauthConnections: { + findFirst: vi.fn().mockResolvedValue(null), + }, + }, + insert: vi.fn().mockReturnValue({ + values: vi.fn().mockRejectedValue(new Error("db_down")), + }), + }; + const db = { + update, + query: { + oauthAuthorizationStates: { + findFirst: vi.fn(), + }, + oauthConnections: { + findFirst: vi.fn().mockResolvedValue(null), + }, + }, + transaction: vi.fn(async (fn: (transaction: typeof tx) => Promise) => + fn(tx), + ), + }; + const remove = vi.fn().mockResolvedValue(undefined); + + const app = express(); + app.use( + "/api/oauth/callback/:providerId", + oauthCallbackRoute({ + db: db as unknown as never, + registry: { + get: () => ({ + config: { + id: "github", + endpoints: { + token: "https://github.example/token", + accountInfo: "https://github.example/me", + }, + authMethod: "post", + responseFormat: "json", + }, + clientId: "client", + clientSecret: "secret", + shape: { + parseTokenResponse: () => ({ + accessToken: "ACCESS", + refreshToken: "REFRESH", + expiresInSeconds: 3600, + scope: ["repo"], + }), + parseAccountInfo: () => ({ + accountId: "acct-1", + accountLabel: "octocat", + }), + }, + }), + } as unknown as never, + publicUrl: "https://app.paperclip.test", + secretService: { + upsertSecretByName: vi + .fn() + .mockResolvedValueOnce({ id: "access-secret" }) + .mockResolvedValueOnce({ id: "refresh-secret" }), + remove, + }, + }), + ); + app.use( + ( + _err: unknown, + _req: express.Request, + res: express.Response, + _next: express.NextFunction, + ) => { + res.status(500).json({ errorCode: "internal_error" }); + }, + ); + + const res = await request(app).get( + "/api/oauth/callback/github?state=state-1&code=code", + ); + + expect(res.status).toBe(500); + expect(remove).toHaveBeenCalledWith("access-secret"); + expect(remove).toHaveBeenCalledWith("refresh-secret"); + }); +}); diff --git a/server/src/routes/__tests__/oauth-connections.test.ts b/server/src/routes/__tests__/oauth-connections.test.ts index adf4e04392f..000c0150b25 100644 --- a/server/src/routes/__tests__/oauth-connections.test.ts +++ b/server/src/routes/__tests__/oauth-connections.test.ts @@ -16,14 +16,14 @@ const conn = { refreshTokenSecretId: null, accessTokenExpiresAt: new Date(Date.now() + 3_600_000), lastRefreshedAt: new Date(), - lastError: null, - lastErrorAt: null, + lastError: "invalid_grant: account user@example.com", + lastErrorAt: new Date(), refreshAttemptCount: 0, createdAt: new Date(), updatedAt: new Date(), }; -function makeApp(rows: Array) { +function makeApp(rows: Array, membershipRole = "admin") { const env = { GH_ID: "id", GH_SECRET: "s" }; const registry = new ProviderRegistry({ env }); registry.register( @@ -61,7 +61,7 @@ function makeApp(rows: Array) { (req as unknown as { actor: unknown }).actor = { type: "board", userId: "u1", - memberships: [{ companyId: req.params.companyId, membershipRole: "admin" }], + memberships: [{ companyId: req.params.companyId, membershipRole }], }; next(); }, @@ -85,6 +85,21 @@ describe("Connection management routes", () => { expect(JSON.stringify(res.body)).not.toContain("refreshTokenSecretId"); }); + it("GET /connections hides provider error diagnostics from non-admin members", async () => { + const res = await request(makeApp([conn], "member")).get( + "/api/companies/c1/oauth/connections", + ); + expect(res.status).toBe(200); + expect(res.body.connections[0]).not.toHaveProperty("lastError"); + expect(res.body.connections[0]).not.toHaveProperty("lastErrorAt"); + }); + + it("GET /connections includes provider error diagnostics for admins", async () => { + const res = await request(makeApp([conn])).get("/api/companies/c1/oauth/connections"); + expect(res.status).toBe(200); + expect(res.body.connections[0].lastError).toBe(conn.lastError); + }); + it("GET /connections/:id returns 404 for missing", async () => { const res = await request(makeApp([])).get("/api/companies/c1/oauth/connections/missing"); expect(res.status).toBe(404); diff --git a/server/src/routes/oauth-callback.ts b/server/src/routes/oauth-callback.ts index 335516061a6..ded0854c06e 100644 --- a/server/src/routes/oauth-callback.ts +++ b/server/src/routes/oauth-callback.ts @@ -208,6 +208,24 @@ export function oauthCallbackRoute(deps: OAuthCallbackDeps): RequestHandler { ); } + const existingBeforeSecretWrite = + await deps.db.query.oauthConnections.findFirst({ + where: and( + eq(oauthConnections.companyId, stateRow.companyId), + eq(oauthConnections.providerId, providerId), + ), + }); + if ( + existingBeforeSecretWrite && + existingBeforeSecretWrite.accountId && + parsedAccount.accountId !== existingBeforeSecretWrite.accountId + ) { + return res.redirect( + 302, + back(deps, stateRow.returnUrl, { oauth_error: "account_mismatch" }), + ); + } + // Persist secrets via upsert (deterministic name → idempotent reconnects). const accessName = secretName(providerId, parsedAccount.accountId, "access"); const accessSecret = await deps.secretService.upsertSecretByName( @@ -232,23 +250,38 @@ export function oauthCallbackRoute(deps: OAuthCallbackDeps): RequestHandler { : null; const finalScopes = parsedToken.scope ?? stateRow.scopesRequested ?? []; - // Track the secret IDs we just wrote so that, on ACCOUNT_MISMATCH, we - // can roll them back. Without this, a mismatched callback leaves - // orphaned encrypted access/refresh tokens that no connection row - // references — names matching `oauth:::access` / - // `:refresh`. The connection upsert runs in its own transaction below; - // those secret writes are NOT inside that transaction (they require - // their own DB writes for versions/encryption metadata via the - // secret-service factory bound to the outer db), so a transactional - // rollback alone cannot undo them. - // - // We use Option C from the design notes: connection writes stay in a - // single transaction; on rollback we explicitly `secretService.remove` - // any freshly-persisted secrets. Option B (passing `tx` through the - // secret service) is the cleaner long-term fix but requires a wider - // refactor of the secret-service factory. + // Track the secret IDs we just wrote. The connection upsert runs in its own + // transaction below, but these secret writes are outside that transaction, + // so a failed connection write cannot roll them back automatically. Only + // remove IDs that did not already back the pre-existing connection; on a + // reconnect, upsertSecretByName rotates the same secret ID in place and + // deleting it would break the still-current connection. const newSecretIds: string[] = [accessSecret.id]; if (refreshSecret) newSecretIds.push(refreshSecret.id); + const reusableSecretIds = new Set( + [ + existingBeforeSecretWrite?.accessTokenSecretId, + existingBeforeSecretWrite?.refreshTokenSecretId, + ].filter((id): id is string => typeof id === "string" && id.length > 0), + ); + + const rollbackNewSecrets = async (reason: string): Promise => { + if (typeof deps.secretService.remove !== "function") return; + for (const secretId of newSecretIds) { + if (reusableSecretIds.has(secretId)) continue; + await deps.secretService.remove(secretId).catch((removeErr) => { + oauthLogger.warn( + { + provider: providerId, + secretId, + reason, + err: { message: (removeErr as Error).message }, + }, + "failed to roll back orphan OAuth secret after callback failure", + ); + }); + } + }; try { await deps.db.transaction(async (tx: any) => { @@ -303,29 +336,13 @@ export function oauthCallbackRoute(deps: OAuthCallbackDeps): RequestHandler { }); } catch (err) { if ((err as Error).message === "ACCOUNT_MISMATCH") { - // Roll back the secret writes that landed before mismatch was - // detected. `remove` is idempotent and best-effort — swallow - // failures so a secrets cleanup hiccup doesn't override the - // user-visible mismatch error redirect. - if (typeof deps.secretService.remove === "function") { - for (const secretId of newSecretIds) { - await deps.secretService.remove(secretId).catch((removeErr) => { - oauthLogger.warn( - { - provider: providerId, - secretId, - err: { message: (removeErr as Error).message }, - }, - "failed to roll back orphan OAuth secret after account mismatch", - ); - }); - } - } + await rollbackNewSecrets("account_mismatch"); return res.redirect( 302, back(deps, stateRow.returnUrl, { oauth_error: "account_mismatch" }), ); } + await rollbackNewSecrets("connection_transaction_failed"); throw err; } diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index f7880926b24..638f33ed615 100644 --- a/server/src/routes/oauth.ts +++ b/server/src/routes/oauth.ts @@ -118,6 +118,11 @@ function ensureCompanyAdmin(req: Request, res: Response): boolean { return true; } +function isCompanyAdmin(req: Request, companyId: string): boolean { + const membership = actorMembership(req, companyId); + return ADMIN_ROLES.has(String(membership?.membershipRole ?? "")); +} + export function oauthRoutes(deps: OAuthRouteDeps): Router { const r = Router({ mergeParams: true }); @@ -215,10 +220,15 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router { r.get("/connections", async (req, res) => { if (!ensureMember(req, res)) return; const companyId = (req.params as unknown as { companyId: string }).companyId; + const includeDiagnostics = isCompanyAdmin(req, companyId); const rows = await deps.db.query.oauthConnections.findMany({ where: eq(oauthConnections.companyId, companyId), }); - res.json({ connections: (rows as unknown[]).map((c) => publicConnection(c)) }); + res.json({ + connections: (rows as unknown[]).map((c) => + publicConnection(c, includeDiagnostics), + ), + }); }); r.get("/connections/:id", async (req, res) => { @@ -234,7 +244,7 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router { res.status(404).end(); return; } - res.json(publicConnection(row)); + res.json(publicConnection(row, isCompanyAdmin(req, companyId))); }); r.post("/connections/:id/refresh", async (req, res) => { @@ -426,7 +436,7 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router { return r; } -function publicConnection(c: unknown) { +function publicConnection(c: unknown, includeDiagnostics = false) { const row = c as { id: string; providerId: string; @@ -451,8 +461,12 @@ function publicConnection(c: unknown) { scopes: row.scopes, accessTokenExpiresAt: row.accessTokenExpiresAt, lastRefreshedAt: row.lastRefreshedAt, - lastError: row.lastError, - lastErrorAt: row.lastErrorAt, + ...(includeDiagnostics + ? { + lastError: row.lastError, + lastErrorAt: row.lastErrorAt, + } + : {}), refreshAttemptCount: row.refreshAttemptCount, createdAt: row.createdAt, updatedAt: row.updatedAt, diff --git a/server/src/services/secrets.ts b/server/src/services/secrets.ts index c6ff3407533..57e620240cd 100644 --- a/server/src/services/secrets.ts +++ b/server/src/services/secrets.ts @@ -2338,6 +2338,7 @@ export function secretService(db: Db, oauthDeps?: SecretServiceOAuthDeps) { } let resolvedAccessToken: string | null = null; + let resolvedAccessTokenSecretId = conn.accessTokenSecretId; if (needsRefresh) { if (!oauthDeps.refreshFn) { const e = new Error( @@ -2354,6 +2355,8 @@ export function secretService(db: Db, oauthDeps?: SecretServiceOAuthDeps) { }); if (refreshResult.outcome === "success") { resolvedAccessToken = refreshResult.accessToken; + resolvedAccessTokenSecretId = + refreshResult.accessTokenSecretId ?? conn.accessTokenSecretId; } else if (refreshResult.outcome === "revoked") { const e = new Error( `oauth_connection_revoked: ${conn.providerId}`, @@ -2378,7 +2381,7 @@ export function secretService(db: Db, oauthDeps?: SecretServiceOAuthDeps) { manifestEntry = { configPath: `env.${key}`, envKey: key, - secretId: conn.accessTokenSecretId, + secretId: resolvedAccessTokenSecretId, secretKey: `oauth:${conn.providerId}:access`, version: 0, provider: "local_encrypted",