From 49f4e3f8006f90f9336f5b63cf2a4684d06487ce Mon Sep 17 00:00:00 2001 From: Ian Dees Date: Sat, 22 Aug 2026 07:39:55 -0500 Subject: [PATCH 1/3] always extract nested zip files regardless of the conform file filter --- openaddr/conform.py | 8 +++ openaddr/tests/conform.py | 101 +++++++++++++++++++++++++++++++++++++- 2 files changed, 108 insertions(+), 1 deletion(-) diff --git a/openaddr/conform.py b/openaddr/conform.py index 04f70811..1c11865b 100644 --- a/openaddr/conform.py +++ b/openaddr/conform.py @@ -240,6 +240,14 @@ def decompress(self, source_paths, workdir, filenames): def _extract_zip(self, source_path, expand_path, filenames): with ZipFile(source_path, 'r') as z: for name in z.namelist(): + # Nested zip files are always extracted regardless of the + # filenames filter, since the requested file may be inside + # one of them. The filter is re-applied when that nested + # zip is itself extracted. + if name.lower().endswith('.zip'): + z.extract(name, expand_path) + continue + if len(filenames) and not is_in(name, filenames): # Download only the named file, if any. _L.debug("Skipped file {}".format(name)) diff --git a/openaddr/tests/conform.py b/openaddr/tests/conform.py index 071dc9fd..8393e444 100644 --- a/openaddr/tests/conform.py +++ b/openaddr/tests/conform.py @@ -12,6 +12,8 @@ import tempfile import shutil +from zipfile import ZipFile + from .. import SourceConfig from ..conform import ( @@ -25,7 +27,8 @@ row_fxn_first_non_empty, row_fxn_constant, row_fxn_map, row_canonicalize_unit_and_number, conform_cli, convert_regexp_replace, normalize_ogr_filename_case, - is_in, geojson_source_to_csv, ogr_source_to_csv, check_source_tests + is_in, geojson_source_to_csv, ogr_source_to_csv, check_source_tests, + ZipDecompressTask, DecompressionError, elaborate_filenames ) " Return an x,y array given a wkt point string" @@ -2680,3 +2683,99 @@ def test_row_fxn_map_else(self): d = row_fxn_map(c, d, "accuracy", c.data_source["conform"]["accuracy"]) self.assertEqual(e, d) + + +class TestZipDecompressTask(unittest.TestCase): + ''' Regression tests for GitHub issue #35: "Support zipped shapefiles + within source zip" -- an outer source zip contains a nested zip that + itself contains the shapefile, plus miscellaneous extra files + (license, readme, lookup tables) sitting next to the nested zip. + ''' + + def setUp(self): + self.testdir = tempfile.mkdtemp(prefix='openaddr-TestZipDecompressTask-') + self.workdir = os.path.join(self.testdir, 'work') + os.mkdir(self.workdir) + + # A real shapefile (.shp/.shx/.dbf/.prj) fixture, already zipped up. + self.inner_zip_path = os.path.join( + os.path.dirname(__file__), 'conforms', 'lake-man.zip') + + def tearDown(self): + shutil.rmtree(self.testdir) + + def _make_outer_zip(self, extra_files={'license.txt': 'be nice', 'readme.txt': 'read me'}): + ''' Build an outer.zip containing nested.zip (the shapefile zip) + alongside some unrelated non-zip files, matching the structure + described in issue #35. + ''' + outer_zip_path = os.path.join(self.testdir, 'outer.zip') + + with ZipFile(outer_zip_path, 'w') as outer_zip: + outer_zip.write(self.inner_zip_path, arcname='nested.zip') + for name, content in extra_files.items(): + outer_zip.writestr(name, content) + + return outer_zip_path + + def test_single_nested_zip_is_extracted(self): + ''' A zip-of-a-zip-containing-a-shapefile, with no "file" filter + (i.e. no conform "file" tag), should be fully extracted so the + shapefile members end up available. + ''' + outer_zip_path = self._make_outer_zip() + + task = ZipDecompressTask() + output_files = task.decompress([outer_zip_path], self.workdir, []) + + output_names = {os.path.basename(path) for path in output_files} + self.assertIn('lake-man.shp', output_names) + self.assertIn('lake-man.shx', output_names) + self.assertIn('lake-man.dbf', output_names) + self.assertIn('lake-man.prj', output_names) + + # The extra sibling files should also have survived extraction. + self.assertIn('license.txt', output_names) + self.assertIn('readme.txt', output_names) + + shp_path = next(path for path in output_files if path.endswith('lake-man.shp')) + self.assertGreater(os.path.getsize(shp_path), 0) + + def test_single_nested_zip_is_extracted_with_file_filter(self): + ''' Same structure as above, but exercised the way a real source + would use it: with a conform "file" tag naming the shapefile + inside the nested zip (e.g. "file": "lake-man.shp"), which is + expanded by elaborate_filenames() into the .shp/.shx/.dbf/.prj + set and passed through as the `filenames` allow-list. + ''' + outer_zip_path = self._make_outer_zip() + filenames = elaborate_filenames('lake-man.shp') + + task = ZipDecompressTask() + output_files = task.decompress([outer_zip_path], self.workdir, filenames) + + output_names = {os.path.basename(path) for path in output_files} + self.assertIn('lake-man.shp', output_names) + self.assertIn('lake-man.shx', output_names) + self.assertIn('lake-man.dbf', output_names) + self.assertIn('lake-man.prj', output_names) + + # Sibling non-zip files that don't match the filter should still be + # skipped, since the caller only asked for the named shapefile. + self.assertNotIn('license.txt', output_names) + self.assertNotIn('readme.txt', output_names) + + def test_multiple_nested_zips_raise_decompression_error(self): + ''' If more than one zip appears at the same directory level inside + the outer zip, extraction should fail loudly rather than + silently pick one. + ''' + outer_zip_path = os.path.join(self.testdir, 'outer-ambiguous.zip') + + with ZipFile(outer_zip_path, 'w') as outer_zip: + outer_zip.write(self.inner_zip_path, arcname='nested-a.zip') + outer_zip.write(self.inner_zip_path, arcname='nested-b.zip') + + task = ZipDecompressTask() + with self.assertRaises(DecompressionError): + task.decompress([outer_zip_path], self.workdir, []) From c90ee0e4d1b19f25aa4d1ec0b3abe108d9a88bbf Mon Sep 17 00:00:00 2001 From: Ian Dees Date: Sat, 22 Aug 2026 07:58:41 -0500 Subject: [PATCH 2/3] cap declared entry size and nesting depth in recursive zip extraction --- openaddr/conform.py | 29 +++++++++++++++++++++++++++-- openaddr/tests/conform.py | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+), 2 deletions(-) diff --git a/openaddr/conform.py b/openaddr/conform.py index 1c11865b..72aa4589 100644 --- a/openaddr/conform.py +++ b/openaddr/conform.py @@ -203,6 +203,15 @@ def is_in(path, names): return False class ZipDecompressTask(DecompressionTask): + # Recursing into nested zips (see #35/#112) means a maliciously or + # accidentally crafted zip bomb could otherwise fill a job's disk before + # any per-entry file type filtering applies. Cap the declared + # (uncompressed) size of any single entry and how many nested zips deep + # we'll recurse, so worst case is bounded and the job fails loudly + # instead of exhausting disk. + MAX_ZIP_ENTRY_BYTES = 2 * 1024 ** 3 # 2GB + MAX_NESTED_ZIP_DEPTH = 10 + def decompress(self, source_paths, workdir, filenames): output_files = [] expand_path = os.path.join(workdir, UNZIPPED_DIRNAME) @@ -218,6 +227,11 @@ def decompress(self, source_paths, workdir, filenames): pending = list(self._find_single_zips(expand_path)) while pending: + if len(processed) >= self.MAX_NESTED_ZIP_DEPTH: + raise DecompressionError( + "Refusing to recurse more than {} nested zip files deep - possible zip bomb" + .format(self.MAX_NESTED_ZIP_DEPTH) + ) zip_path = pending.pop() if zip_path in processed: continue @@ -239,13 +253,24 @@ def decompress(self, source_paths, workdir, filenames): def _extract_zip(self, source_path, expand_path, filenames): with ZipFile(source_path, 'r') as z: - for name in z.namelist(): + for zinfo in z.infolist(): + name = zinfo.filename + + # Check the declared (uncompressed) size from the zip's + # central directory before extracting anything - a bomb's + # compressed size can be tiny, but its declared size isn't. + if zinfo.file_size > self.MAX_ZIP_ENTRY_BYTES: + raise DecompressionError( + "Refusing to extract {} - declared size {} bytes exceeds {} byte limit, possible zip bomb" + .format(name, zinfo.file_size, self.MAX_ZIP_ENTRY_BYTES) + ) + # Nested zip files are always extracted regardless of the # filenames filter, since the requested file may be inside # one of them. The filter is re-applied when that nested # zip is itself extracted. if name.lower().endswith('.zip'): - z.extract(name, expand_path) + z.extract(zinfo, expand_path) continue if len(filenames) and not is_in(name, filenames): diff --git a/openaddr/tests/conform.py b/openaddr/tests/conform.py index 8393e444..9ab64098 100644 --- a/openaddr/tests/conform.py +++ b/openaddr/tests/conform.py @@ -12,6 +12,7 @@ import tempfile import shutil +from unittest import mock from zipfile import ZipFile from .. import SourceConfig @@ -2779,3 +2780,37 @@ def test_multiple_nested_zips_raise_decompression_error(self): task = ZipDecompressTask() with self.assertRaises(DecompressionError): task.decompress([outer_zip_path], self.workdir, []) + + def test_oversized_entry_raises_decompression_error(self): + ''' A zip entry declaring an uncompressed size over the configured + cap should be refused before extraction, as a zip bomb guard. + Uses a real (small) fixture but lowers the cap far below its + actual size, rather than crafting an actual multi-GB payload. + ''' + outer_zip_path = self._make_outer_zip() + + task = ZipDecompressTask() + with mock.patch.object(ZipDecompressTask, 'MAX_ZIP_ENTRY_BYTES', 10): + with self.assertRaises(DecompressionError): + task.decompress([outer_zip_path], self.workdir, []) + + def test_deeply_nested_zips_raise_decompression_error(self): + ''' A chain of nested zips deeper than the configured limit should + be refused, as a zip bomb guard against chained amplification. + Each level's nested zip is placed inside its own subfolder + (arcname "levelN/nested.zip") so unwrapping one level doesn't + land in the same directory as the next - matching how the + existing "one zip per directory" width check expects real + nested archives to be laid out. + ''' + current_path = self.inner_zip_path + for level in range(4): + next_path = os.path.join(self.testdir, 'wrap-{}.zip'.format(level)) + with ZipFile(next_path, 'w') as z: + z.write(current_path, arcname='level{}/nested.zip'.format(level)) + current_path = next_path + + task = ZipDecompressTask() + with mock.patch.object(ZipDecompressTask, 'MAX_NESTED_ZIP_DEPTH', 2): + with self.assertRaises(DecompressionError): + task.decompress([current_path], self.workdir, []) From c6e1114450bee0b01ea5b294089b06facf45cb44 Mon Sep 17 00:00:00 2001 From: Ian Dees Date: Sat, 22 Aug 2026 08:03:12 -0500 Subject: [PATCH 3/3] stop recursing into nested zips once a file filter is already satisfied --- openaddr/conform.py | 39 +++++++++++++++++++++++++++++++++++---- openaddr/tests/conform.py | 38 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 4 deletions(-) diff --git a/openaddr/conform.py b/openaddr/conform.py index 72aa4589..653d8eac 100644 --- a/openaddr/conform.py +++ b/openaddr/conform.py @@ -218,13 +218,27 @@ def decompress(self, source_paths, workdir, filenames): mkdirsp(expand_path) # Extract contents of zip file into expand_path directory. + found = set() for source_path in source_paths: - self._extract_zip(source_path, expand_path, filenames) + found |= self._extract_zip(source_path, expand_path, filenames) + + def fully_satisfied(): + # Only short-circuit when there's an explicit file filter AND + # we can prove every name it asked for has actually been + # extracted - if filenames is empty (caller wants everything) + # or matching is inexact (e.g. a directory-style entry in + # `filenames` that `is_in()` matched but doesn't literally + # equal), this stays False and we fall back to full recursion, + # same as before this optimization existed. + return bool(filenames) and set(filenames) <= found # Recursively extract nested zip files, but fail if more than one zip - # appears at the same directory level. + # appears at the same directory level. Skip recursing at all once the + # requested file(s) are already found - there's no reason to keep + # opening nested zips we don't need, which also limits exposure to a + # zip bomb hiding deeper in the chain than what was asked for. processed = set() - pending = list(self._find_single_zips(expand_path)) + pending = [] if fully_satisfied() else list(self._find_single_zips(expand_path)) while pending: if len(processed) >= self.MAX_NESTED_ZIP_DEPTH: @@ -236,7 +250,9 @@ def decompress(self, source_paths, workdir, filenames): if zip_path in processed: continue processed.add(zip_path) - self._extract_zip(zip_path, os.path.dirname(zip_path), filenames) + found |= self._extract_zip(zip_path, os.path.dirname(zip_path), filenames) + if fully_satisfied(): + break pending.extend(self._find_single_zips(os.path.dirname(zip_path))) # Collect names of directories and files in expand_path directory. @@ -246,12 +262,23 @@ def decompress(self, source_paths, workdir, filenames): output_files.append(os.path.join(dirpath, dirname)) _L.debug("Expanded directory {}".format(output_files[-1])) for filename in filenames: + if filename.lower().endswith('.zip'): + # A zip left un-recursed-into (e.g. because the request + # was already satisfied without it, or filtered out at + # its own directory level) isn't a usable source file. + continue output_files.append(os.path.join(dirpath, filename)) _L.debug("Expanded file {}".format(output_files[-1])) return output_files def _extract_zip(self, source_path, expand_path, filenames): + ''' Extract matching entries, returning the subset of `filenames` + (lower-cased) that were found by exact name - used by + decompress() to tell whether it's safe to stop recursing into + further nested zips. + ''' + found = set() with ZipFile(source_path, 'r') as z: for zinfo in z.infolist(): name = zinfo.filename @@ -278,7 +305,11 @@ def _extract_zip(self, source_path, expand_path, filenames): _L.debug("Skipped file {}".format(name)) continue + if len(filenames) and name.lower() in filenames: + found.add(name.lower()) + z.extract(name, expand_path) + return found def _find_single_zips(self, root_path): zip_paths = [] diff --git a/openaddr/tests/conform.py b/openaddr/tests/conform.py index 9ab64098..d5cc1327 100644 --- a/openaddr/tests/conform.py +++ b/openaddr/tests/conform.py @@ -2781,6 +2781,44 @@ def test_multiple_nested_zips_raise_decompression_error(self): with self.assertRaises(DecompressionError): task.decompress([outer_zip_path], self.workdir, []) + def test_skips_nested_zip_once_filtered_file_is_already_found(self): + ''' If the outer zip already directly contains everything the + "file" filter asked for, an unrelated nested zip sitting next + to it should never be opened/extracted at all - there's no + reason to recurse once the request is satisfied, and not doing + so limits exposure to a zip bomb hiding in that nested zip. + ''' + outer_zip_path = os.path.join(self.testdir, 'outer-already-satisfied.zip') + conforms_dir = os.path.join(os.path.dirname(__file__), 'conforms') + + with ZipFile(outer_zip_path, 'w') as outer_zip: + for ext in ('.shp', '.shx', '.dbf', '.prj'): + outer_zip.write( + os.path.join(conforms_dir, 'lake-man' + ext), + arcname='lake-man' + ext + ) + # An unrelated nested zip that should never get opened. + with tempfile.NamedTemporaryFile(suffix='.zip') as decoy_zip_file: + with ZipFile(decoy_zip_file.name, 'w') as decoy_zip: + decoy_zip.writestr('decoy.txt', 'should never be extracted') + outer_zip.write(decoy_zip_file.name, arcname='decoy.zip') + + filenames = elaborate_filenames('lake-man.shp') + + task = ZipDecompressTask() + output_files = task.decompress([outer_zip_path], self.workdir, filenames) + + output_names = {os.path.basename(path) for path in output_files} + self.assertIn('lake-man.shp', output_names) + self.assertIn('lake-man.shx', output_names) + self.assertIn('lake-man.dbf', output_names) + self.assertIn('lake-man.prj', output_names) + + # Proves decoy.zip was never opened/extracted: neither its contents + # nor the zip file itself should appear anywhere in the output. + self.assertNotIn('decoy.txt', output_names) + self.assertNotIn('decoy.zip', output_names) + def test_oversized_entry_raises_decompression_error(self): ''' A zip entry declaring an uncompressed size over the configured cap should be refused before extraction, as a zip bomb guard.