diff --git a/os/src/ZipOps.scala b/os/src/ZipOps.scala index 14fad6fd..f7cb9fb2 100644 --- a/os/src/ZipOps.scala +++ b/os/src/ZipOps.scala @@ -345,12 +345,11 @@ object unzip { ) }) .toList - .sortBy { case (_, path, _, isSymLink, _) => - // Unzipping symbolic links last. - // Enclosing directories are unzipped before their contents. - // This makes sure directory permissions are applied correctly. - (isSymLink, path) - } + .sortBy { case (_, path, _, _, _) => + // Unzipping children first before the enclosing directories + // This prevents crash for the case where directories lack READ/EXECUTE permission + path + }(Ordering[os.SubPath].reverse) try { for ((zipEntry, path, mode, isSymLink, zipInputStream) <- zipEntryInputStreams) { @@ -360,10 +359,10 @@ object unzip { } else null if (zipEntry.isDirectory) { - os.makeDir.all(newFile) - if (perms != null) { - // make sure directories at least have OWNER_EXECUTE - os.perms.set(newFile, perms + PosixFilePermission.OWNER_EXECUTE) + os.makeDir.all(newFile, perms = perms) + if (perms != null && os.perms(newFile) != perms) { + // because of umask + os.perms.set(newFile, perms) } } else if (isSymLink) { val target = scala.io.Source.fromInputStream(zipInputStream).mkString diff --git a/os/test/src/ZipOpTests.scala b/os/test/src/ZipOpTests.scala index afa61d0a..21d18e68 100644 --- a/os/test/src/ZipOpTests.scala +++ b/os/test/src/ZipOpTests.scala @@ -538,24 +538,86 @@ object ZipOpTests extends TestSuite { assert(file2Content == "Content of file2") } - test("unzipDirectoryEnsureExecutablePermission") - prep { wd => + test("unzipDirectoriesWithoutReadOrExecute") - prep { wd => if (!scala.util.Properties.isWin) { - val zipFileName = "zipDirExecutable" - val source = wd / "folder1" - val dir = source / "dir" + def zip_(sources: Seq[(os.Path, os.PermSet)], root: os.Path, dest: os.Path): os.Path = { + import os.{shaded_org_apache_tools_zip => apache} - os.makeDir(dir) - val perms = os.perms(dir) - os.perms.set(dir, perms - PosixFilePermission.OWNER_EXECUTE) + val zipOut = new apache.ZipOutputStream( + java.nio.file.Files.newOutputStream(dest.toNIO) + ) - val zipped = os.zip( - dest = wd / s"$zipFileName.zip", - sources = Seq(source) + try { + sources.foreach { case (p, perms) => + val name = p.subRelativeTo(root).toString + (if (os.isDir(p)) "/" else "") + + val fileType = apache.PermissionUtils.FileType.of(p.toNIO) + val mode = apache.PermissionUtils.modeFromPermissions(perms.toSet(), fileType) + val fis = if (os.isDir(p)) + None + else Some(os.read.inputStream(p)) + + val zipEntry = new apache.ZipEntry(name) + zipEntry.setUnixMode(mode) + + try { + zipOut.putNextEntry(zipEntry) + fis.foreach(os.Internals.transfer(_, zipOut, close = false)) + zipOut.closeEntry() + } finally { + fis.foreach(_.close()) + } + } + zipOut.finish() + } finally { + zipOut.close() + } + + dest + } + + def walk_(p: os.Path): geny.Generator[os.Path] = { + if (os.isDir(p)) + os.list.stream(p) ++ os.list.stream(p).flatMap(walk_) + else geny.Generator() + } + + import java.nio.file.attribute.PosixFilePermission._ + + val zipFileName = "zipDirNoReadExecute" + val source = wd / "dirNoReadExecute" + val dir = source / "dir" + val nested = dir / "nested" + val file = nested / "file.txt" + + os.makeDir.all(nested) + os.write(file, "Contents of file.txt") + + val readAndExecute = os.PermSet.fromSet(java.util.Set.of( + OWNER_READ, + OWNER_EXECUTE, + GROUP_READ, + GROUP_EXECUTE, + OTHERS_READ, + OTHERS_EXECUTE + )) + + val filesToZip: Seq[(os.Path, os.PermSet)] = + Seq(dir, nested, file) + .map(p => (p, if (os.isDir(p)) os.perms(p) -- readAndExecute else os.perms(p))) + + val zipped = zip_(filesToZip, source, wd / s"$zipFileName.zip") + val unzipped = os.unzip( + zipped, + dest = wd / zipFileName ) - val unzipped = os.unzip(zipped, dest = wd / zipFileName) - assert(os.perms(unzipped / "dir").contains(PosixFilePermission.OWNER_EXECUTE)) - assert(os.perms(unzipped / "dir") == perms) + walk_(unzipped).foreach { p => + os.perms.set(p, os.perms(p) + OWNER_READ + OWNER_EXECUTE) + } + + assert(os.walk(unzipped).map(_.subRelativeTo(unzipped)) == + os.walk(source).map(_.subRelativeTo(source))) } } }