revert: Phishing resistant multi factor authentication#40679
Open
yash-rajpal wants to merge 2 commits into
Open
revert: Phishing resistant multi factor authentication#40679yash-rajpal wants to merge 2 commits into
yash-rajpal wants to merge 2 commits into
Conversation
Contributor
|
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 |
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent 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)
🔇 Additional comments (1)
WalkthroughRemoves Passport/express-session OAuth wiring and OAuth 2FA challenge storage/endpoints, migrates provider setup to CustomOAuth instances (server-side), and removes client deep-link session sharing and related desktop integrations. ChangesOAuth Architecture Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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. 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 |
KevLehman
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40679 +/- ##
===========================================
- Coverage 69.65% 69.64% -0.02%
===========================================
Files 3339 3339
Lines 123269 123213 -56
Branches 21982 21968 -14
===========================================
- Hits 85864 85810 -54
- Misses 34033 34052 +19
+ Partials 3372 3351 -21
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
apps/meteor/definition/externals/meteor/accounts-base.d.ts (1)
40-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep
updateOrCreateUserFromExternalServicetyped for async overrides.This declaration is now sync-only, but
apps/meteor/server/configuration/accounts_meld.jsLine 7 replaces it with anasyncfunction. The type should reflect both shapes to prevent incorrect non-awaited usage.Proposed fix
- function updateOrCreateUserFromExternalService( + function updateOrCreateUserFromExternalService( serviceName: string, serviceData: Record<string, unknown>, options: Record<string, unknown>, - ): Record<string, unknown>; + ): Record<string, unknown> | Promise<Record<string, unknown> | undefined> | undefined;🤖 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/definition/externals/meteor/accounts-base.d.ts` around lines 40 - 44, The declaration for updateOrCreateUserFromExternalService is currently synchronous but can be overridden with an async implementation; update the type to allow both sync and async shapes (e.g., change the return type to support Promise<Record<string, unknown>> or provide an overload that returns either Record<string, unknown> or Promise<Record<string, unknown>>), so callers aren’t incorrectly treated as non-awaited when an async override (like the one in accounts_meld.js) is used; update the function signature (updateOrCreateUserFromExternalService) accordingly to accept serviceName, serviceData, options and return either Record<string, unknown> or Promise<Record<string, unknown>>.apps/meteor/app/apple/lib/handleIdentityToken.ts (1)
30-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the return type with the values you actually guarantee.
handleIdentityTokenpromises{ id: string; email: string; name: string }, but it only checksiss;subandid/undefinedat runtime).loginHandler.tsonly patches missingid.Suggested fix
-export async function handleIdentityToken(identityToken: string): Promise<{ id: string; email: string; name: string }> { +export async function handleIdentityToken(identityToken: string): Promise<{ id: string; email?: string; name: string }> { const decodedToken = KJUR.jws.JWS.parse(identityToken); @@ - const { iss, sub, email } = decodedToken.payloadObj as any; - if (!iss) { + const { iss, sub, email } = decodedToken.payloadObj as { iss?: string; sub?: string; email?: string }; + if (!iss || !sub) { throw new Error('Insufficient data in auth response token'); }🤖 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/app/apple/lib/handleIdentityToken.ts` around lines 30 - 49, handleIdentityToken currently claims to return {id:string,email:string,name:string} but only validates iss; ensure sub and email are validated (or the return type changed). Update handleIdentityToken to check decodedToken.payloadObj.sub and .email (unique symbols: handleIdentityToken, decodedToken.payloadObj, sub, email) and either throw clear errors when they are missing or coerce/derive safe defaults for id/email before building serviceData, and keep the function signature in sync (change the Promise return type to allow undefined for email/id only if you intentionally allow missing values). Also consider how loginHandler.ts patches email and ensure there is a guaranteed non-empty id for downstream logic (e.g., throw if sub is missing).apps/meteor/app/2fa/server/code/index.ts (1)
66-74:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard invalid
RememberForvalues before building the expiry.
parseInt(...)returnsNaNfor an empty or malformed setting, andNaN <= 0is false. That makesexpiresbecomeInvalid Date, whichrememberAuthorization()later persists as the token's 2FA-until timestamp.Suggested fix
function getRememberDate(from: Date = new Date()): Date | undefined { - const rememberFor = parseInt(settings.get('Accounts_TwoFactorAuthentication_RememberFor') as string, 10); + const rememberFor = Number.parseInt(String(settings.get('Accounts_TwoFactorAuthentication_RememberFor') ?? ''), 10); - if (rememberFor <= 0) { + if (!Number.isFinite(rememberFor) || rememberFor <= 0) { return; }🤖 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/app/2fa/server/code/index.ts` around lines 66 - 74, getRememberDate currently parses Accounts_TwoFactorAuthentication_RememberFor with parseInt and then compares to <= 0, but parseInt can produce NaN which makes the check fail and yields an Invalid Date persisted by rememberAuthorization; update getRememberDate to validate the parsed value (e.g., const rememberFor = parseInt(...); if (!Number.isFinite(rememberFor) || rememberFor <= 0) return undefined) before constructing expires, so only a valid positive numeric rememberFor is used to compute and return the expiry Date.apps/meteor/app/apple/server/appleOauthRegisterService.ts (1)
25-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat partially configured Apple OAuth as disabled.
The watcher only hides the Apple login button when all
clientId,serverSecret,iss, andkidare empty (to support “mobile-only” mode), but if any single value is missing it still proceeds to signsecretand upsert the Apple provider withenabledandshowButton: true. The OAuth handshake then relies onServiceConfigurationfields likeclientId/secretfor the token exchange, so partial credentials lead to a broken login flow or runtime errors during reconfiguration.Suggested fix
- // if everything is empty but Apple login is enabled, don't show the login button - if (!clientId && !serverSecret && !iss && !kid) { + if (!clientId || !serverSecret || !iss || !kid) { await ServiceConfiguration.configurations.upsertAsync( { service: 'apple', }, { $set: { showButton: false, enabled: settings.get('Accounts_OAuth_Apple'), }, }, ); return; }🤖 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/app/apple/server/appleOauthRegisterService.ts` around lines 25 - 38, The watcher currently only disables Apple OAuth when all of clientId, serverSecret, iss, and kid are empty; change the logic in appleOauthRegisterService.ts so that any missing required credential (clientId, serverSecret, iss, or kid) is treated as "disabled": call ServiceConfiguration.configurations.upsertAsync (the same document keyed by service: 'apple') setting enabled: false and showButton: false and then return instead of proceeding to sign the secret or upserting an enabled provider; ensure the check references the same variables (clientId, serverSecret, iss, kid) used later so partial configurations never create an enabled provider.
🤖 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.
Inline comments:
In `@apps/meteor/app/api/server/index.ts`:
- Around line 48-50: The file dropped the side-effect import for the two-factor
routes so those endpoints aren't registered before OpenAPI generation; re-add
the import for the module (import './v1/twoFactorChallenges') in
apps/meteor/app/api/server/index.ts so it is executed before the existing import
'./default/openApi' (keep the comment ordering so all endpoints register before
OpenAPI is built).
In `@apps/meteor/app/gitlab/server/lib.ts`:
- Line 24: The code calls settings.get<string>('API_Gitlab_URL').trim() without
guarding for null/undefined or non-string values; update the assignment that
sets config.serverURL so it first reads the raw value (e.g., const raw =
settings.get('API_Gitlab_URL')), verify typeof raw === 'string' and raw.trim()
is used only then, otherwise fall back to the existing config.serverURL; ensure
you still perform the replace(/\/*$/, '') on the trimmed string when valid and
keep the overall fallback behavior intact for config.serverURL.
In `@apps/meteor/app/nextcloud/server/lib.ts`:
- Around line 23-28: The debounced initializer fillServerURL currently
self-reschedules by calling fillServerURL() when
settings.get('Accounts_OAuth_Nextcloud_URL') returns undefined; remove that
recursive call so the debounced function simply returns when nextcloudURL is
undefined (letting settings.watch trigger future invocations) — edit the
fillServerURL function to check nextcloudURL and if it === undefined just return
instead of calling fillServerURL(), leaving the rest of the _.debounce behavior
unchanged.
In `@apps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsx`:
- Around line 36-40: The resend handler (onClickResendCode) currently calls
sendEmailCode directly and allows overlapping requests which can invalidate
previous codes; add a local boolean state (e.g., isResending) and wrap
onClickResendCode with a guard that returns immediately if isResending is true,
set isResending=true before awaiting sendEmailCode(...) and set it false in
finally, and ensure the resend button is disabled while isResending is true;
apply the same guarded state pattern to the other resend handler at the similar
block (lines referenced as 97-99) so no concurrent resend requests can occur.
In `@apps/meteor/client/lib/2fa/process2faReturn.ts`:
- Around line 31-34: The assertModalProps check is too strict for the 'email'
branch because callers to process2faReturn may pass an object identifier ({
username } | { email } | { id }) and getProps/getUser fallback can return
objects; update process2faReturn (and the other spots flagged around lines 40-50
and 75-77) to normalize the login identifier to a plain string before calling
assertModalProps: extract a string emailOrUsername from possible object shapes
(e.g., prefer a .email or .username property, or call toString on primitives)
and pass that normalized string into assertModalProps so the 'email' case
receives a true string rather than an object. Ensure you update any helpers used
by getProps so all three locations use the same normalization logic.
In `@apps/meteor/server/configuration/accounts_meld.js`:
- Around line 21-23: The unconditional assignment serviceData.email =
serviceData.emailAddress when serviceName === 'linkedin' can overwrite a valid
normalized email with undefined or non-string data; change it to only set
serviceData.email when serviceData.emailAddress is a non-empty string (or
normalize/validate it) and otherwise leave existing serviceData.email intact so
the email-based linking path still works—look for the serviceName check and the
serviceData.email/serviceData.emailAddress references to implement this guard.
In `@packages/web-ui-registration/src/LoginServicesButton.tsx`:
- Around line 30-37: The LDAP button fails because
AuthenticationProvider.loginWithService builds the Meteor method name with
capitalize(service) (e.g., "Ldap") but Meteor exposes "loginWithLDAP"; update
loginWithService (used by useLoginWithService) to special-case known acronym
services (at least 'ldap') so the generated method name matches Meteor (e.g., if
service.toLowerCase() === 'ldap' use "loginWithLDAP"), or implement a small
mapping/normalization that converts service names like 'ldap' to the exact
expected casing before calling Meteor.loginWithX.
---
Outside diff comments:
In `@apps/meteor/app/2fa/server/code/index.ts`:
- Around line 66-74: getRememberDate currently parses
Accounts_TwoFactorAuthentication_RememberFor with parseInt and then compares to
<= 0, but parseInt can produce NaN which makes the check fail and yields an
Invalid Date persisted by rememberAuthorization; update getRememberDate to
validate the parsed value (e.g., const rememberFor = parseInt(...); if
(!Number.isFinite(rememberFor) || rememberFor <= 0) return undefined) before
constructing expires, so only a valid positive numeric rememberFor is used to
compute and return the expiry Date.
In `@apps/meteor/app/apple/lib/handleIdentityToken.ts`:
- Around line 30-49: handleIdentityToken currently claims to return
{id:string,email:string,name:string} but only validates iss; ensure sub and
email are validated (or the return type changed). Update handleIdentityToken to
check decodedToken.payloadObj.sub and .email (unique symbols:
handleIdentityToken, decodedToken.payloadObj, sub, email) and either throw clear
errors when they are missing or coerce/derive safe defaults for id/email before
building serviceData, and keep the function signature in sync (change the
Promise return type to allow undefined for email/id only if you intentionally
allow missing values). Also consider how loginHandler.ts patches email and
ensure there is a guaranteed non-empty id for downstream logic (e.g., throw if
sub is missing).
In `@apps/meteor/app/apple/server/appleOauthRegisterService.ts`:
- Around line 25-38: The watcher currently only disables Apple OAuth when all of
clientId, serverSecret, iss, and kid are empty; change the logic in
appleOauthRegisterService.ts so that any missing required credential (clientId,
serverSecret, iss, or kid) is treated as "disabled": call
ServiceConfiguration.configurations.upsertAsync (the same document keyed by
service: 'apple') setting enabled: false and showButton: false and then return
instead of proceeding to sign the secret or upserting an enabled provider;
ensure the check references the same variables (clientId, serverSecret, iss,
kid) used later so partial configurations never create an enabled provider.
In `@apps/meteor/definition/externals/meteor/accounts-base.d.ts`:
- Around line 40-44: The declaration for updateOrCreateUserFromExternalService
is currently synchronous but can be overridden with an async implementation;
update the type to allow both sync and async shapes (e.g., change the return
type to support Promise<Record<string, unknown>> or provide an overload that
returns either Record<string, unknown> or Promise<Record<string, unknown>>), so
callers aren’t incorrectly treated as non-awaited when an async override (like
the one in accounts_meld.js) is used; update the function signature
(updateOrCreateUserFromExternalService) accordingly to accept serviceName,
serviceData, options and return either Record<string, unknown> or
Promise<Record<string, unknown>>.
🪄 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: 002076d7-c2b8-46d8-ac96-3c39c1a6cd39
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (73)
.changeset/flat-poets-cheat.mdapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/2fa/server/code/EmailCheckForOAuth.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/app/2fa/server/code/TOTPCheckForOAuth.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/api/server/index.tsapps/meteor/app/api/server/v1/twoFactorChallenges.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/apple/server/appleOauthRegisterService.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/custom-oauth/server/customOAuth.tsapps/meteor/app/custom-oauth/server/custom_oauth_server.jsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/linkedin/server/index.tsapps/meteor/app/linkedin/server/lib.tsapps/meteor/app/meteor-developer/server/index.tsapps/meteor/app/meteor-developer/server/lib.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/client/lib/2fa/process2faReturn.tsapps/meteor/client/lib/buildAuthDeeplinkURL.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/client/startup/routes.tsxapps/meteor/client/views/OAuthTwoFactorAuthentication/OAuthTwoFactorAuthenticationRouter.tsxapps/meteor/client/views/root/AppLayout.tsxapps/meteor/client/views/root/hooks/useLoginOtherClients.tsapps/meteor/client/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/views/root/hooks/useShareSessionWithOtherClients.tsapps/meteor/definition/externals/express-session.d.tsapps/meteor/definition/externals/express.d.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/package.jsonapps/meteor/server/configuration/accounts_meld.jsapps/meteor/server/configuration/configurePassport.tsapps/meteor/server/configuration/index.tsapps/meteor/server/importPackages.tsapps/meteor/server/lib/oauth/addPassportCustomOAuth.tsapps/meteor/server/lib/oauth/configureOAuthServices.tsapps/meteor/server/lib/oauth/createOAuthServiceConfig.tsapps/meteor/server/lib/oauth/getOAuthServices.tsapps/meteor/server/lib/oauth/oauthConfigs.tsapps/meteor/server/lib/oauth/passportOAuthCallback.tsapps/meteor/server/lib/oauth/twoFactorAuth.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/server/lib/oauth/verifyFunction.tsapps/meteor/server/models.tsapps/meteor/server/settings/oauth.tsapps/meteor/tests/e2e/fixtures/addCustomOAuth.tspackages/core-typings/src/ILoginServiceConfiguration.tspackages/core-typings/src/ITwoFactorChallenge.tspackages/core-typings/src/IUser.tspackages/core-typings/src/index.tspackages/desktop-api/src/index.tspackages/i18n/src/locales/en.i18n.jsonpackages/model-typings/src/index.tspackages/model-typings/src/models/ITwoFactorChallengesModel.tspackages/models/src/index.tspackages/models/src/modelClasses.tspackages/models/src/models/TwoFactorChallenges.tspackages/rest-typings/src/index.tspackages/rest-typings/src/v1/twoFactorChallenges.tspackages/web-ui-registration/global.d.tspackages/web-ui-registration/src/LoginServices.tsxpackages/web-ui-registration/src/LoginServicesButton.tsxpackages/web-ui-registration/tsconfig.build.jsonpackages/web-ui-registration/tsconfig.json
💤 Files with no reviewable changes (46)
- packages/model-typings/src/models/ITwoFactorChallengesModel.ts
- apps/meteor/server/lib/oauth/oauthConfigs.ts
- apps/meteor/app/custom-oauth/server/custom_oauth_server.js
- apps/meteor/app/meteor-developer/server/index.ts
- apps/meteor/app/2fa/server/code/TOTPCheckForOAuth.ts
- apps/meteor/server/importPackages.ts
- apps/meteor/client/views/root/hooks/useShareSessionWithOtherClients.ts
- apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts
- apps/meteor/server/lib/oauth/getOAuthServices.ts
- apps/meteor/server/lib/oauth/twoFactorAuth.ts
- apps/meteor/client/lib/buildAuthDeeplinkURL.ts
- apps/meteor/server/lib/oauth/configureOAuthServices.ts
- apps/meteor/server/lib/oauth/passportOAuthCallback.ts
- apps/meteor/server/configuration/index.ts
- packages/web-ui-registration/global.d.ts
- apps/meteor/server/lib/oauth/verifyFunction.ts
- apps/meteor/server/configuration/configurePassport.ts
- apps/meteor/client/views/root/AppLayout.tsx
- apps/meteor/app/2fa/server/code/EmailCheckForOAuth.ts
- apps/meteor/app/linkedin/server/lib.ts
- apps/meteor/app/custom-oauth/server/customOAuth.ts
- apps/meteor/client/views/OAuthTwoFactorAuthentication/OAuthTwoFactorAuthenticationRouter.tsx
- apps/meteor/definition/externals/express-session.d.ts
- packages/core-typings/src/ITwoFactorChallenge.ts
- apps/meteor/tests/e2e/fixtures/addCustomOAuth.ts
- .changeset/flat-poets-cheat.md
- apps/meteor/app/linkedin/server/index.ts
- packages/core-typings/src/index.ts
- apps/meteor/app/api/server/v1/twoFactorChallenges.ts
- packages/core-typings/src/ILoginServiceConfiguration.ts
- packages/model-typings/src/index.ts
- apps/meteor/app/meteor-developer/server/lib.ts
- packages/models/src/index.ts
- packages/desktop-api/src/index.ts
- apps/meteor/server/models.ts
- packages/rest-typings/src/v1/twoFactorChallenges.ts
- apps/meteor/package.json
- apps/meteor/client/startup/routes.tsx
- apps/meteor/client/views/root/hooks/useLoginOtherClients.ts
- packages/models/src/modelClasses.ts
- packages/models/src/models/TwoFactorChallenges.ts
- apps/meteor/server/lib/oauth/createOAuthServiceConfig.ts
- apps/meteor/server/settings/oauth.ts
- packages/rest-typings/src/index.ts
- apps/meteor/definition/externals/express.d.ts
- packages/i18n/src/locales/en.i18n.json
📜 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). (3)
- GitHub Check: 📦 Build Packages
- GitHub Check: Hacktron Security Check
- 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/views/root/hooks/useLoginViaQuery.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/server/configuration/accounts_meld.jsapps/meteor/app/gitlab/server/lib.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/apple/server/loginHandler.tspackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/app/api/server/index.tsapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/app/drupal/server/lib.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/nextcloud/server/lib.tspackages/web-ui-registration/src/LoginServices.tsxapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.tspackages/core-typings/src/IUser.tsapps/meteor/app/apple/server/appleOauthRegisterService.ts
🧠 Learnings (6)
📚 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/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/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/views/root/hooks/useLoginViaQuery.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/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/views/root/hooks/useLoginViaQuery.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/api/server/index.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.tspackages/core-typings/src/IUser.tsapps/meteor/app/apple/server/appleOauthRegisterService.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/views/root/hooks/useLoginViaQuery.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/apple/server/loginHandler.tsapps/meteor/app/api/server/index.tsapps/meteor/app/drupal/server/lib.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/nextcloud/server/lib.tsapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.tspackages/core-typings/src/IUser.tsapps/meteor/app/apple/server/appleOauthRegisterService.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/views/root/hooks/useLoginViaQuery.tsapps/meteor/app/2fa/server/code/TOTPCheck.tsapps/meteor/client/lib/sdk/ddpSdk.tsapps/meteor/app/gitlab/server/lib.tsapps/meteor/definition/externals/meteor/accounts-base.d.tsapps/meteor/app/lib/server/methods/createToken.tsapps/meteor/app/dolphin/server/lib.tsapps/meteor/app/api/server/ApiClass.tsapps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxapps/meteor/app/apple/lib/handleIdentityToken.tsapps/meteor/app/2fa/server/code/EmailCheck.tsapps/meteor/app/apple/server/loginHandler.tspackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/app/api/server/index.tsapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxapps/meteor/app/drupal/server/lib.tsapps/meteor/server/lib/oauth/updateOAuthServices.tsapps/meteor/app/wordpress/server/lib.tsapps/meteor/app/nextcloud/server/lib.tspackages/web-ui-registration/src/LoginServices.tsxapps/meteor/app/2fa/server/code/index.tsapps/meteor/client/lib/2fa/process2faReturn.tspackages/core-typings/src/IUser.tsapps/meteor/app/apple/server/appleOauthRegisterService.ts
📚 Learning: 2026-03-27T14:52:56.865Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 39892
File: apps/meteor/client/views/room/contextualBar/Threads/Thread.tsx:150-155
Timestamp: 2026-03-27T14:52:56.865Z
Learning: In Rocket.Chat, there are two different `ModalBackdrop` components with different prop APIs. During review, confirm the import source: (1) `rocket.chat/fuselage` `ModalBackdrop` uses `ModalBackdropProps` based on `BoxProps` (so it supports `onClick` and other Box/DOM props) and does not have an `onDismiss` prop; (2) `rocket.chat/ui-client` `ModalBackdrop` uses a narrower props interface like `{ children?: ReactNode; onDismiss?: () => void }` and handles Escape keypress and outside mouse-up, and it does not forward arbitrary DOM props such as `onClick`. Flag mismatched props (e.g., `onDismiss` passed to the fuselage component or `onClick` passed to the ui-client component) and ensure the usage matches the correct component being imported.
Applied to files:
apps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsxpackages/web-ui-registration/src/LoginServicesButton.tsxapps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsxpackages/web-ui-registration/src/LoginServices.tsx
🧬 Code graph analysis (10)
apps/meteor/client/lib/sdk/ddpSdk.ts (1)
apps/meteor/client/lib/sdk/storage.ts (1)
getStoredItem(19-19)
apps/meteor/app/gitlab/server/lib.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
apps/meteor/app/dolphin/server/lib.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
apps/meteor/client/components/TwoFactorModal/TwoFactorModal.tsx (1)
apps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsx (3)
onClickResendCode(38-48)TwoFactorEmailModalProps(12-16)TwoFactorEmailModal(22-102)
apps/meteor/client/components/TwoFactorModal/TwoFactorEmailModal.tsx (1)
apps/meteor/app/2fa/server/code/EmailCheck.ts (1)
sendEmailCode(99-113)
apps/meteor/app/drupal/server/lib.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
apps/meteor/app/wordpress/server/lib.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
apps/meteor/app/nextcloud/server/lib.ts (1)
apps/meteor/app/custom-oauth/server/custom_oauth_server.d.ts (1)
CustomOAuth(1-7)
packages/web-ui-registration/src/LoginServices.tsx (1)
packages/web-ui-registration/src/LoginServicesButton.tsx (1)
LoginServicesButton(11-55)
🔇 Additional comments (11)
packages/web-ui-registration/tsconfig.build.json (1)
3-3: LGTM!packages/web-ui-registration/tsconfig.json (1)
7-7: LGTM!apps/meteor/client/views/root/hooks/useLoginViaQuery.ts (1)
10-10: LGTM!apps/meteor/app/2fa/server/code/TOTPCheck.ts (1)
8-8: LGTM!apps/meteor/client/lib/sdk/ddpSdk.ts (1)
71-71: LGTM!apps/meteor/app/gitlab/server/lib.ts (1)
1-1: LGTM!Also applies to: 5-5, 8-23, 25-31
apps/meteor/definition/externals/meteor/accounts-base.d.ts (1)
26-26: LGTM!apps/meteor/app/lib/server/methods/createToken.ts (1)
19-21: _insertLoginToken is synchronous (: void), so removingawaitincreateToken.tsshouldn’t cause a persistence race.
The localaccounts-basetyping declares_insertLoginToken(...): void; callers can safely treat it as immediate beforeawait User.ensureLoginTokensLimit(userId);. (Other sites usingawait Accounts._insertLoginToken(...)are likely redundant.)> Likely an incorrect or invalid review comment.apps/meteor/app/apple/server/loginHandler.ts (1)
32-35: Remove the Promise/await contract concern forupdateOrCreateUserFromExternalService
Local externals typing declaresAccounts.updateOrCreateUserFromExternalService(...)as a non-Promise return, and there are noawait Accounts.updateOrCreateUserFromExternalService(...)call sites in the repo—so the “Line 35 always hits the failure branch because it’s a Promise” concern doesn’t apply.apps/meteor/server/lib/oauth/updateOAuthServices.ts (1)
71-96: CustomOAuth repeated construction is safe here (cached perserviceKey).
CustomOAuthcaches instances inServices[this.name]; on subsequentnew CustomOAuth(serviceKey, ...)calls it only runsconfigure(options)and returns, avoiding duplicateAccounts.oauth.registerService(...),registerService(),addHookToProcessUser(), andregisterAccessTokenService(...)executions.packages/core-typings/src/IUser.ts (1)
169-238: ConfirmIUsercan dropproviderIdsafely
- No
providerIdmember exists inpackages/core-typings/src/IUser.ts, and there are no in-repo usages ofuser.providerId/profile.providerIdor"providerId"keys within user payloads (the onlyproviderIdreferences are outbound-comms provider identifiers).- This still remains a breaking exported-type change for any downstream consumers still relying on
IUser.providerId; either deprecate for one cycle or ensure/document the migration.
Comment on lines
+48
to
50
|
|
||
| // This has to come last so all endpoints are registered before generating the OpenAPI documentation | ||
| import './default/openApi'; |
Contributor
There was a problem hiding this comment.
Re-add the twoFactorChallenges side-effect import.
This file is the API registration entrypoint. Dropping ./v1/twoFactorChallenges means that module never runs here, so its routes disappear before ./default/openApi generates the spec.
Suggested fix
import './v1/teams';
import './v1/moderation';
import './v1/uploads';
+import './v1/twoFactorChallenges';
// This has to come last so all endpoints are registered before generating the OpenAPI documentation
import './default/openApi';🤖 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/app/api/server/index.ts` around lines 48 - 50, The file dropped
the side-effect import for the two-factor routes so those endpoints aren't
registered before OpenAPI generation; re-add the import for the module (import
'./v1/twoFactorChallenges') in apps/meteor/app/api/server/index.ts so it is
executed before the existing import './default/openApi' (keep the comment
ordering so all endpoints register before OpenAPI is built).
| updateConfig, | ||
| ); | ||
| const updateConfig = _.debounce(() => { | ||
| config.serverURL = settings.get<string>('API_Gitlab_URL').trim().replace(/\/*$/, '') || config.serverURL; |
Contributor
There was a problem hiding this comment.
Guard API_Gitlab_URL before calling .trim().
If the setting resolves to a non-string value, this throws and can break OAuth reconfiguration flow.
Proposed fix
- config.serverURL = settings.get<string>('API_Gitlab_URL').trim().replace(/\/*$/, '') || config.serverURL;
+ const rawServerURL = settings.get<string>('API_Gitlab_URL');
+ config.serverURL =
+ (typeof rawServerURL === 'string' ? rawServerURL.trim().replace(/\/*$/, '') : '') || config.serverURL;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| config.serverURL = settings.get<string>('API_Gitlab_URL').trim().replace(/\/*$/, '') || config.serverURL; | |
| const rawServerURL = settings.get<string>('API_Gitlab_URL'); | |
| config.serverURL = | |
| (typeof rawServerURL === 'string' ? rawServerURL.trim().replace(/\/*$/, '') : '') || config.serverURL; |
🤖 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/app/gitlab/server/lib.ts` at line 24, The code calls
settings.get<string>('API_Gitlab_URL').trim() without guarding for
null/undefined or non-string values; update the assignment that sets
config.serverURL so it first reads the raw value (e.g., const raw =
settings.get('API_Gitlab_URL')), verify typeof raw === 'string' and raw.trim()
is used only then, otherwise fall back to the existing config.serverURL; ensure
you still perform the replace(/\/*$/, '') on the trimmed string when valid and
keep the overall fallback behavior intact for config.serverURL.
Comment on lines
+23
to
+28
| const fillServerURL = _.debounce((): void => { | ||
| const nextcloudURL = settings.get<string>('Accounts_OAuth_Nextcloud_URL'); | ||
| if (!nextcloudURL) { | ||
| if (nextcloudURL === undefined) { | ||
| return fillServerURL(); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't self-reschedule the debounced initializer.
When Accounts_OAuth_Nextcloud_URL is undefined, this schedules fillServerURL() again from inside its own debounced body, which leaves a perpetual 1-second retry loop on instances where the setting never materializes. settings.watch(...) already re-invokes this path when the setting changes, so this branch should just return.
Suggested fix
const fillServerURL = _.debounce((): void => {
const nextcloudURL = settings.get<string>('Accounts_OAuth_Nextcloud_URL');
if (!nextcloudURL) {
- if (nextcloudURL === undefined) {
- return fillServerURL();
- }
return;
}
config.serverURL = nextcloudURL.trim().replace(/\/*$/, '');
return Nextcloud.configure(config);
}, 1000);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const fillServerURL = _.debounce((): void => { | |
| const nextcloudURL = settings.get<string>('Accounts_OAuth_Nextcloud_URL'); | |
| if (!nextcloudURL) { | |
| if (nextcloudURL === undefined) { | |
| return fillServerURL(); | |
| } | |
| const fillServerURL = _.debounce((): void => { | |
| const nextcloudURL = settings.get<string>('Accounts_OAuth_Nextcloud_URL'); | |
| if (!nextcloudURL) { | |
| return; | |
| } | |
| config.serverURL = nextcloudURL.trim().replace(/\/*$/, ''); | |
| return Nextcloud.configure(config); | |
| }, 1000); |
🤖 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/app/nextcloud/server/lib.ts` around lines 23 - 28, The debounced
initializer fillServerURL currently self-reschedules by calling fillServerURL()
when settings.get('Accounts_OAuth_Nextcloud_URL') returns undefined; remove that
recursive call so the debounced function simply returns when nextcloudURL is
undefined (letting settings.watch trigger future invocations) — edit the
fillServerURL function to check nextcloudURL and if it === undefined just return
instead of calling fillServerURL(), leaving the rest of the _.debounce behavior
unchanged.
Comment on lines
+36
to
+40
| const sendEmailCode = useEndpoint('POST', '/v1/users.2fa.sendEmailCode'); | ||
|
|
||
| const onClickResendCode = async (): Promise<void> => { | ||
| try { | ||
| if (!resendEmail) { | ||
| throw new Error('resendEmail is not defined'); | ||
| } | ||
| await resendEmail(); | ||
| await sendEmailCode({ emailOrUsername }); |
Contributor
There was a problem hiding this comment.
Prevent overlapping resend requests.
Each resend regenerates and stores a fresh email code in apps/meteor/app/2fa/server/code/EmailCheck.ts:98-112, so repeated clicks here can invalidate the previous email before the user reads it. Gate the handler with local resend state and disable the button while that request is in flight.
Suggested fix
import { useToastMessageDispatch, useEndpoint } from '`@rocket.chat/ui-contexts`';
-import type { ReactElement } from 'react';
+import { useState, type ReactElement } from 'react';
import { useForm, Controller } from 'react-hook-form';
import { useTranslation } from 'react-i18next';
@@
const TwoFactorEmailModal = ({ onConfirm, onClose, emailOrUsername }: TwoFactorEmailModalProps): ReactElement => {
const dispatchToastMessage = useToastMessageDispatch();
const { t } = useTranslation();
+ const [isResending, setIsResending] = useState(false);
@@
const sendEmailCode = useEndpoint('POST', '/v1/users.2fa.sendEmailCode');
const onClickResendCode = async (): Promise<void> => {
+ if (isResending) {
+ return;
+ }
+
+ setIsResending(true);
try {
await sendEmailCode({ emailOrUsername });
dispatchToastMessage({ type: 'success', message: t('Email_sent') });
} catch (error) {
dispatchToastMessage({
type: 'error',
message: t('error-email-send-failed', { message: error }),
});
+ } finally {
+ setIsResending(false);
}
};
@@
- <Button display='flex' justifyContent='end' onClick={onClickResendCode} small mbs={24}>
+ <Button display='flex' justifyContent='end' onClick={onClickResendCode} disabled={isSubmitting || isResending} small mbs={24}>
{t('Cloud_resend_email')}
</Button>Also applies to: 97-99
🤖 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/components/TwoFactorModal/TwoFactorEmailModal.tsx` around
lines 36 - 40, The resend handler (onClickResendCode) currently calls
sendEmailCode directly and allows overlapping requests which can invalidate
previous codes; add a local boolean state (e.g., isResending) and wrap
onClickResendCode with a guard that returns immediately if isResending is true,
set isResending=true before awaiting sendEmailCode(...) and set it false in
finally, and ensure the resend button is disabled while isResending is true;
apply the same guarded state pattern to the other resend handler at the similar
block (lines referenced as 97-99) so no concurrent resend requests can occur.
Comment on lines
31
to
+34
| function assertModalProps(props: { | ||
| method: TwoFactorMethod; | ||
| emailOrUsername?: string; | ||
| }): asserts props is | ||
| | { method: 'totp' | 'password'; invalidAttempt?: boolean } | ||
| | { method: 'email'; emailOrUsername: string; invalidAttempt?: boolean } { | ||
| }): asserts props is { method: 'totp' } | { method: 'password' } | { method: 'email'; emailOrUsername: string } { |
Contributor
There was a problem hiding this comment.
Normalize object login identifiers before asserting email 2FA props.
process2faReturn() still accepts { username } | { email } | { id }, but the tightened email assertion now requires a plain string. When a caller passes an object before a user exists on the client, getProps() falls back to getUser()?.username, so the modal throws instead of opening.
Suggested fix
+const getLoginIdentifier = (
+ value: { username: string } | { email: string } | { id: string } | string | null | undefined,
+): string | undefined => {
+ if (typeof value === 'string') {
+ return value;
+ }
+
+ if (value && 'username' in value) {
+ return value.username;
+ }
+
+ if (value && 'email' in value) {
+ return value.email;
+ }
+
+ if (value && 'id' in value) {
+ return value.id;
+ }
+
+ return undefined;
+};
+
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: getLoginIdentifier(emailOrUsername) ?? getUser()?.username,
};
case 'password':
return { method };
}
};Also applies to: 40-50, 75-77
🤖 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 31 - 34, The
assertModalProps check is too strict for the 'email' branch because callers to
process2faReturn may pass an object identifier ({ username } | { email } | { id
}) and getProps/getUser fallback can return objects; update process2faReturn
(and the other spots flagged around lines 40-50 and 75-77) to normalize the
login identifier to a plain string before calling assertModalProps: extract a
string emailOrUsername from possible object shapes (e.g., prefer a .email or
.username property, or call toString on primitives) and pass that normalized
string into assertModalProps so the 'email' case receives a true string rather
than an object. Ensure you update any helpers used by getProps so all three
locations use the same normalization logic.
Comment on lines
+21
to
+23
| if (serviceName === 'linkedin') { | ||
| serviceData.email = serviceData.emailAddress; | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid overwriting serviceData.email with non-string LinkedIn values.
The unconditional assignment can replace a valid normalized email with undefined/non-string data and bypass the email-based linking path.
Proposed fix
- if (serviceName === 'linkedin') {
- serviceData.email = serviceData.emailAddress;
- }
+ if (serviceName === 'linkedin' && typeof serviceData.email !== 'string' && typeof serviceData.emailAddress === 'string') {
+ serviceData.email = serviceData.emailAddress;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (serviceName === 'linkedin') { | |
| serviceData.email = serviceData.emailAddress; | |
| } | |
| if (serviceName === 'linkedin' && typeof serviceData.email !== 'string' && typeof serviceData.emailAddress === 'string') { | |
| serviceData.email = serviceData.emailAddress; | |
| } |
🤖 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/server/configuration/accounts_meld.js` around lines 21 - 23, The
unconditional assignment serviceData.email = serviceData.emailAddress when
serviceName === 'linkedin' can overwrite a valid normalized email with undefined
or non-string data; change it to only set serviceData.email when
serviceData.emailAddress is a non-empty string (or normalize/validate it) and
otherwise leave existing serviceData.email intact so the email-based linking
path still works—look for the serviceName check and the
serviceData.email/serviceData.emailAddress references to implement this guard.
Comment on lines
30
to
+37
| const handleOnClick = useCallback(() => { | ||
| if (!servicesSupportedByMeteor.includes(service)) { | ||
| const url = new URL(window.location.href); | ||
| const queryParams = url.searchParams; | ||
| const loginClient = queryParams.get('loginClient'); | ||
|
|
||
| const redirectUrl = new URL(`/oauth/${service}`, window.location.origin); | ||
|
|
||
| if (loginClient) { | ||
| redirectUrl.searchParams.set('loginClient', loginClient); | ||
| } | ||
|
|
||
| window.location.href = redirectUrl.toString(); | ||
| return; | ||
| } | ||
|
|
||
| handler().catch((e: { error?: LoginErrors; reason?: string }) => { | ||
| if (!e.error || typeof e.error !== 'string') { | ||
| return; | ||
| } | ||
| setError?.([e.error, e.reason]); | ||
| }); | ||
| }, [handler, setError, service]); | ||
| }, [handler, setError]); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the hook implementation and remaining handling for Meteor-backed auth services.
# Expected: either useLoginWithService or another path still maps saml/cas/ldap to the correct redirect flow.
rg -n -C3 '\buseLoginWithService\b' packages apps
rg -n -C3 '\b(saml|cas|ldap)\b' packages appsRepository: RocketChat/Rocket.Chat
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Read the hook implementation directly
HOOK_FILE="packages/ui-contexts/src/hooks/useLoginWithService.ts"
if [ -f "$HOOK_FILE" ]; then
echo "=== $HOOK_FILE ==="
nl -ba "$HOOK_FILE" | sed -n '1,200p'
else
echo "Missing expected file: $HOOK_FILE"
fi
# 2) Locate the underlying AuthenticationContext loginWithService implementation
echo "=== find loginWithService implementations ==="
rg -n --hidden --no-ignore-vcs "\bloginWithService\b" packages/ui-contexts/src packages/ui-client/src packages/web-ui-registration/src
# 3) Inspect AuthenticationContext for how service types are handled
AUTH_CTX="packages/ui-contexts/src/AuthenticationContext.tsx"
if [ -f "$AUTH_CTX" ]; then
echo "=== $AUTH_CTX ==="
nl -ba "$AUTH_CTX" | sed -n '1,220p'
fi
# 4) Check whether /oauth/${service} redirect logic still exists anywhere in relevant login code
echo "=== search for /oauth/${service} mapping ==="
rg -n --hidden --no-ignore-vcs "/oauth/\$\{?service\}?" packages/ui-contexts/src packages/ui-client/src packages/web-ui-registration/src
# 5) Narrow search for saml/cas/ldap handling specifically near login/service code
echo "=== search saml/cas/ldap near service login ==="
rg -n -C3 --hidden --no-ignore-vcs "(saml|cas|ldap)" packages/ui-contexts/src packages/ui-client/src packages/web-ui-registration/srcRepository: RocketChat/Rocket.Chat
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Hook: packages/ui-contexts/src/hooks/useLoginWithService.ts ==="
HOOK_FILE="packages/ui-contexts/src/hooks/useLoginWithService.ts"
if [ -f "$HOOK_FILE" ]; then
cat -n "$HOOK_FILE" | sed -n '1,220p'
else
echo "Missing expected file: $HOOK_FILE"
fi
echo
echo "=== AuthenticationContext: packages/ui-contexts/src/AuthenticationContext.tsx ==="
AUTH_CTX="packages/ui-contexts/src/AuthenticationContext.tsx"
if [ -f "$AUTH_CTX" ]; then
cat -n "$AUTH_CTX" | sed -n '1,260p'
else
echo "Missing expected file: $AUTH_CTX"
fi
echo
echo "=== Find loginWithService mapping/redirect logic (scoped) ==="
rg -n --hidden --no-ignore-vcs "/oauth/" packages/ui-contexts/src packages/web-ui-registration/src packages/ui-client/src --max-count 50
rg -n --hidden --no-ignore-vcs "(saml|cas|ldap)" packages/ui-contexts/src packages/web-ui-registration/src packages/ui-client/src --max-count 80
echo
echo "=== Search inside useLoginWithService.ts for service type branching ==="
rg -n "saml|cas|ldap|oauth|redirect" "$HOOK_FILE" --max-count 200 || trueRepository: RocketChat/Rocket.Chat
Length of output: 903
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate AuthenticationContext source ==="
# Find anything exporting/declaring AuthenticationContext in ui-contexts
rg -n --hidden --no-ignore-vcs "AuthenticationContext" packages/ui-contexts/src --max-count 50
echo
echo "=== List files likely containing AuthenticationContext ==="
fd -e ts -e tsx -e js -e jsx "AuthenticationContext" packages/ui-contexts/src || true
fd -e ts -e tsx -e js -e jsx "authentication" packages/ui-contexts/src || true
echo
echo "=== Find loginWithService implementation in ui-contexts ==="
rg -n --hidden --no-ignore-vcs "loginWithService" packages/ui-contexts/src --max-count 50
echo
echo "=== Inspect whichever file defines loginWithService ==="
# Try common candidates
for f in \
packages/ui-contexts/src/AuthenticationContext.ts \
packages/ui-contexts/src/AuthenticationContext/index.ts \
packages/ui-contexts/src/AuthenticationContext.tsx \
packages/ui-contexts/src/AuthenticationContext/index.tsx \
packages/ui-contexts/src/contexts/AuthenticationContext.ts \
packages/ui-contexts/src/contexts/AuthenticationContext.tsx \
packages/ui-contexts/src/hooks/../AuthenticationContext.ts \
packages/ui-contexts/src/hooks/../AuthenticationContext/index.ts
do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,260p'
fi
doneRepository: RocketChat/Rocket.Chat
Length of output: 9607
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the real AuthenticationContext provider implementation that supplies loginWithService
rg -n --hidden --no-ignore-vcs "loginWithService\s*:\s*\(" packages --max-count 50
rg -n --hidden --no-ignore-vcs "loginWithService\s*\(" packages/ui-client/src packages/web-ui-registration/src packages/ui-contexts/src --max-count 50
rg -n --hidden --no-ignore-vcs "loginWithCustomOauth" packages/ui-client/src packages/web-ui-registration/src packages/ui-contexts/src --max-count 50
rg -n --hidden --no-ignore-vcs "AuthenticationContext\.Provider" packages --max-count 50
echo "---- also search for assigning loginWithService in an object ----"
rg -n --hidden --no-ignore-vcs "loginWithService\s*,\s*queryLoginServices|loginWithService\s*,|queryLoginServices\s*:" packages --max-count 50Repository: RocketChat/Rocket.Chat
Length of output: 1568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
set -o pipefail
rg -n --hidden --no-ignore-vcs "AuthenticationContext\.Provider" . --max-count 50
echo
echo "---- Inspect packages/mock-providers/src/MockedAppRootBuilder.tsx loginWithService implementation ----"
sed -n '220,320p' packages/mock-providers/src/MockedAppRootBuilder.tsx
echo
echo "---- Search for a production login/authentication provider in ui-client/web-ui-registration ----"
rg -n --hidden --no-ignore-vcs "loginWithService" packages/ui-client packages/web-ui-registration packages/ui-contexts --max-count 50Repository: RocketChat/Rocket.Chat
Length of output: 4517
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx"
if [ -f "$FILE" ]; then
echo "=== Inspect $FILE around loginWithService ==="
# show a reasonably sized window where loginWithService is likely defined/assigned
rg -n "loginWithService" "$FILE" --max-count 20
# print a couple windows around each match
rg -n "loginWithService" "$FILE" --max-count 20 | cut -d: -f1 | while read -r line; do
start=$((line-40)); end=$((line+80));
echo
echo "--- lines ${start}-${end} (match at $line) ---"
sed -n "${start},${end}p" "$FILE"
done
else
echo "Missing expected file: $FILE"
fi
echo
echo "=== Search within AuthenticationProvider for saml/cas/ldap and oauth mapping ==="
if [ -f "$FILE" ]; then
rg -n --hidden --no-ignore-vcs "(saml|cas|ldap|/oauth/)" "$FILE" --max-count 80
rg -n --hidden --no-ignore-vcs "oauth" "$FILE" --max-count 80
fiRepository: RocketChat/Rocket.Chat
Length of output: 11858
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Find where login service buttons are defined (service names) ==="
rg -n --hidden --no-ignore-vcs "getLoginServiceButtons" apps packages --max-count 50
echo
echo "=== Inspect login service buttons type/fields (look for saml/cas/ldap) ==="
rg -n --hidden --no-ignore-vcs "(saml|cas|ldap)" apps/meteor/client apps/meteor/lib apps/meteor/server packages --max-count 80
echo
echo "=== Search Meteor loginWith* methods for SAML/CAS/LDAP ==="
rg -n --hidden --no-ignore-vcs "loginWith(.*Saml|.*SAML|.*Cas|.*CAS|.*Ldap|.*LDAP)" apps/meteor --max-count 80
echo
echo "=== Search for /oauth/${service} or generic /oauth/ redirects ==="
rg -n --hidden --no-ignore-vcs "/oauth/\\$\\{?service\\}?|/oauth/\\$\\{service\\}|/oauth/\\$\\{?serviceName\\}?" apps/meteor --max-count 80Repository: RocketChat/Rocket.Chat
Length of output: 50443
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== apps/meteor/client/lib/loginServices.ts (getLoginServiceButtons) ==="
FILE="apps/meteor/client/lib/loginServices.ts"
if [ -f "$FILE" ]; then
cat -n "$FILE" | sed -n '1,260p'
echo
rg -n "getLoginServiceButtons" "$FILE"
echo
# show around the function
rg -n "getLoginServiceButtons" "$FILE" | cut -d: -f1 | head -n 5 | while read -r line; do
start=$((line-40)); end=$((line+200));
echo "--- lines ${start}-${end} ---"
sed -n "${start},${end}p" "$FILE"
done
else
echo "Missing expected file: $FILE"
fi
echo
echo "=== service button definitions for saml/cas/ldap (in loginServices.ts) ==="
if [ -f "$FILE" ]; then
rg -n "(service\\s*[:=]\\s*['\"](saml|cas|ldap)['\"]|['\"](saml|cas|ldap)['\"])"
"$FILE" || true
fi
echo
echo "=== Meteor loginWith* method implementations for saml/cas/ldap (apps/meteor) ==="
rg -n --hidden --no-ignore-vcs "loginWith(Saml|CAS|Cas|Ldap|LDAP|SAML|CAS)" apps/meteor --max-count 80 || true
rg -n --hidden --no-ignore-vcs "loginWithSaml|loginWithCas|loginWithLdap|loginWithLDAP|loginWithSAML" apps/meteor --max-count 80 || true
echo
echo "=== Any remaining hardcoded /oauth/${service} redirects (apps/meteor) ==="
rg -n --hidden --no-ignore-vcs "/oauth/\\$\\{?service\\}?|/oauth/\\$\\{service\\}|/oauth/" apps/meteor --max-count 80 || trueRepository: RocketChat/Rocket.Chat
Length of output: 16003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect capitalize implementation ==="
CAP_FILE="$(rg -n --hidden --no-ignore-vcs "export function capitalize|export const capitalize" packages -S --max-count 20 || true | head -n 1 | awk -F: '{print $1}')"
if [ -n "${CAP_FILE:-}" ] && [ -f "$CAP_FILE" ]; then
echo "capitalize file: $CAP_FILE"
cat -n "$CAP_FILE" | sed -n '1,160p'
else
rg -n --hidden --no-ignore-vcs "capitalize\(" packages --max-count 20 || true
fi
echo
echo "=== Inspect Meteor SAML/CAS/LDAP loginWith* implementations ==="
for f in \
apps/meteor/client/meteor/login/saml.ts \
apps/meteor/client/meteor/login/cas.ts \
apps/meteor/client/meteor/login/ldap.ts
do
echo
echo "--- $f ---"
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,220p'
else
echo "missing: $f"
fi
done
echo
echo "=== Inspect LDAP service name used server-side ==="
for f in \
apps/meteor/server/services/ldap/service.ts \
apps/meteor/server/configuration/ldap.ts
do
echo
echo "--- $f ---"
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,220p'
else
echo "missing: $f"
fi
done
echo
echo "=== Inspect service.configurations endpoint shaping the service field ==="
rg -n --hidden --no-ignore-vcs "service\.configurations|get\\/v1\\/service\\.configurations|\\/v1\\/service\\.configurations" apps/meteor/app apps/meteor/server --max-count 50Repository: RocketChat/Rocket.Chat
Length of output: 10348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate capitalize implementation ==="
# Try to find actual implementation in `@rocket.chat/string-helpers`
rg -n --hidden --no-ignore-vcs "export (function|const) capitalize" packages -S --max-count 50 || true
rg -n --hidden --no-ignore-vcs "capitalize\s*=\s*\(" packages -S --max-count 50 || true
rg -n --hidden --no-ignore-vcs "function capitalize\(" packages -S --max-count 50 || true
# Also locate usage site that imports capitalize from '`@rocket.chat/string-helpers`'
rg -n --hidden --no-ignore-vcs "from '`@rocket.chat/string-helpers`'.*capitalize" packages -S --max-count 20 || true
echo
echo "=== Inspect service.configurations endpoint (/v1/service.configurations) ==="
rg -n --hidden --no-ignore-vcs "get\\(\\s*['\"]\\/v1\\/service\\.configurations['\"]|/v1\\/service\\.configurations" apps/meteor --max-count 50 || true
rg -n --hidden --no-ignore-vcs "service\\.configurations" apps/meteor --max-count 80 || true
# Find where the service string is set for LDAP in that endpoint/provider code
rg -n --hidden --no-ignore-vcs "service:\\s*['\"]ldap['\"]|service:\\s*['\"]LDAP['\"]|['\"]ldap['\"]\\s*\\)|['\"]LDAP['\"]" apps/meteor/app apps/meteor/server apps/meteor/lib --max-count 80 || trueRepository: RocketChat/Rocket.Chat
Length of output: 1167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== capitalize() implementation candidates ==="
rg -n --hidden --no-ignore-vcs "capitalize" packages -S --max-count 50 | head -n 50
echo
echo "=== Inspect apps/meteor/app/api/server/v1/settings.ts for service.configurations handler ==="
FILE="apps/meteor/app/api/server/v1/settings.ts"
if [ -f "$FILE" ]; then
# show sections around the previously seen line ~188 and around where route map includes service.configurations (~408)
sed -n '150,260p' "$FILE"
echo
sed -n '360,480p' "$FILE"
else
echo "Missing $FILE"
fi
echo
echo "=== Inspect apps/meteor/tests/end-to-end/api/settings.ts around /service.configurations assertions ==="
TESTFILE="apps/meteor/tests/end-to-end/api/settings.ts"
if [ -f "$TESTFILE" ]; then
sed -n '200,330p' "$TESTFILE"
else
echo "Missing $TESTFILE"
fiRepository: RocketChat/Rocket.Chat
Length of output: 9594
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate capitalize implementation in-repo ==="
rg -n --hidden --no-ignore-vcs "function capitalize|export const capitalize|export function capitalize" packages -S --max-count 50 || true
rg -n --hidden --no-ignore-vcs "capitalize\(" packages/string-helpers packages -S --max-count 50 || true
fd -e ts -e tsx -e js -e jsx "capitalize*" packages -t f | head -n 50 || true
echo
echo "=== inspect AuthenticationProvider.tsx imports for capitalize ==="
FILE="apps/meteor/client/providers/AuthenticationProvider/AuthenticationProvider.tsx"
if [ -f "$FILE" ]; then
sed -n '1,120p' "$FILE"
fi
echo
echo "=== find where service configurations are created for LDAP (service field casing) ==="
rg -n --hidden --no-ignore-vcs "service\s*:\s*['\"]LDAP['\"]|service\s*:\s*['\"]ldap['\"]" apps/meteor --max-count 80 || true
rg -n --hidden --no-ignore-vcs "LoginServiceConfigurationModel.*ldap|ldap.*LoginServiceConfigurationModel" apps/meteor --max-count 80 || true
rg -n --hidden --no-ignore-vcs "'ldap'|\"ldap\"" apps/meteor/server apps/meteor/app apps/meteor/lib --max-count 80 || trueRepository: RocketChat/Rocket.Chat
Length of output: 5105
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate package providing `@rocket.chat/string-helpers` ==="
rg -n --hidden --no-ignore-vcs "\"`@rocket.chat/string-helpers`\"" packages -S --max-count 50 || true
rg -n --hidden --no-ignore-vcs "\"name\"\\s*:\\s*\"`@rocket.chat/string-helpers`\"" . -S --max-count 50 || true
echo
echo "=== Search for LoginServiceConfigurationModel definition/usages ==="
rg -n --hidden --no-ignore-vcs "LoginServiceConfigurationModel" apps/meteor packages -S --max-count 80 || true
echo
echo "=== Search for service value 'ldap' / 'LDAP' used when creating LoginServiceConfiguration entries ==="
rg -n --hidden --no-ignore-vcs "service\\s*:\\s*['\"](ldap|LDAP)['\"]" apps/meteor -S --max-count 80 || true
rg -n --hidden --no-ignore-vcs "['\"](ldap|LDAP)['\"]\\s*\\)\\s*[,}]|['\"](ldap|LDAP)['\"]\\s*," apps/meteor -S --max-count 80 || true
echo
echo "=== Search for service values 'saml'/'cas' casing in service configurations creation ==="
rg -n --hidden --no-ignore-vcs "service\\s*:\\s*['\"](saml|cas)['\"]" apps/meteor -S --max-count 80 || true
rg -n --hidden --no-ignore-vcs "service\\s*:\\s*['\"](Saml|CAS)['\"]" apps/meteor -S --max-count 80 || true
echo
echo "=== As a fallback, inspect schema/type for LoginServiceConfiguration.service ==="
rg -n --hidden --no-ignore-vcs "type\\s+LoginServiceConfiguration|interface\\s+LoginServiceConfiguration|service\\s*:\\s*['\"]" packages/core-typings apps/meteor -S --max-count 80 || trueRepository: RocketChat/Rocket.Chat
Length of output: 6023
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspect service typing (core-typings) ==="
for f in \
packages/core-typings/src/ILoginServiceConfiguration.ts \
packages/core-typings/src/ILoginServiceConfiguration.tsx \
packages/core-typings/src/index.ts
do
if [ -f "$f" ]; then
echo "--- $f (first 220 lines) ---"
cat -n "$f" | sed -n '1,220p'
fi
done
echo
echo "=== Inspect service model typing/normalization (model-typings + models) ==="
for f in \
packages/model-typings/src/models/ILoginServiceConfigurationModel.ts \
packages/models/src/models/LoginServiceConfiguration.ts
do
if [ -f "$f" ]; then
echo "--- $f (first 260 lines) ---"
cat -n "$f" | sed -n '1,260p'
fi
done
echo
echo "=== Search for loginWithLdap usage (should exist only if capitalize('ldap') yields Ldap) ==="
rg -n --hidden --no-ignore-vcs "loginWithLdap" apps/meteor || true
echo
echo "=== Search for loginWithLDAP usage (ground truth target method) ==="
rg -n --hidden --no-ignore-vcs "loginWithLDAP" apps/meteor || trueRepository: RocketChat/Rocket.Chat
Length of output: 13819
🌐 Web query:
query="site:github.com \"@rocket.chat/string-helpers\" capitalize ldap"
💡 Result:
The @rocket.chat/string-helpers package includes a capitalize utility [1][2], which has been used within the Rocket.Chat codebase for various string manipulation tasks, including OAuth service naming [3][4]. Regarding LDAP, Rocket.Chat has historically faced challenges with username casing during LDAP synchronization [5]. In the past, LDAP fields mapped to usernames were often automatically converted to lowercase, which could cause issues for environments requiring mixed-case usernames [5]. Recent developments have addressed the need for more flexible LDAP string manipulation. Specifically, a feature merged in April 2025 (as part of the 7.6.0 milestone) introduced support for LDAP custom variables and string manipulation [6]. This update allows for more granular control over how LDAP attributes are processed and mapped, providing a solution for users who need to customize how LDAP data—including username fields—is handled during synchronization [6].
Citations:
- 1: String helpers #21988
- 2: https://registry.npmjs.org/%40rocket.chat%2Fstring-helpers
- 3: https://gitlab.ow2.org/RocketChat/Rocket.Chat/-/blame/d2737f8a582faa9f81288be6cf198163d6790b7f/app/lib/server/functions/addOAuthService.ts
- 4: https://gitlab.ow2.org/RocketChat/Rocket.Chat/-/commit/0c94ea356d0630cdc152ae191eb89dade6725f34.patch
- 5: Allow mixed-case usernames from LDAP #12116
- 6: feat: ldap custom variables / string manipulation #35717
🌐 Web query:
query="\"@rocket.chat/string-helpers\" capitalize implementation"
💡 Result:
In @rocket.chat/string-helpers, the capitalize helper is implemented as a small function that: 1) returns an empty string when the input is not a string or is empty, and 2) otherwise uppercases the first character and appends the rest of the string. Implementation (behaviorally): - if typeof str!== "string" or str.length === 0 => "" - else => str.charAt(0).toUpperCase + str.slice(1) [1] Rocket.Chat’s codebase uses the external @rocket.chat/string-helpers package for its string helper utilities (i.e., Rocket.Chat calls into this package rather than re-implementing capitalize internally). [2]
Citations:
Fix LDAP login button wiring (service: 'ldap' calls the wrong Meteor method)
useLoginWithService forwards to AuthenticationProvider.loginWithService, which builds the Meteor method name as loginWith${capitalize(service)}. With service = 'ldap', capitalize('ldap') => Ldap, so it looks for Meteor.loginWithLdap, but Meteor defines Meteor.loginWithLDAP. Because LoginServiceConfiguration normalizes service to lowercase, this is likely a real break for the LDAP button after removing the /oauth/${service} redirect.
🤖 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 `@packages/web-ui-registration/src/LoginServicesButton.tsx` around lines 30 -
37, The LDAP button fails because AuthenticationProvider.loginWithService builds
the Meteor method name with capitalize(service) (e.g., "Ldap") but Meteor
exposes "loginWithLDAP"; update loginWithService (used by useLoginWithService)
to special-case known acronym services (at least 'ldap') so the generated method
name matches Meteor (e.g., if service.toLowerCase() === 'ldap' use
"loginWithLDAP"), or implement a small mapping/normalization that converts
service names like 'ldap' to the exact expected casing before calling
Meteor.loginWithX.
![hacktron-app[bot]](https://avatars.githubusercontent.com/in/1250513?s=60&v=4){kind=small}
Comment on lines
+21
to
+24
| if (serviceName === 'linkedin') { | ||
| serviceData.email = serviceData.emailAddress; | ||
| } | ||
|
|
There was a problem hiding this comment.
🔴 High: Insecure Account Linking via OAuth Provider Email Trust
The configureAccounts function in accounts_meld.js implements an insecure account linking mechanism. When a user authenticates via an external OAuth provider, the application retrieves the email address from the provider's serviceData. If a local user account exists with the same email address, the application automatically links the external OAuth identity to that local account and marks the email as verified, regardless of whether the local account had previously verified that email. An attacker who can register an account on a supported OAuth provider (or a custom OAuth provider) with a victim's email address can use this to take over the victim's account on the Rocket.Chat instance.
Steps to Reproduce
- Identify a target user's email address (e.g.,
victim@example.com). - Register an account on an OAuth provider supported by the Rocket.Chat instance (e.g., GitHub, GitLab, or a custom OAuth provider) using the target's email address.
- Initiate an OAuth login on the Rocket.Chat instance using the registered OAuth provider account.
- The Rocket.Chat instance will extract the email address from the OAuth provider's response and automatically link the OAuth identity to the existing local account associated with
victim@example.com. - The attacker is now logged into the victim's account and has full access.
Trace
graph TD
subgraph SG0 ["./Rocket.Chat/apps/meteor/server/configuration/accounts_meld.js"]
configureAccounts{{"Initializes account configuration by overriding the external service user creation handler."}}
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["./Rocket.Chat/apps/meteor/server/configuration/index.ts"]
configureServer["Initializes and configures various server-side components and features."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
configureServer --> configureAccounts
Fix with AI
Fix the following security vulnerability found by Hacktron.
File: apps/meteor/server/configuration/accounts_meld.js
Lines: 21-24
Severity: high
Vulnerability: Insecure Account Linking via OAuth Provider Email Trust
Description:
The `configureAccounts` function in `accounts_meld.js` implements an insecure account linking mechanism. When a user authenticates via an external OAuth provider, the application retrieves the email address from the provider's `serviceData`. If a local user account exists with the same email address, the application automatically links the external OAuth identity to that local account and marks the email as verified, regardless of whether the local account had previously verified that email. An attacker who can register an account on a supported OAuth provider (or a custom OAuth provider) with a victim's email address can use this to take over the victim's account on the Rocket.Chat instance.
Proof of Concept:
**Steps to Reproduce**
1. Identify a target user's email address (e.g., `victim@example.com`).
2. Register an account on an OAuth provider supported by the Rocket.Chat instance (e.g., GitHub, GitLab, or a custom OAuth provider) using the target's email address.
3. Initiate an OAuth login on the Rocket.Chat instance using the registered OAuth provider account.
4. The Rocket.Chat instance will extract the email address from the OAuth provider's response and automatically link the OAuth identity to the existing local account associated with `victim@example.com`.
5. The attacker is now logged into the victim's account and has full access.
Affected Code:
if (serviceData.email) {
const user = await Users.findOneByEmailAddress(serviceData.email);
if (user != null && user.services?.[serviceName]?.id !== serviceData.id) {
const findQuery = {
address: serviceData.email,
verified: true,
};
if (user.services?.password && !_.findWhere(user.emails, findQuery)) {
await Users.resetPasswordAndSetRequirePasswordChange(
user._id,
true,
'This_email_has_already_been_used_and_has_not_been_verified__Please_change_your_password',
);
}
await Users.setServiceId(user._id, serviceName, serviceData.id);
await Users.setEmailVerified(user._id, serviceData.email);
}
}
Fix this vulnerability. Only change what's necessary - don't modify unrelated code.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing. Any other reply is saved as a triage note.
There was a problem hiding this comment.
🔴 High: Account Takeover via Unverified OAuth Account Merging
The CustomOAuth class implements a mergeUsers feature that allows linking external OAuth identities to existing local accounts based solely on email or username matches. This merge occurs without verifying if the external identity's email address is verified by the provider, and without requiring the target user to authorize the link. If an attacker registers an account on a configured external OAuth provider using a target user's email address, they can log into Rocket.Chat via that OAuth provider. Rocket.Chat will automatically merge the accounts, granting the attacker unauthorized access to the target user's account. This affects any CustomOAuth provider where mergeUsers is enabled (e.g., Drupal is enabled by default).
Steps to Reproduce
- Identify a Rocket.Chat instance with a CustomOAuth provider enabled and
mergeUsersset to true (e.g., Drupal, or Gitlab with merge enabled). - Identify the email address of a target user on the Rocket.Chat instance.
- Register an account on the external OAuth provider using the target user's email address. Do not verify the email address if the provider does not force it before OAuth login.
- Log into Rocket.Chat using the external OAuth provider.
- Rocket.Chat will automatically merge the new OAuth identity with the existing target user's account based on the email match.
- The attacker is now logged into the target user's account.
Trace
graph TD
subgraph SG0 ["./Rocket.Chat/apps/meteor/app/custom-oauth/server/custom_oauth_server.js"]
._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js{{"Top-level initialization of the CustomOAuth logger."}}
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["./Rocket.Chat/apps/meteor/app/dolphin/server/lib.ts"]
._Rocket.Chat_apps_meteor_app_dolphin_server_lib.ts["Top-level setup for Dolphin OAuth integration, defining configuration and initializing the OAuth provider."]
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["./Rocket.Chat/apps/meteor/app/drupal/server/lib.ts"]
._Rocket.Chat_apps_meteor_app_drupal_server_lib.ts["Top-level setup for Drupal OAuth integration, defining configuration and initializing the OAuth provider."]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG3 ["./Rocket.Chat/apps/meteor/app/gitlab/server/lib.ts"]
._Rocket.Chat_apps_meteor_app_gitlab_server_lib.ts["Top-level setup for Gitlab OAuth integration, defining configuration and initializing the OAuth provider."]
end
style SG3 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG4 ["./Rocket.Chat/apps/meteor/app/nextcloud/server/lib.ts"]
._Rocket.Chat_apps_meteor_app_nextcloud_server_lib.ts["Initializes the Nextcloud OAuth integration and configures the server URL."]
end
style SG4 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG5 ["./Rocket.Chat/apps/meteor/app/wordpress/server/lib.ts"]
._Rocket.Chat_apps_meteor_app_wordpress_server_lib.ts["Initializes WordPress OAuth configuration and sets up watchers for relevant settings."]
end
style SG5 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG6 ["./Rocket.Chat/apps/meteor/server/lib/oauth/updateOAuthServices.ts"]
updateOAuthServices["Updates OAuth service configurations in the database based on system settings."]
end
style SG6 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG7 ["./Rocket.Chat/ee/packages/media-calls/src/logger.ts"]
._Rocket.Chat_ee_packages_media-calls_src_logger.ts["Initializes and exports a logger instance for the MediaCallsServer."]
end
style SG7 fill:#2a2a2a,stroke:#444,color:#aaa
._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js --> ._Rocket.Chat_ee_packages_media-calls_src_logger.ts
._Rocket.Chat_ee_packages_media-calls_src_logger.ts --> ._Rocket.Chat_ee_packages_media-calls_src_logger.ts
._Rocket.Chat_apps_meteor_app_gitlab_server_lib.ts --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
._Rocket.Chat_apps_meteor_app_nextcloud_server_lib.ts --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
._Rocket.Chat_apps_meteor_app_wordpress_server_lib.ts --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
updateOAuthServices --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
._Rocket.Chat_apps_meteor_app_dolphin_server_lib.ts --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
._Rocket.Chat_apps_meteor_app_drupal_server_lib.ts --> ._Rocket.Chat_apps_meteor_app_custom-oauth_server_custom_oauth_server.js
Fix with AI
Fix the following security vulnerability found by Hacktron.
File: apps/meteor/app/gitlab/server/lib.ts
Severity: high
Vulnerability: Account Takeover via Unverified OAuth Account Merging
Description:
The `CustomOAuth` class implements a `mergeUsers` feature that allows linking external OAuth identities to existing local accounts based solely on email or username matches. This merge occurs without verifying if the external identity's email address is verified by the provider, and without requiring the target user to authorize the link. If an attacker registers an account on a configured external OAuth provider using a target user's email address, they can log into Rocket.Chat via that OAuth provider. Rocket.Chat will automatically merge the accounts, granting the attacker unauthorized access to the target user's account. This affects any CustomOAuth provider where `mergeUsers` is enabled (e.g., Drupal is enabled by default).
Proof of Concept:
**Steps to Reproduce**
1. Identify a Rocket.Chat instance with a CustomOAuth provider enabled and `mergeUsers` set to true (e.g., Drupal, or Gitlab with merge enabled).
2. Identify the email address of a target user on the Rocket.Chat instance.
3. Register an account on the external OAuth provider using the target user's email address. Do not verify the email address if the provider does not force it before OAuth login.
4. Log into Rocket.Chat using the external OAuth provider.
5. Rocket.Chat will automatically merge the new OAuth identity with the existing target user's account based on the email match.
6. The attacker is now logged into the target user's account.
Fix this vulnerability. Only change what's necessary - don't modify unrelated code.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing. Any other reply is saved as a triage note.
Comment on lines
+32
to
+35
| const result = Accounts.updateOrCreateUserFromExternalService('apple', serviceData, { profile }); | ||
|
|
||
| // Ensure processing succeeded | ||
| if (result?.userId === undefined) { | ||
| if (result === undefined || result.userId === undefined) { |
There was a problem hiding this comment.
🚨 Critical: Account Takeover via Client-Controlled Email in Apple OAuth Handler
The Apple OAuth login handler in Rocket.Chat registers a custom login handler that processes the loginRequest payload. The handler first validates the Apple identityToken to extract the serviceData (which includes the user's Apple ID sub and email).
Apple's OAuth implementation is known to only include the email claim in the identity token during the first time a user authorizes the application. On subsequent logins, the identity token does not contain the email claim.
To handle this, the Rocket.Chat code attempts to use an email field provided directly in the client-controlled loginRequest if the validated token lacks one. Because the loginRequest is entirely controlled by the client, an attacker can supply an arbitrary email address (e.g., victim@example.com).
When Accounts.updateOrCreateUserFromExternalService is subsequently called, the accounts_meld.js configuration intercepts it. This code looks up an existing user by the (attacker-controlled) email address. If found, it links the attacker's Apple ID (serviceData.id) to the victim's account and logs the attacker in as the victim. This results in a full Account Takeover (ATO) with no user interaction required from the victim.
Steps to Reproduce
- The attacker logs into the target Rocket.Chat instance using their own Apple ID to authorize the app (or uses an Apple ID that has already authorized the app so that Apple omits the
emailclaim in the token). - The attacker intercepts or crafts a DDP
loginrequest to the server, injecting the victim's email address:
{
"msg": "method",
"method": "login",
"id": "1",
"params": [
{
"identityToken": "<ATTACKER_VALID_APPLE_TOKEN_WITHOUT_EMAIL>",
"email": "victim@example.com",
"fullName": {
"givenName": "Attacker",
"familyName": "Attacker"
}
}
]
}- The server validates the attacker's token, finds no email in it, and falls back to
"victim@example.com". - The server links the attacker's Apple ID to the victim's account.
- The server returns a valid authentication token for the victim's account, granting the attacker full access.
Trace
graph TD
subgraph SG0 ["./Rocket.Chat/apps/meteor/app/apple/lib/handleIdentityToken.ts"]
isValidAppleJWT["isValidAppleJWT"]
handleIdentityToken["Parses and validates Apple identity tokens for OAuth authentication."]
end
style SG0 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG1 ["./Rocket.Chat/apps/meteor/app/apple/server/loginHandler.ts"]
Accounts.registerLoginHandler.arg2{{"Registers the Apple login handler to process identity tokens and create/update users."}}
end
style SG1 fill:#2a2a2a,stroke:#444,color:#aaa
subgraph SG2 ["./Rocket.Chat/apps/meteor/app/custom-oauth/server/custom_oauth_server.js"]
Accounts.updateOrCreateUserFromExternalService["Accounts.updateOrCreateUserFromExternalService"]
end
style SG2 fill:#2a2a2a,stroke:#444,color:#aaa
Accounts.registerLoginHandler.arg2 --> handleIdentityToken
Accounts.registerLoginHandler.arg2 --> Accounts.updateOrCreateUserFromExternalService
handleIdentityToken --> isValidAppleJWT
Fix with AI
Fix the following security vulnerability found by Hacktron.
File: apps/meteor/app/apple/server/loginHandler.ts
Lines: 32-35
Severity: critical
Vulnerability: Account Takeover via Client-Controlled Email in Apple OAuth Handler
Description:
The Apple OAuth login handler in Rocket.Chat registers a custom login handler that processes the `loginRequest` payload. The handler first validates the Apple `identityToken` to extract the `serviceData` (which includes the user's Apple ID `sub` and `email`).
Apple's OAuth implementation is known to only include the `email` claim in the identity token during the *first* time a user authorizes the application. On subsequent logins, the identity token does not contain the `email` claim.
To handle this, the Rocket.Chat code attempts to use an `email` field provided directly in the client-controlled `loginRequest` if the validated token lacks one. Because the `loginRequest` is entirely controlled by the client, an attacker can supply an arbitrary email address (e.g., `victim@example.com`).
When `Accounts.updateOrCreateUserFromExternalService` is subsequently called, the `accounts_meld.js` configuration intercepts it. This code looks up an existing user by the (attacker-controlled) email address. If found, it links the attacker's Apple ID (`serviceData.id`) to the victim's account and logs the attacker in as the victim. This results in a full Account Takeover (ATO) with no user interaction required from the victim.
Proof of Concept:
**Steps to Reproduce**
1. The attacker logs into the target Rocket.Chat instance using their own Apple ID to authorize the app (or uses an Apple ID that has already authorized the app so that Apple omits the `email` claim in the token).
2. The attacker intercepts or crafts a DDP `login` request to the server, injecting the victim's email address:
```json
{
"msg": "method",
"method": "login",
"id": "1",
"params": [
{
"identityToken": "<ATTACKER_VALID_APPLE_TOKEN_WITHOUT_EMAIL>",
"email": "victim@example.com",
"fullName": {
"givenName": "Attacker",
"familyName": "Attacker"
}
}
]
}
```
3. The server validates the attacker's token, finds no email in it, and falls back to `"victim@example.com"`.
4. The server links the attacker's Apple ID to the victim's account.
5. The server returns a valid authentication token for the victim's account, granting the attacker full access.
Affected Code:
if (!serviceData.email && email) {
serviceData.email = email;
}
Fix this vulnerability. Only change what's necessary - don't modify unrelated code.
Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing. Any other reply is saved as a triage note.
There was a problem hiding this comment.
1 issue found across 1 file
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
Comments Outside Diff (1)
🟡 Medium: Insecure CSP frame-ancestors policy bypass via omitted Referer header
Location: apps/meteor/app/livechat/server/livechat.ts:41
The Livechat widget dynamically sets the frame-ancestors Content-Security-Policy (CSP) header based on the Referer HTTP header. If the Referer header is missing or stripped by the user's browser (e.g., due to 'Referrer-Policy' settings), the server removes the Content-Security-Policy header entirely. This allows the Livechat widget to be embedded in an iframe on any website, potentially enabling clickjacking or UI redressing attacks against the Livechat interface.
Steps to Reproduce
- Create an HTML file with the following content on any domain:
<!DOCTYPE html>
<html>
<head>
<title>Clickjacking PoC</title>
</head>
<body>
<h1>Clickjacking PoC</h1>
<!-- The referrerpolicy="no-referrer" attribute ensures the Referer header is not sent -->
<iframe src="http://<rocket_chat_url>/livechat" width="800" height="600" referrerpolicy="no-referrer"></iframe>
</body>
</html>- Open the HTML file in a browser.
- Observe that the Livechat widget is successfully loaded in the iframe, bypassing the
Livechat_AllowedDomainsListrestriction because the server removes the CSP header when theRefereris missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes (including videos or screenshots)
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
New Features
Bug Fixes