refactor(router/request): clean up construction of canonical redirect uris; add missing tests

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))

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
 	}

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"),