Skip to content

Commit

Permalink
[JENKINS-64621] Fix zip regression (jenkinsci#5187)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Wadeck
Wadeck committed Jan 19, 2021
1 parent 1aa95d9 commit ecf3095
Show file tree
Hide file tree
Showing 7 changed files with 133 additions and 22 deletions.
6 changes: 4 additions & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Expand Up @@ -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);
}

Expand Down
11 changes: 10 additions & 1 deletion core/src/main/java/hudson/model/DirectoryBrowserSupport.java
View file
Expand Up @@ -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) {
Expand Down
15 changes: 12 additions & 3 deletions core/src/main/java/hudson/util/io/ArchiverFactory.java
View file
Expand Up @@ -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;
Expand All @@ -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;
Expand Down
22 changes: 18 additions & 4 deletions core/src/main/java/hudson/util/io/ZipArchiver.java
View file
Expand Up @@ -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;
Expand All @@ -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"));
Expand All @@ -69,15 +83,15 @@ 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);
dirZipEntry.setTime(f.lastModified());
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);
Expand Down
10 changes: 5 additions & 5 deletions core/src/main/java/jenkins/util/VirtualFile.java
View file
Expand Up @@ -338,7 +338,7 @@ private List<TokenizedPattern> 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.");
}

Expand Down Expand Up @@ -611,10 +611,10 @@ public Collection<String> 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();
Expand Down Expand Up @@ -920,11 +920,11 @@ public Collection<String> 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);
}
Expand Down
64 changes: 62 additions & 2 deletions core/src/test/java/jenkins/util/VirtualFileTest.java
View file
Expand Up @@ -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());
Expand All @@ -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 {
Expand All @@ -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());
Expand All @@ -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());
Expand Down
27 changes: 22 additions & 5 deletions test/src/test/java/hudson/model/DirectoryBrowserSupportTest.java
View file
Expand Up @@ -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);
Expand Down Expand Up @@ -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<String> 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<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, containsInAnyOrder(
"intermediateFolder/public2.key",
Expand All @@ -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<String> 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<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("public2.key"));
}
Expand Down Expand Up @@ -739,16 +756,16 @@ public void junctionAndSymlink_outsideWorkspace_areNotAllowed_windowsJunction()

List<String> 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
Page zipPage = wc.goTo(p.getUrl() + "ws/intermediateFolder/*zip*/intermediateFolder.zip", null);
assertThat(zipPage.getWebResponse().getStatusCode(), equalTo(HttpURLConnection.HTTP_OK));

List<String> entryNames = getListOfEntriesInDownloadedZip((UnexpectedPage) zipPage);
assertThat(entryNames, contains("public2.key"));
assertThat(entryNames, contains("intermediateFolder/public2.key"));
}
}

Expand Down

0 comments on commit ecf3095

Please sign in to comment.