Check that the file the user chose has a MIME type of image/* (#28467)

* Check that the file the user chose has a MIME type of `image/*` Signed-off-by: Michael Telatynski <[email protected]> * i18n Signed-off-by: Michael Telatynski <[email protected]> * Optional Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * DRY Signed-off-by: Michael Telatynski <[email protected]> * Iterate Signed-off-by: Michael Telatynski <[email protected]> * Improve coverage Signed-off-by: Michael Telatynski <[email protected]> * Update src/components/views/settings/AvatarSetting.tsx Co-authored-by: Florian Duros <[email protected]> * prettier Signed-off-by: Michael Telatynski <[email protected]> --------- Signed-off-by: Michael Telatynski <[email protected]> Co-authored-by: Florian Duros <[email protected]>
element-hq · Nov 18, 2024 · f8b9368 · f8b9368
1 parent abf6d58
commit f8b9368
Show file tree

Hide file tree

Showing 5 changed files with 110 additions and 6 deletions.
diff --git a/src/components/views/elements/MiniAvatarUploader.tsx b/src/components/views/elements/MiniAvatarUploader.tsx
@@ -17,6 +17,7 @@ import { useTimeout } from "../../../hooks/useTimeout";
 import { chromeFileInputFix } from "../../../utils/BrowserWorkarounds";
 import AccessibleButton from "./AccessibleButton";
 import Spinner from "./Spinner";
+import { getFileChanged } from "../settings/AvatarSetting.tsx";
 
 export const AVATAR_SIZE = "52px";
 
@@ -72,11 +73,12 @@ const MiniAvatarUploader: React.FC<IProps> = ({
                     onClick?.(ev);
                 }}
                 onChange={async (ev): Promise<void> => {
-                    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);
+                    const file = getFileChanged(ev);
+                    if (file) {
+                        const { content_uri: uri } = await cli.uploadContent(file);
+                        await setAvatarUrl(uri);
+                    }
                     setBusy(false);
                 }}
                 accept="image/*"

diff --git 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;
@@ -103,6 +105,18 @@ interface IProps {
     placeholderName: string;
 }
 
+export function getFileChanged(e: React.ChangeEvent<HTMLInputElement>): File | null {
+    if (!e.target.files?.length) return null;
+    const file = e.target.files[0];
+    if (file.type.startsWith("image/")) return file;
+
+    Modal.createDialog(ErrorDialog, {
+        title: _t("upload_failed_title"),
+        description: _t("upload_file|not_image"),
+    });
+    return null;
+}
+
 /**
  * Component for setting or removing an avatar on something (eg. a user or a room)
  */
@@ -139,7 +153,10 @@ const AvatarSetting: React.FC<IProps> = ({
 
     const onFileChanged = useCallback(
         (e: React.ChangeEvent<HTMLInputElement>) => {
-            if (e.target.files) onChange?.(e.target.files[0]);
+            const file = getFileChanged(e);
+            if (file) {
+                onChange?.(file);
+            }
         },
         [onChange],
     );

diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json
@@ -3742,6 +3742,7 @@
         "error_files_too_large": "These files are <b>too large</b> to upload. The file size limit is %(limit)s.",
         "error_some_files_too_large": "Some files are <b>too large</b> 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",

diff --git a/test/unit-tests/components/views/elements/MiniAvatarUploader-test.tsx 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("<MiniAvatarUploader />", () => {
+    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(
+            <MiniAvatarUploader hasAvatar={false} noAvatarLabel="Upload" setAvatarUrl={setAvatarUrl} isUserAvatar />,
+            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");
+    });
+});
diff --git 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("<AvatarSetting />", () => {
     beforeEach(() => {
@@ -70,4 +73,45 @@ describe("<AvatarSetting />", () => {
 
         expect(onChange).toHaveBeenCalledWith(AVATAR_FILE);
     });
+
+    it("should noop when selecting no file", async () => {
+        const onChange = jest.fn();
+
+        render(
+            <AvatarSetting
+                placeholderId="blee"
+                placeholderName="boo"
+                avatar="mxc://example.org/my-avatar"
+                avatarAltText="Avatar of Peter Fox"
+                onChange={onChange}
+            />,
+        );
+
+        const fileInput = screen.getByAltText("Upload");
+        // Can't use userEvent.upload here as it doesn't support uploading invalid files
+        fireEvent.change(fileInput, { target: { files: [] } });
+
+        expect(onChange).not.toHaveBeenCalled();
+    });
+
+    it("should show error if user tries to use non-image file", async () => {
+        const onChange = jest.fn();
+
+        render(
+            <AvatarSetting
+                placeholderId="blee"
+                placeholderName="boo"
+                avatar="mxc://example.org/my-avatar"
+                avatarAltText="Avatar of Peter Fox"
+                onChange={onChange}
+            />,
+        );
+
+        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();
+    });
 });