Improve SPF record validation and include error message

pkg/analyzer/dns_spf.go

@@ -103,14 +103,14 @@ func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool,
 	}
 
 	// Basic validation
-	valid := d.validateSPF(spfRecord)
+	validationErr := d.validateSPF(spfRecord)
 
 	// Extract the "all" mechanism qualifier
 	var allQualifier *api.SPFRecordAllQualifier
 	var errMsg *string
 
-	if !valid {
+	if validationErr != nil {
-		errMsg = api.PtrTo("SPF record appears malformed")
+		errMsg = api.PtrTo(validationErr.Error())
 	} else {
 		// Extract qualifier from the "all" mechanism
 		if strings.HasSuffix(spfRecord, " -all") {
@@ -130,7 +130,7 @@ func (d *DNSAnalyzer) resolveSPFRecords(domain string, visited map[string]bool,
 	results = append(results, api.SPFRecord{
 		Domain:       &domain,
 		Record:       &spfRecord,
-		Valid:        valid,
+		Valid:        validationErr == nil,
 		AllQualifier: allQualifier,
 		Error:        errMsg,
 	})
@@ -183,16 +183,90 @@ func (d *DNSAnalyzer) extractSPFRedirect(record string) string {
 	return ""
 }
 
+// isValidSPFMechanism checks if a token is a valid SPF mechanism or modifier
+func (d *DNSAnalyzer) isValidSPFMechanism(token string) error {
+	// Remove qualifier prefix if present (+, -, ~, ?)
+	mechanism := strings.TrimLeft(token, "+-~?")
+
+	// Check if it's a modifier (contains =)
+	if strings.Contains(mechanism, "=") {
+		// Only allow known modifiers: redirect= and exp=
+		if strings.HasPrefix(mechanism, "redirect=") || strings.HasPrefix(mechanism, "exp=") {
+			return nil
+		}
+
+		// Check if it's a common mistake (using = instead of :)
+		parts := strings.SplitN(mechanism, "=", 2)
+		if len(parts) == 2 {
+			mechanismName := parts[0]
+			knownMechanisms := []string{"include", "a", "mx", "ptr", "exists"}
+			for _, known := range knownMechanisms {
+				if mechanismName == known {
+					return fmt.Errorf("invalid syntax '%s': mechanism '%s' should use ':' not '='", token, mechanismName)
+				}
+			}
+		}
+
+		return fmt.Errorf("unknown modifier '%s'", token)
+	}
+
+	// Check standalone mechanisms (no domain/value required)
+	if mechanism == "all" || mechanism == "a" || mechanism == "mx" || mechanism == "ptr" {
+		return nil
+	}
+
+	// Check mechanisms with domain/value
+	knownPrefixes := []string{
+		"include:",
+		"a:", "a/",
+		"mx:", "mx/",
+		"ptr:",
+		"ip4:",
+		"ip6:",
+		"exists:",
+	}
+
+	for _, prefix := range knownPrefixes {
+		if strings.HasPrefix(mechanism, prefix) {
+			return nil
+		}
+	}
+
+	return fmt.Errorf("unknown mechanism '%s'", token)
+}
+
 // validateSPF performs basic SPF record validation
-func (d *DNSAnalyzer) validateSPF(record string) bool {
+func (d *DNSAnalyzer) validateSPF(record string) error {
 	// Must start with v=spf1
 	if !strings.HasPrefix(record, "v=spf1") {
-		return false
+		return fmt.Errorf("SPF record must start with 'v=spf1'")
+	}
+
+	// Parse and validate each token in the SPF record
+	tokens := strings.Fields(record)
+	hasRedirect := false
+
+	for i, token := range tokens {
+		// Skip the version tag
+		if i == 0 && token == "v=spf1" {
+			continue
+		}
+
+		// Check if it's a valid mechanism
+		if err := d.isValidSPFMechanism(token); err != nil {
+			return err
+		}
+
+		// Track if we have a redirect modifier
+		mechanism := strings.TrimLeft(token, "+-~?")
+		if strings.HasPrefix(mechanism, "redirect=") {
+			hasRedirect = true
+		}
 	}
 
 	// Check for redirect= modifier (which replaces the need for an 'all' mechanism)
-	if strings.Contains(record, "redirect=") {
+	if hasRedirect {
-		return true
+		return nil
 	}
 
 	// Check for common syntax issues
@@ -206,7 +280,11 @@ func (d *DNSAnalyzer) validateSPF(record string) bool {
 		}
 	}
 
-	return hasValidEnding
+	if !hasValidEnding {
+		return fmt.Errorf("SPF record should end with an 'all' mechanism (e.g., '-all', '~all') or have a 'redirect=' modifier")
+	}
+
+	return nil
 }
 
 // hasSPFStrictFail checks if SPF record has strict -all mechanism

pkg/analyzer/dns_spf_test.go

@@ -22,60 +22,105 @@
 package analyzer
 
 import (
+	"strings"
 	"testing"
 	"time"
 )
 
 func TestValidateSPF(t *testing.T) {
 	tests := []struct {
-		name     string
+		name        string
-		record   string
+		record      string
-		expected bool
+		expectError bool
+		errorMsg    string // Expected error message (substring match)
 	}{
 		{
-			name:     "Valid SPF with -all",
+			name:        "Valid SPF with -all",
-			record:   "v=spf1 include:_spf.example.com -all",
+			record:      "v=spf1 include:_spf.example.com -all",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Valid SPF with ~all",
+			name:        "Valid SPF with ~all",
-			record:   "v=spf1 ip4:192.0.2.0/24 ~all",
+			record:      "v=spf1 ip4:192.0.2.0/24 ~all",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Valid SPF with +all",
+			name:        "Valid SPF with +all",
-			record:   "v=spf1 +all",
+			record:      "v=spf1 +all",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Valid SPF with ?all",
+			name:        "Valid SPF with ?all",
-			record:   "v=spf1 mx ?all",
+			record:      "v=spf1 mx ?all",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Valid SPF with redirect",
+			name:        "Valid SPF with redirect",
-			record:   "v=spf1 redirect=_spf.example.com",
+			record:      "v=spf1 redirect=_spf.example.com",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Valid SPF with redirect and mechanisms",
+			name:        "Valid SPF with redirect and mechanisms",
-			record:   "v=spf1 ip4:192.0.2.0/24 redirect=_spf.example.com",
+			record:      "v=spf1 ip4:192.0.2.0/24 redirect=_spf.example.com",
-			expected: true,
+			expectError: false,
 		},
 		{
-			name:     "Invalid SPF - no version",
+			name:        "Valid SPF with multiple mechanisms",
-			record:   "include:_spf.example.com -all",
+			record:      "v=spf1 a mx ip4:192.0.2.0/24 include:_spf.example.com -all",
-			expected: false,
+			expectError: false,
 		},
 		{
-			name:     "Invalid SPF - no all mechanism or redirect",
+			name:        "Valid SPF with exp modifier",
-			record:   "v=spf1 include:_spf.example.com",
+			record:      "v=spf1 mx exp=explain.example.com -all",
-			expected: false,
+			expectError: false,
 		},
 		{
-			name:     "Invalid SPF - wrong version",
+			name:        "Invalid SPF - no version",
-			record:   "v=spf2 include:_spf.example.com -all",
+			record:      "include:_spf.example.com -all",
-			expected: false,
+			expectError: true,
+			errorMsg:    "must start with 'v=spf1'",
+		},
+		{
+			name:        "Invalid SPF - no all mechanism or redirect",
+			record:      "v=spf1 include:_spf.example.com",
+			expectError: true,
+			errorMsg:    "should end with an 'all' mechanism",
+		},
+		{
+			name:        "Invalid SPF - wrong version",
+			record:      "v=spf2 include:_spf.example.com -all",
+			expectError: true,
+			errorMsg:    "must start with 'v=spf1'",
+		},
+		{
+			name:        "Invalid SPF - include= instead of include:",
+			record:      "v=spf1 include=icloud.com ~all",
+			expectError: true,
+			errorMsg:    "should use ':' not '='",
+		},
+		{
+			name:        "Invalid SPF - a= instead of a:",
+			record:      "v=spf1 a=example.com -all",
+			expectError: true,
+			errorMsg:    "should use ':' not '='",
+		},
+		{
+			name:        "Invalid SPF - mx= instead of mx:",
+			record:      "v=spf1 mx=example.com -all",
+			expectError: true,
+			errorMsg:    "should use ':' not '='",
+		},
+		{
+			name:        "Invalid SPF - unknown mechanism",
+			record:      "v=spf1 foobar -all",
+			expectError: true,
+			errorMsg:    "unknown mechanism",
+		},
+		{
+			name:        "Invalid SPF - unknown modifier",
+			record:      "v=spf1 -all unknown=value",
+			expectError: true,
+			errorMsg:    "unknown modifier",
 		},
 	}
 
@@ -83,9 +128,17 @@ func TestValidateSPF(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := analyzer.validateSPF(tt.record)
+			err := analyzer.validateSPF(tt.record)
-			if result != tt.expected {
+			if tt.expectError {
-				t.Errorf("validateSPF(%q) = %v, want %v", tt.record, result, tt.expected)
+				if err == nil {
+					t.Errorf("validateSPF(%q) expected error but got nil", tt.record)
+				} else if tt.errorMsg != "" && !strings.Contains(err.Error(), tt.errorMsg) {
+					t.Errorf("validateSPF(%q) error = %q, want error containing %q", tt.record, err.Error(), tt.errorMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("validateSPF(%q) unexpected error: %v", tt.record, err)
+				}
 			}
 		})
 	}