From be51810be52f199ccf41660d81cb89dab841d284 Mon Sep 17 00:00:00 2001 From: Hugh O'Brien Date: Wed, 9 Dec 2020 11:55:36 +0000 Subject: [PATCH] Merge pull request #3410 from overleaf/hb-validate-new-folder React File Tree - Validate new folder names and renames GitOrigin-RevId: f040eb08e2daefb3dfd639e18aaef95d7123d727 --- .../web/app/src/Features/Project/SafePath.js | 7 +- .../frontend/extracted-translation-keys.json | 8 +- .../file-tree-item/file-tree-item-name.js | 3 +- .../file-tree/components/file-tree-root.js | 2 + .../modals/file-tree-modal-create-folder.js | 56 +++++++++++-- .../modals/file-tree-modal-error.js | 62 ++++++++++++++ .../contexts/file-tree-actionable.js | 33 +++++++- .../frontend/js/features/file-tree/errors.js | 11 +++ .../util/is-name-unique-in-folder.js | 13 +++ .../js/features/file-tree/util/safe-path.js | 81 +++++++++++++++++++ .../frontend/js/ide/directives/SafePath.js | 7 +- .../components/file-tree-root.test.js | 3 + .../file-tree/flows/create-folder.test.js | 49 ++++++++++- .../file-tree/flows/delete-entity.test.js | 2 + .../file-tree/flows/rename-entity.test.js | 42 +++++++++- 15 files changed, 357 insertions(+), 22 deletions(-) create mode 100644 services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-error.js create mode 100644 services/web/frontend/js/features/file-tree/errors.js create mode 100644 services/web/frontend/js/features/file-tree/util/is-name-unique-in-folder.js create mode 100644 services/web/frontend/js/features/file-tree/util/safe-path.js diff --git a/services/web/app/src/Features/Project/SafePath.js b/services/web/app/src/Features/Project/SafePath.js index be6c2dd652..d3cc05ff4a 100644 --- a/services/web/app/src/Features/Project/SafePath.js +++ b/services/web/app/src/Features/Project/SafePath.js @@ -13,9 +13,10 @@ */ // This file is shared between the frontend and server code of web, so that // filename validation is the same in both implementations. -// Both copies must be kept in sync: -// app/coffee/Features/Project/SafePath.coffee -// public/coffee/ide/directives/SafePath.coffee +// The logic in all copies must be kept in sync: +// app/src/Features/Project/SafePath.js +// frontend/js/ide/directives/SafePath.js +// frontend/js/features/file-tree/util/safe-path.js const load = function() { let SafePath diff --git a/services/web/frontend/extracted-translation-keys.json b/services/web/frontend/extracted-translation-keys.json index f5d5543221..fefc81fc46 100644 --- a/services/web/frontend/extracted-translation-keys.json +++ b/services/web/frontend/extracted-translation-keys.json @@ -19,6 +19,8 @@ "expand", "fast", "file_outline", + "file_already_exists", + "files_cannot_include_invalid_characters", "find_out_more_about_the_file_outline", "first_error_popup_label", "following_paths_conflict", @@ -95,5 +97,9 @@ "n_items", "please_refresh", "generic_something_went_wrong", - "refresh" + "refresh", + "duplicate_file", + "error", + "invalid_file_name", + "ok" ] diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-name.js b/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-name.js index 76669d4f1e..be4c994ced 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-name.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-item/file-tree-item-name.js @@ -10,10 +10,11 @@ function FileTreeItemName({ name, isSelected }) { isRenaming, startRenaming, finishRenaming, + error, cancel } = useFileTreeActionable() - const isRenamingEntity = isRenaming && isSelected + const isRenamingEntity = isRenaming && isSelected && !error if (isRenamingEntity) { return ( diff --git a/services/web/frontend/js/features/file-tree/components/file-tree-root.js b/services/web/frontend/js/features/file-tree/components/file-tree-root.js index fc590caa59..61623cbcbb 100644 --- a/services/web/frontend/js/features/file-tree/components/file-tree-root.js +++ b/services/web/frontend/js/features/file-tree/components/file-tree-root.js @@ -8,6 +8,7 @@ import FileTreeFolderList from './file-tree-folder-list' import FileTreeToolbar from './file-tree-toolbar' import FileTreeModalDelete from './modals/file-tree-modal-delete' import FileTreeModalCreateFolder from './modals/file-tree-modal-create-folder' +import FileTreeModalError from './modals/file-tree-modal-error' import FileTreeContextMenu from './file-tree-context-menu' import FileTreeError from './file-tree-error' @@ -50,6 +51,7 @@ function FileTreeRoot({ + ) } diff --git a/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-create-folder.js b/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-create-folder.js index 20444d123d..51e4f8df35 100644 --- a/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-create-folder.js +++ b/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-create-folder.js @@ -7,9 +7,14 @@ import { useRefWithAutoFocus } from '../../../../infrastructure/auto-focus' import { useFileTreeActionable } from '../../contexts/file-tree-actionable' +import { DuplicateFilenameError } from '../../errors' + +import { isCleanFilename } from '../../util/safe-path' + function FileTreeModalCreateFolder() { const { t } = useTranslation() const [name, setName] = useState('') + const [validName, setValidName] = useState(true) const { isCreatingFolder, @@ -29,6 +34,15 @@ function FileTreeModalCreateFolder() { finishCreatingFolder(name) } + function errorMessage() { + switch (error.constructor) { + case DuplicateFilenameError: + return t('file_already_exists') + default: + return t('generic_something_went_wrong') + } + } + return ( @@ -39,13 +53,28 @@ function FileTreeModalCreateFolder() { - {error && ( -
- {t('generic_something_went_wrong')} + {!validName ? ( +
+ {t('files_cannot_include_invalid_characters')}
- )} + ) : null} + {error ? ( +
+ {errorMessage()} +
+ ) : null} @@ -56,7 +85,11 @@ function FileTreeModalCreateFolder() { ) : ( <> - @@ -66,7 +99,13 @@ function FileTreeModalCreateFolder() { ) } -function InputName({ name, setName, handleCreateFolder }) { +function InputName({ + name, + setName, + validName, + setValidName, + handleCreateFolder +}) { const { autoFocusedRef } = useRefWithAutoFocus() function handleFocus(ev) { @@ -74,11 +113,12 @@ function InputName({ name, setName, handleCreateFolder }) { } function handleChange(ev) { + setValidName(isCleanFilename(ev.target.value.trim())) setName(ev.target.value) } function handleKeyDown(ev) { - if (ev.key === 'Enter') { + if (ev.key === 'Enter' && validName) { handleCreateFolder() } } @@ -99,6 +139,8 @@ function InputName({ name, setName, handleCreateFolder }) { InputName.propTypes = { name: PropTypes.string.isRequired, setName: PropTypes.func.isRequired, + validName: PropTypes.bool.isRequired, + setValidName: PropTypes.func.isRequired, handleCreateFolder: PropTypes.func.isRequired } diff --git a/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-error.js b/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-error.js new file mode 100644 index 0000000000..17e11a6820 --- /dev/null +++ b/services/web/frontend/js/features/file-tree/components/modals/file-tree-modal-error.js @@ -0,0 +1,62 @@ +import React from 'react' + +import { Button, Modal } from 'react-bootstrap' +import { useTranslation } from 'react-i18next' + +import { useFileTreeActionable } from '../../contexts/file-tree-actionable' + +import { InvalidFilenameError, DuplicateFilenameError } from '../../errors' + +function FileTreeModalError() { + const { t } = useTranslation() + + const { isRenaming, cancel, error } = useFileTreeActionable() + + if (!isRenaming || !error) return null // the modal will not be rendered; return early + + function handleHide() { + cancel() + } + + function errorTitle() { + switch (error.constructor) { + case DuplicateFilenameError: + return t('duplicate_file') + case InvalidFilenameError: + return t('invalid_file_name') + default: + return t('error') + } + } + + function errorMessage() { + switch (error.constructor) { + case DuplicateFilenameError: + return t('file_already_exists') + case InvalidFilenameError: + return t('files_cannot_include_invalid_characters') + default: + return t('generic_something_went_wrong') + } + } + + return ( + + + {errorTitle()} + + + +
+ {errorMessage()} +
+ + + + + + + ) +} + +export default FileTreeModalError diff --git a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js index 238e0aa970..f631178ff6 100644 --- a/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js +++ b/services/web/frontend/js/features/file-tree/contexts/file-tree-actionable.js @@ -10,11 +10,15 @@ import { syncCreateEntity } from '../util/sync-mutation' import { findInTreeOrThrow } from '../util/find-in-tree' +import { isNameUniqueInFolder } from '../util/is-name-unique-in-folder' +import { isCleanFilename } from '../util/safe-path' import { FileTreeMainContext } from './file-tree-main' import { useFileTreeMutable } from './file-tree-mutable' import { useFileTreeSelectable } from './file-tree-selectable' +import { InvalidFilenameError, DuplicateFilenameError } from '../errors' + const FileTreeActionableContext = createContext() const ACTION_TYPES = { @@ -120,15 +124,33 @@ export function useFileTreeActionable() { // update the entity with the new name immediately in the tree, but revert to // the old name if the sync fails function finishRenaming(newName) { - dispatch({ type: ACTION_TYPES.CLEAR }) const selectedEntityId = Array.from(selectedEntityIds)[0] const found = findInTreeOrThrow(fileTreeData, selectedEntityId) const oldName = found.entity.name + if (newName === oldName) { + return dispatch({ type: ACTION_TYPES.CLEAR }) + } + let error + // check valid name + if (!isCleanFilename(newName)) { + error = new InvalidFilenameError() + return dispatch({ type: ACTION_TYPES.ERROR, error }) + } + + // check for duplicates + if (!isNameUniqueInFolder(fileTreeData, found.parentFolderId, newName)) { + error = new DuplicateFilenameError() + return dispatch({ type: ACTION_TYPES.ERROR, error }) + } + + dispatch({ type: ACTION_TYPES.CLEAR }) dispatchRename(selectedEntityId, newName) return syncRename(projectId, found.type, found.entity._id, newName).catch( error => { - dispatch({ type: ACTION_TYPES.ERROR, error }) dispatchRename(selectedEntityId, oldName) + // The state from this error action isn't used anywhere right now + // but we need to handle the error for linting + dispatch({ type: ACTION_TYPES.ERROR, error }) } ) } @@ -189,7 +211,12 @@ export function useFileTreeActionable() { const parentFolderId = found.type === 'folder' ? found.entity._id : found.parentFolderId - return syncCreateEntity(projectId, parentFolderId, entity) + // check for duplicates and throw + if (isNameUniqueInFolder(fileTreeData, parentFolderId, entity.name)) { + return syncCreateEntity(projectId, parentFolderId, entity) + } else { + return Promise.reject(new DuplicateFilenameError()) + } } function finishCreatingFolder(name) { diff --git a/services/web/frontend/js/features/file-tree/errors.js b/services/web/frontend/js/features/file-tree/errors.js new file mode 100644 index 0000000000..c90a977d90 --- /dev/null +++ b/services/web/frontend/js/features/file-tree/errors.js @@ -0,0 +1,11 @@ +export class InvalidFilenameError extends Error { + constructor() { + super('invalid filename') + } +} + +export class DuplicateFilenameError extends Error { + constructor() { + super('duplicate filename') + } +} diff --git a/services/web/frontend/js/features/file-tree/util/is-name-unique-in-folder.js b/services/web/frontend/js/features/file-tree/util/is-name-unique-in-folder.js new file mode 100644 index 0000000000..d8d44a1520 --- /dev/null +++ b/services/web/frontend/js/features/file-tree/util/is-name-unique-in-folder.js @@ -0,0 +1,13 @@ +import { findInTree } from '../util/find-in-tree' + +export function isNameUniqueInFolder(tree, parentFolderId, name) { + if (tree._id !== parentFolderId) { + tree = findInTree(tree, parentFolderId).entity + } + + if (tree.docs.some(entity => entity.name === name)) return false + if (tree.fileRefs.some(entity => entity.name === name)) return false + if (tree.folders.some(entity => entity.name === name)) return false + + return true +} diff --git a/services/web/frontend/js/features/file-tree/util/safe-path.js b/services/web/frontend/js/features/file-tree/util/safe-path.js new file mode 100644 index 0000000000..cb59ce1adc --- /dev/null +++ b/services/web/frontend/js/features/file-tree/util/safe-path.js @@ -0,0 +1,81 @@ +// This file is shared between the frontend and server code of web, so that +// filename validation is the same in both implementations. +// The logic in all copies must be kept in sync: +// app/src/Features/Project/SafePath.js +// frontend/js/ide/directives/SafePath.js +// frontend/js/features/file-tree/util/safe-path.js + +const BADCHAR_RX = new RegExp( + `\ +[\ +\\/\ +\\\\\ +\\*\ +\\u0000-\\u001F\ +\\u007F\ +\\u0080-\\u009F\ +\\uD800-\\uDFFF\ +]\ +`, + 'g' +) + +const BADFILE_RX = new RegExp( + `\ +(^\\.$)\ +|(^\\.\\.$)\ +|(^\\s+)\ +|(\\s+$)\ +`, + 'g' +) + +// Put a block on filenames which match javascript property names, as they +// can cause exceptions where the code puts filenames into a hash. This is a +// temporary workaround until the code in other places is made safe against +// property names. +// +// The list of property names is taken from +// ['prototype'].concat(Object.getOwnPropertyNames(Object.prototype)) +const BLOCKEDFILE_RX = new RegExp(`\ +^(\ +prototype\ +|constructor\ +|toString\ +|toLocaleString\ +|valueOf\ +|hasOwnProperty\ +|isPrototypeOf\ +|propertyIsEnumerable\ +|__defineGetter__\ +|__lookupGetter__\ +|__defineSetter__\ +|__lookupSetter__\ +|__proto__\ +)$\ +`) + +const MAX_PATH = 1024 // Maximum path length, in characters. This is fairly arbitrary. + +export function clean(filename) { + filename = filename.replace(BADCHAR_RX, '_') + // for BADFILE_RX replace any matches with an equal number of underscores + filename = filename.replace(BADFILE_RX, match => + new Array(match.length + 1).join('_') + ) + // replace blocked filenames 'prototype' with '@prototype' + filename = filename.replace(BLOCKEDFILE_RX, '@$1') + return filename +} + +export function isCleanFilename(filename) { + return ( + isAllowedLength(filename) && + !filename.match(BADCHAR_RX) && + !filename.match(BADFILE_RX) + ) +} + +export function isAllowedLength(pathname) { + return pathname.length > 0 && pathname.length <= MAX_PATH +} diff --git a/services/web/frontend/js/ide/directives/SafePath.js b/services/web/frontend/js/ide/directives/SafePath.js index 123fb48b50..5fcbcb1044 100644 --- a/services/web/frontend/js/ide/directives/SafePath.js +++ b/services/web/frontend/js/ide/directives/SafePath.js @@ -12,9 +12,10 @@ */ // This file is shared between the frontend and server code of web, so that // filename validation is the same in both implementations. -// Both copies must be kept in sync: -// app/coffee/Features/Project/SafePath.coffee -// public/coffee/ide/directives/SafePath.coffee +// The logic in all copies must be kept in sync: +// app/src/Features/Project/SafePath.js +// frontend/js/ide/directives/SafePath.js +// frontend/js/features/file-tree/util/safe-path.js let SafePath const BADCHAR_RX = new RegExp( diff --git a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js index a9022533f0..57cea297c1 100644 --- a/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js +++ b/services/web/test/frontend/features/file-tree/components/file-tree-root.test.js @@ -23,6 +23,7 @@ describe('', function() { it('renders', function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [{ _id: '456def', name: 'main.tex' }], folders: [], fileRefs: [] @@ -47,6 +48,7 @@ describe('', function() { it('fire onSelect', function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [ { _id: '456def', name: 'main.tex' }, { _id: '789ghi', name: 'other.tex' } @@ -93,6 +95,7 @@ describe('', function() { it('listen to editor.openDoc', function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [ { _id: '456def', name: 'main.tex' }, { _id: '789ghi', name: 'other.tex' } diff --git a/services/web/test/frontend/features/file-tree/flows/create-folder.test.js b/services/web/test/frontend/features/file-tree/flows/create-folder.test.js index e92f405db7..dce089a3df 100644 --- a/services/web/test/frontend/features/file-tree/flows/create-folder.test.js +++ b/services/web/test/frontend/features/file-tree/flows/create-folder.test.js @@ -201,19 +201,62 @@ describe('FileTree Create Folder Flow', function() { expect(screen.queryByRole('treeitem', { name: newFolderName })).to.not.exist }) + it('prevents adding duplicate or invalid names', async function() { + const rootFolder = [ + { + _id: 'root-folder-id', + docs: [{ _id: '456def', name: 'existingFile' }], + folders: [], + fileRefs: [] + } + ] + render( + + ) + + var newFolderName = 'existingFile' + + fireCreateFolder(newFolderName) + + expect(fetchMock.called()).to.be.false + + await screen.findByRole('alert', { + name: 'A file or folder with this name already exists', + hidden: true + }) + + newFolderName = 'in/valid ' + setFolderName(newFolderName) + await screen.findByRole('alert', { + name: 'File name is empty or contains invalid characters', + hidden: true + }) + }) + function fireCreateFolder(name) { const createFolderButton = screen.getByRole('button', { name: 'New Folder' }) fireEvent.click(createFolderButton) + setFolderName(name) + + const modalCreateButton = getModalCreateButton() + fireEvent.click(modalCreateButton) + } + + function setFolderName(name) { const input = screen.getByRole('textbox', { hidden: true // FIXME: modal should not be hidden but it has the aria-hidden label due to a react-bootstrap bug }) fireEvent.change(input, { target: { value: name } }) - - const modalCreateButton = getModalCreateButton() - fireEvent.click(modalCreateButton) } function fakeId() { diff --git a/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js b/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js index 6cf96ed7d1..a75e0dde43 100644 --- a/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js +++ b/services/web/test/frontend/features/file-tree/flows/delete-entity.test.js @@ -28,6 +28,7 @@ describe('FileTree Delete Entity Flow', function() { beforeEach(function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [{ _id: '456def', name: 'main.tex' }], folders: [], fileRefs: [] @@ -118,6 +119,7 @@ describe('FileTree Delete Entity Flow', function() { beforeEach(function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [{ _id: '456def', name: 'main.tex' }], folders: [], fileRefs: [{ _id: '789ghi', name: 'my.bib' }] diff --git a/services/web/test/frontend/features/file-tree/flows/rename-entity.test.js b/services/web/test/frontend/features/file-tree/flows/rename-entity.test.js index 09427b1620..7cb1165424 100644 --- a/services/web/test/frontend/features/file-tree/flows/rename-entity.test.js +++ b/services/web/test/frontend/features/file-tree/flows/rename-entity.test.js @@ -29,13 +29,17 @@ describe('FileTree Rename Entity Flow', function() { beforeEach(function() { const rootFolder = [ { + _id: 'root-folder-id', docs: [{ _id: '456def', name: 'a.tex' }], folders: [ { _id: '987jkl', name: 'folder', docs: [], - fileRefs: [{ _id: '789ghi', name: 'c.tex' }], + fileRefs: [ + { _id: '789ghi', name: 'c.tex' }, + { _id: '981gkp', name: 'e.tex' } + ], folders: [] } ], @@ -110,6 +114,42 @@ describe('FileTree Rename Entity Flow', function() { screen.getByRole('treeitem', { name: 'b.tex' }) }) + it('shows error modal on invalid filename', async function() { + const input = initItemRename('a.tex') + fireEvent.change(input, { target: { value: '///' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await screen.findByRole('alert', { + name: 'File name is empty or contains invalid characters', + hidden: true + }) + }) + + it('shows error modal on duplicate filename', async function() { + const input = initItemRename('a.tex') + fireEvent.change(input, { target: { value: 'folder' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await screen.findByRole('alert', { + name: 'A file or folder with this name already exists', + hidden: true + }) + }) + + it('shows error modal on duplicate filename in subfolder', async function() { + const expandButton = screen.getByRole('button', { name: 'Expand' }) + fireEvent.click(expandButton) + + const input = initItemRename('c.tex') + fireEvent.change(input, { target: { value: 'e.tex' } }) + fireEvent.keyDown(input, { key: 'Enter' }) + + await screen.findByRole('alert', { + name: 'A file or folder with this name already exists', + hidden: true + }) + }) + describe('via socket event', function() { it('renames doc', function() { screen.getByRole('treeitem', { name: 'a.tex' })