fix: add frontend and backend restrictions for custom sounds size/format#39159
Open
nazabucciarelli wants to merge 13 commits intodevelopfrom
Open
fix: add frontend and backend restrictions for custom sounds size/format#39159nazabucciarelli wants to merge 13 commits intodevelopfrom
nazabucciarelli wants to merge 13 commits intodevelopfrom
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: 8530352 The changes in this PR will be included in the next version bump. This PR includes changesets to release 41 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 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent 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)
WalkthroughAdds client- and server-side validation for custom sound uploads: enforces a 5MB size limit, requires MP3 files (detected via file signature), surfaces client errors in admin UI, updates the upload implementation to store Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (Browser)
participant UI as Admin UI
participant Hook as useSingleFileInput
participant Server as uploadCustomSound (Meteor)
participant FS as File Storage
User->>UI: Select file
UI->>Hook: handleFiles(file)
Hook-->>Hook: if maxSize && file.size > maxSize -> onError, clear input, stop
alt file within size
Hook->>Server: POST FormData (file)
Server->>Server: read buffer
Server->>Server: fromBuffer -> detect MIME/type
alt detected MP3 and size <= MAX
Server->>FS: write file as <id>.mp3 with audio/mpeg
Server->>UI: respond success
Server->>Server: notify.updateCustomSound (delayed)
else
Server->>UI: respond error (invalid-file-type / file-too-large)
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
nazabucciarelli
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #39159 +/- ##
===========================================
- Coverage 70.91% 70.85% -0.06%
===========================================
Files 3208 3208
Lines 113426 113431 +5
Branches 20532 20536 +4
===========================================
- Hits 80431 80370 -61
- Misses 30949 31009 +60
- Partials 2046 2052 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
nazabucciarelli
force-pushed
the
fix/sounds-upload-mp3-hardening
branch
from
bf667e0 to
f7a2807
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (1)
73-80:⚠️ Potential issue | 🟠 MajorComplete the MP3 migration in storage-reactivity URL assertions.
After switching test setup to MP3 (Line 73 onward), later assertions still request
.wavfiles (e.g., Lines 289, 290, 300, 301, 313, 327). With MP3-only hardening, these checks become inconsistent and may fail for the wrong reason.🔧 Proposed fix
- await request.get(`/custom-sounds/${gridFsFileId}.wav`).set(credentials).expect(200); - await request.get(`/custom-sounds/${fsFileId}.wav`).set(credentials).expect(404); + await request.get(`/custom-sounds/${gridFsFileId}.mp3`).set(credentials).expect(200); + await request.get(`/custom-sounds/${fsFileId}.mp3`).set(credentials).expect(404); - await request.get(`/custom-sounds/${gridFsFileId}.wav`).set(credentials).expect(404); - await request.get(`/custom-sounds/${fsFileId}.wav`).set(credentials).expect(200); + await request.get(`/custom-sounds/${gridFsFileId}.mp3`).set(credentials).expect(404); + await request.get(`/custom-sounds/${fsFileId}.mp3`).set(credentials).expect(200); - await request.get(`/custom-sounds/${fsFileId}.wav`).set(credentials).expect(200); + await request.get(`/custom-sounds/${fsFileId}.mp3`).set(credentials).expect(200); - await request.get(`/custom-sounds/${fsFileId}.wav`).set(credentials).expect(404); + await request.get(`/custom-sounds/${fsFileId}.mp3`).set(credentials).expect(404);🤖 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/custom-sounds.ts` around lines 73 - 80, Tests load an MP3 (readFileSync -> binary) and upload via insertOrUpdateSound and uploadCustomSound, but later storage-reactivity URL assertions still expect “.wav”; update those assertions to expect “.mp3” instead. Locate assertions that build or validate storage/reactivity URLs (search for occurrences of “.wav” or the test helpers that reference sound URLs in this file) and change the expected extension to “.mp3” so the checks match the MP3-only uploads performed by readFileSync/binary, uploadCustomSound, insertOrUpdateSound, fileId and fileId2.
🧹 Nitpick comments (1)
apps/meteor/client/hooks/useSingleFileInput.ts (1)
8-9: Drop inline implementation comment in the hook signature.Please remove the
// In bytescomment in Line 8 to stay aligned with repository style.🔧 Proposed fix
- maxSize?: number, // In bytes + maxSize?: number,As per coding guidelines "Avoid code comments in the implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/meteor/client/hooks/useSingleFileInput.ts` around lines 8 - 9, Remove the inline implementation comment from the hook signature by deleting the trailing "// In bytes" after the maxSize parameter in the useSingleFileInput hook; update the function signature that declares "maxSize?: number" (and keep onError?: () => void) so the parameter remains but no inline comment is present to conform with the repository style.
🤖 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/custom-sounds/server/methods/uploadCustomSound.ts`:
- Around line 32-35: Detected MP3 by server-side sniffing (fromBuffer) but later
metadata still uses client-provided values (contentType and
soundData.extension); normalize these to the validated MP3 values before
persisting. After the fromBuffer check in uploadCustomSound.ts, set/overwrite
contentType to the server-validated MIME (use MIME.mp3 or 'audio/mpeg') and set
soundData.extension to the sniffed mimeType.ext (or 'mp3'), and then continue
using those normalized values for storage/DB writes so client-controlled
metadata cannot be spoofed.
In `@apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx`:
- Line 33: The file picker currently passes the non-standard MIME type
'audio/mp3' to useSingleFileInput; change this to the canonical 'audio/mpeg' and
also supply the '.mp3' extension for broader browser compatibility. Update the
useSingleFileInput call (the one referencing handleChangeFile and
MAX_CUSTOM_SOUND_SIZE_BYTES) to use 'audio/mpeg' and include '.mp3' (or an array
of accepted types containing both 'audio/mpeg' and '.mp3') so the file input
reliably accepts MP3 files across browsers.
---
Outside diff comments:
In `@apps/meteor/tests/end-to-end/api/custom-sounds.ts`:
- Around line 73-80: Tests load an MP3 (readFileSync -> binary) and upload via
insertOrUpdateSound and uploadCustomSound, but later storage-reactivity URL
assertions still expect “.wav”; update those assertions to expect “.mp3”
instead. Locate assertions that build or validate storage/reactivity URLs
(search for occurrences of “.wav” or the test helpers that reference sound URLs
in this file) and change the expected extension to “.mp3” so the checks match
the MP3-only uploads performed by readFileSync/binary, uploadCustomSound,
insertOrUpdateSound, fileId and fileId2.
---
Nitpick comments:
In `@apps/meteor/client/hooks/useSingleFileInput.ts`:
- Around line 8-9: Remove the inline implementation comment from the hook
signature by deleting the trailing "// In bytes" after the maxSize parameter in
the useSingleFileInput hook; update the function signature that declares
"maxSize?: number" (and keep onError?: () => void) so the parameter remains but
no inline comment is present to conform with the repository style.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
apps/meteor/tests/mocks/files/audio_mock.mp3is excluded by!**/*.mp3apps/meteor/tests/mocks/files/audio_mock.wavis excluded by!**/*.wav
📒 Files selected for processing (7)
.changeset/forty-geese-do.mdapps/meteor/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/client/hooks/useSingleFileInput.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/lib/constants.tsapps/meteor/tests/end-to-end/api/custom-sounds.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: 🔨 Test API (EE) / MongoDB 8.0 coverage (1/1)
- GitHub Check: 🔨 Test API (CE) / MongoDB 8.0 (1/1)
🧰 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/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/lib/constants.tsapps/meteor/client/views/admin/customSounds/AddCustomSound.tsxapps/meteor/client/views/admin/customSounds/EditSound.tsxapps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/client/hooks/useSingleFileInput.ts
🧠 Learnings (10)
📚 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:
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.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/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/lib/constants.tsapps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/client/hooks/useSingleFileInput.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/app/custom-sounds/server/methods/uploadCustomSound.tsapps/meteor/lib/constants.tsapps/meteor/tests/end-to-end/api/custom-sounds.tsapps/meteor/client/hooks/useSingleFileInput.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/client/views/admin/customSounds/AddCustomSound.tsx
📚 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/tests/end-to-end/api/custom-sounds.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/custom-sounds.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/tests/end-to-end/api/custom-sounds.ts
📚 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/forty-geese-do.md
📚 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/forty-geese-do.md
📚 Learning: 2026-02-10T16:32:42.586Z
Learnt from: tassoevan
Repo: RocketChat/Rocket.Chat PR: 38528
File: apps/meteor/client/startup/roles.ts:14-14
Timestamp: 2026-02-10T16:32:42.586Z
Learning: In Rocket.Chat's Meteor client code, DDP streams use EJSON and Date fields arrive as Date objects; do not manually construct new Date() in stream handlers (for example, in sdk.stream()). Only REST API responses return plain JSON where dates are strings, so implement explicit conversion there if needed. Apply this guidance to all TypeScript files under apps/meteor/client to ensure consistent date handling in DDP streams and REST responses.
Applied to files:
apps/meteor/client/hooks/useSingleFileInput.ts
🧬 Code graph analysis (4)
apps/meteor/app/custom-sounds/server/methods/uploadCustomSound.ts (1)
apps/meteor/lib/constants.ts (1)
MAX_CUSTOM_SOUND_SIZE_BYTES(2-2)
apps/meteor/client/views/admin/customSounds/AddCustomSound.tsx (2)
apps/meteor/client/hooks/useSingleFileInput.ts (1)
useSingleFileInput(4-73)apps/meteor/lib/constants.ts (1)
MAX_CUSTOM_SOUND_SIZE_BYTES(2-2)
apps/meteor/client/views/admin/customSounds/EditSound.tsx (2)
apps/meteor/client/hooks/useSingleFileInput.ts (1)
useSingleFileInput(4-73)apps/meteor/lib/constants.ts (1)
MAX_CUSTOM_SOUND_SIZE_BYTES(2-2)
apps/meteor/tests/end-to-end/api/custom-sounds.ts (2)
apps/meteor/app/custom-sounds/server/startup/custom-sounds.js (1)
path(29-29)apps/meteor/app/emoji-custom/server/startup/emoji-custom.js (1)
path(59-59)
🔇 Additional comments (4)
.changeset/forty-geese-do.md (1)
1-5: Changeset entry matches the hardening scope.Release note is aligned with the actual security/validation changes.
apps/meteor/lib/constants.ts (1)
2-2: Good centralization of the custom sound size limit.Using a shared constant reduces drift between frontend and backend checks.
apps/meteor/client/views/admin/customSounds/EditSound.tsx (1)
128-133: Size-limit UX wiring looks good.The hook integration and user-facing toast on oversize selection are correctly connected.
apps/meteor/client/hooks/useSingleFileInput.ts (1)
48-52: Oversize short-circuit logic is solid.Early return with input reset and callback is a good guard for client-side validation.
…sounds-upload-mp3-hardening
…at/Rocket.Chat into fix/sounds-upload-mp3-hardening
![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 9 files
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/custom-sounds.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/custom-sounds.ts:73">
P2: The test setup now uploads MP3 files, but the storage-type tests later still request `.wav` URLs. This mismatch will cause those storage tests to fail because the uploaded custom sounds are no longer WAVs. Update the remaining `.wav` requests to `.mp3` (or align the uploaded extension) to keep the suite consistent.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
1 issue found across 1 file (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/custom-sounds.ts">
<violation number="1" location="apps/meteor/tests/end-to-end/api/custom-sounds.ts:63">
P2: Avoid committing focused tests. `describe.only` will skip all other test suites in CI. Remove `.only` so the full test suite runs.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
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.
Proposed changes (including videos or screenshots)
useSingleFileInput.tscomponent to restrict file uploads by size, with a customonErrorcallback.MAX_CUSTOM_SOUND_SIZE_BYTESconstant to share between client and server.AddCustomSound.tsxandEditSound.tsx.mp3files with a max size of 5MB.custom-soundsAPI tests. New tests to cover this hardening will not be added on this PR since it was spotted thatcustom-soundsis still using Meteor instead of REST, so a task for creating these rest endpoints and making the Custom Sounds creation atomic will be created, and the proper tests will be included there.Issue(s)
CORE-1863 Custom Sounds endpoint allows arbitrary file upload (SVG accepted) with no size limit
Steps to test or reproduce
.mp3files larger than 5MB and check that the toast error message shows up. It works with different languages as well.useSingleFileInputfunction parameters to allows other formats/sizes) and check how the backend rejects the upload (specifically, theuploadCustomSoundcall fails, theinsertOrUpdateSoundone will have success and I think this should be atomic, but it's out of this task's scope)Further comments
The added audio file was generated programmatically for testing purposes.
It contains synthetic silence and is free of copyright and DRM restrictions.
Summary by CodeRabbit
Bug Fixes
Tests
Chores