From 3f9f675c43040713a259946a34a76b74bf278b2a Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Wed, 30 Sep 2015 08:38:31 -0600 Subject: [PATCH] redir: Include scheme in redirect rules And added tests for status code and scheme --- config/setup/redir.go | 45 ++++++++++++++++--------- middleware/redirect/redirect.go | 13 +++++--- middleware/redirect/redirect_test.go | 50 +++++++++++++++++++--------- 3 files changed, 73 insertions(+), 35 deletions(-) diff --git a/config/setup/redir.go b/config/setup/redir.go index 008ba6c77..63488f4ab 100644 --- a/config/setup/redir.go +++ b/config/setup/redir.go @@ -37,13 +37,13 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { // checkAndSaveRule checks the rule for validity (except the redir code) // and saves it if it's valid, or returns an error. checkAndSaveRule := func(rule redirect.Rule) error { - if rule.From == rule.To { + if rule.FromPath == rule.To { return c.Err("'from' and 'to' values of redirect rule cannot be the same") } for _, otherRule := range redirects { - if otherRule.From == rule.From { - return c.Errf("rule with duplicate 'from' value: %s -> %s", otherRule.From, otherRule.To) + if otherRule.FromPath == rule.FromPath { + return c.Errf("rule with duplicate 'from' value: %s -> %s", otherRule.FromPath, otherRule.To) } } @@ -60,6 +60,12 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { var rule redirect.Rule + if c.Config.TLS.Enabled { + rule.FromScheme = "https" + } else { + rule.FromScheme = "http" + } + // 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 @@ -84,15 +90,15 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { // 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.From = "/" + rule.FromPath = "/" rule.To = insideArgs[0] case 2: // From and To specified - rule.From = insideArgs[0] + rule.FromPath = insideArgs[0] rule.To = insideArgs[1] case 3: // From, To, and Code specified - rule.From = insideArgs[0] + rule.FromPath = insideArgs[0] rule.To = insideArgs[1] err := setRedirCode(insideArgs[2], &rule) if err != nil { @@ -110,16 +116,23 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { if !hadOptionalBlock { var rule redirect.Rule + + if c.Config.TLS.Enabled { + rule.FromScheme = "https" + } else { + rule.FromScheme = "http" + } + rule.Code = http.StatusMovedPermanently // default switch len(args) { case 1: // To specified (catch-all redirect) - rule.From = "/" + rule.FromPath = "/" rule.To = args[0] case 2: // To and Code specified (catch-all redirect) - rule.From = "/" + rule.FromPath = "/" rule.To = args[0] err := setRedirCode(args[1], &rule) if err != nil { @@ -127,7 +140,7 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { } case 3: // From, To, and Code specified - rule.From = args[0] + rule.FromPath = args[0] rule.To = args[1] err := setRedirCode(args[2], &rule) if err != nil { @@ -149,12 +162,12 @@ func redirParse(c *Controller) ([]redirect.Rule, error) { // httpRedirs is a list of supported HTTP redirect codes. var httpRedirs = map[string]int{ - "300": 300, // Multiple Choices - "301": 301, // Moved Permanently - "302": 302, // Found (NOT CORRECT for "Temporary Redirect", see 307) - "303": 303, // See Other - "304": 304, // Not Modified - "305": 305, // Use Proxy - "307": 307, // Temporary Redirect + "300": http.StatusMultipleChoices, + "301": http.StatusMovedPermanently, + "302": http.StatusFound, // (NOT CORRECT for "Temporary Redirect", see 307) + "303": http.StatusSeeOther, + "304": http.StatusNotModified, + "305": http.StatusUseProxy, + "307": http.StatusTemporaryRedirect, "308": 308, // Permanent Redirect } diff --git a/middleware/redirect/redirect.go b/middleware/redirect/redirect.go index 6f9ab35e4..04fb1c63a 100644 --- a/middleware/redirect/redirect.go +++ b/middleware/redirect/redirect.go @@ -19,7 +19,7 @@ type Redirect struct { // ServeHTTP implements the middleware.Handler interface. func (rd Redirect) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) { for _, rule := range rd.Rules { - if rule.From == "/" || r.URL.Path == rule.From { + if (rule.FromPath == "/" || r.URL.Path == rule.FromPath) && schemeMatches(rule, r) { to := middleware.NewReplacer(r, nil, "").Replace(rule.To) if rule.Meta { safeTo := html.EscapeString(to) @@ -33,11 +33,16 @@ func (rd Redirect) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error return rd.Next.ServeHTTP(w, r) } +func schemeMatches(rule Rule, req *http.Request) bool { + return (rule.FromScheme == "https" && req.TLS != nil) || + (rule.FromScheme != "https" && req.TLS == nil) +} + // Rule describes an HTTP redirect rule. type Rule struct { - From, To string - Code int - Meta bool + FromScheme, FromPath, To string + Code int + Meta bool } // Script tag comes first since that will better imitate a redirect in the browser's diff --git a/middleware/redirect/redirect_test.go b/middleware/redirect/redirect_test.go index fd98b33a2..b9096564a 100644 --- a/middleware/redirect/redirect_test.go +++ b/middleware/redirect/redirect_test.go @@ -2,9 +2,11 @@ package redirect import ( "bytes" + "crypto/tls" "io/ioutil" "net/http" "net/http/httptest" + "strings" "testing" "github.com/mholt/caddy/middleware" @@ -14,15 +16,22 @@ func TestRedirect(t *testing.T) { for i, test := range []struct { from string expectedLocation string + expectedCode int }{ - {"/from", "/to"}, - {"/a", "/b"}, - {"/aa", ""}, - {"/", ""}, - {"/a?foo=bar", "/b"}, - {"/asdf?foo=bar", ""}, - {"/foo#bar", ""}, - {"/a#foo", "/b"}, + {"http://localhost/from", "/to", http.StatusMovedPermanently}, + {"http://localhost/a", "/b", http.StatusTemporaryRedirect}, + {"http://localhost/aa", "", http.StatusOK}, + {"http://localhost/", "", http.StatusOK}, + {"http://localhost/a?foo=bar", "/b", http.StatusTemporaryRedirect}, + {"http://localhost/asdf?foo=bar", "", http.StatusOK}, + {"http://localhost/foo#bar", "", http.StatusOK}, + {"http://localhost/a#foo", "/b", http.StatusTemporaryRedirect}, + {"http://localhost/scheme", "https://localhost/scheme", http.StatusMovedPermanently}, + {"https://localhost/scheme", "", http.StatusOK}, + {"https://localhost/scheme2", "http://localhost/scheme2", http.StatusMovedPermanently}, + {"http://localhost/scheme2", "", http.StatusOK}, + {"http://localhost/scheme3", "https://localhost/scheme3", http.StatusMovedPermanently}, + {"https://localhost/scheme3", "", http.StatusOK}, } { var nextCalled bool @@ -32,8 +41,11 @@ func TestRedirect(t *testing.T) { return 0, nil }), Rules: []Rule{ - {From: "/from", To: "/to"}, - {From: "/a", To: "/b"}, + {FromPath: "/from", To: "/to", Code: http.StatusMovedPermanently}, + {FromPath: "/a", To: "/b", Code: http.StatusTemporaryRedirect}, + {FromScheme: "http", FromPath: "/scheme", To: "https://localhost/scheme", Code: http.StatusMovedPermanently}, + {FromScheme: "https", FromPath: "/scheme2", To: "http://localhost/scheme2", Code: http.StatusMovedPermanently}, + {FromScheme: "", FromPath: "/scheme3", To: "https://localhost/scheme3", Code: http.StatusMovedPermanently}, }, } @@ -41,6 +53,9 @@ func TestRedirect(t *testing.T) { if err != nil { t.Fatalf("Test %d: Could not create HTTP request: %v", i, err) } + if strings.HasPrefix(test.from, "https://") { + req.TLS = new(tls.ConnectionState) // faux HTTPS + } rec := httptest.NewRecorder() re.ServeHTTP(rec, req) @@ -50,6 +65,11 @@ func TestRedirect(t *testing.T) { i, test.expectedLocation, rec.Header().Get("Location")) } + if rec.Code != test.expectedCode { + t.Errorf("Test %d: Expected status code to be %d but was %d", + i, test.expectedCode, rec.Code) + } + if nextCalled && test.expectedLocation != "" { t.Errorf("Test %d: Next handler was unexpectedly called", i) } @@ -59,7 +79,7 @@ func TestRedirect(t *testing.T) { func TestParametersRedirect(t *testing.T) { re := Redirect{ Rules: []Rule{ - {From: "/", Meta: false, To: "http://example.com{uri}"}, + {FromPath: "/", Meta: false, To: "http://example.com{uri}"}, }, } @@ -77,7 +97,7 @@ func TestParametersRedirect(t *testing.T) { re = Redirect{ Rules: []Rule{ - {From: "/", Meta: false, To: "http://example.com/a{path}?b=c&{query}"}, + {FromPath: "/", Meta: false, To: "http://example.com/a{path}?b=c&{query}"}, }, } @@ -96,13 +116,13 @@ func TestParametersRedirect(t *testing.T) { func TestMetaRedirect(t *testing.T) { re := Redirect{ Rules: []Rule{ - {From: "/whatever", Meta: true, To: "/something"}, - {From: "/", Meta: true, To: "https://example.com/"}, + {FromPath: "/whatever", Meta: true, To: "/something"}, + {FromPath: "/", Meta: true, To: "https://example.com/"}, }, } for i, test := range re.Rules { - req, err := http.NewRequest("GET", test.From, nil) + req, err := http.NewRequest("GET", test.FromPath, nil) if err != nil { t.Fatalf("Test %d: Could not create HTTP request: %v", i, err) }