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

lib_form/filemanager: erroneous escaping on reference list

    XMLWordPrintable

Details

    • MOODLE_25_STABLE, MOODLE_26_STABLE
    • MOODLE_25_STABLE, MOODLE_26_STABLE
    • MDL-45387-master
    • Hide

      This test should be performed on 2.5, 2.6 and master; there shouldn't be a need to do a cross-browser test.

      1. Before applying the patch, upload an image to your private files area.
      2. In a new course:
        • Create a new page, and insert the image from step 2 as an alias/shortcut.
        • Repeat with another page.
      3. Go to your private files area.
        • Observe that your image now has a 'link' icon next to it signifying that it has associated aliases/shortcuts.
      4. Click your image icon to bring up the editing dialog.
        • Observe that the list of aliases/shortcuts does not appear as it should; li tags appear in the escaped output.
      5. Apply the patch, and refresh the page.
        • Observe that the list of aliases/shortcuts now appears as it should.
      Show
      This test should be performed on 2.5, 2.6 and master; there shouldn't be a need to do a cross-browser test. Before applying the patch, upload an image to your private files area. In a new course: Create a new page, and insert the image from step 2 as an alias/shortcut . Repeat with another page. Go to your private files area. Observe that your image now has a 'link' icon next to it signifying that it has associated aliases/shortcuts. Click your image icon to bring up the editing dialog. Observe that the list of aliases/shortcuts does not appear as it should; li tags appear in the escaped output. Apply the patch, and refresh the page. Observe that the list of aliases/shortcuts now appears as it should.

    Description

      Discovered while testing MDLQA-7051. When a file has aliases/shortcuts present in a course, and a user uses the file manager to edit the file's settings, the list of aliases/shortcuts is erroneously escaped.

      What should happen
      Each li element's contents should be escaped.

      What actually happens
      The entire contents of the ul, including the li tags, are escaped - resulting in http://puu.sh/8vLVh/70c902163c.png

      The offending line of Javascript is here, introduced with MDL-37507 (security issue): https://github.com/moodle/moodle/blame/MOODLE_25_STABLE/lib/form/filemanager.js#L1022

      After consulting with dobedobedoh, a way to fix this has been identified as instead of performing a Y.Escape on the entire contents as above, we should instead perform a Y.Escape in the preceding for loop, i.e.

      for (var i in obj.references) {
          node.reflist += '<li>'+Y.Escape.html(obj.references[i])+'</li>';
      }
      selectnode.one('.fp-reflist .fp-value').setContent(node.reflist);
      

      Testing should encompass a re-test of MDL-37507 just in case to avoid any regressions.

      Attachments

        1. MDL-45387_25.patch
          1 kB
        2. MDL-45387_26.patch
          1 kB
        3. MDL-45387_master.patch
          1 kB
        4. Unescaped list.jpg
          Unescaped list.jpg
          49 kB

        Issue Links

          Activity

            People

              jethac Jetha Chan
              jethac Jetha Chan
              Frédéric Massart Frédéric Massart
              Marina Glancy Marina Glancy
              Sam Hemelryk Sam Hemelryk
              Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Sujith Haridasan, Adrian Greeve, Jake Dallimore, Mathew May, Mihail Geshoski, Peter Dias, Sujith Haridasan
              Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:
                12/May/14