revert PR #3009 changes to just disabling path escaping by default in static methods/middleware (#3016)

revert PR #3009 changes to just disabling path escaping by default in static methods/middleware (#3016)

Author: aldas

_fixture/dist/public/admin/private.txt

@@ -0,0 +1 @@
+public/admin/private.txt - private file

_fixture/dist/public/hello world.txt

@@ -0,0 +1 @@
+hello world file

echo.go

@@ -59,8 +59,6 @@ import (
 	"sync"
 	"sync/atomic"
 	"syscall"
-
-	"github.com/labstack/echo/v5/internal/pathutil"
 )
 
 // Echo is the top-level framework instance.
@@ -111,6 +109,8 @@ type Echo struct {
 
 	// formParseMaxMemory is passed to Context for multipart form parsing (See http.Request.ParseMultipartForm)
 	formParseMaxMemory int64
+
+	enablePathUnescapingStaticFiles bool
 }
 
 // JSONSerializer is the interface that encodes and decodes JSON to and from interfaces.
@@ -299,6 +299,31 @@ type Config struct {
 	// FormParseMaxMemory is default value for memory limit that is used
 	// when parsing multipart forms (See (*http.Request).ParseMultipartForm)
 	FormParseMaxMemory int64
+
+	// EnablePathUnescapingStaticFiles enables path parameter (param: *) unescaping for static file methods.
+	// Default false (safe): encoded slashes (%2f) in the wildcard param are NOT decoded,
+	// preventing ACL bypass where /admin%2fprivate.txt bypasses a /admin/* route guard by
+	// not matching that route but having its wildcard param decoded to admin/private.txt.
+	// Set to true only when serving files whose names contain URL-encoded characters
+	// (e.g. "hello world.txt" via /hello%20world.txt) and you are not relying on
+	// route-based ACL guards to restrict access.
+	// If you are enabling this option, make sure you understand the security implications.
+	// See: https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
+	//
+	// Enabling RouterConfig.UseEscapedPathForMatching makes this field irrelevant and can lead to security issues when
+	// using different Routes to exclude some of the files from being served.
+	// e.g. if you serve files from directory as such and use different route to exclude some of the files from being served.
+	// 0. given folder structure:
+	//   public/
+	//   public/index.html
+	//   public/admin/private.txt
+	// 1. share `public/` folder contents from the server root with `e.Static("/", "public")`
+	// 2. naively assume that everything under /admin folder is now forbidden
+	//       e.GET("/admin/*", func(c *Context) error { return echo.ErrForbidden })
+	// Then request to `/assets/../admin%2fprivate.txt` will be served as router does not match it to guarded route.
+	//
+	// Applies to methods: Echo.Static, Echo.StaticFS, Group.Static, Group.StaticFS.
+	EnablePathUnescapingStaticFiles bool
 }
 
 // NewWithConfig creates an instance of Echo with given configuration.
@@ -337,6 +362,8 @@ func NewWithConfig(config Config) *Echo {
 	if config.FormParseMaxMemory > 0 {
 		e.formParseMaxMemory = config.FormParseMaxMemory
 	}
+	e.enablePathUnescapingStaticFiles = config.EnablePathUnescapingStaticFiles
+
 	return e
 }
 
@@ -567,7 +594,7 @@ func (e *Echo) Static(pathPrefix, fsRoot string, middleware ...MiddlewareFunc) R
 	return e.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(subFs, false),
+		StaticDirectoryHandler(subFs, !e.enablePathUnescapingStaticFiles),
 		middleware...,
 	)
 }
@@ -581,35 +608,35 @@ func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Middl
 	return e.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
+		StaticDirectoryHandler(filesystem, !e.enablePathUnescapingStaticFiles),
 		middleware...,
 	)
 }
 
 // StaticDirectoryHandler creates handler function to serve files from provided file system
 // When disablePathUnescaping is set then file name from path is not unescaped and is served as is.
+//
+// Note: when disablePathUnescaping=false, the handler decodes the wildcard param before serving.
+// If route guards (e.g. e.GET("/admin/*", forbidden)) are used to restrict parts of the
+// filesystem, an encoded separator (%2F) or encoded dot-dot (%2E%2E) in the URL can resolve to
+// a path that the router never matched against the guard route. Enabling
+// RouterConfig.UseEscapedPathForMatching does NOT fix this — it changes which path the router
+// uses for matching but still lets path.Clean resolve ".." segments into a guarded directory.
+// Do not rely on route guards alone to restrict a filesystem served by this handler.
+// See https://github.com/labstack/echo/security/advisories/GHSA-vfp3-v2gw-7wfq
 func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) HandlerFunc {
 	return func(c *Context) error {
 		p := c.Param("*")
 		if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
-			// By default the router matches routes against the raw, still-encoded request path
-			// (unless UseEscapedPathForMatching is enabled), so an encoded path separator is not
-			// treated as a segment boundary during routing. Unescaping it here would let it act as
-			// a separator and resolve a file outside the path the router authorized, bypassing
-			// route-level middleware (e.g. auth on a sibling route). No real filename contains a
-			// separator, so reject it as not found, carrying the reason internally for operators.
-			if pathutil.HasEncodedPathSeparator(p) {
-				return NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)).
-					Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p))
-			}
 			tmpPath, err := url.PathUnescape(p)
 			if err != nil {
 				return fmt.Errorf("failed to unescape path variable: %w", err)
 			}
 			p = tmpPath
 		}
 
-		// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid.
+		// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/`
+		// as invalid
 		// Use path.Clean (not filepath.Clean): fs.FS paths are always forward-slash, so a backslash must stay a literal
 		// character rather than being interpreted as a separator on Windows (which would resolve a file across a boundary
 		// the router never matched on, the same Windows backslash traversal class as GHSA-pgvm-wxw2-hrv9).
@@ -619,10 +646,9 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle
 			return ErrNotFound
 		}
 
-		// If the request is for a directory and does not end with "/"
-		p = c.Request().URL.Path // path must not be empty.
+		// If the request is for a directory and does not end with "/" redirect to path which ends with "/"
+		p = c.Request().URL.Path
 		if fi.IsDir() && len(p) > 0 && p[len(p)-1] != '/' {
-			// Redirect to ends with "/"
 			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/"))
 		}
 		return fsFile(c, name, fileSystem)

echo_test.go

@@ -20,6 +20,7 @@ import (
 	"runtime"
 	"strings"
 	"testing"
+	"testing/fstest"
 	"time"
 
 	"github.com/stretchr/testify/assert"
@@ -133,15 +134,21 @@ func TestNewDefaultFS(t *testing.T) {
 }
 
 func TestEcho_StaticFS(t *testing.T) {
+	dotsInFilenameFS := fstest.MapFS{
+		// On Windows filename `%2f..` can not be created but in Linux it is possible.
+		"%2f..": {Data: []byte("This filename is escaped to `/..` in URL.Path")},
+	}
+
 	var testCases = []struct {
-		givenFs              fs.FS
-		name                 string
-		givenPrefix          string
-		givenFsRoot          string
-		whenURL              string
-		expectHeaderLocation string
-		expectBodyStartsWith string
-		expectStatus         int
+		givenFs                              fs.FS
+		name                                 string
+		givenPrefix                          string
+		givenFsRoot                          string
+		givenEnablePathUnescapingStaticFiles bool
+		whenURL                              string
+		expectHeaderLocation                 string
+		expectBodyStartsWith                 string
+		expectStatus                         int
 	}{
 		{
 			name:                 "ok",
@@ -250,6 +257,33 @@ func TestEcho_StaticFS(t *testing.T) {
 			expectStatus:         http.StatusNotFound,
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
+		{
+			name:                                 "do not accept encoded dots in path (%2E%2E is `..`) to traverse within filesystem boundary",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              `/dist/public/%2E%2E/private.txt`, // `/dist/public/../private.txt`
+			expectStatus:                         http.StatusNotFound,
+			expectBodyStartsWith:                 "{\"message\":\"Not Found\"}\n",
+		},
+		{
+			name:                                 "allow encoded dots in path (%2E%2E is `..`) to traverse within filesystem",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: true,
+			whenURL:                              `/dist/public/%2E%2E/private.txt`, // `/dist/public/../private.txt`
+			expectStatus:                         http.StatusOK,
+			expectBodyStartsWith:                 "private file",
+		},
+		{
+			name:                                 "ok, file with space in name is served when path unescaping is enabled",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/dist/public"),
+			givenEnablePathUnescapingStaticFiles: true,
+			whenURL:                              "/hello%20world.txt",
+			expectStatus:                         http.StatusOK,
+			expectBodyStartsWith:                 "hello world file",
+		},
 		{
 			name:                 "do not allow directory traversal (slash - unix separator)",
 			givenPrefix:          "/",
@@ -259,22 +293,42 @@ func TestEcho_StaticFS(t *testing.T) {
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
 		{
-			// An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): by default the
-			// router matches on the raw path so %2f is not a separator, and unescaping it here
-			// would let it act as one. No redirect is emitted, closing the open-redirect vector.
-			name:                 "encoded slash is rejected, not redirected",
+			name:                 "do not unescape path variables by default",
 			givenPrefix:          "/",
 			givenFs:              os.DirFS("_fixture/"),
 			whenURL:              "/open.redirect.hackercom%2f..",
 			expectStatus:         http.StatusNotFound,
-			expectHeaderLocation: "",
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
+		{
+			name:                                 "possible open redirect vulnerability when unescaping path variables in static handler",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: true,
+			// `//open.redirect.hackercom/..` resolves to directory but does not end with `/` so redirect is done but this
+			// redirect can not be to path starting with `//` or `\\` (open redirect)
+			whenURL:              "/%2fopen.redirect.hackercom%2f..",
+			expectStatus:         http.StatusMovedPermanently,
+			expectHeaderLocation: "/open.redirect.hackercom/../", // location starting with `//open` would be very bad
+			expectBodyStartsWith: "",
+		},
+		{
+			name:                                 "possible open redirect vulnerability when not unescaping path variables in static handler",
+			givenPrefix:                          "/",
+			givenFs:                              dotsInFilenameFS,
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/%2f..",
+			expectStatus:                         http.StatusOK,
+			expectHeaderLocation:                 "",
+			expectBodyStartsWith:                 "This filename is escaped to `/..` in URL.Path",
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			e := New()
+			e := NewWithConfig(Config{
+				EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles,
+			})
 
 			tmpFs := tc.givenFs
 			if tc.givenFsRoot != "" {
@@ -305,6 +359,122 @@ func TestEcho_StaticFS(t *testing.T) {
 	}
 }
 
+func TestStaticDirectoryHandlerAndRouterInconsistentEscaping(t *testing.T) {
+	var testCases = []struct {
+		name                                 string
+		givenEnablePathUnescapingStaticFiles bool
+		givenRouterUnescapePathParamValues   bool
+		givenRouterUseEscapedPathForMatching bool
+		whenURL                              string
+		expectBody                           string
+		expectStatus                         int
+	}{
+		{
+			name:                                 "ok, file is served from not-forbidden path",
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/test.txt",
+			expectBody:                           "test.txt contents",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                                 "ok, forbidden path is matched by route wildcard and forbidden by that",
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              "/admin/private.txt",
+			expectBody:                           "{\"message\":\"Forbidden\"}",
+			expectStatus:                         http.StatusForbidden,
+		},
+		{
+			name:                                 "ok, escaped filename from forbidden path is routed to guarded route",
+			givenEnablePathUnescapingStaticFiles: false,
+			givenRouterUnescapePathParamValues:   false,
+			givenRouterUseEscapedPathForMatching: true, // Router uses escaped path (req.URL.RawPath) for matching
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBody:                           "{\"message\":\"Forbidden\"}",
+			expectStatus:                         http.StatusForbidden,
+		},
+		{
+			name:                                 "ok, escaped filename from forbidden path is not unescaped and results 404",
+			givenEnablePathUnescapingStaticFiles: false, // router path escaping and StaticDirectoryHandler is consistent
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBody:                           "{\"message\":\"Not Found\"}",
+			expectStatus:                         http.StatusNotFound,
+		},
+		{
+			name:                                 "nok, escaped filename from forbidden path is unescaped and returns file contents (handler unescapes)",
+			givenEnablePathUnescapingStaticFiles: true, // router path escaping and StaticDirectoryHandler is NOT consistent
+			givenRouterUnescapePathParamValues:   false,
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBody:                           "public/admin/private.txt - private file",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                                 "nok, escaped filename from forbidden path is unescaped and returns file contents (router unescapes)",
+			givenEnablePathUnescapingStaticFiles: false,
+			givenRouterUnescapePathParamValues:   true, // router path escaping and StaticDirectoryHandler is NOT consistent
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBody:                           "public/admin/private.txt - private file",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                                 "nok, unescaped filename from forbidden path is escaped and returns file contents (router unescapes and method unescapes)",
+			givenEnablePathUnescapingStaticFiles: true,
+			givenRouterUnescapePathParamValues:   true, // consistent path unescaping - makes no difference
+			whenURL:                              "/admin%2fprivate.txt",
+			expectBody:                           "public/admin/private.txt - private file",
+			expectStatus:                         http.StatusOK,
+		},
+		{
+			name:                                 "nok, escaped filename, resolves to from forbidden path is not routed to guarded route and includes guarded file",
+			givenEnablePathUnescapingStaticFiles: false,
+			givenRouterUnescapePathParamValues:   false,
+			// Router uses escaped path (req.URL.RawPath) for matching, but that file resolves to `admin/private.txt` after path.Clean()
+			givenRouterUseEscapedPathForMatching: true,
+			whenURL:                              "/assets/../admin%2fprivate.txt",
+			expectBody:                           "public/admin/private.txt - private file",
+			expectStatus:                         http.StatusOK,
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			r := NewRouter(RouterConfig{
+				UnescapePathParamValues:   tc.givenRouterUnescapePathParamValues,
+				UseEscapedPathForMatching: tc.givenRouterUseEscapedPathForMatching,
+			})
+			e := NewWithConfig(Config{
+				EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles,
+				Router:                          r,
+				Filesystem:                      os.DirFS("./_fixture/dist"),
+			})
+
+			// 0.
+			// given folder structure:
+			// private.txt
+			// public/
+			// public/index.html
+			// public/text.txt
+			// public/admin/private.txt
+
+			// 1. share `public/` folder contents from the server root. This folder actually contains subfolder `admin` which
+			// contents we want to forbid from downloading
+			e.Static("/", "public")
+
+			// 2. naively assume that everything under /admin folder is now forbidden
+			e.GET("/admin/*", func(c *Context) error {
+				return ErrForbidden
+			})
+
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.Equal(t, tc.expectStatus, rec.Code)
+			body := strings.TrimRight(rec.Body.String(), "\r\n")
+			assert.Equal(t, tc.expectBody, body)
+		})
+	}
+}
+
 func TestEcho_FileFS(t *testing.T) {
 	var testCases = []struct {
 		whenFS           fs.FS

group.go

@@ -123,7 +123,7 @@ func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS, middleware ...Midd
 	return g.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
+		StaticDirectoryHandler(filesystem, !g.echo.enablePathUnescapingStaticFiles),
 		middleware...,
 	)
 }