diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 3a28ed5c..321f671b 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -38,11 +38,11 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 # Initializes the CodeQL tools for scanning. - name: Initialize CodeQL - uses: github/codeql-action/init@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4.36.2 + uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6 with: languages: ${{ matrix.language }} # If you wish to specify custom queries, you can do so here or in a config file. @@ -56,7 +56,7 @@ jobs: # Autobuild attempts to build any compiled languages (C/C++, C#, or Java). # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild - uses: github/codeql-action/autobuild@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4.36.2 + uses: github/codeql-action/autobuild@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6 # Command-line programs to run using the OS shell. # See https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsrun @@ -69,4 +69,4 @@ jobs: # ./location_of_script_within_repo/buildscript.sh - name: Perform CodeQL Analysis - uses: github/codeql-action/analyze@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4.36.2 + uses: github/codeql-action/analyze@5595ccaf912efad79be6eef63a5619ff05969be3 # v4.37.6 diff --git a/.github/workflows/gradle.yml b/.github/workflows/gradle.yml index b8474831..3c449acf 100644 --- a/.github/workflows/gradle.yml +++ b/.github/workflows/gradle.yml @@ -22,16 +22,16 @@ jobs: runs-on: ${{ matrix.os }} steps: - - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Set up openJDK version - uses: actions/setup-java@ad2b38190b15e4d6bdf0c97fb4fca8412226d287 # v5.3.0 + uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 with: java-version: ${{ matrix.jdk }} distribution: "zulu" - name: Install Dependencies run: npm install -g ro-crate-html-js - name: Setup Gradle - uses: gradle/actions/setup-gradle@3f131e8634966bd73d06cc69884922b02e6faf92 # v6.2.0 + uses: gradle/actions/setup-gradle@9c971963bec38e04b3d30dcc455b5382be2fdbfb # v6.3.0 - name: Build and Test with Gradle run: ./gradlew -Dprofile=release build - name: Upload (test) reports as artifact on GitHub on manual runs @@ -47,9 +47,9 @@ jobs: jdkversion: 25 steps: - name: Checkout repo - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Set up OpenJDK version ... - uses: actions/setup-java@ad2b38190b15e4d6bdf0c97fb4fca8412226d287 # v5.3.0 + uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 with: distribution: "zulu" java-version: ${{ env.jdkversion }} diff --git a/.github/workflows/publishRelease.yml b/.github/workflows/publishRelease.yml index 1508010f..2bf14be3 100644 --- a/.github/workflows/publishRelease.yml +++ b/.github/workflows/publishRelease.yml @@ -8,9 +8,9 @@ jobs: publish: runs-on: ubuntu-latest steps: - - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - name: Set up Java - uses: actions/setup-java@ad2b38190b15e4d6bdf0c97fb4fca8412226d287 # v5.3.0 + uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 with: java-version: 21 distribution: 'zulu' # openjdk diff --git a/build.gradle b/build.gradle index f3faa49a..0eb49487 100644 --- a/build.gradle +++ b/build.gradle @@ -38,12 +38,12 @@ repositories { } ext { - jacksonVersion = '2.22.0' + jacksonVersion = '2.22.1' } dependencies { // JUnit setup for testing - testImplementation(platform("org.junit:junit-bom:6.1.0")) + testImplementation(platform("org.junit:junit-bom:6.1.3")) testImplementation('org.junit.jupiter:junit-jupiter') testRuntimeOnly('org.junit.platform:junit-platform-launcher') // JSON object mapping / (de-)serialization @@ -58,7 +58,7 @@ dependencies { // compare json documents in tests implementation 'com.github.fslev:json-compare:8.1' // url validator - implementation group: 'commons-validator', name: 'commons-validator', version: '1.10.1' + implementation group: 'commons-validator', name: 'commons-validator', version: '1.11.0' // logging implementation group: 'org.slf4j', name: 'slf4j-jdk14', version: '2.0.18' // JSON-LD, Zenodo mapping diff --git a/src/main/java/edu/kit/datamanager/ro_crate/entities/data/DataEntity.java b/src/main/java/edu/kit/datamanager/ro_crate/entities/data/DataEntity.java index 864d3044..829ee9c2 100644 --- a/src/main/java/edu/kit/datamanager/ro_crate/entities/data/DataEntity.java +++ b/src/main/java/edu/kit/datamanager/ro_crate/entities/data/DataEntity.java @@ -48,6 +48,16 @@ public void addAuthorId(String id) { this.addIdProperty("author", id); } + /** + * Gets the path of this entity. + *

+ * When reading a crate, this will point to the entity's file within the crate. + * When creating a new entity or crate, it might point to a file outside the crate, + * as set by {@link DataEntityBuilder#setLocation(Path)}. + * Such a file will be copied into the crate when writing. + * + * @return the path to the file this entity represents. + */ @JsonIgnore public Path getPath() { return path; diff --git a/src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java b/src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java index 15cae146..c63ba21a 100644 --- a/src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java +++ b/src/main/java/edu/kit/datamanager/ro_crate/reader/CrateReader.java @@ -22,6 +22,7 @@ import java.nio.file.Path; import java.util.*; import java.util.stream.Collectors; +import java.util.stream.Stream; import java.util.stream.StreamSupport; /** @@ -240,12 +241,13 @@ protected Optional checkFolderHasFile(String filepathOrId, File folder) { if (IdentifierUtils.isUrl(filepathOrId)) { return Optional.empty(); } - return IdentifierUtils.decode(filepathOrId) - .map(decoded -> folder.toPath().resolve(decoded).normalize()) + return Stream.of(IdentifierUtils.decode(filepathOrId).orElse(filepathOrId), filepathOrId) + .map(filename -> folder.toPath().resolve(filename).normalize().toAbsolutePath()) // defence-in-depth: ensure we are still inside the crate folder .filter(resolved -> resolved.startsWith(folder.toPath())) .map(Path::toFile) - .filter(File::exists); + .filter(File::exists) + .findFirst(); } /** diff --git a/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteFolderStrategy.java b/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteFolderStrategy.java index 8a4844ed..0816cd59 100644 --- a/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteFolderStrategy.java +++ b/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteFolderStrategy.java @@ -5,6 +5,7 @@ import edu.kit.datamanager.ro_crate.Crate; import edu.kit.datamanager.ro_crate.entities.data.DataEntity; import edu.kit.datamanager.ro_crate.objectmapper.MyObjectMapper; +import edu.kit.datamanager.ro_crate.special.IdentifierUtils; import org.apache.commons.io.FileUtils; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -14,6 +15,7 @@ import java.io.IOException; import java.io.InputStream; import java.nio.charset.StandardCharsets; +import java.nio.file.Path; /** * A class for writing a crate to a folder. @@ -64,16 +66,25 @@ public void save(Crate crate, String destination) throws IOException { } } for (DataEntity dataEntity : crate.getAllDataEntities()) { - savetoFile(dataEntity, file); + saveToFile(dataEntity, file); } } - private void savetoFile(DataEntity entity, File file) throws IOException { + private void saveToFile(DataEntity entity, File file) throws IOException { if (entity.getPath() != null) { + String id = entity.getId(); + String filename = IdentifierUtils.decode(id).orElse(id); + Path baseFolder = file.toPath().toAbsolutePath().normalize(); + Path destination = baseFolder.resolve(filename).normalize(); + // defence-in-depth: ensure the resolved path remains inside the crate folder + if (!destination.startsWith(baseFolder)) { + logger.warn("Skipping entity '{}': resolved path '{}' escapes destination folder '{}'", id, destination, file); + return; + } if (entity.getPath().toFile().isDirectory()) { - FileUtils.copyDirectory(entity.getPath().toFile(), file.toPath().resolve(entity.getId()).toFile()); + FileUtils.copyDirectory(entity.getPath().toFile(), destination.toFile()); } else { - FileUtils.copyFile(entity.getPath().toFile(), file.toPath().resolve(entity.getId()).toFile()); + FileUtils.copyFile(entity.getPath().toFile(), destination.toFile()); } } } diff --git a/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteZipStreamStrategy.java b/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteZipStreamStrategy.java index 33572db2..8dfb6adf 100644 --- a/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteZipStreamStrategy.java +++ b/src/main/java/edu/kit/datamanager/ro_crate/writer/WriteZipStreamStrategy.java @@ -10,11 +10,14 @@ import java.io.*; import java.nio.charset.StandardCharsets; import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; import java.util.Optional; import java.util.Set; import java.util.UUID; import edu.kit.datamanager.ro_crate.preview.CratePreview; +import edu.kit.datamanager.ro_crate.special.IdentifierUtils; import edu.kit.datamanager.ro_crate.util.FileSystemUtil; import edu.kit.datamanager.ro_crate.util.ZipStreamUtil; import net.lingala.zip4j.io.outputstream.ZipOutputStream; @@ -154,16 +157,67 @@ private void saveToStream(DataEntity entity, ZipOutputStream zipStream, String p } boolean isDirectory = entity.getPath().toFile().isDirectory(); + String id = entity.getId(); + String filename = IdentifierUtils.decode(id).orElse(id); + String safeName = sanitizeZipEntryName(filename); + if (safeName.isEmpty()) { + logger.warn("Skipping entity '{}': decoded name resolves outside the crate root", id); + return; + } + String entryName = prefix + safeName; if (isDirectory) { ZipStreamUtil.addFolderToZipStream( zipStream, entity.getPath().toFile(), - prefix + entity.getId()); + entryName); } else { ZipStreamUtil.addFileToZipStream( zipStream, entity.getPath().toFile(), - prefix + entity.getId()); + entryName); + } + } + + /** + * Normalizes a decoded entry name into a safe relative path, consistent + * with the containment check in {@link WriteFolderStrategy#saveToFile}. + *

+ * Absolute paths (leading {@code /}) and Windows drive-qualified paths + * (e.g. {@code C:/}) are rejected. Internal {@code .} and {@code ..} + * segments are resolved; if the result escapes the virtual crate root + * the name is rejected. + * + * @param name the raw decoded entry name + * @return a canonical relative entry name, or an empty string if the name + * is absolute, drive-qualified, or escapes the crate root + */ + private static String sanitizeZipEntryName(String name) { + // zip entries always use forward slashes as separators + String normalized = name.replace('\\', '/'); + + // Reject absolute paths and Windows drive-qualified paths (e.g. C:/) + if (normalized.startsWith("/") + || (normalized.length() >= 2 && normalized.charAt(1) == ':' + && Character.isLetter(normalized.charAt(0)))) { + return ""; + } + + // Normalize "." and ".." segments, rejecting paths that escape the root + String[] segments = normalized.split("/"); + List stack = new ArrayList<>(); + for (String segment : segments) { + if (segment.isEmpty() || segment.equals(".")) { + continue; + } + if (segment.equals("..")) { + if (stack.isEmpty()) { + return ""; + } + stack.remove(stack.size() - 1); + } else { + stack.add(segment); + } } + return String.join("/", stack); } } diff --git a/src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java b/src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java index ca742ded..3475d185 100644 --- a/src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java +++ b/src/test/java/edu/kit/datamanager/ro_crate/crate/ReadAndWriteTest.java @@ -1,42 +1,61 @@ package edu.kit.datamanager.ro_crate.crate; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + import edu.kit.datamanager.ro_crate.Crate; import edu.kit.datamanager.ro_crate.HelpFunctions; import edu.kit.datamanager.ro_crate.RoCrate; +import edu.kit.datamanager.ro_crate.entities.data.DataEntity; +import edu.kit.datamanager.ro_crate.entities.data.FileEntity; import edu.kit.datamanager.ro_crate.preview.StaticPreview; import edu.kit.datamanager.ro_crate.reader.CrateReader; import edu.kit.datamanager.ro_crate.reader.Readers; - +import edu.kit.datamanager.ro_crate.special.IdentifierUtils; +import edu.kit.datamanager.ro_crate.writer.CrateWriter; import edu.kit.datamanager.ro_crate.writer.Writers; -import org.apache.commons.io.FileUtils; -import org.junit.jupiter.api.Test; -import org.junit.jupiter.api.io.TempDir; - import java.io.IOException; import java.nio.charset.Charset; +import java.nio.file.Files; import java.nio.file.Path; - -import static org.junit.jupiter.api.Assertions.assertEquals; - +import java.util.List; +import java.util.UUID; +import org.apache.commons.io.FileUtils; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; class ReadAndWriteTest { @Test void testReadingAndWriting(@TempDir Path path) throws IOException { Path htmlFile = path.resolve("htmlFile.html"); - FileUtils.writeStringToFile(htmlFile.toFile(), "useful file", Charset.defaultCharset()); + FileUtils.writeStringToFile( + htmlFile.toFile(), + "useful file", + Charset.defaultCharset() + ); Path htmlDir = path.resolve("dir"); Path fileInDir = htmlDir.resolve("file.html"); - FileUtils.writeStringToFile(fileInDir.toFile(), "fileN2", Charset.defaultCharset()); + FileUtils.writeStringToFile( + fileInDir.toFile(), + "fileN2", + Charset.defaultCharset() + ); - RoCrate crate = new RoCrate.RoCrateBuilder("name", "description", "2024", "https://creativecommons.org/licenses/by-nc-sa/3.0/au/") - .setPreview(new StaticPreview(htmlFile.toFile(), htmlDir.toFile())) - .build(); + RoCrate crate = new RoCrate.RoCrateBuilder( + "name", + "description", + "2024", + "https://creativecommons.org/licenses/by-nc-sa/3.0/au/" + ) + .setPreview(new StaticPreview(htmlFile.toFile(), htmlDir.toFile())) + .build(); Path writeDir = path.resolve("crate"); - Writers.newFolderWriter() - .save(crate, writeDir.toAbsolutePath().toString()); + Writers.newFolderWriter().save(crate, writeDir.toAbsolutePath().toString()); CrateReader reader = Readers.newFolderReader(); Crate newCrate = reader.readCrate(writeDir.toAbsolutePath().toString()); @@ -51,8 +70,215 @@ void testReadingAndWriting(@TempDir Path path) throws IOException { @Test void testReadCrateWithHasPartHierarchy() throws IOException { CrateReader reader = Readers.newFolderReader(); - RoCrate crate = reader.readCrate(ReadAndWriteTest.class.getResource("/crates/hasPartHierarchy").getPath()); + RoCrate crate = reader.readCrate( + ReadAndWriteTest.class.getResource("/crates/hasPartHierarchy").getPath() + ); assertEquals(1, crate.getAllContextualEntities().size()); assertEquals(6, crate.getAllDataEntities().size()); } + + @Test + void testEncodedIdsFindTheirPaths(@TempDir Path tempDir) throws IOException { + RoCrate.RoCrateBuilder builder = new RoCrate.RoCrateBuilder(); + { + FileEntity.FileEntityBuilder dataEntityBuilder = + new FileEntity.FileEntityBuilder(); + dataEntityBuilder.setId("id 1"); + dataEntityBuilder.addTypes(List.of("File")); + UUID uuid = UUID.randomUUID(); + Path path = tempDir.resolve(uuid.toString()); + Files.writeString(path, "File"); + dataEntityBuilder.setLocation(path); + + builder.addDataEntity(dataEntityBuilder.build()); + } + { + FileEntity.FileEntityBuilder dataEntityBuilder = + new FileEntity.FileEntityBuilder(); + dataEntityBuilder.setId("id\uD83E\uDD791"); + dataEntityBuilder.addTypes(List.of("File")); + UUID uuid = UUID.randomUUID(); + Path path = tempDir.resolve(uuid.toString()); + Files.writeString(path, "File"); + dataEntityBuilder.setLocation(path); + + builder.addDataEntity(dataEntityBuilder.build()); + } + { + FileEntity.FileEntityBuilder dataEntityBuilder = + new FileEntity.FileEntityBuilder(); + dataEntityBuilder.setId("id面试1"); + dataEntityBuilder.addTypes(List.of("File")); + UUID uuid = UUID.randomUUID(); + Path path = tempDir.resolve(uuid.toString()); + Files.writeString(path, "File"); + dataEntityBuilder.setLocation(path); + + builder.addDataEntity(dataEntityBuilder.build()); + } + Path location = tempDir.resolve("out"); + { + RoCrate crate = builder.build(); + CrateWriter writer = Writers.newFolderWriter(); + writer.save(crate, location.toString()); + } + { + CrateReader roCrateReader = Readers.newFolderReader(); + + RoCrate roCrate = roCrateReader.readCrate( + location.toString() + ); + for (DataEntity dataEntity : roCrate.getAllDataEntities()) { + System.out.println(dataEntity.getId() + ": " + dataEntity.getPath()); + } + for (DataEntity dataEntity : roCrate.getAllDataEntities()) { + assertNotNull( + dataEntity.getPath(), + "Path of ID: " + dataEntity.getId() + ); + } + } + } + + /** + * Test we detect files which use the encoded IDs as filename, + * as well as ones which use the decoded filename. + */ + @Test + void testDetectingEncodedFileNames(@TempDir Path tempDir) throws IOException { + // This is how we add the id. But the space will be encoded + String id = "id 42"; + // This is how we get it out (the encoded id as it will exist in the crate) + String idEncoded = IdentifierUtils.encode(id).orElseThrow(); + + RoCrate.RoCrateBuilder builder = new RoCrate.RoCrateBuilder(); + + { + // Add file entity without a file with id + FileEntity.FileEntityBuilder fileEntityBuilder = + new FileEntity.FileEntityBuilder(); + fileEntityBuilder.setId(id); + fileEntityBuilder.addTypes(List.of("File")); + + builder.addDataEntity(fileEntityBuilder.build()); + } + + Path cratepath1 = tempDir.resolve("test1"); + { + Writers.newFolderWriter().save(builder.build(), cratepath1.toString()); + // add file manually (decoded id) + Path filepath = cratepath1.resolve(id); + Files.writeString(filepath, "File"); + } + + Path cratepath2 = tempDir.resolve("test2"); + { + Writers.newFolderWriter().save(builder.build(), cratepath2.toString()); + // add file manually (encoded id) + Path filepath = cratepath2.resolve(idEncoded); + Files.writeString(filepath, "File"); + } + + { + RoCrate crate = Readers.newFolderReader().readCrate( + cratepath1.toString() + ); + + DataEntity entity = crate.getDataEntityById(idEncoded); + assertEquals(idEncoded, entity.getId()); + + Path filepath = entity.getPath(); + assertNotNull(filepath); + assertTrue(Files.exists(filepath)); + assertEquals(id, filepath.getFileName().toString()); + } + + { + RoCrate crate = Readers.newFolderReader().readCrate( + cratepath2.toString() + ); + + DataEntity entity = crate.getDataEntityById(idEncoded); + assertEquals(idEncoded, entity.getId()); + + Path filepath = entity.getPath(); + assertNotNull(filepath); + assertTrue(Files.exists(filepath)); + assertEquals(idEncoded, filepath.getFileName().toString()); + } + } + + @Test + void testFilenamesAreSelfHealing(@TempDir Path tempDir) throws IOException { + // This is how we add the id. But the space will be encoded + String id = "id 42"; + // This is how we get it out (the encoded id as it will exist in the crate) + String idEncoded = IdentifierUtils.encode(id).orElseThrow(); + + RoCrate.RoCrateBuilder builder = new RoCrate.RoCrateBuilder(); + + // add dummy file + Path filepath_outside = tempDir.resolve("someFile.txt"); + Files.writeString(filepath_outside, "File"); + + { + // Add file entity without a file with id + FileEntity.FileEntityBuilder fileEntityBuilder = + new FileEntity.FileEntityBuilder(); + fileEntityBuilder.setId(id); + fileEntityBuilder.addTypes(List.of("File")); + fileEntityBuilder.setLocation(filepath_outside); + + builder.addDataEntity(fileEntityBuilder.build()); + } + + Path cratepath = tempDir.resolve("test1"); + { + Writers.newFolderWriter().save(builder.build(), cratepath.toString()); + Path currentFilePath = cratepath.resolve(id); + assertTrue(currentFilePath.toFile().exists()); + // swap file names + Path newFilePath = cratepath.resolve(idEncoded); + Files.move(currentFilePath, newFilePath); + assertFalse(currentFilePath.toFile().exists()); + assertTrue(newFilePath.toFile().exists()); + } + + { + RoCrate crate = Readers.newFolderReader().readCrate( + cratepath.toString() + ); + + DataEntity entity = crate.getDataEntityById(idEncoded); + assertEquals(idEncoded, entity.getId()); + + // Even if a file's name is encoded, the path will work as expected + Path filepath = entity.getPath(); + assertNotNull(filepath); + assertTrue(Files.exists(filepath)); + assertEquals(idEncoded, filepath.getFileName().toString()); + } + + // When saving the crate again, the file will be renamed to the decoded id + Path cratepath2 = tempDir.resolve("test2"); + { + Writers.newFolderWriter().save(builder.build(), cratepath2.toString()); + } + + { + RoCrate crate = Readers.newFolderReader().readCrate( + cratepath2.toString() + ); + + DataEntity entity = crate.getDataEntityById(idEncoded); + assertEquals(idEncoded, entity.getId()); + + // The filename is now decoded to the original id + Path filepath = entity.getPath(); + assertNotNull(filepath); + assertTrue(Files.exists(filepath)); + assertEquals(id, filepath.getFileName().toString()); + } + + } } diff --git a/src/test/java/edu/kit/datamanager/ro_crate/writer/CommonWriterTest.java b/src/test/java/edu/kit/datamanager/ro_crate/writer/CommonWriterTest.java index 0eae96c2..15f38df7 100644 --- a/src/test/java/edu/kit/datamanager/ro_crate/writer/CommonWriterTest.java +++ b/src/test/java/edu/kit/datamanager/ro_crate/writer/CommonWriterTest.java @@ -4,6 +4,7 @@ import edu.kit.datamanager.ro_crate.RoCrate; import edu.kit.datamanager.ro_crate.entities.data.DataSetEntity; +import edu.kit.datamanager.ro_crate.entities.data.FileEntity; import org.apache.commons.io.FileUtils; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -34,6 +35,7 @@ default void testFilesBeingAdjusted(@TempDir Path tempDir) throws IOException { Path writtenCrate = tempDir.resolve("written-crate"); Path extractionPath = tempDir.resolve("checkMe"); + String id = "id will be encoded"; { RoCrate builtCrate = getCrateWithFileAndDir(pathToFile, pathToDir) .addDataEntity(new DataSetEntity.DataSetBuilder() @@ -43,6 +45,11 @@ default void testFilesBeingAdjusted(@TempDir Path tempDir) throws IOException { .setId("lots_of_little_files/subdir-renamed/") .build() ) + .addDataEntity(new FileEntity.FileEntityBuilder() + .setId(id) + .setLocation(pathToFile) + .build() + ) .build(); this.saveCrate(builtCrate, writtenCrate); ensureCrateIsExtractedIn(writtenCrate, extractionPath); @@ -51,6 +58,12 @@ default void testFilesBeingAdjusted(@TempDir Path tempDir) throws IOException { HelpFunctions.printFileTree(correctCrate); HelpFunctions.printFileTree(extractionPath); + // Ensure the file uses the id, not the encoded id as a file name + assertTrue( + Files.exists(extractionPath.resolve(id)), + "The file '%s' should exist, because this is the ID of the entity".formatted(id) + ); + // The actual file name should **not** appear in the crate String fileName = pathToFile.getFileName().toString(); assertFalse( @@ -172,4 +185,116 @@ default void testWritingOnlyConsidersAddedFiles(@TempDir Path tempDir) throws IO roCrate, "/json/crate/fileAndDir.json"); } + + /** + * Tests that data entity ids containing path traversal segments (e.g. "../") + * cannot cause files to be written outside the crate destination. + * + * @param tempDir the temporary directory given by junit for our test + * @throws IOException if an error occurs while writing the crate + */ + @Test + default void testPathTraversalIsBlocked(@TempDir Path tempDir) throws IOException { + Path sourceFile = tempDir.resolve("source.txt"); + FileUtils.writeStringToFile(sourceFile.toFile(), "content", Charset.defaultCharset()); + + RoCrate crate = new RoCrate.RoCrateBuilder( + "Traversal Test", + "Crate with a traversal id", + "2024", + "https://creativecommons.org/licenses/by/4.0/") + .setPreview(new edu.kit.datamanager.ro_crate.preview.AutomaticPreview()) + .addDataEntity(new FileEntity.FileEntityBuilder() + .setLocationWithExceptions(sourceFile) + .setId("../escape.txt") + .build()) + .build(); + + Path crateDestination = tempDir.resolve("my-crate"); + this.saveCrate(crate, crateDestination); + + Path extractionPath = tempDir.resolve("extracted"); + ensureCrateIsExtractedIn(crateDestination, extractionPath); + + Path escapeTarget = tempDir.resolve("escape.txt"); + assertFalse(Files.exists(escapeTarget), + "Path traversal must be blocked: '%s' must not exist outside the crate".formatted(escapeTarget)); + } + + /** + * Tests that valid files are accepted even when the destination path itself + * contains parent segments (e.g. "../"). The containment check must compare + * normalized paths so equivalent base folders are not falsely rejected. + * + * @param tempDir the temporary directory given by junit for our test + * @throws IOException if an error occurs while writing the crate + */ + @Test + default void testValidFileAcceptedWithParentSegmentInDestination(@TempDir Path tempDir) throws IOException { + // Create a subdirectory so the ".." in the destination path resolves correctly + Files.createDirectories(tempDir.resolve("outer")); + + Path sourceFile = tempDir.resolve("source.txt"); + FileUtils.writeStringToFile(sourceFile.toFile(), "content", Charset.defaultCharset()); + + RoCrate crate = new RoCrate.RoCrateBuilder( + "Parent Segment Test", + "Crate written to a destination with a parent segment", + "2024", + "https://creativecommons.org/licenses/by/4.0/") + .setPreview(new edu.kit.datamanager.ro_crate.preview.AutomaticPreview()) + .addDataEntity(new FileEntity.FileEntityBuilder() + .setLocationWithExceptions(sourceFile) + .setId("valid.txt") + .build()) + .build(); + + // Destination contains a parent segment (..) that resolves inside tempDir + Path crateDestination = tempDir.resolve("outer").resolve("../my-crate"); + this.saveCrate(crate, crateDestination); + + Path extractionPath = tempDir.resolve("extracted"); + ensureCrateIsExtractedIn(crateDestination, extractionPath); + + assertTrue(Files.isRegularFile(extractionPath.resolve("valid.txt")), + "Valid file should be written even when destination path contains parent segments"); + } + + /** + * Tests that internal ".." segments are normalized rather than deleted. + * An entity id like "subdir/../valid.txt" should resolve to "valid.txt" + * (consistent with WriteFolderStrategy), not "subdir/valid.txt". + * + * @param tempDir the temporary directory given by junit for our test + * @throws IOException if an error occurs while writing the crate + */ + @Test + default void testInternalTraversalSegmentsAreNormalized(@TempDir Path tempDir) throws IOException { + Path sourceFile = tempDir.resolve("source.txt"); + FileUtils.writeStringToFile(sourceFile.toFile(), "content", Charset.defaultCharset()); + + RoCrate crate = new RoCrate.RoCrateBuilder( + "Normalization Test", + "Crate with an internal traversal segment in an entity id", + "2024", + "https://creativecommons.org/licenses/by/4.0/") + .setPreview(new edu.kit.datamanager.ro_crate.preview.AutomaticPreview()) + .addDataEntity(new FileEntity.FileEntityBuilder() + .setLocationWithExceptions(sourceFile) + .setId("subdir/../valid.txt") + .build()) + .build(); + + Path crateDestination = tempDir.resolve("my-crate"); + this.saveCrate(crate, crateDestination); + + Path extractionPath = tempDir.resolve("extracted"); + ensureCrateIsExtractedIn(crateDestination, extractionPath); + + // After normalization, the file should be at the root, not inside "subdir" + assertTrue(Files.isRegularFile(extractionPath.resolve("valid.txt")), + "Internal '..' should be normalized: 'subdir/../valid.txt' must resolve to 'valid.txt'"); + assertFalse(Files.exists(extractionPath.resolve("subdir")), + "The 'subdir' segment should have been cancelled by the following '..'"); + } }