Uploaded image for project: '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
    • Status: Closed
    • Priority: 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

          Attachments

            Issue Links

              Activity

              Hide
              oa_sychev Oleg Sychev added a comment -

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

              Show
              oa_sychev Oleg Sychev added a comment - Here is a promised patch. Hope it doesn't take much of you precious time to review.
              Hide
              timhunt 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
              timhunt 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
              oa_sychev 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
              oa_sychev 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
              timhunt Tim Hunt added a comment -

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

              Show
              timhunt Tim Hunt added a comment - I still don't get it. There are no problems with single quote characters in XML.
              Hide
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              oa_sychev 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
              timhunt Tim Hunt added a comment -

              Great! Thanks Oleg.

              Show
              timhunt 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:
                    Fix Release Date:
                    25/Oct/10