Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.3.28
    • Fix Version/s: 2.3.31, 2.5.5
    • Component/s: None
    • Labels:
      None

      Description

      With this JSP snippet using Struts 2 taglib:

      myParameter=[<s:property value="%{#parameters['myParameter']}"/>]
      <br/>
      <s:url action="url" includeParams="get"/>
      <br/>
      <s:url action="url">
          <s:param name="myParameter" value="%{#parameters['myParameter']}"/>
      </s:url>
      

      When the action is called with a parameter containing a space (which is escaped in the URL) :

      localhost:8080/url?myParameter=with+space
      

      The output is as follows :

      myParameter=[with space]
      /url.action?myParameter=with%2Bspace
      /url.action?myParameter=with+space    
      

      The includeParams attribute of the “url” tag seems to transform the "space" character into the "plus" character (both in their url-encoded forms).

      1. Struts2UrlTest.zip
        5 kB
        Pierre-Yves Soblet

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user cnenning commented on the issue:

          https://github.com/apache/struts/pull/108

          > How would this be fixed now?

          I fixed it already just by running `git cherry-pick`. (there is a jira comment about it)

          I feared that monstrous issue (WW-4628(https://issues.apache.org/jira/browse/WW-4628)) would get another round of discussion and was relieved that it was just an accident :wink:

          Show
          githubbot ASF GitHub Bot added a comment - Github user cnenning commented on the issue: https://github.com/apache/struts/pull/108 > How would this be fixed now? I fixed it already just by running `git cherry-pick`. (there is a jira comment about it) I feared that monstrous issue ( WW-4628 ( https://issues.apache.org/jira/browse/WW-4628 )) would get another round of discussion and was relieved that it was just an accident :wink:
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user gregh3269 commented on the issue:

          https://github.com/apache/struts/pull/108

          Revert "WW-4628: proper decoding of parameters in query-string"

          Sorry, this was a bad by not really understanding git and git making
          automatic commits, it should not have modified any other code. Especially not the revert.

          How would this be fixed now?

          Show
          githubbot ASF GitHub Bot added a comment - Github user gregh3269 commented on the issue: https://github.com/apache/struts/pull/108 Revert " WW-4628 : proper decoding of parameters in query-string" Sorry, this was a bad by not really understanding git and git making automatic commits, it should not have modified any other code. Especially not the revert. How would this be fixed now?
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Struts-JDK7-master #533 (See https://builds.apache.org/job/Struts-JDK7-master/533/)
          WW-4628: proper decoding of parameters in query-string (cnenning: rev 9b38ad8fcf20bdbdb6baacd5812bcf196d80ea07)

          • (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          • (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-JDK7-master #533 (See https://builds.apache.org/job/Struts-JDK7-master/533/ ) WW-4628 : proper decoding of parameters in query-string (cnenning: rev 9b38ad8fcf20bdbdb6baacd5812bcf196d80ea07) (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Hide
          cn42 Christoph Nenning added a comment -

          Revert "WW-4628: proper decoding of parameters in query-string"

          Obviously the reverting was by accident, so I reverted the revert

          Show
          cn42 Christoph Nenning added a comment - Revert " WW-4628 : proper decoding of parameters in query-string" Obviously the reverting was by accident, so I reverted the revert
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 9b38ad8fcf20bdbdb6baacd5812bcf196d80ea07 in struts's branch refs/heads/master from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9b38ad8 ]

          WW-4628: proper decoding of parameters in query-string

          Show
          jira-bot ASF subversion and git services added a comment - Commit 9b38ad8fcf20bdbdb6baacd5812bcf196d80ea07 in struts's branch refs/heads/master from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=9b38ad8 ] WW-4628 : proper decoding of parameters in query-string
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build Struts-JDK7-master #531 (See https://builds.apache.org/job/Struts-JDK7-master/531/)
          Revert "WW-4628: proper decoding of parameters in query-string" (gregh3269: rev 51a49201adf73e33ba68d533f3535a32f507b531)

          • (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          • (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          • (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
            Revert "WW-4628: proper decoding of parameters in query-string" (lukaszlenart: rev d9fe1406ee287226704fdd5e3a53d59e6b0a556f)
          • (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          • (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build Struts-JDK7-master #531 (See https://builds.apache.org/job/Struts-JDK7-master/531/ ) Revert " WW-4628 : proper decoding of parameters in query-string" (gregh3269: rev 51a49201adf73e33ba68d533f3535a32f507b531) (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java Revert " WW-4628 : proper decoding of parameters in query-string" (lukaszlenart: rev d9fe1406ee287226704fdd5e3a53d59e6b0a556f) (edit) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java (edit) core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java (edit) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java (edit) core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 51a49201adf73e33ba68d533f3535a32f507b531 in struts's branch refs/heads/master from Greg Huber
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=51a4920 ]

          Revert "WW-4628: proper decoding of parameters in query-string"

          This reverts commit ef9c66118ede16f3ff239ea864641d5bdadeecae.

          Show
          jira-bot ASF subversion and git services added a comment - Commit 51a49201adf73e33ba68d533f3535a32f507b531 in struts's branch refs/heads/master from Greg Huber [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=51a4920 ] Revert " WW-4628 : proper decoding of parameters in query-string" This reverts commit ef9c66118ede16f3ff239ea864641d5bdadeecae.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit d9fe1406ee287226704fdd5e3a53d59e6b0a556f in struts's branch refs/heads/master from Greg Huber
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=d9fe140 ]

          Revert "WW-4628: proper decoding of parameters in query-string"

          This reverts commit ef9c66118ede16f3ff239ea864641d5bdadeecae.

          Show
          jira-bot ASF subversion and git services added a comment - Commit d9fe1406ee287226704fdd5e3a53d59e6b0a556f in struts's branch refs/heads/master from Greg Huber [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=d9fe140 ] Revert " WW-4628 : proper decoding of parameters in query-string" This reverts commit ef9c66118ede16f3ff239ea864641d5bdadeecae.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          WW-4692 to not forget

          Show
          lukaszlenart Lukasz Lenart added a comment - WW-4692 to not forget
          Hide
          cn42 Christoph Nenning added a comment -

          sounds like a plan

          Show
          cn42 Christoph Nenning added a comment - sounds like a plan
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          That's why I called it hell But we can deal with it by extracting small pieces of code and adding alternative implementations - you can have your own UrlProvider but this is huge thing, I thought about extracting the whole encoding logic into a bean UrlEncoder with default implementation which will match existing behaviour plus adding some other implementations.

          Show
          lukaszlenart Lukasz Lenart added a comment - That's why I called it hell But we can deal with it by extracting small pieces of code and adding alternative implementations - you can have your own UrlProvider but this is huge thing, I thought about extracting the whole encoding logic into a bean UrlEncoder with default implementation which will match existing behaviour plus adding some other implementations.
          Hide
          cn42 Christoph Nenning added a comment -

          I fear that could easily break apps. IMO there are some bad defaults for URL-Building (like the 2 things mentioned in some comments of this issue). Existing apps are forced to deal with those defaults. If we change defaults it will break apps

          Show
          cn42 Christoph Nenning added a comment - I fear that could easily break apps. IMO there are some bad defaults for URL-Building (like the 2 things mentioned in some comments of this issue). Existing apps are forced to deal with those defaults. If we change defaults it will break apps
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Hm... I think it's time to clean up UrlProvider and UrlHelper's hell :\

          Show
          lukaszlenart Lukasz Lenart added a comment - Hm... I think it's time to clean up UrlProvider and UrlHelper 's hell :\
          Hide
          cn42 Christoph Nenning added a comment - - edited

          Now when I want to define two params in <s:url/> tag, the & is replaced with & amp; which is invalid :/

          As far as I can tell that behavior exists since some years. It is caused by setting setEscapeAmp() to true by default in org.apache.struts2.components.URL.

          I double checked with one of my apps which uses 2.3.28.1. Obviously browsers are ignoring that error in URLs. When looking at them in browser debug tools they are shown correctly but when using "view source" in browser it shows & amp; instead of &.

          Show
          cn42 Christoph Nenning added a comment - - edited Now when I want to define two params in <s:url/> tag, the & is replaced with & amp; which is invalid :/ As far as I can tell that behavior exists since some years. It is caused by setting setEscapeAmp() to true by default in org.apache.struts2.components.URL . I double checked with one of my apps which uses 2.3.28.1. Obviously browsers are ignoring that error in URLs. When looking at them in browser debug tools they are shown correctly but when using "view source" in browser it shows & amp; instead of & .
          Hide
          cn42 Christoph Nenning added a comment -

          I'll look into it today

          Show
          cn42 Christoph Nenning added a comment - I'll look into it today
          Hide
          lukaszlenart Lukasz Lenart added a comment - - edited

          Now when I want to define two params in <s:url/> tag, the & is replaced with & amp; which is invalid :/

          <s:url action="url">
              <s:param name="param1" value="1"/>
              <s:param name="param2" value="2"/>
          </s:url>
          

          will produce

          /url?param1=1&amp;param2=2
          
          Show
          lukaszlenart Lukasz Lenart added a comment - - edited Now when I want to define two params in <s:url/> tag, the & is replaced with & amp; which is invalid :/ <s:url action= "url" > <s:param name= "param1" value= "1" /> <s:param name= "param2" value= "2" /> </s:url> will produce /url?param1=1&amp;param2=2
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK6-support-2.3 #1043 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1043/)
          merged fix for WW-4628 (proper url decoding of query-string) (cnenning: rev 7d8c3598ef4a4699515a2b8e1064d4e9b921a58b)

          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-support-2.3 #1043 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1043/ ) merged fix for WW-4628 (proper url decoding of query-string) (cnenning: rev 7d8c3598ef4a4699515a2b8e1064d4e9b921a58b) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK7-master #507 (See https://builds.apache.org/job/Struts-JDK7-master/507/)
          WW-4628: proper decoding of parameters in query-string (cnenning: rev ef9c66118ede16f3ff239ea864641d5bdadeecae)

          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java
          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #507 (See https://builds.apache.org/job/Struts-JDK7-master/507/ ) WW-4628 : proper decoding of parameters in query-string (cnenning: rev ef9c66118ede16f3ff239ea864641d5bdadeecae) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java core/src/test/java/org/apache/struts2/util/URLDecoderUtilTest.java core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java core/src/main/java/org/apache/struts2/util/URLDecoderUtil.java
          Hide
          cn42 Christoph Nenning added a comment -

          As explained in comments: it is a difference whether a query-string or other parts of an URL are decoded.

          Show
          cn42 Christoph Nenning added a comment - As explained in comments: it is a difference whether a query-string or other parts of an URL are decoded.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 7d8c3598ef4a4699515a2b8e1064d4e9b921a58b in struts's branch refs/heads/support-2-3 from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7d8c359 ]

          merged fix for WW-4628 (proper url decoding of query-string)

          Show
          jira-bot ASF subversion and git services added a comment - Commit 7d8c3598ef4a4699515a2b8e1064d4e9b921a58b in struts's branch refs/heads/support-2-3 from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=7d8c359 ] merged fix for WW-4628 (proper url decoding of query-string)
          Hide
          cn42 Christoph Nenning added a comment -

          Tomcat's decoder implementation has two modes:

          • decode query string (decodes '+' character)
          • decode other parts of URL (not decodes '+' character)

          Mode is triggered by method parameter isQuery which was not set by struts. My change is to set that flag for DefaultUrlHelper.parseQueryString().

          Show
          cn42 Christoph Nenning added a comment - Tomcat's decoder implementation has two modes: decode query string (decodes '+' character) decode other parts of URL (not decodes '+' character) Mode is triggered by method parameter isQuery which was not set by struts. My change is to set that flag for DefaultUrlHelper.parseQueryString() .
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit ef9c66118ede16f3ff239ea864641d5bdadeecae in struts's branch refs/heads/master from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ef9c661 ]

          WW-4628: proper decoding of parameters in query-string

          Show
          jira-bot ASF subversion and git services added a comment - Commit ef9c66118ede16f3ff239ea864641d5bdadeecae in struts's branch refs/heads/master from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ef9c661 ] WW-4628 : proper decoding of parameters in query-string
          Hide
          cn42 Christoph Nenning added a comment -

          According to commit history the decoding implementation of tomcat has been copied to fix WW-4507.

          Show
          cn42 Christoph Nenning added a comment - According to commit history the decoding implementation of tomcat has been copied to fix WW-4507 .
          Hide
          cn42 Christoph Nenning added a comment -

          This approach brought me much enlightenment

          When <s:url includeParams=""/> is used, it takes existing params from HttpServletRequest. That class provides methods like getParameterMap() or getParameterValues() (among others). Those return parameters decoded. So no issue with them.

          But as struts wants to explicitly handle GET parameters only it uses a different approach (in some cases): HttpServletRequest().getQueryString() (or: HttpServletRequest().getAttribute("javax.servlet.forward.query_string")). When paraters are accessed in this way they are not decoded.

          Existing struts code is already prepared for that and does decoding for exactly these parameters. So it does already what I was about to add.

          But here comes the other issue mentioned: URLDecoderUtil.decode() does not handle the + character for spaces.

          So finally the decoding issue is the only issue.

          Show
          cn42 Christoph Nenning added a comment - This approach brought me much enlightenment When <s:url includeParams=""/> is used, it takes existing params from HttpServletRequest . That class provides methods like getParameterMap() or getParameterValues() (among others). Those return parameters decoded. So no issue with them. But as struts wants to explicitly handle GET parameters only it uses a different approach (in some cases): HttpServletRequest().getQueryString() (or: HttpServletRequest().getAttribute("javax.servlet.forward.query_string") ). When paraters are accessed in this way they are not decoded. Existing struts code is already prepared for that and does decoding for exactly these parameters. So it does already what I was about to add. But here comes the other issue mentioned: URLDecoderUtil.decode() does not handle the + character for spaces. So finally the decoding issue is the only issue.
          Hide
          cn42 Christoph Nenning added a comment -

          My new plan is to change ServletUrlRenderer to add existing parameters in decoded form only.

          Show
          cn42 Christoph Nenning added a comment - My new plan is to change ServletUrlRenderer to add existing parameters in decoded form only.
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK6-support-2.3 #1042 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1042/)
          WW-4628: new issues were introduced by last change, restoring old (cnenning: rev 83bb64f4c232348c4162ad2272c7eae1855a7362)

          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK6-support-2.3 #1042 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1042/ ) WW-4628 : new issues were introduced by last change, restoring old (cnenning: rev 83bb64f4c232348c4162ad2272c7eae1855a7362) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Struts-JDK7-master #506 (See https://builds.apache.org/job/Struts-JDK7-master/506/)
          WW-4628: new issues were introduced by last change, restoring old (cnenning: rev a6b33018561f12724f89aa28f7849f3f2fc34092)

          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Struts-JDK7-master #506 (See https://builds.apache.org/job/Struts-JDK7-master/506/ ) WW-4628 : new issues were introduced by last change, restoring old (cnenning: rev a6b33018561f12724f89aa28f7849f3f2fc34092) core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          Hide
          cn42 Christoph Nenning added a comment -

          To make the API break more clear: the existing behavior is to encode paramters when encode=false, which is default.

          Show
          cn42 Christoph Nenning added a comment - To make the API break more clear: the existing behavior is to encode paramters when encode=false , which is default.
          Hide
          cn42 Christoph Nenning added a comment -

          REPOENED as 'fix' caused new issues

          Show
          cn42 Christoph Nenning added a comment - REPOENED as 'fix' caused new issues
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit 83bb64f4c232348c4162ad2272c7eae1855a7362 in struts's branch refs/heads/support-2-3 from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=83bb64f ]

          WW-4628: new issues were introduced by last change, restoring old behavior and ignoring new tests

          Show
          jira-bot ASF subversion and git services added a comment - Commit 83bb64f4c232348c4162ad2272c7eae1855a7362 in struts's branch refs/heads/support-2-3 from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=83bb64f ] WW-4628 : new issues were introduced by last change, restoring old behavior and ignoring new tests
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit a6b33018561f12724f89aa28f7849f3f2fc34092 in struts's branch refs/heads/master from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=a6b3301 ]

          WW-4628: new issues were introduced by last change, restoring old behavior and ignoring new tests

          Show
          jira-bot ASF subversion and git services added a comment - Commit a6b33018561f12724f89aa28f7849f3f2fc34092 in struts's branch refs/heads/master from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=a6b3301 ] WW-4628 : new issues were introduced by last change, restoring old behavior and ignoring new tests
          Hide
          cn42 Christoph Nenning added a comment -

          That could be considered an API breaking change as <s:url encode="" would behave differently than before. An alternative could be to introduce a new attribute like <s:url encodeParameters="".

          Show
          cn42 Christoph Nenning added a comment - That could be considered an API breaking change as <s:url encode="" would behave differently than before. An alternative could be to introduce a new attribute like <s:url encodeParameters="" .
          Hide
          cn42 Christoph Nenning added a comment -

          Not as easy as I thought.

          By looking at it again I think it makes to sense to encode complete URL in DefaultUrlHelper.buildUrl() as it is done right now. Instead just parameters should be encoded.

          Show
          cn42 Christoph Nenning added a comment - Not as easy as I thought. By looking at it again I think it makes to sense to encode complete URL in DefaultUrlHelper.buildUrl() as it is done right now. Instead just parameters should be encoded.
          Hide
          hudson Hudson added a comment -

          UNSTABLE: Integrated in Struts-JDK6-support-2.3 #1041 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1041/)
          merged fix for WW-4628 (avoid double encoding of url parameters) (cnenning: rev ae2840f18386492530de1fb875ca9f8804adbb3e)

          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          Show
          hudson Hudson added a comment - UNSTABLE: Integrated in Struts-JDK6-support-2.3 #1041 (See https://builds.apache.org/job/Struts-JDK6-support-2.3/1041/ ) merged fix for WW-4628 (avoid double encoding of url parameters) (cnenning: rev ae2840f18386492530de1fb875ca9f8804adbb3e) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          Show
          lukaszlenart Lukasz Lenart added a comment - Some tests are failing https://builds.apache.org/job/Struts-JDK7-master/lastCompletedBuild/testReport/
          Hide
          cn42 Christoph Nenning added a comment -

          has been merged to support-2-3 as well

          Show
          cn42 Christoph Nenning added a comment - has been merged to support-2-3 as well
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit ae2840f18386492530de1fb875ca9f8804adbb3e in struts's branch refs/heads/support-2-3 from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ae2840f ]

          merged fix for WW-4628 (avoid double encoding of url parameters)

          Show
          jira-bot ASF subversion and git services added a comment - Commit ae2840f18386492530de1fb875ca9f8804adbb3e in struts's branch refs/heads/support-2-3 from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=ae2840f ] merged fix for WW-4628 (avoid double encoding of url parameters)
          Hide
          hudson Hudson added a comment -

          UNSTABLE: Integrated in Struts-JDK7-master #504 (See https://builds.apache.org/job/Struts-JDK7-master/504/)
          WW-4628: avoid double encoding of url parameters, e.g. when spaces are (cnenning: rev af50018f11d4fdfa8ac04a55f942a4973cb15c87)

          • core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java
          • core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          Show
          hudson Hudson added a comment - UNSTABLE: Integrated in Struts-JDK7-master #504 (See https://builds.apache.org/job/Struts-JDK7-master/504/ ) WW-4628 : avoid double encoding of url parameters, e.g. when spaces are (cnenning: rev af50018f11d4fdfa8ac04a55f942a4973cb15c87) core/src/main/java/org/apache/struts2/views/util/DefaultUrlHelper.java core/src/test/java/org/apache/struts2/views/util/DefaultUrlHelperTest.java
          Hide
          cn42 Christoph Nenning added a comment -

          I was sure somebody would ask for it. Did not expect it so quick

          Yes, I will do it.

          Show
          cn42 Christoph Nenning added a comment - I was sure somebody would ask for it. Did not expect it so quick Yes, I will do it.
          Hide
          lukaszlenart Lukasz Lenart added a comment -

          Christoph Nenning shouldn't we backport this into 2.3.31?

          Show
          lukaszlenart Lukasz Lenart added a comment - Christoph Nenning shouldn't we backport this into 2.3.31?
          Hide
          cn42 Christoph Nenning added a comment -

          Has been fixed in master branch, testcases were added.

          Show
          cn42 Christoph Nenning added a comment - Has been fixed in master branch, testcases were added.
          Hide
          jira-bot ASF subversion and git services added a comment -

          Commit af50018f11d4fdfa8ac04a55f942a4973cb15c87 in struts's branch refs/heads/master from cnenning
          [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=af50018 ]

          WW-4628: avoid double encoding of url parameters, e.g. when spaces are present in parameter values

          Show
          jira-bot ASF subversion and git services added a comment - Commit af50018f11d4fdfa8ac04a55f942a4973cb15c87 in struts's branch refs/heads/master from cnenning [ https://git-wip-us.apache.org/repos/asf?p=struts.git;h=af50018 ] WW-4628 : avoid double encoding of url parameters, e.g. when spaces are present in parameter values
          Hide
          cn42 Christoph Nenning added a comment -

          Please create another issue for that. As that uses a class called UDecoder from tomcat it might be necessary to copy a newer version from tomcat.

          Show
          cn42 Christoph Nenning added a comment - Please create another issue for that. As that uses a class called UDecoder from tomcat it might be necessary to copy a newer version from tomcat.
          Hide
          cn42 Christoph Nenning added a comment -

          Recently I had an issue with <s:url includeParams="get" encode="false" />, too. In my case it was also a parameter with spaces. That parameter is already encoded and gets encoded a 2nd time. That causes problems when generated URL is used.

          Creation of URL happens in class DefaultUrlHelper. The method buildParameterSubstring(String name, String value) always encodes name and value, without checking encode parameter (in this class it is called encodeResult).

          IMHO the issue is that DefaultUrlHelper has two use cases:

          • Invoked with buildUrl(). In that case the parameter encodeResult is present and taken into account for the whole url. But parameters might have been already encoded.
          • Invoked with buildParametersString(). In that case no encoding parameter is present and it gets always encoded.

          I'm going to add another version of buildParametersString() which has an encoding parameter. The default will still be to encode.

          Show
          cn42 Christoph Nenning added a comment - Recently I had an issue with <s:url includeParams="get" encode="false" /> , too. In my case it was also a parameter with spaces. That parameter is already encoded and gets encoded a 2nd time. That causes problems when generated URL is used. Creation of URL happens in class DefaultUrlHelper . The method buildParameterSubstring(String name, String value) always encodes name and value, without checking encode parameter (in this class it is called encodeResult ). IMHO the issue is that DefaultUrlHelper has two use cases: Invoked with buildUrl() . In that case the parameter encodeResult is present and taken into account for the whole url. But parameters might have been already encoded. Invoked with buildParametersString() . In that case no encoding parameter is present and it gets always encoded. I'm going to add another version of buildParametersString() which has an encoding parameter. The default will still be to encode.
          Hide
          pys Pierre-Yves Soblet added a comment -

          I have tested the attached sample with previous Struts 2 releases from Maven Central and it appears that the most recent relase (before 2.3.28.1) that is NOT affected by this issue is 2.3.24.3.

          There is a behavioral difference between URLDecoderUtil from Struts 2 and URLDecoder from java.net.

              @Test
              public void shouldDecodeSpace() throws UnsupportedEncodingException {
                  String queryStringWithSpace = "a+b";
                  Assert.assertEquals(
                          java.net.URLDecoder.decode(queryStringWithSpace, StandardCharsets.UTF_8.displayName()),
                          org.apache.struts2.util.URLDecoderUtil.decode(queryStringWithSpace, StandardCharsets.UTF_8.displayName()));
              }
          

          yields :

          	org.junit.ComparisonFailure: expected:<a[ ]b> but was:<a[+]b>
          
          Show
          pys Pierre-Yves Soblet added a comment - I have tested the attached sample with previous Struts 2 releases from Maven Central and it appears that the most recent relase (before 2.3.28.1) that is NOT affected by this issue is 2.3.24.3. There is a behavioral difference between URLDecoderUtil from Struts 2 and URLDecoder from java.net. @Test public void shouldDecodeSpace() throws UnsupportedEncodingException { String queryStringWithSpace = "a+b" ; Assert.assertEquals( java.net.URLDecoder.decode(queryStringWithSpace, StandardCharsets.UTF_8.displayName()), org.apache.struts2.util.URLDecoderUtil.decode(queryStringWithSpace, StandardCharsets.UTF_8.displayName())); } yields : org.junit.ComparisonFailure: expected:<a[ ]b> but was:<a[+]b>
          Hide
          pys Pierre-Yves Soblet added a comment -

          Hi victorsosa,

          I would expect both URLs of my example to be the same.

          Suppose I have a site with two search actions searchSimple and searchExtended that both take a single parameter searchCriteria. In the first search page, a link is built referring to the second search page using <s:a> or <s:url> tag with includeParams to propagate the searchCriteria value (present in the URL) from one page to the other.
          If in my first search page, the searchCriteria value contains a space (as entered by the user in a search form), going to the second search page via the link will change the value (the space in the first search form becomes a plus in the second search form). It is apparent once you put an input field displaying the value.

          Show
          pys Pierre-Yves Soblet added a comment - Hi victorsosa, I would expect both URLs of my example to be the same. Suppose I have a site with two search actions searchSimple and searchExtended that both take a single parameter searchCriteria. In the first search page, a link is built referring to the second search page using <s:a> or <s:url> tag with includeParams to propagate the searchCriteria value (present in the URL) from one page to the other. If in my first search page, the searchCriteria value contains a space (as entered by the user in a search form), going to the second search page via the link will change the value (the space in the first search form becomes a plus in the second search form). It is apparent once you put an input field displaying the value.
          Hide
          victorsosa victorsosa added a comment -

          Hi,

          Thanks for the example;

          Of course includeParams is encoding the parameters for the URL.

          sorry but I didn't catch the issue, what is the issue?

          Show
          victorsosa victorsosa added a comment - Hi, Thanks for the example; Of course includeParams is encoding the parameters for the URL. sorry but I didn't catch the issue, what is the issue?
          Hide
          pys Pierre-Yves Soblet added a comment -

          I attach a sample that illustrates the issue.
          You can execute it with:

          mvn jetty:run
          
          Show
          pys Pierre-Yves Soblet added a comment - I attach a sample that illustrates the issue. You can execute it with: mvn jetty:run

            People

            • Assignee:
              cn42 Christoph Nenning
              Reporter:
              pys Pierre-Yves Soblet
            • Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development