Change the logic to build path with query params

- Use escaped path + raw query string to build the path set in envoy request
- Add test cases to cover few additional special cases (empty query string, space in path)

filters/openpolicyagent/internal/envoy/skipperadapter.go

@@ -3,6 +3,7 @@ package envoy
 import (
 	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 	"unicode/utf8"
 
@@ -11,9 +12,8 @@ import (
 )
 
 func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metadata, contextExtensions map[string]string, rawBody []byte) (*ext_authz_v3.CheckRequest, error) {
-
-	if !utf8.ValidString(req.URL.Path) {
-		return nil, fmt.Errorf("invalid utf8 in path: %q", req.URL.Path)
+	if err := validateURLForInvalidUTF8(req.URL); err != nil {
+		return nil, fmt.Errorf("invalid url: %w", err)
 	}
 
 	headers := make(map[string]string, len(req.Header))
@@ -29,7 +29,7 @@ func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metada
 				Http: &ext_authz_v3.AttributeContext_HttpRequest{
 					Host:    req.Host,
 					Method:  req.Method,
-					Path:    req.URL.RequestURI(),
+					Path:    buildPathWithQueryParams(req),
 					Headers: headers,
 					RawBody: rawBody,
 				},
@@ -41,3 +41,27 @@ func AdaptToExtAuthRequest(req *http.Request, metadata *ext_authz_v3_core.Metada
 
 	return ereq, nil
 }
+
+func buildPathWithQueryParams(req *http.Request) string {
+	if req.URL.RawQuery == "" {
+		return url.PathEscape(req.URL.Path)
+	}
+	return url.PathEscape(req.URL.Path) + "?" + req.URL.RawQuery
+}
+
+func validateURLForInvalidUTF8(u *url.URL) error {
+	if !utf8.ValidString(u.Path) {
+		return fmt.Errorf("invalid utf8 in path: %q", u.Path)
+	}
+
+	decodedQuery, err := url.QueryUnescape(u.RawQuery)
+	if err != nil {
+		return fmt.Errorf("error decoding query string: %w", err)
+	}
+
+	if !utf8.ValidString(decodedQuery) {
+		return fmt.Errorf("invalid utf8 in query: %q", u.RawQuery)
+	}
+
+	return nil
+}

filters/openpolicyagent/opaauthorizerequest/opaauthorizerequest_test.go

@@ -2,6 +2,7 @@ package opaauthorizerequest
 
 import (
 	"fmt"
+	"github.com/zalando/skipper/filters/builtin"
 	"io"
 	"log"
 	"net/http"
@@ -29,7 +30,8 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 	for _, ti := range []struct {
 		msg               string
 		filterName        string
-		extraeskip        string
+		extraeskipBefore  string
+		extraeskipAfter   string
 		bundleName        string
 		regoQuery         string
 		requestPath       string
@@ -58,11 +60,26 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			removeHeaders:     make(http.Header),
 		},
 		{
-			msg:               "Allow Requests with trailing ? in URL",
+			msg:               "Allow Requests with spaces in path",
 			filterName:        "opaAuthorizeRequest",
 			bundleName:        "somebundle.tar.gz",
 			regoQuery:         "envoy/authz/allow",
-			requestPath:       "/allow?",
+			requestPath:       "/my%20path",
+			requestMethod:     "GET",
+			contextExtensions: "",
+			expectedStatus:    http.StatusOK,
+			expectedBody:      "Welcome!",
+			expectedHeaders:   make(http.Header),
+			backendHeaders:    make(http.Header),
+			removeHeaders:     make(http.Header),
+		},
+		{
+			msg:               "Allow Requests with request path overridden by the setPath filter",
+			filterName:        "opaAuthorizeRequest",
+			extraeskipBefore:  `setPath("/allow") ->`,
+			bundleName:        "somebundle.tar.gz",
+			regoQuery:         "envoy/authz/allow",
+			requestPath:       "/some-random-path-that-would-fail",
 			requestMethod:     "GET",
 			contextExtensions: "",
 			expectedStatus:    http.StatusOK,
@@ -99,6 +116,20 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			backendHeaders:    make(http.Header),
 			removeHeaders:     make(http.Header),
 		},
+		{
+			msg:               "Allow Requests with an empty query string",
+			filterName:        "opaAuthorizeRequest",
+			bundleName:        "somebundle.tar.gz",
+			regoQuery:         "envoy/authz/allow_context_extensions",
+			requestPath:       "/path-with-empty-query?",
+			requestMethod:     "GET",
+			contextExtensions: "com.mycompany.myprop: myvalue",
+			expectedStatus:    http.StatusOK,
+			expectedBody:      "Welcome!",
+			expectedHeaders:   make(http.Header),
+			backendHeaders:    make(http.Header),
+			removeHeaders:     make(http.Header),
+		},
 		{
 			msg:             "Allow Matching Environment",
 			filterName:      "opaAuthorizeRequest",
@@ -283,7 +314,7 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 		{
 			msg:             "Chained OPA filter with body",
 			filterName:      "opaAuthorizeRequestWithBody",
-			extraeskip:      `-> opaAuthorizeRequestWithBody("somebundle.tar.gz")`,
+			extraeskipAfter: `-> opaAuthorizeRequestWithBody("somebundle.tar.gz")`,
 			bundleName:      "somebundle.tar.gz",
 			regoQuery:       "envoy/authz/allow_body",
 			requestMethod:   "POST",
@@ -325,6 +356,20 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			backendHeaders:    make(http.Header),
 			removeHeaders:     make(http.Header),
 		},
+		{
+			msg:               "Invalid UTF-8 in Query",
+			filterName:        "opaAuthorizeRequest",
+			bundleName:        "somebundle.tar.gz",
+			regoQuery:         "envoy/authz/allow",
+			requestPath:       "/allow?%c0%ae=%c0%ae%c0%ae",
+			requestMethod:     "GET",
+			contextExtensions: "",
+			expectedStatus:    http.StatusBadRequest,
+			expectedBody:      "",
+			expectedHeaders:   make(http.Header),
+			backendHeaders:    make(http.Header),
+			removeHeaders:     make(http.Header),
+		},
 	} {
 		t.Run(ti.msg, func(t *testing.T) {
 			t.Logf("Running test for %v", ti)
@@ -350,6 +395,16 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 
 						allow {
 							input.parsed_path = [ "allow" ]
+							input.parsed_query = {}
+						}
+
+						allow {
+							input.parsed_path = [ "my path" ]
+						}
+
+						allow {
+							input.parsed_path = [ "/path-with-empty-query" ]
+							input.parsed_query = {}
 						}
 
 						allow {
@@ -467,8 +522,9 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			fr.Register(ftSpec)
 			ftSpec = NewOpaAuthorizeRequestWithBodySpec(opaFactory, opts...)
 			fr.Register(ftSpec)
+			fr.Register(builtin.NewSetPath())
 
-			r := eskip.MustParse(fmt.Sprintf(`* -> %s("%s", "%s") %s -> "%s"`, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskip, clientServer.URL))
+			r := eskip.MustParse(fmt.Sprintf(`* -> %s %s("%s", "%s") %s -> "%s"`, ti.extraeskipBefore, ti.filterName, ti.bundleName, ti.contextExtensions, ti.extraeskipAfter, clientServer.URL))
 
 			proxy := proxytest.New(fr, r...)
 

filters/openpolicyagent/opaserveresponse/opaserveresponse_test.go

@@ -17,7 +17,7 @@ import (
 	"github.com/zalando/skipper/filters/openpolicyagent"
 )
 
-func TestAuthorizeRequestFilter(t *testing.T) {
+func TestServerResponseFilter(t *testing.T) {
 	for _, ti := range []struct {
 		msg               string
 		filterName        string
@@ -70,6 +70,16 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			expectedBody:    "Welcome from policy!",
 			expectedHeaders: map[string][]string{"X-Ext-Auth-Allow": {"yes"}},
 		},
+		{
+			msg:             "Allow With Structured Rules and empty query string in path",
+			filterName:      "opaServeResponse",
+			bundleName:      "somebundle.tar.gz",
+			regoQuery:       "envoy/authz/allow_object",
+			requestPath:     "/allow/structured/with-empty-query-string?",
+			expectedStatus:  http.StatusOK,
+			expectedBody:    "Welcome from policy!",
+			expectedHeaders: map[string][]string{"X-Ext-Auth-Allow": {"yes"}},
+		},
 		{
 			msg:             "Allow With Structured Rules and Query Params",
 			filterName:      "opaServeResponse",
@@ -143,6 +153,16 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 			expectedBody:    "",
 			expectedHeaders: make(http.Header),
 		},
+		{
+			msg:             "Invalid UTF-8 in Query String",
+			filterName:      "opaServeResponse",
+			bundleName:      "somebundle.tar.gz",
+			regoQuery:       "envoy/authz/allow",
+			requestPath:     "/allow?%c0%ae=%c0%ae%c0%ae",
+			expectedStatus:  http.StatusBadRequest,
+			expectedBody:    "",
+			expectedHeaders: make(http.Header),
+		},
 	} {
 		t.Run(ti.msg, func(t *testing.T) {
 			t.Logf("Running test for %v", ti)
@@ -175,6 +195,17 @@ func TestAuthorizeRequestFilter(t *testing.T) {
 							}
 						}
 
+						allow_object = response {
+							input.parsed_path = [ "allow", "structured", "with-empty-query-string" ]
+							input.parsed_query == {}
+							response := {
+								"allowed": true,
+								"headers": {"x-ext-auth-allow": "yes"},
+								"body": "Welcome from policy!",
+								"http_status": 200
+							}
+						}
+
 						allow_object = response {
 							input.parsed_path = [ "allow", "structured", "with-query" ]
 							input.parsed_query.pass == ["yes"]