Moodle
  1. Moodle
  2. MDL-23198

Special characters in question fields in Moodle XML export didn't properly escaped for plugins using extra_question_fields

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.9
    • Fix Version/s: 1.9.10
    • Component/s: Questions
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE

      Description

      I discovered bug in my universal XML export code from MDL-20157 (it's affects only 3d party plugins for now). Extra question fields could contain HTML special chars and they are not escaped properly.

      Should be easy to fix using <![CDATA[ ]]> tag if such characters existed, adapting solution used by XML format writetext function. I'm going to commit patch in the near time.

        Gliffy Diagrams

        1. questiontype_xml_export_simple.patch
          1 kB
          Oleg Sychev
        2. questiontype_xml_export.patch
          1 kB
          Oleg Sychev
        3. questiontype_xml_export.patch
          1 kB
          Oleg Sychev

          Issue Links

            Activity

            Hide
            Oleg Sychev added a comment -

            Here is a promised patch. Hope it doesn't take much of you precious time to review.

            Show
            Oleg Sychev added a comment - Here is a promised patch. Hope it doesn't take much of you precious time to review.
            Hide
            Tim Hunt added a comment -

            I don't think this patch is right.

            1. I think we should just always do htmlspecialchars on $text on output, so we generate valid XML. I prefer that to using CDATA.

            Oh, but I see you have copied that logic from question/format/xml/format.php. Hmm. Oh well, we had better be consistent.

            2. The bit that adds the <text> tags does not make any sense to me. And there is no equivalent change to remove the <text> tags on import, so how can this possibly work?

            Show
            Tim Hunt added a comment - I don't think this patch is right. 1. I think we should just always do htmlspecialchars on $text on output, so we generate valid XML. I prefer that to using CDATA. Oh, but I see you have copied that logic from question/format/xml/format.php. Hmm. Oh well, we had better be consistent. 2. The bit that adds the <text> tags does not make any sense to me. And there is no equivalent change to remove the <text> tags on import, so how can this possibly work?
            Hide
            Oleg Sychev added a comment -

            1. I used existing code for consistence. Also I have my doubts about using htmlspecialchars() in case some extra fields would contain HTML text.

            2. You are right about no equivalent code on import, I added it in the new version of the patch.

            All mess with <text> tags are needed to escape single quotes properly for DB.

            So for Moodle 2.0 these parts of the code should be deleted:

            + if (is_array($qo->$field))

            { + $fieldvalue = $qo->$field; + $qo->$field = $format->import_text($fieldvalue['text']); + }

            And

            + if (strpos($exportedvalue,'\'')!=false)

            { + $exportedvalue = '<text>'.$exportedvalue.'</text>'; + }

            I beleive none other change for Moodle 2.0 is necessary.

            Show
            Oleg Sychev added a comment - 1. I used existing code for consistence. Also I have my doubts about using htmlspecialchars() in case some extra fields would contain HTML text. 2. You are right about no equivalent code on import, I added it in the new version of the patch. All mess with <text> tags are needed to escape single quotes properly for DB. So for Moodle 2.0 these parts of the code should be deleted: + if (is_array($qo->$field)) { + $fieldvalue = $qo->$field; + $qo->$field = $format->import_text($fieldvalue['text']); + } And + if (strpos($exportedvalue,'\'')!=false) { + $exportedvalue = '<text>'.$exportedvalue.'</text>'; + } I beleive none other change for Moodle 2.0 is necessary.
            Hide
            Tim Hunt added a comment -

            I still don't get it. There are no problems with single quote characters in XML.

            Show
            Tim Hunt added a comment - I still don't get it. There are no problems with single quote characters in XML.
            Hide
            Oleg Sychev added a comment -

            There are no problen with single quote in XML. There are problem with it when inserting in DB (in Moodle 1.9 and before).

            The way it currently solved in question/format/xml/format.php is to wrap fields which could contain signle quote character in <text> tag and then use import_text which basically does addslashes() (look at import_text function) - save_question_options await already escaped data.

            I see no reason to break consistency with the rest of format and the problem will go away with Moodle 1.9. Alternatively, you could don't add <text> tags and just call addslashes on all fields when importing, this will be easier thought it somewhat breaks consistency. I could rewrite patch if you want this, thought I see little reason to polish up code that will become obsolete very soon.

            Show
            Oleg Sychev added a comment - There are no problen with single quote in XML. There are problem with it when inserting in DB (in Moodle 1.9 and before). The way it currently solved in question/format/xml/format.php is to wrap fields which could contain signle quote character in <text> tag and then use import_text which basically does addslashes() (look at import_text function) - save_question_options await already escaped data. I see no reason to break consistency with the rest of format and the problem will go away with Moodle 1.9. Alternatively, you could don't add <text> tags and just call addslashes on all fields when importing, this will be easier thought it somewhat breaks consistency. I could rewrite patch if you want this, thought I see little reason to polish up code that will become obsolete very soon.
            Hide
            Oleg Sychev added a comment -

            Ok, Tim, here is a simpler patch without <text> tags if you wish (tested).

            For 2.0 version just delete addslashes() call from import.

            Show
            Oleg Sychev added a comment - Ok, Tim, here is a simpler patch without <text> tags if you wish (tested). For 2.0 version just delete addslashes() call from import.
            Hide
            Tim Hunt added a comment -

            Great! Thanks Oleg.

            Show
            Tim Hunt added a comment - Great! Thanks Oleg.

              People

              • Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: