From b4eecfc663b69651fb2ed7f9cdd3fecadc795882 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Mon, 12 Sep 2022 12:33:42 +0200 Subject: [PATCH] fix(handler/autologin): only trigger for GET requests --- README.md | 2 +- pkg/config/config.go | 2 +- pkg/handler/autologin/autologin.go | 2 +- pkg/handler/handler_test.go | 37 ++++++++++++++++++++++-- pkg/handler/reverseproxy/reverseproxy.go | 2 +- 5 files changed, 39 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6d69e34..8c35868 100644 --- a/README.md +++ b/README.md @@ -85,7 +85,7 @@ openid.client-id -> WONDERWALL_OPENID_CLIENT_ID The following flags are available: ```shell ---auto-login Automatically redirect user to login if the user does not have a valid session for all proxied downstream requests. +--auto-login Automatically redirect all HTTP GET requests to login if the user does not have a valid session for all matching upstream paths. --auto-login-ignore-paths strings Comma separated list of absolute paths to ignore when 'auto-login' is enabled. Supports basic wildcard matching with glob-style single asterisks using the stdlib path.Match. Invalid patterns are ignored. --bind-address string Listen address for public connections. (default "127.0.0.1:3000") --encryption-key string Base64 encoded 256-bit cookie encryption key; must be identical in instances that share session store. diff --git a/pkg/config/config.go b/pkg/config/config.go index 773b727..2e211bf 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -77,7 +77,7 @@ func Initialize() (*Config, error) { flag.String(LogLevel, "info", "Logging verbosity level.") flag.String(MetricsBindAddress, "127.0.0.1:3001", "Listen address for metrics only.") - flag.Bool(AutoLogin, false, "Automatically redirect user to login if the user does not have a valid session for all proxied downstream requests.") + flag.Bool(AutoLogin, false, "Automatically redirect all HTTP GET requests to login if the user does not have a valid session for all matching upstream paths.") flag.StringSlice(AutoLoginIgnorePaths, []string{}, "Comma separated list of absolute paths to ignore when 'auto-login' is enabled. Supports basic wildcard matching with glob-style single asterisks using the stdlib path.Match. Invalid patterns are ignored.") flag.String(EncryptionKey, "", "Base64 encoded 256-bit cookie encryption key; must be identical in instances that share session store.") flag.String(ErrorRedirectURI, "", "URI to redirect user to on errors for custom error handling.") diff --git a/pkg/handler/autologin/autologin.go b/pkg/handler/autologin/autologin.go index cbd2db7..1ed92ee 100644 --- a/pkg/handler/autologin/autologin.go +++ b/pkg/handler/autologin/autologin.go @@ -19,7 +19,7 @@ type AutoLogin struct { } func (a *AutoLogin) NeedsLogin(r *http.Request, isAuthenticated bool) bool { - if isAuthenticated || !a.Enabled { + if isAuthenticated || !a.Enabled || r.Method != http.MethodGet { return false } diff --git a/pkg/handler/handler_test.go b/pkg/handler/handler_test.go index fd3df19..adb1069 100644 --- a/pkg/handler/handler_test.go +++ b/pkg/handler/handler_test.go @@ -371,7 +371,7 @@ func TestHandler_Default(t *testing.T) { target := idp.RelyingPartyServer.URL + "/" resp := get(t, rpClient, target) - assert.Equal(t, http.StatusSeeOther, resp.StatusCode) + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) // redirect should point to local login endpoint loginLocation := resp.Location @@ -416,6 +416,39 @@ func TestHandler_Default(t *testing.T) { assert.Equal(t, "ok", resp.Body) }) + t.Run("with auto-login for non-GET requests", func(t *testing.T) { + for _, method := range []string{ + http.MethodConnect, + http.MethodDelete, + http.MethodHead, + http.MethodOptions, + http.MethodPatch, + http.MethodPost, + http.MethodPut, + http.MethodTrace, + } { + t.Run(method, func(t *testing.T) { + cfg := mock.Config() + cfg.AutoLogin = true + cfg.UpstreamHost = up.URL.Host + idp := mock.NewIdentityProvider(cfg) + defer idp.Close() + + up.SetReverseProxyUrl(idp.RelyingPartyServer.URL) + rpClient := idp.RelyingPartyClient() + + req, err := http.NewRequest(method, idp.RelyingPartyServer.URL, nil) + assert.NoError(t, err) + + resp, err := rpClient.Do(req) + assert.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusUnauthorized, resp.StatusCode) + }) + } + }) + t.Run("with auto-login and ignored paths", func(t *testing.T) { cfg := mock.Config() cfg.UpstreamHost = up.URL.Host @@ -480,7 +513,7 @@ func TestHandler_Default(t *testing.T) { target := idp.RelyingPartyServer.URL + path resp := get(t, rpClient, target) - assert.Equal(t, http.StatusSeeOther, resp.StatusCode) + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) }) } }) diff --git a/pkg/handler/reverseproxy/reverseproxy.go b/pkg/handler/reverseproxy/reverseproxy.go index d45982c..42a4563 100644 --- a/pkg/handler/reverseproxy/reverseproxy.go +++ b/pkg/handler/reverseproxy/reverseproxy.go @@ -85,7 +85,7 @@ func (rp *ReverseProxy) Handler(src Source, w http.ResponseWriter, r *http.Reque path := src.GetPath(r) loginUrl := url.LoginURL(path, redirectTarget) - http.Redirect(w, r, loginUrl, http.StatusSeeOther) + http.Redirect(w, r, loginUrl, http.StatusTemporaryRedirect) return }