diff --git a/middleware/static.go b/middleware/static.go index a9e67b626..152ddedee 100644 --- a/middleware/static.go +++ b/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 diff --git a/middleware/static_percent_test.go b/middleware/static_percent_test.go new file mode 100644 index 000000000..a65d75c90 --- /dev/null +++ b/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) + } +}