Merge pull request #3410 from overleaf/hb-validate-new-folder

React File Tree - Validate new folder names and renames

GitOrigin-RevId: f040eb08e2daefb3dfd639e18aaef95d7123d727

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

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"
 ]

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 (

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({
       </div>
       <FileTreeModalDelete />
       <FileTreeModalCreateFolder />
+      <FileTreeModalError />
     </FileTreeContext>
   )
 }

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 (
     <Modal show={isCreatingFolder} onHide={handleHide}>
       <Modal.Header>
@@ -39,13 +53,28 @@ function FileTreeModalCreateFolder() {
         <InputName
           name={name}
           setName={setName}
+          validName={validName}
+          setValidName={setValidName}
           handleCreateFolder={handleCreateFolder}
         />
-        {error && (
-          <div className="alert alert-danger file-tree-modal-alert">
-            {t('generic_something_went_wrong')}
+        {!validName ? (
+          <div
+            role="alert"
+            aria-label={t('files_cannot_include_invalid_characters')}
+            className="alert alert-danger file-tree-modal-alert"
+          >
+            {t('files_cannot_include_invalid_characters')}
           </div>
-        )}
+        ) : null}
+        {error ? (
+          <div
+            role="alert"
+            aria-label={errorMessage()}
+            className="alert alert-danger file-tree-modal-alert"
+          >
+            {errorMessage()}
+          </div>
+        ) : null}
       </Modal.Body>
 
       <Modal.Footer>
@@ -56,7 +85,11 @@ function FileTreeModalCreateFolder() {
         ) : (
           <>
             <Button onClick={handleHide}>{t('cancel')}</Button>
-            <Button bsStyle="primary" onClick={handleCreateFolder}>
+            <Button
+              bsStyle="primary"
+              onClick={handleCreateFolder}
+              disabled={!validName}
+            >
               {t('create')}
             </Button>
           </>
@@ -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
 }
 

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 (
+    <Modal show onHide={handleHide}>
+      <Modal.Header>
+        <Modal.Title>{errorTitle()}</Modal.Title>
+      </Modal.Header>
+
+      <Modal.Body>
+        <div role="alert" aria-label={errorMessage()}>
+          {errorMessage()}
+        </div>
+      </Modal.Body>
+
+      <Modal.Footer>
+        <Button onClick={handleHide}>{t('ok')}</Button>
+      </Modal.Footer>
+    </Modal>
+  )
+}
+
+export default FileTreeModalError

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) {

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')
+  }
+}

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
+}

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
+}

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(

services/web/test/frontend/features/file-tree/components/file-tree-root.test.js

@@ -23,6 +23,7 @@ describe('<FileTreeRoot/>', function() {
   it('renders', function() {
     const rootFolder = [
       {
+        _id: 'root-folder-id',
         docs: [{ _id: '456def', name: 'main.tex' }],
         folders: [],
         fileRefs: []
@@ -47,6 +48,7 @@ describe('<FileTreeRoot/>', 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('<FileTreeRoot/>', function() {
   it('listen to editor.openDoc', function() {
     const rootFolder = [
       {
+        _id: 'root-folder-id',
         docs: [
           { _id: '456def', name: 'main.tex' },
           { _id: '789ghi', name: 'other.tex' }

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(
+      <FileTreeRoot
+        rootFolder={rootFolder}
+        projectId="123abc"
+        hasWritePermissions
+        rootDocId="456def"
+        onSelect={onSelect}
+        onInit={onInit}
+      />
+    )
+
+    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() {

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' }]

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' })