From b64894c31e1ecf3bb07b422e19a6e7b86ac24445 Mon Sep 17 00:00:00 2001 From: Tw Date: Mon, 1 Aug 2016 14:38:18 +0800 Subject: [PATCH] redir: loading block arguments before parsing matcher fix issue #977 Signed-off-by: Tw --- caddyhttp/redirect/setup.go | 131 ++++++++++++------------------- caddyhttp/redirect/setup_test.go | 37 +++++++-- 2 files changed, 83 insertions(+), 85 deletions(-) diff --git a/caddyhttp/redirect/setup.go b/caddyhttp/redirect/setup.go index 0880ddcb..1a97ffef 100644 --- a/caddyhttp/redirect/setup.go +++ b/caddyhttp/redirect/setup.go @@ -33,15 +33,49 @@ func redirParse(c *caddy.Controller) ([]Rule, error) { cfg := httpserver.GetConfig(c) - // setRedirCode sets the redirect code for rule if it can, or returns an error - setRedirCode := func(code string, rule *Rule) error { + initRule := func(rule *Rule, defaultCode string, args []string) error { + if cfg.TLS.Enabled { + rule.FromScheme = "https" + } else { + rule.FromScheme = "http" + } + + var ( + from = "/" + to string + code = defaultCode + ) + switch len(args) { + case 1: + // To specified (catch-all redirect) + // Not sure why user is doing this in a table, as it causes all other redirects to be ignored. + // As such, this feature remains undocumented. + to = args[0] + case 2: + // From and To specified + from = args[0] + to = args[1] + case 3: + // From, To, and Code specified + from = args[0] + to = args[1] + code = args[2] + default: + return c.ArgErr() + } + + rule.FromPath = from + rule.To = to if code == "meta" { rule.Meta = true - } else if codeNumber, ok := httpRedirs[code]; ok { + code = defaultCode + } + if codeNumber, ok := httpRedirs[code]; ok { rule.Code = codeNumber } else { return c.Errf("Invalid redirect code '%v'", code) } + return nil } @@ -62,12 +96,14 @@ func redirParse(c *caddy.Controller) ([]Rule, error) { return nil } + const initDefaultCode = "301" + for c.Next() { + args := c.RemainingArgs() matcher, err := httpserver.SetupIfMatcher(c) if err != nil { return nil, err } - args := c.RemainingArgs() var hadOptionalBlock bool for c.NextBlock() { @@ -77,103 +113,40 @@ func redirParse(c *caddy.Controller) ([]Rule, error) { hadOptionalBlock = true - var rule = Rule{ + rule := Rule{ RequestMatcher: matcher, } - if cfg.TLS.Enabled { - rule.FromScheme = "https" - } else { - rule.FromScheme = "http" - } - + defaultCode := initDefaultCode // Set initial redirect code - // BUG: If the code is specified for a whole block and that code is invalid, - // the line number will appear on the first line inside the block, even if that - // line overwrites the block-level code with a valid redirect code. The program - // still functions correctly, but the line number in the error reporting is - // misleading to the user. if len(args) == 1 { - err := setRedirCode(args[0], &rule) - if err != nil { - return redirects, err - } - } else { - rule.Code = http.StatusMovedPermanently // default code + defaultCode = args[0] } // RemainingArgs only gets the values after the current token, but in our // case we want to include the current token to get an accurate count. insideArgs := append([]string{c.Val()}, c.RemainingArgs()...) - - switch len(insideArgs) { - case 1: - // To specified (catch-all redirect) - // Not sure why user is doing this in a table, as it causes all other redirects to be ignored. - // As such, this feature remains undocumented. - rule.FromPath = "/" - rule.To = insideArgs[0] - case 2: - // From and To specified - rule.FromPath = insideArgs[0] - rule.To = insideArgs[1] - case 3: - // From, To, and Code specified - rule.FromPath = insideArgs[0] - rule.To = insideArgs[1] - err := setRedirCode(insideArgs[2], &rule) - if err != nil { - return redirects, err - } - default: - return redirects, c.ArgErr() + err := initRule(&rule, defaultCode, insideArgs) + if err != nil { + return redirects, err } - err := checkAndSaveRule(rule) + err = checkAndSaveRule(rule) if err != nil { return redirects, err } } if !hadOptionalBlock { - var rule = Rule{ + rule := Rule{ RequestMatcher: matcher, } - - if cfg.TLS.Enabled { - rule.FromScheme = "https" - } else { - rule.FromScheme = "http" + err := initRule(&rule, initDefaultCode, args) + if err != nil { + return redirects, err } - rule.Code = http.StatusMovedPermanently // default - - switch len(args) { - case 1: - // To specified (catch-all redirect) - rule.FromPath = "/" - rule.To = args[0] - case 2: - // To and Code specified (catch-all redirect) - rule.FromPath = "/" - rule.To = args[0] - err := setRedirCode(args[1], &rule) - if err != nil { - return redirects, err - } - case 3: - // From, To, and Code specified - rule.FromPath = args[0] - rule.To = args[1] - err := setRedirCode(args[2], &rule) - if err != nil { - return redirects, err - } - default: - return redirects, c.ArgErr() - } - - err := checkAndSaveRule(rule) + err = checkAndSaveRule(rule) if err != nil { return redirects, err } diff --git a/caddyhttp/redirect/setup_test.go b/caddyhttp/redirect/setup_test.go index 4ebea3e8..5eb04eb8 100644 --- a/caddyhttp/redirect/setup_test.go +++ b/caddyhttp/redirect/setup_test.go @@ -1,6 +1,7 @@ package redirect import ( + "fmt" "testing" "github.com/mholt/caddy" @@ -15,7 +16,7 @@ func TestSetup(t *testing.T) { expectedRules []Rule }{ // test case #0 tests the recognition of a valid HTTP status code defined outside of block statement - {"redir 300 {\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300}}}, + {"redir 300 {\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #1 tests the recognition of an invalid HTTP status code defined outside of block statement {"redir 9000 {\n/ /foo\n}", true, []Rule{{}}}, @@ -27,22 +28,43 @@ func TestSetup(t *testing.T) { {"redir 9000 {\n/ /foo 300\n}", true, []Rule{{}}}, // test case #4 tests the recognition of a TO redirection in a block statement.The HTTP status code is set to the default of 301 - MovedPermanently - {"redir 302 {\n/foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 302}}}, + {"redir 302 {\n/foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 302, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #5 tests the recognition of a TO and From redirection in a block statement - {"redir {\n/bar /foo 303\n}", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303}}}, + {"redir {\n/bar /foo 303\n}", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #6 tests the recognition of a TO redirection in a non-block statement. The HTTP status code is set to the default of 301 - MovedPermanently - {"redir /foo", false, []Rule{{FromPath: "/", To: "/foo", Code: 301}}}, + {"redir /foo", false, []Rule{{FromPath: "/", To: "/foo", Code: 301, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #7 tests the recognition of a TO and From redirection in a non-block statement - {"redir /bar /foo 303", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303}}}, + {"redir /bar /foo 303", false, []Rule{{FromPath: "/bar", To: "/foo", Code: 303, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #8 tests the recognition of multiple redirections - {"redir {\n / /foo 304 \n} \n redir {\n /bar /foobar 305 \n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 304}, {FromPath: "/bar", To: "/foobar", Code: 305}}}, + {"redir {\n / /foo 304 \n} \n redir {\n /bar /foobar 305 \n}", false, + []Rule{{FromPath: "/", To: "/foo", Code: 304, RequestMatcher: httpserver.IfMatcher{}}, + {FromPath: "/bar", To: "/foobar", Code: 305, RequestMatcher: httpserver.IfMatcher{}}}}, // test case #9 tests the detection of duplicate redirections {"redir {\n /bar /foo 304 \n} redir {\n /bar /foo 304 \n}", true, []Rule{{}}}, + + // test case #10 tests the detection of a valid HTTP status code outside of a block statement being overridden by an valid HTTP status code inside statement of a block statement + {"redir 300 {\n/ /foo 301\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 301, RequestMatcher: httpserver.IfMatcher{}}}}, + + // test case #11 tests the recognition of a matcher + {"redir {\n if {port} is 80\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 301, + RequestMatcher: func() httpserver.IfMatcher { + c := caddy.NewTestController("http", "{\n if {port} is 80\n}") + matcher, _ := httpserver.SetupIfMatcher(c) + return matcher.(httpserver.IfMatcher) + }()}}}, + + // test case #12 tests the detection of a valid HTTP status code outside of a block statement with a matcher + {"redir 300 {\n if {port} is 80\n/ /foo\n}", false, []Rule{{FromPath: "/", To: "/foo", Code: 300, + RequestMatcher: func() httpserver.IfMatcher { + c := caddy.NewTestController("http", "{\n if {port} is 80\n}") + matcher, _ := httpserver.SetupIfMatcher(c) + return matcher.(httpserver.IfMatcher) + }()}}}, } { c := caddy.NewTestController("http", test.input) err := setup(c) @@ -64,6 +86,9 @@ func TestSetup(t *testing.T) { if recievedRule.Code != test.expectedRules[i].Code { t.Errorf("Test case #%d.%d expected a HTTP status code of %d, but recieved a code of %d", j, i, test.expectedRules[i].Code, recievedRule.Code) } + if gotMatcher, expectMatcher := fmt.Sprint(recievedRule.RequestMatcher), fmt.Sprint(test.expectedRules[i].RequestMatcher); gotMatcher != expectMatcher { + t.Errorf("Test case #%d.%d expected a Matcher %s, but recieved a Matcher %s", j, i, expectMatcher, gotMatcher) + } } }