SQL injection via query field name when using PostgreSQL (GHSA-c442-97qw-j6c6)#10177
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 PostgreSQL SQL-injection tests for Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(220,220,255,0.5)
participant Client
end
rect rgba(220,255,220,0.5)
participant DBController as DatabaseController
end
rect rgba(255,220,220,0.5)
participant Adapter as PostgresStorageAdapter
end
rect rgba(240,240,240,0.5)
participant PostgresDB as PostgreSQL
end
Client->>DBController: send query (filter with fieldName/$regex, masterKey?)
DBController->>DBController: validateQuery(keyName, masterKey, internalFields, queryOperators)
alt invalid key
DBController-->>Client: return invalid-key error
else allowed
DBController->>Adapter: buildWhereClause(filter)
alt fieldName contains dot
Adapter->>Adapter: transformDotField(fieldName) -> quoted dotted identifier
else non-dot field
Adapter->>Adapter: escape double quotes in fieldName -> use raw $index:name
end
Adapter->>PostgresDB: execute SQL WHERE clause
PostgresDB-->>Adapter: results
Adapter-->>DBController: results
DBController-->>Client: return results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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.
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
2291-2316: Exercise the exact_email_verify_token+$regexpath here.
$existsproves the allowlist, but it doesn’t hit the PostgreSQL$regexbranch changed in this PR. Switching this to a trivial regex would make the regression test cover the exact master-only path.🧪 Suggested tweak
const response = await request({ method: 'GET', url: `${serverURL}/classes/_User`, headers, qs: { - where: JSON.stringify({ _email_verify_token: { $exists: true } }), + where: JSON.stringify({ _email_verify_token: { $regex: '.*' } }), }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 2291 - 2316, The test currently uses $exists to exercise the allowlist but not the PostgreSQL $regex branch; update the request qs in the test "allows master key to query whitelisted internal field _email_verify_token" to use a trivial regex on _email_verify_token (e.g. { _email_verify_token: { $regex: '.*' } }) so the request sent by request(...) hits the $regex code path; locate the test using reconfigureServer, request, serverURL, and headers and replace the where clause JSON.stringify payload accordingly.
🤖 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 2291-2316: The test currently uses $exists to exercise the
allowlist but not the PostgreSQL $regex branch; update the request qs in the
test "allows master key to query whitelisted internal field _email_verify_token"
to use a trivial regex on _email_verify_token (e.g. { _email_verify_token: {
$regex: '.*' } }) so the request sent by request(...) hits the $regex code path;
locate the test using reconfigureServer, request, serverURL, and headers and
replace the where clause JSON.stringify payload accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9062add-fc49-47dc-9edd-9888df4d0b44
📒 Files selected for processing (3)
spec/vulnerabilities.spec.jssrc/Adapters/Storage/Postgres/PostgresStorageAdapter.jssrc/Controllers/DatabaseController.js
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #10177 +/- ##
==========================================
- Coverage 92.61% 92.61% -0.01%
==========================================
Files 192 192
Lines 16249 16256 +7
Branches 190 190
==========================================
+ Hits 15049 15055 +6
- Misses 1187 1188 +1
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 `@src/Controllers/DatabaseController.js`:
- Around line 62-80: The update validation is incorrectly allowing non-master
callers to use fields derived from internalFields[*].masterWrite via
specialKeysForUpdate; in DatabaseController.update you must gate acceptance of
those masterWrite internal keys behind the isMaster check (i.e., when building
or applying specialKeysForUpdate during update-key validation only include
masterWrite keys if isMaster is true), so that sensitive fields like
_email_verify_token, _perishable_token, _failed_login_count,
_account_lockout_expires_at, and _password_history are treated as master-only;
update the validation logic in DatabaseController.update to consult isMaster
before treating any key from specialKeysForUpdate as allowed for the caller.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 37e54f52-f4d0-4439-ac60-47fc7c5a1185
📒 Files selected for processing (1)
src/Controllers/DatabaseController.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
2330-2367: Internal fields list should match the registry in DatabaseController.The test iterates through internal fields to verify non-master updates are rejected. Ensure this list stays synchronized with
internalFieldsinDatabaseController.js. Consider extracting the field list to a shared constant or generating it dynamically from the registry to prevent drift.Suggestion: Import field list from source
If the
internalFieldsregistry is exported from DatabaseController, the test could derive the field list:const { internalFields } = require('../lib/Controllers/DatabaseController'); const masterWriteFields = Object.keys(internalFields).filter(k => internalFields[k].masterWrite);This would keep tests automatically in sync with implementation changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 2330 - 2367, The hard-coded internalFields array in the test should be sourced from the authoritative registry in DatabaseController rather than duplicated; update the test to require/import the registry (e.g., DatabaseController's internalFields or its registry map) and derive the list to test (for example filter keys where masterWrite is true) so the test uses the same symbols as DatabaseController (internalFields, masterWriteFields) and stays in sync with implementation changes.
🤖 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 2330-2367: The hard-coded internalFields array in the test should
be sourced from the authoritative registry in DatabaseController rather than
duplicated; update the test to require/import the registry (e.g.,
DatabaseController's internalFields or its registry map) and derive the list to
test (for example filter keys where masterWrite is true) so the test uses the
same symbols as DatabaseController (internalFields, masterWriteFields) and stays
in sync with implementation changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5488da10-15bb-49a9-bf66-46b51035136d
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/Controllers/DatabaseController.js
mtrezza
changed the title
|
🎉 This change has been released in version 9.6.0-alpha.10 |
2 participants
Pull Request
Issue
SQL injection via query field name when using PostgreSQL (GHSA-c442-97qw-j6c6)
Tasks
Summary by CodeRabbit
Bug Fixes
Tests
Refactor