Skip to content

Commit fc5588e

Browse files
vishrclaude
andcommitted
fix(static): reject encoded path separators that bypass route-level middleware
An encoded path separator in a static file URL could bypass route-level access control and disclose files (GHSA-vfp3-v2gw-7wfq, CWE-22). The router matches routes against the raw, still-encoded request path (by default), so %2F is not a segment separator -- /admin%2Fsecret.txt never matches a protected /admin/* group and falls through to the static handler, which then unescaped %2F back to "/" and served admin/secret.txt from disk. The auth middleware on the group never ran. Fixes, across both static serving paths: - StaticDirectoryHandler (echo.go) and the static middleware (middleware/static.go) now reject a wildcard containing an encoded separator (%2F/%2f or %5C/%5c) with 404 before unescaping, via a shared internal helper (internal/pathutil). No real filename contains a separator, so legitimate requests are unaffected. - StaticDirectoryHandler resolves the file name with path.Clean instead of the OS-specific filepath.Clean. fs.FS paths are always forward-slash, so a decoded backslash must stay a literal character rather than being treated as a separator on Windows (the GHSA-pgvm-wxw2-hrv9 backslash traversal class), matching what middleware/static.go already does. - Rejections return NewHTTPError(404).Wrap(...) carrying the offending path as an internal error, so logging middleware can observe the attempt while the client still sees a plain 404. Tests cover the bypass and non-disclosure for %2F, %5C, double-encoded %252F, the group StaticFS path, and the static middleware on a group, plus a unit test for the separator detector. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c9477eb commit fc5588e

7 files changed

Lines changed: 196 additions & 6 deletions

File tree

echo.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,11 +53,14 @@ import (
5353
"net/url"
5454
"os"
5555
"os/signal"
56+
"path"
5657
"path/filepath"
5758
"strings"
5859
"sync"
5960
"sync/atomic"
6061
"syscall"
62+
63+
"github.com/labstack/echo/v5/internal/pathutil"
6164
)
6265

6366
// Echo is the top-level framework instance.
@@ -589,15 +592,28 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle
589592
return func(c *Context) error {
590593
p := c.Param("*")
591594
if !disablePathUnescaping { // when router is already unescaping we do not want to do is twice
595+
// By default the router matches routes against the raw, still-encoded request path
596+
// (unless UseEscapedPathForMatching is enabled), so an encoded path separator is not
597+
// treated as a segment boundary during routing. Unescaping it here would let it act as
598+
// a separator and resolve a file outside the path the router authorized, bypassing
599+
// route-level middleware (e.g. auth on a sibling route). No real filename contains a
600+
// separator, so reject it as not found, carrying the reason internally for operators.
601+
if pathutil.HasEncodedPathSeparator(p) {
602+
return NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)).
603+
Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p))
604+
}
592605
tmpPath, err := url.PathUnescape(p)
593606
if err != nil {
594607
return fmt.Errorf("failed to unescape path variable: %w", err)
595608
}
596609
p = tmpPath
597610
}
598611

599-
// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid
600-
name := filepath.ToSlash(filepath.Clean(strings.TrimPrefix(p, "/")))
612+
// fs.FS.Open() already assumes that file names are relative to FS root path and considers name with prefix `/` as invalid.
613+
// Use path.Clean (not filepath.Clean): fs.FS paths are always forward-slash, so a backslash must stay a literal
614+
// character rather than being interpreted as a separator on Windows (which would resolve a file across a boundary
615+
// the router never matched on, the same Windows backslash traversal class as GHSA-pgvm-wxw2-hrv9).
616+
name := path.Clean(strings.TrimPrefix(p, "/"))
601617
fi, err := fs.Stat(fileSystem, name)
602618
if err != nil {
603619
return ErrNotFound

echo_test.go

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -259,13 +259,16 @@ func TestEcho_StaticFS(t *testing.T) {
259259
expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
260260
},
261261
{
262-
name: "open redirect vulnerability",
262+
// An encoded slash (%2f) is rejected outright (GHSA-vfp3-v2gw-7wfq): by default the
263+
// router matches on the raw path so %2f is not a separator, and unescaping it here
264+
// would let it act as one. No redirect is emitted, closing the open-redirect vector.
265+
name: "encoded slash is rejected, not redirected",
263266
givenPrefix: "/",
264267
givenFs: os.DirFS("_fixture/"),
265268
whenURL: "/open.redirect.hackercom%2f..",
266-
expectStatus: http.StatusMovedPermanently,
267-
expectHeaderLocation: "/open.redirect.hackercom/../", // location starting with `//open` would be very bad
268-
expectBodyStartsWith: "",
269+
expectStatus: http.StatusNotFound,
270+
expectHeaderLocation: "",
271+
expectBodyStartsWith: "{\"message\":\"Not Found\"}\n",
269272
},
270273
}
271274

internal/pathutil/pathutil.go

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// SPDX-License-Identifier: MIT
2+
// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors
3+
4+
// Package pathutil holds internal helpers for safely handling request and file
5+
// paths. It is internal so it can be shared between the echo package and the
6+
// middleware package without becoming part of the public API.
7+
package pathutil
8+
9+
// HasEncodedPathSeparator reports whether s contains a percent-encoded path
10+
// separator, case-insensitively: %2F/%2f (forward slash) or %5C/%5c (backslash).
11+
// Backslash is included as defense-in-depth against Windows-style separators even
12+
// though fs.FS itself only uses forward slashes.
13+
//
14+
// Such sequences let an attacker smuggle a separator past the router, which by
15+
// default matches on the raw encoded path, so they must be rejected before
16+
// unescaping when resolving static files.
17+
func HasEncodedPathSeparator(s string) bool {
18+
for i := 0; i+2 < len(s); i++ {
19+
if s[i] != '%' {
20+
continue
21+
}
22+
switch {
23+
case s[i+1] == '2' && (s[i+2] == 'f' || s[i+2] == 'F'): // %2F
24+
return true
25+
case s[i+1] == '5' && (s[i+2] == 'c' || s[i+2] == 'C'): // %5C
26+
return true
27+
}
28+
}
29+
return false
30+
}

internal/pathutil/pathutil_test.go

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
// SPDX-License-Identifier: MIT
2+
// SPDX-FileCopyrightText: © 2015 LabStack LLC and Echo contributors
3+
4+
package pathutil
5+
6+
import (
7+
"testing"
8+
9+
"github.com/stretchr/testify/assert"
10+
)
11+
12+
func TestHasEncodedPathSeparator(t *testing.T) {
13+
for s, want := range map[string]bool{
14+
"foo/bar.txt": false,
15+
"100%25.txt": false, // encoded percent, not a separator
16+
"a%2Fb": true,
17+
"a%2fb": true,
18+
"a%5Cb": true,
19+
"a%5cb": true,
20+
"trailing%2F": true,
21+
"%2F": true,
22+
"%2": false, // truncated, not a full sequence
23+
"": false,
24+
} {
25+
assert.Equal(t, want, HasEncodedPathSeparator(s), "input=%q", s)
26+
}
27+
}

middleware/static.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"sync"
1919

2020
"github.com/labstack/echo/v5"
21+
"github.com/labstack/echo/v5/internal/pathutil"
2122
)
2223

2324
// StaticConfig defines the config for Static middleware.
@@ -205,6 +206,14 @@ func (config StaticConfig) ToMiddleware() (echo.MiddlewareFunc, error) {
205206
pathUnescape = !config.DisablePathUnescaping // because router could already do PathUnescape
206207
}
207208
if pathUnescape {
209+
// The router matched on the raw, still-encoded path (by default), so an encoded
210+
// path separator in the wildcard would only now become a real separator and
211+
// resolve a file the matched route never authorized, bypassing route-level
212+
// middleware. Reject it before unescaping (see echo.StaticDirectoryHandler).
213+
if pathutil.HasEncodedPathSeparator(p) {
214+
return echo.NewHTTPError(http.StatusNotFound, http.StatusText(http.StatusNotFound)).
215+
Wrap(fmt.Errorf("rejected encoded path separator in static path %q", p))
216+
}
208217
p, err = url.PathUnescape(p)
209218
if err != nil {
210219
return err

middleware/static_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -280,6 +280,35 @@ func TestStatic(t *testing.T) {
280280
}
281281
}
282282

283+
// Regression for GHSA-vfp3-v2gw-7wfq: the static middleware mounted on a group
284+
// must not let an encoded separator in the wildcard bypass route-level middleware
285+
// and disclose a file the matched route never authorized.
286+
func TestStatic_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
287+
fsys := fstest.MapFS{
288+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
289+
"index.html": {Data: []byte("public")},
290+
}
291+
e := echo.New()
292+
g := e.Group("/files", StaticWithConfig(StaticConfig{Filesystem: fsys}))
293+
g.GET("/*", func(c *echo.Context) error { return echo.ErrNotFound })
294+
295+
cases := []struct {
296+
target string
297+
wantCode int
298+
}{
299+
{"/files/index.html", http.StatusOK},
300+
{"/files/admin%2Fsecret.txt", http.StatusNotFound},
301+
{"/files/admin%5Csecret.txt", http.StatusNotFound},
302+
}
303+
for _, tc := range cases {
304+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
305+
rec := httptest.NewRecorder()
306+
e.ServeHTTP(rec, req)
307+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
308+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
309+
}
310+
}
311+
283312
func TestMustStaticWithConfig_panicsInvalidDirListTemplate(t *testing.T) {
284313
assert.Panics(t, func() {
285314
StaticWithConfig(StaticConfig{DirectoryListTemplate: `{{}`})

static_encoded_separator_test.go

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
package echo
2+
3+
import (
4+
"net/http"
5+
"net/http/httptest"
6+
"testing"
7+
"testing/fstest"
8+
9+
"github.com/stretchr/testify/assert"
10+
)
11+
12+
// Regression for GHSA-vfp3-v2gw-7wfq: an encoded slash (%2F) must not let a static
13+
// file request resolve across a path separator and bypass route-level middleware.
14+
func TestStaticDirectoryHandler_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
15+
fsys := fstest.MapFS{
16+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
17+
"index.html": {Data: []byte("public")},
18+
}
19+
e := New()
20+
g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
21+
return func(c *Context) error { return c.String(http.StatusForbidden, "denied") }
22+
})
23+
g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
24+
e.StaticFS("/", fsys)
25+
26+
cases := []struct {
27+
target string
28+
wantCode int
29+
wantBody string
30+
}{
31+
{"/admin/secret.txt", http.StatusForbidden, "denied"}, // protected route fires
32+
{"/admin%2Fsecret.txt", http.StatusNotFound, ""}, // encoded slash rejected, no disclosure
33+
{"/admin%2fsecret.txt", http.StatusNotFound, ""}, // lower-case hex variant
34+
{"/admin%5Csecret.txt", http.StatusNotFound, ""}, // encoded backslash variant
35+
{"/admin%252Fsecret.txt", http.StatusNotFound, ""}, // double-encoded: single unescape -> literal filename, not a separator
36+
{"/index.html", http.StatusOK, "public"}, // legitimate static file still served
37+
}
38+
for _, tc := range cases {
39+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
40+
rec := httptest.NewRecorder()
41+
e.ServeHTTP(rec, req)
42+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
43+
if tc.wantBody != "" {
44+
assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
45+
}
46+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
47+
}
48+
}
49+
50+
// A Group-mounted StaticFS shares StaticDirectoryHandler, so it must reject the
51+
// same encoded separators when served under a non-root prefix.
52+
func TestGroupStaticFS_EncodedSeparatorDoesNotBypassRoute(t *testing.T) {
53+
fsys := fstest.MapFS{
54+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
55+
"index.html": {Data: []byte("public")},
56+
}
57+
e := New()
58+
g := e.Group("/files")
59+
g.StaticFS("/", fsys)
60+
61+
cases := []struct {
62+
target string
63+
wantCode int
64+
}{
65+
{"/files/index.html", http.StatusOK},
66+
{"/files/admin%2Fsecret.txt", http.StatusNotFound},
67+
{"/files/admin%5Csecret.txt", http.StatusNotFound},
68+
}
69+
for _, tc := range cases {
70+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
71+
rec := httptest.NewRecorder()
72+
e.ServeHTTP(rec, req)
73+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
74+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
75+
}
76+
}

0 commit comments

Comments
 (0)