Skip to content

Commit c338491

Browse files
lesserwhirlslesserwhirls
committed
Tighten up processing of zip file entries for Zarr
Additionally, add tests for ControllerZip
1 parent 359a13b commit c338491

2 files changed

Lines changed: 153 additions & 4 deletions

File tree

‎cdm/zarr/src/main/java/thredds/filesystem/zarr/ControllerZip.java‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -123,11 +123,17 @@ protected static class FilteredIterator extends MFileIterator implements Iterato
123123
Path relativePath = file.getRelativePath();
124124

125125
for (ZipEntry entry : entries) {
126-
Path entryPath = Paths.get(File.separator + entry.getName());
127-
if (!entryPath.startsWith(relativePath)) {
128-
logger.warn(entryPath.toString() + " is not an entry in " + relativePath.toString());
126+
// entry path (relative to zip file), normalized to remove
127+
// segments like '.' and '..'
128+
Path normEntryPath = Paths.get(entry.getName()).normalize();
129+
// if normEntryPath starts with '..', it has tried to escape
130+
// above its original starting level
131+
if (normEntryPath.startsWith("..")) {
132+
logger.warn(normEntryPath.toString() + " is not an entry in " + relativePath.toString());
129133
continue;
130134
}
135+
// anchor entry path to root of zip file
136+
Path entryPath = Paths.get(File.separator + entry.getName());
131137
// truncate path to one level below current path (i.e. direct child)
132138
Path childPath = entryPath.subpath(0, relativePath.getNameCount() + 1);
133139
fileNames.add(childPath);
@@ -155,7 +161,14 @@ protected static class MFileIteratorLeaves extends MFileIterator implements Iter
155161
List<ZipEntry> entries = file.getLeafEntries();
156162
for (ZipEntry entry : entries) {
157163
try {
158-
this.files.add(new MFileZip(file.getRootPath() + File.separator + entry.getName()));
164+
File entryFile = new File(file.getRootPath() + File.separator + entry.getName());
165+
if (entryFile.toPath().normalize().startsWith(file.getRootPath())) {
166+
this.files.add(new MFileZip(entryFile.toString()));
167+
} else {
168+
// don't allow external references to escape the zip file
169+
// (e.g., skip entries like ../path/outside/of/zip)
170+
logger.warn("Zip entry references external entity in {}: {}. Skipping.", file.getPath(), entryFile);
171+
}
159172
} catch (IOException ioe) {
160173
logger.error(ioe.getMessage(), ioe);
161174
}

‎cdm/zarr/src/test/java/thredds/filesystem/zarr/TestControllerZip.java‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
/*
2+
* Copyright (c) 2026 University Corporation for Atmospheric Research/Unidata
3+
* See LICENSE for license information.
4+
*/
5+
6+
package thredds.filesystem.zarr;
7+
8+
import static com.google.common.truth.Truth.assertThat;
9+
10+
import java.nio.file.Files;
11+
import java.util.Arrays;
12+
import java.util.Collections;
13+
import org.junit.BeforeClass;
14+
import org.junit.ClassRule;
15+
import org.junit.Test;
16+
import org.junit.rules.TemporaryFolder;
17+
import thredds.inventory.CollectionConfig;
18+
import thredds.inventory.MFile;
19+
20+
import java.io.File;
21+
import java.io.IOException;
22+
import java.nio.file.DirectoryStream;
23+
import java.util.ArrayList;
24+
import java.util.List;
25+
import java.util.zip.ZipEntry;
26+
import java.util.zip.ZipOutputStream;
27+
28+
public class TestControllerZip {
29+
30+
@ClassRule
31+
public static final TemporaryFolder tempFolder = new TemporaryFolder();
32+
33+
private static File zipFile, zipFileBad;
34+
35+
@BeforeClass
36+
public static void setUp() throws IOException {
37+
zipFile = tempFolder.newFile("test.zip");
38+
try (ZipOutputStream zos = new ZipOutputStream(Files.newOutputStream(zipFile.toPath()))) {
39+
// file1 (top level)
40+
zos.putNextEntry(new ZipEntry("file1"));
41+
zos.write("content1".getBytes());
42+
zos.closeEntry();
43+
44+
// dir1/file2
45+
zos.putNextEntry(new ZipEntry("dir1/file2"));
46+
zos.write("content2".getBytes());
47+
zos.closeEntry();
48+
49+
// dir1/file3
50+
zos.putNextEntry(new ZipEntry("dir1/file3"));
51+
zos.write("content3".getBytes());
52+
zos.closeEntry();
53+
}
54+
55+
zipFileBad = tempFolder.newFile("test_bad.zip");
56+
try (ZipOutputStream zos = new ZipOutputStream(Files.newOutputStream(zipFileBad.toPath()))) {
57+
// good entries
58+
zos.putNextEntry(new ZipEntry("dir1/file_good1"));
59+
zos.write("content1".getBytes());
60+
zos.closeEntry();
61+
62+
zos.putNextEntry(new ZipEntry("dir1/file_good2"));
63+
zos.write("content2".getBytes());
64+
zos.closeEntry();
65+
66+
// bad entries
67+
// reference outside of zip
68+
zos.putNextEntry(new ZipEntry("../../file_bad"));
69+
zos.write("content3".getBytes());
70+
zos.closeEntry();
71+
72+
// reference outside of zip
73+
zos.putNextEntry(new ZipEntry("dir3/../../file_bad2"));
74+
zos.write("content4".getBytes());
75+
zos.closeEntry();
76+
}
77+
}
78+
79+
@Test
80+
public void testFilteredIteratorFiles() throws IOException {
81+
ControllerZip controller = new ControllerZip();
82+
CollectionConfig mc = new CollectionConfig("test", zipFile.getAbsolutePath(), false, null, null);
83+
try (DirectoryStream<MFile> stream = controller.getInventoryTop(mc, false)) {
84+
assertThat(stream).isNotNull();
85+
List<String> names = new ArrayList<>();
86+
for (MFile mfile : stream) {
87+
names.add(mfile.getName());
88+
}
89+
// only one item in the top level of the zip
90+
assertThat(names).containsExactly(File.separator + "file1");
91+
}
92+
}
93+
94+
@Test
95+
public void testFilteredIteratorDirs() throws IOException {
96+
ControllerZip controller = new ControllerZip();
97+
CollectionConfig mc = new CollectionConfig("test", zipFile.getAbsolutePath(), false, null, null);
98+
try (DirectoryStream<MFile> stream = controller.getSubdirs(mc, false)) {
99+
assertThat(stream).isNotNull();
100+
List<String> names = new ArrayList<>();
101+
for (MFile mfile : stream) {
102+
names.add(mfile.getName());
103+
}
104+
assertThat(names).containsExactly(File.separator + "dir1");
105+
}
106+
}
107+
108+
@Test
109+
public void testFilteredFilesBad() throws IOException {
110+
ControllerZip controller = new ControllerZip();
111+
CollectionConfig mc = new CollectionConfig("test", zipFileBad.getAbsolutePath(), false, null, null);
112+
try (DirectoryStream<MFile> stream = controller.getInventoryAll(mc, false)) {
113+
assertThat(stream).isNotNull();
114+
List<String> names = new ArrayList<>();
115+
for (MFile mfile : stream) {
116+
names.add(mfile.getName());
117+
}
118+
assertThat(names).containsExactlyElementsIn(
119+
Arrays.asList(File.separator + "dir1/file_good1", File.separator + "dir1/file_good2"));
120+
}
121+
}
122+
123+
@Test
124+
public void testFilteredIteratorDirsBad() throws IOException {
125+
ControllerZip controller = new ControllerZip();
126+
CollectionConfig mc = new CollectionConfig("test", zipFileBad.getAbsolutePath(), false, null, null);
127+
try (DirectoryStream<MFile> stream = controller.getSubdirs(mc, false)) {
128+
assertThat(stream).isNotNull();
129+
List<String> names = new ArrayList<>();
130+
for (MFile mfile : stream) {
131+
names.add(mfile.getName());
132+
}
133+
assertThat(names).containsExactlyElementsIn(Collections.singletonList(File.separator + "dir1"));
134+
}
135+
}
136+
}

0 commit comments

Comments
 (0)