From ecf309536208cdf847732f3f0a0a7a2a0e1fa2c1 Mon Sep 17 00:00:00 2001 From: Wadeck Follonier Date: Tue, 19 Jan 2021 10:11:30 +0100 Subject: [PATCH] [JENKINS-64621] Fix zip regression (#5187) --- core/src/main/java/hudson/FilePath.java | 6 +- .../hudson/model/DirectoryBrowserSupport.java | 11 +++- .../java/hudson/util/io/ArchiverFactory.java | 15 ++++- .../main/java/hudson/util/io/ZipArchiver.java | 22 +++++-- .../main/java/jenkins/util/VirtualFile.java | 10 +-- .../java/jenkins/util/VirtualFileTest.java | 64 ++++++++++++++++++- .../model/DirectoryBrowserSupportTest.java | 27 ++++++-- 7 files changed, 133 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/hudson/FilePath.java b/core/src/main/java/hudson/FilePath.java index ba8e7f26b4ef..8f8521e52d39 100644 --- a/core/src/main/java/hudson/FilePath.java +++ b/core/src/main/java/hudson/FilePath.java @@ -484,13 +484,15 @@ public int zip(OutputStream out, DirScanner scanner) throws IOException, Interru * Any symlinks between a file and root should be ignored. * Symlinks in the parentage outside root will not be checked. * @param noFollowLinks true if it should not follow links. + * @param prefix The portion of file path that will be added at the beginning of the relative path inside the archive. + * If non-empty, a trailing forward slash will be enforced. * * @return The number of files/directories archived. * This is only really useful to check for a situation where nothing */ @Restricted(NoExternalUse.class) - public int zip(OutputStream out, DirScanner scanner, String verificationRoot, boolean noFollowLinks) throws IOException, InterruptedException { - ArchiverFactory archiverFactory = noFollowLinks ? ArchiverFactory.ZIP_WTHOUT_FOLLOWING_SYMLINKS : ArchiverFactory.ZIP; + public int zip(OutputStream out, DirScanner scanner, String verificationRoot, boolean noFollowLinks, String prefix) throws IOException, InterruptedException { + ArchiverFactory archiverFactory = noFollowLinks ? ArchiverFactory.createZipWithoutSymlink(prefix) : ArchiverFactory.ZIP; return archive(archiverFactory, out, scanner, verificationRoot, noFollowLinks); } diff --git a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java index 0a03b2504049..c6ab15d7542d 100644 --- a/core/src/main/java/hudson/model/DirectoryBrowserSupport.java +++ b/core/src/main/java/hudson/model/DirectoryBrowserSupport.java @@ -251,7 +251,16 @@ private void serveFile(StaplerRequest req, StaplerResponse rsp, VirtualFile root if(baseFile.isDirectory()) { if(zip) { rsp.setContentType("application/zip"); - baseFile.zip(rsp.getOutputStream(), StringUtils.isBlank(rest) ? "**" : rest, null, true, getNoFollowLinks()); + String includes, prefix; + if (StringUtils.isBlank(rest)) { + includes = "**"; + // JENKINS-19947, JENKINS-61473: traditional behavior is to prepend the directory name + prefix = baseFile.getName(); + } else { + includes = rest; + prefix = ""; + } + baseFile.zip(rsp.getOutputStream(), includes, null, true, getNoFollowLinks(), prefix); return; } if (plain) { diff --git a/core/src/main/java/hudson/util/io/ArchiverFactory.java b/core/src/main/java/hudson/util/io/ArchiverFactory.java index 75c7748a5f05..b4f053b27e19 100644 --- a/core/src/main/java/hudson/util/io/ArchiverFactory.java +++ b/core/src/main/java/hudson/util/io/ArchiverFactory.java @@ -61,10 +61,13 @@ public abstract class ArchiverFactory implements Serializable { /** * Zip format, without following symlinks. + * @param prefix The portion of file path that will be added at the beginning of the relative path inside the archive. + * If non-empty, a trailing forward slash will be enforced. */ @Restricted(NoExternalUse.class) - public static ArchiverFactory ZIP_WTHOUT_FOLLOWING_SYMLINKS = new ZipWithoutSymLinksArchiverFactory(); - + public static ArchiverFactory createZipWithoutSymlink(String prefix) { + return new ZipWithoutSymLinksArchiverFactory(prefix); + } private static final class TarArchiverFactory extends ArchiverFactory { private final TarCompression method; @@ -89,8 +92,14 @@ public Archiver create(OutputStream out) { } private static final class ZipWithoutSymLinksArchiverFactory extends ArchiverFactory { + private final String prefix; + + ZipWithoutSymLinksArchiverFactory(String prefix){ + this.prefix = prefix; + } + public Archiver create(OutputStream out) { - return new ZipArchiver(out, true); + return new ZipArchiver(out, true, prefix); } private static final long serialVersionUID = 1L; diff --git a/core/src/main/java/hudson/util/io/ZipArchiver.java b/core/src/main/java/hudson/util/io/ZipArchiver.java index 9c0ce62a9de0..216252f24835 100644 --- a/core/src/main/java/hudson/util/io/ZipArchiver.java +++ b/core/src/main/java/hudson/util/io/ZipArchiver.java @@ -24,14 +24,19 @@ package hudson.util.io; +import hudson.Util; import hudson.util.FileVisitor; import hudson.util.IOUtils; import java.io.InputStream; import java.nio.file.Files; import java.nio.file.InvalidPathException; + +import org.apache.commons.lang.StringUtils; import org.apache.tools.zip.ZipEntry; import org.apache.tools.zip.Zip64Mode; import org.apache.tools.zip.ZipOutputStream; +import org.kohsuke.accmod.Restricted; +import org.kohsuke.accmod.restrictions.NoExternalUse; import java.io.File; import java.io.IOException; @@ -48,12 +53,21 @@ final class ZipArchiver extends Archiver { private final byte[] buf = new byte[8192]; private final ZipOutputStream zip; private final OpenOption[] openOptions; + private final String prefix; ZipArchiver(OutputStream out) { - this(out, false); + this(out, false, ""); } - ZipArchiver(OutputStream out, boolean failOnSymLink) { + // Restriction added for clarity, it's a package class, you should not use it outside of Jenkins core + @Restricted(NoExternalUse.class) + ZipArchiver(OutputStream out, boolean failOnSymLink, String prefix) { + if (StringUtils.isBlank(prefix)) { + this.prefix = ""; + } else { + this.prefix = Util.ensureEndsWith(prefix, "/"); + } + zip = new ZipOutputStream(out); openOptions = failOnSymLink ? new LinkOption[]{LinkOption.NOFOLLOW_LINKS} : new OpenOption[0]; zip.setEncoding(System.getProperty("file.encoding")); @@ -69,7 +83,7 @@ public void visit(final File f, final String _relativePath) throws IOException { String relativePath = _relativePath.replace('\\', '/'); if(f.isDirectory()) { - ZipEntry dirZipEntry = new ZipEntry(relativePath+'/'); + ZipEntry dirZipEntry = new ZipEntry(this.prefix + relativePath+'/'); // Setting this bit explicitly is needed by some unzipping applications (see JENKINS-3294). dirZipEntry.setExternalAttributes(BITMASK_IS_DIRECTORY); if (mode!=-1) dirZipEntry.setUnixMode(mode); @@ -77,7 +91,7 @@ public void visit(final File f, final String _relativePath) throws IOException { zip.putNextEntry(dirZipEntry); zip.closeEntry(); } else { - ZipEntry fileZipEntry = new ZipEntry(relativePath); + ZipEntry fileZipEntry = new ZipEntry(this.prefix + relativePath); if (mode!=-1) fileZipEntry.setUnixMode(mode); fileZipEntry.setTime(f.lastModified()); zip.putNextEntry(fileZipEntry); diff --git a/core/src/main/java/jenkins/util/VirtualFile.java b/core/src/main/java/jenkins/util/VirtualFile.java index 8b16775ea431..c80fad7bb39a 100644 --- a/core/src/main/java/jenkins/util/VirtualFile.java +++ b/core/src/main/java/jenkins/util/VirtualFile.java @@ -338,7 +338,7 @@ private List patterns(String patts) { */ @Restricted(NoExternalUse.class) public int zip(OutputStream outputStream, String includes, String excludes, boolean useDefaultExcludes, - boolean noFollowLinks) throws IOException, UnsupportedOperationException { + boolean noFollowLinks, String prefix) throws IOException, UnsupportedOperationException { throw new UnsupportedOperationException("Not implemented."); } @@ -611,10 +611,10 @@ public Collection list(String includes, String excludes, boolean useDefa @Override public int zip(OutputStream outputStream, String includes, String excludes, boolean useDefaultExcludes, - boolean noFollowLinks) throws IOException { + boolean noFollowLinks, String prefix) throws IOException { String rootPath = determineRootPath(); DirScanner.Glob globScanner = new DirScanner.Glob(includes, excludes, useDefaultExcludes, !noFollowLinks); - ArchiverFactory archiverFactory = noFollowLinks ? ArchiverFactory.ZIP_WTHOUT_FOLLOWING_SYMLINKS : ArchiverFactory.ZIP; + ArchiverFactory archiverFactory = noFollowLinks ? ArchiverFactory.createZipWithoutSymlink(prefix) : ArchiverFactory.ZIP; try (Archiver archiver = archiverFactory.create(outputStream)) { globScanner.scan(f, FilePath.ignoringSymlinks(archiver, rootPath, noFollowLinks)); return archiver.countEntries(); @@ -920,11 +920,11 @@ public Collection list(String includes, String excludes, boolean useDefa @Override public int zip(OutputStream outputStream, String includes, String excludes, boolean useDefaultExcludes, - boolean noFollowLinks) throws IOException { + boolean noFollowLinks, String prefix) throws IOException { try { String rootPath = root == null ? null : root.getRemote(); DirScanner.Glob globScanner = new DirScanner.Glob(includes, excludes, useDefaultExcludes, !noFollowLinks); - return f.zip(outputStream, globScanner, rootPath, noFollowLinks); + return f.zip(outputStream, globScanner, rootPath, noFollowLinks, prefix); } catch (InterruptedException x) { throw new IOException(x); } diff --git a/core/src/test/java/jenkins/util/VirtualFileTest.java b/core/src/test/java/jenkins/util/VirtualFileTest.java index 6d98e9ba2285..62af1ebb0362 100644 --- a/core/src/test/java/jenkins/util/VirtualFileTest.java +++ b/core/src/test/java/jenkins/util/VirtualFileTest.java @@ -249,7 +249,7 @@ public void zip_NoFollowLinks_FilePathVF() throws Exception { VirtualFile sourcePath = VirtualFile.forFilePath(new FilePath(source)); try (FileOutputStream outputStream = new FileOutputStream(zipFile)) { - sourcePath.zip( outputStream,"**", null, true, true); + sourcePath.zip( outputStream,"**", null, true, true, ""); } FilePath zipPath = new FilePath(zipFile); assertTrue(zipPath.exists()); @@ -267,6 +267,36 @@ public void zip_NoFollowLinks_FilePathVF() throws Exception { assertFalse(unzipPath.child("b").child("_aatxt").exists()); } + @Test + @Issue({"JENKINS-19947", "JENKINS-61473"}) + public void zip_NoFollowLinks_FilePathVF_withPrefix() throws Exception { + File zipFile = new File(tmp.getRoot(), "output.zip"); + File root = tmp.getRoot(); + File source = new File(root, "source"); + prepareFileStructureForIsDescendant(source); + + VirtualFile sourcePath = VirtualFile.forFilePath(new FilePath(source)); + String prefix = "test1"; + try (FileOutputStream outputStream = new FileOutputStream(zipFile)) { + sourcePath.zip( outputStream,"**", null, true, true, prefix + "/"); + } + FilePath zipPath = new FilePath(zipFile); + assertTrue(zipPath.exists()); + assertFalse(zipPath.isDirectory()); + FilePath unzipPath = new FilePath(new File(tmp.getRoot(), "unzip")); + zipPath.unzip(unzipPath); + assertTrue(unzipPath.exists()); + assertTrue(unzipPath.isDirectory()); + assertTrue(unzipPath.child(prefix).isDirectory()); + assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aa.txt").exists()); + assertTrue(unzipPath.child(prefix).child("a").child("ab").child("ab.txt").exists()); + assertFalse(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists()); + assertFalse(unzipPath.child(prefix).child("a").child("_b").exists()); + assertTrue(unzipPath.child(prefix).child("b").child("ba").child("ba.txt").exists()); + assertFalse(unzipPath.child(prefix).child("b").child("_a").exists()); + assertFalse(unzipPath.child(prefix).child("b").child("_aatxt").exists()); + } + @Test @Issue("SECURITY-1452") public void zip_NoFollowLinks_FileVF() throws Exception { @@ -277,7 +307,7 @@ public void zip_NoFollowLinks_FileVF() throws Exception { VirtualFile sourcePath = VirtualFile.forFile(source); try (FileOutputStream outputStream = new FileOutputStream(zipFile)) { - sourcePath.zip( outputStream,"**", null, true, true); + sourcePath.zip( outputStream,"**", null, true, true, ""); } FilePath zipPath = new FilePath(zipFile); assertTrue(zipPath.exists()); @@ -295,6 +325,36 @@ public void zip_NoFollowLinks_FileVF() throws Exception { assertFalse(unzipPath.child("b").child("_aatxt").exists()); } + @Test + @Issue({"JENKINS-19947", "JENKINS-61473"}) + public void zip_NoFollowLinks_FileVF_withPrefix() throws Exception { + File zipFile = new File(tmp.getRoot(), "output.zip"); + File root = tmp.getRoot(); + File source = new File(root, "source"); + prepareFileStructureForIsDescendant(source); + + String prefix = "test1"; + VirtualFile sourcePath = VirtualFile.forFile(source); + try (FileOutputStream outputStream = new FileOutputStream(zipFile)) { + sourcePath.zip( outputStream,"**", null, true, true, prefix + "/"); + } + FilePath zipPath = new FilePath(zipFile); + assertTrue(zipPath.exists()); + assertFalse(zipPath.isDirectory()); + FilePath unzipPath = new FilePath(new File(tmp.getRoot(), "unzip")); + zipPath.unzip(unzipPath); + assertTrue(unzipPath.exists()); + assertTrue(unzipPath.isDirectory()); + assertTrue(unzipPath.child(prefix).isDirectory()); + assertTrue(unzipPath.child(prefix).child("a").child("aa").child("aa.txt").exists()); + assertTrue(unzipPath.child(prefix).child("a").child("ab").child("ab.txt").exists()); + assertFalse(unzipPath.child(prefix).child("a").child("aa").child("aaa").exists()); + assertFalse(unzipPath.child(prefix).child("a").child("_b").exists()); + assertTrue(unzipPath.child(prefix).child("b").child("ba").child("ba.txt").exists()); + assertFalse(unzipPath.child(prefix).child("b").child("_a").exists()); + assertFalse(unzipPath.child(prefix).child("b").child("_aatxt").exists()); + } + @Issue("JENKINS-26810") @Test public void readLink() throws Exception { assumeFalse("Symlinks do not work well on Windows", Functions.isWindows()); diff --git a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java index c88adf0d4862..db59c696fb74 100644 --- a/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java +++ b/test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java @@ -173,7 +173,7 @@ public void zipDownload() throws Exception { ZipFile readzip = new ZipFile(zipfile); - InputStream is = readzip.getInputStream(readzip.getEntry("artifact.out")); + InputStream is = readzip.getInputStream(readzip.getEntry("archive/artifact.out")); // ZipException in case of JENKINS-19752 assertNotEquals("Downloaded zip file must not be empty", is.read(), -1); @@ -520,10 +520,20 @@ public void symlink_outsideWorkspace_areNotAllowed() throws Exception { } // zip feature - { // all the outside folders / files are not included in the zip + { // all the outside folders / files are not included in the zip, also the parent folder is included Page zipPage = wc.goTo(p.getUrl() + "ws/*zip*/ws.zip", null); assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK)); + List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); + assertThat(entryNames, containsInAnyOrder( + p.getName() + "/intermediateFolder/public2.key", + p.getName() + "/public1.key" + )); + } + { // workaround for JENKINS-19947 is still supported, i.e. no parent folder + Page zipPage = wc.goTo(p.getUrl() + "ws/**/*zip*/ws.zip", null); + assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK)); + List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); assertThat(entryNames, containsInAnyOrder( "intermediateFolder/public2.key", @@ -534,6 +544,13 @@ public void symlink_outsideWorkspace_areNotAllowed() throws Exception { Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/*zip*/intermediateFolder.zip", null); assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK)); + List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); + assertThat(entryNames, contains("intermediateFolder/public2.key")); + } + { // workaround for JENKINS-19947 is still supported, i.e. no parent folder, even inside a sub-folder + Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/**/*zip*/intermediateFolder.zip", null); + assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK)); + List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); assertThat(entryNames, contains("public2.key")); } @@ -739,8 +756,8 @@ public void junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction() List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); assertThat(entryNames, containsInAnyOrder( - "intermediateFolder/public2.key", - "public1.key" + p.getName() + "/intermediateFolder/public2.key", + p.getName() + "/public1.key" )); } { // all the outside folders / files are not included in the zip @@ -748,7 +765,7 @@ public void junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction() assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK)); List entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage); - assertThat(entryNames, contains("public2.key")); + assertThat(entryNames, contains("intermediateFolder/public2.key")); } }