fix: Empty authData bypasses credential requirement on signup (GHSA-wjqw-r9x4-j59v)#10219
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughRefines signup authData handling and validation: adds tests for empty/invalid authData cases and updates RestWrite to compute a hasAuthData flag, changing when and how authData is validated or short-circuited during user signup and related flows. Changes
Sequence Diagram(s)sequenceDiagram
Participant Client
Participant RestWrite
Participant Providers
Participant Database
Client->>RestWrite: signup request (username?, password?, authData)
RestWrite->>RestWrite: compute hasAuthData (is any provider object with keys?)
alt hasAuthData true
RestWrite->>Providers: validate provider authData
Providers-->>RestWrite: validation result
RestWrite->>Database: create user + session
Database-->>RestWrite: created userId, sessionToken
RestWrite-->>Client: 201 with objectId/sessionToken
else hasAuthData false
alt username+password present
RestWrite->>Database: create user + session (skip provider validation)
Database-->>RestWrite: created userId, sessionToken
RestWrite-->>Client: 201 with objectId/sessionToken
else
RestWrite-->>Client: 400 USERNAME_MISSING or UNSUPPORTED_SERVICE
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
spec/vulnerabilities.spec.js (1)
2938-2998: Strengthen rejection assertions to verify the intended failure path.Using only
toBeRejected()can mask false positives (any rejection passes). For security regression tests, assert status/error code/message.✅ Example pattern
- await expectAsync( - request({ - method: 'POST', - url: 'http://localhost:8378/1/users', - headers: { - 'Content-Type': 'application/json', - 'X-Parse-Application-Id': 'test', - 'X-Parse-REST-API-Key': 'rest', - }, - body: JSON.stringify({ authData: {} }), - }) - ).toBeRejected(); + const res = await request({ + method: 'POST', + url: 'http://localhost:8378/1/users', + headers: { + 'Content-Type': 'application/json', + 'X-Parse-Application-Id': 'test', + 'X-Parse-REST-API-Key': 'rest', + }, + body: JSON.stringify({ authData: {} }), + }).catch(e => e); + expect(res.status).toBe(400); + expect(res.data.code).toBe(Parse.Error.USERNAME_MISSING);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/vulnerabilities.spec.js` around lines 2938 - 2998, The tests currently use expectAsync(request(...)).toBeRejected() which only asserts any rejection; update each failing test (the ones calling request POST /1/users with bodies like { authData: {} }, { authData: { bogus: {} } }, and { authData: { bogus: null } }) to explicitly verify the rejection reason: call request(...) inside a try/catch (or use expectAsync(...).toBeRejectedWith(jasmine.objectContaining(...))) and assert the caught error contains the expected HTTP status (e.g., statusCode === 400 or error.status === 400) and the response body/error message indicates invalid authData or missing credentials so the test ensures the specific failure path rather than any rejection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/RestWrite.js`:
- Around line 472-474: The current guard uses hasAuthData to short-circuit
provider validation and skips validation for cases like { authData: { bogus: 'x'
} }; change the logic so that hasAuthData still gates credential flow but does
not bypass provider validation: only skip validation when the authData property
is entirely absent, and when authData exists ensure it's an object and run the
provider-specific validation (or reject if authData is present but not an
object). Update the conditional around hasAuthData /
Object.prototype.hasOwnProperty.call(this.data, 'authData') so that provider
validation functions (the authData validation routine used in this file) are
invoked whenever this.data.authData exists and is an object, and treat
non-object authData as invalid instead of skipping validation.
---
Nitpick comments:
In `@spec/vulnerabilities.spec.js`:
- Around line 2938-2998: The tests currently use
expectAsync(request(...)).toBeRejected() which only asserts any rejection;
update each failing test (the ones calling request POST /1/users with bodies
like { authData: {} }, { authData: { bogus: {} } }, and { authData: { bogus:
null } }) to explicitly verify the rejection reason: call request(...) inside a
try/catch (or use
expectAsync(...).toBeRejectedWith(jasmine.objectContaining(...))) and assert the
caught error contains the expected HTTP status (e.g., statusCode === 400 or
error.status === 400) and the response body/error message indicates invalid
authData or missing credentials so the test ensures the specific failure path
rather than any rejection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 117f8a39-6a3b-4a1f-86e7-906391feb068
📒 Files selected for processing (2)
spec/vulnerabilities.spec.jssrc/RestWrite.js
- Separate credential gating from provider validation skip - Non-object authData values now still go through provider validation - Strengthen test assertions with specific status codes and error codes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10219 +/- ##
=======================================
Coverage 92.57% 92.57%
=======================================
Files 192 192
Lines 16296 16300 +4
Branches 199 199
=======================================
+ Hits 15086 15090 +4
Misses 1193 1193
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# [9.6.0-alpha.29](9.6.0-alpha.28...9.6.0-alpha.29) (2026-03-16) ### Bug Fixes * Empty authData bypasses credential requirement on signup ([GHSA-wjqw-r9x4-j59v](GHSA-wjqw-r9x4-j59v)) ([#10219](#10219)) ([5dcbf41](5dcbf41))
# [9.6.0-alpha.29](9.6.0-alpha.28...9.6.0-alpha.29) (2026-03-16) ### Bug Fixes * Empty authData bypasses credential requirement on signup ([GHSA-wjqw-r9x4-j59v](GHSA-wjqw-r9x4-j59v)) ([#10219](#10219)) ([5dcbf41](5dcbf41))
|
🎉 This change has been released in version 9.6.0-alpha.29 |
Issue
Empty authData bypasses credential requirement on signup (GHSA-wjqw-r9x4-j59v)
Tasks