handle edge cases

internal/controller/proxy_controller.go

@@ -9,14 +9,14 @@ import (
 	"regexp"
 	"strings"
 
+	"github.com/gin-gonic/gin"
+	"github.com/google/go-querystring/query"
+	"go.uber.org/dig"
+
 	"github.com/tinyauthapp/tinyauth/internal/model"
 	"github.com/tinyauthapp/tinyauth/internal/service"
 	"github.com/tinyauthapp/tinyauth/internal/utils"
 	"github.com/tinyauthapp/tinyauth/internal/utils/logger"
-	"go.uber.org/dig"
-
-	"github.com/gin-gonic/gin"
-	"github.com/google/go-querystring/query"
 )
 
 type AuthModuleType int
@@ -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,6 +403,11 @@ func (controller *ProxyController) getForwardAuthContext(c *gin.Context) (ProxyC
 		return ProxyContext{}, errors.New("x-forwarded-uri not found")
 	}
 
+	path, err := getRequestPath(uri)
+	if err != nil {
+		return ProxyContext{}, fmt.Errorf("invalid x-forwarded-uri: %w", err)
+	}
+
 	proto, ok := controller.getHeader(c, "x-forwarded-proto")
 
 	if !ok {
@@ -403,7 +421,7 @@ func (controller *ProxyController) getForwardAuthContext(c *gin.Context) (ProxyC
 	return ProxyContext{
 		Host:   host,
 		Proto:  proto,
-		Path:   uri,
+		Path:   path,
 		Method: method,
 		Type:   ForwardAuth,
 	}, nil
@@ -435,6 +453,9 @@ func (controller *ProxyController) getAuthRequestContext(c *gin.Context) (ProxyC
 	}
 
 	path := url.Path
+	if path == "" {
+		path = "/"
+	}
 	method := c.Request.Method
 
 	return ProxyContext{
@@ -462,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

internal/controller/proxy_controller_test.go

@@ -287,6 +287,76 @@ func TestProxyController(t *testing.T) {
 				assert.Equal(t, http.StatusOK, recorder.Code)
 			},
 		},
+		{
+			description: "Ensure path allow ACL does not match forwarded URI query string",
+			middlewares: []gin.HandlerFunc{},
+			run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {
+				req := httptest.NewRequest("GET", "/api/auth/traefik", nil)
+				req.Header.Set("x-forwarded-host", "path-allow.example.com")
+				req.Header.Set("x-forwarded-proto", "https")
+				req.Header.Set("x-forwarded-uri", "/admin?path=/allowed")
+				router.ServeHTTP(recorder, req)
+				assert.Equal(t, http.StatusUnauthorized, recorder.Code)
+			},
+		},
+		{
+			description: "Ensure path allow ACL does not match path substrings",
+			middlewares: []gin.HandlerFunc{},
+			run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {
+				req := httptest.NewRequest("GET", "/api/auth/traefik", nil)
+				req.Header.Set("x-forwarded-host", "path-allow.example.com")
+				req.Header.Set("x-forwarded-proto", "https")
+				req.Header.Set("x-forwarded-uri", "/admin/allowed")
+				router.ServeHTTP(recorder, req)
+				assert.Equal(t, http.StatusUnauthorized, recorder.Code)
+			},
+		},
+		{
+			description: "Ensure path block ACL works on forward auth",
+			middlewares: []gin.HandlerFunc{},
+			run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {
+				req := httptest.NewRequest("GET", "/api/auth/traefik", nil)
+				req.Header.Set("x-forwarded-host", "path-block.example.com")
+				req.Header.Set("x-forwarded-proto", "https")
+				req.Header.Set("x-forwarded-uri", "/blocked")
+				router.ServeHTTP(recorder, req)
+				assert.Equal(t, http.StatusUnauthorized, recorder.Code)
+			},
+		},
+		{
+			description: "Ensure path block ACL does not match forwarded URI query string",
+			middlewares: []gin.HandlerFunc{},
+			run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {
+				req := httptest.NewRequest("GET", "/api/auth/traefik", nil)
+				req.Header.Set("x-forwarded-host", "path-block.example.com")
+				req.Header.Set("x-forwarded-proto", "https")
+				req.Header.Set("x-forwarded-uri", "/admin?path=/blocked")
+				router.ServeHTTP(recorder, req)
+				assert.Equal(t, http.StatusOK, recorder.Code)
+			},
+		},
+		{
+			description: "Ensure path block ACL does not match path substrings",
+			middlewares: []gin.HandlerFunc{},
+			run: func(t *testing.T, router *gin.Engine, recorder *httptest.ResponseRecorder) {
+				req := httptest.NewRequest("GET", "/api/auth/traefik", nil)
+				req.Header.Set("x-forwarded-host", "path-block.example.com")
+				req.Header.Set("x-forwarded-proto", "https")
+				req.Header.Set("x-forwarded-uri", "/admin/blocked")
+				router.ServeHTTP(recorder, req)
+				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{},
@@ -297,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{},

internal/service/access_controls_rules.go

@@ -2,6 +2,7 @@ package service
 
 import (
 	"errors"
+	"fmt"
 	"regexp"
 	"strings"
 
@@ -180,33 +181,61 @@ type AuthEnabledRule struct {
 	Log *logger.Logger
 }
 
+func matchPathRule(paths, path string) (bool, error) {
+	paths = strings.TrimRight(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 == "" {
+			continue
+		}
+
+		if strings.HasPrefix(path, configuredPath) {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
+
 func (rule *AuthEnabledRule) Evaluate(ctx *ACLContext) Effect {
 	if ctx.ACLs == nil {
 		return EffectDeny
 	}
 
 	if ctx.ACLs.Path.Block != "" {
-		regex, err := regexp.Compile(ctx.ACLs.Path.Block)
+		match, err := matchPathRule(ctx.ACLs.Path.Block, ctx.Path)
-
 		if err != nil {
-			rule.Log.App.Error().Err(err).Msg("Failed to compile block regex")
+			rule.Log.App.Warn().Err(err).Msg("Invalid path block rule")
 			return EffectDeny
 		}
 
-		if !regex.MatchString(ctx.Path) {
+		if !match {
 			return EffectAllow
 		}
 	}
 
 	if ctx.ACLs.Path.Allow != "" {
-		regex, err := regexp.Compile(ctx.ACLs.Path.Allow)
+		match, err := matchPathRule(ctx.ACLs.Path.Allow, ctx.Path)
-
 		if err != nil {
-			rule.Log.App.Error().Err(err).Msg("Failed to compile allow regex")
+			rule.Log.App.Warn().Err(err).Msg("Invalid path allow rule")
 			return EffectDeny
 		}
 
-		if regex.MatchString(ctx.Path) {
+		if match {
 			return EffectAllow
 		}
 	}

internal/service/access_controls_rules_test.go

@@ -527,52 +527,82 @@ func TestAuthEnabledRule(t *testing.T) {
 			expected: EffectDeny,
 		},
 		{
-			name: "allows when path does not match block regex",
+			name: "allows when path starts with allow path",
 			ctx: &ACLContext{
 				ACLs: &model.App{
-					Path: model.AppPath{Block: "^/admin"},
+					Path: model.AppPath{Allow: "/public"},
 				},
-				Path: "/public",
+				Path: "/publicity",
 			},
 			expected: EffectAllow,
 		},
 		{
-			name: "denies when path matches block regex and no allow regex",
+			name: "allows when path matches a comma-separated allow path",
 			ctx: &ACLContext{
 				ACLs: &model.App{
-					Path: model.AppPath{Block: "^/admin"},
+					Path: model.AppPath{Allow: "/bar,/foo/bar,/hello"},
 				},
-				Path: "/admin/users",
+				Path: "/foo/bar/baz",
 			},
-			expected: EffectDeny,
+			expected: EffectAllow,
+		},
+		{
+			name: "allows when comma-separated allow paths have trailing whitespace and commas",
+			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{
 				ACLs: &model.App{
-					Path: model.AppPath{Allow: "^/public"},
+					Path: model.AppPath{Allow: "/^/public-[0-9]+$/"},
 				},
-				Path: "/public/index",
+				Path: "/public-42",
 			},
 			expected: EffectAllow,
 		},
 		{
-			name: "denies when path does not match allow regex",
+			name: "denies when comma-separated allow paths do not match",
 			ctx: &ACLContext{
 				ACLs: &model.App{
-					Path: model.AppPath{Allow: "^/public"},
+					Path: model.AppPath{Allow: "/bar,/foo/bar,/hello"},
 				},
 				Path: "/private",
 			},
 			expected: EffectDeny,
 		},
 		{
-			name: "allows when blocked path is also explicitly allowed",
+			name: "denies when allow paths contain only whitespace and commas",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					Path: model.AppPath{Allow: " , , "},
+				},
+				Path: "/anything",
+			},
+			expected: EffectDeny,
+		},
+		{
+			name: "denies when path does not match allow path",
+			ctx: &ACLContext{
+				ACLs: &model.App{
+					Path: model.AppPath{Allow: "/public"},
+				},
+				Path: "/private",
+			},
+			expected: EffectDeny,
+		},
+		{
+			name: "allows when blocked path is explicitly allowed",
 			ctx: &ACLContext{
 				ACLs: &model.App{
 					Path: model.AppPath{
-						Block: "^/admin",
+						Block: "/admin",
-						Allow: "^/admin/public",
+						Allow: "/admin/public",
 					},
 				},
 				Path: "/admin/public/page",
@@ -580,20 +610,10 @@ func TestAuthEnabledRule(t *testing.T) {
 			expected: EffectAllow,
 		},
 		{
-			name: "denies when block regex fails to compile",
+			name: "denies when root is blocked",
 			ctx: &ACLContext{
 				ACLs: &model.App{
-					Path: model.AppPath{Block: "[invalid"},
+					Path: model.AppPath{Block: "/"},
-				},
-				Path: "/anything",
-			},
-			expected: EffectDeny,
-		},
-		{
-			name: "denies when allow regex fails to compile",
-			ctx: &ACLContext{
-				ACLs: &model.App{
-					Path: model.AppPath{Allow: "[invalid"},
 				},
 				Path: "/anything",
 			},

internal/test/test.go

@@ -61,6 +61,14 @@ func CreateTestConfigs(t *testing.T) (model.Config, model.RuntimeConfig) {
 					Allow: "/allowed",
 				},
 			},
+			"app_path_block": {
+				Config: model.AppConfig{
+					Domain: "path-block.example.com",
+				},
+				Path: model.AppPath{
+					Block: "/blocked",
+				},
+			},
 			"app_user_allow": {
 				Config: model.AppConfig{
 					Domain: "user-allow.example.com",