From ea40df699b8c3f3fc0f8b1db309ef83e2afc0858 Mon Sep 17 00:00:00 2001 From: Sebastian Mancke Date: Tue, 9 Jan 2018 22:29:59 +0100 Subject: [PATCH] Overworked the dynamic redirect patch * Reduced nesting in functions with early returns * Some renamings * Resolve whitelist path relative to caddy config * Moved all redirect methods to redirect.go and redirect_test.go * Only read the url direct from the parameter, if it is a post request * Removed the parameter prevent-external-redirects, because it can be implicit assumed, when a whitelist is configured --- caddy/setup.go | 8 ++- caddy/setup_test.go | 120 ++++++++++++++++----------------- login/config.go | 85 ++++++++++++----------- login/config_test.go | 88 ++++++++++++------------ login/handler.go | 22 +----- login/handler_test.go | 108 ------------------------------ login/redirect.go | 148 +++++++++++++++++++---------------------- login/redirect_test.go | 126 +++++++++++++++++++++++++++++++++++ 8 files changed, 347 insertions(+), 358 deletions(-) create mode 100644 login/redirect_test.go diff --git a/caddy/setup.go b/caddy/setup.go index 10362d5..926608f 100644 --- a/caddy/setup.go +++ b/caddy/setup.go @@ -43,6 +43,10 @@ func setup(c *caddy.Controller) error { config.Template = filepath.Join(httpserver.GetConfig(c).Root, config.Template) } + if config.WhitelistDomainsFile != "" && !filepath.IsAbs(config.WhitelistDomainsFile) { + config.WhitelistDomainsFile = filepath.Join(httpserver.GetConfig(c).Root, config.WhitelistDomainsFile) + } + if len(args) == 1 { logging.Logger.Warnf("DEPRECATED: Please set the login path by parameter login_path and not as directive argument (%v:%v)", c.File(), c.Line()) config.LoginPath = path.Join(args[0], "/login") @@ -89,11 +93,11 @@ func parseConfig(c *caddy.Controller) (*login.Config, error) { f := fs.Lookup(name) if f == nil { - return cfg, c.ArgErr() + return cfg, fmt.Errorf("Unknown parameter for login directive: %v (%v:%v)", name, c.File(), c.Line()) } err := f.Value.Set(value) if err != nil { - return cfg, c.Err(err.Error()) + return cfg, fmt.Errorf("Invalid value for parameter %v: %v (%v:%v)", name, value, c.File(), c.Line()) } } diff --git a/caddy/setup_test.go b/caddy/setup_test.go index 9a894be..613db2f 100644 --- a/caddy/setup_test.go +++ b/caddy/setup_test.go @@ -4,7 +4,6 @@ import ( "fmt" "io/ioutil" "os" - "path/filepath" "testing" "time" @@ -29,16 +28,15 @@ func TestSetup(t *testing.T) { }`, shouldErr: false, config: login.Config{ - JwtSecret: "jwtsecret", - JwtExpiry: 24 * time.Hour, - SuccessURL: "/", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - LoginPath: "/login", - CookieName: "jwt_token", - CookieHTTPOnly: true, + JwtSecret: "jwtsecret", + JwtExpiry: 24 * time.Hour, + SuccessURL: "/", + AllowRedirects: true, + RedirectQueryParameter: "backTo", + CheckRefererOnRedirects: true, + LoginPath: "/login", + CookieName: "jwt_token", + CookieHTTPOnly: true, Backends: login.Options{ "simple": map[string]string{ "bob": "secret", @@ -52,9 +50,10 @@ func TestSetup(t *testing.T) { success_url successurl jwt_expiry 42h login_path /foo/bar - allow-redirects true - prevent-external-redirects true - check-referer-on-redirects true + allow_redirects true + redirect_query_parameter comingFrom + check_referer_on_redirects true + whitelist_domains_file domainWhitelist.txt cookie_name cookiename cookie_http_only false cookie_domain example.com @@ -64,18 +63,18 @@ func TestSetup(t *testing.T) { }`, shouldErr: false, config: login.Config{ - JwtSecret: "jwtsecret", - JwtExpiry: 42 * time.Hour, - SuccessURL: "successurl", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - LoginPath: "/foo/bar", - CookieName: "cookiename", - CookieDomain: "example.com", - CookieExpiry: 23*time.Hour + 23*time.Minute, - CookieHTTPOnly: false, + JwtSecret: "jwtsecret", + JwtExpiry: 42 * time.Hour, + SuccessURL: "successurl", + AllowRedirects: true, + RedirectQueryParameter: "comingFrom", + CheckRefererOnRedirects: true, + WhitelistDomainsFile: "domainWhitelist.txt", + LoginPath: "/foo/bar", + CookieName: "cookiename", + CookieDomain: "example.com", + CookieExpiry: 23*time.Hour + 23*time.Minute, + CookieHTTPOnly: false, Backends: login.Options{ "simple": map[string]string{ "bob": "secret", @@ -99,16 +98,15 @@ func TestSetup(t *testing.T) { }`, shouldErr: false, config: login.Config{ - JwtSecret: "jwtsecret", - JwtExpiry: 24 * time.Hour, - SuccessURL: "/", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - LoginPath: "/context/login", - CookieName: "cookiename", - CookieHTTPOnly: true, + JwtSecret: "jwtsecret", + JwtExpiry: 24 * time.Hour, + SuccessURL: "/", + AllowRedirects: true, + RedirectQueryParameter: "backTo", + CheckRefererOnRedirects: true, + LoginPath: "/context/login", + CookieName: "cookiename", + CookieHTTPOnly: true, Backends: login.Options{ "simple": map[string]string{ "bob": "secret", @@ -127,16 +125,15 @@ func TestSetup(t *testing.T) { }`, shouldErr: false, config: login.Config{ - JwtSecret: "jwtsecret", - JwtExpiry: 24 * time.Hour, - SuccessURL: "/", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - LoginPath: "/login", - CookieName: "cookiename", - CookieHTTPOnly: true, + JwtSecret: "jwtsecret", + JwtExpiry: 24 * time.Hour, + SuccessURL: "/", + AllowRedirects: true, + RedirectQueryParameter: "backTo", + CheckRefererOnRedirects: true, + LoginPath: "/login", + CookieName: "cookiename", + CookieHTTPOnly: true, Backends: login.Options{ "simple": map[string]string{ "bob": "secret", @@ -153,16 +150,15 @@ func TestSetup(t *testing.T) { }`, shouldErr: false, config: login.Config{ - JwtSecret: "jwtsecret", - JwtExpiry: 24 * time.Hour, - SuccessURL: "/", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - LoginPath: "/login", - CookieName: "jwt_token", - CookieHTTPOnly: true, + JwtSecret: "jwtsecret", + JwtExpiry: 24 * time.Hour, + SuccessURL: "/", + AllowRedirects: true, + RedirectQueryParameter: "backTo", + CheckRefererOnRedirects: true, + LoginPath: "/login", + CookieName: "jwt_token", + CookieHTTPOnly: true, Backends: login.Options{ "simple": map[string]string{ "bob": "secret", @@ -198,10 +194,13 @@ func TestSetup(t *testing.T) { } } -func TestSetup_RelativeTemplateFile(t *testing.T) { - caddyfile := "loginsrv {\n template myTemplate.tpl\n simple bob=secret\n}" +func TestSetup_RelativeFiles(t *testing.T) { + caddyfile := `loginsrv { + template myTemplate.tpl + whitelist_domains_file redirectDomains.txt + simple bob=secret + }` root, _ := ioutil.TempDir("", "") - expectedPath := filepath.FromSlash(root + "/myTemplate.tpl") c := caddy.NewTestController("http", caddyfile) c.Key = "RelativeTemplateFileTest" @@ -216,5 +215,6 @@ func TestSetup_RelativeTemplateFile(t *testing.T) { } middleware := mids[len(mids)-1](nil).(*CaddyHandler) - Equal(t, expectedPath, middleware.config.Template) + Equal(t, root+"/myTemplate.tpl", middleware.config.Template) + Equal(t, root+"/redirectDomains.txt", middleware.config.WhitelistDomainsFile) } diff --git a/login/config.go b/login/config.go index ee14275..2759b80 100644 --- a/login/config.go +++ b/login/config.go @@ -23,25 +23,24 @@ func init() { // DefaultConfig for the loginsrv handler func DefaultConfig() *Config { return &Config{ - Host: "localhost", - Port: "6789", - LogLevel: "info", - JwtSecret: jwtDefaultSecret, - JwtExpiry: 24 * time.Hour, - JwtRefreshes: 0, - SuccessURL: "/", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - WhitelistDomainsFile: "", - LogoutURL: "", - LoginPath: "/login", - CookieName: "jwt_token", - CookieHTTPOnly: true, - Backends: Options{}, - Oauth: Options{}, - GracePeriod: 5 * time.Second, + Host: "localhost", + Port: "6789", + LogLevel: "info", + JwtSecret: jwtDefaultSecret, + JwtExpiry: 24 * time.Hour, + JwtRefreshes: 0, + SuccessURL: "/", + AllowRedirects: true, + RedirectQueryParameter: "backTo", + CheckRefererOnRedirects: true, + WhitelistDomainsFile: "", + LogoutURL: "", + LoginPath: "/login", + CookieName: "jwt_token", + CookieHTTPOnly: true, + Backends: Options{}, + Oauth: Options{}, + GracePeriod: 5 * time.Second, } } @@ -49,29 +48,28 @@ const envPrefix = "LOGINSRV_" // Config for the loginsrv handler type Config struct { - Host string - Port string - LogLevel string - TextLogging bool - JwtSecret string - JwtExpiry time.Duration - JwtRefreshes int - SuccessURL string - AllowRedirects bool - RedirectQueryParameter string - PreventExternalRedirects bool - CheckRefererOnRedirects bool - WhitelistDomainsFile string - LogoutURL string - Template string - LoginPath string - CookieName string - CookieExpiry time.Duration - CookieDomain string - CookieHTTPOnly bool - Backends Options - Oauth Options - GracePeriod time.Duration + Host string + Port string + LogLevel string + TextLogging bool + JwtSecret string + JwtExpiry time.Duration + JwtRefreshes int + SuccessURL string + AllowRedirects bool + RedirectQueryParameter string + CheckRefererOnRedirects bool + WhitelistDomainsFile string + LogoutURL string + Template string + LoginPath string + CookieName string + CookieExpiry time.Duration + CookieDomain string + CookieHTTPOnly bool + Backends Options + Oauth Options + GracePeriod time.Duration } // Options is the configuration structure for oauth and backend provider @@ -116,9 +114,8 @@ func (c *Config) ConfigureFlagSet(f *flag.FlagSet) { f.StringVar(&c.SuccessURL, "success-url", c.SuccessURL, "The url to redirect after login") f.BoolVar(&c.AllowRedirects, "allow-redirects", c.AllowRedirects, "Allow dynamic redirects by parameter") f.StringVar(&c.RedirectQueryParameter, "redirect-query-parameter", c.RedirectQueryParameter, "Allow dynamic redirects by parameter") - f.BoolVar(&c.PreventExternalRedirects, "prevent-external-redirects", c.PreventExternalRedirects, "Prevent dynamic redirects from redirecting to an external domain") f.BoolVar(&c.CheckRefererOnRedirects, "check-referer-on-redirects", c.CheckRefererOnRedirects, "When redirecting check that the referer is the same domain") - f.StringVar(&c.WhitelistDomainsFile, "whitelist-domains-file", c.WhitelistDomainsFile, "the file containing a list of domains that redirects are allowed to, one domain per line") + f.StringVar(&c.WhitelistDomainsFile, "whitelist-domains-file", c.WhitelistDomainsFile, "A file containing a list of domains that redirects are allowed to, one domain per line") f.StringVar(&c.LogoutURL, "logout-url", c.LogoutURL, "The url or path to redirect after logout") f.StringVar(&c.Template, "template", c.Template, "An alternative template for the login form") diff --git a/login/config_test.go b/login/config_test.go index 95dc59d..93ace72 100644 --- a/login/config_test.go +++ b/login/config_test.go @@ -29,10 +29,9 @@ func TestConfig_ReadConfig(t *testing.T) { "--jwt-secret=jwtsecret", "--jwt-expiry=42h42m", "--success-url=successurl", - "--allow-redirects=true", - "--redirect-query-parameter=backTo", - "--prevent-external-redirects=true", - "--check-referer-on-redirects=true", + "--allow-redirects=false", + "--redirect-query-parameter=comingFrom", + "--check-referer-on-redirects=false", "--whitelist-domains-file=File", "--logout-url=logouturl", "--template=template", @@ -48,25 +47,24 @@ func TestConfig_ReadConfig(t *testing.T) { } expected := &Config{ - Host: "host", - Port: "port", - LogLevel: "loglevel", - TextLogging: true, - JwtSecret: "jwtsecret", - JwtExpiry: 42*time.Hour + 42*time.Minute, - SuccessURL: "successurl", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - WhitelistDomainsFile: "File", - LogoutURL: "logouturl", - Template: "template", - LoginPath: "loginpath", - CookieName: "cookiename", - CookieExpiry: 23 * time.Minute, - CookieDomain: "*.example.com", - CookieHTTPOnly: false, + Host: "host", + Port: "port", + LogLevel: "loglevel", + TextLogging: true, + JwtSecret: "jwtsecret", + JwtExpiry: 42*time.Hour + 42*time.Minute, + SuccessURL: "successurl", + AllowRedirects: false, + RedirectQueryParameter: "comingFrom", + CheckRefererOnRedirects: false, + WhitelistDomainsFile: "File", + LogoutURL: "logouturl", + Template: "template", + LoginPath: "loginpath", + CookieName: "cookiename", + CookieExpiry: 23 * time.Minute, + CookieDomain: "*.example.com", + CookieHTTPOnly: false, Backends: Options{ "simple": map[string]string{}, "foo": map[string]string{}, @@ -93,10 +91,9 @@ func TestConfig_ReadConfigFromEnv(t *testing.T) { NoError(t, os.Setenv("LOGINSRV_JWT_SECRET", "jwtsecret")) NoError(t, os.Setenv("LOGINSRV_JWT_EXPIRY", "42h42m")) NoError(t, os.Setenv("LOGINSRV_SUCCESS_URL", "successurl")) - NoError(t, os.Setenv("LOGINSRV_ALLOW_REDIRECTS", "true")) - NoError(t, os.Setenv("LOGINSRV_REDIRECT_QUERY_PARAMETER", "backTo")) - NoError(t, os.Setenv("LOGINSRV_PREVENT_EXTERNAL_REDIRECTS", "true")) - NoError(t, os.Setenv("LOGINSRV_CHECK_REFERER_ON_REDIRECTS", "true")) + NoError(t, os.Setenv("LOGINSRV_ALLOW_REDIRECTS", "false")) + NoError(t, os.Setenv("LOGINSRV_REDIRECT_QUERY_PARAMETER", "comingFrom")) + NoError(t, os.Setenv("LOGINSRV_CHECK_REFERER_ON_REDIRECTS", "false")) NoError(t, os.Setenv("LOGINSRV_WHITELIST_DOMAINS_FILE", "File")) NoError(t, os.Setenv("LOGINSRV_LOGOUT_URL", "logouturl")) NoError(t, os.Setenv("LOGINSRV_TEMPLATE", "template")) @@ -110,25 +107,24 @@ func TestConfig_ReadConfigFromEnv(t *testing.T) { NoError(t, os.Setenv("LOGINSRV_GRACE_PERIOD", "4s")) expected := &Config{ - Host: "host", - Port: "port", - LogLevel: "loglevel", - TextLogging: true, - JwtSecret: "jwtsecret", - JwtExpiry: 42*time.Hour + 42*time.Minute, - SuccessURL: "successurl", - AllowRedirects: true, - RedirectQueryParameter: "backTo", - PreventExternalRedirects: true, - CheckRefererOnRedirects: true, - WhitelistDomainsFile: "File", - LogoutURL: "logouturl", - Template: "template", - LoginPath: "loginpath", - CookieName: "cookiename", - CookieExpiry: 23 * time.Minute, - CookieDomain: "*.example.com", - CookieHTTPOnly: false, + Host: "host", + Port: "port", + LogLevel: "loglevel", + TextLogging: true, + JwtSecret: "jwtsecret", + JwtExpiry: 42*time.Hour + 42*time.Minute, + SuccessURL: "successurl", + AllowRedirects: false, + RedirectQueryParameter: "comingFrom", + CheckRefererOnRedirects: false, + WhitelistDomainsFile: "File", + LogoutURL: "logouturl", + Template: "template", + LoginPath: "loginpath", + CookieName: "cookiename", + CookieExpiry: 23 * time.Minute, + CookieDomain: "*.example.com", + CookieHTTPOnly: false, Backends: Options{ "simple": map[string]string{ "foo": "bar", diff --git a/login/handler.go b/login/handler.go index f0f7a52..572c2ea 100644 --- a/login/handler.go +++ b/login/handler.go @@ -6,7 +6,6 @@ import ( "fmt" "io/ioutil" "net/http" - "net/url" "strings" "time" @@ -68,16 +67,7 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) { return } - if (h.shouldRedirect(r)) && (r.Method != "POST") { - queries, _ := url.ParseQuery(r.URL.RawQuery) - if queries.Get(h.config.RedirectQueryParameter) != "" { - cookie := http.Cookie{ - Name: h.config.RedirectQueryParameter, - Value: queries.Get(h.config.RedirectQueryParameter), - } - http.SetCookie(w, &cookie) - } - } + h.setRedirectCookie(w, r) _, err := h.oauth.GetConfigFromRequest(r) if err == nil { @@ -245,15 +235,7 @@ func (h *Handler) respondAuthenticated(w http.ResponseWriter, r *http.Request, u http.SetCookie(w, cookie) w.Header().Set("Location", h.redirectURL(r, w)) - _, err := r.Cookie(h.config.RedirectQueryParameter) - if err == nil { - cookie := http.Cookie{ - Name: h.config.RedirectQueryParameter, - Value: "delete", - Expires: time.Unix(0, 0), - } - http.SetCookie(w, &cookie) - } + h.deleteRedirectCookie(w, r) w.WriteHeader(303) return } diff --git a/login/handler_test.go b/login/handler_test.go index 2287a08..e1d659c 100644 --- a/login/handler_test.go +++ b/login/handler_test.go @@ -3,10 +3,8 @@ package login import ( "errors" "fmt" - "io/ioutil" "net/http" "net/http/httptest" - "os" "strconv" "strings" "testing" @@ -22,8 +20,6 @@ const TypeJSON = "Content-Type: application/json" const TypeForm = "Content-Type: application/x-www-form-urlencoded" const AcceptHTML = "Accept: text/html" const AcceptJwt = "Accept: application/jwt" -const Host = "Host: example.com" -const BadReferer = "Referer: http://evildomain.com" func testConfig() *Config { testConfig := DefaultConfig() @@ -243,110 +239,6 @@ func TestHandler_LoginWeb(t *testing.T) { Equal(t, recorder.Header().Get("Set-Cookie"), "") } -func TestHandler_Redirect(t *testing.T) { - //by default set redirect_cookie - recorder := call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML)) - setCookieList := readSetCookies(recorder.Header()) - Equal(t, 1, len(setCookieList)) - cookie := setCookieList[0] - Equal(t, "backTo", cookie.Name) - Equal(t, "/website", cookie.Value) - - //by default allowed redirects - recorder = call(req("POST", "/context/login?backTo=/website", "username=bob&password=secret", TypeForm, AcceptHTML)) - Equal(t, 303, recorder.Code) - Equal(t, "/website", recorder.Header().Get("Location")) - - //redirect to SuccessURL if AllowRedirects is false - cfg := DefaultConfig() - cfg.AllowRedirects = false - h := &Handler{ - backends: []Backend{ - NewSimpleBackend(map[string]string{"bob": "secret"}), - }, - oauth: oauth2.NewManager(), - config: cfg, - } - recorder = httptest.NewRecorder() - h.ServeHTTP(recorder, req("POST", "/login?backTo=/website", "username=bob&password=secret", TypeForm, AcceptHTML)) - Equal(t, 303, recorder.Code) - Equal(t, "/", recorder.Header().Get("Location")) - - //by default don't set redirect cookie if Referer doesn't match origin - recorder = call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML, BadReferer)) - setCookieList = readSetCookies(recorder.Header()) - Equal(t, 0, len(setCookieList)) - - //don't set redirect cookie if referrer is malformed - recorder = call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML, "Referer: :notvalid")) - setCookieList = readSetCookies(recorder.Header()) - Equal(t, 0, len(setCookieList)) - - //set redirect cookie with mismatch referer if CheckRefererOnRedirects is false - cfg = DefaultConfig() - cfg.CheckRefererOnRedirects = false - h = &Handler{ - backends: []Backend{ - NewSimpleBackend(map[string]string{"bob": "secret"}), - }, - oauth: oauth2.NewManager(), - config: cfg, - } - recorder = httptest.NewRecorder() - h.ServeHTTP(recorder, req("GET", "/login?backTo=/website", "", TypeForm, AcceptHTML, BadReferer)) - setCookieList = readSetCookies(recorder.Header()) - Equal(t, 1, len(setCookieList)) - cookie = setCookieList[0] - Equal(t, "backTo", cookie.Name) - Equal(t, "/website", cookie.Value) - - //by default prevent redirect to external site - recorder = call(req("POST", "/context/login?backTo=//evildomain.com/phishing.html", "username=bob&password=secret", TypeForm, AcceptHTML)) - Equal(t, 303, recorder.Code) - Equal(t, "/", recorder.Header().Get("Location")) - - //by default if the parsed path is empty redirect to SuccessURL - recorder = call(req("POST", "/context/login?backTo=https://evildomain.com", "username=bob&password=secret", TypeForm, AcceptHTML)) - Equal(t, 303, recorder.Code) - Equal(t, "/", recorder.Header().Get("Location")) - - //redirect to success url if domains whitelist file doesn't exist - cfg = DefaultConfig() - cfg.PreventExternalRedirects = false - cfg.WhitelistDomainsFile = "domains_whitelist.txt" - h = &Handler{ - backends: []Backend{ - NewSimpleBackend(map[string]string{"bob": "secret"}), - }, - oauth: oauth2.NewManager(), - config: cfg, - } - recorder = httptest.NewRecorder() - h.ServeHTTP(recorder, req("POST", "/login?backTo=https://gooddomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) - Equal(t, 303, recorder.Code) - Equal(t, "/", recorder.Header().Get("Location")) - - //setup domain whitelist file - d1 := []byte("gooddomain.com\n") - _ = ioutil.WriteFile("domains_whitelist.txt", d1, 0644) - - //allow redirect to domains on whitelist - recorder = httptest.NewRecorder() - h.ServeHTTP(recorder, req("POST", "/login?backTo=https://gooddomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) - Equal(t, 303, recorder.Code) - Equal(t, "https://gooddomain.com/website", recorder.Header().Get("Location")) - - //allow redirect to domains on whitelist - recorder = httptest.NewRecorder() - h.ServeHTTP(recorder, req("POST", "/login?backTo=https://evildomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) - Equal(t, 303, recorder.Code) - Equal(t, "/", recorder.Header().Get("Location")) - - //remove domains whitelist file - err := os.Remove("domains_whitelist.txt") - Equal(t, nil, err) -} - func TestHandler_Refresh(t *testing.T) { h := testHandler() input := model.UserInfo{Sub: "bob", Expiry: time.Now().Add(time.Second).Unix()} diff --git a/login/redirect.go b/login/redirect.go index 4988650..650c972 100644 --- a/login/redirect.go +++ b/login/redirect.go @@ -2,118 +2,110 @@ package login import ( "bufio" - "errors" - "fmt" "net/http" "net/url" "os" "github.com/tarent/loginsrv/logging" + "strings" + "time" ) -func (h *Handler) shouldRedirect(r *http.Request) bool { - if h.config.AllowRedirects { - if h.config.CheckRefererOnRedirects { - referer, err := url.Parse(r.Header.Get("Referer")) - if err != nil { - logging.Application(r.Header).Warnf( - "couldn't parse redirect url %s", - err, - ) - return false - } - if referer.Host != r.Host { - logging.Application(r.Header).Warnf( - "Referer domain: '%s' does not match current domain '%s'", - referer.Host, - r.Host, - ) - return false - } +func (h *Handler) setRedirectCookie(w http.ResponseWriter, r *http.Request) { + redirectTo := r.URL.Query().Get(h.config.RedirectQueryParameter) + if redirectTo != "" && h.allowRedirect(r) && r.Method != "POST" { + cookie := http.Cookie{ + Name: h.config.RedirectQueryParameter, + Value: redirectTo, } + http.SetCookie(w, &cookie) + } +} + +func (h *Handler) deleteRedirectCookie(w http.ResponseWriter, r *http.Request) { + _, err := r.Cookie(h.config.RedirectQueryParameter) + if err == nil { + cookie := http.Cookie{ + Name: h.config.RedirectQueryParameter, + Value: "delete", + Expires: time.Unix(0, 0), + } + http.SetCookie(w, &cookie) + } +} + +func (h *Handler) allowRedirect(r *http.Request) bool { + if !h.config.AllowRedirects { + return false + } + if !h.config.CheckRefererOnRedirects { return true } - return false + + referer, err := url.Parse(r.Header.Get("Referer")) + if err != nil { + logging.Application(r.Header).Warnf("couldn't parse redirect url %s", err) + return false + } + if referer.Host != r.Host { + logging.Application(r.Header).Warnf("redirect from referer domain: '%s', not matching current domain '%s'", referer.Host, r.Host) + return false + } + return true } func (h *Handler) redirectURL(r *http.Request, w http.ResponseWriter) string { - if h.config.AllowRedirects { - parsedURL, err := h.parseURL(r) - if err != nil { - logging.Application(r.Header).Warnf( - "error parsing redirect URL: %s", - err, - ) - return h.config.SuccessURL + targetURL, foundTarget := h.getRedirectTarget(r) + if foundTarget && h.config.AllowRedirects { + sameHost := targetURL.Host == "" || r.Host == targetURL.Host + if sameHost && targetURL.Path != "" { + return targetURL.Path } - if h.config.PreventExternalRedirects { - if parsedURL.Path == "" { - return h.config.SuccessURL - } else { - if (parsedURL.Host != "") && (r.Host != parsedURL.Host) { - logging.Application(r.Header).Warnf( - "Attempted redirect to %s", - parsedURL.Host, - ) - return h.config.SuccessURL - } - return parsedURL.Path - } - } else { - if h.checkWhiteListDomains(r, parsedURL.Host) { - return fmt.Sprintf( - "%s://%s%s", - parsedURL.Scheme, - parsedURL.Host, - parsedURL.Path, - ) - } else { - return h.config.SuccessURL - } + if !sameHost && h.isRedirectDomainWhitelisted(r, targetURL.Host) { + return targetURL.String() } } return h.config.SuccessURL } -func (h *Handler) parseURL(r *http.Request) (*url.URL, error) { +func (h *Handler) getRedirectTarget(r *http.Request) (*url.URL, bool) { cookie, err := r.Cookie(h.config.RedirectQueryParameter) - if err != nil { - //try reading parameter as it might be a POST request and so not have set the cookie yet - queries, err := url.ParseQuery(r.URL.RawQuery) + if err == nil { + url, err := url.Parse(cookie.Value) if err != nil { - return nil, err - } - if queries.Get(h.config.RedirectQueryParameter) != "" { - parsedURL, err := url.Parse(queries.Get(h.config.RedirectQueryParameter)) - return parsedURL, err - } else { - return nil, errors.New("no redirect") + logging.Application(r.Header).Warnf("error parsing redirect URL: %s", err) + return nil, false } + return url, true } - parsedURL, err := url.Parse(cookie.Value) - return parsedURL, err + + // try reading parameter as it might be a POST request and so not have set the cookie yet + redirectTo := r.URL.Query().Get(h.config.RedirectQueryParameter) + if redirectTo == "" || r.Method != "POST" { + return nil, false + } + url, err := url.Parse(redirectTo) + if err != nil { + logging.Application(r.Header).Warnf("error parsing redirect URL: %s", err) + return nil, false + } + return url, true } -func (h *Handler) checkWhiteListDomains(r *http.Request, host string) bool { +func (h *Handler) isRedirectDomainWhitelisted(r *http.Request, host string) bool { f, err := os.Open(h.config.WhitelistDomainsFile) - defer f.Close() if err != nil { - logging.Application(r.Header).Warnf( - "can't open domains file '%s'", - h.config.WhitelistDomainsFile, - ) + logging.Application(r.Header).Warnf("can't open redirect whitelist domains file '%s'", h.config.WhitelistDomainsFile) return false } + defer f.Close() scanner := bufio.NewScanner(f) scanner.Split(bufio.ScanLines) for scanner.Scan() { - if host == scanner.Text() { + if host == strings.TrimSpace(scanner.Text()) { return true } } - logging.Application(r.Header).Warnf( - "Domain '%s' not in whitelist", - host, - ) + logging.Application(r.Header).Warnf("domain '%s' not in redirect whitelist", host) return false } diff --git a/login/redirect_test.go b/login/redirect_test.go new file mode 100644 index 0000000..fd25740 --- /dev/null +++ b/login/redirect_test.go @@ -0,0 +1,126 @@ +package login + +import ( + "net/http/httptest" + "os" + "testing" + + . "github.com/stretchr/testify/assert" + "github.com/tarent/loginsrv/oauth2" + "io/ioutil" +) + +const BadReferer = "Referer: http://evildomain.com" + +func TestRedirect(t *testing.T) { + // by default set redirect_cookie + recorder := call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML)) + setCookieList := readSetCookies(recorder.Header()) + Equal(t, 1, len(setCookieList)) + cookie := setCookieList[0] + Equal(t, "backTo", cookie.Name) + Equal(t, "/website", cookie.Value) + + // by default allowed redirects + recorder = call(req("POST", "/context/login?backTo=/website", "username=bob&password=secret", TypeForm, AcceptHTML)) + Equal(t, 303, recorder.Code) + Equal(t, "/website", recorder.Header().Get("Location")) +} + +func TestRedirect_NotAllowed(t *testing.T) { + // redirect to SuccessURL if AllowRedirects is false + cfg := DefaultConfig() + cfg.AllowRedirects = false + h := &Handler{ + backends: []Backend{ + NewSimpleBackend(map[string]string{"bob": "secret"}), + }, + oauth: oauth2.NewManager(), + config: cfg, + } + recorder := httptest.NewRecorder() + h.ServeHTTP(recorder, req("POST", "/login?backTo=/website", "username=bob&password=secret", TypeForm, AcceptHTML)) + Equal(t, 303, recorder.Code) + Equal(t, "/", recorder.Header().Get("Location")) +} + +func TestRedirect_NonMatchingReferrer(t *testing.T) { + // by default don't set redirect cookie if Referer doesn't match origin + recorder := call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML, BadReferer)) + setCookieList := readSetCookies(recorder.Header()) + Equal(t, 0, len(setCookieList)) + + // don't set redirect cookie if referrer is malformed + recorder = call(req("GET", "/context/login?backTo=/website", "", TypeForm, AcceptHTML, "Referer: :notvalid")) + setCookieList = readSetCookies(recorder.Header()) + Equal(t, 0, len(setCookieList)) + + // set redirect cookie with mismatch referer if CheckRefererOnRedirects is false + cfg := DefaultConfig() + cfg.CheckRefererOnRedirects = false + h := &Handler{ + backends: []Backend{ + NewSimpleBackend(map[string]string{"bob": "secret"}), + }, + oauth: oauth2.NewManager(), + config: cfg, + } + recorder = httptest.NewRecorder() + h.ServeHTTP(recorder, req("GET", "/login?backTo=/website", "", TypeForm, AcceptHTML, BadReferer)) + setCookieList = readSetCookies(recorder.Header()) + Equal(t, 1, len(setCookieList)) + cookie := setCookieList[0] + Equal(t, "backTo", cookie.Name) + Equal(t, "/website", cookie.Value) +} + +func TestRedirect_PreventExternal(t *testing.T) { + // by default prevent redirect to external site + recorder := call(req("POST", "/context/login?backTo=//evildomain.com/phishing.html", "username=bob&password=secret", TypeForm, AcceptHTML)) + Equal(t, 303, recorder.Code) + Equal(t, "/", recorder.Header().Get("Location")) + + // by default if the parsed path is empty redirect to SuccessURL + recorder = call(req("POST", "/context/login?backTo=https://evildomain.com", "username=bob&password=secret", TypeForm, AcceptHTML)) + Equal(t, 303, recorder.Code) + Equal(t, "/", recorder.Header().Get("Location")) +} + +func TestRedirect_Whitelisting(t *testing.T) { + whitelistFile, _ := ioutil.TempFile("", "loginsrv_test_domains_whitelist") + whitelistFile.Close() + os.Remove(whitelistFile.Name()) + + // redirect to success url if domains whitelist file doesn't exist + cfg := DefaultConfig() + cfg.WhitelistDomainsFile = whitelistFile.Name() + h := &Handler{ + backends: []Backend{ + NewSimpleBackend(map[string]string{"bob": "secret"}), + }, + oauth: oauth2.NewManager(), + config: cfg, + } + recorder := httptest.NewRecorder() + h.ServeHTTP(recorder, req("POST", "/login?backTo=https://gooddomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) + Equal(t, 303, recorder.Code) + Equal(t, "/", recorder.Header().Get("Location")) + + // setup domain whitelist file + domains := []byte("foo.com\ngooddomain.com \nbar.com") + _ = ioutil.WriteFile(whitelistFile.Name(), domains, 0644) + defer os.Remove(whitelistFile.Name()) + + // allow redirect to domains on whitelist + recorder = httptest.NewRecorder() + h.ServeHTTP(recorder, req("POST", "/login?backTo=https://gooddomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) + Equal(t, 303, recorder.Code) + Equal(t, "https://gooddomain.com/website", recorder.Header().Get("Location")) + + // still permit access to domains which are not in the whitelist + recorder = httptest.NewRecorder() + h.ServeHTTP(recorder, req("POST", "/login?backTo=https://evildomain.com/website", "username=bob&password=secret", TypeForm, AcceptHTML, BadReferer)) + Equal(t, 303, recorder.Code) + Equal(t, "/", recorder.Header().Get("Location")) + +}