fix(policy): reject access to undefined models

[ymc9]

Summary by CodeRabbit

• Bug Fixes

  • Policy enforcement now correctly rejects operations targeting tables or models not defined in your schema.

• Tests

  • Added comprehensive test coverage verifying policy validation across read, create, update, and delete operations.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The PR marks a TODO item as completed and introduces validation logic to reject policy operations targeting nonexistent models. A new private helper method tryRejectNonexistentModel() is added to PolicyHandler and invoked across mutation handling, join transformation, and filter creation paths. Comprehensive end-to-end tests verify rejection behavior across read, write, and delete operations.

Changes

Cohort / File(s)	Summary
Documentation TODO.md	Marks "Accessing tables not in the schema" item as completed in the Access Policy section.
Policy enforcement packages/plugins/policy/src/policy-handler.ts	Adds private method tryRejectNonexistentModel(model: string) that validates model existence in schema (excluding many-to-many join tables) and throws NO_ACCESS policy error if nonexistent. Invoked in handle() for pre-mutation validation, in transformJoin() for nested table policies, and in createPolicyFilterForTables() when processing referenced tables.
Test coverage tests/e2e/orm/policy/nonexistent-models.test.ts	New end-to-end test file verifying policy rejection across read, join, create, update, update-from, and delete operations targeting nonexistent models and fields using validators like toBeRejectedByPolicy() and toBeRejectedByValidation().

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Review the logic flow and placement of tryRejectNonexistentModel() calls across handle(), transformJoin(), and createPolicyFilterForTables() to ensure complete coverage
• Verify the test cases exercise all code paths where nonexistent model validation occurs
• Confirm the distinction between many-to-many join tables and regular tables in the validation logic

Possibly related PRs

• zenstackhq/zenstack-v3#439: Modifies PolicyHandler and policy transformation logic for expression context and collection traversal, overlapping with this PR's policy handler changes.

Poem

🐰 A model that doesn't exist in our schema?
Not on our watch—no way, José!
With tryRejectNonexistentModel so keen,
We catch the ghosts before they're seen ✨
Policy enforced from end to end,
Our access control's finest friend!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(policy): reject access to undefined models' accurately and concisely describes the main change: adding validation to reject operations on nonexistent models in the policy handler.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/non-existing-models

---

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.}

[claude]

Claude finished @ymc9's task —— View job

---

Code Review Complete ✅

I've conducted a comprehensive review of this PR which adds validation to reject access to undefined/non-existent models in the policy system. Here's my detailed analysis:

Review Checklist

• Read and analyze changed files
• Review policy handler implementation
• Examine test coverage and quality
• Check for security concerns
• Assess performance implications
• Provide comprehensive feedback

Review Summary

This is a solid security improvement that prevents access to models not defined in the schema. The implementation is clean, well-tested, and follows established patterns in the codebase.

Code Quality ⭐⭐⭐⭐⭐

Strengths:

• Clean implementation: The tryRejectNonexistentModel() method is simple, focused, and follows single responsibility principle
• Strategic placement: Validation is added at key decision points in the policy handler:
  • Line 93: Before processing mutations (policy-handler.ts:93)
  • Line 336: Before processing joins (policy-handler.ts:336)
  • Line 879: Before processing table filters (policy-handler.ts:879)
• Consistent error handling: Uses the existing createRejectedByPolicyError() utility with appropriate RejectedByPolicyReason.NO_ACCESS
• Handles edge cases: Correctly allows many-to-many join tables via isManyToManyJoinTable() check

Security Assessment ⭐⭐⭐⭐⭐

Excellent security enhancement:

• Prevents unauthorized access: Blocks queries against undefined models that could potentially access unintended data
• Defense in depth: Catches undefined model access at multiple query transformation points
• Proper error reporting: Uses NO_ACCESS reason without revealing schema details
• No information leakage: Error messages don't expose internal schema structure

Test Coverage ⭐⭐⭐⭐⭐

Comprehensive test coverage in nonexistent-models.test.ts:

• Tests multiple query types: SELECT, INSERT, UPDATE, DELETE
• Tests JOIN operations with undefined models
• Tests UPDATE with FROM clauses
• Properly sets up test environment with real table creation
• Uses appropriate matchers (toBeRejectedByPolicy(), toBeRejectedByValidation())
• Good separation between model-level rejection vs validation rejection

Performance Impact ⭐⭐⭐⭐⭐

Minimal performance overhead:

• QueryUtils.hasModel() is likely a simple schema lookup (O(1) or O(log n))
• isManyToManyJoinTable() check is only performed when model doesn't exist
• No additional database queries - purely in-memory validation
• Early rejection prevents unnecessary query processing

Potential Issues & Suggestions

Minor suggestions:

1. Consider caching: For high-frequency scenarios, could cache the list of valid model names, though the current lookup is probably fast enough
2. Error context: The error message could potentially include more context about the rejected model name (though this might be intentionally omitted for security)

Code style note:

• The TODO.md change (line 107: "Accessing tables not in the schema") appears to mark this feature as completed ✅

Architecture Alignment ⭐⭐⭐⭐⭐

• Follows established patterns in the policy handler
• Integrates well with existing error handling system
• Maintains consistency with other validation checks
• Doesn't break existing functionality

Recommendation: APPROVE ✅

This PR effectively closes a potential security gap by preventing access to undefined models. The implementation is robust, well-tested, and maintains good performance characteristics. The code quality meets high standards and follows established patterns in the codebase.

---

[Copilot]

Pull request overview

This PR adds security enforcement to reject queries accessing database tables that are not defined in the schema. The policy handler now validates that all tables referenced in queries correspond to either schema-defined models or implicit many-to-many join tables before processing them.

Key Changes

• Added tryRejectNonexistentModel() method to validate model existence before allowing operations
• Integrated validation checks at three key points: mutations, joins, and SELECT FROM clauses
• Comprehensive test coverage for various query operations against undefined tables

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
packages/plugins/policy/src/policy-handler.ts	Implements the tryRejectNonexistentModel() method and calls it at strategic points (mutations, joins, and FROM clause processing) to reject access to undefined models
tests/e2e/orm/policy/nonexistent-models.test.ts	Adds comprehensive test coverage verifying that all types of operations (read, create, update, delete, join) are properly rejected when accessing tables not defined in the schema
TODO.md	Updates the TODO list to mark "Accessing tables not in the schema" as completed

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

tests/e2e/orm/policy/nonexistent-models.test.ts (1)

31-57: LGTM - Comprehensive coverage of policy rejection scenarios.

The test thoroughly validates that all operations (read, join, create, update, update-with-from, delete) on the nonexistent Bar table are rejected by policy. This directly exercises the new tryRejectNonexistentModel() validation introduced in policy-handler.ts.

Minor observation: Line 39 attempts to insert with id: '1' which duplicates the existing row from line 23. While the policy rejection will prevent this from reaching the database, using a unique id like '2' would make the test intent clearer.

Consider this optional clarification:

 // create
-await expect(db.$qb.insertInto('Bar').values({ id: '1', string: 'test' }).execute()).toBeRejectedByPolicy();
+await expect(db.$qb.insertInto('Bar').values({ id: '2', string: 'test' }).execute()).toBeRejectedByPolicy();

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5ac9e13 and 4a942a8.

📒 Files selected for processing (3)

• TODO.md (1 hunks)
• packages/plugins/policy/src/policy-handler.ts (4 hunks)
• tests/e2e/orm/policy/nonexistent-models.test.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (1)

tests/e2e/**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

E2E tests should validate real-world schema compatibility with established projects

Files:

• tests/e2e/orm/policy/nonexistent-models.test.ts

🧠 Learnings (3)

📓 Common learnings

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`

📚 Learning: 2025-11-26T01:55:04.540Z

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to packages/zenstackhq/orm/**/*.test.{ts,tsx} : ORM package tests should include comprehensive client API tests and policy tests

Applied to files:

• tests/e2e/orm/policy/nonexistent-models.test.ts

📚 Learning: 2025-11-26T01:55:04.540Z

Learnt from: CR
Repo: zenstackhq/zenstack-v3 PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-26T01:55:04.540Z
Learning: Applies to tests/e2e/**/*.{ts,tsx} : E2E tests should validate real-world schema compatibility with established projects

Applied to files:

• tests/e2e/orm/policy/nonexistent-models.test.ts

🧬 Code graph analysis (1)

tests/e2e/orm/policy/nonexistent-models.test.ts (1)

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

• createPolicyTestClient (258-269)

🪛 markdownlint-cli2 (0.18.1)

TODO.md

107-107: Unordered list indentation
Expected: 2; Actual: 4

(MD007, ul-indent)

⏰ 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). (4)

• GitHub Check: Agent
• GitHub Check: build-test (20.x, sqlite)
• GitHub Check: build-test (20.x, postgresql)
• GitHub Check: claude-review

🔇 Additional comments (8)

TODO.md (1)

107-107: LGTM - TODO item appropriately marked as complete.

The checkbox toggle correctly reflects the new policy enforcement that rejects access to nonexistent models, implemented in packages/plugins/policy/src/policy-handler.ts.

Note: The markdownlint indentation warning appears to be a style preference issue, as the 4-space indentation is consistent with other items at the same nesting level (lines 103-106).

tests/e2e/orm/policy/nonexistent-models.test.ts (3)

1-3: LGTM - Imports are correct.

The test properly imports the policy test client and testing utilities.

---

6-24: LGTM - Test setup correctly creates the scenario for testing nonexistent model access.

The test appropriately:

• Defines a Foo model in the schema with permissive policies
• Creates a Bar table outside the schema via raw SQL
• Sets up test data for both tables

This aligns with the E2E testing guidelines for validating real-world schema compatibility.

---

25-28: LGTM - Validation checks are correct.

The test correctly verifies:

• db.bar is undefined since Bar is not in the schema
• Unknown relation field access is rejected by validation (not policy)

packages/plugins/policy/src/policy-handler.ts (4)

1019-1023: LGTM - Validation logic correctly rejects nonexistent models.

The implementation properly:

• Checks model existence using QueryUtils.hasModel
• Excludes many-to-many join tables which are implicitly created
• Throws an appropriate policy error with NO_ACCESS reason

The method name uses the "try" prefix which could suggest it returns a boolean, but the "try and reject if needed" pattern is acceptable here.

---

93-93: LGTM - Early validation prevents mutations on nonexistent models.

The call is correctly placed after extracting the mutation model and before any pre-mutation policy work. This ensures fast rejection of invalid operations.

---

336-336: LGTM - Join validation rejects references to nonexistent tables.

The check correctly validates that the joined table exists in the schema before building policy filters. This prevents joins with tables outside the schema definition.

---

879-879: LGTM - Read policy validation rejects nonexistent tables in FROM/USING clauses.

The check ensures all tables referenced in read operations exist in the schema before building policy filters. This validation applies to tables in FROM clauses, USING clauses (for DELETE), and join references.