Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Use application/octet-stream as a fallback media type to avoid "Unknown media type" errors #32471

Merged
merged 19 commits into from
Jun 19, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
64a2c25
feat: Add setting to allow uploading unknown media types
matheusbsilva137 May 22, 2024
8b36f09
test: add end-to-end tests
matheusbsilva137 May 22, 2024
d267286
Create changeset
matheusbsilva137 May 22, 2024
06b92b6
feat: Improve error when uploading unknown media types
matheusbsilva137 May 22, 2024
7de1eab
Merge branch 'feat/setting-allow-upload-unknown-media' of https://git…
matheusbsilva137 May 22, 2024
cd4113c
Update setting's name and description
matheusbsilva137 May 24, 2024
bf5d06e
Update changeset
matheusbsilva137 May 24, 2024
57a36af
fix end-to-end tests
matheusbsilva137 May 24, 2024
9cfb465
Merge branch 'feat/setting-allow-upload-unknown-media' of https://git…
matheusbsilva137 May 24, 2024
a857479
improve: Remove setting and make application/octet-stream the default…
matheusbsilva137 Jun 13, 2024
a35783e
i18n: remove unused translation
matheusbsilva137 Jun 13, 2024
13cd06c
Merge branch 'develop' of https://github.com/RocketChat/Rocket.Chat i…
matheusbsilva137 Jun 13, 2024
fcb6086
i18n: re-add translations
matheusbsilva137 Jun 13, 2024
49a553b
improve: Add alert to media type blocked list setting
matheusbsilva137 Jun 13, 2024
e4d0a91
Update alert
matheusbsilva137 Jun 14, 2024
3f3e6ae
improve: Use helper function to get file MIME type
matheusbsilva137 Jun 17, 2024
93717af
Merge branch 'develop' into feat/setting-allow-upload-unknown-media
matheusbsilva137 Jun 17, 2024
bb81896
Merge branch 'develop' into feat/setting-allow-upload-unknown-media
matheusbsilva137 Jun 17, 2024
2886ac3
Merge branch 'develop' into feat/setting-allow-upload-unknown-media
matheusbsilva137 Jun 19, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/breezy-pens-sing.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@rocket.chat/meteor": minor
"@rocket.chat/i18n": minor
---

Removed "Unknown media type" errors on the client side by using `application/octet-stream` as a fallback media type (MIME type) for all files
7 changes: 5 additions & 2 deletions apps/meteor/app/api/server/lib/getUploadFormData.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import type { ValidateFunction } from 'ajv';
import busboy from 'busboy';
import type { Request } from 'express';

import { mime } from '../../../utils/lib/mimeTypes';

type UploadResult<K> = {
file: Readable & { truncated: boolean };
fieldname: string;
Expand Down Expand Up @@ -61,7 +63,7 @@ export async function getUploadFormData<
function onFile(
fieldname: string,
file: Readable & { truncated: boolean },
{ filename, encoding, mimeType: mimetype }: { filename: string; encoding: string; mimeType: string },
{ filename, encoding, mimeType }: { filename: string; encoding: string; mimeType: string },
) {
if (options.field && fieldname !== options.field) {
file.resume();
Expand All @@ -78,12 +80,13 @@ export async function getUploadFormData<
fileChunks.length = 0;
return returnError(new MeteorError('error-file-too-large'));
}
const mimeTypeFromFilename = mime.lookup(filename);

uploadedFile = {
file,
filename,
encoding,
mimetype,
mimetype: typeof mimeTypeFromFilename === 'string' ? mimeTypeFromFilename : mimeType,
fieldname,
fields,
fileBuffer: Buffer.concat(fileChunks),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export const useFileUploadDropTarget = (): readonly [

const t = useTranslation();

const fileUploadEnabled = useSetting('FileUpload_Enabled') as boolean;
const fileUploadEnabled = useSetting<boolean>('FileUpload_Enabled');
const user = useUser();
const fileUploadAllowedForUser = useReactiveValue(
useCallback(() => !roomCoordinator.readOnly(room._id, { username: user?.username }), [room._id, user?.username]),
Expand All @@ -55,7 +55,8 @@ export const useFileUploadDropTarget = (): readonly [
const uniqueFiles = getUniqueFiles();

const uploads = Array.from(uniqueFiles).map((file) => {
Object.defineProperty(file, 'type', { value: mime.lookup(file.name) });
const fileMimeType = mime.lookup(file.name);
Object.defineProperty(file, 'type', { value: typeof fileMimeType === 'string' ? fileMimeType : 'application/octet-stream' });
matheusbsilva137 marked this conversation as resolved.
Show resolved Hide resolved
return file;
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const fileInputProps = { type: 'file', multiple: true };

export const useFileUploadAction = (disabled: boolean): GenericMenuItemProps => {
const t = useTranslation();
const fileUploadEnabled = useSetting('FileUpload_Enabled');
const fileUploadEnabled = useSetting<boolean>('FileUpload_Enabled');
const fileInputRef = useFileInput(fileInputProps);
const chat = useChat();

Expand All @@ -25,8 +25,9 @@ export const useFileUploadAction = (disabled: boolean): GenericMenuItemProps =>
const handleUploadChange = async () => {
const { mime } = await import('../../../../../../../app/utils/lib/mimeTypes');
const filesToUpload = Array.from(fileInputRef?.current?.files ?? []).map((file) => {
const fileMimeType = mime.lookup(file.name);
Object.defineProperty(file, 'type', {
value: mime.lookup(file.name),
value: typeof fileMimeType === 'string' ? fileMimeType : 'application/octet-stream',
matheusbsilva137 marked this conversation as resolved.
Show resolved Hide resolved
});
return file;
});
Expand Down
1 change: 1 addition & 0 deletions apps/meteor/server/settings/file-upload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ export const createFileUploadSettings = () =>
type: 'string',
public: true,
i18nDescription: 'FileUpload_MediaTypeBlackListDescription',
alert: 'FileUpload_MediaTypeBlackList_Alert',
});

await this.add('FileUpload_ProtectFiles', true, {
Expand Down
1 change: 1 addition & 0 deletions apps/meteor/tests/data/interactions.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export const targetUser = 'rocket.cat';
export const imgURL = './public/images/logo/1024x1024.png';
export const lstURL = './tests/e2e/fixtures/files/lst-test.lst';
export const drawioURL = './tests/e2e/fixtures/files/diagram.drawio';
export const svgLogoURL = './public/images/logo/logo.svg';
export const svgLogoFileName = 'logo.svg';
21 changes: 21 additions & 0 deletions apps/meteor/tests/e2e/file-upload.spec.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Users } from './fixtures/userStates';
import { HomeChannel } from './page-objects';
import { createTargetChannel } from './utils';
import { setSettingValueById } from './utils/setSettingValueById';
import { expect, test } from './utils/test';

test.use({ storageState: Users.user1.state });
Expand All @@ -10,6 +11,7 @@ test.describe.serial('file-upload', () => {
let targetChannel: string;

test.beforeAll(async ({ api }) => {
await setSettingValueById(api, 'FileUpload_MediaTypeBlackList', 'image/svg+xml');
targetChannel = await createTargetChannel(api);
});

Expand All @@ -21,6 +23,7 @@ test.describe.serial('file-upload', () => {
});

test.afterAll(async ({ api }) => {
await setSettingValueById(api, 'FileUpload_MediaTypeBlackList', 'image/svg+xml');
expect((await api.post('/channels.delete', { roomName: targetChannel })).status()).toBe(200);
});

Expand Down Expand Up @@ -54,4 +57,22 @@ test.describe.serial('file-upload', () => {
await expect(poHomeChannel.content.getFileDescription).toHaveText('lst_description');
await expect(poHomeChannel.content.lastMessageFileName).toContainText('lst-test.lst');
});

test('expect send drawio (unknown media type) file succesfully', async ({ page }) => {
await page.reload();
await poHomeChannel.content.sendFileMessage('diagram.drawio');
await poHomeChannel.content.descriptionInput.fill('drawio_description');
await poHomeChannel.content.btnModalConfirm.click();

await expect(poHomeChannel.content.getFileDescription).toHaveText('drawio_description');
await expect(poHomeChannel.content.lastMessageFileName).toContainText('diagram.drawio');
});

test('expect not to send drawio file (unknown media type) when the default media type is blocked', async ({ api, page }) => {
await setSettingValueById(api, 'FileUpload_MediaTypeBlackList', 'application/octet-stream');

await page.reload();
await poHomeChannel.content.sendFileMessage('diagram.drawio');
await expect(poHomeChannel.content.btnModalConfirm).not.toBeVisible();
});
});
13 changes: 13 additions & 0 deletions apps/meteor/tests/e2e/fixtures/files/diagram.drawio
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<mxfile host="app.diagrams.net" modified="2024-05-21T16:10:09.295Z" agent="Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/124.0.0.0 Safari/537.36" etag="ZxZRCTHi-kxhlzk7b9_Z" version="24.4.4" type="device">
<diagram name="Página-1" id="9eBILa8281JaQ4yUkDbp">
<mxGraphModel dx="1434" dy="786" grid="1" gridSize="10" guides="1" tooltips="1" connect="1" arrows="1" fold="1" page="1" pageScale="1" pageWidth="827" pageHeight="1169" math="0" shadow="0">
<root>
<mxCell id="0" />
<mxCell id="1" parent="0" />
<mxCell id="dopCU4gkJe7Sfp6IDYO1-1" value="&lt;b&gt;&lt;font style=&quot;font-size: 30px;&quot;&gt;Rocket.Chat&lt;/font&gt;&lt;/b&gt;" style="text;html=1;align=center;verticalAlign=middle;resizable=0;points=[];autosize=1;strokeColor=none;fillColor=none;" vertex="1" parent="1">
<mxGeometry x="314" y="350" width="200" height="50" as="geometry" />
</mxCell>
</root>
</mxGraphModel>
</diagram>
</mxfile>
47 changes: 41 additions & 6 deletions apps/meteor/tests/end-to-end/api/09-rooms.js
matheusbsilva137 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { after, afterEach, before, beforeEach, describe, it } from 'mocha';
import { sleep } from '../../../lib/utils/sleep';
import { getCredentials, api, request, credentials } from '../../data/api-data.js';
import { sendSimpleMessage, deleteMessage } from '../../data/chat.helper';
import { imgURL, lstURL, svgLogoFileName, svgLogoURL } from '../../data/interactions';
import { drawioURL, imgURL, lstURL, svgLogoFileName, svgLogoURL } from '../../data/interactions';
import { getSettingValueById, updateEEPermission, updatePermission, updateSetting } from '../../data/permissions.helper';
import { createRoom, deleteRoom } from '../../data/rooms.helper';
import { deleteTeam } from '../../data/teams.helper';
Expand Down Expand Up @@ -179,8 +179,8 @@ describe('[Rooms]', function () {
});
});

it('should upload a LST file to room', (done) => {
request
it('should upload a LST file to room', () => {
return request
.post(api(`rooms.upload/${testChannel._id}`))
.set(credentials)
.attach('file', lstURL)
Expand All @@ -196,12 +196,33 @@ describe('[Rooms]', function () {
expect(res.body.message).to.have.property('files');
expect(res.body.message.files).to.be.an('array').of.length(1);
expect(res.body.message.files[0]).to.have.property('name', 'lst-test.lst');
})
.end(done);
expect(res.body.message.files[0]).to.have.property('type', 'text/plain');
});
});

it('should upload a DRAWIO file (unknown media type) to room', () => {
return request
.post(api(`rooms.upload/${testChannel._id}`))
.set(credentials)
.attach('file', drawioURL)
.expect('Content-Type', 'application/json')
.expect(200)
.expect((res) => {
expect(res.body).to.have.property('success', true);
expect(res.body).to.have.property('message');
expect(res.body.message).to.have.property('attachments');
expect(res.body.message.attachments).to.be.an('array').of.length(1);
expect(res.body.message.attachments[0]).to.have.property('format', 'DRAWIO');
expect(res.body.message.attachments[0]).to.have.property('title', 'diagram.drawio');
expect(res.body.message).to.have.property('files');
expect(res.body.message.files).to.be.an('array').of.length(1);
expect(res.body.message.files[0]).to.have.property('name', 'diagram.drawio');
expect(res.body.message.files[0]).to.have.property('type', 'application/octet-stream');
});
});

it('should not allow uploading a blocked media type to a room', async () => {
await updateSetting('FileUpload_MediaTypeBlackList', 'application/octet-stream');
await updateSetting('FileUpload_MediaTypeBlackList', 'text/plain');
await request
.post(api(`rooms.upload/${testChannel._id}`))
.set(credentials)
Expand All @@ -214,6 +235,20 @@ describe('[Rooms]', function () {
});
});

it('should not allow uploading an unknown media type to a room if the default one is blocked', async () => {
await updateSetting('FileUpload_MediaTypeBlackList', 'application/octet-stream');
await request
.post(api(`rooms.upload/${testChannel._id}`))
.set(credentials)
.attach('file', drawioURL)
.expect('Content-Type', 'application/json')
.expect(400)
.expect((res) => {
expect(res.body).to.have.property('success', false);
expect(res.body).to.have.property('errorType', 'error-invalid-file-type');
});
});

it('should be able to get the file', async () => {
await request.get(fileNewUrl).set(credentials).expect('Content-Type', 'image/png').expect(200);
await request.get(fileOldUrl).set(credentials).expect('Content-Type', 'image/png').expect(200);
Expand Down
1 change: 1 addition & 0 deletions packages/i18n/src/locales/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -2381,6 +2381,7 @@
"FileUpload_MediaType_NotAccepted": "Media Types Not Accepted",
"FileUpload_MediaTypeBlackList": "Blocked Media Types",
"FileUpload_MediaTypeBlackListDescription": "Comma-separated list of media types. This setting has priority over the Accepted Media Types.",
"FileUpload_MediaTypeBlackList_Alert": "The default media type for unknown file extensions is \"application/octet-stream\", to work only with known file extensions you can add it to the \"Blocked Media Types\" list.",
"FileUpload_MediaTypeWhiteList": "Accepted Media Types",
"FileUpload_MediaTypeWhiteListDescription": "Comma-separated list of media types. Leave it blank for accepting all media types.",
"FileUpload_ProtectFiles": "Protect Uploaded Files",
Expand Down