fix(db): prevent prototype pollution via select() alias paths (#1584)#1595
Open
kevin-dp wants to merge 6 commits into
Open
fix(db): prevent prototype pollution via select() alias paths (#1584)#1595kevin-dp wants to merge 6 commits into
kevin-dp wants to merge 6 commits into
Conversation
Adds a failing test demonstrating that .select() alias paths like `__proto__.polluted` or `constructor.prototype.polluted` are split on '.' and walked into the result object without sanitization, allowing prototype pollution through queryOnce(). This commit intentionally fails CI to demonstrate the vulnerability; the next commit fixes it.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughPrototype pollution protection is added to the ChangesPrototype Pollution Guard for select() Aliases
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.
Actionable comments posted: 2
🤖 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/db/tests/query/select-prototype-pollution.test.ts`:
- Around line 26-27: The test uses `({} as any).polluted` to check for prototype
pollution, which relies on unsafe `any` casts and makes the security assertion
ambiguous. Replace this pattern by creating a properly typed object literal
(without `any` casts) and explicitly check that the `polluted` property is NOT
an own property of Object.prototype using Object.prototype.hasOwnProperty or
Object.getOwnPropertyNames to make the security assertion clearer and more
explicit. Apply this change to all occurrences of the `({} as any).polluted`
pattern in the file (found at lines 26-27, 36-39, and 50-50).
- Around line 28-35: The test using `.rejects.toThrow()` without specifying the
expected error type or message is too permissive and will pass on any error, not
just the security-related one. Modify the `.rejects.toThrow()` calls in the
queryOnce test blocks to specify the expected error type or message that should
be thrown for unsafe alias paths (such as UnsafeAliasPathError or a specific
error message pattern). Apply this same fix to all similar test assertions in
the file that validate unsafe alias path handling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0c210b7-5304-44f3-b8a8-47afe8f22ea6
📒 Files selected for processing (1)
packages/db/tests/query/select-prototype-pollution.test.ts
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Adds a new `UnsafeAliasPathError` (extends QueryCompilationError) and an `assertSafeAliasSegments` helper invoked in three places in packages/db/src/query/compiler/select.ts: - `addFromObject` validates each non-spread key at compile time, including dotted keys, before recording any select operation. - `processNonMergeOp` validates the split alias path before walking into the result object. - `processMerge` validates `targetPath` for the same reason. Segments matching `__proto__`, `prototype`, or `constructor` are rejected, which prevents prototype pollution via aliases like `__proto__.polluted` or `constructor.prototype.polluted` going through queryOnce() / createLiveQueryCollection(). Fixes TanStack#1584
- Import UnsafeAliasPathError and assert that the rejection is exactly
that error class instead of a permissive .rejects.toThrow().
- Drop the `({} as any).polluted` pattern in favour of
Object.prototype.hasOwnProperty.call(Object.prototype, 'polluted'),
which is type-safe and a more explicit assertion that
Object.prototype itself was not mutated.
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
1 participant
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 #1584.
Summary
.select()alias keys are split on.and walked into the output object without sanitization. Aliases like__proto__.pollutedorconstructor.prototype.pollutedtherefore reachObject.prototypeand pollute it for the rest of the process. Exploitable through any public entry point that compiles a select (e.g.queryOnce,createLiveQueryCollection).Plan for this PR (intentional red → green)
To make the vulnerability + fix visible in CI history, this PR is structured as two commits:
tests/query/select-prototype-pollution.test.ts. CI is expected to fail on this commit, demonstrating the bug exists onmain.UnsafeAliasPathErrorand rejects unsafe alias path segments (__proto__,prototype,constructor) in the select compiler. CI should turn green.The fix commit will be pushed after CI on commit 1 has reported the expected failure.
Fix details (commit 2, pushed after red is observed)
UnsafeAliasPathError extends QueryCompilationErrorinpackages/db/src/errors.ts.assertSafeAliasSegmentshelper inpackages/db/src/query/compiler/select.ts, invoked from:addFromObject(validates each non-spread key at compile time, including dotted keys)processNonMergeOp(validates the split alias path at row-processing time)processMerge(validatestargetPath)The thrown error matches the existing
QueryCompilationErrorstyle (e.g.DistinctRequiresSelectError).Summary by CodeRabbit
.select()alias path handling to reject unsafe prototype-related segments (__proto__,prototype,constructor) and prevent prototype-pollution-style effects.UnsafeAliasPathErrorto explicitly fail queries that attempt unsafe alias paths.Object.prototyperemains unmodified after failures.