From d79f31c18d0ef06cf8d5392f271e297111220d65 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 21 Jul 2022 11:38:37 +0200 Subject: [PATCH] refactor(autologin): use glob-style matching instead of regex Regexes are powerful, but completely overkill and error-prone for this use-case. So instead, we'll use path.Match with its simpler glob-style patterns. --- README.md | 4 +-- pkg/autologin/autologin.go | 48 ++++++++++---------------------- pkg/config/config.go | 30 +------------------- pkg/config/config_test.go | 55 ------------------------------------- pkg/handler/handler_test.go | 37 ++++++++++++++----------- pkg/mock/config.go | 9 +----- 6 files changed, 39 insertions(+), 144 deletions(-) delete mode 100644 pkg/config/config_test.go diff --git a/README.md b/README.md index 7c621ec..09abd4b 100644 --- a/README.md +++ b/README.md @@ -77,7 +77,7 @@ 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-skip-paths strings Comma separated list of paths to ignore when 'auto-login' is enabled. Paths are evaluated as regular expressions. +--auto-login-skip-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. --error-redirect-uri string URI to redirect user to on errors for custom error handling. @@ -148,7 +148,7 @@ described previously: ### Requirements -- Go 1.17 +- Go 1.18 ### Binary diff --git a/pkg/autologin/autologin.go b/pkg/autologin/autologin.go index 92c1e39..322f5ed 100644 --- a/pkg/autologin/autologin.go +++ b/pkg/autologin/autologin.go @@ -2,14 +2,15 @@ package autologin import ( "net/http" - "regexp" + pathlib "path" + "strings" "github.com/nais/wonderwall/pkg/config" ) type Options struct { - Enabled bool - SkipRoutes []Route + Enabled bool + SkipPatterns []string } func (o *Options) NeedsLogin(r *http.Request, isAuthenticated bool) bool { @@ -17,8 +18,14 @@ func (o *Options) NeedsLogin(r *http.Request, isAuthenticated bool) bool { return false } - for _, route := range o.SkipRoutes { - if route.Regexp.MatchString(r.URL.Path) { + for _, pattern := range o.SkipPatterns { + path := r.URL.Path + if !strings.HasPrefix(path, "/") { + path = "/" + path + } + + match, _ := pathlib.Match(pattern, r.URL.Path) + if match { return false } } @@ -26,36 +33,9 @@ func (o *Options) NeedsLogin(r *http.Request, isAuthenticated bool) bool { return true } -type Route struct { - Path string - Regexp *regexp.Regexp -} - func NewOptions(cfg *config.Config) (*Options, error) { - routes, err := skippedRoutes(cfg) - if err != nil { - return nil, err - } - return &Options{ - Enabled: cfg.AutoLogin, - SkipRoutes: routes, + Enabled: cfg.AutoLogin, + SkipPatterns: cfg.AutoLoginSkipPaths, }, nil } - -func skippedRoutes(cfg *config.Config) ([]Route, error) { - routes := make([]Route, 0) - for _, path := range cfg.AutoLoginSkipPaths { - re, err := regexp.Compile(path) - if err != nil { - return nil, err - } - - routes = append(routes, Route{ - Path: path, - Regexp: re, - }) - } - - return routes, nil -} diff --git a/pkg/config/config.go b/pkg/config/config.go index 13faf71..ce49e72 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -1,8 +1,6 @@ package config import ( - "fmt" - "regexp" "time" "github.com/nais/liberator/pkg/conftools" @@ -33,29 +31,6 @@ type Config struct { Loginstatus Loginstatus `json:"loginstatus"` } -func (in *Config) Validate() error { - if err := in.validateAutoLoginSkipPaths(); err != nil { - return fmt.Errorf("validating '%s': %w", AutoLoginSkipPaths, err) - } - - return nil -} - -func (in *Config) validateAutoLoginSkipPaths() error { - for _, path := range in.AutoLoginSkipPaths { - if len(path) <= 0 { - return fmt.Errorf("path cannot be empty") - } - - _, err := regexp.Compile(path) - if err != nil { - return fmt.Errorf("could not compile regex for path '%s': %w", path, err) - } - } - - return nil -} - type Loginstatus struct { Enabled bool `json:"enabled"` CookieDomain string `json:"cookie-domain"` @@ -94,7 +69,7 @@ func Initialize() (*Config, error) { 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.StringSlice(AutoLoginSkipPaths, []string{}, "Comma separated list of paths to ignore when 'auto-login' is enabled. Paths are evaluated as regular expressions.") + flag.StringSlice(AutoLoginSkipPaths, []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.") flag.String(Ingress, "", "Ingress used to access the main application.") @@ -136,9 +111,6 @@ func Initialize() (*Config, error) { if err := conftools.Load(cfg); err != nil { return nil, err } - if err := cfg.Validate(); err != nil { - return nil, err - } if err := logging.Setup(cfg.LogLevel, cfg.LogFormat); err != nil { return nil, err diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go deleted file mode 100644 index a1014f0..0000000 --- a/pkg/config/config_test.go +++ /dev/null @@ -1,55 +0,0 @@ -package config_test - -import ( - "testing" - - "github.com/stretchr/testify/assert" - - "github.com/nais/wonderwall/pkg/config" -) - -func TestConfig_Validate(t *testing.T) { - t.Run("auto-login-skip-paths", func(t *testing.T) { - t.Run("valid", func(t *testing.T) { - paths := []string{ - "/some/path", - "^/some/path$", - "/some/.+/static/.+$", - } - - for _, path := range paths { - t.Run(path, func(t *testing.T) { - cfg := config.Config{ - AutoLoginSkipPaths: []string{path}, - } - - err := cfg.Validate() - assert.NoError(t, err) - }) - } - }) - - t.Run("invalid", func(t *testing.T) { - paths := []string{ - "[/some/path", - "^)/some/path$", - "[/some/.*$", - "", - "\\", - "/some/path\\", - "*", - } - - for _, path := range paths { - t.Run(path, func(t *testing.T) { - cfg := config.Config{ - AutoLoginSkipPaths: []string{path}, - } - - err := cfg.Validate() - assert.Error(t, err) - }) - } - }) - }) -} diff --git a/pkg/handler/handler_test.go b/pkg/handler/handler_test.go index 9f73639..7e2a27f 100644 --- a/pkg/handler/handler_test.go +++ b/pkg/handler/handler_test.go @@ -231,12 +231,12 @@ func TestHandler_Default(t *testing.T) { cfg.UpstreamHost = up.URL.Host cfg.AutoLogin = true cfg.AutoLoginSkipPaths = []string{ - "^/exact/match$", - "^/allowed(/?|/.*)$", - "/partial/(yup|yes)", + "/exact/match", + "/allowed", + "/wildcard/*", + "/deeper/*/*", + "/any*", } - err := cfg.Validate() - assert.NoError(t, err) idp := mock.NewIdentityProvider(cfg) defer idp.Close() @@ -247,14 +247,12 @@ func TestHandler_Default(t *testing.T) { matched := []string{ "/exact/match", "/allowed", - "/allowed/", - "/allowed/very", - "/allowed/very/cool", - "/partial/yes", - "/partial/yup", - "/partial/yes/no", - "/partial/yup/no", - "/parent/partial/yup/no", + "/wildcard/", + "/wildcard/very", + "/deeper/1/", + "/deeper/1/2", + "/anything", + "/anywho", } for _, path := range matched { t.Run(path, func(t *testing.T) { @@ -273,11 +271,18 @@ func TestHandler_Default(t *testing.T) { "/", "/exact/match/", "/exact/match/huh", + "/allowed/", "/not-allowed", "/not-allowed/allowed", - "/alloweded", - "/nope/partial/", - "/nope/partial/child", + "/wildcard", + "/wildcard/yup/nope", + "/deeper", + "/deeper/", + "/deeper/1", + "/deeper/1/2/", + "/deeper/1/2/3", + "/any/", + "/anywho/stvent", } for _, path := range nonMatched { t.Run(path, func(t *testing.T) { diff --git a/pkg/mock/config.go b/pkg/mock/config.go index e5e6c1e..ca885df 100644 --- a/pkg/mock/config.go +++ b/pkg/mock/config.go @@ -8,7 +8,7 @@ import ( ) func Config() *config.Config { - cfg := &config.Config{ + return &config.Config{ EncryptionKey: `G8Roe6AcoBpdr5GhO3cs9iORl4XIC8eq`, // 256 bits AES Ingress: "/", OpenID: config.OpenID{ @@ -21,13 +21,6 @@ func Config() *config.Config { }, SessionMaxLifetime: time.Hour, } - - err := cfg.Validate() - if err != nil { - panic(err) - } - - return cfg } type TestConfiguration struct {