Invalid url encoding handling #25
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for the PR. Can you ensure that the commits adhere to the contributing guidelines. |
|
@weavejester I think they generally do. Would you like for me to squash the commits and / or remove the additional tests? Or is there something else that I’m missing? Thanks. |
|
Can you change the subject of the second commit to something like: The rest of it is fine. |
Given the squashing of all exceptions in form-decode-str, the prior implementation of form-decode would return nil keys and/or values for parameters which failed to URL decode. With this change the parameters are silently dropped. Including nil parameters creates problems for downstream middleware (e.g. ring.middleware.nested-params -- see ring-clojure/ring#243) which are not expecting them, and has no basis in the spec (https://url.spec.whatwg.org/#urlencoded-parsing). One can second guess the decision to catch all decoding exceptions (see #22) rather than allowing them to bubble up (and thus potentially allow the application to return an error status), but that decision being what it is, it seems most in the spirit of the library to drop the invalid parameters. For completeness, here are the (known) URL decoding failure cases: - Illegal hex values in % escape pattern - Incomplete % escape pattern
|
@weavejester Done! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Given the squashing of all exceptions in form-decode-str, the prior implementation of form-decode would return nil keys and/or values for parameters which failed to URL decode. With this change the parameters are silently dropped.
Including nil parameters creates problems for downstream middleware (e.g. ring.middleware.nested-params -- see ring-clojure/ring#243) which are not expecting them.
One can second guess the decision to catch all decoding exceptions (see #22) rather than allowing them to bubble up (and thus potentially allow the application to return an error status), but that decision being what it is, it seems most in the spirit of the library to drop the invalid parameters.
Note the two commits, with the first locking down the existing behavior so the second can clearly show the changes in behavior.