fix: SQL injection via aggregate and distinct field names in PostgreSQL adapter (GHSA-p2w6-rmh7-w8q3)

[mtrezza]

Issue

SQL injection via aggregate and distinct field names in PostgreSQL adapter (GHSA-p2w6-rmh7-w8q3)

[parse-github-assistant]

🚀 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.

Tip

• Keep pull requests small. Large PRs will be rejected. Break complex features into smaller, incremental PRs.
• Use Test Driven Development. Write failing tests before implementing functionality. Ensure tests pass.
• Group code into logical blocks. Add a short comment before each block to explain its purpose.
• We offer conceptual guidance. Coding is up to you. PRs must be merge-ready for human review.
• Our review focuses on concept, not quality. PRs with code issues will be rejected. Use an AI agent.
• Human review time is precious. Avoid review ping-pong. Inspect and test your AI-generated code.

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.

[parseplatformorg]

✅ Snyk checks have passed. No issues have been found so far.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[coderabbitai]

📝 Walkthrough

Walkthrough

Validate and reject unsafe user-supplied field names for Postgres aggregate and distinct operations; adjust aggregate router error handling to rethrow existing Parse.Error instances. Added tests exercising malicious and valid inputs for aggregate $group._id and distinct fields.

Changes

Cohort / File(s)	Summary
Tests spec/vulnerabilities.spec.js	New vulnerability test suite (GHSA-p2w6-rmh7-w8q3) that seeds TestClass and verifies malicious $group._id and distinct inputs are rejected with Parse.Error.INVALID_KEY_NAME, while valid field references succeed.
Postgres validation src/Adapters/Storage/Postgres/PostgresStorageAdapter.js	Added validateAggregateFieldName(name) enforcing /^[a-zA-Z][a-zA-Z0-9_]*$/ for aggregate/distinct segments; updated transformAggregateField() to validate and strip leading $; tightened distinct() to validate each dot-separated segment and reuse precomputed segments.
Router error handling src/Routers/AggregateRouter.js	handleFind catch block now rethrows exceptions that are already Parse.Error instances instead of wrapping them; non-Parse.Error exceptions still become Parse.Error.INVALID_QUERY.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client
    participant Router as AggregateRouter
    participant Adapter as PostgresStorageAdapter
    participant DB as Postgres DB

    Client->>Router: GET /aggregate/TestClass (with $group._id / distinct)
    Router->>Adapter: handleAggregateRequest(parsedQuery)
    Adapter->>Adapter: transformAggregateField / validateAggregateFieldName
    alt invalid segment
        Adapter-->>Router: throw Parse.Error(INVALID_KEY_NAME)
        Router-->>Client: 400 Parse.Error(INVALID_KEY_NAME) rgba(255,0,0,0.5)
    else valid segments
        Adapter->>DB: execute safe SQL using validated identifiers
        DB-->>Adapter: rows
        Adapter-->>Router: result
        Router-->>Client: 200 OK result rgba(0,128,0,0.5)
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• PR #10159: Modifies PostgresStorageAdapter to add quoting/escaping for SQL identifiers when handling dot-notated field components.
• PR #10165: Adds field-name sanitization/validation in PostgresStorageAdapter for user-supplied dotted keys to mitigate SQL injection.
• PR #9689: Related changes to AggregateRouter.handleFind error handling converting or rethrowing aggregation errors as Parse.Error instances.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The pull request description is entirely empty, failing to provide any information about the issue, approach, or completed tasks as required by the template.	Provide a complete description following the template: explain the security issue being fixed, describe the changes made, and check off completed tasks like tests and documentation.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately and specifically describes the main change: a fix for SQL injection vulnerability (GHSA-p2w6-rmh7-w8q3) affecting aggregate and distinct field names in the PostgreSQL adapter.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)

2190-2195: Reuse validateAggregateFieldName here to avoid regex drift.

distinct() currently duplicates the validation pattern inline; calling the helper keeps aggregate/distinct behavior consistent.

Proposed refactor

   const fieldSegments = fieldName.split('.');
   for (const segment of fieldSegments) {
-    if (!segment.match(/^[a-zA-Z][a-zA-Z0-9_]*$/)) {
-      throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid field name: ${fieldName}`);
-    }
+    validateAggregateFieldName(segment);
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js` around lines 2190 -
2195, Replace the inline regex validation in the distinct() implementation with
a call to the existing validateAggregateFieldName helper to avoid regex drift:
remove the manual split/match loop and invoke
validateAggregateFieldName(fieldName) (or propagate its thrown Parse.Error) so
behavior and error type remain identical; update the distinct() code path that
currently contains the const fieldSegments = fieldName.split('.') ... loop to
use validateAggregateFieldName instead.

🤖 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 253-255: The code currently slices aggregate field expressions
without verifying the required '$' prefix, causing inputs like "playerName" to
be truncated; update the logic in the function handling fieldName so it first
checks that fieldName.startsWith('$') and throws a clear validation error if
not, then extract the name via substring(1) and pass it to
validateAggregateFieldName(name) (referencing the fieldName variable and
validateAggregateFieldName function to locate the change).

---

Nitpick comments:
In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js`:
- Around line 2190-2195: Replace the inline regex validation in the distinct()
implementation with a call to the existing validateAggregateFieldName helper to
avoid regex drift: remove the manual split/match loop and invoke
validateAggregateFieldName(fieldName) (or propagate its thrown Parse.Error) so
behavior and error type remain identical; update the distinct() code path that
currently contains the const fieldSegments = fieldName.split('.') ... loop to
use validateAggregateFieldName instead.

🪄 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: 88ba5676-ddf7-46dd-b7d0-5668f73ba679

📥 Commits

Reviewing files that changed from the base of the PR and between cdd3776 and 8af6e28.

📒 Files selected for processing (3)

• spec/vulnerabilities.spec.js
• src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
• src/Routers/AggregateRouter.js

[coderabbitai]

🧹 Nitpick comments (1)

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)

2193-2198: Prefer reusing validateAggregateFieldName() in distinct() to avoid rule drift.

distinct() re-implements the same regex check already encapsulated in validateAggregateFieldName(). Reusing the helper keeps validation behavior consistent across aggregate and distinct code paths.

♻️ Proposed refactor

-    const fieldSegments = fieldName.split('.');
-    for (const segment of fieldSegments) {
-      if (!segment.match(/^[a-zA-Z][a-zA-Z0-9_]*$/)) {
-        throw new Parse.Error(Parse.Error.INVALID_KEY_NAME, `Invalid field name: ${fieldName}`);
-      }
-    }
+    const fieldSegments = fieldName.split('.');
+    for (const segment of fieldSegments) {
+      validateAggregateFieldName(segment);
+    }

Also applies to: 2204-2204

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js` around lines 2193 -
2198, The distinct() implementation duplicates the regex validation; replace the
inline segment regex loop with a call to the existing helper
validateAggregateFieldName(fieldName) so both aggregate and distinct paths share
the same validation logic—remove the manual segment splitting and regex check in
distinct() and invoke validateAggregateFieldName(fieldName) (or iterate segments
through that helper if it expects segments) to validate and throw the same
Parse.Error on invalid names.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js`:
- Around line 2193-2198: The distinct() implementation duplicates the regex
validation; replace the inline segment regex loop with a call to the existing
helper validateAggregateFieldName(fieldName) so both aggregate and distinct
paths share the same validation logic—remove the manual segment splitting and
regex check in distinct() and invoke validateAggregateFieldName(fieldName) (or
iterate segments through that helper if it expects segments) to validate and
throw the same Parse.Error on invalid names.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fd864437-902c-4b98-ac03-dd081ff6cf0d

📥 Commits

Reviewing files that changed from the base of the PR and between 8af6e28 and 9208138.

📒 Files selected for processing (1)

• src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

[codecov]

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.53%. Comparing base (fbac847) to head (9208138).
⚠️ Report is 12 commits behind head on alpha.

Files with missing lines	Patch %	Lines
...dapters/Storage/Postgres/PostgresStorageAdapter.js	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha   #10272      +/-   ##
==========================================
- Coverage   92.53%   92.53%   -0.01%     
==========================================
  Files         192      192              
  Lines       16445    16458      +13     
  Branches      226      226              
==========================================
+ Hits        15218    15230      +12     
- Misses       1207     1208       +1     
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[parseplatformorg]

🎉 This change has been released in version 9.6.0-alpha.53

[parseplatformorg]

🎉 This change has been released in version 9.6.0