git » spf » commit a230f6e

Fix redirect= order handling

author Alberto Bertogli
2018-03-18 10:56:41 UTC
committer Alberto Bertogli
2018-03-18 13:16:26 UTC
parent cb8e8c38f89daa1dfcac9d5bbf1de33d8937fe18

Fix redirect= order handling

The redirect= modifier should be checked _after_ mechanisms, regardless
of where it appears in the SPF record.

From https://tools.ietf.org/html/rfc7208#section-6.1":

  If all mechanisms fail to match, and a "redirect" modifier is present,
  then processing proceeds as follows:

  [...]

  For clarity, any "redirect" modifier SHOULD appear as the very last
  term in a record.  Any "redirect" modifier MUST be ignored if there is
  an "all" mechanism anywhere in the record.

To implement this, the library meant to move any redirect= modifier to
the end of the record. However, due to a typo, this was not working.  To
make it worse, there were no tests covering redirects :(

The impact of this bug was that mechanisms that came after redirect=
modifiers were ignored. This should be quite rare in practice, but it's
possible.

This patch fixes the reordering (by fixing the typo), and more
importantly adds tests for various redirect= cases.

spf.go +1 -1
spf_test.go +46 -0

diff --git a/spf.go b/spf.go
index 029044c..1f22c56 100644
--- a/spf.go
+++ b/spf.go
@@ -128,7 +128,7 @@ func (r *resolution) Check(domain string) (Result, error) {
 	// we just move them to the end.
 	var newfields, redirects []string
 	for _, field := range fields {
-		if strings.HasPrefix(field, "redirect:") {
+		if strings.HasPrefix(field, "redirect=") {
 			redirects = append(redirects, field)
 		} else {
 			newfields = append(newfields, field)
diff --git a/spf_test.go b/spf_test.go
index 8d8d813..8337e75 100644
--- a/spf_test.go
+++ b/spf_test.go
@@ -147,6 +147,7 @@ func TestNotSupported(t *testing.T) {
 		"v=spf1 exists:blah -all",
 		"v=spf1 exp=blah -all",
 		"v=spf1 a:%{o} -all",
+		"v=spf1 redirect=_spf.%{d}",
 	}
 
 	for _, txt := range cases {
@@ -168,6 +169,51 @@ func TestRecursion(t *testing.T) {
 	}
 }
 
+func TestRedirect(t *testing.T) {
+	txtResults["domain"] = []string{"v=spf1 redirect=domain2"}
+	txtResults["domain2"] = []string{"v=spf1 ip4:1.1.1.1 -all"}
+
+	res, err := CheckHost(ip1111, "domain")
+	if res != Pass {
+		t.Errorf("expected pass, got %v (%v)", res, err)
+	}
+}
+
+func TestInvalidRedirect(t *testing.T) {
+	// Redirect to a non-existing host; the inner check returns None, but due
+	// to the redirection, this lookup should return PermError.
+	// https://tools.ietf.org/html/rfc7208#section-6.1
+	txtResults["domain"] = []string{"v=spf1 redirect=doesnotexist"}
+
+	res, err := CheckHost(ip1111, "doesnotexist")
+	if res != None {
+		t.Errorf("expected none, got %v (%v)", res, err)
+	}
+
+	res, err = CheckHost(ip1111, "domain")
+	if res != PermError {
+		t.Errorf("expected permerror, got %v (%v)", res, err)
+	}
+}
+
+func TestRedirectOrder(t *testing.T) {
+	// We should only check redirects after all mechanisms, even if the
+	// redirect modifier appears before them.
+	txtResults["faildom"] = []string{"v=spf1 -all"}
+
+	txtResults["domain"] = []string{"v=spf1 redirect=faildom"}
+	res, err := CheckHost(ip1111, "domain")
+	if res != Fail {
+		t.Errorf("expected fail, got %v (%v)", res, err)
+	}
+
+	txtResults["domain"] = []string{"v=spf1 redirect=faildom all"}
+	res, err = CheckHost(ip1111, "domain")
+	if res != Pass {
+		t.Errorf("expected pass, got %v (%v)", res, err)
+	}
+}
+
 func TestNoRecord(t *testing.T) {
 	txtResults["d1"] = []string{""}
 	txtResults["d2"] = []string{"loco", "v=spf2"}