From 429860371386f3ed99aa33a8649ac808b92d3c2b Mon Sep 17 00:00:00 2001 From: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Date: Wed, 9 Sep 2026 18:44:15 -0400 Subject: [PATCH] Fix CodeQL findings: use sys.exit(), remove dead index_date stores (#898) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit py/use-of-exit-or-quit (29 sites): replace bare `exit(...)` calls with `sys.exit(...)` in cli.py (20 sites), sortlists.py (4 sites), and find_unknown_base_reverse_dns.py (5 sites). `exit` is injected into the interactive namespace by the `site` module, so it is not guaranteed to exist under `python -S` or an embedded interpreter — the very error paths that called it would raise NameError instead of exiting cleanly. `sys.exit` is always available and was already used elsewhere in these files. sortlists.py did not import `sys`; the import was added to its existing stdlib import block. find_unknown_base_reverse_dns.py already imported `sys` (used at line 71), so only the call sites changed there. Checked tests/test_cli.py for anything patching `exit` directly — none do; the SystemExit-based assertions exercise the same behavior for either spelling, so no test changes were needed. py/multiple-definition (4 sites): removed the dead `index_date` computation in save_aggregate_report_to_elasticsearch() (elastic.py:944-947) and save_aggregate_report_to_opensearch() (opensearch.py:875-878). Both blocks derived index_date from the report-level begin_date, but the per-record loop immediately below unconditionally recomputes index_date from each record's own begin_date before the only read of the variable (the index name build near the end of the loop body). Verified by reading both functions that no code between the outer assignment and the loop's recompute reads index_date, and that the recompute is unconditional on every path that reaches the read, before deleting. Co-authored-by: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + parsedmarc/cli.py | 46 +++++++++---------- parsedmarc/elastic.py | 5 -- parsedmarc/opensearch.py | 5 -- .../maps/find_unknown_base_reverse_dns.py | 10 ++-- parsedmarc/resources/maps/sortlists.py | 9 ++-- 6 files changed, 34 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2720d72..a516e4a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ ### 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. +- **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. ## 11.0.1 diff --git a/parsedmarc/cli.py b/parsedmarc/cli.py index 5a670e7d..8a19f028 100644 --- a/parsedmarc/cli.py +++ b/parsedmarc/cli.py @@ -231,7 +231,7 @@ def _log_msgraph_failure( fetch, send, or watch failure, identifying the mailbox/tenant/auth method and the Graph request-id/client-request-id when available. The full traceback is preserved at --debug via a follow-up DEBUG - record. Never calls exit() - the call site keeps its own exit(1).""" + record. Never calls sys.exit() - the call site keeps its own sys.exit(1).""" if isinstance(error, APIError): detail = getattr(error, "primary_message", None) or error.message or str(error) detail = " ".join(str(detail).split()) @@ -2766,7 +2766,7 @@ def _main(): index_prefix_domain_map = _parse_config(config, opts) except ConfigurationError as e: logger.critical(str(e)) - exit(-1) + sys.exit(-1) logger.setLevel(logging.ERROR) @@ -2798,7 +2798,7 @@ def _main(): and len(opts.file_path) == 0 ): logger.error("You must supply input files or a mailbox connection") - exit(1) + sys.exit(1) logger.info("Starting parsedmarc") @@ -2814,7 +2814,7 @@ def _main(): configure_ipinfo_api(opts.ipinfo_api_token) except InvalidIPinfoAPIKey as e: logger.critical(str(e)) - exit(1) + sys.exit(1) load_psl_overrides( always_use_local_file=opts.always_use_local_files, @@ -2835,7 +2835,7 @@ def _main(): break except ConfigurationError as e: logger.critical(str(e)) - exit(1) + sys.exit(1) except Exception as error_: if attempt < max_retries: logger.warning( @@ -2849,10 +2849,10 @@ def _main(): retry_delay *= 2 else: logger.error(f"Output client error: {error_}") - exit(1) + sys.exit(1) # Always close output clients on the way out (normal return, - # exit(N), uncaught exception, or SystemExit from a signal-driven + # sys.exit(N), uncaught exception, or SystemExit from a signal-driven # shutdown). atexit does NOT fire on os._exit(130) — that's # intentional for the SIGINT double-tap. The lambda closes whatever # `clients` currently points at, so a SIGHUP reload that swaps the @@ -2992,7 +2992,7 @@ def _main(): logger.error( "IMAP user and password must be specified if host is specified" ) - exit(1) + sys.exit(1) ssl = True verify = True @@ -3022,7 +3022,7 @@ def _main(): except Exception: logger.exception("IMAP Error") - exit(1) + sys.exit(1) if opts.graph_client_id: try: @@ -3088,10 +3088,10 @@ def _main(): tenant_id=opts.graph_tenant_id, auth_method=opts.graph_auth_method, ) - exit(1) + sys.exit(1) except Exception: logger.exception("MS Graph Error") - exit(1) + sys.exit(1) if opts.gmail_api_credentials_file: # Any effective delete flag needs the deletion scope: the per-report-type @@ -3135,7 +3135,7 @@ def _main(): except Exception: logger.exception("Gmail API Error") - exit(1) + sys.exit(1) if opts.maildir_path: try: @@ -3145,7 +3145,7 @@ def _main(): ) except Exception: logger.exception("Maildir Error") - exit(1) + sys.exit(1) if mailbox_connection: mailbox_batch_size_value = ( @@ -3219,10 +3219,10 @@ def _main(): tenant_id=opts.graph_tenant_id, auth_method=opts.graph_auth_method, ) - exit(1) + sys.exit(1) except Exception: logger.exception("Mailbox Error") - exit(1) + sys.exit(1) # Filtered here rather than relying on process_reports()'s in-place # filtering: the dicts it filters are the file snapshot and the mailbox @@ -3286,7 +3286,7 @@ def _main(): ) except Exception: logger.exception("Failed to email results") - exit(1) + sys.exit(1) elif msgraph_connection is not None and smtp_to_value: try: email_results_via_msgraph( @@ -3305,10 +3305,10 @@ def _main(): tenant_id=opts.graph_tenant_id, auth_method=opts.graph_auth_method, ) - exit(1) + sys.exit(1) except Exception: logger.exception("Failed to email results via Microsoft Graph") - exit(1) + sys.exit(1) if mailbox_connection and opts.mailbox_watch: logger.info("Watching for email - Ctrl-C once to quit, twice to force") @@ -3346,10 +3346,10 @@ def _main(): ) except FileExistsError as error: logger.error(f"{error.__str__()}") - exit(1) + sys.exit(1) except ParserError as error: logger.error(error.__str__()) - exit(1) + sys.exit(1) except (ClientAuthenticationError, APIError, httpx.HTTPError) as error: if msgraph_connection is None: logger.exception("Mailbox Error") @@ -3361,7 +3361,7 @@ def _main(): tenant_id=opts.graph_tenant_id, auth_method=opts.graph_auth_method, ) - exit(1) + sys.exit(1) # Prioritize shutdown over reload if both flags are set (e.g. # SIGHUP followed by SIGTERM). atexit closes output clients. @@ -3426,7 +3426,7 @@ def _main(): ) except InvalidIPinfoAPIKey as e: logger.critical(str(e)) - exit(1) + sys.exit(1) for k, v in vars(new_opts).items(): setattr(opts, k, v) @@ -3492,7 +3492,7 @@ def _main(): # Close output clients on the success path (one-shot or graceful # watch-loop exit). atexit-registered above is the safety net for - # exit(1) / uncaught-exception paths. + # sys.exit(1) / uncaught-exception paths. _close_output_clients(clients) diff --git a/parsedmarc/elastic.py b/parsedmarc/elastic.py index df717b45..1da12d90 100644 --- a/parsedmarc/elastic.py +++ b/parsedmarc/elastic.py @@ -941,11 +941,6 @@ def save_aggregate_report_to_elasticsearch( begin_date = human_timestamp_to_datetime(metadata["begin_date"], to_utc=True) end_date = human_timestamp_to_datetime(metadata["end_date"], to_utc=True) - if monthly_indexes: - index_date = begin_date.strftime("%Y-%m") - else: - index_date = begin_date.strftime("%Y-%m-%d") - org_name_query = Q(dict(match_phrase=dict(org_name=org_name))) # type: ignore report_id_query = Q(dict(match_phrase=dict(report_id=report_id))) # pyright: ignore[reportArgumentType] domain_query = Q(dict(match_phrase={"published_policy.domain": domain})) # pyright: ignore[reportArgumentType] diff --git a/parsedmarc/opensearch.py b/parsedmarc/opensearch.py index 8232fa57..eca65acf 100644 --- a/parsedmarc/opensearch.py +++ b/parsedmarc/opensearch.py @@ -872,11 +872,6 @@ def save_aggregate_report_to_opensearch( begin_date = human_timestamp_to_datetime(metadata["begin_date"], to_utc=True) end_date = human_timestamp_to_datetime(metadata["end_date"], to_utc=True) - if monthly_indexes: - index_date = begin_date.strftime("%Y-%m") - else: - index_date = begin_date.strftime("%Y-%m-%d") - org_name_query = Q(dict(match_phrase=dict(org_name=org_name))) report_id_query = Q(dict(match_phrase=dict(report_id=report_id))) domain_query = Q(dict(match_phrase={"published_policy.domain": domain})) diff --git a/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py b/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py index 9b7ac705..9d754677 100755 --- a/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py +++ b/parsedmarc/resources/maps/find_unknown_base_reverse_dns.py @@ -174,7 +174,7 @@ def _main(): domain = line.lower().strip() if domain in list_var: print(f"Error: {domain} is in {file_path} multiple times") - exit(1) + sys.exit(1) elif domain != "": list_var.append(domain) @@ -182,7 +182,7 @@ def _main(): load_list(psl_overrides_file_path, psl_overrides) if not os.path.exists(mmdb_file_path): print(f"Error: {mmdb_file_path} does not exist") - exit(1) + sys.exit(1) print(f"Loading {mmdb_file_path}") as_name_index = _load_as_name_index(mmdb_file_path) print(f"Indexed {len(as_name_index)} as_names from the MMDB") @@ -196,7 +196,7 @@ def _main(): print( f"Error: {domain} is in {base_reverse_dns_map_file_path} multiple times" ) - exit() + sys.exit(1) else: known_domains.append(domain) if domain in known_unknown_domains and known_domains: @@ -204,10 +204,10 @@ def _main(): f"Error:{domain} is in {known_unknown_list_file_path} and \ {base_reverse_dns_map_file_path}" ) - exit(1) + sys.exit(1) if not os.path.exists(args.input): print(f"Error: {args.input} does not exist") - exit(1) + sys.exit(1) for row in _read_input_rows(args.input): domain = row["source_name"].lower().strip() if domain == "": diff --git a/parsedmarc/resources/maps/sortlists.py b/parsedmarc/resources/maps/sortlists.py index c08ebad6..6e1d685b 100755 --- a/parsedmarc/resources/maps/sortlists.py +++ b/parsedmarc/resources/maps/sortlists.py @@ -5,6 +5,7 @@ from __future__ import annotations import os import csv import re +import sys from pathlib import Path from collections.abc import Mapping, Iterable, Collection @@ -243,23 +244,23 @@ def _main(): if not os.path.exists(readme_file): print(f"Error: {readme_file} does not exist") - exit(1) + sys.exit(1) try: types = normalize_types_in_readme(readme_file) except ValueError as e: print(f"Error: {e}") - exit(1) + sys.exit(1) map_allowed_values = {"type": types} for list_file in list_files: if not os.path.exists(list_file): print(f"Error: {list_file} does not exist") - exit(1) + sys.exit(1) sort_list_file(list_file) if not os.path.exists(map_file): print(f"Error: {map_file} does not exist") - exit(1) + sys.exit(1) try: sort_csv( map_file,