From 7f9a76411aa12f7b89ef6a8bed2f76080a6ad157 Mon Sep 17 00:00:00 2001 From: dotta Date: Mon, 23 Mar 2026 08:45:56 -0500 Subject: [PATCH] Address Greptile review on board CLI auth Co-Authored-By: Paperclip --- cli/src/client/board-auth.ts | 6 +- cli/src/client/command-label.ts | 4 + cli/src/commands/client/common.ts | 6 +- packages/db/src/client.test.ts | 30 +++++ .../db/src/migrations/0044_illegal_toad.sql | 3 +- packages/db/src/schema/board_api_keys.ts | 4 +- server/src/__tests__/cli-auth-routes.test.ts | 66 +++++++++ server/src/routes/access.ts | 15 ++- server/src/services/board-auth.ts | 127 +++++++++++++----- 9 files changed, 207 insertions(+), 54 deletions(-) create mode 100644 cli/src/client/command-label.ts diff --git a/cli/src/client/board-auth.ts b/cli/src/client/board-auth.ts index 363109e7..5ed7cd7a 100644 --- a/cli/src/client/board-auth.ts +++ b/cli/src/client/board-auth.ts @@ -2,6 +2,7 @@ import { spawn } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import pc from "picocolors"; +import { buildCliCommandLabel } from "./command-label.js"; import { resolveDefaultCliAuthPath } from "../config/home.js"; type RequestedAccess = "board" | "instance_admin_required"; @@ -189,11 +190,6 @@ function openUrl(url: string): boolean { } } -function buildCliCommandLabel(): string { - const parts = process.argv.slice(2); - return parts.length > 0 ? `paperclipai ${parts.join(" ")}` : "paperclipai"; -} - export async function loginBoardCli(params: { apiBase: string; requestedAccess: RequestedAccess; diff --git a/cli/src/client/command-label.ts b/cli/src/client/command-label.ts new file mode 100644 index 00000000..21143b3b --- /dev/null +++ b/cli/src/client/command-label.ts @@ -0,0 +1,4 @@ +export function buildCliCommandLabel(): string { + const args = process.argv.slice(2); + return args.length > 0 ? `paperclipai ${args.join(" ")}` : "paperclipai"; +} diff --git a/cli/src/commands/client/common.ts b/cli/src/commands/client/common.ts index 868d552d..db5f7dbc 100644 --- a/cli/src/commands/client/common.ts +++ b/cli/src/commands/client/common.ts @@ -1,6 +1,7 @@ import pc from "picocolors"; import type { Command } from "commander"; import { getStoredBoardCredential, loginBoardCli } from "../../client/board-auth.js"; +import { buildCliCommandLabel } from "../../client/command-label.js"; import { readConfig } from "../../config/store.js"; import { readContext, resolveProfile, type ClientContextProfile } from "../../client/context.js"; import { ApiRequestError, PaperclipApiClient } from "../../client/http.js"; @@ -112,11 +113,6 @@ function canAttemptInteractiveBoardAuth(): boolean { return Boolean(process.stdin.isTTY && process.stdout.isTTY); } -function buildCliCommandLabel(): string { - const args = process.argv.slice(2); - return args.length > 0 ? `paperclipai ${args.join(" ")}` : "paperclipai"; -} - export function printOutput(data: unknown, opts: { json?: boolean; label?: string } = {}): void { if (opts.json) { console.log(JSON.stringify(data, null, 2)); diff --git a/packages/db/src/client.test.ts b/packages/db/src/client.test.ts index 074188e7..9a86a7b3 100644 --- a/packages/db/src/client.test.ts +++ b/packages/db/src/client.test.ts @@ -198,4 +198,34 @@ describe("applyPendingMigrations", () => { }, 20_000, ); + + it( + "enforces a unique board_api_keys.key_hash after migration 0044", + async () => { + const connectionString = await createTempDatabase(); + + await applyPendingMigrations(connectionString); + + const sql = postgres(connectionString, { max: 1, onnotice: () => {} }); + try { + await sql.unsafe(` + INSERT INTO "user" ("id", "name", "email", "email_verified", "created_at", "updated_at") + VALUES ('user-1', 'User One', 'user@example.com', true, now(), now()) + `); + await sql.unsafe(` + INSERT INTO "board_api_keys" ("id", "user_id", "name", "key_hash", "created_at") + VALUES ('00000000-0000-0000-0000-000000000001', 'user-1', 'Key One', 'dup-hash', now()) + `); + await expect( + sql.unsafe(` + INSERT INTO "board_api_keys" ("id", "user_id", "name", "key_hash", "created_at") + VALUES ('00000000-0000-0000-0000-000000000002', 'user-1', 'Key Two', 'dup-hash', now()) + `), + ).rejects.toThrow(); + } finally { + await sql.end(); + } + }, + 20_000, + ); }); diff --git a/packages/db/src/migrations/0044_illegal_toad.sql b/packages/db/src/migrations/0044_illegal_toad.sql index 79daa2ba..5f1f18bd 100644 --- a/packages/db/src/migrations/0044_illegal_toad.sql +++ b/packages/db/src/migrations/0044_illegal_toad.sql @@ -48,7 +48,8 @@ DO $$ BEGIN ALTER TABLE "cli_auth_challenges" ADD CONSTRAINT "cli_auth_challenges_board_api_key_id_board_api_keys_id_fk" FOREIGN KEY ("board_api_key_id") REFERENCES "public"."board_api_keys"("id") ON DELETE set null ON UPDATE no action; END IF; END $$;--> statement-breakpoint -CREATE INDEX IF NOT EXISTS "board_api_keys_key_hash_idx" ON "board_api_keys" USING btree ("key_hash");--> statement-breakpoint +DROP INDEX IF EXISTS "board_api_keys_key_hash_idx";--> statement-breakpoint +CREATE UNIQUE INDEX IF NOT EXISTS "board_api_keys_key_hash_idx" ON "board_api_keys" USING btree ("key_hash");--> statement-breakpoint CREATE INDEX IF NOT EXISTS "board_api_keys_user_idx" ON "board_api_keys" USING btree ("user_id");--> statement-breakpoint CREATE INDEX IF NOT EXISTS "cli_auth_challenges_secret_hash_idx" ON "cli_auth_challenges" USING btree ("secret_hash");--> statement-breakpoint CREATE INDEX IF NOT EXISTS "cli_auth_challenges_approved_by_idx" ON "cli_auth_challenges" USING btree ("approved_by_user_id");--> statement-breakpoint diff --git a/packages/db/src/schema/board_api_keys.ts b/packages/db/src/schema/board_api_keys.ts index f603d44d..e786760f 100644 --- a/packages/db/src/schema/board_api_keys.ts +++ b/packages/db/src/schema/board_api_keys.ts @@ -1,4 +1,4 @@ -import { pgTable, uuid, text, timestamp, index } from "drizzle-orm/pg-core"; +import { pgTable, uuid, text, timestamp, index, uniqueIndex } from "drizzle-orm/pg-core"; import { authUsers } from "./auth.js"; export const boardApiKeys = pgTable( @@ -14,7 +14,7 @@ export const boardApiKeys = pgTable( createdAt: timestamp("created_at", { withTimezone: true }).notNull().defaultNow(), }, (table) => ({ - keyHashIdx: index("board_api_keys_key_hash_idx").on(table.keyHash), + keyHashIdx: uniqueIndex("board_api_keys_key_hash_idx").on(table.keyHash), userIdx: index("board_api_keys_user_idx").on(table.userId), }), ); diff --git a/server/src/__tests__/cli-auth-routes.test.ts b/server/src/__tests__/cli-auth-routes.test.ts index e3d91697..d2491673 100644 --- a/server/src/__tests__/cli-auth-routes.test.ts +++ b/server/src/__tests__/cli-auth-routes.test.ts @@ -18,6 +18,7 @@ const mockBoardAuthService = vi.hoisted(() => ({ approveCliAuthChallenge: vi.fn(), cancelCliAuthChallenge: vi.fn(), resolveBoardAccess: vi.fn(), + resolveBoardActivityCompanyIds: vi.fn(), assertCurrentBoardKey: vi.fn(), revokeBoardApiKey: vi.fn(), })); @@ -132,6 +133,7 @@ describe("cli auth routes", () => { companyIds: ["company-1"], isInstanceAdmin: false, }); + mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-1"]); const app = await createApp({ type: "board", @@ -161,4 +163,68 @@ describe("cli auth routes", () => { }), ); }); + + it("logs approve activity for instance admins without company memberships", async () => { + mockBoardAuthService.approveCliAuthChallenge.mockResolvedValue({ + status: "approved", + challenge: { + id: "challenge-2", + boardApiKeyId: "board-key-2", + requestedAccess: "instance_admin_required", + requestedCompanyId: null, + expiresAt: new Date("2026-03-23T13:00:00.000Z"), + }, + }); + mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-a", "company-b"]); + + const app = await createApp({ + type: "board", + userId: "admin-1", + source: "session", + isInstanceAdmin: true, + companyIds: [], + }); + const res = await request(app) + .post("/api/cli-auth/challenges/challenge-2/approve") + .send({ token: "pcp_cli_auth_secret" }); + + expect(res.status).toBe(200); + expect(mockBoardAuthService.resolveBoardActivityCompanyIds).toHaveBeenCalledWith({ + userId: "admin-1", + requestedCompanyId: null, + boardApiKeyId: "board-key-2", + }); + expect(mockLogActivity).toHaveBeenCalledTimes(2); + }); + + it("logs revoke activity with resolved audit company ids", async () => { + mockBoardAuthService.assertCurrentBoardKey.mockResolvedValue({ + id: "board-key-3", + userId: "admin-2", + }); + mockBoardAuthService.resolveBoardActivityCompanyIds.mockResolvedValue(["company-z"]); + + const app = await createApp({ + type: "board", + userId: "admin-2", + keyId: "board-key-3", + source: "board_key", + isInstanceAdmin: true, + companyIds: [], + }); + const res = await request(app).post("/api/cli-auth/revoke-current").send({}); + + expect(res.status).toBe(200); + expect(mockBoardAuthService.resolveBoardActivityCompanyIds).toHaveBeenCalledWith({ + userId: "admin-2", + boardApiKeyId: "board-key-3", + }); + expect(mockLogActivity).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + companyId: "company-z", + action: "board_api_key.revoked", + }), + ); + }); }); diff --git a/server/src/routes/access.ts b/server/src/routes/access.ts index e66b3d44..6d8c5af2 100644 --- a/server/src/routes/access.ts +++ b/server/src/routes/access.ts @@ -1671,8 +1671,12 @@ export function accessRoutes( ); if (approved.status === "approved") { - const accessSnapshot = await boardAuth.resolveBoardAccess(userId); - for (const companyId of accessSnapshot.companyIds) { + const companyIds = await boardAuth.resolveBoardActivityCompanyIds({ + userId, + requestedCompanyId: approved.challenge.requestedCompanyId, + boardApiKeyId: approved.challenge.boardApiKeyId, + }); + for (const companyId of companyIds) { await logActivity(db, { companyId, actorType: "user", @@ -1737,8 +1741,11 @@ export function accessRoutes( req.actor.userId, ); await boardAuth.revokeBoardApiKey(key.id); - const accessSnapshot = await boardAuth.resolveBoardAccess(key.userId); - for (const companyId of accessSnapshot.companyIds) { + const companyIds = await boardAuth.resolveBoardActivityCompanyIds({ + userId: key.userId, + boardApiKeyId: key.id, + }); + for (const companyId of companyIds) { await logActivity(db, { companyId, actorType: "user", diff --git a/server/src/services/board-auth.ts b/server/src/services/board-auth.ts index bbcdc5b3..19e533c1 100644 --- a/server/src/services/board-auth.ts +++ b/server/src/services/board-auth.ts @@ -1,5 +1,5 @@ import { createHash, randomBytes, timingSafeEqual } from "node:crypto"; -import { and, eq, isNull } from "drizzle-orm"; +import { and, eq, isNull, sql } from "drizzle-orm"; import type { Db } from "@paperclipai/db"; import { authUsers, @@ -86,6 +86,46 @@ export function boardAuthService(db: Db) { }; } + async function resolveBoardActivityCompanyIds(input: { + userId: string; + requestedCompanyId?: string | null; + boardApiKeyId?: string | null; + }) { + const access = await resolveBoardAccess(input.userId); + const companyIds = new Set(access.companyIds); + + if (companyIds.size === 0 && input.requestedCompanyId?.trim()) { + companyIds.add(input.requestedCompanyId.trim()); + } + + if (companyIds.size === 0 && input.boardApiKeyId?.trim()) { + const challengeCompanyIds = await db + .select({ requestedCompanyId: cliAuthChallenges.requestedCompanyId }) + .from(cliAuthChallenges) + .where(eq(cliAuthChallenges.boardApiKeyId, input.boardApiKeyId.trim())) + .then((rows) => + rows + .map((row) => row.requestedCompanyId?.trim() ?? null) + .filter((value): value is string => Boolean(value)), + ); + for (const companyId of challengeCompanyIds) { + companyIds.add(companyId); + } + } + + if (companyIds.size === 0 && access.isInstanceAdmin) { + const allCompanyIds = await db + .select({ id: companies.id }) + .from(companies) + .then((rows) => rows.map((row) => row.id)); + for (const companyId of allCompanyIds) { + companyIds.add(companyId); + } + } + + return Array.from(companyIds); + } + async function findBoardApiKeyByToken(token: string) { const tokenHash = hashBearerToken(token); const now = new Date(); @@ -210,47 +250,59 @@ export function boardAuthService(db: Db) { } async function approveCliAuthChallenge(id: string, token: string, userId: string) { - const challenge = await getCliAuthChallengeBySecret(id, token); - if (!challenge) throw notFound("CLI auth challenge not found"); - - const status = challengeStatusForRow(challenge); - if (status === "expired") return { status, challenge }; - if (status === "cancelled") return { status, challenge }; - const access = await resolveBoardAccess(userId); - if (challenge.requestedAccess === "instance_admin_required" && !access.isInstanceAdmin) { - throw forbidden("Instance admin required"); - } + return db.transaction(async (tx) => { + await tx.execute( + sql`select ${cliAuthChallenges.id} from ${cliAuthChallenges} where ${cliAuthChallenges.id} = ${id} for update`, + ); - let boardKeyId = challenge.boardApiKeyId; - if (!boardKeyId) { - const createdKey = await db - .insert(boardApiKeys) - .values({ - userId, - name: challenge.pendingKeyName, - keyHash: challenge.pendingKeyHash, - expiresAt: boardApiKeyExpiresAt(), + const challenge = await tx + .select() + .from(cliAuthChallenges) + .where(eq(cliAuthChallenges.id, id)) + .then((rows) => rows[0] ?? null); + if (!challenge || !tokenHashesMatch(challenge.secretHash, hashBearerToken(token))) { + throw notFound("CLI auth challenge not found"); + } + + const status = challengeStatusForRow(challenge); + if (status === "expired") return { status, challenge }; + if (status === "cancelled") return { status, challenge }; + + if (challenge.requestedAccess === "instance_admin_required" && !access.isInstanceAdmin) { + throw forbidden("Instance admin required"); + } + + let boardKeyId = challenge.boardApiKeyId; + if (!boardKeyId) { + const createdKey = await tx + .insert(boardApiKeys) + .values({ + userId, + name: challenge.pendingKeyName, + keyHash: challenge.pendingKeyHash, + expiresAt: boardApiKeyExpiresAt(), + }) + .returning() + .then((rows) => rows[0]); + boardKeyId = createdKey.id; + } + + const approvedAt = challenge.approvedAt ?? new Date(); + const updated = await tx + .update(cliAuthChallenges) + .set({ + approvedByUserId: userId, + boardApiKeyId: boardKeyId, + approvedAt, + updatedAt: new Date(), }) + .where(eq(cliAuthChallenges.id, challenge.id)) .returning() - .then((rows) => rows[0]); - boardKeyId = createdKey.id; - } + .then((rows) => rows[0] ?? challenge); - const approvedAt = challenge.approvedAt ?? new Date(); - const updated = await db - .update(cliAuthChallenges) - .set({ - approvedByUserId: userId, - boardApiKeyId: boardKeyId, - approvedAt, - updatedAt: new Date(), - }) - .where(eq(cliAuthChallenges.id, challenge.id)) - .returning() - .then((rows) => rows[0] ?? challenge); - - return { status: "approved" as const, challenge: updated }; + return { status: "approved" as const, challenge: updated }; + }); } async function cancelCliAuthChallenge(id: string, token: string) { @@ -297,5 +349,6 @@ export function boardAuthService(db: Db) { approveCliAuthChallenge, cancelCliAuthChallenge, assertCurrentBoardKey, + resolveBoardActivityCompanyIds, }; }