fix(oauth): handle final review edge cases

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

server/src/oauth/__tests__/yaml-loader.test.ts

@@ -1,4 +1,6 @@
 import { describe, it, expect } from "vitest";
+import { mkdtemp, rm, writeFile } from "node:fs/promises";
+import os from "node:os";
 import path from "node:path";
 import { fileURLToPath } from "node:url";
 import { loadProviderConfigsFromDirectory } from "../yaml-loader.js";
@@ -19,4 +21,20 @@ describe("loadProviderConfigsFromDirectory", () => {
     );
     expect(configs).toEqual([]);
   });
+
+  it("reports tab indentation explicitly", async () => {
+    const dir = await mkdtemp(path.join(os.tmpdir(), "oauth-provider-yaml-"));
+    try {
+      await writeFile(
+        path.join(dir, "bad.yaml"),
+        "id: bad\n\tclientCredentials:\n\t\tclientIdEnv: BAD_ID\n",
+        "utf8",
+      );
+      await expect(loadProviderConfigsFromDirectory(dir)).rejects.toThrow(
+        /Tab indentation is not supported/,
+      );
+    } finally {
+      await rm(dir, { recursive: true, force: true });
+    }
+  });
 });

server/src/oauth/yaml-loader.ts

@@ -39,7 +39,11 @@ function parseProviderYaml(raw: string): unknown {
     if (!originalLine.trim() || originalLine.trimStart().startsWith("#")) {
       continue;
     }
-    const indent = originalLine.match(/^ */)?.[0].length ?? 0;
+    const leadingWhitespace = originalLine.match(/^\s*/)?.[0] ?? "";
+    if (leadingWhitespace.includes("\t")) {
+      throw new Error(`Tab indentation is not supported on line ${index + 1}`);
+    }
+    const indent = leadingWhitespace.length;
     if (indent % 2 !== 0) {
       throw new Error(`Invalid indentation on line ${index + 1}`);
     }
@@ -117,7 +121,7 @@ export async function loadProviderConfigsFromDirectory(
         { file: fullPath, err },
         "failed to parse OAuth provider yaml",
       );
-      throw new Error(`Invalid YAML in ${fullPath}`);
+      throw new Error(`Invalid YAML in ${fullPath}: ${(err as Error).message}`);
     }
     const result = OAuthProviderConfigSchema.safeParse(parsed);
     if (!result.success) {

server/src/routes/__tests__/oauth-connect.test.ts

@@ -4,7 +4,10 @@ import request from "supertest";
 import { oauthRoutes } from "../oauth.js";
 import { ProviderRegistry } from "../../oauth/registry.js";
 
-function setup() {
+function setup(opts: {
+  rateLimiter?: { check: ReturnType<typeof vi.fn> };
+  connectFloodLimiter?: { check: ReturnType<typeof vi.fn> };
+} = {}) {
   const insertMock = vi.fn().mockResolvedValue([{ id: "state-uuid-123" }]);
   const db = {
     insert: () => ({ values: () => ({ returning: insertMock }) }),
@@ -43,13 +46,14 @@ function setup() {
       };
       next();
     },
-    oauthRoutes({
-      registry,
-      db: db as unknown as never,
-      publicUrl: "https://app.paperclip.test",
-      rateLimiter: { check: async () => true },
-      secretService: {},
-    }),
+      oauthRoutes({
+        registry,
+        db: db as unknown as never,
+        publicUrl: "https://app.paperclip.test",
+        rateLimiter: opts.rateLimiter ?? { check: vi.fn(async () => true) },
+        connectFloodLimiter: opts.connectFloodLimiter,
+        secretService: {},
+      }),
   );
   return { app, insertMock };
 }
@@ -82,6 +86,19 @@ describe("POST /connect/:providerId", () => {
     expect(res.body.errorCode).toBe("invalid_scope");
   });
 
+  it("checks company flood limit before consuming a per-user rate-limit slot", async () => {
+    const rateLimiter = { check: vi.fn(async () => true) };
+    const connectFloodLimiter = { check: vi.fn(async () => false) };
+    const { app } = setup({ rateLimiter, connectFloodLimiter });
+
+    const res = await request(app).post("/api/companies/c1/oauth/connect/github");
+
+    expect(res.status).toBe(429);
+    expect(res.body.errorCode).toBe("connect_flood");
+    expect(connectFloodLimiter.check).toHaveBeenCalledTimes(1);
+    expect(rateLimiter.check).not.toHaveBeenCalled();
+  });
+
   it("returns 404 for unknown provider", async () => {
     const { app } = setup();
     const res = await request(app).post("/api/companies/c1/oauth/connect/unknown");

server/src/routes/__tests__/oauth-refresh.test.ts

@@ -11,6 +11,7 @@ interface MakeAppOptions {
     | { outcome: "transient"; error: string }
     | { outcome: "skipped"; reason: string };
   conn?: Record<string, unknown> | null;
+  afterRefreshConn?: Record<string, unknown> | null;
 }
 
 function makeApp(opts: MakeAppOptions = {}) {
@@ -56,7 +57,12 @@ function makeApp(opts: MakeAppOptions = {}) {
           updatedAt: new Date(),
         }
       : opts.conn;
-  const findFirst = vi.fn().mockResolvedValue(conn);
+  const findFirst = vi
+    .fn()
+    .mockResolvedValueOnce(conn)
+    .mockResolvedValue(
+      opts.afterRefreshConn === undefined ? conn : opts.afterRefreshConn,
+    );
   const db = {
     query: {
       oauthConnections: { findFirst },
@@ -140,6 +146,18 @@ describe("POST /connections/:id/refresh", () => {
     expect(res.body.errorCode).toBe("connection_revoked");
   });
 
+  it("returns 404 when connection is deleted after refresh completes", async () => {
+    const { app, refreshFn } = makeApp({
+      refreshOutcome: { outcome: "success", accessToken: "x" },
+      afterRefreshConn: null,
+    });
+    const res = await request(app).post(
+      "/api/companies/c1/oauth/connections/conn/refresh",
+    );
+    expect(res.status).toBe(404);
+    expect(refreshFn).toHaveBeenCalledTimes(1);
+  });
+
   it("returns 404 when connection does not belong to company", async () => {
     const { app } = makeApp({ conn: null });
     const res = await request(app).post(

server/src/routes/oauth.ts

@@ -150,11 +150,6 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router {
     }
 
     const actor = (req as Request & { actor: { userId: string } }).actor;
-    const ok = await deps.rateLimiter.check(`connect:${actor.userId}`);
-    if (!ok) {
-      res.status(429).json({ errorCode: "rate_limited" });
-      return;
-    }
     if (deps.connectFloodLimiter) {
       const floodOk = await deps.connectFloodLimiter.check(
         `connect-flood:${(req.params as unknown as { companyId: string }).companyId}`,
@@ -164,6 +159,11 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router {
         return;
       }
     }
+    const ok = await deps.rateLimiter.check(`connect:${actor.userId}`);
+    if (!ok) {
+      res.status(429).json({ errorCode: "rate_limited" });
+      return;
+    }
 
     const { scopes, returnUrl } = (req.body ?? {}) as {
       scopes?: unknown;
@@ -302,20 +302,24 @@ export function oauthRoutes(deps: OAuthRouteDeps): Router {
     const updated = await deps.db.query.oauthConnections.findFirst({
       where: eq(oauthConnections.id, row.id),
     });
+    if (!updated) {
+      res.status(404).end();
+      return;
+    }
     if (result.outcome === "success") {
-      res.json(publicConnection(updated));
+      res.json(publicConnection(updated, true));
       return;
     }
     if (result.outcome === "revoked") {
       res.status(409).json({
         errorCode: "connection_revoked",
-        connection: publicConnection(updated),
+        connection: publicConnection(updated, true),
       });
       return;
     }
     res.status(503).json({
       errorCode: "refresh_failed",
-      connection: publicConnection(updated),
+      connection: publicConnection(updated, true),
     });
   });