fix(policy): resolve this.relation.field against @@allow model, unnest array columns in subqueries#2734
Open
hechang27-sprt wants to merge 4 commits into
Conversation
…t array columns in subqueries Fix zenstackhq#1: In _member() isThis branch, pass modelOrType: context.thisType to transformRelationAccess() so that this.relation.field correctly resolves relation fields against the @@Allow model (thisType) rather than the collection predicate's model (modelOrType). Fix zenstackhq#2: In _member() when the innermost field of a subquery selection is an array type AND the provider is PostgreSQL, wrap the column with unnest() so that ANY(subquery) receives scalar rows instead of array rows. Other providers (SQLite, MySQL) leave the column as-is since they store arrays differently (JSON) and would need different handling.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPolicy expression resolution now handles nested ChangesPolicy expression resolution and array IN
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
from
1965841 to
cfbc2ce
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 1073-1082: The buildArrayInExists path is reconstructing a new
select from only the first table and blindly reading subquery.where, which
breaks many-to-many relation queries that carry correlation in joins and may
have no where clause. Update buildArrayInExists to work from the existing
SelectQueryNode shape, preserving any joins and other query parts instead of
using froms[0] alone, and append the containment predicate to the incoming
subquery rather than dereferencing subquery.where! or rebuilding the query from
scratch.
- Around line 712-715: The first `this` relation hop in
`expression-transformer.ts` is still carrying `restContext.alias`, which can
resolve to the wrong table alias in nested policy predicates. Update the
`transformRelationAccess` call in this branch so that when `modelOrType` is
switched back to `context.thisType`, the alias also uses `context.thisAlias` for
consistency with the one-segment `this.foo` handling, keeping the outer
`@@allow` model reference stable.
- Around line 257-262: The array-subquery branch in expression-transformer is
returning early before the enum scalar cast logic runs, so enum comparisons skip
the same normalization used by the non-subquery any(...) path. Update the
handling in ExpressionTransformer around buildArrayInExists so the right-hand
expression is still passed through the existing enum scalar casting logic before
routing to buildArrayInExists, keeping the cast behavior consistent for
array-column subqueries and regular any(...) comparisons.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f6aeb4a-8549-4cdc-82d9-5aa6b985dfd8
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/e2e/orm/policy/auth-access.test.ts
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
2 times, most recently
from
da34156 to
34d86b9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/plugins/policy/src/expression-transformer.ts (1)
1083-1128: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftPreserve joins when rebuilding the non-PostgreSQL
EXISTS.Line 1127 creates a fresh
selectFrom(tableName), so anysubquery.joinsfrom relation access—especially many-to-many joins fromtransformManyToManyRelationAccess—are dropped. That can make SQLite/MySQL array membership checks uncorrelated or incorrect even thoughsubquery.whereis merged.Verification script
#!/bin/bash # Inspect relation subquery shapes and generated array-in helper call sites. ast-grep outline packages/plugins/policy/src/expression-transformer.ts --match ExpressionTransformer --view expanded rg -n -C4 'buildArrayInExists|transformManyToManyRelationAccess|subquery\.joins|selectFrom\(tableName\)' packages/plugins/policy/src/expression-transformer.ts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/policy/src/expression-transformer.ts` around lines 1083 - 1128, The non-PostgreSQL EXISTS rebuild in expression-transformer drops relation joins by creating a new selectFrom(tableName), which breaks many-to-many and other correlated array checks. Update the logic around buildArrayInExists so it reuses the existing subquery shape, including subquery.joins, when constructing the final EXISTS for SQLite/MySQL, while still merging subquery.where with the array membership predicate. Keep the original table/alias handling from transformManyToManyRelationAccess and preserve all join conditions instead of starting a fresh root query.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 1083-1128: The non-PostgreSQL EXISTS rebuild in
expression-transformer drops relation joins by creating a new
selectFrom(tableName), which breaks many-to-many and other correlated array
checks. Update the logic around buildArrayInExists so it reuses the existing
subquery shape, including subquery.joins, when constructing the final EXISTS for
SQLite/MySQL, while still merging subquery.where with the array membership
predicate. Keep the original table/alias handling from
transformManyToManyRelationAccess and preserve all join conditions instead of
starting a fresh root query.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e26234ba-921d-477d-8c75-4d3c1efeac5c
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/e2e/orm/policy/auth-access.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/orm/policy/auth-access.test.ts
…-db array-in EXISTS Fixes zenstackhq#2733. Fix 1: `_member` isThis branch now passes `modelOrType: context.thisType` to `transformRelationAccess`, so `this.relation.field` resolves against the @@Allow model instead of the collection predicate model. Fix 2: `this.scalarField in this.relation.arrayField` now generates a cross-database EXISTS subquery instead of `= ANY(subquery)`. Each provider uses its native array operator: - PostgreSQL: EXISTS (SELECT 1 FROM t WHERE join AND scalar = ANY(t.col)) - SQLite: EXISTS (... AND EXISTS (SELECT 1 FROM json_each(t.col) WHERE value = scalar)) - MySQL: EXISTS (... AND EXISTS (SELECT 1 FROM JSON_TABLE(t.col, ...) AS jt WHERE jt.value = scalar)) Also fixes `getFieldDefFromFieldRef` to resolve multi-segment `this.relation.field` chains, needed to detect array fields in the `in`-operator RHS. Tests: two new PG tests in auth-access.test.ts, verified to fail without the fix and pass with it.
hechang27-sprt
force-pushed
the
fix/this-authscope-relation-resolution
branch
from
34d86b9 to
9e78de7
Compare
Author
|
Pushed commit 29a60bb to branch fix/this-authscope-relation-resolution. What changed:
Verification:
So the new fix is necessary, and those two broader-suite failures are not reproduced by simply removing this patch alone. |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/plugins/policy/src/expression-transformer.ts (1)
330-338: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winCompare every relation id field, or reject composite-id relations.
requireIdFields(...)can return multiple fields, but this normalizes relation equality to onlyidFields[0]. With composite ids, policy relation comparisons can match on one key and over-authorize records. Prefer expanding equality across all id fields, or fail closed for composite-id relation comparisons.Safer minimal guard
const idFields = QueryUtils.requireIdFields(this.schema, leftRelDef.type); +if (idFields.length !== 1) { + throw createUnsupportedError('Relation comparison with composite id fields is not supported'); +} normalizedLeft = this.makeOrAppendMember(normalizedLeft, idFields[0]!);Apply the same guard on the right-hand relation branch.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/policy/src/expression-transformer.ts` around lines 330 - 338, The relation-equality normalization in expression-transformer currently uses only idFields[0] from QueryUtils.requireIdFields, which is unsafe for composite IDs. Update the handling around isRelationField and makeOrAppendMember so relation comparisons either expand across every ID field returned by requireIdFields or reject composite-id relations outright, and apply the same fail-closed guard to the right-hand relation branch as well.
♻️ Duplicate comments (1)
packages/plugins/policy/src/expression-transformer.ts (1)
1147-1153: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick winPreserve the original subquery joins in the EXISTS rewrite.
The rebuilt
SelectQueryNodeonly keepsfrom,where, andselections, so many-to-many relation subqueries lose theirinnerJoincorrelation. On SQLite/MySQL this can turn an array-membership policy into an uncorrelated globalEXISTS.Preserve the incoming query shape
return UnaryOperationNode.create( { - kind: 'SelectQueryNode', - from: subquery.from, + ...subquery, where: WhereNode.create(combinedWhere), selections: [SelectionNode.create(AliasNode.create(ValueNode.createImmediate(1), IdentifierNode.create('_')))], } as SelectQueryNode, OperatorNode.create('exists'), );#!/bin/bash # Verify that many-to-many relation access creates joins and buildArrayInExists preserves them. sed -n '916,936p' packages/plugins/policy/src/expression-transformer.ts sed -n '1141,1155p' packages/plugins/policy/src/expression-transformer.ts rg -n "joins\\??:|interface SelectQueryNode|type SelectQueryNode" .🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/plugins/policy/src/expression-transformer.ts` around lines 1147 - 1153, The EXISTS rewrite in the subquery builder is dropping query shape by reconstructing SelectQueryNode with only from, where, and selections. Update the rewrite in buildArrayInExists/expression-transformer to preserve the incoming subquery properties, especially innerJoin (and any other relevant query fields), while only replacing the where and selections needed for EXISTS. Use the existing subquery object as the base so many-to-many relation correlation survives on SQLite/MySQL.
🧹 Nitpick comments (2)
tests/e2e/orm/policy/auth-access.test.ts (2)
535-568: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the SQLite/MySQL array-subquery branches.
This test uses PostgreSQL only, but the implementation adds SQLite/MySQL-specific
json_each/JSON_TABLESQL. Add provider coverage here so those branches don’t regress silently.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/orm/policy/auth-access.test.ts` around lines 535 - 568, The current auth-access test only covers PostgreSQL for the this.relation.arrayField in operator case, so the SQLite/MySQL-specific json_each and JSON_TABLE branches are untested. Update the createPolicyTestClient setup in auth-access.test.ts and the related test case to run under SQLite and MySQL as well (using the same model/auth().permissions?[p, this.id in p.allowedDocIds] path), so these provider-specific SQL branches are exercised and guarded against regressions.
675-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a denial assertion for the auth-scope predicate.
The new test proves the intended ancestor assignment grants access, but it would still pass if the policy overgranted all authenticated users. Add a no-assignment or unrelated-scope reader expectation that resolves null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/orm/policy/auth-access.test.ts` around lines 675 - 680, Add a denial check in the auth-access test so it verifies the auth-scope predicate is actually enforced, not just that an ancestor assignment works. In the same reader setup around db.$setAuth and reader.doc.findUnique, add a second expectation using a reader with no assignments or an unrelated scope that resolves null. Keep the existing granted-access assertion for the intended ancestor case and ensure the new negative case uses the same findUnique path to validate the policy behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 330-338: The relation-equality normalization in
expression-transformer currently uses only idFields[0] from
QueryUtils.requireIdFields, which is unsafe for composite IDs. Update the
handling around isRelationField and makeOrAppendMember so relation comparisons
either expand across every ID field returned by requireIdFields or reject
composite-id relations outright, and apply the same fail-closed guard to the
right-hand relation branch as well.
---
Duplicate comments:
In `@packages/plugins/policy/src/expression-transformer.ts`:
- Around line 1147-1153: The EXISTS rewrite in the subquery builder is dropping
query shape by reconstructing SelectQueryNode with only from, where, and
selections. Update the rewrite in buildArrayInExists/expression-transformer to
preserve the incoming subquery properties, especially innerJoin (and any other
relevant query fields), while only replacing the where and selections needed for
EXISTS. Use the existing subquery object as the base so many-to-many relation
correlation survives on SQLite/MySQL.
---
Nitpick comments:
In `@tests/e2e/orm/policy/auth-access.test.ts`:
- Around line 535-568: The current auth-access test only covers PostgreSQL for
the this.relation.arrayField in operator case, so the SQLite/MySQL-specific
json_each and JSON_TABLE branches are untested. Update the
createPolicyTestClient setup in auth-access.test.ts and the related test case to
run under SQLite and MySQL as well (using the same model/auth().permissions?[p,
this.id in p.allowedDocIds] path), so these provider-specific SQL branches are
exercised and guarded against regressions.
- Around line 675-680: Add a denial check in the auth-access test so it verifies
the auth-scope predicate is actually enforced, not just that an ancestor
assignment works. In the same reader setup around db.$setAuth and
reader.doc.findUnique, add a second expectation using a reader with no
assignments or an unrelated scope that resolves null. Keep the existing
granted-access assertion for the intended ancestor case and ensure the new
negative case uses the same findUnique path to validate the policy behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d644c5f-6492-449a-9fe6-0055a687d37f
📒 Files selected for processing (2)
packages/plugins/policy/src/expression-transformer.tstests/e2e/orm/policy/auth-access.test.ts
Author
|
Addressed the latest CodeRabbit finding in commit 9992d72. Change:
This preserves joins/correlation from relation access subqueries, including the many-to-many path CodeRabbit called out, and also avoids assuming Verification:
I tried to add a dedicated m2m regression for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2733.
Problem
Two bugs in the policy plugin expression transformer:
this.relation.fieldresolves against the wrong model insideauth().collection?[...]predicates. The expression transformer usescontext.thisTypeto resolve the field definition but passesrestContext(withmodelOrTypestill pointing to the collection model) totransformRelationAccess. This causesField "X" not found in model "<collection>"crashes.x in this.relation.arrayFieldgenerates broken SQL on PostgreSQL. The expression compiles to= ANY((SELECT arrayCol FROM ...))where the subquery returns array-typed rows (integer[]). PostgreSQLANY(subquery)requires scalar rows, producingoperator does not exist: integer = integer[].Fix
Fix 1: Resolve
this.relation.fieldagainst the@@allowmodelPass
modelOrType: context.thisTypeandalias: context.thisAliastotransformRelationAccesswhen the receiver isthis, so relation fields and table aliases resolve against the model bearing the policy. Also fixedgetFieldDefFromFieldRefto resolve multi-segmentthis.relation.fieldchains (needed for the array-field detection in Fix 2).Fix 2: Cross-database
scalar IN relation.arrayFieldSingle entry point
buildArrayInExistsin_binary, with provider-specific handling:unnest()in subquery column += ANY(subquery)scalar = ANY(SELECT unnest("col") FROM "t" WHERE <join>)json_eachEXISTS (SELECT 1 FROM "t" WHERE <join> AND EXISTS (SELECT 1 FROM json_each("t"."col") WHERE "value" = scalar))JSON_TABLEEXISTS (SELECT 1 FROM "t" WHERE <join> AND EXISTS (SELECT 1 FROM JSON_TABLE(...) AS jt WHERE jt."value" = scalar))The SQLite/MySQL
EXISTSrebuild preserves the original subqueryFROMclause, so many-to-many joins are kept intact.Verification
Two new PG-specific tests added to
auth-access.test.ts:this.author.levelinauth().permissions?[p, ...]this.id in this.group.visibleDocIdsAll existing policy tests continue to pass on PostgreSQL, SQLite, and MySQL (no regressions).
Summary by CodeRabbit
New Features
inchecks against array-based subqueries across databases.this-based relation and collection references in authorization conditions.Bug Fixes