Daprd router normalize only seperator (#6430)

* Adds third party URI path normalization, removing segment decoding
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Adds e2e tests to ensure only slashes are normalized. Segments remain
encoded as is.
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Adds copyright headers
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Write back escaped path from echoPath e2e handler
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* linting
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Don't escape path in e2e service invocation app router
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Use correct expected path
Signed-off-by: Njoshvanl <me@joshvanl.dev>

* Remove trailing slashes since we use strict slashes in router
Signed-off-by: Njoshvanl <me@joshvanl.dev>

---------
Signed-off-by: Njoshvanl <me@joshvanl.dev>

tests/apps/service_invocation/app.go

@@ -353,7 +353,7 @@ func constructRequest(id, method, httpVerb string, body []byte) *runtimev1pb.Inv
 
 // appRouter initializes restful api router
 func appRouter() http.Handler {
-	router := mux.NewRouter().StrictSlash(true)
+	router := mux.NewRouter().StrictSlash(true).UseEncodedPath()
 
 	// Log requests and their processing time
 	router.Use(utils.LoggerMiddleware)
@@ -392,6 +392,12 @@ func appRouter() http.Handler {
 	router.HandleFunc("/badservicecalltesthttp", badServiceCallTestHTTP).Methods("POST")
 	router.HandleFunc("/badservicecalltestgrpc", badServiceCallTestGrpc).Methods("POST")
 
+	// called by Dapr invocation to ensure path separators are correctly
+	// normalized, but not path segment contents.
+	router.HandleFunc("/foo/%2E", echoPathHandler).Methods("GET", "POST")
+	router.HandleFunc("/foo/%2Fbbb%2F%2E", echoPathHandler).Methods("GET", "POST")
+	router.HandleFunc("/foo/%2Fb/bb%2F%2E", echoPathHandler).Methods("GET", "POST")
+
 	// service invocation v1 e2e tests
 	router.HandleFunc("/tests/dapr_id_httptohttptest", testDaprIDRequestHTTPToHTTP).Methods("POST")
 	router.HandleFunc("/tests/v1_httptohttptest", testV1RequestHTTPToHTTP).Methods("POST")
@@ -1267,6 +1273,13 @@ func badServiceCallTestHTTP(w http.ResponseWriter, r *http.Request) {
 	json.NewEncoder(w).Encode(testResponse)
 }
 
+// echoPathHandler is a test endpoint that returns the path of the request as
+// is.
+func echoPathHandler(w http.ResponseWriter, r *http.Request) {
+	w.Write([]byte(r.URL.EscapedPath()))
+	w.WriteHeader(http.StatusOK)
+}
+
 func badServiceCallTestGrpc(w http.ResponseWriter, r *http.Request) {
 	fmt.Println("Enter badServiceCallTestGrpc")
 	var commandBody testCommandRequest

tests/e2e/service_invocation/service_invocation_test.go

@@ -1652,3 +1652,40 @@ func TestCrossNamespaceCases(t *testing.T) {
 	t.Run("serviceinvocation-caller", testFn("serviceinvocation-caller"))
 	t.Run("serviceinvocation-caller-stream", testFn("serviceinvocation-caller-stream"))
 }
+
+func TestPathURLNormalization(t *testing.T) {
+	t.Parallel()
+
+	externalURL := tr.Platform.AcquireAppExternalURL("serviceinvocation-caller")
+	require.NotEmpty(t, externalURL, "external URL must not be empty!")
+
+	// This initial probe makes the test wait a little bit longer when needed,
+	// making this test less flaky due to delays in the deployment.
+	_, err := utils.HTTPGetNTimes(externalURL, numHealthChecks)
+	require.NoError(t, err)
+
+	t.Logf("externalURL is '%s'\n", externalURL)
+
+	for path, exp := range map[string]string{
+		`/foo/%2Fbbb%2F%2E`:     `/foo/%2Fbbb%2F%2E`,
+		`//foo/%2Fb/bb%2F%2E`:   `/foo/%2Fb/bb%2F%2E`,
+		`//foo/%2Fb///bb%2F%2E`: `/foo/%2Fb/bb%2F%2E`,
+		`/foo/%2E`:              `/foo/%2E`,
+		`///foo///%2E`:          `/foo/%2E`,
+	} {
+		t.Run(path, func(t *testing.T) {
+			body, err := json.Marshal(testCommandRequest{
+				RemoteApp: "serviceinvocation-callee-0",
+				Method:    "normalization",
+			})
+			require.NoError(t, err)
+
+			url := fmt.Sprintf("http://%s/%s", externalURL, path)
+			resp, err := utils.HTTPPost(url, body)
+			require.NoError(t, err)
+
+			t.Logf("checking piped path..%s\n", string(resp))
+			assert.Contains(t, exp, string(resp))
+		})
+	}
+}

utils/nethttpadaptor/nethttpadaptor.go

@@ -23,6 +23,7 @@ import (
 	"github.com/valyala/fasthttp"
 
 	diagUtils "github.com/dapr/dapr/pkg/diagnostics/utils"
+	"github.com/dapr/dapr/utils/nethttpadaptor/uri"
 	"github.com/dapr/kit/logger"
 )
 
@@ -49,8 +50,13 @@ func NewNetHTTPHandlerFunc(h fasthttp.RequestHandler) http.HandlerFunc {
 			}
 			c.Request.SetBody(reqBody)
 		}
+
+		// Normalize path ourselves so that we can preserve the encoded path
+		// segments, but clean up URI separators.
+		// Use EscapePath() to preserve the encoded path segments.
+		c.Request.URI().DisablePathNormalizing = true
 		c.Request.URI().SetQueryString(r.URL.RawQuery)
-		c.Request.URI().SetPath(r.URL.Path)
+		c.Request.URI().SetPathBytes(uri.NormalizePath(c.Request.URI().PathOriginal(), []byte(r.URL.EscapedPath())))
 		c.Request.URI().SetScheme(r.URL.Scheme)
 		c.Request.SetHost(r.Host)
 		c.Request.Header.SetMethod(r.Method)

utils/nethttpadaptor/nethttpadaptor_test.go

@@ -347,16 +347,16 @@ func TestNewNetHTTPHandlerFuncRequests(t *testing.T) {
 		{
 			"http request path is normalized",
 			func() *http.Request {
-				req, _ := http.NewRequestWithContext(context.Background(), http.MethodGet, "https://localhost:8080//foo//bar/123/?hello=world", nil)
-				return req
+				return httptest.NewRequest(http.MethodGet, "https://localhost:8080///foo/bar/123aaa///%2Fbbb%2F%2E.%2Fxxx//?hell%2F%2Eo=wor%2F%2Eld", nil)
 			},
 			func(t *testing.T) func(ctx *fasthttp.RequestCtx) {
 				return func(ctx *fasthttp.RequestCtx) {
-					assert.Equal(t, "/foo/bar/123/", string(ctx.Request.URI().Path()))
+					assert.True(t, ctx.Request.URI().DisablePathNormalizing)
+					assert.Equal(t, "/foo/bar/123aaa/%2Fbbb%2F%2E.%2Fxxx/", string(ctx.Request.URI().PathOriginal()))
 					assert.Equal(t, "localhost:8080", string(ctx.Request.URI().Host()))
 					assert.Equal(t, "https", string(ctx.Request.URI().Scheme()))
-					assert.Equal(t, "hello=world", string(ctx.Request.URI().QueryString()))
-					assert.Equal(t, "https://localhost:8080/foo/bar/123/?hello=world", string(ctx.Request.URI().FullURI()))
+					assert.Equal(t, "hell%2F%2Eo=wor%2F%2Eld", string(ctx.Request.URI().QueryString()))
+					assert.Equal(t, "https://localhost:8080/foo/bar/123aaa/%2Fbbb%2F%2E.%2Fxxx/?hell%2F%2Eo=wor%2F%2Eld", string(ctx.Request.URI().FullURI()))
 				}
 			},
 		},

utils/nethttpadaptor/uri/strings.go

+/*
+Copyright 2022 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package uri
+
+var (
+	strSlash                    = []byte("/")
+	strSlashSlash               = []byte("//")
+	strSlashDotDot              = []byte("/..")
+	strSlashDotSlash            = []byte("/./")
+	strSlashDotDotSlash         = []byte("/../")
+	strBackSlashDotDot          = []byte(`\..`)
+	strBackSlashDotBackSlash    = []byte(`\.\`)
+	strSlashDotDotBackSlash     = []byte(`/..\`)
+	strBackSlashDotDotBackSlash = []byte(`\..\`)
+)

utils/nethttpadaptor/uri/uri.go

+/*
+Copyright 2022 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package uri
+
+import (
+	"bytes"
+	"path/filepath"
+)
+
+// NormalizePath is a function copied from
+// https://github.com/valyala/fasthttp/blob/f0865d4aabbbea51a81d56ab31a3de2dfc5a9b05/uri.go#L575
+// but with Path segment normalization removed. This is because the Dapr should
+// leave encoded path segments _as is_, and not attempt to normalize them for
+// forwarding them on to apps.
+func NormalizePath(dst, src []byte) []byte {
+	dst = dst[:0]
+	dst = addLeadingSlash(dst, src)
+	dst = append(dst, src...)
+
+	// remove duplicate slashes
+	b := dst
+	bSize := len(b)
+	for {
+		n := bytes.Index(b, strSlashSlash)
+		if n < 0 {
+			break
+		}
+		b = b[n:]
+		copy(b, b[1:])
+		b = b[:len(b)-1]
+		bSize--
+	}
+	dst = dst[:bSize]
+
+	// remove /./ parts
+	b = dst
+	for {
+		n := bytes.Index(b, strSlashDotSlash)
+		if n < 0 {
+			break
+		}
+		nn := n + len(strSlashDotSlash) - 1
+		copy(b[n:], b[nn:])
+		b = b[:len(b)-nn+n]
+	}
+
+	// remove /foo/../ parts
+	for {
+		n := bytes.Index(b, strSlashDotDotSlash)
+		if n < 0 {
+			break
+		}
+		nn := bytes.LastIndexByte(b[:n], '/')
+		if nn < 0 {
+			nn = 0
+		}
+		n += len(strSlashDotDotSlash) - 1
+		copy(b[nn:], b[n:])
+		b = b[:len(b)-n+nn]
+	}
+
+	// remove trailing /foo/..
+	n := bytes.LastIndex(b, strSlashDotDot)
+	if n >= 0 && n+len(strSlashDotDot) == len(b) {
+		nn := bytes.LastIndexByte(b[:n], '/')
+		if nn < 0 {
+			return append(dst[:0], strSlash...)
+		}
+		b = b[:nn+1]
+	}
+
+	if filepath.Separator == '\\' {
+		// remove \.\ parts
+		b = dst
+		for {
+			n := bytes.Index(b, strBackSlashDotBackSlash)
+			if n < 0 {
+				break
+			}
+			nn := n + len(strSlashDotSlash) - 1
+			copy(b[n:], b[nn:])
+			b = b[:len(b)-nn+n]
+		}
+
+		// remove /foo/..\ parts
+		for {
+			n := bytes.Index(b, strSlashDotDotBackSlash)
+			if n < 0 {
+				break
+			}
+			nn := bytes.LastIndexByte(b[:n], '/')
+			if nn < 0 {
+				nn = 0
+			}
+			n += len(strSlashDotDotBackSlash) - 1
+			copy(b[nn:], b[n:])
+			b = b[:len(b)-n+nn]
+		}
+
+		// remove /foo\..\ parts
+		for {
+			n := bytes.Index(b, strBackSlashDotDotBackSlash)
+			if n < 0 {
+				break
+			}
+			nn := bytes.LastIndexByte(b[:n], '/')
+			if nn < 0 {
+				nn = 0
+			}
+			n += len(strBackSlashDotDotBackSlash) - 1
+			copy(b[nn:], b[n:])
+			b = b[:len(b)-n+nn]
+		}
+
+		// remove trailing \foo\..
+		n := bytes.LastIndex(b, strBackSlashDotDot)
+		if n >= 0 && n+len(strSlashDotDot) == len(b) {
+			nn := bytes.LastIndexByte(b[:n], '/')
+			if nn < 0 {
+				return append(dst[:0], strSlash...)
+			}
+			b = b[:nn+1]
+		}
+	}
+
+	return b
+}

utils/nethttpadaptor/uri/uri_test.go

+/*
+Copyright 2022 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package uri
+
+import (
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func Test_NormalizePath(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.SkipNow()
+	}
+
+	t.Parallel()
+
+	tests := map[string]string{
+		"/aa//bb":                     "/aa/bb",
+		"/x///y/":                     "/x/y/",
+		"/abc//de///fg////":           "/abc/de/fg/",
+		"/xxxx%2fy//yy%2f%2/F%2F///":  "/xxxx%2fy/yy%2f%2/F%2F/",
+		"/aaa/..":                     "/",
+		"/xxx/yyy/../":                "/xxx/",
+		"/aaa/bbb/ccc/../../ddd":      "/aaa/ddd",
+		"/a/b/../c/d/../e/..":         "/a/c/",
+		"/aaa/../../../../xxx":        "/xxx",
+		"/../../../../../..":          "/",
+		"/../../../../../../":         "/",
+		"/////aaa%2Fbbb%2F%2E.%2Fxxx": "/aaa%2Fbbb%2F%2E.%2Fxxx",
+		"/aaa////..//b":               "/b",
+		"/aaa/..bbb/ccc/..":           "/aaa/..bbb/",
+		"/a/./b/././c/./d.html":       "/a/b/c/d.html",
+		"./foo/":                      "/foo/",
+		"./../.././../../aaa/bbb/../../../././../": "/",
+		"./a/./.././../b/./foo.html":               "/b/foo.html",
+	}
+
+	for input, expout := range tests {
+		t.Run(input, func(t *testing.T) {
+			var output []byte
+			output = NormalizePath(output, []byte(input))
+
+			assert.Equal(t, expout, string(output), "input: "+input)
+		})
+	}
+}

utils/nethttpadaptor/uri/uri_unix.go

+//go:build !windows
+// +build !windows
+
+/*
+Copyright 2022 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package uri
+
+func addLeadingSlash(dst, src []byte) []byte {
+	// add leading slash for unix paths
+	if len(src) == 0 || src[0] != '/' {
+		dst = append(dst, '/')
+	}
+
+	return dst
+}

utils/nethttpadaptor/uri/uri_windows.go

+//go:build windows
+// +build windows
+
+/*
+Copyright 2022 The Dapr Authors
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+    http://www.apache.org/licenses/LICENSE-2.0
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package uri
+
+func addLeadingSlash(dst, src []byte) []byte {
+	// zero length and "C:/" case
+	if len(src) == 0 || (len(src) > 2 && src[1] != ':') {
+		dst = append(dst, '/')
+	}
+
+	return dst
+}