-
-
Notifications
You must be signed in to change notification settings - Fork 21
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 URL parameter encoding #102
Fix URL parameter encoding #102
Conversation
src/apig_wsgi.py
Outdated
@@ -55,7 +55,8 @@ def get_environ(event, binary_support): | |||
else: | |||
body = body.encode("utf-8") | |||
params = event.get("queryStringParameters") or {} | |||
|
|||
# decoding first to prevent double encoding | |||
params = {k: unquote(v) for k, v in params.items()} |
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.
You're only unquoting values - did you verify the issue doesn't exist in keys?
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.
My thinking was that you would hopefully never have an application that has keys like that :). Probably not a safe assumption though.
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.
A URL like example.com/?+123
should have a key with an empty string as the value
I started to test more encoding and looks like there's also an issue with spaces. Interested to hear your thoughts on how this could be handled? I'll have a look at how others are handling this (maybe they aren't) |
They definitely are in other WSGI servers, like wsgiref or gunicorn. If you refer to the WSGI specification that should also give a clue. |
After a bit of investigation, I think using
or
with no issues. This is testing my specific use case. |
I've done some testing with other wsgi servers, specifically Django's Merged, added history note and split tests in #102, and release in version 2.4.1: https://pypi.org/project/apig-wsgi/2.4.1/ Enjoy! |
Yup, this update actually breaks Flask (werkzeug) app as it replaces
|
I'm using Flask and I can assure you it is not broken. The problem is the ambiguity around what a
I much prefer having the option to send encoded |
Just sharing my scenerio here to give more details of the issue. In query string i am passing pagination token (automatically generated, can contain literal
This was not happening before this update because in step 4, query string params were encoded ( Should we add a flag to enable/disable query string encoding in |
I am not sure steps 1 and 2 are true. Using an example Django app I have seen the browser always sends + as a +, never encoding it. Therefore it seems API Gateway isn't touching that. In fact in my demo app, + is always decoded by Django's Can you share a demo app using whatever framework you're using that replicates the behaviour you want @falloutcoder ? |
I added an example Django app with Ansible deployment in #110. It confirms to me that current behaviour is correct. Here's the app locally using Django's And on API Gateway: An interesting learning is that API Gateway, unlike most webservers, returns a 400 for badly formatted parameters like a double %: |
I ran into this when using swaggerui to test my lambda. The swaggerui was urlencode params prior to sending them. In our exisisting setup, this was getting handled, I could provide encoded or not and it worked.