Closed
Conversation
|
Contributor
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds filename sanitization and tests, omits sensitive service fields from returned user data, makes getUserInfo optionally skip preferences (used by updated routes), restricts authentication to resume tokens, strengthens JWT generation/validation for file access and updates related tests, types, settings, i18n, and changesets. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant FileUpload as FileUpload
participant JWTHelper as JWTHelper
participant Settings as Settings
Client->>FileUpload: request generated file URL(s)
FileUpload->>Settings: check ProtectFiles & JWT secret configured
alt secret configured & JWT enabled
FileUpload->>JWTHelper: generateJWT({fileId,rid}, secret, aud?)
JWTHelper-->>FileUpload: token
FileUpload-->>Client: url(s) with token
else JWT not enabled or no secret
FileUpload-->>Client: url(s) without token
end
Note over Client,FileUpload: Later, Client requests file with token
Client->>FileUpload: GET /file/<id>?token=...
FileUpload->>JWTHelper: validateAndDecodeJWT(token, secret, aud?)
JWTHelper-->>FileUpload: payload or null
alt payload valid and matches fileId/rid
FileUpload-->>Client: serve file
else invalid token
FileUpload->>FileUpload: fallback to isAuthorizedByRoom()
FileUpload-->>Client: allow or deny based on room auth
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 @@
## master #38353 +/- ##
==========================================
+ Coverage 70.60% 70.65% +0.04%
==========================================
Files 3145 3145
Lines 108708 108708
Branches 19532 19520 -12
==========================================
+ Hits 76758 76808 +50
+ Misses 29946 29900 -46
+ Partials 2004 2000 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
📦 Docker Image Size Report📈 Changes
📊 Historical Trend---
config:
theme: "dark"
xyChart:
width: 900
height: 400
---
xychart
title "Image Size Evolution by Service (Last 30 Days + This PR)"
x-axis ["11/18 22:53", "11/19 23:02", "11/21 16:49", "11/24 17:34", "11/27 22:32", "11/28 19:05", "12/01 23:01", "12/02 21:57", "12/03 21:00", "12/04 18:17", "12/05 21:56", "12/08 20:15", "12/09 22:17", "12/10 23:26", "12/11 21:56", "12/12 22:45", "12/13 01:34", "12/15 22:31", "12/16 22:18", "12/17 21:04", "12/18 23:12", "12/19 23:27", "12/20 21:03", "12/22 18:54", "12/23 16:16", "12/24 19:38", "12/25 17:51", "12/26 13:18", "12/29 19:01", "12/30 20:52", "01/26 21:23 (PR)"]
y-axis "Size (GB)" 0 --> 0.5
line "account-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "authorization-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "ddp-streamer-service" [0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12, 0.12]
line "omnichannel-transcript-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "presence-service" [0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11, 0.11]
line "queue-worker-service" [0.14, 0.14, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13, 0.13]
line "rocketchat" [0.35, 0.35, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.34, 0.35]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Mon, 26 Jan 2026 21:23:08 GMT |
Co-authored-by: FlyeraX <79267723+jonasflorencio@users.noreply.github.com> Co-authored-by: Nazareno Bucciarelli <84046180+nazabucciarelli@users.noreply.github.com> Co-authored-by: Julio Araujo <julio.araujo@rocket.chat> Co-authored-by: Kevin Aleman <kaleman960@gmail.com> Co-authored-by: Nazareno Bucciarelli <bnazareno03@gmail.com>
Co-authored-by: FlyeraX <79267723+jonasflorencio@users.noreply.github.com> Co-authored-by: Nazareno Bucciarelli <84046180+nazabucciarelli@users.noreply.github.com> Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/file/server/functions/sanitizeFileName.ts">
<violation number="1" location="apps/meteor/app/file/server/functions/sanitizeFileName.ts:1">
P2: Redundant defensive checks obscure validation logic and incorrectly reject valid filenames like '..foo'. Simplify to exact matches for `.` and `..`.\n\n(Based on your team's feedback about preferring simple, sufficient validation and avoiding extra defensive layers that obscure the flow.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @@ -0,0 +1,19 @@ | |||
| import path from 'path'; | |||
Contributor
There was a problem hiding this comment.
P2: Redundant defensive checks obscure validation logic and incorrectly reject valid filenames like '..foo'. Simplify to exact matches for . and ...\n\n
(Based on your team's feedback about preferring simple, sufficient validation and avoiding extra defensive layers that obscure the flow.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/file/server/functions/sanitizeFileName.ts, line 1:
<comment>Redundant defensive checks obscure validation logic and incorrectly reject valid filenames like '..foo'. Simplify to exact matches for `.` and `..`.\n\n
(Based on your team's feedback about preferring simple, sufficient validation and avoiding extra defensive layers that obscure the flow.) </comment>
<file context>
@@ -0,0 +1,19 @@
+import path from 'path';
+
+export function sanitizeFileName(fileName: string) {
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts (1)
42-54: Suggest adding a direct backslash-as-character test and edge-case inputs.The
'..\\passwd'test verifies traversal behaviour but on POSIX it throws via thestartsWith('..')guard (line 10 of the implementation), not the character-allowlist check. A test with a non-traversal backslash filename (e.g.,'foo\\bar.txt') would directly exercise the character rejection path. Separately, direct inputs of'.','..', and''are handled by the implementation but have no corresponding tests.♻️ Suggested additions
describe('invalid characters', () => { it('should reject filenames with spaces', () => { expect(() => sanitizeFileName('my sound.mp3')).to.throw(); }); it('should reject filenames with special characters', () => { expect(() => sanitizeFileName('sound$.mp3')).to.throw(); }); it('should reject filenames with backslashes', () => { expect(() => sanitizeFileName('..\\passwd')).to.throw(); }); + + it('should reject filenames with backslash separators (non-traversal)', () => { + expect(() => sanitizeFileName('foo\\bar.txt')).to.throw(); + }); }); + + describe('edge cases', () => { + it('should reject the single dot "."', () => { + expect(() => sanitizeFileName('.')).to.throw(); + }); + + it('should reject the double dot ".."', () => { + expect(() => sanitizeFileName('..')).to.throw(); + }); + + it('should reject empty string', () => { + expect(() => sanitizeFileName('')).to.throw(); + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts` around lines 42 - 54, Add unit tests in sanitizeFileName.spec.ts that directly assert backslash characters are rejected by calling sanitizeFileName('foo\\bar.txt') and expecting an exception (this exercises the character-allowlist path rather than traversal), and add edge-case tests calling sanitizeFileName('.') , sanitizeFileName('..') and sanitizeFileName('') each expecting an error to cover the explicit guards for '.' and '' inputs; reference the existing describe block for 'invalid characters' and the sanitizeFileName function to place these new it() cases.apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
4-4: Optional: preferpath.posix.basenamefor explicit POSIX semantics in a security-sensitive function.
path.basenameis OS-native and treats backslash as a separator on Windows but not on POSIX. While the function is safe on both platforms (backslash traversal is caught bystartsWith('..')or the character-allowlist on POSIX, and bybase !== fileNameon Windows), usingpath.posix.basenamemakes the intended behaviour explicit and removes any OS-dependent divergence from a security control.♻️ Proposed change
-import path from 'path'; +import path from 'path'; export function sanitizeFileName(fileName: string) { - const base = path.basename(fileName); + const base = path.posix.basename(fileName);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/file/server/functions/sanitizeFileName.ts` at line 4, Replace the OS-native call to path.basename with the POSIX-specific variant in the sanitizeFileName function: change the expression that assigns base (currently using path.basename(fileName)) to use path.posix.basename(fileName) instead; ensure any imports still reference the Node 'path' module so path.posix is available and run tests that exercise sanitizeFileName to confirm behavior remains correct.ee/apps/ddp-streamer/src/configureServer.ts (1)
89-91: No-optry/catchcan be removed.The catch block unconditionally re-throws without logging or transforming the error, making it dead code. Drop the
try/catchwrapper entirely.♻️ Proposed cleanup
- try { - const result = await Account.login({ resume }); - if (!result) { - throw new MeteorError(403, "You've been logged out by the server. Please log in again"); - } - - this.userId = result.uid; - this.userToken = result.hashedToken; - this.connection.loginToken = result.hashedToken; - - this.emit(DDP_EVENTS.LOGGED); - - server.emit(DDP_EVENTS.LOGGED, this); - - return { - id: result.uid, - token: result.token, - tokenExpires: result.tokenExpires, - type: result.type, - }; - } catch (error) { - throw error; - } + const result = await Account.login({ resume }); + if (!result) { + throw new MeteorError(403, "You've been logged out by the server. Please log in again"); + } + + this.userId = result.uid; + this.userToken = result.hashedToken; + this.connection.loginToken = result.hashedToken; + + this.emit(DDP_EVENTS.LOGGED); + + server.emit(DDP_EVENTS.LOGGED, this); + + return { + id: result.uid, + token: result.token, + tokenExpires: result.tokenExpires, + type: result.type, + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ee/apps/ddp-streamer/src/configureServer.ts` around lines 89 - 91, Remove the no-op try/catch in configureServer.ts: locate the try/catch that simply does "catch (error) { throw error; }" (inside the configureServer function or the exported initializer in that file) and delete the entire try/catch wrapper so the original code runs without an unconditional rethrow; ensure any needed error behavior is preserved by letting errors bubble naturally or add handling only if you intend to log/transform them.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts`:
- Around line 42-54: Add unit tests in sanitizeFileName.spec.ts that directly
assert backslash characters are rejected by calling
sanitizeFileName('foo\\bar.txt') and expecting an exception (this exercises the
character-allowlist path rather than traversal), and add edge-case tests calling
sanitizeFileName('.') , sanitizeFileName('..') and sanitizeFileName('') each
expecting an error to cover the explicit guards for '.' and '' inputs; reference
the existing describe block for 'invalid characters' and the sanitizeFileName
function to place these new it() cases.
In `@apps/meteor/app/file/server/functions/sanitizeFileName.ts`:
- Line 4: Replace the OS-native call to path.basename with the POSIX-specific
variant in the sanitizeFileName function: change the expression that assigns
base (currently using path.basename(fileName)) to use
path.posix.basename(fileName) instead; ensure any imports still reference the
Node 'path' module so path.posix is available and run tests that exercise
sanitizeFileName to confirm behavior remains correct.
In `@ee/apps/ddp-streamer/src/configureServer.ts`:
- Around line 89-91: Remove the no-op try/catch in configureServer.ts: locate
the try/catch that simply does "catch (error) { throw error; }" (inside the
configureServer function or the exported initializer in that file) and delete
the entire try/catch wrapper so the original code runs without an unconditional
rethrow; ensure any needed error behavior is preserved by letting errors bubble
naturally or add handling only if you intend to log/transform them.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.changeset/twelve-meals-develop.mdapps/meteor/app/file/server/file.server.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.tsapps/meteor/app/file/server/functions/sanitizeFileName.tsee/apps/account-service/src/Account.tsee/apps/account-service/src/lib/loginViaResume.tsee/apps/account-service/src/lib/loginViaUsername.tsee/apps/account-service/tsconfig.jsonee/apps/ddp-streamer/src/configureServer.tspackages/core-services/src/types/IAccount.ts
💤 Files with no reviewable changes (1)
- ee/apps/account-service/src/lib/loginViaUsername.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
packages/core-services/src/types/IAccount.tsee/apps/account-service/src/Account.tsee/apps/ddp-streamer/src/configureServer.tsee/apps/account-service/src/lib/loginViaResume.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.tsapps/meteor/app/file/server/file.server.tsapps/meteor/app/file/server/functions/sanitizeFileName.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/keychain.ts:148-156
Timestamp: 2025-10-16T21:09:51.816Z
Learning: In the RocketChat/Rocket.Chat repository, only platforms with native crypto.randomUUID() support are targeted, so fallback implementations for crypto.randomUUID() are not required in E2EE or cryptographic code.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 Learning: 2026-01-27T20:57:56.529Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat PR: 38294
File: apps/meteor/server/hooks/sauMonitorHooks.ts:0-0
Timestamp: 2026-01-27T20:57:56.529Z
Learning: In Rocket.Chat, the `accounts.login` event listened to by DeviceManagementService is only broadcast when running in microservices mode (via DDPStreamer), not in monolith mode. The `Accounts.onLogin` hook in sauMonitorHooks.ts runs in monolith deployments. These are mutually exclusive deployment modes, so there's no duplication of event emissions between these two code paths.
Applied to files:
ee/apps/ddp-streamer/src/configureServer.ts
📚 Learning: 2026-01-17T01:51:47.764Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38219
File: packages/core-typings/src/cloud/Announcement.ts:5-6
Timestamp: 2026-01-17T01:51:47.764Z
Learning: In packages/core-typings/src/cloud/Announcement.ts, the AnnouncementSchema.createdBy field intentionally overrides IBannerSchema.createdBy (object with _id and optional username) with a string enum ['cloud', 'system'] to match existing runtime behavior. This is documented as technical debt with a FIXME comment at apps/meteor/app/cloud/server/functions/syncWorkspace/handleCommsSync.ts:53 and should not be flagged as an error until the runtime behavior is corrected.
Applied to files:
ee/apps/ddp-streamer/src/configureServer.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.tsee/apps/account-service/tsconfig.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.tsee/apps/account-service/tsconfig.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.tsee/apps/account-service/tsconfig.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/file/server/file.server.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
ee/apps/account-service/tsconfig.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
ee/apps/account-service/tsconfig.json
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
ee/apps/account-service/tsconfig.json
🧬 Code graph analysis (5)
packages/core-services/src/types/IAccount.ts (1)
packages/core-services/src/index.ts (1)
IServiceClass(61-61)
ee/apps/account-service/src/Account.ts (1)
packages/core-services/src/types/IAccount.ts (1)
ILoginResult(3-9)
ee/apps/ddp-streamer/src/configureServer.ts (1)
ee/apps/account-service/src/Account.ts (1)
Account(9-47)
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts (1)
apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
sanitizeFileName(3-19)
apps/meteor/app/file/server/file.server.ts (1)
apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
sanitizeFileName(3-19)
🔇 Additional comments (7)
.changeset/twelve-meals-develop.md (1)
1-5: LGTM!Changeset entry is correctly formatted and accurately describes the security hotfix patch.
ee/apps/account-service/src/lib/loginViaResume.ts (1)
11-21: LGTM!Adding
active: trueto the query filter is the correct pattern — it enforces the constraint at the DB level and is fail-closed (documents missing theactivefield won't match). The_idfield is implicitly projected so theuidreturn is safe.ee/apps/account-service/tsconfig.json (1)
15-17: LGTM!Widening the
includeglob to./src/**/*.tsis appropriate given the changes to source files in this directory.apps/meteor/app/file/server/file.server.ts (1)
13-14: LGTM!
sanitizeFileNameis correctly scoped to theFileSystemclass (filesystem-backed storage) and intentionally not applied toGridFS, which uses MongoDB GridFS where filenames are opaque identifiers, not OS paths.ee/apps/account-service/src/Account.ts (1)
30-36: LGTM!The updated signature correctly implements the narrowed
IAccount.logincontract. Theif (resume)guard is appropriate as a defence against an empty string being passed at runtime.packages/core-services/src/types/IAccount.ts (1)
12-12: AllIAccountimplementors have been updated to the new interface.The breaking change to
IAccount.loginis complete: only one implementor exists (Account.ts), it has been updated to accept only{ resume }, and all call sites (includingconfigureServer.ts) have been updated accordingly. No orphaned implementations or call sites with the old signature (user/password) remain.ee/apps/ddp-streamer/src/configureServer.ts (1)
68-70: LGTM — login signature correctly narrowed to resume-only.The destructuring aligns with
Account.login({ resume }: { resume: string })inee/apps/account-service/src/Account.ts, and the runtime safety net (if (resume)→ returnsfalse→MeteorError(403)) keeps the behaviour well-defined even if a client omits the field.
Signed-off-by: Abhinav Kumar <abhinav@avitechlab.com> Co-authored-by: Julio Araujo <julio.araujo@rocket.chat> Co-authored-by: Abhinav Kumar <abhinav@avitechlab.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/meteor/app/utils/server/lib/JWTHelper.ts (1)
39-46: Tokens withoutexp/nbfclaims bypass time-based validation.The
typeofguards on lines 40 and 44 mean a JWT that omitsexpentirely will never be considered expired. SincegenerateJWTalways sets these claims, this only matters if externally crafted (but validly signed) tokens are a concern. If that's intentional, it's fine — just calling it out for awareness.If you want to enforce mandatory expiration:
- if (typeof payload.exp === 'number' && now >= payload.exp) { + if (typeof payload.exp !== 'number' || now >= payload.exp) { return null; } - if (typeof payload.nbf === 'number' && now < payload.nbf) { + if (typeof payload.nbf !== 'number' || now < payload.nbf) { return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/app/utils/server/lib/JWTHelper.ts` around lines 39 - 46, The current guards (typeof payload.exp === 'number' and typeof payload.nbf === 'number') allow tokens that omit exp/nbf to bypass time validation; to enforce mandatory expiration and not accept externally crafted tokens without these claims, change the checks in the JWTHelper validation logic so missing or non-number claims are treated as invalid (e.g., make the exp check fail if payload.exp is not a number or is in the past, and likewise fail if payload.nbf is not a number or is in the future). Update the validation code that references payload.exp and payload.nbf (and consider the generateJWT function which currently sets these claims) so tokens lacking numeric exp/nbf return null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/app/utils/server/lib/JWTHelper.spec.ts`:
- Around line 274-288: The test uses hardcoded past timestamps so
validateAndDecodeJWT returns null for expiration instead of missing context;
update the stubbed parsed payload in the spec (the object returned by
jsrsasignStub.KJUR.jws.JWS.parse) to use future-relative iat/nbf/exp values
(e.g., Date.now()/1000 + offset) so the token is not expired and the call to
validateAndDecodeJWT reaches the payload.context check; ensure the payload still
omits context to verify that validateAndDecodeJWT returns null for missing
context.
In `@packages/i18n/src/locales/fr.i18n.json`:
- Around line 1557-1559: The new French-only translation key
"FileUpload_Enable_json_web_token_for_files_Alert" was added directly to the fr
locale which breaks the repo i18n workflow; either remove this key from
packages/i18n/src/locales/fr.i18n.json and instead add the new key (and its
English text) to en.i18n.json so the translation pipeline can propagate it, or
if you intentionally need a locale-specific override, add a short note to the PR
justifying bypassing the pipeline and keep the key removal from fr unless that
justification is accepted.
---
Nitpick comments:
In `@apps/meteor/app/utils/server/lib/JWTHelper.ts`:
- Around line 39-46: The current guards (typeof payload.exp === 'number' and
typeof payload.nbf === 'number') allow tokens that omit exp/nbf to bypass time
validation; to enforce mandatory expiration and not accept externally crafted
tokens without these claims, change the checks in the JWTHelper validation logic
so missing or non-number claims are treated as invalid (e.g., make the exp check
fail if payload.exp is not a number or is in the past, and likewise fail if
payload.nbf is not a number or is in the future). Update the validation code
that references payload.exp and payload.nbf (and consider the generateJWT
function which currently sets these claims) so tokens lacking numeric exp/nbf
return null.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/meteor/.mocharc.jsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/utils/server/lib/JWTHelper.tsapps/meteor/server/settings/file-upload.tspackages/i18n/src/locales/en.i18n.jsonpackages/i18n/src/locales/es.i18n.jsonpackages/i18n/src/locales/fr.i18n.jsonpackages/i18n/src/locales/pt-BR.i18n.json
✅ Files skipped from review due to trivial changes (2)
- packages/i18n/src/locales/pt-BR.i18n.json
- packages/i18n/src/locales/es.i18n.json
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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/server/settings/file-upload.tsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.tsapps/meteor/app/utils/server/lib/JWTHelper.ts
**/*.spec.ts
📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)
**/*.spec.ts: Use descriptive test names that clearly communicate expected behavior in Playwright tests
Use.spec.tsextension for test files (e.g.,login.spec.ts)
Files:
apps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
🧠 Learnings (21)
📓 Common learnings
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:06.802Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/keychain.ts:148-156
Timestamp: 2025-10-16T21:09:51.816Z
Learning: In the RocketChat/Rocket.Chat repository, only platforms with native crypto.randomUUID() support are targeted, so fallback implementations for crypto.randomUUID() are not required in E2EE or cryptographic code.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 Learning: 2025-11-19T12:32:29.696Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
Applied to files:
packages/i18n/src/locales/en.i18n.jsonpackages/i18n/src/locales/fr.i18n.json
📚 Learning: 2026-02-12T15:39:28.416Z
Learnt from: dougfabris
Repo: RocketChat/Rocket.Chat PR: 32703
File: apps/meteor/client/lib/chats/flows/uploadFiles.ts:52-58
Timestamp: 2026-02-12T15:39:28.416Z
Learning: In `apps/meteor/client/lib/chats/flows/uploadFiles.ts`, when E2E encryption is required but not allowed (e.g., `E2E_Enable_Encrypt_Files` setting is disabled), the function intentionally abandons the entire upload queue and displays a toast error. This fail-fast behavior prevents partial uploads when encryption requirements cannot be met and is the expected behavior, not a bug.
Applied to files:
packages/i18n/src/locales/en.i18n.jsonapps/meteor/server/settings/file-upload.tsapps/meteor/app/file-upload/server/lib/FileUpload.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `expect` matchers for assertions (`toEqual`, `toContain`, `toBeTruthy`, `toHaveLength`, etc.) instead of `assert` statements in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Store commonly used locators in variables/constants for reuse
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/.mocharc.jsapps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.{ts,tsx,js} : Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests
Applied to files:
apps/meteor/.mocharc.js
📚 Learning: 2025-09-19T15:15:04.642Z
Learnt from: rodrigok
Repo: RocketChat/Rocket.Chat PR: 36991
File: apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts:219-221
Timestamp: 2025-09-19T15:15:04.642Z
Learning: The Federation_Matrix_homeserver_domain setting in apps/meteor/server/services/federation/infrastructure/rocket-chat/adapters/Settings.ts is part of the old federation system and is being deprecated/removed, so configuration issues with this setting should not be flagged for improvement.
Applied to files:
apps/meteor/server/settings/file-upload.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/app/utils/server/lib/JWTHelper.spec.tsapps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.beforeAll()` and `test.afterAll()` for setup/teardown in Playwright tests
Applied to files:
apps/meteor/app/utils/server/lib/JWTHelper.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to **/*.spec.ts : Use descriptive test names that clearly communicate expected behavior in Playwright tests
Applied to files:
apps/meteor/app/utils/server/lib/JWTHelper.spec.ts
📚 Learning: 2025-10-06T20:30:45.540Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37152
File: packages/apps-engine/tests/test-data/storage/storage.ts:101-122
Timestamp: 2025-10-06T20:30:45.540Z
Learning: In `packages/apps-engine/tests/test-data/storage/storage.ts`, the stub methods (updatePartialAndReturnDocument, updateStatus, updateSetting, updateAppInfo, updateMarketplaceInfo) intentionally throw "Method not implemented." Tests using these methods must stub them using `SpyOn` from the test library rather than relying on actual implementations.
Applied to files:
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2026-01-15T22:03:35.587Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38071
File: apps/meteor/app/apps/server/bridges/listeners.ts:257-271
Timestamp: 2026-01-15T22:03:35.587Z
Learning: In the file upload pipeline (apps/meteor/app/apps/server/bridges/listeners.ts), temporary files are created by the server in the same filesystem, so symlinks between temp files are safe and don't require cross-filesystem fallbacks.
Applied to files:
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure clean state for each test execution in Playwright tests
Applied to files:
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts
🧬 Code graph analysis (3)
apps/meteor/app/utils/server/lib/JWTHelper.spec.ts (1)
apps/meteor/app/utils/server/lib/JWTHelper.ts (2)
generateJWT(8-20)validateAndDecodeJWT(22-56)
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts (1)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
FileUpload(110-709)
apps/meteor/app/file-upload/server/lib/FileUpload.ts (1)
apps/meteor/app/utils/server/lib/JWTHelper.ts (1)
validateAndDecodeJWT(22-56)
🔇 Additional comments (10)
apps/meteor/.mocharc.js (1)
31-33: LGTM — new Mocha spec globs look consistent with existing patterns.packages/i18n/src/locales/en.i18n.json (1)
2266-2266: Clear, user-facing guidance for JWT dependency.
The wording is concise and explains the prerequisite clearly.apps/meteor/server/settings/file-upload.ts (1)
68-77: LGTM: alert metadata wiring looks correct.apps/meteor/app/utils/server/lib/JWTHelper.ts (1)
22-55: Solid implementation of JWT validation with proper layered checks.The verify-then-parse order, algorithm pinning to
HS256, and separate guards forexp,nbf, andaudare all well-structured.apps/meteor/app/utils/server/lib/JWTHelper.spec.ts (2)
1-98: Good test coverage forgenerateJWTand overall test structure.The proxyquire-based stub isolation,
beforeEachresets, and descriptive test names all look solid.
100-261: Thorough coverage ofvalidateAndDecodeJWTedge cases.Null/empty inputs, signature failures, time-based claims, audience matching, parse errors, and custom audience scenarios are all well-covered.
apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts (2)
131-289: Comprehensive test coverage forrequestCanAccessFilesJWT flow.The tests methodically cover protection-disabled bypass, missing URL, token absence, secret misconfiguration, invalid tokens, payload mismatches, missing file objects, and the happy path — all with appropriate assertions on stub invocations.
14-74: Clean stub setup and teardown for the new dependencies.The new stubs (
usersModelStub,subscriptionsModelStub,validateAndDecodeJWTStub,systemLoggerStub,roomCoordinatorStub) are properly wired through proxyquire and reset inbeforeEach.apps/meteor/app/file-upload/server/lib/FileUpload.ts (2)
461-491: Well-structured JWT authorization with proper layered guards.The validation flow (feature toggle → secret presence → JWT decode → payload field checks → file/room matching) is thorough and fails safely at each stage. The
file?.optional chaining on line 483 correctly handles the undefined-file edge case.
667-683: Good:generateJWTToFileUrlsnow also requires the secret to be configured.This ensures JWT generation and validation are symmetrically guarded, preventing tokens from being generated when the secret isn't set.
Comment on lines
+274
to
+288
| it('should return null if context is missing from payload', () => { | ||
| jsrsasignStub.KJUR.jws.JWS.verify.returns(true); | ||
| jsrsasignStub.KJUR.jws.JWS.parse.returns({ | ||
| payloadObj: { | ||
| iat: 1609459200, | ||
| nbf: 1609459200, | ||
| exp: 1609462800, | ||
| aud: 'RocketChat', | ||
| }, | ||
| }); | ||
|
|
||
| const result = validateAndDecodeJWT('jwt.token', 'secret'); | ||
|
|
||
| expect(result).to.be.null; | ||
| }); |
Contributor
There was a problem hiding this comment.
Test passes for the wrong reason — hardcoded timestamps are in the past.
The exp: 1609462800 (Jan 1, 2021) is before the current time, so validateAndDecodeJWT returns null because the token is expired (line 40 in JWTHelper.ts), not because context is missing from the payload. The test name and intent don't match the actual failure path exercised.
Use future-relative timestamps (like other tests in this file) so the function actually reaches the payload.context || null check on line 52.
Proposed fix
it('should return null if context is missing from payload', () => {
jsrsasignStub.KJUR.jws.JWS.verify.returns(true);
jsrsasignStub.KJUR.jws.JWS.parse.returns({
payloadObj: {
- iat: 1609459200,
- nbf: 1609459200,
- exp: 1609462800,
+ iat: Math.floor(Date.now() / 1000) - 60,
+ nbf: Math.floor(Date.now() / 1000) - 60,
+ exp: Math.floor(Date.now() / 1000) + 3600,
aud: 'RocketChat',
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('should return null if context is missing from payload', () => { | |
| jsrsasignStub.KJUR.jws.JWS.verify.returns(true); | |
| jsrsasignStub.KJUR.jws.JWS.parse.returns({ | |
| payloadObj: { | |
| iat: 1609459200, | |
| nbf: 1609459200, | |
| exp: 1609462800, | |
| aud: 'RocketChat', | |
| }, | |
| }); | |
| const result = validateAndDecodeJWT('jwt.token', 'secret'); | |
| expect(result).to.be.null; | |
| }); | |
| it('should return null if context is missing from payload', () => { | |
| jsrsasignStub.KJUR.jws.JWS.verify.returns(true); | |
| jsrsasignStub.KJUR.jws.JWS.parse.returns({ | |
| payloadObj: { | |
| iat: Math.floor(Date.now() / 1000) - 60, | |
| nbf: Math.floor(Date.now() / 1000) - 60, | |
| exp: Math.floor(Date.now() / 1000) + 3600, | |
| aud: 'RocketChat', | |
| }, | |
| }); | |
| const result = validateAndDecodeJWT('jwt.token', 'secret'); | |
| expect(result).to.be.null; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/app/utils/server/lib/JWTHelper.spec.ts` around lines 274 - 288,
The test uses hardcoded past timestamps so validateAndDecodeJWT returns null for
expiration instead of missing context; update the stubbed parsed payload in the
spec (the object returned by jsrsasignStub.KJUR.jws.JWS.parse) to use
future-relative iat/nbf/exp values (e.g., Date.now()/1000 + offset) so the token
is not expired and the call to validateAndDecodeJWT reaches the payload.context
check; ensure the payload still omits context to verify that
validateAndDecodeJWT returns null for missing context.
Comment on lines
1557
to
+1559
| "FileUpload_Enable_json_web_token_for_files": "Activer la protection des jetons Web JSON pour le chargement de fichiers", | ||
| "FileUpload_Enable_json_web_token_for_files_description": "Ajoute un jeton Web JSON aux URL des fichiers chargés", | ||
| "FileUpload_Enable_json_web_token_for_files_Alert": "Cette option n'a d'effet que si un secret JWT est configuré. Si le champ \"Secret JWT pour les fichiers\" est vide, aucun jeton ne sera généré et la protection JWT des fichiers ne sera pas appliquée.", |
Contributor
There was a problem hiding this comment.
Confirm non‑English locale addition aligns with the i18n workflow.
Line 1559 adds a new key directly in the French locale. The repo workflow expects new keys to land in en.i18n.json only, with other locales handled by the translation pipeline/fallback. Please remove this entry or confirm that bypassing the pipeline is intended to avoid divergence.
🧹 Proposed adjustment
- "FileUpload_Enable_json_web_token_for_files_Alert": "Cette option n'a d'effet que si un secret JWT est configuré. Si le champ \"Secret JWT pour les fichiers\" est vide, aucun jeton ne sera généré et la protection JWT des fichiers ne sera pas appliquée.",Based on learnings: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "FileUpload_Enable_json_web_token_for_files": "Activer la protection des jetons Web JSON pour le chargement de fichiers", | |
| "FileUpload_Enable_json_web_token_for_files_description": "Ajoute un jeton Web JSON aux URL des fichiers chargés", | |
| "FileUpload_Enable_json_web_token_for_files_Alert": "Cette option n'a d'effet que si un secret JWT est configuré. Si le champ \"Secret JWT pour les fichiers\" est vide, aucun jeton ne sera généré et la protection JWT des fichiers ne sera pas appliquée.", | |
| "FileUpload_Enable_json_web_token_for_files": "Activer la protection des jetons Web JSON pour le chargement de fichiers", | |
| "FileUpload_Enable_json_web_token_for_files_description": "Ajoute un jeton Web JSON aux URL des fichiers chargés", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/i18n/src/locales/fr.i18n.json` around lines 1557 - 1559, The new
French-only translation key "FileUpload_Enable_json_web_token_for_files_Alert"
was added directly to the fr locale which breaks the repo i18n workflow; either
remove this key from packages/i18n/src/locales/fr.i18n.json and instead add the
new key (and its English text) to en.i18n.json so the translation pipeline can
propagate it, or if you intentionally need a locale-specific override, add a
short note to the PR justifying bypassing the pipeline and keep the key removal
from fr unless that justification is accepted.
Contributor
There was a problem hiding this comment.
2 issues found across 10 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/app/utils/server/lib/JWTHelper.ts">
<violation number="1" location="apps/meteor/app/utils/server/lib/JWTHelper.ts:40">
P2: A JWT missing the `exp` claim (or with a non-numeric `exp`) bypasses the expiry check entirely and is treated as never-expiring. Consider requiring `exp` to be present and be a number, returning `null` if it isn't, to prevent accepting tokens that should have been rejected.</violation>
</file>
<file name="apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts">
<violation number="1" location="apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts:215">
P3: This test is practically identical to the preceding test and should be removed to reduce duplicate code.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const payload = decoded.payloadObj as Record<string, any>; | ||
|
|
||
| const now = Math.floor(Date.now() / 1000); | ||
| if (typeof payload.exp === 'number' && now >= payload.exp) { |
Contributor
There was a problem hiding this comment.
P2: A JWT missing the exp claim (or with a non-numeric exp) bypasses the expiry check entirely and is treated as never-expiring. Consider requiring exp to be present and be a number, returning null if it isn't, to prevent accepting tokens that should have been rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/utils/server/lib/JWTHelper.ts, line 40:
<comment>A JWT missing the `exp` claim (or with a non-numeric `exp`) bypasses the expiry check entirely and is treated as never-expiring. Consider requiring `exp` to be present and be a number, returning `null` if it isn't, to prevent accepting tokens that should have been rejected.</comment>
<file context>
@@ -19,10 +19,38 @@ export const generateJWT = (payload: Record<string, any>, secret: string): strin
+ const payload = decoded.payloadObj as Record<string, any>;
+
+ const now = Math.floor(Date.now() / 1000);
+ if (typeof payload.exp === 'number' && now >= payload.exp) {
+ return null;
+ }
</file context>
Suggested change
| if (typeof payload.exp === 'number' && now >= payload.exp) { | |
| if (typeof payload.exp !== 'number' || now >= payload.exp) { |
| expect(validateAndDecodeJWTStub.calledOnce).to.be.true; | ||
| }); | ||
|
|
||
| it('should deny access if token is invalid or payload cannot be decoded', async () => { |
Contributor
There was a problem hiding this comment.
P3: This test is practically identical to the preceding test and should be removed to reduce duplicate code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/app/file-upload/server/lib/FileUpload.spec.ts, line 215:
<comment>This test is practically identical to the preceding test and should be removed to reduce duplicate code.</comment>
<file context>
@@ -100,4 +127,165 @@ describe('FileUpload', () => {
+ expect(validateAndDecodeJWTStub.calledOnce).to.be.true;
+ });
+
+ it('should deny access if token is invalid or payload cannot be decoded', async () => {
+ settingsGetMap.set('FileUpload_ProtectFiles', true);
+ settingsGetMap.set('FileUpload_Enable_json_web_token_for_files', true);
</file context>
Co-authored-by: Nazareno Bucciarelli <84046180+nazabucciarelli@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/tests/end-to-end/api/incoming-integrations.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/incoming-integrations.ts:517">
P2: The test assertion is too weak: it only checks that `scriptCompiled` exists, not that compilation succeeded. Assert it is `true` to actually validate the intended behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }) | ||
| .expect(200); | ||
|
|
||
| expect(res.body.integration).to.have.property('scriptCompiled'); |
Contributor
There was a problem hiding this comment.
P2: The test assertion is too weak: it only checks that scriptCompiled exists, not that compilation succeeded. Assert it is true to actually validate the intended behavior.
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/incoming-integrations.ts, line 517:
<comment>The test assertion is too weak: it only checks that `scriptCompiled` exists, not that compilation succeeded. Assert it is `true` to actually validate the intended behavior.</comment>
<file context>
@@ -473,6 +473,53 @@ describe('[Incoming Integrations]', () => {
+ })
+ .expect(200);
+
+ expect(res.body.integration).to.have.property('scriptCompiled');
+ expect(res.body.integration).to.not.have.property('scriptError');
+
</file context>
Suggested change
| expect(res.body.integration).to.have.property('scriptCompiled'); | |
| expect(res.body.integration).to.have.property('scriptCompiled', true); |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts`:
- Around line 480-482: The test setup grants only the narrower permission but
never revokes the broader one, so update the test to explicitly remove the
'manage-incoming-integrations' permission before exercising the "manage-own"
path and restore original permissions after the test; specifically use
updatePermission('manage-incoming-integrations', []) to revoke the broader
permission in the same before block (and/or in a beforeEach where used), and add
an after or afterEach that resets both 'manage-own-incoming-integrations' and
'manage-incoming-integrations' back to their original roles to avoid leaking
state.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/blue-pugs-greet.md.github/actions/meteor-build/action.ymlapps/meteor/tests/end-to-end/api/incoming-integrations.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: new-release
- GitHub Check: cubic · AI code reviewer
- GitHub Check: CodeQL-Build
🧰 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/incoming-integrations.ts
🧠 Learnings (20)
📓 Common learnings
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Learnt from: ggazzo
Repo: RocketChat/Rocket.Chat PR: 35995
File: apps/meteor/app/api/server/v1/rooms.ts:1107-1112
Timestamp: 2026-02-23T17:53:18.785Z
Learning: In Rocket.Chat PR reviews, maintain strict scope boundaries—when a PR is focused on a specific endpoint (e.g., rooms.favorite), avoid reviewing or suggesting changes to other endpoints that were incidentally refactored (e.g., rooms.invite) unless explicitly requested by maintainers.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:26:01.702Z
Learning: The RocketChat/Rocket.Chat project does not use Biome for linting, despite the presence of a biome.json file in the repository. Lint-related suggestions should not reference Biome rules.
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:26:01.702Z
Learning: The RocketChat/Rocket.Chat project does not use Biome for linting, despite the presence of a biome.json file in the repository. Lint-related suggestions should not reference Biome rules.
Learnt from: cardoso
Repo: RocketChat/Rocket.Chat PR: 36942
File: apps/meteor/client/lib/e2ee/keychain.ts:148-156
Timestamp: 2025-10-16T21:09:51.816Z
Learning: In the RocketChat/Rocket.Chat repository, only platforms with native crypto.randomUUID() support are targeted, so fallback implementations for crypto.randomUUID() are not required in E2EE or cryptographic code.
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 37547
File: packages/i18n/src/locales/en.i18n.json:634-634
Timestamp: 2025-11-19T12:32:29.696Z
Learning: Repo: RocketChat/Rocket.Chat
Context: i18n workflow
Learning: In this repository, new translation keys should be added to packages/i18n/src/locales/en.i18n.json only; other locale files are populated via the external translation pipeline and/or fall back to English. Do not request adding the same key to all locale files in future reviews.
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: Rocket.Chat repo context: When a workspace manifest on develop already pins a dependency version (e.g., packages/web-ui-registration → "rocket.chat/ui-contexts": "27.0.1"), a lockfile change in a feature PR that upgrades only that dependency’s resolution is considered a manifest-driven sync and can be kept, preferably as a small "chore: sync yarn.lock with manifests" commit.
Applied to files:
.changeset/blue-pugs-greet.md.github/actions/meteor-build/action.yml
📚 Learning: 2026-02-24T19:09:09.561Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38974
File: apps/meteor/app/api/server/v1/im.ts:220-221
Timestamp: 2026-02-24T19:09:09.561Z
Learning: In RocketChat/Rocket.Chat OpenAPI migration PRs for apps/meteor/app/api/server/v1 endpoints, maintainers prefer to avoid any logic changes; style-only cleanups (like removing inline comments) may be deferred to follow-ups to keep scope tight.
Applied to files:
.changeset/blue-pugs-greet.md.github/actions/meteor-build/action.yml
📚 Learning: 2026-02-24T19:05:56.710Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 0
File: :0-0
Timestamp: 2026-02-24T19:05:56.710Z
Learning: In Rocket.Chat PRs, keep feature PRs free of unrelated lockfile-only dependency bumps; prefer reverting lockfile drift or isolating such bumps into a separate "chore" commit/PR, and always use yarn install --immutable with the Yarn version pinned in package.json via Corepack.
Applied to files:
.changeset/blue-pugs-greet.md.github/actions/meteor-build/action.yml
📚 Learning: 2026-02-25T20:10:16.987Z
Learnt from: ahmed-n-abdeltwab
Repo: RocketChat/Rocket.Chat PR: 38913
File: packages/ddp-client/src/legacy/types/SDKLegacy.ts:34-34
Timestamp: 2026-02-25T20:10:16.987Z
Learning: In the RocketChat/Rocket.Chat monorepo, packages/ddp-client and apps/meteor do not use TypeScript project references. Module augmentations in apps/meteor (e.g., declare module 'rocket.chat/rest-typings') are not visible when compiling packages/ddp-client in isolation, which is why legacy SDK methods that depend on OperationResult types for OpenAPI-migrated endpoints must remain commented out.
Applied to files:
.changeset/blue-pugs-greet.md.github/actions/meteor-build/action.yml
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : All test files must be created in `apps/meteor/tests/e2e/` directory
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Utilize Playwright fixtures (`test`, `page`, `expect`) for consistency in test files
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Group related tests in the same file
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Ensure tests run reliably in parallel without shared state conflicts
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Use `test.step()` for complex test scenarios to improve organization in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Maintain test isolation between test cases in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-12-10T21:00:54.909Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:54.909Z
Learning: Rocket.Chat monorepo: Jest testMatch pattern '<rootDir>/src/**/*.spec.(ts|js|mjs)' is valid in this repo and used across multiple packages (e.g., packages/tools, ee/packages/omnichannel-services). Do not flag it as invalid in future reviews.
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.{ts,spec.ts} : Follow Page Object Model pattern consistently in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/page-objects/**/*.ts : Utilize existing page objects pattern from `apps/meteor/tests/e2e/page-objects/`
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.ts
📚 Learning: 2025-11-24T17:08:17.065Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/playwright.mdc:0-0
Timestamp: 2025-11-24T17:08:17.065Z
Learning: Applies to apps/meteor/tests/e2e/**/*.spec.ts : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/tests/end-to-end/api/incoming-integrations.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 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/incoming-integrations.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/incoming-integrations.ts
📚 Learning: 2026-02-26T19:26:01.702Z
Learnt from: gabriellsh
Repo: RocketChat/Rocket.Chat PR: 38778
File: packages/ui-voip/src/providers/useMediaSession.ts:192-192
Timestamp: 2026-02-26T19:26:01.702Z
Learning: The RocketChat/Rocket.Chat project does not use Biome for linting, despite the presence of a biome.json file in the repository. Lint-related suggestions should not reference Biome rules.
Applied to files:
.github/actions/meteor-build/action.yml
📚 Learning: 2026-01-08T15:03:59.621Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38068
File: apps/meteor/tests/data/apps/app-packages/README.md:14-16
Timestamp: 2026-01-08T15:03:59.621Z
Learning: For the RocketChat/Rocket.Chat repository, do not analyze or report formatting issues (such as hard tabs vs spaces, line breaks, etc.). The project relies on automated linting tools to enforce formatting standards.
Applied to files:
.github/actions/meteor-build/action.yml
📚 Learning: 2025-11-10T19:06:20.146Z
Learnt from: MartinSchoeler
Repo: RocketChat/Rocket.Chat PR: 37408
File: apps/meteor/client/views/admin/ABAC/useRoomAttributeOptions.tsx:53-69
Timestamp: 2025-11-10T19:06:20.146Z
Learning: In the Rocket.Chat repository, do not provide suggestions or recommendations about code sections marked with TODO comments. The maintainers have already identified these as future work and external reviewers lack the full context about implementation plans and timing.
Applied to files:
.github/actions/meteor-build/action.yml
🔇 Additional comments (2)
.changeset/blue-pugs-greet.md (1)
1-5: Changeset entry looks correct and release-scoped.The frontmatter structure and patch bump declaration are valid, and the note is aligned with the stated fix for this release preview.
.github/actions/meteor-build/action.yml (1)
168-178: LGTM – preserving@babelfor Minimongo compatibility.The change correctly comments out the removal of
meteor/babel-compiler/node_modules/@babel, consistent with keeping the root-level@babel(line 177). This increases the bundle by ~14M but prevents runtime issues with Minimongo. The relevant code snippet frombuild-docker/action.ymlconfirms the Docker build step also preserves@babel, so this change aligns with the overall build strategy.
Comment on lines
+480
to
+482
| before(async () => { | ||
| await updatePermission('manage-own-incoming-integrations', ['admin']); | ||
| }); |
Contributor
There was a problem hiding this comment.
Ensure this test actually validates the manage-own only permission path.
Right now, Line 481 grants manage-own-incoming-integrations, but the block never revokes manage-incoming-integrations. This means the test can pass through the broader permission and not truly verify the intended access path. Also, this permission change is not reset in this block, which can leak state into later suites.
✅ Suggested fix
describe('With manage-own-incoming-integrations permission', () => {
let integrationId: string;
before(async () => {
- await updatePermission('manage-own-incoming-integrations', ['admin']);
+ await Promise.all([
+ updatePermission('manage-incoming-integrations', []),
+ updatePermission('manage-own-incoming-integrations', ['admin']),
+ ]);
});
after(async () => {
- if (integrationId) {
- await removeIntegration(integrationId, 'incoming');
- }
+ await Promise.all([
+ integrationId ? removeIntegration(integrationId, 'incoming') : Promise.resolve(),
+ updatePermission('manage-own-incoming-integrations', []),
+ ]);
});Also applies to: 484-488, 490-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/meteor/tests/end-to-end/api/incoming-integrations.ts` around lines 480 -
482, The test setup grants only the narrower permission but never revokes the
broader one, so update the test to explicitly remove the
'manage-incoming-integrations' permission before exercising the "manage-own"
path and restore original permissions after the test; specifically use
updatePermission('manage-incoming-integrations', []) to revoke the broader
permission in the same before block (and/or in a beforeEach where used), and add
an after or afterEach that resets both 'manage-own-incoming-integrations' and
'manage-incoming-integrations' back to their original roles to avoid leaking
state.
[no ci]
Contributor
There was a problem hiding this comment.
1 issue found across 85 files (changes from recent commits).
Note: This PR contains a large number of files. cubic only reviews up to 75 files per PR, so some files may not have been reviewed.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/meteor/CHANGELOG.md">
<violation number="1" location="apps/meteor/CHANGELOG.md:9">
P3: Remove the duplicated patch note entry to keep the 8.0.2 changelog accurate and non-redundant.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| - ([#38351](https://github.com/RocketChat/Rocket.Chat/pull/38351) by [@dionisio-bot](https://github.com/dionisio-bot)) Fixes integration saving error because of missing babel dependencies inside the docker container | ||
|
|
||
| - Bump @rocket.chat/meteor version. |
Contributor
There was a problem hiding this comment.
P3: Remove the duplicated patch note entry to keep the 8.0.2 changelog accurate and non-redundant.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/meteor/CHANGELOG.md, line 9:
<comment>Remove the duplicated patch note entry to keep the 8.0.2 changelog accurate and non-redundant.</comment>
<file context>
@@ -1,5 +1,52 @@
+
+- ([#38351](https://github.com/RocketChat/Rocket.Chat/pull/38351) by [@dionisio-bot](https://github.com/dionisio-bot)) Fixes integration saving error because of missing babel dependencies inside the docker container
+
+- Bump @rocket.chat/meteor version.
+
+- Bump @rocket.chat/meteor version.
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores
You can see below a preview of the release change log:
8.0.2
Engine versions
22.16.01.43.58.21.59.0Patch Changes
Bump @rocket.chat/meteor version.
Updated dependencies []: