Skip to content

Commit db7bebd

Browse files
vishrvishr
authored
fix(middleware): preserve escaped paths in slash middleware in v4 (#3225)
Backport of #3221. Closes #3206 for v4.
1 parent bf9d756 commit db7bebd

2 files changed

Lines changed: 186 additions & 16 deletions

File tree

‎middleware/slash.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,12 @@ func AddTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFunc
4949
req := c.Request()
5050
url := req.URL
5151
path := url.Path
52+
rawPath := url.EscapedPath()
5253
qs := c.QueryString()
53-
if !strings.HasSuffix(path, "/") {
54+
if !strings.HasSuffix(rawPath, "/") {
5455
path += "/"
55-
uri := path
56+
rawPath += "/"
57+
uri := rawPath
5658
if qs != "" {
5759
uri += "?" + qs
5860
}
@@ -65,6 +67,9 @@ func AddTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFunc
6567
// Forward
6668
req.RequestURI = uri
6769
url.Path = path
70+
if url.RawPath != "" {
71+
url.RawPath = rawPath
72+
}
6873
}
6974
return next(c)
7075
}
@@ -96,11 +101,12 @@ func RemoveTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFu
96101
req := c.Request()
97102
url := req.URL
98103
path := url.Path
104+
rawPath := url.EscapedPath()
99105
qs := c.QueryString()
100-
l := len(path) - 1
101-
if l > 0 && strings.HasSuffix(path, "/") {
102-
path = path[:l]
103-
uri := path
106+
if len(rawPath) > 1 && strings.HasSuffix(rawPath, "/") {
107+
path = path[:len(path)-1]
108+
rawPath = rawPath[:len(rawPath)-1]
109+
uri := rawPath
104110
if qs != "" {
105111
uri += "?" + qs
106112
}
@@ -113,6 +119,9 @@ func RemoveTrailingSlashWithConfig(config TrailingSlashConfig) echo.MiddlewareFu
113119
// Forward
114120
req.RequestURI = uri
115121
url.Path = path
122+
if url.RawPath != "" {
123+
url.RawPath = rawPath
124+
}
116125
}
117126
return next(c)
118127
}

‎middleware/slash_test.go‎

Lines changed: 171 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -43,17 +43,17 @@ func TestAddTrailingSlashWithConfig(t *testing.T) {
4343
{
4444
whenURL: "http://localhost:1323/%5Cexample.com",
4545
expectPath: `/\example.com`,
46-
expectLocation: []string{`/example.com/`},
46+
expectLocation: []string{`/%5Cexample.com/`},
4747
},
4848
{
4949
whenURL: `http://localhost:1323/\example.com`,
5050
expectPath: `/\example.com`,
51-
expectLocation: []string{`/example.com/`},
51+
expectLocation: []string{`/%5Cexample.com/`},
5252
},
5353
{
5454
whenURL: `http://localhost:1323/\\%5C////%5C\\\example.com`,
5555
expectPath: `/\\\////\\\\example.com`,
56-
expectLocation: []string{`/example.com/`},
56+
expectLocation: []string{`/%5C%5C%5C////%5C%5C%5C%5Cexample.com/`},
5757
},
5858
{
5959
whenURL: "http://localhost:1323//example.com",
@@ -63,7 +63,7 @@ func TestAddTrailingSlashWithConfig(t *testing.T) {
6363
{
6464
whenURL: "http://localhost:1323/%5C%5C",
6565
expectPath: `/\\`,
66-
expectLocation: []string{`/`},
66+
expectLocation: []string{`/%5C%5C/`},
6767
},
6868
// control characters that browsers strip from URLs must not hide a `//` prefix (GHSA-v753-g4cw-jm48)
6969
{
@@ -74,7 +74,7 @@ func TestAddTrailingSlashWithConfig(t *testing.T) {
7474
{
7575
whenURL: "http://localhost:1323/%09%5Cevil.example/x",
7676
expectPath: "/\t\\evil.example/x",
77-
expectLocation: []string{`/%09\evil.example/x/`},
77+
expectLocation: []string{`/%09%5Cevil.example/x/`},
7878
},
7979
{
8080
whenURL: "http://localhost:1323/%09//evil.example/x",
@@ -200,17 +200,17 @@ func TestRemoveTrailingSlashWithConfig(t *testing.T) {
200200
{
201201
whenURL: "http://localhost:1323/%5Cexample.com/",
202202
expectPath: `/\example.com/`,
203-
expectLocation: []string{`/example.com`},
203+
expectLocation: []string{`/%5Cexample.com`},
204204
},
205205
{
206206
whenURL: `http://localhost:1323/\example.com/`,
207207
expectPath: `/\example.com/`,
208-
expectLocation: []string{`/example.com`},
208+
expectLocation: []string{`/%5Cexample.com`},
209209
},
210210
{
211211
whenURL: `http://localhost:1323/\\%5C////%5C\\\example.com/`,
212212
expectPath: `/\\\////\\\\example.com/`,
213-
expectLocation: []string{`/example.com`},
213+
expectLocation: []string{`/%5C%5C%5C////%5C%5C%5C%5Cexample.com`},
214214
},
215215
{
216216
whenURL: "http://localhost:1323//example.com/",
@@ -220,7 +220,7 @@ func TestRemoveTrailingSlashWithConfig(t *testing.T) {
220220
{
221221
whenURL: "http://localhost:1323/%5C%5C/",
222222
expectPath: `/\\/`,
223-
expectLocation: []string{`/`},
223+
expectLocation: []string{`/%5C%5C`},
224224
},
225225
// control characters that browsers strip from URLs must not hide a `//` prefix (GHSA-v753-g4cw-jm48)
226226
{
@@ -231,7 +231,7 @@ func TestRemoveTrailingSlashWithConfig(t *testing.T) {
231231
{
232232
whenURL: "http://localhost:1323/%09%5Cevil.example/x/",
233233
expectPath: "/\t\\evil.example/x/",
234-
expectLocation: []string{`/%09\evil.example/x`},
234+
expectLocation: []string{`/%09%5Cevil.example/x`},
235235
},
236236
{
237237
whenURL: "http://localhost:1323/%09//evil.example/x/",
@@ -344,3 +344,164 @@ func TestSanitizeURI(t *testing.T) {
344344
})
345345
}
346346
}
347+
348+
func TestTrailingSlashRedirectEscapedPath(t *testing.T) {
349+
for _, tc := range []struct{ path, add, remove string }{
350+
{"/files/a%2Fb", "/files/a%2Fb/", "/files/a%2Fb"},
351+
{"/files/a%3Fb?x=1", "/files/a%3Fb/?x=1", "/files/a%3Fb?x=1"},
352+
{"/files/a%20b", "/files/a%20b/", "/files/a%20b"},
353+
{"/files/a%23b", "/files/a%23b/", "/files/a%23b"},
354+
{"/%5Cexample.com", "/%5Cexample.com/", "/%5Cexample.com"},
355+
{"/%2f%2Fexample.com", "/%2f%2Fexample.com/", "/%2f%2Fexample.com"},
356+
} {
357+
for _, remove := range []bool{false, true} {
358+
name := "add"
359+
target, want := tc.path, tc.add
360+
mw := AddTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
361+
if remove {
362+
name = "remove"
363+
target, want = tc.add, tc.remove
364+
// Keep the original escaping when constructing the remove request.
365+
req := httptest.NewRequest(http.MethodGet, tc.path, nil)
366+
target = req.URL.EscapedPath() + "/"
367+
if req.URL.RawQuery != "" {
368+
target += "?" + req.URL.RawQuery
369+
}
370+
mw = RemoveTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
371+
}
372+
t.Run(name+tc.path, func(t *testing.T) {
373+
e := echo.New()
374+
rec := httptest.NewRecorder()
375+
req := httptest.NewRequest(http.MethodGet, target, nil)
376+
assert.NoError(t, mw(func(echo.Context) error { return nil })(e.NewContext(req, rec)))
377+
assert.Equal(t, want, rec.Header().Get(echo.HeaderLocation))
378+
})
379+
}
380+
}
381+
}
382+
383+
func TestRemoveTrailingSlashRedirectAfterEncodedSuffix(t *testing.T) {
384+
for _, target := range []string{"/files/a%2Fb%2f", "/files/a%2Fb%2F"} {
385+
t.Run(target, func(t *testing.T) {
386+
e := echo.New()
387+
rec := httptest.NewRecorder()
388+
req := httptest.NewRequest(http.MethodGet, target+"/?x=1", nil)
389+
mw := RemoveTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
390+
assert.NoError(t, mw(func(echo.Context) error { return nil })(e.NewContext(req, rec)))
391+
assert.Equal(t, target+"?x=1", rec.Header().Get(echo.HeaderLocation))
392+
})
393+
}
394+
}
395+
396+
func TestTrailingSlashRedirectPreservesRoute(t *testing.T) {
397+
for _, remove := range []bool{false, true} {
398+
name, target, route := "add", "/files/a%2Fb", "/files/:id/"
399+
mw := AddTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
400+
if remove {
401+
name, target, route = "remove", "/files/a%2Fb/", "/files/:id"
402+
mw = RemoveTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
403+
}
404+
t.Run(name, func(t *testing.T) {
405+
e := echo.New()
406+
e.Pre(mw)
407+
e.GET(route, func(c echo.Context) error { return c.String(http.StatusOK, c.Param("id")) })
408+
first := httptest.NewRecorder()
409+
e.ServeHTTP(first, httptest.NewRequest(http.MethodGet, target, nil))
410+
if !assert.Equal(t, http.StatusMovedPermanently, first.Code) {
411+
return
412+
}
413+
second := httptest.NewRecorder()
414+
e.ServeHTTP(second, httptest.NewRequest(http.MethodGet, first.Header().Get(echo.HeaderLocation), nil))
415+
assert.Equal(t, http.StatusOK, second.Code)
416+
assert.Equal(t, "a%2Fb", second.Body.String())
417+
})
418+
}
419+
}
420+
421+
func TestTrailingSlashForwardEscapedPath(t *testing.T) {
422+
for _, tc := range []struct{ segment, want string }{
423+
{"a%2Fb", "a%2Fb"},
424+
{"a%20b", "a b"},
425+
{"a%3Fb", "a?b"},
426+
{"a%23b", "a#b"},
427+
{"caf%C3%A9", "café"},
428+
} {
429+
for _, remove := range []bool{false, true} {
430+
name, target, route := "add", "/files/"+tc.segment+"?q=1", "/files/:id/"
431+
mw := AddTrailingSlash()
432+
if remove {
433+
name, target, route = "remove", "/files/"+tc.segment+"/?q=1", "/files/:id"
434+
mw = RemoveTrailingSlash()
435+
}
436+
t.Run(name+"/"+tc.segment, func(t *testing.T) {
437+
req := httptest.NewRequest(http.MethodGet, target, nil)
438+
hadRawPath := req.URL.RawPath != ""
439+
e := echo.New()
440+
e.Pre(mw)
441+
e.GET(route, func(c echo.Context) error {
442+
assert.Equal(t, c.Request().URL.EscapedPath()+"?q=1", c.Request().RequestURI)
443+
assert.Equal(t, hadRawPath, c.Request().URL.RawPath != "")
444+
return c.String(http.StatusOK, c.Param("id"))
445+
})
446+
rec := httptest.NewRecorder()
447+
e.ServeHTTP(rec, req)
448+
assert.Equal(t, http.StatusOK, rec.Code)
449+
assert.Equal(t, tc.want, rec.Body.String())
450+
})
451+
}
452+
}
453+
}
454+
455+
func TestTrailingSlashPreservesEncodedLeadingSeparators(t *testing.T) {
456+
for _, path := range []string{"/%2Fdocs", "/%5Cdocs", "/%2f%2Fdocs"} {
457+
t.Run(path, func(t *testing.T) {
458+
for _, remove := range []bool{false, true} {
459+
target, want := path, path+"/"
460+
mw := AddTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
461+
if remove {
462+
target, want = path+"/", path
463+
mw = RemoveTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: http.StatusMovedPermanently})
464+
}
465+
e := echo.New()
466+
rec := httptest.NewRecorder()
467+
assert.NoError(t, mw(func(echo.Context) error { return nil })(e.NewContext(httptest.NewRequest(http.MethodGet, target, nil), rec)))
468+
assert.Equal(t, want, rec.Header().Get(echo.HeaderLocation))
469+
}
470+
})
471+
}
472+
}
473+
474+
func TestTrailingSlashEncodedFinalSegment(t *testing.T) {
475+
for _, tc := range []struct {
476+
name, target, route, location string
477+
remove bool
478+
}{
479+
{"add", "/files/a%2F", "/files/:id/", "/files/a%2F/", false},
480+
{"remove", "/files/a%2F/", "/files/:id", "/files/a%2F", true},
481+
{"already removed", "/files/a%2F", "/files/:id", "", true},
482+
} {
483+
for _, code := range []int{0, http.StatusMovedPermanently} {
484+
t.Run(tc.name+map[bool]string{true: "/redirect", false: "/forward"}[code != 0], func(t *testing.T) {
485+
e := echo.New()
486+
mw := AddTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: code})
487+
if tc.remove {
488+
mw = RemoveTrailingSlashWithConfig(TrailingSlashConfig{RedirectCode: code})
489+
}
490+
e.Pre(mw)
491+
e.GET(tc.route, func(c echo.Context) error { return c.String(http.StatusOK, c.Param("id")) })
492+
rec := httptest.NewRecorder()
493+
e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, tc.target, nil))
494+
if code != 0 && tc.location != "" {
495+
if !assert.Equal(t, code, rec.Code) {
496+
return
497+
}
498+
assert.Equal(t, tc.location, rec.Header().Get(echo.HeaderLocation))
499+
rec = httptest.NewRecorder()
500+
e.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, tc.location, nil))
501+
}
502+
assert.Equal(t, http.StatusOK, rec.Code)
503+
assert.Equal(t, "a%2F", rec.Body.String())
504+
})
505+
}
506+
}
507+
}

0 commit comments

Comments
 (0)