From 353bd3d06fa093c76d51982b657eb920511e9839 Mon Sep 17 00:00:00 2001 From: robotte Date: Tue, 3 Mar 2020 16:20:05 +0100 Subject: [PATCH] Added support for replacement containing escaped characters Co-authored-by: Ludovic Fernandez --- pkg/middlewares/replacepath/replace_path.go | 19 +++- .../replacepath/replace_path_test.go | 86 +++++++++++++---- .../replacepathregex/replace_path_regex.go | 28 +++++- .../replace_path_regex_test.go | 93 ++++++++++++++----- 4 files changed, 180 insertions(+), 46 deletions(-) diff --git a/pkg/middlewares/replacepath/replace_path.go b/pkg/middlewares/replacepath/replace_path.go index bec16827d..13dd2d98e 100644 --- a/pkg/middlewares/replacepath/replace_path.go +++ b/pkg/middlewares/replacepath/replace_path.go @@ -3,6 +3,7 @@ package replacepath import ( "context" "net/http" + "net/url" "github.com/containous/traefik/v2/pkg/config/dynamic" "github.com/containous/traefik/v2/pkg/log" @@ -40,8 +41,22 @@ func (r *replacePath) GetTracingInformation() (string, ext.SpanKindEnum) { } func (r *replacePath) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - req.Header.Add(ReplacedPathHeader, req.URL.Path) - req.URL.Path = r.path + if req.URL.RawPath == "" { + req.Header.Add(ReplacedPathHeader, req.URL.Path) + } else { + req.Header.Add(ReplacedPathHeader, req.URL.RawPath) + } + + req.URL.RawPath = r.path + + var err error + req.URL.Path, err = url.PathUnescape(req.URL.RawPath) + if err != nil { + log.FromContext(middlewares.GetLoggerCtx(context.Background(), r.name, typeName)).Error(err) + http.Error(rw, err.Error(), http.StatusInternalServerError) + return + } + req.RequestURI = req.URL.RequestURI() r.next.ServeHTTP(rw, req) diff --git a/pkg/middlewares/replacepath/replace_path_test.go b/pkg/middlewares/replacepath/replace_path_test.go index 65824dcde..2042f3586 100644 --- a/pkg/middlewares/replacepath/replace_path_test.go +++ b/pkg/middlewares/replacepath/replace_path_test.go @@ -3,43 +3,93 @@ package replacepath import ( "context" "net/http" + "net/http/httptest" "testing" "github.com/containous/traefik/v2/pkg/config/dynamic" - "github.com/containous/traefik/v2/pkg/testhelpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestReplacePath(t *testing.T) { - var replacementConfig = dynamic.ReplacePath{ - Path: "/replacement-path", + testCases := []struct { + desc string + path string + config dynamic.ReplacePath + expectedPath string + expectedRawPath string + expectedHeader string + }{ + { + desc: "simple path", + path: "/example", + config: dynamic.ReplacePath{ + Path: "/replacement-path", + }, + expectedPath: "/replacement-path", + expectedRawPath: "", + expectedHeader: "/example", + }, + { + desc: "long path", + path: "/some/really/long/path", + config: dynamic.ReplacePath{ + Path: "/replacement-path", + }, + expectedPath: "/replacement-path", + expectedRawPath: "", + expectedHeader: "/some/really/long/path", + }, + { + desc: "path with escaped value", + path: "/foo%2Fbar", + config: dynamic.ReplacePath{ + Path: "/replacement-path", + }, + expectedPath: "/replacement-path", + expectedRawPath: "", + expectedHeader: "/foo%2Fbar", + }, + { + desc: "replacement with escaped value", + path: "/path", + config: dynamic.ReplacePath{ + Path: "/foo%2Fbar", + }, + expectedPath: "/foo/bar", + expectedRawPath: "/foo%2Fbar", + expectedHeader: "/path", + }, } - paths := []string{ - "/example", - "/some/really/long/path", - } - - for _, path := range paths { - t.Run(path, func(t *testing.T) { - var expectedPath, actualHeader, requestURI string + for _, test := range testCases { + t.Run(test.desc, func(t *testing.T) { + var actualPath, actualRawPath, actualHeader, requestURI string next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - expectedPath = r.URL.Path + actualPath = r.URL.Path + actualRawPath = r.URL.RawPath actualHeader = r.Header.Get(ReplacedPathHeader) requestURI = r.RequestURI }) - handler, err := New(context.Background(), next, replacementConfig, "foo-replace-path") + handler, err := New(context.Background(), next, test.config, "foo-replace-path") require.NoError(t, err) - req := testhelpers.MustNewRequest(http.MethodGet, "http://localhost"+path, nil) + server := httptest.NewServer(handler) + defer server.Close() - handler.ServeHTTP(nil, req) + resp, err := http.Get(server.URL + test.path) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) - assert.Equal(t, expectedPath, replacementConfig.Path, "Unexpected path.") - assert.Equal(t, path, actualHeader, "Unexpected '%s' header.", ReplacedPathHeader) - assert.Equal(t, expectedPath, requestURI, "Unexpected request URI.") + assert.Equal(t, test.expectedPath, actualPath, "Unexpected path.") + assert.Equal(t, test.expectedHeader, actualHeader, "Unexpected '%s' header.", ReplacedPathHeader) + + if actualRawPath == "" { + assert.Equal(t, actualPath, requestURI, "Unexpected request URI.") + } else { + assert.Equal(t, actualRawPath, requestURI, "Unexpected request URI.") + } }) } } diff --git a/pkg/middlewares/replacepathregex/replace_path_regex.go b/pkg/middlewares/replacepathregex/replace_path_regex.go index c23820fbd..9aa755455 100644 --- a/pkg/middlewares/replacepathregex/replace_path_regex.go +++ b/pkg/middlewares/replacepathregex/replace_path_regex.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/http" + "net/url" "regexp" "strings" @@ -49,10 +50,31 @@ func (rp *replacePathRegex) GetTracingInformation() (string, ext.SpanKindEnum) { } func (rp *replacePathRegex) ServeHTTP(rw http.ResponseWriter, req *http.Request) { - if rp.regexp != nil && len(rp.replacement) > 0 && rp.regexp.MatchString(req.URL.Path) { - req.Header.Add(replacepath.ReplacedPathHeader, req.URL.Path) - req.URL.Path = rp.regexp.ReplaceAllString(req.URL.Path, rp.replacement) + var currentPath string + if req.URL.RawPath == "" { + currentPath = req.URL.Path + } else { + currentPath = req.URL.RawPath + } + + if rp.regexp != nil && len(rp.replacement) > 0 && rp.regexp.MatchString(currentPath) { + req.Header.Add(replacepath.ReplacedPathHeader, currentPath) + + req.URL.RawPath = rp.regexp.ReplaceAllString(currentPath, rp.replacement) + + // as replacement can introduce escaped characters + // Path must remain an unescaped version of RawPath + // Doesn't handle multiple times encoded replacement (`/` => `%2F` => `%252F` => ...) + var err error + req.URL.Path, err = url.PathUnescape(req.URL.RawPath) + if err != nil { + log.FromContext(middlewares.GetLoggerCtx(context.Background(), rp.name, typeName)).Error(err) + http.Error(rw, err.Error(), http.StatusInternalServerError) + return + } + req.RequestURI = req.URL.RequestURI() } + rp.next.ServeHTTP(rw, req) } diff --git a/pkg/middlewares/replacepathregex/replace_path_regex_test.go b/pkg/middlewares/replacepathregex/replace_path_regex_test.go index 8215df968..594e4eabd 100644 --- a/pkg/middlewares/replacepathregex/replace_path_regex_test.go +++ b/pkg/middlewares/replacepathregex/replace_path_regex_test.go @@ -3,23 +3,24 @@ package replacepathregex import ( "context" "net/http" + "net/http/httptest" "testing" "github.com/containous/traefik/v2/pkg/config/dynamic" "github.com/containous/traefik/v2/pkg/middlewares/replacepath" - "github.com/containous/traefik/v2/pkg/testhelpers" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestReplacePathRegex(t *testing.T) { testCases := []struct { - desc string - path string - config dynamic.ReplacePathRegex - expectedPath string - expectedHeader string - expectsError bool + desc string + path string + config dynamic.ReplacePathRegex + expectedPath string + expectedRawPath string + expectedHeader string + expectsError bool }{ { desc: "simple regex", @@ -28,8 +29,9 @@ func TestReplacePathRegex(t *testing.T) { Replacement: "/who-am-i/$1", Regex: `^/whoami/(.*)`, }, - expectedPath: "/who-am-i/and/whoami", - expectedHeader: "/whoami/and/whoami", + expectedPath: "/who-am-i/and/whoami", + expectedRawPath: "/who-am-i/and/whoami", + expectedHeader: "/whoami/and/whoami", }, { desc: "simple replace (no regex)", @@ -38,8 +40,9 @@ func TestReplacePathRegex(t *testing.T) { Replacement: "/who-am-i", Regex: `/whoami`, }, - expectedPath: "/who-am-i/and/who-am-i", - expectedHeader: "/whoami/and/whoami", + expectedPath: "/who-am-i/and/who-am-i", + expectedRawPath: "/who-am-i/and/who-am-i", + expectedHeader: "/whoami/and/whoami", }, { desc: "no match", @@ -57,8 +60,9 @@ func TestReplacePathRegex(t *testing.T) { Replacement: "/downloads/$1-$2", Regex: `^(?i)/downloads/([^/]+)/([^/]+)$`, }, - expectedPath: "/downloads/src-source.go", - expectedHeader: "/downloads/src/source.go", + expectedPath: "/downloads/src-source.go", + expectedRawPath: "/downloads/src-source.go", + expectedHeader: "/downloads/src/source.go", }, { desc: "invalid regular expression", @@ -70,13 +74,46 @@ func TestReplacePathRegex(t *testing.T) { expectedPath: "/invalid/regexp/test", expectsError: true, }, + { + desc: "replacement with escaped char", + path: "/aaa/bbb", + config: dynamic.ReplacePathRegex{ + Replacement: "/foo%2Fbar", + Regex: `/aaa/bbb`, + }, + expectedPath: "/foo/bar", + expectedRawPath: "/foo%2Fbar", + expectedHeader: "/aaa/bbb", + }, + { + desc: "path and regex with escaped char", + path: "/aaa%2Fbbb", + config: dynamic.ReplacePathRegex{ + Replacement: "/foo/bar", + Regex: `/aaa%2Fbbb`, + }, + expectedPath: "/foo/bar", + expectedRawPath: "/foo/bar", + expectedHeader: "/aaa%2Fbbb", + }, + { + desc: "path with escaped char (no match)", + path: "/aaa%2Fbbb", + config: dynamic.ReplacePathRegex{ + Replacement: "/foo/bar", + Regex: `/aaa/bbb`, + }, + expectedPath: "/aaa/bbb", + expectedRawPath: "/aaa%2Fbbb", + }, } for _, test := range testCases { t.Run(test.desc, func(t *testing.T) { - var actualPath, actualHeader, requestURI string + var actualPath, actualRawPath, actualHeader, requestURI string next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { actualPath = r.URL.Path + actualRawPath = r.URL.RawPath actualHeader = r.Header.Get(replacepath.ReplacedPathHeader) requestURI = r.RequestURI }) @@ -84,19 +121,29 @@ func TestReplacePathRegex(t *testing.T) { handler, err := New(context.Background(), next, test.config, "foo-replace-path-regexp") if test.expectsError { require.Error(t, err) - } else { - require.NoError(t, err) + return + } - req := testhelpers.MustNewRequest(http.MethodGet, "http://localhost"+test.path, nil) - req.RequestURI = test.path + require.NoError(t, err) - handler.ServeHTTP(nil, req) + server := httptest.NewServer(handler) + defer server.Close() - assert.Equal(t, test.expectedPath, actualPath, "Unexpected path.") + resp, err := http.Get(server.URL + test.path) + require.NoError(t, err, "Unexpected error while making test request") + require.Equal(t, http.StatusOK, resp.StatusCode) + + assert.Equal(t, test.expectedPath, actualPath, "Unexpected path.") + assert.Equal(t, test.expectedRawPath, actualRawPath, "Unexpected raw path.") + + if actualRawPath == "" { assert.Equal(t, actualPath, requestURI, "Unexpected request URI.") - if test.expectedHeader != "" { - assert.Equal(t, test.expectedHeader, actualHeader, "Unexpected '%s' header.", replacepath.ReplacedPathHeader) - } + } else { + assert.Equal(t, actualRawPath, requestURI, "Unexpected request URI.") + } + + if test.expectedHeader != "" { + assert.Equal(t, test.expectedHeader, actualHeader, "Unexpected '%s' header.", replacepath.ReplacedPathHeader) } }) }