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 .changeset/bump-patch-1758105326594.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Bump @rocket.chat/meteor version.
5 changes: 5 additions & 0 deletions .changeset/green-ants-shop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Fixes login using iframe authentication.
5 changes: 5 additions & 0 deletions .changeset/grumpy-berries-arrive.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates)
6 changes: 6 additions & 0 deletions .changeset/old-meals-pull.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@rocket.chat/apps-engine': patch
'@rocket.chat/meteor': patch
---

Changes a strict behavior on reporting slash commands provided by apps
5 changes: 5 additions & 0 deletions .changeset/spicy-crabs-complain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Ensures the formatted volume value is kept between 0 and 1
7 changes: 5 additions & 2 deletions apps/meteor/app/api/server/ApiClass.ts
Original file line number Diff line number Diff line change
Expand Up @@ -811,12 +811,15 @@ export class APIClass<
if (options.authRequired || options.authOrAnonRequired) {
const user = await api.authenticatedRoute.call(this, this.request);
this.user = user!;
this.userId = String(this.request.headers.get('x-user-id'));
this.userId = this.user?._id;
const authToken = this.request.headers.get('x-auth-token');
this.token = (authToken && Accounts._hashLoginToken(String(authToken)))!;
}
Comment on lines 811 to 817
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

🧩 Analysis chain

Good change: derive userId from the authenticated user

Setting this.userId = this.user?._id prevents header spoofing from leaking into permission checks. LGTM.

Run to spot any lingering server code that still trusts x-user-id directly:

As a follow-up, consider eliminating x-user-id dependency during authentication entirely (derive the user solely from the token). Example refactor for authenticatedRoute:

// token-only lookup; avoids trusting a client-supplied user id
protected async authenticatedRoute(req: Request): Promise<IUser | null> {
  const authToken = req.headers.get('x-auth-token');
  if (!authToken) return null;

  const hashed = Accounts._hashLoginToken(String(authToken));
  return Users.findOne(
    { 'services.resume.loginTokens.hashedToken': hashed },
    { projection: getDefaultUserFields() },
  );
}

🏁 Script executed:

#!/bin/bash
rg -nP -C2 --type=ts "(x-user-id|X-User-Id)" apps/meteor | sed 's/^/apps\/meteor\//'

Length of output: 49525


Do not trust X-User-Id headers — remove/replace remaining server-side usages

Good change to derive this.userId from the authenticated user (prevents header spoofing). However the repo still contains server-side code that reads/trusts X-User-Id; these must be refactored to derive the user from the token (token-only lookup) before merging. Representative locations to fix:

  • apps/meteor/server/routes/userDataDownload.ts: req.headers['x-user-id'] (line ~36).
  • apps/meteor/app/api/server/middlewares/authentication.ts: copies/uses rc_uid / x-user-id from cookies/headers (lines ~22–27).
  • apps/meteor/app/api/server/helpers/getLoggedInUser.ts: request.headers.get('x-user-id') (lines ~6–9).
  • apps/meteor/app/api/server/ApiClass.ts: const { 'x-user-id': userId } = headers (lines ~964–965).
  • apps/meteor/app/file-upload/server/lib/FileUpload.ts: const uid = rc_uid || headers['x-user-id'] (line ~470).
  • Logging/CORS/docs: app/api/server/middlewares/logger.ts logs x-user-id; cors.ts and default/openApi.ts still advertise/allow X-User-Id — update accordingly.
  • Many end-to-end / e2e tests and fixtures under apps/meteor/tests/** still set X-User-Id — update tests or test helpers to use token-derived user ids.

Action: implement token-only authenticatedRoute (or equivalent) and remove header-based authorization/readers, update middleware/helpers/CORS/docs and tests. Block merge until server-side reliance on client-supplied X-User-Id is removed or explicitly validated.

🤖 Prompt for AI Agents
In apps/meteor/app/api/server/ApiClass.ts around lines 811 to 817, the code
still reads and trusts client-supplied headers (x-user-id) when setting
userId/token; remove any reliance on X-User-Id here and throughout the server.
Replace header-derived user identification with a token-only lookup: call the
authenticatedRoute (or equivalent token-validation routine) to resolve the user
object and set this.user and this.userId from that object only, derive
this.token from the x-auth-token after validating it server-side, and delete any
code that reads or copies rc_uid / x-user-id. Then audit and update the other
listed files (apps/meteor/server/routes/userDataDownload.ts,
app/api/server/middlewares/authentication.ts,
app/api/server/helpers/getLoggedInUser.ts, file-upload server code,
logger/CORS/docs) to remove header-based auth, update CORS/docs to no longer
advertise X-User-Id, and adjust tests/fixtures to use token-derived
authentication; do not merge until all server-side usages of X-User-Id are
removed or replaced with token-validated user lookups.


if (!this.user && options.authRequired && !options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead')) {
const shouldPreventAnonymousRead = !this.user && options.authOrAnonRequired && !settings.get('Accounts_AllowAnonymousRead');
const shouldPreventUserRead = !this.user && options.authRequired;

if (shouldPreventAnonymousRead || shouldPreventUserRead) {
const result = api.unauthorized('You must be logged in to do this.');
// compatibility with the old API
// TODO: MAJOR
Expand Down
20 changes: 14 additions & 6 deletions apps/meteor/client/hooks/iframe/useIframe.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { useLoginWithIframe, useLoginWithToken, useSetting } from '@rocket.chat/ui-contexts';
import { useCallback, useState } from 'react';
import { useCallback, useEffect, useState } from 'react';

export const useIframe = () => {
const [iframeLoginUrl, setIframeLoginUrl] = useState<string | undefined>(undefined);
Expand All @@ -12,6 +12,8 @@ export const useIframe = () => {
const iframeLogin = useLoginWithIframe();
const tokenLogin = useLoginWithToken();

const enabled = Boolean(iframeEnabled && accountIframeUrl && apiUrl && apiMethod);

const loginWithToken = useCallback(
(tokenData: string | { loginToken: string } | { token: string }, callback?: (error: Error | null | undefined) => void) => {
if (typeof tokenData === 'string') {
Expand All @@ -31,6 +33,10 @@ export const useIframe = () => {

const tryLogin = useCallback(
async (callback?: (error: Error | null | undefined, result: unknown) => void) => {
if (!enabled) {
return;
}

let url = accountIframeUrl;
let separator = '?';
if (url.indexOf('?') > -1) {
Expand All @@ -43,9 +49,7 @@ export const useIframe = () => {

const result = await fetch(apiUrl, {
method: apiMethod,
headers: {
'Content-Type': 'application/json',
},
headers: undefined,
credentials: 'include',
});

Expand All @@ -64,11 +68,15 @@ export const useIframe = () => {
callback?.(error, await result.json());
});
},
[apiMethod, apiUrl, accountIframeUrl, loginWithToken],
[apiMethod, apiUrl, accountIframeUrl, loginWithToken, enabled],
);

useEffect(() => {
tryLogin();
}, [tryLogin]);

return {
enabled: Boolean(iframeEnabled && accountIframeUrl && apiUrl && apiMethod),
enabled,
tryLogin,
loginWithToken,
iframeLoginUrl,
Expand Down
185 changes: 185 additions & 0 deletions apps/meteor/client/hooks/useAppSlashCommands.spec.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
import type { SlashCommand } from '@rocket.chat/core-typings';
import { mockAppRoot, type StreamControllerRef } from '@rocket.chat/mock-providers';
import { renderHook, waitFor } from '@testing-library/react';

import { useAppSlashCommands } from './useAppSlashCommands';
import { slashCommands } from '../../app/utils/client/slashCommand';

const mockSlashCommands: SlashCommand[] = [
{
command: '/test',
description: 'Test command',
params: 'param1 param2',
clientOnly: false,
providesPreview: false,
appId: 'test-app-1',
permission: undefined,
},
{
command: '/weather',
description: 'Get weather information',
params: 'city',
clientOnly: false,
providesPreview: true,
appId: 'weather-app',
permission: undefined,
},
];

const mockApiResponse = {
commands: mockSlashCommands,
total: mockSlashCommands.length,
};

describe('useAppSlashCommands', () => {
let mockGetSlashCommands: jest.Mock;

beforeEach(() => {
mockGetSlashCommands = jest.fn().mockResolvedValue(mockApiResponse);

slashCommands.commands = {};
});

afterEach(() => {
jest.clearAllMocks();
});

it('should not fetch data when user ID is not available', () => {
renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).build(),
});

expect(mockGetSlashCommands).not.toHaveBeenCalled();
expect(slashCommands.commands).toEqual({});
});

it('should fetch slash commands when user ID is available', async () => {
renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot().withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands).withJohnDoe().build(),
});

await waitFor(() => {
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
});
});

it('should handle command/removed event by invalidating queries', async () => {
const streamRef: StreamControllerRef<'apps'> = {};

renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot()
.withJohnDoe()
.withStream('apps', streamRef)
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
.build(),
});

expect(streamRef.controller).toBeDefined();

await waitFor(() => {
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
});

streamRef.controller?.emit('apps', [['command/removed', ['/test']]]);

expect(slashCommands.commands['/test']).toBeUndefined();
expect(slashCommands.commands['/weather']).toBeDefined();
});

it('should handle command/added event by invalidating queries', async () => {
const streamRef: StreamControllerRef<'apps'> = {};

renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot()
.withJohnDoe()
.withStream('apps', streamRef)
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
.build(),
});

expect(streamRef.controller).toBeDefined();

await waitFor(() => {
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
});

mockGetSlashCommands.mockResolvedValue({
commands: [
...mockSlashCommands,
{
command: '/newcommand',
description: 'New command',
params: 'param1 param2',
clientOnly: false,
},
],
total: mockSlashCommands.length + 1,
});

streamRef.controller?.emit('apps', [['command/added', ['/newcommand']]]);

await waitFor(() => {
expect(slashCommands.commands['/newcommand']).toBeDefined();
});

expect(slashCommands.commands['/test']).toBeDefined();
expect(slashCommands.commands['/weather']).toBeDefined();
});

it('should handle command/updated event by invalidating queries', async () => {
const streamRef: StreamControllerRef<'apps'> = {};

renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot()
.withJohnDoe()
.withStream('apps', streamRef)
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
.build(),
});

expect(streamRef.controller).toBeDefined();

await waitFor(() => {
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
});

streamRef.controller?.emit('apps', [['command/updated', ['/test']]]);

expect(slashCommands.commands['/test']).toBeUndefined();
expect(slashCommands.commands['/weather']).toBeDefined();
});

it('should ignore command/disabled event', async () => {
const streamRef: StreamControllerRef<'apps'> = {};

renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot()
.withJohnDoe()
.withStream('apps', streamRef)
.withEndpoint('GET', '/v1/commands.list', mockGetSlashCommands)
.build(),
});

expect(streamRef.controller).toBeDefined();

await waitFor(() => {
expect(Object.keys(slashCommands.commands)).toHaveLength(mockSlashCommands.length);
});

streamRef.controller?.emit('apps', [['command/disabled', ['/test']]]);

expect(slashCommands.commands['/test']).toBeDefined();
expect(slashCommands.commands['/weather']).toBeDefined();
});

it('should not set up stream listener when user ID is not available', () => {
const streamRef: StreamControllerRef<'apps'> = {};

renderHook(() => useAppSlashCommands(), {
wrapper: mockAppRoot().withStream('apps', streamRef).build(),
});

expect(streamRef.controller).toBeDefined();
expect(streamRef.controller?.has('apps')).toBe(false);
});
});
2 changes: 1 addition & 1 deletion apps/meteor/client/hooks/useAppSlashCommands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const useAppSlashCommands = () => {
return;
}
return apps('apps', ([key, [command]]) => {
if (['command/added', 'command/updated', 'command/disabled', 'command/removed'].includes(key)) {
if (['command/added', 'command/updated', 'command/removed'].includes(key)) {
if (typeof command === 'string') {
delete slashCommands.commands[command];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { CustomSoundContext, useStream, useUserPreference } from '@rocket.chat/u
import { useQuery, useQueryClient } from '@tanstack/react-query';
import { useEffect, useMemo, useRef, type ReactNode } from 'react';

import { defaultSounds, formatVolume, getCustomSoundURL } from './lib/helpers';
import { defaultSounds, getCustomSoundURL, formatVolume } from './lib';
import { sdk } from '../../../app/utils/client/lib/SDKClient';
import { useUserSoundPreferences } from '../../hooks/useUserSoundPreferences';

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { formatVolume } from './formatVolume';

describe('formatVolume', () => {
it('returns 1 if volume is 100', () => {
expect(formatVolume(100)).toBe(1);
});

it('returns 1 if volume is 200', () => {
expect(formatVolume(200)).toBe(1);
});

it('returns 0.5 if volume is 50', () => {
expect(formatVolume(50)).toBe(0.5);
});

it('returns 0 if volume is -10', () => {
expect(formatVolume(-10)).toBe(0);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export const formatVolume = (volume: number) => {
const clamped = Math.max(0, Math.min(volume, 100));
return Number((clamped / 100).toPrecision(2));
};
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,3 @@ export const defaultSounds: ICustomSound[] = [
{ _id: 'dialtone', name: 'Sound_Dialtone', extension: 'mp3', src: getAssetUrl('sounds/dialtone.mp3') },
{ _id: 'ringtone', name: 'Sound_Ringtone', extension: 'mp3', src: getAssetUrl('sounds/ringtone.mp3') },
];

export const formatVolume = (volume: number) => {
return Number((volume / 100).toPrecision(2));
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './helpers';
export * from './formatVolume';
4 changes: 2 additions & 2 deletions apps/meteor/tests/end-to-end/api/channels.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3503,10 +3503,10 @@ describe('[Channels]', () => {
roomId: testChannel._id,
})
.expect('Content-Type', 'application/json')
.expect(400)
.expect(401)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
expect(res.body).to.have.a.property('error', 'Enable "Allow Anonymous Read" [error-not-allowed]');
expect(res.body).to.have.a.property('error', 'You must be logged in to do this.');
})
.end(done);
});
Expand Down
Loading
Loading