Skip to content

URL-encode POST parameters in yesod-test#1617

Merged
snoyberg merged 5 commits intoyesodweb:masterfrom
league:url-encode
Aug 20, 2019
Merged

URL-encode POST parameters in yesod-test#1617
snoyberg merged 5 commits intoyesodweb:masterfrom
league:url-encode

Conversation

@league
Copy link
Copy Markdown
Contributor

@league league commented Aug 13, 2019

Addresses #1616. Includes tests that fail without the corresponding change to the code. Potentially this change could cause somebody's tests to fail, if they were already working around the issue (because URL-encoding is not idempotent).

Before submitting your PR, check that you've:

  • Bumped the version number

After submitting your PR:

  • Update the Changelog.md file with a link to your PR
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@league
Copy link
Copy Markdown
Contributor Author

league commented Aug 13, 2019

Wait – sorry, I got something wrong in the way it works for multipart/form-data. (I only ran into the bug with x-www-form-urlencoded.) Will push something new when I'm more confident!

@league
Copy link
Copy Markdown
Contributor Author

league commented Aug 13, 2019

Okay,* I think this is right (08a9632). multipart/form-data didn't need the fix, only x-www-form-urlencoded. (I fooled myself into thinking it did because of a confounding difference in one test returning text/plain and the other text/html.) Please squash when merging!

Copy link
Copy Markdown
Member

@snoyberg snoyberg left a comment

Choose a reason for hiding this comment

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

Please add a ChangeLog comment. Also, it doesn't look like the modified test case is testing both url-encoded and form data request types, am I correct?

@league
Copy link
Copy Markdown
Contributor Author

league commented Aug 19, 2019

Please add a ChangeLog comment. Also, it doesn't look like the modified test case is testing both url-encoded and form data request types, am I correct?

I believe it does test both types. The byLabel on line 160 tests passing through special characters, and that example also uses fileByLabel, which necessitates encoding using multipart/form-data.

setUrl ("/form" :: Text)
byLabel "Some Label" "12345"
byLabel "Some Label" "foo+bar%41<&baz"
fileByLabel "Some File" "test/main.hs" "text/plain"
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Here's the test case that passes through special characters with multipart/form-data. (This test did not fail previously, so the bug I'm fixing here is only with form-url-encoded. I thought it failed at one point, because the endpoint this test is accessing returns HTML instead fo text, so & comes through as &amp; etc.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got it, thanks for clarifying.

@snoyberg snoyberg merged commit d7a2997 into yesodweb:master Aug 20, 2019
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