fix: SQL injection via dot-notation field name in PostgreSQL (GHSA-qpr4-jrj4-6f27)#10159
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 Postgres-focused tests that probe SQL injection via dot-notation sort fields and updates PostgresStorageAdapter to escape quotes when constructing SQL identifiers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 887-888: The integrity check currently uses new
Parse.Query('InjectionTest').first() and then
expect(verify.get('name')).toBe('original'), which can pick up a different row;
instead re-fetch the specific object by its objectId (e.g., use new
Parse.Query('InjectionTest').get(obj.id) or add equalTo('objectId', obj.id) and
first()) prior to the assertion so you validate the same saved row (ensure you
capture obj.id from the created object before re-querying); alternatively
isolate tests with reconfigureServer() to avoid shared class state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5ab81807-1a37-4d0f-9022-312a1231e350
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/Adapters/Storage/Postgres/PostgresStorageAdapter.js
There was a problem hiding this comment.
♻️ Duplicate comments (1)
spec/vulnerabilities.spec.js (1)
927-928:⚠️ Potential issue | 🟡 MinorQuery the created row by
objectIdin these integrity checks.
first()can pick up a differentInjectionTestrow once earlier specs have inserted more data, so these assertions may validate the wrong object. Re-fetchobj.idhere, like the first test already does.🔧 Suggested fix
- const verify = await new Parse.Query('InjectionTest').first(); + const verify = await new Parse.Query('InjectionTest').get(obj.id);Also applies to: 947-948, 967-968, 987-988
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 927 - 928, The integrity checks use new Parse.Query('InjectionTest').first() which can return a different row if other tests inserted data; instead re-query by the created object's id (use obj.id) so you fetch the exact row — replace the generic first() query with a query.where('objectId', obj.id) (or equivalent getById) and then assert verify.get('name') === 'original' for the same object; apply the same change for the other occurrences around the tests that reference Parse.Query('InjectionTest').first().
🤖 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 927-928: The integrity checks use new
Parse.Query('InjectionTest').first() which can return a different row if other
tests inserted data; instead re-query by the created object's id (use obj.id) so
you fetch the exact row — replace the generic first() query with a
query.where('objectId', obj.id) (or equivalent getById) and then assert
verify.get('name') === 'original' for the same object; apply the same change for
the other occurrences around the tests that reference
Parse.Query('InjectionTest').first().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 032dab6f-4885-4473-b714-52a377fd100a
📒 Files selected for processing (1)
spec/vulnerabilities.spec.js
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/vulnerabilities.spec.js (2)
991-1021: Consider verifying actual sort order in positive tests.The positive tests confirm requests succeed (HTTP 200), but they don't verify that sorting actually works correctly. Adding objects with different values and asserting the response order would strengthen confidence that the fix doesn't break legitimate functionality.
💡 Example enhancement
it('allows valid dot-notation sort on object field', async () => { - const obj = new Parse.Object('InjectionTest'); - obj.set('data', { key: 'value' }); - await obj.save(); + const obj1 = new Parse.Object('SortTest'); + obj1.set('data', { key: 'b' }); + await obj1.save(); + const obj2 = new Parse.Object('SortTest'); + obj2.set('data', { key: 'a' }); + await obj2.save(); const response = await request({ method: 'GET', - url: 'http://localhost:8378/1/classes/InjectionTest', + url: 'http://localhost:8378/1/classes/SortTest', headers, qs: { order: 'data.key', }, }); expect(response.status).toBe(200); + expect(response.data.results[0].objectId).toBe(obj2.id); // 'a' comes before 'b' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 991 - 1021, Update the two tests "allows valid dot-notation sort on object field" and "allows valid dot-notation with special characters in sub-field" to assert actual sort order, not just HTTP 200: create and save at least two Parse.Object instances with differing values for data.key (or data['my-field']), perform the same GET request via request({ ... qs: { order: 'data.key' } }) or order: 'data.my-field', parse response.results and add assertions that the returned array is in the expected ascending/descending order (compare the specific field values), and clean up any created objects if necessary to keep tests isolated.
863-1022: PR title suggestion per repository conventions.Based on learnings: For Parse Server PRs, suggest an Angular commit convention PR title that would make a meaningful changelog entry. The current title
fix: GHSA-qpr4-jrj4-6f27-v9is valid but could be more descriptive for the changelog:fix(security): SQL injection via sort dot-notation field name in PostgresThis clearly communicates:
- Type: security fix
- Scope: Postgres adapter
- Impact: SQL injection vulnerability
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 863 - 1022, The PR title is too terse; update it from "fix: GHSA-qpr4-jrj4-6f27-v9" to a descriptive Angular-style title so changelogs capture the fix — e.g. rename the PR to "fix(security): SQL injection via sort dot-notation field name in Postgres" (this applies to the commit/PR that touches the tests in spec/vulnerabilities.spec.js and the Postgres adapter code paths handling dot-notation sort fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 991-1021: Update the two tests "allows valid dot-notation sort on
object field" and "allows valid dot-notation with special characters in
sub-field" to assert actual sort order, not just HTTP 200: create and save at
least two Parse.Object instances with differing values for data.key (or
data['my-field']), perform the same GET request via request({ ... qs: { order:
'data.key' } }) or order: 'data.my-field', parse response.results and add
assertions that the returned array is in the expected ascending/descending order
(compare the specific field values), and clean up any created objects if
necessary to keep tests isolated.
- Around line 863-1022: The PR title is too terse; update it from "fix:
GHSA-qpr4-jrj4-6f27-v9" to a descriptive Angular-style title so changelogs
capture the fix — e.g. rename the PR to "fix(security): SQL injection via sort
dot-notation field name in Postgres" (this applies to the commit/PR that touches
the tests in spec/vulnerabilities.spec.js and the Postgres adapter code paths
handling dot-notation sort fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 622281e3-9175-4af4-b302-baf3cdee5ff3
📒 Files selected for processing (1)
spec/vulnerabilities.spec.js
mtrezza
changed the title
# [9.6.0-alpha.2](9.6.0-alpha.1...9.6.0-alpha.2) (2026-03-09) ### Bug Fixes * SQL injection via dot-notation field name in PostgreSQL ([GHSA-qpr4-jrj4-6f27](GHSA-qpr4-jrj4-6f27)) ([#10159](#10159)) ([ea538a4](ea538a4))
|
🎉 This change has been released in version 9.6.0-alpha.2 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10159 +/- ##
==========================================
- Coverage 92.58% 92.17% -0.42%
==========================================
Files 192 192
Lines 16207 16207
Branches 183 183
==========================================
- Hits 15005 14938 -67
- Misses 1190 1253 +63
- Partials 12 16 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Pull Request
Issue
SQL injection via dot-notation field name in PostgreSQL (GHSA-qpr4-jrj4-6f27)
Tasks
Summary by CodeRabbit