fix: Login timing side-channel reveals user existence (GHSA-mmpq-5hcv-hf2v)#10398
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. |
📝 WalkthroughWalkthroughAdds a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Router as UsersRouter
participant DB as _User (DB)
participant PW as passwordCrypto
rect rgba(200,200,255,0.5)
Client->>Router: Login request (username, password)
Router->>DB: Query user by username
DB-->>Router: No user (empty result)
Router->>PW: compare(password, dummyHash)
PW-->>Router: compare result (false)
Router-->>Client: Error INVALID_USERNAME/PASSWORD
end
sequenceDiagram
autonumber
participant Client as Client
participant Router as UsersRouter
participant DB as _User (DB)
participant PW as passwordCrypto
rect rgba(200,255,200,0.5)
Client->>Router: Login request (username, password)
Router->>DB: Query user by username
DB-->>Router: User record (may have password)
Router->>PW: compare(password, user.password || dummyHash)
PW-->>Router: compare result (true/false)
alt valid
Router-->>Client: Success (session/token)
else invalid
Router-->>Client: Error INVALID_USERNAME/PASSWORD
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10398 +/- ##
==========================================
- Coverage 92.51% 92.50% -0.02%
==========================================
Files 192 192
Lines 16721 16725 +4
Branches 229 229
==========================================
+ Hits 15470 15471 +1
- Misses 1229 1232 +3
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@spec/ParseUser.spec.js`:
- Around line 85-109: Update the test around Parse.User.logIn/signUp to
explicitly assert that logIn rejects (rather than silently allowing a resolve)
and verify the actual arguments passed to passwordCrypto.compare: for the
non-existent user case assert compare was called with the entered password and
the dummy hash (not undefined), and for the existing-user-wrong-password case
assert compare was called with the entered password and the stored user's hash;
use the existing compareSpy on passwordCrypto.compare to inspect the call
arguments after each login attempt and ensure the promise rejection is asserted
(e.g., expect/rejects or explicit fail on unexpected resolve) so the test fails
if logIn unexpectedly resolves or compare was called with undefined.
In `@src/Routers/UsersRouter.js`:
- Around line 111-117: When a user record lacks user.password the code path
returns immediately and thus leaks timing; update the branch that handles
missing passwords in UsersRouter (the logic surrounding passwordCrypto.compare
and passwordCrypto.dummyHash) to call passwordCrypto.compare(password,
passwordCrypto.dummyHash) instead of returning immediately, await/then the
promise and then throw the same Parse.Error(Parse.Error.OBJECT_NOT_FOUND,
'Invalid username/password.') so both missing-password users and nonexistent
users go through the identical dummy compare timing path.
🪄 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: ca157313-93b8-4672-b2f9-39fb3773d49e
📒 Files selected for processing (3)
spec/ParseUser.spec.jssrc/Routers/UsersRouter.jssrc/password.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Routers/UsersRouter.js`:
- Around line 130-132: The current fallback assigns passwordCrypto.dummyHash to
hashedPassword and returns passwordCrypto.compare(password, hashedPassword),
which lets the dummy value decide authentication; instead, preserve the dummy
compare for timing but never allow it to authenticate: in the branch where
user.password is not a string (use passwordCrypto.dummyHash), still call/await
passwordCrypto.compare(password, passwordCrypto.dummyHash) to consume equivalent
time, but always return false for that case; when user.password is a string,
continue to return the actual passwordCrypto.compare result. Reference:
user.password, passwordCrypto.dummyHash, passwordCrypto.compare, hashedPassword.
🪄 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: f18fb2a1-9ded-4ffc-ad6e-0bad9ddfbeb9
📒 Files selected for processing (2)
spec/ParseUser.spec.jssrc/Routers/UsersRouter.js
✅ Files skipped from review due to trivial changes (1)
- spec/ParseUser.spec.js
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Routers/UsersRouter.js (1)
130-134: Add or point to a regression test for the passwordless fallback.This branch is the security-sensitive part of the fix, and the provided context only mentions coverage for nonexistent vs wrong-password users. A focused spec that makes
passwordCrypto.compare()resolve truthy here and still expects/loginor/verifyPasswordto fail would help prevent the dummy result from becoming authoritative again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Routers/UsersRouter.js` around lines 130 - 134, Add a regression test that exercises the passwordless branch in UsersRouter.js: locate the handler(s) that call passwordCrypto.compare (the login and verifyPassword routes) and create a test user with user.password not a string (e.g., null) so the code path with passwordCrypto.dummyHash is hit; stub/mock passwordCrypto.compare to resolve truthy and assert that the HTTP response for /login and /verifyPassword still fails (unauthorized) despite the compare resolving truthy, and also verify passwordCrypto.compare was called with passwordCrypto.dummyHash to prove the dummy-compare behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Routers/UsersRouter.js`:
- Around line 130-134: Add a regression test that exercises the passwordless
branch in UsersRouter.js: locate the handler(s) that call passwordCrypto.compare
(the login and verifyPassword routes) and create a test user with user.password
not a string (e.g., null) so the code path with passwordCrypto.dummyHash is hit;
stub/mock passwordCrypto.compare to resolve truthy and assert that the HTTP
response for /login and /verifyPassword still fails (unauthorized) despite the
compare resolving truthy, and also verify passwordCrypto.compare was called with
passwordCrypto.dummyHash to prove the dummy-compare behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a96a7aeb-9830-4a4e-bae1-990bba00b232
📒 Files selected for processing (1)
src/Routers/UsersRouter.js
mtrezza
changed the title
# [9.8.0-alpha.6](9.8.0-alpha.5...9.8.0-alpha.6) (2026-04-05) ### Bug Fixes * Login timing side-channel reveals user existence ([GHSA-mmpq-5hcv-hf2v](GHSA-mmpq-5hcv-hf2v)) ([#10398](#10398)) ([531b9ab](531b9ab))
|
🎉 This change has been released in version 9.8.0-alpha.6 |
2 participants
Issue
Login timing side-channel reveals user existence (GHSA-mmpq-5hcv-hf2v)
Tasks
Add security checkAdd benchmark