git » spf » commit 2868f0b

diff --git a/spf.go b/spf.go
index b976c44..403a7d2 100644
--- a/spf.go
+++ b/spf.go
@@ -969,9 +969,11 @@ func (r *resolution) redirectField(field, domain string) (Result, error) {
 }
 
 // Group extraction of macro-string from the formal specification.
+// Note this is anchored: the macro body must match it entirely, otherwise we
+// would accept invalid characters around a valid macro letter.
 // https://tools.ietf.org/html/rfc7208#section-7.1
 var macroRegexp = regexp.MustCompile(
-	`([slodiphcrtvSLODIPHCRTV])([0-9]+)?([rR])?([-.+,/_=]+)?`)
+	`^([slodiphcrtvSLODIPHCRTV])([0-9]+)?([rR])?([-.+,/_=]+)?$`)
 
 // Expand macros, return the expanded string.
 // This expects to be passed the domain-spec within a field, not an entire
@@ -1126,6 +1128,14 @@ func (r *resolution) expandMacros(s, domain string) (string, error) {
 		n.WriteString(string(c))
 	}
 
+	// If we got to the end of the string in the middle of a macro, it is
+	// unterminated and therefore invalid. Otherwise we would silently return
+	// a truncated value.
+	if afterPercent || inMacroDefinition {
+		r.trace("macro not terminated")
+		return "", ErrInvalidMacro
+	}
+
 	r.trace("macro expanded %q to %q", s, n.String())
 	return n.String(), nil
 }
diff --git a/spf_test.go b/spf_test.go
index 05f426f..980424e 100644
--- a/spf_test.go
+++ b/spf_test.go
@@ -690,10 +690,83 @@ func TestIPToMacroStr(t *testing.T) {
 	}
 }
 
+func TestValidMacro(t *testing.T) {
+	// Test that valid macros are expanded, and in particular that the macro
+	// grammar is not tightened too much: each of these exercises a different
+	// combination of the optional transformers.
+	r := resolution{
+		ip:     ip1111,
+		sender: "user@a.b.example.com",
+		helo:   "helo",
+		trace:  t.Logf,
+	}
+
+	cases := []struct {
+		macro string
+		out   string
+	}{
+		{"%{d}", "a.b.example.com"},
+		{"%{d4}", "a.b.example.com"},
+		{"%{d9}", "a.b.example.com"},
+		{"%{d2}", "example.com"},
+		{"%{dr}", "com.example.b.a"},
+		{"%{d2r}", "b.a"},
+		{"%{dR}", "com.example.b.a"},
+		{"%{l}", "user"},
+		{"%{o}", "a.b.example.com"},
+		{"%{h}", "helo"},
+		{"%{i}", "1.1.1.1"},
+		{"%{v}", "in-addr"},
+		{"%{ir}", "1.1.1.1"},
+
+		// Explicit delimiters, alone and combined with the rest.
+		{"%{d-}", "a.b.example.com"},
+		{"%{o.-}", "a.b.example.com"},
+		{"%{l+}", "user"},
+		{"%{s_}", "user@a.b.example.com"},
+
+		// Uppercase letters mean the result is URL-escaped.
+		{"%{S}", "user%40a.b.example.com"},
+		{"%{D2}", "example.com"},
+
+		// Literals and escapes around the macros.
+		{"foo.%{d}.bar", "foo.a.b.example.com.bar"},
+		{"%%", "%"},
+		{"%_", " "},
+		{"%-", "%20"},
+		{"%{d}%{l}", "a.b.example.comuser"},
+		{"no-macros-here", "no-macros-here"},
+	}
+
+	for _, c := range cases {
+		out, err := r.expandMacros(c.macro, "a.b.example.com")
+		if err != nil {
+			t.Errorf("%q: unexpected error %v", c.macro, err)
+		}
+		if out != c.out {
+			t.Errorf("%q: expected %q, got %q", c.macro, c.out, out)
+		}
+	}
+}
+
 func TestInvalidMacro(t *testing.T) {
 	// Test that the macro expansion detects some invalid macros.
 	macros := []string{
+		// Unknown macro letters.
 		"%{x}", "%{z}", "%{c}", "%{r}", "%{t}",
+
+		// Junk around an otherwise valid macro letter: the macro body has
+		// to match the grammar entirely.
+		"%{zzd}", "%{ssss}", "%{d!}", "%{s1x}", "%{1d}", "%{dr2}",
+		"%{d2rr}", "%{}", "%{ }", "%{d }",
+
+		// Unterminated macros: we must not silently return the truncated
+		// value expanded so far.
+		"%{d", "foo%{d", "foo%{", "foo%", "%",
+		"foo%{d2r", "%{d}%{s",
+
+		// Invalid character right after the "%".
+		"%d", "%a{d}", "% ",
 	}
 	for _, macro := range macros {
 		r := resolution{