Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions docs/planning/status.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
`<adkim>`/`<aspf>` 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
Expand Down
15 changes: 10 additions & 5 deletions src/api.tests/DmarcRuaReportParserTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -517,11 +517,16 @@ public void Parse_ExtractsPublishedPolicy()

/// <summary>
/// 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.
/// </summary>
[Theory]
[InlineData("", "", "relaxed", "relaxed")] // both omitted
Expand Down
56 changes: 18 additions & 38 deletions src/api/Application/Reports/DmarcRuaReportParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

/// <summary>
Expand Down Expand Up @@ -176,47 +176,27 @@ private static bool IsEmptyRecord(DmarcReportRecordParseResult record)
};

/// <summary>
/// adkim/aspf, read from DmarcRua's raw strings rather than its <c>Adkim</c>/<c>Aspf</c>.
/// adkim/aspf, defaulting to relaxed when the reporter does not usably state one.
/// <para>
/// 2.0.1 replaced those two settable <c>AlignmentType?</c> properties with get-only ones
/// computed from new <c>AdkimRaw</c>/<c>AspfRaw</c> strings, and the helper behind them
/// calls <c>Regex.Replace</c> on the raw value with no null check. Both tags are
/// <c>minOccurs="0"</c> 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.
/// </para>
/// <para>
/// 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
/// <c>.Adkim</c>/<c>.Aspf</c>.
/// </para>
/// <para>
/// 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.
/// </para>
/// <para>
/// 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 '&#160;S&#160;' still reads
/// as strict instead of silently becoming relaxed.
/// This read the raw <c>AdkimRaw</c>/<c>AspfRaw</c> strings between 2.0.1 and 2.1.0,
/// because 2.0.1's computed properties called <c>Regex.Replace</c> 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
/// '&#160;S&#160;' still reads as strict.
/// </para>
/// </summary>
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";

/// <summary>
/// Read-through only, per the DMARCbis (RFC 9989/9990/9991) impact report: np,
Expand Down
Loading