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
    • Rank:
      27128

      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.

      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: