Repository navigation
fix(aws_lambda): handle null headers, query string, and body in API Gateway events - #1587
Conversation
|
Thanks for the contribution! Before we can merge this, we need @sahiljagtap08 to sign the Salesforce Inc. Contributor License Agreement. |
There was a problem hiding this comment.
thank you for opening this PR @sahiljagtap08! l'll give it a more thorough review once the CLA is signed and CI passes 😸
|
recheck |
…ateway events API Gateway sends null, not a missing key, for request fields that have no value. For example "headers", "multiValueHeaders" and "queryStringParameters" are null when the request has none of them. The Lambda adapter assumed these were always dicts and raised AttributeError before the request was dispatched. It also raised KeyError when "isBase64Encoded" was absent, and TypeError when the flag was true but the body was null. - Treat None the same as a missing key for every event field - Copy the headers dict so the caller's event is not modified - Add tests for null fields, a missing base64 flag, and a real base64-encoded body
ae7ccba to
f62cda5
Compare
|
The CLA is signed and the check is passing on this one too. Happy to make any changes if needed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1587 +/- ##
==========================================
+ Coverage 91.54% 91.57% +0.02%
==========================================
Files 228 228
Lines 7285 7285
==========================================
+ Hits 6669 6671 +2
+ Misses 616 614 -2 ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
@sahiljagtap08 this PR looks good! thank you for adding stricter checks and tests to the lambda adapter! i left some suggestions to keep our code clean!
| # API Gateway sends null (not a missing key) for fields that have no value, | ||
| # such as "headers", "multiValueHeaders" and "queryStringParameters". | ||
| # Every lookup below treats None the same as a missing key. |
There was a problem hiding this comment.
lets get rid of these comments
| # API Gateway sends null (not a missing key) for fields that have no value, | |
| # such as "headers", "multiValueHeaders" and "queryStringParameters". | |
| # Every lookup below treats None the same as a missing key. |
| cookies = multiValueHeaders.get("Cookie", []) | ||
| headers = event.get("headers", {}) | ||
| cookies = multiValueHeaders.get("Cookie") or [] | ||
| # Copy the headers so the caller's event dict is left untouched |
There was a problem hiding this comment.
same as above
| # Copy the headers so the caller's event dict is left untouched |
| # API Gateway sends null for headers, multiValueHeaders and queryStringParameters | ||
| # when the request has none. These used to crash before the request was dispatched. |
There was a problem hiding this comment.
same as above
| # API Gateway sends null for headers, multiValueHeaders and queryStringParameters | |
| # when the request has none. These used to crash before the request was dispatched. |
| # isBase64Encoded is absent (for example when invoked from a test tool) | ||
| event = {"requestContext": {"http": {"method": "POST"}}, "headers": {}, "body": "{}"} | ||
| assert SlackRequestHandler(app).handle(event, self.context)["statusCode"] == 401 | ||
| # isBase64Encoded is true but the body is null |
There was a problem hiding this comment.
| # isBase64Encoded is true but the body is null |
| "isBase64Encoded": False, | ||
| } | ||
| response = SlackRequestHandler(app).handle(event, self.context) | ||
| # No signature headers, so the request is rejected rather than crashing |
There was a problem hiding this comment.
| # No signature headers, so the request is rejected rather than crashing |
| response = SlackRequestHandler(app).handle(event, self.context) | ||
| # No signature headers, so the request is rejected rather than crashing | ||
| assert response["statusCode"] == 401 | ||
| # The caller's event must not be modified |
There was a problem hiding this comment.
| # The caller's event must not be modified |
|
|
||
| def test_missing_is_base64_encoded_and_null_body(self): | ||
| app = App(client=self.web_client, signing_secret=self.signing_secret) | ||
| # isBase64Encoded is absent (for example when invoked from a test tool) |
There was a problem hiding this comment.
| # isBase64Encoded is absent (for example when invoked from a test tool) |
|
@srtaalej thanks for the review and the approval! Applied all your suggestions and removed those comments. Checks pass locally. |
Summary
API Gateway sends
null, not a missing key, for request fields that have no value. For a REST API (payload format v1)headers,multiValueHeaders, andqueryStringParametersare allnullwhen the request has none of them. The AWS Lambda adapter assumed these were always dicts and raisedAttributeErrorinsideto_bolt_request, before the request reached the middleware chain.Two related edge cases were fixed at the same time:
isBase64Encodedmissing from the event raisedKeyError. This happens when the function is invoked by test tooling or a custom integration that does not set the flag.isBase64Encoded: truewith anullbody raisedTypeErrorfrombase64.b64decode.Changes in
slack_bolt/adapter/aws_lambda/handler.py:Nonethe same as a missing key.trueand a body is present.Testing
New tests in
tests/adapter_tests/aws/test_aws_lambda.py:test_null_fields_in_api_gateway_event: nullheaders,multiValueHeaders, andqueryStringParametersnow produce a 401 (no signature) instead of a crash, and the original event is not modified.test_missing_is_base64_encoded_and_null_body: covers the missing flag and the null body with the flag set.test_base64_encoded_body: a properly signed base64-encoded body is decoded and handled with a 200.Both crash tests fail on
mainand pass with this change. Ran./scripts/format.sh,./scripts/lint.sh,./scripts/run_mypy.sh, and the adapter test suite.Category
Requirements
./scripts/install_all_and_run_tests.shafter making the changes.