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

request.form uses urllib.parse_qs which does not persist blank values by default when requests submit application/x-www-form-urlencoded data #2427

Closed
pobk opened this issue Apr 10, 2022 · 3 comments · Fixed by #2439
Assignees
Labels
Milestone

Comments

@pobk
Copy link

pobk commented Apr 10, 2022

Describe the bug
This seems to be an acknowledged limitation of parse_qs, but I'm raising as a bug as this introduces undesired behaviour rather than adding new features.

While request.get_args and and request.get_query_args accept the keep_blank_values argument in order to persist blanks, request.form does not provide an interface to persist blank values.

My assertion is that a framework should not remove data simply because it doesn't have a value.

Relevant anecdote: Twilio webhooks submit a POST request using application/x-www-form-urlencoded encoded body. The body includes all values including some potentially blank/empty values.

When the request is processed by Sanic, it is decoded using urllib.parse_qs and does not have a mechanism to allow blank elements to persist. This results in missing elements from the request when accessing the variables from request.form.

In my case, the elements from the submission are relevant to generate a signature for request validation (see https://www.twilio.com/docs/usage/security). The signature is constructed using SHA1 hash of the submitted request key/value pairs and missing pairs generates a different signature to that expected.

In this instance, I have been receiving requests which include two elements that, depending on the hook, may or may not have a value.

Expected behavior
All variables, irrespective of existence of a value should be passed to the application.

Environment (please complete the following information):

  • Version v22.3.0
@pobk pobk changed the title Sanic "disappears" data submitted to it request.form uses urllib.parse_qs which does not persist blank values by default Apr 10, 2022
@pobk pobk changed the title request.form uses urllib.parse_qs which does not persist blank values by default request.form uses urllib.parse_qs which does not persist blank values by default when requests submit application/x-www-form-urlencoded data Apr 10, 2022
@sjsadowski
Copy link
Contributor

@pobk Thanks for the heads up. I agree that we should not be dropping blank values as the parameter in and of itself is a value that may be necessary.

@sjsadowski sjsadowski added the bug label Apr 12, 2022
@sjsadowski sjsadowski self-assigned this Apr 12, 2022
@sjsadowski sjsadowski modified the milestones: v21.12, Future Release, v22.6 Apr 12, 2022
@prryplatypus
Copy link
Member

Just popping here to say that changing its behaviour now is probably a breaking change.

A different approach to avoid sudden breaking changes could be adding a method like get_query_args but for get_form_args, to which users can pass arguments for the parse_qs method.

If we want to change the behaviour of the .form attribute, we could also add a warning when accessing the .form attribute warning users that from a certain version we will no longer omit blank values by default, although in that case we would probably want to do the same with any other attributes (e.g.: query strings) for consistence.

@sjsadowski
Copy link
Contributor

This can be done as a non-breaking change where the behavior defaults to what it is currently, which should be fine. Already working on it, just need some tests before I submit a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants