diff --git a/packages/db/src/migrations/0103_tool_connection_application_no_cascade.sql b/packages/db/src/migrations/0103_tool_connection_application_no_cascade.sql new file mode 100644 index 00000000000..6250cfe0a8c --- /dev/null +++ b/packages/db/src/migrations/0103_tool_connection_application_no_cascade.sql @@ -0,0 +1,18 @@ +-- Harden DELETE /tool-applications/:applicationId against concurrent connection creation. +-- The tool_connections.application_id FK was ON DELETE CASCADE, so a connection created in +-- the gap between the endpoint's "any connections?" pre-check and its DELETE could be silently +-- removed by the cascade instead of forcing the promised 409. Switch the FK to ON DELETE +-- NO ACTION so the database fails closed: an application with connections can never be deleted, +-- and a concurrently-inserted connection (which holds a FOR KEY SHARE lock on the parent row) +-- forces the delete to raise a foreign_key_violation rather than cascade. +-- +-- NO ACTION (end-of-statement check) rather than RESTRICT (immediate check) is deliberate so a +-- company delete still cascades cleanly: companies -> tool_applications and companies -> +-- tool_connections both fire within the one DELETE statement, and the connections are already +-- gone by the time this constraint is verified. +DO $$ BEGIN + IF EXISTS (SELECT 1 FROM pg_constraint WHERE conname = 'tool_connections_application_id_tool_applications_id_fk') THEN + ALTER TABLE "tool_connections" DROP CONSTRAINT "tool_connections_application_id_tool_applications_id_fk"; + END IF; + ALTER TABLE "tool_connections" ADD CONSTRAINT "tool_connections_application_id_tool_applications_id_fk" FOREIGN KEY ("application_id") REFERENCES "public"."tool_applications"("id") ON DELETE no action ON UPDATE no action; +END $$; diff --git a/packages/db/src/migrations/meta/_journal.json b/packages/db/src/migrations/meta/_journal.json index de55a73dccc..edd29e0ecee 100644 --- a/packages/db/src/migrations/meta/_journal.json +++ b/packages/db/src/migrations/meta/_journal.json @@ -722,6 +722,13 @@ "when": 1780876800000, "tag": "0102_tool_gateway_sessions", "breakpoints": true + }, + { + "idx": 103, + "version": "7", + "when": 1780963200000, + "tag": "0103_tool_connection_application_no_cascade", + "breakpoints": true } ] } diff --git a/packages/db/src/schema/tool_access.ts b/packages/db/src/schema/tool_access.ts index 6c7cd150c52..fafba58a5ad 100644 --- a/packages/db/src/schema/tool_access.ts +++ b/packages/db/src/schema/tool_access.ts @@ -80,7 +80,15 @@ export const toolConnections = pgTable( { id: uuid("id").primaryKey().defaultRandom(), companyId: uuid("company_id").notNull().references(() => companies.id, { onDelete: "cascade" }), - applicationId: uuid("application_id").notNull().references(() => toolApplications.id, { onDelete: "cascade" }), + // NO ACTION (not CASCADE) so the database itself refuses to delete an application that still + // has connections. This closes the delete-vs-create race in DELETE + // /tool-applications/:applicationId: a connection inserted concurrently takes a FOR KEY SHARE + // lock on the parent row, so the delete fails closed with a foreign-key violation instead of + // silently cascading the new connection away. NO ACTION (checked at end-of-statement) rather + // than RESTRICT (checked immediately) is deliberate: it lets a company delete still cascade — + // companies → tool_applications and companies → tool_connections both fire in one statement, + // and the connections are gone by the time this constraint is checked. + applicationId: uuid("application_id").notNull().references(() => toolApplications.id, { onDelete: "no action" }), name: text("name").notNull(), connectionKind: text("connection_kind").$type().notNull().default("managed"), transport: text("transport").$type().notNull(), diff --git a/server/src/__tests__/tool-access-service.test.ts b/server/src/__tests__/tool-access-service.test.ts index db262604047..48406609be3 100644 --- a/server/src/__tests__/tool-access-service.test.ts +++ b/server/src/__tests__/tool-access-service.test.ts @@ -603,6 +603,63 @@ describeEmbeddedPostgres("tool access service", () => { expect(remaining).toHaveLength(1); }); + it("fails closed at the database when a connection races an application delete (no silent cascade)", async () => { + const company = await createCompany(db); + const service = toolAccessService(db); + const connection = await service.createConnection(company.id, { + name: "Racy connection", + transport: "remote_http", + config: { url: "https://fixture.example/mcp" }, + }); + + // Simulate the delete-vs-create race: skip the endpoint's "any connections?" pre-check and + // issue the raw DELETE it would run afterwards, standing in for a connection created in the + // gap. Under the old ON DELETE CASCADE schema this silently removed the linked connection; + // the hardened ON DELETE NO ACTION FK must reject it so the delete can never become an + // implicit cascade. + await expect( + db.delete(toolApplications).where(eq(toolApplications.id, connection.applicationId)), + ).rejects.toThrow(); + + const remainingApp = await db + .select() + .from(toolApplications) + .where(eq(toolApplications.id, connection.applicationId)); + const remainingConnection = await db + .select() + .from(toolConnections) + .where(eq(toolConnections.id, connection.id)); + expect(remainingApp).toHaveLength(1); + expect(remainingConnection).toHaveLength(1); + }); + + it("still cascades application + connection deletes when the owning company is removed", async () => { + const company = await createCompany(db); + const service = toolAccessService(db); + const connection = await service.createConnection(company.id, { + name: "Company-scoped connection", + transport: "remote_http", + config: { url: "https://fixture.example/mcp" }, + }); + + // NO ACTION (not RESTRICT) must keep the company teardown cascade intact: deleting the + // company cascades to both tool_applications and tool_connections in one statement, and the + // end-of-statement FK check passes because the connection is already gone. RESTRICT would + // abort this delete mid-cascade. + await db.delete(companies).where(eq(companies.id, company.id)); + + const remainingApp = await db + .select() + .from(toolApplications) + .where(eq(toolApplications.id, connection.applicationId)); + const remainingConnection = await db + .select() + .from(toolConnections) + .where(eq(toolConnections.id, connection.id)); + expect(remainingApp).toHaveLength(0); + expect(remainingConnection).toHaveLength(0); + }); + it("returns 403 for cross-company application deletes and 404 for missing applications", async () => { const allowedCompany = await createCompany(db); const otherCompany = await createCompany(db); diff --git a/server/src/services/tool-access.ts b/server/src/services/tool-access.ts index de209cd0d66..0f4fc86a9f4 100644 --- a/server/src/services/tool-access.ts +++ b/server/src/services/tool-access.ts @@ -181,6 +181,36 @@ function asRecord(value: unknown): Record { return {}; } +// Detects a Postgres foreign_key_violation (SQLSTATE 23503) raised by the +// tool_connections.application_id constraint — i.e. an application delete that lost the race to +// a concurrently-created connection now that the FK is ON DELETE RESTRICT. Walks the error and +// its `cause` since the driver may wrap the original pg error. +function isToolConnectionForeignKeyViolation(error: unknown): boolean { + const records: Record[] = []; + let current: unknown = error; + for (let depth = 0; depth < 4 && current && typeof current === "object"; depth += 1) { + const record = current as Record; + records.push(record); + current = record.cause; + } + return records.some((record) => { + const code = typeof record.code === "string" ? record.code : null; + const constraint = + typeof record.constraint === "string" + ? record.constraint + : typeof record.constraint_name === "string" + ? record.constraint_name + : null; + const message = typeof record.message === "string" ? record.message : ""; + return ( + code === "23503" && + (constraint === "tool_connections_application_id_tool_applications_id_fk" || + /tool_connections/.test(constraint ?? "") || + /tool_connections/.test(message)) + ); + }); +} + function numberValue(value: unknown): number | null { const parsed = typeof value === "number" ? value : Number(value); return Number.isFinite(parsed) ? parsed : null; @@ -1901,7 +1931,22 @@ export function toolAccessService(db: Db, options: ToolAccessServiceOptions = {} { connectionCount: linkedConnections.length }, ); } - const [row] = await db.delete(toolApplications).where(eq(toolApplications.id, applicationId)).returning(); + // The pre-check above gives a friendly 409 in the common case, but it cannot close the + // race where a connection is created in the gap before this delete runs. The FK is now + // ON DELETE RESTRICT, so such a delete fails closed with a foreign_key_violation instead + // of silently cascading the new connection away. Translate that into the same 409 so the + // endpoint keeps its contract instead of surfacing a 500. + let row: typeof toolApplications.$inferSelect | undefined; + try { + [row] = await db.delete(toolApplications).where(eq(toolApplications.id, applicationId)).returning(); + } catch (error) { + if (isToolConnectionForeignKeyViolation(error)) { + throw conflict( + "This application still has connections. Remove its connections or archive the application instead of deleting it.", + ); + } + throw error; + } if (!row) throw notFound("Tool application not found"); return toApplication(row); },