-
Notifications
You must be signed in to change notification settings - Fork 6
Add security fix for unzip process #58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ repositories { | |
| } | ||
|
|
||
| ext { | ||
| rlibVersion = "10.0.alpha11" | ||
| rlibVersion = "10.0.alpha12" | ||
| } | ||
|
|
||
| dependencies { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| rootProject.version = "10.0.alpha11" | ||
| rootProject.version = "10.0.alpha12" | ||
| group = 'javasabr.rlib' | ||
|
|
||
| allprojects { | ||
|
|
||
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -400,36 +400,37 @@ public static String getFirstFreeName(Path directory, Path file) { | |||||||||||
| * | ||||||||||||
| * @param destination the destination folder. | ||||||||||||
| * @param zipFile the zip file. | ||||||||||||
| * | ||||||||||||
| * @return the count of unpacked files | ||||||||||||
| */ | ||||||||||||
| public static void unzip(Path destination, Path zipFile) { | ||||||||||||
|
|
||||||||||||
| public static int unzip(Path destination, Path zipFile) { | ||||||||||||
| if (!Files.exists(destination)) { | ||||||||||||
JavaSaBr marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||
| throw new IllegalArgumentException("The folder " + destination + " doesn't exist."); | ||||||||||||
| } | ||||||||||||
|
||||||||||||
| } | |
| } | |
| if (!Files.isDirectory(destination)) { | |
| throw new IllegalArgumentException("The path " + destination + " is not a directory."); | |
| } |
JavaSaBr marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Feb 6, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The zip-slip protection based on Path.normalize()+startsWith() does not protect against traversal via pre-existing symlinks inside the destination (e.g., destination contains a symlinked subdir pointing outside). To make this a robust security fix, validate against destination.toRealPath(NOFOLLOW_LINKS) and ensure the resolved target’s real parent path stays within the destination before writing.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,13 @@ | |
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.charset.StandardCharsets; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.StandardOpenOption; | ||
| import java.util.zip.ZipEntry; | ||
| import java.util.zip.ZipOutputStream; | ||
| import javasabr.rlib.io.util.FileUtils; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
|
|
@@ -74,4 +81,60 @@ void shouldCheckExistingExtension() { | |
| assertThat(FileUtils.hasExtension(path6)).isFalse(); | ||
| assertThat(FileUtils.hasExtension(path7)).isFalse(); | ||
| } | ||
|
|
||
| @Test | ||
| void shouldUnzipFileCorrectly() throws IOException { | ||
| // given: | ||
| Path zipFile = Files.createTempFile("test-archive", ".zip"); | ||
|
|
||
| try (var zout = new ZipOutputStream(Files.newOutputStream(zipFile, StandardOpenOption.CREATE))) { | ||
| zout.putNextEntry(new ZipEntry("fileA.txt")); | ||
| zout.write("test text".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| zout.putNextEntry(new ZipEntry("../fileB.txt")); | ||
| zout.write("test text 2".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| ZipEntry dirAEntry = new ZipEntry("dir_a/"); | ||
| dirAEntry.setMethod(ZipEntry.STORED); | ||
| dirAEntry.setSize(0); | ||
| dirAEntry.setCrc(0); | ||
| zout.putNextEntry(dirAEntry); | ||
|
|
||
| zout.putNextEntry(new ZipEntry("dir_a/fileC.txt")); | ||
| zout.write("test text 3".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| zout.putNextEntry(new ZipEntry("dir_a/../fileD.txt")); | ||
| zout.write("test text 4".getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| zout.putNextEntry(new ZipEntry("dir_a/../../../fileE.txt")); | ||
| zout.write("test text 5".getBytes(StandardCharsets.UTF_8)); | ||
| } | ||
|
|
||
| Path tempDirectory = Files.createTempDirectory("test-unzip"); | ||
| Path outputDir = tempDirectory | ||
| .resolve("output") | ||
| .resolve("folder"); | ||
|
|
||
| Files.createDirectories(outputDir); | ||
|
|
||
| // when: | ||
| int unpackedFiles = FileUtils.unzip(outputDir, zipFile); | ||
|
|
||
| // then: | ||
| assertThat(unpackedFiles).isEqualTo(3); | ||
JavaSaBr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| assertThat(outputDir | ||
| .resolve("fileA.txt")) | ||
| .exists(); | ||
| assertThat(outputDir | ||
| .resolve("dir_a") | ||
| .resolve("fileC.txt")) | ||
| .exists(); | ||
|
Comment on lines
+124
to
+131
|
||
| assertThat(tempDirectory | ||
| .resolve("output") | ||
| .resolve("fileB.txt")) | ||
| .doesNotExist(); | ||
| assertThat(tempDirectory | ||
| .resolve("fileE.txt")) | ||
| .doesNotExist(); | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.