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
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
29 changes: 29 additions & 0 deletions apps/meteor/client/views/root/hooks/useLoginOtherClients.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
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;

const timeout = setTimeout(() => {
router.navigate('/home', { replace: true });
}, 0);

return () => clearTimeout(timeout);
}, [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.
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,41 @@
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;

const timeout = setTimeout(() => {
router.navigate('/home', { replace: true });
}, 100);

return () => clearTimeout(timeout);
}, [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;
}
}
31 changes: 28 additions & 3 deletions apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import type { DoneCallback, Profile } from 'passport';

import { verifyFunction } from './verifyFunction';
import { CustomOAuthStrategy } from '../../../app/custom-oauth/server/customOAuth';
import { settings } from '../../../app/settings/server';
import { oAuthRouter } from '../../configuration/configurePassport';

interface IOAuthRequest extends Request {
Expand All @@ -25,25 +26,49 @@ export const addPassportCustomOAuth = (serviceName: string, config: Partial<OAut
),
);

const siteUrl = settings.get<string>('Site_Url');
Comment thread
yash-rajpal marked this conversation as resolved.

oAuthRouter.get(
`/oauth/${serviceName}`,
passport.authenticate(serviceName, { 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();
});
} else {
next();
}
},
passport.authenticate(serviceName, { scope: config.scope, prompt: 'consent', failureRedirect: '/login', keepSessionInfo: true }),
);

oAuthRouter.get(
`/oauth/${serviceName}/callback`,
passport.authenticate(serviceName, { failureRedirect: '/login', failureFlash: true, failWithError: true }),
passport.authenticate(serviceName, { failureRedirect: '/login', failureFlash: true, failWithError: true, keepSessionInfo: true }),
async (req: IOAuthRequest, res: Response) => {
const oAuthUser = req.user as IUser;

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

const { loginClient } = req.session;

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

res.redirect(`/home?resumeToken=${stampedToken.token}`);
const redirectUrl = new URL(`/home`, siteUrl);

redirectUrl.searchParams.set('resumeToken', stampedToken.token);
redirectUrl.searchParams.set('userId', oAuthUser._id);

if (loginClient) {
redirectUrl.searchParams.set('loginClient', loginClient);
}

res.redirect(redirectUrl.toString());

req.session.destroy((err) => {
if (err) {
Expand Down
38 changes: 27 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,51 @@

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.
} else {
next();
}
Comment thread
yash-rajpal marked this conversation as 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}`);
const redirectUrl = new URL(`/home`, siteUrl);

redirectUrl.searchParams.set('resumeToken', stampedToken.token);
redirectUrl.searchParams.set('userId', oAuthUser._id);

if (loginClient) {
redirectUrl.searchParams.set('loginClient', loginClient);
}

res.redirect(redirectUrl.toString());

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;
}
1 change: 1 addition & 0 deletions packages/i18n/src/locales/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -6810,6 +6810,7 @@
"registration.component.form.usernameAlreadyInUse": "Username already in use",
"registration.component.form.usernameContainsInvalidChars": "Username contains invalid characters",
"registration.component.login": "Login",
"registration.component.login.onWeb": "Login on web",
"registration.component.login.incorrectPassword": "Incorrect password",
"registration.component.login.userNotFound": "User not found",
"registration.component.resetPassword": "Reset password",
Expand Down
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}>
{t('registration.component.login.onWeb')}
</Button>
Comment thread
coderabbitai[bot] marked this conversation as 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');

const redirectUrl = new URL(`/oauth/${service}`, window.location.origin);

if (loginClient) {
redirectUrl.searchParams.set('loginClient', loginClient);
}

window.location.href = redirectUrl.toString();
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