Fix path check logic

Author: nothing0012

pkg/config/middleware.go

@@ -4,15 +4,16 @@ import (
 	"crypto/subtle"
 	"fmt"
 	"net/http"
+	"slices"
 	"strconv"
-	"strings"
 	"time"
 
 	"github.com/DataDog/datadog-go/statsd"
 	jwtmiddleware "github.com/auth0/go-jwt-middleware"
 	jwt "github.com/form3tech-oss/jwt-go"
 	"github.com/gohttp/pprof"
 	negronilogrus "github.com/meatballhat/negroni-logrus"
+	"github.com/openflagr/flagr/pkg/util"
 	"github.com/phyber/negroni-gzip/gzip"
 	"github.com/prometheus/client_golang/prometheus"
 	"github.com/prometheus/client_golang/prometheus/promhttp"
@@ -198,15 +199,13 @@ func (a *jwtAuth) whitelist(req *http.Request) bool {
 
 	// If we set to 401 unauthorized, let the client handles the 401 itself
 	if Config.JWTAuthNoTokenStatusCode == http.StatusUnauthorized {
-		for _, p := range a.ExactWhitelistPaths {
-			if p == path {
-				return true
-			}
+		if slices.Contains(a.ExactWhitelistPaths, path) {
+			return true
 		}
 	}
 
 	for _, p := range a.PrefixWhitelistPaths {
-		if p != "" && strings.HasPrefix(path, p) {
+		if p != "" && util.HasSafePrefix(path, p) {
 			return true
 		}
 	}
@@ -243,14 +242,12 @@ type basicAuth struct {
 func (a *basicAuth) whitelist(req *http.Request) bool {
 	path := req.URL.Path
 
-	for _, p := range a.ExactWhitelistPaths {
-		if p == path {
-			return true
-		}
+	if slices.Contains(a.ExactWhitelistPaths, path) {
+		return true
 	}
 
 	for _, p := range a.PrefixWhitelistPaths {
-		if p != "" && strings.HasPrefix(path, p) {
+		if p != "" && util.HasSafePrefix(path, p) {
 			return true
 		}
 	}

pkg/config/middleware_test.go

@@ -314,6 +314,27 @@ func TestJWTAuthMiddlewareWithUnauthorized(t *testing.T) {
 			})
 		}
 	})
+
+	t.Run("it will return 401 for some paths", func(t *testing.T) {
+		Config.JWTAuthEnabled = true
+		Config.JWTAuthNoTokenStatusCode = http.StatusUnauthorized
+		defer func() {
+			Config.JWTAuthEnabled = false
+			Config.JWTAuthNoTokenStatusCode = http.StatusTemporaryRedirect
+		}()
+
+		testPaths := []string{"/api/v1/flags", "/api/v1/health/..", "/api/v1/admin", "//api/v1/flags", "/..", "/."}
+		for _, path := range testPaths {
+			t.Run(fmt.Sprintf("path: %s", path), func(t *testing.T) {
+				hh := SetupGlobalMiddleware(h)
+				res := httptest.NewRecorder()
+				res.Body = new(bytes.Buffer)
+				req, _ := http.NewRequest("GET", fmt.Sprintf("http://localhost:18000%s", path), nil)
+				hh.ServeHTTP(res, req)
+				assert.Equal(t, http.StatusUnauthorized, res.Code)
+			})
+		}
+	})
 }
 
 func TestBasicAuthMiddleware(t *testing.T) {

pkg/util/util.go

@@ -3,7 +3,9 @@ package util
 import (
 	"fmt"
 	"math"
+	"path"
 	"regexp"
+	"strings"
 	"time"
 
 	"github.com/dchest/uniuri"
@@ -43,6 +45,24 @@ func IsSafeValue(s string) (bool, string) {
 	return true, ""
 }
 
+// HasSafePrefix checks if the given string is a safe URL path prefix
+func HasSafePrefix(s string, prefix string) bool {
+	if prefix == "" {
+		return true
+	}
+
+	// Check for path traversal attempts or suspicious patterns
+	if s == "." || s == ".." || strings.Contains(s, "..") {
+		return false
+	}
+
+	// First normalize the path (prefix is controlled by us, no need to clean it)
+	cleanedS := path.Clean(s)
+
+	// Check if the normalized path starts with the prefix
+	return strings.HasPrefix(cleanedS, prefix)
+}
+
 // NewSecureRandomKey creates a new secure random key
 func NewSecureRandomKey() string {
 	return randomKeyPrefix + uniuri.NewLenChars(uniuri.StdLen, randomKeyCharset)

pkg/util/util_test.go

@@ -191,3 +191,110 @@ func TestNewSecureRandomKey(t *testing.T) {
 	ok, _ := IsSafeKey(NewSecureRandomKey())
 	assert.True(t, ok)
 }
+
+func TestHasSafePrefix(t *testing.T) {
+	tests := []struct {
+		name   string
+		s      string
+		prefix string
+		want   bool
+	}{
+		{
+			name:   "empty prefix always matches",
+			s:      "any/path/here",
+			prefix: "",
+			want:   true,
+		},
+		{
+			name:   "exact prefix match",
+			s:      "api/v1/flags",
+			prefix: "api/v1",
+			want:   true,
+		},
+		{
+			name:   "non-matching prefix",
+			s:      "api/v1/flags",
+			prefix: "api/v2",
+			want:   false,
+		},
+		{
+			name:   "prefix with trailing slash",
+			s:      "api/v1/flags",
+			prefix: "api/v1/",
+			want:   true,
+		},
+		{
+			name:   "path traversal attempt should fail",
+			s:      "../api/v1/flags",
+			prefix: "api",
+			want:   false,
+		},
+		{
+			name:   "good match",
+			s:      "api",
+			prefix: "api",
+			want:   true,
+		},
+		{
+			name:   "path traversal attempt should fail",
+			s:      "..",
+			prefix: "api",
+			want:   false,
+		},
+		{
+			name:   "path traversal attempt should fail",
+			s:      ".",
+			prefix: "api",
+			want:   false,
+		},
+		{
+			name:   "sneaky path traversal should fail",
+			s:      "api/v1/../../secrets",
+			prefix: "api",
+			want:   false,
+		},
+		{
+			name:   "path with dot should be cleaned",
+			s:      "api/./v1/flags",
+			prefix: "api/v1",
+			want:   true,
+		},
+		{
+			name:   "prefix with dot should be cleaned",
+			s:      "api/v1/flags",
+			prefix: "api/./v1",
+			want:   false,
+		},
+		{
+			name:   "multiple slashes should be cleaned",
+			s:      "api///v1////flags",
+			prefix: "api/v1",
+			want:   true,
+		},
+		{
+			name:   "complex traversal attempt should fail",
+			s:      "api/v1/flags/../../../etc/passwd",
+			prefix: "api",
+			want:   false,
+		},
+		{
+			name:   "complex traversal attempt should fail",
+			s:      "api/v1/health/../flags",
+			prefix: "api/v1/health",
+			want:   false,
+		},
+		{
+			name:   "longer path with valid prefix",
+			s:      "api/v1/flags/123/settings",
+			prefix: "api/v1/flags",
+			want:   true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := HasSafePrefix(tt.s, tt.prefix)
+			assert.Equal(t, tt.want, got, "HasSafePrefix(%v, %v)", tt.s, tt.prefix)
+		})
+	}
+}