From b651db40e48f1e7e2135c48751cf03a23d7e4707 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 22 Sep 2022 13:05:13 +0200 Subject: [PATCH] refactor(handler/url): remove support for Referer header The header isn't guaranteed to be set or sent with requests, and all of our users prefer the `redirect` query parameter anyways. --- pkg/handler/error/error_test.go | 88 +++++++++------------------------ pkg/handler/url/url.go | 10 +--- pkg/handler/url/url_test.go | 35 ------------- 3 files changed, 25 insertions(+), 108 deletions(-) diff --git a/pkg/handler/error/error_test.go b/pkg/handler/error/error_test.go index 003a794..5bde4e2 100644 --- a/pkg/handler/error/error_test.go +++ b/pkg/handler/error/error_test.go @@ -60,12 +60,8 @@ func TestHandler_Retry(t *testing.T) { handler := idp.RelyingPartyHandler.GetErrorHandler() - httpRequest := func(url string, referer ...string) *http.Request { - req := httptest.NewRequest(http.MethodGet, url, nil) - if len(referer) > 0 { - req.Header.Add("Referer", referer[0]) - } - return req + get := func(url string) *http.Request { + return httptest.NewRequest(http.MethodGet, url, nil) } for _, test := range []struct { @@ -77,134 +73,96 @@ func TestHandler_Retry(t *testing.T) { }{ { name: "login path", - request: httpRequest("/oauth2/login"), + request: get("/oauth2/login"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { name: "callback path", - request: httpRequest("/oauth2/callback"), + request: get("/oauth2/callback"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { name: "logout path", - request: httpRequest("/oauth2/logout"), + request: get("/oauth2/logout"), want: "/oauth2/logout", }, { name: "front-channel logout path", - request: httpRequest("/oauth2/logout/frontchannel"), + request: get("/oauth2/logout/frontchannel"), want: "/oauth2/logout/frontchannel", }, { name: "login with non-default ingress", - request: httpRequest("/domene/oauth2/login"), + request: get("/domene/oauth2/login"), ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/domene"), }, { name: "logout with non-default ingress", - request: httpRequest("/domene/oauth2/logout"), + request: get("/domene/oauth2/logout"), ingress: "https://test.nav.no/domene", want: "/domene/oauth2/logout", }, - { - name: "login with referer", - request: httpRequest("/oauth2/login", "/api/me"), - want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/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-encoded=" + urlpkg.RedirectEncoded("/api/me"), - }, - { - name: "login with root referer", - request: httpRequest("/oauth2/login", "/"), - want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), - }, - { - 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-encoded=" + urlpkg.RedirectEncoded("/"), - }, { name: "login with cookie referer", - request: httpRequest("/oauth2/login"), + request: get("/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: "/"}, want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { name: "login with empty cookie referer", - request: httpRequest("/oauth2/login"), + request: get("/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: ""}, want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, - { - 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-encoded=" + urlpkg.RedirectEncoded("/api/headers"), - }, { name: "login with cookie referer on non-default ingress", - request: httpRequest("/domene/oauth2/login"), + request: get("/domene/oauth2/login"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/domene/api/me"), }, { name: "login with redirect parameter set", - request: httpRequest("/oauth2/login?redirect=/api/me"), + request: get("/oauth2/login?redirect=/api/me"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/api/me"), }, { name: "login with redirect parameter set and query parameters", - request: httpRequest("/oauth2/login?redirect=/api/me?a=b%26c=d"), + request: get("/oauth2/login?redirect=/api/me?a=b%26c=d"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/api/me?a=b&c=d"), }, { name: "login with redirect parameter set on non-default ingress", - request: httpRequest("/domene/oauth2/login?redirect=/api/me"), + request: get("/domene/oauth2/login?redirect=/api/me"), ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/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-encoded=" + urlpkg.RedirectEncoded("/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-encoded=" + urlpkg.RedirectEncoded("/"), - }, - { - 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"), + name: "login with redirect parameter set to relative root on non-default ingress", + request: get("/domene/oauth2/login?redirect=/"), ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { - name: "login with redirect parameter set to absolute url takes precedence over referer header", - request: httpRequest("/oauth2/login?redirect=http://localhost:8080", "/api/me"), + name: "login with redirect parameter set to absolute url", + request: get("/oauth2/login?redirect=http://localhost:8080"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { - 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"), + name: "login with redirect parameter set to absolute url with trailing slash", + request: get("/oauth2/login?redirect=http://localhost:8080/"), want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { - 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"), + name: "login with redirect parameter set to absolute url on non-default ingress", + request: get("/domene/oauth2/login?redirect=http://localhost:8080/"), ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/"), }, { name: "login with cookie referer takes precedence over redirect parameter", - request: httpRequest("/oauth2/login?redirect=/other"), + request: get("/oauth2/login?redirect=/other"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, want: "/oauth2/login?redirect-encoded=" + urlpkg.RedirectEncoded("/domene/api/me"), }, diff --git a/pkg/handler/url/url.go b/pkg/handler/url/url.go index aef3520..ce12a60 100644 --- a/pkg/handler/url/url.go +++ b/pkg/handler/url/url.go @@ -26,19 +26,13 @@ func CanonicalRedirect(r *http.Request) string { // 1. Default redirect := ingressPath - // 2. Referer header is set - referer := r.Referer() - if len(referer) > 0 { - redirect = referer - } - - // 3. Redirect parameter is set + // 2. Redirect parameter is set redirectParam := r.URL.Query().Get(RedirectURLParameter) if len(redirectParam) > 0 { redirect = redirectParam } - // 4. Redirect-encoded parameter is set + // 3. Redirect-encoded parameter is set redirectEncoded := RedirectDecoded(r) if len(redirectEncoded) > 0 { redirect = redirectEncoded diff --git a/pkg/handler/url/url_test.go b/pkg/handler/url/url_test.go index 35c7be2..7853310 100644 --- a/pkg/handler/url/url_test.go +++ b/pkg/handler/url/url_test.go @@ -60,41 +60,6 @@ func TestCanonicalRedirect(t *testing.T) { r := httptest.NewRequest(http.MethodGet, defaultIngress+"/oauth2/login", nil) r = mw.RequestWithPath(r, "/some-path") - // HTTP Referer header is 2nd priority - t.Run("Referer header is set", func(t *testing.T) { - for _, test := range []struct { - name string - value string - expected string - }{ - { - name: "full URL", - value: "http://localhost:8080/foo/bar/baz", - expected: "/foo/bar/baz", - }, - { - name: "full URL with query parameters", - value: "http://localhost:8080/foo/bar/baz?gnu=notunix", - expected: "/foo/bar/baz?gnu=notunix", - }, - { - name: "absolute path", - value: "/foo/bar/baz", - expected: "/foo/bar/baz", - }, - { - name: "absolute path with query parameters", - value: "/foo/bar/baz?gnu=notunix", - expected: "/foo/bar/baz?gnu=notunix", - }, - } { - t.Run(test.name, func(t *testing.T) { - r.Header.Set("Referer", test.value) - assert.Equal(t, test.expected, urlpkg.CanonicalRedirect(r)) - }) - } - }) - // If either redirect or redirect-encoded parameter is set, use that t.Run("redirect parameter is set", func(t *testing.T) { for _, test := range []struct {