From 8fc34e0c091bb90be2e59339fb56729168dcc038 Mon Sep 17 00:00:00 2001 From: Dotta Date: Tue, 12 May 2026 11:17:22 -0500 Subject: [PATCH] fix(oauth): handle final review edge cases Co-Authored-By: Paperclip --- .../src/oauth/__tests__/yaml-loader.test.ts | 18 ++++++++++ server/src/oauth/yaml-loader.ts | 8 +++-- .../routes/__tests__/oauth-connect.test.ts | 33 ++++++++++++++----- .../routes/__tests__/oauth-refresh.test.ts | 20 ++++++++++- server/src/routes/oauth.ts | 20 ++++++----- 5 files changed, 80 insertions(+), 19 deletions(-) diff --git a/server/src/oauth/__tests__/yaml-loader.test.ts b/server/src/oauth/__tests__/yaml-loader.test.ts index fc6e82c0c98..dcd2e96c241 100644 --- a/server/src/oauth/__tests__/yaml-loader.test.ts +++ b/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 }); + } + }); }); diff --git a/server/src/oauth/yaml-loader.ts b/server/src/oauth/yaml-loader.ts index 2df79e22ff7..c5c02bebe0b 100644 --- a/server/src/oauth/yaml-loader.ts +++ b/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) { diff --git a/server/src/routes/__tests__/oauth-connect.test.ts b/server/src/routes/__tests__/oauth-connect.test.ts index 88b02dfd42b..1743299fc9a 100644 --- a/server/src/routes/__tests__/oauth-connect.test.ts +++ b/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 }; + connectFloodLimiter?: { check: ReturnType }; +} = {}) { 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"); diff --git a/server/src/routes/__tests__/oauth-refresh.test.ts b/server/src/routes/__tests__/oauth-refresh.test.ts index d588fcd3ced..d49f03c7c64 100644 --- a/server/src/routes/__tests__/oauth-refresh.test.ts +++ b/server/src/routes/__tests__/oauth-refresh.test.ts @@ -11,6 +11,7 @@ interface MakeAppOptions { | { outcome: "transient"; error: string } | { outcome: "skipped"; reason: string }; conn?: Record | null; + afterRefreshConn?: Record | 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( diff --git a/server/src/routes/oauth.ts b/server/src/routes/oauth.ts index 638f33ed615..53ac93704fb 100644 --- a/server/src/routes/oauth.ts +++ b/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), }); });