Add warning for deprecated Sender ID TXT records#255
Conversation
|
test domain: emaildeliveryguy.ca result of testing: "spf": { |
seanthegeek
left a comment
There was a problem hiding this comment.
Thanks for this, Matt — surfacing leftover Sender ID records is a genuinely useful check. A few things to address before this can land:
1. CI is failing on formatting (blocker)
The build job fails at ruff format --check .:
Would reformat: checkdmarc/spf.py
1 file would be reformatted, 25 files already formatted
ruff check passes; it's only the formatter. Running ruff format . and committing fixes it. The repo's ./build.sh runs this step, so running that locally before pushing will catch it. (This is also likely why the missing tests below slipped through — the change doesn't appear to have gone through the standard build flow.)
2. The PR description needs updating — this does change SPF parsing
The description says the change "does not change SPF parsing or validation behaviour," but refactoring the matching block from record.strip('"').startswith(txt_prefix) to the lowercased exact/space-prefix comparison changes three behaviors:
| Input | Before | After |
|---|---|---|
v=spf10 |
accepted | rejected |
v=spf1foo |
accepted | rejected |
V=SPF1 -all |
ignored | accepted |
v=spf1 mx |
ignored | accepted |
These are all reasonable — rejecting v=spf10 actually matches RFC 7208 §4.5 and the code comment already in query_spf_record, so it's a latent bug fix. But they need to be (a) described accurately in the PR, and (b) covered by tests (below).
One related note worth adding to the description: the Sender ID warning surfaces only for domains that also have a valid SPF record. A domain with only a Sender ID record and no SPF correctly returns the "An SPF record does not exist" error (a missing SPF record is an error, not a warning), and error results don't carry a warnings field by design — so the warning won't appear there. That's expected behavior, just worth stating so it isn't mistaken for a bug later.
3. Tests are required
Per the repo convention ("every bit of code should have a test"), please add tests for:
- The Sender ID warning — one per scope spelling:
v=spf2.0/pra,v=spf2.0/mfrom,v=spf2.0/mfrom,pra,v=spf2.0/pra,mfrom. These can be mocked like the existingtestSplitSPFRecordMocked/testUndecodableCharactersInNonSPFRecordtests. - The parser behavior changes above: that
v=spf10/v=spf1foonow raiseSPFRecordNotFound, that an uppercaseV=SPF1record is accepted, and that surrounding whitespace is tolerated.
4. Minor
- The leading
"?inSENDER_ID_VERSION_TAG_REGEXis dead — the regex runs againstrecord.strip().strip('"'), so the quote is already gone. Drop it (or run the regex against the raw record). - The
(?:pra|mfrom|mfrom,pra|pra,mfrom)alternation can be simplified to(?:pra|mfrom)(?:,(?:pra|mfrom))?— functionally identical, easier to read. Optional.
This review was generated by Claude (Claude Code) at a maintainer's request.
|
@seanthegeek I believe I have addresses the issues - please review again. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #255 +/- ##
==========================================
+ Coverage 95.43% 95.50% +0.07%
==========================================
Files 12 12
Lines 2911 2917 +6
==========================================
+ Hits 2778 2786 +8
+ Misses 133 131 -2 ☔ View full report in Codecov by Harness. |
|
Thanks! |
Sender ID was deprecated in 2018 and these records are no longer recommended.
Adds a warning when deprecated Sender ID TXT records are detected during SPF checks.
This change detects:
The warning is added during SPF TXT record discovery and does not change SPF parsing or validation behaviour.
Example warning:
"A deprecated Sender ID record was found. Sender ID using spf2.0/pra or spf2.0/mfrom was deprecated and should be removed."
Tested against domains containing both valid SPF and legacy Sender ID TXT records.