fix: MFA recovery code single-use bypass via concurrent requests (GHSA-2299-ghjr-6vjp)

[mtrezza]

Issue

MFA recovery code single-use bypass via concurrent requests (GHSA-2299-ghjr-6vjp)

[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

Adds a regression test for concurrent MFA recovery-code use, relaxes Mongo authData query key parsing, adds JSONB object-equality handling in Postgres for dot-notated fields, and applies optimistic-locking predicates when updating _User.authData to prevent race conditions.

Changes

Cohort / File(s)	Summary
Vulnerability & Recovery Code Test spec/vulnerabilities.spec.js	Adds GHSA-2299-ghjr-6vjp regression test: enables TOTP MFA, seeds a recovery code, issues 10 concurrent POST /1/login requests with the same recovery code, asserts exactly one succeeds and the code is consumed.
Mongo authData query parsing src/Adapters/Storage/Mongo/MongoTransform.js	Relaxes regex and mapping for _User authData keys to accept optional subfields (e.g. authData.<provider>.<subField>), emitting _auth_data_<provider>.<subField> when present instead of always .id.
Postgres JSONB dot-field handling src/Adapters/Storage/Postgres/PostgresStorageAdapter.js	Adds handling for dot-notated fields whose value is a non-$-prefixed object: translates dot path to JSONB components and emits a JSONB equality predicate with the object value stringified as a parameter.
Optimistic-locking on authData updates src/Routers/UsersRouter.js	When updating _User.authData, includes equality predicates for original authData.<provider>.<field> values that changed (optimistic-locking); wraps update in try/catch to convert OBJECT_NOT_FOUND into SCRIPT_FAILED: 'Invalid auth data'.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant UsersRouter as Router
  participant Storage as DB

  rect rgba(0,128,0,0.5)
  Client->>Router: POST /1/login (recoveryCode)
  end

  Router->>DB: SELECT user (master key, authData)
  DB-->>Router: user record (authData with recoveryCodes)
  Router->>DB: UPDATE _User SET authData.. WHERE objectId=... AND authData.<prov>.<field>=<original> (optimistic-lock)
  alt update succeeds
    DB-->>Router: 1 row updated
    Router-->>Client: 200 OK (login success)
  else update fails (no rows)
    DB-->>Router: 0 rows updated
    Router-->>Client: 403 / failure (recovery code invalid)
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• fix: Queries with object field authData.provider.id are incorrectly transformed to _auth_data_provider.id for custom classes #9932: Adjusts MongoTransform authData mapping for _User, directly related to the relaxed authData subfield parsing.
• fix: SQL Injection via dot-notation sub-key name in Increment operation on PostgreSQL (GHSA-gqpp-xgvh-9h7h) #10165: Modifies Postgres dot-notated/JSONB field handling, closely related to the JSONB equality changes here.
• fix: MFA recovery codes not consumed after use (GHSA-4hf6-3x24-c9m8) #10170: Implements recovery-code consumption logic for MFA; functionally related to the concurrent recovery-code test added in this PR.

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	No pull request description was provided by the author; the template requires sections for Issue, Approach, and Tasks, all of which are missing.	Add a pull request description following the template: include the Issue (link to GHSA-2299-ghjr-6vjp), Approach (explain the MFA recovery code reuse fix and query optimization), and confirm completed Tasks.
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly identifies the security fix (GHSA-2299-ghjr-6vjp) and the specific vulnerability being addressed (MFA recovery code single-use bypass via concurrent requests).

_{✏️ 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: 2

🤖 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`:
- Line 4242: The GHSA-2299-ghjr-6vjp test suite describe('(GHSA-2299-ghjr-6vjp)
MFA recovery code reuse via concurrent requests') is wrongly nested inside the
GHSA-g4cf-xj29-wqqr describe; close the GHSA-g4cf-xj29-wqqr describe block
before adding the new describe so both are siblings. Locate the
GHSA-g4cf-xj29-wqqr describe block and move its closing brace (and any necessary
closing parentheses) so that describe('GHSA-2299-ghjr-6vjp', ...) starts at the
same nesting level, and ensure you also fix the corresponding misplaced closing
brace later in the file (same pattern at lines ~4315-4316).

In `@src/Adapters/Storage/Postgres/PostgresStorageAdapter.js`:
- Around line 343-347: The code treats any object value as raw JSONB equality
(in the branch guarded by typeof fieldValue === 'object'), which incorrectly
converts operator-documents like { $eq:.., $ne:.., $exists:.. } into a JSON
equality predicate and breaks later operator handling in buildWhereClause;
update the logic in that branch (referencing transformDotFieldToComponents,
patterns, values, index, and fieldValue) to detect operator documents (e.g.,
object keys starting with '$' and not arrays) and skip the raw JSONB equality
path for them so they fall through to the operator-handling code in
buildWhereClause.

🪄 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: 4d2b283f-e269-4f88-81fd-3013ea6dad74

📥 Commits

Reviewing files that changed from the base of the PR and between 1915f2c and 2d2b2fa.

📒 Files selected for processing (4)

• spec/vulnerabilities.spec.js
• src/Adapters/Storage/Mongo/MongoTransform.js
• src/Adapters/Storage/Postgres/PostgresStorageAdapter.js
• src/Routers/UsersRouter.js

[coderabbitai]

🧹 Nitpick comments (1)

spec/vulnerabilities.spec.js (1)

4302-4314: Tighten the loser-path assertions.

Right now any 9 rejections satisfy this test, so it would still pass if the losing requests returned an unrelated 4xx/5xx instead of the expected SCRIPT_FAILED / "Invalid auth data" remap from src/Routers/UsersRouter.js. I’d also pin the final recovery-list size so the test catches accidental over-deletion.

🧪 Suggested assertion tightening

     const results = await Promise.allSettled(Array(10).fill().map(() => loginWithRecovery()));

     const succeeded = results.filter(r => r.status === 'fulfilled');
     const failed = results.filter(r => r.status === 'rejected');

     // Exactly one request should succeed; all others should fail
     expect(succeeded.length).toBe(1);
     expect(failed.length).toBe(9);
+    failed.forEach(result => {
+      expect(result.reason.data.code).toBe(Parse.Error.SCRIPT_FAILED);
+      expect(result.reason.data.error).toBe('Invalid auth data');
+    });

     // Verify the recovery code has been consumed
     await user.fetch({ useMasterKey: true });
     const remainingRecovery = user.get('authData').mfa.recovery;
+    expect(remainingRecovery.length).toBe(1);
     expect(remainingRecovery).not.toContain(recoveryCode);

🤖 Prompt for AI Agents

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

In `@spec/vulnerabilities.spec.js` around lines 4302 - 4314, The test should
assert that each failed promise from loginWithRecovery() fails for the expected
reason and that exactly one recovery code was removed; before calling
Promise.allSettled capture the initial recovery list length from
user.get('authData').mfa.recovery, then after awaiting results assert
succeeded.length === 1 and for every failed result assert its reason contains
the expected error signature (e.g. error.code === 'SCRIPT_FAILED' or
error.message includes "Invalid auth data" as remapped by UsersRouter.js) to
rule out unrelated 4xx/5xx failures, and finally fetch the user and assert
remainingRecovery.length === initialLength - 1 to pin the exact count.

🤖 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 4302-4314: The test should assert that each failed promise from
loginWithRecovery() fails for the expected reason and that exactly one recovery
code was removed; before calling Promise.allSettled capture the initial recovery
list length from user.get('authData').mfa.recovery, then after awaiting results
assert succeeded.length === 1 and for every failed result assert its reason
contains the expected error signature (e.g. error.code === 'SCRIPT_FAILED' or
error.message includes "Invalid auth data" as remapped by UsersRouter.js) to
rule out unrelated 4xx/5xx failures, and finally fetch the user and assert
remainingRecovery.length === initialLength - 1 to pin the exact count.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: fe951534-26c5-4d13-b0b7-b8a37d97af01

📥 Commits

Reviewing files that changed from the base of the PR and between 2d2b2fa and 4c42259.

📒 Files selected for processing (2)

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

[codecov]

Codecov Report

❌ Patch coverage is 95.45455% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.13%. Comparing base (1915f2c) to head (7d91f3d).
⚠️ Report is 8 commits behind head on alpha.

Files with missing lines	Patch %	Lines
src/Routers/UsersRouter.js	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha   #10275      +/-   ##
==========================================
- Coverage   92.53%   92.13%   -0.41%     
==========================================
  Files         192      192              
  Lines       16458    16477      +19     
  Branches      226      226              
==========================================
- Hits        15230    15181      -49     
- Misses       1208     1272      +64     
- Partials       20       24       +4     

☔ 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.54

[parseplatformorg]

🎉 This change has been released in version 9.6.0