From 1c756bbfdf276d2c6d95f6ea091ddf962d86e48f Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 14 Nov 2024 17:19:31 +0000 Subject: [PATCH 1/7] Check that the file the user chose has a MIME type of `image/*` Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/elements/MiniAvatarUploader.tsx | 14 ++++++++++++-- src/components/views/settings/AvatarSetting.tsx | 15 +++++++++++++-- src/i18n/strings/en_EN.json | 3 ++- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/components/views/elements/MiniAvatarUploader.tsx b/src/components/views/elements/MiniAvatarUploader.tsx index 8bbca5b309b..dc5f1f966c6 100644 --- a/src/components/views/elements/MiniAvatarUploader.tsx +++ b/src/components/views/elements/MiniAvatarUploader.tsx @@ -17,6 +17,9 @@ import { useTimeout } from "../../../hooks/useTimeout"; import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds"; import AccessibleButton from "./AccessibleButton"; import Spinner from "./Spinner"; +import Modal from "../../../Modal.tsx"; +import ErrorDialog from "../dialogs/ErrorDialog.tsx"; +import { _t } from "../../../languageHandler.tsx"; export const AVATAR_SIZE = "52px"; @@ -75,8 +78,15 @@ const MiniAvatarUploader: React.FC = ({ if (!ev.target.files?.length) return; setBusy(true); const file = ev.target.files[0]; - const { content_uri: uri } = await cli.uploadContent(file); - await setAvatarUrl(uri); + if (file.type.startsWith("image/")) { + const { content_uri: uri } = await cli.uploadContent(file); + await setAvatarUrl(uri); + } else { + Modal.createDialog(ErrorDialog, { + title: _t("upload_failed_title"), + description: _t("upload_file|not_image"), + }); + } setBusy(false); }} accept="image/*" diff --git a/src/components/views/settings/AvatarSetting.tsx b/src/components/views/settings/AvatarSetting.tsx index eaeabc641b9..8e5ef5d62ce 100644 --- a/src/components/views/settings/AvatarSetting.tsx +++ b/src/components/views/settings/AvatarSetting.tsx @@ -19,6 +19,8 @@ import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds"; import { useId } from "../../../utils/useId"; import AccessibleButton from "../elements/AccessibleButton"; import BaseAvatar from "../avatars/BaseAvatar"; +import Modal from "../../../Modal.tsx"; +import ErrorDialog from "../dialogs/ErrorDialog.tsx"; interface MenuProps { trigger: ReactNode; @@ -80,7 +82,7 @@ interface IProps { * Called when the user has selected a new avatar * The callback is passed a File object for the new avatar data */ - onChange?: (f: File) => void; + onChange: (f: File) => void; /** * Called when the user wishes to remove the avatar @@ -139,7 +141,16 @@ const AvatarSetting: React.FC = ({ const onFileChanged = useCallback( (e: React.ChangeEvent) => { - if (e.target.files) onChange?.(e.target.files[0]); + if (!e.target.files?.length) return; + const file = e.target.files[0]; + if (file.type.startsWith("image/")) { + onChange(file); + } else { + Modal.createDialog(ErrorDialog, { + title: _t("upload_failed_title"), + description: _t("upload_file|not_image"), + }); + } }, [onChange], ); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 4a524db97cd..5218e8ccbe1 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3748,7 +3748,8 @@ "upload_n_others_button": { "one": "Upload %(count)s other file", "other": "Upload %(count)s other files" - } + }, + "not_image": "The file you have chosen is not a valid image file." }, "user_info": { "admin_tools_section": "Admin Tools", From 315afdead1ec76dc1e333d144bb2f4d2909b22e5 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 14 Nov 2024 17:24:05 +0000 Subject: [PATCH 2/7] i18n Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/i18n/strings/en_EN.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 5218e8ccbe1..3b4765b0ad2 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3742,14 +3742,14 @@ "error_files_too_large": "These files are too large to upload. The file size limit is %(limit)s.", "error_some_files_too_large": "Some files are too large to be uploaded. The file size limit is %(limit)s.", "error_title": "Upload Error", + "not_image": "The file you have chosen is not a valid image file.", "title": "Upload files", "title_progress": "Upload files (%(current)s of %(total)s)", "upload_all_button": "Upload all", "upload_n_others_button": { "one": "Upload %(count)s other file", "other": "Upload %(count)s other files" - }, - "not_image": "The file you have chosen is not a valid image file." + } }, "user_info": { "admin_tools_section": "Admin Tools", From 30a6048aa2b99c368f01dd346dc4b37c15c50416 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 14 Nov 2024 17:34:13 +0000 Subject: [PATCH 3/7] Optional Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/settings/AvatarSetting.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/settings/AvatarSetting.tsx b/src/components/views/settings/AvatarSetting.tsx index 8e5ef5d62ce..9b500a51e23 100644 --- a/src/components/views/settings/AvatarSetting.tsx +++ b/src/components/views/settings/AvatarSetting.tsx @@ -82,7 +82,7 @@ interface IProps { * Called when the user has selected a new avatar * The callback is passed a File object for the new avatar data */ - onChange: (f: File) => void; + onChange?: (f: File) => void; /** * Called when the user wishes to remove the avatar @@ -144,7 +144,7 @@ const AvatarSetting: React.FC = ({ if (!e.target.files?.length) return; const file = e.target.files[0]; if (file.type.startsWith("image/")) { - onChange(file); + onChange?.(file); } else { Modal.createDialog(ErrorDialog, { title: _t("upload_failed_title"), From 175d35e2984308314940c8c26c2da934a019c94d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 15 Nov 2024 09:14:52 +0000 Subject: [PATCH 4/7] Improve coverage Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/settings/AvatarSetting-test.tsx | 46 ++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) diff --git a/test/unit-tests/components/views/settings/AvatarSetting-test.tsx b/test/unit-tests/components/views/settings/AvatarSetting-test.tsx index e3e2b1cf96d..613affad26a 100644 --- a/test/unit-tests/components/views/settings/AvatarSetting-test.tsx +++ b/test/unit-tests/components/views/settings/AvatarSetting-test.tsx @@ -6,7 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only Please see LICENSE files in the repository root for full details. */ import React from "react"; -import { render, screen } from "jest-matrix-react"; +import { render, screen, fireEvent } from "jest-matrix-react"; import userEvent from "@testing-library/user-event"; import AvatarSetting from "../../../../../src/components/views/settings/AvatarSetting"; @@ -16,6 +16,9 @@ const BASE64_GIF = "R0lGODlhAQABAAAAACw="; const AVATAR_FILE = new File([Uint8Array.from(atob(BASE64_GIF), (c) => c.charCodeAt(0))], "avatar.gif", { type: "image/gif", }); +const GENERIC_FILE = new File([Uint8Array.from(atob(BASE64_GIF), (c) => c.charCodeAt(0))], "not-avatar.doc", { + type: "application/msword", +}); describe("", () => { beforeEach(() => { @@ -70,4 +73,45 @@ describe("", () => { expect(onChange).toHaveBeenCalledWith(AVATAR_FILE); }); + + it("should noop when selecting no file", async () => { + const onChange = jest.fn(); + const user = userEvent.setup(); + + render( + , + ); + + const fileInput = screen.getByAltText("Upload"); + await user.upload(fileInput, []); + + expect(onChange).not.toHaveBeenCalled(); + }); + + it("should show error if user tries to use non-image file", async () => { + const onChange = jest.fn(); + + render( + , + ); + + const fileInput = screen.getByAltText("Upload"); + // Can't use userEvent.upload here as it doesn't support uploading invalid files + fireEvent.change(fileInput, { target: { files: [GENERIC_FILE] } }); + + expect(onChange).not.toHaveBeenCalled(); + await expect(screen.findByRole("heading", { name: "Upload Failed" })).resolves.toBeInTheDocument(); + }); }); From 9cb340e98327b54cb224d37793c582ec5604e5bb Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 15 Nov 2024 09:30:13 +0000 Subject: [PATCH 5/7] DRY Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../views/elements/MiniAvatarUploader.tsx | 14 +++--------- .../views/settings/AvatarSetting.tsx | 22 ++++++++++++------- .../views/settings/AvatarSetting-test.tsx | 4 ++-- 3 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/components/views/elements/MiniAvatarUploader.tsx b/src/components/views/elements/MiniAvatarUploader.tsx index dc5f1f966c6..cf5a2398148 100644 --- a/src/components/views/elements/MiniAvatarUploader.tsx +++ b/src/components/views/elements/MiniAvatarUploader.tsx @@ -17,9 +17,7 @@ import { useTimeout } from "../../../hooks/useTimeout"; import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds"; import AccessibleButton from "./AccessibleButton"; import Spinner from "./Spinner"; -import Modal from "../../../Modal.tsx"; -import ErrorDialog from "../dialogs/ErrorDialog.tsx"; -import { _t } from "../../../languageHandler.tsx"; +import { getFileChanged } from "../settings/AvatarSetting.tsx"; export const AVATAR_SIZE = "52px"; @@ -75,17 +73,11 @@ const MiniAvatarUploader: React.FC = ({ onClick?.(ev); }} onChange={async (ev): Promise => { - if (!ev.target.files?.length) return; setBusy(true); - const file = ev.target.files[0]; - if (file.type.startsWith("image/")) { + const file = getFileChanged(ev); + if (file) { const { content_uri: uri } = await cli.uploadContent(file); await setAvatarUrl(uri); - } else { - Modal.createDialog(ErrorDialog, { - title: _t("upload_failed_title"), - description: _t("upload_file|not_image"), - }); } setBusy(false); }} diff --git a/src/components/views/settings/AvatarSetting.tsx b/src/components/views/settings/AvatarSetting.tsx index 9b500a51e23..a64c084ba8c 100644 --- a/src/components/views/settings/AvatarSetting.tsx +++ b/src/components/views/settings/AvatarSetting.tsx @@ -105,6 +105,18 @@ interface IProps { placeholderName: string; } +export function getFileChanged(e: React.ChangeEvent): File | null { + if (!e.target.files?.length) return null; + const file = e.target.files[0]; + if (!file.type.startsWith("image/")) { + Modal.createDialog(ErrorDialog, { + title: _t("upload_failed_title"), + description: _t("upload_file|not_image"), + }); + } + return file; +} + /** * Component for setting or removing an avatar on something (eg. a user or a room) */ @@ -141,15 +153,9 @@ const AvatarSetting: React.FC = ({ const onFileChanged = useCallback( (e: React.ChangeEvent) => { - if (!e.target.files?.length) return; - const file = e.target.files[0]; - if (file.type.startsWith("image/")) { + const file = getFileChanged(e); + if (file) { onChange?.(file); - } else { - Modal.createDialog(ErrorDialog, { - title: _t("upload_failed_title"), - description: _t("upload_file|not_image"), - }); } }, [onChange], diff --git a/test/unit-tests/components/views/settings/AvatarSetting-test.tsx b/test/unit-tests/components/views/settings/AvatarSetting-test.tsx index 613affad26a..1b88c416bc5 100644 --- a/test/unit-tests/components/views/settings/AvatarSetting-test.tsx +++ b/test/unit-tests/components/views/settings/AvatarSetting-test.tsx @@ -76,7 +76,6 @@ describe("", () => { it("should noop when selecting no file", async () => { const onChange = jest.fn(); - const user = userEvent.setup(); render( ", () => { ); const fileInput = screen.getByAltText("Upload"); - await user.upload(fileInput, []); + // Can't use userEvent.upload here as it doesn't support uploading invalid files + fireEvent.change(fileInput, { target: { files: [] } }); expect(onChange).not.toHaveBeenCalled(); }); From 29a1c4b501b71777b8da297c877d89d8d76c5d9a Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 15 Nov 2024 09:38:31 +0000 Subject: [PATCH 6/7] Iterate Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- src/components/views/settings/AvatarSetting.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/src/components/views/settings/AvatarSetting.tsx b/src/components/views/settings/AvatarSetting.tsx index a64c084ba8c..633d5930e5d 100644 --- a/src/components/views/settings/AvatarSetting.tsx +++ b/src/components/views/settings/AvatarSetting.tsx @@ -113,6 +113,7 @@ export function getFileChanged(e: React.ChangeEvent): File | n title: _t("upload_failed_title"), description: _t("upload_file|not_image"), }); + return null; } return file; } From 1457a2b3065a8cf9619dc2dd802312bdbe350155 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Fri, 15 Nov 2024 11:30:17 +0000 Subject: [PATCH 7/7] Improve coverage Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --- .../elements/MiniAvatarUploader-test.tsx | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 test/unit-tests/components/views/elements/MiniAvatarUploader-test.tsx diff --git a/test/unit-tests/components/views/elements/MiniAvatarUploader-test.tsx b/test/unit-tests/components/views/elements/MiniAvatarUploader-test.tsx new file mode 100644 index 00000000000..cf6ed6ae624 --- /dev/null +++ b/test/unit-tests/components/views/elements/MiniAvatarUploader-test.tsx @@ -0,0 +1,40 @@ +/* +Copyright 2024 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +import React from "react"; +import { render } from "jest-matrix-react"; +import userEvent from "@testing-library/user-event"; +import { mocked } from "jest-mock"; + +import MiniAvatarUploader from "../../../../../src/components/views/elements/MiniAvatarUploader.tsx"; +import { stubClient, withClientContextRenderOptions } from "../../../../test-utils"; + +const BASE64_GIF = "R0lGODlhAQABAAAAACw="; +const AVATAR_FILE = new File([Uint8Array.from(atob(BASE64_GIF), (c) => c.charCodeAt(0))], "avatar.gif", { + type: "image/gif", +}); + +describe("", () => { + it("calls setAvatarUrl when a file is uploaded", async () => { + const cli = stubClient(); + mocked(cli.uploadContent).mockResolvedValue({ content_uri: "mxc://example.com/1234" }); + + const setAvatarUrl = jest.fn(); + const user = userEvent.setup(); + + const { container, findByText } = render( + , + withClientContextRenderOptions(cli), + ); + + await findByText("Upload"); + await user.upload(container.querySelector("input")!, AVATAR_FILE); + + expect(cli.uploadContent).toHaveBeenCalledWith(AVATAR_FILE); + expect(setAvatarUrl).toHaveBeenCalledWith("mxc://example.com/1234"); + }); +});