Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-64621] Fix zip regression #5187

Merged
merged 3 commits into from Jan 19, 2021
Merged

[JENKINS-64621] Fix zip regression #5187

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2be90a5
[JENKINS-64621] Fix zip regression
Wadeck Jan 18, 2021
735def2
Add trim as requested
Wadeck Jan 19, 2021
465ff48
Manual revert
Wadeck Jan 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions core/src/main/java/hudson/FilePath.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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.trim(), "/");
Wadeck marked this conversation as resolved.
Show resolved Hide resolved
}

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
Open in desktop
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
Open in desktop
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
Open in desktop
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