Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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': minor
Comment thread
ggazzo marked this conversation as resolved.
Outdated
---

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
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
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

🧩 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
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
47 changes: 47 additions & 0 deletions apps/meteor/tests/end-to-end/api/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,53 @@ describe('[Users]', () => {
]),
);

it('should fail when request is without authentication credentials', async () => {
await request
.get(api('users.info'))
.query({
userId: targetUser._id,
})
.expect('Content-Type', 'application/json')
.expect(401)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error');
});
});

describe('authentication', () => {
before(() => updateSetting('Accounts_AllowAnonymousRead', true));
after(() => updateSetting('Accounts_AllowAnonymousRead', false));
it('should fail when request is without authentication credentials and Anonymous Read is enabled', async () => {
await request
.get(api('users.info'))
.query({
userId: targetUser._id,
})
.expect('Content-Type', 'application/json')
.expect(401)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error');
});
});

it('should fail when request is without token and Anonymous Read is enabled', async () => {
await request
.get(api('users.info'))
.query({
userId: targetUser._id,
})
.set({ 'X-User-Id': credentials['X-User-Id'] })
.expect('Content-Type', 'application/json')
.expect(401)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('error');
});
});
});

it('should return an error when the user does not exist', (done) => {
void request
.get(api('users.info'))
Expand Down
Loading
Loading