From c5bccd0b7b83ec636b32c2231a39f075cd6b6759 Mon Sep 17 00:00:00 2001 From: Scott McKendry Date: Thu, 30 Jul 2026 07:06:54 +1200 Subject: [PATCH] should be non-breaking now ;) --- internal/controller/proxy_controller.go | 30 ++++++++++---- internal/controller/proxy_controller_test.go | 21 ++++++++++ internal/service/access_controls_rules.go | 41 ++++++++----------- .../service/access_controls_rules_test.go | 20 +++++++++ 4 files changed, 79 insertions(+), 33 deletions(-) diff --git a/internal/controller/proxy_controller.go b/internal/controller/proxy_controller.go index 01326c61..fb149508 100644 --- a/internal/controller/proxy_controller.go +++ b/internal/controller/proxy_controller.go @@ -345,6 +345,19 @@ func (controller *ProxyController) getHeader(c *gin.Context, header string) (str return val, strings.TrimSpace(val) != "" } +func getRequestPath(uri string) (string, error) { + parsedURI, err := url.ParseRequestURI(uri) + if err != nil { + return "", err + } + + if parsedURI.Path == "" { + return "/", nil + } + + return parsedURI.Path, nil +} + func (controller *ProxyController) useBrowserResponse(proxyCtx ProxyContext) bool { // If it's nginx we need non-browser response if proxyCtx.ProxyType == Nginx { @@ -390,16 +403,11 @@ func (controller *ProxyController) getForwardAuthContext(c *gin.Context) (ProxyC return ProxyContext{}, errors.New("x-forwarded-uri not found") } - parsedURI, err := url.ParseRequestURI(uri) - + path, err := getRequestPath(uri) if err != nil { return ProxyContext{}, fmt.Errorf("invalid x-forwarded-uri: %w", err) } - if parsedURI.Path == "" { - parsedURI.Path = "/" - } - proto, ok := controller.getHeader(c, "x-forwarded-proto") if !ok { @@ -413,7 +421,7 @@ func (controller *ProxyController) getForwardAuthContext(c *gin.Context) (ProxyC return ProxyContext{ Host: host, Proto: proto, - Path: parsedURI.Path, + Path: path, Method: method, Type: ForwardAuth, }, nil @@ -445,6 +453,9 @@ func (controller *ProxyController) getAuthRequestContext(c *gin.Context) (ProxyC } path := url.Path + if path == "" { + path = "/" + } method := c.Request.Method return ProxyContext{ @@ -472,7 +483,10 @@ func (controller *ProxyController) getExtAuthzContext(c *gin.Context) (ProxyCont } // We get the path from the query string - path := c.Query("path") + path, err := getRequestPath(c.Query("path")) + if err != nil { + return ProxyContext{}, fmt.Errorf("invalid path: %w", err) + } // For envoy we need to support every method method := c.Request.Method diff --git a/internal/controller/proxy_controller_test.go b/internal/controller/proxy_controller_test.go index f297f27f..27bb9ab2 100644 --- a/internal/controller/proxy_controller_test.go +++ b/internal/controller/proxy_controller_test.go @@ -347,6 +347,16 @@ func TestProxyController(t *testing.T) { assert.Equal(t, http.StatusOK, recorder.Code) }, }, + { + description: "Ensure path allow ACL ignores query strings for nginx auth request", + middlewares: []gin.HandlerFunc{}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + req := httptest.NewRequest("GET", "/api/auth/nginx", nil) + req.Header.Set("x-original-url", "https://path-allow.example.com/admin?path=/allowed") + router.ServeHTTP(recorder, req) + assert.Equal(t, http.StatusUnauthorized, recorder.Code) + }, + }, { description: "Ensure path allow ACL works on nginx auth request", middlewares: []gin.HandlerFunc{}, @@ -357,6 +367,17 @@ func TestProxyController(t *testing.T) { assert.Equal(t, http.StatusOK, recorder.Code) }, }, + { + description: "Ensure path allow ACL ignores query strings for envoy ext authz", + middlewares: []gin.HandlerFunc{}, + run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) { + req := httptest.NewRequest("HEAD", "/api/auth/envoy?path=/admin%3Fpath%3D/allowed", nil) + req.Host = "path-allow.example.com" + req.Header.Set("x-forwarded-proto", "https") + router.ServeHTTP(recorder, req) + assert.Equal(t, http.StatusUnauthorized, recorder.Code) + }, + }, { description: "Ensure path allow ACL works on envoy ext authz", middlewares: []gin.HandlerFunc{}, diff --git a/internal/service/access_controls_rules.go b/internal/service/access_controls_rules.go index 3062fe82..1a1e0d0a 100644 --- a/internal/service/access_controls_rules.go +++ b/internal/service/access_controls_rules.go @@ -182,32 +182,23 @@ type AuthEnabledRule struct { } func matchPathRule(paths, path string) (bool, error) { + paths = strings.TrimSpace(paths) + + if paths == "/" { + return true, nil + } + + if strings.HasPrefix(paths, "/") && strings.HasSuffix(paths, "/") { + regex, err := regexp.Compile(paths[1 : len(paths)-1]) + if err != nil { + return false, fmt.Errorf("invalid path regex %q: %w", paths, err) + } + + return regex.MatchString(path), nil + } + for _, configuredPath := range strings.Split(paths, ",") { - configuredPath = strings.TrimSpace(configuredPath) - - if configuredPath == "/" { - return true, nil - } - - if configuredPath == "" { - continue - } - - // only apply regex if the path starts and ends with a slash, e.g. /regex/ - if strings.HasPrefix(configuredPath, "/") && strings.HasSuffix(configuredPath, "/") { - regex, err := regexp.Compile(configuredPath[1 : len(configuredPath)-1]) - if err != nil { - return false, fmt.Errorf("invalid path regex %q: %w", configuredPath, err) - } - - if regex.MatchString(path) { - return true, nil - } - - continue - } - - if strings.HasPrefix(path, configuredPath) { + if strings.HasPrefix(path, strings.TrimSpace(configuredPath)) { return true, nil } } diff --git a/internal/service/access_controls_rules_test.go b/internal/service/access_controls_rules_test.go index 381206a3..e9018520 100644 --- a/internal/service/access_controls_rules_test.go +++ b/internal/service/access_controls_rules_test.go @@ -536,6 +536,16 @@ func TestAuthEnabledRule(t *testing.T) { }, expected: EffectAllow, }, + { + name: "allows when path matches a comma-separated allow path", + ctx: &ACLContext{ + ACLs: &model.App{ + Path: model.AppPath{Allow: "/bar,/foo/bar,/hello"}, + }, + Path: "/foo/bar/baz", + }, + expected: EffectAllow, + }, { name: "allows when path matches allow regex", ctx: &ACLContext{ @@ -546,6 +556,16 @@ func TestAuthEnabledRule(t *testing.T) { }, expected: EffectAllow, }, + { + name: "denies when comma-separated allow paths do not match", + ctx: &ACLContext{ + ACLs: &model.App{ + Path: model.AppPath{Allow: "/bar,/foo/bar,/hello"}, + }, + Path: "/private", + }, + expected: EffectDeny, + }, { name: "denies when path does not match allow path", ctx: &ACLContext{