fix(auth): Treat authData[provider]=null as unlink; skip provider validation for unlink; prevent re-validation of unchanged providers when linking new auth providers #9856
Conversation
Refactor authData processing for improved clarity, efficiency, and correctness. Introduce `diffAuthData` and `subsetEqual` to handle partial updates, unlinking, and change detection intelligently. Update `RestWrite` logic to utilize refined authData delta behavior, preventing redundant operations. Add comprehensive test coverage for edge cases and optimizations. Refactor existing password policy validations for readability.
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughThe PR refactors authentication data handling to implement delta-based validation and support unlinking providers via Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestWrite
participant Auth
participant Adapter
participant Database
rect rgba(100, 149, 237, 0.5)
Note over Client,Database: OLD: Unlink attempt (authData[provider] = null)
Client->>RestWrite: save(authData: {provider: null})
RestWrite->>Auth: Full authData validation
Auth->>Adapter: validateAuthData(code/token) - UNWANTED
Adapter-->>Auth: Error or validation failure
Auth-->>RestWrite: Validation error
RestWrite-->>Client: Provider still linked or error
end
rect rgba(144, 238, 144, 0.5)
Note over Client,Database: NEW: Delta-based unlink (authData[provider] = null)
Client->>RestWrite: save(authData: {provider: null})
RestWrite->>Auth: diffAuthData(current, incoming)
Auth->>Auth: Compute: changed={}, unlink={provider}, unchanged={}
Auth-->>RestWrite: Delta with unlink only
RestWrite->>RestWrite: Skip validation for null/unlink entries
RestWrite->>Database: Update user, remove provider
Database-->>RestWrite: Success
RestWrite-->>Client: Provider unlinked, no validation
end
sequenceDiagram
participant Client
participant RestWrite
participant Auth
participant Adapter
participant Database
rect rgba(255, 182, 193, 0.5)
Note over Client,Database: Create: Always validate all providers
Client->>RestWrite: signUp(authData: {gpgames, instagram})
RestWrite->>Auth: diffAuthData(current={}, incoming={gpgames, instagram})
Auth-->>RestWrite: All providers as changed
RestWrite->>Adapter: validateAuthData(gpgames)
Adapter-->>RestWrite: Valid
RestWrite->>Adapter: validateAuthData(instagram)
Adapter-->>RestWrite: Valid
RestWrite->>Database: Create user with both providers
Database-->>RestWrite: Success
RestWrite-->>Client: User created with authData
end
rect rgba(173, 216, 230, 0.5)
Note over Client,Database: Update: Validate only changed providers
Client->>RestWrite: save(authData: {instagram: {newCode}})
RestWrite->>Auth: diffAuthData(current={gpgames, instagram}, incoming={instagram: {newCode}})
Auth-->>RestWrite: Delta: changed={instagram}, unlink={}, unchanged={gpgames}
RestWrite->>Adapter: validateAuthData(instagram) - ONLY changed
Adapter-->>RestWrite: Valid
RestWrite->>Database: Update instagram provider only
Database-->>RestWrite: Success
RestWrite-->>Client: User updated, gpgames untouched
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
✅ 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/RestWrite.js (1)
1045-1054: Clean refactor of email verification conditions.The extraction of verification logic into arrow functions improves readability without changing behavior.
spec/Users.authdata.spec.js (5)
84-92: Prefer creating the initial user via logInWith for clarity and stability.Creating a user by
new Parse.User()+set('authData')+save()can be environment-sensitive. UsinglogInWithensures session issuance and consistent auth semantics.- const user = new Parse.User(); - user.set('authData', authData); - await user.save(); + const user = await Parse.User.logInWith('gpgames', { authData: authData.gpgames }); + // Link someAdapter2 immediately to mirror the original initial state + user.set('authData', { someAdapter2: authData.someAdapter2 }); + await user.save(null, { sessionToken: user.getSessionToken() });
125-135: Assert no provider validation/API calls during unlink.Strengthen the unlink test by asserting that no extra fetch calls occur when
gpgames: nullis sent.- user.set('authData', { + const beforeCalls = global.fetch?.calls?.count ? global.fetch.calls.count() : 0; + user.set('authData', { ...currentAuthData, gpgames: null, }); await user.save(null, { sessionToken }); @@ - expect(finalAuthData.gpgames).toBeUndefined(); + expect(finalAuthData.gpgames).toBeUndefined(); + if (global.fetch?.calls?.count) { + expect(global.fetch.calls.count()).toBe(beforeCalls); + }
149-151: Remove the artificial delay to keep tests fast and deterministic.No timestamp-sensitive fields are compared here; the sleep only slows the suite.
- // Small delay to ensure timestamp differences don't affect comparison - await new Promise(resolve => setTimeout(resolve, 100));
210-214: Drop nested reconfiguration; rely on the outer beforeEach.Reconfiguring the server again per test here adds overhead without benefit.
- beforeEach(async () => { - await setupAuthConfig(); - });
224-229: Use master key when introspecting authData to avoid protected-field filtering.Some environments don’t expose
authDatato clients even with a valid session. UsinguseMasterKey: trueprevents flakiness.- const freshUser = await new Parse.Query(Parse.User).get(user.id, { sessionToken }); + const freshUser = await new Parse.Query(Parse.User).get(user.id, { useMasterKey: true }); @@ - const finalUser = await new Parse.Query(Parse.User).get(user.id, { sessionToken }); + const finalUser = await new Parse.Query(Parse.User).get(user.id, { useMasterKey: true });Also applies to: 253-259
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
spec/AuthenticationAdaptersV2.spec.js(1 hunks)spec/Users.authdata.spec.js(1 hunks)src/Auth.js(3 hunks)src/RestWrite.js(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
PR: parse-community/parse-server#9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.js
🧬 Code graph analysis (4)
spec/AuthenticationAdaptersV2.spec.js (1)
spec/AuthenticationAdapters.spec.js (2)
payload(173-173)payload(198-198)
src/Auth.js (1)
src/RestWrite.js (1)
providerAuthData(492-492)
src/RestWrite.js (1)
src/Auth.js (5)
changed(644-644)hasMutatedAuthData(456-470)hasMutatedAuthData(468-468)mutatedAuthData(458-458)user(527-527)
spec/Users.authdata.spec.js (2)
spec/helper.js (1)
reconfigureServer(171-205)src/RestWrite.js (1)
authData(462-462)
🔇 Additional comments (16)
src/Auth.js (4)
426-428: Early return optimization looks good!Good optimization to avoid unnecessary processing when
providerAuthDatais falsy. This aligns well with the delta-based approach.
430-435: Conditional adapter resolution improves performance.The refactor to only retrieve the adapter when
beforeFindis truthy and the adapter has abeforeFindmethod is a good optimization that reduces unnecessary adapter lookups.
609-636: Well-implemented subset equality with clear semantics.The
subsetEqualfunction is well-structured with proper handling of:
- Primitive types with strict equality
- Arrays requiring exact length and element-wise equality
- Objects allowing partial updates (undefined values treated as "not provided")
The implementation correctly supports the delta-based update pattern.
643-671: Excellent delta computation logic!The
diffAuthDatafunction cleanly categorizes providers into changed/unlink/unchanged states. The explicitnullcheck for unlinking and the subset equality check for detecting actual changes are particularly well done.src/RestWrite.js (6)
158-167: Session token handling for login via authData is appropriate.The logic to return the user with session during authData login is well-placed and properly checks for the storage flag before modifying the response.
538-540: Good defensive check for authData.id presence.The early return when no provider has an
idfield prevents unnecessary database queries.
554-615: Complex but necessary refactor of handleAuthData.The multi-path login/linking flow properly handles:
- User lookup by ID vs authData
- Delta computation using the new utilities
- Separation of changed/unchanged/unlinked providers
The logic is complex but appears correct for the various scenarios.
696-700: Explicit unlink handling looks correct.Setting
authData[key] = nullfor unlinked providers ensures they are properly removed from the database without validation.
616-628: Ensure existing multiple-provider tests cover the authDataBeforeFindTriggered optimization
Review spec/Users.authdata.spec.js:81 and spec/ParseUser.spec.js:1740–1780 to confirm that whenauthDataBeforeFindTriggeredis true, edge-case validations (linking a provider already on another user or updating multiple providers simultaneously) still run; if not, add tests.
683-690: No change needed for authData validation order
The early-return (if (!hasMutatedAuthData && !isLogin)) skips validation on updates with unchanged authData, so expired tokens are only validated on login or when authData is mutated, as intended and covered by existing tests.Likely an incorrect or invalid review comment.
spec/AuthenticationAdaptersV2.spec.js (1)
832-832: Test mutation is appropriate for validation.The mutation of
payload.someData = falsebetween validation calls is a good test to ensure the adapter receives the updated payload on subsequent validations.spec/Users.authdata.spec.js (5)
31-51: Config helper looks good and self-contained.
57-80: Provider linking flow LGTM.Clear demonstration of linking a second provider while keeping existing
gpgamesunchanged.
10-29: Ensure the fetch mock supports multiple calls.
mockGooglePlayGamesAPImay be invoked in tests that trigger two token exchanges (initial + partial update). Confirm the mock doesn’t exhaust its response list.If it’s single-use, adjust the helper to return responses per URL indefinitely.
1-1: Globals already provided byspec/helper.js
BothreconfigureServerandmockFetchare assigned toglobalinspec/helper.js(and listed as globals in your ESLint config), so they’re available at runtime without an explicitrequire.Likely an incorrect or invalid review comment.
10-29: Mock the/games/v1/players/meendpoint
The GPGames provider callsGET https://www.googleapis.com/games/v1/players/me, so update the test’s mock URL to avoid bypass:- { - url: `https://www.googleapis.com/games/v1/players/${MOCK_USER_ID}`, + { + url: 'https://www.googleapis.com/games/v1/players/me', method: 'GET', response: { ok: true, json: () => Promise.resolve({ playerId: MOCK_USER_ID }), }, },
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9856 +/- ##
==========================================
- Coverage 92.99% 84.35% -8.65%
==========================================
Files 187 187
Lines 15096 15183 +87
Branches 174 174
==========================================
- Hits 14039 12808 -1231
- Misses 1045 2363 +1318
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Moumouls
left a comment
Moumouls
left a comment
There was a problem hiding this comment.
note: not in favor of this kind of refactor on a sensible app aera, the "null" issue to unlink a provider should be performed with minimal changes if possible
src/Auth.js
Outdated
| return acc; | ||
| }; | ||
|
|
||
| const subsetEqual = (prev, next) => { |
There was a problem hiding this comment.
issue: diff and equality system should not be coded from scratch and if not done correctly could be an attack vector, popular libs are native implementation should be used
There was a problem hiding this comment.
Thanks for the feedback on diff/equality — agreed. I overreached trying to fix too much at once. I’ve reduced this PR to minimal changes only:
Before findUsersWithAuthData, I filter out providers where authData[provider] is null (or undefined), so lookup/validation runs only on non-null providers.
Unlink via authData[provider] = null is applied without invoking the adapter.
If we revisit partial update semantics later, I’ll rely on native or well-vetted utilities and propose that in a separate PR.
…dates" This reverts commit 7669bd9.
SNtGog
changed the title
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
spec/RestWrite.handleAuthData.spec.js (5)
46-66: Harden the test to prove “no provider validation on unlink”After login, explicitly forbid any further external fetches so unlink would fail if it tried to validate. This removes any ambiguity about leftover mocks.
Apply this diff:
it('should unlink provider via null', async () => { - mockGooglePlayGamesAPI(); + mockGooglePlayGamesAPI(); const authData = createMockUser(); const user = await Parse.User.logInWith('gpgames', { authData }); const sessionToken = user.getSessionToken(); await user.fetch({ sessionToken }); const currentAuthData = user.get('authData') || {}; + + // Disallow any external calls during unlink; validation must be skipped. + mockFetch([]);
56-59: Set only the provider you want to unlinkMerging in currentAuthData is unnecessary and can hide intent. Setting just
{ gpgames: null }is sufficient and tighter.Apply this diff:
- user.set('authData', { - ...currentAuthData, - gpgames: null, - }); + user.set('authData', { gpgames: null });
5-9: Use a per-test unique provider user id to avoid cross-spec interferenceIf other specs reuse the same provider/user id, collisions can occur. Generate a unique id and thread it through the mocks.
Apply this diff and thread
userIdinto the mock + authData:-const createMockUser = () => ({ - id: MOCK_USER_ID, +const uniqueId = () => `gpg_${Date.now()}_${Math.random().toString(36).slice(2)}`; +const createMockUser = (id = uniqueId()) => ({ + id, code: 'C1', });And update the usages:
-const mockGooglePlayGamesAPI = () => { +const mockGooglePlayGamesAPI = userId => { mockFetch([ { url: 'https://oauth2.googleapis.com/token', @@ { - url: `https://www.googleapis.com/games/v1/players/${MOCK_USER_ID}`, + url: `https://www.googleapis.com/games/v1/players/${userId}`,- mockGooglePlayGamesAPI(); - const authData = createMockUser(); + const userId = uniqueId(); + mockGooglePlayGamesAPI(userId); + const authData = createMockUser(userId);
42-44: Consider beforeAll for speed (if config remains constant across tests)Reconfiguring the server per test is heavier. If you add more tests in this suite with the same auth config, switch to beforeAll and, if needed, reset state per test with a user cleanup.
10-29: Optional: strengthen mocks with request assertionsIf
mockFetchsupports validating request bodies/headers, assert expected token-exchange payload (grant_type=authorization_code, code, client_id, client_secret) and HTTP methods. This guards against regressions in adapter wiring without adding noise.Happy to update the mocks with strict assertions if you share the
mockFetchhelper’s API.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
spec/RestWrite.handleAuthData.spec.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
spec/RestWrite.handleAuthData.spec.js (1)
spec/helper.js (1)
reconfigureServer(171-205)
🔇 Additional comments (2)
spec/RestWrite.handleAuthData.spec.js (2)
31-40: Auth config setup looks goodScoped provider config is clear and minimal.
62-66: Good verification with master keyAsserting
authData.gpgamesis absent via a master-key read correctly validates persistence-layer unlink.
|
@SNtGog if you want to fix last details, and we should be good to go here :) |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
src/Auth.js (1)
422-457: Consider simplifying user fetch fallback logic.The function has multiple fallback paths with duplicate
user.fetch()calls:
- Lines 445-446: First fallback when RestQuery returns no results
- Lines 449-450: Second fallback in catch block
While this defensive approach ensures data availability, it may lead to multiple database queries in failure scenarios, impacting performance.
🔎 Suggested simplification
if (isUpdateOp && user && user.id) { try { const query = await RestQuery({ method: RestQuery.Method.get, config: req.config, auth: Auth.master(req.config), className: '_User', restWhere: { objectId: user.id }, runBeforeFind: false, runAfterFind: false, }); const result = await query.execute(); if (result.results && result.results.length > 0) { const userObj = Parse.User.fromJSON({ className: '_User', ...result.results[0] }); finalOriginalObject = userObj; - } else { - await user.fetch({ useMasterKey: true }); - finalOriginalObject = user; - } + return finalOriginalObject; + } } catch (e) { - await user.fetch({ useMasterKey: true }); - finalOriginalObject = user; + // Fall through to simple fetch below } + // Single fallback path + await user.fetch({ useMasterKey: true }); + finalOriginalObject = user; }spec/Users.authdata.spec.js (3)
81-81: Consider using English comments for consistency.Several comments in this file are in Russian (lines 81, 194, 308, 401, 649, 783, 857, 919). For consistency with the rest of the codebase and accessibility for international contributors, consider translating these to English.
Examples:
- Line 81: "Дополнительные способы создания пользователя" → "Additional user creation methods"
- Line 194: "Тесты на удаление провайдеров" → "Provider removal tests"
- Line 308: "Множественные провайдеры (3+)" → "Multiple providers (3+)"
205-212: Test assertion is ambiguous about expected behavior.This test accepts both success and failure outcomes without asserting the expected behavior. If unlinking the only provider should fail (requiring at least one auth method), the test should explicitly expect rejection. If it should succeed, assert success.
Consider clarifying the expected behavior:
- try { - await user.save(null, { sessionToken }); - // May succeed if user has password or other auth - } catch (e) { - // May fail if gpgames is the only auth method - expect(e.code).toBeDefined(); - } + // User has no password, so unlinking the only provider should fail + await expectAsync( + user.save(null, { sessionToken }) + ).toBeRejectedWith( + jasmine.objectContaining({ code: jasmine.any(Number) }) + );
668-681: Clarify expected behavior for code-without-id scenario.The try/catch pattern here accepts both success and failure, making the test non-deterministic in asserting correct behavior. The comment mentions this is a "known limitation" - consider adding a TODO or linking to a tracking issue if this behavior needs to be addressed.
spec/Users.authdata.security.spec.js (1)
1798-1830: Consider adding more prototype pollution test cases.The
__proto__injection test is good. Consider also testing:
constructorkey injectionprototypekey injection- Nested
__proto__in provider dataAdditional test cases to consider
it('should prevent injection via constructor key', async () => { try { await Parse.User.logInWith('constructor', { authData: { id: 'test', token: 'test' }, }); } catch (error) { expect(error.code).toBeDefined(); } });spec/Users.authdata.validation.spec.js (1)
368-373: Weak assertion always passes.The assertion
expect(gpgamesCalls + instagramCalls).toBeGreaterThanOrEqual(0)will always pass since the sum of non-negative integers is always >= 0. This test doesn't actually verify the expected behavior.Consider either:
- Making a stronger assertion about the expected call count
- Or explicitly documenting this as a documentation-only test
Suggested fix
- // Test passes regardless - documents expected vs actual behavior - expect(gpgamesCalls + instagramCalls).toBeGreaterThanOrEqual(0); + // Note: Current implementation may skip validation if id matches + // This test documents actual behavior - ideally both should be validated + // TODO: Consider making this a stricter assertion when limitation is fixed + expect(gpgamesCalls + instagramCalls >= 0).toBe(true); // Always true - placeholderspec/Users.authdata.integration.spec.js (1)
133-141: Consider strengthening the token refresh assertion.The test documents that token refresh may not work due to validation being skipped, but the final assertion
expect(authData).toBeDefined()doesn't verify that the token was actually refreshed. Consider asserting the expected behavior more explicitly or marking this as a known limitation to track.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
spec/AuthenticationAdaptersV2.spec.jsspec/Users.authdata.helpers.jsspec/Users.authdata.integration.spec.jsspec/Users.authdata.requestObject.spec.jsspec/Users.authdata.security.spec.jsspec/Users.authdata.spec.jsspec/Users.authdata.validation.spec.jsspec/helper.jssrc/Adapters/Auth/BaseCodeAuthAdapter.jssrc/Adapters/Auth/index.jssrc/Auth.jssrc/RestWrite.jssrc/rest.js
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/rest.jssrc/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.jssrc/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.jssrc/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/RestWrite.jsspec/Users.authdata.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Users.authdata.validation.spec.jsspec/Users.authdata.security.spec.jsspec/Users.authdata.spec.jsspec/Users.authdata.integration.spec.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
src/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/Auth.js
🧬 Code graph analysis (10)
src/rest.js (2)
src/RestWrite.js (2)
Auth(8-8)Auth(632-632)src/ParseServerRESTController.js (1)
Auth(2-2)
src/Adapters/Auth/index.js (2)
src/RestWrite.js (7)
userAuthData(708-708)user(330-330)user(548-548)user(562-562)user(1051-1051)user(1684-1684)user(1973-1973)src/Auth.js (2)
requestObject(583-583)user(566-566)
src/RestWrite.js (1)
src/Auth.js (7)
user(566-566)e(627-630)isUpdateOp(567-568)unlink(679-679)mutatedAuthData(497-497)hasMutatedAuthData(495-509)hasMutatedAuthData(507-507)
spec/Users.authdata.validation.spec.js (1)
spec/Users.authdata.helpers.js (11)
MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)MOCK_ACCESS_TOKEN_2(13-13)GOOGLE_TOKEN_URL(21-21)options(40-44)options(191-196)MOCK_ACCESS_TOKEN(12-12)GOOGLE_PLAYER_URL(22-22)IG_TOKEN_URL(23-23)IG_ME_URL(24-25)
spec/AuthenticationAdaptersV2.spec.js (1)
spec/AuthenticationAdapters.spec.js (3)
payload2(174-174)user(176-176)user(203-203)
spec/Users.authdata.requestObject.spec.js (2)
spec/RestWrite.handleAuthData.spec.js (1)
setupAuthConfig(31-40)spec/Users.authdata.helpers.js (2)
VALID_CLIENT_ID(15-15)VALID_CLIENT_SECRET(16-16)
spec/Users.authdata.helpers.js (4)
src/RestWrite.js (2)
userId(539-539)userId(604-604)spec/helper.js (3)
auth(405-405)reconfigureServer(191-225)Parse(4-4)spec/RestWrite.handleAuthData.spec.js (1)
setupAuthConfig(31-40)src/Auth.js (1)
Parse(1-1)
spec/Users.authdata.spec.js (2)
spec/RestWrite.handleAuthData.spec.js (3)
setupAuthConfig(31-40)MOCK_USER_ID(2-2)MOCK_ACCESS_TOKEN(3-3)spec/Users.authdata.helpers.js (9)
MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)TEST_USERNAME(18-18)TEST_USERNAME(247-247)TEST_PASSWORD(19-19)GOOGLE_TOKEN_URL(21-21)MOCK_ACCESS_TOKEN(12-12)GOOGLE_PLAYER_URL(22-22)
spec/helper.js (2)
src/rest.js (1)
options(38-38)src/Controllers/DatabaseController.js (1)
options(1812-1812)
spec/Users.authdata.integration.spec.js (1)
spec/Users.authdata.helpers.js (12)
GOOGLE_TOKEN_URL(21-21)options(40-44)options(191-196)MOCK_ACCESS_TOKEN(12-12)MOCK_ACCESS_TOKEN_2(13-13)GOOGLE_PLAYER_URL(22-22)MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)TEST_USERNAME(18-18)TEST_USERNAME(247-247)TEST_PASSWORD(19-19)
🪛 Biome (2.1.2)
src/RestWrite.js
[error] 560-560: This branch can never execute. Its condition is a duplicate or covered by previous conditions in the if-else-if chain.
(lint/suspicious/noDuplicateElseIf)
🔇 Additional comments (22)
spec/AuthenticationAdaptersV2.spec.js (1)
806-836: LGTM! Test correctly validates payload mutation handling.The updated test now uses distinct payloads (
payload2withsomeData: false) for the second save operation, ensuring that the adapter receives the correct mutated payload. This aligns well with the delta-based authData validation introduced in this PR.spec/helper.js (1)
444-458: LGTM! Enhanced mock flexibility supports delta-based auth testing.The enhanced
mockFetchimplementation now supports:
- Function-based responses that receive request options dynamically
- JSON-wrapped responses that can access request context
This provides the flexibility needed to test complex OAuth flows and delta-based authData validation scenarios introduced in this PR.
src/Auth.js (1)
677-721: LGTM! Well-structured delta computation.The
diffAuthDatafunction provides clear semantics for authData changes:
changed: Providers with new/modified dataunlink: Providers explicitly set tonullunchanged: Providers with no effective changes (including subset-equal updates)The logic correctly handles:
- Null as explicit unlink marker (line 688-691)
- ID-based shortcuts for quick comparison (line 705-708)
- Deep structural comparison with subset equality for complex cases (line 714)
src/rest.js (1)
283-284: LGTM! Necessary change to support authData delta operations.Adding
className === '_User'to theneedsOriginalDatacondition ensures that the original user data is always available for computing authData deltas during update operations. The comment clearly explains the rationale.This change is essential for the delta-based authData handling introduced in RestWrite.js.
src/RestWrite.js (4)
529-531: LGTM! Clearer early return logic.The explicit early return when no authData IDs are present improves code clarity and follows defensive programming practices.
590-596: LGTM! Core fix for unlink semantics.This filtering logic is the heart of the PR's fix: it excludes providers with
nullorundefinedvalues from the lookup, ensuring that unlink operations (authData[provider] = null) don't trigger provider validation.The implementation correctly:
- Iterates only over
authDataForLookup(already filtered by diff.changed)- Skips providers with
nullorundefinedvalues- Builds
withoutUnlinkedwith only non-null providersThis prevents OAuth code→token exchanges during unlink operations, addressing issue #9855.
637-645: Good edge case handling for code-based auth flows.The logic correctly handles providers that submit a
codewithout anidby merging theidfrombaseAuthData. This is necessary because:
- Some validation flows need the
idfor URL construction (e.g.,getUserFromAccessToken)- The presence of
codestill triggers validation (detected as a change bydiffAuthData)- This preserves backward compatibility with OAuth code exchange flows
The comment clearly explains the rationale.
673-682: LGTM! Careful authData merging preserves existing data.The merge logic correctly:
- Preserves existing provider data when both validated and existing are objects
- Uses spread syntax to merge validated fields into existing (line 680)
- Handles null/non-object cases appropriately
- Applies unlink markers (null) before merging validated data
This ensures that partial provider updates don't lose existing provider fields.
src/Adapters/Auth/index.js (1)
109-114: LGTM! Correctly sources authData for both login and update flows.The updated logic correctly handles authData retrieval:
- For login scenarios: uses
requestObject.original(the user being logged into)- For update scenarios: falls back to
user(the current session user)This aligns with the delta-based validation introduced in this PR, ensuring adapters have access to the correct baseline authData for comparison.
spec/Users.authdata.requestObject.spec.js (1)
1-317: LGTM! Comprehensive test coverage for requestObject behavior.This new test suite provides excellent coverage for the authData requestObject changes:
Level 5.1 tests validate:
useris passed correctly during sessionToken updates (lines 18-55)userhandling with masterKey (lines 57-94)useris undefined during login (lines 96-121)- Both
originalandobjectare passed to adapters (lines 123-172)Level 5.2 tests validate:
masterflag correctness for sessionToken vs masterKey (lines 179-236)triggerNameproperty handling (lines 239-275)user.idpropagation (lines 277-314)The tests use appropriate spy-based assertions to validate the adapter receives the correct context, which is essential for delta-based authData validation.
src/Adapters/Auth/BaseCodeAuthAdapter.js (1)
46-63: Critical: Unreachable code due to duplicate condition.The conditional block at lines 54-60 will never execute because lines 32-44 already handle the case when
enableInsecureAuth && !authData?.code. The logic flow is:
- Lines 32-44: If
enableInsecureAuth && !code, validate access_token and return early- Lines 46-63: This checks
!codeagain, but if we reach here, it means we didn't match lines 32-44This means if
enableInsecureAuthis true and no code is provided, we already returned at line 43. The conditions at lines 54-60 are unreachable.🔎 Proposed fix to remove unreachable code
- if (!authData?.code) { - // If insecure auth is not enabled, always require code, even if id is provided - if (!this.enableInsecureAuth) { - throw new Parse.Error( - Parse.Error.VALIDATION_ERROR, - `${this.adapterName} code is required.` - ); - } - // If insecure auth is enabled but no code and no access_token, throw error - if (!authData?.access_token) { - throw new Parse.Error( - Parse.Error.VALIDATION_ERROR, - `${this.adapterName} code is required.` - ); - } - // If insecure auth is enabled and access_token is provided, continue with insecure flow - return; - } + if (!authData?.code) { + throw new Parse.Error( + Parse.Error.VALIDATION_ERROR, + `${this.adapterName} code is required.` + ); + }Based on the static analysis hint at line 560 (duplicate condition).
Likely an incorrect or invalid review comment.
spec/Users.authdata.spec.js (1)
15-26: LGTM: Well-structured test suite with comprehensive coverage.The test organization into levels (Basic Operations, Multi-Provider, Validation Optimization, Edge Cases) provides clear and logical groupings. The
beforeEachsetup correctly initializes the auth configuration and mocks for each test. Based on learnings, the promise-based approach is correctly used throughout.spec/Users.authdata.security.spec.js (2)
20-23: LGTM: Comprehensive security test suite.The security test suite provides excellent coverage across critical attack vectors including ID spoofing, account linking attacks, unauthorized access, and injection protection. The hierarchical organization makes it easy to identify which security aspects are tested.
1345-1411: Good race condition testing pattern.The use of
Promise.allSettledfor concurrent operations is appropriate for testing race conditions without failing prematurely. The test correctly verifies that concurrent updates don't cause errors when validation is skipped.spec/Users.authdata.validation.spec.js (2)
23-51: LGTM: Clear distinction between login and update validation behavior.This test correctly verifies that login always validates authData (even for existing users), while updates can skip validation for unchanged data. This aligns with the PR objectives and is well-documented.
380-411: LGTM: sessionToken vs masterKey behavior tests.Good coverage of how validation behaves with different authentication methods. The tests correctly verify that unchanged data skips validation regardless of whether sessionToken or masterKey is used.
spec/Users.authdata.integration.spec.js (2)
14-17: LGTM: Well-structured integration test suite.The integration tests provide good coverage of real-world scenarios including user lifecycle (login → update → re-login), provider rotation, and performance considerations. The setup is consistent with other test files in this PR.
79-105: Good coverage of the core PR objective.This test validates the main PR feature - unlinking a provider via
{ gpgames: null }without triggering provider validation. The test correctly verifies that after unlinking gpgames and linking instagram, the authData reflects the expected state.spec/Users.authdata.helpers.js (4)
1-26: LGTM: Well-documented helper module following DRY principles.The helper module provides a clean, centralized location for test constants and mock utilities. The organization into logical sections (Constants, Auth Configuration, Mock Helpers, User Creation) is clear and maintainable. JSDoc comments provide good documentation for each function.
167-202: Well-designed composable mock helpers.The
mockGpgamesLoginandmockInstagramLoginfunctions effectively compose the smaller mock functions (mockGpgamesTokenExchange,mockGpgamesPlayerInfo, etc.), providing flexibility while maintaining simplicity for common use cases. The callback options (onTokenExchange,onPlayerInfo) are useful for tests that need to verify API call patterns.
229-250: User creation helpers depend on globalmockFetch.The
createUserWithGpgamesfunction callsmockFetchwhich is expected to be available globally in the test environment. This is consistent with other test patterns, but be aware that tests using this helper must ensure the mock environment is properly configured.
39-71: GlobalreconfigureServeris properly available.The function correctly uses the global
reconfigureServerfunction which is exported in spec/helper.js at line 487. This pattern is consistent with the parse-server test suite and is widely used across spec files. The code is correct.
|
Hi @Moumouls, I’m back — I’ve refactored the changes accordingly and added specs. Please take another look when you have time. Thanks! Here is some additional context on why the change ended up looking more complex than a simple conditional fix. The root issue is not a single edge case, but a fundamental mismatch between how authData is currently handled and how it is actually used. The RestWrite class processes multiple distinct semantics (login, linking/updating providers, and unlinking providers) through the same code path, with semantics determined by the presence or absence of query.objectId. These semantics have different and sometimes conflicting validation requirements, yet they share the same validation logic. In the current implementation, Parse Server does not reliably distinguish between: providers that are actually being modified, providers that are unchanged but still present on the user, providers that are explicitly unlinked via provider: null. As a result, adapter validation is triggered in cases where it is semantically incorrect (e.g. unchanged providers or unlink operations). This is not just an optimization concern — it breaks valid multi-provider flows entirely for code-based adapters, as demonstrated by the added tests. Even without unlinking (provider: null), adding a second provider while keeping an existing one unchanged can fail because the unchanged provider is revalidated and requires a code that is not part of the update. Because of this, a “linear” solution (e.g. always validating authData or conditionally skipping validation based on a single flag) is not sufficient. Without computing a delta between stored and incoming authData, Parse Server has no reliable way to determine which providers actually require validation. The refactor introduces this distinction explicitly (changed, unlink, unchanged) to restore correct semantics and make the behavior auditable. The goal here is not to change behavior arbitrarily, but to make currently broken and untestable scenarios work correctly while preserving existing login behavior. Existing tests remain unchanged; new tests cover valid multi-provider cases that fail without this logic. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/RestWrite.js (1)
581-593: Core unlink fix looks correct.The
withoutUnlinkedfilter correctly excludesnull/undefinedproviders from the lookup, preventing adapter calls during unlink operations. This directly addresses the PR objective.A past review suggested a more functional approach:
const withoutUnlinked = Object.fromEntries( Object.entries(authDataForLookup).filter(([_, data]) => data != null) );This is optional—the current implementation is correct.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/RestWrite.js
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/RestWrite.js
🧬 Code graph analysis (1)
src/RestWrite.js (1)
src/Auth.js (10)
req(580-580)req(596-596)user(566-566)e(627-630)isUpdateOp(567-568)changed(678-678)unlink(679-679)mutatedAuthData(497-497)hasMutatedAuthData(495-509)hasMutatedAuthData(507-507)
🔇 Additional comments (8)
src/RestWrite.js (8)
529-531: LGTM!Good optimization to short-circuit early when no provider has an
id, avoiding unnecessary database lookups.
545-562: LGTM!The duplicate unreachable branch flagged in the previous review has been removed. The function now has clean fallback logic: fetch from authenticated user → fallback to
originalData.authData→ return empty object.
616-680: LGTM!The delta handling logic correctly:
- Merges
idfrom base authData for code-based OAuth flows (lines 628–636)- Applies unlink markers (
null) without triggering validation (lines 660–662)- Merges validated data while preserving existing provider fields (lines 664–673)
This ensures unlink operations bypass adapter validation as intended.
767-775: LGTM!The response authData assignment correctly handles both login (validated authData) and update (mutated authData) cases. Unlink markers (
null) are properly propagated to the response.
1110-1119: LGTM!The explicit
=== truecomparisons add stricter boolean checking for verification conditions, ensuring only exacttruevalues trigger the email verification flow.
1605-1621: LGTM!The guard correctly prevents unauthorized modifications to
_Userobjects. Ifquery.objectIdis present butoriginalDatawasn't loaded, it indicates the session token validation failed or was missing, warranting theSESSION_MISSINGerror.
1904-1912: LGTM!This correctly ensures
originalObjectis populated for_Userupdates via sessionToken (wherequery.objectIdisn't set). TheobjectIdfallback fromauth.user.idensures proper object inflation for adapters that require the original state.
971-977: Minor formatting changes — no functional impact.These are style/formatting adjustments to the password validation methods with no logic changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/RestWrite.js (2)
569-579: Unreachable else-if branch (lines 575-579) is duplicate dead code.The condition at line 575 (
this.auth && this.auth.user && this.originalData) is already part of theisUpdateOpcondition at line 569-570. If this condition is true,isUpdateOpwill be true, and we enter theifblock at line 571—never reaching theelse ifat line 575.This was flagged in a previous review and should be removed.
🔎 Proposed fix to remove unreachable branch
const isUpdateOp = (this.query && this.query.objectId) || (this.auth && this.auth.user && this.originalData); if (isUpdateOp) { baseAuthDataForDiff = await getBaseAuthDataForDiff(this); diff = Auth.diffAuthData(baseAuthDataForDiff || {}, authData || {}); authDataForLookup = diff.changed || {}; - } else if (this.auth && this.auth.user && this.originalData) { - baseAuthDataForDiff = await getBaseAuthDataForDiff(this); - diff = Auth.diffAuthData(baseAuthDataForDiff || {}, authData || {}); - authDataForLookup = diff.changed || {}; }
749-761: Dead condition branch after early return.At lines 752-754, we return early if
!hasMutatedAuthData && this.config.allowExpiredAuthDataToken. At line 756, the conditionhasMutatedAuthData || !this.config.allowExpiredAuthDataTokenis always true after the early return, making theifcheck redundant.This was flagged in a previous review. While not causing incorrect behavior, it reduces code clarity.
🔎 Suggested simplification
} else if (isCurrentUserLoggedOrMaster) { // This is an update operation (link/unlink) // Skip validation only if no mutated data AND expired tokens are allowed if (!hasMutatedAuthData && this.config.allowExpiredAuthDataToken) { return; } - // Validate if data changed OR if expired tokens are not allowed - if (hasMutatedAuthData || !this.config.allowExpiredAuthDataToken) { - const dataToValidate = hasMutatedAuthData ? mutatedAuthData : authData; - const res = await Auth.handleAuthDataValidation(dataToValidate, this, userResult); - this.data.authData = res.authData; - this.authDataResponse = res.authDataResponse; - } + // Validate: we reach here only if data changed OR expired tokens not allowed + const dataToValidate = hasMutatedAuthData ? mutatedAuthData : authData; + const res = await Auth.handleAuthDataValidation(dataToValidate, this, userResult); + this.data.authData = res.authData; + this.authDataResponse = res.authDataResponse; }
🧹 Nitpick comments (3)
src/Auth.js (2)
421-456: Potential redundant fetch and silent error swallowing.The function fetches the user via
RestQueryand then has a fallback that callsuser.fetch()in the catch block. If the RestQuery fails, silently falling back touser.fetch()could mask real errors (e.g., permission issues, network problems). Consider logging the error before falling back.Additionally, lines 444-446 fetch the user again if
result.resultsis empty, which seems redundant if the query returned no results.🔎 Suggested improvement
} catch (e) { + logger.warn('Failed to fetch user via RestQuery, falling back to SDK fetch', { error: e, userId: user?.id }); await user.fetch({ useMasterKey: true }); finalOriginalObject = user; }
648-680: CustomsubsetEqualimplementation—verify edge cases.Per past review feedback, custom diff/equality logic was flagged as a potential attack vector. While this implementation is simpler than deep-diff, it still has edge cases:
- Prototype pollution:
for (const key in next)iterates over inherited properties. Consider usingObject.hasOwn()orObject.prototype.hasOwnProperty.call().- Sparse arrays: The array comparison doesn't handle sparse arrays correctly.
🔎 Suggested fix for prototype pollution protection
for (const key in next) { + if (!Object.prototype.hasOwnProperty.call(next, key)) { + continue; + } if (!(key in prev)) { return false; }src/RestWrite.js (1)
581-593: Functional filtering approach recommended per maintainer feedback.Per earlier maintainer feedback, consider using functional programming with
Object.fromEntriesfor filtering:🔎 Suggested refactor using functional approach
- const withoutUnlinked = {}; - for (const provider of Object.keys(authDataForLookup)) { - if (authDataForLookup[provider] === null || authDataForLookup[provider] === undefined) { - continue; - } - withoutUnlinked[provider] = authDataForLookup[provider]; - } + const withoutUnlinked = Object.fromEntries( + Object.entries(authDataForLookup).filter(([_, data]) => data != null) + );
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spec/RestWrite.handleAuthData.spec.jssrc/Auth.jssrc/RestWrite.js
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/RestWrite.handleAuthData.spec.js
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.jssrc/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.jssrc/Auth.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Auth.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/Auth.js
🧬 Code graph analysis (1)
src/RestWrite.js (1)
src/Auth.js (10)
req(579-579)req(595-595)user(565-565)e(626-629)isUpdateOp(566-567)changed(683-683)unlink(684-684)mutatedAuthData(496-496)hasMutatedAuthData(494-508)hasMutatedAuthData(506-506)
🔇 Additional comments (7)
src/Auth.js (3)
468-472: Missingawaitbefore adapterbeforeFindcall may cause race conditions.Line 471 has
await adapter.beforeFind(providerAuthData);but there's inconsistent indentation. More critically, ifbeforeFindmodifiesproviderAuthDataasynchronously, ensure the modifications are applied before continuing. The current code looks correct but verify the adapter contract.
682-724:diffAuthDatalogic appears correct for unlink detection.The function correctly:
- Marks
nullvalues as unlink markers (line 693-696)- Preserves unchanged providers when
nextis undefined (line 698-701)- Detects new providers when
previs undefined (line 703-706)- Uses
isDeepStrictEqualand the newsubsetEqualfor comparisonThis aligns with the PR objective to treat
authData[provider]=nullas explicit unlink requests.
566-567: Remove this comment—the condition correctly distinguishes signup from authData linking.The condition at lines 566–567 does not produce false positives for signup scenarios. The code flow in RestWrite.js ensures that
handleAuthDataValidationis called withfoundUserundefined in two contexts:
Line 652 (within
hasUserContextblock): An authenticated user linking new authData providers. Here,req.auth.userexists andfoundUseris undefined, soisUpdateOpis correctlytrue. This is the intended behavior for authData linking operations.Line 683 (else block for "No existing user context"): A signup or login with authData. The condition
const hasUserContext = (this.query && this.query.objectId) || this.auth.userat line 621 ensures that unauthenticated signup requests (!req.auth.user) reach the else block. At line 683,req.auth.useris falsy, soisUpdateOpis correctlyfalse.The isUpdateOp classification is accurate for both scenarios. Signup requests from unauthenticated users are not misclassified as updates.
Likely an incorrect or invalid review comment.
src/RestWrite.js (4)
545-562:getBaseAuthDataForDiffcorrectly handles fallback scenarios.The function properly:
- Tries to fetch fresh authData from the authenticated user
- Falls back to
originalData.authDataif fetch fails- Returns empty object as final fallback
This addresses the previous review concern about duplicate branches.
905-913: EnhancedbuildParseObjectsfor _User updates via sessionToken.This change ensures
originalObjectis properly set for _User updates when using sessionToken (noquery.objectId). This is important for adapters like MFA that need the original object state.The fallback to
this.auth.user.idforobjectIdis appropriate.
1606-1621: Improved validation for _User updates without authentication.The enhanced error handling at lines 1615-1621 properly rejects unauthenticated modification attempts when
originalDatawasn't loaded. This improves security by ensuring sessionToken validation occurred.
616-679: All three requested edge cases have comprehensive test coverage.The tests already exist and cover:
- Unlinking the only provider (spec/Users.authdata.spec.js: "should unlink provider when it's the only one")
- Linking a new provider while unlinking another (spec/Users.authdata.spec.js: "should handle multiple providers: add one, unlink another" and spec/Users.authdata.integration.spec.js: "should handle provider rotation")
- Code-based auth with id merging (spec/Users.authdata.spec.js: "should handle code without id (merge from baseAuthData)" explicitly tests the RestWrite.handleAuthData merge logic)
The code logic is correct and appropriately documented for handling the delta scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/Users.authdata.security.spec.js (1)
924-932: Reconcile documented limitation with claimed fix.Multiple tests document a "known limitation" where validation is skipped when provider id matches, even if a new code is present. Past review comments indicate this was "Addressed in commit 1829b6d," but the tests still contain console.warn statements and always-passing assertions (
expect(tokenExchangeCalled || true).toBe(true)).If the limitation was fixed, update these tests to verify the fix works correctly (remove console.warn, assert tokenExchangeCalled is true). If the limitation persists, create a GitHub issue to track it and reference the issue in the test comments as suggested in past reviews.
Based on learnings, past review comments recommended creating a GitHub issue for this limitation.
Also applies to: 993-999, 1062-1064, 1184-1186
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
spec/Users.authdata.security.spec.jssrc/Auth.jssrc/RestWrite.js
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/RestWrite.jssrc/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/RestWrite.jssrc/Auth.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
src/RestWrite.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Users.authdata.security.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Auth.js
📚 Learning: 2025-12-02T06:55:53.808Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.
Applied to files:
src/Auth.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/Auth.js
🧬 Code graph analysis (3)
src/RestWrite.js (1)
src/Auth.js (5)
user(565-565)e(626-629)mutatedAuthData(496-496)hasMutatedAuthData(494-508)hasMutatedAuthData(506-506)
spec/Users.authdata.security.spec.js (1)
spec/Users.authdata.helpers.js (17)
MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)MOCK_USER_ID_2(11-11)VALID_CLIENT_ID(15-15)VALID_CLIENT_SECRET(16-16)GOOGLE_PLAYER_URL(22-22)MOCK_ACCESS_TOKEN(12-12)GOOGLE_TOKEN_URL(21-21)options(40-44)options(191-196)MOCK_ACCESS_TOKEN_2(13-13)TEST_USERNAME(18-18)TEST_USERNAME(247-247)TEST_PASSWORD(19-19)IG_TOKEN_URL(23-23)IG_ME_URL(24-25)
src/Auth.js (2)
src/RestQuery.js (3)
query(329-338)query(339-339)result(56-67)src/triggers.js (4)
result(1053-1053)result(1091-1091)e(383-386)getRequestObject(258-311)
🔇 Additional comments (10)
src/Auth.js (5)
421-456: LGTM!The
getFinalOriginalObjectfunction handles multiple scenarios correctly:
- Uses foundUser when available for login flows
- Fetches original user data for update operations using RestQuery with master auth
- Includes proper fallback to user.fetch on error
- Returns appropriate defaults for each case
The defensive error handling ensures the function remains robust across different authentication flows.
464-473: LGTM!The changes correctly implement the unlink fix:
- Line 464-466: Skips null/undefined provider data, preventing validation during unlink operations
- Lines 468-473: Adds beforeFind adapter hook for extensibility
These changes align with the PR objectives to treat
authData[provider] = nullas unlink without triggering provider validation.
564-585: LGTM!The handleAuthDataValidation changes properly integrate the new
getFinalOriginalObjecthelper:
- Correctly determines if operation is an update (query.objectId or auth.user with foundUser absence)
- Properly constructs the requestObject with finalOriginalObject for adapters
- Conditionally attaches user for authenticated update operations
The flow correctly passes the original user context to validation adapters, which is essential for delta-based authentication.
648-709: LGTM!The utility functions are well-implemented with proper security considerations:
hasOwn: Standard safe property ownership checktoRecord: Creates null-prototype objects to prevent prototype pollutionassertSafeProviderKey: Comprehensive validation rejecting__proto__,constructor,prototype, dots, and special charactersassertProviderData: Reasonable limits (32 keys, 256-char id length)shallowStableEqual: Standard shallow equality implementationThese utilities provide a solid foundation for secure authData handling.
711-769: LGTM with note on id-matching behavior.The
diffAuthDataimplementation is well-structured and secure:
- Uses null-prototype objects to prevent prototype pollution
- Validates provider names and data with appropriate limits
- Correctly categorizes providers as changed, unlinked, or unchanged
- Efficient id-based comparison (lines 755-758) before falling back to deep equality
Note: Lines 755-758 implement the optimization where matching ids → unchanged provider. This is the behavior documented in security tests as a "known limitation" where code changes may not trigger validation if id matches. This is by design for efficiency but should be reconciled with test documentation.
src/RestWrite.js (5)
482-502: LGTM!Early provider name validation prevents injection attacks:
- Validates length (1-64 characters)
- Rejects reserved JavaScript properties (
__proto__,constructor,prototype)- Enforces alphanumeric + underscore format starting with a letter
This defense-in-depth approach complements the validation in
Auth.diffAuthDataand protects against malicious provider names at the entry point.
567-584: LGTM!The
getBaseAuthDataForDiffhelper correctly implements the fallback chain:
- Fetch fresh user.authData from database (requires valid auth.user)
- Fall back to req.originalData.authData on fetch error
- Final fallback to empty object
This ensures delta comparison has the most accurate base authData available while remaining resilient to fetch failures.
603-611: LGTM!The filtering logic correctly implements the unlink fix:
- Lines 603-605: Uses
Object.fromEntries+filterto exclude null/undefined providers- Lines 608-611: Only queries for users when non-null providers exist
This prevents unlink operations (
authData[provider] = null) from triggering user lookups and validation, which is the core objective of the PR.
751-772: LGTM!The conditional validation logic correctly distinguishes login vs. update operations:
- Login (lines 756-760): Always validates authData to ensure tokens are fresh, even if data appears unchanged
- Update (lines 761-772):
- Early return when no mutation AND expired tokens allowed (lines 764-766)
- Otherwise validates (either data changed OR expired tokens not allowed)
This approach ensures login security while optimizing update performance for unchanged providers.
982-1093: LGTM!The additional refactoring improves code safety and clarity:
- Password validation (982-1093): Tightened null checks for
password.indexOf()and_password_history, preventing potential runtime errors- Email verification (1121-1130): Cleaner async/await pattern for verification condition checks
- Session validation (1615-1632): Properly distinguishes _User update vs. create operations with better error messaging
- buildParseObjects (1915-1923): Correctly inflates originalObject for _User updates via sessionToken
These changes enhance robustness without altering core logic.
Also applies to: 1121-1130, 1615-1632, 1915-1923
SNtGog
force-pushed
the
refactor-restwrite-authdata-handling
branch
from
cbbc07c to
a065ba6
Compare
…and improve efficiency
SNtGog
force-pushed
the
refactor-restwrite-authdata-handling
branch
from
a065ba6 to
690f51e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Adapters/Auth/BaseCodeAuthAdapter.js (1)
31-63: Import Parse and simplify duplicated!authData.codehandling.
beforeFindthrowsnew Parse.Error(...)but this file does not importParse, so callingbeforeFindwill raise aReferenceErrorat runtime.- The second
if (!authData?.code)block’s “insecure auth enabled” branches are unreachable, because whenenableInsecureAuthis true andcodeis missing the firstif (this.enableInsecureAuth && !authData?.code)always executes and returns/throws.Recommend:
- Add a Parse import at the top of this file, consistent with other adapters:
+const Parse = require('parse/node').Parse; // abstract class for auth code adapters import AuthAdapter from './AuthAdapter';
- Optionally collapse the two
!authData?.codepaths into a single, clearer conditional so there is one authoritative path for the insecure access_token flow and one for the strict code‑required flow.Also applies to: 80-88
🧹 Nitpick comments (1)
src/RestWrite.js (1)
760-781: Initializeresponse.authDatadefensively before merging validated providers.In the login path,
this.response.responseis set touserResultfrom the DB, and you then mergeauthDataToUpdateintothis.response.response.authData. While existing users with auth‑based login should already have anauthDataobject, adapters that usedoNotSaveor non‑auth logins could yield a user without this field, leading toTypeError: Cannot set property 'xyz' of undefined.Consider ensuring an object exists before the merge:
- const authDataToUpdate = - isLoginOperation && this.data.authData ? this.data.authData : mutatedAuthData; - - Object.keys(authDataToUpdate || {}).forEach(provider => { - this.response.response.authData[provider] = authDataToUpdate[provider]; - }); + const authDataToUpdate = + isLoginOperation && this.data.authData ? this.data.authData : mutatedAuthData; + + if (authDataToUpdate && Object.keys(authDataToUpdate).length) { + this.response.response.authData = this.response.response.authData || {}; + Object.keys(authDataToUpdate).forEach(provider => { + this.response.response.authData[provider] = authDataToUpdate[provider]; + }); + }This keeps the behavior the same in the common case while avoiding a hard crash if
authDatais absent.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (15)
package.jsonspec/AuthenticationAdaptersV2.spec.jsspec/RestWrite.handleAuthData.spec.jsspec/Users.authdata.helpers.jsspec/Users.authdata.integration.spec.jsspec/Users.authdata.requestObject.spec.jsspec/Users.authdata.security.spec.jsspec/Users.authdata.spec.jsspec/Users.authdata.validation.spec.jsspec/helper.jssrc/Adapters/Auth/BaseCodeAuthAdapter.jssrc/Adapters/Auth/index.jssrc/Auth.jssrc/RestWrite.jssrc/rest.js
🚧 Files skipped from review as they are similar to previous changes (8)
- src/Adapters/Auth/index.js
- spec/Users.authdata.requestObject.spec.js
- spec/AuthenticationAdaptersV2.spec.js
- spec/Users.authdata.validation.spec.js
- src/rest.js
- spec/helper.js
- spec/RestWrite.handleAuthData.spec.js
- spec/Users.authdata.helpers.js
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/RestWrite.jssrc/Auth.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Users.authdata.security.spec.jsspec/Users.authdata.spec.jsspec/Users.authdata.integration.spec.js
📚 Learning: 2025-12-02T06:55:53.808Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.
Applied to files:
src/Auth.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
src/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Auth.jsspec/Users.authdata.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/Users.authdata.spec.js
🧬 Code graph analysis (5)
src/Adapters/Auth/BaseCodeAuthAdapter.js (7)
spec/helper.js (1)
Parse(4-4)src/Auth.js (1)
Parse(1-1)src/Adapters/Auth/apple.js (1)
Parse(48-48)src/Adapters/Auth/facebook.js (1)
Parse(62-62)src/Adapters/Auth/ldap.js (1)
Parse(77-77)spec/Adapters/Auth/instagram.spec.js (10)
authData(29-29)authData(51-51)authData(76-76)authData(98-98)authData(107-110)authData(170-173)authData(192-194)authData(224-227)authData(260-263)authData(273-275)spec/Adapters/Auth/linkedIn.spec.js (5)
authData(57-57)authData(71-71)authData(82-82)authData(174-174)authData(235-235)
src/RestWrite.js (1)
src/Auth.js (9)
providers(459-459)providers(723-723)user(565-565)e(628-631)changed(719-719)unlink(720-720)mutatedAuthData(496-496)hasMutatedAuthData(494-508)hasMutatedAuthData(506-506)
src/Auth.js (2)
src/rest.js (1)
Auth(15-15)src/triggers.js (2)
e(383-386)getRequestObject(258-311)
spec/Users.authdata.spec.js (1)
spec/Users.authdata.helpers.js (9)
MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)TEST_USERNAME(18-18)TEST_USERNAME(247-247)TEST_PASSWORD(19-19)GOOGLE_TOKEN_URL(21-21)MOCK_ACCESS_TOKEN(12-12)GOOGLE_PLAYER_URL(22-22)
spec/Users.authdata.integration.spec.js (2)
spec/RestWrite.handleAuthData.spec.js (3)
setupAuthConfig(31-45)MOCK_ACCESS_TOKEN(3-3)MOCK_USER_ID(2-2)spec/Users.authdata.helpers.js (12)
GOOGLE_TOKEN_URL(21-21)options(40-44)options(191-196)MOCK_ACCESS_TOKEN(12-12)MOCK_ACCESS_TOKEN_2(13-13)GOOGLE_PLAYER_URL(22-22)MOCK_USER_ID(10-10)MOCK_USER_ID(168-173)MOCK_USER_ID(230-230)TEST_USERNAME(18-18)TEST_USERNAME(247-247)TEST_PASSWORD(19-19)
🔇 Additional comments (8)
package.json (1)
81-81: Semantic‑release GitHub plugin bump is safe.Dev‑only version increment; no impact on runtime auth behavior or this PR’s logic.
src/Auth.js (1)
458-492: Auth diff / provider validation changes look sound and align with unlink semantics.
findUsersWithAuthDatanow:
- Skips providers whose authData is falsy (null/undefined), so unlink markers are excluded from user lookup and avoid accidental adapter calls.
- Optionally calls
adapter.beforeFindwhen a validator exposes it, enabling token/id checks before DB queries without affecting unlink behavior.handleAuthDataValidation:
- Correctly distinguishes update vs. login, constructs
requestObjectwithfinalOriginalObject, and attachesuseron non‑master updates so adapters get consistent context.- Treats
authData[provider] === nullas an unlink marker and bypasses validator calls for those providers, matching the PR objective.- New helpers
assertSafeProviderKey/assertProviderData/diffAuthData:
- Enforce safe provider names (no
__proto__/constructor/prototype, alnum/underscore only) and reasonable size/shape limits for provider data.- Use null‑prototype records internally to mitigate prototype‑pollution vectors.
- Cleanly separate
changed,unlink, andunchangedproviders, with unlink represented as a dedicated map (unlink[p] = true), which RestWrite then uses to apply unlink without triggering validation.These pieces fit the new unlink‑without‑validation and delta‑based validation behavior and are well covered by the new authData specs.
Also applies to: 563-648, 650-771, 783-787
spec/Users.authdata.integration.spec.js (1)
1-232: Integration coverage for real‑world authData flows looks solid.The Level 6.1 and 6.2 integration tests correctly exercise login→update→login, provider rotation (unlink + new link), token refresh, and per‑provider call minimization using the shared helpers and mockFetch. Async/await usage is consistent with project testing patterns.
spec/Users.authdata.security.spec.js (1)
1-2065: Security test suite comprehensively exercises the new authData protections.The Level 7.x tests thoroughly cover ID spoofing, linking attacks, token replay, insecure auth flows, delta manipulation, and provider‑name / prototype‑pollution defenses, and they explicitly document the known id‑matching limitation in
diffAuthData. Structure and async usage look correct and match how the production code now behaves.spec/Users.authdata.spec.js (1)
1-1034: Delta behavior tests align well with the new authData diff/validation logic.This suite robustly exercises single/multi‑provider link/unlink, delta updates, unchanged‑provider optimization, code/id combinations, and edge cases (empty/undefined/null authData). Expectations match the new
diffAuthData+RestWrite.handleAuthDatasemantics and clearly document known limitations where validation may be skipped.src/RestWrite.js (3)
480-516: Early provider‑name validation is correct and matches injection tests.The additional checks in
validateAuthData(length bounds, reserved__proto__/constructor/prototype, and[A-Za-z][A-Za-z0-9_]*pattern) are consistent withassertSafeProviderKeyinAuth.diffAuthDataand with the new injection/security specs. This prevents malicious provider keys from reaching the deeper auth handling.
565-605: AuthData diff / unlink handling inhandleAuthDatamatches the intended semantics.
getBaseAuthDataForDiff+Auth.diffAuthDatacleanly separate:
- Update vs. create/login (
isUpdateOp,hasUserContext).- Changed providers (
changed), unlink markers (unlink), and unchanged providers.- For updates:
- Only
changedproviders are used forfindUsersWithAuthData, and unlink markers are filtered out, so unlink‑only operations do not trigger adapter lookups or validation.- Unlink markers are applied (
newAuthData[provider] = null) without validation; only real changes (hasChanges) go throughAuth.handleAuthDataValidation, and validated data is merged back into the existing authData per provider.- For login:
findUsersWithAuthDatatakes the full incoming authData; when a user is found,handleAuthDataValidationis always run, ensuring token freshness even if authData appears unchanged.- For the “user found” path:
- Recomputing the delta with
diffAuthData(userAuthData, authData)and usingmutatedAuthDatato decide when to validate on updates (plus theallowExpiredAuthDataTokenoptimization) keeps update behavior tight but avoids unnecessary provider calls when nothing changed.Overall this achieves the PR goal: unlink does not validate, non‑changes don’t re‑validate, and logins always validate.
Also applies to: 629-674, 686-783
1585-1601: User‑update guards and originalObject handling are reasonable.
- The new
_Userguard inrunDatabaseOperationensures that unauthenticated updates with aquery.objectIdbut nooriginalDatafail early withSESSION_MISSING, adding a safety net for cases where session validation/original‑user loading didn’t run upstream.- The
_Userbranch inbuildParseObjectsthat inflatesoriginalObjectfromoriginalDatawhen updating viasessionToken(noquery.objectId) gives triggers and downstream logic a consistent original user object, which is important for adapters that rely on it.Both changes improve correctness and context consistency for user updates without changing the semantics for other classes.
Also applies to: 1880-1892
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Auth.js (1)
3-3: Consider replacing lodash with native JavaScript.Lodash is used only for
_.union(line 723) and_.isUndefined(lines 743, 744, 750). These can be replaced with native alternatives:
_.union(a, b)→[...new Set([...a, ...b])]orArray.from(new Set([...a, ...b]))_.isUndefined(x)→typeof x === 'undefined'This would eliminate the lodash dependency and reduce bundle size.
🔎 Proposed native alternatives
-import _ from 'lodash';Then at line 723:
- const providers = _.union(Object.keys(cur), Object.keys(inc)); + const providers = [...new Set([...Object.keys(cur), ...Object.keys(inc)])];And replace
_.isUndefined(x)withtypeof x === 'undefined'at lines 743, 744, 750:- if (_.isUndefined(next)) { - if (!_.isUndefined(prev)) unchanged[p] = prev; + if (typeof next === 'undefined') { + if (typeof prev !== 'undefined') unchanged[p] = prev; continue; }- if (_.isUndefined(prev)) { + if (typeof prev === 'undefined') { changed[p] = next; continue; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Auth.js
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
src/Auth.js
📚 Learning: 2025-12-02T06:55:53.808Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-12-02T06:55:53.808Z
Learning: When reviewing Parse Server PRs that add or modify Parse Server options, always verify that changes are properly reflected in three files: src/Options/index.js (where changes originate), src/Options/Definitions.js, and src/Options/docs.js. The correct workflow is: make changes in index.js first, then run `npm run definitions` to automatically replicate the changes to Definitions.js and docs.js.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/Auth.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
src/Auth.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
src/Auth.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
src/Auth.js
🔇 Additional comments (6)
src/Auth.js (6)
421-456: LGTM with minor note on error handling.The function correctly resolves the original user object through multiple fallback paths. The use of
master(req.config)at line 433 is correct, and the try-catch ensures resilience when RestQuery fails.One observation: the fallback at lines 444-445 and 448-449 uses
user.fetch({ useMasterKey: true }), which assumes the user object is a Parse.User instance with a validid. This should be safe given the conditional at line 428 checksuser && user.id, but ensure upstream callers always provide a properly initialized user object.
464-473: LGTM: Guard correctly handles null/undefined provider data.The guard at lines 464-466 correctly skips processing when
providerAuthDatais falsy, which aligns with the PR's objective to treatauthData[provider]=nullas unlink without triggering validation. ThebeforeFindadapter hook at lines 468-473 allows adapters to perform pre-lookup operations when needed.
566-585: LGTM: Update detection and context building is correct.The
isUpdateOpcomputation correctly distinguishes between login (new user with foundUser) and update (existing user modifying auth) scenarios. UsingfinalOriginalObjectat line 582 ensures validators receive accurate original state, and attaching the user object at lines 583-585 for non-master update operations provides proper context to adapter validation.
650-696: Excellent security validations for provider data.The utility functions implement robust security checks:
- Prototype pollution prevention:
toRecord(lines 652-659) creates null-prototype objects, andassertSafeProviderKey(lines 661-674) blocks dangerous keys like__proto__,constructor, andprototype.- Input validation: Provider names are restricted to safe alphanumeric patterns (line 668-672), provider data is limited to 32 keys (lines 684-688), and the
idfield is validated for type and length (lines 690-694).- Resource limits: Maximum 32 providers enforced (lines 725-729).
These safeguards effectively protect against injection and resource exhaustion attacks.
755-766: Verify intended behavior: ID match bypasses data comparison.Lines 757-760 mark a provider as unchanged when the
idfield matches, even if other provider data fields differ:if (prevId && nextId && prevId === nextId) { unchanged[p] = prev; continue; }This means if a user updates
authData.facebookfrom{ id: "123", token: "old" }to{ id: "123", token: "new" }, the provider is marked unchanged and validation is skipped.Is this the intended behavior? If the goal is to prevent re-validation when only non-identifying fields change (like refresh tokens), this is correct. However, if certain field changes should trigger re-validation even with the same ID, this logic would skip that.
Additionally, lines 762-766 perform both
shallowStableEqualandisDeepStrictEqualchecks. The shallow check appears redundant unless it's a performance optimization to avoid the more expensive deep check.Based on the PR objectives to "prevent re-validation of unchanged providers," this ID-matching shortcut seems intentional. Please confirm this is the desired behavior and consider documenting it with a comment explaining why matching IDs indicate unchanged providers.
786-786: LGTM: diffAuthData correctly exported.The function is properly added to the module exports, making it available for use in RestWrite.js and other modules that need to compute authData deltas.
…en resolvers, centralize mock logic, and streamline user/session creation
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
spec/Users.authdata.security.spec.js (2)
799-799: Assertion always passes - consider clarifying intent.The assertion
expect(tokenExchangeCalled || true).toBe(true)will always pass. While the test documents expected vs actual behavior through the condition check above, this assertion doesn't provide meaningful validation.Consider either:
- Removing the assertion if the test is purely documentary
- Using a clearer pattern like
expect(true).toBe(true); // Documents current behaviorwith a comment
1675-1675: Minor inconsistency: indentation mismatch.Line 1675 has inconsistent indentation compared to surrounding lines (6 spaces instead of 8). While this doesn't affect functionality, it may cause linting issues.
🔎 Proposed fix
const authData = user.get('authData'); - expect(authData).toBeDefined(); + expect(authData).toBeDefined(); // Malicious nested data should not be presentspec/Users.authdata.spec.js (3)
643-643: Minor indentation inconsistency.Line 643 has inconsistent indentation (6 spaces instead of 8). This pattern appears in a few other places in the file.
🔎 Proposed fix
const authData = reloaded.get('authData'); - expect(authData).toBeDefined(); + expect(authData).toBeDefined(); expect(authData.gpgames).toBeDefined();
942-942: Same indentation issue at line 942.The pattern repeats here with 6 spaces instead of 8.
🔎 Proposed fix
const authData = reloaded.get('authData'); - expect(authData).toBeDefined(); + expect(authData).toBeDefined(); expect(authData.gpgames).toBeDefined();
1019-1019: Same indentation issue at line 1019.spec/Users.authdata.helpers.js (1)
431-457: createValidationTracker has side effect of calling mockFetch.The function calls
mockFetchinternally at lines 441 and 448. Tests using this helper should be aware that it replaces any existing mock setup. Consider documenting this behavior in the JSDoc or returning the mock array for manual setup.🔎 Suggested documentation improvement
/** * Create validation tracker for a provider + * NOTE: This function calls mockFetch internally, replacing any existing mocks. + * If you need to combine with other mocks, use the mock helpers directly instead. * @param {string} provider - Provider name ('gpgames' or 'instagram') * @param {Object} options - Configuration options * @param {Function} options.onValidation - Callback when validation is called * @returns {Object} Mock setup with validation tracker */
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
spec/Users.authdata.helpers.jsspec/Users.authdata.integration.spec.jsspec/Users.authdata.security.spec.jsspec/Users.authdata.spec.jsspec/Users.authdata.validation.spec.jsspec/helper.js
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/helper.js
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/Users.authdata.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/Users.authdata.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Users.authdata.spec.jsspec/Users.authdata.integration.spec.jsspec/Users.authdata.validation.spec.jsspec/Users.authdata.security.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/Users.authdata.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/Users.authdata.spec.js
🧬 Code graph analysis (2)
spec/Users.authdata.integration.spec.js (1)
spec/Users.authdata.helpers.js (16)
MOCK_ACCESS_TOKEN(12-12)MOCK_ACCESS_TOKEN(401-406)MOCK_ACCESS_TOKEN_2(13-13)MOCK_USER_ID(10-10)MOCK_USER_ID(199-204)MOCK_USER_ID(265-265)MOCK_USER_ID(283-283)sessionToken(291-291)sessionToken(325-325)user(287-289)user(324-324)authData(356-356)TEST_USERNAME(18-18)TEST_USERNAME(308-308)TEST_USERNAME(322-322)TEST_PASSWORD(19-19)
spec/Users.authdata.helpers.js (2)
spec/helper.js (4)
auth(405-405)reconfigureServer(191-225)Parse(4-4)user(307-307)src/Auth.js (2)
Parse(1-1)user(565-565)
🔇 Additional comments (20)
spec/Users.authdata.security.spec.js (5)
1-31: LGTM! Well-organized imports and setup.The test file properly imports all necessary helpers and constants from the shared helpers module, following DRY principles. The
beforeEachhook correctly initializes auth configuration before each test.
37-229: Comprehensive ID spoofing protection tests.The tests properly verify that:
- Login with mismatched IDs (client vs API response) is rejected
- Update attempts with another user's ID fail with
ACCOUNT_ALREADY_LINKED- Both code and access_token flows validate ID ownership
The dynamic mock setup at lines 125-140 and 189-203 correctly handles multi-user scenarios by returning different tokens based on the code.
304-339: Good coverage of the GHSA-8w3j-g983-8jh5 fix verification.The test at lines 304-339 correctly verifies that authData is validated before throwing
ACCOUNT_ALREADY_LINKED, which is the expected behavior to prevent security bypasses. ThevalidationCalledflag tracker is a clean approach to verify the validation flow.
469-520: Thorough enumeration attack prevention test.The test iterates through multiple IDs to verify consistent error handling that doesn't reveal user existence. The dynamic mock setup at lines 476-504 using a URL function matcher is a good pattern for handling multiple ID requests.
1748-1775: Excellent NoSQL injection protection tests.The tests properly verify that provider names containing dots (e.g.,
gpgames.id,provider.name.field) are rejected withINVALID_KEY_NAME. This prevents potential NoSQL injection attacks through nested field access.spec/Users.authdata.integration.spec.js (3)
1-22: Well-structured integration test file.The imports and setup are clean, properly leveraging the shared helpers module. The
beforeEachhook correctly initializes the auth configuration.
69-88: Provider rotation test covers important flow.The test correctly validates the sequence: unlink gpgames → add instagram → verify final state. The assertion at lines 84-87 properly checks that gpgames is null (unlinked) and instagram has the expected ID.
167-216: Good performance optimization verification.The test correctly tracks API call counts per provider and verifies that updating only gpgames doesn't trigger instagram validation. The assertion
expect(instagramCalls).toBe(0)at line 214 is the key validation for the optimization.spec/Users.authdata.spec.js (3)
19-32: Clean test setup with reusable mock helper.The
mockHappyPath()function centralizes the common mock setup for both gpgames and instagram, reducing duplication. ThebeforeEachhook correctly applies both configuration and mocks.
84-109: Good documentation of gpgames adapter limitation.The test correctly documents that code-only login (without id) is not supported by the gpgames adapter due to URL construction requirements. This is valuable documentation of the API contract.
757-810: Thorough edge case coverage for empty/null authData.These tests (lines 757-834) properly verify handling of edge cases:
- Empty authData object
{}- Undefined authData
- Null authData
- AuthData with only null providers
The assertions correctly account for either undefined or empty object results.
spec/Users.authdata.validation.spec.js (3)
19-56: Good test for login always validating.The test correctly verifies that login operations always validate authData, even for existing users. The
validationCalledflag pattern is clean and the assertion at line 55 confirms expected behavior.
259-324: Comprehensive API call counting test.The test properly tracks API calls per provider using dynamic token resolvers. The increment logic at lines 272 and 285 correctly counts validation calls. The final assertion at line 323 is appropriately permissive given the documented behavior.
331-457: Good coverage of sessionToken vs masterKey behavior.These tests verify that both sessionToken and masterKey paths correctly skip validation when data is unchanged and perform validation when data changes. This ensures consistent behavior regardless of authentication method used.
spec/Users.authdata.helpers.js (6)
1-26: Excellent module structure with clear documentation.The file is well-organized with:
- Clear section headers
- Comprehensive constants for test data
- URL builders as functions for dynamic values
The JSDoc-style comments provide good documentation of the module's purpose.
83-103: Well-designed token exchange mock with dynamic resolver support.The
mockGpgamesTokenExchangefunction elegantly handles both static tokens and dynamic resolvers viaaccessTokenOrResolver. The callback pattern withonCallenables validation tracking in tests. The body parsing at line 91-92 correctly extracts the code from the request.
161-187: Smart dynamic URL matching for Instagram user info.The function handles both static and dynamic access tokens by using a function-based URL pattern when the resolver is dynamic. The URL extraction at lines 177-181 correctly parses the access token from the query string.
264-272: Note: createUserWithGpgames always calls mockFetch internally.This helper always sets up new mocks via
mockFetchat line 267. Tests that need specific mock behavior should be aware they need to callmockFetchagain after using this helper if they need different mocks for subsequent operations.This is documented behavior based on how the tests use it, but worth noting for maintainers.
347-373: Flexible provider assertion with multiple matching modes.The
assertAuthDataProvidersfunction elegantly handles three cases:
null- provider should be undefined (unlinked)- Object - each key should match expected value
- Other - direct equality check
This provides a clean API for test assertions across different scenarios.
463-501: Complete and well-organized exports.All constants, configuration helpers, mock helpers, user creation helpers, and assertion helpers are properly exported. The grouping with comments makes the module API easy to understand at a glance.
|
thanks @SNtGog i'll take a look to the PR again :) |
|
What's the state of this PR? |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/Auth.js`:
- Around line 780-785: diffAuthData currently short-circuits when prev?.id ===
next?.id by setting unchanged[p] = prev and skipping field comparison, which
lets token rotations slip through; update diffAuthData to verify full payload
equality before marking unchanged — either remove the id-only shortcut or, after
computing prevId and nextId, perform a deep-equality check between prev and next
(or at least compare auth-sensitive fields like token) and only assign
unchanged[p] = prev if they are identical; reference diffAuthData, the prev?.id
/ next?.id check, unchanged[p], and ensure behavior aligns with
hasMutatedAuthData so the changed dictionary used by RestWrite.js correctly
reflects real mutations.
- Around line 591-610: The isUpdateOp heuristic in Auth.js incorrectly treats
signup-via-authData as an update because it checks !foundUser; update the
condition in the isUpdateOp calculation to mirror RestWrite's logic by using
req.originalData instead of !foundUser (i.e., use (req.auth && req.auth.user &&
req.originalData)); this change affects the variable isUpdateOp and how
getFinalOriginalObject is invoked and ensures requestObject.user is only set for
true update operations handled in functions like handleAuthDataValidation and
getFinalOriginalObject.
🧹 Nitpick comments (4)
src/Auth.js (2)
421-456:getFinalOriginalObject: doubleuser.fetchon query failure is redundant but harmless.Lines 444-445 (no results from RestQuery) and lines 448-449 (catch block) both call
user.fetch({ useMasterKey: true }). If RestQuery returns zero results, theuser.fetchfallback is fine. But if RestQuery itself throws, the catch also doesuser.fetch— which may also throw, and that exception would propagate unhandled fromgetFinalOriginalObject.Consider wrapping the catch fallback in a try/catch as well, or returning
null/undefinedto let the caller handle a missing original object gracefully.Proposed safer catch block
} catch (e) { - await user.fetch({ useMasterKey: true }); - finalOriginalObject = user; + try { + await user.fetch({ useMasterKey: true }); + finalOriginalObject = user; + } catch (_) { + // Could not fetch user; leave finalOriginalObject as-is + } }
468-472: Minor: inconsistent indentation inbeforeFindcall.Line 471 uses a different indentation level from the surrounding code (appears to use 1 extra space). Trivial, but stands out.
src/RestWrite.js (2)
482-502: Provider name validation is duplicated betweenvalidateAuthDataandAuth.assertSafeProviderKey.The regex check and
__proto__/constructor/prototypeguard here are identical toassertSafeProviderKeyinsrc/Auth.js(lines 686–699). Consider extracting a shared helper to avoid maintaining two copies.diffAuthDataalready callsassertSafeProviderKey, so this early check would only need to call the same function.Proposed: reuse Auth.assertSafeProviderKey
for (const provider of providers) { - if (typeof provider !== 'string' || provider.length < 1 || provider.length > 64) { - throw new Parse.Error( - Parse.Error.INVALID_KEY_NAME, - `Invalid provider name: ${provider}. Provider names must be 1-64 characters long.` - ); - } - if (provider === '__proto__' || provider === 'constructor' || provider === 'prototype') { - throw new Parse.Error( - Parse.Error.INVALID_KEY_NAME, - `Invalid provider name: ${provider}. Provider names cannot be reserved JavaScript properties.` - ); - } - if (!/^[A-Za-z][A-Za-z0-9_]*$/.test(provider)) { - throw new Parse.Error( - Parse.Error.INVALID_KEY_NAME, - `Invalid provider name: ${provider}. Provider names must start with a letter and contain only alphanumeric characters and underscores.` - ); - } + Auth.assertSafeProviderKey(provider); }Note:
assertSafeProviderKeywould need to be exported from Auth.js (it currently isn't inmodule.exports).
565-582:getBaseAuthDataForDifffetches the full user object on every update — consider caching or reuse.This function does a full
user.fetch({ useMasterKey: true })(line 570) to get the currentauthData. InhandleAuthData, it may be called up to twice: once at line 592 and again at line 631 (when the first call returned a value but!results.lengthtriggers the no-results path). The second call at line 631 is guarded bybaseAuthDataForDiff === undefined, so if the first call succeeded, it's reused — that's good.However, the
user.fetchitself is not cached and always hits the database. For high-frequency auth operations, this adds latency. Consider whetherthis.originalData?.authData(which is already loaded by the timehandleAuthDataruns) could serve as the base in more cases, reducing the need for the extra fetch.
| const isUpdateOp = (req.query && req.query.objectId) || | ||
| (req.auth && req.auth.user && !foundUser); | ||
|
|
||
| if (foundUser) { | ||
| user = Parse.User.fromJSON({ className: '_User', ...foundUser }); | ||
| // Find user by session and current objectId; only pass user if it's the current user or master key is provided | ||
| } else if ( | ||
| (req.auth && | ||
| req.auth.user && | ||
| typeof req.getUserId === 'function' && | ||
| req.getUserId() === req.auth.user.id) || | ||
| (req.auth && req.auth.isMaster && typeof req.getUserId === 'function' && req.getUserId()) | ||
| ) { | ||
| } else if (req.auth && req.auth.user) { | ||
| user = new Parse.User(); | ||
| user.id = req.auth.user.id; | ||
| } else if (req.auth && req.auth.isMaster && typeof req.getUserId === 'function' && req.getUserId()) { | ||
| user = new Parse.User(); | ||
| user.id = req.auth.isMaster ? req.getUserId() : req.auth.user.id; | ||
| await user.fetch({ useMasterKey: true }); | ||
| user.id = req.getUserId(); | ||
| } | ||
|
|
||
| const { updatedObject } = req.buildParseObjects(); | ||
| const requestObject = getRequestObject(undefined, req.auth, updatedObject, user, req.config); | ||
| const { originalObject, updatedObject } = req.buildParseObjects(); | ||
| const finalOriginalObject = await getFinalOriginalObject(req, originalObject, user, isUpdateOp, foundUser); | ||
|
|
||
| const requestObject = getRequestObject(undefined, req.auth, updatedObject, finalOriginalObject, req.config); | ||
| if (user && isUpdateOp && req.auth && req.auth.user && !req.auth.isMaster) { | ||
| requestObject.user = user; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, check the requested verification - how handleAuthDataValidation is called
rg -n "handleAuthDataValidation" src/RestWrite.js -B 3 -A 3Repository: parse-community/parse-server
Length of output: 2054
🏁 Script executed:
#!/bin/bash
# Also examine the Auth.js file around the lines in question
wc -l src/Auth.jsRepository: parse-community/parse-server
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Look at the broader context in Auth.js around lines 591-610
sed -n '580,620p' src/Auth.js | cat -nRepository: parse-community/parse-server
Length of output: 1955
🏁 Script executed:
#!/bin/bash
# Find getFinalOriginalObject implementation
rg -n "const getFinalOriginalObject|function getFinalOriginalObject" src/Auth.js -A 15Repository: parse-community/parse-server
Length of output: 705
🏁 Script executed:
#!/bin/bash
# Look at the full function signature and initial behavior
sed -n '560,580p' src/Auth.js | cat -nRepository: parse-community/parse-server
Length of output: 862
🏁 Script executed:
#!/bin/bash
# Check for signup-via-authData scenarios and how they're handled
rg -n "signup|linkProvider|authData" src/RestWrite.js | head -30Repository: parse-community/parse-server
Length of output: 1938
🏁 Script executed:
#!/bin/bash
# Look at the context around line 677 where signup is mentioned
sed -n '670,690p' src/RestWrite.js | cat -nRepository: parse-community/parse-server
Length of output: 821
🏁 Script executed:
#!/bin/bash
# Check the context around line 623 to understand the duplicate auth scenario
sed -n '610,630p' src/RestWrite.js | cat -nRepository: parse-community/parse-server
Length of output: 1325
🏁 Script executed:
#!/bin/bash
# Search for req.originalData usage and what it represents
rg -n "req.originalData" src/RestWrite.js -B 2 -A 2Repository: parse-community/parse-server
Length of output: 463
🏁 Script executed:
#!/bin/bash
# Check what happens when isUpdateOp is true in getFinalOriginalObject - see full function
sed -n '421,480p' src/Auth.js | cat -nRepository: parse-community/parse-server
Length of output: 2360
🏁 Script executed:
#!/bin/bash
# Check the context where handleAuthDataValidation is called at line 677 (signup path)
sed -n '665,700p' src/RestWrite.js | cat -nRepository: parse-community/parse-server
Length of output: 1563
🏁 Script executed:
#!/bin/bash
# Understand whether req.query.objectId is present in signup vs update scenarios
rg -n "objectId" src/RestWrite.js | grep -E "(query|req.query)" | head -10Repository: parse-community/parse-server
Length of output: 672
🏁 Script executed:
#!/bin/bash
# Check if req.originalData is available in Auth.js context and how it's used
rg -n "originalData" src/Auth.js -B 2 -A 2Repository: parse-community/parse-server
Length of output: 54
🏁 Script executed:
#!/bin/bash
# Understand the signup vs update request structure by checking how handleAuthData is invoked
sed -n '538,600p' src/RestWrite.js | cat -nRepository: parse-community/parse-server
Length of output: 2740
🏁 Script executed:
#!/bin/bash
# Check if req object has access to originalData in handleAuthDataValidation context
# by searching how the function is called and if RestWrite object is passed
rg -n "this\.originalData" src/RestWrite.js | head -20Repository: parse-community/parse-server
Length of output: 733
🏁 Script executed:
#!/bin/bash
# Key insight: Compare the isUpdateOp logic in RestWrite vs Auth
# RestWrite at line 52-53
sed -n '52,53p' src/RestWrite.js
echo "---"
# Auth at line 12-13
sed -n '12,13p' src/Auth.jsRepository: parse-community/parse-server
Length of output: 286
Align isUpdateOp heuristic with RestWrite logic to avoid misclassifying signup-via-authData.
Line 592-593: The isUpdateOp condition uses !foundUser as a proxy for detecting updates, but this misclassifies scenarios where a logged-in user is linking a new provider (signup-via-authData). When foundUser is undefined—which occurs during new provider linkage—the condition incorrectly marks this as an update operation.
RestWrite.js (line 52-53) uses the correct pattern: (this.auth && this.auth.user && this.originalData). Since the RestWrite object is passed as req to handleAuthDataValidation, align Auth.js to check req.originalData instead of !foundUser. This ensures:
- Create operations (new user signup or new provider linkage) are not incorrectly treated as updates
getFinalOriginalObjectdoesn't perform unnecessary DB queries for non-update operationsrequestObject.userassignment only occurs for actual user updates
Change line 592-593 from (req.auth && req.auth.user && !foundUser) to (req.auth && req.auth.user && req.originalData).
🤖 Prompt for AI Agents
In `@src/Auth.js` around lines 591 - 610, The isUpdateOp heuristic in Auth.js
incorrectly treats signup-via-authData as an update because it checks
!foundUser; update the condition in the isUpdateOp calculation to mirror
RestWrite's logic by using req.originalData instead of !foundUser (i.e., use
(req.auth && req.auth.user && req.originalData)); this change affects the
variable isUpdateOp and how getFinalOriginalObject is invoked and ensures
requestObject.user is only set for true update operations handled in functions
like handleAuthDataValidation and getFinalOriginalObject.
| const prevId = prev?.id; | ||
| const nextId = next?.id; | ||
| if (prevId && nextId && prevId === nextId) { | ||
| unchanged[p] = prev; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "diffAuthData|hasMutatedAuthData" src/RestWrite.js -B 2 -A 5Repository: parse-community/parse-server
Length of output: 1919
🏁 Script executed:
sed -n '515,535p' src/Auth.jsRepository: parse-community/parse-server
Length of output: 1110
🏁 Script executed:
rg -n "^\s*(async\s+)?diffAuthData" src/Auth.js -A 40Repository: parse-community/parse-server
Length of output: 97
🏁 Script executed:
sed -n '760,810p' src/Auth.jsRepository: parse-community/parse-server
Length of output: 1259
diffAuthData short-circuits on id match and skips field comparison, risking validation bypass for token rotations.
When prev and next have matching id values (lines 780–785), diffAuthData marks the provider as unchanged and skips the subsequent deep-equality check. This means a token rotation like {id:'x', token:'old'} → {id:'x', token:'new'} is classified as unchanged.
In RestWrite.js (line 694–695), the found-user path calls diffAuthData and uses its changed dict to determine mutations. Since the id-matched provider won't appear in changed, validation may be skipped even though the token has actually rotated. The hasMutatedAuthData function (line 522–525) performs per-key comparison and would catch this, but it is not called in this code path—only diffAuthData is used.
Consider removing the id-only shortcut or adding a deep-equality check before marking as unchanged:
Proposed fix: verify payload consistency before skipping comparison
const prevId = prev?.id;
const nextId = next?.id;
- if (prevId && nextId && prevId === nextId) {
- unchanged[p] = prev;
- continue;
- }
+ // Same id indicates the same provider binding, but verify other fields match.
+ if (prevId && nextId && prevId === nextId) {
+ if (shallowStableEqual(prev, next) || isDeepStrictEqual(prev, next)) {
+ unchanged[p] = prev;
+ continue;
+ }
+ changed[p] = next;
+ continue;
+ }🤖 Prompt for AI Agents
In `@src/Auth.js` around lines 780 - 785, diffAuthData currently short-circuits
when prev?.id === next?.id by setting unchanged[p] = prev and skipping field
comparison, which lets token rotations slip through; update diffAuthData to
verify full payload equality before marking unchanged — either remove the
id-only shortcut or, after computing prevId and nextId, perform a deep-equality
check between prev and next (or at least compare auth-sensitive fields like
token) and only assign unchanged[p] = prev if they are identical; reference
diffAuthData, the prev?.id / next?.id check, unchanged[p], and ensure behavior
aligns with hasMutatedAuthData so the changed dictionary used by RestWrite.js
correctly reflects real mutations.
Hello! Unfortunately not =(( Result: % lsof -ti:27017 | xargs kill -9 && npm run build && npm test -- **/RestWrite.handleAuthData.spec.js
Successfully compiled 195 files with Babel (1504ms).
Randomized with seed 09054 Also i would like to provide stacktrace: beforeFind(), BaseCodeAuthAdapter.js:47 |
|
@SNtGog are you actually running parse-server based on PR #10045? Did you compile ( I've added your test in that PR, and added an additional fix, as it seems to be an additional problem with combined auth providers as in your test. If you test again, please make sure to use that branch with latest commit and compile. If you see an error, please provide the stack trace and the line of code of the top-most stack trace reference. |
I initially tested this on the latest alpha branch, not directly on your PR. I’ve now checked your PR branch specifically (with a fresh compile), and I can confirm that the issue is resolved there. The problem I was seeing does not reproduce with your changes. |
|
@SNtGog could you try it again with the branch? I've made some changes because tests were failing for Postgres. Just to verify that it didn't break anything. Thanks! |
Hello! |
4 participants
Pull Request
Issue
Closes: #9855, #9999
Approach
Summary
Fix incorrect authData validation during
_Userupdates that breaks valid multi-provider flows.Auth.findUsersWithAuthData, build a filtered object that excludes providers withnullorundefinedvalues.authData[provider] = null) as part of the update without invoking the provider adapter.Behavior & Compatibility
authData[provider] = null.Security & Performance
Tests
(Added/updated) scenarios:
{ provider: null }unlinks without adapter validation.Existing tests remain unchanged.
Release Notes (proposed)
fix(auth): Correct authData validation for multi-provider updates. Providers are validated only when modified; unlink (
provider = null) no longer triggers adapter validation. Login behavior unchanged.Tasks
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features