BUGFIX: Fix SPF parsing of qualified mechanisms like +mx (#4062)

cafferata · web-flow · commit d77c2634fd51 · 2026-02-10T09:23:17.000-05:00
Fixes a regression where `SPF_BUILDER` with `overflow` rejected valid RFC 7208 qualified mechanisms like `+mx`, `~mx`, `?a`, etc. with the error `unsupported SPF part mx`. The root cause was that `lcpart` (the lowercase comparison variable) was created before stripping the qualifier prefix (`+`,`-`,`~`,`?`), so `HasPrefix` checks against `"mx"` or `"a"` failed when the string was still `"+mx"` or `"+a"`. Qualifiers on mechanisms are valid per [RFC 7208 Section 4.7](https://www.rfc-editor.org/rfc/rfc7208#section-4.7). A regression test covering all qualifier/mechanism combinations has been added. Fixes #4042
diff --git a/pkg/spflib/parse.go b/pkg/spflib/parse.go
@@ -52,11 +52,11 @@ func Parse(text string, dnsres Resolver) (*SPFRecord, error) {
 		if part == "" {
 			continue
 		}
-		lcpart := strings.ToLower(part) // We have seen "Ip4" instead of "ip4".  Let's be gracious and allow it.
 		p := &SPFPart{Text: part}
 		if qualifiers[part[0]] {
 			part = part[1:]
 		}
+		lcpart := strings.ToLower(part) // We have seen "Ip4" instead of "ip4".  Let's be gracious and allow it.
 		rec.Parts = append(rec.Parts, p)
 		if part == "all" {
 			// all. nothing else matters.
diff --git a/pkg/spflib/parse_test.go b/pkg/spflib/parse_test.go
@@ -117,6 +117,40 @@ func TestParseRedirectOnly(t *testing.T) {
 	t.Log(rec.Print())
 }
 
+func TestParseQualifiedMechanisms(t *testing.T) {
+	// Regression test: SPF mechanisms with qualifiers (+mx, -all, ~all, ?mx)
+	// should be parsed correctly. See https://github.com/StackExchange/dnscontrol/issues/4042
+	dnsres, err := NewCache("testdata-dns1.json")
+	if err != nil {
+		t.Fatal(err)
+	}
+	tests := []struct {
+		input   string
+		wantErr bool
+		lookups int
+	}{
+		{"v=spf1 +mx -all", false, 1},
+		{"v=spf1 ~mx -all", false, 1},
+		{"v=spf1 ?mx -all", false, 1},
+		{"v=spf1 +a -all", false, 1},
+		{"v=spf1 ~a -all", false, 1},
+		{"v=spf1 +mx +a -all", false, 2},
+		{"v=spf1 +ip4:192.168.0.0/24 -all", false, 0},
+		{"v=spf1 +ip6:2001:db8::/32 -all", false, 0},
+	}
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			rec, err := Parse(tt.input, dnsres)
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("Parse(%q) error = %v, wantErr %v", tt.input, err, tt.wantErr)
+			}
+			if err == nil && rec.Lookups() != tt.lookups {
+				t.Errorf("Parse(%q) lookups = %d, want %d", tt.input, rec.Lookups(), tt.lookups)
+			}
+		})
+	}
+}
+
 func TestParseRedirectLast(t *testing.T) {
 	dnsres, err := NewCache("testdata-dns1.json")
 	if err != nil {

Original file line number	Diff line number	Diff line change
`@@ -52,11 +52,11 @@ func Parse(text string, dnsres Resolver) (*SPFRecord, error) {`
`52`	`52`	`if part == "" {`
`53`	`53`	`continue`
`54`	`54`	`}`
`55`		`- lcpart := strings.ToLower(part) // We have seen "Ip4" instead of "ip4". Let's be gracious and allow it.`
`56`	`55`	`p := &SPFPart{Text: part}`
`57`	`56`	`if qualifiers[part[0]] {`
`58`	`57`	`part = part[1:]`
`59`	`58`	`}`
	`59`	`+ lcpart := strings.ToLower(part) // We have seen "Ip4" instead of "ip4". Let's be gracious and allow it.`
`60`	`60`	`rec.Parts = append(rec.Parts, p)`
`61`	`61`	`if part == "all" {`
`62`	`62`	`// all. nothing else matters.`