revert StaticDirectoryHandler to redirect using unescaped path as basis for redirect

Author: aldas

_fixture/dist/public/admin/private.txt

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

echo.go

@@ -43,7 +43,6 @@ Learn more at https://echo.labstack.com
 package echo
 
 import (
-	"cmp"
 	stdContext "context"
 	"encoding/json"
 	"errors"
@@ -310,7 +309,19 @@ type Config struct {
 	// 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 path escaping in static files and router consistent.
+	//
+	// 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
 }
@@ -635,10 +646,7 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle
 		// 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] != '/' {
-			// prefer RawPath in redirect to avoid inconsistency when router is possible already escaping or not escaping
-			// as we are redirecting to same path, except added "/" at the end the RawPath is more consistent behavior
-			redirectPath := cmp.Or(c.Request().URL.RawPath, c.Request().URL.Path)
-			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(redirectPath+"/"))
+			return c.Redirect(http.StatusMovedPermanently, sanitizeURI(p+"/"))
 		}
 		return fsFile(c, name, fileSystem)
 	}

echo_test.go

@@ -278,10 +278,12 @@ func TestEcho_StaticFS(t *testing.T) {
 			givenPrefix:                          "/",
 			givenFs:                              os.DirFS("_fixture/"),
 			givenEnablePathUnescapingStaticFiles: true,
-			whenURL:                              "/open.redirect.hackercom%2f..",
-			expectStatus:                         http.StatusMovedPermanently,
-			expectHeaderLocation:                 "/open.redirect.hackercom%2f../", // location starting with `//open` would be very bad
-			expectBodyStartsWith:                 "",
+			// `//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",
@@ -335,59 +337,98 @@ func TestStaticDirectoryHandlerAndRouterInconsistentEscaping(t *testing.T) {
 		name                                 string
 		givenEnablePathUnescapingStaticFiles bool
 		givenRouterUnescapePathParamValues   bool
+		givenRouterUseEscapedPathForMatching bool
 		whenURL                              string
-		expectBodyStartsWith                 string
+		expectBody                           string
 		expectStatus                         int
 	}{
 		{
 			name:                                 "ok, file is served from not-forbidden path",
 			givenEnablePathUnescapingStaticFiles: false,
 			whenURL:                              "/test.txt",
-			expectBodyStartsWith:                 "test.txt contents\n",
+			expectBody:                           "test.txt contents\n",
 			expectStatus:                         http.StatusOK,
 		},
 		{
 			name:                                 "ok, forbidden path is matched by route wildcard and forbidden by that",
 			givenEnablePathUnescapingStaticFiles: false,
 			whenURL:                              "/admin/private.txt",
-			expectBodyStartsWith:                 "{\"message\":\"Forbidden\"}\n",
+			expectBody:                           "{\"message\":\"Forbidden\"}\n",
 			expectStatus:                         http.StatusForbidden,
 		},
 		{
-			name:                                 "ok, escaped filename from forbidden path is not escaped and results 404",
+			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\"}\n",
+			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",
-			expectBodyStartsWith:                 "{\"message\":\"Not Found\"}\n",
+			expectBody:                           "{\"message\":\"Not Found\"}\n",
 			expectStatus:                         http.StatusNotFound,
 		},
 		{
-			name:                                 "nok, escaped filename from forbidden path is escaped and returns file contents (handler escapes)",
+			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\n",
+			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",
-			expectBodyStartsWith:                 "private file\n",
+			expectBody:                           "public/admin/private.txt - private file\n",
 			expectStatus:                         http.StatusOK,
 		},
 		{
-			name:                               "nok, escaped filename from forbidden path is escaped and returns file contents (router escapes)",
-			givenRouterUnescapePathParamValues: true, // router path escaping and StaticDirectoryHandler is NOT consistent
-			whenURL:                            "/admin%2fprivate.txt",
-			expectBodyStartsWith:               "private file\n",
-			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\n",
+			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\n",
+			expectStatus:                         http.StatusOK,
 		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			r := NewRouter(RouterConfig{
-				UnescapePathParamValues: tc.givenRouterUnescapePathParamValues,
+				UnescapePathParamValues:   tc.givenRouterUnescapePathParamValues,
+				UseEscapedPathForMatching: tc.givenRouterUseEscapedPathForMatching,
 			})
 			e := NewWithConfig(Config{
 				EnablePathUnescapingStaticFiles: tc.givenEnablePathUnescapingStaticFiles,
 				Router:                          r,
 				Filesystem:                      os.DirFS("./_fixture/dist"),
 			})
 
-			// 1. share folder contents from the server root. This folder actually contains folder `admin` which contents we
-			// want to forbid from downloading
+			// 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
@@ -402,11 +443,7 @@ func TestStaticDirectoryHandlerAndRouterInconsistentEscaping(t *testing.T) {
 
 			assert.Equal(t, tc.expectStatus, rec.Code)
 			body := rec.Body.String()
-			if tc.expectBodyStartsWith != "" {
-				assert.True(t, strings.HasPrefix(body, tc.expectBodyStartsWith))
-			} else {
-				assert.Equal(t, "", body)
-			}
+			assert.Equal(t, tc.expectBody, body)
 		})
 	}
 }

middleware/static.go

@@ -53,7 +53,7 @@ type StaticConfig struct {
 	// Optional. Default value false.
 	IgnoreBase bool
 
-	// Deprecated: Use EnablePathUnescaping instead. DisablePathUnescaping will be removed in a future version.
+	// Deprecated: this field is ignored, use EnablePathUnescaping instead. DisablePathUnescaping will be removed in a future version.
 	// Note: previously the zero value (false) enabled unescaping, which was the unsafe default.
 	DisablePathUnescaping bool
 
@@ -64,6 +64,18 @@ type StaticConfig struct {
 	// 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.
+	//
+	// Enabling echo.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.
 	EnablePathUnescaping bool
 
 	// DirectoryListTemplate is template to list directory contents