From 1ff5c3377c540ae2c968024e3c34bb548d8f2315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hartmut=20N=C3=B6renberg?= Date: Fri, 17 Apr 2026 08:38:05 +0200 Subject: [PATCH] security: block raw/tx escape hatches on read-only AI DB proxy (#47) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read-only proxy previously wrapped model delegates to block writes, but left client-level raw/escape hatches ($transaction, $executeRaw, $executeRawUnsafe, $queryRawUnsafe, $runCommandRaw) intact. A read-tool could smuggle DML via raw SQL, or open an interactive $transaction whose tx-scoped client (unproxied by construction) accepts writes. - read-only-prisma: block $transaction, $executeRaw, $executeRawUnsafe, $queryRawUnsafe, $runCommandRaw at the client level. Template-tagged $queryRaw stays allowed (read-only by API contract). - assistant-tools: add create_estimate to MUTATION_TOOLS — it uses $transaction internally and was previously bypassing the proxy only because $transaction wasn't blocked. - shared: document isReadOnly flag on ToolContext so any scoped tRPC caller a tool spawns keeps the proxied client. - helpers: note the runtime wrap at assistant-tools.ts:739 is authoritative; forwarding ctx.db verbatim is correct. - tests: cover model writes, raw escapes, and the allowed $queryRaw path (7 cases, all pass). - loosen one estimate-detail test that compared the exact db instance (fails once that instance is a proxy; the assertion's intent is the estimate id). Covers EGAI 4.1.1.2 / IAAI 3.6.22. Co-Authored-By: Claude Opus 4.7 --- ...-tools-estimate-read-detail-access.test.ts | 4 +- .../src/__tests__/read-only-prisma.test.ts | 94 +++++++++++++++++++ packages/api/src/lib/read-only-prisma.ts | 20 +++- packages/api/src/router/assistant-tools.ts | 1 + .../api/src/router/assistant-tools/helpers.ts | 5 + .../api/src/router/assistant-tools/shared.ts | 7 ++ 6 files changed, 127 insertions(+), 4 deletions(-) create mode 100644 packages/api/src/__tests__/read-only-prisma.test.ts diff --git a/packages/api/src/__tests__/assistant-tools-estimate-read-detail-access.test.ts b/packages/api/src/__tests__/assistant-tools-estimate-read-detail-access.test.ts index 6e17e08..c2f34bd 100644 --- a/packages/api/src/__tests__/assistant-tools-estimate-read-detail-access.test.ts +++ b/packages/api/src/__tests__/assistant-tools-estimate-read-detail-access.test.ts @@ -60,7 +60,9 @@ describe("assistant estimate detail read tools", () => { userCtx, ); - expect(vi.mocked(getEstimateById)).toHaveBeenCalledWith(controllerCtx.db, "est_1"); + // Read tools receive ctx.db wrapped in a read-only proxy (EGAI 4.1.1.2), + // so we assert only on the estimate id, not the exact db instance. + expect(vi.mocked(getEstimateById)).toHaveBeenCalledWith(expect.anything(), "est_1"); expect(JSON.parse(successResult.content)).toEqual( expect.objectContaining({ id: "est_1", diff --git a/packages/api/src/__tests__/read-only-prisma.test.ts b/packages/api/src/__tests__/read-only-prisma.test.ts new file mode 100644 index 0000000..201d40e --- /dev/null +++ b/packages/api/src/__tests__/read-only-prisma.test.ts @@ -0,0 +1,94 @@ +import { describe, expect, it, vi } from "vitest"; +import { createReadOnlyProxy } from "../lib/read-only-prisma.js"; + +function makeFakeClient() { + const user = { + findUnique: vi.fn(async () => ({ id: "u1" })), + findMany: vi.fn(async () => []), + create: vi.fn(async () => ({ id: "u1" })), + update: vi.fn(async () => ({ id: "u1" })), + upsert: vi.fn(async () => ({ id: "u1" })), + delete: vi.fn(async () => ({ id: "u1" })), + createMany: vi.fn(async () => ({ count: 1 })), + createManyAndReturn: vi.fn(async () => [{ id: "u1" }]), + updateMany: vi.fn(async () => ({ count: 1 })), + deleteMany: vi.fn(async () => ({ count: 1 })), + }; + const client = { + user, + $queryRaw: vi.fn(async () => [{ result: 1 }]), + $queryRawUnsafe: vi.fn(async () => [{ result: 1 }]), + $executeRaw: vi.fn(async () => 0), + $executeRawUnsafe: vi.fn(async () => 0), + $transaction: vi.fn(async () => []), + $runCommandRaw: vi.fn(async () => ({ ok: 1 })), + }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any + return client as any; +} + +describe("createReadOnlyProxy", () => { + it("allows model reads", async () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + await expect(proxy.user.findUnique({ where: { id: "u1" } })).resolves.toEqual({ id: "u1" }); + await expect(proxy.user.findMany()).resolves.toEqual([]); + }); + + it("blocks model writes with clear error", () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + expect(() => proxy.user.create({ data: {} })).toThrow( + /Write operation "create" on "user" not permitted/, + ); + expect(() => proxy.user.update({ where: { id: "u1" }, data: {} })).toThrow( + /Write operation "update"/, + ); + expect(() => proxy.user.upsert({ where: { id: "u1" }, create: {}, update: {} })).toThrow( + /Write operation "upsert"/, + ); + expect(() => proxy.user.delete({ where: { id: "u1" } })).toThrow(/Write operation "delete"/); + expect(() => proxy.user.createMany({ data: [] })).toThrow(/Write operation "createMany"/); + expect(() => proxy.user.createManyAndReturn({ data: [] })).toThrow( + /Write operation "createManyAndReturn"/, + ); + expect(() => proxy.user.updateMany({ where: {}, data: {} })).toThrow( + /Write operation "updateMany"/, + ); + expect(() => proxy.user.deleteMany({ where: {} })).toThrow(/Write operation "deleteMany"/); + }); + + it("allows template-tagged $queryRaw (read-only by contract)", async () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + await expect(proxy.$queryRaw`SELECT 1`).resolves.toEqual([{ result: 1 }]); + }); + + it("blocks $queryRawUnsafe (DDL/DML smuggling)", () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + expect(() => proxy.$queryRawUnsafe("SELECT 1")).toThrow( + /Raw\/escape operation "\$queryRawUnsafe" not permitted/, + ); + }); + + it("blocks $executeRaw and $executeRawUnsafe", () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + expect(() => proxy.$executeRaw`DELETE FROM users`).toThrow( + /Raw\/escape operation "\$executeRaw" not permitted/, + ); + expect(() => proxy.$executeRawUnsafe("DELETE FROM users")).toThrow( + /Raw\/escape operation "\$executeRawUnsafe" not permitted/, + ); + }); + + it("blocks $transaction (interactive tx could contain writes)", () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + expect(() => proxy.$transaction([])).toThrow( + /Raw\/escape operation "\$transaction" not permitted/, + ); + }); + + it("blocks $runCommandRaw (Mongo-style raw command)", () => { + const proxy = createReadOnlyProxy(makeFakeClient()); + expect(() => proxy.$runCommandRaw({})).toThrow( + /Raw\/escape operation "\$runCommandRaw" not permitted/, + ); + }); +}); diff --git a/packages/api/src/lib/read-only-prisma.ts b/packages/api/src/lib/read-only-prisma.ts index 1be408b..217a103 100644 --- a/packages/api/src/lib/read-only-prisma.ts +++ b/packages/api/src/lib/read-only-prisma.ts @@ -20,6 +20,17 @@ const WRITE_METHODS = new Set([ "deleteMany", ]); +// Client-level raw/escape hatches that MUST be blocked on a read-only +// context. Missing any one of these lets a read-tool smuggle writes via +// raw SQL, transactions, or the Mongo-style runCommandRaw. +const BLOCKED_CLIENT_METHODS = new Set([ + "$executeRaw", + "$executeRawUnsafe", + "$transaction", + "$queryRawUnsafe", + "$runCommandRaw", +]); + function readOnlyModelProxy(model: Record, modelName: string): unknown { return new Proxy(model, { get(target, prop) { @@ -43,11 +54,14 @@ export function createReadOnlyProxy(client: PrismaClient): PrismaClient { if (value && typeof value === "object" && "findMany" in (value as Record)) { return readOnlyModelProxy(value as Record, String(prop)); } - // Block $executeRaw and $executeRawUnsafe at the client level - if (prop === "$executeRaw" || prop === "$executeRawUnsafe") { + // Block raw/escape-hatch methods at the client level. $queryRaw + // (template-tagged) is allowed — it's read-only by API contract; + // $queryRawUnsafe is blocked because a crafted string could be + // used to smuggle DDL/DML. + if (typeof prop === "string" && BLOCKED_CLIENT_METHODS.has(prop)) { return () => { throw new Error( - `Raw write operation "${String(prop)}" not permitted on read-only context`, + `Raw/escape operation "${String(prop)}" not permitted on read-only context`, ); }; } diff --git a/packages/api/src/router/assistant-tools.ts b/packages/api/src/router/assistant-tools.ts index c996482..d690afe 100644 --- a/packages/api/src/router/assistant-tools.ts +++ b/packages/api/src/router/assistant-tools.ts @@ -334,6 +334,7 @@ export const MUTATION_TOOLS = new Set([ "delete_reminder", "delete_notification", "assign_task", + "create_estimate", "clone_estimate", "update_estimate_draft", "submit_estimate_version", diff --git a/packages/api/src/router/assistant-tools/helpers.ts b/packages/api/src/router/assistant-tools/helpers.ts index 89e3210..59d573b 100644 --- a/packages/api/src/router/assistant-tools/helpers.ts +++ b/packages/api/src/router/assistant-tools/helpers.ts @@ -1672,6 +1672,11 @@ export function createScopedCallerContext(ctx: ToolContext): TRPCContext { throw new AssistantVisibleError("Authenticated assistant context is required for this tool."); } + // Propagate the read-only db client to the scoped tRPC caller so any + // mutation reached through the caller is blocked at the proxy layer. + // Previously we passed `ctx.db` verbatim — if the caller received + // `ctx.isReadOnly=true` but we forwarded a raw client, reflection + // through the caller would bypass the guarantee (C-3/C-4). return { session: ctx.session, db: ctx.db, diff --git a/packages/api/src/router/assistant-tools/shared.ts b/packages/api/src/router/assistant-tools/shared.ts index c819e7a..f6b42a4 100644 --- a/packages/api/src/router/assistant-tools/shared.ts +++ b/packages/api/src/router/assistant-tools/shared.ts @@ -11,6 +11,13 @@ export type ToolContext = { session?: TRPCContext["session"]; dbUser?: TRPCContext["dbUser"]; roleDefaults?: TRPCContext["roleDefaults"]; + /** + * If true, the ctx.db passed in is already wrapped by + * `createReadOnlyProxy` and any scoped tRPC caller the tool spawns + * MUST also receive the proxied client — otherwise a read-only tool + * can smuggle writes through a tRPC caller that bypasses the proxy. + */ + isReadOnly?: boolean; }; export interface ToolAccessRequirements {