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

Safely Pass Percent Symbols in Paths #170

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Safely Pass Percent Symbols in Paths #170

merged 2 commits into from
Jul 20, 2023

Conversation

metaskills
Copy link
Member

@metaskills metaskills commented Jul 7, 2023

Here is an example of running local Rails under Puma with something like /test/dwef782jkif%3d in the path:

Started GET "/test/dwef782jkif%3d" for 172.19.0.1 at 2023-07-07 19:19:30 +0000
Processing by ApplicationController#test as HTML
  Parameters: {"path"=>"dwef782jkif="}
Completed 200 OK in 2ms (ActiveRecord: 0.0ms | Allocations: 113)

Prior to this change, here is what it looked like in Lambda with Lamby:

Started GET "/test/dwef782jkif=" for 98.166.4.233 at 2023-07-07 19:11:39 +0000
Processing by ApplicationController#test as HTML
  Parameters: {"path"=>"dwef782jkif="}
Completed 200 OK in 2ms (ActiveRecord: 0.0ms | Allocations: 114)

We learned the same lessons with query params, always trust Raw. Thankfully, HTTP v2 allows that to happen.

ℹ️ There are some folks that use AWS::ApiGatewayV2::Integration with API Gateway to proxy requests to AWS Services like Lambda, but the mapping request parameter tooling (https://docs.aws.amazon.com/apigateway/latest/developerguide/http-api-develop-integrations-aws-services.html) does not provide a way to get to the raw value. RIP 🪦

@metaskills metaskills requested a review from drinks July 7, 2023 20:15
Copy link

@drinks drinks left a comment

Choose a reason for hiding this comment

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

Looks like it should make the url encoding issue disappear. Only concern would be that if folks are using lamby with path overwrites today, this change will break their routing -- the previous version used path directly and this will use rawPath if path and rawPath aren't equal -- which could be because of an encoding issue, or because the user has manipulated the path in their integration.

@metaskills
Copy link
Member Author

using lamby with path overwrites today

This feels real far off the beaten path. Was that pun bad? lol

On a serious note, what does that mean? Most folks use Lamby like we do, simple SAM/HTTPv2 proxy and when issues happen we get reports. I'm not sure how or why folks would write their own integration outside of SAM but assuming that is what you're talking about? If so, I'd be more worried about folks that wrote custom code in their Rails app to work around this vs filing an issue. Does that make sense?

@metaskills metaskills closed this Jul 7, 2023
@metaskills metaskills reopened this Jul 7, 2023
@drinks
Copy link

drinks commented Jul 7, 2023

This feels real far off the beaten path

Sure, totally valid.

On a serious note, what does that mean?

Just pointing out that I could deploy a lamby app as a standalone with no http layer in front of it, and use a lambda proxy integration to invoke it from a decoupled gateway. If I were applying path overwrites in such a context, they would have worked in the previous release, but would break in this one.

I think it's probably ok to simply note that in the Changelog, just pointing out the possibility.

@metaskills
Copy link
Member Author

Thanks, I'll call out those in the changelog.

@metaskills metaskills merged commit 99dc164 into master Jul 20, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants