Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-29712

The REST web service creates invalid XML by encoding HTML entities

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.6, 2.1, 2.2
    • Fix Version/s: 2.0.7, 2.1.4, 2.2.1
    • Component/s: Web Services
    • Labels:
      None
    • Testing Instructions:
      Hide

      Edit the user linked to the token and set the first name as Noël. Enable mobile web service and also the REST protocol.

      Use the Javascript-REST demo client (https://github.com/moodlehq/sample-ws-clients/tree/master/JAVASCRIPT-REST) and change these lines:

       
      var functionname = 'moodle_webservice_get_siteinfo';
       
      // ....
       
      var data = {
          wstoken: token,
          wsfunction: functionname,
          moodlewsrestformat: 'xml'
      }
       
      var response = $.ajax(
                            {   type: 'POST',
                                data: data,
                                dataType: 'text',
                                url: serverurl,
                                success: function(data) { 
                                     console.log('Parsed data');
                                     data = $.parseXML(data); 
                                     console.log(data);
                                }
                             }
                            );

      In firebug: no JS errors (mainly an xml parsing error) should appeared. You should be able to see the resulting XML in your JS logs.

      For 2.0 testing you'll have to create your own service and use moodle_user_get_users_by_id ws function. You'll also have to change these lines:

       
      var functionname = 'moodle_user_get_users_by_id';
       
      // ...
       
      var users = ['2'];
       
      var data = {
          wstoken: token,
          wsfunction: functionname,
                      userids: users
      }
       
      var response = $.ajax(
                            {   type: 'POST',
                                data: data,
                                dataType: 'text',
                                url: serverurl,
                                success: function(data) { 
                                     console.log('Parsed data');
                                     data = $.parseXML(data); 
                                     console.log(data);
                                }
                             }
                            );

      Show
      Edit the user linked to the token and set the first name as Noël. Enable mobile web service and also the REST protocol. Use the Javascript-REST demo client ( https://github.com/moodlehq/sample-ws-clients/tree/master/JAVASCRIPT-REST ) and change these lines:   var functionname = 'moodle_webservice_get_siteinfo';   // ....   var data = { wstoken: token, wsfunction: functionname, moodlewsrestformat: 'xml' }   var response = $.ajax( { type: 'POST', data: data, dataType: 'text', url: serverurl, success: function(data) { console.log('Parsed data'); data = $.parseXML(data); console.log(data); } } ); In firebug: no JS errors (mainly an xml parsing error) should appeared. You should be able to see the resulting XML in your JS logs. For 2.0 testing you'll have to create your own service and use moodle_user_get_users_by_id ws function. You'll also have to change these lines:   var functionname = 'moodle_user_get_users_by_id';   // ...   var users = ['2'];   var data = { wstoken: token, wsfunction: functionname, userids: users }   var response = $.ajax( { type: 'POST', data: data, dataType: 'text', url: serverurl, success: function(data) { console.log('Parsed data'); data = $.parseXML(data); console.log(data); } } );
    • Workaround:
      Hide

      For javascript client using jquery:

      $.ajax({
      type: "POST",
      url: siteurl+"/webservice/rest/server.php?wstoken="+mytoken,
      data: data,
      dataType: 'text',
      dataFilter: function(data, dataType)

      { // XML returned by Moodle is not well parsed data = data.replace(/\<VALUE\>/gi,'<VALUE><![CDATA[').replace(/\<\/VALUE\>/gi,']]></VALUE>'); data = data.replace(/\<MESSAGE\>/gi,'<MESSAGE><![CDATA[').replace(/\<\/MESSAGE\>/gi,']]></MESSAGE>'); return data; }

      ,
      success: function(data)

      { ....data = $.parseXML(data); // }

      });

      Show
      For javascript client using jquery: $.ajax({ type: "POST", url: siteurl+"/webservice/rest/server.php?wstoken="+mytoken, data: data, dataType: 'text', dataFilter: function(data, dataType) { // XML returned by Moodle is not well parsed data = data.replace(/\<VALUE\>/gi,'<VALUE><![CDATA[').replace(/\<\/VALUE\>/gi,']]></VALUE>'); data = data.replace(/\<MESSAGE\>/gi,'<MESSAGE><![CDATA[').replace(/\<\/MESSAGE\>/gi,']]></MESSAGE>'); return data; } , success: function(data) { ....data = $.parseXML(data); // } });
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:

      Description

      The XML output produced by the REST web service converts all applicable characters to HTML entities. Most of these entities are not defined in XML and if they are in output of the REST web service function that output is not valid XML.

      In /webservice/rest/locallib.php in the xmlize_result() function all values in the output are filtered through the php htmlentities() function. Instead use the php htmlspecialchars function to only convert these characters – & < > " ' – to html entities.

        Gliffy Diagrams

        1. MDL-29712.patch
          2 kB
          Tom Cubanski
        2. rest_webservice_bad_xml.txt
          1 kB
          Tom Cubanski

          Issue Links

            Activity

            Hide
            tcubanski Tom Cubanski added a comment -

            Its hard to discuss html entities through a web browser because the browser keeps changing them in the display. I've attached a text file that I have pasted in a web services call and response to better demonstrate this problem. In this example the XML generated by moodle_user_get_users_by_id that I pasted into the attached file is not valid XML.

            Show
            tcubanski Tom Cubanski added a comment - Its hard to discuss html entities through a web browser because the browser keeps changing them in the display. I've attached a text file that I have pasted in a web services call and response to better demonstrate this problem. In this example the XML generated by moodle_user_get_users_by_id that I pasted into the attached file is not valid XML.
            Hide
            jleyva Juan Leyva added a comment -

            I think this bug is very important, is preventing using the REST + XML combination

            I have fixed it retrieving the document as it where plaintext and adding <![CDATA[ .. ]] to all the values:

            javascript code using jquery:

            $.ajax({
            type: "POST",
            url: siteurl+"/webservice/rest/server.php?wstoken="+mytoken,
            data: data,
            dataType: 'text',
            dataFilter: function(data, dataType)

            { // XML returned by Moodle is not well parsed data = data.replace(/\<VALUE\>/gi,'<VALUE><![CDATA[').replace(/\<\/VALUE\>/gi,']]></VALUE>'); data = data.replace(/\<MESSAGE\>/gi,'<MESSAGE><![CDATA[').replace(/\<\/MESSAGE\>/gi,']]></MESSAGE>'); return data; }

            ,
            success: function(data)

            { ....data = $.parseXML(data); // }

            });

            Regards

            Show
            jleyva Juan Leyva added a comment - I think this bug is very important, is preventing using the REST + XML combination I have fixed it retrieving the document as it where plaintext and adding <![CDATA[ .. ]] to all the values: javascript code using jquery: $.ajax({ type: "POST", url: siteurl+"/webservice/rest/server.php?wstoken="+mytoken, data: data, dataType: 'text', dataFilter: function(data, dataType) { // XML returned by Moodle is not well parsed data = data.replace(/\<VALUE\>/gi,'<VALUE><![CDATA[').replace(/\<\/VALUE\>/gi,']]></VALUE>'); data = data.replace(/\<MESSAGE\>/gi,'<MESSAGE><![CDATA[').replace(/\<\/MESSAGE\>/gi,']]></MESSAGE>'); return data; } , success: function(data) { ....data = $.parseXML(data); // } }); Regards
            Hide
            tcubanski Tom Cubanski added a comment -

            I've attached a patch against the master branch of the fix that we a have been using to resolve this issue.

            Show
            tcubanski Tom Cubanski added a comment - I've attached a patch against the master branch of the fix that we a have been using to resolve this issue.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Tom and Juan. I'll have a look

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Tom and Juan. I'll have a look
            Hide
            ppollet Patrick Pollet added a comment -
            Show
            ppollet Patrick Pollet added a comment - see also http://tracker.moodle.org/browse/MDL-28461 Cheers
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I'm testing Tom's patch. I don't have experience using REST-XML servers. As you (Juan, Patrick, and Tom) are using it, do you see any possible regression?

            If I change htmlentities for htmlspecialchars now, would it be possible that an existing REST-XML client expect all characters to be translated in HTML?

            At the end I could add a flag to the server to choose between htmlentities/htmlspecialchars. But if htmlentities makes no sens for any good client coding then it's overkill. And in this case I would prefer without a flag.

            Show
            jerome Jérôme Mouneyrac added a comment - I'm testing Tom's patch. I don't have experience using REST-XML servers. As you (Juan, Patrick, and Tom) are using it, do you see any possible regression? If I change htmlentities for htmlspecialchars now, would it be possible that an existing REST-XML client expect all characters to be translated in HTML? At the end I could add a flag to the server to choose between htmlentities/htmlspecialchars. But if htmlentities makes no sens for any good client coding then it's overkill. And in this case I would prefer without a flag.
            Hide
            tcubanski Tom Cubanski added a comment -

            I tested the xml I attached to this ticket in Java and with the php XML parser. Both parsers gave an error and failed to parse the bad xml.

            I don't think adding a flag to choose between htmlentities/htmlspecialchars is a good idea. It allows Moodle to produce output that standard xml parsers can't use.

            Show
            tcubanski Tom Cubanski added a comment - I tested the xml I attached to this ticket in Java and with the php XML parser. Both parsers gave an error and failed to parse the bad xml. I don't think adding a flag to choose between htmlentities/htmlspecialchars is a good idea. It allows Moodle to produce output that standard xml parsers can't use.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Thanks Tom.

            I now understand it breaks the xml valiation. With the patch, Juan "fixed" JS client will get:

            <![CDATA['Noël']]>

            instead of

            <![CDATA['No&euml;l']]>

            Juan, do you think it would break your client? It doesn't seem to me.

            I'm also wondering what would be the impact between htmlspecialchars and nothing as Patrick mentioned in MDL-28461.

            Show
            jerome Jérôme Mouneyrac added a comment - Thanks Tom. I now understand it breaks the xml valiation. With the patch, Juan "fixed" JS client will get: <![CDATA['Noël']]> instead of <![CDATA['No&euml;l']]> Juan, do you think it would break your client? It doesn't seem to me. I'm also wondering what would be the impact between htmlspecialchars and nothing as Patrick mentioned in MDL-28461 .
            Hide
            jerome Jérôme Mouneyrac added a comment -

            I pushed a commit as the patch seems good to me and it doesn't seem it would any clients.
            However before to backport and send to integration, I'll do more search about htmlspecialchars versus nothing.

            Show
            jerome Jérôme Mouneyrac added a comment - I pushed a commit as the patch seems good to me and it doesn't seem it would any clients. However before to backport and send to integration, I'll do more search about htmlspecialchars versus nothing.
            Hide
            tcubanski Tom Cubanski added a comment -

            Thanks for committing the patch Jerome. When choosing between htmlspecialchars and nothing you have make sure you convert the characters that define the XML structure: < > &. If those characters are in the document unconverted the document may not be able to be parsed. htmlspecialchars converts that characters that need to be converted for XML.

            Show
            tcubanski Tom Cubanski added a comment - Thanks for committing the patch Jerome. When choosing between htmlspecialchars and nothing you have make sure you convert the characters that define the XML structure: < > &. If those characters are in the document unconverted the document may not be able to be parsed. htmlspecialchars converts that characters that need to be converted for XML.
            Hide
            ppollet Patrick Pollet added a comment -

            I second Tom's comment. Changing to htmlspecialchars is the way to go and it also fixed http://tracker.moodle.org/browse/MDL-28461 where errors messages were also messed when testing the REST mode in a browser.

            Show
            ppollet Patrick Pollet added a comment - I second Tom's comment. Changing to htmlspecialchars is the way to go and it also fixed http://tracker.moodle.org/browse/MDL-28461 where errors messages were also messed when testing the REST mode in a browser.
            Hide
            jerome Jérôme Mouneyrac added a comment -

            Submitted for integration, thanks

            Show
            jerome Jérôme Mouneyrac added a comment - Submitted for integration, thanks
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Integrated thanks Jerome

            Show
            samhemelryk Sam Hemelryk added a comment - Integrated thanks Jerome
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tom and Jerome, for working on this.
            Works Great

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tom and Jerome, for working on this. Works Great
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Yes, you did it!

            Now your code is part of the best weeklies released ever, many thanks!

            Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Yes, you did it! Now your code is part of the best weeklies released ever, many thanks! Closing, ciao

              People

              • Votes:
                3 Vote for this issue
                Watchers:
                2 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jan/12