QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370

helllamer · 2020-07-01T08:48:36Z

Fixes #10369 - Let's DO URLEncode.encode() for all queryString keys in QSB.unbind().
Unify unbind() behaviour over all query-string binders.

Currently, different binders handle qs-keys differently (look at res3):

$ sbt console
import play.api.mvc.QueryStringBindable

scala> QueryStringBindable.bindableChar.unbind( "items[1]", '1' )
val res1: String = items[1]=1

scala> QueryStringBindable.bindableInt.unbind( "items[1]", 1 )
val res2: String = items[1]=1

scala> QueryStringBindable.bindableString.unbind( "items[1]", "1" )
val res3: String = items%5B1%5D=1

scala> QueryStringBindable.bindableDouble.unbind( "items[1]", 1 )
val res4: String = items[1]=1.0

Special characters like [, ] and others MUST be Form-URL-encoded according to RFC.
Added documentation about applying URLEncode for handy-implemented unbinders.
Added tests for several common types of QueryStringBindables.

Pull Request Checklist

Have you read How to write the perfect pull request?
Have you read through the contributor guidelines?
Have you signed the Lightbend CLA?
Have you referenced any issues you're fixing using commit message keywords?
Have you added copyright headers to new files?
Have you checked that both Scala and Java APIs are updated?
Have you updated the documentation for both Scala and Java sections?
Have you added tests for any changed functionality?

Helpful things

Fixes

Fixes #10369

core/play/src/test/scala/play/api/mvc/BindersSpec.scala

helllamer · 2020-07-01T12:29:45Z

PR has been rewritten from scratch.
Now, all qs-binders using URLEncode.encode() against all queryString keys.
Tests extended.
Documentation extended.

ihostage · 2020-07-01T18:11:51Z

core/play/src/main/scala/play/api/mvc/Binders.scala

@@ -309,8 +319,7 @@ object QueryStringBindable {

    // Use an option here in case users call index(null) in the routes -- see #818
    def unbind(key: String, value: String) =
-      URLEncoder.encode(Option(key).getOrElse(""), "utf-8") + "=" + URLEncoder
-        .encode(Option(value).getOrElse(""), "utf-8")
+      _urlEncode(key) + "=" + Option(value).fold("")(_urlEncode)


Why Option dropped for key? 🤔 He is no longer needed?

Possibly, it never needed.
Option(key).getOrElse() added year ago in #9403 (diff) during fixing URLEncode for values.

In Binders.scala there are no Option(key) for any other binders (Char, Long, Int, UUID, etc).

There are zero tests for such behavior, where parsed queryString contains null keys with String-only values.

Generated Router.scala can't pass nulls as parameter names.

null as keys in scala Map[String,_] must always throw NPE. It indicates serious problem in somewhere outer logic.

@ihostage Этот Option(key) появился, т.к. код поддержки URLEncode просто скопипастили из value-кода в той же строке. Для value действительно нужен Option(), о чём в комменте там выше написано.

helllamer · 2020-07-15T14:33:14Z

@mkurz @ihostage I don't understand, is reviews completed ok? Is it time to remove status:block-merge flag?

ihostage · 2020-07-15T15:05:22Z

@helllamer Sorry, I'm not a maintainer and can't edit PR 🤷‍♂️

mkurz · 2020-07-21T21:12:35Z

Sorry, I am too busy these days... Maybe @ignasi35 and/or @renatocaval can review?

Added docs for safety implementing own custom QSBs. Tests for several other QSB implementations. Fixes playframework#10369

mkurz · 2021-02-08T20:25:02Z

@helllamer I had an intense look again at this pull request and it looks good to me. Actually I think it's a necessary fix and want to include it in the next Play 2.8.8 release. However I would like to push some small fixes to your patch (documentation wording and one more test).
Could you please give me access to your branch by selecting "Allow edits by maintainers" at the right side of the pull request?
Thanks!

helllamer · 2021-02-08T21:01:52Z

@mkurz Glad to hear you.
I've added you with WRITE access to suggestio:master branch on github.
Because i cannot find such checkbox, possibly due to using of master branch name.

mkurz · 2021-02-08T21:46:04Z

@Mergifyio backport 2.8.x

mergify · 2021-02-08T21:46:21Z

Command backport 2.8.x: pending

Waiting for the pull request to get merged

mkurz · 2021-02-08T21:47:18Z

@helllamer Thanks, I pushed two commits. Let's see what Travis says.

mergify · 2021-02-09T00:59:22Z

Command backport 2.8.x: success

Backports have been created

#10694 QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! (bp #10370) has been created for branch 2.8.x

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! (bp #10370)

mkurz added the status:block-merge label Jul 1, 2020

mkurz reviewed Jul 1, 2020

View reviewed changes

core/play/src/test/scala/play/api/mvc/BindersSpec.scala Show resolved Hide resolved

helllamer force-pushed the master branch from ab791ab to 78e5984 Compare July 1, 2020 12:22

helllamer changed the title ~~Binders.bindableString.unbind(): Do not URLEncode qs-keys.~~ QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! Jul 1, 2020

helllamer force-pushed the master branch 7 times, most recently from 8fea400 to e2a1171 Compare July 1, 2020 17:33

ihostage reviewed Jul 1, 2020

View reviewed changes

helllamer force-pushed the master branch from e2a1171 to 85f133e Compare July 23, 2020 08:22

mkurz added this to the Play 2.9.0 milestone Sep 2, 2020

QueryStringBindables.unbind(): Do URLEncode qs-keys always.

03539bd

Added docs for safety implementing own custom QSBs. Tests for several other QSB implementations. Fixes playframework#10369

helllamer force-pushed the master branch from 85f133e to 03539bd Compare September 11, 2020 20:00

mkurz modified the milestones: 2.9.0, 2.8.8 Feb 7, 2021

Also test value encoding

ce550e2

mkurz added status:merge-when-green and removed status:block-merge labels Feb 8, 2021

Documentation wording

40c9413

mkurz force-pushed the master branch from ac35e91 to 40c9413 Compare February 8, 2021 22:17

mkurz approved these changes Feb 8, 2021

View reviewed changes

mergify bot merged commit b2645bd into playframework:master Feb 9, 2021

mergify bot mentioned this pull request Feb 9, 2021

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! (bp #10370) #10694

Merged

mergify bot removed the status:merge-when-green label Feb 9, 2021

mergify bot added a commit that referenced this pull request Feb 9, 2021

Merge pull request #10694 from playframework/mergify/bp/2.8.x/pr-10370

0f45dd9

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! (bp #10370)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370

helllamer commented Jul 1, 2020 •

edited

Loading

helllamer commented Jul 1, 2020 •

edited

Loading

ihostage Jul 1, 2020

helllamer Jul 1, 2020

helllamer Jul 1, 2020 •

edited

Loading

ihostage Jul 2, 2020

helllamer commented Jul 15, 2020

ihostage commented Jul 15, 2020

mkurz commented Jul 21, 2020 •

edited

Loading

mkurz commented Feb 8, 2021

helllamer commented Feb 8, 2021

mkurz commented Feb 8, 2021

mergify bot commented Feb 8, 2021

mkurz commented Feb 8, 2021

mergify bot commented Feb 9, 2021

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370

QueryStringBindable.unbind(): Do-do-do URLEncode for all queryString keys! #10370

Conversation

helllamer commented Jul 1, 2020 • edited Loading

Pull Request Checklist

Helpful things

Fixes

helllamer commented Jul 1, 2020 • edited Loading

ihostage Jul 1, 2020

Choose a reason for hiding this comment

helllamer Jul 1, 2020

Choose a reason for hiding this comment

helllamer Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

ihostage Jul 2, 2020

Choose a reason for hiding this comment

helllamer commented Jul 15, 2020

ihostage commented Jul 15, 2020

mkurz commented Jul 21, 2020 • edited Loading

mkurz commented Feb 8, 2021

helllamer commented Feb 8, 2021

mkurz commented Feb 8, 2021

mergify bot commented Feb 8, 2021

mkurz commented Feb 8, 2021

mergify bot commented Feb 9, 2021

helllamer commented Jul 1, 2020 •

edited

Loading

helllamer commented Jul 1, 2020 •

edited

Loading

helllamer Jul 1, 2020 •

edited

Loading

mkurz commented Jul 21, 2020 •

edited

Loading