From 7d80f9b3cc76ab09b5538ee786127735dc8433a3 Mon Sep 17 00:00:00 2001 From: Hamza Date: Thu, 3 Sep 2026 13:49:39 +0200 Subject: [PATCH] fix(files): validate dropped filenames before upload Assisted-by: Codex:gpt-5.6-sol Signed-off-by: Hamza --- apps/files/src/services/DropService.spec.ts | 66 +++++++++++++++++++ apps/files/src/services/DropService.ts | 16 ++++- .../src/services/DropServiceUtils.spec.ts | 51 +++++++++++++- apps/files/src/services/DropServiceUtils.ts | 31 +++++++++ 4 files changed, 160 insertions(+), 4 deletions(-) create mode 100644 apps/files/src/services/DropService.spec.ts diff --git a/apps/files/src/services/DropService.spec.ts b/apps/files/src/services/DropService.spec.ts new file mode 100644 index 0000000000000..922234adc030a --- /dev/null +++ b/apps/files/src/services/DropService.spec.ts @@ -0,0 +1,66 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import type { IFolder } from '@nextcloud/files' +import type { RootDirectory } from './DropServiceUtils.ts' + +import { showError, showSuccess } from '@nextcloud/dialogs' +import { getUploader, hasConflict } from '@nextcloud/upload' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { onDropExternalFiles } from './DropService.ts' +import { createDirectoryIfNotExists, Directory } from './DropServiceUtils.ts' + +vi.mock('@nextcloud/dialogs') +vi.mock('@nextcloud/upload', () => ({ + getUploader: vi.fn(), + hasConflict: vi.fn(), +})) +vi.mock('./DropServiceUtils.ts', async (importOriginal) => ({ + ...await importOriginal(), + createDirectoryIfNotExists: vi.fn(), +})) +vi.mock('@nextcloud/capabilities', () => ({ + getCapabilities: () => ({ + files: { + forbidden_filename_characters: ['/', '\\'], + forbidden_filenames: ['.htaccess'], + forbidden_filename_basenames: [], + forbidden_filename_extensions: ['.part'], + }, + }), +})) + +describe('onDropExternalFiles', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(getUploader).mockReturnValue({ + pause: vi.fn(), + start: vi.fn(), + } as never) + vi.mocked(hasConflict).mockReturnValue(false) + }) + + it('rejects an invalid dropped tree before starting the upload', async () => { + const root = new Directory('root', [new Directory('test\\')]) as RootDirectory + + const uploads = await onDropExternalFiles(root, {} as IFolder, []) + + expect(uploads).toEqual([]) + expect(showError).toHaveBeenCalledWith('Cannot upload "test\\": "\\" is not allowed inside a folder name.') + expect(getUploader).not.toHaveBeenCalled() + expect(hasConflict).not.toHaveBeenCalled() + }) + + it('does not report success after a directory creation failure', async () => { + const root = new Directory('root', [new Directory('folder')]) as RootDirectory + vi.mocked(createDirectoryIfNotExists).mockRejectedValue(new Error('Failed to create directory')) + + const uploads = await onDropExternalFiles(root, {} as IFolder, []) + + expect(uploads).toEqual([]) + expect(showError).toHaveBeenCalledWith('Unable to create the directory folder') + expect(showSuccess).not.toHaveBeenCalled() + }) +}) diff --git a/apps/files/src/services/DropService.ts b/apps/files/src/services/DropService.ts index c408f183ac37d..59ace6c7cda38 100644 --- a/apps/files/src/services/DropService.ts +++ b/apps/files/src/services/DropService.ts @@ -14,7 +14,7 @@ import { getUploader, hasConflict } from '@nextcloud/upload' import { handleCopyMoveNodesTo, HintException } from '../actions/moveOrCopyAction.ts' import { MoveCopyAction } from '../actions/moveOrCopyActionUtils.ts' import { logger } from '../utils/logger.ts' -import { createDirectoryIfNotExists, Directory, resolveConflict, traverseTree } from './DropServiceUtils.ts' +import { createDirectoryIfNotExists, Directory, findInvalidDroppedEntry, resolveConflict, traverseTree } from './DropServiceUtils.ts' /** * This function converts a list of DataTransferItems to a file tree. @@ -94,6 +94,12 @@ export async function dataTransferToFileTree(items: DataTransferItem[]): Promise * @param contents - The contents of the destination folder */ export async function onDropExternalFiles(root: RootDirectory, destination: IFolder, contents: INode[]): Promise { + const invalidEntry = findInvalidDroppedEntry(root) + if (invalidEntry) { + showError(t('files', 'Cannot upload "{path}": {reason}', invalidEntry)) + return [] + } + const uploader = getUploader() // Check for conflicts on root elements @@ -112,6 +118,7 @@ export async function onDropExternalFiles(root: RootDirectory, destination: IFol // Let's process the files logger.debug(`Uploading files to ${destination.path}`, { root, contents: root.contents }) const queue = [] as Promise[] + let hasDirectoryErrors = false const uploadDirectoryContents = async (directory: Directory, path: string) => { for (const file of directory.contents) { @@ -127,6 +134,7 @@ export async function onDropExternalFiles(root: RootDirectory, destination: IFol await createDirectoryIfNotExists(relativePath, destination) await uploadDirectoryContents(file, relativePath) } catch (error) { + hasDirectoryErrors = true showError(t('files', 'Unable to create the directory {directory}', { directory: file.name })) logger.error('Unable to create the directory', { error, relativePath, directory: file }) } @@ -155,9 +163,11 @@ export async function onDropExternalFiles(root: RootDirectory, destination: IFol // Check for errors const errors = results.filter((result) => result.status === 'rejected') - if (errors.length > 0) { + if (errors.length > 0 || hasDirectoryErrors) { logger.error('Error while uploading files', { errors }) - showError(t('files', 'Some files could not be uploaded')) + if (errors.length > 0) { + showError(t('files', 'Some files could not be uploaded')) + } return [] } diff --git a/apps/files/src/services/DropServiceUtils.spec.ts b/apps/files/src/services/DropServiceUtils.spec.ts index 2d9d602e62e46..de17951d3cad9 100644 --- a/apps/files/src/services/DropServiceUtils.spec.ts +++ b/apps/files/src/services/DropServiceUtils.spec.ts @@ -8,9 +8,19 @@ import { beforeAll, describe, expect, it, vi } from 'vitest' import { DataTransferItem as DataTransferItemMock, FileSystemDirectoryEntry, fileSystemEntryToDataTransferItem, FileSystemFileEntry } from '../../../../__tests__/FileSystemAPIUtils.ts' import { logger } from '../utils/logger.ts' import { dataTransferToFileTree } from './DropService.ts' -import { Directory, traverseTree } from './DropServiceUtils.ts' +import { Directory, findInvalidDroppedEntry, traverseTree } from './DropServiceUtils.ts' vi.mock('@nextcloud/dialogs') +vi.mock('@nextcloud/capabilities', () => ({ + getCapabilities: () => ({ + files: { + forbidden_filename_characters: ['/', '\\'], + forbidden_filenames: ['.htaccess'], + forbidden_filename_basenames: [], + forbidden_filename_extensions: ['.part', ' '], + }, + }), +})) const dataTree = { 'file0.txt': ['Hello, world!', 1234567890], @@ -87,6 +97,45 @@ describe('Filesystem API traverseTree', () => { }) }) +describe('findInvalidDroppedEntry', () => { + it('returns nothing for a valid tree', () => { + const tree = new Directory('root', [ + new Directory('folder', [new File([], 'file.txt')]), + ]) + + expect(findInvalidDroppedEntry(tree)).toBeUndefined() + }) + + it('reports an invalid top-level folder', () => { + const tree = new Directory('root', [new Directory('folder\\')]) + + expect(findInvalidDroppedEntry(tree)).toEqual({ + path: 'folder\\', + reason: '"\\" is not allowed inside a folder name.', + }) + }) + + it('reports the path of an invalid nested file', () => { + const tree = new Directory('root', [ + new Directory('folder', [new File([], 'file\\.txt')]), + ]) + + expect(findInvalidDroppedEntry(tree)).toEqual({ + path: 'folder/file\\.txt', + reason: '"\\" is not allowed inside a filename.', + }) + }) + + it('uses forbidden filename extensions advertised by the server', () => { + const tree = new Directory('root', [new Directory('folder ')]) + + expect(findInvalidDroppedEntry(tree)).toEqual({ + path: 'folder ', + reason: 'Folder names must not end with " ".', + }) + }) +}) + describe('DropService dataTransferToFileTree', () => { beforeAll(() => { // @ts-expect-error jsdom doesn't have DataTransferItem diff --git a/apps/files/src/services/DropServiceUtils.ts b/apps/files/src/services/DropServiceUtils.ts index 448923755f3b6..95c629ddbba41 100644 --- a/apps/files/src/services/DropServiceUtils.ts +++ b/apps/files/src/services/DropServiceUtils.ts @@ -12,6 +12,7 @@ import { defaultRemoteURL, defaultRootPath, getClient, getDefaultPropfind, resul import { t } from '@nextcloud/l10n' import { join } from '@nextcloud/paths' import { openConflictPicker } from '@nextcloud/upload' +import { getFilenameValidity } from '../utils/filenameValidity.ts' import { logger } from '../utils/logger.ts' /** @@ -84,6 +85,36 @@ export type RootDirectory = Directory & { name: 'root' } +export type InvalidDroppedEntry = { + path: string + reason: string +} + +/** + * Find the first invalid file or folder in a dropped file tree. + * + * @param directory Directory to validate + * @param path Path of the directory relative to the upload destination + */ +export function findInvalidDroppedEntry(directory: Directory, path = ''): InvalidDroppedEntry | undefined { + for (const entry of directory.contents) { + const entryPath = join(path, entry.name) + const isFolder = entry instanceof Directory + const reason = getFilenameValidity(entry.name, false, isFolder) + + if (reason !== '') { + return { path: entryPath, reason } + } + + if (isFolder) { + const invalidEntry = findInvalidDroppedEntry(entry, entryPath) + if (invalidEntry) { + return invalidEntry + } + } + } +} + /** * Traverse a file tree using the Filesystem API *