From cd57e72d5635f9dc14316d7ac585ee8ce23369c5 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 5 May 2022 08:12:23 +0200 Subject: [PATCH] refactor(router/request): clean up construction of canonical redirect uris; add missing tests --- pkg/router/handler_login.go | 3 +- pkg/router/request/request.go | 100 +++++++++++++++------------- pkg/router/request/request_test.go | 101 +++++++++++++++++++++++++---- 3 files changed, 146 insertions(+), 58 deletions(-) diff --git a/pkg/router/handler_login.go b/pkg/router/handler_login.go index 49d1960..c7bc788 100644 --- a/pkg/router/handler_login.go +++ b/pkg/router/handler_login.go @@ -37,11 +37,12 @@ func (h *Handler) Login(w http.ResponseWriter, r *http.Request) { return } + redirect := request.CanonicalRedirectURL(r, h.Config.Ingress) err = h.setLoginCookies(w, &openid.LoginCookie{ State: params.State, Nonce: params.Nonce, CodeVerifier: params.CodeVerifier, - Referer: request.CanonicalRedirectURL(r), + Referer: redirect, }) if err != nil { h.InternalError(w, r, fmt.Errorf("login: setting cookie: %w", err)) diff --git a/pkg/router/request/request.go b/pkg/router/request/request.go index 25caff9..772996a 100644 --- a/pkg/router/request/request.go +++ b/pkg/router/request/request.go @@ -17,36 +17,64 @@ var ( ) // CanonicalRedirectURL constructs a redirect URL that points back to the application. -func CanonicalRedirectURL(r *http.Request) string { - // 1. default - redirectURL := "/" +func CanonicalRedirectURL(r *http.Request, ingress string) string { + // 1. Default + defaultPath := defaultRedirectURL(ingress) + redirect := defaultPath // 2. Referer header is set - referer := RefererPath(r) + referer := refererPath(r) if len(referer) > 0 { - redirectURL = referer + redirect = referer } - // 3. redirect parameter is set - override := r.URL.Query().Get(RedirectURLParameter) - if len(override) <= 0 { - return redirectURL + // 3. Redirect parameter is set + redirectParam, found := parseRedirectParam(r) + if found { + redirect = redirectParam } - overrideUrl, err := url.Parse(override) + // 4. Ensure that empty path redirections falls back to the ingress' context path if applicable + if len(redirect) == 0 { + redirect = defaultPath + } + + return redirect +} + +func defaultRedirectURL(ingress string) string { + defaultPath := "/" + ingressPath := config.ParseIngress(ingress) + + if len(ingressPath) > 0 { + defaultPath = ingressPath + } + + return defaultPath +} + +func parseRedirectParam(r *http.Request) (string, bool) { + redirectParam := r.URL.Query().Get(RedirectURLParameter) + if len(redirectParam) <= 0 { + return "", false + } + + redirectParamURL, err := url.Parse(redirectParam) if err != nil { - return redirectURL + return "", false } // strip scheme and host to avoid cross-domain redirects - overrideUrl.Scheme = "" - overrideUrl.Host = "" - overrideString := overrideUrl.String() - if len(overrideString) <= 0 { - return "/" + redirectParamURL.Scheme = "" + redirectParamURL.Host = "" + + redirectParamURLString := redirectParamURL.String() + + if len(redirectParamURLString) == 0 { + redirectParamURLString = "/" } - return overrideString + return redirectParamURLString, true } // LoginURLParameter attempts to get a given parameter from the given HTTP request, falling back if none found. @@ -74,15 +102,17 @@ func PostLogoutRedirectURI(r *http.Request, fallback string) string { return fallback } -func RefererPath(r *http.Request) string { - result := "" - - referer, err := url.Parse(r.Referer()) - if err == nil && len(referer.Path) > 0 { - result = referer.Path +func refererPath(r *http.Request) string { + if len(r.Referer()) == 0 { + return "" } - return result + referer, err := url.Parse(r.Referer()) + if err != nil { + return "" + } + + return referer.Path } // RetryURI returns a URI that should retry the desired route that failed. @@ -91,34 +121,14 @@ func RefererPath(r *http.Request) string { // `/oauth2/login` endpoint unless the original request attempted to reach the logout-flow. func RetryURI(r *http.Request, ingress string, loginCookie *openid.LoginCookie) string { retryURI := r.URL.Path - prefix := config.ParseIngress(ingress) if strings.HasSuffix(retryURI, paths.OAuth2+paths.Logout) || strings.HasSuffix(retryURI, paths.OAuth2+paths.FrontChannelLogout) { return prefix + retryURI } - // 1. Default - redirect := "/" + redirect := CanonicalRedirectURL(r, ingress) - // 2. Ingress has path prefix - if len(prefix) > 0 { - redirect = prefix - } - - // 3. Referer header is set - referer := RefererPath(r) - if len(referer) > 0 { - redirect = referer - } - - // 4. Redirect parameter is set - redirectURLFromParam, err := url.Parse(r.URL.Query().Get(RedirectURLParameter)) - if err == nil && len(redirectURLFromParam.Path) > 0 { - redirect = redirectURLFromParam.Path - } - - // 5. Login cookie exists and referer is set if loginCookie != nil && len(loginCookie.Referer) > 0 { redirect = loginCookie.Referer } diff --git a/pkg/router/request/request_test.go b/pkg/router/request/request_test.go index 032646e..9d34853 100644 --- a/pkg/router/request/request_test.go +++ b/pkg/router/request/request_test.go @@ -15,15 +15,50 @@ func TestCanonicalRedirectURL(t *testing.T) { r, err := http.NewRequest("GET", "http://localhost:8080/oauth2/login", nil) assert.NoError(t, err) - // Default URL is / - assert.Equal(t, "/", request.CanonicalRedirectURL(r)) + t.Run("default redirect", func(t *testing.T) { + for _, test := range []struct { + name string + ingress string + expected string + }{ + { + name: "root with trailing slash", + ingress: "http://localhost:8080/", + expected: "/", + }, + { + name: "root without trailing slash", + ingress: "http://localhost:8080", + expected: "/", + }, + { + name: "path with trailing slash", + ingress: "http://localhost:8080/path/", + expected: "/path", + }, + { + name: "path without trailing slash", + ingress: "http://localhost:8080/path", + expected: "/path", + }, + } { + t.Run(test.name, func(t *testing.T) { + assert.Equal(t, test.expected, request.CanonicalRedirectURL(r, test.ingress)) + }) + } + }) + + // Default path is /some-path + ingress := "http://localhost:8080/some-path" // HTTP Referer header is 2nd priority - r.Header.Set("referer", "http://localhost:8080/foo/bar/baz?gnu=notunix") - assert.Equal(t, "/foo/bar/baz", request.CanonicalRedirectURL(r)) + t.Run("Referer header is set", func(t *testing.T) { + r.Header.Set("referer", "http://localhost:8080/foo/bar/baz?gnu=notunix") + assert.Equal(t, "/foo/bar/baz", request.CanonicalRedirectURL(r, ingress)) + }) // If redirect parameter is set, use that - t.Run("redirect parameter", func(t *testing.T) { + t.Run("redirect parameter is set", func(t *testing.T) { for _, test := range []struct { name string value string @@ -31,27 +66,27 @@ func TestCanonicalRedirectURL(t *testing.T) { }{ { name: "complete url with parameters", - value: "https://google.com/path/to/redirect?val1=foo&val2=bar", + value: "http://localhost:8080/path/to/redirect?val1=foo&val2=bar", expected: "/path/to/redirect?val1=foo&val2=bar", }, { name: "root url with trailing slash", - value: "https://google.com/", + value: "http://localhost:8080/", expected: "/", }, { name: "root url without trailing slash", - value: "https://google.com", + value: "http://localhost:8080", expected: "/", }, { name: "url path with trailing slash", - value: "https://google.com/path/", + value: "http://localhost:8080/path/", expected: "/path/", }, { name: "url path without trailing slash", - value: "https://google.com/path", + value: "http://localhost:8080/path", expected: "/path", }, } { @@ -59,11 +94,10 @@ func TestCanonicalRedirectURL(t *testing.T) { v := &url.Values{} v.Set("redirect", test.value) r.URL.RawQuery = v.Encode() - assert.Equal(t, test.expected, request.CanonicalRedirectURL(r)) + assert.Equal(t, test.expected, request.CanonicalRedirectURL(r, ingress)) }) } }) - } func TestLoginURLParameter(t *testing.T) { @@ -201,6 +235,17 @@ func TestRetryURI(t *testing.T) { ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect=/api/me", }, + { + name: "login with root referer", + request: httpRequest("/oauth2/login", "/"), + want: "/oauth2/login?redirect=/", + }, + { + name: "login with root referer on non-default ingress", + request: httpRequest("/oauth2/login", "/"), + ingress: "https://test.nav.no/domene", + want: "/domene/oauth2/login?redirect=/", + }, { name: "login with cookie referer", request: httpRequest("/oauth2/login"), @@ -231,6 +276,11 @@ func TestRetryURI(t *testing.T) { request: httpRequest("/oauth2/login?redirect=/api/me"), want: "/oauth2/login?redirect=/api/me", }, + { + name: "login with redirect parameter set and query parameters", + request: httpRequest("/oauth2/login?redirect=/api/me?a=b%26c=d"), + want: "/oauth2/login?redirect=/api/me?a=b&c=d", + }, { name: "login with redirect parameter set on non-default ingress", request: httpRequest("/oauth2/login?redirect=/api/me"), @@ -242,6 +292,33 @@ func TestRetryURI(t *testing.T) { request: httpRequest("/oauth2/login?redirect=/other", "/api/me"), want: "/oauth2/login?redirect=/other", }, + { + name: "login with redirect parameter set to relative root takes precedence over referer header", + request: httpRequest("/oauth2/login?redirect=/", "/api/me"), + want: "/oauth2/login?redirect=/", + }, + { + name: "login with redirect parameter set to relative root on non-default ingress takes precedence over referer header", + request: httpRequest("/oauth2/login?redirect=/", "/api/me"), + ingress: "https://test.nav.no/domene", + want: "/domene/oauth2/login?redirect=/", + }, + { + name: "login with redirect parameter set to absolute url takes precedence over referer header", + request: httpRequest("/oauth2/login?redirect=http://localhost:8080", "/api/me"), + want: "/oauth2/login?redirect=/", + }, + { + name: "login with redirect parameter set to absolute url with trailing slash takes precedence over referer header", + request: httpRequest("/oauth2/login?redirect=http://localhost:8080/", "/api/me"), + want: "/oauth2/login?redirect=/", + }, + { + name: "login with redirect parameter set to absolute url on non-default ingress takes precedence over referer header", + request: httpRequest("/oauth2/login?redirect=http://localhost:8080/", "/api/me"), + ingress: "https://test.nav.no/domene", + want: "/domene/oauth2/login?redirect=/", + }, { name: "login with cookie referer takes precedence over redirect parameter", request: httpRequest("/oauth2/login?redirect=/other"),