fix(oauth): address callback and diagnostics review

Co-Authored-By: Paperclip <noreply@paperclip.ing>

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",

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<RefreshResul
     return {
       outcome: "success",
       accessToken: parsed.accessToken,
+      accessTokenSecretId: access.id,
     } as const;
   });
 }

server/src/routes/__tests__/oauth-callback-cleanup.test.ts

@@ -0,0 +1,116 @@
+import { describe, it, expect, vi } from "vitest";
+import express from "express";
+import request from "supertest";
+import { oauthCallbackRoute } from "../oauth-callback.js";
+
+vi.mock("../../oauth/http.js", () => ({
+  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<void>) =>
+        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");
+  });
+});

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<typeof conn>) {
+function makeApp(rows: Array<typeof conn>, 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<typeof conn>) {
       (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);

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:<provider>:<accountId>: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<void> => {
+      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;
     }
 

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,

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",