Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fixed event matcher to focus on properties specific to web requests (v1/ALB and v2) #2863

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mrickard
Copy link
Member

@mrickard mrickard commented Jan 3, 2025

Description

Customer reported an error when triggering an instrumented Lambda function via APIGatewayRequestAuthorizerEvent.
This PR makes the event matching more restrictive to avoid false positives.

How to Test

Added event fixture to test/unit/serverless/fixtures.js and assertions to test/unit/serverless/utils.test.js. Added unit test for API Gateway V2 websocket events in test/unit/serverless/api-gateway-v2-websocket.test.js

Related Issues

Closes #2857
Closes NR-354739

…ed test for API Gateway Lambda authorizers, fixed typo

Signed-off-by: mrickard <maurice@mauricerickard.com>
… keys, but changed to bail early when a key isn't found

Signed-off-by: mrickard <maurice@mauricerickard.com>
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.22%. Comparing base (763c0e6) to head (bcef1de).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2863      +/-   ##
==========================================
- Coverage   97.27%   97.22%   -0.06%     
==========================================
  Files         296      296              
  Lines       46625    46552      -73     
==========================================
- Hits        45354    45259      -95     
- Misses       1271     1293      +22     
Flag Coverage Δ
integration-tests-cjs-18.x 73.17% <22.22%> (+0.01%) ⬆️
integration-tests-cjs-20.x 73.18% <22.22%> (+0.01%) ⬆️
integration-tests-cjs-22.x 73.21% <22.22%> (+0.02%) ⬆️
integration-tests-esm-18.x 49.88% <22.22%> (-0.04%) ⬇️
integration-tests-esm-20.x 49.89% <22.22%> (-0.04%) ⬇️
integration-tests-esm-22.x 49.94% <22.22%> (-0.04%) ⬇️
unit-tests-18.x 88.88% <100.00%> (-0.02%) ⬇️
unit-tests-20.x 88.88% <100.00%> (-0.02%) ⬇️
unit-tests-22.x 88.89% <100.00%> (-0.02%) ⬇️
versioned-tests-18.x 79.08% <22.22%> (-0.17%) ⬇️
versioned-tests-20.x 79.09% <22.22%> (-0.17%) ⬇️
versioned-tests-22.x 79.10% <22.22%> (-0.17%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: mrickard <maurice@mauricerickard.com>
…, renamed methods to focus on proxying, folded ALB check into Gateway v1

Signed-off-by: mrickard <maurice@mauricerickard.com>
@mrickard mrickard marked this pull request as ready for review January 6, 2025 20:41
@mrickard mrickard changed the title fix: Fixed event matcher to limit to API Gateway web and ALB web fix: Fixed event matcher to focus on properties specific to web requests (v1/ALB and v2) Jan 6, 2025
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic has been pared down quite a lot. Are we certain this won't regress on the driver for the initial refactor of this file?

@mrickard
Copy link
Member Author

mrickard commented Jan 7, 2025

@bizob2828 That's a valid concern. The initial driver was for support for API Gateway V2 events, which are structured differently from V1, particularly regarding the path, httpMethod, and headers (or, in the case of v1 multiValueHeaders) we used for determining whether or not an event represented a web request.

Initially we focused on identifying v1 vs v2 events in this commit. While this takes a similar approach to the current PR, it checks event version first. That could result in non-web requests being matched as web requests, as non-web-requests can use API Gateway v1 and v2.

This subsequent fix was driven by a similar issue to the current issue: a non-web request was identified as a web request. The fix matched on more event properties, and better identified web requests because of that, but it was too restrictive, excluding ALB web events from being identified as web requests.

Subsequent fixes added explicit support for v1 REST events and for ALB events. These focused on those event sources, but did not focus explicitly on identifying web from non-web requests. (ALB matching is removed for this PR because its web-request signature is the same as that of API Gateway v1.)

One distinct advantage of all of these fixes is that they've added tests we can use to check whether or not the matching behaves as expected. Those are still passing for this refactoring.

@mrickard mrickard requested a review from bizob2828 January 7, 2025 18:55
Copy link
Member

@bizob2828 bizob2828 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one tiny suggestion on the code comments, but otherwise LGTM

test/unit/serverless/fixtures.js Outdated Show resolved Hide resolved
Co-authored-by: Bob Evans <robert.evans25@gmail.com>
@mrickard mrickard requested a review from bizob2828 January 7, 2025 19:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Needs PR Review
Development

Successfully merging this pull request may close these issues.

Setting lambda handler is broken for API Gateway V2 Websocket lambda payloads since 12.8.1
2 participants