Skip to content

Commit

Permalink
Merge pull request #646 from arteme/better-fix-issue-256
Browse files Browse the repository at this point in the history
Fix issue #256 logic and test
  • Loading branch information
pathikrit authored Mar 18, 2024
2 parents 0cefce9 + 2fdb050 commit faffdb8
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 5 deletions.
4 changes: 3 additions & 1 deletion src/main/scala/better/files/File.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1094,7 +1094,9 @@ class File private (val path: Path)(implicit val fileSystem: FileSystem = path.g
} 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))
} else if (!safeUnzip) {
entry.extractTo(destination, zipFile.getInputStream(entry))
}
destination
}

Expand Down
17 changes: 13 additions & 4 deletions src/test/scala/better/files/FileSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -544,19 +544,28 @@ class FileSpec extends CommonSpec {
}

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]
var destList = List.empty[File]
var list = List.empty[File]
try {
list = File("src/test/resources/better/files/issue-624.zip")
.unzipTo(safeUnzip = false)
// create the directory structure safe for issue 624 zip file
val toplevel = File.newTemporaryDirectory("issue-624")
val dest = (toplevel / "a" / "b").createDirectories()

destList = File("src/test/resources/better/files/issue-624.zip")
.unzipTo(destination = dest, safeUnzip = false)
.listRecursively()
.toList
list = toplevel
.listRecursively()
.filterNot(_.isDirectory())
.toList
} catch {
// 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(destList.length === 1)
assert(list.length === 3)
}

Expand Down

0 comments on commit faffdb8

Please sign in to comment.