Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 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)
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
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
65 changes: 62 additions & 3 deletions packages/mock-providers/src/MockedAppRootBuilder.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,16 @@ import type {
Serialized,
SettingValue,
} from '@rocket.chat/core-typings';
import type { ServerMethodName, ServerMethodParameters, ServerMethodReturn } from '@rocket.chat/ddp-client';
import type {
ServerMethodName,
ServerMethodParameters,
ServerMethodReturn,
StreamerCallback,
StreamerCallbackArgs,
StreamerEvents,
StreamKeys,
StreamNames,
} from '@rocket.chat/ddp-client';
import { Emitter } from '@rocket.chat/emitter';
import languages from '@rocket.chat/i18n/dist/languages';
import { createPredicateFromFilter } from '@rocket.chat/mongo-adapter';
Expand Down Expand Up @@ -52,9 +61,35 @@ type Mutable<T> = {
};

// eslint-disable-next-line @typescript-eslint/naming-convention
interface MockedAppRootEvents {
// interface MockedAppRootEvents extends Record<`stream-${StreamNames}-${StreamKeys<StreamNames>}`, any> {
// 'update-modal': void;
// }
// Extract all key values from objects that have a 'key' property
type ExtractKeys<T, N extends string> = T extends readonly (infer U)[]
? U extends { key: infer K }
? K extends string
? string extends K
? never
: `stream-${N}-${K}`
: never
: never
: never;

// Union of all key values from all streams
type AllStreamerEventKeys = {
[K in keyof StreamerEvents]: ExtractKeys<StreamerEvents[K], K>;
}[keyof StreamerEvents];

type MockedAppRootEvents = {
'update-modal': void;
}
} & Record<AllStreamerEventKeys, any>;

export type StreamControllerRef<N extends StreamNames> = {
controller?: {
emit: <K extends StreamKeys<N>>(eventName: K, args: StreamerCallbackArgs<N, K>) => void;
has: (eventName: StreamKeys<N>) => boolean;
};
};

const empty = [] as const;

Expand Down Expand Up @@ -248,6 +283,30 @@ export class MockedAppRootBuilder {
return this;
}

withStream<N extends StreamNames>(streamName: N, ref: StreamControllerRef<N>): this {
const innerFn = this.server.getStream;

const outerFn: ServerContextValue['getStream'] = (innerStreamName) => {
if (innerStreamName === (streamName as StreamNames)) {
ref.controller = {
emit: <K extends StreamKeys<N>>(eventName: K, args: StreamerCallbackArgs<N, K>) => {
this.events.emit(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys, ...args);
},
has: (eventName: string) => this.events.has(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys),
};

return <K extends StreamKeys<N>>(eventName: K, callback: StreamerCallback<N, K>) =>
this.events.on(`stream-${innerStreamName}-${eventName}` as AllStreamerEventKeys, callback);
}

return innerFn(innerStreamName);
};

this.server.getStream = outerFn;

return this;
}

withMethod<TMethodName extends ServerMethodName>(methodName: TMethodName, response: () => ServerMethodReturn<TMethodName>): this {
const innerFn = this.server.callMethod;

Expand Down
1 change: 1 addition & 0 deletions packages/mock-providers/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ export * from './MockedServerContext';
export * from './MockedSettingsContext';
export * from './MockedUserContext';
export * from './MockedDeviceContext';
export * from './MockedAppRootBuilder';
2 changes: 1 addition & 1 deletion yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -9387,7 +9387,7 @@ __metadata:
peerDependencies:
"@rocket.chat/layout": "*"
"@rocket.chat/tools": 0.2.3
"@rocket.chat/ui-contexts": 22.0.0-rc.6
"@rocket.chat/ui-contexts": 22.0.0
"@tanstack/react-query": "*"
react: "*"
react-hook-form: "*"
Expand Down
Loading