fix(orm): use raw alias for join keys when PK/FK has field access policy#2675
Open
lsmith77 wants to merge 1 commit into
Open
fix(orm): use raw alias for join keys when PK/FK has field access policy#2675lsmith77 wants to merge 1 commit into
lsmith77 wants to merge 1 commit into
Conversation
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:
📝 WalkthroughWalkthroughAdds JOIN_KEY_RAW_PREFIX and helpers to detect conditional read policies, selects raw PK/FK aliases in policy-wrapped selects, updates join-pair and dialect join-filter code to use those raw aliases, and expands regression tests validating include behavior across five scenarios. ChangesFix
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
lsmith77
force-pushed
the
fix-deny-allow-joins
branch
from
022e5dd to
c109d4a
Compare
lsmith77
changed the title
lsmith77
force-pushed
the
fix-deny-allow-joins
branch
from
c109d4a to
531e8d1
Compare
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/regression/test/issue-2674.test.ts (1)
148-148: ⚡ Quick winAssert FK redaction across all included rows.
Checking only
comments[0]can miss partial leaks. Assert every included comment haspostId === nullin this scenario.♻️ Proposed test tightening
- expect(userResult?.comments[0]?.postId).toBeNull(); + expect(userResult?.comments.every((comment) => comment.postId === null)).toBe(true);🤖 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/regression/test/issue-2674.test.ts` at line 148, The test currently only asserts the first included comment's postId is null via expect(userResult?.comments[0]?.postId).toBeNull(); and can miss partial leaks—update the assertion to verify every included comment has postId === null by iterating over userResult?.comments (or using an array matcher) and asserting each comment.postId is null; reference the userResult variable and comments property in the test (replace the single-index check with a forEach/map-based assertion that fails if any comment has a non-null postId).
🤖 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.
Nitpick comments:
In `@tests/regression/test/issue-2674.test.ts`:
- Line 148: The test currently only asserts the first included comment's postId
is null via expect(userResult?.comments[0]?.postId).toBeNull(); and can miss
partial leaks—update the assertion to verify every included comment has postId
=== null by iterating over userResult?.comments (or using an array matcher) and
asserting each comment.postId is null; reference the userResult variable and
comments property in the test (replace the single-index check with a
forEach/map-based assertion that fails if any comment has a non-null postId).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7ecac85c-bd93-4695-aaac-a2c298cfbec0
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2674.test.ts
lsmith77
force-pushed
the
fix-deny-allow-joins
branch
3 times, most recently
from
3f814a8 to
b682b06
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@tests/regression/test/issue-2674.test.ts`:
- Around line 59-103: Add a positive-control assertion that the ADMIN principal
can resolve the relation before the USER checks: after creating post and comment
with db.$setAuth(admin) and before the USER query, call
db.$setAuth(admin).comment.findUnique({ where: { id: comment.id }, include: {
post: true } }) (or reuse an adminResult variable) and assert adminResult?.post
is not null (and optionally adminResult?.postId equals post.id); then proceed
with the existing userResult assertions so failures that affect all roles are
caught.
🪄 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: 817b55b5-eea1-47c4-af67-6414aacd2233
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2674.test.ts
lsmith77
force-pushed
the
fix-deny-allow-joins
branch
from
b682b06 to
2345079
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/regression/test/issue-2674.test.ts (2)
204-247: 💤 Low valueOptionally add a USER assertion for cross-role validation.
Since
@allow('read', true)should behave identically for all roles, adding a USER query would confirm that join resolution works correctly across different auth contexts when no policy wrapping occurs.🔍 Optional additional coverage
const result = await db.$setAuth(admin).post.findUnique({ where: { title: 'Test Post' }, include: { comments: true }, }); expect(result?.id).toBeGreaterThan(0); expect(result?.comments).toHaveLength(2); + + // USER should see identical behavior (allow read for all) + const user = { id: 'user-1', role: 'USER' }; + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeGreaterThan(0); + expect(userResult?.comments).toHaveLength(2); });🤖 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/regression/test/issue-2674.test.ts` around lines 204 - 247, Add a second assertion that runs the same create+findUnique flow under a non-admin user to verify `@allow`('read', true) behavior across roles: after the existing admin flow, call db.$setAuth(user) with a USER-like principal (e.g., { id: 'user-1', role: 'USER' }), perform the same post.findUnique({ where: { title: 'Test Post' }, include: { comments: true } }) and assert result?.id and result?.comments length match the admin results; update the test case named "'`@allow`(\"read\", true) on PK/FK should not emit a missing raw alias'" to include these USER checks referencing db.$setAuth, post.create, and post.findUnique so join resolution is validated for both roles.
159-202: ⚡ Quick winConsider adding a USER assertion to strengthen this test.
The test currently only validates ADMIN behavior. Adding a USER query would confirm that: (1) update-scoped denies truly don't interfere with reads for non-admin roles, and (2) join resolution works correctly for USER when no read-time CASE wrapping occurs.
📊 Proposed additional coverage
const result = await db.$setAuth(admin).post.findUnique({ where: { title: 'Test Post' }, include: { comments: true }, }); expect(result?.id).toBeGreaterThan(0); expect(result?.comments).toHaveLength(2); + + // USER should also see normal reads (update-deny does not block reads) + const user = { id: 'user-1', role: 'USER' }; + const userResult = await db.$setAuth(user).post.findUnique({ + where: { title: 'Test Post' }, + include: { comments: true }, + }); + expect(userResult?.id).toBeGreaterThan(0); + expect(userResult?.comments).toHaveLength(2); });🤖 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/regression/test/issue-2674.test.ts` around lines 159 - 202, Add a USER-path assertion to the existing test "update-only `@deny` on PK/FK should not affect read joins": after seeding as admin with createPolicyTestClient and the existing admin checks (admin variable, db.$setAuth(admin).post.create and post.findUnique include: { comments: true }), also set a non-admin user (e.g. user = { id: 'user-1', role: 'USER' }) and run db.$setAuth(user).post.findUnique with the same where/include to assert the user can read the post and sees two comments; this ensures update-scoped `@deny` on Post.id and Comment.postId does not alter read-time join resolution for non-admins.
🤖 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 `@tests/regression/test/issue-2674.test.ts`:
- Around line 113-157: Add a positive-control check that the ADMIN can fully
resolve the post and its comments before the USER assertions: after creating the
post (using db.$setAuth(admin).post.create) call
db.$setAuth(admin).post.findUnique with the same where: { title: 'Test Post' }
and include: { comments: true } and assert that the returned result has a
non-null id and comments length === 2; place this admin check immediately before
the existing USER findUnique/assertions to ensure the test fails if relation
resolution is broken for all roles.
---
Nitpick comments:
In `@tests/regression/test/issue-2674.test.ts`:
- Around line 204-247: Add a second assertion that runs the same
create+findUnique flow under a non-admin user to verify `@allow`('read', true)
behavior across roles: after the existing admin flow, call db.$setAuth(user)
with a USER-like principal (e.g., { id: 'user-1', role: 'USER' }), perform the
same post.findUnique({ where: { title: 'Test Post' }, include: { comments: true
} }) and assert result?.id and result?.comments length match the admin results;
update the test case named "'`@allow`(\"read\", true) on PK/FK should not emit a
missing raw alias'" to include these USER checks referencing db.$setAuth,
post.create, and post.findUnique so join resolution is validated for both roles.
- Around line 159-202: Add a USER-path assertion to the existing test
"update-only `@deny` on PK/FK should not affect read joins": after seeding as
admin with createPolicyTestClient and the existing admin checks (admin variable,
db.$setAuth(admin).post.create and post.findUnique include: { comments: true }),
also set a non-admin user (e.g. user = { id: 'user-1', role: 'USER' }) and run
db.$setAuth(user).post.findUnique with the same where/include to assert the user
can read the post and sees two comments; this ensures update-scoped `@deny` on
Post.id and Comment.postId does not alter read-time join resolution for
non-admins.
🪄 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: 1befbd77-8aa6-4ac2-8d84-3966613612bb
📒 Files selected for processing (5)
packages/orm/src/client/crud/dialects/lateral-join-dialect-base.tspackages/orm/src/client/crud/dialects/sqlite.tspackages/orm/src/client/query-utils.tspackages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2674.test.ts
lsmith77
force-pushed
the
fix-deny-allow-joins
branch
from
2345079 to
1134cb9
Compare
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.
fix #2674
When a PK or FK field carries a
@denyor@allowfield-level policy, thepolicy handler wraps it in
CASE WHEN … THEN NULL. This causedincludetosilently return empty arrays because join conditions evaluated as
fk = NULL.Fix: Two-part:
The policy handler emits a
__zs_raw_<field>alias (the raw column value)alongside the potentially-nulled expression for any PK or FK field that has
a field access policy.
Join condition builders (
buildJoinPairs, SQLite'sbuildRelationJoinFilter,M2M path in the lateral-join dialect) use the raw alias selectively:
ownedByModel = false): raw alias on both sides — the child'sFK may be denied to hide which parent it belongs to, but the parent must
still be able to fetch its children.
ownedByModel = true): raw alias only on the relation's PKside; the FK side stays as a plain ref. Denying the FK is designed to hide
the relation entirely (preserves pre-existing behavior verified by
field-level-policy.test.ts).Regression tests added in
tests/regression/test/issue-2674.test.ts.Note:
externalIdMappingin the issue title is a REST API option unrelated tothe fix; the root cause is purely at the ORM layer.
Design notes:
fieldHasConditionalReadPolicy is static schema analysis — it can't know the live auth context. In practice this is fine: non-literal conditions like auth().role != 'ADMIN' compile to parameterised SQL regardless of the caller's role, so the raw alias is always projected. The one unsupported edge case is a policy whose condition is non-literal but collapses to trueNode at runtime for a specific auth value (e.g. @Allow('read', auth() == null) with a null-auth caller); documented in the JSDoc.
BelongsTo FK intentionally keeps a plain ref — denying the FK nulls the join and hides the relation. This is pre-existing behaviour, verified by test 2.
The M2M test uses an explicit join model because $pushSchema does not create implicit _AToB tables. The implicit-M2M joinKeyRef branch is therefore not covered by a regression test; a follow-up would require usePrismaPush or extending $pushSchema.
Summary by CodeRabbit
Bug Fixes
New Features
Tests