Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type { ContextType, ReactElement, ReactNode } from 'react';
import { useMemo } from 'react';

import { useLDAPAndCrowdCollisionWarning } from './hooks/useLDAPAndCrowdCollisionWarning';
import { capitalize as capitalizeService } from '../../../lib/utils/stringUtils';
import { useReactiveValue } from '../../hooks/useReactiveValue';
import { loginServices } from '../../lib/loginServices';

Expand Down Expand Up @@ -90,6 +91,16 @@ const AuthenticationProvider = ({ children }: AuthenticationProviderProps): Reac
});
});
},
loginWithCustomOauth: (service: string, options: { redirectUrl: string }, callback) => {
const methodName = `loginWith${capitalizeService(service, true)}`;
const method = (Meteor as any)[methodName] as
| ((options: { redirectUrl: string }, cb?: (response: unknown) => void) => void)
| undefined;
if (!method) {
return;
}
method.call(Meteor, options, callback);
},
Comment on lines +94 to +103

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

loginWithCustomOauth silently no-ops on missing method; callback never fires.

When Meteor[methodName] is undefined the function returns without invoking callback. Callers that await the callback (e.g., the iframe call-custom-oauth-login command path) won't receive any signal, so they can't surface or recover from the failure. The previous direct-Meteor call site would have thrown a TypeError instead, which made the failure observable. Consider invoking callback with an error so the caller can react.

🛡️ Proposed fix
 			loginWithCustomOauth: (service: string, options: { redirectUrl: string }, callback) => {
 				const methodName = `loginWith${capitalizeService(service, true)}`;
 				const method = (Meteor as any)[methodName] as
 					| ((options: { redirectUrl: string }, cb?: (response: unknown) => void) => void)
 					| undefined;
 				if (!method) {
-					return;
+					callback?.(new Error(`Login method ${methodName} not found`));
+					return;
 				}
 				method.call(Meteor, options, callback);
 			},
🤖 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/providers/AuthenticationProvider/AuthenticationProvider.tsx`
around lines 94 - 103, The loginWithCustomOauth helper currently returns early
when the dynamically computed Meteor method (methodName) is missing, leaving
callers waiting; update the loginWithCustomOauth implementation to detect when
method is undefined and, instead of silent return, invoke the provided callback
(if any) with an Error describing the missing method (e.g., new Error(`Missing
OAuth method ${methodName}`)) and then return; keep using the same methodName,
capitalizeService, and Meteor symbols and ensure the callback is only called if
it was passed to avoid changing semantics for callers that don’t supply a
callback.

loginWithIframe: (token: string, callback) =>
new Promise<void>((resolve, reject) => {
callLoginMethod({ iframe: true, token }, (error) => {
Expand All @@ -112,6 +123,19 @@ const AuthenticationProvider = ({ children }: AuthenticationProviderProps): Reac
resolve();
});
}),
getLoginToken: () => Accounts.storageLocation.getItem(Accounts.LOGIN_TOKEN_KEY) ?? null,
wipeLocalAuth: () => {
try {
Accounts._unstoreLoginToken();
} catch {
// ignore
}
try {
(Meteor.connection as unknown as { setUserId: (uid: string | null) => void }).setUserId(null);
} catch {
// ignore
}
},
unstoreLoginToken: (callback) => {
const { _unstoreLoginToken } = Accounts;
Accounts._unstoreLoginToken = function (...args) {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import type { IOAuthApps, IUser } from '@rocket.chat/core-typings';
import { Box, Button, ButtonGroup } from '@rocket.chat/fuselage';
import { Form } from '@rocket.chat/layout';
import { useLogout, useRoute } from '@rocket.chat/ui-contexts';
import { Accounts } from 'meteor/accounts-base';
import { useEffect, useId, useMemo, useRef } from 'react';
import { useLoginToken, useLogout, useRoute } from '@rocket.chat/ui-contexts';
import { useEffect, useId, useRef } from 'react';
import { Trans, useTranslation } from 'react-i18next';

import CurrentUserDisplay from './CurrentUserDisplay';
Expand All @@ -16,7 +15,7 @@ type AuthorizationFormPageProps = {
};

const AuthorizationFormPage = ({ oauthApp, redirectUri, user }: AuthorizationFormPageProps) => {
const token = useMemo(() => Accounts.storageLocation.getItem(Accounts.LOGIN_TOKEN_KEY) ?? undefined, []);
const token = useLoginToken() ?? undefined;

const formLabelId = useId();

Expand Down
22 changes: 6 additions & 16 deletions apps/meteor/client/views/root/hooks/loggedIn/useForceLogout.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { useStream, useSessionDispatch } from '@rocket.chat/ui-contexts';
import { Accounts } from 'meteor/accounts-base';
import { Meteor } from 'meteor/meteor';
import { useSessionDispatch, useStream, useWipeLocalAuth } from '@rocket.chat/ui-contexts';
import { useEffect } from 'react';

import { isSdkTransportEnabled } from '../../../../lib/sdk/sdkTransportEnabled';
Expand All @@ -10,6 +8,7 @@ const sdkTransportEnabled = isSdkTransportEnabled();
export const useForceLogout = (userId: string) => {
const getNotifyUserStream = useStream('notify-user');
const setForceLogout = useSessionDispatch('forceLogout');
const wipeLocalAuth = useWipeLocalAuth();

useEffect(() => {
setForceLogout(false);
Expand All @@ -27,20 +26,11 @@ export const useForceLogout = (userId: string) => {
// With the SDK socket as the transport, that chain no longer fires
// reliably: DDPSDK auto-retries loginWithToken on every `connected`
// and swallows the rejection with `void`, so the navbar stays on
// Home with stale credentials. Wipe Meteor's stored login token +
// userId here so the router falls back to /login.
try {
Accounts._unstoreLoginToken();
} catch {
// ignore
}
try {
(Meteor.connection as unknown as { setUserId: (uid: string | null) => void }).setUserId(null);
} catch {
// ignore
}
// Home with stale credentials. Wipe the stored login token + userId
// here so the router falls back to /login.
wipeLocalAuth();
});

return unsubscribe;
}, [getNotifyUserStream, setForceLogout, userId]);
}, [getNotifyUserStream, setForceLogout, userId, wipeLocalAuth]);
};
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { useIsLoggingIn } from '@rocket.chat/ui-contexts';
import { Accounts } from 'meteor/accounts-base';
import { useIsLoggingIn, useLoginToken } from '@rocket.chat/ui-contexts';
import { useEffect } from 'react';

export const useStoreCookiesOnLogin = (userId: string) => {
const isLoggingIn = useIsLoggingIn();
const loginToken = useLoginToken();

useEffect(() => {
// Check for isLoggingIn to be reactive and ensure it will process only after login finishes
Expand All @@ -12,7 +12,7 @@ export const useStoreCookiesOnLogin = (userId: string) => {
const secure = location.protocol === 'https:' ? '; secure' : '';

document.cookie = `rc_uid=${encodeURI(userId)}; path=/${secure}`;
document.cookie = `rc_token=${encodeURI(Accounts._storedLoginToken() as string)}; path=/${secure}`;
document.cookie = `rc_token=${encodeURI(loginToken ?? '')}; path=/${secure}`;
}
}, [isLoggingIn, userId]);
}, [isLoggingIn, loginToken, userId]);
};
165 changes: 80 additions & 85 deletions apps/meteor/client/views/root/hooks/useIframeCommands.ts
Original file line number Diff line number Diff line change
@@ -1,105 +1,100 @@
import type { UserStatus, IUser } from '@rocket.chat/core-typings';
import type { UserStatus } from '@rocket.chat/core-typings';
import { escapeRegExp } from '@rocket.chat/string-helpers';
import { type LocationPathname, useSetting } from '@rocket.chat/ui-contexts';
import { Meteor } from 'meteor/meteor';
import { useEffect } from 'react';
import { type LocationPathname, UserContext, useLoginWithCustomOauth, useLoginWithToken, useSetting } from '@rocket.chat/ui-contexts';
import { useContext, useEffect } from 'react';

import { AccountBox } from '../../../../app/ui-utils/client/lib/AccountBox';
import { sdk } from '../../../../app/utils/client/lib/SDKClient';
import { capitalize, ltrim, rtrim } from '../../../../lib/utils/stringUtils';
import { baseURI } from '../../../lib/baseURI';
import { loginServices } from '../../../lib/loginServices';
import { settings } from '../../../lib/settings';
import { getUser } from '../../../lib/user';
import { router } from '../../../providers/RouterProvider';

const commands = {
'go'(data: { path: string }) {
if (typeof data.path !== 'string' || data.path.trim().length === 0) {
return console.error('`path` not defined');
}
const newUrl = new URL(`${rtrim(baseURI, '/')}/${ltrim(data.path, '/')}`);

const newParams = Array.from(newUrl.searchParams.entries()).reduce(
(ret, [key, value]) => {
ret[key] = value;
return ret;
},
{} as Record<string, string>,
);

const newPath = newUrl.pathname.replace(
new RegExp(`^${escapeRegExp(__meteor_runtime_config__.ROOT_URL_PATH_PREFIX)}`),
'',
) as LocationPathname;
router.navigate({
pathname: newPath,
search: { ...router.getSearchParameters(), ...newParams },
});
},

'set-user-status'(data: { status: UserStatus }) {
AccountBox.setStatus(data.status);
},

'call-custom-oauth-login'(data: { service: string; redirectUrl?: string | null }, event: MessageEvent) {
const customOAuthCallback = (response: unknown) => {
event.source?.postMessage(
{
event: 'custom-oauth-callback',
response,
},
{ targetOrigin: event.origin },
);
};
export const useIframeCommands = () => {
const iframeReceiveEnabled = useSetting('Iframe_Integration_receive_enable');
const iframeReceiveOrigin = useSetting('Iframe_Integration_receive_origin', '*');
const loginWithToken = useLoginWithToken();
const loginWithCustomOauth = useLoginWithCustomOauth();
const { logout } = useContext(UserContext);

const siteUrl = `${settings.peek('Site_Url') ?? ''}/`;
if (typeof data.redirectUrl !== 'string' || !data.redirectUrl.startsWith(siteUrl)) {
data.redirectUrl = null;
useEffect(() => {
if (!iframeReceiveEnabled) {
return;
}

if (typeof data.service === 'string' && window.ServiceConfiguration) {
const customOauth = loginServices.getLoginService(data.service);

if (customOauth) {
const customLoginWith = (Meteor as any)[`loginWith${capitalize(customOauth.service, true)}`];
const customRedirectUri = data.redirectUrl || siteUrl;
customLoginWith.call(Meteor, { redirectUrl: customRedirectUri }, customOAuthCallback);
}
}
},
const commands = {
'go'(data: { path: string }) {
if (typeof data.path !== 'string' || data.path.trim().length === 0) {
return console.error('`path` not defined');
}
const newUrl = new URL(`${rtrim(baseURI, '/')}/${ltrim(data.path, '/')}`);

const newParams = Array.from(newUrl.searchParams.entries()).reduce(
(ret, [key, value]) => {
ret[key] = value;
return ret;
},
{} as Record<string, string>,
);

const newPath = newUrl.pathname.replace(
new RegExp(`^${escapeRegExp(__meteor_runtime_config__.ROOT_URL_PATH_PREFIX)}`),
'',
) as LocationPathname;
router.navigate({
pathname: newPath,
search: { ...router.getSearchParameters(), ...newParams },
});
},

'login-with-token'(data: { token: string }) {
if (typeof data.token === 'string') {
Meteor.loginWithToken(data.token, () => {
console.log('Iframe command [login-with-token]: result', data);
});
}
},
'set-user-status'(data: { status: UserStatus }) {
AccountBox.setStatus(data.status);
},

async 'logout'() {
const user = getUser();
Meteor.logout(() => {
if (!user) return;
'call-custom-oauth-login'(data: { service: string; redirectUrl?: string | null }, event: MessageEvent) {
const customOAuthCallback = (response: unknown) => {
event.source?.postMessage(
{
event: 'custom-oauth-callback',
response,
},
{ targetOrigin: event.origin },
);
};

const siteUrl = `${settings.peek('Site_Url') ?? ''}/`;
if (typeof data.redirectUrl !== 'string' || !data.redirectUrl.startsWith(siteUrl)) {
data.redirectUrl = null;
}
Comment on lines +65 to +68

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

redirectUrl allow-list check is fragile when Site_Url has a trailing slash or is empty.

Two edge cases worth tightening:

  • If Site_Url already ends with /, siteUrl becomes …//, and a legitimate redirectUrl like https://host/path fails the startsWith check and gets silently nulled.
  • If Site_Url is empty, siteUrl becomes /, so any string beginning with / (e.g. a relative path) passes the check. Combined with the data.redirectUrl || siteUrl fallback below, the effective customRedirectUri then defaults to /, which is unlikely what you want for an OAuth redirect.

Normalizing the trailing slash (e.g. via rtrim) and rejecting an empty Site_Url would make the validation more robust.

🛡️ Suggested hardening
-			const siteUrl = `${settings.peek('Site_Url') ?? ''}/`;
-			if (typeof data.redirectUrl !== 'string' || !data.redirectUrl.startsWith(siteUrl)) {
-				data.redirectUrl = null;
-			}
+			const rawSiteUrl = settings.peek('Site_Url');
+			const siteUrl = typeof rawSiteUrl === 'string' && rawSiteUrl.length > 0 ? `${rtrim(rawSiteUrl, '/')}/` : '';
+			if (!siteUrl || typeof data.redirectUrl !== 'string' || !data.redirectUrl.startsWith(siteUrl)) {
+				data.redirectUrl = null;
+			}
🤖 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/views/root/hooks/useIframeCommands.ts` around lines 71 -
74, Normalize and validate Site_Url before the allow-list check: read the raw
value from settings.peek('Site_Url'), trim any trailing slashes (e.g. remove
trailing "/" characters) into a non-empty siteBase; if siteBase is empty set
data.redirectUrl = null; otherwise only accept data.redirectUrl when it's a
string and either equals siteBase or startsWith siteBase + '/' (so legitimate
URLs like https://host/path still pass even if the configured Site_Url had a
trailing slash). Update the logic around the siteUrl variable and the check that
currently uses siteUrl and the subsequent fallback (data.redirectUrl || siteUrl)
to use this normalized siteBase and ensure empty Site_Url cannot become "/" by
default.


if (typeof data.service === 'string' && window.ServiceConfiguration) {
const customOauth = loginServices.getLoginService(data.service);

if (customOauth) {
const customRedirectUri = data.redirectUrl || siteUrl;
loginWithCustomOauth(capitalize(customOauth.service, true), { redirectUrl: customRedirectUri }, customOAuthCallback);
}
}
},

sdk.call('logoutCleanUp', user as unknown as IUser);
return router.navigate('/home');
});
},
} as const;
'login-with-token'(data: { token: string }) {
if (typeof data.token === 'string') {
void loginWithToken(data.token, () => {
console.log('Iframe command [login-with-token]: result', data);
});
Comment on lines +80 to +84

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Do not log raw login token payloads.

Line 83 logs data, which includes the login token. This leaks sensitive auth material into browser logs.

Safer logging change
 			'login-with-token'(data: { token: string }) {
 				if (typeof data.token === 'string') {
 					void loginWithToken(data.token, () => {
-						console.log('Iframe command [login-with-token]: result', data);
+						console.log('Iframe command [login-with-token]: completed');
 					});
 				}
 			},
📝 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
'login-with-token'(data: { token: string }) {
if (typeof data.token === 'string') {
void loginWithToken(data.token, () => {
console.log('Iframe command [login-with-token]: result', data);
});
'login-with-token'(data: { token: string }) {
if (typeof data.token === 'string') {
void loginWithToken(data.token, () => {
console.log('Iframe command [login-with-token]: completed');
});
}
},
🤖 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/views/root/hooks/useIframeCommands.ts` around lines 80 -
84, The 'login-with-token' iframe command handler in useIframeCommands.ts is
currently logging the entire data object (including the sensitive token); remove
the console.log(data) to avoid leaking the token, and instead log a
non-sensitive success/failure message or a sanitized indicator (e.g., "Iframe
command [login-with-token]: success" or include only masked/token-present
boolean) inside the callback passed to loginWithToken; update the handler that
receives data: { token: string } accordingly and ensure no other code in the
same function (login-with-token) prints the raw token.

}
},

type CommandMessage<TCommandName extends keyof typeof commands = keyof typeof commands> = {
externalCommand: TCommandName;
} & Parameters<(typeof commands)[TCommandName]>[0];
'logout'() {
void logout();
router.navigate('/home');
},
} as const;

export const useIframeCommands = () => {
const iframeReceiveEnabled = useSetting('Iframe_Integration_receive_enable');
const iframeReceiveOrigin = useSetting('Iframe_Integration_receive_origin', '*');
type CommandMessage<TCommandName extends keyof typeof commands = keyof typeof commands> = {
externalCommand: TCommandName;
} & Parameters<(typeof commands)[TCommandName]>[0];

useEffect(() => {
if (!iframeReceiveEnabled) {
return;
}
const messageListener = (event: MessageEvent<CommandMessage>) => {
if (typeof event.data !== 'object' || typeof event.data.externalCommand !== 'string') {
return;
Expand All @@ -124,5 +119,5 @@ export const useIframeCommands = () => {
return () => {
window.removeEventListener('message', messageListener);
};
}, [iframeReceiveEnabled, iframeReceiveOrigin]);
}, [iframeReceiveEnabled, iframeReceiveOrigin, loginWithToken, loginWithCustomOauth, logout]);
};
5 changes: 4 additions & 1 deletion packages/mock-providers/src/MockedAppRootBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -250,13 +250,16 @@ export class MockedAppRootBuilder {
loginWithPassword: () => Promise.resolve(),
loginWithToken: () => Promise.resolve(),
loginWithService: () => () => Promise.resolve(true),
loginWithCustomOauth: () => undefined,
loginWithIframe: async () => Promise.reject('loginWithIframe not implemented'),
loginWithTokenRoute: async () => Promise.reject('loginWithTokenRoute not implemented'),
queryLoginServices: {
getCurrentValue: () => this.authServices,
subscribe: () => () => undefined,
},
unstoreLoginToken: () => async () => Promise.reject('unstoreLoginToken not implemented'),
getLoginToken: () => null,
unstoreLoginToken: () => () => undefined,
wipeLocalAuth: () => undefined,
};

private events = new Emitter<MockedAppRootEvents>();
Expand Down
12 changes: 12 additions & 0 deletions packages/ui-contexts/src/AuthenticationContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,12 @@ export type AuthenticationContextValue = {
loginWithPassword: (user: string | { username: string } | { email: string } | { id: string }, password: string) => Promise<void>;
loginWithToken: (user: string, callback?: (error: Error | null | undefined) => void) => Promise<void>;
loginWithService<T extends LoginServiceConfiguration>(service: T): () => Promise<true>;
loginWithCustomOauth: (service: string, options: { redirectUrl: string }, callback?: (response: unknown) => void) => void;
loginWithIframe: (token: string, callback?: (error: Error | null | undefined) => void) => Promise<void>;
loginWithTokenRoute: (token: string, callback?: (error: Error | null | undefined) => void) => Promise<void>;
getLoginToken: () => string | null;
unstoreLoginToken: (callback: () => void) => () => void;
wipeLocalAuth: () => void;
queryLoginServices: {
getCurrentValue: () => LoginService[];
subscribe: (onStoreChange: () => void) => () => void;
Expand All @@ -23,13 +26,22 @@ export type AuthenticationContextValue = {
export const AuthenticationContext = createContext<AuthenticationContextValue>({
isLoggingIn: false,
loginWithService: () => () => Promise.reject(new Error('loginWithService not implemented')),
loginWithCustomOauth: () => {
throw new Error('loginWithCustomOauth not implemented');
},
loginWithPassword: async () => Promise.reject(new Error('loginWithPassword not implemented')),
loginWithToken: async () => Promise.reject(new Error('loginWithToken not implemented')),
loginWithIframe: async () => Promise.reject(new Error('loginWithIframe not implemented')),
loginWithTokenRoute: async () => Promise.reject(new Error('loginWithTokenRoute not implemented')),
getLoginToken: () => {
throw new Error('getLoginToken not implemented');
},
unstoreLoginToken: () => {
throw new Error('unstoreLoginToken not implemented');
},
wipeLocalAuth: () => {
throw new Error('wipeLocalAuth not implemented');
},
queryLoginServices: {
getCurrentValue: () => [],
subscribe: (_: () => void) => {
Expand Down
5 changes: 5 additions & 0 deletions packages/ui-contexts/src/hooks/useLoginToken.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { useContext } from 'react';

import { AuthenticationContext } from '../AuthenticationContext';

export const useLoginToken = (): string | null => useContext(AuthenticationContext).getLoginToken();
5 changes: 5 additions & 0 deletions packages/ui-contexts/src/hooks/useLoginWithCustomOauth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { useContext } from 'react';

import { AuthenticationContext } from '../AuthenticationContext';

export const useLoginWithCustomOauth = () => useContext(AuthenticationContext).loginWithCustomOauth;
5 changes: 5 additions & 0 deletions packages/ui-contexts/src/hooks/useWipeLocalAuth.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import { useContext } from 'react';

import { AuthenticationContext } from '../AuthenticationContext';

export const useWipeLocalAuth = (): (() => void) => useContext(AuthenticationContext).wipeLocalAuth;
Loading
Loading