fix: ensures the translateMessage Meteor method validates access and type#40528
Conversation
|
Looks like this PR is ready to merge! 🎉 |
🦋 Changeset detectedLatest commit: cfbf57d The changes in this PR will be included in the next version bump. This PR includes changesets to release 42 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThe PR hardens the ChangesautoTranslate.translateMessage server-side authorization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #40528 +/- ##
===========================================
+ Coverage 69.61% 69.62% +0.01%
===========================================
Files 3324 3324
Lines 122657 122657
Branches 21855 21847 -8
===========================================
+ Hits 85384 85406 +22
+ Misses 33936 33919 -17
+ Partials 3337 3332 -5
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/meteor/tests/end-to-end/api/autotranslate.ts (2)
479-499: 💤 Low valueConsider clarifying the test name.
The test name states "when messageId is not a string" but the actual test sends
{ _id: { $gt: '' } }as the first parameter. This tests both type validation (the_idproperty should be a string) and NoSQL injection prevention (rejecting MongoDB operators). Consider a more descriptive name such as "should fail when message parameter _id is not a string" or "should reject invalid types and operators in message _id" to better reflect what is being tested.🤖 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/autotranslate.ts` around lines 479 - 499, Rename the test's description string in the it(...) block that calls methodCall('autoTranslate.translateMessage') to more accurately reflect what's being asserted; for example change "should fail when messageId is not a string" to "should fail when message parameter _id is not a string" or "should reject invalid types and operators in message _id" so the intent of validating the message.params[0]._id (and rejecting MongoDB operator objects like { $gt: '' }) is clear.
437-523: ⚡ Quick winConsider adding a positive test case for the method endpoint.
The new test suite covers error scenarios (invalid input and unauthorized access) which aligns with the PR objectives. However, adding at least one positive test case where an authorized room member successfully calls the method with valid parameters would provide more complete coverage of the method endpoint's behavior and ensure the happy path works correctly.
Example test structure
it('should successfully translate message when user is a room member with valid parameters', (done) => { void request .post(methodCall('autoTranslate.translateMessage')) .set(credA) .send({ message: JSON.stringify({ msg: 'method', id: 'id', method: 'autoTranslate.translateMessage', params: [{ _id: privateMessage._id }, 'en'], }), }) .expect('Content-Type', 'application/json') .expect(200) .expect((res) => { expect(res.body).to.have.a.property('success', true); }) .end(done); });🤖 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/autotranslate.ts` around lines 437 - 523, Add a positive "happy path" test that verifies an authorized room member can successfully call autoTranslate.translateMessage with valid params: use the existing credA credentials and privateMessage._id (as used in the failing tests) to POST to methodCall('autoTranslate.translateMessage') with params [{ _id: privateMessage._id }, 'en'] and assert a 200 response and res.body.success === true; place this new it(...) alongside the other tests in the same describe block so it runs after the setup in before() and before teardown in after().
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@apps/meteor/tests/end-to-end/api/autotranslate.ts`:
- Around line 479-499: Rename the test's description string in the it(...) block
that calls methodCall('autoTranslate.translateMessage') to more accurately
reflect what's being asserted; for example change "should fail when messageId is
not a string" to "should fail when message parameter _id is not a string" or
"should reject invalid types and operators in message _id" so the intent of
validating the message.params[0]._id (and rejecting MongoDB operator objects
like { $gt: '' }) is clear.
- Around line 437-523: Add a positive "happy path" test that verifies an
authorized room member can successfully call autoTranslate.translateMessage with
valid params: use the existing credA credentials and privateMessage._id (as used
in the failing tests) to POST to methodCall('autoTranslate.translateMessage')
with params [{ _id: privateMessage._id }, 'en'] and assert a 200 response and
res.body.success === true; place this new it(...) alongside the other tests in
the same describe block so it runs after the setup in before() and before
teardown in after().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f92875-5ab8-4691-b178-ebf068de27cc
📒 Files selected for processing (1)
apps/meteor/tests/end-to-end/api/autotranslate.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx,js}
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.{ts,tsx,js}: Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Avoid code comments in the implementation
Files:
apps/meteor/tests/end-to-end/api/autotranslate.ts
🧠 Learnings (3)
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In the Rocket.Chat repository, do not reference Biome lint rules in code review feedback. Biome is not used even if biome.json exists; only reference Biome rules if there is explicit, project-wide usage documented. For TypeScript files, review lint implications without Biome guidance unless the project enables Biome rules.
Applied to files:
apps/meteor/tests/end-to-end/api/autotranslate.ts
📚 Learning: 2026-02-26T19:25:44.063Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:25:44.063Z
Learning: In this repository (RocketChat/Rocket.Chat), Biome lint rules are not used even if a biome.json exists. When reviewing TypeScript files (e.g., packages/ui-voip/src/providers/useMediaSession.ts), ensure lint suggestions do not reference Biome-specific rules. Rely on general ESLint/TypeScript lint rules and project conventions instead.
Applied to files:
apps/meteor/tests/end-to-end/api/autotranslate.ts
📚 Learning: 2026-05-06T12:21:44.083Z
Learnt from: juliajforesti
Repo: RocketChat/Rocket.Chat PR: 40256
File: apps/meteor/client/components/CreateDiscussion/CreateDiscussion.tsx:121-149
Timestamp: 2026-05-06T12:21:44.083Z
Learning: Field wrappers in rocket.chat/fuselage-forms (Field, FieldLabel, FieldRow, FieldError, FieldHint) auto-create htmlFor/id associations, aria-describedby, and role="alert" for errors. Do not manually set htmlFor, id, aria-describedby, or role attributes when using these wrappers. This automatic wiring does not apply to plain rocket.chat/fuselage components, which require explicit ID wiring per the accessibility docs. In code reviews, prefer using fuselage-forms wrappers for form fields and verify there is no unnecessary manual ID/aria wiring in files that use these wrappers. If a component uses plain fuselage components, ensure proper id wiring as per docs.
Applied to files:
apps/meteor/tests/end-to-end/api/autotranslate.ts
🔇 Additional comments (2)
apps/meteor/tests/end-to-end/api/autotranslate.ts (2)
6-6: LGTM!
501-522: LGTM!
jonasflorencio
added
the
stat: QA assured
dionisio-bot
Bot
added
the
stat: ready to merge
dionisio-bot
Bot
deleted the
validate-access-translatemessage-meteormethod
branch
|
/backport 8.4.2 |
|
Pull request #40539 added to Project: "Patch 8.4.2" |
|
/backport 8.3.4 |
|
Pull request #40540 added to Project: "Patch 8.3.4" |
|
/backport 8.2.4 |
|
Pull request #40541 added to Project: "Patch 8.2.4" |
|
/backport 8.1.5 |
|
Pull request #40543 added to Project: "Patch 8.1.5" |
|
/backport 8.0.6 |
|
Pull request #40544 added to Project: "Patch 8.0.6" |
|
/backport 7.13.7 |
|
7.13.7 already exists in the project |
|
/backport 7.13.8 |
|
Pull request #40545 added to Project: "Patch 7.13.8" |
|
/backport 7.10.12 |
|
Pull request #40546 added to Project: "Patch 7.10.12" |
3 participants
Proposed changes (including videos or screenshots)
Issue(s)
https://rocketchat.atlassian.net/browse/VLN-374
Steps to test or reproduce
Further comments
Summary by CodeRabbit
Bug Fixes
Tests