backport PR 3016 from v5 to v4

echo.go

@@ -106,6 +106,17 @@ type Echo struct {
 	Debug             bool
 	HideBanner        bool
 	HidePort          bool
+
+	// EnablePathUnescapingStaticFiles enables path parameter (param: *) unescaping for Static/StaticFS 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
+	EnablePathUnescapingStaticFiles bool
 }
 
 // Route contains a handler and information for matching against requests.

echo_fs.go

@@ -12,8 +12,6 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
-
-	"github.com/labstack/echo/v4/internal/pathutil"
 )
 
 type filesystem struct {
@@ -38,7 +36,7 @@ func (e *Echo) Static(pathPrefix, fsRoot string) *Route {
 	return e.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(subFs, false),
+		StaticDirectoryHandler(subFs, !e.EnablePathUnescapingStaticFiles),
 	)
 }
 
@@ -51,24 +49,23 @@ func (e *Echo) StaticFS(pathPrefix string, filesystem fs.FS) *Route {
 	return e.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
+		StaticDirectoryHandler(filesystem, !e.EnablePathUnescapingStaticFiles),
 	)
 }
 
-// StaticDirectoryHandler creates handler function to serve files from provided file system
+// 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. 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
-			// The router matches routes against the raw, still-encoded request path, so an
-			// encoded path separator (%2F or %5C) 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.
-			if pathutil.HasEncodedPathSeparator(p) {
-				return ErrNotFound
-			}
 			tmpPath, err := url.PathUnescape(p)
 			if err != nil {
 				return fmt.Errorf("failed to unescape path variable: %w", err)

echo_fs_encoded_separator_test.go

@@ -15,65 +15,113 @@ import (
 // Regression for GHSA-vfp3-v2gw-7wfq (v4 backport): an encoded path separator (%2F or %5C)
 // must not let a static file request resolve across a separator and bypass route-level middleware.
 func TestStaticDirectoryHandler_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
-	fsys := fstest.MapFS{
-		"admin/secret.txt": {Data: []byte("TOP-SECRET")},
-		"index.html":       {Data: []byte("public")},
-	}
-	e := New()
-	g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
-		return func(c Context) error { return c.String(http.StatusForbidden, "denied") }
-	})
-	g.GET("/*", func(c Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
-	e.StaticFS("/", fsys)
-
-	cases := []struct {
+	var testCases = []struct {
+		name     string
 		target   string
 		wantCode int
 		wantBody string
 	}{
-		{"/admin/secret.txt", http.StatusForbidden, "denied"}, // protected route fires
-		{"/admin%2Fsecret.txt", http.StatusNotFound, ""},      // encoded slash rejected, no disclosure
-		{"/admin%2fsecret.txt", http.StatusNotFound, ""},      // lower-case hex variant
-		{"/admin%5Csecret.txt", http.StatusNotFound, ""},      // encoded backslash (Windows separator) neutralized by path.Clean
-		{"/admin%252Fsecret.txt", http.StatusNotFound, ""},    // double-encoded: single unescape -> literal filename, not a separator
-		{"/index.html", http.StatusOK, "public"},              // legitimate static file still served
+		{
+			name:     "protected route fires",
+			target:   "/admin/secret.txt",
+			wantCode: http.StatusForbidden,
+			wantBody: "denied",
+		},
+		{
+			name:     "encoded slash rejected, no disclosure",
+			target:   "/admin%2Fsecret.txt",
+			wantCode: http.StatusNotFound,
+			wantBody: "",
+		},
+		{
+			name:     "lower-case hex variant",
+			target:   "/admin%2fsecret.txt",
+			wantCode: http.StatusNotFound,
+			wantBody: "",
+		},
+		{
+			name:     "encoded backslash variant - Windows specific related",
+			target:   "/admin%5Csecret.txt",
+			wantCode: http.StatusNotFound,
+			wantBody: "",
+		},
+		{
+			name:     "double-encoded: single unescape -> literal filename, not a separator",
+			target:   "/admin%252Fsecret.txt",
+			wantCode: http.StatusNotFound,
+			wantBody: "",
+		},
+		{
+			name:     "legitimate static file still served",
+			target:   "/index.html",
+			wantCode: http.StatusOK,
+			wantBody: "public",
+		},
 	}
-	for _, tc := range cases {
-		req := httptest.NewRequest(http.MethodGet, tc.target, nil)
-		rec := httptest.NewRecorder()
-		e.ServeHTTP(rec, req)
-		assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
-		if tc.wantBody != "" {
-			assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
-		}
-		assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fsys := fstest.MapFS{
+				"admin/secret.txt": {Data: []byte("TOP-SECRET")},
+				"index.html":       {Data: []byte("public")},
+			}
+			e := New()
+			g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
+				return func(c Context) error { return c.String(http.StatusForbidden, "denied") }
+			})
+			g.GET("/*", func(c Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
+			e.StaticFS("/", fsys)
+
+			req := httptest.NewRequest(http.MethodGet, tc.target, nil)
+			rec := httptest.NewRecorder()
+			e.ServeHTTP(rec, req)
+			assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
+			if tc.wantBody != "" {
+				assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
+			}
+			assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
+		})
 	}
 }
 
 // A Group-mounted StaticFS shares StaticDirectoryHandler, so it must reject the
 // same encoded separators when served under a non-root prefix.
 func TestGroupStaticFS_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
-	fsys := fstest.MapFS{
-		"admin/secret.txt": {Data: []byte("TOP-SECRET")},
-		"index.html":       {Data: []byte("public")},
-	}
-	e := New()
-	g := e.Group("/files")
-	g.StaticFS("/", fsys)
-
-	cases := []struct {
+	var testCases = []struct {
+		name     string
 		target   string
 		wantCode int
 	}{
-		{"/files/index.html", http.StatusOK},
-		{"/files/admin%2Fsecret.txt", http.StatusNotFound},
-		{"/files/admin%5Csecret.txt", http.StatusNotFound},
+		{
+			name:     "ok",
+			target:   "/files/index.html",
+			wantCode: http.StatusOK,
+		},
+		{
+			name:     "nok, encoded slash",
+			target:   "/files/admin%2Fsecret.txt",
+			wantCode: http.StatusNotFound,
+		},
+		{
+			name:     "nok encoded backslash",
+			target:   "/files/admin%5Csecret.txt",
+			wantCode: http.StatusNotFound,
+		},
 	}
-	for _, tc := range cases {
-		req := httptest.NewRequest(http.MethodGet, tc.target, nil)
-		rec := httptest.NewRecorder()
-		e.ServeHTTP(rec, req)
-		assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
-		assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			fsys := fstest.MapFS{
+				"admin/secret.txt": {Data: []byte("TOP-SECRET")},
+				"index.html":       {Data: []byte("public")},
+			}
+			e := New()
+			g := e.Group("/files")
+			g.StaticFS("/", fsys)
+
+			req := httptest.NewRequest(http.MethodGet, tc.target, nil)
+			rec := httptest.NewRecorder()
+			e.ServeHTTP(rec, req)
+			assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
+			assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
+		})
 	}
 }

echo_fs_test.go

@@ -15,14 +15,15 @@ import (
 
 func TestEcho_StaticFS(t *testing.T) {
 	var testCases = []struct {
-		name                 string
-		givenPrefix          string
-		givenFs              fs.FS
-		givenFsRoot          string
-		whenURL              string
-		expectStatus         int
-		expectHeaderLocation string
-		expectBodyStartsWith string
+		name                                 string
+		givenPrefix                          string
+		givenFs                              fs.FS
+		givenFsRoot                          string
+		givenEnablePathUnescapingStaticFiles bool
+		whenURL                              string
+		expectStatus                         int
+		expectHeaderLocation                 string
+		expectBodyStartsWith                 string
 	}{
 		{
 			name:                 "ok",
@@ -140,22 +141,38 @@ func TestEcho_StaticFS(t *testing.T) {
 			expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
 		},
 		{
-			// An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): 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:                                 "do not accept encoded dots in path (%2E%2E is `..`) to traverse within filesystem boundary",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: false,
+			whenURL:                              `/folder/%2E%2E/index.html`, // `/folder/../index.html`
+			expectStatus:                         http.StatusNotFound,
+			expectBodyStartsWith:                 "{\"message\":\"Not Found\"}\n",
+		},
+		{
+			name:                                 "allow encoded dots in path (%2E%2E is `..`) to traverse within filesystem when path unescaping is enabled",
+			givenPrefix:                          "/",
+			givenFs:                              os.DirFS("_fixture/"),
+			givenEnablePathUnescapingStaticFiles: true,
+			whenURL:                              `/folder/%2E%2E/index.html`, // `/folder/../index.html`
+			expectStatus:                         http.StatusOK,
+			expectBodyStartsWith:                 "<!doctype html>",
+		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			e := New()
+			e.EnablePathUnescapingStaticFiles = tc.givenEnablePathUnescapingStaticFiles
 
 			tmpFs := tc.givenFs
 			if tc.givenFsRoot != "" {

group_fs.go

@@ -23,7 +23,7 @@ func (g *Group) StaticFS(pathPrefix string, filesystem fs.FS) {
 	g.Add(
 		http.MethodGet,
 		pathPrefix+"*",
-		StaticDirectoryHandler(filesystem, false),
+		StaticDirectoryHandler(filesystem, !g.echo.EnablePathUnescapingStaticFiles),
 	)
 }