From 250a4efe03523b60f803c08d10ae2a464f183ba9 Mon Sep 17 00:00:00 2001 From: Matheus Meneses Date: Wed, 13 Apr 2022 20:12:48 -0300 Subject: [PATCH 1/2] prevent double enconding in path parameters --- .gitignore | 1 + gin.go | 4 ++-- tree.go | 36 +++++++++++++++++++++++++++++++----- tree_test.go | 23 ++++++++++++----------- 4 files changed, 46 insertions(+), 18 deletions(-) diff --git a/.gitignore b/.gitignore index bdd50c95..4bbf1836 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ count.out test profile.out tmp.out +*.iml diff --git a/gin.go b/gin.go index c2e95f29..0c6ebcf3 100644 --- a/gin.go +++ b/gin.go @@ -603,7 +603,7 @@ func (engine *Engine) handleHTTPRequest(c *Context) { } root := t[i].root // Find route in tree - value := root.getValue(rPath, c.params, c.skippedNodes, unescape) + value := root.getValue(rPath, c.params, c.skippedNodes, unescape, engine.RedirectFixedPath) if value.params != nil { c.Params = *value.params } @@ -631,7 +631,7 @@ func (engine *Engine) handleHTTPRequest(c *Context) { if tree.method == httpMethod { continue } - if value := tree.root.getValue(rPath, nil, c.skippedNodes, unescape); value.handlers != nil { + if value := tree.root.getValue(rPath, nil, c.skippedNodes, unescape, engine.RedirectFixedPath); value.handlers != nil { c.handlers = engine.allNoMethod serveError(c, http.StatusMethodNotAllowed, default405Body) return diff --git a/tree.go b/tree.go index 88100eec..5014c3b3 100644 --- a/tree.go +++ b/tree.go @@ -415,7 +415,7 @@ type skippedNode struct { // If no handle can be found, a TSR (trailing slash redirect) recommendation is // made if a handle exists with an extra (without the) trailing slash for the // given path. -func (n *node) getValue(path string, params *Params, skippedNodes *[]skippedNode, unescape bool) (value nodeValue) { +func (n *node) getValue(path string, params *Params, skippedNodes *[]skippedNode, unescape, redirectFixedPath bool) (value nodeValue) { var globalParamsCount int16 walk: // Outer loop for walking the tree @@ -504,8 +504,21 @@ walk: // Outer loop for walking the tree *value.params = (*value.params)[:i+1] val := path[:end] if unescape { - if v, err := url.QueryUnescape(val); err == nil { - val = v + v := val + for { + beforeDecoding := v + if decoded, err := url.QueryUnescape(v); err == nil { + v = decoded + } else { + break + } + if beforeDecoding == v { + val = v + break + } + } + if redirectFixedPath { + val = cleanPath(val) } } (*value.params)[i] = Param{ @@ -550,8 +563,21 @@ walk: // Outer loop for walking the tree *value.params = (*value.params)[:i+1] val := path if unescape { - if v, err := url.QueryUnescape(path); err == nil { - val = v + v := val + for { + beforeDecoding := v + if decoded, err := url.QueryUnescape(v); err == nil { + v = decoded + } else { + break + } + if beforeDecoding == v { + val = v + break + } + } + if redirectFixedPath { + val = cleanPath(val) } } (*value.params)[i] = Param{ diff --git a/tree_test.go b/tree_test.go index 085b5803..e21537e4 100644 --- a/tree_test.go +++ b/tree_test.go @@ -38,14 +38,15 @@ func getSkippedNodes() *[]skippedNode { return &ps } -func checkRequests(t *testing.T, tree *node, requests testRequests, unescapes ...bool) { +func checkRequests(t *testing.T, tree *node, requests testRequests, redirectFixedPath bool, unescapes ...bool) { unescape := false + if len(unescapes) >= 1 { unescape = unescapes[0] } for _, request := range requests { - value := tree.getValue(request.path, getParams(), getSkippedNodes(), unescape) + value := tree.getValue(request.path, getParams(), getSkippedNodes(), unescape, redirectFixedPath) if value.handlers == nil { if !request.nilHandler { @@ -130,7 +131,7 @@ func TestTreeAddAndGet(t *testing.T) { {"/ab", false, "/ab", nil}, {"/α", false, "/α", nil}, {"/β", false, "/β", nil}, - }) + }, false) checkPriorities(t, tree) } @@ -315,7 +316,7 @@ func TestTreeWildcard(t *testing.T) { {"/get/abc/123abg/test", false, "/get/abc/123abg/:param", Params{Param{Key: "param", Value: "test"}}}, {"/get/abc/123abf/testss", false, "/get/abc/123abf/:param", Params{Param{Key: "param", Value: "testss"}}}, {"/get/abc/123abfff/te", false, "/get/abc/123abfff/:param", Params{Param{Key: "param", Value: "te"}}}, - }) + }, false) checkPriorities(t, tree) } @@ -352,7 +353,7 @@ func TestUnescapeParameters(t *testing.T) { {"/info/slash%2Fgordon/project/Project%20%231", false, "/info/:user/project/:project", Params{Param{Key: "user", Value: "slash/gordon"}, Param{Key: "project", Value: "Project #1"}}}, {"/info/slash%%%%", false, "/info/:user", Params{Param{Key: "user", Value: "slash%%%%"}}}, {"/info/slash%%%%2Fgordon/project/Project%%%%20%231", false, "/info/:user/project/:project", Params{Param{Key: "user", Value: "slash%%%%2Fgordon"}, Param{Key: "project", Value: "Project%%%%20%231"}}}, - }, unescape) + }, false, unescape) checkPriorities(t, tree) } @@ -482,7 +483,7 @@ func TestTreeDuplicatePath(t *testing.T) { {"/src/some/file.png", false, "/src/*filepath", Params{Param{"filepath", "/some/file.png"}}}, {"/search/someth!ng+in+ünìcodé", false, "/search/:query", Params{Param{"query", "someth!ng+in+ünìcodé"}}}, {"/user_gopher", false, "/user_:name", Params{Param{"name", "gopher"}}}, - }) + }, false) } func TestEmptyWildcardName(t *testing.T) { @@ -636,7 +637,7 @@ func TestTreeTrailingSlashRedirect(t *testing.T) { } for _, route := range tsrRoutes { - value := tree.getValue(route, nil, getSkippedNodes(), false) + value := tree.getValue(route, nil, getSkippedNodes(), false, false) if value.handlers != nil { t.Fatalf("non-nil handler for TSR route '%s", route) } else if !value.tsr { @@ -657,7 +658,7 @@ func TestTreeTrailingSlashRedirect(t *testing.T) { "/foo/p/p", } for _, route := range noTsrRoutes { - value := tree.getValue(route, nil, getSkippedNodes(), false) + value := tree.getValue(route, nil, getSkippedNodes(), false, false) if value.handlers != nil { t.Fatalf("non-nil handler for No-TSR route '%s", route) } else if value.tsr { @@ -676,7 +677,7 @@ func TestTreeRootTrailingSlashRedirect(t *testing.T) { t.Fatalf("panic inserting test route: %v", recv) } - value := tree.getValue("/", nil, getSkippedNodes(), false) + value := tree.getValue("/", nil, getSkippedNodes(), false, false) if value.handlers != nil { t.Fatalf("non-nil handler") } else if value.tsr { @@ -856,7 +857,7 @@ func TestTreeInvalidNodeType(t *testing.T) { // normal lookup recv := catchPanic(func() { - tree.getValue("/test", nil, getSkippedNodes(), false) + tree.getValue("/test", nil, getSkippedNodes(), false, false) }) if rs, ok := recv.(string); !ok || rs != panicMsg { t.Fatalf("Expected panic '"+panicMsg+"', got '%v'", recv) @@ -881,7 +882,7 @@ func TestTreeInvalidParamsType(t *testing.T) { params := make(Params, 0) // try to trigger slice bounds out of range with capacity 0 - tree.getValue("/test", ¶ms, getSkippedNodes(), false) + tree.getValue("/test", ¶ms, getSkippedNodes(), false, false) } func TestTreeWildcardConflictEx(t *testing.T) { From 3f436fce7eaaf109defa67c8646e69ba971015a1 Mon Sep 17 00:00:00 2001 From: Matheus Meneses Date: Tue, 19 Apr 2022 09:57:06 -0300 Subject: [PATCH 2/2] improve code --- tree.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/tree.go b/tree.go index 5014c3b3..3150454e 100644 --- a/tree.go +++ b/tree.go @@ -507,12 +507,11 @@ walk: // Outer loop for walking the tree v := val for { beforeDecoding := v - if decoded, err := url.QueryUnescape(v); err == nil { + decoded, err := url.QueryUnescape(v) + if err == nil { v = decoded - } else { - break } - if beforeDecoding == v { + if beforeDecoding == v || err != nil { val = v break }