Merge pull request #1796 from mholt/bugfix_rewrite_1794

Fix for #1794: Fixes issues with IfMatcher and regular expressions.

caddyhttp/httpserver/condition.go

@@ -53,100 +53,88 @@ const (
 	matchOp      = "match"
 )
 
-func operatorError(operator string) error {
-	return fmt.Errorf("Invalid operator %v", operator)
-}
-
 // ifCondition is a 'if' condition.
-type ifCondition func(string, string) bool
-
-var ifConditions = map[string]ifCondition{
-	isOp:         isFunc,
-	notOp:        notFunc,
-	hasOp:        hasFunc,
-	startsWithOp: startsWithFunc,
-	endsWithOp:   endsWithFunc,
-	matchOp:      matchFunc,
-}
+type ifFunc func(a, b string) bool
 
-// isFunc is condition for Is operator.
-// It checks for equality.
-func isFunc(a, b string) bool {
-	return a == b
+// ifCond is statement for a IfMatcher condition.
+type ifCond struct {
+	a   string
+	op  string
+	b   string
+	neg bool
+	rex *regexp.Regexp
+	f   ifFunc
 }
 
-// notFunc is condition for Not operator.
-// It checks for inequality.
-func notFunc(a, b string) bool {
-	return !isFunc(a, b)
-}
+// newIfCond creates a new If condition.
+func newIfCond(a, op, b string) (ifCond, error) {
+	i := ifCond{a: a, op: op, b: b}
+	if strings.HasPrefix(op, "not_") {
+		i.neg = true
+		i.op = op[4:]
+	}
+
+	switch i.op {
+	case isOp:
+		// It checks for equality.
+		i.f = i.isFunc
+	case notOp:
+		// It checks for inequality.
+		i.f = i.notFunc
+	case hasOp:
+		// It checks if b is a substring of a.
+		i.f = strings.Contains
+	case startsWithOp:
+		// It checks if b is a prefix of a.
+		i.f = strings.HasPrefix
+	case endsWithOp:
+		// It checks if b is a suffix of a.
+		i.f = strings.HasSuffix
+	case matchOp:
+		// It does regexp matching of a against pattern in b and returns if they match.
+		var err error
+		if i.rex, err = regexp.Compile(i.b); err != nil {
+			return ifCond{}, fmt.Errorf("Invalid regular expression: '%s', %v", i.b, err)
+		}
+		i.f = i.matchFunc
+	default:
+		return ifCond{}, fmt.Errorf("Invalid operator %v", i.op)
+	}
 
-// hasFunc is condition for Has operator.
-// It checks if b is a substring of a.
-func hasFunc(a, b string) bool {
-	return strings.Contains(a, b)
+	return i, nil
 }
 
-// startsWithFunc is condition for StartsWith operator.
-// It checks if b is a prefix of a.
-func startsWithFunc(a, b string) bool {
-	return strings.HasPrefix(a, b)
+// isFunc is condition for Is operator.
+func (i ifCond) isFunc(a, b string) bool {
+	return a == b
 }
 
-// endsWithFunc is condition for EndsWith operator.
-// It checks if b is a suffix of a.
-func endsWithFunc(a, b string) bool {
-	return strings.HasSuffix(a, b)
+// notFunc is condition for Not operator.
+func (i ifCond) notFunc(a, b string) bool {
+	return a != b
 }
 
 // matchFunc is condition for Match operator.
-// It does regexp matching of a against pattern in b
-// and returns if they match.
-func matchFunc(a, b string) bool {
-	matched, _ := regexp.MatchString(b, a)
-	return matched
-}
-
-// ifCond is statement for a IfMatcher condition.
-type ifCond struct {
-	a   string
-	op  string
-	b   string
-	neg bool
-}
-
-// newIfCond creates a new If condition.
-func newIfCond(a, operator, b string) (ifCond, error) {
-	neg := false
-	if strings.HasPrefix(operator, "not_") {
-		neg = true
-		operator = operator[4:]
-	}
-	if _, ok := ifConditions[operator]; !ok {
-		return ifCond{}, operatorError(operator)
-	}
-	return ifCond{
-		a:   a,
-		op:  operator,
-		b:   b,
-		neg: neg,
-	}, nil
+func (i ifCond) matchFunc(a, b string) bool {
+	return i.rex.MatchString(a)
 }
 
 // True returns true if the condition is true and false otherwise.
 // If r is not nil, it replaces placeholders before comparison.
 func (i ifCond) True(r *http.Request) bool {
-	if c, ok := ifConditions[i.op]; ok {
+	if i.f != nil {
 		a, b := i.a, i.b
 		if r != nil {
 			replacer := NewReplacer(r, nil, "")
 			a = replacer.Replace(i.a)
+			if i.op != matchOp {
 				b = replacer.Replace(i.b)
 			}
+		}
 		if i.neg {
-			return !c(a, b)
+			return !i.f(a, b)
 		}
-		return c(a, b)
+		return i.f(a, b)
 	}
 	return i.neg // false if not negated, true otherwise
 }

caddyhttp/httpserver/condition_test.go

@@ -2,8 +2,8 @@ package httpserver
 
 import (
 	"context"
-	"fmt"
 	"net/http"
+	"regexp"
 	"strings"
 	"testing"
 
@@ -14,66 +14,72 @@ func TestConditions(t *testing.T) {
 	tests := []struct {
 		condition string
 		isTrue    bool
+		shouldErr bool
 	}{
-		{"a is b", false},
-		{"a is a", true},
-		{"a not b", true},
-		{"a not a", false},
-		{"a has a", true},
-		{"a has b", false},
-		{"ba has b", true},
-		{"bab has b", true},
-		{"bab has bb", false},
-		{"a not_has a", false},
-		{"a not_has b", true},
-		{"ba not_has b", false},
-		{"bab not_has b", false},
-		{"bab not_has bb", true},
-		{"bab starts_with bb", false},
-		{"bab starts_with ba", true},
-		{"bab starts_with bab", true},
-		{"bab not_starts_with bb", true},
-		{"bab not_starts_with ba", false},
-		{"bab not_starts_with bab", false},
-		{"bab ends_with bb", false},
-		{"bab ends_with bab", true},
-		{"bab ends_with ab", true},
-		{"bab not_ends_with bb", true},
-		{"bab not_ends_with ab", false},
-		{"bab not_ends_with bab", false},
-		{"a match *", false},
-		{"a match a", true},
-		{"a match .*", true},
-		{"a match a.*", true},
-		{"a match b.*", false},
-		{"ba match b.*", true},
-		{"ba match b[a-z]", true},
-		{"b0 match b[a-z]", false},
-		{"b0a match b[a-z]", false},
-		{"b0a match b[a-z]+", false},
-		{"b0a match b[a-z0-9]+", true},
-		{"a not_match *", true},
-		{"a not_match a", false},
-		{"a not_match .*", false},
-		{"a not_match a.*", false},
-		{"a not_match b.*", true},
-		{"ba not_match b.*", false},
-		{"ba not_match b[a-z]", false},
-		{"b0 not_match b[a-z]", true},
-		{"b0a not_match b[a-z]", true},
-		{"b0a not_match b[a-z]+", true},
-		{"b0a not_match b[a-z0-9]+", false},
+		{"a is b", false, false},
+		{"a is a", true, false},
+		{"a not b", true, false},
+		{"a not a", false, false},
+		{"a has a", true, false},
+		{"a has b", false, false},
+		{"ba has b", true, false},
+		{"bab has b", true, false},
+		{"bab has bb", false, false},
+		{"a not_has a", false, false},
+		{"a not_has b", true, false},
+		{"ba not_has b", false, false},
+		{"bab not_has b", false, false},
+		{"bab not_has bb", true, false},
+		{"bab starts_with bb", false, false},
+		{"bab starts_with ba", true, false},
+		{"bab starts_with bab", true, false},
+		{"bab not_starts_with bb", true, false},
+		{"bab not_starts_with ba", false, false},
+		{"bab not_starts_with bab", false, false},
+		{"bab ends_with bb", false, false},
+		{"bab ends_with bab", true, false},
+		{"bab ends_with ab", true, false},
+		{"bab not_ends_with bb", true, false},
+		{"bab not_ends_with ab", false, false},
+		{"bab not_ends_with bab", false, false},
+		{"a match *", false, true},
+		{"a match a", true, false},
+		{"a match .*", true, false},
+		{"a match a.*", true, false},
+		{"a match b.*", false, false},
+		{"ba match b.*", true, false},
+		{"ba match b[a-z]", true, false},
+		{"b0 match b[a-z]", false, false},
+		{"b0a match b[a-z]", false, false},
+		{"b0a match b[a-z]+", false, false},
+		{"b0a match b[a-z0-9]+", true, false},
+		{"bac match b[a-z]{2}", true, false},
+		{"a not_match *", false, true},
+		{"a not_match a", false, false},
+		{"a not_match .*", false, false},
+		{"a not_match a.*", false, false},
+		{"a not_match b.*", true, false},
+		{"ba not_match b.*", false, false},
+		{"ba not_match b[a-z]", false, false},
+		{"b0 not_match b[a-z]", true, false},
+		{"b0a not_match b[a-z]", true, false},
+		{"b0a not_match b[a-z]+", true, false},
+		{"b0a not_match b[a-z0-9]+", false, false},
+		{"bac not_match b[a-z]{2}", false, false},
 	}
 
 	for i, test := range tests {
 		str := strings.Fields(test.condition)
 		ifCond, err := newIfCond(str[0], str[1], str[2])
 		if err != nil {
+			if !test.shouldErr {
 				t.Error(err)
 			}
+			continue
+		}
 		isTrue := ifCond.True(nil)
 		if isTrue != test.isTrue {
-			t.Errorf("Test %d: expected %v found %v", i, test.isTrue, isTrue)
+			t.Errorf("Test %d: '%s' expected %v found %v", i, test.condition, test.isTrue, isTrue)
 		}
 	}
 
@@ -192,6 +198,7 @@ func TestIfMatcher(t *testing.T) {
 }
 
 func TestSetupIfMatcher(t *testing.T) {
+	rex_b, _ := regexp.Compile("b")
 	tests := []struct {
 		input     string
 		shouldErr bool
@@ -201,7 +208,7 @@ func TestSetupIfMatcher(t *testing.T) {
 			if	a match b
 		 }`, false, IfMatcher{
 			ifs: []ifCond{
-				{a: "a", op: "match", b: "b", neg: false},
+				{a: "a", op: "match", b: "b", neg: false, rex: rex_b},
 			},
 		}},
 		{`test {
@@ -209,7 +216,7 @@ func TestSetupIfMatcher(t *testing.T) {
 			if_op or
 		 }`, false, IfMatcher{
 			ifs: []ifCond{
-				{a: "a", op: "match", b: "b", neg: false},
+				{a: "a", op: "match", b: "b", neg: false, rex: rex_b},
 			},
 			isOr: true,
 		}},
@@ -255,6 +262,7 @@ func TestSetupIfMatcher(t *testing.T) {
 	for i, test := range tests {
 		c := caddy.NewTestController("http", test.input)
 		c.Next()
+
 		matcher, err := SetupIfMatcher(c)
 		if err == nil && test.shouldErr {
 			t.Errorf("Test %d didn't error, but it should have", i)
@@ -263,15 +271,60 @@ func TestSetupIfMatcher(t *testing.T) {
 		} else if err != nil && test.shouldErr {
 			continue
 		}
-		if _, ok := matcher.(IfMatcher); !ok {
+
+		test_if, ok := matcher.(IfMatcher)
+		if !ok {
 			t.Error("RequestMatcher should be of type IfMatcher")
 		}
+
 		if err != nil {
 			t.Errorf("Expected no error, but got: %v", err)
 		}
-		if fmt.Sprint(matcher) != fmt.Sprint(test.expected) {
-			t.Errorf("Test %v: Expected %v, found %v", i,
-				fmt.Sprint(test.expected), fmt.Sprint(matcher))
+
+		if len(test_if.ifs) != len(test.expected.ifs) {
+			t.Errorf("Test %d: Expected %d ifConditions, found %v", i,
+				len(test.expected.ifs), len(test_if.ifs))
+		}
+
+		for j, if_c := range test_if.ifs {
+			expected_c := test.expected.ifs[j]
+
+			if if_c.a != expected_c.a {
+				t.Errorf("Test %d, ifCond %d: Expected A=%s, got %s",
+					i, j, if_c.a, expected_c.a)
+			}
+
+			if if_c.op != expected_c.op {
+				t.Errorf("Test %d, ifCond %d: Expected Op=%s, got %s",
+					i, j, if_c.op, expected_c.op)
+			}
+
+			if if_c.b != expected_c.b {
+				t.Errorf("Test %d, ifCond %d: Expected B=%s, got %s",
+					i, j, if_c.b, expected_c.b)
+			}
+
+			if if_c.neg != expected_c.neg {
+				t.Errorf("Test %d, ifCond %d: Expected Neg=%v, got %v",
+					i, j, if_c.neg, expected_c.neg)
+			}
+
+			if expected_c.rex != nil && if_c.rex == nil {
+				t.Errorf("Test %d, ifCond %d: Expected Rex=%v, got <nil>",
+					i, j, expected_c.rex)
+			}
+
+			if expected_c.rex == nil && if_c.rex != nil {
+				t.Errorf("Test %d, ifCond %d: Expected Rex=<nil>, got %v",
+					i, j, if_c.rex)
+			}
+
+			if expected_c.rex != nil && if_c.rex != nil {
+				if if_c.rex.String() != expected_c.rex.String() {
+					t.Errorf("Test %d, ifCond %d: Expected Rex=%v, got %v",
+						i, j, if_c.rex, expected_c.rex)
+				}
+			}
 		}
 	}
 }

caddyhttp/rewrite/rewrite.go

@@ -84,19 +84,19 @@ type ComplexRule struct {
 	// Request matcher
 	httpserver.RequestMatcher
 
-	*regexp.Regexp
+	Regexp *regexp.Regexp
 }
 
 // NewComplexRule creates a new RegexpRule. It returns an error if regexp
 // pattern (pattern) or extensions (ext) are invalid.
-func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.RequestMatcher) (*ComplexRule, error) {
+func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.RequestMatcher) (ComplexRule, error) {
 	// validate regexp if present
 	var r *regexp.Regexp
 	if pattern != "" {
 		var err error
 		r, err = regexp.Compile(pattern)
 		if err != nil {
-			return nil, err
+			return ComplexRule{}, err
 		}
 	}
 
@@ -105,7 +105,7 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 		if len(v) < 2 || (len(v) < 3 && v[0] == '!') {
 			// check if no extension is specified
 			if v != "/" && v != "!/" {
-				return nil, fmt.Errorf("invalid extension %v", v)
+				return ComplexRule{}, fmt.Errorf("invalid extension %v", v)
 			}
 		}
 	}
@@ -118,7 +118,7 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 		httpserver.PathMatcher(base),
 	)
 
-	return &ComplexRule{
+	return ComplexRule{
 		Base:           base,
 		To:             to,
 		Exts:           ext,
@@ -128,13 +128,13 @@ func NewComplexRule(base, pattern, to string, ext []string, matcher httpserver.R
 }
 
 // BasePath satisfies httpserver.Config
-func (r *ComplexRule) BasePath() string { return r.Base }
+func (r ComplexRule) BasePath() string { return r.Base }
 
 // Match satisfies httpserver.Config.
 //
 // Though ComplexRule embeds a RequestMatcher, additional
 // checks are needed which requires a custom implementation.
-func (r *ComplexRule) Match(req *http.Request) bool {
+func (r ComplexRule) Match(req *http.Request) bool {
 	// validate RequestMatcher
 	// includes if and path
 	if !r.RequestMatcher.Match(req) {
@@ -155,7 +155,7 @@ func (r *ComplexRule) Match(req *http.Request) bool {
 }
 
 // Rewrite rewrites the internal location of the current request.
-func (r *ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result) {
+func (r ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result) {
 	replacer := newReplacer(req)
 
 	// validate regexp if present
@@ -189,7 +189,7 @@ func (r *ComplexRule) Rewrite(fs http.FileSystem, req *http.Request) (re Result)
 
 // matchExt matches rPath against registered file extensions.
 // Returns true if a match is found and false otherwise.
-func (r *ComplexRule) matchExt(rPath string) bool {
+func (r ComplexRule) matchExt(rPath string) bool {
 	f := filepath.Base(rPath)
 	ext := path.Ext(f)
 	if ext == "" {
@@ -216,14 +216,14 @@ func (r *ComplexRule) matchExt(rPath string) bool {
 	return !mustUse
 }
 
-func (r *ComplexRule) regexpMatches(rPath string) []string {
+func (r ComplexRule) regexpMatches(rPath string) []string {
 	if r.Regexp != nil {
 		// include trailing slash in regexp if present
 		start := len(r.Base)
 		if strings.HasSuffix(r.Base, "/") {
 			start--
 		}
-		return r.FindStringSubmatch(rPath[start:])
+		return r.Regexp.FindStringSubmatch(rPath[start:])
 	}
 	return nil
 }

caddyhttp/rewrite/setup_test.go

@@ -3,6 +3,7 @@ package rewrite
 import (
 	"fmt"
 	"regexp"
+	"strings"
 	"testing"
 
 	"github.com/mholt/caddy"
@@ -97,14 +98,14 @@ func TestRewriteParse(t *testing.T) {
 			r	.*
 			to	/to /index.php?
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to /index.php?", Regexp: regexp.MustCompile(".*")},
+			ComplexRule{Base: "/", To: "/to /index.php?", Regexp: regexp.MustCompile(".*")},
 		}},
 		{`rewrite {
 			regexp	.*
 			to		/to
 			ext		/ html txt
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to", Exts: []string{"/", "html", "txt"}, Regexp: regexp.MustCompile(".*")},
+			ComplexRule{Base: "/", To: "/to", Exts: []string{"/", "html", "txt"}, Regexp: regexp.MustCompile(".*")},
 		}},
 		{`rewrite /path {
 			r	rr
@@ -115,27 +116,27 @@ func TestRewriteParse(t *testing.T) {
 		 	to 		/to /to2
 		 }
 		 `, false, []Rule{
-			&ComplexRule{Base: "/path", To: "/dest", Regexp: regexp.MustCompile("rr")},
-			&ComplexRule{Base: "/", To: "/to /to2", Regexp: regexp.MustCompile("[a-z]+")},
+			ComplexRule{Base: "/path", To: "/dest", Regexp: regexp.MustCompile("rr")},
+			ComplexRule{Base: "/", To: "/to /to2", Regexp: regexp.MustCompile("[a-z]+")},
 		}},
 		{`rewrite {
 			r	.*
 		 }`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite {
 
 		 }`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite /`, true, []Rule{
-			&ComplexRule{},
+			ComplexRule{},
 		}},
 		{`rewrite {
 			if {path} match /
 			to		/to
 		 }`, false, []Rule{
-			&ComplexRule{Base: "/", To: "/to"},
+			ComplexRule{Base: "/", To: "/to"},
 		}},
 	}
 
@@ -156,8 +157,8 @@ func TestRewriteParse(t *testing.T) {
 		}
 
 		for j, e := range test.expected {
-			actualRule := actual[j].(*ComplexRule)
-			expectedRule := e.(*ComplexRule)
+			actualRule := actual[j].(ComplexRule)
+			expectedRule := e.(ComplexRule)
 
 			if actualRule.Base != expectedRule.Base {
 				t.Errorf("Test %d, rule %d: Expected Base=%s, got %s",
@@ -175,13 +176,15 @@ func TestRewriteParse(t *testing.T) {
 			}
 
 			if actualRule.Regexp != nil {
-				if actualRule.String() != expectedRule.String() {
+				if actualRule.Regexp.String() != expectedRule.Regexp.String() {
 					t.Errorf("Test %d, rule %d: Expected Pattern=%s, got %s",
-						i, j, expectedRule.String(), actualRule.String())
+						i, j, actualRule.Regexp.String(), expectedRule.Regexp.String())
+				}
 			}
 		}
 
+		if rules_fmt := fmt.Sprintf("%v", actual); strings.HasPrefix(rules_fmt, "%!") {
+			t.Errorf("Test %d: Failed to string encode: %#v", i, rules_fmt)
 		}
 	}
-
 }