MyFaces Core
  1. MyFaces Core
  2. MYFACES-2835

ResponseWriter.startCDATA() and endCDATA() methods should take no action according to the Javadocs

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: None
    • Component/s: JSR-252
    • Labels:
      None

      Description

      Hence, the body of this methods should be empty.

      This is highlighted by a problem with Primefaces, where Mojarra works without problems and Myfaces not, because it adds nested CDATA sections, hence breaking the custom partial response XML created by Primefaces.

        Issue Links

          Activity

          Hide
          Bruno Aranda added a comment -

          This has now been fixed

          Show
          Bruno Aranda added a comment - This has now been fixed
          Hide
          Jakob Korherr added a comment -

          This one is related to (or even a duplicate of) MYFACES-2831.

          Show
          Jakob Korherr added a comment - This one is related to (or even a duplicate of) MYFACES-2831 .
          Hide
          Jakob Korherr added a comment -

          This is not the whole solution for this issue.

          startCData() and endCData() are used multiple times in both ResponseWriter implementations of MyFaces core (HtmlResponseWriterImpl and PartialResponseWriterImpl). Thus their behavior is now broken.

          Show
          Jakob Korherr added a comment - This is not the whole solution for this issue. startCData() and endCData() are used multiple times in both ResponseWriter implementations of MyFaces core (HtmlResponseWriterImpl and PartialResponseWriterImpl). Thus their behavior is now broken.
          Hide
          Bruno Aranda added a comment -

          I am afraid it is the solution. The javadocs read:

          "Open an XML CDATA block. Note that XML does not allow nested CDATA blocks, though this method does not enforce that constraint. The default implementation of this method takes no action when invoked."

          https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/index.html

          Which means that ResponseWriter (and maybe HtmlResponseWriterImpl) should do nothing where invoked. For the PartialResponseWriterImpl we already have a nested CDATA check in place...

          Show
          Bruno Aranda added a comment - I am afraid it is the solution. The javadocs read: "Open an XML CDATA block. Note that XML does not allow nested CDATA blocks, though this method does not enforce that constraint. The default implementation of this method takes no action when invoked." https://javaserverfaces.dev.java.net/nonav/docs/2.0/javadocs/index.html Which means that ResponseWriter (and maybe HtmlResponseWriterImpl) should do nothing where invoked. For the PartialResponseWriterImpl we already have a nested CDATA check in place...
          Hide
          Jakob Korherr added a comment -

          Yes, I know. The solution is right on ResponseWriter.

          But there are changes included in HtmlResponseWriterImpl and PartialResponseWriterImpl too, because they override startCData() and endCData(), but use super.xxxCData() and expect this to really happen in this case. These calls have to be replaced by write ("<![CDATA["); and write ("]]>"); respectively.

          Show
          Jakob Korherr added a comment - Yes, I know. The solution is right on ResponseWriter. But there are changes included in HtmlResponseWriterImpl and PartialResponseWriterImpl too, because they override startCData() and endCData(), but use super.xxxCData() and expect this to really happen in this case. These calls have to be replaced by write ("<![CDATA ["); and write ("] ]>"); respectively.
          Hide
          Bruno Aranda added a comment -

          Ah yes, I see, you are right. I will fix that. I guess nothing should be done in the HtmlResponseWriterImpl, right (if that is the default implementation - I will do a black box test with Mojarra and see) but PartialResponseWriterImpl needs to be updated definitely

          Show
          Bruno Aranda added a comment - Ah yes, I see, you are right. I will fix that. I guess nothing should be done in the HtmlResponseWriterImpl, right (if that is the default implementation - I will do a black box test with Mojarra and see) but PartialResponseWriterImpl needs to be updated definitely
          Hide
          Jakob Korherr added a comment -

          FYI test cases which currently fail because of your previous commit:

          teststandardNestingTest(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testIllegalNestingResolvementTest(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testIllegalNestingResolvementTest2(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testStandardUpdate(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testStandardUpdateNestedCDATA(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testComponentAuthorNestingFailureTest(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testStandardInsertAfter(org.apache.myfaces.context.PartialResponseWriterImplTest)
          testStandardInsertBefore(org.apache.myfaces.context.PartialResponseWriterImplTest)

          Thanks for digging into this.

          Jakob

          Show
          Jakob Korherr added a comment - FYI test cases which currently fail because of your previous commit: teststandardNestingTest(org.apache.myfaces.context.PartialResponseWriterImplTest) testIllegalNestingResolvementTest(org.apache.myfaces.context.PartialResponseWriterImplTest) testIllegalNestingResolvementTest2(org.apache.myfaces.context.PartialResponseWriterImplTest) testStandardUpdate(org.apache.myfaces.context.PartialResponseWriterImplTest) testStandardUpdateNestedCDATA(org.apache.myfaces.context.PartialResponseWriterImplTest) testComponentAuthorNestingFailureTest(org.apache.myfaces.context.PartialResponseWriterImplTest) testStandardInsertAfter(org.apache.myfaces.context.PartialResponseWriterImplTest) testStandardInsertBefore(org.apache.myfaces.context.PartialResponseWriterImplTest) Thanks for digging into this. Jakob
          Hide
          Jakob Korherr added a comment -

          Yes, PartialResponseWriterImpl has to be updated definitely.

          On HtmlResponseWriterImpl I am not sure. I think we should ask Werner about this case, because he put a lot of work into this CDATA stuff.

          Show
          Jakob Korherr added a comment - Yes, PartialResponseWriterImpl has to be updated definitely. On HtmlResponseWriterImpl I am not sure. I think we should ask Werner about this case, because he put a lot of work into this CDATA stuff.
          Hide
          Bruno Aranda added a comment -

          Diferent quick tests I have done (black box tests):

          // USING MOJARRA
          ///////////////

          // The abstract class

          // DEFAULT IMPL

          ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8");
          rw.startCDATA();
          rw.endCDATA();

          Renders:

          <![CDATA[]]>

          /////////

          ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8");
          rw.startCDATA();
          rw.startCDATA();
          rw.endCDATA();
          rw.endCDATA();

          This throws IllegalStateException: CDATA tags may not nest

          // CUSTOM IMPL

          ResponseWriter cw = new ResponseWriter()

          { ... empty implementation here ... }

          ;

          cw.startCDATA();
          cw.endCDATA();

          // PARTIAL

          ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8");
          ResponseWriter pw = new PartialResponseWriter(rw);
          pw.startCDATA();
          pw.startCDATA();
          pw.endCDATA();
          pw.endCDATA();

          This throws IllegalStateException: CDATA tags may not nest

          Conclusions (differences between Mojarra and MyFaces)
          ***************

          ResponseWriter:

          The endCDATA method in Mojarra throws an UnsupportedOperationException (undocumented in the javadocs). This shows how the ResponseWriter class has an empty startCDATA and throws an exception on endCDATA.

          HtmlResponseWriterImpl

          Apparently we should take care here of nested CDATAs. In Mojarra they throw an exception if that is the case.

          PartialResponseWriterImpl

          The same than in the HtmlResponseWriter. An exception is thrown if there are nested CDATAs.

          Show
          Bruno Aranda added a comment - Diferent quick tests I have done (black box tests): // USING MOJARRA /////////////// // The abstract class // DEFAULT IMPL ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8"); rw.startCDATA(); rw.endCDATA(); Renders: <![CDATA[]]> ///////// ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8"); rw.startCDATA(); rw.startCDATA(); rw.endCDATA(); rw.endCDATA(); This throws IllegalStateException: CDATA tags may not nest // CUSTOM IMPL ResponseWriter cw = new ResponseWriter() { ... empty implementation here ... } ; cw.startCDATA(); cw.endCDATA(); // PARTIAL ResponseWriter rw = new HtmlResponseWriter(w, "text/html", "UTF-8"); ResponseWriter pw = new PartialResponseWriter(rw); pw.startCDATA(); pw.startCDATA(); pw.endCDATA(); pw.endCDATA(); This throws IllegalStateException: CDATA tags may not nest Conclusions (differences between Mojarra and MyFaces) *************** ResponseWriter: The endCDATA method in Mojarra throws an UnsupportedOperationException (undocumented in the javadocs). This shows how the ResponseWriter class has an empty startCDATA and throws an exception on endCDATA. HtmlResponseWriterImpl Apparently we should take care here of nested CDATAs. In Mojarra they throw an exception if that is the case. PartialResponseWriterImpl The same than in the HtmlResponseWriter. An exception is thrown if there are nested CDATAs.
          Hide
          Werner Punz added a comment -

          Hi since I am sitll on vacation until the end of the week just a short response. I did a fixup for the Partial Response Writer for nested CDATA blocks. Because there fixing it was valid because we enforce the html data to be rendered into a CDATA and nested CDATA blocks must be properly fixed so that a valid decoding of it results in a final CDATA on the browsers side.
          The fixup simply was to split a ]]> into something like ]]><Unable to render embedded object: File ([CDATA[]]]]><) not found.[CDATA[>
          Now this looks weird but makes sense, because the xml spec clearly says that in a cdata block every sequence of characters is allowed to occur unless it is a ]]> which marks the end. The solution is to split the sequence ]]> into separate cdata blocks so that we have multiple cdatas which if decoded result in a valid ]]>

          I implemented this solution in the partial response writer via double buffering. If a cdata block was opened then the strings being pushed into the response writer were buffered and once the end of cdata within the proper depth was reached the string was cdata escaped before being given to the final underyling response writer.

          I also tested against mojarra which had similar fixes in as it seems, but it failed in one of my testcases where a cdata block was embedded somewhere in another cdata block in a javascript alert box.

          But I also said that the error clearly also is on primefaces side here, because they issue nested cdata blocks and rely on the impls standard response writer to resolve that clearly. Now fixing this on the response writers side is perfectly viable if we have an xmlis response format, but what if we have a an output format where nested CDATA blocks are perfectly legal?
          Ok this is hypothetical, but the way the response writer is designed it does not prevent false inputs it follows the rule garbage in -> garbage out.
          Just to say my 2c, but I have promised to discuss that next week on the open list (due to my vacation I cannot do it this week properly), if someone wants to bring it earlier to jsf open, feel free

          But as I said this is only valid because our partial lifecycle enforces an xml and enforces that every html string has to be embedded in a cdata block outside, hence naturally nested cdata blocks can occur.

          Show
          Werner Punz added a comment - Hi since I am sitll on vacation until the end of the week just a short response. I did a fixup for the Partial Response Writer for nested CDATA blocks. Because there fixing it was valid because we enforce the html data to be rendered into a CDATA and nested CDATA blocks must be properly fixed so that a valid decoding of it results in a final CDATA on the browsers side. The fixup simply was to split a ]]> into something like ]]>< Unable to render embedded object: File ([CDATA[]]]]><) not found. [CDATA[> Now this looks weird but makes sense, because the xml spec clearly says that in a cdata block every sequence of characters is allowed to occur unless it is a ]]> which marks the end. The solution is to split the sequence ]]> into separate cdata blocks so that we have multiple cdatas which if decoded result in a valid ]]> I implemented this solution in the partial response writer via double buffering. If a cdata block was opened then the strings being pushed into the response writer were buffered and once the end of cdata within the proper depth was reached the string was cdata escaped before being given to the final underyling response writer. I also tested against mojarra which had similar fixes in as it seems, but it failed in one of my testcases where a cdata block was embedded somewhere in another cdata block in a javascript alert box. But I also said that the error clearly also is on primefaces side here, because they issue nested cdata blocks and rely on the impls standard response writer to resolve that clearly. Now fixing this on the response writers side is perfectly viable if we have an xmlis response format, but what if we have a an output format where nested CDATA blocks are perfectly legal? Ok this is hypothetical, but the way the response writer is designed it does not prevent false inputs it follows the rule garbage in -> garbage out. Just to say my 2c, but I have promised to discuss that next week on the open list (due to my vacation I cannot do it this week properly), if someone wants to bring it earlier to jsf open, feel free But as I said this is only valid because our partial lifecycle enforces an xml and enforces that every html string has to be embedded in a cdata block outside, hence naturally nested cdata blocks can occur.
          Hide
          Bruno Aranda added a comment -

          Ok, I will revert my change of this morning and we can work on this when we get an answer from the jsf open list.

          And the problem may be in Primefaces, but I guess it is due to Mojarra and MyFaces behaving in a different way for the same code. It can be Primefaces today, but any other library in the future.

          Show
          Bruno Aranda added a comment - Ok, I will revert my change of this morning and we can work on this when we get an answer from the jsf open list. And the problem may be in Primefaces, but I guess it is due to Mojarra and MyFaces behaving in a different way for the same code. It can be Primefaces today, but any other library in the future.
          Hide
          Leonardo Uribe added a comment -

          Recently I review this problem and the latest code is ok. It was found a bug on ajax part (MYFACES-3339 and MYFACES-3318). It seems we forgot to close this issue, so I'll close it as fixed.

          Show
          Leonardo Uribe added a comment - Recently I review this problem and the latest code is ok. It was found a bug on ajax part ( MYFACES-3339 and MYFACES-3318 ). It seems we forgot to close this issue, so I'll close it as fixed.

            People

            • Assignee:
              Bruno Aranda
              Reporter:
              Bruno Aranda
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development