Skip to content

Commit 093573c

Browse files
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 <[email protected]>
1 parent 42867b7 commit 093573c

File tree

4 files changed

+324
-16
lines changed

4 files changed

+324
-16
lines changed
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
changelog:
2+
- type: FIX
3+
description: >
4+
Fixes an issue where the Lambda filter would choke on unexpected JSON input
5+
when unwrapAsApiGateway was enabled. Specifically, the filter would fail to
6+
parse non-string header values under the multiValueHeaders key in the Lambda
7+
response.
8+
issueLink: https://github.com/solo-io/gloo/issues/8867
9+
resolvesIssue: false

source/extensions/transformers/aws_lambda/api_gateway_transformer.cc

+25-8
Original file line numberDiff line numberDiff line change
@@ -95,13 +95,14 @@ void ApiGatewayTransformer::transform_response(
9595
// set response status code
9696
if (json_body.contains("statusCode")) {
9797
uint64_t status_value;
98-
try {
99-
status_value = json_body["statusCode"].get<uint64_t>();
100-
} catch (std::exception& exception){
101-
ENVOY_STREAM_LOG(debug, "Error parsing statusCode: " + std::string(exception.what()), stream_filter_callbacks);
102-
ApiGatewayError error = {500, "500", "Non-integer status code"};
98+
if (!json_body["statusCode"].is_number_unsigned()) {
99+
// add duplicate log line to not break tests for now
100+
ENVOY_STREAM_LOG(debug, "cannot parse non unsigned integer status code", stream_filter_callbacks);
101+
ENVOY_STREAM_LOG(debug, "received status code with value: " + json_body["statusCode"].dump(), stream_filter_callbacks);
102+
ApiGatewayError error = {500, "500", "cannot parse non unsigned integer status code"};
103103
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
104104
}
105+
status_value = json_body["statusCode"].get<uint64_t>();
105106
response_headers->setStatus(status_value);
106107
} else {
107108
response_headers->setStatus(DEFAULT_STATUS_VALUE);
@@ -132,17 +133,33 @@ void ApiGatewayTransformer::transform_response(
132133
if (json_body.contains("multiValueHeaders")) {
133134
const auto& multi_value_headers = json_body["multiValueHeaders"];
134135
if (!multi_value_headers.is_object()) {
135-
ENVOY_STREAM_LOG(debug, "invalid multi headers object", stream_filter_callbacks);
136-
ApiGatewayError error = {500, "500", "invalid multi headers object"};
136+
ENVOY_STREAM_LOG(debug, "invalid multiValueHeaders object", stream_filter_callbacks);
137+
ApiGatewayError error = {500, "500", "invalid multiValueHeaders object"};
137138
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
138139
}
139140

140141
for (json::const_iterator it = multi_value_headers.cbegin(); it != multi_value_headers.cend(); it++) {
141142
const auto& header_key = it.key();
142143
const auto& header_values = it.value();
143144

145+
// need to validate that header_values is an array/iterable
146+
if (!header_values.is_array()) {
147+
// if it's an object, we reject the request
148+
if (header_values.is_object()) {
149+
ENVOY_STREAM_LOG(debug, "invalid multi header value object", stream_filter_callbacks);
150+
ApiGatewayError error = {500, "500", "invalid multi header value object"};
151+
return ApiGatewayTransformer::format_error(*response_headers, body, error, stream_filter_callbacks);
152+
}
153+
154+
ENVOY_STREAM_LOG(debug, "warning: using non-array value for multi header value", stream_filter_callbacks);
155+
}
144156
for (json::const_iterator inner_it = header_values.cbegin(); inner_it != header_values.cend(); inner_it++) {
145-
const auto& header_value = inner_it.value().get<std::string>();
157+
std::string header_value;
158+
if (inner_it.value().is_string()) {
159+
header_value = inner_it.value().get<std::string>();
160+
} else {
161+
header_value = inner_it.value().dump();
162+
}
146163
add_response_header(*response_headers, header_key, header_value, stream_filter_callbacks, true);
147164
}
148165
}

test/extensions/transformers/aws_lambda/BUILD

-2
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,5 @@ envoy_gloo_cc_test(
1919
"//api/envoy/config/transformer/aws_lambda/v2:pkg_cc_proto",
2020
"//source/extensions/transformers/aws_lambda:api_gateway_transformer_lib",
2121
"@envoy//test/mocks/http:http_mocks",
22-
"@envoy//test/mocks/server:server_mocks",
23-
"@envoy//test/test_common:utility_lib",
2422
],
2523
)

test/extensions/transformers/aws_lambda/transformer_test.cc

+290-6
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,8 @@
33
#include "source/extensions/transformers/aws_lambda/api_gateway_transformer.h"
44

55
#include "test/mocks/http/mocks.h"
6-
#include "test/mocks/server/mocks.h"
7-
#include "test/test_common/utility.h"
86

9-
#include "fmt/format.h"
107
#include "nlohmann/json.hpp"
11-
#include "gmock/gmock.h"
12-
#include "gtest/gtest.h"
138

149
using testing::_;
1510
using testing::AtLeast;
@@ -305,7 +300,296 @@ TEST(ApiGatewayTransformer, error) {
305300
EXPECT_EQ(response_headers.get(Http::LowerCaseString("x-amzn-errortype"))[0]->value().getStringView(), "500");
306301
}
307302

303+
// helper used in multi value headers type safety tests
304+
// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
305+
// - expected_error_message: if present, expect that an error message will be logged that contains this string
306+
// - 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,
307+
void test_multi_value_headers_transformation(
308+
std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
309+
std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be string, but is null",
310+
std::string expected_multi_value_header = "multi-value-0,multi-value-1,multi-value-2") {
311+
Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
312+
{":authority", "www.solo.io"},
313+
{"x-test", "789"},
314+
{":path", "/users/123"}};
315+
Http::TestResponseHeaderMapImpl response_headers{};
316+
ApiGatewayTransformer transformer;
317+
NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};
318+
319+
if (expected_error_message.empty()) {
320+
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
321+
auto lowercase_multi_header_name = Http::LowerCaseString("test-multi-header");
322+
auto header_values = response_headers.get(lowercase_multi_header_name);
323+
324+
if (expected_multi_value_header.empty()) {
325+
EXPECT_EQ(header_values.empty(), true);
326+
return;
327+
}
328+
329+
EXPECT_EQ(header_values.empty(), false);
330+
auto str_header_value = header_values[0]->value().getStringView();
331+
EXPECT_EQ(expected_multi_value_header, str_header_value);
332+
} else {
333+
EXPECT_LOG_CONTAINS(
334+
"debug",
335+
expected_error_message,
336+
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
337+
);
338+
}
339+
}
340+
341+
// helper used in status code type safety tests
342+
// - bodyPtr: json payload in the format used by AWS API Gateway/returned from an upstream Lambda
343+
// - expected_error_message: if present, expect that an error message will be logged that contains this string
344+
// - expected_status_code: if present, expect that the status code in the response headers will be equal to this string
345+
void test_status_code_transform(
346+
std::unique_ptr<Buffer::OwnedImpl> bodyPtr,
347+
std::string expected_error_message = "Error transforming response: [json.exception.type_error.302] type must be number, but is",
348+
std::string expected_status_code = "200") {
349+
Http::TestRequestHeaderMapImpl headers{{":method", "GET"},
350+
{":authority", "www.solo.io"},
351+
{"x-test", "789"},
352+
{":path", "/users/123"}};
353+
Http::TestResponseHeaderMapImpl response_headers{};
354+
ApiGatewayTransformer transformer;
355+
NiceMock<Http::MockStreamDecoderFilterCallbacks> filter_callbacks_{};
356+
357+
if (expected_error_message.empty()) {
358+
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_);
359+
EXPECT_EQ(expected_status_code, response_headers.getStatusValue());
360+
} else {
361+
EXPECT_LOG_CONTAINS(
362+
"debug",
363+
expected_error_message,
364+
transformer.transform(response_headers, &headers, *bodyPtr, filter_callbacks_)
365+
);
366+
}
367+
}
368+
369+
///////////////////////////////////////////
370+
// multi value headers type safety tests //
371+
///////////////////////////////////////////
372+
TEST(ApiGatewayTransformer, transform_null_multi_value_header) {
373+
test_multi_value_headers_transformation(
374+
std::make_unique<Buffer::OwnedImpl>(R"json({
375+
"multiValueHeaders": {
376+
"test-multi-header": null
377+
}
378+
})json"),
379+
"",
380+
""
381+
);
382+
}
383+
384+
TEST(ApiGatewayTransformer, transform_int_multi_value_header) {
385+
test_multi_value_headers_transformation(
386+
std::make_unique<Buffer::OwnedImpl>(R"json({
387+
"multiValueHeaders": {
388+
"test-multi-header": 123
389+
}
390+
})json"),
391+
"",
392+
"123"
393+
);
394+
}
395+
396+
TEST(ApiGatewayTransformer, transform_float_multi_value_header) {
397+
test_multi_value_headers_transformation(
398+
std::make_unique<Buffer::OwnedImpl>(R"json({
399+
"multiValueHeaders": {
400+
"test-multi-header": 123.456
401+
}
402+
})json"),
403+
"",
404+
"123.456"
405+
);
406+
}
407+
408+
TEST(ApiGatewayTransformer, transform_object_multi_value_header) {
409+
test_multi_value_headers_transformation(
410+
std::make_unique<Buffer::OwnedImpl>(R"json({
411+
"multiValueHeaders": {
412+
"test-multi-header": {"test": "test-value"}
413+
}
414+
})json"),
415+
"Returning error with message: invalid multi header value object",
416+
""
417+
);
418+
}
419+
420+
TEST(ApiGatewayTransformer, transform_list_multi_value_header) {
421+
test_multi_value_headers_transformation(
422+
std::make_unique<Buffer::OwnedImpl>(R"json({
423+
"multiValueHeaders": {
424+
"test-multi-header": ["test-value"]
425+
}
426+
})json"),
427+
"",
428+
"test-value"
429+
);
430+
}
431+
432+
TEST(ApiGatewayTransformer, transform_list_with_null_multi_value_header) {
433+
test_multi_value_headers_transformation(
434+
std::make_unique<Buffer::OwnedImpl>(R"json({
435+
"multiValueHeaders": {
436+
"test-multi-header": ["test-value", null]
437+
}
438+
})json"),
439+
"",
440+
"test-value,null"
441+
);
442+
}
443+
444+
TEST(ApiGatewayTransformer, transform_list_with_int_multi_value_header) {
445+
test_multi_value_headers_transformation(
446+
std::make_unique<Buffer::OwnedImpl>(R"json({
447+
"multiValueHeaders": {
448+
"test-multi-header": ["test-value", 123]
449+
}
450+
})json"),
451+
"",
452+
"test-value,123"
453+
);
454+
}
455+
456+
TEST(ApiGatewayTransformer, transform_list_with_float_multi_value_header) {
457+
test_multi_value_headers_transformation(
458+
std::make_unique<Buffer::OwnedImpl>(R"json({
459+
"multiValueHeaders": {
460+
"test-multi-header": ["test-value", 123.456]
461+
}
462+
})json"),
463+
"",
464+
"test-value,123.456"
465+
);
466+
}
467+
468+
TEST(ApiGatewayTransformer, transform_list_with_object_multi_value_header) {
469+
test_multi_value_headers_transformation(
470+
std::make_unique<Buffer::OwnedImpl>(R"json({
471+
"multiValueHeaders": {
472+
"test-multi-header": ["test-value", {"test": "test-value"}]
473+
}
474+
})json"),
475+
"",
476+
"test-value,{\"test\":\"test-value\"}"
477+
);
478+
}
479+
480+
TEST(ApiGatewayTransformer, transform_list_with_list_multi_value_header) {
481+
test_multi_value_headers_transformation(
482+
std::make_unique<Buffer::OwnedImpl>(R"json({
483+
"multiValueHeaders": {
484+
"test-multi-header": ["test-value", ["test-value"]]
485+
}
486+
})json"),
487+
"",
488+
"test-value,[\"test-value\"]"
489+
);
490+
}
491+
492+
TEST(ApiGatewayTransformer, transform_list_with_list_with_null_multi_value_header) {
493+
test_multi_value_headers_transformation(
494+
std::make_unique<Buffer::OwnedImpl>(R"json({
495+
"multiValueHeaders": {
496+
"test-multi-header": ["test-value", [null]]
497+
}
498+
})json"),
499+
"",
500+
"test-value,[null]"
501+
);
502+
}
503+
504+
///////////////////////////////////
505+
// status code type safety tests //
506+
///////////////////////////////////
507+
TEST(ApiGatewayTransformer, transform_null_status_code) {
508+
test_status_code_transform(
509+
std::make_unique<Buffer::OwnedImpl>(R"json({
510+
"statusCode": null
511+
})json"),
512+
"cannot parse non unsigned integer status code",
513+
""
514+
);
515+
}
516+
517+
TEST(ApiGatewayTransformer, transform_string_status_code) {
518+
test_status_code_transform(
519+
std::make_unique<Buffer::OwnedImpl>(R"json({
520+
"statusCode": "200"
521+
})json"),
522+
"cannot parse non unsigned integer status code",
523+
""
524+
);
525+
}
526+
527+
TEST(ApiGatewayTransformer, transform_string_non_int_status_code) {
528+
test_status_code_transform(
529+
std::make_unique<Buffer::OwnedImpl>(R"json({
530+
"statusCode": "200fasdfasdf"
531+
})json"),
532+
"cannot parse non unsigned integer status code",
533+
""
534+
);
535+
}
536+
537+
TEST(ApiGatewayTransformer, transform_int_status_code) {
538+
test_status_code_transform(
539+
std::make_unique<Buffer::OwnedImpl>(R"json({
540+
"statusCode": 200
541+
})json"),
542+
"",
543+
"200"
544+
);
545+
}
546+
547+
TEST(ApiGatewayTransformer, transform_negative_int_status_code) {
548+
test_status_code_transform(
549+
std::make_unique<Buffer::OwnedImpl>(R"json({
550+
"statusCode": -200
551+
})json"),
552+
"cannot parse non unsigned integer status code",
553+
""
554+
);
555+
}
556+
557+
558+
// note to reviewers: as it stands, this is a breaking change
559+
// we used to support float input for status code (which would be rounded down to the nearest integer)
560+
// this PR propses that we reject float input for status code
561+
TEST(ApiGatewayTransformer, transform_float_status_code) {
562+
test_status_code_transform(
563+
std::make_unique<Buffer::OwnedImpl>(R"json({
564+
"statusCode": 201.6
565+
})json"),
566+
"cannot parse non unsigned integer status code",
567+
""
568+
);
569+
}
570+
571+
TEST(ApiGatewayTransformer, transform_object_status_code) {
572+
test_status_code_transform(
573+
std::make_unique<Buffer::OwnedImpl>(R"json({
574+
"statusCode": {"test": "test-value"}
575+
})json"),
576+
"cannot parse non unsigned integer status code",
577+
""
578+
);
579+
}
580+
581+
TEST(ApiGatewayTransformer, transform_list_status_code) {
582+
test_status_code_transform(
583+
std::make_unique<Buffer::OwnedImpl>(R"json({
584+
"statusCode": ["test-value"]
585+
})json"),
586+
"cannot parse non unsigned integer status code",
587+
""
588+
);
589+
}
590+
591+
308592
} // namespace AwsLambda
309593
} // namespace HttpFilters
310594
} // namespace Extensions
311-
} // namespace Envoy
595+
} // namespace Envoy

0 commit comments

Comments
 (0)