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

Bump @rocket.chat/meteor version.
6 changes: 6 additions & 0 deletions .changeset/fix-presence-comma-ids.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@rocket.chat/meteor': patch
'@rocket.chat/rest-typings': patch
---

Fixes the `users.presence` endpoint returning an empty array when called with multiple comma-separated IDs, caused by `ajvQuery` coercing the string into a single-element array after the OpenAPI migration
7 changes: 7 additions & 0 deletions .changeset/good-rules-lie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
'@rocket.chat/model-typings': patch
'@rocket.chat/models': patch
'@rocket.chat/meteor': patch
---

Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle

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.

P2: The changeset summary appears unrelated to this PR’s actual fixes, so release notes for this patch will be misleading.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .changeset/good-rules-lie.md, line 7:

<comment>The changeset summary appears unrelated to this PR’s actual fixes, so release notes for this patch will be misleading.</comment>

<file context>
@@ -0,0 +1,7 @@
+'@rocket.chat/meteor': patch
+---
+
+Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle
</file context>
Suggested change
Ensures that deactivated users have their login tokens cleaned up in users.deactivateidle
Fixes users.presence handling for multiple IDs and strengthens autotranslate input validation and room-access enforcement.

5 changes: 5 additions & 0 deletions .changeset/neat-trams-juggle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Ensures the Meteor method for translateMessage validates access and types
5 changes: 5 additions & 0 deletions .changeset/shaggy-kiwis-burn.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@rocket.chat/meteor': patch
---

Ensures the autotranslate.translateMessage endpoint checks for room access
10 changes: 9 additions & 1 deletion apps/meteor/app/api/server/v1/autotranslate.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
import type { IMessage, ISupportedLanguage } from '@rocket.chat/core-typings';
import { Messages } from '@rocket.chat/models';
import { Messages, Rooms } from '@rocket.chat/models';
import {
ajv,
validateUnauthorizedErrorResponse,
validateBadRequestErrorResponse,
validateForbiddenErrorResponse,
isAutotranslateSaveSettingsParamsPOST,
isAutotranslateGetSupportedLanguagesParamsGET,
} from '@rocket.chat/rest-typings';

import { canAccessRoomAsync } from '../../../authorization/server';
import { getSupportedLanguages } from '../../../autotranslate/server/functions/getSupportedLanguages';
import { saveAutoTranslateSettings } from '../../../autotranslate/server/functions/saveSettings';
import { translateMessage } from '../../../autotranslate/server/functions/translateMessage';
Expand Down Expand Up @@ -124,6 +126,7 @@ const autotranslateEndpoints = API.v1
}),
400: validateBadRequestErrorResponse,
401: validateUnauthorizedErrorResponse,
403: validateForbiddenErrorResponse,
},
},
async function action() {
Expand All @@ -139,6 +142,11 @@ const autotranslateEndpoints = API.v1
return API.v1.failure('Message not found.');
}

const room = await Rooms.findOneById(message.rid);
if (!room || !(await canAccessRoomAsync(room, { _id: this.userId }))) {
return API.v1.forbidden();
}

const translatedMessage = await translateMessage(targetLanguage, message);

if (!translatedMessage) {
Expand Down
13 changes: 12 additions & 1 deletion apps/meteor/app/api/server/v1/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -548,9 +548,20 @@ API.v1.post(
const lastLoggedIn = new Date();
lastLoggedIn.setDate(lastLoggedIn.getDate() - daysIdle);

// since we're deactiving users that are not logged in, there is no need to send data through WS
const ids = await Users.findActiveNotLoggedInAfterWithRole(lastLoggedIn, role, { projection: { _id: 1 } })

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.

P2: This read-then-update-then-notify sequence can broadcast incorrect user state for users that no longer match the update filter at update time.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/api/server/v1/users.ts, line 551:

<comment>This read-then-update-then-notify sequence can broadcast incorrect user state for users that no longer match the update filter at update time.</comment>

<file context>
@@ -548,9 +548,20 @@ API.v1.post(
 		lastLoggedIn.setDate(lastLoggedIn.getDate() - daysIdle);
 
-		// since we're deactiving users that are not logged in, there is no need to send data through WS
+		const ids = await Users.findActiveNotLoggedInAfterWithRole(lastLoggedIn, role, { projection: { _id: 1 } })
+			.map(({ _id }: { _id: string }) => _id)
+			.toArray();
</file context>

.map(({ _id }: { _id: string }) => _id)
.toArray();

const { modifiedCount: count } = await Users.setActiveNotLoggedInAfterWithRole(lastLoggedIn, role, false);

ids.forEach((_id) => {
void notifyOnUserChange({
clientAction: 'updated',
id: _id,
diff: { 'services.resume.loginTokens': [], 'active': false },
});
});

return API.v1.success({
count,
});
Expand Down
21 changes: 20 additions & 1 deletion apps/meteor/app/autotranslate/server/methods/translateMessage.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import type { IMessage } from '@rocket.chat/core-typings';
import type { ServerMethods } from '@rocket.chat/ddp-client';
import { Messages, Rooms } from '@rocket.chat/models';
import { check } from 'meteor/check';
import { Meteor } from 'meteor/meteor';

import { canAccessRoomAsync } from '../../../authorization/server';
import { translateMessage } from '../functions/translateMessage';

declare module '@rocket.chat/ddp-client' {
Expand All @@ -13,6 +16,22 @@ declare module '@rocket.chat/ddp-client' {

Meteor.methods<ServerMethods>({
async 'autoTranslate.translateMessage'(message, targetLanguage) {
return translateMessage(targetLanguage, message);
const userId = Meteor.userId();
if (!userId) {
throw new Meteor.Error('error-invalid-user', 'Invalid user', {
method: 'autoTranslate.translateMessage',
});
}
check(message?._id, String);
check(targetLanguage, String);
const msg = await Messages.findOneById(message._id);
if (!msg) {
throw new Meteor.Error('error-message-not-found', 'Message not found');
}
const room = await Rooms.findOneById(msg.rid);
if (!room || !(await canAccessRoomAsync(room, { _id: userId }))) {
throw new Meteor.Error('error-not-allowed', 'Not allowed');
}
return translateMessage(targetLanguage, msg);
},
});
148 changes: 147 additions & 1 deletion apps/meteor/tests/end-to-end/api/autotranslate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { IMessage, IRoom, IUser } from '@rocket.chat/core-typings';
import { expect } from 'chai';
import { before, describe, after, it } from 'mocha';

import { getCredentials, api, request, credentials } from '../../data/api-data';
import { getCredentials, api, request, credentials, methodCall } from '../../data/api-data';
import { sendSimpleMessage } from '../../data/chat.helper';
import { updatePermission, updateSetting } from '../../data/permissions.helper';
import { createRoom, deleteRoom } from '../../data/rooms.helper';
Expand Down Expand Up @@ -374,6 +374,152 @@ describe('AutoTranslate', () => {
})
.end(done);
});

describe('room access check', () => {
let userA: TestUser<IUser>;
let userB: TestUser<IUser>;
let credA: Credentials;
let credB: Credentials;
let privateRoom: IRoom;
let privateMessage: IMessage;

before(async () => {
await updateSetting('AutoTranslate_Enabled', true);

userA = await createUser();
userB = await createUser();

credA = await login(userA.username, password);
credB = await login(userB.username, password);

privateRoom = (
await createRoom({
type: 'p',
name: `test-autotranslate-access-${Date.now()}`,
credentials: credA,
})
).body.group;

const msgRes = await sendSimpleMessage({
roomId: privateRoom._id,
text: 'Isso é um teste',
userCredentials: credA,
});
privateMessage = msgRes.body.message;
});

after(async () => {
await Promise.all([
updateSetting('AutoTranslate_Enabled', false),
deleteUser(userA),
deleteUser(userB),
deleteRoom({ type: 'p', roomId: privateRoom._id }),
]);
});

it('should return 403 forbidden when the user is not a member of the room', (done) => {
void request
.post(api('autotranslate.translateMessage'))
.set(credB)
.send({
messageId: privateMessage._id,
})
.expect('Content-Type', 'application/json')
.expect(403)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
})
.end(done);
});
});
});

describe('[autoTranslate.translateMessage method]', () => {
let userA: TestUser<IUser>;
let userB: TestUser<IUser>;
let credA: Credentials;
let credB: Credentials;
let privateRoom: IRoom;
let privateMessage: IMessage;

before(async () => {
await updateSetting('AutoTranslate_Enabled', true);

userA = await createUser();
userB = await createUser();

credA = await login(userA.username, password);
credB = await login(userB.username, password);

privateRoom = (
await createRoom({
type: 'p',
name: `test-autotranslate-method-${Date.now()}`,
credentials: credA,
})
).body.group;

const msgRes = await sendSimpleMessage({
roomId: privateRoom._id,
text: 'Isso é um teste',
userCredentials: credA,
});
privateMessage = msgRes.body.message;
});

after(async () => {
await Promise.all([
updateSetting('AutoTranslate_Enabled', false),
deleteUser(userA),
deleteUser(userB),
deleteRoom({ type: 'p', roomId: privateRoom._id }),
]);
});

it('should fail when messageId is not a string', (done) => {
void request
.post(methodCall('autoTranslate.translateMessage'))
.set(credA)
.send({
message: JSON.stringify({
msg: 'method',
id: 'id',
method: 'autoTranslate.translateMessage',
params: [{ _id: { $gt: '' } }, 'en'],
}),
})
.expect('Content-Type', 'application/json')
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
const parsedBody = JSON.parse(res.body.message);
expect(parsedBody).to.have.a.property('error');
})
.end(done);
});

it('should return error-not-allowed when the caller is not a member of the room', (done) => {
void request
.post(methodCall('autoTranslate.translateMessage'))
.set(credB)
.send({
message: JSON.stringify({
msg: 'method',
id: 'id',
method: 'autoTranslate.translateMessage',
params: [{ _id: privateMessage._id }, 'en'],
}),
})
.expect('Content-Type', 'application/json')
.expect(400)
.expect((res) => {
expect(res.body).to.have.a.property('success', false);
const parsedBody = JSON.parse(res.body.message);
expect(parsedBody).to.have.a.property('error');
expect(parsedBody.error).to.have.a.property('error', 'error-not-allowed');
})
.end(done);
});
});

describe('Autoenable setting', () => {
Expand Down
65 changes: 65 additions & 0 deletions apps/meteor/tests/end-to-end/api/users.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1392,6 +1392,48 @@ describe('[Users]', () => {
.end(done);
});

it('should return presence for a single id', async () => {
const res = await request
.get(api('users.presence'))
.query({ ids: 'rocket.cat' })
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200);

expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('full', false);
expect(res.body).to.have.property('users').that.is.an('array').with.lengthOf(1);
expect(res.body.users[0]).to.have.property('_id', 'rocket.cat');
});

it('should correctly parse comma-separated ids and not return an empty result', async () => {
const res = await request
.get(api('users.presence'))
.query({ ids: `rocket.cat,${credentials['X-User-Id']}` })
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200);

expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('full', false);
// only rocket.cat is guaranteed to be online; admin may be offline
expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat');
});

it('should return presence for repeated ids params', async () => {
const res = await request
.get(api('users.presence'))
.query(`ids=rocket.cat&ids=${credentials['X-User-Id']}`)
.set(credentials)
.expect('Content-Type', 'application/json')
.expect(200);

expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('full', false);
// only rocket.cat is guaranteed to be online; admin may be offline
expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat');
});
Comment on lines +1423 to +1435

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

Repeated-ids test can pass even if only the first ids value is parsed.

Current assertion still passes when the backend ignores repeated params, because rocket.cat is the first value and also the only asserted one. Make the first id non-existent and keep rocket.cat second to prove repeated-param parsing works.

Suggested test hardening
 it('should return presence for repeated ids params', async () => {
 	const res = await request
 		.get(api('users.presence'))
-		.query(`ids=rocket.cat&ids=${credentials['X-User-Id']}`)
+		.query('ids=non-existent-user-id&ids=rocket.cat')
 		.set(credentials)
 		.expect('Content-Type', 'application/json')
 		.expect(200);

 	expect(res.body).to.have.property('success', true);
 	expect(res.body).to.have.property('full', false);
-	// only rocket.cat is guaranteed to be online; admin may be offline
 	expect(res.body.users.map((u: IUser) => u._id)).to.include('rocket.cat');
 });
🧰 Tools
🪛 OpenGrep (1.20.0)

[ERROR] 1424-1426: SQL query built via string concatenation or template literal passed to query()/execute(). Use parameterized queries instead.

(coderabbit.sql-injection.raw-query-concat-js)

🤖 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/tests/end-to-end/api/users.ts` around lines 1423 - 1435, The test
"should return presence for repeated ids params" currently uses `rocket.cat` as
the first `ids` value so the test passes even if the server ignores repeated
params; change the first `ids` value to a non-existent id (e.g.,
"not.real.user") and keep `rocket.cat` as the second param in the `.query(...)`
call to ensure repeated-param parsing is exercised, then update the assertions
on `res.body.users` (the mapping used in the existing expects) to assert that
`rocket.cat` is present and that the non-existent id is not returned (or that
only `rocket.cat` is present) to prove the second param is parsed.


it('should return full list of online users for more than 10 minutes in the past', (done) => {
const date = new Date();
date.setMinutes(date.getMinutes() - 11);
Expand Down Expand Up @@ -4770,6 +4812,29 @@ describe('[Users]', () => {
expect(res.body).to.have.property('success', false);
});
});

it('should revoke login tokens of deactivated idle users', async () => {
const idleUser = await createUser();
await request.post(api('roles.addUserToRole')).set(credentials).send({ roleId: testRoleId, username: idleUser.username }).expect(200);

const idleUserCredentials = await login(idleUser.username, password);
await request.get(api('me')).set(idleUserCredentials).expect(200);

await updatePermission('edit-other-user-active-status', ['admin']);
await request
.post(api('users.deactivateIdle'))
.set(credentials)
.send({ daysIdle: 0, role: testRoleId })
.expect(200)
.expect((res: Response) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('count').that.is.greaterThan(0);
});

await request.get(api('me')).set(idleUserCredentials).expect(401);

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.

P2: This test does not prove login tokens were revoked; a 401 can happen just because the user was deactivated.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/tests/end-to-end/api/users.ts, line 4834:

<comment>This test does not prove login tokens were revoked; a 401 can happen just because the user was deactivated.</comment>

<file context>
@@ -4770,6 +4812,29 @@ describe('[Users]', () => {
+					expect(res.body).to.have.property('count').that.is.greaterThan(0);
+				});
+
+			await request.get(api('me')).set(idleUserCredentials).expect(401);
+
+			await deleteUser(idleUser);
</file context>


await deleteUser(idleUser);
});
});

describe('[/users.requestDataDownload]', () => {
Expand Down
1 change: 1 addition & 0 deletions packages/model-typings/src/models/IUsersModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -393,6 +393,7 @@ export interface IUsersModel extends IBaseModel<IUser> {
unsetAvatarData(userId: string): Promise<UpdateResult>;
setUserActive(userId: string, active: boolean): Promise<UpdateResult>;
setActiveNotLoggedInAfterWithRole(latestLastLoginDate: Date, role?: string, active?: boolean): Promise<UpdateResult | Document>;
findActiveNotLoggedInAfterWithRole(latestLastLoginDate: Date, role?: string, options?: FindOptions<IUser>): FindCursor<IUser>;
unsetRequirePasswordChange(userId: string): Promise<UpdateResult>;
resetPasswordAndSetRequirePasswordChange(
userId: string,
Expand Down
Loading
Loading