From 8c86ed9493f36e36e5909b7507c5ca9e714c743e Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 29 Sep 2026 10:38:23 -0700 Subject: [PATCH 1/3] static: test that a non-validating custom fs.FS cannot escape its root, fix comment --- echo_test.go | 33 +++++++++++++++++++++++++++++++ middleware/static.go | 3 ++- middleware/static_test.go | 41 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 1 deletion(-) diff --git a/echo_test.go b/echo_test.go index 6c2201750..3ee1080c4 100644 --- a/echo_test.go +++ b/echo_test.go @@ -1889,3 +1889,36 @@ func TestStaticDirectoryHandler_encodedDotsWithPathUnescaping(t *testing.T) { }) } } + +// nonValidatingDirFS is a custom fs.FS that does not enforce fs.ValidPath, so a name with ".." escapes its root. Echo +// must never pass such a name to a user supplied filesystem. +type nonValidatingDirFS struct{ root string } + +func (f nonValidatingDirFS) Open(name string) (fs.File, error) { + return os.Open(filepath.Join(f.root, name)) +} + +func TestEcho_StaticFS_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + e := New() + e.StaticFS("/", nonValidatingDirFS{root: filepath.Join(dir, "public")}) + + for _, target := range []string{"/../secret.txt", "/%2e%2e/secret.txt", "/..%2fsecret.txt", "/sub/../../secret.txt"} { + req := httptest.NewRequest(http.MethodGet, target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNotFound, rec.Code, target) + assert.NotContains(t, rec.Body.String(), "secret", target) + } + + req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "public", rec.Body.String()) +} diff --git a/middleware/static.go b/middleware/static.go index deb77cd64..b4cb1e288 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -246,7 +246,8 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { // Security: We use path.Clean() (not filepath.Clean()) because: // 1. HTTP URLs always use forward slashes, regardless of server OS // 2. path.Clean() provides platform-independent behavior for URL paths - // 3. The "/" prefix forces absolute path interpretation, removing ".." components + // 3. A path with a ".." segment was rejected above as unclean, so the "./" prefix only keeps the name relative + // to the filesystem root; path.Clean() does not remove a leading ".." from a relative path // 4. Backslashes are treated as literal characters (not path separators), preventing traversal // See static_windows.go for Go 1.20+ filepath.Clean compatibility notes filePath := path.Clean("./" + p) diff --git a/middleware/static_test.go b/middleware/static_test.go index d1d59566e..c271540d0 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -8,6 +8,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "testing" "testing/fstest" @@ -795,3 +796,43 @@ func TestStatic_HTML5WithUncleanPath(t *testing.T) { assert.Equal(t, http.StatusOK, rec.Code) assert.Equal(t, "spa", rec.Body.String()) } + +// nonValidatingFS is a custom fs.FS that does not enforce fs.ValidPath, so a name with ".." escapes its root. Echo must +// never pass such a name to a user supplied filesystem. +type nonValidatingFS struct{ root string } + +func (f nonValidatingFS) Open(name string) (fs.File, error) { + return os.Open(filepath.Join(f.root, name)) +} + +func TestStatic_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { + dir := t.TempDir() + assert.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + assert.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + for _, target := range []string{"/../secret.txt", "/%2e%2e/secret.txt", "/sub/../../secret.txt"} { + for _, unescape := range []bool{false, true} { + e := echo.New() + e.Use(StaticWithConfig(StaticConfig{ + Filesystem: nonValidatingFS{root: filepath.Join(dir, "public")}, + EnablePathUnescaping: unescape, + })) + + req := httptest.NewRequest(http.MethodGet, target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", target, unescape) + assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", target, unescape) + } + } + + e := echo.New() + e.Use(StaticWithConfig(StaticConfig{Filesystem: nonValidatingFS{root: filepath.Join(dir, "public")}})) + req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "public", rec.Body.String()) +} From 865e5ac1c566244e20338b89711d80eecb52924c Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 29 Sep 2026 10:44:37 -0700 Subject: [PATCH 2/3] static: reject backslash dot segments, cover unescaping and group routes in tests --- echo.go | 9 +++++ echo_test.go | 67 ++++++++++++++++++++++++++-------- middleware/static.go | 15 ++++++-- middleware/static_test.go | 77 ++++++++++++++++++++++++++++++--------- 4 files changed, 132 insertions(+), 36 deletions(-) diff --git a/echo.go b/echo.go index c95033f8e..4ac4dff88 100644 --- a/echo.go +++ b/echo.go @@ -1033,6 +1033,15 @@ func hasDotOrEmptySegment(p string) bool { if segment == "" || segment == "." || segment == ".." { return true } + // A backslash is a literal character in fs.FS names, but a filesystem that wrongly treats it as a separator + // (for example one built on filepath.Join on Windows) would resolve `..\` outside its root. + if strings.Contains(segment, `\`) { + for part := range strings.SplitSeq(segment, `\`) { + if part == "." || part == ".." { + return true + } + } + } } return false } diff --git a/echo_test.go b/echo_test.go index 3ee1080c4..f919dc28a 100644 --- a/echo_test.go +++ b/echo_test.go @@ -24,6 +24,7 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type user struct { @@ -1900,25 +1901,61 @@ func (f nonValidatingDirFS) Open(name string) (fs.File, error) { func TestEcho_StaticFS_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { dir := t.TempDir() - assert.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) - assert.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) - assert.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + require.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + for _, unescape := range []bool{false, true} { + e := NewWithConfig(Config{EnablePathUnescapingStaticFiles: unescape}) + e.StaticFS("/", nonValidatingDirFS{root: filepath.Join(dir, "public")}) + + for _, target := range []string{ + "/../secret.txt", + "/%2e%2e/secret.txt", + "/..%2fsecret.txt", + "/sub/../../secret.txt", + "/..%5csecret.txt", + `/..\secret.txt`, + } { + req := httptest.NewRequest(http.MethodGet, target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) - e := New() - e.StaticFS("/", nonValidatingDirFS{root: filepath.Join(dir, "public")}) + assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", target, unescape) + assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", target, unescape) + } - for _, target := range []string{"/../secret.txt", "/%2e%2e/secret.txt", "/..%2fsecret.txt", "/sub/../../secret.txt"} { - req := httptest.NewRequest(http.MethodGet, target, nil) + req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) rec := httptest.NewRecorder() e.ServeHTTP(rec, req) - - assert.Equal(t, http.StatusNotFound, rec.Code, target) - assert.NotContains(t, rec.Body.String(), "secret", target) + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "public", rec.Body.String()) } +} - req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, http.StatusOK, rec.Code) - assert.Equal(t, "public", rec.Body.String()) +func TestHasDotOrEmptySegment(t *testing.T) { + var testCases = []struct { + path string + expect bool + }{ + {path: "", expect: false}, + {path: "/", expect: false}, + {path: "/index.html", expect: false}, + {path: "/css/app.css", expect: false}, + {path: "/..", expect: true}, + {path: "/a/../b", expect: true}, + {path: "/a/./b", expect: true}, + {path: "/a//b", expect: true}, + {path: `/..\secret.txt`, expect: true}, + {path: `/a\..\b`, expect: true}, + {path: `/.\secret.txt`, expect: true}, + {path: `/dir\file.txt`, expect: false}, + {path: `/a\\b`, expect: false}, + {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped + } + for _, tc := range testCases { + t.Run(tc.path, func(t *testing.T) { + assert.Equal(t, tc.expect, hasDotOrEmptySegment(tc.path)) + }) + } } diff --git a/middleware/static.go b/middleware/static.go index b4cb1e288..398273189 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -246,10 +246,10 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { // Security: We use path.Clean() (not filepath.Clean()) because: // 1. HTTP URLs always use forward slashes, regardless of server OS // 2. path.Clean() provides platform-independent behavior for URL paths - // 3. A path with a ".." segment was rejected above as unclean, so the "./" prefix only keeps the name relative - // to the filesystem root; path.Clean() does not remove a leading ".." from a relative path + // 3. A path with a ".." segment is unclean and is not opened (it is handled like a missing file below), so the + // "./" prefix only keeps the name relative to the filesystem root; path.Clean() does not remove a leading + // ".." from a relative path // 4. Backslashes are treated as literal characters (not path separators), preventing traversal - // See static_windows.go for Go 1.20+ filepath.Clean compatibility notes filePath := path.Clean("./" + p) if config.IgnoreBase { @@ -427,6 +427,15 @@ func hasDotOrEmptySegment(p string) bool { if segment == "" || segment == "." || segment == ".." { return true } + // A backslash is a literal character in fs.FS names, but a filesystem that wrongly treats it as a separator + // (for example one built on filepath.Join on Windows) would resolve `..\` outside its root. + if strings.Contains(segment, `\`) { + for part := range strings.SplitSeq(segment, `\`) { + if part == "." || part == ".." { + return true + } + } + } } return false } diff --git a/middleware/static_test.go b/middleware/static_test.go index c271540d0..67c6668b6 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -14,6 +14,7 @@ import ( "github.com/labstack/echo/v5" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestStatic_useCaseForApiAndSPAs(t *testing.T) { @@ -807,32 +808,72 @@ func (f nonValidatingFS) Open(name string) (fs.File, error) { func TestStatic_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { dir := t.TempDir() - assert.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) - assert.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) - assert.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) - - for _, target := range []string{"/../secret.txt", "/%2e%2e/secret.txt", "/sub/../../secret.txt"} { + require.NoError(t, os.Mkdir(filepath.Join(dir, "public"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "public", "index.txt"), []byte("public"), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "secret.txt"), []byte("secret"), 0o644)) + + targets := []string{ + "/../secret.txt", + "/%2e%2e/secret.txt", + "/..%2fsecret.txt", + "/sub/../../secret.txt", + "/..%5csecret.txt", + `/..\secret.txt`, + } + for _, group := range []string{"", "/static"} { for _, unescape := range []bool{false, true} { e := echo.New() - e.Use(StaticWithConfig(StaticConfig{ + mw := StaticWithConfig(StaticConfig{ Filesystem: nonValidatingFS{root: filepath.Join(dir, "public")}, EnablePathUnescaping: unescape, - })) + }) + if group == "" { + e.Use(mw) + } else { + e.Group(group, mw) + } - req := httptest.NewRequest(http.MethodGet, target, nil) + for _, target := range targets { + req := httptest.NewRequest(http.MethodGet, group+target, nil) + rec := httptest.NewRecorder() + e.ServeHTTP(rec, req) + + assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", group+target, unescape) + assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", group+target, unescape) + } + + req := httptest.NewRequest(http.MethodGet, group+"/index.txt", nil) rec := httptest.NewRecorder() e.ServeHTTP(rec, req) - - assert.Equal(t, http.StatusNotFound, rec.Code, "%s unescape=%v", target, unescape) - assert.NotContains(t, rec.Body.String(), "secret", "%s unescape=%v", target, unescape) + assert.Equal(t, http.StatusOK, rec.Code, "group=%q unescape=%v", group, unescape) + assert.Equal(t, "public", rec.Body.String()) } } +} - e := echo.New() - e.Use(StaticWithConfig(StaticConfig{Filesystem: nonValidatingFS{root: filepath.Join(dir, "public")}})) - req := httptest.NewRequest(http.MethodGet, "/index.txt", nil) - rec := httptest.NewRecorder() - e.ServeHTTP(rec, req) - assert.Equal(t, http.StatusOK, rec.Code) - assert.Equal(t, "public", rec.Body.String()) +func TestHasDotOrEmptySegment(t *testing.T) { + var testCases = []struct { + path string + expect bool + }{ + {path: "", expect: false}, + {path: "/", expect: false}, + {path: "/index.html", expect: false}, + {path: "/css/app.css", expect: false}, + {path: "/..", expect: true}, + {path: "/a/../b", expect: true}, + {path: "/a/./b", expect: true}, + {path: "/a//b", expect: true}, + {path: `/..\secret.txt`, expect: true}, + {path: `/a\..\b`, expect: true}, + {path: `/.\secret.txt`, expect: true}, + {path: `/dir\file.txt`, expect: false}, + {path: `/a\\b`, expect: false}, + {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped + } + for _, tc := range testCases { + t.Run(tc.path, func(t *testing.T) { + assert.Equal(t, tc.expect, hasDotOrEmptySegment(tc.path)) + }) + } } From b123cc4842eb280d0572fda48cfd5b8455d07ca9 Mon Sep 17 00:00:00 2001 From: Vishal Rana Date: Tue, 29 Sep 2026 10:54:52 -0700 Subject: [PATCH 3/3] static: test backslash hardening on every OS, add default-config vectors, fix comments --- echo.go | 4 ++-- echo_test.go | 9 ++++++++- middleware/static.go | 7 ++++--- middleware/static_test.go | 10 +++++++++- 4 files changed, 23 insertions(+), 7 deletions(-) diff --git a/echo.go b/echo.go index 4ac4dff88..614de188a 100644 --- a/echo.go +++ b/echo.go @@ -1020,8 +1020,8 @@ func escapeControlChars(s string) string { return b.String() } -// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment. A single leading and a single -// trailing slash are allowed. +// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment, or a segment with a "." or ".." +// part between backslashes (e.g. `..\x`). A single leading and a single trailing slash are allowed. // Keep in sync with the copy in middleware/static.go. func hasDotOrEmptySegment(p string) bool { p = strings.TrimPrefix(p, "/") diff --git a/echo_test.go b/echo_test.go index f919dc28a..e7c022541 100644 --- a/echo_test.go +++ b/echo_test.go @@ -1896,7 +1896,8 @@ func TestStaticDirectoryHandler_encodedDotsWithPathUnescaping(t *testing.T) { type nonValidatingDirFS struct{ root string } func (f nonValidatingDirFS) Open(name string) (fs.File, error) { - return os.Open(filepath.Join(f.root, name)) + // treat a backslash as a separator on every OS, like filepath.Join does on Windows + return os.Open(filepath.Join(f.root, filepath.FromSlash(strings.ReplaceAll(name, `\`, "/")))) } func TestEcho_StaticFS_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { @@ -1915,6 +1916,8 @@ func TestEcho_StaticFS_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { "/..%2fsecret.txt", "/sub/../../secret.txt", "/..%5csecret.txt", + "/..%5Csecret.txt", + "/a%5C..%5C..%5Csecret.txt", `/..\secret.txt`, } { req := httptest.NewRequest(http.MethodGet, target, nil) @@ -1949,7 +1952,11 @@ func TestHasDotOrEmptySegment(t *testing.T) { {path: `/..\secret.txt`, expect: true}, {path: `/a\..\b`, expect: true}, {path: `/.\secret.txt`, expect: true}, + {path: `/\..`, expect: true}, + {path: `/..\`, expect: true}, {path: `/dir\file.txt`, expect: false}, + {path: "/...", expect: false}, + {path: "/..foo", expect: false}, {path: `/a\\b`, expect: false}, {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped } diff --git a/middleware/static.go b/middleware/static.go index 398273189..3fbef23ed 100644 --- a/middleware/static.go +++ b/middleware/static.go @@ -249,7 +249,8 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) { // 3. A path with a ".." segment is unclean and is not opened (it is handled like a missing file below), so the // "./" prefix only keeps the name relative to the filesystem root; path.Clean() does not remove a leading // ".." from a relative path - // 4. Backslashes are treated as literal characters (not path separators), preventing traversal + // 4. path.Clean() treats backslashes as literal characters; "."/".." parts between backslashes are rejected above + // for filesystems that wrongly treat a backslash as a separator filePath := path.Clean("./" + p) if config.IgnoreBase { @@ -414,8 +415,8 @@ func format(b int64) string { return fmt.Sprintf("%.2f%s", value, multiple) } -// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment. A single leading and a single -// trailing slash are allowed. +// hasDotOrEmptySegment reports whether URL path p has a ".", ".." or empty segment, or a segment with a "." or ".." +// part between backslashes (e.g. `..\x`). A single leading and a single trailing slash are allowed. // Keep in sync with the copy in echo.go. func hasDotOrEmptySegment(p string) bool { p = strings.TrimPrefix(p, "/") diff --git a/middleware/static_test.go b/middleware/static_test.go index 67c6668b6..5db1be2c7 100644 --- a/middleware/static_test.go +++ b/middleware/static_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "strings" "testing" "testing/fstest" @@ -803,7 +804,8 @@ func TestStatic_HTML5WithUncleanPath(t *testing.T) { type nonValidatingFS struct{ root string } func (f nonValidatingFS) Open(name string) (fs.File, error) { - return os.Open(filepath.Join(f.root, name)) + // treat a backslash as a separator on every OS, like filepath.Join does on Windows + return os.Open(filepath.Join(f.root, filepath.FromSlash(strings.ReplaceAll(name, `\`, "/")))) } func TestStatic_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { @@ -818,6 +820,8 @@ func TestStatic_nonValidatingCustomFSCannotEscapeRoot(t *testing.T) { "/..%2fsecret.txt", "/sub/../../secret.txt", "/..%5csecret.txt", + "/..%5Csecret.txt", + "/a%5C..%5C..%5Csecret.txt", `/..\secret.txt`, } for _, group := range []string{"", "/static"} { @@ -867,7 +871,11 @@ func TestHasDotOrEmptySegment(t *testing.T) { {path: `/..\secret.txt`, expect: true}, {path: `/a\..\b`, expect: true}, {path: `/.\secret.txt`, expect: true}, + {path: `/\..`, expect: true}, + {path: `/..\`, expect: true}, {path: `/dir\file.txt`, expect: false}, + {path: "/...", expect: false}, + {path: "/..foo", expect: false}, {path: `/a\\b`, expect: false}, {path: "/..%2fsecret.txt", expect: false}, // still encoded, only unsafe once unescaped }