Wicket
  1. Wicket
  2. WICKET-4425

Wicket 1.5 rewrites template content where it should not

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.4
    • Fix Version/s: 1.5.5, 6.0.0-beta1
    • Component/s: wicket
    • Labels:
      None

      Description

      I have recently upgraded from Wicket 1.4.14 to 1.5.4. One issue that I
      encountered is that <script> tags in panel templates are rewritten by
      Wicket, even when the <script> tags in question have no wicket handlers
      attached to them. I.e. the following panel template (notice that there
      are no wicket:id attributes whatsoever):

      <wicket:panel>
      <script id="template-upload" type="text/x-jquery-tmpl">
      <span>$

      {name}</span>
      </script>
      </wicket:panel>

      Would render in the panel as:

      <script id="template-upload" type="text/x-jquery-tmpl">
      /<![CDATA[/
      <span>${name}

      </span>
      /]]>/
      </script>

      Imho this is unwanted behavior that is a regression from the behavior in
      Wicket 1.4.x (or at least 1.4.14). Wicket should not add content to the
      body of the script tags (or any other tags in a template, unless their
      content is provided programmatically), as it does not have the knowledge
      how that affects the functionality of the page. Moreover, the content
      that Wicket adds to these script tags is only correct for Javascript
      (hence incorrect for the scripts in the example as they are not
      javascript). In the above example adding /*, */
      will change the functionality of the script tag. If the "/<![CDATA[/"
      part was necessary in the script tags above, they should be added by the
      person that provides the template, not magically added by Wicket.

      I have attached a quickstart that demonstrates the issue. The quickstart has a <script id="script1">Some Text</script> element in HomePage.html that (by javascript) is shown in an alert box. Because of this bug, the alert will now start with "/<![CDATA[/", while it should simply show the text. See HomePage.html in the provided quickstart.

      1. WICKET-4425.patch
        1 kB
        Martin Grigorov
      2. myproject.zip
        26 kB
        Pointbreak

        Issue Links

          Activity

          Pointbreak created issue -
          Hide
          Pointbreak added a comment -

          quickstart that demonstrates the issue

          Show
          Pointbreak added a comment - quickstart that demonstrates the issue
          Pointbreak made changes -
          Field Original Value New Value
          Attachment myproject.zip [ 12515357 ]
          Martin Grigorov made changes -
          Link This issue is broken by WICKET-3420 [ WICKET-3420 ]
          Hide
          Martin Grigorov added a comment -

          Here is a patch that wraps the content of <script> element only if its type is a plain javascript.

          Show
          Martin Grigorov added a comment - Here is a patch that wraps the content of <script> element only if its type is a plain javascript.
          Martin Grigorov made changes -
          Attachment WICKET-4425.patch [ 12515361 ]
          Martin Grigorov made changes -
          Assignee Martin Grigorov [ mgrigorov ]
          Hide
          Pointbreak added a comment -

          Martin, although your patch would probably solve my issue in this particular case, I don't think it is the correct approach. Imho the content of the <script> tags in this quickstart should not be altered by wicket in any way, even when they contain javascript. Wicket should not try to "correct" what it deems incorrect html in a template. The programmer that provides the template is responsible for that. It would be different if the script elements were attached to a wicket Component (i.e. had a wicket:id) and/or their content was programmatically added to them. But template code should be rendered as-is where not attached to wicket Components. That wicket alters content of html-elements that are not attached to Components a regression from pre 1.5 versions. I don't really understand why wicket would want to do that.

          Show
          Pointbreak added a comment - Martin, although your patch would probably solve my issue in this particular case, I don't think it is the correct approach. Imho the content of the <script> tags in this quickstart should not be altered by wicket in any way, even when they contain javascript. Wicket should not try to "correct" what it deems incorrect html in a template. The programmer that provides the template is responsible for that. It would be different if the script elements were attached to a wicket Component (i.e. had a wicket:id) and/or their content was programmatically added to them. But template code should be rendered as-is where not attached to wicket Components. That wicket alters content of html-elements that are not attached to Components a regression from pre 1.5 versions. I don't really understand why wicket would want to do that.
          Hide
          Martin Grigorov added a comment -

          I guess you didn't notice that I linked this ticket to the one that introduced this change. Check it and you'll see that we need to "help" in some cases.

          Show
          Martin Grigorov added a comment - I guess you didn't notice that I linked this ticket to the one that introduced this change. Check it and you'll see that we need to "help" in some cases.
          Hide
          Pointbreak added a comment -

          Well actually I did see that, but am still confused. My point is that if there is a verbatim <script> tag in the template and its code needs to be wrapped inside "/<![CDATA[/", this should be done by the programmer that writes the <script> tag. NOT as an afterthought by wicket. I don't see why wicket should help here. (And Wicket did not do that in older versions).

          This also applies to the issue you linked. The SCRIPT tag in that issue should itself contain the wrapping /<![CDATA[*/ ... /*]]>/ stuff. Not providing that is an error in the template, since the template can't be parsed by an XML/XHTML parser. Of course Wicket should be smart enough that if such a CDATA wrapper code is available inside contributions in the <wicket:head> section that it doesn't skip that in the context of e.g. ajax requests. But providing that wrapping code is part of the job of the template writer. I.e. I would argue that the the following template code is faulty, and to fix that is the responsibility of the template programmer:

          <wicket:head>
          <script>
          if (someVariable < 0)

          { someVariable = 0; }

          </script>
          </wicket:head>

          Show
          Pointbreak added a comment - Well actually I did see that, but am still confused. My point is that if there is a verbatim <script> tag in the template and its code needs to be wrapped inside "/ <![CDATA[ /", this should be done by the programmer that writes the <script> tag. NOT as an afterthought by wicket. I don't see why wicket should help here. (And Wicket did not do that in older versions). This also applies to the issue you linked. The SCRIPT tag in that issue should itself contain the wrapping / <![CDATA [*/ ... /*] ]> / stuff. Not providing that is an error in the template, since the template can't be parsed by an XML/XHTML parser. Of course Wicket should be smart enough that if such a CDATA wrapper code is available inside contributions in the <wicket:head> section that it doesn't skip that in the context of e.g. ajax requests. But providing that wrapping code is part of the job of the template writer. I.e. I would argue that the the following template code is faulty, and to fix that is the responsibility of the template programmer: <wicket:head> <script> if (someVariable < 0) { someVariable = 0; } </script> </wicket:head>
          Hide
          Martin Grigorov added a comment -

          The developer should not be aware of the fact that Wicket uses XML to deliver the HTML response in Ajax response. This CDATA is there exactly for this reason - to allow DOMParser (JS) to create a proper JS Document no matter what is the actual content.

          Additionally <div/> is a valid by HTML specs but some browsers don't behave well with with. And Wicket tries to help in this situation as well, by expanding it to <div></div>.

          I'll commit this patch soon if there are no technical problems with it.

          Show
          Martin Grigorov added a comment - The developer should not be aware of the fact that Wicket uses XML to deliver the HTML response in Ajax response. This CDATA is there exactly for this reason - to allow DOMParser (JS) to create a proper JS Document no matter what is the actual content. Additionally <div/> is a valid by HTML specs but some browsers don't behave well with with. And Wicket tries to help in this situation as well, by expanding it to <div></div>. I'll commit this patch soon if there are no technical problems with it.
          Hide
          Pointbreak added a comment -

          You are right in that he/she should not need to be aware of details of how wicket TRANSPORTS. He/she should be aware how wicket PARSES templates though. Thus, that > and < have special meaning in XML/XHTML templates, and will trigger a parsing error unless used or escaped correctly. Why would the template parser not simply throw an error on templates that can't be parsed as valid XML or XHTML? Unbalanced tags in templates also result in parse errors from wicket (even when they are valid in HTML and accepted by browsers). Your <div/> vs. <div></div> example has nothing to do with this, as for either choice the template will be valid/parsable by any XML parser. A script tag that contains special characters is not. I don't understand why you would want a parser that accepts and and tries to fix incorrect use of special characters in scripts tags in template files. Additionally it does a pretty bad job at that. E.g. try this in a template for an actual error message from the template parser; just a variation of the same thing though:

          <script>
          alert("</script>");
          </script>

          But I fail in getting my message across, so eh.. go ahead, I am happy with your patch either way. Yet convinced that there is a flawed design decision in the (new) wicket template processing logic, that your patch not fixes but works around only for this specific case.

          Show
          Pointbreak added a comment - You are right in that he/she should not need to be aware of details of how wicket TRANSPORTS. He/she should be aware how wicket PARSES templates though. Thus, that > and < have special meaning in XML/XHTML templates, and will trigger a parsing error unless used or escaped correctly. Why would the template parser not simply throw an error on templates that can't be parsed as valid XML or XHTML? Unbalanced tags in templates also result in parse errors from wicket (even when they are valid in HTML and accepted by browsers). Your <div/> vs. <div></div> example has nothing to do with this, as for either choice the template will be valid/parsable by any XML parser. A script tag that contains special characters is not. I don't understand why you would want a parser that accepts and and tries to fix incorrect use of special characters in scripts tags in template files. Additionally it does a pretty bad job at that. E.g. try this in a template for an actual error message from the template parser; just a variation of the same thing though: <script> alert("</script>"); </script> But I fail in getting my message across, so eh.. go ahead, I am happy with your patch either way. Yet convinced that there is a flawed design decision in the (new) wicket template processing logic, that your patch not fixes but works around only for this specific case.
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Fix Version/s 1.5.5 [ 12319052 ]
          Fix Version/s 6.0.0 [ 12315431 ]
          Resolution Fixed [ 1 ]

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Pointbreak
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development