feat: Phishing resistant MFA#40721
Conversation
This reverts commit 741b87f.
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: 50f6c69 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a Passport-based modern OAuth flow, OAuth-specific 2FA challenge handling, login-code redemption, deep-link support for desktop and mobile clients, provider reconfiguration for multiple OAuth services, new typings and models, UI routing updates, and release notes. ChangesModern OAuth and phishing-resistant MFA
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
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❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #40721 +/- ##
===========================================
- Coverage 70.18% 70.06% -0.13%
===========================================
Files 3368 3383 +15
Lines 130022 130360 +338
Branches 22561 22609 +48
===========================================
+ Hits 91256 91332 +76
- Misses 35446 35704 +258
- Partials 3320 3324 +4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Co-authored-by: Tasso Evangelista <tasso.evangelista@rocket.chat>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
tassoevan
added
the
stat: QA assured
…nto feat/phishing-resistant-mfa
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
| export async function handlePassportIdentityToken(identityToken: string): Promise<Record<string, any>> { | ||
| const decodedToken = KJUR.jws.JWS.parse(identityToken); | ||
|
|
||
| if (!(await isValidAppleJWT(identityToken, decodedToken.headerObj))) { | ||
| throw new Error('identityToken is not a valid JWT'); | ||
| } | ||
|
|
||
| if (!decodedToken.payloadObj) { | ||
| throw new Error('identityToken does not have a payload'); | ||
| } | ||
|
|
||
| const { iss, sub, exp } = decodedToken.payloadObj as any; | ||
|
|
||
| if (iss !== 'https://appleid.apple.com') { | ||
| throw new Error('Invalid token issuer'); | ||
| } | ||
|
|
||
| if (typeof exp !== 'number' || Math.floor(Date.now() / 1000) >= exp) { | ||
| throw new Error('identityToken is expired or missing expiration'); | ||
| } | ||
|
|
||
| const serviceData = { | ||
| id: sub, | ||
| ...decodedToken.payloadObj, | ||
| }; | ||
|
|
||
| return serviceData; | ||
| } |
There was a problem hiding this comment.
The handlePassportIdentityToken function in apps/meteor/app/apple/lib/handlePassportIdentityToken.ts is responsible for parsing and validating Apple Identity Tokens during the Passport.js-based Apple Sign-In flow (apps/meteor/app/apple/server/applePassportOAuth.ts). While it correctly validates the token's cryptographic signature, issuer (iss), and expiration (exp), it fails to validate the audience (aud) claim.
In JWT-based authentication flows like OpenID Connect, the aud claim indicates the intended recipient of the token (the Client ID). Without validating that the aud matches the Rocket.Chat instance's configured Apple Client ID, the application will accept any valid Apple Identity Token.
An attacker can exploit this by creating their own application with "Sign in with Apple" enabled. When a victim logs into the attacker's application, the attacker receives a valid Apple Identity Token for that victim. The attacker can then submit this token to the Rocket.Chat server's Apple OAuth callback endpoint. Because the token is genuinely signed by Apple, has a valid issuer, and is not expired, handlePassportIdentityToken will return the victim's data. The upstream Passport strategy (AppleStrategy in applePassportOAuth.ts) then uses this data to log the attacker in as the victim, leading to complete account takeover.
Note that another function, handleIdentityToken in apps/meteor/app/apple/lib/handleIdentityToken.ts, correctly implements audience validation, but handlePassportIdentityToken completely omits it.
Steps to Reproduce
- The attacker creates a malicious application and configures 'Sign in with Apple' for it, obtaining their own Client ID.
- The attacker tricks a victim (who has an account on the target Rocket.Chat instance) into logging into the malicious application using 'Sign in with Apple'.
- The attacker intercepts the Apple Identity Token generated for the victim.
- The attacker initiates an Apple login flow on the target Rocket.Chat instance and submits the captured Identity Token to the
/_oauth/applecallback endpoint. - The
handlePassportIdentityTokenfunction validates the signature and issuer but ignores theaudclaim, returning the victim'sserviceData. - The Rocket.Chat server authenticates the attacker as the victim, granting full access to their account.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/apple/lib/handlePassportIdentityToken.ts"]
isValidAppleJWT["Verifies an Apple Identity Token against Apple's public keys."]
handlePassportIdentityToken{{"Parses and validates an Apple identity token JWT, checking its signature, issuer, and expiration."}}
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
handlePassportIdentityToken --> isValidAppleJWT
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/app/apple/lib/handlePassportIdentityToken.ts
Lines: 30-57
Severity: critical
Vulnerability: Missing JWT Audience (aud) Validation in Apple Identity Token Handler Leads to Account Takeover
Description:
The `handlePassportIdentityToken` function in `apps/meteor/app/apple/lib/handlePassportIdentityToken.ts` is responsible for parsing and validating Apple Identity Tokens during the Passport.js-based Apple Sign-In flow (`apps/meteor/app/apple/server/applePassportOAuth.ts`). While it correctly validates the token's cryptographic signature, issuer (`iss`), and expiration (`exp`), it fails to validate the audience (`aud`) claim.
In JWT-based authentication flows like OpenID Connect, the `aud` claim indicates the intended recipient of the token (the Client ID). Without validating that the `aud` matches the Rocket.Chat instance's configured Apple Client ID, the application will accept any valid Apple Identity Token.
An attacker can exploit this by creating their own application with "Sign in with Apple" enabled. When a victim logs into the attacker's application, the attacker receives a valid Apple Identity Token for that victim. The attacker can then submit this token to the Rocket.Chat server's Apple OAuth callback endpoint. Because the token is genuinely signed by Apple, has a valid issuer, and is not expired, `handlePassportIdentityToken` will return the victim's data. The upstream Passport strategy (`AppleStrategy` in `applePassportOAuth.ts`) then uses this data to log the attacker in as the victim, leading to complete account takeover.
Note that another function, `handleIdentityToken` in `apps/meteor/app/apple/lib/handleIdentityToken.ts`, correctly implements audience validation, but `handlePassportIdentityToken` completely omits it.
Proof of Concept:
**Steps to Reproduce**
1. The attacker creates a malicious application and configures 'Sign in with Apple' for it, obtaining their own Client ID.
2. The attacker tricks a victim (who has an account on the target Rocket.Chat instance) into logging into the malicious application using 'Sign in with Apple'.
3. The attacker intercepts the Apple Identity Token generated for the victim.
4. The attacker initiates an Apple login flow on the target Rocket.Chat instance and submits the captured Identity Token to the `/_oauth/apple` callback endpoint.
5. The `handlePassportIdentityToken` function validates the signature and issuer but ignores the `aud` claim, returning the victim's `serviceData`.
6. The Rocket.Chat server authenticates the attacker as the victim, granting full access to their account.
Affected Code:
export async function handlePassportIdentityToken(identityToken: string): Promise<Record<string, any>> {
const decodedToken = KJUR.jws.JWS.parse(identityToken);
if (!(await isValidAppleJWT(identityToken, decodedToken.headerObj))) {
throw new Error('identityToken is not a valid JWT');
}
if (!decodedToken.payloadObj) {
throw new Error('identityToken does not have a payload');
}
const { iss, sub, exp } = decodedToken.payloadObj as any;
if (iss !== 'https://appleid.apple.com') {
throw new Error('Invalid token issuer');
}
if (typeof exp !== 'number' || Math.floor(Date.now() / 1000) >= exp) {
throw new Error('identityToken is expired or missing expiration');
}
const serviceData = {
id: sub,
...decodedToken.payloadObj,
};
return serviceData;
}
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
| export const doesUserRequire2FA = (user: IUser) => { | ||
| const rememberAfterRegistration = getRememberDate(user.createdAt); | ||
|
|
||
| if (rememberAfterRegistration && rememberAfterRegistration > new Date()) { | ||
| return false; | ||
| } | ||
|
|
||
| const secondFactorMethod = getSecondFactorMethod(user); | ||
|
|
||
| if (!secondFactorMethod) { | ||
| return false; | ||
| } | ||
|
|
||
| return secondFactorMethod; | ||
| }; |
There was a problem hiding this comment.
Rocket.Chat allows administrators to enforce a password-based 2FA fallback policy via the Accounts_TwoFactorAuthentication_Enforce_Password_Fallback setting. When enabled, users who have a password set are required to verify their password as a second factor during sensitive actions or logins.
However, during the modern Passport OAuth login flow, the application determines if the authenticating user requires 2FA by calling doesUserRequire2FA from twoFactorAuth.ts. Inside twoFactorAuth.ts, the helper getSecondFactorMethod only checks the methods registered in twoFACheckMethodsForOAuth, which is restricted to email and totp.
Because PasswordCheckFallback is completely omitted from this map, doesUserRequire2FA returns false for any user whose only active/enforced 2FA method is the password fallback. As a result, the OAuth login callback in passportOAuthCallback.ts immediately logs the user in and generates a session token/code without prompting for the password-based 2FA challenge.
An attacker who compromises a victim's linked external OAuth account (or leverages a malicious/compromised OAuth provider) can log in to the victim's Rocket.Chat account, completely bypassing the enforced password-based 2FA fallback.
Steps to Reproduce
- Enable
Accounts_TwoFactorAuthentication_Enforce_Password_Fallbackin Rocket.Chat settings. - Ensure a victim user has a password set and has linked an OAuth provider (e.g., GitHub).
- The victim does not have TOTP or Email 2FA enabled.
- Initiate the OAuth login flow for the victim user.
- Upon successful OAuth authentication, the callback
passportOAuthCallbackis triggered. - Observe that the user is logged in directly and redirected to
/homewith aloginCodewithout being challenged for their password, despite the password fallback policy being globally enforced.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/2fa/server/code/PasswordCheckFallback.ts"]
PasswordCheckFallback.isEnabled["Determines if password fallback is enabled for 2FA."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/2fa/server/code/index.ts"]
getRememberDate["Calculates the expiration date for remember-me functionality."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/app/apple/server/applePassportOAuth.ts"]
settings.watchMultiple.arg2["Handles Apple OAuth configuration changes via settings updates, including passport strategy registration and route setup."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/app/meteor-developer/server/lib.ts"]
configureMeteorDeveloperOAuth["Configures Meteor Developer OAuth settings using Passport."]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["apps/meteor/app/settings/server/CachedSettings.ts"]
CachedSettings.get["Retrieves the value of a setting from the cache."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["apps/meteor/app/wordpress/server/lib.ts"]
.debounce.arg1["Dynamically configures WordPress OAuth settings by reading application settings and updating the service configuration in the database."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["apps/meteor/server/configuration/configurePassport.ts"]
configurePassport["Configures Passport.js middleware, session management, and OAuth routes for the application."]
settings.watchByRegex.arg2["Callback function that reconfigures OAuth services dynamically when relevant application settings are updated."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts"]
addPassportCustomOAuth["Configures and registers a custom OAuth passport strategy with routes."]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG8 ["apps/meteor/server/lib/oauth/configureOAuthServices.ts"]
configureOAuthServices["Configures multiple OAuth services by registering passport strategies and setting up routes."]
oauthServiceConfig.forEach.arg1["Configures an individual OAuth service, including passport strategy and callback handling."]
end
style SG8 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG9 ["apps/meteor/server/lib/oauth/passportOAuthCallback.ts"]
handlePopupStyleOAuth["Handles the finalization of OAuth login for popup-based flows, including 2FA."]
passportOAuthCallback["Main entrypoint for processing OAuth callbacks from passport."]
end
style SG9 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG10 ["apps/meteor/server/lib/oauth/twoFactorAuth.ts"]
getSecondFactorMethod["Finds an enabled 2FA method for a user."]
doesUserRequire2FA{{"Determines if a user needs to perform 2FA verification."}}
end
style SG10 fill:#2a2a2a,stroke:#444,color:#aaa
doesUserRequire2FA --> getRememberDate
doesUserRequire2FA --> getSecondFactorMethod
getRememberDate --> CachedSettings.get
getSecondFactorMethod --> PasswordCheckFallback.isEnabled
CachedSettings.get --> CachedSettings.get
PasswordCheckFallback.isEnabled --> CachedSettings.get
handlePopupStyleOAuth --> doesUserRequire2FA
passportOAuthCallback --> doesUserRequire2FA
passportOAuthCallback --> handlePopupStyleOAuth
addPassportCustomOAuth --> passportOAuthCallback
settings.watchMultiple.arg2 --> passportOAuthCallback
configureOAuthServices --> passportOAuthCallback
oauthServiceConfig.forEach.arg1 --> passportOAuthCallback
configureMeteorDeveloperOAuth --> addPassportCustomOAuth
.debounce.arg1 --> addPassportCustomOAuth
configurePassport --> configureOAuthServices
settings.watchByRegex.arg2 --> configureOAuthServices
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/server/lib/oauth/twoFactorAuth.ts
Lines: 23-37
Severity: high
Vulnerability: Bypass of Enforced Password-Based 2FA Fallback During OAuth Login
Description:
Rocket.Chat allows administrators to enforce a password-based 2FA fallback policy via the `Accounts_TwoFactorAuthentication_Enforce_Password_Fallback` setting. When enabled, users who have a password set are required to verify their password as a second factor during sensitive actions or logins.
However, during the modern Passport OAuth login flow, the application determines if the authenticating user requires 2FA by calling `doesUserRequire2FA` from `twoFactorAuth.ts`. Inside `twoFactorAuth.ts`, the helper `getSecondFactorMethod` only checks the methods registered in `twoFACheckMethodsForOAuth`, which is restricted to `email` and `totp`.
Because `PasswordCheckFallback` is completely omitted from this map, `doesUserRequire2FA` returns `false` for any user whose only active/enforced 2FA method is the password fallback. As a result, the OAuth login callback in `passportOAuthCallback.ts` immediately logs the user in and generates a session token/code without prompting for the password-based 2FA challenge.
An attacker who compromises a victim's linked external OAuth account (or leverages a malicious/compromised OAuth provider) can log in to the victim's Rocket.Chat account, completely bypassing the enforced password-based 2FA fallback.
Proof of Concept:
1. Enable `Accounts_TwoFactorAuthentication_Enforce_Password_Fallback` in Rocket.Chat settings.
2. Ensure a victim user has a password set and has linked an OAuth provider (e.g., GitHub).
3. The victim does not have TOTP or Email 2FA enabled.
4. Initiate the OAuth login flow for the victim user.
5. Upon successful OAuth authentication, the callback `passportOAuthCallback` is triggered.
6. Observe that the user is logged in directly and redirected to `/home` with a `loginCode` without being challenged for their password, despite the password fallback policy being globally enforced.
Affected Code:
- In [twoFactorAuth.ts](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/server/lib/oauth/twoFactorAuth.ts#L10-L13), `twoFACheckMethodsForOAuth` is defined containing only `email` and `totp` checks:
```typescript
const twoFACheckMethodsForOAuth = {
[emailCheckForOAuth.method]: emailCheckForOAuth,
[totpCheckForOAuth.method]: totpCheckForOAuth,
};
```
- In [twoFactorAuth.ts](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/server/lib/oauth/twoFactorAuth.ts#L19-L21), `getSecondFactorMethod` only queries this restricted map:
```typescript
const getSecondFactorMethod = (user: IUser) => {
return Array.from(Object.values(twoFACheckMethodsForOAuth)).find((method) => method.isEnabled(user));
};
```
- In [passportOAuthCallback.ts](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/server/lib/oauth/passportOAuthCallback.ts#L78), `doesUserRequire2FA` is called during the OAuth callback to determine if the user must be challenged:
```typescript
const secondFactorMethod = doesUserRequire2FA(oAuthUser);
if (secondFactorMethod) {
const challengeId = await secondFactorMethod.sendTwoFactorChallenge(oAuthUser);
```
- Because `PasswordCheckFallback` is omitted from `twoFACheckMethodsForOAuth`, `doesUserRequire2FA` returns `false` for users who only have password-based 2FA fallback enabled, completely bypassing the 2FA requirement.
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
| export const useOAuthLogin = () => { | ||
| const router = useRouter(); | ||
| const loginCode = useSearchParameter('loginCode'); | ||
| const loginClient = useSearchParameter('loginClient'); | ||
| const redeemLoginCode = useEndpoint('POST', '/v1/loginCode.redeem'); | ||
| const loginWithToken = useLoginWithToken(); | ||
|
|
||
| const { mutate: redeemLoginCodeMutation } = useMutation({ | ||
| mutationFn: async (loginCode: string) => { | ||
| const { loginToken, userId } = await redeemLoginCode({ code: loginCode }); | ||
|
|
||
| if (!loginToken || !userId) { | ||
| throw new Error('Invalid response from login code redemption'); | ||
| } | ||
|
|
||
| return { loginToken, userId }; | ||
| }, | ||
| onSuccess: async ({ loginToken, userId }) => { | ||
| if (loginClient === 'desktop' || loginClient === 'mobile') { | ||
| window.location.href = buildDeepLinkURL(loginToken, userId); | ||
| return; | ||
| } | ||
|
|
||
| await loginWithToken(loginToken); | ||
| router.navigate('/home', { replace: true }); | ||
| }, | ||
| onError: (error) => { | ||
| console.error('Failed to redeem login code for client redirect', error); | ||
| router.navigate('/login', { replace: true }); | ||
| }, | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!loginCode) { | ||
| return; | ||
| } | ||
|
|
||
| redeemLoginCodeMutation(loginCode); | ||
| }, [loginCode, redeemLoginCodeMutation]); | ||
| }; |
There was a problem hiding this comment.
The newly introduced useOAuthLogin hook automatically redeems any loginCode found in the URL query parameters and authenticates the user's session without verifying if they are already logged in or requiring user confirmation.
An attacker can exploit this behavior to perform a Login CSRF (Session Fixation) attack by sending a crafted link containing a loginCode for an attacker-controlled account to a victim. When the victim clicks the link, their browser session will be silently logged into the attacker's account. If the victim does not notice the account switch, they may post sensitive information or upload files that the attacker can then monitor in real-time.
Steps to Reproduce
An attacker generates a valid `loginCode` for their own account on the Rocket.Chat instance, then sends the following link to a victim:
`https://<rocketchat-server>/?loginCode=ATTACKER_LOGIN_CODE`
When the victim clicks the link, the application automatically redeems the code and logs the victim into the attacker's account.
Trace
graph TD
subgraph SG0 ["apps/meteor/client/lib/buildAuthDeeplinkURL.ts"]
buildDeepLinkURL["Constructs a deep link URL for authentication using a resume token and user ID."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/client/views/root/AppLayout.tsx"]
AppLayout["Main application layout component, initializing global hooks and providers."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/client/views/root/hooks/useOAuthLogin.ts"]
useOAuthLogin{{"Handles OAuth login flow by redeeming a login code and redirecting or authenticating the user."}}
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
useOAuthLogin --> buildDeepLinkURL
AppLayout --> useOAuthLogin
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/client/views/root/hooks/useOAuthLogin.ts
Lines: 7-46
Severity: medium
Vulnerability: Login CSRF / Session Fixation via loginCode Parameter
Description:
The newly introduced `useOAuthLogin` hook automatically redeems any `loginCode` found in the URL query parameters and authenticates the user's session without verifying if they are already logged in or requiring user confirmation.
An attacker can exploit this behavior to perform a Login CSRF (Session Fixation) attack by sending a crafted link containing a `loginCode` for an attacker-controlled account to a victim. When the victim clicks the link, their browser session will be silently logged into the attacker's account. If the victim does not notice the account switch, they may post sensitive information or upload files that the attacker can then monitor in real-time.
Proof of Concept:
An attacker generates a valid `loginCode` for their own account on the Rocket.Chat instance, then sends the following link to a victim:
`https://<rocketchat-server>/?loginCode=ATTACKER_LOGIN_CODE`
When the victim clicks the link, the application automatically redeems the code and logs the victim into the attacker's account.
Affected Code:
- [useOAuthLogin.ts:9-10](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/client/views/root/hooks/useOAuthLogin.ts#L9-L10) - Extracts `loginCode` and `loginClient` from search parameters.
- [useOAuthLogin.ts:39-45](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/client/views/root/hooks/useOAuthLogin.ts#L39-L45) - Automatically triggers `redeemLoginCodeMutation` when `loginCode` is present.
- [useOAuthLogin.ts:15-23](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/client/views/root/hooks/useOAuthLogin.ts#L15-L23) - Redeems the code via `/v1/loginCode.redeem` API.
- [useOAuthLogin.ts:30-31](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/client/views/root/hooks/useOAuthLogin.ts#L30-L31) - Logs in with the redeemed token and navigates to `/home`.
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
| if (key === 'Accounts_OAuth_Use_Modern_Flow') { | ||
| continue; | ||
| } | ||
|
|
||
| logger.debug({ oauth_updated: key }); | ||
| let serviceName = key.replace('Accounts_OAuth_', ''); | ||
| if (serviceName === 'Meteor') { |
There was a problem hiding this comment.
The pull request modifies how custom OAuth services are initialized in updateOAuthServices.ts by introducing clientSecret and clientId into the config object passed to the legacy CustomOAuth constructor.
However, the legacy CustomOAuth constructor in custom_oauth_server.js logs the entire options object (which is the config object) at the debug level using logger.debug({ msg: 'Init CustomOAuth', name, options }).
This results in the sensitive OAuth clientSecret being written to the application logs in plain text whenever the custom OAuth services are updated or initialized (such as during system startup or configuration changes).
An attacker with access to the application logs (e.g., via a compromised log shipping pipeline, read-only monitoring accounts, or local file access) could retrieve the client secret and potentially impersonate the application or perform unauthorized actions against the OAuth provider.
Steps to Reproduce
- Enable debug logging in Rocket.Chat settings.
- Configure or update any Custom OAuth service.
- Inspect the application logs for the
Init CustomOAuthdebug message. The logged payload will contain the plain-textclientSecret.
Trace
graph TD
subgraph SG0 ["apps/meteor/app/custom-oauth/server/custom_oauth_server.js"]
._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js["Logger instance for CustomOAuth server module."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["apps/meteor/app/lib/server/lib/notifyListener.ts"]
notifyOnLoginServiceConfigurationChanged["Broadcasts changes to login service configurations."]
notifyOnLoginServiceConfigurationChangedByService["Fetches and broadcasts login service configuration changes by service name."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["apps/meteor/server/lib/cas/logger.ts"]
._Rocket.Chat_apps_meteor_server_lib_cas_logger.ts["Initializes the logger for the CAS (Central Authentication Service) module."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["apps/meteor/server/lib/oauth/updateOAuthServices.ts"]
updateOAuthServices{{"Synchronizes OAuth service settings with the database."}}
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
updateOAuthServices --> notifyOnLoginServiceConfigurationChanged
updateOAuthServices --> notifyOnLoginServiceConfigurationChangedByService
updateOAuthServices --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
notifyOnLoginServiceConfigurationChangedByService --> notifyOnLoginServiceConfigurationChanged
._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js --> ._Rocket.Chat_apps_meteor_server_lib_cas_logger.ts
._Rocket.Chat_apps_meteor_server_lib_cas_logger.ts --> ._Rocket.Chat_apps_meteor_server_lib_cas_logger.ts
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/server/lib/oauth/updateOAuthServices.ts
Lines: 23-106
Severity: medium
Vulnerability: Sensitive OAuth Client Secret Exposed in Debug Logs
Description:
The pull request modifies how custom OAuth services are initialized in `updateOAuthServices.ts` by introducing `clientSecret` and `clientId` into the `config` object passed to the legacy `CustomOAuth` constructor.
However, the legacy `CustomOAuth` constructor in `custom_oauth_server.js` logs the entire `options` object (which is the `config` object) at the `debug` level using `logger.debug({ msg: 'Init CustomOAuth', name, options })`.
This results in the sensitive OAuth `clientSecret` being written to the application logs in plain text whenever the custom OAuth services are updated or initialized (such as during system startup or configuration changes).
An attacker with access to the application logs (e.g., via a compromised log shipping pipeline, read-only monitoring accounts, or local file access) could retrieve the client secret and potentially impersonate the application or perform unauthorized actions against the OAuth provider.
Proof of Concept:
1. Enable debug logging in Rocket.Chat settings.
2. Configure or update any Custom OAuth service.
3. Inspect the application logs for the `Init CustomOAuth` debug message. The logged payload will contain the plain-text `clientSecret`.
Affected Code:
- [apps/meteor/server/lib/oauth/updateOAuthServices.ts#L76-L105](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/server/lib/oauth/updateOAuthServices.ts#L76-L105)
```typescript
const config = {
...
clientSecret: data.secret,
clientId: data.clientId,
};
new CustomOAuth(serviceKey, config);
```
- [apps/meteor/app/custom-oauth/server/custom_oauth_server.js#L30-L31](https://github.com/RocketChat/Rocket.Chat/blob/main/apps/meteor/app/custom-oauth/server/custom_oauth_server.js#L30-L31)
```javascript
constructor(name, options) {
logger.debug({ msg: 'Init CustomOAuth', name, options });
```
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
cardoso
dismissed stale reviews from tassoevan, ricardogarim, and KevLehman
via
c0c9f4b
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
2 issues found across 2 files
| Severity | Count |
|---|---|
|
1 |
|
1 |
Findings outside your changes (1)
1 additional security finding was found outside your changes. Details are hidden on public repositories — review it in Hacktron: View finding
| async ([enabled, clientId, serverSecret, iss, kid, useModernFlow]) => { | ||
| passport.unuse('apple'); | ||
|
|
||
| if (!useModernFlow) { | ||
| return; | ||
| } | ||
|
|
||
| if (!enabled) { | ||
| return ServiceConfiguration.configurations.removeAsync({ | ||
| service: 'apple', | ||
| }); | ||
| } | ||
|
|
||
| // if everything is empty but Apple login is enabled, don't show the login button | ||
| if (!clientId && !serverSecret && !iss && !kid) { | ||
| await ServiceConfiguration.configurations.upsertAsync( | ||
| { | ||
| service: 'apple', | ||
| }, | ||
| { | ||
| $set: { | ||
| showButton: false, | ||
| enabled: settings.get('Accounts_OAuth_Apple'), | ||
| }, | ||
| }, | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (typeof clientId !== 'string' || !clientId) { | ||
| return ServiceConfiguration.configurations.removeAsync({ | ||
| service: 'apple', | ||
| }); | ||
| } | ||
|
|
||
| passport.use( | ||
| 'apple', | ||
|
|
||
| new AppleStrategy( | ||
| { | ||
| clientID: clientId, | ||
| teamID: settings.get<string>('Accounts_OAuth_Apple_iss'), | ||
| keyID: settings.get<string>('Accounts_OAuth_Apple_kid'), | ||
| privateKeyString: settings.get<string>('Accounts_OAuth_Apple_secretKey').replace(/\\n/g, '\n'), | ||
| callbackURL: `${settings.get<string>('Site_Url').replace(/\/$/, '')}/_oauth/apple`, | ||
| scope: ['name', 'email'], | ||
| passReqToCallback: false, | ||
| state: false, | ||
| }, | ||
| async (accessToken: string, refreshToken: string, idToken: string, _profile: Profile, done) => { | ||
| try { | ||
| const serviceData = await handleIdentityToken(idToken, clientId); | ||
|
|
||
| const user = await Accounts.updateOrCreateUserFromExternalService( | ||
| 'apple', | ||
| { | ||
| accessToken, | ||
| refreshToken, | ||
| ...serviceData, | ||
| }, | ||
| {}, | ||
| ); | ||
|
|
||
| if (!user?.userId || typeof user?.userId !== 'string') { | ||
| return done(new Error('User not found')); | ||
| } | ||
|
|
||
| const userFromDB = await Users.findOneById(user.userId); | ||
|
|
||
| if (!userFromDB) { | ||
| return done(new Error('User not found')); | ||
| } | ||
|
|
||
| return done(null, userFromDB); | ||
| } catch (error: any) { | ||
| done(error); | ||
| return { | ||
| type: 'apple', | ||
| error: new MeteorError(Accounts.LoginCancelledError.numericError, error.message), | ||
| }; | ||
| } | ||
| }, | ||
| ), | ||
| ); | ||
|
|
||
| const callbackHandler = [ | ||
| allowPassportOAuthMiddleware('apple'), | ||
| express.urlencoded({ extended: true }), | ||
| passport.authenticate('apple', { failWithError: true, session: true, keepSessionInfo: true }), | ||
| passportOAuthCallback(settings.get<string>('Site_Url').replace(/\/$/, '')), | ||
| ]; | ||
|
|
||
| oAuthRouter.get( | ||
| '/oauth/apple', | ||
| allowPassportOAuthMiddleware('apple'), | ||
| (req, _res, next) => { | ||
| const { loginClient } = req.query; | ||
| if (loginClient === 'mobile' || loginClient === 'desktop') { | ||
| req.session.loginClient = loginClient; | ||
| req.session.save(() => { | ||
| next(); | ||
| }); | ||
| } else { | ||
| //delete stale value from previous sessions if any | ||
| delete req.session.loginClient; | ||
| next(); | ||
| } | ||
| }, | ||
| passport.authenticate('apple', { | ||
| scope: ['name', 'email'], | ||
| }), | ||
| ); | ||
|
|
||
| oAuthRouter | ||
| .route('/_oauth/apple') | ||
| .post(...callbackHandler) | ||
| .get(...callbackHandler); | ||
| }, |
There was a problem hiding this comment.
Duplicate of: Missing CSRF protection in Apple OAuth callback
The Apple Passport OAuth configuration explicitly disables CSRF protection by setting state: false in the AppleStrategy options.
In OAuth 2.0 / OpenID Connect flows, the state parameter is a critical security control used to prevent Cross-Site Request Forgery (CSRF). When state is disabled, the application does not validate that the callback request was initiated by the same user session.
An attacker can exploit this to perform an OAuth Login CSRF (Session Injection) attack. By intercepting their own Apple authentication response (authorization code and identity token) and hosting a malicious page that submits these values via a cross-site POST request to /_oauth/apple, the attacker can force a victim's browser to log into the attacker's account. The server processes the callback and redirects the victim to /home?loginCode=..., which the client consumes to establish the attacker's session.
Note: The original finding claimed this could lead to Account Takeover via account linking. However, because Rocket.Chat uses token-based authentication (via X-Auth-Token and localStorage) rather than cookies for primary user sessions, the cross-site POST request to the OAuth callback does not carry the victim's authentication context. Therefore, the server cannot link the attacker's Apple ID to the victim's account. The impact is limited to Login CSRF.
Steps to Reproduce
- The attacker initiates an Apple Sign-In flow on Rocket.Chat and intercepts the
codeandid_tokensent by Apple. - The attacker halts their own login flow.
- The attacker hosts a malicious HTML page with an auto-submitting form:
<form id="csrf-form" action="https://<rocket-chat-instance>/_oauth/apple" method="POST">
<input type="hidden" name="code" value="<intercepted_code>">
<input type="hidden" name="id_token" value="<intercepted_id_token>">
</form>
<script>document.getElementById('csrf-form').submit();</script>- A victim visits the attacker's page, triggering the form submission.
- The Rocket.Chat server processes the callback without state validation and redirects the victim to
/home?loginCode=.... - The victim's client consumes the
loginCodeand logs them into the attacker's account (Session Injection).
Fix with AI
A security vulnerability was found by Hacktron.
File: apps/meteor/app/apple/server/applePassportOAuth.ts
Lines: 29-146
Severity: medium
Vulnerability: OAuth Login CSRF (Session Injection) via Disabled State Parameter in Apple Strategy
Description:
The Apple Passport OAuth configuration explicitly disables CSRF protection by setting `state: false` in the `AppleStrategy` options.
In OAuth 2.0 / OpenID Connect flows, the `state` parameter is a critical security control used to prevent Cross-Site Request Forgery (CSRF). When `state` is disabled, the application does not validate that the callback request was initiated by the same user session.
An attacker can exploit this to perform an OAuth Login CSRF (Session Injection) attack. By intercepting their own Apple authentication response (authorization code and identity token) and hosting a malicious page that submits these values via a cross-site POST request to `/_oauth/apple`, the attacker can force a victim's browser to log into the attacker's account. The server processes the callback and redirects the victim to `/home?loginCode=...`, which the client consumes to establish the attacker's session.
*Note: The original finding claimed this could lead to Account Takeover via account linking. However, because Rocket.Chat uses token-based authentication (via `X-Auth-Token` and `localStorage`) rather than cookies for primary user sessions, the cross-site POST request to the OAuth callback does not carry the victim's authentication context. Therefore, the server cannot link the attacker's Apple ID to the victim's account. The impact is limited to Login CSRF.*
Proof of Concept:
**Steps to Reproduce**
1. The attacker initiates an Apple Sign-In flow on Rocket.Chat and intercepts the `code` and `id_token` sent by Apple.
2. The attacker halts their own login flow.
3. The attacker hosts a malicious HTML page with an auto-submitting form:
```html
<form id="csrf-form" action="https://<rocket-chat-instance>/_oauth/apple" method="POST">
<input type="hidden" name="code" value="<intercepted_code>">
<input type="hidden" name="id_token" value="<intercepted_id_token>">
</form>
<script>document.getElementById('csrf-form').submit();</script>
```
4. A victim visits the attacker's page, triggering the form submission.
5. The Rocket.Chat server processes the callback without state validation and redirects the victim to `/home?loginCode=...`.
6. The victim's client consumes the `loginCode` and logs them into the attacker's account (Session Injection).
Affected Code:
passReqToCallback: false,
state: false,
},
Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.
Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.
…nto feat/phishing-resistant-mfa
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
1 issue found across 1 file
| Severity | Count |
|---|---|
|
1 |
Findings outside your changes (1)
1 additional security finding was found outside your changes. Details are hidden on public repositories — review it in Hacktron: View finding
There was a problem hiding this comment.
1 issue found across 1 file
| Severity | Count |
|---|---|
|
1 |
Findings outside your changes (1)
1 additional security finding was found outside your changes. Details are hidden on public repositories — review it in Hacktron: View finding
…nto feat/phishing-resistant-mfa
dionisio-bot
Bot
added
the
stat: ready to merge
7 participants
This reverts commit 741b87f.
Proposed changes (including videos or screenshots)
Phishing-Resistant Multi-Factor Authentication
Introduces a more secure and reliable server-side OAuth authentication flow.
Improved OAuth login security
OAuth authentication now happens fully on the server, reducing the risk of token theft, phishing attacks, and client-side credential interception.
Built-in CSRF, state validation, and PKCE protection
OAuth logins now include stronger protection against CSRF attacks, request tampering, and authorization code interception through secure state validation and PKCE support.
Improved two-step verification with OAuth logins
Users with email or TOTP two-factor authentication enabled will now be asked to complete 2FA even when signing in with providers like Google, GitHub, GitLab, and others.
Improved mobile & desktop app login
Mobile and desktop apps now support a smoother and more secure deep-link OAuth login flow.
Issue(s)
Steps to test or reproduce
Further comments
PRM-57
Summary by CodeRabbit
Release Notes