From 8f3c5cde8879cd0d1f0cd69e5bfec9057be2aa16 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Tue, 12 Dec 2023 14:56:16 +0100 Subject: [PATCH] fix(handler/error): redirect callbacks to initial handlers, retry others as-is --- pkg/handler/error.go | 25 +++++++----- pkg/handler/error_test.go | 83 +++++++++++++++++++-------------------- 2 files changed, 57 insertions(+), 51 deletions(-) diff --git a/pkg/handler/error.go b/pkg/handler/error.go index a8375a8..d116827 100644 --- a/pkg/handler/error.go +++ b/pkg/handler/error.go @@ -48,18 +48,25 @@ func (s *Standalone) Retry(r *http.Request, loginCookie *openid.LoginCookie) str requestPath := r.URL.Path ingressPath := s.GetPath(r) - for _, path := range []string{paths.Logout, paths.LogoutLocal, paths.LogoutFrontChannel} { - if strings.HasSuffix(requestPath, paths.OAuth2+path) { - return requestPath + // redirect failed logout callbacks to logout + if strings.HasSuffix(requestPath, paths.OAuth2+paths.LogoutCallback) { + return ingressPath + paths.OAuth2 + paths.Logout + } + + // redirect failed login callbacks to login with original referer as the redirect_uri + if strings.HasSuffix(requestPath, paths.OAuth2+paths.LoginCallback) { + redirect := s.Redirect.Canonical(r) + if loginCookie != nil && len(loginCookie.Referer) > 0 { + redirect = s.Redirect.Clean(r, loginCookie.Referer) } + + return urlpkg.LoginRelative(ingressPath, redirect) } - redirect := s.Redirect.Canonical(r) - if loginCookie != nil && len(loginCookie.Referer) > 0 { - redirect = s.Redirect.Clean(r, loginCookie.Referer) - } - - return urlpkg.LoginRelative(ingressPath, redirect) + u := *r.URL + u.Host = "" + u.Scheme = "" + return u.String() } func (s *Standalone) respondError(w http.ResponseWriter, r *http.Request, statusCode int, cause error, level log.Level) { diff --git a/pkg/handler/error_test.go b/pkg/handler/error_test.go index ee3c4f8..826b352 100644 --- a/pkg/handler/error_test.go +++ b/pkg/handler/error_test.go @@ -85,7 +85,7 @@ func TestHandler_Retry(t *testing.T) { { name: "login path", request: get("/oauth2/login"), - want: "/oauth2/login?redirect=%2F", + want: "/oauth2/login", }, { name: "callback path", @@ -102,6 +102,11 @@ func TestHandler_Retry(t *testing.T) { request: get("/oauth2/logout/local"), want: "/oauth2/logout/local", }, + { + name: "logout callback path", + request: get("/oauth2/logout/callback"), + want: "/oauth2/logout", + }, { name: "front-channel logout path", request: get("/oauth2/logout/frontchannel"), @@ -111,7 +116,7 @@ func TestHandler_Retry(t *testing.T) { name: "login with non-default ingress", request: get("/domene/oauth2/login"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=%2Fdomene", + want: "/domene/oauth2/login", }, { name: "logout with non-default ingress", @@ -120,65 +125,59 @@ func TestHandler_Retry(t *testing.T) { want: "/domene/oauth2/logout", }, { - name: "login with cookie referer", - request: get("/oauth2/login"), + name: "login with query parameters", + request: get("/oauth2/login?acr=Level3&locale=en"), + want: "/oauth2/login?acr=Level3&locale=en", + }, + { + name: "login with redirect parameter set", + request: get("/oauth2/login?redirect=%2Fapi%2Fme"), + want: "/oauth2/login?redirect=%2Fapi%2Fme", + }, + { + name: "login with redirect parameter set and query parameters", + request: get("/oauth2/login?redirect=%2Fapi%2Fme%3Fa%3Db%26c%3Dd"), + want: "/oauth2/login?redirect=%2Fapi%2Fme%3Fa%3Db%26c%3Dd", + }, + { + name: "login with redirect parameter set on non-default ingress", + request: get("/domene/oauth2/login?redirect=%2Fapi%2Fme"), + ingress: "https://test.nav.no/domene", + want: "/domene/oauth2/login?redirect=%2Fapi%2Fme", + }, + { + name: "callback with cookie referer", + request: get("/oauth2/callback"), loginCookie: &openid.LoginCookie{Referer: "/"}, want: "/oauth2/login?redirect=%2F", }, { - name: "login with empty cookie referer", - request: get("/oauth2/login"), + name: "callback with empty cookie referer", + request: get("/oauth2/callback"), loginCookie: &openid.LoginCookie{Referer: ""}, want: "/oauth2/login?redirect=%2F", }, { - name: "login with cookie referer on non-default ingress", - request: get("/domene/oauth2/login"), + name: "callback with cookie referer on non-default ingress", + request: get("/domene/oauth2/callback"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, ingress: "https://test.nav.no/domene", want: "/domene/oauth2/login?redirect=%2Fdomene%2Fapi%2Fme", }, { - name: "login with redirect parameter set", - request: get("/oauth2/login?redirect=/api/me"), - want: "/oauth2/login?redirect=%2Fapi%2Fme", - }, - { - name: "login with redirect parameter set and query parameters", - request: get("/oauth2/login?redirect=/api/me?a=b%26c=d"), - want: "/oauth2/login?redirect=%2Fapi%2Fme%3Fa%3Db%26c%3Dd", - }, - { - name: "login with redirect parameter set on non-default ingress", - request: get("/domene/oauth2/login?redirect=/api/me"), - ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=%2Fapi%2Fme", - }, - { - 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=%2F", - }, - { - name: "login with redirect parameter set to absolute url", - request: get("/oauth2/login?redirect=http://localhost:8080"), + name: "callback with query parameters", + request: get("/oauth2/callback?code=some-code&state=some-state"), want: "/oauth2/login?redirect=%2F", }, { - name: "login with redirect parameter set to absolute url with trailing slash", - request: get("/oauth2/login?redirect=http://localhost:8080/"), - want: "/oauth2/login?redirect=%2F", - }, - { - name: "login with redirect parameter set to absolute url on non-default ingress", - request: get("/domene/oauth2/login?redirect=http://localhost:8080/"), + name: "callback with redirect parameter on non-default ingress", + request: get("/domene/oauth2/callback"), ingress: "https://test.nav.no/domene", - want: "/domene/oauth2/login?redirect=%2F", + want: "/domene/oauth2/login?redirect=%2Fdomene", }, { - name: "login with cookie referer takes precedence over redirect parameter", - request: get("/oauth2/login?redirect=/other"), + name: "callback with cookie referer takes precedence over redirect parameter", + request: get("/oauth2/callback?redirect=/other"), loginCookie: &openid.LoginCookie{Referer: "/domene/api/me"}, want: "/oauth2/login?redirect=%2Fdomene%2Fapi%2Fme", },