4 Commits

Include renames

Author	SHA1	Message	Date
3 people	02fcfd6e82	Merge commit from fork ... * Evict expired dedup-cache entries on insert and hash the key (GHSA-9fw6-mwqr-p43m) The seen-aggregate-report cache is an expiringdict.ExpiringDict, which removes an expired entry only when that same key is read again, or drops its oldest entry once it holds max_len (100 million) entries. A report that is never received again is never read again, so in watch mode every distinct aggregate report stayed in memory for the life of the process. The key was the JSON of the sender-controlled reporter, Report-ID and policy domain, so ten 5.6 KB attachments with a 5 MiB report_id held 64 MiB indefinitely. SeenReportCache (parsedmarc/config.py) subclasses ExpiringDict and removes expired entries from the head of its insertion order on every insert; the cache's max_len is now 100,000. _aggregate_report_dedup_key() now returns the SHA-256 hex digest of that JSON, so a key is 64 characters whatever the report contains. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * Keep a re-added dedup-cache key at the tail so the expiry sweep is not blocked OrderedDict refreshes a re-set key's time in place, so a key added again while still stored kept its old position at the head and stopped the insert-time sweep before the expired entries behind it. SeenReportCache now moves such a key to the tail before storing it, keeping insertion order equal to age order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Bound failed reverse-DNS lookups to 120 seconds per aggregate report (GHSA-rwfh-ww9c-jm9m) Every record of an aggregate report made one blocking PTR lookup of a sender-chosen source IP, with no bound on count or time; an unanswering reverse zone costs the full 8 s DNS lifetime with the default nameservers, and a weak-fallback result is never cached, so one IP repeated in every record was looked up every time. A 3,500-record report could block processing for hours. Lookups that return no PTR now share a 120-second budget per report; once it is spent, the remaining records are enriched from the local IP database only, their results are not cached, and one warning is logged. Each distinct source IP is looked up at most once per report, and lookups that find a PTR are not charged, so large legitimate reports keep their enrichment. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Keep values PostgreSQL rejects out of its output, and let one rejected report not hold the batch (GHSA-fxr7-7h9q-v949) An aggregate count above 2**31 - 1, an SMTP TLS sending-mta-ip or receiving-ip that is not an IP address, and a NUL in a failure report's sample each made the PostgreSQL save raise PostgreSQLError, which marked the whole mailbox batch unsaved, so legitimate reports from other senders were never archived. The parser now skips a record whose count is outside 0..MAX_MESSAGE_COUNT and rejects such an SMTP TLS session count (closing the same rejection in Elasticsearch/OpenSearch), drops an invalid TLS IP with a warning, and the PostgreSQL output replaces NUL with U+FFFD in text, text array and JSONB values. Any other psycopg.DataError raises the new PostgreSQLRejectedReport, which the CLI logs and skips for that one report unless fail_on_output_error is set. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Check an aggregate record's count before its reverse-DNS lookup A record whose count is outside 0..MAX_MESSAGE_COUNT is dropped, so its source IP is no longer looked up first, which could cost up to 8 seconds per record. A regression test mocks dnspython and asserts that the dropped record's address is never queried. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Require TLS for the PostgreSQL output by default and add [postgresql] sslmode and sslrootcert (GHSA-xvp7-7762-gx9g) PostgreSQLClient connected without an sslmode, so libpq used its default, prefer: when the server, or an on-path attacker, declined TLS, it continued over plain TCP and answered a clear-text password request. The client now passes sslmode=require unless the new sslmode argument, an sslmode in the connection string, or PGSSLMODE chooses a mode, and sslrootcert names the CA certificate for verify-ca and verify-full. A warning is logged at startup when a TCP host other than localhost or a loopback address is used with a mode that does not verify the server certificate. A malformed connection string is now rejected by a parse before connecting, with the same withheld-details error. A server without TLS fails with libpq's "server does not support SSL, but SSL was required" until sslmode is set to disable or prefer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Withhold the hosts URL from Elasticsearch and OpenSearch client-build errors (GHSA-74xv-rmpx-2pjm) A password with an unencoded "/", "?" or "#" in a [elasticsearch] or [opensearch] hosts URL makes elastic_transport raise "Could not parse URL '<url>'" and opensearch-py (via urllib.parse) raise "Port could not be cast to integer value as '<password prefix>'". The startup wrapper repeated that text at ERROR without --debug and chained the original, which the SIGHUP reload's logger.exception printed again. set_hosts() now runs through _set_search_hosts(), which replaces any ValueError with a fixed message raised outside the except block. Other errors in the block keep their text with URL credentials masked and are raised from None. This completes the GHSA-hjr3-rx4c-24cv fix. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Cache the downloaded IP database in a private per-user directory and validate it (GHSA-5f56-8ph8-v9q5) load_ip_db() cached the IPinfo Lite download at the fixed, shared <tmp>/parsedmarc/ipinfo_lite.mmdb, where another local user could plant a symlink to have the download overwrite a file the parsedmarc user can write, or plant a database that was used whenever the download failed. The cache now lives in $XDG_CACHE_HOME/parsedmarc or ~/.cache/parsedmarc (%LOCALAPPDATA%\parsedmarc\cache on Windows), created 0700 and used only if it is a real directory the user owns and can write that is not group- or world-writable; otherwise a per-run mkdtemp() directory is used. The download is written with mkstemp(), opened with maxminddb to validate it, and moved into place with os.replace(). A cached copy is used only if it is a regular file owned by the user. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Skip reports over the Logs Ingestion API size limit and publish the Log Analytics streams independently (GHSA-rg4h-58fm-8pc9) The Azure Monitor ingestion SDK packs whole reports into chunks of up to 1 MiB and never splits one, so a report whose JSON is larger is sent as a call of its own, and the Logs Ingestion API rejects any call over 1 MB (https://learn.microsoft.com/en-us/azure/azure-monitor/fundamentals/service-limits). The rejected upload raised before the failure and SMTP TLS streams were sent, and the CLI held the whole batch for retry and finally Unsaved. publish_json() now measures each report as the SDK does and leaves out one over 1,000,000 bytes with a warning naming it, and publish_results() publishes each stream in its own try and raises one LogAnalyticsException naming every stream that failed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Address review: redact the PostgreSQL warning host, drop the connect error chain, and fix Log Analytics and docs wording libpq ends a URI's userinfo at the first "@", so the rest of a password containing "@" becomes part of the host. The PostgreSQL startup warning now credential-masks every host-like value it logs (host list entries, hostaddr, PGHOST, PGHOSTADDR), checks hostaddr before host as libpq does, and the general connect error is raised from None so the driver's unredacted text no longer reaches a logged traceback. The CLI's output-client Raises docs now say which outputs do not chain. publish_json() returns the number of reports uploaded, and publish_results() logs "Successfully pushed" only for a stream that uploaded something. The [postgresql] URI example uses the Unix socket and explains that TCP to localhost needs TLS on the server or sslmode = disable, the [log_analytics] docs note the SDK's 1 MiB call packing, the reverse-DNS-budget wording says "local IP database and reverse DNS map" everywhere, and the GHSA-fxr7 entry no longer claims the parser rejects every unreadable start-datetime. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Require mailsuite 2.3.7 mailsuite 2.3.7 fixes parse_email() keeping a CRLF message's carriage returns in address header values (seanthegeek/mailsuite#74). The floor is raised in the base dependency and the msgraph, gmail and all extras, and the usage docs' stated requirement is updated to match. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * Drop Python 3.10 and add Python 3.15 to the supported versions and CI (#953) Python 3.10 reaches end of life in October 2026. requires-python is now >=3.11, the unused requires_python key under [build-system] (not read by hatchling, and its <3.15 bound read as excluding 3.15) is removed, and Python 3.15 joins the CI matrix, classifiers, and support tables. Docs, AGENTS.md, and comments that named 3.10 as the floor are updated. Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> * Keep aggregate identifiers and reasons within their schema (#950) * Keep aggregate identifiers and reasons within their schema AI assistance: OpenAI Codex. * Expand the #948 CHANGELOG entry and tighten its tests RFC 9990 Appendix A and RFC 7489 Appendix C define only header_from, envelope_from and envelope_to under identifiers, and only type and comment in a reason. Cite them in the test, check that envelope_from and envelope_to survive next to a dropped nested unknown element, and cover an unknown key on the legacy identities path. The CHANGELOG entry now names the affected outputs and the compatibility change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Sean Whalen <44679+seanthegeek@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> * Reject reports dated in the future and bound what parse_report_file() reads (#954) * Reject reports whose index-naming date is more than two days in the future (#947) The date that names a report's dated Elasticsearch/OpenSearch index (an aggregate report's begin date, a failure report's Arrival-Date, an SMTP TLS report's start-datetime) was never compared with the present, so a report dated 2100 named an index for 2100. Each parser now rejects such a report with its own Invalid*Report; past dates stay unbounded. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Refuse an input larger than the report size cap before parse_report_file() reads it (#949) The 32 MiB cap was applied only after the whole input was read. A path is now checked with os.path.getsize(), bytes-like input with len(), and a file object is read at most one byte past the cap. The error is the ParserError with the message the content check already gives. Three existing tests that fed over-cap content now see the earlier rejection. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Measure a memoryview input by its byte size len() of a memoryview counts items, which are not bytes for every format, so the size cap in parse_report_file() compares nbytes instead. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * Bound the aggregate end date and read file objects to their end Address the Copilot review of #954. A normalized report's per-day records name their indexes from their own interval starts, so the report's end date is bounded like its begin date. A raw stream may return fewer bytes than asked for before its end, so parse_report_file() now reads in a loop until end of file or one byte past the cap. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * Accumulate bounded reads in one buffer Address the second Copilot review of #954. Keeping every chunk in a list cost one bytes object per read, about 85 bytes per byte for a stream that returns a byte at a time (22.7 MiB for 256 KiB of input). One bytearray keeps memory proportional to the bytes read. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> * Skip a message with an invalid report attachment instead of aborting the mbox (#955) * Skip a message with an invalid report attachment instead of aborting the mbox parse_report_email() raised a bare ParserError when a report attachment was invalid, which get_dmarc_reports_from_mbox() re-raises, so one such message ended the whole file. Raise InvalidDMARCReport (a ParserError subclass), which the mbox loop skips with a warning. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> * Skip an invalid SMTP TLS attachment in an mbox too Address the Copilot review of #955. An invalid SMTP TLS attachment raised InvalidSMTPTLSReport, which is not an InvalidDMARCReport, so it still stopped the mbox. parse_report_email() now names the message's subject in that error, and both mbox loops skip it with a warning. A message that cannot be parsed at all still raises a plain ParserError. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> * Address the second review: refuse an unencoded @ in a PostgreSQL URI, charge slow reverse-DNS lookups, and smaller fixes - PostgreSQL: a connection string whose parsed host, hostaddr, port or dbname holds an "@" (the rest of a URI password with an unencoded "@", which libpq splits at the first "@") is refused with a fixed error before any warning is logged or any connection is attempted; _connect parses the string on every path. A leading "@" in a host entry (Linux abstract socket) is still allowed. _log_safe_host stays as defence in depth for the host argument and PGHOST/PGHOSTADDR. - Log Analytics: publish_json catches AzureError, so a ServiceRequestError or ServiceResponseError no longer stops the remaining streams. - MAX_MESSAGE_COUNT comment and CHANGELOG: the bound matches PostgreSQL's INTEGER columns and the Elasticsearch/OpenSearch integer mapping of the SMTP TLS session counts; the aggregate message_count is dynamically mapped as long. - Reverse DNS budget: a lookup taking at least SLOW_REVERSE_DNS_SECONDS (1.0) is charged whether or not it returned a name; the warning now says "failed or were slow". - An IPv6 zone ID in an aggregate record's source IP skips the record before any DNS lookup, and in a failure report's Source-IP raises InvalidFailureReport; one helper serves these and the SMTP TLS path. - IP database cache: a relative candidate directory (HOME unset, no passwd entry) is rejected; offline / always_use_local_file creates no directory and no temporary directory and logs no "downloaded again". - No startup warning for sslmode=require with an existing sslrootcert file, which libpq verifies as verify-ca. - PostgreSQLRejectedReport truncates the driver text to 200 characters. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * Address the third review: refuse a URI with more than one @, bound total reverse-DNS time, strip IPv6 zone IDs - postgres: a URI connection string (postgresql:// or postgres://) with more than one literal "@" is refused on the raw string, before any log line or connect, closing the abstract-socket host-list and query parameter bypasses of the parsed-value check. libpq decodes %40 in a password, host, database name and query value, so a URI never needs a second "@". The parsed-value check remains for keyword/value strings. - Reverse DNS: every online lookup is charged to a new 600 s per-report total budget (MAX_REVERSE_DNS_SECONDS_PER_REPORT); failed or slow ones still also use the 120 s budget. The warning names the limit reached. - An IPv6 zone ID in an aggregate source_ip or a failure report's Source-IP is now stripped with a warning instead of rejecting the record or report; SMTP TLS still drops its optional IP fields. - _rejection_detail truncates the driver text before escaping it. - Docs and CHANGELOG entries updated. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * Withhold everything derived from a PostgreSQL connection string from logs and errors Fourth review of GHSA-xvp7-7762-gx9g: libpq ends a URI's userinfo at the first "@" or "/", so postgresql://u:SECRETX/x@db.invalid/d parses to host "u", port "SECRETX" and dbname "x@db.invalid/d". It has one "@", so the raw-count refusal let it through, and connecting raised 'invalid integer value "SECRETX" for connection option "port"', a password fragment in the error and its traceback. A variant put the fragment in sslmode, which the startup warning printed. Three rounds of shape checks each missed a shape. A. With a connection_string, nothing parsed from it is logged or raised: - A connection failure raises a fixed "PostgreSQL connection failed (details withheld ...)" from None, with " [SQLSTATE xxxxx]" appended only when the driver error carries a well-formed SQLSTATE (^[0-9A-Z]{5}$). A string libpq cannot parse keeps its own fixed message, and any error from the pre-parse is replaced by it. - The startup warning names no host and no value from the string; the mode is printed only when it is one of SSL_MODES. With the separate host/port/user/password/database arguments, the driver's message (credential-masked) and the host in the warning are kept. B. The early refusal stays as a convenience that names the common mistake, mirroring libpq's scan: a URI is refused when the text after "://" holds more than one "@", or an "@" with a "/" before it, and the parsed-value check (_has_misplaced_at) runs on URIs again as well as keyword/value strings. Docstrings, usage.md and the CHANGELOG now say the refusal is a convenience and A is what keeps the password out. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> * Wrap every PostgreSQL connect failure and narrow the connection-string refusal - postgres._connect(): everything from parsing the settings to holding a configured connection (pre-parse, ssl kwargs, connect, autocommit, dumper registration) is protected. A non-driver error (psycopg's UnicodeEncodeError carrying the whole built connection string for a password with a lone surrogate; on Python 3.15 the unwrapped UnicodeEncodeError('idna', '<host>') for an over-long host label) is replaced with a fixed message from None: the withheld connection failure message with a connection string, a new fixed "a connection setting could not be passed to the driver" message with the separate settings. KeyboardInterrupt/SystemExit still propagate. - cli._init_output_clients(): the PostgreSQL wrapper no longer chains (from None), and a non-PostgreSQLError gets a fixed message. - _has_unencoded_at() now mirrors libpq's URI scan (userinfo up to the first "@" or "/") instead of counting "@", so a percent-encoded database name (d%40x) and an "@" in a query value after a "/" (a certificate path) are accepted, while every password-spill shape in the table is still refused except two that read like legitimate strings. - _has_misplaced_at() is removed: a keyword/value password is its own keyword, so an "@" in a kv host, hostaddr or dbname is not a password fragment, and libpq does not percent-decode kv values, so the encoding advice was wrong. The refusal message now names the database name. - Docstrings, usage.md and the GHSA-xvp7 CHANGELOG entry updated; tests for real psycopg (surrogate password, long host label), fake-psycopg non-driver errors at connect/autocommit/dumpers for both modes, a KeyboardInterrupt check, the rule table (also checked against the real parser), legitimate strings that must connect, and the CLI wrapper. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: jwalith4418-Mj <jwalith4418@gmail.com>	2026-11-10 18:44:59 -05:00
3 people	07bca1ad28	Make the output and mailbox integrations optional extras (#888) ... * Make the output and mailbox integrations optional extras (#883) Breaking change for the next major release: pip install parsedmarc now installs the parsing core plus a working core CLI (file, IMAP, Maildir, and mbox input; CSV/JSON, Splunk HEC, webhook, and syslog output). Everything else moves behind an extra: elastic, opensearch, kafka, s3, gelf, loganalytics, msgraph, and gmail, joining the existing postgresql extra, with an umbrella [all] that deliberately excludes postgresql (psycopg's binary wheels do not exist on every platform, so parsedmarc[all] must never fail to install there). cli.py imports the six SDK-dependent output modules behind the #884 TYPE_CHECKING/try-except guard; a configured section whose extra is missing fails fast with a ConfigurationError naming the section and the exact pip install command — including the msgraph and gmail_api mailbox sections (detected via parsedmarc.mail's placeholder classes) and postgresql (checked before the constructor so the startup retry loop does not retry a missing dependency for a minute). The Azure/kiota Graph error types fall back to never-raised sentinel classes. The Docker image installs [all,postgresql], so container users see no change. CI lint installs [build,all,postgresql]; the unit-test job installs [build,all], deliberately without postgresql so test_postgres.py's absent-psycopg arm stays exercised. The never-imported dateparser dependency is dropped in favor of declaring python-dateutil, which utils.py actually imports; pytz moves to the build extra for the one test that uses it. Verified live: a no-extras wheel install imports, parses samples, and reports the install hint for each gated section; a [all] install restores every integration; the Docker image builds with every SDK importable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Patch psycopg presence in the PostgreSQL CLI wiring tests CI's unit-test job deliberately installs [build,all] without the postgresql extra, so parsedmarc.cli.postgres.psycopg is None there and the new missing-extra presence check correctly made _main exit 1 before the wiring under test ran. The tests simulate the SDK being available (PostgreSQLClient is mocked at the SDK boundary), so the module-level psycopg handle is now patched present in setUp. Verified against a simulated psycopg-absent environment as well as the local full install. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Address Copilot review: narrow guards to ModuleNotFoundError, fix docs - The optional-integration and Graph error-type import guards now catch ModuleNotFoundError instead of ImportError, so only a genuinely absent package reads as a missing extra; a broken-but-present SDK fails loudly with its real error instead of masquerading as one. The test blocker raises ModuleNotFoundError accordingly — the exact exception a missing package produces. - _missing_extra_hint docstring no longer calls every gated integration an output module (it also serves the msgraph/gmail_api mailbox sections). - Fix the pre-existing passsword typo in usage.md's kafka section; the INI key the code reads is password (cli.py _parse_config). Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Quote extras specs in copy-paste install commands From Copilot's second review round: zsh treats an unquoted .[build,all] as a glob and fails with 'no matches found', so the commands shown in AGENTS.md, CONTRIBUTING.md, dashboards/README.md, and the bootstrap script's comment are now quoted. The CI workflows keep the unquoted form: they run under bash, which passes unmatched globs through literally. The suggestion to change the 'Choosing what to install' heading level was rejected — it is a subsection of 'Installing parsedmarc', matching the file's existing hierarchy. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix upgrade command in the changelog * Documentation review: accuracy, spelling, grammar, and clarity pass A full prose review of docs/source, README, CONTRIBUTING, and the dashboards README, with every accuracy claim verified against the code before changing it. Highlights: - usage.md: documented six missing [general] options (the CSV/JSON filename options, prettify_json, normalize_timespan_threshold_hours), the required kafka smtp_tls_topic, [imap] timeout/max_retries, and the postgresql env-var prefix; corrected the maildir_path default (None, not INBOX — cli.py Namespace defaults), the mailbox check_timeout option name, the systemd restart interval (RestartSec is 5m), and merged the duplicate silent entry; quoted every copy-paste extras spec for zsh safety. - elasticsearch.md: fixed an invalid openssl command (rsa:4096 -nodes), the dashboards filename (opensearch_dashboards.ndjson, matching the file the link serves), and assorted grammar. - davmail.md: the service-enable command now enables davmail.service (was parsedmarc.service — a copy-paste error that left DavMail unenabled), plus a view typo and DavMail capitalization. - output.md: the example schema reference is RFC 7489 Appendix C (7480 is RDAP). kibana.md: SPF relies on the SMTP envelope, not session headers (RFC 7208). dmarc.md: DKM -> DKIM. - README: the intro now also names the OpenSearch/Grafana stack, matching the feature list. CONTRIBUTING: pre-PR checks now include ruff format --check and pyright, matching CI's lint job. - dashboards/README: the service table and seed description now include the PostgreSQL backend the compose stack runs. Sample data blocks, the CLI-help mirror block, and released CHANGELOG entries were deliberately left untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Docstring review: accuracy, spelling, grammar, and clarity pass Every docstring in parsedmarc/, parsedmarc/mail/, the maps maintainer scripts, and the test suite reviewed with each claim verified against the code it documents. Text-only — no behavior changes. Highlights: - Copy-paste errors corrected: parsed_smtp_tls_reports_to_csv and splunk/loganalytics save functions described aggregate or failure reports they do not handle; LogAnalyticsException claimed to be an Elasticsearch error. - Docstring/behavior mismatches: parse_report_email's report_type enumeration omitted smtp_tls; parse_failure_report typed msg_date as str (it is datetime); strip_attachment_payloads claimed payloads are replaced with None (the key is deleted); kafkaclient's failure and SMTP TLS savers claimed per-record slicing while sending the whole list in one message (docstrings now describe reality — whether slicing was intended is flagged for follow-up); the postgres savers claimed to take parse_report_file's return value but receive the inner report dict; elastic/opensearch save functions' Raises listed only AlreadySaved. - None-as-semantic-state documented where missing (get_base_domain, get_ip_address_country), enumeration completeness fixed (get_ip_address_info's 9 result keys, maps script outputs, TSV columns), and the stale 44-industry-types count corrected to the 46 the authoritative README list defines. - Test docstrings aligned with what the tests actually assert, including two that overstated coverage of the elastic/opensearch address-list tests. - Two argparse help strings fixed: file_path now names SMTP TLS report files alongside aggregate and failure, mirrored into usage.md's CLI-help block; --offline's doubled spaces removed (rendered help unchanged). - elasticsearch.md's security claim corrected against Elastic's docs: security is enabled and auto-configured on first startup since 8.0 (not "8.7 secure mode"), so the settings are verified, not hand-written. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>	2026-08-28 17:33:18 -04:00
Sean WhalenandClaude Opus 4.7	b7b8383fa4	Expand honest test coverage from 59% to 83%; fix two latent bugs (#775) ... * Expand honest test coverage from 59% to 83%; fix two latent bugs 271 new tests across the output modules, ES/OS clients, CLI config parsing, and the top-level parsing surface. Coverage measured against shipped code only (see [tool.coverage.run] source = ["parsedmarc"] omit = ["*/parsedmarc/resources/maps/*.py"] in pyproject.toml). Per-module results: s3.py 38% → 100% (also fixes SMTP-TLS-to-S3 bug below) gelf.py 40% → 100% syslog.py 46% → 100% kafkaclient.py 34% → 100% splunk.py 24% → 100% loganalytics.py 56% → 100% webhook.py 78% → 100% (also removes redundant try/except) elastic.py 36% → 99% opensearch.py 40% → 99% cli.py 52% → 69% __init__.py 74% → 76% (also fixes append_json bug below) utils.py 84% (unchanged in this PR) TOTAL 59% → 83% The remaining 17% is honest. The biggest unreached blocks are _main() in cli.py and the watch-mode mailbox iteration in __init__.py, both of which would require either standing up live subsystems (real Elasticsearch, real IMAP) or mocking deep enough that the test would verify the mock rather than the code. The PR-A AGENTS.md guidance — "if 90% requires faking it, ship 85% honestly" — applies here. Bugs fixed while writing tests: 1. parsedmarc/s3.py — SMTP-TLS-to-S3 was completely broken. save_report_to_s3 unconditionally read report["report_metadata"] when building S3 object metadata, but RFC 8460 §4.3 SMTP TLS reports are flat (no report_metadata sub-object). The CLI's surrounding try/except silently swallowed the KeyError, so every SMTP-TLS report quietly failed to upload. Also fixes a related issue: parse_smtp_tls_report_json stores begin_date as the raw ISO-8601 string from the report (per the SMTPTLSReport TypedDict and RFC 8460 §4.3), but the S3 code path assumed a datetime with .year / .month / .day attributes. Both fixed; the broken metadata-extraction branch now uses the flat-report fields, and the date branch normalizes via human_timestamp_to_datetime. 2. parsedmarc/__init__.py — append_json corrupted JSON output files on the second write. The original implementation opened files in "a+" mode, then seek()ed backwards to overwrite the trailing "]" with ",\n" before appending more elements. Python's docs are explicit (https://docs.python.org/3/library/functions.html#open): on POSIX, writes in "a"/"a+" mode always go to EOF regardless of seek() position. The result was that the second call produced [...]\n],\n[...] -style corrupted output instead of a single merged array. Replaced with a read-merge-write pattern: load the existing array (if any), append the new elements, rewrite the whole file. The CSV cousin append_csv was not affected — it doesn't seek backwards. 3. parsedmarc/webhook.py — removed redundant try/except blocks in save_aggregate_report_to_webhook / save_failure_report_to_webhook / save_smtp_tls_report_to_webhook. _send_to_webhook already catches every Exception itself, so the outer except blocks were unreachable dead code (covered nothing, defended against nothing, and inflated the source-line count without testing value). Testing approach: mocks at SDK boundaries (boto3 resource, kafka producer, requests session, opensearch/elasticsearch Document/Search, azure LogsIngestionClient). Tests verify the parsedmarc-side transformation logic — document/event construction, index/topic naming, dedup queries, error wrapping — rather than asserting on mock invocations as a proxy for behaviour. Where a branch is defensive against a caller that doesn't exist in the codebase, the test is omitted (commented in code rather than hidden behind a pragma). 547 tests total (was 276), all passing. ruff check + format clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document the two bug fixes from this PR in the 10.0.0 changelog Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Document testing standards in AGENTS.md Adds a "Testing standards" section covering the principles applied in PR-A (split) and PR-B (coverage expansion): - Coverage measures shipped code only — don't reintroduce tests/* to the scope, don't expand omit, don't use # pragma: no cover. - Honest tests assert on observable behaviour, not "the mock was called". Mock at SDK boundaries; parse the payload that gets sent. - "If 90% requires faking it, ship 85% honestly" — coverage is a tool, not a goal. PR-B's deliberate stops at cli.py 69% and __init__.py 76% are the documented precedent for when to halt. - Verify bug claims against the relevant RFC, internal types, installed SDK source, or upstream docs before changing code. Cite the source in the commit message and test docstring (RFC 8460 §4.3 and the Python open() docs for #775's two bug fixes are the pattern to follow). - Bugs found while writing tests are fixed in the same PR; the test doubles as the regression guard. - File layout (tests/test_<module>.py) is non-negotiable; module-level test loggers need fresh-handler setup so test ordering doesn't break assertLogs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Cover the corrupt-file fallback in append_json Codecov flagged 2 missing patch-coverage lines on PR #775: the except (json.JSONDecodeError, OSError) branch in append_json, which falls back to overwriting when the existing file isn't a parseable JSON array. Two new tests in tests/test_init.py:TestAppendJson exercise both paths: - test_corrupt_existing_file_is_overwritten_cleanly: existing file contains invalid JSON; append_json overwrites with the new array. - test_existing_file_with_non_list_root_is_overwritten: existing file parses as {"foo": ...} (dict, not list); the isinstance guard rejects it and we overwrite cleanly. Patch coverage now 100% on the bug fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-20 20:35:22 -04:00
Sean WhalenandClaude Opus 4.7	5b08627eaa	Split tests.py into per-module tests/test_<module>.py (#774) ... * Split tests.py into per-module tests/test_<module>.py The 5174-line tests.py monolith is split into per-module files under tests/, mirroring the checkdmarc layout: tests/test_init.py parsedmarc/__init__.py parsing surface tests/test_cli.py parsedmarc/cli.py + config / env-vars / SIGHUP tests/test_utils.py parsedmarc/utils.py (DNS, IP info, PSL, etc.) tests/test_webhook.py parsedmarc/webhook.py tests/test_kafkaclient.py parsedmarc/kafkaclient.py tests/test_splunk.py parsedmarc/splunk.py tests/test_syslog.py parsedmarc/syslog.py tests/test_loganalytics.py parsedmarc/loganalytics.py tests/test_gelf.py parsedmarc/gelf.py tests/test_s3.py parsedmarc/s3.py tests/test_maps.py parsedmarc/resources/maps/ maintainer scripts The split is purely a redistribution — no test bodies changed, no tests added or removed. All 276 existing tests pass under the new layout. The current tests.py contains two kitchen-sink classes (`Test` at line 54 and `TestEnvVarConfig` at line 2360) holding tests that span many modules. Their methods are routed to the correct per-module file by name prefix; the wholly-thematic classes (TestExtractReport, TestUtilsXxx, TestSighupReload, etc.) move whole. Each target file gets its own `class Test(unittest.TestCase)` for the redistributed kitchen-sink methods, plus the thematic classes verbatim. Wiring updates: - `.github/workflows/python-tests.yml`: `pytest ... tests.py` → `python -m pytest ... tests/` (also switches to `python -m pytest` per the checkdmarc convention so cwd lands on the project root). - `pyproject.toml`: adds `[tool.pytest.ini_options] testpaths = ["tests"]` and `[tool.coverage.run] source = ["parsedmarc"]` with an `omit` for `parsedmarc/resources/maps/*.py`. The maps scripts are maintainer-only batch tooling that ships out of the wheel; excluding them from coverage makes the headline number reflect only installed library code. Runtime coverage on the new layout is 59% (was 45% with maps counted), and PR-B will push it to 90%+. - `AGENTS.md`: documents the new layout and how to run individual files / tests; tells future contributors not to reintroduce a monolithic tests.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Restore 66.9% coverage baseline (count tests/ + parsedmarc) Master's headline 66.9% number on Codecov includes the tests.py file itself (99.35% covered) being measured alongside parsedmarc/*. The original tests.py had no `[tool.coverage.run]` block, so coverage's default — "measure every file imported during the run" — counted the test code as if it were product code. The split commit added `source = ["parsedmarc"]` which suppressed measurement of the test files (correct in principle, since test files aren't shipped code), and that alone made the headline number drop by ~8 percentage points without any actual loss of testing. This commit swaps `source` for an explicit `include = ["parsedmarc/*", "tests/*"]` so both halves are measured the way they were on master. Verified: 276 tests, 66.96% line coverage (effectively unchanged from master's 66.90%). If you want the shipped-code-only number (was the headline that this commit overrides), run `pytest --cov=parsedmarc tests/`. That number is currently 59% and is the focus of the upcoming coverage-expansion PR. Also adds junit.xml to .gitignore so the CI artefact doesn't get accidentally committed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * Restrict coverage to shipped code (`source = ["parsedmarc"]`) Reverts the prior commit's `include = ["tests/*"]`. Counting the test files toward coverage was wrong — it conflates "shipped code exercised by tests" with "test code that pytest auto-runs", inflates the headline number, and rewards writing more tests rather than tests that verify more code. Master's apparent 66.9% was an artefact of the old monolithic tests.py having no [tool.coverage.run] block at all; coverage's default behaviour measured every imported file, including the test file itself at ~99% "covered", which added ~8 percentage points to the displayed number without any real testing signal. Restricting to `source = ["parsedmarc"]` plus the existing maps omit gives a meaningful baseline: 59% of shipped code is exercised by the test suite today. That's the number the next PR is targeting to lift to 90%+ before the 10.0.0 release; the Codecov "drop" here is a measurement correction, not a regression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-20 19:29:09 -04:00