fix #256

CHANGES.md

@@ -41,6 +41,7 @@ better-files follows the following `MAJOR.MINOR.PATCH` release conventions:
     ```
 * (_Planned_) [Issue #88](https://github.com/pathikrit/better-files/issues/88): Path ASTs for relative vs. absolute path
 * [Issue #593](https://github.com/pathikrit/better-files/pull/598): Remove compilation warnings for Scala 2.13 and Scala 3
+* [Issue #624](https://github.com/pathikrit/better-files/pull/633): Fix zip-slip vulnerability
 * (_Planned_) [Issue #590](https://github.com/pathikrit/better-files/issues/590): `file.walk()` can handle errors
 * (_Planned_) [Issue #591](https://github.com/pathikrit/better-files/issues/591): New APIs
 * (_Planned_) [Issue #3](https://github.com/pathikrit/better-files/issues/3): Walk File Tree APIs

src/main/scala/better/files/File.scala

@@ -1074,28 +1074,6 @@ class File private (val path: Path)(implicit val fileSystem: FileSystem = path.g
   def zip(compressionLevel: Int = Deflater.DEFAULT_COMPRESSION, charset: Charset = DefaultCharset): File =
     zipTo(destination = File.newTemporaryFile(prefix = name, suffix = ".zip"), compressionLevel, charset)
 
-  /**
-    * Skips unzipping a ZipEntry from a ZipFile if the entry is going outside the target directory
-    *
-    * @param destination destination folder to extract zipEntry to
-    * @param zipEntry Instantiated ZipEntry to unzip
-    * @param zipFile Instantiated ZipFile to unzip from
-    */
-  private def safeExtractTo(
-      destination: File,
-      zipEntry: ZipEntry,
-      zipFile: ZipFile
-  ): destination.type = {
-    // https://developer.android.com/topic/security/risks/zip-path-traversal
-    val entryName          = zipEntry.getName()
-    val entryDestination   = File(destination, entryName)
-    val entryCanonicalPath = entryDestination.canonicalPath
-    if (entryCanonicalPath.startsWith(destination.canonicalPath + JFile.separator)) {
-      zipEntry.extractTo(destination, zipFile.getInputStream(zipEntry))
-    }
-    destination
-  }
-
   /** Unzips this zip file
     *
     * @param destination destination folder; Creates this if it does not exist
@@ -1113,8 +1091,10 @@ class File private (val path: Path)(implicit val fileSystem: FileSystem = path.g
     for {
       zipFile <- new ZipFile(toJava, charset).autoClosed
       entry   <- zipFile.entries().asScala if zipFilter(entry)
-    } if (safeUnzip) safeExtractTo(destination, entry, zipFile)
-    else entry.extractTo(destination, zipFile.getInputStream(entry))
+    } if (safeUnzip && File(destination, entry.getName).canonicalPath.startsWith(destination.canonicalPath + JFile.separator)) {
+      // https://developer.android.com/topic/security/risks/zip-path-traversal
+      entry.extractTo(destination, zipFile.getInputStream(entry))
+    } else entry.extractTo(destination, zipFile.getInputStream(entry))
     destination
   }
 

src/test/scala/better/files/FileSpec.scala

@@ -539,27 +539,25 @@ class FileSpec extends CommonSpec {
       .unzipTo()
       .listRecursively()
       .toList
-    assert(
-      list.length === 1
-    ) // Three total entries in the zip file: 1 without directory traversal characters and 2 with. Default secure unzip should only unzip the entry without directory traversal characters.
+    // Three total entries in the zip file: 1 without directory traversal characters and 2 with. Default secure unzip should only unzip the entry without directory traversal characters.
+    assert(list.length === 1)
   }
 
   it should "unzip unsafely when safe unzip is disabled" in {
+    assume(isLinux) // See: https://github.com/pathikrit/better-files/pull/633#issuecomment-1741292532
     // Unsafe unzip with a zipslip attack may result in OS access denied exceptions. If these occur, the test should still pass.
+    var list = List.empty[File]
     try {
-      val list = File("src/test/resources/better/files/issue-624.zip")
+      list = File("src/test/resources/better/files/issue-624.zip")
         .unzipTo(safeUnzip = false)
         .listRecursively()
         .toList
-      assert(
-        list.length === 3
-      ) // Three total entries in the zip file: 1 without directory traversal characters and 2 with. Unsafe unzip should try to extract all entries, might result in access denied exception from OS.
     } catch {
-      case exception: Throwable =>
-        assert(
-          exception.isInstanceOf[AccessDeniedException]
-        )
+      // Unsafe unzip should try to extract all entries, might result in access denied exception from OS.
+      case exception: Throwable => assert(exception.isInstanceOf[AccessDeniedException])
     }
+    // Three total entries in the zip file: 1 without directory traversal characters and 2 with.
+    assert(list.length === 3)
   }
 
   it should "ungzip" in {