regression: Password TOTP failing for meteor methods#40626
Conversation
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
|
WalkthroughThe 2FA validation flow in ChangesTwo-Factor Authentication Validation Flow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40626 +/- ##
===========================================
- Coverage 69.80% 69.72% -0.08%
===========================================
Files 3325 3325
Lines 122974 122975 +1
Branches 21943 21895 -48
===========================================
- Hits 85839 85742 -97
- Misses 33784 33884 +100
+ Partials 3351 3349 -2
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
did you think about any way of adding a test for this? maybe an e2e checking the proper behavior of the TOTP modal? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/client/lib/2fa/process2faReturn.ts (1)
43-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize object-shaped
emailOrUsernamebefore building email props.
getProps('email', ...)still drops{ username },{ email }, and{ id }inputs togetUser()?.username. In the pre-login 2FA flow that can beundefined, so the email modal fails the assertion and this newresendEmailaction never gets attached.Suggested fix
+const resolveEmailOrUsername = ( + emailOrUsername?: { username: string } | { email: string } | { id: string } | string, +): string | undefined => { + if (typeof emailOrUsername === 'string') { + return emailOrUsername; + } + + if (!emailOrUsername) { + return undefined; + } + + if ('username' in emailOrUsername) { + return emailOrUsername.username; + } + + if ('email' in emailOrUsername) { + return emailOrUsername.email; + } + + return emailOrUsername.id; +}; + const getProps = ( method: 'totp' | 'email' | 'password', emailOrUsername?: { username: string } | { email: string } | { id: string } | string, ) => { switch (method) { case 'totp': return { method }; case 'email': return { method, - emailOrUsername: typeof emailOrUsername === 'string' ? emailOrUsername : getUser()?.username, + emailOrUsername: resolveEmailOrUsername(emailOrUsername) ?? getUser()?.username, }; case 'password': return { method }; } };Also applies to: 168-171
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/meteor/client/lib/2fa/process2faReturn.ts` around lines 43 - 54, getProps currently ignores object-shaped emailOrUsername inputs and always falls back to getUser()?.username for the 'email' case; change getProps to normalize emailOrUsername first by checking if it's an object and extracting a string from keys like username, email, or id (e.g., if typeof emailOrUsername !== 'string' and emailOrUsername?.username/emailOrUsername?.email/emailOrUsername?.id exists, use that value) and only then fall back to getUser()?.username; apply the same normalization logic to the other occurrence that builds email props so object inputs ( {username}, {email}, {id} ) are preserved and the resendEmail flow is properly attached.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/meteor/client/lib/2fa/process2faReturn.ts`:
- Around line 43-54: getProps currently ignores object-shaped emailOrUsername
inputs and always falls back to getUser()?.username for the 'email' case; change
getProps to normalize emailOrUsername first by checking if it's an object and
extracting a string from keys like username, email, or id (e.g., if typeof
emailOrUsername !== 'string' and
emailOrUsername?.username/emailOrUsername?.email/emailOrUsername?.id exists, use
that value) and only then fall back to getUser()?.username; apply the same
normalization logic to the other occurrence that builds email props so object
inputs ( {username}, {email}, {id} ) are preserved and the resendEmail flow is
properly attached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28e455e9-d3ea-43ad-a556-1100da9c49e2
📒 Files selected for processing (1)
apps/meteor/client/lib/2fa/process2faReturn.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/client/lib/2fa/process2faReturn.ts
🧠 Learnings (5)
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-05-11T20:30:35.265Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 40480
File: apps/meteor/client/meteor/startup/accounts.ts:59-61
Timestamp: 2026-05-11T20:30:35.265Z
Learning: In Rocket.Chat’s Meteor client code, when calling `dispatchToastMessage` with `{ type: 'error' }`, pass the raw caught error object as `message` without manual normalization. `dispatchToastMessage` is designed to accept `message: unknown` for error toasts, so avoid converting errors to strings (e.g., `String(error)`) or extracting `error.message` before passing them.
Applied to files:
apps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/client/lib/2fa/process2faReturn.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/client/lib/2fa/process2faReturn.ts
🔇 Additional comments (1)
apps/meteor/client/lib/2fa/process2faReturn.ts (1)
8-8: LGTM!Also applies to: 35-37, 143-150
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 5 files
| Severity | Count |
|---|---|
| 🔴 High | 3 |
| 🟡 Medium | 2 |
Comments Outside Diff (5)
🔴 High: Potential Session Fixation via Client-Side Deep Link Redirection
Location: apps/meteor/client/views/root/hooks/useLoginOtherClients.ts:6-30
The useLoginOtherClients hook in AppLayout directly reads the resumeToken and userId from the URL search parameters and automatically redirects the user to a rocketchat:// deep link without any user confirmation or validation of the token's origin.
By crafting a URL with their own resumeToken and userId, an attacker can force a victim who clicks the link to be automatically logged into the attacker's account on their Rocket.Chat desktop or mobile client. This constitutes a Login CSRF / Session Fixation vulnerability. Furthermore, the adjacent useLoginViaQuery hook exhibits similar behavior for the web client when loginClient is omitted, confirming that the application broadly accepts authentication tokens from the URL without state validation or confirmation prompts.
Steps to Reproduce
- The attacker logs into their own Rocket.Chat account and retrieves their active
resumeTokenanduserId(e.g., from local storage or cookies). - The attacker crafts a malicious link:
https://<rocket-chat-instance>/?resumeToken=<ATTACKER_TOKEN>&userId=<ATTACKER_ID>&loginClient=desktop - The attacker sends this link to the victim.
- The victim clicks the link. The web application loads and immediately executes
window.location.href = "rocketchat://auth?host=...&token=<ATTACKER_TOKEN>&userId=<ATTACKER_ID>". - The victim's operating system opens the Rocket.Chat desktop/mobile client, which consumes the deep link and logs the victim into the attacker's account.
PoC Url: https:///?resumeToken=<ATTACKER_TOKEN>&userId=<ATTACKER_ID>&loginClient=desktop
🟡 Medium: Missing CSRF protection in Apple OAuth callback
Location: apps/meteor/app/apple/server/appleOauthRegisterService.ts:29-141
The Apple OAuth callback endpoint /_oauth/apple supports both GET and POST methods and does not explicitly validate an anti-CSRF state parameter. The manual configuration in appleOauthRegisterService.ts explicitly sets state: false in the AppleStrategy options, disabling the library's built-in CSRF protection. This makes the login process vulnerable to Login CSRF attacks, where an attacker could force a victim to log into an attacker-controlled Apple identity. If the victim unknowingly uses the attacker's account, any sensitive information they enter (e.g., messages, files) could be accessed by the attacker.
Steps to Reproduce
- An attacker creates a malicious webpage that automatically submits a POST request to
https://target.rocket.chat/_oauth/applewith a validid_tokengenerated from the attacker's Apple account. - The victim visits the malicious webpage.
- The victim's browser sends the POST request to the callback endpoint.
- The server processes the
id_token, authenticates the attacker's Apple account, and redirects the victim to/homewith a new login token. - The victim is now logged into the attacker's account.
🟡 Medium: Potential Denial of Service via Image Processing (Image Bomb)
Location: apps/meteor/app/file-upload/server/lib/FileUpload.ts:287
The application is vulnerable to a Denial of Service (DoS) attack via image processing. The FileUpload service uses the sharp library to process uploaded images (e.g., for resizing, thumbnails, or metadata extraction) without validating the image dimensions beforehand. An attacker can upload a specially crafted, highly compressed image (a "pixel bomb" or "decompression bomb") with a small file size but extremely large dimensions. When sharp attempts to decompress and process this image, it will consume excessive CPU and memory, potentially leading to resource exhaustion and crashing the application.
Steps to Reproduce
- Create a highly compressed image with large dimensions (e.g., 16383x16383 pixels) that is small in file size (e.g., a few KB).
- Authenticate to the application.
- Upload the image via an endpoint that triggers image processing (e.g., avatar upload or message attachment).
- Observe the server's CPU and memory usage spike, potentially causing the service to become unresponsive or crash.
🔴 High: Sensitive Session Token Exposure in URL Search Parameters
Location: apps/meteor/client/views/OAuthTwoFactorAuthentication/OAuthTwoFactorAuthenticationRouter.tsx:20-99
The application exposes sensitive session credentials (resumeToken and userId) via URL search parameters during the OAuth 2FA authentication flow for mobile and desktop clients. This exposes the session token to potential interception through browser history, server logs, proxy logs, and the Referer header, which could lead to session hijacking. The use of URL parameters for sensitive tokens is a significant security risk.
Steps to Reproduce
- Initiate a 2FA login flow for a user.
- When prompted for the 2FA code, intercept the request or manipulate the URL to include
?loginClient=mobile. - Provide the correct 2FA code.
- Observe the browser navigation to the home route.
- The URL will contain
resumeToken=<TOKEN>&userId=<USER_ID>. - This token can be used to authenticate as the user from any other browser/client.
🔴 High: Authentication via URL Query Parameter (Token-in-URL)
Location: apps/meteor/client/views/root/hooks/useLoginViaQuery.ts:10-20
The useLoginViaQuery hook extracts the resumeToken parameter from the browser's URL query string and passes it to the loginWithToken function to authenticate the user session. This token acts as a sensitive credential. URL query parameters are persisted in browser history and may be leaked through referrer headers or logs. Furthermore, it enables Login CSRF where an attacker can trick a victim into authenticating as the attacker by sending them a crafted link.
Steps to Reproduce
PoC Url: https://<rocket-chat-instance>/home?resumeToken=<token>
2 participants
Proposed changes (including videos or screenshots)
Issue(s)
CORE-2212
Steps to test or reproduce
Further comments
Introduced here #37049
Summary by CodeRabbit