diff --git a/CHANGELOG.md b/CHANGELOG.md index a516e4a4..bc8f6a76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ - **The prebuilt Docker image (`ghcr.io/domainaware/parsedmarc`) is roughly 40% smaller to pull** ([#893](https://github.com/domainaware/parsedmarc/pull/893)). The runtime stage copied the built wheel out of the build stage and deleted it again at the end of the next `RUN`, but a `RUN` can only write a whiteout over a layer an earlier instruction already committed: the wheel shipped in every published image and every `docker pull` downloaded it (10,713,473 bytes of the 11.0.0 image, on both architectures). The wheel is now bind-mounted from the build stage instead, and a bind mount is never committed to a layer. `pip install` also runs with `--no-cache-dir`, which drops a further ~99 MB of pip's download cache that the image had been carrying in the same layer as `site-packages`. Measured on linux/amd64: 272,138,724 compressed bytes across six layers before, 163,757,439 across five after. - **Bare `exit(...)` calls are now `sys.exit(...)` throughout the CLI and the maintainer map-tooling scripts.** `exit` is installed into `builtins` by the `site` module, not just for interactive sessions, so it isn't guaranteed to exist under `python -S` or an embedded interpreter that skips `site` — the failure paths that called it would raise `NameError` instead of actually exiting. `sys.exit` is always available and was already used elsewhere in `cli.py` and `find_unknown_base_reverse_dns.py`. Also removed the dead, immediately-recomputed `index_date` stores in the Elasticsearch and OpenSearch aggregate-report savers; behavior is unchanged. +### Bug fixes + +- `find_unknown_base_reverse_dns.py`'s missing-file checks for `base_reverse_dns_map.csv` and the `known_unknown`/PSL-override lists printed a clean error message but fell through into an unhandled `FileNotFoundError` traceback instead of exiting. + ## 11.0.1 ### Security diff --git a/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py b/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py index 9d754677..5752700d 100755 --- a/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py +++ b/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py @@ -168,6 +168,7 @@ def _main(): def load_list(file_path, list_var): if not os.path.exists(file_path): print(f"Error: {file_path} does not exist") + sys.exit(1) print(f"Loading {file_path}") with open(file_path) as f: for line in f.readlines(): @@ -188,6 +189,7 @@ def _main(): print(f"Indexed {len(as_name_index)} as_names from the MMDB") if not os.path.exists(base_reverse_dns_map_file_path): print(f"Error: {base_reverse_dns_map_file_path} does not exist") + sys.exit(1) print(f"Loading {base_reverse_dns_map_file_path}") with open(base_reverse_dns_map_file_path) as f: for row in csv.DictReader(f): @@ -201,8 +203,8 @@ def _main(): known_domains.append(domain) if domain in known_unknown_domains and known_domains: print( - f"Error:{domain} is in {known_unknown_list_file_path} and \ - {base_reverse_dns_map_file_path}" + f"Error: {domain} is in {known_unknown_list_file_path} " + f"and {base_reverse_dns_map_file_path}" ) sys.exit(1) if not os.path.exists(args.input): diff --git a/tests/test_maps.py b/tests/test_maps.py index 3696d142..68652af0 100644 --- a/tests/test_maps.py +++ b/tests/test_maps.py @@ -4,7 +4,14 @@ These scripts are maintainer-only batch tooling — they do not ship in the wheel — but they still need regression coverage because they enforce the privacy and integrity rules for the reverse-DNS map data files.""" +import contextlib +import io +import os +import shutil +import sys +import tempfile import unittest +from unittest import mock class TestMapScriptsIPDetection(unittest.TestCase): @@ -53,6 +60,116 @@ class TestMapScriptsIPDetection(unittest.TestCase): self.assertEqual(cdi._apply_psl_override("example.com", ov), "example.com") +class TestFindUnknownBaseReverseDNS(unittest.TestCase): + """Missing-file error paths in find_unknown_base_reverse_dns.py's + ``_main()``. + + Both sites below printed the intended ``"Error: ... does not exist"`` + message but had no ``sys.exit(1)`` after it, so execution fell through + into the next ``open()`` call on the same missing path and raised an + unhandled ``FileNotFoundError`` instead of the clean, intended exit. + """ + + def test_missing_known_unknown_list_exits_cleanly(self): + """A missing known_unknown_base_reverse_dns.txt must print the + intended error message and exit(1), not fall through into an + unhandled FileNotFoundError from the subsequent open() call. + + Regression test: the nested ``load_list()`` helper in ``_main()`` + printed ``f"Error: {file_path} does not exist"`` but had no + ``sys.exit(1)`` after it, so execution fell through into + ``print(f"Loading {file_path}")`` and then ``open(file_path)`` on + the same missing path, raising an unhandled ``FileNotFoundError`` + instead of the clean, intended error exit. The sibling + duplicate-entry check a few lines below (``domain in list_var``) + already did print-then-``sys.exit(1)``; this verifies the + missing-file check now matches that style. Capturing stdout and + asserting on the exact message (rather than only the exit code) + pins the exit to this specific site, not just any ``sys.exit(1)`` + in the function. + """ + import parsedmarc.resources.maps.find_unknown_base_reverse_dns as fu + + old_cwd = os.getcwd() + tmp_dir = tempfile.mkdtemp() + # Register rmtree first so, under LIFO cleanup ordering, chdir back + # to old_cwd always runs before rmtree removes tmp_dir -- and, since + # unittest runs each addCleanup independently, rmtree still runs + # even if os.chdir were to raise. + self.addCleanup(shutil.rmtree, tmp_dir, ignore_errors=True) + self.addCleanup(os.chdir, old_cwd) + os.chdir(tmp_dir) + + stdout = io.StringIO() + with mock.patch.object(sys, "argv", ["find_unknown_base_reverse_dns.py"]): + with contextlib.redirect_stdout(stdout): + with self.assertRaises(SystemExit) as cm: + fu._main() + self.assertEqual(cm.exception.code, 1) + self.assertIn( + "Error: known_unknown_base_reverse_dns.txt does not exist", + stdout.getvalue(), + ) + + def test_missing_base_reverse_dns_map_exits_cleanly(self): + """A missing base_reverse_dns_map.csv must print the intended error + message and exit(1), not fall through into an unhandled + FileNotFoundError from the subsequent open() call. + + Regression test for the second site fixed alongside the + ``load_list()`` one above: ``_main()`` printed + ``f"Error: {base_reverse_dns_map_file_path} does not exist"`` but + had no ``sys.exit(1)`` after it. Reaching this check requires + getting past the MMDB load first. ``_load_as_name_index`` does its + external work entirely through ``maxminddb.open_database()`` -- + the actual SDK boundary per AGENTS.md's "mock at SDK boundaries" + rule -- so that call is mocked to a context manager over an empty + iterable instead of loading the real, ~23MB bundled MMDB or + mocking an internal helper of this codebase. + """ + import parsedmarc.resources.maps.find_unknown_base_reverse_dns as fu + + old_cwd = os.getcwd() + tmp_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp_dir, ignore_errors=True) + self.addCleanup(os.chdir, old_cwd) + + maps_dir = os.path.join(tmp_dir, "maps") + ipinfo_dir = os.path.join(tmp_dir, "ipinfo") + os.makedirs(maps_dir) + os.makedirs(ipinfo_dir) + # Both lists are loaded before the MMDB check and must exist, but + # their content isn't exercised by this test. + open(os.path.join(maps_dir, "known_unknown_base_reverse_dns.txt"), "w").close() + open(os.path.join(maps_dir, "psl_overrides.txt"), "w").close() + # A placeholder for the MMDB: only os.path.exists() touches this + # path directly, since maxminddb.open_database() itself is mocked + # below and never actually reads the file. + open(os.path.join(ipinfo_dir, "ipinfo_lite.mmdb"), "w").close() + # base_reverse_dns_map.csv is deliberately NOT created -- that's + # the missing-file condition under test. + + os.chdir(maps_dir) + + class _EmptyMMDBReader: + def __enter__(self): + return iter(()) + + def __exit__(self, *exc_info): + return False + + stdout = io.StringIO() + with mock.patch.object(sys, "argv", ["find_unknown_base_reverse_dns.py"]): + with mock.patch("maxminddb.open_database", return_value=_EmptyMMDBReader()): + with contextlib.redirect_stdout(stdout): + with self.assertRaises(SystemExit) as cm: + fu._main() + self.assertEqual(cm.exception.code, 1) + self.assertIn( + "Error: base_reverse_dns_map.csv does not exist", stdout.getvalue() + ) + + class TestDetectPSLOverrides(unittest.TestCase): """Cluster detection, brand-tail extraction, and full-pipeline behaviour for `detect_psl_overrides.py`."""