fix: never close a caller-supplied file object in parse_report_file/extract_report (#909)

parse_report_file() and extract_report() both closed a file-like object
the caller passed in once they finished reading it (on the success path
only). A function must not close a handle it did not open: the caller
may still want to seek(0) and retry, log tell(), or reuse the handle.

- parse_report_file(): the caller-supplied-object branch no longer calls
  .close(). A path is still opened and closed via `with`; bytes are still
  wrapped in a function-owned BytesIO that the function closes.
- extract_report(): the seekable-stream branch previously aliased the
  caller's stream into `file_object` and then unconditionally closed it
  in `finally`, on the success path and on the exception path alike. An
  `owns_file_object` flag now tracks whether `file_object` is a buffer
  this function created (str/bytes input, or a copy made from a
  non-seekable caller stream) versus an alias of the caller's own
  seekable stream; only the former is closed.
- parse_aggregate_report_file() forwards its input straight to
  extract_report() and does no closing of its own, so it inherits the
  fix; its docstring now says so and points to extract_report()'s for the
  exact post-call position.

Both extract_report()'s and parse_report_file()'s docstrings now state
the contract explicitly. extract_report()'s docstring initially claimed
a successful call leaves a seekable caller stream "positioned at 0" and
described the seek(0) as seeking "back" to the caller's prior position;
neither is true. The unconditional stream.seek(0) after reading the
6-byte header runs regardless of where the caller's stream was
positioned, and the subsequent content read (member/gzip/text) advances
the stream again, so a successful call typically leaves it at EOF, not
0 (measured: XML 866, gzip 428, zip 901/974). The docstring now matches
parse_report_file()'s wording: left open, positioned wherever the
function's own reads left it.

Added testParseReportFileLeavesCallerHandleOpenOnSuccess,
testExtractReportLeavesSeekableCallerStreamOpen (success path), and
testExtractReportLeavesSeekableCallerStreamOpenOnError (exception path,
completing the "never closed on success or failure" claim, which had
gone untested on its exception half) -- asserting real BytesIO `.closed`
state and, for the success cases, that the handle is still readable
after seek(0). All three fail against the pre-fix code; verified with
`parsedmarc.__file__` that the import resolved to this worktree's
package, not a stale venv install, before trusting the failure/pass
results. Updated testParseReportFileLeavesCallerHandleOpenOnReadError's
docstring, which described the old "closed on success" behavior as
intentional.

Also added testParseAggregateReportFileLeavesCallerStreamOpen: the
contract paragraph added to parse_aggregate_report_file()'s docstring
was previously untested through that entry point -- its existing
stream-based tests only exercise extract_report() and parse_report_file()
directly, while parse_aggregate_report_file() itself was only ever
called with bytes. The new test passes a caller-owned BytesIO through
parse_aggregate_report_file() and asserts it is still open after both a
successful parse and an InvalidAggregateReport raised on garbage
content. It fails against the pre-fix code (AssertionError: True is not
false on the success-path assertion); verified via `parsedmarc.__file__`
that the import resolved to this worktree's package before trusting the
result.

CHANGELOG: added an Unreleased/Changes entry (behavior change for
library callers, naming all three affected functions) and corrected the
existing Unreleased/Bug fixes entry for parse_report_file's path-close
fix, whose closing sentence about caller-supplied objects this PR makes
false and whose cross-reference to "the Changes section below" pointed
the wrong direction (Changes is above Bug fixes).

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

CHANGELOG.md

@@ -5,12 +5,13 @@
 ### Changes
 
 - **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.
+- **`parse_report_file()`, `extract_report()`, and `parse_aggregate_report_file()` no longer close a file object supplied by the caller, on success or failure.** A path input is still opened and closed by the function, and a `bytes`/`bytearray`/`memoryview` input is still wrapped in an internally-created `BytesIO` that the function closes; but a file-like object the caller passed in may be seeked, `tell()`'d, or reused afterward, and closing it out from under the caller (as these functions previously did on the success path) makes that impossible. `parse_aggregate_report_file()` forwards its input straight to `extract_report()`, so it inherits the fix without changes of its own. `extract_report()`'s handling of a caller-supplied *non-seekable* stream is unaffected: its contents are still copied into an internally-owned buffer that the function closes, since that buffer was never the caller's own handle.
 - **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.
-- **`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.
+- **`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 caller-supplied file-like object's closing behavior changed again later in this same Unreleased version — see the *Changes* section above.)
 - **A SIGHUP configuration reload no longer breaks every subsequent save to Elasticsearch or OpenSearch.** With `[elasticsearch]` or `[opensearch]` configured in watch mode, reloading the configuration re-registered the search client under the client library's `default` connection alias and then closed the previous run's clients. The close step re-resolved that alias instead of remembering the client it was created for, so it closed the *newly built* client and deleted the `default` alias outright — after which every report save failed with `KeyError: "There is no connection with alias 'default'."` until parsedmarc was restarted, and the original client was left open. Each backend's handle now closes the exact client it was created for and gives up the alias only while the alias still points at that client. Both the Elasticsearch and OpenSearch backends were affected.
 - **A configuration reload that fails part-way no longer leaves reports being written to the new Elasticsearch or OpenSearch hosts under the old configuration.** The search client is registered under the client library's process-wide `default` connection alias as soon as it is constructed — before the index migration runs, and before the outputs configured after it are created. When a later step then failed on a SIGHUP reload — for example an `[opensearch]` section that cannot build its client (an unsupported `auth_type`, `awssigv4` without an `aws_region`, AWS credentials that will not load) — parsedmarc logged `Config reload failed, continuing with previous config` and kept the old configuration. But the alias had already been handed to the new client, so every subsequent save resolved it to the *new* hosts while the index prefixes, suffixes, and `index_prefix_domain_map` still came from the old configuration. Reports were silently written to a destination that was never successfully configured, with the log saying nothing had changed. The half-built client was never closed either, nor were the other clients (S3, Kafka, PostgreSQL, and so on) built earlier in the same failed reload — the same leak occurred on every attempt of the startup retry loop, which calls the same function. Building the output clients is now all-or-nothing: if any step fails, everything built so far is closed and the module-level state the search backends keep is put back — each configured backend's `default` alias restored to exactly the client it named beforehand, and the Elasticsearch `serverless` flag (which decides whether `number_of_shards`/`number_of_replicas` are sent when an index is created) to its old value — so a failed reload no longer changes where reports are written, or how indexes are created.
 

parsedmarc/__init__.py

@@ -1223,6 +1223,16 @@ def extract_report(content: bytes | str | BinaryIO) -> str:
     Extracts report text from zip- or gzip-compressed content, and returns
     plain XML or JSON content decoded as-is.
 
+    A caller-supplied file-like object is read from but never closed by
+    this function, on either the success or the exception path; it is
+    left open and positioned wherever the function's own reads left it,
+    so the caller is free to seek(0) and reuse it. A seekable stream is
+    seeked to position 0 unconditionally (not back to wherever the caller
+    had left it) after its 6-byte header is sniffed, and then read from
+    there, so a successful call typically leaves it at EOF. A non-seekable
+    stream is drained into a buffer this function creates and closes
+    itself; the caller's stream is left open but exhausted.
+
     Args:
         content: The report as a base64-encoded string, file-like object,
             or bytes. A string that is not valid base64 is returned
@@ -1232,6 +1242,12 @@ def extract_report(content: bytes | str | BinaryIO) -> str:
         str: The extracted text
     """
     file_object: BinaryIO | None = None
+    # True while file_object is a BytesIO this function created itself
+    # (from a str/bytes input, or as a buffer copied from a non-seekable
+    # caller stream); it is closed in the ``finally`` below. It is set to
+    # False when file_object instead aliases a caller-supplied seekable
+    # stream, which this function must never close.
+    owns_file_object = True
     header: bytes
     try:
         if isinstance(content, str):
@@ -1265,7 +1281,10 @@ def extract_report(content: bytes | str | BinaryIO) -> str:
                     raise ParserError("File objects must be opened in binary (rb) mode")
                 header = bytes(header_raw)
                 stream.seek(0)
+                # file_object aliases the caller's own stream here, so it
+                # must not be closed by this function.
                 file_object = stream
+                owns_file_object = False
             else:
                 header_raw = stream.read(6)
                 if isinstance(header_raw, str):
@@ -1299,7 +1318,7 @@ def extract_report(content: bytes | str | BinaryIO) -> str:
             f"Invalid archive file: {error.__str__()}{_exc_origin(error)}"
         ) from error
     finally:
-        if file_object:
+        if file_object and owns_file_object:
             try:
                 file_object.close()
             except Exception:
@@ -1340,6 +1359,11 @@ def parse_aggregate_report_file(
     """Parses a file at the given path, a file-like object, or bytes as an
     aggregate DMARC report
 
+    ``_input`` is forwarded to ``extract_report()`` unchanged, so a
+    caller-supplied file-like object is read from but never closed, on
+    either the success or the exception path; see ``extract_report()``
+    for exactly how such an object is left positioned afterward.
+
     Args:
         _input (str | bytes | IO): A path to a file, a file-like object, or bytes
         offline (bool): Do not query online for geolocation or DNS
@@ -2283,6 +2307,11 @@ def parse_report_file(
     """Parses a DMARC aggregate report, DMARC failure report, or SMTP TLS
     report from a file at the given path, a file-like object, or bytes
 
+    A path is opened and closed by this function. A caller-supplied file
+    object is read from but never closed, on either the success or the
+    exception path; it is left open and positioned wherever its own
+    ``read()`` left it, so the caller is free to ``seek(0)`` and reuse it.
+
     Args:
         input_ (str | os.PathLike | bytes | BinaryIO): A path to a file,
             a file-like object, or bytes
@@ -2332,14 +2361,18 @@ def parse_report_file(
             content = file_object.read()
     else:
         if isinstance(input_, (bytes, bytearray, memoryview)):
+            # The BytesIO wrapper is created here, so this function owns
+            # it and closes it once the bytes have been read out.
             file_object = BytesIO(bytes(input_))
+            content = file_object.read()
+            file_object.close()
         else:
-            # A caller-supplied file-like object is only closed on success,
-            # matching long-standing behavior; it is left open if read()
-            # raises.
+            # A caller-supplied file-like object is never closed by this
+            # function, on success or failure: the caller may want to
+            # seek(0) and retry, inspect tell(), or otherwise reuse the
+            # handle afterward.
             file_object = input_
-        content = file_object.read()
-        file_object.close()
+            content = file_object.read()
     if content.startswith(MAGIC_ZIP) or content.startswith(MAGIC_GZIP):
         content = extract_report(content)
 

tests/test_init.py

@@ -2083,6 +2083,39 @@ class Test(unittest.TestCase):
         self.assertEqual(report["policy_published"]["domain"], "example.com")
         print("Passed!")
 
+    def testParseAggregateReportFileLeavesCallerStreamOpen(self):
+        """A caller-supplied BytesIO passed to parse_aggregate_report_file
+        is left open (never closed), both on a successful parse and when
+        unrecognized content raises InvalidAggregateReport.
+
+        parse_aggregate_report_file() forwards ``_input`` straight to
+        extract_report() (see both functions' docstrings), so it inherits
+        extract_report()'s contract of never closing a caller-supplied
+        file-like object, on either the success or the exception path.
+        This is a regression test for that contract reached through the
+        public parse_aggregate_report_file() entry point -- extract_report()
+        and parse_report_file() are covered by their own direct tests, but
+        neither exercises parse_aggregate_report_file() with a stream, only
+        with bytes (see testParseAggregateReportFile above). A real BytesIO
+        is used, not a mock, so the assertion is on real observable state
+        (``closed``), and because MagicMock auto-implements ``__fspath__``.
+        """
+        sample_path = "samples/aggregate/rfc9990-sample.xml"
+        with open(sample_path, "rb") as f:
+            data = f.read()
+
+        success_stream = BytesIO(data)
+        report = parsedmarc.parse_aggregate_report_file(
+            success_stream, offline=True, always_use_local_files=True
+        )
+        self.assertEqual(report["report_metadata"]["org_name"], "Sample Reporter")
+        self.assertFalse(success_stream.closed)
+
+        garbage_stream = BytesIO(b"this is not a valid report")
+        with self.assertRaises(parsedmarc.InvalidAggregateReport):
+            parsedmarc.parse_aggregate_report_file(garbage_stream, offline=True)
+        self.assertFalse(garbage_stream.closed)
+
     def testParseInvalidAggregateSample(self):
         """Test invalid aggregate samples are handled"""
         print()
@@ -2216,6 +2249,47 @@ class TestExtractReport(unittest.TestCase):
         result = parsedmarc.extract_report(bio)
         self.assertIn("<feedback>", result)
 
+    def testExtractReportLeavesSeekableCallerStreamOpen(self):
+        """A caller-supplied seekable stream is left open after a
+        successful call, and remains usable afterward.
+
+        Before this fix, extract_report's seekable-stream branch set
+        file_object = stream (aliasing the caller's own handle) and then
+        unconditionally closed file_object in its `finally` block on
+        every path, success included -- closing a handle it did not open.
+        This asserts the real observable state of a real BytesIO handle
+        (``closed`` and a post-seek re-read), not a mock's bookkeeping.
+        """
+        xml = b'<?xml version="1.0"?><feedback></feedback>'
+        bio = BytesIO(xml)
+
+        result = parsedmarc.extract_report(bio)
+
+        self.assertIn("<feedback>", result)
+        self.assertFalse(bio.closed)
+        bio.seek(0)
+        self.assertEqual(bio.read(), xml)
+
+    def testExtractReportLeavesSeekableCallerStreamOpenOnError(self):
+        """A caller-supplied seekable stream is left open when
+        extract_report raises ParserError, the other half of the "never
+        closed on success or failure" claim above.
+
+        Before this fix, extract_report's seekable-stream branch aliased
+        file_object to the caller's own stream and its `finally` block
+        closed file_object unconditionally -- including on this
+        exception path, since content matching no known format still
+        reaches the header sniff, `stream.seek(0)`, and the `finally`
+        close before raising. Uses a real BytesIO so the assertion is on
+        its own observable ``closed`` state, not a mock's bookkeeping.
+        """
+        bio = BytesIO(b"this is not a valid archive")
+
+        with self.assertRaises(parsedmarc.ParserError):
+            parsedmarc.extract_report(bio)
+
+        self.assertFalse(bio.closed)
+
     def testExtractReportFromNonSeekableStream(self):
         """extract_report handles non-seekable streams"""
         xml = b'<?xml version="1.0"?><feedback></feedback>'
@@ -2799,13 +2873,13 @@ class TestParseReportFile(unittest.TestCase):
 
     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.
+        when reading it raises.
 
         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.
+        never closed by parse_report_file, on success or failure -- the
+        caller may want to seek(0) and retry, log tell(), or reuse the
+        handle otherwise.
 
         A plain class is used instead of MagicMock because MagicMock
         auto-implements ``__fspath__`` (supported since Python 3.8's
@@ -2831,6 +2905,30 @@ class TestParseReportFile(unittest.TestCase):
 
         self.assertFalse(fake_handle.close_called)
 
+    def testParseReportFileLeavesCallerHandleOpenOnSuccess(self):
+        """A caller-supplied file-like object is left open after a
+        successful parse, and remains usable afterward.
+
+        Before this fix, parse_report_file called .close() on any
+        caller-supplied file object once it had been read, on the success
+        path only. A function must not close a handle it did not open --
+        the caller may still want to seek(0) and re-read it, inspect
+        tell(), or otherwise reuse it. This asserts the real observable
+        state of a real BytesIO handle (``closed`` and post-seek
+        ``read()``), not a mock's call-tracking.
+        """
+        xml_path = "samples/aggregate/!example.com!1538204542!1538463818.xml"
+        with open(xml_path, "rb") as f:
+            data = f.read()
+        handle = BytesIO(data)
+
+        result = parsedmarc.parse_report_file(handle, offline=True)
+
+        self.assertEqual(result["report_type"], "aggregate")
+        self.assertFalse(handle.closed)
+        handle.seek(0)
+        self.assertEqual(handle.read(), data)
+
 
 class TestParseReportEmail(unittest.TestCase):
     """Tests for parse_report_email edge cases"""