From 831f7f10130931daa5a7bb005c9fdfeec19e298d Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Mon, 22 Jun 2026 17:29:24 +0200 Subject: [PATCH] fix(files): distinguish between files and folders for validation messages Unit tests are AI assisted. Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Ferdinand Thiessen --- .../components/FileEntry/FileEntryName.vue | 2 +- apps/files/src/components/NewNodeDialog.vue | 10 +- apps/files/src/newMenu/newFolder.ts | 2 +- apps/files/src/newMenu/newTemplatesFolder.ts | 10 +- apps/files/src/utils/filenameValidity.spec.ts | 100 ++++++++++++++++++ apps/files/src/utils/filenameValidity.ts | 28 +++-- apps/files/src/utils/newNodeDialog.ts | 13 ++- 7 files changed, 147 insertions(+), 18 deletions(-) create mode 100644 apps/files/src/utils/filenameValidity.spec.ts diff --git a/apps/files/src/components/FileEntry/FileEntryName.vue b/apps/files/src/components/FileEntry/FileEntryName.vue index ef7bd66b108b6..5cde6b2e5a87d 100644 --- a/apps/files/src/components/FileEntry/FileEntryName.vue +++ b/apps/files/src/components/FileEntry/FileEntryName.vue @@ -190,7 +190,7 @@ export default defineComponent({ return } - let validity = getFilenameValidity(newName) + let validity = getFilenameValidity(newName, false, this.source.type === FileType.Folder) // Checking if already exists if (validity === '' && this.checkIfNodeExists(newName)) { validity = t('files', 'Another entry with the same name already exists.') diff --git a/apps/files/src/components/NewNodeDialog.vue b/apps/files/src/components/NewNodeDialog.vue index a0bc4d15ef0c7..21a15a0a74ce6 100644 --- a/apps/files/src/components/NewNodeDialog.vue +++ b/apps/files/src/components/NewNodeDialog.vue @@ -89,6 +89,14 @@ const props = defineProps({ type: String, default: t('files', 'Folder name'), }, + + /** + * Whether the name is for a folder, which affects the validation of the name. Defaults to false. + */ + isFolder: { + type: Boolean, + default: false, + }, }) const emit = defineEmits<{ @@ -142,7 +150,7 @@ watchEffect(() => { if (props.otherNames.includes(localDefaultName.value.trim())) { validity.value = t('files', 'This name is already in use.') } else { - validity.value = getFilenameValidity(localDefaultName.value.trim()) + validity.value = getFilenameValidity(localDefaultName.value.trim(), false, props.isFolder) } const input = nameInput.value?.$el.querySelector('input') if (input) { diff --git a/apps/files/src/newMenu/newFolder.ts b/apps/files/src/newMenu/newFolder.ts index a383fd8ee3647..d98c6ac1755f7 100644 --- a/apps/files/src/newMenu/newFolder.ts +++ b/apps/files/src/newMenu/newFolder.ts @@ -29,7 +29,7 @@ export const entry: NewMenuEntry = { }, async handler(context: IFolder, content: INode[]) { - const name = await newNodeName(t('files', 'New folder'), content) + const name = await newNodeName(t('files', 'New folder'), content, { isFolder: true }) if (name === null) { return } diff --git a/apps/files/src/newMenu/newTemplatesFolder.ts b/apps/files/src/newMenu/newTemplatesFolder.ts index 247f618e5044f..2e8f3431f84e7 100644 --- a/apps/files/src/newMenu/newTemplatesFolder.ts +++ b/apps/files/src/newMenu/newTemplatesFolder.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import type { Folder, NewMenuEntry, Node } from '@nextcloud/files' +import type { IFolder, INode, NewMenuEntry } from '@nextcloud/files' import PlusSvg from '@mdi/svg/svg/plus.svg?raw' import { getCurrentUser } from '@nextcloud/auth' @@ -28,7 +28,7 @@ logger.debug('Initial templates folder', { templatesPath }) * @param directory Folder where to create the templates folder * @param name Name to use or the templates folder */ -async function initTemplatesFolder(directory: Folder, name: string) { +async function initTemplatesFolder(directory: IFolder, name: string) { const templatePath = join(directory.path, name) try { logger.debug('Initializing the templates directory', { templatePath }) @@ -59,7 +59,7 @@ export const entry: NewMenuEntry = { displayName: t('files', 'Create templates folder'), iconSvgInline: PlusSvg, order: 30, - enabled(context: Folder): boolean { + enabled(context: IFolder): boolean { // Templates disabled or templates folder already initialized if (!templatesEnabled || templatesPath) { return false @@ -70,8 +70,8 @@ export const entry: NewMenuEntry = { } return (context.permissions & Permission.CREATE) !== 0 }, - async handler(context: Folder, content: Node[]) { - const name = await newNodeName(t('files', 'Templates'), content, { name: t('files', 'New template folder') }) + async handler(context: IFolder, content: INode[]) { + const name = await newNodeName(t('files', 'Templates'), content, { name: t('files', 'New template folder'), isFolder: true }) if (name !== null) { // Create the template folder diff --git a/apps/files/src/utils/filenameValidity.spec.ts b/apps/files/src/utils/filenameValidity.spec.ts new file mode 100644 index 0000000000000..2687956efeb6c --- /dev/null +++ b/apps/files/src/utils/filenameValidity.spec.ts @@ -0,0 +1,100 @@ +/*! + * SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +import { describe, expect, it, vi } from 'vitest' +import { getFilenameValidity } from './filenameValidity.ts' + +vi.mock('@nextcloud/capabilities', () => ({ + getCapabilities: () => ({ + files: { + forbidden_filename_characters: ['/', '\\', '>'], + forbidden_filenames: ['.htaccess'], + forbidden_filename_basenames: ['con'], + forbidden_filename_extensions: ['.exe', '.~'], + }, + }), +})) + +describe('getFilenameValidity', () => { + it('returns no error for a valid filename', () => { + expect(getFilenameValidity('valid-name.txt')).toBe('') + }) + + describe('empty name', () => { + it('reports empty filename', () => { + expect(getFilenameValidity('')).toBe('Filename must not be empty.') + }) + + it('reports empty filename for whitespace only', () => { + expect(getFilenameValidity(' ')).toBe('Filename must not be empty.') + }) + + it('reports empty folder name when isFolder is set', () => { + expect(getFilenameValidity('', false, true)).toBe('Folder name must not be empty.') + }) + }) + + describe('forbidden character', () => { + it('reports forbidden character for a filename', () => { + expect(getFilenameValidity('inva/lid')).toBe('"/" is not allowed inside a filename.') + }) + + it('reports forbidden character for a folder name', () => { + expect(getFilenameValidity('inva/lid', false, true)).toBe('"/" is not allowed inside a folder name.') + }) + }) + + describe('reserved name', () => { + it('reports a reserved filename', () => { + expect(getFilenameValidity('.htaccess')).toBe('".htaccess" is a reserved name and not allowed for filenames.') + }) + + it('reports a reserved folder name', () => { + expect(getFilenameValidity('.htaccess', false, true)).toBe('".htaccess" is a reserved name and not allowed for folder names.') + }) + + it('reports a reserved basename', () => { + expect(getFilenameValidity('con.txt')).toBe('"con" is a reserved name and not allowed for filenames.') + }) + }) + + describe('forbidden extension', () => { + it('reports a disallowed filetype when the extension looks like a real type', () => { + expect(getFilenameValidity('virus.exe')).toBe('".exe" is not an allowed filetype.') + }) + + it('reports the extension as not allowed at the end of a filename when it is not a recognisable filetype', () => { + // '.~' does not match /\.[a-z]/i, so the generic "must not end with" message is used. + expect(getFilenameValidity('document.~')).toBe('Filenames must not end with ".~".') + }) + + it('reports the extension as not allowed for a folder name', () => { + expect(getFilenameValidity('folder.exe', false, true)).toBe('Folder names must not end with ".exe".') + }) + }) + + describe('escape option', () => { + it('does not affect the returned string for safe characters', () => { + expect(getFilenameValidity('inva/lid', true)).toBe('"/" is not allowed inside a filename.') + }) + + it('escapes the matched character when requested', () => { + expect(getFilenameValidity('inva>lid', true)).toBe('">" is not allowed inside a filename.') + }) + + it('does not escape the matched character by default', () => { + expect(getFilenameValidity('inva>lid')).toBe('">" is not allowed inside a filename.') + }) + }) + + it('rethrows errors that are not InvalidFilenameError', async () => { + const files = await import('@nextcloud/files') + const spy = vi.spyOn(files, 'validateFilename').mockImplementation(() => { + throw new Error('unexpected') + }) + expect(() => getFilenameValidity('anything')).toThrow('unexpected') + spy.mockRestore() + }) +}) diff --git a/apps/files/src/utils/filenameValidity.ts b/apps/files/src/utils/filenameValidity.ts index bb8996e6db237..b018352a8b272 100644 --- a/apps/files/src/utils/filenameValidity.ts +++ b/apps/files/src/utils/filenameValidity.ts @@ -11,9 +11,13 @@ import { t } from '@nextcloud/l10n' * * @param name The filename * @param escape Escape the matched string in the error (only set when used in HTML) + * @param isFolder Whether the filename is for a folder */ -export function getFilenameValidity(name: string, escape = false): string { +export function getFilenameValidity(name: string, escape = false, isFolder = false): string { if (name.trim() === '') { + if (isFolder) { + return t('files', 'Folder name must not be empty.') + } return t('files', 'Filename must not be empty.') } @@ -27,15 +31,27 @@ export function getFilenameValidity(name: string, escape = false): string { switch (error.reason) { case InvalidFilenameErrorReason.Character: - return t('files', '"{char}" is not allowed inside a filename.', { char: error.segment }, undefined, { escape }) + if (isFolder) { + return t('files', '"{char}" is not allowed inside a folder name.', { char: error.segment }, { escape }) + } + return t('files', '"{char}" is not allowed inside a filename.', { char: error.segment }, { escape }) case InvalidFilenameErrorReason.ReservedName: - return t('files', '"{segment}" is a reserved name and not allowed for filenames.', { segment: error.segment }, undefined, { escape: false }) + if (isFolder) { + return t('files', '"{segment}" is a reserved name and not allowed for folder names.', { segment: error.segment }, { escape: false }) + } + return t('files', '"{segment}" is a reserved name and not allowed for filenames.', { segment: error.segment }, { escape: false }) case InvalidFilenameErrorReason.Extension: - if (error.segment.match(/\.[a-z]/i)) { - return t('files', '"{extension}" is not an allowed filetype.', { extension: error.segment }, undefined, { escape: false }) + if (!isFolder && error.segment.match(/\.[a-z]/i)) { + return t('files', '"{extension}" is not an allowed filetype.', { extension: error.segment }, { escape: false }) } - return t('files', 'Filenames must not end with "{extension}".', { extension: error.segment }, undefined, { escape: false }) + if (isFolder) { + return t('files', 'Folder names must not end with "{extension}".', { extension: error.segment }, { escape: false }) + } + return t('files', 'Filenames must not end with "{extension}".', { extension: error.segment }, { escape: false }) default: + if (isFolder) { + return t('files', 'Invalid folder name.') + } return t('files', 'Invalid filename.') } } diff --git a/apps/files/src/utils/newNodeDialog.ts b/apps/files/src/utils/newNodeDialog.ts index 67bee62dd73c8..30e26a0f9b649 100644 --- a/apps/files/src/utils/newNodeDialog.ts +++ b/apps/files/src/utils/newNodeDialog.ts @@ -8,7 +8,7 @@ import type { INode } from '@nextcloud/files' import { spawnDialog } from '@nextcloud/vue/functions/dialog' import NewNodeDialog from '../components/NewNodeDialog.vue' -interface ILabels { +interface NewNodeDialogOptions { /** * Dialog heading, defaults to "New folder name" */ @@ -17,6 +17,11 @@ interface ILabels { * Label for input box, defaults to "New folder" */ label?: string + + /** + * Whether the name is for a folder, defaults to false. + */ + isFolder?: boolean } /** @@ -24,15 +29,15 @@ interface ILabels { * * @param defaultName Default name to use * @param folderContent Nodes with in the current folder to check for unique name - * @param labels Labels to set on the dialog + * @param options Options for the dialog * @return string if successful otherwise null if aborted */ -export function newNodeName(defaultName: string, folderContent: INode[], labels: ILabels = {}) { +export function newNodeName(defaultName: string, folderContent: INode[], options: NewNodeDialogOptions = {}) { const contentNames = folderContent.map((node: INode) => node.basename) return new Promise((resolve) => { spawnDialog(NewNodeDialog, { - ...labels, + ...options, defaultName, otherNames: contentNames, }, (folderName) => {