Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork4.5k
fix proxied requests forwarding to Moto#11653
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
github-actionsbot commentedOct 8, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
alexrashed left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Wow! Great catch! This really is an interesting case! Thanks a lot for jumping on this and digging this deep!
I think we could move forward with this fix, but I think the problem here could actually be caused byrolo (sincerolo creates the request).
I think it would be worth it to try to:
- See if this issue can really be reproduced with werkzeug and raise an issue in their repo is this really is the case.Their own code suggest that you are right and the path variable really shouldn't contain anything before the root path slash.
- If it is not reproducible in plain werkzeug, I think this would actually be a subtle issue in
rolo, and we should fix it there. /cc@thrau
What do you think?
| raw_path=get_full_raw_path(request) | ||
| # this is where we skip the HTTP roundtrip between the moto server and the boto client | ||
| dispatch=get_dispatcher(service.service_name,request.path) | ||
| dispatch=get_dispatcher(service.service_name,raw_path.split("?")[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
nit: Would be great to add a comment line here explaining what's happening here and why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yep, agreed. The thing is, the "best" would be to callget_raw_path only to get the path to match the request, but it is called internally inget_full_raw_path and we'd be doing twice. I'll add a comment!
bentsku commentedOct 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@alexrashed sure! I'll try recreating against Werkzeug, I believe that during my first investigation I had already found that Werkzeug had the issue itself. In Rolo, it creates the request by directly instantiating I'll set up a reproducer for Werkzeug, but I think we should go forward with this fix for now! I'll add the comment edit: working on the reproducer, I can't reproduce with either base Werkzeug or even base hello world rolo, printing |
a9482e0 to21ed1a8Comparebentsku commentedOct 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@alexrashed the issue seems to be in Twisted, when getting the WSGI Small reproducer: if__name__=='__main__':fromtwisted.webimportserver,resourcefromtwisted.internetimportreactorclassSimple(resource.Resource):isLeaf=Truedefrender_POST(self,request):print(f"{request.path=}")return"<html>Hello, world!</html>"site=server.Site(Simple())reactor.listenTCP(4566,site) Using the reproducer given inhttps://github.com/Blind-Striker/localstack-sns-issue-sandbox, this prints: |
alexrashed left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Sure! We can definitely move forward with this fix to unblock users, and then carefully look at the issue in rolo and twisted! Thanks a lot for digging into this and addressing the comment! 🏅 💯
bentsku commentedOct 11, 2024
@alexrashed here's the PR in Rolo:localstack/rolo#23 |
Uh oh!
There was an error while loading.Please reload this page.
Motivation
As reported by#11652, we have an issue forwarding requests to
motowhen those have a full URI in the HTTP request, as explained in#8962.(Example HTTP request:
POST http://sns.eu-central-1.amazonaws.com/ HTTP/1.1instead of the usualPOST / HTTP/1.1)This PR uses the helper created with the above PR to get the request path to send towards the moto dispatcher, so that we only get the
pathpart of the request. It only changes the logic to get the actual endpoint to call, as we were already using that helper to forward the request to moto. The logic change is very minimal.Looking at it, I believe this is an issue in how Werkzeug handles those requests, putting the full
RAW_URIin thepathand not doing sanitization beforehand, as we've seen withhttps://www.ietf.org/rfc/rfc2616.txt that web servers should handle those.Also, the question comes as to what changed in
3.7.2, as this used to work with versions before, tested by the user down to3.4.Changes
fixes#11652