Fix circular SPF reference detection (A→B→A loops) #1398
Conversation
The existing self-reference checks only caught direct loops (A→A) but missed indirect circular references like A→B→A or longer chains. Added a -Visited parameter to track all domains in the resolution chain. Before recursing into an include or redirect, the function now checks if the target domain has already been visited in the current resolution path. - Direct self-references return specific messages (e.g., "Self-referencing SPF include") - Indirect circular references return "Circular SPF reference detected" Tested against oldmomutual.com which has a circular include with oldmissourimutual.com. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR fixes circular SPF reference detection by introducing a tracking mechanism for visited domains during SPF record resolution. Previously, only direct self-references (A→A) were detected, but indirect circular references (A→B→A) caused infinite loops and stack overflows.
Key changes:
- Added a
$Visitedparameter array to track all domains in the resolution chain - Updated circular reference detection logic for both redirect and include directives
- Distinguished between direct self-references and indirect circular references in error messages
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
Accepted Copilot's suggestions, they make sense. |
OK, cool. I didn't have a chance to test yet, so I just kicked off the Copilot review. |
|
A little bump on this @SamErde. We're seeing this is production for quite a few tenants so would love to get the fix in place soon. |
|
Checking in again @SamErde. Would love to continue using the main Maester PS package, rather than running a local fork. |
Updated the Resolve-SPFRecord function to enhance documentation and improve parameter descriptions. Added error handling and refined DNS resolution logic for better compatibility across platforms. 1. Fixed `a:` Mechanism Domain Resolution - Now correctly extracts and resolves the domain specified in the directive (e.g., `a:sub.example.com`) - Previously always resolved `$Name` instead of the directive's target domain, returning incorrect IP addresses 2. Fixed MX Mechanism A Record Lookup - Corrected non-Windows path that was querying `$Name` instead of each `$MXRecord` when resolving A records - Previously returned wrong IP addresses for MX hosts 3. Improved DNS Server Parameter Handling - Now only passes `-NameServer`/`-Server` parameter when explicitly provided by user - Uses parameter splatting to conditionally build DNS query parameters - Prevents passing empty/null values to DNS cmdlets 4. Better Error Handling - Changed error returns from strings to proper `Write-Error` and `Write-Warning` cmdlets - Maintains consistent output type contract `([OutputType([SPFRecord[]])])` - Added `-ErrorAction SilentlyContinue` to DNS lookups to gracefully handle failures 5. Parameter Validation - Added `[ValidateNotNullOrEmpty()]` to `$Name` and `$Server` parameters - Prevents unnecessary DNS calls with invalid input 6. Improved Variable Naming - Used distinct variable names (`$MXDNSRecords`, `$ADNSRecords`) to prevent variable overwriting in nested loops - Improves code clarity and prevents data loss 7. Documentation - Completed comment-based help section - Moved comment-based help header into the function block to align with best practices
SamErde
added
bug
I was hoping for input and testing from others, but think it's in a good state. I re-reviewed today and added a few more enhancements and fixes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
|
@f-bader, if you like these improvements, feel free to update your original blog post! :) |
2 participants
Description
The existing self-reference checks only caught direct loops (A→A) but missed indirect circular references like A→B→A or longer chains. This causes infinite loops which eventually result in a stack overflow.
Added a -Visited parameter to track all domains in the resolution chain. Before recursing into an include or redirect, the function now checks if the target domain has already been visited in the current resolution path.
Tested against oldmomutual.com which has a circular include with oldmissourimutual.com.
Contribution Checklist
Before submitting this PR, please confirm you have completed the following:
/powershell/tests/pester.ps1on your local system.