Open
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 |
🦋 Changeset detectedLatest commit: 2c4a6bf The changes in this PR will be included in the next version bump. This PR includes changesets to release 40 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #38768 +/- ##
==========================================
- Coverage 70.76% 70.71% -0.05%
==========================================
Files 3142 3142
Lines 108948 108954 +6
Branches 19668 19594 -74
==========================================
- Hits 77099 77051 -48
- Misses 29848 29896 +48
- Partials 2001 2007 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
WalkthroughUpdates include Traefik image bumps and Docker Compose build removals, addition of a filename sanitizer with tests and integration into filesystem operations, improvements to record/object type checking and query validation logic, and new changeset entries for a patch version bump. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
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/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", "02/12 22:57", "02/13 22:38", "02/16 14:04", "02/18 23:15", "02/19 02:13 (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.00]
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.00]
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.00]
line "omnichannel-transcript-service" [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, 0.13, 0.00]
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.00]
line "queue-worker-service" [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, 0.13, 0.00]
line "rocketchat" [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.34, 0.34, 0.00]
Statistics (last 30 days):
ℹ️ About this reportThis report compares Docker image sizes from this build against the
Updated: Thu, 19 Feb 2026 02:13:38 GMT |
Co-authored-by: Julio Araujo <julio.araujo@rocket.chat>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
10-12:startsWith('..')slightly over-rejects safe filenames like'..name'.After the
base !== fileNamegate (line 6) ensures there are no path separators, the only truly dangerous value is exactly'..'(becausepath.join('/uploads', '..')normalizes to'/'). Strings like'...wav'or'..audio'are not traversal risks. The current check is conservative and harmless for a security hotfix, but worth tightening if legitimate filenames starting with..are ever needed.♻️ Tighter alternative (optional)
- if (base === '.' || base.startsWith('..')) { + if (base === '.' || base === '..') {🤖 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` around lines 10 - 12, The check in sanitizeFileName erroneously rejects filenames beginning with '..' (e.g., '..name') even though only the exact value '..' is a traversal risk; update the condition that currently reads "if (base === '.' || base.startsWith('..'))" to instead reject only the exact parent-dir token by using "base === '.' || base === '..'". Keep the existing earlier guard that compares base !== fileName intact (it ensures no path separators).apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts (1)
51-53:'..\\passwd'is caught by thestartsWith('..')check, not the character regex — miscategorized.The test sits in "invalid characters" but on POSIX,
path.basename('..\passwd')returns the full string'..\passwd'(backslash is not a POSIX separator), and it is rejected on line 10 (startsWith('..')) rather than by the character-class regex. Placing it under "invalid paths" or renaming the test description would more accurately reflect the enforced invariant.🤖 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 51 - 53, The test for sanitizeFileName currently asserts sanitizeFileName('..\\passwd') throws but is miscategorized under "invalid characters" because the rejection happens via the startsWith('..') check (not the character-class regex); either move this test into the "invalid paths" suite that covers path traversal checks or update the test description to reflect that it's asserting rejection for path traversal/backslashes (e.g., "should reject path traversal/backslash filenames"), keeping the same expect(() => sanitizeFileName('..\\passwd')).to.throw() assertion and referencing sanitizeFileName and the startsWith('..') check in your change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/nice-glasses-love.mdapps/meteor/app/file/server/file.server.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.tsapps/meteor/app/file/server/functions/sanitizeFileName.ts
🧰 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/app/file/server/file.server.tsapps/meteor/app/file/server/functions/sanitizeFileName.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.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 (12)
📓 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-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/**/*.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.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/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 **/*.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 : 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-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.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-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 `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: 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/app/file/server/functions/sanitizeFileName.spec.ts
🧬 Code graph analysis (1)
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts (1)
apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
sanitizeFileName(3-19)
⏰ 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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
.changeset/nice-glasses-love.md (1)
1-5: LGTM!Changeset is correctly scoped as a patch with the security hotfix reference.
apps/meteor/app/file/server/file.server.ts (1)
13-14: LGTM — all fourFileSystemI/O entry points are correctly guarded.
GridFSis intentionally left unsanitized (MongoDB key, not a filesystem path). The sanitization is idempotent, so the indirect double-call throughgetFileWithReadStream → stat + createReadStreamis harmless.Also applies to: 146-146, 154-154, 158-158, 162-162
🤖 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/file/server/functions/sanitizeFileName.spec.ts`:
- Around line 6-54: Add missing edge-case tests and tighten throw assertions in
the sanitizeFileName spec: add tests that call sanitizeFileName(''),
sanitizeFileName('.'), sanitizeFileName('..') and a positive test for
sanitizeFileName('.hidden'), and update all existing expects that use
.to.throw() to assert the specific error message (e.g.
.to.throw('error-invalid-file-name') or a regex) so failures only pass when the
expected error from sanitizeFileName is thrown; locate and modify the tests in
the sanitizeFileName spec (functions referenced: sanitizeFileName and the
existing it() blocks) to include these cases and replace bare .to.throw() checks
with the scoped error string.
In `@apps/meteor/app/file/server/functions/sanitizeFileName.ts`:
- Around line 14-16: The validation in sanitizeFileName.ts currently rejects
percent-encoded names because the allowlist regex tested against the variable
base disallows '%' — update the validation so percent signs are accepted (i.e.,
include '%' in the allowed characters in the regex used in sanitizeFileName) to
preserve compatibility with encodeURIComponent-based callers (functions like
createWriteStream in uploadEmojiCustom.ts, deleteFile callers in
insertOrUpdateEmoji.ts, uploadZipFile.ts, and getFileWithReadStream usage in
emoji-custom.js); alternatively, if you prefer not to change the regex,
decodeURIComponent(base) before running the existing validation, but prefer the
regex change to avoid breaking existing encoded filenames.
---
Nitpick comments:
In `@apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts`:
- Around line 51-53: The test for sanitizeFileName currently asserts
sanitizeFileName('..\\passwd') throws but is miscategorized under "invalid
characters" because the rejection happens via the startsWith('..') check (not
the character-class regex); either move this test into the "invalid paths" suite
that covers path traversal checks or update the test description to reflect that
it's asserting rejection for path traversal/backslashes (e.g., "should reject
path traversal/backslash filenames"), keeping the same expect(() =>
sanitizeFileName('..\\passwd')).to.throw() assertion and referencing
sanitizeFileName and the startsWith('..') check in your change.
In `@apps/meteor/app/file/server/functions/sanitizeFileName.ts`:
- Around line 10-12: The check in sanitizeFileName erroneously rejects filenames
beginning with '..' (e.g., '..name') even though only the exact value '..' is a
traversal risk; update the condition that currently reads "if (base === '.' ||
base.startsWith('..'))" to instead reject only the exact parent-dir token by
using "base === '.' || base === '..'". Keep the existing earlier guard that
compares base !== fileName intact (it ensures no path separators).
Comment on lines
+6
to
+54
| describe('sanitizeFileName', () => { | ||
| describe('valid filenames', () => { | ||
| it('should allow simple filenames', () => { | ||
| const result = sanitizeFileName('sound.mp3'); | ||
| expect(result).to.equal('sound.mp3'); | ||
| }); | ||
|
|
||
| it('should allow filenames with dashes, underscores and dots', () => { | ||
| const result = sanitizeFileName('alert_test-01.wav'); | ||
| expect(result).to.equal('alert_test-01.wav'); | ||
| }); | ||
|
|
||
| it('should allow filenames without extension', () => { | ||
| const result = sanitizeFileName('beep'); | ||
| expect(result).to.equal('beep'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('invalid paths', () => { | ||
| it('should reject path traversal using ../', () => { | ||
| expect(() => sanitizeFileName('../etc/passwd')).to.throw(); | ||
| }); | ||
|
|
||
| it('should reject path traversal using ./', () => { | ||
| expect(() => sanitizeFileName('./folder/sounds')).to.throw(); | ||
| }); | ||
|
|
||
| it('should reject nested paths', () => { | ||
| expect(() => sanitizeFileName('sounds/alert.mp3')).to.throw(); | ||
| }); | ||
|
|
||
| it('should reject absolute paths', () => { | ||
| expect(() => sanitizeFileName('/etc/passwd')).to.throw(); | ||
| }); | ||
| }); | ||
|
|
||
| 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(); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
Missing edge-case coverage and weak error-type assertions.
Two gaps worth addressing:
-
Missing tests for
'','.', and'..'— all currently throw'error-invalid-file-name'but are not exercised. A test for a valid hidden-style filename like'.hidden'(which does pass today) would also document the intended boundary. -
Weak throw assertion —
.to.throw()passes for any exception including an unexpectedTypeError. Scoping to the expected error string removes false-positive risk.
🛡️ Suggested additions
+ describe('edge cases', () => {
+ it('should reject empty string', () => {
+ expect(() => sanitizeFileName('')).to.throw(Error, 'error-invalid-file-name');
+ });
+
+ it('should reject single dot', () => {
+ expect(() => sanitizeFileName('.')).to.throw(Error, 'error-invalid-file-name');
+ });
+
+ it('should reject double dot', () => {
+ expect(() => sanitizeFileName('..')).to.throw(Error, 'error-invalid-file-name');
+ });
+ });And tighten existing assertions, e.g.:
- expect(() => sanitizeFileName('../etc/passwd')).to.throw();
+ expect(() => sanitizeFileName('../etc/passwd')).to.throw(Error, 'error-invalid-file-name');- expect(() => sanitizeFileName('my sound.mp3')).to.throw();
+ expect(() => sanitizeFileName('my sound.mp3')).to.throw(Error, 'error-invalid-characters-in-file-name');🤖 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
6 - 54, Add missing edge-case tests and tighten throw assertions in the
sanitizeFileName spec: add tests that call sanitizeFileName(''),
sanitizeFileName('.'), sanitizeFileName('..') and a positive test for
sanitizeFileName('.hidden'), and update all existing expects that use
.to.throw() to assert the specific error message (e.g.
.to.throw('error-invalid-file-name') or a regex) so failures only pass when the
expected error from sanitizeFileName is thrown; locate and modify the tests in
the sanitizeFileName spec (functions referenced: sanitizeFileName and the
existing it() blocks) to include these cases and replace bare .to.throw() checks
with the scoped error string.
Comment on lines
+14
to
+16
| if (!/^[a-zA-Z0-9._-]+$/.test(base)) { | ||
| throw new Error('error-invalid-characters-in-file-name'); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all callers that instantiate FileSystem or invoke its IO methods
# 1. Where is RocketChatFile.FileSystem used and what is passed as fileName?
echo "=== RocketChatFile.FileSystem usage ==="
rg -n --type=ts -C 4 'RocketChatFile\.FileSystem\b' --glob '!*.spec.ts' | head -100
# 2. Where are createWriteStream / createReadStream called on FileSystem instances?
echo -e "\n=== FileSystem method calls in apps/meteor/app/file/ ==="
rg -n --type=ts -C 3 'createWriteStream|createReadStream|\.remove\b|\.deleteFile\b' apps/meteor/app/file/ --glob '!*.spec.ts' | head -150Repository: RocketChat/Rocket.Chat
Length of output: 4907
🏁 Script executed:
#!/bin/bash
# Find who calls createWriteStream and createReadStream on fileStore instances
echo "=== Callers of createWriteStream ==="
rg -n --type=ts 'fileStore\s*\.\s*createWriteStream\(' --glob '!*.spec.ts' -A 2 -B 2 | head -80
echo -e "\n=== Callers of createReadStream ==="
rg -n --type=ts 'fileStore\s*\.\s*createReadStream\(' --glob '!*.spec.ts' -A 2 -B 2 | head -80
echo -e "\n=== Callers of getFileWithReadStream ==="
rg -n --type=ts '\.getFileWithReadStream\(' --glob '!*.spec.ts' -A 2 -B 2 | head -80Repository: RocketChat/Rocket.Chat
Length of output: 1522
🏁 Script executed:
#!/bin/bash
# Find all usages of createWriteStream across the codebase
echo "=== All createWriteStream callers ==="
rg -n --type=ts 'createWriteStream' apps/meteor --glob '!*.spec.ts' -B 3 -A 1 | head -150
# Find encodeURIComponent usage with file operations
echo -e "\n=== encodeURIComponent with file operations ==="
rg -n --type=ts 'encodeURIComponent.*\.' apps/meteor --glob '!*.spec.ts' -B 2 -A 2
# Check the emoji custom file store usage more broadly
echo -e "\n=== RocketChatFileEmojiCustomInstance usage ==="
rg -n --type=ts 'RocketChatFileEmojiCustomInstance' apps/meteor --glob '!*.spec.ts' -B 2 -A 3Repository: RocketChat/Rocket.Chat
Length of output: 29005
🏁 Script executed:
#!/bin/bash
# Check what the asset filenames look like and understand the actual impact
echo "=== Asset filenames usage ==="
rg -n --type=ts 'RocketChatAssetsInstance\.' apps/meteor/app/assets/server -B 3 -A 3 | head -100
echo -e "\n=== Importer filenames ==="
rg -n --type=ts 'RocketChatImportFileInstance\.' apps/meteor/app/importer/server -B 2 -A 2 | head -100
# Check if there are any database records of emoji or other files that might have encoded names
echo -e "\n=== Check emoji-custom startup/init ==="
cat apps/meteor/app/emoji-custom/server/startup/emoji-custom.ts 2>/dev/null | head -50Repository: RocketChat/Rocket.Chat
Length of output: 3404
🏁 Script executed:
#!/bin/bash
# Check what asset keys are used
echo "=== Asset keys/names ==="
rg -n --type=ts 'Assets_' apps/meteor/app/assets/server/assets.ts | head -20
# Check what newFileName is in importer
echo -e "\n=== Importer newFileName source ==="
rg -n --type=ts 'newFileName.*=' apps/meteor/app/importer/server/methods/ -B 3 -A 1 | head -80
# Check emoji-custom initialization
echo -e "\n=== RocketChatFileEmojiCustomInstance type ==="
rg -n --type=ts 'RocketChatFileEmojiCustomInstance\s*=' apps/meteor/app/emoji-custom/ -A 5 | head -30Repository: RocketChat/Rocket.Chat
Length of output: 1631
🏁 Script executed:
#!/bin/bash
# Verify the actual emoji-custom instance and understand the real scope
echo "=== Emoji custom FileStore type ==="
rg -n --type=ts 'new RocketChatFile' apps/meteor/app/emoji-custom/server/startup/ -B 5 -A 5
# Understand what happens with existing emoji in database
echo -e "\n=== Check emoji-custom schema/model ==="
fd -t f 'emoji.*\.ts' apps/meteor/app/emoji-custom/server/ --type f -exec head -30 {} \;
# Check if sanitizeFileName is called before encodeURIComponent anywhere
echo -e "\n=== Check sanitizeFileName calls ==="
rg -n --type=ts 'sanitizeFileName\(' apps/meteor --glob '!*.spec.ts' -B 2 -A 2 | head -100Repository: RocketChat/Rocket.Chat
Length of output: 2238
🏁 Script executed:
#!/bin/bash
# Find what RocketChatFileEmojiCustomInstance actually is
echo "=== Find emoji-custom instance definition ==="
rg -n --type=ts 'RocketChatFileEmojiCustomInstance' apps/meteor/app/emoji-custom/server/startup/emoji-custom.ts -A 10 -B 5
# Check all FileSystem instances to understand the storage backends
echo -e "\n=== FileSystem instances ==="
rg -n --type=ts 'new RocketChatFileSystem\|new RocketChatFileMongoDB' apps/meteor/app/file/server/ -B 3 -A 5
# Check where custom sounds FileStore is initialized
echo -e "\n=== Custom sounds FileStore ==="
rg -n --type=ts 'RocketChatFileCustomSoundsInstance\|FileStore' apps/meteor/app/custom-sounds/server/ -B 2 -A 3Repository: RocketChat/Rocket.Chat
Length of output: 291
🏁 Script executed:
#!/bin/bash
# Find the actual emoji-custom startup file
echo "=== Find emoji-custom startup files ==="
find apps/meteor/app/emoji-custom -name '*.ts' -type f | grep -i startup
# Find RocketChatFileEmojiCustomInstance definition
echo -e "\n=== Find RocketChatFileEmojiCustomInstance ==="
rg -n --type=ts 'RocketChatFileEmojiCustomInstance' apps/meteor/app/emoji-custom --glob '!*.spec.ts' -B 5 -A 5 | head -100
# Find all FileStore definitions
echo -e "\n=== All FileStore definitions ==="
rg -n --type=ts 'const.*FileStore.*=.*new' apps/meteor --glob '!*.spec.ts' -B 2 -A 2 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 9577
🏁 Script executed:
#!/bin/bash
# Find where RocketChatFileEmojiCustomInstance is exported from
echo "=== Find emoji-custom export ==="
rg -n --type=ts 'export.*RocketChatFileEmojiCustomInstance' apps/meteor/app/emoji-custom -A 5 -B 5
# Find the startup directory
echo -e "\n=== Find emoji-custom server files ==="
find apps/meteor/app/emoji-custom/server -type f -name '*.ts' | head -30
# Search in the entire app for where these instances are created
echo -e "\n=== Search for RocketChatFile instance creation ==="
rg -n --type=ts 'new RocketChatFile' apps/meteor --glob '!*.spec.ts' -B 3 -A 3 | head -150Repository: RocketChat/Rocket.Chat
Length of output: 1052
🏁 Script executed:
#!/bin/bash
# Check the emoji-custom index.ts
echo "=== emoji-custom index.ts ==="
cat apps/meteor/app/emoji-custom/server/index.ts
# Search for RocketChatFileEmojiCustomInstance initialization across entire codebase
echo -e "\n=== Search for RocketChatFileEmojiCustomInstance initialization ==="
rg -n --type=ts 'RocketChatFileEmojiCustomInstance\s*=' apps/meteor --glob '!*.spec.ts' -B 3 -A 5
# Check imports from 'startup/emoji-custom' module
echo -e "\n=== Files that import from emoji-custom startup ==="
rg -n --type=ts "from.*'.*emoji-custom'" apps/meteor/app/emoji-custom --glob '!*.spec.ts' | grep -i startupRepository: RocketChat/Rocket.Chat
Length of output: 793
🏁 Script executed:
#!/bin/bash
# The startup file must exist since it's being imported. Let's find it with a broader search
echo "=== Search for emoji-custom.ts files ==="
find apps/meteor/app/emoji-custom -name '*emoji-custom*' -type f
# Also try with git ls-files
echo -e "\n=== git ls-files for emoji-custom ==="
git ls-files apps/meteor/app/emoji-custom | grep -i startup
# If file exists, read it
echo -e "\n=== Read the startup file ==="
if [ -f apps/meteor/app/emoji-custom/server/startup/emoji-custom.ts ]; then
cat apps/meteor/app/emoji-custom/server/startup/emoji-custom.ts
else
echo "File not found at expected path"
# Try to find all .ts files in emoji-custom
find apps/meteor/app/emoji-custom -type f -name '*.ts'
fiRepository: RocketChat/Rocket.Chat
Length of output: 694
🏁 Script executed:
#!/bin/bash
# Read the .js startup file
echo "=== Read emoji-custom.js startup ==="
cat apps/meteor/app/emoji-custom/server/startup/emoji-custom.js
# Also check assets startup for comparison
echo -e "\n=== Check assets startup ==="
find apps/meteor/app/assets/server -name '*startup*' -o -name '*assets*' | grep -v node_modules
cat apps/meteor/app/assets/server/assets.ts | head -50Repository: RocketChat/Rocket.Chat
Length of output: 5248
The strict allowlist regex breaks backward compatibility with encodeURIComponent-encoded filenames.
The regex ^[a-zA-Z0-9._-]+$ rejects the % character, but the codebase passes encoded filenames to FileSystem methods:
uploadEmojiCustom.ts:70callscreateWriteStream(encodeURIComponent(...))insertOrUpdateEmoji.ts:114-117callsdeleteFile(encodeURIComponent(...))uploadZipFile.ts:24usesencodeURIComponent(...)on data export filenamesemoji-custom.js:56callsgetFileWithReadStream(encodeURIComponent(...))
Any emoji with non-ASCII or space characters (e.g., "smile 😊", "my sound") will be encoded as smile%20%F0%9F%98%8A, which fails the regex validation. This renders existing files inaccessible and breaks deletion/updates.
The regex should either allow % or callers should sanitize before encoding, not after.
🤖 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` around lines 14 -
16, The validation in sanitizeFileName.ts currently rejects percent-encoded
names because the allowlist regex tested against the variable base disallows '%'
— update the validation so percent signs are accepted (i.e., include '%' in the
allowed characters in the regex used in sanitizeFileName) to preserve
compatibility with encodeURIComponent-based callers (functions like
createWriteStream in uploadEmojiCustom.ts, deleteFile callers in
insertOrUpdateEmoji.ts, uploadZipFile.ts, and getFileWithReadStream usage in
emoji-custom.js); alternatively, if you prefer not to change the regex,
decodeURIComponent(base) before running the existing validation, but prefer the
regex change to avoid breaking existing encoded filenames.
Co-authored-by: Kevin Aleman <11577696+KevLehman@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.changeset/nice-glasses-love.mdapps/meteor/app/api/server/helpers/parseJsonQuery.tsapps/meteor/app/api/server/lib/isValidQuery.tsapps/meteor/app/api/server/v1/users.tsapps/meteor/app/file/server/file.server.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.tsapps/meteor/app/file/server/functions/sanitizeFileName.tspackages/tools/src/isRecord.spec.tspackages/tools/src/isRecord.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/nice-glasses-love.md
🧰 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/app/api/server/v1/users.tsapps/meteor/app/file/server/functions/sanitizeFileName.spec.tsapps/meteor/app/api/server/lib/isValidQuery.tsapps/meteor/app/file/server/functions/sanitizeFileName.tspackages/tools/src/isRecord.spec.tsapps/meteor/app/api/server/helpers/parseJsonQuery.tsapps/meteor/app/file/server/file.server.tspackages/tools/src/isRecord.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.tspackages/tools/src/isRecord.spec.ts
🧠 Learnings (19)
📓 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-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:
apps/meteor/app/api/server/v1/users.tsapps/meteor/app/file/server/functions/sanitizeFileName.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.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/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 : 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 **/*.spec.ts : Use `.spec.ts` extension for test files (e.g., `login.spec.ts`)
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 **/*.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-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.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-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 `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: 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 : Prefer web-first assertions (`toBeVisible`, `toHaveText`, etc.) in Playwright tests
Applied to files:
apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts
📚 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:
apps/meteor/app/file/server/functions/sanitizeFileName.ts
📚 Learning: 2025-11-24T17:08:26.531Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat PR: 0
File: .cursor/rules/test-cases.mdc:0-0
Timestamp: 2025-11-24T17:08:26.531Z
Learning: Reference the test-cases.json file to ensure consistency with existing test case structures and standardized format
Applied to files:
packages/tools/src/isRecord.spec.ts
📚 Learning: 2025-12-10T21:00:43.645Z
Learnt from: KevLehman
Repo: RocketChat/Rocket.Chat PR: 37091
File: ee/packages/abac/jest.config.ts:4-7
Timestamp: 2025-12-10T21:00:43.645Z
Learning: Adopt the monorepo-wide Jest testMatch pattern: <rootDir>/src/**/*.spec.{ts,js,mjs} (represented here as '**/src/**/*.spec.{ts,js,mjs}') to ensure spec files under any package's src directory are picked up consistently across all packages in the Rocket.Chat monorepo. Apply this pattern in jest.config.ts for all relevant packages to maintain uniform test discovery.
Applied to files:
packages/tools/src/isRecord.spec.ts
📚 Learning: 2026-01-26T18:26:01.279Z
Learnt from: d-gubert
Repo: RocketChat/Rocket.Chat PR: 38227
File: apps/meteor/app/api/server/router.ts:44-49
Timestamp: 2026-01-26T18:26:01.279Z
Learning: In apps/meteor/app/api/server/router.ts, when retrieving bodyParams and queryParams from the Hono context via c.get(), do not add defensive defaults (e.g., ?? {}). The code should fail fast if these parameters are missing, as endpoint handlers expect them to be present and breaking here helps surface parsing problems rather than hiding them.
Applied to files:
apps/meteor/app/api/server/helpers/parseJsonQuery.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
🧬 Code graph analysis (4)
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/api/server/lib/isValidQuery.ts (1)
packages/tools/src/isRecord.ts (1)
isRecord(1-17)
apps/meteor/app/api/server/helpers/parseJsonQuery.ts (1)
apps/meteor/app/api/server/lib/isValidQuery.ts (1)
isValidQuery(7-24)
apps/meteor/app/file/server/file.server.ts (1)
apps/meteor/app/file/server/functions/sanitizeFileName.ts (1)
sanitizeFileName(3-19)
⏰ 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). (5)
- GitHub Check: 📦 Build Packages
- GitHub Check: cubic · AI code reviewer
- GitHub Check: update-pr
- GitHub Check: CodeQL-Build
- GitHub Check: CodeQL-Build
🔇 Additional comments (6)
apps/meteor/app/api/server/v1/users.ts (1)
518-535: LGTM — broadening allowed operators to$regex/$optionsis correct here.
nonEmptyQueryis server-constructed (viagetNonEmptyQuery/getInclusiveFields) and legitimately uses$regexfor username/name pattern matching. Since attribute paths are still tightly gated byinclusiveFieldsKeys, allowing these operators is safe.packages/tools/src/isRecord.ts (1)
6-16: LGTM — robust improvement to object-type detection.Explicitly guarding against arrays, handling null-prototype objects from EJSON/JSON parsers, and using
prototype.constructor === Objectinstead ofinstanceofcorrectly covers all edge cases.apps/meteor/app/api/server/lib/isValidQuery.ts (2)
1-16: LGTM — correct migration frominstanceof ObjecttoisRecord.Null-prototype objects (which EJSON/MongoDB query parsing can produce) would fail
instanceof Objectand previously cause a spurious'query must be an object'throw.isRecordcorrectly handles them.
27-44: LGTM — spread beforeremoveDangerousPropsandisRecordfor recursive check are both correct.
{ ...query }ensures the helper receives a plain-Object-prototype object regardless of the original query's prototype. TheisRecordguard for recursive traversal correctly excludes arrays (which are handled by the precedingArray.isArraybranch).apps/meteor/app/api/server/helpers/parseJsonQuery.ts (1)
145-148: LGTM — correct gating on non-emptyqueryFields.Routes that provide
queryFieldsstill get validated; routes that don't (likeusers.list) handle their ownisValidQuerycall independently. Theclean()pass at line 120 continues to strip dangerous operators for all cases, so skippingisValidQueryhere whenqueryFieldsis empty doesn't open a gap.apps/meteor/app/file/server/file.server.ts (1)
13-13: LGTM —sanitizeFileNameis correctly integrated into all fourFileSystemFS operations.
GridFSis rightly left untouched (filenames are GridFS metadata, not filesystem paths). The double invocation ofsanitizeFileNameinsidegetFileWithReadStream(viastat→sanitizeFileNameandcreateReadStream→sanitizeFileName) is redundant but benign. The existingtry/catchingetFileWithReadStreammeans invalid filenames (including path-traversal attempts) are logged and returnundefined, consistent with the pre-existing "file not found" pattern.Note: the
%-character rejection issue flagged insanitizeFileName.tsapplies transitively to all four methods here.Also applies to: 145-163
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/tools/src/isRecord.spec.ts`:
- Around line 31-39: The three test cases currently in the "should return true
for records" table—identified as 'case-7' (new String('string')), 'case-8' (new
Number(1)), and 'case-9' ([])—are expecting false but have a misleading
description; move these three entries into the existing "should return false for
objects that are not records" test group that calls isRecord, remove the
duplicate array case (the [] already exists as case-2 in the not-records group),
and ensure the test descriptions remain accurate so the isRecord spec's
failing-true/false expectations match their group names.
---
Duplicate comments:
In `@apps/meteor/app/file/server/functions/sanitizeFileName.spec.ts`:
- Around line 26-53: The tests use bare .to.throw() which accepts any exception;
update each failing-case assertion in sanitizeFileName.spec.ts to assert the
specific error (e.g., .to.throw(Error) or the exact error class/message your
sanitizeFileName throws) so failures are precise, and add new test cases
asserting that sanitizeFileName(''), sanitizeFileName('.') and
sanitizeFileName('..') each throw the same specific error; reference the
sanitizeFileName function and modify the existing "path traversal" and "invalid
characters" it(...) blocks to use the stronger assertions and include the three
new it(...) cases.
In `@apps/meteor/app/file/server/functions/sanitizeFileName.ts`:
- Around line 14-16: sanitizeFileName currently rejects '%' so
encodeURIComponent(...)-encoded filenames from callers like
uploadEmojiCustom.ts, insertOrUpdateEmoji.ts, and emoji-custom.js fail
validation; either decode the incoming name with decodeURIComponent before
running the existing /^[a-zA-Z0-9._-]+$/ test or expand the regex to include '%'
(e.g., /^[a-zA-Z0-9._%\-]+$/) so percent-encoded names are accepted; update the
sanitizeFileName function accordingly and ensure FileSystem callers still
receive/expect either decoded or percent-preserving names consistently.
Comment on lines
+31
to
39
| // we testing, we ignore eslint | ||
| // eslint-disable-next-line no-new-wrappers | ||
| ['case-7', new String('string'), false], | ||
| // eslint-disable-next-line no-new-wrappers | ||
| ['case-8', new Number(1), false], | ||
| ['case-9', [], false], | ||
| ])('should return true for records %# (%s)', (_caseId, input, expected) => { | ||
| expect(isRecord(input)).toEqual(expected); | ||
| }); |
Contributor
There was a problem hiding this comment.
Misleading test description for false-expecting cases.
case-7 (new String), case-8 (new Number), and case-9 ([]) all expect false but are grouped under 'should return true for records'. This produces test names like "should return true for records 6 (case-7)" which directly contradicts the assertion. These cases should be moved to the existing 'should return false for objects that are not records' group (or a new dedicated group), keeping the description accurate.
Additionally, [] is tested twice — as case-9 here and as case-2 in the "not records" group on line 43 — which is redundant.
As per coding guidelines, test names should clearly communicate expected behavior.
♻️ Suggested restructure
test.each([
['case-1', {}, true],
['case-2', { prop: 'value' }, true],
['case-3', Object.fromEntries([]), true],
['case-4', Object.create(null), true],
['case-5', { 0: 'zero', 1: 'one' }, true],
['case-6', { [Symbol('key')]: 'value' }, true],
- // we testing, we ignore eslint
- // eslint-disable-next-line no-new-wrappers
- ['case-7', new String('string'), false],
- // eslint-disable-next-line no-new-wrappers
- ['case-8', new Number(1), false],
- ['case-9', [], false],
])('should return true for records %# (%s)', (_caseId, input, expected) => {
test.each([
['case-1', new Date(), false],
['case-2', [], false],
- ['case-2', [{}], false],
+ ['case-3', [{}], false],
+ // eslint-disable-next-line no-new-wrappers
+ ['case-4', new String('string'), false],
+ // eslint-disable-next-line no-new-wrappers
+ ['case-5', new Number(1), false],
])('should return false for objects that are not records %# (%s)', (_caseId, input, expected) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/tools/src/isRecord.spec.ts` around lines 31 - 39, The three test
cases currently in the "should return true for records" table—identified as
'case-7' (new String('string')), 'case-8' (new Number(1)), and 'case-9' ([])—are
expecting false but have a misleading description; move these three entries into
the existing "should return false for objects that are not records" test group
that calls isRecord, remove the duplicate array case (the [] already exists as
case-2 in the not-records group), and ensure the test descriptions remain
accurate so the isRecord spec's failing-true/false expectations match their
group names.
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
1 participant
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
Chores
Security
Bug Fixes
Tests
You can see below a preview of the release change log:
8.1.1
Engine versions
22.16.01.43.58.21.59.1Patch Changes
Bump @rocket.chat/meteor version.
(#38786 by @dionisio-bot) Security Hotfix (https://docs.rocket.chat/docs/security-fixes-and-updates)
Updated dependencies []: