fix(middleware/static): don't double-unescape request path (#2599)

middleware/static.go

@@ -196,7 +196,10 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
 			}
 
 			p := c.Request().URL.Path
-			pathUnescape := true
+			// URL.Path is already decoded by net/http, so it must not be unescaped
+			// again (doing so breaks file names containing '%', see #2599). Only the
+			// wildcard param from a group route (set below) may still be escaped.
+			pathUnescape := false
 			if strings.HasSuffix(c.Path(), "*") { // When serving from a group, e.g. `/static*`.
 				p = c.Param("*")
 				pathUnescape = !config.DisablePathUnescaping // because router could already do PathUnescape

middleware/static_percent_test.go

@@ -0,0 +1,64 @@
+// SPDX-License-Identifier: MIT
+// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors
+
+package middleware
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"testing/fstest"
+
+	"github.com/labstack/echo/v5"
+	"github.com/stretchr/testify/assert"
+)
+
+// Regression test for #2599: a file whose name contains a percent sign must be
+// downloadable. http.Request.URL.Path is already decoded by net/http, so the
+// static middleware must not unescape it a second time (which turned
+// "/100%25.txt" into an "invalid URL escape" error or a missing file).
+func TestStatic_servesFileWithPercentInName(t *testing.T) {
+	e := echo.New()
+	e.Use(StaticWithConfig(StaticConfig{
+		Root: ".",
+		Filesystem: fstest.MapFS{
+			"100%.txt":      &fstest.MapFile{Data: []byte("hundred percent")},
+			"foo%20bar.txt": &fstest.MapFile{Data: []byte("literal percent twenty")},
+		},
+	}))
+
+	cases := map[string]string{
+		"/100%25.txt":      "hundred percent",
+		"/foo%2520bar.txt": "literal percent twenty",
+	}
+	for url, want := range cases {
+		req := httptest.NewRequest(http.MethodGet, url, nil)
+		rec := httptest.NewRecorder()
+		e.ServeHTTP(rec, req)
+		assert.Equal(t, http.StatusOK, rec.Code, "GET %s should serve the file", url)
+		assert.Equal(t, want, rec.Body.String(), "GET %s should return the file contents", url)
+	}
+}
+
+// Companion to #2599: not unescaping the already-decoded path must not weaken
+// traversal protection. A percent-encoded "../" must not escape the served root
+// (and notably must not be re-assembled from double-encoded input, as the old
+// double-unescape could do).
+func TestStatic_percentEncodedTraversalIsBlocked(t *testing.T) {
+	e := echo.New()
+	e.Use(StaticWithConfig(StaticConfig{
+		Root: "public",
+		Filesystem: fstest.MapFS{
+			"public/page.txt": &fstest.MapFile{Data: []byte("public page")},
+			"secret.txt":      &fstest.MapFile{Data: []byte("SECRET")},
+		},
+	}))
+
+	for _, url := range []string{"/..%2fsecret.txt", "/..%252fsecret.txt", "/%2e%2e%2fsecret.txt"} {
+		req := httptest.NewRequest(http.MethodGet, url, nil)
+		rec := httptest.NewRecorder()
+		e.ServeHTTP(rec, req)
+		assert.NotEqual(t, http.StatusOK, rec.Code, "GET %s must not escape the served root", url)
+		assert.NotContains(t, rec.Body.String(), "SECRET", "GET %s must not leak files above the root", url)
+	}
+}