-
Notifications
You must be signed in to change notification settings - Fork 92
Correctly handle and validate SPF macros in mechanisms and modifiers #218
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…t and stop processing them further, since macros can only be validated in the context of an actual email.
|
Hi Sean, It turns out the previous pull request had a small bug: it was not incrementing the DNS lookup counts when macros were used. I’ve fixed that issue and created a new pull request. I also added assertions for DNS lookup counts in the unit tests to prevent regressions. |
|
Hi Sean, I have done a full regression test and detected no regression. |
|
Hi. I'm working on adding detailed typing across the library (I'm not done yet), so there are now mere conflicts. Can you address these so I can merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR enhances SPF record parsing in checkdmarc to correctly handle SPF macros (e.g., %{d}, %{i}). Previously, the parser attempted DNS lookups for mechanism values containing macros, which would fail since macros require runtime context to expand. The changes validate macro syntax while skipping DNS lookups, treating macro-containing mechanisms as statically unresolvable but syntactically valid.
Key changes:
- Added macro detection (
%in value) fora,mx,include,ptr,existsmechanisms andredirect,expmodifiers - Implemented syntax validation for all macros using existing
_validate_spf_macrosfunction - Fixed critical bugs in
expmodifier: corrected parentheses placement (len(exp_txt_records) == 0instead oflen(exp_txt_records == 0)) and added missing f-string prefixes
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| checkdmarc/spf.py | Added macro detection and DNS lookup skipping for mechanisms/modifiers; fixed bugs in exp modifier (parentheses and f-strings); mechanisms with macros still count toward DNS lookup limits per RFC 7208 |
| tests.py | Added comprehensive test coverage for valid and invalid macro syntax across all mechanisms (a, mx, ptr, include, exists) and modifiers (redirect, exp); added DNS lookup count assertions to existing tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Initial plan * Convert dict() and OrderedDict() to literal dictionaries with TypedDict annotations in spf.py Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com> * Fix parameter type annotation for 'seen' parameter in parse_spf_record Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com> * Add TypedDict definitions and type annotations to bimi.py Co-authored-by: seanthegeek <44679+seanthegeek@users.noreply.github.com> * Properly Reintigrate the code from PR #218 * Code formatting * Use dict[str, Any] for BIMI return types There isn't a way to make a TypeDict field optional until Python 3.11, and we need to support 3.9. Leaving the typedicts in place for now, so that they can be used when Python 3.10 is no longer suported. * Always raise a warning when a PTR mechanism is used * Refine warnings in SPF record parsing and update SVGMetadata TypedDict comment for clarity * Fix macro validation --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Description
This PR improves the handling of SPF macros within
checkdmarc. Previously, the parser attempted to resolve DNS records for mechanisms containing macros (e.g.,a:%{d}), which would fail or produce incorrect results since macros require runtime context (like the sender's IP or domain) to be expanded.This update ensures that when a macro is detected in a mechanism or modifier,
checkdmarcvalidates the macro's syntax but skips the associated DNS lookup.Changes
checkdmarc/spf.py:%) ina,mx,include,ptr,existsmechanisms andredirect,expmodifiers._validate_spf_macros.a,mx,include,redirect, andexpwhen macros are present, treating them as valid but unresolvable in a static context.parsedoutput.tests.py:a:testSPFValidAMechanismMacro,testSPFBrokenAMechanismMacromx:testSPFValidMXMechanismMacro,testSPFBrokenMXMechanismMacroptr:testSPFValidPTRMechanismMacro,testSPFBrokenPTRMechanismMacroinclude:testSPFValidIncludeMechanismMacro,testSPFBrokenIncludeMechanismMacroexists:testSPFValidExistsMechanismMacro,testSPFBrokenExistsMechanismMacroredirect:testSPFValidRedirectModifierMacro,testSPFBrokenRedirectModifierMacroexp:testSPFValidExpModifierMacro,testSPFBrokenExpModifierMacroMotivation
SPF macros (RFC 7208) allow for dynamic policy records. A static analyzer like
checkdmarccannot resolve these dynamic values. Attempting to do so leads toNXDOMAINerrors or meaningless queries (e.g., looking up%{i}.example.com). This change aligns the parser's behavior with the limitations of static analysis by validating the syntax without performing the impossible resolution.