From 8c5199b253f80ff84adbc2fd61e533d86af94ffb Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 23 Nov 2023 19:51:58 +0200 Subject: [PATCH 1/5] Make file.name required in attachment prop types Some bugs can be caught earlier if we're warned when `file` or `file.name` is missing --- src/components/Attachments/AttachmentCarousel/CarouselItem.js | 4 ++-- src/components/Attachments/propTypes.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/CarouselItem.js b/src/components/Attachments/AttachmentCarousel/CarouselItem.js index b6cc0cbf21a4..fbc49590d5ae 100644 --- a/src/components/Attachments/AttachmentCarousel/CarouselItem.js +++ b/src/components/Attachments/AttachmentCarousel/CarouselItem.js @@ -27,8 +27,8 @@ const propTypes = { /** Additional information about the attachment file */ file: PropTypes.shape({ /** File name of the attachment */ - name: PropTypes.string, - }), + name: PropTypes.string.isRequired, + }).isRequired, /** Whether the attachment has been flagged */ hasBeenFlagged: PropTypes.bool, diff --git a/src/components/Attachments/propTypes.js b/src/components/Attachments/propTypes.js index 698a41de9648..13adc468ce64 100644 --- a/src/components/Attachments/propTypes.js +++ b/src/components/Attachments/propTypes.js @@ -2,7 +2,7 @@ import PropTypes from 'prop-types'; const attachmentSourcePropType = PropTypes.oneOfType([PropTypes.string, PropTypes.func, PropTypes.number]); const attachmentFilePropType = PropTypes.shape({ - name: PropTypes.string, + name: PropTypes.string.isRequired, }); const attachmentPropType = PropTypes.shape({ @@ -13,7 +13,7 @@ const attachmentPropType = PropTypes.shape({ source: attachmentSourcePropType.isRequired, /** File object can be an instance of File or Object */ - file: attachmentFilePropType, + file: attachmentFilePropType.isRequired, }); const attachmentsPropType = PropTypes.arrayOf(attachmentPropType); From 4823b7ec72591894c16f9c573633222237385c35 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 23 Nov 2023 20:50:07 +0200 Subject: [PATCH 2/5] fix(AttachmentCarousel): Use source URL for fallback file names Introduce getFileName in FileUtils to generate fallback names for attachments lacking original file names. This function extracts and sanitizes filenames from URLs, ensuring attachments in AttachmentCarousel have meaningful names even when original names are missing. Updated AttachmentCarousel to utilize this new utility function. At this time, the only know case is embedded attachments shared by the Concierge account via drag and drop --- .../extractAttachmentsFromReport.js | 7 +++++-- src/libs/fileDownload/FileUtils.ts | 21 +++++++++++++++++++ 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js index 0f1fa15c99ca..6cd85e4243b2 100644 --- a/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js +++ b/src/components/Attachments/AttachmentCarousel/extractAttachmentsFromReport.js @@ -1,6 +1,7 @@ import {Parser as HtmlParser} from 'htmlparser2'; import lodashGet from 'lodash/get'; import _ from 'underscore'; +import * as FileUtils from '@libs/fileDownload/FileUtils'; import * as ReceiptUtils from '@libs/ReceiptUtils'; import * as ReportActionsUtils from '@libs/ReportActionsUtils'; import * as TransactionUtils from '@libs/TransactionUtils'; @@ -25,14 +26,16 @@ function extractAttachmentsFromReport(parentReportAction, reportActions, transac } const expensifySource = attribs[CONST.ATTACHMENT_SOURCE_ATTRIBUTE]; + const source = tryResolveUrlFromApiRoot(expensifySource || attribs.src); + const fileName = attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE] || FileUtils.getFileName(`${source}`); // By iterating actions in chronological order and prepending each attachment // we ensure correct order of attachments even across actions with multiple attachments. attachments.unshift({ + source, reportActionID: attribs['data-id'], - source: tryResolveUrlFromApiRoot(expensifySource || attribs.src), isAuthTokenRequired: Boolean(expensifySource), - file: {name: attribs[CONST.ATTACHMENT_ORIGINAL_FILENAME_ATTRIBUTE]}, + file: {name: fileName}, isReceipt: false, hasBeenFlagged: attribs['data-flagged'] === 'true', }); diff --git a/src/libs/fileDownload/FileUtils.ts b/src/libs/fileDownload/FileUtils.ts index 618571ddf400..253e0ff13426 100644 --- a/src/libs/fileDownload/FileUtils.ts +++ b/src/libs/fileDownload/FileUtils.ts @@ -1,6 +1,7 @@ import {Alert, Linking, Platform} from 'react-native'; import DateUtils from '@libs/DateUtils'; import * as Localize from '@libs/Localize'; +import Log from '@libs/Log'; import CONST from '@src/CONST'; import type {ReadFileAsync, SplitExtensionFromFileName} from './types'; @@ -74,6 +75,25 @@ function showCameraPermissionsAlert() { ); } +/** + * Extracts a filename from a given URL and sanitizes it for file system usage. + * + * This function takes a URL as input and performs the following operations: + * 1. Extracts the last segment of the URL, which could be a file name, a path segment, + * or a query string parameter. + * 2. Decodes the extracted segment from URL encoding to a plain string for better readability. + * 3. Replaces any characters in the decoded string that are illegal in file names + * with underscores. + */ +function getFileName(url: string): string { + const fileName = url.split(/[#?/]/).pop() ?? ''; + if (!fileName) { + Log.warn('[FileUtils] Could not get attachment name', {url}); + } + + return decodeURIComponent(fileName).replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_'); +} + /** * Generate a random file name with timestamp and file extension */ @@ -232,6 +252,7 @@ export { showCameraPermissionsAlert, splitExtensionFromFileName, getAttachmentName, + getFileName, getFileType, cleanFileName, appendTimeToFileName, From 999155008f3deec85a328d9c7906e45cb2340136 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 23 Nov 2023 21:04:38 +0200 Subject: [PATCH 3/5] fix(libs/fileDownload): Ensure reachable fallback for filenames Addressed a bug in libs/fileDownload where the intended fallback for filenames was never reached due to `FileUtils.appendTimeToFileName` always providing a return value. This fix refines the filename determination logic, ensuring effective use of fallback mechanisms across various components including AttachmentCarousel, particularly for cases like attachments shared by the Concierge account. --- src/libs/fileDownload/index.android.ts | 3 ++- src/libs/fileDownload/index.ios.ts | 3 ++- src/libs/fileDownload/index.ts | 6 ++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/libs/fileDownload/index.android.ts b/src/libs/fileDownload/index.android.ts index 41c7cb29550a..3bb91e538b00 100644 --- a/src/libs/fileDownload/index.android.ts +++ b/src/libs/fileDownload/index.android.ts @@ -38,7 +38,8 @@ function handleDownload(url: string, fileName: string): Promise { // Android files will download to Download directory const path = dirs.DownloadDir; - const attachmentName = FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(url); + const name = fileName.length > 0 ? fileName : FileUtils.getFileName(url); + const attachmentName = FileUtils.appendTimeToFileName(name); const isLocalFile = url.startsWith('file://'); diff --git a/src/libs/fileDownload/index.ios.ts b/src/libs/fileDownload/index.ios.ts index fdc4a78e0b9b..11a74c43f15a 100644 --- a/src/libs/fileDownload/index.ios.ts +++ b/src/libs/fileDownload/index.ios.ts @@ -73,7 +73,8 @@ const fileDownload: FileDownload = (fileUrl, fileName) => new Promise((resolve) => { let fileDownloadPromise; const fileType = FileUtils.getFileType(fileUrl); - const attachmentName = FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(fileUrl); + const name = fileName.length > 0 ? fileName : FileUtils.getFileName(fileUrl); + const attachmentName = FileUtils.appendTimeToFileName(name); switch (fileType) { case CONST.ATTACHMENT_FILE_TYPE.IMAGE: diff --git a/src/libs/fileDownload/index.ts b/src/libs/fileDownload/index.ts index ef36647e549d..91ec7d5058a2 100644 --- a/src/libs/fileDownload/index.ts +++ b/src/libs/fileDownload/index.ts @@ -25,14 +25,12 @@ const fileDownload: FileDownload = (url, fileName) => { // creating anchor tag to initiate download const link = document.createElement('a'); + const name = fileName.length > 0 ? fileName : FileUtils.getFileName(url); // adding href to anchor link.href = href; link.style.display = 'none'; - link.setAttribute( - 'download', - FileUtils.appendTimeToFileName(fileName) || FileUtils.getAttachmentName(url), // generating the file name - ); + link.setAttribute('download', FileUtils.appendTimeToFileName(name)); // Append to html link element page document.body.appendChild(link); From 1b58d05718ba57d20696f5d1f9fb404caeecf15d Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 23 Nov 2023 21:05:32 +0200 Subject: [PATCH 4/5] refactor(libs/FileUtils): remove `getAttachmentName` - unused --- src/libs/fileDownload/FileUtils.ts | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/libs/fileDownload/FileUtils.ts b/src/libs/fileDownload/FileUtils.ts index 253e0ff13426..09cc1222310f 100644 --- a/src/libs/fileDownload/FileUtils.ts +++ b/src/libs/fileDownload/FileUtils.ts @@ -94,16 +94,6 @@ function getFileName(url: string): string { return decodeURIComponent(fileName).replace(CONST.REGEX.ILLEGAL_FILENAME_CHARACTERS, '_'); } -/** - * Generate a random file name with timestamp and file extension - */ -function getAttachmentName(url: string): string { - if (!url) { - return ''; - } - return `${DateUtils.getDBTime()}.${url.split(/[#?]/)[0].split('.').pop()?.trim()}`; -} - function isImage(fileName: string): boolean { return CONST.FILE_TYPE_REGEX.IMAGE.test(fileName); } @@ -251,7 +241,6 @@ export { showPermissionErrorAlert, showCameraPermissionsAlert, splitExtensionFromFileName, - getAttachmentName, getFileName, getFileType, cleanFileName, From 931b7857e7f5b8faff9481f0a7b05aa0cac061f7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Wed, 29 Nov 2023 20:18:12 +0200 Subject: [PATCH 5/5] `fileDownload`: Apply PR suggestions https://github.com/Expensify/App/pull/31791#discussion_r1405428758 --- src/libs/fileDownload/index.android.ts | 3 +-- src/libs/fileDownload/index.ios.ts | 3 +-- src/libs/fileDownload/index.ts | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/libs/fileDownload/index.android.ts b/src/libs/fileDownload/index.android.ts index 3bb91e538b00..8496c1cb6cf5 100644 --- a/src/libs/fileDownload/index.android.ts +++ b/src/libs/fileDownload/index.android.ts @@ -38,8 +38,7 @@ function handleDownload(url: string, fileName: string): Promise { // Android files will download to Download directory const path = dirs.DownloadDir; - const name = fileName.length > 0 ? fileName : FileUtils.getFileName(url); - const attachmentName = FileUtils.appendTimeToFileName(name); + const attachmentName = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(url)); const isLocalFile = url.startsWith('file://'); diff --git a/src/libs/fileDownload/index.ios.ts b/src/libs/fileDownload/index.ios.ts index 11a74c43f15a..7672b4b14926 100644 --- a/src/libs/fileDownload/index.ios.ts +++ b/src/libs/fileDownload/index.ios.ts @@ -73,8 +73,7 @@ const fileDownload: FileDownload = (fileUrl, fileName) => new Promise((resolve) => { let fileDownloadPromise; const fileType = FileUtils.getFileType(fileUrl); - const name = fileName.length > 0 ? fileName : FileUtils.getFileName(fileUrl); - const attachmentName = FileUtils.appendTimeToFileName(name); + const attachmentName = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(fileUrl)); switch (fileType) { case CONST.ATTACHMENT_FILE_TYPE.IMAGE: diff --git a/src/libs/fileDownload/index.ts b/src/libs/fileDownload/index.ts index 91ec7d5058a2..4f43b215925f 100644 --- a/src/libs/fileDownload/index.ts +++ b/src/libs/fileDownload/index.ts @@ -25,12 +25,11 @@ const fileDownload: FileDownload = (url, fileName) => { // creating anchor tag to initiate download const link = document.createElement('a'); - const name = fileName.length > 0 ? fileName : FileUtils.getFileName(url); // adding href to anchor link.href = href; link.style.display = 'none'; - link.setAttribute('download', FileUtils.appendTimeToFileName(name)); + link.download = FileUtils.appendTimeToFileName(fileName || FileUtils.getFileName(url)); // Append to html link element page document.body.appendChild(link);