Skip to content

Commit acb3391

Browse files
committed
revert to path.Clean for FS path
1 parent 086005d commit acb3391

2 files changed

Lines changed: 98 additions & 46 deletions

File tree

echo.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,7 @@ import (
5454
"net/url"
5555
"os"
5656
"os/signal"
57+
"path"
5758
"path/filepath"
5859
"strings"
5960
"sync"
@@ -622,7 +623,10 @@ func StaticDirectoryHandler(fileSystem fs.FS, disablePathUnescaping bool) Handle
622623

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

static_encoded_separator_test.go

Lines changed: 93 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -12,65 +12,113 @@ import (
1212
// Regression for GHSA-vfp3-v2gw-7wfq: an encoded slash (%2F) must not let a static
1313
// file request resolve across a path separator and bypass route-level middleware.
1414
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 {
15+
var testCases = []struct {
16+
name string
2717
target string
2818
wantCode int
2919
wantBody string
3020
}{
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
21+
{
22+
name: "protected route fires",
23+
target: "/admin/secret.txt",
24+
wantCode: http.StatusForbidden,
25+
wantBody: "denied",
26+
},
27+
{
28+
name: "encoded slash rejected, no disclosure",
29+
target: "/admin%2Fsecret.txt",
30+
wantCode: http.StatusNotFound,
31+
wantBody: "",
32+
},
33+
{
34+
name: "lower-case hex variant",
35+
target: "/admin%2fsecret.txt",
36+
wantCode: http.StatusNotFound,
37+
wantBody: "",
38+
},
39+
{
40+
name: "encoded backslash variant - Windows specific related",
41+
target: "/admin%5Csecret.txt",
42+
wantCode: http.StatusNotFound,
43+
wantBody: "",
44+
},
45+
{
46+
name: "double-encoded: single unescape -> literal filename, not a separator",
47+
target: "/admin%252Fsecret.txt",
48+
wantCode: http.StatusNotFound,
49+
wantBody: "",
50+
},
51+
{
52+
name: "legitimate static file still served",
53+
target: "/index.html",
54+
wantCode: http.StatusOK,
55+
wantBody: "public",
56+
},
3757
}
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)
58+
for _, tc := range testCases {
59+
t.Run(tc.name, func(t *testing.T) {
60+
fsys := fstest.MapFS{
61+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
62+
"index.html": {Data: []byte("public")},
63+
}
64+
e := New()
65+
g := e.Group("/admin", func(next HandlerFunc) HandlerFunc {
66+
return func(c *Context) error { return c.String(http.StatusForbidden, "denied") }
67+
})
68+
g.GET("/*", func(c *Context) error { return c.String(http.StatusOK, "reached-protected-handler") })
69+
e.StaticFS("/", fsys)
70+
71+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
72+
rec := httptest.NewRecorder()
73+
e.ServeHTTP(rec, req)
74+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
75+
if tc.wantBody != "" {
76+
assert.Equal(t, tc.wantBody, rec.Body.String(), "GET %s", tc.target)
77+
}
78+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
79+
})
4780
}
4881
}
4982

5083
// A Group-mounted StaticFS shares StaticDirectoryHandler, so it must reject the
5184
// same encoded separators when served under a non-root prefix.
5285
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 {
86+
var testCases = []struct {
87+
name string
6288
target string
6389
wantCode int
6490
}{
65-
{"/files/index.html", http.StatusOK},
66-
{"/files/admin%2Fsecret.txt", http.StatusNotFound},
67-
{"/files/admin%5Csecret.txt", http.StatusNotFound},
91+
{
92+
name: "ok",
93+
target: "/files/index.html",
94+
wantCode: http.StatusOK,
95+
},
96+
{
97+
name: "nok, encoded slash",
98+
target: "/files/admin%2Fsecret.txt",
99+
wantCode: http.StatusNotFound,
100+
},
101+
{
102+
name: "nok encoded backslash",
103+
target: "/files/admin%5Csecret.txt",
104+
wantCode: http.StatusNotFound,
105+
},
68106
}
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)
107+
for _, tc := range testCases {
108+
t.Run(tc.name, func(t *testing.T) {
109+
fsys := fstest.MapFS{
110+
"admin/secret.txt": {Data: []byte("TOP-SECRET")},
111+
"index.html": {Data: []byte("public")},
112+
}
113+
e := New()
114+
g := e.Group("/files")
115+
g.StaticFS("/", fsys)
116+
117+
req := httptest.NewRequest(http.MethodGet, tc.target, nil)
118+
rec := httptest.NewRecorder()
119+
e.ServeHTTP(rec, req)
120+
assert.Equal(t, tc.wantCode, rec.Code, "GET %s", tc.target)
121+
assert.NotContains(t, rec.Body.String(), "TOP-SECRET", "GET %s leaked protected file", tc.target)
122+
})
75123
}
76124
}

0 commit comments

Comments
 (0)