From 79881ac7135d6872073bcc0e4c30e519c5b8434c Mon Sep 17 00:00:00 2001 From: Michael Fjeldsted Date: Wed, 2 Sep 2026 12:23:37 +0000 Subject: [PATCH] Read the alignment tags from DmarcRua's properties again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 2.0.1 replaced PolicyPublishedType.Adkim/.Aspf with get-only properties computed from new AdkimRaw/AspfRaw strings, and the helper behind them called Regex.Replace on a value that can be null. Both tags are minOccurs="0", so a reporter that omits them made merely *reading* the property throw ArgumentNullException — after deserialization had already succeeded, so it surfaced in the parser rather than as a parse error. That is 1.5% of the reports vendored in the library's own test resources, Mail.Ru and Fastmail among them. MapAlignment read the raw strings instead to keep 2.0.0's behaviour. 2.1.0 null-guards the helper (danielsen/DmarcRua#11), so the properties are safe again, and the comment on the workaround set out what to check before removing it: that an absent tag returns null rather than throwing, and that the library has not changed what absent *means*. Both hold — verified against the published package rather than inferred from the diff — so MapAlignment takes the AlignmentType? directly and collapses to one line. Relaxed stays this project's reading, not the library's. DmarcRua returns null for absent, empty and unrecognised alike, and all three land on relaxed: an alignment we could not parse is not a reason to claim the stricter policy. The existing theory covers all three cases and needed no change, which is the point of it — every other parser test supplies both tags, so without those cases the suite would go green on exactly the kind of upgrade that broke 2.0.1. Its comment is updated to say it now guards the library rather than a workaround around it. No behaviour change: 846 unit tests pass, including the six alignment cases. Co-Authored-By: Claude Opus 5 --- docs/planning/status.md | 26 +++++---- src/api.tests/DmarcRuaReportParserTests.cs | 15 +++-- .../Reports/DmarcRuaReportParser.cs | 56 ++++++------------- 3 files changed, 44 insertions(+), 53 deletions(-) diff --git a/docs/planning/status.md b/docs/planning/status.md index 9d0f494..436be11 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -506,14 +506,15 @@ Current implementation snapshot for `DmarcAnalyzerApp`. of the 3241 real reports vendored in 2.0.1's own test resources, Mail.Ru and Fastmail among them, and it would have failed ingestion for every report from those reporters where 2.0.0 quietly returned null. - - `DmarcRuaReportParser` therefore reads `AdkimRaw`/`AspfRaw` and maps them - itself, which preserves 2.0.0's behaviour and does not wait on an upstream fix. + - `DmarcRuaReportParser` therefore read `AdkimRaw`/`AspfRaw` and mapped them + itself, which preserved 2.0.0's behaviour without waiting on an upstream fix. Absent means `relaxed`, the fixed RFC 7489 §6.3 default for both tags. Reported - upstream as [danielsen/DmarcRua#11](https://github.com/danielsen/DmarcRua/issues/11). - Note the library merges contributions by reimplementing them in its own commits - rather than by merging pull requests — every PR since 2022 is closed unmerged, - including one of ours — so treat a fix as arriving whenever it arrives, and keep - the workaround until a release actually carries one. + upstream as [danielsen/DmarcRua#11](https://github.com/danielsen/DmarcRua/issues/11) + and fixed in 2.1.0, at which point the workaround was retired — see below. Note the + library merges contributions by reimplementing them in its own commits rather than + by merging pull requests — every PR since 2022 is closed unmerged, including one of + ours — so treat a fix as arriving whenever it arrives, and keep a workaround until a + release actually carries one. - the upgrade was verified by running the parser over all 3242 reports in that corpus on both versions: no regressions, identical output on every report both parse, and one report gained — a `trusted_forwarder` report that 2.0.0 discarded @@ -548,9 +549,14 @@ Current implementation snapshot for `DmarcAnalyzerApp`. all 61 `DmarcRuaReportParser` tests pass against it untouched. - **#11 is fixed.** `CleanOutStringSpecials` now null-guards, so an omitted ``/`` returns null from `.Adkim`/`.Aspf` instead of throwing. - Verified against the published package, not inferred from the diff. `MapAlignment` - can therefore go back to the properties — both conditions its doc comment sets out - now hold — but that is a separate change from the version bump. + Verified against the published package, not inferred from the diff. Both conditions + the old `MapAlignment` comment set out therefore hold — an absent tag returns null + rather than throwing, and the library still does not decide what absent *means* — so + the workaround was retired in a follow-up and `MapAlignment` now takes the + `AlignmentType?` directly. Relaxed stays this project's reading of null, which + DmarcRua returns for absent, empty and unrecognised alike. The theory that covers + those three cases is the one guarding it: every other parser test supplies both + tags, which is how 2.0.1 went green here while production would have broken. - **#12 is half-fixed, and the missing half matters.** `PolicyEvaluatedType.Disposition` is now an `ActionDispositionType`, so RFC 9990's `pass` deserializes and reads back as `pass`. But `rua.xsd` still types that diff --git a/src/api.tests/DmarcRuaReportParserTests.cs b/src/api.tests/DmarcRuaReportParserTests.cs index f809795..cad811e 100644 --- a/src/api.tests/DmarcRuaReportParserTests.cs +++ b/src/api.tests/DmarcRuaReportParserTests.cs @@ -517,11 +517,16 @@ public void Parse_ExtractsPublishedPolicy() /// /// Both alignment tags are optional, and real reporters omit them — Mail.Ru and Fastmail - /// among them, 1.5% of the reports vendored in DmarcRua 2.0.1's own test resources. On - /// 2.0.1 that is not merely a default to fill in: reading its computed Adkim/Aspf - /// properties throws ArgumentNullException on an absent tag, which would have failed - /// ingestion for every report from those reporters. Every other test here supplies both - /// tags, so the suite went green on that upgrade while production would have broken. + /// among them, 1.5% of the reports vendored in DmarcRua's own test resources. Every other + /// test here supplies both tags, so without these cases the suite goes green on an + /// upgrade that would break production: that is exactly what 2.0.1 did, where reading the + /// computed Adkim/Aspf properties threw ArgumentNullException on an absent tag. + /// + /// 2.1.0 fixed that (danielsen/DmarcRua#11) and the parser reads those properties again, + /// so this now guards the library rather than a workaround around it. The cases that + /// matter most are the ones where DmarcRua returns null for three different reasons — + /// absent, present-but-empty, and unrecognised — because relaxed is this project's + /// reading of all three, not something the library decides. /// [Theory] [InlineData("", "", "relaxed", "relaxed")] // both omitted diff --git a/src/api/Application/Reports/DmarcRuaReportParser.cs b/src/api/Application/Reports/DmarcRuaReportParser.cs index 2e61e11..7d3420e 100644 --- a/src/api/Application/Reports/DmarcRuaReportParser.cs +++ b/src/api/Application/Reports/DmarcRuaReportParser.cs @@ -129,8 +129,8 @@ public DmarcReportParseResult Parse(Stream xmlStream) MapDisposition(policyPublished.P), hasSubdomainPolicy ? MapDisposition(policyPublished.Sp) : null, ParsePercent(policyPublished.Percent), - MapAlignment(policyPublished.AdkimRaw), - MapAlignment(policyPublished.AspfRaw)); + MapAlignment(policyPublished.Adkim), + MapAlignment(policyPublished.Aspf)); } /// @@ -176,47 +176,27 @@ private static bool IsEmptyRecord(DmarcReportRecordParseResult record) }; /// - /// adkim/aspf, read from DmarcRua's raw strings rather than its Adkim/Aspf. + /// adkim/aspf, defaulting to relaxed when the reporter does not usably state one. /// - /// 2.0.1 replaced those two settable AlignmentType? properties with get-only ones - /// computed from new AdkimRaw/AspfRaw strings, and the helper behind them - /// calls Regex.Replace on the raw value with no null check. Both tags are - /// minOccurs="0" in DmarcRua's own schema, so a reporter that just omits them - /// leaves the raw string null and merely *reading* the property throws - /// ArgumentNullException — after deserialization has already succeeded, so it surfaces - /// here rather than as a parse error. That is 1.5% of the 3241 real reports vendored in - /// 2.0.1's own test resources, Mail.Ru and Fastmail among them; every report from such a - /// reporter would fail ingestion outright, where 2.0.0 returned null and fell to the - /// default below. + /// Absent means "relaxed" here, and that is this method's decision rather than the + /// library's: unlike sp, adkim and aspf have fixed RFC 7489 §6.3 defaults, so collapsing + /// an absent tag to its default is correct and needs no HasSubdomainPolicyTag-style + /// presence sniff. DmarcRua returns null for absent, empty and unrecognised alike, and + /// all three land on relaxed — an unparseable alignment is not a reason to claim the + /// stricter policy. /// /// - /// Reading the raw string keeps 2.0.0's behaviour and does not wait on an upstream fix. - /// Absent means "relaxed": unlike sp, adkim and aspf have fixed RFC 7489 §6.3 defaults, - /// so collapsing an absent tag to its default is correct and needs no - /// HasSubdomainPolicyTag-style presence sniff. Do not simplify this back to - /// .Adkim/.Aspf. - /// - /// - /// Reported upstream as danielsen/DmarcRua#11. If a later release fixes it, this can - /// go back to the properties — but check first that an absent tag returns null rather - /// than throwing, and that the library has not changed what absent *means*: "relaxed" - /// is this method's decision to make, not the library's. - /// - /// - /// Trimming, lowercasing and dropping non-alphanumerics mirrors what 2.0.1 does to these - /// values — that much of its change is a real improvement, so ' S ' still reads - /// as strict instead of silently becoming relaxed. + /// This read the raw AdkimRaw/AspfRaw strings between 2.0.1 and 2.1.0, + /// because 2.0.1's computed properties called Regex.Replace on a value that can be + /// null and threw ArgumentNullException on merely *reading* an absent tag — after + /// deserialization had already succeeded, so it surfaced here rather than as a parse + /// error. 2.1.0 null-guards the helper (danielsen/DmarcRua#11), so the properties are + /// safe again and carry the library's own trimming and case folding, which is why + /// ' S ' still reads as strict. /// /// - private static string MapAlignment(string? alignment) - { - var cleaned = (alignment ?? string.Empty) - .ToLowerInvariant() - .Where(char.IsAsciiLetterOrDigit) - .ToArray(); - - return cleaned is ['s'] ? "strict" : "relaxed"; - } + private static string MapAlignment(AlignmentType? alignment) + => alignment == AlignmentType.Strict ? "strict" : "relaxed"; /// /// Read-through only, per the DMARCbis (RFC 9989/9990/9991) impact report: np,