fix(orm): escape special characters in string search patterns#479
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughConsolidates JSON string-filter logic into the base dialect by implementing escaping and validation, removes dialect-specific overrides, removes Volta config from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
Claude finished @ymc9's task —— View job PR Review: String Pattern Escaping ImplementationReview Tasks
SummaryThis PR implements proper escaping of special characters ( Code Quality & Implementation ✅Excellent Implementation QualityThe code changes demonstrate high-quality engineering:
Security Assessment ✅Critical Vulnerability FixedThis PR addresses a significant SQL injection vulnerability: Before: User input like Example Attack Prevention: -- Vulnerable (before): matches any string ending with 'x'
WHERE field LIKE '%admin%x%'
-- Secure (after): matches literal string '%admin%x'
WHERE field LIKE '%\%admin\%x%' ESCAPE '\'Proper Escaping Strategy
Test Coverage Assessment ✅Comprehensive Test SuiteExcellent test coverage across multiple areas:
Real-world Test ScenariosTests include realistic attack patterns:
Performance Implications ✅Minimal Performance Impact
Negligible OverheadThe escaping logic adds ~3 regex operations per string filter, which is insignificant compared to database query overhead. Architecture & Design ✅Clean Architecture
Backward Compatibility
Minor Recommendations
Overall AssessmentThis is an excellent, security-critical fix that should be merged immediately. Strengths:
Security Impact:
This PR demonstrates exemplary security engineering practices with thorough testing and clean implementation. |
There was a problem hiding this comment.
Pull request overview
This PR implements proper escaping of special SQL LIKE pattern characters (%, _, ) in string search operations to prevent them from being interpreted as wildcards. The fix addresses a security and correctness issue where user input containing these characters could cause unexpected matching behavior or potential SQL injection vulnerabilities.
Key Changes:
- Centralized LIKE pattern escaping logic in
BaseCrudDialectwith properESCAPE '\'clause - Updated policy functions (
contains,startsWith,endsWith) to use SQL-based escaping instead of relying on unescaped patterns - Removed dialect-specific JSON string filter implementations in favor of the unified approach
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/orm/src/client/functions.ts | Added SQL-based escaping of special characters (%, _, ) in textMatch function for policy operations, with proper ESCAPE clause |
| packages/orm/src/client/crud/dialects/base-dialect.ts | Introduced escapeLikePattern() and buildStringLike() methods to centralize escaping logic; moved buildJsonStringFilter() from abstract to private implementation; improved error handling by replacing invariant(false) with createInvalidInputError() |
| packages/orm/src/client/crud/dialects/postgresql.ts | Removed dialect-specific buildJsonStringFilter() override as it's now handled by base class |
| packages/orm/src/client/crud/dialects/sqlite.ts | Removed dialect-specific buildJsonStringFilter() override as it's now handled by base class |
| tests/e2e/orm/policy/policy-functions.test.ts | Added test cases verifying that '%' character in policy function arguments is treated literally |
| tests/e2e/orm/client-api/json-filter.test.ts | Added test cases for JSON field string filters with '%' character to verify literal matching |
| tests/e2e/orm/client-api/filter.test.ts | Added user with '%' in email address and updated test expectations; verified proper escaping in contains/startsWith filters |
| packages/language/package.json | Removed volta configuration (unrelated to the main PR purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/e2e/orm/client-api/json-filter.test.ts (1)
635-644: Consider adding tests for underscore and backslash escaping.The implementation escapes
_and\in addition to%, but only%is tested here. Consider adding test cases for these characters to ensure complete coverage of the escaping logic.tests/e2e/orm/policy/policy-functions.test.ts (1)
26-41: Good policy function escaping test, but consider removing debug flag.The test correctly validates that
%is treated as a literal character incontainspolicy functions. However,debug: trueon line 35 will produce verbose output during test runs.Consider removing the debug flag before merging:
`, - { debug: true }, + {}, );Or simply remove the options object entirely if not needed.
packages/orm/src/client/crud/dialects/base-dialect.ts (1)
859-861: Consider extracting shared escaping logic.The
escapeLikePatternmethod here duplicates the escaping logic inpackages/orm/src/client/functions.ts(lines 56-57). Consider extracting this to a shared utility to maintain consistency and reduce duplication.Example shared utility:
// In a shared utils file export function escapeLikePattern(pattern: string): string { return pattern.replace(/\\/g, '\\\\').replace(/%/g, '\\%').replace(/_/g, '\\_'); }This would ensure both the client API filters and policy functions use identical escaping logic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/language/package.json(0 hunks)packages/orm/src/client/crud/dialects/base-dialect.ts(4 hunks)packages/orm/src/client/crud/dialects/postgresql.ts(0 hunks)packages/orm/src/client/crud/dialects/sqlite.ts(0 hunks)packages/orm/src/client/functions.ts(2 hunks)tests/e2e/orm/client-api/filter.test.ts(8 hunks)tests/e2e/orm/client-api/json-filter.test.ts(4 hunks)tests/e2e/orm/policy/policy-functions.test.ts(2 hunks)
💤 Files with no reviewable changes (3)
- packages/orm/src/client/crud/dialects/sqlite.ts
- packages/language/package.json
- packages/orm/src/client/crud/dialects/postgresql.ts
🧰 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/policy-functions.test.tstests/e2e/orm/client-api/json-filter.test.tstests/e2e/orm/client-api/filter.test.ts
🧠 Learnings (6)
📚 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/policy-functions.test.tstests/e2e/orm/client-api/json-filter.test.tstests/e2e/orm/client-api/filter.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/policy-functions.test.tstests/e2e/orm/client-api/filter.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 packages/zenstackhq/orm/**/*.{ts,tsx} : Use Kysely as the query builder interface for low-level database queries, avoiding raw SQL when possible
Applied to files:
packages/orm/src/client/functions.tspackages/orm/src/client/crud/dialects/base-dialect.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 packages/zenstackhq/orm/**/*.{ts,tsx} : Implement plugin hooks at ORM, Kysely, and entity mutation levels for query interception and customization
Applied to files:
packages/orm/src/client/functions.tspackages/orm/src/client/crud/dialects/base-dialect.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 **/*.zmodel : ZModel schema files should define database structure and policies that compile to TypeScript via `zenstack generate`
Applied to files:
packages/orm/src/client/functions.ts
📚 Learning: 2025-10-21T16:04:56.292Z
Learnt from: ymc9
Repo: zenstackhq/zenstack-v3 PR: 319
File: packages/runtime/src/client/crud/dialects/base-dialect.ts:745-747
Timestamp: 2025-10-21T16:04:56.292Z
Learning: In packages/runtime/src/client/crud/dialects/base-dialect.ts, it's intentional that buildCursorFilter applies default ordering (via makeDefaultOrderBy fallback) while buildOrderBy does not. This ensures cursor-based pagination always has stable ordering for correctness, while regular queries remain unordered unless explicitly specified. This design is to be consistent with Prisma's pagination requirements.
Applied to files:
packages/orm/src/client/crud/dialects/base-dialect.ts
🧬 Code graph analysis (3)
tests/e2e/orm/policy/policy-functions.test.ts (1)
packages/testtools/src/client.ts (1)
createPolicyTestClient(258-269)
tests/e2e/orm/client-api/filter.test.ts (2)
packages/testtools/src/client.ts (1)
createTestClient(101-248)tests/e2e/orm/client-api/utils.ts (1)
createUser(6-21)
packages/orm/src/client/crud/dialects/base-dialect.ts (2)
packages/orm/src/client/errors.ts (1)
createInvalidInputError(116-120)packages/common-helpers/src/tiny-invariant.ts (1)
invariant(4-14)
⏰ 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 (10)
tests/e2e/orm/client-api/json-filter.test.ts (1)
495-622: Good test coverage for special character escaping.The tests comprehensively verify that
%is treated as a literal character across all JSON string filter operations (string_contains,string_starts_with,string_ends_with). The test data setup with'%Foo'enables both negative tests (pattern not matching when%should be literal) and positive tests (matching actual%characters in data).tests/e2e/orm/policy/policy-functions.test.ts (1)
163-177: LGTM!The
startsWithescaping test properly validates that%at the beginning of the pattern is treated as a literal character.tests/e2e/orm/client-api/filter.test.ts (4)
47-47: LGTM!Adding a user with
%in the email enables comprehensive testing of escaping behavior in string filters.
97-132: Comprehensive SQLite escaping tests.Good coverage testing that
%is treated as literal incontainsandstartsWithfilters for SQLite.
179-192: Comprehensive PostgreSQL escaping tests.Good coverage for case-insensitive mode with
%escaping on PostgreSQL.
264-279: Correctly updated result counts.The expected lengths are properly adjusted to account for the new
%u3@test.comuser:
lt: 'a@test.com'→ 1 (only%u3@test.comis lexicographically less thana)lt: 'z@test.com'→ 3 (all users)lte: 'u1@test.com'→ 2 (%u3@test.comandu1@test.com)lte: 'u2@test.com'→ 3 (all users)packages/orm/src/client/crud/dialects/base-dialect.ts (3)
842-867: Well-structured escaping implementation for JSON string filters.The implementation correctly:
- Quotes the pattern (
"%...%") since JSON strings are stored with quotes- Uses consistent escaping logic via
escapeLikePattern- Properly handles case-insensitive mode by checking
supportsILike
820-828: LGTM!The
buildStringFiltermethod now properly escapes patterns before constructing LIKE clauses, ensuring%,_, and\are treated as literals.
544-544: Improved error messaging.The updated error message provides clearer feedback when JSON filter payloads are invalid.
packages/orm/src/client/functions.ts (1)
56-65: Proper escaping implementation for LIKE patterns.The escaping logic correctly:
- Escapes backslashes first (
\\→\\\\) to avoid double-escaping- Then escapes
%and_wildcards- Uses explicit
ESCAPE '\\'clause for cross-database compatibilityThe
CAST(... as text)ensures the expression is treated as text before applyingREPLACE. TheESCAPEclause syntax is supported by both SQLite and PostgreSQL, with backslash as a valid single-character escape character in each database.
|
Claude finished @ymc9's task —— View job PR Review: String Pattern Escaping ImplementationReview Tasks
SummaryThis PR implements proper escaping of special characters ( Code Quality & Implementation ✅Excellent Implementation QualityThe code changes demonstrate high-quality engineering:
Security Assessment ✅Critical Vulnerability FixedThis PR addresses a significant SQL injection vulnerability: Before: User input like Example Attack Prevention: -- Vulnerable (before): matches any string ending with 'x'
WHERE field LIKE '%admin%x%'
-- Secure (after): matches literal string '%admin%x'
WHERE field LIKE '%\%admin\%x%' ESCAPE '\'Proper Escaping Strategy
Test Coverage Assessment ✅Comprehensive Test SuiteExcellent test coverage across multiple areas:
Real-world Test ScenariosTests include realistic attack patterns:
Performance Implications ✅Minimal Performance Impact
Negligible OverheadThe escaping logic adds ~3 regex operations per string filter, which is insignificant compared to database query overhead. Architecture & Design ✅Clean Architecture
Backward Compatibility
Minor Recommendations
Overall AssessmentThis is an excellent, security-critical fix that should be merged immediately. Strengths:
Security Impact:
This PR demonstrates exemplary security engineering practices with thorough testing and clean implementation. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/e2e/orm/client-api/filter.test.ts (2)
97-142: Good coverage forcontainsandstartsWithescape behavior.These tests effectively validate that
%is treated as a literal character. Theu3avsu3%assertions confirm the wildcard is properly escaped.Consider adding
endsWithescape tests for completeness (e.g.,endsWith: '%@test.com'should not match,endsWith: '@test.com'should match all).
44-365: Consider adding tests for underscore (_) escape handling.The tests thoroughly validate
%escape behavior. Since_is also a SQL LIKE wildcard (matches any single character), consider adding a test user with_in the email (e.g.,u4_@test.com) to verify complete escape coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/orm/client-api/filter.test.ts(10 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/client-api/filter.test.ts
🧠 Learnings (2)
📚 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/client-api/filter.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/client-api/filter.test.ts
🧬 Code graph analysis (1)
tests/e2e/orm/client-api/filter.test.ts (1)
packages/testtools/src/client.ts (1)
createTestClient(101-248)
⏰ 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). (3)
- GitHub Check: claude-review
- GitHub Check: build-test (20.x, postgresql)
- GitHub Check: build-test (20.x, sqlite)
🔇 Additional comments (6)
tests/e2e/orm/client-api/filter.test.ts (6)
10-10: LGTM!Removing the redundant
as anycast is a good cleanup sincecreateTestClientalready returns a properly typed client.
44-47: Good test data for escape validation.Adding a user with
%in the email is essential for validating that the ORM correctly escapes LIKE pattern special characters.
189-210: LGTM!Good parity with SQLite escape tests, now covering PostgreSQL's case-insensitive mode. The assertions correctly validate that
%is escaped in the ILIKE pattern.
265-269: LGTM!Correctly updated to exclude all three users, maintaining the test's intent of verifying that
notInwith all users returns falsy.
276-316: LGTM!The length expectations are correctly updated to account for the third user. The alphabetical ordering (
u3%@test.comcomes afteru2@test.combecause'3' > '2') is correctly reflected in thegteassertions.
324-328: LGTM!Using
'4@'instead of'3@'is a reasonable change that makes the test's intent clearer—searching for a pattern that definitely doesn't exist in any of the test emails.
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.