github/wonderwall
1
0
Fork 0
mirror of https://github.com/nais/wonderwall.git synced 2026-05-15 21:06:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(handler/url): ensure that parameters for original url aren't dropped

Browse Source
This commit is contained in:
Trong Huu Nguyen
2022-09-17 14:16:00 +02:00
parent ed56aac3d0
commit 97d2a88bb1
4 changed files with 61 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
pkg/handler/error/error_test.go
View File

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

2
pkg/handler/handler_test.go
View File

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

18
pkg/handler/url/url.go
View File

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

28
pkg/handler/url/url_test.go
View File

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