fix: `FileUploadModal` not validating renamed blacklisted file types #38106

abhinavkrin · 2026-01-08T16:23:55Z

Proposed changes (including videos or screenshots)

This PR fixes an issue where the client continued sending an incorrect MIME type when a file name was changed during the upload flow.

Issue(s)

Steps to test or reproduce

Further comments

SUP-953

Summary by CodeRabbit

New Features
- Added filename-based MIME detection to improve content-type handling.
Bug Fixes
- Fixed bypass of file-type blacklist when renaming files so disallowed uploads are blocked.
Improvements
- Early validation of uploads by filename and content type; better error messaging and added accessibility alerts on validation errors.
Tests
- Added render, accessibility, and validation tests for the file upload modal.
Chores
- Added a changeset to mark a patch release.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

dionisio-bot · 2026-01-08T16:23:59Z

Looks like this PR is ready to merge! 🎉
If you have any trouble, please check the PR guidelines

changeset-bot · 2026-01-08T16:24:00Z

🦋 Changeset detected

Latest commit: aadaadb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 40 packages

Name	Type
@rocket.chat/meteor	Patch
@rocket.chat/core-typings	Patch
@rocket.chat/rest-typings	Patch
@rocket.chat/uikit-playground	Patch
@rocket.chat/api-client	Patch
@rocket.chat/apps	Patch
@rocket.chat/core-services	Patch
@rocket.chat/cron	Patch
@rocket.chat/ddp-client	Patch
@rocket.chat/fuselage-ui-kit	Patch
@rocket.chat/gazzodown	Patch
@rocket.chat/http-router	Patch
@rocket.chat/livechat	Patch
@rocket.chat/model-typings	Patch
@rocket.chat/ui-avatar	Patch
@rocket.chat/ui-client	Patch
@rocket.chat/ui-contexts	Patch
@rocket.chat/ui-voip	Patch
@rocket.chat/web-ui-registration	Patch
@rocket.chat/account-service	Patch
@rocket.chat/authorization-service	Patch
@rocket.chat/ddp-streamer	Patch
@rocket.chat/omnichannel-transcript	Patch
@rocket.chat/presence-service	Patch
@rocket.chat/queue-worker	Patch
@rocket.chat/abac	Patch
@rocket.chat/federation-matrix	Patch
@rocket.chat/license	Patch
@rocket.chat/media-calls	Patch
@rocket.chat/omnichannel-services	Patch
@rocket.chat/pdf-worker	Patch
@rocket.chat/presence	Patch
rocketchat-services	Patch
@rocket.chat/models	Patch
@rocket.chat/network-broker	Patch
@rocket.chat/omni-core-ee	Patch
@rocket.chat/mock-providers	Patch
@rocket.chat/ui-video-conf	Patch
@rocket.chat/instance-status	Patch
@rocket.chat/omni-core	Patch

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

coderabbitai · 2026-01-08T16:24:26Z

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Added filename-based MIME lookup and used it to validate renamed uploads before submission; small upload flow cleanup and tests/changeset to prevent blacklisted types being bypassed by renaming.

Changes

Cohort / File(s)	Summary
MIME Type Utilities `apps/meteor/app/utils/lib/mimeTypes.ts`	Added `getMimeTypeFromFileName(fileName: string)` (falls back to `application/octet-stream`), refactored `getMimeType()` to delegate to it, and exported the new helper.
Upload Flow Adjustments `apps/meteor/client/lib/chats/flows/uploadFiles.ts`	Minor signature cleanup for `onSubmit` (removed inline types) and adjusted content-type check to use `file.type` instead of `file?.type`.
File Upload Modal Logic `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx`	Added filename validation via `getMimeTypeFromFileName` + `fileUploadIsValidContentType` (wired into form `register`), added `useCallback` validator, and added `role="alert"` to field errors for accessibility.
File Upload Modal Tests `apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx`	Added story-based render + a11y tests, updated test fixtures to include `type` and `fileName`, added description length test and renamed-file blacklist test.
Changeset `.changeset/short-jobs-join.md`	Added patch changeset noting fix for file type blacklist bypass via rename.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant User as "User\n(uploads file)"
  participant UI as "FileUploadModal\n(client)"
  participant Util as "mimeTypes Util"
  participant Validator as "ContentType Validator"
  participant Server as "Server\n(upload endpoint)"

  User->>UI: Select file and optionally rename
  UI->>Util: getMimeTypeFromFileName(newFileName)
  Util-->>UI: inferred MIME type
  UI->>Validator: fileUploadIsValidContentType(inferred MIME or file.type)
  Validator-->>UI: accepted / rejected
  alt rejected
    UI-->>User: show name error (role=alert) and abort
  else accepted
    UI->>UI: run other validations (size, description)
    UI->>Server: upload file (original or renamed)
    Server-->>UI: upload response
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

fix: prevent file content buffering in user uploads #38071 — touches MIME-type resolution used in upload flows and may overlap with filename-based MIME handling changes.

Suggested reviewers

rodrigok
ggazzo
cardoso

Poem

🐰 I sniffed the filename, checked its mime with care,
No sneaky rename will slip past my stare.
I hop and I guard each upload today,
Errors announced so none go astray.
Hooray — the blacklist stands, come what may! 🎉

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	The pull request successfully addresses the core objectives from SUP-953 by validating MIME types based on the renamed filename, preventing blacklist bypasses through extension changes.
Out of Scope Changes check	✅ Passed	All changes are focused on fixing the MIME type bypass issue through filename-based validation and improved error handling in the File Upload dialog.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately describes the main fix: validating renamed blacklisted file types in FileUploadModal, which directly addresses the security issue where file type blacklists were being bypassed.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch fix/client-sends-incorrect-mime-type-on-file-name-change

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

cubic-dev-ai

1 issue found across 4 files

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="apps/meteor/tests/e2e/file-upload.spec.ts">

<violation number="1" location="apps/meteor/tests/e2e/file-upload.spec.ts:14">
P1: Typo: trailing comma inside the setting ID string `'FileUpload_MediaTypeBlackList,'` will cause the setting to not be found. The `afterAll` block correctly uses `'FileUpload_MediaTypeBlackList'` without the comma.</violation>
</file>

Since this is your first cubic review, here's how it works:

cubic automatically reviews your code and comments on bugs and improvements
Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
Ask questions if you need clarification on any suggestion

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

apps/meteor/tests/e2e/file-upload.spec.ts

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @apps/meteor/tests/e2e/file-upload.spec.ts:
- Line 14: The call to setSettingValueById uses an incorrect setting ID with a
trailing comma ('FileUpload_MediaTypeBlackList,'), causing the blacklist to be
set on the wrong key; remove the trailing comma so the function uses the correct
ID ('FileUpload_MediaTypeBlackList') in the test (locate the failing invocation
of setSettingValueById in file-upload.spec.ts and update the string argument).

🧹 Nitpick comments (1)

apps/meteor/client/lib/chats/flows/uploadFiles.ts (1)
217-217: Consider validating with prepared file for consistency.

The invalidContentType check uses nextFile?.type (the original file's type), but the actual submission validation at line 87 uses the prepared file's type. This could lead to inconsistent UX where the modal appears but then immediately shows an error on submit when a user renames a file to a blacklisted extension.

However, this might be intentional to allow users to fix the filename. If so, the current behavior is acceptable.
💡 Alternative approach for consistency

If you want to prevent the modal from appearing at all for invalid types after renaming, you could move the file preparation and validation earlier:
	const uploadNextFile = (): void => {
		const nextFile = queue.pop();
		if (!nextFile) {
			chat.composer?.dismissAllQuotedMessages();
			return;
		}
+
+		// Early validation for renamed files
+		const initialFile = prepareFile(nextFile.name, nextFile.name, nextFile);

		imperativeModal.open({
			component: FileUploadModal,
			props: {
-				invalidContentType: !fileUploadIsValidContentType(nextFile?.type),
+				invalidContentType: !fileUploadIsValidContentType(initialFile.type),
Note: This would require adjusting the logic since initially the filename hasn't changed yet.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 69aa0bd and c6c9e0c.

📒 Files selected for processing (4)

apps/meteor/app/utils/lib/mimeTypes.ts
apps/meteor/client/lib/chats/flows/uploadFiles.ts
apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx
apps/meteor/tests/e2e/file-upload.spec.ts

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{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/client/views/room/modals/FileUploadModal/FileUploadModal.tsx
apps/meteor/tests/e2e/file-upload.spec.ts
apps/meteor/client/lib/chats/flows/uploadFiles.ts
apps/meteor/app/utils/lib/mimeTypes.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.ts extension for test files (e.g., login.spec.ts)

Files:

apps/meteor/tests/e2e/file-upload.spec.ts

apps/meteor/tests/e2e/**/*.spec.ts

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.spec.ts: All test files must be created in apps/meteor/tests/e2e/ directory
Avoid using page.locator() in Playwright tests - always prefer semantic locators such as page.getByRole(), page.getByLabel(), page.getByText(), or page.getByTitle()
Use test.beforeAll() and test.afterAll() for setup/teardown in Playwright tests
Use test.step() for complex test scenarios to improve organization in Playwright tests
Group related tests in the same file
Utilize Playwright fixtures (test, page, expect) for consistency in test files
Prefer web-first assertions (toBeVisible, toHaveText, etc.) in Playwright tests
Use expect matchers for assertions (toEqual, toContain, toBeTruthy, toHaveLength, etc.) instead of assert statements in Playwright tests
Use page.waitFor() with specific conditions instead of hardcoded timeouts in Playwright tests
Implement proper wait strategies for dynamic content in Playwright tests
Maintain test isolation between test cases in Playwright tests
Ensure clean state for each test execution in Playwright tests
Ensure tests run reliably in parallel without shared state conflicts

Files:

apps/meteor/tests/e2e/file-upload.spec.ts

apps/meteor/tests/e2e/**/*.{ts,spec.ts}

📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc)

apps/meteor/tests/e2e/**/*.{ts,spec.ts}: Store commonly used locators in variables/constants for reuse
Follow Page Object Model pattern consistently in Playwright tests

Files:

apps/meteor/tests/e2e/file-upload.spec.ts

🧠 Learnings (10)