Conversation
Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com>
…t array columns in subqueries (#2734) Co-authored-by: ymc9 <104139426+ymc9@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAll packages are bumped from version ChangesPolicy Expression Transformer Fixes
Version Bump 3.8.1 → 3.8.2
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
340-348: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDon’t silently compare only the first id for composite relations.
idFields[0]makes direct relation comparisons ignore the remaining key columns for models with composite ids, which can over-authorize policy checks. Either expand relation equality across all id fields or reject composite-id relation comparisons here.Minimal guard to avoid unsafe partial comparisons
const idFields = QueryUtils.requireIdFields(this.schema, leftRelDef.type); +invariant( + idFields.length === 1, + `direct relation comparison for composite id model "${leftRelDef.type}" must compare all id fields`, +); normalizedLeft = this.makeOrAppendMember(normalizedLeft, idFields[0]!);const idFields = QueryUtils.requireIdFields(this.schema, rightRelDef.type); +invariant( + idFields.length === 1, + `direct relation comparison for composite id model "${rightRelDef.type}" must compare all id fields`, +); normalizedRight = this.makeOrAppendMember(normalizedRight, idFields[0]!);🤖 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 340 - 348, The relation comparison logic in expression-transformer’s expression normalization is only using the first identifier field via QueryUtils.requireIdFields and makeOrAppendMember, which silently drops the बाकी composite key columns. Update the handling in this comparison path to either expand expr.left/expr.right normalization across all id fields for relation equality or explicitly reject composite-id relation comparisons here so policy checks cannot use partial keys.
🧹 Nitpick comments (1)
tests/e2e/orm/policy/auth-access.test.ts (1)
605-678: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a denied assertion for the hierarchy policy.
This regression test only proves the happy path. Add an unrelated assignment/scope case that must not resolve the doc, so an over-broad relation comparison or always-true policy can’t pass.
🤖 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 605 - 678, The auth-access regression test for the Doc hierarchy policy only covers the allowed path, so it can miss an overly broad predicate. In the existing test block around createPolicyTestClient and reader.doc.findUnique, add a separate negative case using a different user/assignment/scope combination that should not match the authScope ancestry check, and assert the doc lookup does not resolve. Keep the new assertion alongside the current happy-path setup so the @@allow on Doc and the auth().assignments predicate are both exercised for denial as well.
🤖 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 340-348: The relation comparison logic in expression-transformer’s
expression normalization is only using the first identifier field via
QueryUtils.requireIdFields and makeOrAppendMember, which silently drops the बाकी
composite key columns. Update the handling in this comparison path to either
expand expr.left/expr.right normalization across all id fields for relation
equality or explicitly reject composite-id relation comparisons here so policy
checks cannot use partial keys.
---
Nitpick comments:
In `@tests/e2e/orm/policy/auth-access.test.ts`:
- Around line 605-678: The auth-access regression test for the Doc hierarchy
policy only covers the allowed path, so it can miss an overly broad predicate.
In the existing test block around createPolicyTestClient and
reader.doc.findUnique, add a separate negative case using a different
user/assignment/scope combination that should not match the authScope ancestry
check, and assert the doc lookup does not resolve. Keep the new assertion
alongside the current happy-path setup so the @@allow on Doc and the
auth().assignments predicate are both exercised for denial as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b4a68a88-9c62-4afb-914d-842fefbf21cd
📒 Files selected for processing (30)
package.jsonpackages/auth-adapters/better-auth/package.jsonpackages/cli/package.jsonpackages/clients/client-helpers/package.jsonpackages/clients/fetch-client/package.jsonpackages/clients/tanstack-query/package.jsonpackages/common-helpers/package.jsonpackages/config/eslint-config/package.jsonpackages/config/tsdown-config/package.jsonpackages/config/typescript-config/package.jsonpackages/config/vitest-config/package.jsonpackages/create-zenstack/package.jsonpackages/ide/vscode/package.jsonpackages/language/package.jsonpackages/orm/package.jsonpackages/plugins/policy/package.jsonpackages/plugins/policy/src/expression-transformer.tspackages/plugins/soft-delete/package.jsonpackages/schema/package.jsonpackages/sdk/package.jsonpackages/server/package.jsonpackages/testtools/package.jsonpackages/zod/package.jsonsamples/orm/package.jsonsamples/taskforge/package.jsontests/e2e/orm/policy/auth-access.test.tstests/e2e/package.jsontests/regression/package.jsontests/runtimes/bun/package.jsontests/runtimes/edge-runtime/package.json
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
2 participants
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.
Summary by CodeRabbit
Bug Fixes
thisreferences, andinchecks.Chores