skip parsing HttpBody field and pass query params as-is (#13679)

Check the request url, if there is any HttpBody, and `method_info->request_body_field_path` is not a prefix of `resolved_binding.field_path`, skip parsing HttpBody field. This avoid we may skip some HttpBody objects which were intended to be configurable by query arguments. Testing: Add a new unit test to cover the changes. Docs Changes: n/a Fixes #13634 Signed-off-by: Curry Xu <[email protected]>
envoyproxy · Dec 3, 2020 · 17ad085 · 17ad085
1 parent 9004d7a
commit 17ad085
Show file tree

Hide file tree

Showing 3 changed files with 77 additions and 3 deletions.
diff --git a/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc b/source/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter.cc
@@ -346,9 +346,16 @@ ProtobufUtil::Status JsonTranscoderConfig::createTranscoder(
       return status;
     }
 
-    resolved_binding.value = binding.value;
-
-    request_info.variable_bindings.emplace_back(std::move(resolved_binding));
+    // HttpBody fields should be passed as-is and not be parsed as JSON.
+    const bool is_http_body = method_info->request_type_is_http_body_;
+    const bool is_inside_http_body =
+        is_http_body && absl::c_equal(absl::MakeSpan(resolved_binding.field_path)
+                                          .subspan(0, method_info->request_body_field_path.size()),
+                                      method_info->request_body_field_path);
+    if (!is_inside_http_body) {
+      resolved_binding.value = binding.value;
+      request_info.variable_bindings.emplace_back(std::move(resolved_binding));
+    }
   }
 
   RequestMessageTranslatorPtr request_translator;

diff --git a/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc b/test/extensions/filters/http/grpc_json_transcoder/json_transcoder_filter_test.cc
@@ -860,6 +860,51 @@ TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithHttpBody) {
   EXPECT_THAT(request, ProtoEq(expected_request));
 }
 
+TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithNestedHttpBody) {
+  const std::string path = "/echoNestedBody?nested2.body.data=aGkh";
+  Http::TestRequestHeaderMapImpl request_headers{
+      {":method", "POST"}, {":path", path}, {"content-type", "text/plain"}};
+  EXPECT_CALL(decoder_callbacks_, clearRouteCache());
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
+  EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
+  EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
+  EXPECT_EQ("POST", request_headers.get_("x-envoy-original-method"));
+  EXPECT_EQ("/bookstore.Bookstore/EchoNestedBody", request_headers.get_(":path"));
+  EXPECT_EQ("trailers", request_headers.get_("te"));
+
+  const std::string path2 = "/echoNestedBody?nested2.body.data=oops%7b";
+  request_headers = {{":method", "POST"}, {":path", path2}, {"content-type", "text/plain"}};
+  EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers, true));
+}
+
+TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryPostWithNestedHttpBodys) {
+  const std::string path = "/echoNestedBody?nested1.body.data=oops%7b";
+  Http::TestRequestHeaderMapImpl request_headers{
+      {":method", "POST"}, {":path", path}, {"content-type", "text/plain"}};
+  EXPECT_CALL(decoder_callbacks_, clearRouteCache());
+
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, false));
+  EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
+  EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
+  EXPECT_EQ("POST", request_headers.get_("x-envoy-original-method"));
+  EXPECT_EQ("/bookstore.Bookstore/EchoNestedBody", request_headers.get_(":path"));
+  EXPECT_EQ("trailers", request_headers.get_("te"));
+}
+
+TEST_F(GrpcJsonTranscoderFilterTest, TranscodingUnaryGetWithHttpBody) {
+  const std::string path = "/echoRawBody?data=oops%7b";
+  Http::TestRequestHeaderMapImpl request_headers{{":method", "GET"}, {":path", path}};
+
+  EXPECT_CALL(decoder_callbacks_, clearRouteCache());
+
+  EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers, true));
+  EXPECT_EQ("application/grpc", request_headers.get_("content-type"));
+  EXPECT_EQ(path, request_headers.get_("x-envoy-original-path"));
+  EXPECT_EQ("GET", request_headers.get_("x-envoy-original-method"));
+  EXPECT_EQ("/bookstore.Bookstore/EchoRawBody", request_headers.get_(":path"));
+  EXPECT_EQ("trailers", request_headers.get_("te"));
+}
+
 TEST_F(GrpcJsonTranscoderFilterTest, TranscodingStreamPostWithHttpBody) {
   Http::TestRequestHeaderMapImpl request_headers{
       {":method", "POST"}, {":path", "/streamBody?arg=hi"}, {"content-type", "text/plain"}};

diff --git a/test/proto/bookstore.proto b/test/proto/bookstore.proto
@@ -114,6 +114,20 @@ service Bookstore {
       body: "nested.content"
     };
   }
+
+  rpc EchoRawBody(google.api.HttpBody) returns (google.api.HttpBody) {
+    option (google.api.http) = {
+      get: "/echoRawBody"
+      additional_bindings: {post: "/echoRawBody"}
+    };
+  }
+
+  rpc EchoNestedBody(EchoNestedRequest) returns (google.protobuf.Empty) {
+    option (google.api.http) = {
+      post: "/echoNestedBody"
+      body: "nested1.body"
+    };
+  }
   rpc EchoResponseBodyPath(google.protobuf.Empty) returns (EchoBodyRequest) {
     option (google.api.http) = {
       get: "/echoResponseBodyPath"
@@ -279,6 +293,14 @@ message EchoBodyRequest {
   Nested nested = 3;
 }
 
+message EchoNestedRequest {
+  message Nested {
+    google.api.HttpBody body = 1;
+  }
+  Nested nested1 = 1;
+  Nested nested2 = 2;
+}
+
 // Request and Response message for EchoStructReqResp method.
 message EchoStructReqResp {
   // The content of request.