Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
5 changes: 5 additions & 0 deletions apps/meteor/client/lib/buildAuthDeeplinkURL.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const buildDeepLinkURL = (resumeToken: string, userId: string) => {
const url = new URL(window.location.href);
const { host } = url;
return `rocketchat://auth?host=http://${host}&token=${resumeToken}&userId=${userId}`;
Comment on lines +1 to +4

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

Preserve origin protocol and URL-encode deeplink query params.

This auth deeplink currently hardcodes http:// and interpolates raw query values. That can break HTTPS instances and corrupt token parsing for reserved characters.

Proposed fix
 export const buildDeepLinkURL = (resumeToken: string, userId: string) => {
-	const url = new URL(window.location.href);
-	const { host } = url;
-	return `rocketchat://auth?host=http://${host}&token=${resumeToken}&userId=${userId}`;
+	const { origin } = new URL(window.location.href);
+	const params = new URLSearchParams({
+		host: origin,
+		token: resumeToken,
+		userId,
+	});
+	return `rocketchat://auth?${params.toString()}`;
 };
📝 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
export const buildDeepLinkURL = (resumeToken: string, userId: string) => {
const url = new URL(window.location.href);
const { host } = url;
return `rocketchat://auth?host=http://${host}&token=${resumeToken}&userId=${userId}`;
export const buildDeepLinkURL = (resumeToken: string, userId: string) => {
const { origin } = new URL(window.location.href);
const params = new URLSearchParams({
host: origin,
token: resumeToken,
userId,
});
return `rocketchat://auth?${params.toString()}`;
};
🤖 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/buildAuthDeeplinkURL.ts` around lines 1 - 4, The
deeplink builder in buildDeepLinkURL currently hardcodes "http://" and injects
raw resumeToken and userId; change it to preserve the current page protocol (use
url.protocol + '//' + url.host) instead of hardcoding http, and URL-encode query
values (use encodeURIComponent on resumeToken and userId, and encodeURIComponent
on the host portion if needed) so the returned string uses the origin protocol
and safe, encoded query params.

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.

P1: Build the deeplink query with URLSearchParams and preserve the current protocol instead of hardcoding http://; the current interpolation can produce malformed auth URLs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/client/lib/buildAuthDeeplinkURL.ts, line 4:

<comment>Build the deeplink query with `URLSearchParams` and preserve the current protocol instead of hardcoding `http://`; the current interpolation can produce malformed auth URLs.</comment>

<file context>
@@ -0,0 +1,5 @@
+export const buildDeepLinkURL = (resumeToken: string, userId: string) => {
+	const url = new URL(window.location.href);
+	const { host } = url;
+	return `rocketchat://auth?host=http://${host}&token=${resumeToken}&userId=${userId}`;
+};
</file context>

};
3 changes: 2 additions & 1 deletion apps/meteor/client/lib/sdk/ddpSdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ export const getDdpSdk = (): DDPSDK => {
return instance;
};

const readStoredLoginToken = (): string | null => (typeof window !== 'undefined' ? window.localStorage.getItem('Meteor.loginToken') : null);
export const readStoredLoginToken = (): string | null =>
typeof window !== 'undefined' ? window.localStorage.getItem('Meteor.loginToken') : null;

let inflightLogin: Promise<void> | undefined;

Expand Down
4 changes: 4 additions & 0 deletions apps/meteor/client/views/root/AppLayout.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ import { useKeyboardShortcutsHotkey } from './hooks/useKeyboardShortcutsHotkey';
import { useLivechatEnterprise } from './hooks/useLivechatEnterprise';
import { useLoadMissedMessages } from './hooks/useLoadMissedMessages';
import { useLoadRoomForAllowedAnonymousRead } from './hooks/useLoadRoomForAllowedAnonymousRead';
import { useLoginOtherClients } from './hooks/useLoginOtherClients';
import { useLoginViaQuery } from './hooks/useLoginViaQuery';
import { useMessageLinkClicks } from './hooks/useMessageLinkClicks';
import { useNotificationPermission } from './hooks/useNotificationPermission';
import { useRedirectToSetupWizard } from './hooks/useRedirectToSetupWizard';
import { useSettingsOnLoadSiteUrl } from './hooks/useSettingsOnLoadSiteUrl';
import { useShareSessionWithOtherClients } from './hooks/useShareSessionWithOtherClients';
import { useStartupEvent } from './hooks/useStartupEvent';
import { appLayout } from '../../lib/appLayout';

Expand Down Expand Up @@ -70,6 +72,8 @@ const AppLayout = () => {
useAutoupdate();
useCodeHighlight();
useLoginViaQuery();
useLoginOtherClients();
useShareSessionWithOtherClients();
useLoadMissedMessages();
useDesktopFavicon();
useDesktopTitle();
Expand Down
27 changes: 27 additions & 0 deletions apps/meteor/client/views/root/hooks/useLoginOtherClients.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { useRouter } from '@rocket.chat/ui-contexts';
import { useEffect } from 'react';

import { buildDeepLinkURL } from '../../../lib/buildAuthDeeplinkURL';

export const useLoginOtherClients = () => {
const router = useRouter();

useEffect(() => {
const { resumeToken, loginClient, userId } = router.getSearchParameters();

if (!resumeToken || !userId) {
return;
}

if (loginClient !== 'desktop' && loginClient !== 'mobile') {
return;
}

const loginURL = buildDeepLinkURL(resumeToken, userId);
window.location.href = loginURL;

queueMicrotask(() => {
router.navigate('/home', { replace: true });
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Resolved
}, [router]);
};
7 changes: 6 additions & 1 deletion apps/meteor/client/views/root/hooks/useLoginViaQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,17 @@ export const useLoginViaQuery = () => {

useEffect(() => {
const handleLogin = async () => {
const { resumeToken } = router.getSearchParameters();
const { resumeToken, loginClient } = router.getSearchParameters();

if (!resumeToken) {
return;
}

//Case handled by useLoginOtherClients, we don't want to login here.
if (loginClient) {
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved
return;
}
Comment on lines +17 to +19

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

Guard only supported clients before skipping query-token login.

Line 17 currently bypasses loginWithToken for any loginClient value, while the alternate flow only handles desktop and mobile. That can drop valid token-login attempts when loginClient is present but unsupported.

Proposed fix
-			if (loginClient) {
+			if (loginClient === 'desktop' || loginClient === 'mobile') {
 				return;
 			}
📝 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 (loginClient) {
return;
}
if (loginClient === 'desktop' || loginClient === 'mobile') {
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/client/views/root/hooks/useLoginViaQuery.ts` around lines 17 -
19, The current guard in useLoginViaQuery.ts incorrectly returns whenever
loginClient is truthy, skipping token login for supported clients; change the
check so it only returns when loginClient is present and NOT one of the
supported values ('desktop' or 'mobile'). Update the conditional around
loginClient (the early-return near the top of the hook) to allow the
loginWithToken flow when loginClient is undefined or equals 'desktop'|'mobile'
and only skip when loginClient is a different/unsupported value.


try {
await loginWithToken(resumeToken);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import { useRouter, useUserId } from '@rocket.chat/ui-contexts';
import { useEffect } from 'react';

import { buildDeepLinkURL } from '../../../lib/buildAuthDeeplinkURL';
import { readStoredLoginToken } from '../../../lib/sdk/ddpSdk';

export const useShareSessionWithOtherClients = () => {
const router = useRouter();
const userId = useUserId();

useEffect(() => {
if (!userId) {
return;
}

const loginToken = readStoredLoginToken();

if (!loginToken) {
return;
}

const { resumeToken, loginClient } = router.getSearchParameters();

if (resumeToken) {
return;
}

if (loginClient !== 'desktop' && loginClient !== 'mobile') {
return;
}

const loginURL = buildDeepLinkURL(loginToken, userId);
window.location.href = loginURL;

queueMicrotask(() => {
router.navigate('/home', { replace: true });
});
}, [router, userId]);
};
7 changes: 7 additions & 0 deletions apps/meteor/definition/externals/express-session.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import 'express-session';

declare module 'express-session' {
interface SessionData {
loginClient?: string;
}
}
35 changes: 24 additions & 11 deletions apps/meteor/server/lib/oauth/configureOAuthServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@
import type { ICachedSettings } from '../../../app/settings/server/CachedSettings';
import { oAuthRouter } from '../../configuration/configurePassport';

interface IOAuthRequest extends Request {
user?: IUser;
}

export const configureOAuthServices = (oauthServiceConfig: OAuthServiceConfig[], settings: ICachedSettings) => {
oauthServiceConfig.forEach((config) => {
const Strategy = config.strategy;
Expand All @@ -36,7 +32,6 @@
const profileWithRaw = profile as Profile & { _json?: Record<string, unknown>; _raw?: string };
const { _json, _raw, ...restProfile } = profileWithRaw;

// eslint-disable-next-line @typescript-eslint/await-thenable
const user = await Accounts.updateOrCreateUserFromExternalService(
config.provider,
{
Expand Down Expand Up @@ -67,30 +62,48 @@

oAuthRouter.get(
`/oauth/${config.provider}`,
passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login' }),
(req, _res, next) => {
const { loginClient } = req.query;
if (loginClient === 'mobile' || loginClient === 'desktop') {
req.session.loginClient = loginClient;
req.session.save(() => {
next();
});
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved
} else {
next();
}
Comment thread
yash-rajpal marked this conversation as resolved.
Resolved
},
passport.authenticate(config.provider, { scope: config.scope, prompt: 'consent', failureRedirect: '/login', keepSessionInfo: true }),
);
oAuthRouter.get(
`/oauth/${config.provider}/callback`,
passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true }),
async (req: IOAuthRequest, res: Response) => {
passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true, keepSessionInfo: true }),
async (req: Request, res: Response) => {
const oAuthUser = req.user as IUser;

if (!oAuthUser) {
// return res.redirect('/login');
return res.redirect('/noOauthUser');
return res.redirect('/login');
}

const { loginClient } = req.session;

const stampedToken = Accounts._generateStampedLoginToken();
await Accounts._insertLoginToken(oAuthUser._id, stampedToken);

res.redirect(`/home?resumeToken=${stampedToken.token}`);
let redirectUrl = `/home?resumeToken=${stampedToken.token}&userId=${oAuthUser._id}`;

if (loginClient) {
redirectUrl += `&loginClient=${loginClient}`;
}

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

Only attach resumeToken to deeplink flows, and encode redirect params.

The callback currently includes resumeToken/userId in every OAuth redirect and concatenates values directly. That leaks sensitive auth data on normal web logins and risks broken parsing for reserved characters.

Proposed fix
-				let redirectUrl = `/home?resumeToken=${stampedToken.token}&userId=${oAuthUser._id}`;
-
-				if (loginClient) {
-					redirectUrl += `&loginClient=${loginClient}`;
-				}
+				let redirectUrl = '/home';
+
+				if (loginClient === 'mobile' || loginClient === 'desktop') {
+					const params = new URLSearchParams({
+						resumeToken: stampedToken.token,
+						userId: oAuthUser._id,
+						loginClient,
+					});
+					redirectUrl = `/home?${params.toString()}`;
+				}
📝 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
let redirectUrl = `/home?resumeToken=${stampedToken.token}&userId=${oAuthUser._id}`;
if (loginClient) {
redirectUrl += `&loginClient=${loginClient}`;
}
let redirectUrl = '/home';
if (loginClient === 'mobile' || loginClient === 'desktop') {
const params = new URLSearchParams({
resumeToken: stampedToken.token,
userId: oAuthUser._id,
loginClient,
});
redirectUrl = `/home?${params.toString()}`;
}
🤖 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/lib/oauth/configureOAuthServices.ts` around lines 93 - 97,
The redirect building currently always attaches resumeToken and userId and
concatenates raw values; change the logic in configureOAuthServices.ts around
the redirectUrl variable so resumeToken (stampedToken.token) and userId
(oAuthUser._id) are only appended for deeplink flows (use the existing
loginClient truthy check as the deeplink indicator), and ensure every query
value you add is wrapped with encodeURIComponent (e.g., encodeStamped =
encodeURIComponent(stampedToken.token), encodedUserId =
encodeURIComponent(oAuthUser._id), encodedLoginClient =
encodeURIComponent(loginClient)) and concatenate using those encoded values so
reserved characters are safe.

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.

P2: resumeToken and userId are included in the redirect URL for all OAuth completions, including normal web logins where loginClient is absent. For the regular web flow, only resumeToken is needed (consumed by useLoginViaQuery). Including both params unconditionally leaks sensitive auth data in browser history and server logs. Limit userId (and ideally all deeplink params) to only the loginClient flow.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/server/lib/oauth/configureOAuthServices.ts, line 93:

<comment>`resumeToken` and `userId` are included in the redirect URL for all OAuth completions, including normal web logins where `loginClient` is absent. For the regular web flow, only `resumeToken` is needed (consumed by `useLoginViaQuery`). Including both params unconditionally leaks sensitive auth data in browser history and server logs. Limit `userId` (and ideally all deeplink params) to only the `loginClient` flow.</comment>

<file context>
@@ -67,23 +62,41 @@ export const configureOAuthServices = (oauthServiceConfig: OAuthServiceConfig[],
 				await Accounts._insertLoginToken(oAuthUser._id, stampedToken);
 
-				res.redirect(`/home?resumeToken=${stampedToken.token}`);
+				let redirectUrl = `/home?resumeToken=${stampedToken.token}&userId=${oAuthUser._id}`;
+
+				if (loginClient) {
</file context>
Suggested change
let redirectUrl = `/home?resumeToken=${stampedToken.token}&userId=${oAuthUser._id}`;
if (loginClient) {
redirectUrl += `&loginClient=${loginClient}`;
}
let redirectUrl = '/home';
if (loginClient === 'mobile' || loginClient === 'desktop') {
const params = new URLSearchParams({
resumeToken: stampedToken.token,
userId: oAuthUser._id,
loginClient,
});
redirectUrl = `/home?${params.toString()}`;
} else {
redirectUrl = `/home?resumeToken=${encodeURIComponent(stampedToken.token)}`;
}


res.redirect(redirectUrl);

req.session.destroy((err) => {
if (err) {
console.error('Error destroying session', err);
}
});
},

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
);
});
};
1 change: 1 addition & 0 deletions packages/desktop-api/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,4 +63,5 @@ export interface IRocketChatDesktop {
setUserToken: (token: string, userId: string) => void;
openDocumentViewer: (url: string, format: string, options: any) => void;
reloadServer: () => void;
openInBrowser: (url: string) => void;
}
7 changes: 7 additions & 0 deletions packages/web-ui-registration/global.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import type { IRocketChatDesktop } from '@rocket.chat/desktop-api';

declare global {
interface Window {
RocketChatDesktop?: IRocketChatDesktop;
}
}
29 changes: 23 additions & 6 deletions packages/web-ui-registration/src/LoginServices.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ButtonGroup, Divider } from '@rocket.chat/fuselage';
import { Button, ButtonGroup, Divider } from '@rocket.chat/fuselage';
import { useLoginServices, useSetting } from '@rocket.chat/ui-contexts';
import type { Dispatch, ReactElement, SetStateAction } from 'react';
import { useTranslation } from 'react-i18next';
Expand All @@ -21,18 +21,35 @@ const LoginServices = ({
return null;
}

const isDesktopApp = !!window.RocketChatDesktop?.openInBrowser;

const handleLoginOnWeb = () => {
if (!isDesktopApp) {
return;
}

window.RocketChatDesktop?.openInBrowser(`${window.location.href}?loginClient=desktop`);

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.

P1: Build the deeplink URL with URL/searchParams instead of appending ?loginClient=desktop to window.location.href.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/web-ui-registration/src/LoginServices.tsx, line 31:

<comment>Build the deeplink URL with `URL`/`searchParams` instead of appending `?loginClient=desktop` to `window.location.href`.</comment>

<file context>
@@ -21,18 +21,35 @@ const LoginServices = ({
+			return;
+		}
+
+		window.RocketChatDesktop?.openInBrowser(`${window.location.href}?loginClient=desktop`);
+	};
+
</file context>
Suggested change
window.RocketChatDesktop?.openInBrowser(`${window.location.href}?loginClient=desktop`);
const url = new URL(window.location.href);
url.searchParams.set('loginClient', 'desktop');
window.RocketChatDesktop?.openInBrowser(url.toString());

};
Comment on lines +26 to +32

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

Build the desktop redirect URL via URL/searchParams, not string concat.

Line 31 can generate invalid URLs (...?a=1?loginClient=desktop or hash placement issues) because it appends a raw ? to the full href.

Proposed fix
 	const handleLoginOnWeb = () => {
 		if (!isDesktopApp) {
 			return;
 		}

-		window.RocketChatDesktop?.openInBrowser(`${window.location.href}?loginClient=desktop`);
+		const url = new URL(window.location.href);
+		url.searchParams.set('loginClient', 'desktop');
+		window.RocketChatDesktop?.openInBrowser(url.toString());
 	};
🤖 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/LoginServices.tsx` around lines 26 - 32, The
redirect URL is being built by string concatenation in handleLoginOnWeb which
can produce invalid query/hash placement (e.g., "...?a=1?loginClient=desktop");
instead construct a URL object from window.location.href, set
searchParams.set('loginClient','desktop'), then pass url.toString() to
window.RocketChatDesktop.openInBrowser; update handleLoginOnWeb to use the
URL/searchParams API so existing queries and hashes are preserved correctly.


return (
<>
{showFormLogin && (
<Divider mb={24} p={0}>
{t('registration.component.form.divider')}
</Divider>
)}
<ButtonGroup vertical stretch small>
{services.map((service) => (
<LoginServicesButton disabled={disabled} key={service.service} {...service} setError={setError} />
))}
</ButtonGroup>
{!isDesktopApp && (
<ButtonGroup vertical stretch small>
{services.map((service) => (
<LoginServicesButton disabled={disabled} key={service.service} {...service} setError={setError} />
))}
</ButtonGroup>
)}
{isDesktopApp && (
<Button width='100%' primary onClick={handleLoginOnWeb}>
Login on web
</Button>
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Resolved
)}
</>
);
};
Expand Down
12 changes: 11 additions & 1 deletion packages/web-ui-registration/src/LoginServicesButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,17 @@ const LoginServicesButton = <T extends LoginService>({

const handleOnClick = useCallback(() => {
if (!isLegacyOAuthEnabled) {
window.location.href = `/oauth/${service}`;
const url = new URL(window.location.href);
const queryParams = url.searchParams;
const loginClient = queryParams.get('loginClient');

let redirectUrl = `/oauth/${service}`;

if (loginClient) {
redirectUrl += `?loginClient=${loginClient}`;
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Resolved
}

window.location.href = redirectUrl;
return;
}

Expand Down
2 changes: 1 addition & 1 deletion packages/web-ui-registration/tsconfig.build.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"extends": "./tsconfig.json",
"include": ["./src/**/*"],
"include": ["./src/**/*", "./global.d.ts"],
"exclude": ["./src/**/*.spec.ts", "./src/**/*.stories.tsx"]
}
2 changes: 1 addition & 1 deletion packages/web-ui-registration/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
"rootDirs": ["./src","./.storybook"],
"outDir": "./dist"
},
"include": ["./src", "./.storybook"],
"include": ["./src", "./.storybook", "./global.d.ts"],
}
Loading