fix(policy): extra fields included for policy evaluation should be dropped upon return

[ymc9]

fixes #2283

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request resolves issue #2283 by enhancing field-level policy handling in policy-utils to respect select/include directives and queryArgs.omit when removing unreadable fields from query results. It also extends test infrastructure to support database file injection and adds a regression test with complex nested access policies.

Changes

Cohort / File(s)	Summary
Field-level policy handling packages/runtime/src/enhancements/node/policy/policy-utils.ts	Adds robust handling of queryArgs.omit as an object to remove omitted fields; introduces guards to respect select and include directives by deleting fields not selected (scalar) or not included (relation) when hasFieldLevelPolicy is active; extends post-processing to remove unreadable fields based on selection context.
Test infrastructure packages/testtools/src/schema.ts	Introduces optional dbFile?: string parameter in SchemaLoadOptions interface; updates project setup flow to copy provided dbFile to prisma/dev.db before proceeding, with fallback to prisma db push when dbFile is not provided.
Regression test tests/regression/tests/issue-2283/regression.test.ts	Adds regression test for issue 2283 with extensive Prisma schema covering multiple models and complex @@Allow rules; executes nested query on lab profile and asserts deeply nested relation is undefined to verify no back-reference leakage.

Sequence Diagram(s)

sequenceDiagram
    participant Query as Query Executor
    participant PolicyUtil as Policy Utils
    participant PostProc as Post-Processing
    participant Result as Query Result

    Query->>PolicyUtil: Execute query with select/include
    PolicyUtil->>PolicyUtil: Apply access policy rules
    PolicyUtil->>Result: Fetch data with extra policy fields
    
    Result->>PostProc: Pass result to post-processing
    PostProc->>PostProc: Check queryArgs.omit
    PostProc->>PostProc: Verify select/include for each field
    alt Field selected/included or required for policy
        PostProc->>Result: Keep field
    else Field not in select/include or omitted
        PostProc->>Result: Remove field
    end
    
    PostProc->>Query: Return cleaned result matching select/include

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

• policy-utils.ts: Logic density around field removal conditions and select/include distinction between scalars and relations requires careful verification
• schema.ts: Conditional flow for dbFile vs. prisma db push has straightforward logic but affects test infrastructure
• regression.test.ts: Complex nested schema and assertion path demands validation against the original issue description

Possibly related PRs

• fix: relation fields are included even if they are set false in select clause #1429: Modifies policy-utils.ts to respect select/include and remove fields set as false or omitted, directly addressing the same field visibility problem
• chore: no client generation when pushing db during test #1603: Updates packages/testtools/src/schema.ts with database setup enhancements including dbFile parameter handling
• fix(policy): relation filter should respect field-level policies #1495: Extends field-level policy handling in policy-utils.ts to inject read guards and respect select/include hierarchies for policy fields

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title "fix(policy): extra fields included for policy evaluation should be dropped upon return" is fully related to the main change in the changeset. It clearly and specifically describes the core fix: ensuring that fields added solely to evaluate access policies are removed before returning query results. This directly addresses the issue being fixed and is concise without vague terminology.
Linked Issues Check	✅ Passed	The changes directly address the core requirement from issue #2283 [#2283]. The policy-utils.ts modifications implement handling for omitted fields, respect select/include guards, and remove unreadable fields based on field-level policies—all of which ensure that extra fields used during policy evaluation are dropped before returning results. The regression test validates that nested relations do not leak back-references into returned data. The schema.ts supporting infrastructure (dbFile option) enables proper test setup and configuration necessary for validating the fix.
Out of Scope Changes Check	✅ Passed	The core fix in policy-utils.ts directly addresses the issue requirements to prevent extra fields from appearing in query results. The regression test in regression.test.ts validates the fix. The schema.ts changes add supporting infrastructure (a new optional dbFile parameter in SchemaLoadOptions) that enables proper test configuration and database setup, which is necessary for testing the fix but not explicitly required by the issue itself. This supporting infrastructure appears minimal and enables proper validation of the core fix.
Description Check	✅ Passed	The description "fixes #2283" is minimal but related to the changeset by referencing the specific issue being addressed. According to the evaluation criteria, the level of detail is not important for passing this check—the description simply needs to be related to the changeset in some way, which it is through the issue reference.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix/issue-2283

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (5)

packages/testtools/src/schema.ts (2)

153-154: Document and scope dbFile usage (sqlite-only)
Add a brief JSDoc and clarify it’s intended for sqlite schemas; consider validating provider to avoid silent misuse.

 export type SchemaLoadOptions = {
+    /**
+     * Pre-seeded SQLite database file to copy into prisma/dev.db for tests.
+     * Only applies when provider === 'sqlite'. Ignored otherwise.
+     */
     dbFile?: string;
 };

---

341-344: Harden db file copy: mkdir, safe join, overwrite, existence check
Create target dir before copy, use path.join with segments, force overwrite, and provide a clearer error if the source is missing. Also optionally no-op or warn when provider !== 'sqlite’.

-    if (opt.dbFile) {
-        fs.cpSync(opt.dbFile, path.join(projectDir, 'prisma/dev.db'));
-    }
+    if (opt.dbFile) {
+        if (!fs.existsSync(opt.dbFile)) {
+            throw new Error(`dbFile not found: ${opt.dbFile}`);
+        }
+        const destDb = path.join(projectDir, 'prisma', 'dev.db');
+        fs.mkdirSync(path.dirname(destDb), { recursive: true });
+        fs.cpSync(opt.dbFile, destDb, { force: true });
+    }
     else if (opt.pushDb) {
         run('npx prisma db push --skip-generate --accept-data-loss');
     }

Optional provider guard (if desired):

-    if (opt.dbFile) {
+    if (opt.dbFile) {
+        if (opt.provider !== 'sqlite') {
+            console.warn('SchemaLoadOptions.dbFile is only used for sqlite; skipping copy.');
+        } else {
+            // copy as above
+        }
     }

packages/runtime/src/enhancements/node/policy/policy-utils.ts (1)

1531-1535: Avoid accidental relation drops when both select and include are present; dedupe pruning logic

• If both select and include appear (edge cases / future changes), the relation prune should not drop a field that’s explicitly selected. Add a select guard.
• The selection/inclusion pruning runs both here and again under hasFieldLevelPolicy, causing duplicate checks. Consider consolidating to a single pruning step, then run readability checks.

-                if (fieldInfo.isDataModel && queryArgs?.include && typeof queryArgs.include === 'object' && !queryArgs.include[field]) {
+                if (
+                    fieldInfo.isDataModel &&
+                    queryArgs?.include &&
+                    typeof queryArgs.include === 'object' &&
+                    !queryArgs.include[field] &&
+                    !(queryArgs?.select && queryArgs.select[field])
+                ) {
                     // respect include
                     delete entityData[field];
                     continue;
                 }

Optional cleanup: remove the duplicated select/include deletion inside the hasFieldLevelPolicy block and keep only readability checks there.

Also applies to: 1537-1542, 1543-1548

tests/regression/tests/issue-2283/regression.test.ts (2)

6-7: Close Prisma client to avoid open handles in tests
Call $disconnect() after the test to prevent lingering handles/timeouts.

-        const { enhance } = await loadSchema(
+        const { enhance, prisma } = await loadSchema(
             `
             ...
             `,
             { 
               dbFile: path.join(__dirname, 'dev.db'),
             }
         );
@@
-        const db = enhance();
+        const db = enhance();
+        try {
+            // test body...
+        } finally {
+            await db.$disconnect?.();
+            await prisma.$disconnect?.();
+        }

Also applies to: 636-639

---

681-682: Strengthen the assertion across all returned classes
Ensure no class leaks the module relation when it wasn’t selected.

-        expect(r.lab.content[0].modules[0].classes[0].module).toBeUndefined();
+        for (const c of r.lab.content.flatMap((co) => co.modules).flatMap((m) => m.classes)) {
+            expect(c.module).toBeUndefined();
+        }

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 124fb29 and 119ec8b.

⛔ Files ignored due to path filters (1)

• tests/regression/tests/issue-2283/dev.db is excluded by !**/*.db, !**/*.db

📒 Files selected for processing (3)

• packages/runtime/src/enhancements/node/policy/policy-utils.ts (1 hunks)
• packages/testtools/src/schema.ts (2 hunks)
• tests/regression/tests/issue-2283/regression.test.ts (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

tests/regression/tests/issue-2283/regression.test.ts (1)

packages/testtools/src/schema.ts (1)

• loadSchema (173-249)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)

• GitHub Check: build-test (20.x)
• GitHub Check: OSSAR-Scan
• GitHub Check: build-test (20.x)
• GitHub Check: build-test (20.x)
• GitHub Check: dependency-review