Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-3773

Onclick Script displayed as Link Text

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Trivial
    • Resolution: Fixed
    • Affects Version/s: 1.4.17
    • Fix Version/s: 1.5.1
    • Component/s: wicket
    • Labels:
      None

      Description

      Wicket changes an anchor that has an onclick attribute in a way that breaks it:

      <a href="google.com" onclick="if(history.length > 1)

      {history.back();return false;}else{return true;}">Back to Google</a>
      becomes:
      <a href="google.com" onclick="" if="" history.length=""> 1){history.back();return false;}

      else

      {return true;}

      ">Back to Google</a>

      1. cumulativePatch.txt
        7 kB
        Andrea Del Bene
      2. Quickstart.zip
        7 kB
        bernard
      3. valueAttributeCharsWithTest.txt
        5 kB
        Andrea Del Bene
      4. WICKET-3773.patch
        7 kB
        Martin Grigorov

        Issue Links

          Activity

          Hide
          bitstorm Andrea Del Bene added a comment -

          I've improved code and test case. The cumulative patch 'cumulativePatch.txt' substitute the previous patch I've attached.

          Show
          bitstorm Andrea Del Bene added a comment - I've improved code and test case. The cumulative patch 'cumulativePatch.txt' substitute the previous patch I've attached.
          Hide
          bitstorm Andrea Del Bene added a comment -

          I've attached the new patch (valueAttributeCharsWithTest.txt) here...

          Show
          bitstorm Andrea Del Bene added a comment - I've attached the new patch (valueAttributeCharsWithTest.txt) here...
          Hide
          bitstorm Andrea Del Bene added a comment -

          Hi,

          in order evaluate patch from WICKET-3951 more easily, I moved code to FullyBufferedReader class and I created a test case for this class with escaped quotes.

          Show
          bitstorm Andrea Del Bene added a comment - Hi, in order evaluate patch from WICKET-3951 more easily, I moved code to FullyBufferedReader class and I created a test case for this class with escaped quotes.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Take into account the patch in WICKET-3951.

          Show
          mgrigorov Martin Grigorov added a comment - Take into account the patch in WICKET-3951 .
          Show
          mgrigorov Martin Grigorov added a comment - https://github.com/jhy/jsoup/blob/master/src/test/java/org/jsoup/parser/ParserTest.java#L34
          Hide
          bht@actrix.gen.nz bernard added a comment -

          I am not saying you shouldn't have fun, but I start to think I shouldn't have reported this

          Show
          bht@actrix.gen.nz bernard added a comment - I am not saying you shouldn't have fun, but I start to think I shouldn't have reported this
          Hide
          mgrigorov Martin Grigorov added a comment -

          I think there is no need to log warnings or escape the value. If the parser can properly parse it then there is no need to do anything else.

          Show
          mgrigorov Martin Grigorov added a comment - I think there is no need to log warnings or escape the value. If the parser can properly parse it then there is no need to do anything else.
          Hide
          mgrigorov Martin Grigorov added a comment -

          Here is a patch that solves the problem for this ticket but breaks 4 other tests.

          The problem is that an attribute value can have escaped quotes which will break the calculations.
          E.g.
          "<a href='b > a'>" will be parsed OK, but "<a href='b \' > a'>" will fail.
          Have fun!

          Show
          mgrigorov Martin Grigorov added a comment - Here is a patch that solves the problem for this ticket but breaks 4 other tests. The problem is that an attribute value can have escaped quotes which will break the calculations. E.g. "<a href='b > a'>" will be parsed OK, but "<a href='b \' > a'>" will fail. Have fun!
          Hide
          ivaynberg Igor Vaynberg added a comment -

          in 1.4 we should add a warning when encountering this in raw markup, while in 1.5 we should try to escape automatically

          Show
          ivaynberg Igor Vaynberg added a comment - in 1.4 we should add a warning when encountering this in raw markup, while in 1.5 we should try to escape automatically
          Hide
          akiraly Attila Király added a comment -

          > is a valid character in an xml attribute value while < is not (needs to be escaped): http://www.w3.org/TR/xml/#NT-AttValue

          Show
          akiraly Attila Király added a comment - > is a valid character in an xml attribute value while < is not (needs to be escaped): http://www.w3.org/TR/xml/#NT-AttValue
          Hide
          ivaynberg Igor Vaynberg added a comment -

          does the xml spec require > and < to be escaped inside a quoted attribute value?

          Show
          ivaynberg Igor Vaynberg added a comment - does the xml spec require > and < to be escaped inside a quoted attribute value?
          Hide
          bht@actrix.gen.nz bernard added a comment -

          Thanks for pointing this out. Sure this will happen again. But is it worth fixing this from the performance perspective? Will a better parser be faster?

          Show
          bht@actrix.gen.nz bernard added a comment - Thanks for pointing this out. Sure this will happen again. But is it worth fixing this from the performance perspective? Will a better parser be faster?
          Hide
          bitstorm Andrea Del Bene added a comment -

          The problem should be in class XmlPullParser which "blindly" searches for closing tag '<' without checking if this tag is inside a tag attribute (like onclick="...").

          Show
          bitstorm Andrea Del Bene added a comment - The problem should be in class XmlPullParser which "blindly" searches for closing tag '<' without checking if this tag is inside a tag attribute (like onclick="...").
          Hide
          mgrigorov Martin Grigorov added a comment -

          The problem is the "broken attributes" is that you use '>' in the attribute value.
          Replacing it with > solves the problem.

          Please provide a quickstart that demonstrates such invalid attribute value produced by Wicket and I'll reopen the ticket for you.

          Show
          mgrigorov Martin Grigorov added a comment - The problem is the "broken attributes" is that you use '>' in the attribute value. Replacing it with > solves the problem. Please provide a quickstart that demonstrates such invalid attribute value produced by Wicket and I'll reopen the ticket for you.
          Hide
          mgrigorov Martin Grigorov added a comment -

          That's why I didn't close the ticket
          Definitely there is something wrong producing such kind of html "attributes".

          Show
          mgrigorov Martin Grigorov added a comment - That's why I didn't close the ticket Definitely there is something wrong producing such kind of html "attributes".
          Hide
          bht@actrix.gen.nz bernard added a comment -

          I get this in different scenarios, even with wicket links such as:

          <h2><a wicket:id="backToListItem" onclick="if(history.length > 0)

          {history.back();return false;}else{return true;}">Back</a></h2>

          which renders as:

          <h2><a onclick="" if="" history.length="" href="?wicket:interface=:3:backToListItem::ILinkListener::"> 0){history.back();return false;}

          else

          {return true;}

          ">Back</a></h2>

          I can't see a difference, and I don't know what confuses Wicket. Would it be possible to get Wicket to not get confused?

          Show
          bht@actrix.gen.nz bernard added a comment - I get this in different scenarios, even with wicket links such as: <h2><a wicket:id="backToListItem" onclick="if(history.length > 0) {history.back();return false;}else{return true;}">Back</a></h2> which renders as: <h2><a onclick="" if="" history.length="" href="?wicket:interface=:3:backToListItem::ILinkListener::"> 0){history.back();return false;} else {return true;} ">Back</a></h2> I can't see a difference, and I don't know what confuses Wicket. Would it be possible to get Wicket to not get confused?
          Hide
          mgrigorov Martin Grigorov added a comment -

          Wicket needs to "relativize" all relative links so they keep work in proxy enviroment.
          Since your href is relative (href="google.com") it is confused that it should relativize it.
          Use href="http://google.com" and everything is working again.

          Show
          mgrigorov Martin Grigorov added a comment - Wicket needs to "relativize" all relative links so they keep work in proxy enviroment. Since your href is relative (href="google.com") it is confused that it should relativize it. Use href="http://google.com" and everything is working again.
          Hide
          bht@actrix.gen.nz bernard added a comment -

          Quick start

          Show
          bht@actrix.gen.nz bernard added a comment - Quick start

            People

            • Assignee:
              ivaynberg Igor Vaynberg
              Reporter:
              bht@actrix.gen.nz bernard
            • Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development