From 6c83bf0657004ee9cf43d5c832f51826a6591165 Mon Sep 17 00:00:00 2001 From: Martin Schoeler Date: Mon, 18 Nov 2024 10:47:34 -0300 Subject: [PATCH] fix: File Upload description bypassing message maximum character limit setting (#33218) --- .changeset/lemon-foxes-carry.md | 6 ++ apps/meteor/app/api/server/v1/rooms.ts | 8 ++ .../FileUploadModal/FileUploadModal.spec.tsx | 36 +++++++++ .../FileUploadModal/FileUploadModal.tsx | 56 +++++++------- apps/meteor/tests/e2e/file-upload.spec.ts | 1 + apps/meteor/tests/end-to-end/api/rooms.ts | 74 +++++++++++++++++++ packages/i18n/src/locales/en.i18n.json | 1 + 7 files changed, 154 insertions(+), 28 deletions(-) create mode 100644 .changeset/lemon-foxes-carry.md create mode 100644 apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx diff --git a/.changeset/lemon-foxes-carry.md b/.changeset/lemon-foxes-carry.md new file mode 100644 index 000000000000..7e14dda30747 --- /dev/null +++ b/.changeset/lemon-foxes-carry.md @@ -0,0 +1,6 @@ +--- +"@rocket.chat/meteor": patch +"@rocket.chat/i18n": patch +--- + +Fixes message character limit not being applied to file upload descriptions \ No newline at end of file diff --git a/apps/meteor/app/api/server/v1/rooms.ts b/apps/meteor/app/api/server/v1/rooms.ts index 4a746f60ad61..aaa3cff4e993 100644 --- a/apps/meteor/app/api/server/v1/rooms.ts +++ b/apps/meteor/app/api/server/v1/rooms.ts @@ -196,6 +196,10 @@ API.v1.addRoute( const fileStore = FileUpload.getStore('Uploads'); const uploadedFile = await fileStore.insert(details, fileBuffer); + if ((fields.description?.length ?? 0) > settings.get('Message_MaxAllowedSize')) { + throw new Meteor.Error('error-message-size-exceeded'); + } + uploadedFile.description = fields.description; delete fields.description; @@ -299,6 +303,10 @@ API.v1.addRoute( throw new Meteor.Error('invalid-file'); } + if ((this.bodyParams.description?.length ?? 0) > settings.get('Message_MaxAllowedSize')) { + throw new Meteor.Error('error-message-size-exceeded'); + } + file.description = this.bodyParams.description; delete this.bodyParams.description; diff --git a/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx b/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx new file mode 100644 index 000000000000..0f365ee3673b --- /dev/null +++ b/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.spec.tsx @@ -0,0 +1,36 @@ +import { mockAppRoot } from '@rocket.chat/mock-providers'; +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import React from 'react'; + +import FileUploadModal from './FileUploadModal'; + +const defaultProps = { + onClose: () => undefined, + file: new File([], 'testing.png'), + fileName: 'Testing', + fileDescription: '', + onSubmit: () => undefined, + invalidContentType: false, + showDescription: true, +}; + +it('should show Undo request button when roomOpen is true and transcriptRequest exist', async () => { + render(, { + legacyRoot: true, + wrapper: mockAppRoot() + .withTranslations('en', 'core', { + Cannot_upload_file_character_limit: 'Cannot upload file, description is over the {{count}} character limit', + Send: 'Send', + Upload_file_description: 'File description', + }) + .withSetting('Message_MaxAllowedSize', 10) + .build(), + }); + + const input = await screen.findByRole('textbox', { name: 'File description' }); + expect(input).toBeInTheDocument(); + await userEvent.type(input, '12345678910'); + + expect(screen.getByText('Cannot upload file, description is over the 10 character limit')).toBeInTheDocument(); +}); diff --git a/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx b/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx index 87e1a90d3e5c..5a9f054924ea 100644 --- a/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx +++ b/apps/meteor/client/views/room/modals/FileUploadModal/FileUploadModal.tsx @@ -1,9 +1,9 @@ import { Modal, Box, Field, FieldGroup, FieldLabel, FieldRow, FieldError, TextInput, Button } from '@rocket.chat/fuselage'; -import { useAutoFocus } from '@rocket.chat/fuselage-hooks'; import { useToastMessageDispatch, useTranslation, useSetting } from '@rocket.chat/ui-contexts'; import fileSize from 'filesize'; -import type { ReactElement, ChangeEvent, FormEventHandler, ComponentProps } from 'react'; -import React, { memo, useState, useEffect } from 'react'; +import type { ReactElement, ComponentProps } from 'react'; +import React, { memo, useEffect } from 'react'; +import { useForm } from 'react-hook-form'; import FilePreview from './FilePreview'; @@ -26,31 +26,21 @@ const FileUploadModal = ({ invalidContentType, showDescription = true, }: FileUploadModalProps): ReactElement => { - const [name, setName] = useState(fileName); - const [description, setDescription] = useState(fileDescription || ''); + const { + register, + handleSubmit, + formState: { errors, isValid }, + } = useForm({ mode: 'onChange', defaultValues: { name: fileName, description: fileDescription } }); + const t = useTranslation(); const dispatchToastMessage = useToastMessageDispatch(); + const maxMsgSize = useSetting('Message_MaxAllowedSize', 5000); const maxFileSize = useSetting('FileUpload_MaxFileSize', 104857600); - const ref = useAutoFocus(); - - const handleName = (e: ChangeEvent): void => { - setName(e.currentTarget.value); - }; - - const handleDescription = (e: ChangeEvent): void => { - setDescription(e.currentTarget.value); - }; - - const handleSubmit: FormEventHandler = (e): void => { - e.preventDefault(); - if (!name) { - return dispatchToastMessage({ - type: 'error', - message: t('Required_field', { field: t('Upload_file_name') }), - }); - } + const isDescriptionValid = (description: string) => + description.length >= maxMsgSize ? t('Cannot_upload_file_character_limit', { count: maxMsgSize }) : true; + const submit = ({ name, description }: { name: string; description?: string }): void => { // -1 maxFileSize means there is no limit if (maxFileSize > -1 && (file.size || 0) > maxFileSize) { onClose(); @@ -83,7 +73,7 @@ const FileUploadModal = ({ }, [file, dispatchToastMessage, invalidContentType, t, onClose]); return ( - ) => }> + ) => }> {t('FileUpload')} @@ -97,16 +87,26 @@ const FileUploadModal = ({ {t('Upload_file_name')} - + - {!name && {t('Required_field', { field: t('Upload_file_name') })}} + {errors.name?.message} {showDescription && ( {t('Upload_file_description')} - + isDescriptionValid(value || ''), + })} + aria-label={t('Upload_file_description')} + /> + {errors.description?.message} )} @@ -116,7 +116,7 @@ const FileUploadModal = ({ - diff --git a/apps/meteor/tests/e2e/file-upload.spec.ts b/apps/meteor/tests/e2e/file-upload.spec.ts index 159b2650ac16..626162df47d3 100644 --- a/apps/meteor/tests/e2e/file-upload.spec.ts +++ b/apps/meteor/tests/e2e/file-upload.spec.ts @@ -76,6 +76,7 @@ test.describe.serial('file-upload', () => { await expect(poHomeChannel.content.btnModalConfirm).not.toBeVisible(); }); }); + test.describe('file-upload-not-member', () => { let poHomeChannel: HomeChannel; let targetChannel: string; diff --git a/apps/meteor/tests/end-to-end/api/rooms.ts b/apps/meteor/tests/end-to-end/api/rooms.ts index 712e1917441d..ca033ef88bf8 100644 --- a/apps/meteor/tests/end-to-end/api/rooms.ts +++ b/apps/meteor/tests/end-to-end/api/rooms.ts @@ -550,7 +550,81 @@ describe('[Rooms]', () => { expect(res.body.message.files[0]).to.have.property('name', 'lst-test.lst'); }); }); + describe('/rooms.media - Max allowed size', () => { + before(async () => updateSetting('Message_MaxAllowedSize', 10)); + after(async () => updateSetting('Message_MaxAllowedSize', 5000)); + it('should allow uploading a file with description under the max character limit', async () => { + await request + .post(api(`rooms.media/${testChannel._id}`)) + .set(credentials) + .attach('file', imgURL) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('file'); + expect(res.body.file).to.have.property('_id'); + expect(res.body.file).to.have.property('url'); + + fileNewUrl = res.body.file.url; + fileOldUrl = res.body.file.url.replace('/file-upload/', '/ufs/GridFS:Uploads/'); + fileId = res.body.file._id; + }); + + await request + .post(api(`rooms.mediaConfirm/${testChannel._id}/${fileId}`)) + .set(credentials) + .send({ + description: '123456789', + }) + .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('image_type', 'image/png'); + expect(res.body.message.attachments[0]).to.have.property('title', '1024x1024.png'); + expect(res.body.message).to.have.property('files'); + expect(res.body.message.files).to.be.an('array').of.length(2); + expect(res.body.message.files[0]).to.have.property('type', 'image/png'); + expect(res.body.message.files[0]).to.have.property('name', '1024x1024.png'); + }); + }); + it('should not allow uploading a file with description over the max character limit', async () => { + await request + .post(api(`rooms.media/${testChannel._id}`)) + .set(credentials) + .attach('file', imgURL) + .expect('Content-Type', 'application/json') + .expect(200) + .expect((res) => { + expect(res.body).to.have.property('success', true); + expect(res.body).to.have.property('file'); + expect(res.body.file).to.have.property('_id'); + expect(res.body.file).to.have.property('url'); + + fileNewUrl = res.body.file.url; + fileOldUrl = res.body.file.url.replace('/file-upload/', '/ufs/GridFS:Uploads/'); + fileId = res.body.file._id; + }); + + await request + .post(api(`rooms.mediaConfirm/${testChannel._id}/${fileId}`)) + .set(credentials) + .send({ + description: '12345678910', + }) + .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-message-size-exceeded'); + }); + }); + }); it('should not allow uploading a blocked media type to a room', async () => { await updateSetting('FileUpload_MediaTypeBlackList', 'text/plain'); await request diff --git a/packages/i18n/src/locales/en.i18n.json b/packages/i18n/src/locales/en.i18n.json index 291538f035d5..3b1c8ce3b47d 100644 --- a/packages/i18n/src/locales/en.i18n.json +++ b/packages/i18n/src/locales/en.i18n.json @@ -955,6 +955,7 @@ "Cannot_open_conversation_with_yourself": "Cannot Direct Message with yourself", "Cannot_share_your_location": "Cannot share your location...", "Cannot_disable_while_on_call": "Can't change status during calls ", + "Cannot_upload_file_character_limit": "Cannot upload file, description is over the {{count}} character limit", "Cant_join": "Can't join", "CAS": "CAS", "CAS_Description": "Central Authentication Service allows members to use one set of credentials to sign in to multiple sites over multiple protocols.",