Skip to content

Commit 8e93a49

Browse files
ymc9ymc9
authored
fix(policy): incorrect field-level permission check for nested create (#2011)
1 parent 767d59d commit 8e93a49

File tree

4 files changed

+153
-36
lines changed

4 files changed

+153
-36
lines changed

‎packages/runtime/src/enhancements/node/policy/handler.ts

Lines changed: 38 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -284,9 +284,16 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
284284
if (context.field?.backLink) {
285285
const backLinkField = resolveField(this.modelMeta, model, context.field.backLink);
286286
if (backLinkField?.isRelationOwner) {
287-
// the target side of relation owns the relation,
288-
// check if it's updatable
289-
await this.policyUtils.checkPolicyForUnique(model, args.where, 'update', db, args);
287+
// "connect" is actually "update" to foreign keys, so we need to map the "connect" payload
288+
// to "update" payload by translating pk to fks, and use that to check update policies
289+
const fieldsToUpdate = Object.values(backLinkField.foreignKeyMapping ?? {});
290+
await this.policyUtils.checkPolicyForUnique(
291+
model,
292+
args.where,
293+
'update',
294+
db,
295+
fieldsToUpdate
296+
);
290297
}
291298
}
292299

@@ -319,9 +326,12 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
319326
// check existence
320327
await this.policyUtils.checkExistence(db, model, args, true);
321328

322-
// the target side of relation owns the relation,
323-
// check if it's updatable
324-
await this.policyUtils.checkPolicyForUnique(model, args, 'update', db, args);
329+
// the target side of relation owns the relation, check if it's updatable
330+
331+
// "connect" is actually "update" to foreign keys, so we need to map the "connect" payload
332+
// to "update" payload by translating pk to fks, and use that to check update policies
333+
const fieldsToUpdate = Object.values(backLinkField.foreignKeyMapping ?? {});
334+
await this.policyUtils.checkPolicyForUnique(model, args, 'update', db, fieldsToUpdate);
325335
}
326336
}
327337
},
@@ -909,21 +919,11 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
909919
}
910920

911921
// update happens on the related model, require updatable,
912-
// translate args to foreign keys so field-level policies can be checked
913-
const checkArgs: any = {};
914-
if (args && typeof args === 'object' && backLinkField.foreignKeyMapping) {
915-
for (const key of Object.keys(args)) {
916-
const fk = backLinkField.foreignKeyMapping[key];
917-
if (fk) {
918-
checkArgs[fk] = args[key];
919-
}
920-
}
921-
}
922-
923922
// `uniqueFilter` can be undefined if the entity to be disconnected doesn't exist
924923
if (uniqueFilter) {
925-
// check for update
926-
await this.policyUtils.checkPolicyForUnique(model, uniqueFilter, 'update', db, checkArgs);
924+
// check for update, "connect" and "disconnect" are actually "update" to foreign keys
925+
const fieldsToUpdate = Object.values(backLinkField.foreignKeyMapping ?? {});
926+
await this.policyUtils.checkPolicyForUnique(model, uniqueFilter, 'update', db, fieldsToUpdate);
927927

928928
// register post-update check
929929
await _registerPostUpdateCheck(model, uniqueFilter, uniqueFilter);
@@ -971,12 +971,18 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
971971
this.policyUtils.tryReject(db, this.model, 'update');
972972

973973
// check pre-update guard
974-
await this.policyUtils.checkPolicyForUnique(model, uniqueFilter, 'update', db, args);
974+
await this.policyUtils.checkPolicyForUnique(
975+
model,
976+
uniqueFilter,
977+
'update',
978+
db,
979+
this.queryUtils.getFieldsWithDefinedValues(updatePayload)
980+
);
975981

976982
// handle the case where id fields are updated
977983
const _args: any = args;
978-
const updatePayload = _args.data && typeof _args.data === 'object' ? _args.data : _args;
979-
const postUpdateIds = this.calculatePostUpdateIds(model, existing, updatePayload);
984+
const checkPayload = _args.data && typeof _args.data === 'object' ? _args.data : _args;
985+
const postUpdateIds = this.calculatePostUpdateIds(model, existing, checkPayload);
980986

981987
// register post-update check
982988
await _registerPostUpdateCheck(model, existing, postUpdateIds);
@@ -1068,7 +1074,13 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
10681074
// update case
10691075

10701076
// check pre-update guard
1071-
await this.policyUtils.checkPolicyForUnique(model, existing, 'update', db, args);
1077+
await this.policyUtils.checkPolicyForUnique(
1078+
model,
1079+
existing,
1080+
'update',
1081+
db,
1082+
this.queryUtils.getFieldsWithDefinedValues(args.update)
1083+
);
10721084

10731085
// handle the case where id fields are updated
10741086
const postUpdateIds = this.calculatePostUpdateIds(model, existing, args.update);
@@ -1156,7 +1168,7 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
11561168
await this.policyUtils.checkExistence(db, model, uniqueFilter, true);
11571169

11581170
// check delete guard
1159-
await this.policyUtils.checkPolicyForUnique(model, uniqueFilter, 'delete', db, args);
1171+
await this.policyUtils.checkPolicyForUnique(model, uniqueFilter, 'delete', db, []);
11601172
},
11611173

11621174
deleteMany: async (model, args, context) => {
@@ -1526,7 +1538,7 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
15261538
await this.policyUtils.checkExistence(tx, this.model, args.where, true);
15271539

15281540
// inject delete guard
1529-
await this.policyUtils.checkPolicyForUnique(this.model, args.where, 'delete', tx, args);
1541+
await this.policyUtils.checkPolicyForUnique(this.model, args.where, 'delete', tx, []);
15301542

15311543
// proceed with the deletion
15321544
if (this.shouldLogQuery) {
@@ -1773,7 +1785,7 @@ export class PolicyProxyHandler<DbClient extends DbClientContract> implements Pr
17731785
private async runPostWriteChecks(postWriteChecks: PostWriteCheckRecord[], db: CrudContract) {
17741786
await Promise.all(
17751787
postWriteChecks.map(async ({ model, operation, uniqueFilter, preValue }) =>
1776-
this.policyUtils.checkPolicyForUnique(model, uniqueFilter, operation, db, undefined, preValue)
1788+
this.policyUtils.checkPolicyForUnique(model, uniqueFilter, operation, db, [], preValue)
17771789
)
17781790
);
17791791
}

‎packages/runtime/src/enhancements/node/policy/policy-utils.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,11 @@ export class PolicyUtil extends QueryUtils {
451451

452452
if (operation === 'update' && args) {
453453
// merge field-level policy guards
454-
const fieldUpdateGuard = this.getFieldUpdateGuards(db, model, args);
454+
const fieldUpdateGuard = this.getFieldUpdateGuards(
455+
db,
456+
model,
457+
this.getFieldsWithDefinedValues(args.data ?? args)
458+
);
455459
if (fieldUpdateGuard.rejectedByField) {
456460
// rejected
457461
args.where = this.makeFalse();
@@ -834,7 +838,7 @@ export class PolicyUtil extends QueryUtils {
834838
uniqueFilter: any,
835839
operation: PolicyOperationKind,
836840
db: CrudContract,
837-
args: any,
841+
fieldsToUpdate: string[],
838842
preValue?: any
839843
) {
840844
let guard = this.getAuthGuard(db, model, operation, preValue);
@@ -849,9 +853,9 @@ export class PolicyUtil extends QueryUtils {
849853

850854
let entityChecker: EntityChecker | undefined;
851855

852-
if (operation === 'update' && args) {
856+
if (operation === 'update' && fieldsToUpdate.length > 0) {
853857
// merge field-level policy guards
854-
const fieldUpdateGuard = this.getFieldUpdateGuards(db, model, args);
858+
const fieldUpdateGuard = this.getFieldUpdateGuards(db, model, fieldsToUpdate);
855859
if (fieldUpdateGuard.rejectedByField) {
856860
// rejected
857861
throw this.deniedByPolicy(
@@ -989,16 +993,12 @@ export class PolicyUtil extends QueryUtils {
989993
return this.and(...allFieldGuards);
990994
}
991995

992-
private getFieldUpdateGuards(db: CrudContract, model: string, args: any) {
996+
private getFieldUpdateGuards(db: CrudContract, model: string, fieldsToUpdate: string[]) {
993997
const allFieldGuards = [];
994998
const allOverrideFieldGuards = [];
995999
let entityChecker: EntityChecker | undefined;
9961000

997-
for (const [field, value] of Object.entries<any>(args.data ?? args)) {
998-
if (typeof value === 'undefined') {
999-
continue;
1000-
}
1001-
1001+
for (const field of fieldsToUpdate) {
10021002
const fieldInfo = resolveField(this.modelMeta, model, field);
10031003

10041004
if (fieldInfo?.isDataModel) {

‎packages/runtime/src/enhancements/node/query-utils.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,4 +253,16 @@ export class QueryUtils {
253253

254254
return undefined;
255255
}
256+
257+
/**
258+
* Gets fields of object with defined values.
259+
*/
260+
getFieldsWithDefinedValues(data: object) {
261+
if (!data) {
262+
return [];
263+
}
264+
return Object.entries(data)
265+
.filter(([, v]) => v !== undefined)
266+
.map(([k]) => k);
267+
}
256268
}

‎tests/regression/tests/issue-2007.test.ts

Lines changed: 93 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,93 @@
1+
import { loadSchema } from '@zenstackhq/testtools';
2+
3+
describe('issue 2007', () => {
4+
it('regression1', async () => {
5+
const { enhance } = await loadSchema(
6+
`
7+
model Page {
8+
id String @id @default(cuid())
9+
title String
10+
11+
images Image[]
12+
13+
@@allow('all', true)
14+
}
15+
16+
model Image {
17+
id String @id @default(cuid()) @deny('update', true)
18+
url String
19+
pageId String?
20+
page Page? @relation(fields: [pageId], references: [id])
21+
22+
@@allow('all', true)
23+
}
24+
`
25+
);
26+
27+
const db = enhance();
28+
29+
const image = await db.image.create({
30+
data: {
31+
url: 'https://example.com/image.png',
32+
},
33+
});
34+
35+
await expect(
36+
db.image.update({
37+
where: { id: image.id },
38+
data: {
39+
page: {
40+
create: {
41+
title: 'Page 1',
42+
},
43+
},
44+
},
45+
})
46+
).toResolveTruthy();
47+
});
48+
49+
it('regression2', async () => {
50+
const { enhance } = await loadSchema(
51+
`
52+
model Page {
53+
id String @id @default(cuid())
54+
title String
55+
56+
images Image[]
57+
58+
@@allow('all', true)
59+
}
60+
61+
model Image {
62+
id String @id @default(cuid())
63+
url String
64+
pageId String? @deny('update', true)
65+
page Page? @relation(fields: [pageId], references: [id])
66+
67+
@@allow('all', true)
68+
}
69+
`
70+
);
71+
72+
const db = enhance();
73+
74+
const image = await db.image.create({
75+
data: {
76+
url: 'https://example.com/image.png',
77+
},
78+
});
79+
80+
await expect(
81+
db.image.update({
82+
where: { id: image.id },
83+
data: {
84+
page: {
85+
create: {
86+
title: 'Page 1',
87+
},
88+
},
89+
},
90+
})
91+
).toBeRejectedByPolicy();
92+
});
93+
});

0 commit comments

Comments
 (0)