From d48e51cb78809a19700a530fa1892c07a1c2b50c Mon Sep 17 00:00:00 2001 From: Simon Lightfoot Date: Sun, 30 Jul 2017 16:33:59 +0100 Subject: [PATCH] Changed IfCond to store the condition function and the compiled regular expression. Updated ifCondition test to deep test all fields. Changed NewComplexRule to not return a pointer. Corrected panic detection in formatting. Fixed failing test cases. Fixed review bug for test. Fixes bug caused by Replacer running on the regular expressions in IfMatcher. We also now compile regular expressions up front to detect errors. Fixes rewrite bugs that come from formatting a rule as a string and failing with nil dereference caused by embedding Regexp pointer in a Rule. Re: Issue #1794 --- caddyhttp/httpserver/condition.go | 126 +++++++++---------- caddyhttp/httpserver/condition_test.go | 167 ++++++++++++++++--------- caddyhttp/rewrite/rewrite.go | 22 ++-- caddyhttp/rewrite/setup_test.go | 29 +++-- 4 files changed, 194 insertions(+), 150 deletions(-) diff --git a/caddyhttp/httpserver/condition.go b/caddyhttp/httpserver/condition.go index 7722abd19..81f9ece77 100644 --- a/caddyhttp/httpserver/condition.go +++ b/caddyhttp/httpserver/condition.go @@ -53,59 +53,8 @@ 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, -} - -// isFunc is condition for Is operator. -// It checks for equality. -func isFunc(a, b string) bool { - return a == b -} - -// notFunc is condition for Not operator. -// It checks for inequality. -func notFunc(a, b string) bool { - return !isFunc(a, b) -} - -// 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) -} - -// 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) -} - -// 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) -} - -// 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 -} +type ifFunc func(a, b string) bool // ifCond is statement for a IfMatcher condition. type ifCond struct { @@ -113,40 +62,79 @@ type ifCond struct { op string b string neg bool + rex *regexp.Regexp + f ifFunc } // 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:] +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:] } - if _, ok := ifConditions[operator]; !ok { - return ifCond{}, operatorError(operator) + + 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) } - return ifCond{ - a: a, - op: operator, - b: b, - neg: neg, - }, nil + + return i, nil +} + +// isFunc is condition for Is operator. +func (i ifCond) isFunc(a, b string) bool { + return 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. +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) - b = replacer.Replace(i.b) + 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 } diff --git a/caddyhttp/httpserver/condition_test.go b/caddyhttp/httpserver/condition_test.go index c187b33a0..a63cc997b 100644 --- a/caddyhttp/httpserver/condition_test.go +++ b/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 { - t.Error(err) + 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 ", + i, j, expected_c.rex) + } + + if expected_c.rex == nil && if_c.rex != nil { + t.Errorf("Test %d, ifCond %d: Expected Rex=, 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) + } + } } } } diff --git a/caddyhttp/rewrite/rewrite.go b/caddyhttp/rewrite/rewrite.go index 5c8fd5ff7..14d25ca58 100644 --- a/caddyhttp/rewrite/rewrite.go +++ b/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 } diff --git a/caddyhttp/rewrite/setup_test.go b/caddyhttp/rewrite/setup_test.go index fd355102c..4e32c2eed 100644 --- a/caddyhttp/rewrite/setup_test.go +++ b/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) } } - }