#20 Improve location checking

files/pom.xml

@@ -68,6 +68,11 @@
         <!--            <artifactId>spring-security-test</artifactId>-->
         <!--            <scope>test</scope>-->
         <!--        </dependency>-->
+        <dependency>
+            <groupId>junit</groupId>
+            <artifactId>junit</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 
 </project>

files/src/main/java/de/nbscloud/files/FileSystemService.java

@@ -1,7 +1,6 @@
 package de.nbscloud.files;
 
 import de.nbscloud.files.config.FilesConfig;
-import de.nbscloud.files.controller.FilesController;
 import de.nbscloud.files.exception.FileSystemServiceException;
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.lang3.StringUtils;
@@ -89,7 +88,7 @@ public class FileSystemService {
 
     public Path createDirectory(String name) {
         try {
-            return Files.createDirectories(this.locationTracker.getCurrentLocation().resolve(name));
+            return Files.createDirectories(this.locationTracker.resolve(name));
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not create directory", e);
         }
@@ -97,8 +96,7 @@ public class FileSystemService {
 
     public void createFile(String name, byte[] content) {
         try {
-            Files.write(this.locationTracker.getCurrentLocation()
+            Files.write(this.locationTracker.resolve(name), content, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW);
-                                            .resolve(name), content, StandardOpenOption.WRITE, StandardOpenOption.CREATE_NEW);
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not create file", e);
         }
@@ -112,15 +110,6 @@ public class FileSystemService {
         }
     }
 
-    public boolean delete(String name) {
-        try {
-            // TODO does only delete dirs if they are empty - but maybe we want that?
-            return Files.deleteIfExists(this.locationTracker.getCurrentLocation().resolve(name));
-        } catch (IOException e) {
-            throw new FileSystemServiceException("Could not delete file", e);
-        }
-    }
-
     public Path move(Path originalPath, Path newPath) {
         try {
             return Files.move(originalPath, newPath, StandardCopyOption.ATOMIC_MOVE);
@@ -129,9 +118,18 @@ public class FileSystemService {
         }
     }
 
-    public byte[] get(String name) {
+    public boolean delete(String name) {
         try {
-            return Files.readAllBytes(this.locationTracker.getCurrentLocation().resolve(name));
+            // TODO does only delete dirs if they are empty - but maybe we want that?
+            return Files.deleteIfExists(this.locationTracker.resolve(name));
+        } catch (IOException e) {
+            throw new FileSystemServiceException("Could not delete file", e);
+        }
+    }
+
+    public byte[] get(Path path) {
+        try {
+            return Files.readAllBytes(path);
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not get file", e);
         }
@@ -139,7 +137,7 @@ public class FileSystemService {
 
     public void stream(String name, OutputStream outputStream) {
         try {
-            Files.copy(this.locationTracker.getCurrentLocation().resolve(name), outputStream);
+            Files.copy(this.locationTracker.resolve(name), outputStream);
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not get file", e);
         }
@@ -147,7 +145,7 @@ public class FileSystemService {
 
     public InputStream stream(String name) {
         try {
-            return Files.newInputStream(this.locationTracker.getCurrentLocation().resolve(name));
+            return Files.newInputStream(this.locationTracker.resolve(name));
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not get file", e);
         }
@@ -155,7 +153,7 @@ public class FileSystemService {
 
     public long getSize(String name) {
         try {
-            return Files.size(this.locationTracker.getCurrentLocation().resolve(name));
+            return Files.size(this.locationTracker.resolve(name));
         } catch (IOException e) {
             throw new FileSystemServiceException("Could not get file", e);
         }
@@ -163,8 +161,7 @@ public class FileSystemService {
 
     public String getMimeType(String name) {
         try {
-            final String detectedMimeType = this.mimeTypeDetector.detectMimeType(this.locationTracker.getCurrentLocation()
+            final String detectedMimeType = this.mimeTypeDetector.detectMimeType(this.locationTracker.resolve(name));
-                                                                                                     .resolve(name));
 
             logger.debug("Detected mime type {} for file {}", detectedMimeType, name);
 
@@ -237,7 +234,7 @@ public class FileSystemService {
     public List<String> collectDirs(String sourceFile) {
         try {
             final List<String> resultList = new ArrayList<>();
-            final Path sourcePath = this.locationTracker.getCurrentLocation().resolve(sourceFile);
+            final Path sourcePath = this.locationTracker.resolve(sourceFile);
             final boolean sourceIsDir = Files.isDirectory(sourcePath);
 
             Files.walkFileTree(this.locationTracker.getBaseDirPath(), new SimpleFileVisitor<Path>() {

files/src/main/java/de/nbscloud/files/LocationTracker.java

@@ -1,7 +1,6 @@
 package de.nbscloud.files;
 
 import de.nbscloud.files.config.FilesConfig;
-import de.nbscloud.files.controller.FilesController;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -20,6 +19,12 @@ public class LocationTracker {
     private Path baseDirPath;
     private Path currentLocation;
 
+    // For testing
+    void init_internal(String baseDir) {
+        this.baseDirPath = Paths.get(baseDir);
+        this.currentLocation = this.baseDirPath.resolve("");
+    }
+
     public void init() {
         this.baseDirPath = Paths.get(this.filesConfig.getBaseDir());
         this.currentLocation = this.baseDirPath.resolve("");
@@ -33,19 +38,19 @@ public class LocationTracker {
         init();
     }
 
-    public Path getCurrentLocation() {
+    Path getCurrentLocation() {
         return this.currentLocation;
     }
 
-    public Path getRelativeLocation() {
+    public String getRelativeLocation() {
-        return this.baseDirPath.relativize(this.currentLocation);
+        return this.baseDirPath.relativize(this.currentLocation).toString();
     }
 
     public Path getRelativeToBaseDir(Path other) {
         return this.baseDirPath.relativize(other);
     }
 
-    public Path getTrashBin() {
+    private Path getTrashBin() {
         return this.baseDirPath.resolve(this.filesConfig.getTrashBinName());
     }
 
@@ -80,12 +85,42 @@ public class LocationTracker {
             throw new IllegalStateException("Absolute path: " + navigateTo);
         }
 
-        if(!this.baseDirPath.resolve(navigateTo).startsWith(this.baseDirPath)) {
+        // Regardless of where we navigate to, we must never leave the base directory
+        // The normalize() call is important, as it resolves (possible) relative paths
+        if(!this.currentLocation.resolve(navigateTo).normalize().startsWith(this.baseDirPath)) {
+            throw new IllegalStateException("Illegal path: " + navigateTo);
+        }
+
+        if(!this.baseDirPath.resolve(navigateTo).normalize().startsWith(this.baseDirPath)) {
             throw new IllegalStateException("Illegal path: " + navigateTo);
         }
     }
 
-    public Path getBaseDirPath() {
+    public Path resolve(String name) {
-        return baseDirPath;
+        validate(name);
+
+        return this.currentLocation.resolve(name).normalize();
+    }
+
+    public Path resolveToBaseDir(String name) {
+        validate(name);
+
+        return this.baseDirPath.resolve(name).normalize();
+    }
+
+    public Path resolveShare(String shareUuid) {
+        validate(shareUuid);
+
+        return this.baseDirPath.resolve(this.filesConfig.getSharesName()).resolve(shareUuid);
+    }
+
+    public Path resolveTrash(String name) {
+        validate(name);
+
+        return this.baseDirPath.resolve(this.filesConfig.getTrashBinName()).resolve(name);
+    }
+
+    Path getBaseDirPath() {
+        return this.baseDirPath;
     }
 }

files/src/main/java/de/nbscloud/files/controller/FilesController.java

@@ -82,8 +82,8 @@ public class FilesController implements InitializingBean {
             try {
                 // Soft delete
                 this.fileSystemService.move(
-                        this.locationTracker.getCurrentLocation().resolve(Path.of(filename)),
+                        this.locationTracker.resolve(filename),
-                        this.locationTracker.getTrashBin().resolve(filename));
+                        this.locationTracker.resolveTrash(filename));
 
                 this.infoMessages.add("nbscloud.files.delete.success");
             } catch (RuntimeException e) {
@@ -114,12 +114,12 @@ public class FilesController implements InitializingBean {
 
     @PostMapping("/files/doRename")
     public String doRename(RenameForm form) {
-        final Path sourcePath = this.locationTracker.getCurrentLocation().resolve(Path.of(form.getOriginalFilename()));
+        final Path sourcePath = this.locationTracker.resolve(form.getOriginalFilename());
 
         // We navigate to the target dir, so we display the content of that dir after the move
         this.locationTracker.setCurrentLocation(form.getTargetDir().substring(1));
 
-        final Path targetPath = this.locationTracker.getCurrentLocation().resolve(Path.of(form.getFilename()));
+        final Path targetPath = this.locationTracker.resolve(form.getFilename());
 
         try {
             this.fileSystemService.move(sourcePath, targetPath);
@@ -158,7 +158,7 @@ public class FilesController implements InitializingBean {
                 this.fileSystemService.createFile(file.getOriginalFilename(), file.getBytes());
 
                 this.infoMessages.add("nbscloud.files.created.file.success");
-            } catch (IOException e) {
+            } catch (IOException | RuntimeException e) {
                 logger.error("Could not upload file", e);
 
                 this.errors.add(e.getMessage());
@@ -185,15 +185,11 @@ public class FilesController implements InitializingBean {
 
     @GetMapping("/files/share")
     public String share(String filename) {
-        final Path filePath = this.locationTracker.getRelativeToBaseDir(this.locationTracker.getCurrentLocation()
+        final Path filePath = this.locationTracker.getRelativeToBaseDir(this.locationTracker.resolve(filename));
-                                                                                            .resolve(filename));
         final String shareUuid = UUID.randomUUID().toString();
 
         try {
-            this.fileSystemService.createFile(this.locationTracker.getBaseDirPath()
+            this.fileSystemService.createFile(this.locationTracker.resolveShare(shareUuid), filePath.toString().getBytes(StandardCharsets.UTF_8));
-                                                                  .resolve(this.filesConfig.getSharesName())
-                                                                  .resolve(shareUuid), filePath.toString()
-                                                                                               .getBytes(StandardCharsets.UTF_8));
 
             this.shareInfo.add("/files/shares?shareUuid=" + shareUuid);
         } catch (RuntimeException e) {
@@ -209,11 +205,8 @@ public class FilesController implements InitializingBean {
     @GetMapping("files/shares")
     public ResponseEntity shares(String shareUuid) {
         try {
-            final String sharedFile = new String(this.fileSystemService.get(this.locationTracker.getBaseDirPath()
+            final String sharedFile = new String(this.fileSystemService.get(this.locationTracker.resolveShare(shareUuid)));
-                                                                                                .resolve(this.filesConfig.getSharesName())
+            final Path sharedFilePath = this.locationTracker.resolveToBaseDir(sharedFile);
-                                                                                                .resolve(shareUuid)
-                                                                                                .toString()));
-            final Path sharedFilePath = this.locationTracker.getBaseDirPath().resolve(sharedFile);
             final String filename = sharedFilePath.getFileName().toString();
 
             return ResponseEntity.ok()
@@ -295,19 +288,19 @@ public class FilesController implements InitializingBean {
     }
 
     private String getCurrentLocation() {
-        if (this.locationTracker.getRelativeLocation().toString().isEmpty()) {
+        if (this.locationTracker.getRelativeLocation().isEmpty()) {
             return "/";
         }
 
-        return this.locationTracker.getRelativeLocation().toString();
+        return this.locationTracker.getRelativeLocation();
     }
 
     private String getCurrentLocationPrefixed() {
-        if (this.locationTracker.getRelativeLocation().toString().isEmpty()) {
+        if (this.locationTracker.getRelativeLocation().isEmpty()) {
             return "/";
         }
 
-        return "/" + this.locationTracker.getRelativeLocation().toString();
+        return "/" + this.locationTracker.getRelativeLocation();
     }
 
     @Override

files/src/test/java/de/nbscloud/files/LocationTrackerTest.java

@@ -0,0 +1,75 @@
+package de.nbscloud.files;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+public class LocationTrackerTest {
+    private static final String BASE_DIR = "/tmp";
+
+    private LocationTracker locationTracker = new LocationTracker();
+
+    @Before
+    public void init() {
+        this.locationTracker.init_internal(BASE_DIR);
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_null() {
+        this.locationTracker.resolve(null);
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative1() {
+        this.locationTracker.resolve("../test");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative2() {
+        this.locationTracker.resolve("/../test");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative3() {
+        this.locationTracker.resolve("\\../test");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative4() {
+        this.locationTracker.resolve("\\/../test");
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative5() {
+        this.locationTracker.resolve("..");
+    }
+
+    @Test
+    public void test_relative6_ok() {
+        // While 0x2E translates to the dot character and that might be used to
+        // traverse the path, the Java Path API does not resolve those and just treats them as
+        // any other file/dir name
+        Assert.assertTrue(this.locationTracker.resolve("%2e%2e%2f").toString().equals(BASE_DIR + "/%2e%2e%2f"));
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_relative7() {
+        this.locationTracker.resolve("...");
+    }
+
+    @Test
+    public void test_relative8_ok() {
+        Assert.assertTrue(this.locationTracker.resolve("././").toString().equals(BASE_DIR));
+    }
+
+    @Test
+    public void test_relative9_ok() {
+        // The tilde is treated as regular file/dir name if it is resolved from a path
+        Assert.assertTrue(this.locationTracker.resolve("~").toString().equals(BASE_DIR + "/~"));
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void test_absolut() {
+        this.locationTracker.resolve("/bin");
+    }
+}

pom.xml

@@ -1,7 +1,6 @@
 <?xml version="1.0" encoding="UTF-8"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
     <modelVersion>4.0.0</modelVersion>
-
     <parent>
         <groupId>org.springframework.boot</groupId>
         <artifactId>spring-boot-starter-parent</artifactId>
@@ -114,6 +113,13 @@
                 <artifactId>mime-types</artifactId>
                 <version>1.0.2</version>
             </dependency>
+
+            <dependency>
+                <groupId>junit</groupId>
+                <artifactId>junit</artifactId>
+                <version>4.12</version>
+                <scope>test</scope>
+            </dependency>
         </dependencies>
     </dependencyManagement>
 
@@ -193,5 +199,4 @@
             </plugin>
         </plugins>
     </build>
-
 </project>