Lambda json parsing 1.28 (#336)

* Fix: Lambda json parsing (#328)

* messy early commit: reduce test deps, annotate api gateway transformer, tests to describe failure modes

* safety adjustments to api_gateway_transformer.cc

* clean up some comments in api_gateway_transformer.cc

* messy update to transformer_teest.cc

* remove duplicate log line

* clean up unnecessary tests, todos

* reintroduce transform_single_and_multi_value_headers test

* remove other unintentionally removed tests

* additional test cleanup

* fix whitespace changes + address remaining comments in tests

* add changelog entry

* move changelog

---------

Co-authored-by: Ben Taussig <85883594+ben-taussig-solo@users.noreply.github.com>

changelog/v1.28.2-patch2/lambda-json-parsing-fix.yaml

@@ -0,0 +1,9 @@
+changelog:
+- type: FIX
+  description: >
+    Fixes an issue where the Lambda filter would choke on unexpected JSON input
+    when unwrapAsApiGateway was enabled. Specifically, the filter would fail to
+    parse non-string header values under the multiValueHeaders key in the Lambda
+    response.
+  issueLink: https://github.com/solo-io/gloo/issues/8867 
+  resolvesIssue: false

source/extensions/transformers/aws_lambda/api_gateway_transformer.cc

@@ -95,13 +95,14 @@ void ApiGatewayTransformer::transform_response(
   // set response status code
   if (json_body.contains("statusCode")) {
     uint64_t status_value;
-    try {
-      status_value = json_body["statusCode"].get<uint64_t>();
-    } catch (std::exception& exception){
-      ENVOY_STREAM_LOG(debug, "Error parsing statusCode: " + std::string(exception.what()), stream_filter_callbacks);
-      ApiGatewayError error = {500, "500", "Non-integer status code"};
+    if (!json_body["statusCode"].is_number_unsigned()) {
+      // add duplicate log line to not break tests for now
+      ENVOY_STREAM_LOG(debug, "cannot parse non unsigned integer status code", stream_filter_callbacks);
+      ENVOY_STREAM_LOG(debug, "received status code with value: " + json_body["statusCode"].dump(), stream_filter_callbacks);
+      ApiGatewayError error = {500, "500", "cannot parse non unsigned integer status code"};
       return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
     }
+    status_value = json_body["statusCode"].get<uint64_t>();
     response_headers->setStatus(status_value);
   } else {
     response_headers->setStatus(DEFAULT_STATUS_VALUE);
@@ -132,17 +133,33 @@ void ApiGatewayTransformer::transform_response(
   if (json_body.contains("multiValueHeaders")) {
     const auto& multi_value_headers = json_body["multiValueHeaders"];
     if (!multi_value_headers.is_object()) {
-        ENVOY_STREAM_LOG(debug, "invalid multi headers object", stream_filter_callbacks);
-        ApiGatewayError error = {500, "500", "invalid multi headers object"};
+        ENVOY_STREAM_LOG(debug, "invalid multiValueHeaders object", stream_filter_callbacks);
+        ApiGatewayError error = {500, "500", "invalid multiValueHeaders object"}; 
         return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
     }
 
     for (json::const_iterator it = multi_value_headers.cbegin(); it != multi_value_headers.cend(); it++) {
         const auto& header_key = it.key();
         const auto& header_values = it.value();
 
+        // need to validate that header_values is an array/iterable
+        if (!header_values.is_array()) {
+          // if it's an object, we reject the request
+          if (header_values.is_object()) {
+            ENVOY_STREAM_LOG(debug, "invalid multi header value object", stream_filter_callbacks);
+            ApiGatewayError error = {500, "500", "invalid multi header value object"};
+            return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
+          }
+
+          ENVOY_STREAM_LOG(debug, "warning: using non-array value for multi header value", stream_filter_callbacks);
+        }
         for (json::const_iterator inner_it = header_values.cbegin(); inner_it != header_values.cend(); inner_it++) {
-          const auto& header_value = inner_it.value().get<std::string>();
+          std::string header_value;
+          if (inner_it.value().is_string()) {
+            header_value = inner_it.value().get<std::string>();
+          } else {
+            header_value = inner_it.value().dump();
+          }
           add_response_header(*response_headers, header_key, header_value, stream_filter_callbacks, true);
         }
     }

test/extensions/transformers/aws_lambda/BUILD

@@ -19,7 +19,5 @@ envoy_gloo_cc_test(
         "//api/envoy/config/transformer/aws_lambda/v2:pkg_cc_proto",
         "//source/extensions/transformers/aws_lambda:api_gateway_transformer_lib",
         "@envoy//test/mocks/http:http_mocks",
-        "@envoy//test/mocks/server:server_mocks",
-        "@envoy//test/test_common:utility_lib",
     ],
 )

test/extensions/transformers/aws_lambda/transformer_test.cc

@@ -3,13 +3,8 @@
 #include "source/extensions/transformers/aws_lambda/api_gateway_transformer.h"
 
 #include "test/mocks/http/mocks.h"
-#include "test/mocks/server/mocks.h"
-#include "test/test_common/utility.h"
 
-#include "fmt/format.h"
 #include "nlohmann/json.hpp"
-#include "gmock/gmock.h"
-#include "gtest/gtest.h"
 
 using testing::_;
 using testing::AtLeast;
@@ -305,7 +300,296 @@ TEST(ApiGatewayTransformer, error) {
   EXPECT_EQ(response_headers.get(Http::LowerCaseString("x-amzn-errortype"))[0]->value().getStringView(), "500");
 }
 
+// helper used in multi value headers type safety tests
+// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
+// - expected_error_message: if present, expect that an error message will be logged that contains this string
+// - expected_multi_value_header: if present, expect that the first value of the 'test-multi-header' header in the response will be equal to this string,
+void test_multi_value_headers_transformation(
+    std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
+    std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be string, but is null",
+    std::string expected_multi_value_header = "multi-value-0,multi-value-1,multi-value-2") {
+  Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
+                                         {":authority", "www.solo.io"},
+                                         {"x-test", "789"},
+                                         {":path", "/users/123"}};
+  Http::TestResponseHeaderMapImpl response_headers{};
+  ApiGatewayTransformer transformer;
+  NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};
+
+  if (expected_error_message.empty()) {
+    transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
+    auto lowercase_multi_header_name = Http::LowerCaseString("test-multi-header");
+    auto header_values = response_headers.get(lowercase_multi_header_name);
+
+    if (expected_multi_value_header.empty()) {
+      EXPECT_EQ(header_values.empty(), true);
+      return;
+    }
+
+    EXPECT_EQ(header_values.empty(), false);
+    auto str_header_value = header_values[0]->value().getStringView();
+    EXPECT_EQ(expected_multi_value_header, str_header_value);
+  } else {
+    EXPECT_LOG_CONTAINS(
+      "debug",
+      expected_error_message,
+      transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
+    );
+  }
+}
+
+// helper used in status code type safety tests
+// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
+// - expected_error_message: if present, expect that an error message will be logged that contains this string
+// - expected_status_code: if present, expect that the status code in the response headers will be equal to this string
+void test_status_code_transform(
+    std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
+    std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be number, but is",
+    std::string expected_status_code = "200") {
+  Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
+                                         {":authority", "www.solo.io"},
+                                         {"x-test", "789"},
+                                         {":path", "/users/123"}};
+  Http::TestResponseHeaderMapImpl response_headers{};
+  ApiGatewayTransformer transformer;
+  NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};
+
+  if (expected_error_message.empty()) {
+    transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
+    EXPECT_EQ(expected_status_code, response_headers.getStatusValue());
+  } else {
+    EXPECT_LOG_CONTAINS(
+      "debug",
+      expected_error_message,
+      transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
+    );
+  }
+}
+
+///////////////////////////////////////////
+// multi value headers type safety tests //
+///////////////////////////////////////////
+TEST(ApiGatewayTransformer, transform_null_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": null
+      }
+    })json"),
+    "",
+    ""
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_int_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": 123
+      }
+    })json"),
+    "",
+    "123"
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_float_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": 123.456
+      }
+    })json"),
+    "",
+    "123.456"
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_object_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": {"test": "test-value"}
+      }
+    })json"),
+    "Returning error with message: invalid multi header value object",
+    ""
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value"]
+      }
+    })json"),
+    "",
+    "test-value"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_null_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", null]
+      }
+    })json"),
+    "",
+    "test-value,null"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_int_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", 123]
+      }
+    })json"),
+    "",
+    "test-value,123"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_float_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", 123.456]
+      }
+    })json"),
+    "",
+    "test-value,123.456"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_object_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", {"test": "test-value"}]
+      }
+    })json"),
+    "",
+    "test-value,{\"test\":\"test-value\"}"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_list_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", ["test-value"]]
+      }
+    })json"),
+    "",
+    "test-value,[\"test-value\"]"
+    );
+}
+
+TEST(ApiGatewayTransformer, transform_list_with_list_with_null_multi_value_header) {
+  test_multi_value_headers_transformation(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "multiValueHeaders": {
+          "test-multi-header": ["test-value", [null]]
+      }
+    })json"),
+    "",
+    "test-value,[null]"
+    );
+}
+
+///////////////////////////////////
+// status code type safety tests //
+///////////////////////////////////
+TEST(ApiGatewayTransformer, transform_null_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": null
+    })json"),
+    "cannot parse non unsigned integer status code",
+    ""
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_string_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": "200"
+    })json"),
+    "cannot parse non unsigned integer status code",
+    ""
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_string_non_int_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": "200fasdfasdf"
+    })json"),
+    "cannot parse non unsigned integer status code",
+    ""
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_int_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": 200
+    })json"),
+    "",
+    "200"
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_negative_int_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": -200
+    })json"),
+    "cannot parse non unsigned integer status code",
+    "" 
+  );
+}
+
+
+// note to reviewers: as it stands, this is a breaking change
+// we used to support float input for status code (which would be rounded down to the nearest integer)
+// this PR propses that we reject float input for status code
+TEST(ApiGatewayTransformer, transform_float_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": 201.6
+    })json"),
+    "cannot parse non unsigned integer status code",
+    "" 
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_object_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": {"test": "test-value"}
+    })json"),
+    "cannot parse non unsigned integer status code",
+    ""
+  );
+}
+
+TEST(ApiGatewayTransformer, transform_list_status_code) {
+  test_status_code_transform(
+    std::make_unique<Buffer::OwnedImpl>(R"json({
+      "statusCode": ["test-value"]
+    })json"),
+    "cannot parse non unsigned integer status code",
+    ""
+  );
+}
+
+
 } // namespace AwsLambda
 } // namespace HttpFilters
 } // namespace Extensions
-} // namespace Envoy
+} // namespace Envoy