From 2a72028ce64d129ea7cd7c8ce84d1e54f966bbdf Mon Sep 17 00:00:00 2001 From: "Julien Carion (juca)" Date: Thu, 6 Jul 2023 11:40:54 +0000 Subject: [PATCH] [FIX] web: prevent upload of large files This commit ensures that files uploaded using the FileInput, upload service or AttachDocumentWidget will not exceed the maximum file size of the context in size before sending them to the server. It also moves some utils from the formatters file to the numbers file so that one can use them without requiring the view assets. task-3390746 closes odoo/odoo#126914 Related: odoo/enterprise#45629 Signed-off-by: Samuel Degueldre (sad) --- .../static/src/core/file_input/file_input.js | 23 ++++-- addons/web/static/src/core/utils/files.js | 31 ++++++++ addons/web/static/src/core/utils/numbers.js | 76 ++++++++++++++++++ .../static/src/views/fields/file_handler.js | 18 +---- .../web/static/src/views/fields/formatters.js | 77 +------------------ .../attach_document/attach_document.js | 11 ++- .../web/static/tests/core/file_input_tests.js | 56 ++++++++++++-- .../upload_progress_toast/upload_service.js | 16 ++-- 8 files changed, 196 insertions(+), 112 deletions(-) create mode 100644 addons/web/static/src/core/utils/files.js diff --git a/addons/web/static/src/core/file_input/file_input.js b/addons/web/static/src/core/file_input/file_input.js index 70159beecd7..c2a9badf00c 100644 --- a/addons/web/static/src/core/file_input/file_input.js +++ b/addons/web/static/src/core/file_input/file_input.js @@ -3,6 +3,7 @@ import { useService } from "@web/core/utils/hooks"; import { Component, onMounted, useRef, useState } from "@odoo/owl"; +import { checkFileSize } from "@web/core/utils/files"; /** * Custom file input @@ -25,6 +26,7 @@ import { Component, onMounted, useRef, useState } from "@odoo/owl"; export class FileInput extends Component { setup() { this.http = useService("http"); + this.notification = useService("notification"); this.fileInputRef = useRef("file-input"); this.state = useState({ // Disables upload button if currently uploading. @@ -54,6 +56,12 @@ export class FileInput extends Component { } async uploadFiles(params) { + if (params.ufile.length) { + const fileSize = params.ufile[0].size; + if (!checkFileSize(fileSize, this.notification)) { + return null; + } + } const fileData = await this.http.post(this.props.route, params, "text"); const parsedFileData = JSON.parse(fileData); if (parsedFileData.error) { @@ -76,11 +84,16 @@ export class FileInput extends Component { async onFileInputChange() { this.state.isDisable = true; const parsedFileData = await this.uploadFiles(this.httpParams); - // When calling onUpload, also pass the files to allow to get data like their names - this.props.onUpload(parsedFileData, this.fileInputRef.el ? this.fileInputRef.el.files : []); - // Because the input would not trigger this method if the same file name is uploaded, - // we must clear the value after handling the upload - this.fileInputRef.el.value = null; + if (parsedFileData) { + // When calling onUpload, also pass the files to allow to get data like their names + this.props.onUpload( + parsedFileData, + this.fileInputRef.el ? this.fileInputRef.el.files : [] + ); + // Because the input would not trigger this method if the same file name is uploaded, + // we must clear the value after handling the upload + this.fileInputRef.el.value = null; + } this.state.isDisable = false; } diff --git a/addons/web/static/src/core/utils/files.js b/addons/web/static/src/core/utils/files.js new file mode 100644 index 00000000000..4f615480312 --- /dev/null +++ b/addons/web/static/src/core/utils/files.js @@ -0,0 +1,31 @@ +/** @odoo-module **/ + +import { humanNumber } from "@web/core/utils/numbers"; +import { session } from "@web/session"; +import { _t } from "@web/core/l10n/translation"; + +const DEFAULT_MAX_FILE_SIZE = 128 * 1024 * 1024; + +/** + * @param {Services["notification"]} notificationService + * @param {File} file + * @param {Number} maxUploadSize + * @returns {boolean} + */ +export function checkFileSize(fileSize, notificationService) { + const maxUploadSize = session.max_file_upload_size || DEFAULT_MAX_FILE_SIZE; + if (fileSize > maxUploadSize) { + notificationService.add( + _t( + "The selected file (%sB) is over the maximum allowed file size (%sB).", + humanNumber(fileSize), + humanNumber(maxUploadSize) + ), + { + type: "danger", + } + ); + return false; + } + return true; +} diff --git a/addons/web/static/src/core/utils/numbers.js b/addons/web/static/src/core/utils/numbers.js index cf78c198895..05868ecaae0 100644 --- a/addons/web/static/src/core/utils/numbers.js +++ b/addons/web/static/src/core/utils/numbers.js @@ -1,5 +1,9 @@ /** @odoo-module **/ +import { localization as l10n } from "@web/core/l10n/localization"; +import { _t } from "@web/core/l10n/translation"; +import { intersperse } from "@web/core/utils/strings"; + /** * Returns value clamped to the inclusive range of min and max. * @@ -80,3 +84,75 @@ export function roundDecimals(value, decimals) { export function floatIsZero(value, decimals) { return roundDecimals(value, decimals) === 0; } + +/** + * Inserts "thousands" separators in the provided number. + * + * @param {string} string representing integer number + * @param {string} [thousandsSep=","] the separator to insert + * @param {number[]} [grouping=[]] + * array of relative offsets at which to insert `thousandsSep`. + * See `strings.intersperse` method. + * @returns {string} + */ +export function insertThousandsSep(number, thousandsSep = ",", grouping = []) { + const negative = number[0] === "-"; + number = negative ? number.slice(1) : number; + return (negative ? "-" : "") + intersperse(number, grouping, thousandsSep); +} + +/** + * Format a number to a human readable format. For example, 3000 could become 3k. + * Or massive number can use the scientific exponential notation. + * + * @param {number} number to format + * @param {Object} [options] Options to format + * @param {number} [options.decimals=0] number of decimals to use + * if minDigits > 1 is used and effective on the number then decimals + * will be shrunk to zero, to avoid displaying irrelevant figures ( 0.01 compared to 1000 ) + * @param {number} [options.minDigits=1] + * the minimum number of digits to preserve when switching to another + * level of thousands (e.g. with a value of '2', 4321 will still be + * represented as 4321 otherwise it will be down to one digit (4k)) + * @returns {string} + */ +export function humanNumber(number, options = { decimals: 0, minDigits: 1 }) { + const decimals = options.decimals || 0; + const minDigits = options.minDigits || 1; + const d2 = Math.pow(10, decimals); + const numberMagnitude = +number.toExponential().split("e+")[1]; + number = Math.round(number * d2) / d2; + // the case numberMagnitude >= 21 corresponds to a number + // better expressed in the scientific format. + if (numberMagnitude >= 21) { + // we do not use number.toExponential(decimals) because we want to + // avoid the possible useless O decimals: 1e.+24 preferred to 1.0e+24 + number = Math.round(number * Math.pow(10, decimals - numberMagnitude)) / d2; + return `${number}e+${numberMagnitude}`; + } + // note: we need to call toString here to make sure we manipulate the resulting + // string, not an object with a toString method. + const unitSymbols = _t("kMGTPE").toString(); + const sign = Math.sign(number); + number = Math.abs(number); + let symbol = ""; + for (let i = unitSymbols.length; i > 0; i--) { + const s = Math.pow(10, i * 3); + if (s <= number / Math.pow(10, minDigits - 1)) { + number = Math.round((number * d2) / s) / d2; + symbol = unitSymbols[i - 1]; + break; + } + } + const { decimalPoint, grouping, thousandsSep } = l10n; + + // determine if we should keep the decimals (we don't want to display 1,020.02k for 1020020) + const decimalsToKeep = number >= 1000 ? 0 : decimals; + number = sign * number; + const [integerPart, decimalPart] = number.toFixed(decimalsToKeep).split("."); + const int = insertThousandsSep(integerPart, thousandsSep, grouping); + if (!decimalPart) { + return int + symbol; + } + return int + decimalPoint + decimalPart + symbol; +} diff --git a/addons/web/static/src/views/fields/file_handler.js b/addons/web/static/src/views/fields/file_handler.js index 5592128fe9c..5fcae8fc728 100644 --- a/addons/web/static/src/views/fields/file_handler.js +++ b/addons/web/static/src/views/fields/file_handler.js @@ -2,13 +2,10 @@ import { useService } from "@web/core/utils/hooks"; import { getDataURLFromFile } from "@web/core/utils/urls"; -import { session } from "@web/session"; -import { formatFloat } from "./formatters"; +import { checkFileSize } from "@web/core/utils/files"; import { Component, useRef, useState } from "@odoo/owl"; -const DEFAULT_MAX_FILE_SIZE = 128 * 1024 * 1024; - export class FileUploader extends Component { setup() { this.notification = useService("notification"); @@ -18,9 +15,6 @@ export class FileUploader extends Component { }); } - get maxUploadSize() { - return session.max_file_upload_size || DEFAULT_MAX_FILE_SIZE; - } /** * @param {Event} ev */ @@ -29,14 +23,8 @@ export class FileUploader extends Component { return; } for (const file of ev.target.files) { - if (file.size > this.maxUploadSize) { - this.notification.add( - this.env._t( - "The selected file exceed the maximum file size of %s.", - formatFloat(this.maxUploadSize, { humanReadable: true }) - ), - { title: this.env._t("File upload"), type: "danger" } - ); + if (!checkFileSize(file.size, this.notification)) { + return null; } this.state.isUploading = true; const data = await getDataURLFromFile(file); diff --git a/addons/web/static/src/views/fields/formatters.js b/addons/web/static/src/views/fields/formatters.js index d8c6e9dfe98..c0bd53b57b7 100644 --- a/addons/web/static/src/views/fields/formatters.js +++ b/addons/web/static/src/views/fields/formatters.js @@ -4,8 +4,9 @@ import { formatDate, formatDateTime } from "@web/core/l10n/dates"; import { localization as l10n } from "@web/core/l10n/localization"; import { _t } from "@web/core/l10n/translation"; import { registry } from "@web/core/registry"; -import { escape, intersperse, nbsp } from "@web/core/utils/strings"; +import { escape, nbsp } from "@web/core/utils/strings"; import { isBinarySize } from "@web/core/utils/binary"; +import { humanNumber, insertThousandsSep } from "@web/core/utils/numbers"; import { markup } from "@odoo/owl"; import { getCurrency } from "@web/core/currency"; @@ -14,80 +15,6 @@ import { getCurrency } from "@web/core/currency"; // Helpers // ----------------------------------------------------------------------------- -/** - * Inserts "thousands" separators in the provided number. - * - * @private - * @param {string} string representing integer number - * @param {string} [thousandsSep=","] the separator to insert - * @param {number[]} [grouping=[]] - * array of relative offsets at which to insert `thousandsSep`. - * See `strings.intersperse` method. - * @returns {string} - */ -function insertThousandsSep(number, thousandsSep = ",", grouping = []) { - const negative = number[0] === "-"; - number = negative ? number.slice(1) : number; - return (negative ? "-" : "") + intersperse(number, grouping, thousandsSep); -} - -/** - * Format a number to a human readable format. For example, 3000 could become 3k. - * Or massive number can use the scientific exponential notation. - * - * @private - * @param {number} number to format - * @param {Object} [options] Options to format - * @param {number} [options.decimals=0] number of decimals to use - * if minDigits > 1 is used and effective on the number then decimals - * will be shrunk to zero, to avoid displaying irrelevant figures ( 0.01 compared to 1000 ) - * @param {number} [options.minDigits=1] - * the minimum number of digits to preserve when switching to another - * level of thousands (e.g. with a value of '2', 4321 will still be - * represented as 4321 otherwise it will be down to one digit (4k)) - * @returns {string} - */ -function humanNumber(number, options = { decimals: 0, minDigits: 1 }) { - const decimals = options.decimals || 0; - const minDigits = options.minDigits || 1; - const d2 = Math.pow(10, decimals); - const numberMagnitude = +number.toExponential().split("e+")[1]; - number = Math.round(number * d2) / d2; - // the case numberMagnitude >= 21 corresponds to a number - // better expressed in the scientific format. - if (numberMagnitude >= 21) { - // we do not use number.toExponential(decimals) because we want to - // avoid the possible useless O decimals: 1e.+24 preferred to 1.0e+24 - number = Math.round(number * Math.pow(10, decimals - numberMagnitude)) / d2; - return `${number}e+${numberMagnitude}`; - } - // note: we need to call toString here to make sure we manipulate the resulting - // string, not an object with a toString method. - const unitSymbols = _t("kMGTPE").toString(); - const sign = Math.sign(number); - number = Math.abs(number); - let symbol = ""; - for (let i = unitSymbols.length; i > 0; i--) { - const s = Math.pow(10, i * 3); - if (s <= number / Math.pow(10, minDigits - 1)) { - number = Math.round((number * d2) / s) / d2; - symbol = unitSymbols[i - 1]; - break; - } - } - const { decimalPoint, grouping, thousandsSep } = l10n; - - // determine if we should keep the decimals (we don't want to display 1,020.02k for 1020020) - const decimalsToKeep = number >= 1000 ? 0 : decimals; - number = sign * number; - const [integerPart, decimalPart] = number.toFixed(decimalsToKeep).split("."); - const int = insertThousandsSep(integerPart, thousandsSep, grouping); - if (!decimalPart) { - return int + symbol; - } - return int + decimalPoint + decimalPart + symbol; -} - function humanSize(value) { if (!value) { return ""; diff --git a/addons/web/static/src/views/widgets/attach_document/attach_document.js b/addons/web/static/src/views/widgets/attach_document/attach_document.js index 030f3884598..fafb0ca1479 100644 --- a/addons/web/static/src/views/widgets/attach_document/attach_document.js +++ b/addons/web/static/src/views/widgets/attach_document/attach_document.js @@ -3,8 +3,8 @@ import { FileInput } from "@web/core/file_input/file_input"; import { registry } from "@web/core/registry"; import { useService } from "@web/core/utils/hooks"; +import { checkFileSize } from "@web/core/utils/files"; import { standardWidgetProps } from "@web/views/widgets/standard_widget_props"; - import { Component } from "@odoo/owl"; export class AttachDocumentWidget extends Component { @@ -22,7 +22,6 @@ export class AttachDocumentWidget extends Component { setup() { this.http = useService("http"); this.notification = useService("notification"); - this.fileInput = document.createElement("input"); this.fileInput.type = "file"; this.fileInput.accept = "*"; @@ -31,11 +30,17 @@ export class AttachDocumentWidget extends Component { } async onInputChange() { + const ufile = [...this.fileInput.files]; + for (const file of ufile) { + if (!checkFileSize(file.size, this.notification)) { + return null; + } + } const fileData = await this.http.post( "/web/binary/upload_attachment", { csrf_token: odoo.csrf_token, - ufile: [...this.fileInput.files], + ufile: ufile, model: this.props.record.resModel, id: this.props.record.resId, }, diff --git a/addons/web/static/tests/core/file_input_tests.js b/addons/web/static/tests/core/file_input_tests.js index d346d424d2f..5547da29d99 100644 --- a/addons/web/static/tests/core/file_input_tests.js +++ b/addons/web/static/tests/core/file_input_tests.js @@ -1,9 +1,19 @@ /** @odoo-module **/ import { makeTestEnv } from "@web/../tests/helpers/mock_env"; -import { editInput, getFixture, mount, patchWithCleanup, triggerEvent, makeDeferred, nextTick } from "@web/../tests/helpers/utils"; +import { + editInput, + getFixture, + mount, + patchWithCleanup, + triggerEvent, + makeDeferred, + nextTick, +} from "@web/../tests/helpers/utils"; import { FileInput } from "@web/core/file_input/file_input"; import { registry } from "@web/core/registry"; +import { session } from "@web/session"; +import { makeFakeLocalizationService } from "@web/../tests/helpers/mock_services"; const serviceRegistry = registry.category("services"); @@ -13,7 +23,12 @@ let target; // Helpers // ----------------------------------------------------------------------------- -async function createFileInput({ mockPost, props }) { +async function createFileInput({ mockPost, mockAdd, props }) { + serviceRegistry.add("notification", { + start: () => ({ + add: mockAdd || (() => {}), + }), + }); serviceRegistry.add("http", { start: () => ({ post: mockPost || (() => {}), @@ -121,13 +136,11 @@ QUnit.module("Components", ({ beforeEach }) => { await createFileInput({ props: { onUpload(files) { - assert.step( - files[0].name - ); + assert.step(files[0].name); }, }, mockPost: (route, params) => { - return JSON.stringify([{name: params.ufile[0].name}]); + return JSON.stringify([{ name: params.ufile[0].name }]); }, }); @@ -139,6 +152,37 @@ QUnit.module("Components", ({ beforeEach }) => { assert.verifySteps(["fake_file.txt"], "file has been uploaded a second time"); }); + QUnit.test("uploading a file that is too heavy will send a notification", async (assert) => { + serviceRegistry.add("localization", makeFakeLocalizationService()); + patchWithCleanup(session, { max_file_upload_size: 2 }); + await createFileInput({ + props: { + onUpload(files) { + // This code should be unreachable in this case + assert.step(files[0].name); + }, + }, + mockPost: (route, params) => { + return JSON.stringify([{ name: params.ufile[0].name }]); + }, + mockAdd: (message) => { + assert.step("notification"); + // Message is a bit weird because values (2 and 4 bytes) are simplified to 2 decimals in regards to megabytes + assert.strictEqual( + message, + "The selected file (4B) is over the maximum allowed file size (2B)." + ); + }, + }); + + const file = new File(["test"], "fake_file.txt", { type: "text/plain" }); + await editInput(target, ".o_file_input input", file); + assert.verifySteps( + ["notification"], + "Only the notification will be triggered and the file won't be uploaded." + ); + }); + QUnit.test("Upload button is disabled if attachment upload is not finished", async (assert) => { assert.expect(2); diff --git a/addons/web_editor/static/src/components/upload_progress_toast/upload_service.js b/addons/web_editor/static/src/components/upload_progress_toast/upload_service.js index 1ccaa373128..8aa461caf4b 100644 --- a/addons/web_editor/static/src/components/upload_progress_toast/upload_service.js +++ b/addons/web_editor/static/src/components/upload_progress_toast/upload_service.js @@ -3,14 +3,15 @@ import { registry } from '@web/core/registry'; import { UploadProgressToast } from './upload_progress_toast'; import utils from '@web/legacy/js/core/utils'; - +import { checkFileSize } from "@web/core/utils/files"; +import { humanNumber } from "@web/core/utils/numbers"; import { reactive } from "@odoo/owl"; export const AUTOCLOSE_DELAY = 3000; export const uploadService = { - dependencies: ['rpc'], - start(env, { rpc }) { + dependencies: ['rpc', 'notification'], + start(env, { rpc, notification }) { let fileId = 0; const progressToast = reactive({ files: {}, @@ -69,14 +70,13 @@ export const uploadService = { const sortedFiles = Array.from(files).sort((a, b) => a.size - b.size); for (const file of sortedFiles) { let fileSize = file.size; + if (!checkFileSize(fileSize, notification)) { + return null; + } if (!fileSize) { fileSize = null; - } else if (fileSize < 1024) { - fileSize = fileSize.toFixed(2) + " bytes"; - } else if (fileSize < 1048576) { - fileSize = (fileSize / 1024).toFixed(2) + " KB"; } else { - fileSize = (fileSize / 1048576).toFixed(2) + " MB"; + fileSize = humanNumber(fileSize) + "B"; } const id = ++fileId;