diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/rest/files/FileController.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/rest/files/FileController.kt index 3dff79b..9e5125b 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/rest/files/FileController.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/rest/files/FileController.kt @@ -12,6 +12,7 @@ import org.springframework.web.multipart.MultipartFile import java.net.URI const val FILE_CONTROLLER_PATH = "/api/file" +private const val DEFAULT_MEDIA_TYPE = MediaType.APPLICATION_OCTET_STREAM_VALUE @RestController @RequestMapping(FILE_CONTROLLER_PATH) @@ -21,12 +22,15 @@ class FileController( ) { @GetMapping(produces = [MediaType.APPLICATION_OCTET_STREAM_VALUE]) @PreAuthorize("hasAnyAuthority('READ_FILE')") - fun upload(@RequestParam path: String): ResponseEntity { + fun upload( + @RequestParam path: String, + @RequestParam(required = false) mediaType: String? + ): ResponseEntity { val file = fileService.read(path) return ResponseEntity.ok() - .header("Content-Disposition", "attachment; filename=${getFileNameFromPath(path)}") + .header("Content-Disposition", "filename=${getFileNameFromPath(path)}") .contentLength(file.contentLength()) - .contentType(MediaType.APPLICATION_OCTET_STREAM) + .contentType(MediaType.parseMediaType(mediaType ?: DEFAULT_MEDIA_TYPE)) .body(file) } diff --git a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt index 8fa6d3d..4600a8a 100644 --- a/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt +++ b/src/main/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileService.kt @@ -11,6 +11,13 @@ import java.io.File import java.io.IOException import java.nio.file.Files +/** Banned path shards. These are banned because they can allow access to files outside the data directory. */ +val BANNED_FILE_PATH_SHARDS = setOf( + "~", + "..", + "//" +) + interface FileService { /** Checks if the file at the given [path] exists. */ fun exists(path: String): Boolean @@ -93,8 +100,13 @@ class FileServiceImpl( } } - override fun String.fullPath() = - FilePath("${creProperties.workingDirectory}/$this") + override fun String.fullPath(): FilePath { + BANNED_FILE_PATH_SHARDS + .firstOrNull { this.contains(it) } + ?.let { throw InvalidFilePathException(this, it) } + + return FilePath("${creProperties.workingDirectory}/$this") + } /** Runs the given [block] in the context of a file with the given [fullPath]. */ private fun withFileAt(fullPath: FilePath, block: File.() -> T) = @@ -114,6 +126,18 @@ fun File.create() { private const val FILE_IO_EXCEPTION_TITLE = "File IO error" +class InvalidFilePathException(val path: String, val fragment: String) : + RestException( + "invalid-filepath", + "Invalid file path", + HttpStatus.BAD_REQUEST, + "The given path is invalid because it contains a potentially malicious String '$fragment'", + mapOf( + "path" to path, + "invalidString" to fragment + ) + ) + class FileExistsException(val path: String) : RestException( "io-exists", diff --git a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt index d66f05f..5316d9b 100644 --- a/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt +++ b/src/test/kotlin/dev/fyloz/colorrecipesexplorer/service/files/FileServiceTest.kt @@ -51,14 +51,14 @@ class FileServiceTest { @Test fun `exists() returns true when the file at the given path exists and is a file`() { - fileServiceTest { + test { assertTrue { fileService.exists(mockFilePath) } } } @Test fun `exists() returns false when the file at the given path does not exist`() { - fileServiceTest { + test { every { mockFile.exists() } returns false assertFalse { fileService.exists(mockFilePath) } @@ -67,7 +67,7 @@ class FileServiceTest { @Test fun `exists() returns false when the file at the given path is not a file`() { - fileServiceTest { + test { every { mockFile.isFile } returns false assertFalse { fileService.exists(mockFilePath) } @@ -78,7 +78,7 @@ class FileServiceTest { @Test fun `read() returns a valid ByteArrayResource`() { - fileServiceTest { + test { whenMockFilePathExists { mockkStatic(File::readBytes) every { mockFile.readBytes() } returns mockFileData @@ -92,7 +92,7 @@ class FileServiceTest { @Test fun `read() throws FileNotFoundException when no file exists at the given path`() { - fileServiceTest { + test { whenMockFilePathExists(false) { with(assertThrows { fileService.read(mockFilePath) }) { assertEquals(mockFilePath, this.path) @@ -103,7 +103,7 @@ class FileServiceTest { @Test fun `read() throws FileReadException when an IOException is thrown`() { - fileServiceTest { + test { whenMockFilePathExists { mockkStatic(File::readBytes) every { mockFile.readBytes() } throws IOException() @@ -119,7 +119,7 @@ class FileServiceTest { @Test fun `create() creates a file at the given path`() { - fileServiceTest { + test { whenMockFilePathExists(false) { mockkStatic(File::create) every { mockFile.create() } just Runs @@ -135,7 +135,7 @@ class FileServiceTest { @Test fun `create() does nothing when a file already exists at the given path`() { - fileServiceTest { + test { whenMockFilePathExists { fileService.create(mockFilePath) @@ -148,7 +148,7 @@ class FileServiceTest { @Test fun `create() throws FileCreateException when the file creation throws an IOException`() { - fileServiceTest { + test { whenMockFilePathExists(false) { mockkStatic(File::create) every { mockFile.create() } throws IOException() @@ -164,7 +164,7 @@ class FileServiceTest { @Test fun `write() creates and writes the given MultipartFile to the file at the given path`() { - fileServiceTest { + test { whenMockFilePathExists(false) { every { fileService.create(mockFilePath) } just Runs every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs @@ -181,7 +181,7 @@ class FileServiceTest { @Test fun `write() throws FileExistsException when a file at the given path already exists and overwrite is disabled`() { - fileServiceTest { + test { whenMockFilePathExists { with(assertThrows { fileService.write(mockMultipartFile, mockFilePath, false) }) { assertEquals(mockFilePath, this.path) @@ -192,7 +192,7 @@ class FileServiceTest { @Test fun `write() writes the given MultipartFile to an existing file when overwrite is enabled`() { - fileServiceTest { + test { whenMockFilePathExists { every { mockMultipartFile.transferTo(mockFilePathPath) } just Runs @@ -207,7 +207,7 @@ class FileServiceTest { @Test fun `write() throws FileWriteException when writing the given file throws an IOException`() { - fileServiceTest { + test { whenMockFilePathExists(false) { every { fileService.create(mockFilePath) } just Runs every { mockMultipartFile.transferTo(mockFilePathPath) } throws IOException() @@ -225,7 +225,7 @@ class FileServiceTest { @Test fun `delete() deletes the file at the given path`() { - fileServiceTest { + test { whenMockFilePathExists { every { mockFile.delete() } returns true @@ -236,7 +236,7 @@ class FileServiceTest { @Test fun `delete() throws FileNotFoundException when no file exists at the given path`() { - fileServiceTest { + test { whenMockFilePathExists(false) { with(assertThrows { fileService.delete(mockFilePath) }) { assertEquals(mockFilePath, this.path) @@ -247,7 +247,7 @@ class FileServiceTest { @Test fun `delete() throws FileDeleteException when deleting throw and IOException`() { - fileServiceTest { + test { whenMockFilePathExists { every { mockFile.delete() } throws IOException() @@ -262,7 +262,7 @@ class FileServiceTest { @Test fun `fullPath() appends the given path to the given working directory`() { - fileServiceTest { + test { with(fileService) { val fullFilePath = mockFilePath.fullPath() @@ -271,7 +271,23 @@ class FileServiceTest { } } - private fun fileServiceTest(test: FileServiceTestContext.() -> Unit) { + @Test + fun `fullPath() throws InvalidFilePathException when the given path contains invalid fragments`() { + test { + with(fileService) { + BANNED_FILE_PATH_SHARDS.forEach { + val maliciousPath = "$it/$mockFilePath" + + with(assertThrows { maliciousPath.fullPath() }) { + assertEquals(maliciousPath, this.path) + assertEquals(it, this.fragment) + } + } + } + } + } + + private fun test(test: FileServiceTestContext.() -> Unit) { FileServiceTestContext().test() }