fix(policy): allow dangerous raw SQL opt-in#2502
Merged
ymc9 merged 5 commits intozenstackhq:devfrom Mar 20, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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)
packages/plugins/policy/test/raw-sql.test.ts (1)
35-65: Consider adding cleanup inafterAll.The test setup creates database connections and test data but doesn't clean up afterward. This could lead to:
- Connection pool exhaustion in CI when running multiple test suites
- Test pollution if tests are run multiple times against the same database
♻️ Suggested cleanup pattern
+import { beforeAll, afterAll, describe, expect, it } from 'vitest'; ... + afterAll(async () => { + // Clean up test data and close connections + await rawClient?.secret?.deleteMany({}); + await rawClient?.user?.deleteMany({}); + await defaultRawClient?.secret?.deleteMany({}); + await defaultRawClient?.user?.deleteMany({}); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/test/raw-sql.test.ts` around lines 35 - 65, Add an afterAll teardown that cleans up connections and test data created in beforeAll: call disconnect/teardown on unsafeClient and defaultClient (e.g., unsafeClient.$disconnect() and defaultClient.$disconnect()), and remove or reset any test rows created via rawClient/defaultRawClient (e.g., delete the 'admin' user) to avoid connection pool exhaustion and test pollution; reference beforeAll setup symbols unsafeClient, rawClient, adminClient, defaultClient, defaultRawClient, and defaultAdminClient when implementing the cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/plugins/policy/test/raw-sql.test.ts`:
- Around line 35-65: Add an afterAll teardown that cleans up connections and
test data created in beforeAll: call disconnect/teardown on unsafeClient and
defaultClient (e.g., unsafeClient.$disconnect() and
defaultClient.$disconnect()), and remove or reset any test rows created via
rawClient/defaultRawClient (e.g., delete the 'admin' user) to avoid connection
pool exhaustion and test pollution; reference beforeAll setup symbols
unsafeClient, rawClient, adminClient, defaultClient, defaultRawClient, and
defaultAdminClient when implementing the cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c23f0116-fd99-4ee4-b947-3fedca5cc701
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/plugins/policy/package.jsonpackages/plugins/policy/src/plugin.tspackages/plugins/policy/test/raw-sql.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/e2e/orm/policy/raw-sql.test.ts (1)
39-57: Consider makingdbNamea required top-level parameter.The type
options?: { dangerouslyAllowRawSql?: boolean; dbName: string }is inconsistent:dbNameis required within the object, but the entire object is optional. Both test calls providedbName, suggesting it should always be required.♻️ Proposed refactor for clearer function signature
- async function createPolicyClient(options?: { dangerouslyAllowRawSql?: boolean; dbName: string }) { + async function createPolicyClient(dbName: string, options?: { dangerouslyAllowRawSql?: boolean }) { const unsafeClient = await createTestClient(schema, { - dbName: options?.dbName, + dbName, plugins: [new PolicyPlugin({ dangerouslyAllowRawSql: options?.dangerouslyAllowRawSql })], });Then update the call sites:
- const { adminClient } = await createPolicyClient({ dbName: 'policy_raw_sql_default' }); + const { adminClient } = await createPolicyClient('policy_raw_sql_default');- const { adminClient } = await createPolicyClient({ - dangerouslyAllowRawSql: true, - dbName: 'policy_raw_sql_dangerous', - }); + const { adminClient } = await createPolicyClient('policy_raw_sql_dangerous', { + dangerouslyAllowRawSql: true, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/orm/policy/raw-sql.test.ts` around lines 39 - 57, Change createPolicyClient to require dbName as a top-level parameter instead of being nested in an optional options object: update the signature from createPolicyClient(options?: { dangerouslyAllowRawSql?: boolean; dbName: string }) to something like createPolicyClient(dbName: string, options?: { dangerouslyAllowRawSql?: boolean }) and use the dbName param when calling createTestClient and creating clients; then update all call sites to pass the dbName string as the first argument (and keep dangerouslyAllowRawSql in the optional second argument). Ensure references to options?.dbName in the function body are replaced with the new dbName parameter and preserve existing behavior for dangerouslyAllowRawSql via options.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/e2e/orm/policy/raw-sql.test.ts`:
- Around line 39-57: Change createPolicyClient to require dbName as a top-level
parameter instead of being nested in an optional options object: update the
signature from createPolicyClient(options?: { dangerouslyAllowRawSql?: boolean;
dbName: string }) to something like createPolicyClient(dbName: string, options?:
{ dangerouslyAllowRawSql?: boolean }) and use the dbName param when calling
createTestClient and creating clients; then update all call sites to pass the
dbName string as the first argument (and keep dangerouslyAllowRawSql in the
optional second argument). Ensure references to options?.dbName in the function
body are replaced with the new dbName parameter and preserve existing behavior
for dangerouslyAllowRawSql via options.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce9c492b-152c-41da-ad03-da62bd9568d0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/plugins/policy/package.jsonpackages/plugins/policy/vitest.config.tstests/e2e/orm/policy/raw-sql.test.ts
💤 Files with no reviewable changes (1)
- packages/plugins/policy/vitest.config.ts
✅ Files skipped from review due to trivial changes (1)
- packages/plugins/policy/package.json
ymc9
reviewed
Mar 20, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/plugins/policy/src/policy-handler.ts (1)
92-92: Use kind-based discrimination forRawNodeguards instead of type casting.The cast
as never(or any cast) is unnecessary. SinceRawNodeis not part of theRootOperationNodeunion in Kysely, the idiomatic type-safe pattern is to check the node'skindproperty directly:if (this.options.dangerouslyAllowRawSql && node.kind === 'RawNode') { return proceed(node); }This follows Kysely's discriminated union pattern used throughout the codebase (e.g., via
SelectQueryNode.is(),InsertQueryNode.is()) and avoids type assertions altogether.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/policy-handler.ts` at line 92, Replace the unnecessary type cast in the condition that checks for raw SQL: inside the PolicyHandler logic where it currently does "if (this.options.dangerouslyAllowRawSql && RawNode.is(node as never))", use kind-based discrimination instead—check this.options.dangerouslyAllowRawSql && node.kind === 'RawNode' (or use RawNode.is(node) without casting) and then return proceed(node); update the guard around RawNode accordingly to remove the "as never" cast and rely on the node.kind check so the code is type-safe and follows the discriminated-union pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Around line 26-27: The change introduced a raw SQL escape hatch via the
dangerouslyAllowRawSql flag that permits RawNode/ReferenceNode through policy
checks; revert that bypass by removing/ignoring dangerouslyAllowRawSql and
ensure RawNode and ReferenceNode are explicitly rejected in the policy
validation (the code that inspects node types where RawNode and ReferenceNode
are currently allowed). Instead of allowing raw nodes, update the policy-handler
to return a clear error or guidance directing callers to use the Kysely
query-builder escape hatch (e.g., add a message pointing to the query-builder
pathway) and add/reinforce tests around the functions that validate nodes so
non-CRUD raw nodes remain rejected. Ensure references to dangerouslyAllowRawSql,
RawNode, and ReferenceNode in the policy validation logic are changed
accordingly.
---
Nitpick comments:
In `@packages/plugins/policy/src/policy-handler.ts`:
- Line 92: Replace the unnecessary type cast in the condition that checks for
raw SQL: inside the PolicyHandler logic where it currently does "if
(this.options.dangerouslyAllowRawSql && RawNode.is(node as never))", use
kind-based discrimination instead—check this.options.dangerouslyAllowRawSql &&
node.kind === 'RawNode' (or use RawNode.is(node) without casting) and then
return proceed(node); update the guard around RawNode accordingly to remove the
"as never" cast and rely on the node.kind check so the code is type-safe and
follows the discriminated-union pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee2ef0b2-88a6-43fc-887a-a41e859c1f8e
📒 Files selected for processing (2)
packages/plugins/policy/src/plugin.tspackages/plugins/policy/src/policy-handler.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/plugins/policy/src/plugin.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/plugins/policy/src/options.ts (1)
2-7: Document the preferred safe path alongside this escape hatch.Please add an explicit note that Kysely query builder should be preferred, and this flag is only for last-resort raw SQL paths.
✏️ Suggested doc tweak
/** * Dangerously bypasses access-policy enforcement for raw SQL queries. + * Prefer Kysely query builder APIs whenever possible; enable this only + * for unavoidable raw SQL in tightly controlled code paths. * Raw queries remain in the current transaction, but the policy plugin will * not inspect or reject them. */As per coding guidelines,
**/*.{ts,tsx}: Use Kysely query builder as escape hatch instead of raw SQL in ORM operations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/policy/src/options.ts` around lines 2 - 7, The doc comment for the dangerouslyAllowRawSql option should be expanded to explicitly state that this is a last-resort escape hatch and that the preferred safe path is to use the Kysely query builder for ORM operations; update the comment on dangerouslyAllowRawSql to recommend using Kysely (and follow the project's coding guideline /**/*.{ts,tsx} to prefer Kysely over raw SQL), describe when raw SQL may be acceptable, and emphasize that this flag only disables policy checks for truly unavoidable cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/plugins/policy/src/options.ts`:
- Around line 2-7: The doc comment for the dangerouslyAllowRawSql option should
be expanded to explicitly state that this is a last-resort escape hatch and that
the preferred safe path is to use the Kysely query builder for ORM operations;
update the comment on dangerouslyAllowRawSql to recommend using Kysely (and
follow the project's coding guideline /**/*.{ts,tsx} to prefer Kysely over raw
SQL), describe when raw SQL may be acceptable, and emphasize that this flag only
disables policy checks for truly unavoidable cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fdc361f5-14b2-405a-a3a1-237691c097b6
📒 Files selected for processing (3)
packages/plugins/policy/src/options.tspackages/plugins/policy/src/plugin.tspackages/plugins/policy/src/policy-handler.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/plugins/policy/src/plugin.ts
- packages/plugins/policy/src/policy-handler.ts
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.
Fixes #2495
Summary
PolicyPluginOptionswith adangerouslyAllowRawSqlescape hatchPolicyPlugin.onKyselyQueryforRawNodequeries when that option is enabledWhy
$use()/$unuse()/$unuseAll()transaction preservation (#2497) fixes the transaction-affinity side of the problem, but it does not help with raw SQL if the policy plugin still intercepts the query and rejects non-CRUD nodes.This change keeps the default behavior unchanged and adds an explicit opt-in for applications that need raw SQL to remain available inside authenticated transactions.
Testing
pnpm --filter @zenstackhq/plugin-policy buildTEST_PG_USER=user TEST_PG_PASSWORD=password pnpm --filter @zenstackhq/plugin-policy test -- test/raw-sql.test.tsSummary by CodeRabbit
New Features
Tests