fix(policy): reject implicit m2m join table deletes when a participant side is not updatable - #2781
Conversation
…t side is not updatable Deleting rows from an implicit many-to-many join table (e.g. via `set: []` or `disconnect`) was silently filtered down to zero rows when the caller lacked update permission on a participant model, reporting success without doing anything. Now such deletes are rejected upfront, consistent with how join-table inserts are handled. Fixes #2752 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe policy handler now validates implicit many-to-many participants before delete execution. It extracts constrained join-table values, evaluates participant update policies, and rejects unauthorized operations. Regression tests cover non-empty and empty ChangesImplicit many-to-many policy enforcement
Estimated code review effort: 4 (Complex) | ~35 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/regression/test/issue-2752.test.ts (1)
73-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive-path assertion for authorized
disconnect.This test only verifies that an unauthorized caller is rejected. Test 1 verifies both denial and success paths for
set: []. Add a similar assertion here: after the rejection check, have the owner (or another caller with update permission on bothProfileandTag) successfully disconnect the tag, then confirm the link is removed. This confirmspreDeleteCheckdoes not overreject legitimatedisconnectoperations.As per path instructions, `tests/regression/test/issue-*.test.ts` files should live in `tests/regression/test/` with filename format `issue-{number}.test.ts`; this file already satisfies that pattern.✅ Suggested addition
// links intact await expect( rawDb.profile.findUniqueOrThrow({ where: { id: profile.id }, include: { tags: true } }), ).resolves.toMatchObject({ tags: [expect.objectContaining({ id: 'a' })] }); + + // the owner can do it + const asOwner = db.$setAuth({ id: owner.id }); + await expect( + asOwner.profile.update({ where: { id: profile.id }, data: { tags: { disconnect: { id: 'a' } } } }), + ).toResolveTruthy(); + await expect( + rawDb.profile.findUniqueOrThrow({ where: { id: profile.id }, include: { tags: true } }), + ).resolves.toMatchObject({ tags: [] }); });🤖 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-2752.test.ts` around lines 73 - 123, Extend the test around the unauthorized asOther.profile.update disconnect assertion by performing an authorized disconnect as the owner (or another caller permitted to update both Profile and Tag), then assert through rawDb.profile.findUniqueOrThrow with tags included that the tag link is removed. Keep the existing rejection and link-intact checks before the authorized operation.Source: Path instructions
packages/plugins/policy/src/policy-handler.ts (1)
254-300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate per-side policy-check subquery logic.
The per-side subquery construction here (select participant model, filter by id equality, project the update policy filter as a boolean column) closely duplicates the logic in
enforcePreCreatePolicyForManyToManyJoinTable(lines 966-978). Consider extracting a shared private helper, for examplebuildParticipantUpdatableSelection(model, idField, value, alias), that bothpreDeleteCheckandenforcePreCreatePolicyForManyToManyJoinTablecan call. This reduces the risk of the two code paths silently diverging over time.♻️ Example shared helper sketch
+ private buildParticipantUpdatableSelection(model: string, idField: string, value: unknown, alias: string) { + return SelectionNode.create( + AliasNode.create( + this.eb + .selectFrom(model) + .where(this.eb(this.eb.ref(`${model}.${idField}`), '=', value)) + .select(() => new ExpressionWrapper(this.buildPolicyFilter(model, undefined, 'update')).as('_')) + .toOperationNode(), + IdentifierNode.create(alias), + ), + ); + }🤖 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/policy-handler.ts` around lines 254 - 300, Extract the duplicated participant updatable-selection construction from preDeleteCheck and enforcePreCreatePolicyForManyToManyJoinTable into a shared private helper such as buildParticipantUpdatableSelection(model, idField, value, alias). Have both call sites use the helper while preserving the existing model/id equality filter, update policy projection, and alias 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.
Inline comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 273-299: Distinguish nonexistent many-to-many participants from
failed update-policy checks in the result validation loop after proceed. Use the
participant identity fields in each side to detect when the referenced row is
absent and report a row-not-found outcome (or allow the documented harmless
no-op), while retaining the existing NO_ACCESS error only when the participant
exists but its update policy evaluates false.
---
Nitpick comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 254-300: Extract the duplicated participant updatable-selection
construction from preDeleteCheck and
enforcePreCreatePolicyForManyToManyJoinTable into a shared private helper such
as buildParticipantUpdatableSelection(model, idField, value, alias). Have both
call sites use the helper while preserving the existing model/id equality
filter, update policy projection, and alias behavior.
In `@tests/regression/test/issue-2752.test.ts`:
- Around line 73-123: Extend the test around the unauthorized
asOther.profile.update disconnect assertion by performing an authorized
disconnect as the owner (or another caller permitted to update both Profile and
Tag), then assert through rawDb.profile.findUniqueOrThrow with tags included
that the tag link is removed. Keep the existing rejection and link-intact checks
before the authorized operation.
🪄 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 Plus
Run ID: 78b06225-b6b5-4b2e-8975-d53eae4ed401
📒 Files selected for processing (2)
packages/plugins/policy/src/policy-handler.tstests/regression/test/issue-2752.test.ts
…ejection The first participant side incorrectly reported CANNOT_READ_BACK; both sides now consistently report NO_ACCESS like the delete path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Deleting rows from an implicit many-to-many join table (e.g. via
tags: { set: [] }ortags: { disconnect: ... }) was silently filtered down to zero rows when the caller lacked update permission on a participant model — the operation reported success without actually disconnecting anything.This PR adds a pre-delete check in the policy handler: when a delete targets an implicit m2m join table, the participant sides whose fk columns are constrained to literal values in the where clause are checked upfront for "update" permission, and the operation is rejected with a policy error if any side fails. This mirrors the existing pre-create enforcement for join-table inserts.
Fixes #2752
Changes
PolicyHandler.preDeleteCheck: new pre-mutation check for implicit m2m join-table deletesPolicyHandler.extractEqualityValue: helper to extract the literal value a column is constrained to by a top-level conjunction of the where clauseset: []anddisconnectfor both denied and permitted callersTesting
tests/regression/test/issue-2752.test.ts(2 tests, passing)connect-disconnect,relation-many-to-many-filter,crud/update(36 tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests