Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
22 changes: 19 additions & 3 deletions apps/meteor/app/dolphin/server/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import _ from 'underscore';
import { callbacks } from '../../../server/lib/callbacks';
import { beforeCreateUserCallback } from '../../../server/lib/callbacks/beforeCreateUserCallback';
import { addPassportCustomOAuth } from '../../../server/lib/oauth/addPassportCustomOAuth';
import { CustomOAuth } from '../../custom-oauth/server/custom_oauth_server';
import { settings } from '../../settings/server';

const config: Partial<OAuthConfiguration> = {
Expand All @@ -22,6 +23,8 @@ const config: Partial<OAuthConfiguration> = {
accessTokenParam: 'access_token',
};

const Dolphin = new CustomOAuth('dolphin', config);

function DolphinOnCreateUser(options: any, user?: IUser) {
// TODO: callbacks Fix this
if (user?.services?.dolphin?.NickName) {
Expand All @@ -46,14 +49,27 @@ const configureDolphinOAuth = () => {
return;
}

addPassportCustomOAuth('dolphin', { ...config, serverURL, clientId, clientSecret });
const completeConfig = { ...config, serverURL, clientId, clientSecret };

if (settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport') {
addPassportCustomOAuth('dolphin', completeConfig);
return;
}

Dolphin.configure(completeConfig);
};

Meteor.startup(async () => {
const updateConfig = () => _.debounce(configureDolphinOAuth, 300);
const updateConfig = _.debounce(configureDolphinOAuth, 300);

settings.watchMultiple(
['Accounts_OAuth_Dolphin', 'Accounts_OAuth_Dolphin_URL', 'Accounts_OAuth_Dolphin_id', 'Accounts_OAuth_Dolphin_secret'],
[
'Accounts_OAuth_Dolphin',
'Accounts_OAuth_Dolphin_URL',
'Accounts_OAuth_Dolphin_id',
'Accounts_OAuth_Dolphin_secret',
'Accounts_OAuth_Flow_Engine',
],
updateConfig,
);

Expand Down
16 changes: 14 additions & 2 deletions apps/meteor/app/drupal/server/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,11 @@ import passport from 'passport';
import _ from 'underscore';

import { addPassportCustomOAuth } from '../../../server/lib/oauth/addPassportCustomOAuth';
import { CustomOAuth } from '../../custom-oauth/server/custom_oauth_server';
import { settings } from '../../settings/server';

const config: Partial<OAuthConfiguration> = {
serverURL: '',
identityPath: '/oauth2/UserInfo',
authorizePath: '/oauth2/authorize',
tokenPath: '/oauth2/token',
Expand All @@ -21,6 +23,8 @@ const config: Partial<OAuthConfiguration> = {
accessTokenParam: 'access_token',
};

const Drupal = new CustomOAuth('drupal', config);

const configureDrupalOAuth = () => {
passport.unuse('drupal');
const enabled = settings.get<boolean>('Accounts_OAuth_Drupal');
Expand All @@ -36,14 +40,22 @@ const configureDrupalOAuth = () => {
return;
}

addPassportCustomOAuth('drupal', { ...config, serverURL, clientId, clientSecret });
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';
const completeConfig = { ...config, serverURL, clientId, clientSecret };

if (isPassportFlowEnabled) {
addPassportCustomOAuth('drupal', completeConfig);
return;
}

Drupal.configure(completeConfig);
};

Meteor.startup(() => {
const updateConfig = _.debounce(configureDrupalOAuth, 300);

settings.watchMultiple(
['Accounts_OAuth_Drupal', 'API_Drupal_URL', 'Accounts_OAuth_Drupal_id', 'Accounts_OAuth_Drupal_secret'],
['Accounts_OAuth_Drupal', 'API_Drupal_URL', 'Accounts_OAuth_Drupal_id', 'Accounts_OAuth_Drupal_secret', 'Accounts_OAuth_Flow_Engine'],
updateConfig,
);
});
14 changes: 13 additions & 1 deletion apps/meteor/app/gitlab/server/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import passport from 'passport';
import _ from 'underscore';

import { addPassportCustomOAuth } from '../../../server/lib/oauth/addPassportCustomOAuth';
import { CustomOAuth } from '../../custom-oauth/server/custom_oauth_server';
import { settings } from '../../settings/server';

const config: Partial<OAuthConfiguration> = {
Expand All @@ -18,6 +19,8 @@ const config: Partial<OAuthConfiguration> = {
accessTokenParam: 'access_token',
};

const Gitlab = new CustomOAuth('gitlab', config);

const configureGitlabOAuth = () => {
passport.unuse('gitlab');

Expand All @@ -36,7 +39,15 @@ const configureGitlabOAuth = () => {
return;
}

addPassportCustomOAuth('gitlab', { ...config, clientId, clientSecret, serverURL, identityPath, mergeUsers });
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';
const completeConfig = { ...config, clientId, clientSecret, serverURL, identityPath, mergeUsers };

if (isPassportFlowEnabled) {
addPassportCustomOAuth('gitlab', completeConfig);
return;
}

Gitlab.configure(completeConfig);
Comment thread
yash-rajpal marked this conversation as resolved.
};

Meteor.startup(() => {
Expand All @@ -50,6 +61,7 @@ Meteor.startup(() => {
'Accounts_OAuth_Gitlab_secret',
'Accounts_OAuth_Gitlab_identity_path',
'Accounts_OAuth_Gitlab_merge_users',
'Accounts_OAuth_Flow_Engine',
],
updateConfig,
);
Expand Down
27 changes: 20 additions & 7 deletions apps/meteor/app/nextcloud/server/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ import type { OAuthConfiguration } from '@rocket.chat/core-typings';
import { Meteor } from 'meteor/meteor';

import { addPassportCustomOAuth } from '../../../server/lib/oauth/addPassportCustomOAuth';
import { CustomOAuth } from '../../custom-oauth/server/custom_oauth_server';
import { settings } from '../../settings/server/cached';

const NEXTCLOUD_PATHS = {
serverURL: '',
tokenPath: '/index.php/apps/oauth2/api/v1/token',
tokenSentVia: 'header' as OAuthConfiguration['tokenSentVia'],
authorizePath: '/index.php/apps/oauth2/authorize',
Expand All @@ -16,6 +18,8 @@ const NEXTCLOUD_PATHS = {
},
};

const Nextcloud = new CustomOAuth('nextcloud', NEXTCLOUD_PATHS);

function configureNextcloudOAuth(): void {
const enabled = settings.get<boolean>('Accounts_OAuth_Nextcloud');
if (!enabled) {
Expand All @@ -30,17 +34,26 @@ function configureNextcloudOAuth(): void {
return;
}

addPassportCustomOAuth('nextcloud', {
...NEXTCLOUD_PATHS,
serverURL,
clientId,
clientSecret,
});
const config = { ...NEXTCLOUD_PATHS, serverURL, clientId, clientSecret };
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';

if (isPassportFlowEnabled) {
addPassportCustomOAuth('nextcloud', config);
return;
}

Nextcloud.configure(config);
}

Meteor.startup(() => {
settings.watchMultiple(
['Accounts_OAuth_Nextcloud', 'Accounts_OAuth_Nextcloud_URL', 'Accounts_OAuth_Nextcloud_id', 'Accounts_OAuth_Nextcloud_secret'],
[
'Accounts_OAuth_Nextcloud',
'Accounts_OAuth_Nextcloud_URL',
'Accounts_OAuth_Nextcloud_id',
'Accounts_OAuth_Nextcloud_secret',
'Accounts_OAuth_Flow_Engine',
],
configureNextcloudOAuth,
);
});
14 changes: 12 additions & 2 deletions apps/meteor/app/wordpress/server/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import passport from 'passport';
import _ from 'underscore';

import { addPassportCustomOAuth } from '../../../server/lib/oauth/addPassportCustomOAuth';
import { CustomOAuth } from '../../custom-oauth/server/custom_oauth_server';
import { settings } from '../../settings/server';

const config: Partial<OAuthConfiguration> = {
Expand All @@ -20,6 +21,8 @@ const config: Partial<OAuthConfiguration> = {

const serviceKey = 'wordpress';

const WordPress = new CustomOAuth(serviceKey, config);

const fillSettings = _.debounce(async (): Promise<void> => {
config.serverURL = settings.get('API_Wordpress_URL');
if (!config.serverURL) {
Expand Down Expand Up @@ -66,7 +69,13 @@ const fillSettings = _.debounce(async (): Promise<void> => {
break;
}

addPassportCustomOAuth(serviceKey, config);
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';

if (isPassportFlowEnabled) {
addPassportCustomOAuth(serviceKey, config);
} else {
WordPress.configure(config);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

const enabled = settings.get('Accounts_OAuth_Wordpress');
if (enabled) {
Expand All @@ -86,5 +95,6 @@ const fillSettings = _.debounce(async (): Promise<void> => {
}, 1000);

Meteor.startup(() => {
return settings.watchByRegex(/(API\_Wordpress\_URL)?(Accounts\_OAuth\_Wordpress\_)?/, () => fillSettings());
settings.watchByRegex(/(API\_Wordpress\_URL)?(Accounts\_OAuth\_Wordpress\_)?/, () => fillSettings());
settings.watch('Accounts_OAuth_Flow_Engine', () => fillSettings());
});
18 changes: 15 additions & 3 deletions apps/meteor/server/configuration/configurePassport.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ export const oAuthRouter = router();
const oAuthApp = express();
oAuthApp.set('trust proxy', true);

const configureOAuth = (settings: ICachedSettings) => {
if (settings.get<string>('Accounts_OAuth_Flow_Engine') !== 'passport') {
return;
}

const services = getOAuthServices(settings);
const oauthServiceConfigs = createOAuthServiceConfig(settings, services);
configureOAuthServices(oauthServiceConfigs, settings);
};
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated

export const configurePassport = (settings: ICachedSettings) => {
const { client } = MongoInternals.defaultRemoteCollectionDriver().mongo;

Expand Down Expand Up @@ -84,9 +94,11 @@ export const configurePassport = (settings: ICachedSettings) => {
});

settings.watchByRegex(/^(Accounts_OAuth_)[a-z0-9_]+$/i, () => {
const services = getOAuthServices(settings);
const oauthServiceConfigs = createOAuthServiceConfig(settings, services);
configureOAuthServices(oauthServiceConfigs, settings);
configureOAuth(settings);
});

settings.watch('Accounts_OAuth_Flow_Engine', () => {
Comment thread
cubic-dev-ai[bot] marked this conversation as resolved.
Outdated
configureOAuth(settings);
});

WebApp.rawConnectHandlers.use(oAuthApp);
Expand Down
10 changes: 9 additions & 1 deletion apps/meteor/server/lib/oauth/addPassportCustomOAuth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,15 @@ export const addPassportCustomOAuth = (serviceName: string, config: Partial<OAut

oAuthRouter.get(
`/_oauth/${serviceName}`,
passport.authenticate(serviceName, { failureRedirect: '/login', failureFlash: true, failWithError: true, keepSessionInfo: true }),
(_req, _res, next) => {
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';
if (isPassportFlowEnabled) {
next();
} else {
next('router');
}
},
passport.authenticate(serviceName, { failureRedirect: '/login', failWithError: true, keepSessionInfo: true }),
passportOAuthCallback(siteUrl),
);
};
11 changes: 10 additions & 1 deletion apps/meteor/server/lib/oauth/configureOAuthServices.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,16 @@ export const configureOAuthServices = (oauthServiceConfig: OAuthServiceConfig[],
);
oAuthRouter.get(
`/_oauth/${config.provider}`,
Comment thread
yash-rajpal marked this conversation as resolved.
passport.authenticate(config.provider, { failureRedirect: '/login', failureFlash: true, failWithError: true, keepSessionInfo: true }),
(_req, _res, next) => {
console.log('In check middleware');
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
const isPassportFlowEnabled = settings.get<string>('Accounts_OAuth_Flow_Engine') === 'passport';
if (isPassportFlowEnabled) {
next();
} else {
next('router');
Comment thread
yash-rajpal marked this conversation as resolved.
}
},
passport.authenticate(config.provider, { failureRedirect: '/login', failWithError: true, keepSessionInfo: true }),
passportOAuthCallback(siteUrl),
);
});
Expand Down
10 changes: 10 additions & 0 deletions apps/meteor/server/settings/oauth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@ import { settingsRegistry } from '../../app/settings/server';

export const createOauthSettings = () =>
settingsRegistry.addGroup('OAuth', async function () {
await this.add('Accounts_OAuth_Flow_Engine', 'meteor', {
Comment thread
yash-rajpal marked this conversation as resolved.
type: 'select',
values: [
{ key: 'meteor', i18nLabel: 'Meteor' },
{ key: 'passport', i18nLabel: 'New flow' },
],
public: true,
i18nDescription: 'Accounts_OAuth_Flow_Engine_Description',
});

Comment on lines +7 to +16

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

MEDIUM Sensitive OAuth Credentials (Nextcloud Client Secret and Apple Private Key) Missing secret: true Flag

In Rocket.Chat, settings are registered with metadata that controls how they are handled by the system. The secret: true option is used to mark highly sensitive settings (such as API keys, passwords, and client secrets) so that the platform can apply special protections, such as redacting them in logs, excluding them from certain API responses, or masking them in diagnostic exports.

An analysis of apps/meteor/server/settings/oauth.ts reveals that while almost all OAuth provider secrets (such as Google, GitHub, Facebook, Twitter, and LinkedIn) are correctly registered with the secret: true option, the following two highly sensitive credentials are missing this flag:

  1. Accounts_OAuth_Apple_secretKey (the private key used for Apple Sign-In authentication)
  2. Accounts_OAuth_Nextcloud_secret (the client secret used for Nextcloud OAuth authentication)

Because these settings lack the secret: true metadata, they are treated as standard non-sensitive settings. This can lead to the exposure of the Nextcloud Client Secret and the Apple Private Key in plain text in logs, diagnostic reports, or settings export APIs, allowing an attacker with access to these outputs to compromise the OAuth integration or impersonate the server to the identity providers.

Steps to Reproduce

Reviewing apps/meteor/server/settings/oauth.ts shows the discrepancy between Nextcloud/Apple and other OAuth providers:

// Drupal (Correct)
await this.add('Accounts_OAuth_Drupal_secret', '', { type: 'string', enableQuery, secret: true });

// Apple (Vulnerable - missing secret: true)
await this.add('Accounts_OAuth_Apple_secretKey', '', { type: 'string', multiline: true });

// Nextcloud (Vulnerable - missing secret: true)
await this.add('Accounts_OAuth_Nextcloud_secret', '', { type: 'string', enableQuery });
Fix with AI

Open in Cursor Open in Claude

A security vulnerability was found by Hacktron.

File: apps/meteor/server/settings/oauth.ts
Lines: 7-16
Severity: medium

Vulnerability: Sensitive OAuth Credentials (Nextcloud Client Secret and Apple Private Key) Missing secret: true Flag

Description:
In Rocket.Chat, settings are registered with metadata that controls how they are handled by the system. The `secret: true` option is used to mark highly sensitive settings (such as API keys, passwords, and client secrets) so that the platform can apply special protections, such as redacting them in logs, excluding them from certain API responses, or masking them in diagnostic exports.

An analysis of `apps/meteor/server/settings/oauth.ts` reveals that while almost all OAuth provider secrets (such as Google, GitHub, Facebook, Twitter, and LinkedIn) are correctly registered with the `secret: true` option, the following two highly sensitive credentials are missing this flag:
1. `Accounts_OAuth_Apple_secretKey` (the private key used for Apple Sign-In authentication)
2. `Accounts_OAuth_Nextcloud_secret` (the client secret used for Nextcloud OAuth authentication)

Because these settings lack the `secret: true` metadata, they are treated as standard non-sensitive settings. This can lead to the exposure of the Nextcloud Client Secret and the Apple Private Key in plain text in logs, diagnostic reports, or settings export APIs, allowing an attacker with access to these outputs to compromise the OAuth integration or impersonate the server to the identity providers.

Proof of Concept:
Reviewing `apps/meteor/server/settings/oauth.ts` shows the discrepancy between Nextcloud/Apple and other OAuth providers:

```typescript
// Drupal (Correct)
await this.add('Accounts_OAuth_Drupal_secret', '', { type: 'string', enableQuery, secret: true });

// Apple (Vulnerable - missing secret: true)
await this.add('Accounts_OAuth_Apple_secretKey', '', { type: 'string', multiline: true });

// Nextcloud (Vulnerable - missing secret: true)
await this.add('Accounts_OAuth_Nextcloud_secret', '', { type: 'string', enableQuery });
```

Affected Code:
- [oauth.ts:43](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/server/settings/oauth.ts#L43)
```typescript
await this.add('Accounts_OAuth_Apple_secretKey', '', { type: 'string', multiline: true });
```
- [oauth.ts:113](https://github.com/RocketChat/Rocket.Chat/blob/master/apps/meteor/server/settings/oauth.ts#L113)
```typescript
await this.add('Accounts_OAuth_Nextcloud_secret', '', { type: 'string', enableQuery });
```

Acceptance criteria:
- Acceptance is defined by the **actual reported behavior**, not by tests passing.
- Reproduce the issue, or narrow the exact code path that produces it, *before* changing code. State what you confirmed.
- Fix the underlying cause. Mitigations that paper over the reported behavior do not count as a fix.
- Add a regression test that fails on the unpatched code and passes on the fix. If a regression test is genuinely impractical (e.g. race condition, infra-level issue), say so and explain why.
- Existing tests passing is **not** the bar. Do not declare done on tests-pass theatre.

Only change what is necessary to fix this vulnerability. Do not refactor adjacent code or modify unrelated files.

Triage: Reply !fp <reason> (false positive), !valid (confirmed), or !accepted_risk <reason>. Any other reply is saved as a triage note.
Reason is optional but improves future scans — e.g. !fp internal endpoint, not user-facing.

View finding in Hacktron

await this.section('Drupal', async function () {
const enableQuery = {
_id: 'Accounts_OAuth_Drupal',
Expand Down
4 changes: 3 additions & 1 deletion packages/i18n/src/locales/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,8 @@
"Accounts_OAuth_Wordpress_server_type_wordpress_com": "Wordpress.com",
"Accounts_OAuth_Wordpress_server_type_wp_oauth_server": "WP OAuth Server Plugin",
"Accounts_OAuth_Wordpress_token_path": "Token Path",
"Accounts_OAuth_Flow_Engine": "OAuth Authentication Flow",
"Accounts_OAuth_Flow_Engine_Description": "Choose the OAuth implementation. \n **Modern OAuth (Recommended):** Passport-based flow with system browser support on mobile and deep-link callbacks. \n **Legacy Meteor OAuth:** Original Meteor OAuth implementation for backward compatibility.",
"Accounts_PasswordReset": "Password Reset",
"Accounts_Password_Policy_AtLeastOneLowercase": "At Least One Lowercase",
"Accounts_Password_Policy_AtLeastOneLowercase_Description": "Enforce that a password contain at least one lowercase character.",
Expand Down Expand Up @@ -7253,4 +7255,4 @@
"Avatar_preview_updated": "Avatar preview updated",
"Select_message_from_user": "Select message from {{username}}",
"Select_message_from_user_with_preview": "Select message from {{username}}: {{message}}"
}
}
9 changes: 8 additions & 1 deletion packages/web-ui-registration/src/LoginServices.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const LoginServices = ({
const { t } = useTranslation();
const services = useLoginServices();
const showFormLogin = useSetting('Accounts_ShowFormLogin');
const enableNewOAuthFlow = useSetting('Accounts_OAuth_Flow_Engine') === 'passport';
Comment thread
yash-rajpal marked this conversation as resolved.

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

Expand Down Expand Up @@ -52,7 +53,13 @@ const LoginServices = ({
{servicesToShow.length > 0 && (
<ButtonGroup vertical stretch small>
{servicesToShow.map((service) => (
<LoginServicesButton disabled={disabled} key={service.service} {...service} setError={setError} />
<LoginServicesButton
disabled={disabled}
key={service.service}
{...service}
setError={setError}
enableNewOAuthFlow={enableNewOAuthFlow}
/>
))}
</ButtonGroup>
)}
Expand Down
6 changes: 4 additions & 2 deletions packages/web-ui-registration/src/LoginServicesButton.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,17 +20,19 @@ const LoginServicesButton = <T extends LoginService>({
setError,
buttonColor,
buttonLabelColor,
enableNewOAuthFlow,
...props
}: T & {
className?: string;
disabled?: boolean;
setError?: Dispatch<SetStateAction<LoginErrorState>>;
enableNewOAuthFlow?: boolean;
}): ReactElement => {
const { t } = useTranslation();
const handler = useLoginWithService({ service, buttonLabelText, ...props });

const handleOnClick = useCallback(() => {
if (!servicesSupportedByMeteor.includes(service)) {
if (!servicesSupportedByMeteor.includes(service) && enableNewOAuthFlow) {
const url = new URL(window.location.href);
const queryParams = url.searchParams;
const loginClient = queryParams.get('loginClient');
Expand All @@ -51,7 +53,7 @@ const LoginServicesButton = <T extends LoginService>({
}
setError?.([e.error, e.reason]);
});
}, [handler, setError, service]);
}, [handler, setError, service, enableNewOAuthFlow]);

return (
<Button
Expand Down
Loading