From f540a16545632a71e578a86ae42607c79c997e0a Mon Sep 17 00:00:00 2001 From: Ben Taussig <85883594+ben-taussig-solo@users.noreply.github.com> Date: Tue, 23 Apr 2024 09:57:09 -0400 Subject: [PATCH] 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 --- .../lambda-json-parsing-fix.yaml | 9 + .../aws_lambda/api_gateway_transformer.cc | 33 +- test/extensions/transformers/aws_lambda/BUILD | 2 - .../aws_lambda/transformer_test.cc | 296 +++++++++++++++++- 4 files changed, 324 insertions(+), 16 deletions(-) create mode 100644 changelog/v1.29.3-patch2/lambda-json-parsing-fix.yaml diff --git a/changelog/v1.29.3-patch2/lambda-json-parsing-fix.yaml b/changelog/v1.29.3-patch2/lambda-json-parsing-fix.yaml new file mode 100644 index 000000000..ad4ff63ca --- /dev/null +++ b/changelog/v1.29.3-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 \ No newline at end of file diff --git a/source/extensions/transformers/aws_lambda/api_gateway_transformer.cc b/source/extensions/transformers/aws_lambda/api_gateway_transformer.cc index d8cf76eb4..089a03e47 100644 --- a/source/extensions/transformers/aws_lambda/api_gateway_transformer.cc +++ b/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(); - } 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(); response_headers->setStatus(status_value); } else { response_headers->setStatus(DEFAULT_STATUS_VALUE); @@ -132,8 +133,8 @@ 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); } @@ -141,8 +142,24 @@ void ApiGatewayTransformer::transform_response( 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 header_value; + if (inner_it.value().is_string()) { + header_value = inner_it.value().get(); + } else { + header_value = inner_it.value().dump(); + } add_response_header(*response_headers, header_key, header_value, stream_filter_callbacks, true); } } diff --git a/test/extensions/transformers/aws_lambda/BUILD b/test/extensions/transformers/aws_lambda/BUILD index efdad2e2c..c0a5a26d0 100644 --- a/test/extensions/transformers/aws_lambda/BUILD +++ b/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", ], ) \ No newline at end of file diff --git a/test/extensions/transformers/aws_lambda/transformer_test.cc b/test/extensions/transformers/aws_lambda/transformer_test.cc index 93bb7a4f3..aeea1b713 100644 --- a/test/extensions/transformers/aws_lambda/transformer_test.cc +++ b/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 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 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 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 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(R"json({ + "multiValueHeaders": { + "test-multi-header": null + } + })json"), + "", + "" + ); +} + +TEST(ApiGatewayTransformer, transform_int_multi_value_header) { + test_multi_value_headers_transformation( + std::make_unique(R"json({ + "multiValueHeaders": { + "test-multi-header": 123 + } + })json"), + "", + "123" + ); +} + +TEST(ApiGatewayTransformer, transform_float_multi_value_header) { + test_multi_value_headers_transformation( + std::make_unique(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(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(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(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(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(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(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(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(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(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(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(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(R"json({ + "statusCode": 200 + })json"), + "", + "200" + ); +} + +TEST(ApiGatewayTransformer, transform_negative_int_status_code) { + test_status_code_transform( + std::make_unique(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(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(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(R"json({ + "statusCode": ["test-value"] + })json"), + "cannot parse non unsigned integer status code", + "" + ); +} + + } // namespace AwsLambda } // namespace HttpFilters } // namespace Extensions -} // namespace Envoy +} // namespace Envoy \ No newline at end of file