Validate file and folder names to avoid illegal characters/reserved filenames

Author: Mythicaeda

workspace-server/build.gradle

@@ -35,6 +35,7 @@ dependencies {
   implementation 'com.zaxxer:HikariCP:5.0.1'
 
   testImplementation 'org.junit.jupiter:junit-jupiter-engine:6.0.1'
+  testImplementation 'org.junit.jupiter:junit-jupiter-params:6.0.1'
   testImplementation 'net.jqwik:jqwik:1.6.5'
 
   testRuntimeOnly 'org.junit.platform:junit-platform-launcher'

workspace-server/src/main/java/gov/nasa/jpl/aerie/workspace/server/WorkspaceBindings.java

@@ -424,10 +424,14 @@ private void put(Context context) throws NoSuchWorkspaceException, IOException {
         return;
       }
 
-      if (workspaceService.saveFile(pathInfo.workspaceId, pathInfo.filePath, file)) {
-        context.status(200).result("File " + pathInfo.fileName() + " uploaded to " + pathInfo.filePath);
-      } else {
-        context.status(500).json(new FormattedError("Could not save file."));
+      try {
+        if (workspaceService.saveFile(pathInfo.workspaceId, pathInfo.filePath, file)) {
+          context.status(200).result("File " + pathInfo.fileName() + " uploaded to " + pathInfo.filePath);
+        } else {
+          context.status(500).json(new FormattedError("Could not save file."));
+        }
+      } catch (WorkspaceFileOpException wfe) {
+        context.status(500).json(new FormattedError(wfe, "Could not save file."));
       }
     } else if (type == ItemType.directory) {
       // Reject the request if the "overwrite" flag is supplied
@@ -436,10 +440,14 @@ private void put(Context context) throws NoSuchWorkspaceException, IOException {
         return;
       }
 
-      if (workspaceService.createDirectory(pathInfo.workspaceId, pathInfo.filePath)) {
-        context.status(200).result("Directory created.");
-      } else {
-        context.status(500).json(new FormattedError("Could not create directory."));
+      try {
+        if (workspaceService.createDirectory(pathInfo.workspaceId, pathInfo.filePath)) {
+          context.status(200).result("Directory created.");
+        } else {
+          context.status(500).json(new FormattedError("Could not create directory."));
+        }
+      } catch (WorkspaceFileOpException wfe) {
+        context.status(500).json(new FormattedError(wfe, "Could not create directory."));
       }
     } else {
       context.status(400).json(new FormattedError("Query param 'type' has invalid value "+type));
@@ -803,6 +811,9 @@ public void bulkUpload(Context context) throws NoSuchWorkspaceException {
         } catch (IOException ioe) {
           response.add("status", 500)
                   .add("response", new FormattedError(ioe, "Could not save file.").toJson());
+        } catch (WorkspaceFileOpException wfe) {
+          response.add("status", 500)
+                  .add("response", new FormattedError(wfe, "Could not create directory.").toJson());
         }
       } else if (item.uploadType() == ItemType.directory) {
         // Create directory
@@ -817,6 +828,9 @@ public void bulkUpload(Context context) throws NoSuchWorkspaceException {
         } catch (IOException ioe) {
           response.add("status", 500)
                   .add("response", new FormattedError(ioe, "Could not create directory.").toJson());
+        } catch (WorkspaceFileOpException wfe) {
+          response.add("status", 500)
+                  .add("response", new FormattedError(wfe, "Could not create directory.").toJson());
         }
       } else {
         response.add("status", 501)

workspace-server/src/main/java/gov/nasa/jpl/aerie/workspace/server/WorkspaceFileSystemService.java

@@ -15,7 +15,9 @@
 import java.nio.file.StandardCopyOption;
 import java.sql.SQLException;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Optional;
+import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -45,6 +47,65 @@ Path resolveSubPath(final Path rootPath, final Path filePath) {
     return resolvedPath;
   }
 
+  /**
+   * Validates that the path does not contain any invalid characters.
+   *
+   * Forbidden Characters for File and Folder names:
+   *   < (less than), > (greater than), : (colon), " (double quote), / (forward slash), \ (backslash),
+   *   | (vertical bar or pipe), ? (question mark), * (asterisk),
+   *   % (percent sign - causes issues with URL path resolution as it is not automatically encoded),
+   *   # (pound sign - causes issues with URL path resolution as it is not automatically encoded),
+   *   Unicode Control Characters (0-31, 127-159),
+   *   trailing .
+   *   trailing space
+   *
+   * While / (forward slash) is a forbidden characters in filenames, it's interpreted by Java's Path class as a
+   *  folder delineator, meaning that it will not appear as a path segment.
+   *  The character is still checked for just in case.
+   *
+   * Reserved Filenames (these are not permitted on Windows even if they have an extension):
+   *   CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9,
+   *   LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, LPT9
+   *
+   * @param path the Path to validate
+   */
+  void validatePath(final Path path) throws WorkspaceFileOpException {
+    final String[] reservedFilenames = {"CON", "PRN", "AUX", "NUL", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7",
+                                         "COM8", "COM9", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"};
+    final var controlCharacters = Pattern.compile("([\u0000-\u001F]|[\u007F-\u009F])+", Pattern.UNICODE_CHARACTER_CLASS);
+    final var forbiddenCharacters = Pattern.compile("([|<>:/\"?*%#\\\\])+", Pattern.UNICODE_CHARACTER_CLASS);
+
+
+    for(final var pathSegment : path) {
+      final var segment = pathSegment.toString();
+      // Check for trailing period or space
+      if(segment.endsWith(" ")) {
+        throw new WorkspaceFileOpException("Path segment '"+ segment+ "' cannot end in a space.");
+      }
+      if(segment.endsWith(".")) {
+        throw new WorkspaceFileOpException("Path segment '"+ segment+ "' cannot end in a period.");
+      }
+
+      // Check for control characters
+      final var controlMatcher = controlCharacters.matcher(segment);
+      if(controlMatcher.find()){
+        throw new WorkspaceFileOpException("Path segment '"+ segment+ "' has illegal characters: "+controlMatcher.group());
+      }
+
+      // Check for forbidden characters
+      final var forbiddenMatcher = forbiddenCharacters.matcher(segment);
+      if(forbiddenMatcher.find()){
+        throw new WorkspaceFileOpException("Path segment '"+ segment+ "' has illegal characters: "+forbiddenMatcher.group());
+      }
+
+      // Check that the segment is not a reserved filenames:
+      final var name = segment.split("\\.")[0];
+      if(Arrays.asList(reservedFilenames).contains(name)){
+        throw new WorkspaceFileOpException("Path segment '"+ segment+ "' contains reserved name: "+name);
+      }
+    }
+  }
+
   public WorkspaceFileSystemService(final WorkspacePostgresRepository postgresRepository) {
     this.postgresRepository = postgresRepository;
   }
@@ -149,26 +210,29 @@ public FileStream loadFile(final int workspaceId, final Path filePath) throws IO
 
   @Override
   public boolean saveFile(final int workspaceId, final Path filePath, final UploadedFile file)
-  throws NoSuchWorkspaceException {
+  throws NoSuchWorkspaceException, WorkspaceFileOpException {
     final var repoPath = postgresRepository.workspaceRootPath(workspaceId);
     final var path = resolveSubPath(repoPath, filePath);
 
     if(path.toFile().isDirectory()) return false;
 
+    validatePath(path);
     FileUtil.streamToFile(file.content(), path.toString());
     return true;
   }
 
   @Override
   public boolean moveFile(final int oldWorkspaceId, final Path oldFilePath, final int newWorkspaceId, final Path newFilePath)
-  throws NoSuchWorkspaceException, SQLException
+  throws NoSuchWorkspaceException, SQLException, WorkspaceFileOpException
   {
     final var oldRepoPath = postgresRepository.workspaceRootPath(oldWorkspaceId);
     final var oldPath = resolveSubPath(oldRepoPath, oldFilePath);
     final var newRepoPath = (oldWorkspaceId == newWorkspaceId) ? oldRepoPath : postgresRepository.workspaceRootPath(newWorkspaceId);
     final var newPath = resolveSubPath(newRepoPath, newFilePath);
     boolean success = true;
 
+    validatePath(newPath);
+
     // Find hidden metadata files, if they exist, and move them
     final var metadataExtensions = postgresRepository.getMetadataExtensions();
     for(final var extension : metadataExtensions) {
@@ -191,6 +255,8 @@ public boolean copyFile(final int sourceWorkspaceId, final Path sourceFilePath,
     final var destRepoPath = (sourceWorkspaceId == destWorkspaceId) ? sourceRepoPath : postgresRepository.workspaceRootPath(destWorkspaceId);
     final var destPath = resolveSubPath(destRepoPath, destFilePath);
 
+    validatePath(destPath);
+
     try {
       // Do not copy the file if the source file does not exist
       if(!sourcePath.toFile().exists()) throw new WorkspaceFileOpException("Source file \"%s\" in workspace %d does not exist.".formatted(sourceFilePath, sourceWorkspaceId));
@@ -228,10 +294,11 @@ public boolean copyDirectory(final int sourceWorkspaceId, final Path sourceFileP
     final var destRepoPath = (sourceWorkspaceId == destWorkspaceId) ? sourceRepoPath : postgresRepository.workspaceRootPath(destWorkspaceId);
     final var destPath = resolveSubPath(destRepoPath, destFilePath);
 
+    validatePath(destPath);
     try {
       // Validate source exists and is a directory
       if (!Files.exists(sourcePath)) throw new WorkspaceFileOpException("Source directory \"%s\" in workspace %d does not exist.".formatted(sourceFilePath, sourceWorkspaceId));
-      if (!Files.isDirectory(sourcePath)) throw new WorkspaceFileOpException("Source directory \"%s\" in workspace %d is not actually a directory.".formatted(sourceFilePath, sourceWorkspaceId));
+      if (!Files.isDirectory(sourcePath)) throw new WorkspaceFileOpException("Source directory \"%s\" in workspace %d is not a directory.".formatted(sourceFilePath, sourceWorkspaceId));
 
       // Do not try to copy a directory into itself
       if(sourceWorkspaceId == destWorkspaceId && destPath.startsWith(sourcePath)){
@@ -304,9 +371,11 @@ public DirectoryTree listFiles(final int workspaceId, final Optional<Path> direc
   }
 
   @Override
-  public boolean createDirectory(final int workspaceId, final Path directoryPath) throws IOException, NoSuchWorkspaceException {
+  public boolean createDirectory(final int workspaceId, final Path directoryPath)
+  throws IOException, NoSuchWorkspaceException, WorkspaceFileOpException {
     final var repoPath = postgresRepository.workspaceRootPath(workspaceId);
     final var path = resolveSubPath(repoPath, directoryPath);
+    validatePath(path);
     Files.createDirectories(path);
     return true;
   }
@@ -320,6 +389,7 @@ public boolean moveDirectory(final int oldWorkspaceId, final Path oldDirectoryPa
     final var oldPath = resolveSubPath(oldRepoPath, oldDirectoryPath);
     final var newRepoPath = (oldWorkspaceId == newWorkspaceId) ? oldRepoPath : postgresRepository.workspaceRootPath(newWorkspaceId).normalize();
     final var newPath = resolveSubPath(newRepoPath, newDirectoryPath);
+    validatePath(newPath);
 
     // Do not permit the source workspace's root directory to be moved
     if(Files.isSameFile(oldPath, oldRepoPath)) throw new WorkspaceFileOpException("Cannot move the workspace root directory.");

workspace-server/src/main/java/gov/nasa/jpl/aerie/workspace/server/WorkspaceService.java

@@ -46,7 +46,7 @@ record FileStream(InputStream readingStream, String fileName, long fileSize){}
    * @return true if the file was saved, false otherwise
    */
   boolean saveFile(final int workspaceId, final Path filePath, final UploadedFile file)
-  throws IOException, NoSuchWorkspaceException;
+  throws IOException, NoSuchWorkspaceException, WorkspaceFileOpException;
 
   /**
   * Copy a file within a workspace or between workspaces.
@@ -84,7 +84,8 @@ boolean deleteFile(final int workspaceId, final Path filePath)
   DirectoryTree listFiles(final int workspaceId, final Optional<Path> directoryPath, final int depth)
   throws SQLException, NoSuchWorkspaceException, IOException;
 
-  boolean createDirectory(final int workspaceId, final Path directoryPath) throws IOException, NoSuchWorkspaceException;
+  boolean createDirectory(final int workspaceId, final Path directoryPath)
+  throws IOException, NoSuchWorkspaceException, WorkspaceFileOpException;
   /**
    * Move a directory within a workspace or between workspaces.
    * @param oldWorkspaceId the id of the source workspace

workspace-server/src/test/java/gov/nasa/jpl/aerie/workspace/server/WorkspaceFileSystemServiceTest.java

@@ -3,6 +3,8 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.FieldSource;
 
 import java.nio.file.Path;
 
@@ -57,4 +59,100 @@ void pathTraversalThrowsSecurityException() {
           service.resolveSubPath(Path.of("/workspace/123"), Path.of("folder/../../workspace/123/../456/file.txt")));
     }
   }
+
+  @Nested
+  class ValidatePathTests {
+    final static String[] reservedFileNames = {
+        "CON", "PRN", "AUX", "NUL",
+        "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", "COM8", "COM9",
+        "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", "LPT9"
+    };
+    final static String[] forbiddenChars = {
+        // Normal forbidden characters
+        "<", ">", ":", "\"", "\\", "|", "?", "*", "%", "#",
+        // Unicode/ASCII 0-31 control characters
+        // "\u0000" is not tested as it isn't permitted in a Path, meaning it cannot be passed to validatePath
+        "\u0001", "\u0002", "\u0003", "\u0004", "\u0005", "\u0006", "\u0007", "\u0008", "\t",
+        "\n", "\u000b", "\u000c", "\r", "\u000e", "\u000f",
+        "\u0010", "\u0011", "\u0012", "\u0013", "\u0014", "\u0015", "\u0016", "\u0017", "\u0018", "\u0019",
+        "\u001A", "\u001b", "\u001c", "\u001d", "\u001e", "\u001f",
+        // Unicode 127-159 control characters
+        "\u007F", "\u0080", "\u0081", "\u0082", "\u0083", "\u0084", "\u0085", "\u0086", "\u0087", "\u0088", "\u0089",
+        "\u008A", "\u008B", "\u008C", "\u008D", "\u008E", "\u008F",
+        "\u0090", "\u0091", "\u0092", "\u0093", "\u0094", "\u0095", "\u0096", "\u0097", "\u0098", "\u0099",
+        "\u009A", "\u009B", "\u009C", "\u009D", "\u009E", "\u009F",
+        };
+    final static String[] trailingChars = {"foo ", "foo.", " ", "."};
+
+    @ParameterizedTest
+    @FieldSource("forbiddenChars")
+    void validatePathForbiddenChars(String forbidden) {
+      // These characters are not allowed on their own
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(forbidden)));
+      // These characters are not allowed as part of a longer file name
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("foobar" + forbidden)));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("file"+forbidden+".txt")));
+      // There characters are not allowed as the last part of the path
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("folder", forbidden)));
+      // There characters are not allowed as part of a folder name
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(forbidden, "file")));
+    }
+
+    @ParameterizedTest
+    @FieldSource("trailingChars")
+    void validatePathTrailingChars(String forbidden) {
+      // Trailing characters are not permitted at the end of a file
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(forbidden)));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("foobar" + forbidden)));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("folder", forbidden)));
+
+      // Trailing characters are permitted before an extension (as the extension makes them non-trailing)
+      assertDoesNotThrow(() -> service.validatePath(Path.of("file"+forbidden+".txt")));
+
+      // Trailing characters are not permitted in folder names
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(forbidden, "file")));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("folder"+forbidden, "file")));
+    }
+
+    @ParameterizedTest
+    @FieldSource("reservedFileNames")
+    void validatePathReservedFilenames(String reserved) {
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(reserved)));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(reserved+".txt")));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("folder/"+reserved)));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of("folder/"+reserved+".txt")));
+      assertThrows(WorkspaceFileOpException.class, () -> service.validatePath(Path.of(reserved, "file.txt")));
+
+      // They are allowed to be PART of the file name
+      assertDoesNotThrow(() -> service.validatePath(Path.of("myFile_"+reserved)));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("myFile_"+reserved+".txt")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("myFolder_"+reserved, "myFile.txt")));
+    }
+
+    /**
+     * Forward slashes are considered a path delineator and will not throw
+     */
+    @Test
+    void forwardSlash() {
+      assertDoesNotThrow(() -> service.validatePath(Path.of("/")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("foobar/")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("file/.txt")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("folder", "/")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("folder/")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("/", "file.txt")));
+    }
+
+    /**
+     * Spaces and periods are allowed in the path so long as they are not trailing
+     */
+    @Test
+    void validatePathPermitsSpacePeriod() {
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my file")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my folder","my file")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my folder", " my file")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my.file")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my.folder","my.file")));
+      assertDoesNotThrow(() -> service.validatePath(Path.of("my.folder",".my.file")));
+    }
+  }
 }