From 97d2a88bb18925096a036f28f02a1dd5effb9419 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Sat, 17 Sep 2022 14:16:00 +0200 Subject: [PATCH] fix(handler/url): ensure that parameters for original url aren't dropped --- pkg/handler/error/error_test.go | 43 +++++++++++++++++---------------- pkg/handler/handler_test.go | 2 +- pkg/handler/url/url.go | 18 +++++++++++--- pkg/handler/url/url_test.go | 28 ++++++++++++++++++--- 4 files changed, 61 insertions(+), 30 deletions(-) diff --git a/pkg/handler/error/error_test.go b/pkg/handler/error/error_test.go index 19c966f..1830d1e 100644 --- a/pkg/handler/error/error_test.go +++ b/pkg/handler/error/error_test.go @@ -4,6 +4,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/stretchr/testify/assert" @@ -77,12 +78,12 @@ func TestHandler_Retry(t *testing.T) { { name: "login path", request: httpRequest("/oauth2/login"), - want: "/oauth2/login?redirect=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "callback path", request: httpRequest("/oauth2/callback"), - want: "/oauth2/login?redirect=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "logout path", @@ -98,7 +99,7 @@ func TestHandler_Retry(t *testing.T) { name: "login with non-default ingress", request: httpRequest("/domene/oauth2/login"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/domene", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/domene"), }, { name: "logout with non-default ingress", @@ -109,103 +110,103 @@ func TestHandler_Retry(t *testing.T) { { name: "login with referer", request: httpRequest("/oauth2/login", "/api/me"), - want: "/oauth2/login?redirect=/api/me", + want: "/oauth2/login?redirect=" + url.QueryEscape("/api/me"), }, { name: "login with referer on non-default ingress", request: httpRequest("/domene/oauth2/login", "/api/me"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/api/me", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/api/me"), }, { name: "login with root referer", request: httpRequest("/oauth2/login", "/"), - want: "/oauth2/login?redirect=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with root referer on non-default ingress", request: httpRequest("/domene/oauth2/login", "/"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with cookie referer", request: httpRequest("/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: "/"}, - want: "/oauth2/login?redirect=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with empty cookie referer", request: httpRequest("/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: ""}, - want: "/oauth2/login?redirect=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with cookie referer takes precedence over referer header", request: httpRequest("/oauth2/login", "/api/me"), loginCookie: &openid.LoginCookie{Referer: "/api/headers"}, - want: "/oauth2/login?redirect=/api/headers", + want: "/oauth2/login?redirect=" + url.QueryEscape("/api/headers"), }, { name: "login with cookie referer on non-default ingress", request: httpRequest("/domene/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/domene/api/me", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/domene/api/me"), }, { name: "login with redirect parameter set", request: httpRequest("/oauth2/login?redirect=/api/me"), - want: "/oauth2/login?redirect=/api/me", + want: "/oauth2/login?redirect=" + url.QueryEscape("/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", + want: "/oauth2/login?redirect=" + url.QueryEscape("/api/me?a=b&c=d"), }, { name: "login with redirect parameter set on non-default ingress", request: httpRequest("/domene/oauth2/login?redirect=/api/me"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/api/me", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/api/me"), }, { name: "login with redirect parameter set takes precedence over referer header", request: httpRequest("/oauth2/login?redirect=/other", "/api/me"), - want: "/oauth2/login?redirect=/other", + want: "/oauth2/login?redirect=" + url.QueryEscape("/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=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with redirect parameter set to relative root on non-default ingress takes precedence over referer header", request: httpRequest("/domene/oauth2/login?redirect=/", "/api/me"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/"), }, { 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=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { 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=/", + want: "/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with redirect parameter set to absolute url on non-default ingress takes precedence over referer header", request: httpRequest("/domene/oauth2/login?redirect=http://localhost:8080/", "/api/me"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=/", + want: "/domene/oauth2/login?redirect=" + url.QueryEscape("/"), }, { name: "login with cookie referer takes precedence over redirect parameter", request: httpRequest("/oauth2/login?redirect=/other"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, - want: "/oauth2/login?redirect=/domene/api/me", + want: "/oauth2/login?redirect=" + url.QueryEscape("/domene/api/me"), }, } { t.Run(test.name, func(t *testing.T) { diff --git a/pkg/handler/handler_test.go b/pkg/handler/handler_test.go index adb1069..e7881df 100644 --- a/pkg/handler/handler_test.go +++ b/pkg/handler/handler_test.go @@ -375,7 +375,7 @@ func TestHandler_Default(t *testing.T) { // redirect should point to local login endpoint loginLocation := resp.Location - assert.Equal(t, idp.RelyingPartyServer.URL+"/oauth2/login?redirect=/", loginLocation.String()) + assert.Equal(t, idp.RelyingPartyServer.URL+"/oauth2/login?redirect="+url.QueryEscape("/"), loginLocation.String()) // follow redirect to local login endpoint resp = get(t, rpClient, loginLocation.String()) diff --git a/pkg/handler/url/url.go b/pkg/handler/url/url.go index d23a8cc..b7ae8f6 100644 --- a/pkg/handler/url/url.go +++ b/pkg/handler/url/url.go @@ -39,7 +39,7 @@ func CanonicalRedirect(r *http.Request) string { parsed, err := url.Parse(redirect) if err != nil { // Silently fall back to ingress path - redirect = ingressPath + return ingressPath } // Strip scheme and host to avoid cross-domain redirects @@ -48,6 +48,12 @@ func CanonicalRedirect(r *http.Request) string { redirect = parsed.String() + // Ensure URL isn't encoded + redirect, err = url.QueryUnescape(redirect) + if err != nil { + return ingressPath + } + // Root path without trailing slash is empty if len(parsed.Path) == 0 { redirect = "/" @@ -62,10 +68,14 @@ func CanonicalRedirect(r *http.Request) string { } func LoginURL(prefix, redirectTarget string) string { - loginPath := prefix + paths.OAuth2 + paths.Login - redirectParam := fmt.Sprintf("?%s=%s", RedirectURLParameter, redirectTarget) + u := new(url.URL) + u.Path = path.Join(prefix, paths.OAuth2, paths.Login) - return loginPath + redirectParam + v := url.Values{} + v.Set(RedirectURLParameter, redirectTarget) + u.RawQuery = v.Encode() + + return u.String() } func LoginCallbackURL(r *http.Request) (string, error) { diff --git a/pkg/handler/url/url_test.go b/pkg/handler/url/url_test.go index 1a83ee7..32b0345 100644 --- a/pkg/handler/url/url_test.go +++ b/pkg/handler/url/url_test.go @@ -137,6 +137,26 @@ func TestCanonicalRedirect(t *testing.T) { value: "/path?gnu=notunix", expected: "/path?gnu=notunix", }, + { + name: "url encoded path", + value: "%2Fpath", + expected: "/path", + }, + { + name: "url encoded path and query parameters", + value: "%2Fpath%3Fgnu%3Dnotunix", + expected: "/path?gnu=notunix", + }, + { + name: "url encoded url", + value: "http%3A%2F%2Flocalhost%3A8080%2Fpath", + expected: "http://localhost:8080/path", + }, + { + name: "url encoded url and multiple query parameters", + value: "http%3A%2F%2Flocalhost%3A8080%2Fpath%3Fgnu%3Dnotunix%26foo%3Dbar", + expected: "http://localhost:8080/path?gnu=notunix&foo=bar", + }, } { t.Run(test.name, func(t *testing.T) { v := &url.Values{} @@ -159,25 +179,25 @@ func TestLoginURL(t *testing.T) { name: "no prefix", prefix: "", redirectTarget: "https://test.example.com?some=param&other=param2", - want: "/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + want: "/oauth2/login?redirect=" + url.QueryEscape("https://test.example.com?some=param&other=param2"), }, { name: "with prefix", prefix: "/path", redirectTarget: "https://test.example.com?some=param&other=param2", - want: "/path/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + want: "/path/oauth2/login?redirect=" + url.QueryEscape("https://test.example.com?some=param&other=param2"), }, { name: "we need to go deeper", prefix: "/deeper/path", redirectTarget: "https://test.example.com?some=param&other=param2", - want: "/deeper/path/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + want: "/deeper/path/oauth2/login?redirect=" + url.QueryEscape("https://test.example.com?some=param&other=param2"), }, { name: "relative target", prefix: "", redirectTarget: "/path?some=param&other=param2", - want: "/oauth2/login?redirect=/path?some=param&other=param2", + want: "/oauth2/login?redirect=" + url.QueryEscape("/path?some=param&other=param2"), }, } { t.Run(test.name, func(t *testing.T) {