Fix file descriptor leak in parse_report_file (CodeQL py/file-not-closed) (#899)

parse_report_file's path branch opened the file with open(file_path, "rb")
and then, on a later line, called file_object.read() followed by
file_object.close() with no exception handling between them. An exception
from read() (e.g. an OSError from the underlying storage) skipped close()
and left the descriptor to be released only when Python's garbage
collector eventually finalized the object, rather than closed
deterministically (CPython's io.IOBase.__del__ closes an unclosed file on
finalization: https://docs.python.org/3/library/io.html). This is the
pattern CodeQL's py/file-not-closed query flags, found in a local
code-quality scan.

The path branch now opens the file with a `with` block, so the handle is
closed on both the success and exception paths. The fix is scoped to only
that branch: a BytesIO created internally from bytes/bytearray/memoryview
input, and a file-like object supplied by the caller, are both closed
only after a successful read and left open if read() raises, exactly as
before this commit. Widening that to close a caller-supplied handle on
the exception path too would be a behavior change outside this fix's
scope.

Also closes tests/test_init.py's TestGetDmarcReportsFromMailboxMaildir
._deliver, which did open(source, "rb").read() with no close() at all;
it now uses a `with` block.

Two regression tests cover the narrowed contract:
- testParseReportFileClosesHandleOnReadError patches builtins.open (the
  SDK boundary) with a fake handle implementing the context-manager
  protocol, and asserts close() is called when read() raises. Verified
  against origin/master's version of parsedmarc/__init__.py: the test
  fails with "AssertionError: False is not true" (close was never
  called), confirming it catches the leak.
- testParseReportFileLeavesCallerHandleOpenOnReadError passes a
  caller-supplied fake handle directly as input_ and asserts close() is
  NOT called when read() raises. Verified against a naive whole-block
  try/finally (wrapping close() around all three branches instead of
  only the path branch): the test fails with "AssertionError: True is
  not false" (close was called), confirming it catches the scope
  widening this PR must avoid.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -11,6 +11,10 @@
 
 - `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.
 
+### Bug fixes
+
+- **`parse_report_file()` now closes the file handle it opens itself for a path input if reading it raises.** When `input_` is a path, the function opened the file, read it, and closed it with no exception handling in between; an exception raised by `read()` (e.g. an `OSError` from the underlying storage) skipped the close, so the descriptor was left to be released only when Python's garbage collector eventually finalized the object — CPython's `io.IOBase.__del__` closes an unclosed file on finalization (<https://docs.python.org/3/library/io.html>) — rather than being closed deterministically. This is the pattern CodeQL's `py/file-not-closed` query flags, found in a local code-quality scan. The path branch now opens the file with a `with` block, so the handle is closed on both the success and exception paths. A file-like object or bytes buffer supplied by the caller is unaffected: as before, it is closed only after a successful read, and left open if `read()` raises.
+
 ## 11.0.1
 
 ### Security

parsedmarc/__init__.py

@@ -2319,14 +2319,20 @@ def parse_report_file(
     if isinstance(input_, (str, os.PathLike)):
         file_path = os.fspath(input_)
         logger.debug(f"Parsing {file_path}")
-        file_object = open(file_path, "rb")
-    elif isinstance(input_, (bytes, bytearray, memoryview)):
-        file_object = BytesIO(bytes(input_))
+        # A path is a handle we opened ourselves, so it must be closed on
+        # both the success and exception paths.
+        with open(file_path, "rb") as file_object:
+            content = file_object.read()
     else:
-        file_object = input_
-
-    content = file_object.read()
-    file_object.close()
+        if isinstance(input_, (bytes, bytearray, memoryview)):
+            file_object = BytesIO(bytes(input_))
+        else:
+            # A caller-supplied file-like object is only closed on success,
+            # matching long-standing behavior; it is left open if read()
+            # raises.
+            file_object = input_
+        content = file_object.read()
+        file_object.close()
     if content.startswith(MAGIC_ZIP) or content.startswith(MAGIC_GZIP):
         content = extract_report(content)
 

tests/test_init.py

@@ -2754,6 +2754,83 @@ class TestParseReportFile(unittest.TestCase):
         finally:
             logger.setLevel(previous)
 
+    def testParseReportFileClosesHandleOnReadError(self):
+        """A file opened internally by parse_report_file is closed even
+        when reading it raises (the pattern CodeQL's py/file-not-closed
+        query flags, found in a local code-quality scan).
+
+        parse_report_file's path branch does `open(file_path, "rb")` and
+        then `.read()`; before the fix, `.close()` sat on the line after
+        `.read()` with no try/finally (and no `with`), so a read()-time
+        exception skipped close() and leaked the descriptor. Patch
+        builtins.open (the SDK boundary) to return a handle whose read()
+        raises, and assert the handle was still closed. The fake handle
+        implements the context-manager protocol itself (mirroring a real
+        file object's __exit__ calling close()), rather than relying on
+        MagicMock's default __enter__/__exit__, which would return a
+        different mock object from __enter__ and never call close().
+        """
+
+        class _RaisingHandle:
+            def __init__(self):
+                self.closed = False
+
+            def read(self):
+                raise OSError("boom")
+
+            def close(self):
+                self.closed = True
+
+            def __enter__(self):
+                return self
+
+            def __exit__(self, exc_type, exc, tb):
+                self.close()
+                return False
+
+        fake_handle = _RaisingHandle()
+
+        with patch("builtins.open", return_value=fake_handle) as mock_open:
+            with self.assertRaises(OSError):
+                parsedmarc.parse_report_file("some/path.xml", offline=True)
+
+        mock_open.assert_called_once_with("some/path.xml", "rb")
+        self.assertTrue(fake_handle.closed)
+
+    def testParseReportFileLeavesCallerHandleOpenOnReadError(self):
+        """A caller-supplied file-like object is left open (not closed)
+        when reading it raises, preserving parse_report_file's
+        long-standing contract for handles it did not open itself.
+
+        Only a path input (opened internally by parse_report_file) is
+        closed on the exception path; a handle the caller passed in is
+        closed on success only, exactly as before the fix for the
+        internally-opened-path leak.
+
+        A plain class is used instead of MagicMock because MagicMock
+        auto-implements ``__fspath__`` (supported since Python 3.8's
+        unittest.mock), which would make ``isinstance(fake, os.PathLike)``
+        true and route the object through the path branch instead of the
+        caller-supplied-object branch this test targets.
+        """
+
+        class _RaisingCallerHandle:
+            def __init__(self):
+                self.close_called = False
+
+            def read(self):
+                raise OSError("boom")
+
+            def close(self):
+                self.close_called = True
+
+        fake_handle = _RaisingCallerHandle()
+
+        with self.assertRaises(OSError):
+            parsedmarc.parse_report_file(cast(BinaryIO, fake_handle), offline=True)
+
+        self.assertFalse(fake_handle.close_called)
+
 
 class TestParseReportEmail(unittest.TestCase):
     """Tests for parse_report_email edge cases"""
@@ -3699,7 +3776,11 @@ class TestGetDmarcReportsFromMailboxMaildir(unittest.TestCase):
         self._inbox = mailbox.Maildir(self._maildir, create=True)
 
     def _deliver(self, source):
-        raw = open(source, "rb").read() if isinstance(source, str) else source
+        if isinstance(source, str):
+            with open(source, "rb") as f:
+                raw = f.read()
+        else:
+            raw = source
         self._inbox.add(mailbox.MaildirMessage(raw))
         self._inbox.flush()