From c44fb9889b074ec71f5e7b0e4c110a7495e593ca Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 13 Jul 2023 12:34:47 +0200 Subject: [PATCH] fix(url/redirect): ensure fragments are preserved --- pkg/url/redirect.go | 11 ++++--- pkg/url/redirect_test.go | 68 +++++++++++++++++++++++++++++++--------- 2 files changed, 60 insertions(+), 19 deletions(-) diff --git a/pkg/url/redirect.go b/pkg/url/redirect.go index 9c1bed0..a7e03e2 100644 --- a/pkg/url/redirect.go +++ b/pkg/url/redirect.go @@ -33,7 +33,7 @@ func NewStandaloneRedirect(ingresses *ingress.Ingresses) *StandaloneRedirect { func (h *StandaloneRedirect) Canonical(r *http.Request) string { target := redirectQueryParam(r) - redirect, err := url.ParseRequestURI(target) + redirect, err := url.Parse(target) if err != nil { redirect = fallback(r, target, h.getFallbackRedirect(r)) } @@ -61,7 +61,7 @@ type SSOServerRedirect struct { } func NewSSOServerRedirect(config *config.Config) (*SSOServerRedirect, error) { - u, err := url.ParseRequestURI(config.SSO.ServerDefaultRedirectURL) + u, err := url.Parse(config.SSO.ServerDefaultRedirectURL) if err != nil { return nil, fmt.Errorf("parsing fallback redirect URL: %w", err) } @@ -74,7 +74,7 @@ func NewSSOServerRedirect(config *config.Config) (*SSOServerRedirect, error) { func (h *SSOServerRedirect) Canonical(r *http.Request) string { target := redirectQueryParam(r) - redirect, err := url.ParseRequestURI(target) + redirect, err := url.Parse(target) if err != nil { redirect = fallback(r, target, h.fallbackRedirect) } @@ -109,13 +109,14 @@ func (h *SSOProxyRedirect) Canonical(r *http.Request) string { // get redirect from request query parameter target := redirectQueryParam(r) - redirectParamURL, err := url.ParseRequestURI(target) + redirectParamURL, err := url.Parse(target) if err != nil { logInvalidRedirect(r, target, redirect.String()) } else { - // copy desired path and query to base redirect + // preserve path, query and fragment from redirect parameter to base redirect redirect.Path = redirectParamURL.Path redirect.RawQuery = redirectParamURL.RawQuery + redirect.Fragment = redirectParamURL.Fragment } return h.Clean(r, redirect.String()) diff --git a/pkg/url/redirect_test.go b/pkg/url/redirect_test.go index f28dd57..dd46e30 100644 --- a/pkg/url/redirect_test.go +++ b/pkg/url/redirect_test.go @@ -142,6 +142,22 @@ func TestDefault(t *testing.T) { clean: "/", }, }, + { + name: "relative url with fragment in redirect", + url: "/?redirect=%2F%23%2Fsome-path", + expected: expected{ + canonical: "/#/some-path", + clean: "/?redirect=%2F%23%2Fsome-path", + }, + }, + { + name: "absolute url with fragment in redirect", + url: "/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", + expected: expected{ + canonical: "/#/some-path", + clean: "/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", + }, + }, } { t.Run(tt.name, func(t *testing.T) { r := mock.NewGetRequest(tt.url, ingresses) @@ -306,6 +322,14 @@ func TestSSOServer(t *testing.T) { clean: "http://wonderwall/?redirect=http%3A%2F%2Fsome.app.wonderwall%2Fsome-path%3Fwith%3Dquery", }, }, + { + name: "absolute url with fragment in redirect", + url: "http://wonderwall/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", + expected: expected{ + canonical: "http://wonderwall/#/some-path", + clean: "http://wonderwall/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", + }, + }, } { t.Run(tt.name, func(t *testing.T) { r := mock.NewGetRequest(tt.url, ingresses) @@ -389,10 +413,10 @@ func TestSSOProxy(t *testing.T) { }, { name: "absolute url", - url: "http://wonderwall/?redirect=%2Fpath", + url: "http://app.wonderwall/?redirect=%2Fpath", expected: expected{ canonical: "http://app.wonderwall/path", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/?redirect=%2Fpath", }, }, { @@ -405,26 +429,26 @@ func TestSSOProxy(t *testing.T) { }, { name: "absolute url with context path", - url: "http://wonderwall/some-path?redirect=%2Fpath", + url: "http://app.wonderwall/some-path?redirect=%2Fpath", expected: expected{ canonical: "http://app.wonderwall/path", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/some-path?redirect=%2Fpath", }, }, { name: "absolute url with query parameters", - url: "http://wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix", + url: "http://app.wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix", expected: expected{ canonical: "http://app.wonderwall/path?gnu=notunix", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix", }, }, { name: "absolute url with multiple query parameters", - url: "http://wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix%26foo%3Dbar", + url: "http://app.wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix%26foo%3Dbar", expected: expected{ canonical: "http://app.wonderwall/path?gnu=notunix&foo=bar", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/some-path?redirect=%2Fpath%3Fgnu%3Dnotunix%26foo%3Dbar", }, }, { @@ -445,26 +469,42 @@ func TestSSOProxy(t *testing.T) { }, { name: "absolute url with different domain in redirect", - url: "http://wonderwall/?redirect=http%3A%2F%2Fnot-wonderwall%2Fsome-path%3Fwith%3Dquery", + url: "http://app.wonderwall/?redirect=http%3A%2F%2Fnot-wonderwall%2Fsome-path%3Fwith%3Dquery", expected: expected{ canonical: "http://app.wonderwall/some-path?with=query", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/?redirect=http%3A%2F%2Fnot-wonderwall%2Fsome-path%3Fwith%3Dquery", }, }, { name: "absolute url with subdomain in redirect", - url: "http://wonderwall/?redirect=http%3A%2F%2Fanother-app.wonderwall%2Fsome-path%3Fwith%3Dquery", + url: "http://app.wonderwall/?redirect=http%3A%2F%2Fanother-app.wonderwall%2Fsome-path%3Fwith%3Dquery", expected: expected{ canonical: "http://app.wonderwall/some-path?with=query", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/?redirect=http%3A%2F%2Fanother-app.wonderwall%2Fsome-path%3Fwith%3Dquery", }, }, { name: "absolute url with nested subdomain in redirect", - url: "http://wonderwall/?redirect=http%3A%2F%2Fsome.app.wonderwall%2Fsome-path%3Fwith%3Dquery", + url: "http://app.wonderwall/?redirect=http%3A%2F%2Fsome.app.wonderwall%2Fsome-path%3Fwith%3Dquery", expected: expected{ canonical: "http://app.wonderwall/some-path?with=query", - clean: "http://app.wonderwall", + clean: "http://app.wonderwall/?redirect=http%3A%2F%2Fsome.app.wonderwall%2Fsome-path%3Fwith%3Dquery", + }, + }, + { + name: "relative url with fragment in redirect", + url: "http://app.wonderwall/?redirect=%2F%23%2Fsome-path", + expected: expected{ + canonical: "http://app.wonderwall/#/some-path", + clean: "http://app.wonderwall/?redirect=%2F%23%2Fsome-path", + }, + }, + { + name: "absolute url with fragment in redirect", + url: "http://app.wonderwall/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", + expected: expected{ + canonical: "http://app.wonderwall/#/some-path", + clean: "http://app.wonderwall/?redirect=http%3A%2F%2Fwonderwall%2F%23%2Fsome-path", }, }, } {