From f4ea61ea26e4dc6a1ebbaa5e097006c11a170d42 Mon Sep 17 00:00:00 2001 From: evgenovalov Date: Fri, 30 Oct 2026 17:42:34 +0100 Subject: [PATCH] fix(policy): check row-dependent create policies for the whole batch in one query When the create policy references the inserted row, the pre-create check ran one SELECT EXISTS(...) per row (#2842 only skipped the constant case), so createMany of N rows cost N sequential round trips. Check all rows in one query by counting the rows that satisfy the filter and comparing the count with the number of rows. A row whose filter evaluates to NULL still rejects the batch. DEFAULT cells and Postgres list values are now bound the way the INSERT binds them, so the batched check stays within the INSERT's bind parameter budget. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01RwcqsRcacQHX18hXKbJN4n --- packages/plugins/policy/src/policy-handler.ts | 100 ++++++------------ tests/regression/test/issue-2841.test.ts | 30 ++++-- 2 files changed, 57 insertions(+), 73 deletions(-) diff --git a/packages/plugins/policy/src/policy-handler.ts b/packages/plugins/policy/src/policy-handler.ts index ddc58ee7a..9754ca422 100644 --- a/packages/plugins/policy/src/policy-handler.ts +++ b/packages/plugins/policy/src/policy-handler.ts @@ -38,7 +38,6 @@ import { ValueNode, ValuesNode, WhereNode, - type Expression as KyselyExpression, type OperationNode, type QueryResult, type RootOperationNode, @@ -968,32 +967,30 @@ export class PolicyHandler extends OperationNodeTransf ? this.unwrapCreateValueRows(node.values, mutationModel, fields, isManyToManyJoinTable) : [[]]; - if (!isManyToManyJoinTable) { - // A create filter built only from `auth()` doesn't reference the row being inserted, - // so it holds for every row alike and checking it per row re-asks the same question. - const filter = this.buildPolicyFilter(mutationModel, undefined, 'create'); - if (isTrueNode(filter)) { - return; - } - } - - for (const values of valueRows) { - if (isManyToManyJoinTable) { + if (isManyToManyJoinTable) { + for (const values of valueRows) { await this.enforcePreCreatePolicyForManyToManyJoinTable( mutationModel, fields, values.map((v) => v.node), proceed, ); - } else { - await this.enforcePreCreatePolicyForOne( - mutationModel, - fields, - values.map((v) => v.node), - proceed, - ); } + return; + } + + const filter = this.buildPolicyFilter(mutationModel, undefined, 'create'); + if (isTrueNode(filter)) { + return; } + + await this.enforcePreCreatePolicyForRows( + mutationModel, + fields, + valueRows.map((values) => values.map((v) => v.node)), + filter, + proceed, + ); } private async enforcePreCreatePolicyForManyToManyJoinTable( @@ -1059,52 +1056,33 @@ export class PolicyHandler extends OperationNodeTransf } } - private async enforcePreCreatePolicyForOne( + private async enforcePreCreatePolicyForRows( model: string, fields: string[], - values: OperationNode[], + rows: OperationNode[][], + filter: OperationNode, proceed: ProceedKyselyQueryFunction, ) { const allFields = QueryUtils.getModelFields(this.client.$schema, model, { inherited: true }); - const allValues: KyselyExpression[] = []; + const fieldIndices = allFields.map((def) => fields.indexOf(def.name)); - for (const def of allFields) { - const index = fields.indexOf(def.name); - if (index >= 0) { - allValues.push(new ExpressionWrapper(values[index]!)); - } else { - // set non-provided fields to null - allValues.push(this.eb.lit(null)); - } - } - - // create a `SELECT column1 as field1, column2 as field2, ... FROM (VALUES (...))` table for policy evaluation - const valuesTable = this.dialect.buildValuesTableSelect(allFields, [allValues]); - - const filter = this.buildPolicyFilter(model, undefined, 'create'); + // NULL literals rather than parameters: the check has to fit the INSERT's parameter limit + const allRows = rows.map((values) => + fieldIndices.map((index) => (index >= 0 ? new ExpressionWrapper(values[index]!) : this.eb.lit(null))), + ); - // check if the provided values satisfy the create policy + // create a `SELECT column1 as field1, column2 as field2, ... FROM (VALUES (...), ...)` table for policy evaluation + const valuesTable = this.dialect.buildValuesTableSelect(allFields, allRows); - // `SELECT 1 FROM (VALUES (...)) AS t(column1, column2, ...) WHERE ` - const preCreateInner = this.eb + // Every row must satisfy the filter. Counting the rows that do (rather than looking for one + // that doesn't) also rejects rows whose filter evaluates to NULL. + // `SELECT COUNT(1) = AS $condition FROM (VALUES ...) AS t(column1, column2, ...) WHERE ` + const preCreateQuery = this.eb .selectFrom(valuesTable.as(model)) - .select(this.eb.lit(1).as('_')) + .select(this.eb(this.eb.fn('COUNT', [this.eb.lit(1)]), '=', this.eb.lit(rows.length)).as('$condition')) .where(() => new ExpressionWrapper(filter)); - const result = await proceed( - // `SELECT EXISTS(preCreateInner) AS $condition` - { - kind: 'SelectQueryNode', - selections: [ - SelectionNode.create( - AliasNode.create( - this.eb.exists(preCreateInner).toOperationNode(), - IdentifierNode.create('$condition'), - ), - ), - ], - } satisfies SelectQueryNode, - ); + const result = await proceed(preCreateQuery.toOperationNode()); if (!result.rows[0]?.$condition) { throw createRejectedByPolicyError(model, RejectedByPolicyReason.NO_ACCESS); } @@ -1145,7 +1123,8 @@ export class PolicyHandler extends OperationNodeTransf if (itemsAreNodes) { const itemNode = item as OperationNode; if (DefaultInsertValueNode.is(itemNode)) { - result.push({ node: ValueNode.create(null), raw: null }); + // inlined like the DEFAULT keyword it replaces, so it binds no parameter + result.push({ node: ValueNode.createImmediate(null), raw: null }); continue; } const fieldDef = QueryUtils.requireField(this.client.$schema, model, fields[i]!); @@ -1167,16 +1146,7 @@ export class PolicyHandler extends OperationNodeTransf value = this.dialect.transformInput(item, fieldDef.type as BuiltinType, !!fieldDef.array); } - // handle the case for list column - if (Array.isArray(value)) { - const fieldDef = QueryUtils.requireField(this.client.$schema, model, fields[i]!); - result.push({ - node: this.dialect.buildArrayValue(value, fieldDef.type).toOperationNode(), - raw: value, - }); - } else { - result.push({ node: ValueNode.create(value), raw: value }); - } + result.push({ node: ValueNode.create(value), raw: value }); } } return result; diff --git a/tests/regression/test/issue-2841.test.ts b/tests/regression/test/issue-2841.test.ts index e0acca7c9..e9a4f596a 100644 --- a/tests/regression/test/issue-2841.test.ts +++ b/tests/regression/test/issue-2841.test.ts @@ -20,8 +20,8 @@ model AuditLog { } model Note { - id String @id @default(cuid()) - ownerId String + id String @id @default(cuid()) + ownerId String? @@allow('create', ownerId == auth().id) @@allow('read', true) @@ -69,23 +69,37 @@ model Note { await expect(db.auditLog.findMany()).resolves.toHaveLength(0); }); - it('still checks each row when the create filter references the row', async () => { + it('checks a row-dependent create filter for the whole batch in one query', async () => { const { db, policyChecks } = await createClient(); const authDb = db.$setAuth({ id: 'u1', isSystem: true }); await expect( authDb.note.createMany({ - data: [{ ownerId: 'u1' }, { ownerId: 'u1' }], + data: Array.from({ length: 5 }, () => ({ ownerId: 'u1' })), }), - ).resolves.toMatchObject({ count: 2 }); - expect(policyChecks()).toHaveLength(2); + ).resolves.toMatchObject({ count: 5 }); + expect(policyChecks()).toHaveLength(1); // a single offending row still rejects the whole batch await expect( authDb.note.createMany({ - data: [{ ownerId: 'u1' }, { ownerId: 'someone-else' }], + data: [{ ownerId: 'u1' }, { ownerId: 'someone-else' }, { ownerId: 'u1' }], + }), + ).rejects.toThrow(/rejected by access policies/i); + expect(policyChecks()).toHaveLength(2); + await expect(db.note.findMany()).resolves.toHaveLength(5); + }); + + it('rejects the batch when the create filter evaluates to NULL for a row', async () => { + const { db } = await createClient(); + const authDb = db.$setAuth({ id: 'u1', isSystem: true }); + + // `NULL = 'u1'` is neither true nor false + await expect( + authDb.note.createMany({ + data: [{ ownerId: 'u1' }, { ownerId: null }], }), ).rejects.toThrow(/rejected by access policies/i); - await expect(db.note.findMany()).resolves.toHaveLength(2); + await expect(db.note.findMany()).resolves.toHaveLength(0); }); });