fix: Unlinking auth provider triggers auth data validation

[mtrezza]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.

Issue

Unlinking auth provider triggers auth data validation.

Closes: #9856
Closes: #9855

Approach

Without the fix, the added test fails:

1) Auth Adapter features should unlink a code-based auth provider without triggering adapter validation
  Message:
    Error: gpgames code is required.
  Stack:
    error properties: Object({ code: 142 })
        at handleError (parse-server/node_modules/parse/lib/node/RESTController.js:327:15)
        at process.processTicksAndRejections (node:internal/process/task_queues:105:5)

This is a significant simplification over #9856.

The change in src/Adapters/Storage/Postgres/PostgresStorageAdapter.js also fixes a bug for Postgres (unrelated to the issue reported in #9855, but apparent with the fix implemented here). The change fixes unlinking auth data in Postgres by properly removing JSON keys instead of setting them to null. Before, when auth data had __op: 'Delete', it set the value to null in the JSONB field using json_object_set_key — the key remained with a null value. After, it uses a new generateRemove helper that uses Postgres's operator (jsonb - key) to actually remove the key from the JSONB object. Both explicit __op: 'Delete' and null values trigger key removal.

Summary by CodeRabbit

• New Features

  • Support for unlinking code-based authentication providers.

• Tests

  • Added tests covering unlink flows in multi-provider scenarios, ensuring safe removal without affecting other providers.

• Improvements

  • Unlink operations no longer trigger external validation or network calls.
  • Auth data updates correctly handle and remove provider credentials during updates.

• Documentation

  • Clarified adapter validation and after-find behavior.

[parse-github-assistant]

🚀 Thanks for opening this pull request!

[parseplatformorg]

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

Status	Scanner	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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Avoids validating or contacting code-based auth adapters when unlinking providers by treating authData[provider] = null as removal; propagates an isUpdate flag to skip adapter beforeFind during updates, adjusts Postgres JSONB removal, and adds tests confirming no external calls on unlink.

Changes

Cohort / File(s)	Summary
Tests spec/AuthenticationAdaptersV2.spec.js	Added two tests verifying unlinking a code-based provider (authData[provider] = null) does not trigger adapter validation or external HTTP calls and that other providers remain unchanged.
Auth lookup src/Auth.js	Added isUpdate parameter to findUsersWithAuthData(...); skip processing for providers with null authData and avoid calling beforeFind when echoed-back authData has no credentials in update scenarios.
Rest write flow src/RestWrite.js	Derives isUpdate (presence of a query) and passes it to Auth.findUsersWithAuthData(...) so auth handling distinguishes create vs update (unlink) paths.
Postgres storage src/Adapters/Storage/Postgres/PostgresStorageAdapter.js	Added generateRemove() helper and treat __op: 'Delete'/null authData as JSONB removal expressions in updateObjectsByQuery, producing UPDATE statements that remove provider keys.
Auth adapter docs src/Adapters/Auth/BaseCodeAuthAdapter.js	Added JSDoc comments describing validation flows and data exposure for validateLogin/validateSetUp/validateUpdate/afterFind; behavior unchanged.

Sequence Diagram

sequenceDiagram
    actor Client
    participant RestWrite
    participant Auth as Auth.findUsersWithAuthData
    participant Adapter as BaseCodeAuthAdapter
    participant Storage as PostgresStorageAdapter
    participant DB

    Client->>RestWrite: Request update with authData[provider]=null
    RestWrite->>RestWrite: isUpdate = true (query present)
    RestWrite->>Auth: findUsersWithAuthData(..., isUpdate=true)
    Auth->>Auth: Detect provider authData === null or echoed-back without credentials
    Auth->>Adapter: (skips) beforeFind/validation for removed/empty credentials
    Auth-->>RestWrite: Return no validation lookups for removed provider
    RestWrite->>Storage: updateObjectsByQuery with authData removal instruction
    Storage->>Storage: generateRemove(...) for JSONB key deletion
    Storage->>DB: Execute UPDATE removing provider key from authData
    DB-->>Storage: Ack
    Storage-->>RestWrite: Update complete
    RestWrite-->>Client: Provider unlinked (no external adapter validation)

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Possibly related PRs

• fix: AuthData validation incorrectly triggered on unchanged providers #10025: Similar changes in src/Auth.js to avoid re-validating code-based providers and prevent unnecessary adapter validation for echoed-back/null authData.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: fixing a bug where unlinking auth providers triggers validation instead of simply removing them.
Description check	✅ Passed	The PR description is complete with Issue, Approach, and explains the fix comprehensively including the related Postgres JSONB bug fix.
Linked Issues check	✅ Passed	All code changes directly address the linked issue #9855: preventing authData validation when unlinking by distinguishing between credential updates and null/echo-back scenarios, and skipping beforeFind accordingly.
Out of Scope Changes check	✅ Passed	All changes are scoped to fixing unlink auth provider validation: Auth.js query behavior, BaseCodeAuthAdapter documentation, Postgres JSONB deletion, RestWrite flags, and test coverage for unlink scenarios.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.57%. Comparing base (d186471) to head (c74b44e).

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha   #10045      +/-   ##
==========================================
+ Coverage   83.72%   92.57%   +8.84%     
==========================================
  Files         191      191              
  Lines       15573    15589      +16     
  Branches      176      176              
==========================================
+ Hits        13039    14432    +1393     
+ Misses       2518     1145    -1373     
+ Partials       16       12       -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.

[coderabbitai]

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In `@spec/AuthenticationAdaptersV2.spec.js`:
- Around line 1449-1452: Test calls to Parse.User.logInWith('gpgames') are
missing the redirect_uri in authData which the gpgames adapter's
getAccessTokenFromCode expects; update the test where user is created (the
logInWith call) to include authData.redirect_uri with the same redirect value
used earlier in the spec (so authData includes id, code, and redirect_uri) to
keep the test consistent with the initial link flow and prevent false positives
if mocks are tightened.
- Line 1411: The test is accidentally using Jasmine's focused runner: replace
the focused test declaration `fit` with a normal test declaration `it` for the
test titled "should unlink one code-based provider while echoing back another
unchanged" so the entire suite runs; locate the `fit('should unlink one
code-based provider while echoing back another unchanged', async () => {` and
change `fit` to `it`.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/Auth.js (1)

462-501: ⚠️ Potential issue | 🟡 Minor

hasMutatedAuthData treats empty provider objects as unmutated — add clarifying documentation.

Lines 490-493: The subset check using incomingKeys.some() correctly handles echoed-back data. However, if providerData is an empty object {}, the loop never executes and hasChanges remains false, meaning the provider is silently treated as unmutated. This behavior mirrors the pattern in findUsersWithAuthData (lines 432-436), which explicitly skips authData with "only id or empty" credentials. While this appears intentional for ignoring echoed-back/credential-less updates, the hasMutatedAuthData function lacks an explicit comment or test confirming this edge case. Add a clarifying comment explaining why empty objects are treated as unmutated, or document this behavior in tests.

🧹 Nitpick comments (1)

src/Adapters/Auth/BaseCodeAuthAdapter.js (1)

75-82: Consider clarifying the error message or extracting the shared guard.

The guard if (!authData?.access_token) throws "code is required." — this is technically checking for access_token, not code. While users do submit a code (which beforeFind exchanges for access_token), a developer debugging adapter internals may find this confusing since the validated field doesn't match the message.

Also, the identical guard is copy-pasted across validateLogin, validateSetUp, and validateUpdate.

♻️ Optional: extract shared guard and align the message

+ _requireAccessToken(authData) {
+   if (!authData?.access_token) {
+     throw new Parse.Error(
+       Parse.Error.VALIDATION_ERROR,
+       `${this.adapterName} code is required.`
+     );
+   }
+ }
+
  validateLogin(authData) {
-   if (!authData?.access_token) {
-     throw new Parse.Error(Parse.Error.VALIDATION_ERROR, `${this.adapterName} code is required.`);
-   }
+   this._requireAccessToken(authData);
    return {
      id: authData.id,
    }
  }

  validateSetUp(authData) {
-   if (!authData?.access_token) {
-     throw new Parse.Error(Parse.Error.VALIDATION_ERROR, `${this.adapterName} code is required.`);
-   }
+   this._requireAccessToken(authData);
    return {
      id: authData.id,
    }
  }

  validateUpdate(authData) {
-   if (!authData?.access_token) {
-     throw new Parse.Error(Parse.Error.VALIDATION_ERROR, `${this.adapterName} code is required.`);
-   }
+   this._requireAccessToken(authData);
    return {
      id: authData.id,
    }
  }

Also applies to: 84-91, 99-106

[mtrezza]

@Moumouls if you want to take a quick look at this, just a sanity check, if you see anything fishy in these changes before we merge...

[mtrezza]

@coderabbitai review this PR again for any potential security issues, given the sensitive nature of the changes in authData validation.

[coderabbitai]

🧩 Analysis chain

@mtrezza, I'll conduct a thorough security-focused review of these authData validation changes.

---

🏁 Script executed:

#!/bin/bash
# Read the modified source files to examine the actual implementation
echo "=== src/Auth.js ==="
cat src/Auth.js

echo -e "\n=== src/RestWrite.js (relevant section) ==="
# Find the handleAuthData method
sed -n '/handleAuthData/,/^  [a-zA-Z]/p' src/RestWrite.js | head -100

echo -e "\n=== src/Adapters/Auth/BaseCodeAuthAdapter.js ==="
cat src/Adapters/Auth/BaseCodeAuthAdapter.js

echo -e "\n=== src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (authData section) ==="
# Find authData handling in updateObjectsByQuery
rg -A 30 "authData.*__op.*Delete" src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

Length of output: 26282

---

🏁 Script executed:

#!/bin/bash
# Get the PostgreSQL authData update section more carefully
echo "=== PostgreSQL authData update handling ==="
rg -n -B 5 -A 20 "generateRemove" src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

echo -e "\n=== Check test file for security-relevant tests ==="
cat spec/AuthenticationAdaptersV2.spec.js | grep -A 50 "should unlink a code-based auth provider"

echo -e "\n=== Check RestWrite handleAuthData full context ==="
rg -n -B 10 -A 40 "handleAuthData = async function" src/RestWrite.js

echo -e "\n=== Check findUsersWithAuthData calls ==="
rg -n "findUsersWithAuthData" src/RestWrite.js

Length of output: 5888

---

🏁 Script executed:

#!/bin/bash
# Check authorization enforcement in the update flow
echo "=== Check authorization checks in handleAuthData flow ==="
rg -n -A 30 "isCurrentUserLoggedOrMaster" src/RestWrite.js

echo -e "\n=== Check hasMutatedAuthData implementation for security ==="
rg -n -A 25 "^const hasMutatedAuthData" src/Auth.js

echo -e "\n=== Check if there are ACL checks for authData updates ==="
rg -n "filteredObjectsByACL" src/RestWrite.js

echo -e "\n=== Verify test covers security scenarios ==="
rg -n -B 3 -A 15 "should unlink one code-based provider while echoing" spec/AuthenticationAdaptersV2.spec.js

Length of output: 5330

---

🏁 Script executed:

#!/bin/bash
# Verify the isUpdate flag logic and update vs create detection
echo "=== Check query usage in RestWrite to verify isUpdate logic ==="
rg -n "this\.query\s*=" src/RestWrite.js | head -20

echo -e "\n=== Verify create vs update pathways ==="
rg -n -B 3 -A 3 "new RestWrite" src/Rest.js | head -40

echo -e "\n=== Check if unauthorized users can manipulate authData ==="
rg -n -B 5 -A 10 "validatePermission.*authData" src/RestWrite.js

echo -e "\n=== Verify master key can't be bypassed ==="
rg -n "isMaster" src/RestWrite.js | head -15

echo -e "\n=== Check test for unauthorized unlink attempt ==="
rg -n -B 2 -A 20 "should not.*unlink.*another user|unauthorized.*unlink" spec/AuthenticationAdaptersV2.spec.js

Length of output: 1426

---

🏁 Script executed:

#!/bin/bash
# Check filteredObjectsByACL implementation for security
echo "=== Check filteredObjectsByACL implementation ==="
rg -n -B 5 -A 20 "^RestWrite.prototype.filteredObjectsByACL" src/RestWrite.js

echo -e "\n=== Check if there are tests for unauthorized authData manipulation ==="
rg -n -C 5 "ACCOUNT_ALREADY_LINKED" spec/AuthenticationAdaptersV2.spec.js

echo -e "\n=== Check for session-based authorization in RestWrite ==="
rg -n "sessionToken" src/RestWrite.js | head -10

echo -e "\n=== Verify RestWrite constructor and query parameter ==="
rg -n -A 15 "^function RestWrite" src/RestWrite.js

echo -e "\n=== Check Routes.js for how RestWrite is instantiated ==="
fd -e js Routes.js -x cat {}

Length of output: 2069

---

🏁 Script executed:

#!/bin/bash
# Check how update vs create is determined and authorized
echo "=== Check create vs update in routers ==="
rg -n "new RestWrite" src/routers/*.js | head -30

echo -e "\n=== Check if ACL permissions are properly enforced for updates ==="
rg -n -B 3 -A 10 "canUpdateField.*authData" src/RestWrite.js

echo -e "\n=== Examine validateClientClassCreation for authData ==="
rg -n -B 5 -A 10 "validateClientClassCreation" src/RestWrite.js | head -40

echo -e "\n=== Check if there are any bypass scenarios in handleAuthData ==="
rg -n -B 2 -A 2 "master.*authData|authData.*master" src/RestWrite.js

echo -e "\n=== Look at actual test to understand the security scenario ==="
sed -n '1350,1450p' spec/AuthenticationAdaptersV2.spec.js

Length of output: 4620

---

@mtrezza, I've completed a comprehensive security review of the authData validation changes. Here's my analysis:

✅ Security Controls Verified

1. Authorization Enforcement
The code properly restricts authData modifications to authorized users only:

• Line 544-545: Updates are correctly detected via isUpdate = !!this.query
• Line 580-582: isCurrentUserLoggedOrMaster check ensures only the account owner or master key can modify authData
• Line 586: Operations only proceed if authorized
• Line 546: Results are filtered by ACL before any authData processing

Result: Unauthorized users cannot unlink providers from other users' accounts.

2. Unlink Operation Detection
Null value handling is secure:

• Line 420-423 in Auth.js: Providers with null value are identified early and skip validation
• Line 473-475: hasMutatedAuthData correctly recognizes null as an unlink mutation
• handleAuthDataValidation properly handles null providers (skips validation)

Result: Unlink operations are correctly differentiated from link/update operations.

3. Credential vs Echo-Back Detection
The hasCredentials check prevents validation bypass:

• Line 417-418: Detects if authData contains keys other than 'id'
• Line 422-426: Only skips beforeFind validation when no credentials present AND it's an update
• This correctly handles the afterFind scenario where only {id: 'x'} is returned to client

Result: Echoing back existing authData doesn't trigger unnecessary validation, but actual credential changes do.

4. Cross-User Attack Prevention
Even if an attacker attempts to link to another user's provider ID:

• The database query finds the other user (line 428-430)
• Authorization check fails at line 550: foundUserIsNotCurrentUser
• Throws ACCOUNT_ALREADY_LINKED error (line 556)

Result: Cannot hijack another user's authentication provider.

5. Database Layer Security
PostgreSQL adapter properly handles authData deletion:

• Lines 1619-1620: Treats __op: 'Delete' as null
• Line 1623-1626: Uses generateRemove for safe JSONB key removal
• No SQL injection vectors introduced

Result: Database operations are secure.

🟡 Minor Observations (Not Security Issues)

1. isUpdate Flag Simplicity
The isUpdate = !!this.query logic is straightforward but relies on query being set correctly during RestWrite construction. Based on the constructor (line 31, 78), this appears correct for all update scenarios.

2. filteredObjectsByACL Implementation
Lines 496-507 show a basic ACL filter that removes locked accounts (empty ACL). The full ACL permission check happens via the authorization check at line 580-582, so this is appropriate.

3. Test Coverage
The new tests verify:

• Unlinking doesn't trigger adapter validation
• No external API calls during unlink
• Multi-provider scenarios work correctly
• However, no explicit test for unauthorized unlink attempts (though existing test coverage likely handles this)

🔒 Security Conclusion

No security vulnerabilities identified. The changes properly:

• Maintain authorization controls
• Prevent validation bypass
• Handle unlink operations securely
• Protect against cross-user attacks
• Preserve database integrity

The fix correctly addresses the original issue (unlink triggering validation) while maintaining security boundaries. The isUpdate flag and hasCredentials check work together to distinguish between legitimate echo-back scenarios and actual authentication changes.

The sensitive nature of authData validation is well-handled through multiple layers of security:

1. ACL filtering
2. Authorization checks
3. Credential validation (when appropriate)
4. Database-level integrity

Recommendation: Merge approved from a security perspective. 🎯

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[Copilot]

Pull request overview

Fixes a regression where unlinking an auth provider via authData[provider] = null incorrectly triggers adapter validation (notably for OAuth code-based adapters), and aligns Postgres authData updates with the intended “remove key” semantics.

Changes:

• Pass an “is update” signal into authData user-lookup to skip beforeFind for echoed-back { id } authData during updates and skip unlinked (null) providers.
• Update Postgres JSONB authData updates to remove provider keys (instead of leaving null values) for both null and __op: 'Delete'.
• Add specs that ensure unlinking doesn’t trigger external adapter/network validation and preserves other linked providers.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/RestWrite.js	Treats authData handling as update vs login/signup when calling authData lookup.
src/Auth.js	Skips beforeFind for unlinked providers and for echoed-back { id } authData during updates.
src/Adapters/Storage/Postgres/PostgresStorageAdapter.js	Removes JSONB authData keys on unlink instead of setting them to null.
src/Adapters/Auth/BaseCodeAuthAdapter.js	Adds/updates documentation comments around validation hooks for code-based adapters.
spec/AuthenticationAdaptersV2.spec.js	Adds tests for unlink behavior without triggering network validation and preserving other providers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mtrezza]

@SNtGog would you mind testing this PR again, to verify that #9855 is solved? I had to make some changes to tighten security since your last review. Thanks!