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

encode form data for chars like '&' and '=' #418

Conversation

RolandStuder
Copy link
Contributor

Bug fix for misshandling form data

Description

Form Submissions with & and = do not get encoded properly so if someone inputs a value like "New & Shiny" the resulting transmitted string for the formSerialization will be field=New & Shiny, which will translate into params like {..., field: "New ", shiny: nil

Why should this be added

Because this is thoroughly broken, any SR app using form params and getting user input containing special chars like & and = might get unexpected result, as the params will be weird.

Notes

This was working earlier on, but the encoding was lost in some refactor. Probably we should add some tests, but I got zero experience with that, if anyone would pair on that with me, I would be happy to help.

Could not quite figure it out with what commit, but this line was refactored in a way where econding was forgotten: 45d2514#diff-447e27554532708369d88cce566edf8219f75dfd4fabbdae20de84c606a0fb96R17

Checklist

  • My code follows the style guidelines of this project
  • Checks (StandardRB & Prettier-Standard) are passing
  • This is not a documentation update

@hopsoft
Copy link
Contributor

hopsoft commented Jan 6, 2021

Looks like we have a failing test. Happy to merge once things are passing.

@marcoroth
Copy link
Member

Thanks for looking into this @RolandStuder!

I don't know why I removed the line here but this is of course needed. In the end this was never merged to master with #327.

As long as you don't use & and = in your form values or names you don't have to serialize the values. But given we don't know what value the input has we should serialize it all the time.

The backend part can still handle both, but we should always send it url encoded. So we don't need to change anything on the backend and there is no issue with backwards-compabiltity which is good!

I added some tests and rewrote the existing ones. I'll open a PR against your fork @RolandStuder!

@RolandStuder
Copy link
Contributor Author

@marcoroth Thx for the test fixes and not forgetting about the buttons :-)

@hopsoft Marco fixed things up, so it is ready to merge.

@jonathan-s
Copy link
Contributor

Just a question here, since we are sending the url encoded data to the backend. Does it also mean that the backend needs to decode the data?

@RolandStuder
Copy link
Contributor Author

@jonathan-s No, the AC channel receives the data already decoded.

This was a regression and should actually not concern SR user (well other than maybe running into the bug, but library user don't have to do any encoding/decoding.

@marcoroth
Copy link
Member

marcoroth commented Jan 6, 2021

Just a question here, since we are sending the url encoded data to the backend. Does it also mean that the backend needs to decode the data?

Yes, that's right. In our case the rack method we are using can handle encoded and already decoded form strings, see here: https://github.com/hopsoft/stimulus_reflex/pull/327/files?#diff-c614dfa6e8c03b82e46507b8822de8845b741465a4dcfa6c6ad23fe3cc5cad13R39

As @RolandStuder pointed out there is no difference for the user. But since you are building this in Django this could have an impact if your implementation currently doesn't account for encoded strings.

@leastbad leastbad added this to the 3.4.1 milestone Jan 7, 2021
@leastbad leastbad added the bug Something isn't working label Jan 7, 2021
@hopsoft hopsoft merged commit 6004495 into stimulusreflex:master Jan 7, 2021
@RolandStuder RolandStuder deleted the fix_missing_encoding_for_form_values branch January 7, 2021 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants