Moodle
  1. Moodle
  2. MDL-36808

Activity completion toggle in course view generates bad HTML when resource/activity title contains single quotes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.3
    • Fix Version/s: 2.3.4, 2.4.1
    • Component/s: Activity completion
    • Labels:
      None
    • Testing Instructions:
      Hide

      0. Ensure completion tracking is enabled in advanced features.
      1. Create a new empty course with completion tracking enabled (leave other settings default).
      2. Add a new label to the first week of the course. Set label name to: argh'"s label
      3. Turn editing off.
      4. View source. Search for all instances of the string "argh" so as to find the 'argh'"s label' strings regardless of how they are quoted.

      • Before fixing this issue, some of those instances are invalid HTML code because they contain a single quote inside an attribute that is single-quoted. Firefox displays these in red in its source view.
      • After fixing this issue, any instances that are in attributes should be escaped suitably for the surrounding quotes.

      5. Click on the completion tickbox to verify that it correctly ticks.
      6. Look at the title (mouse over the tickbox) to verify that it contains the correct module name without extra quoting. (This title is written by JS code which uses the modulename field in the form).
      7. Click again to check it unticks.
      8. Turn JavaScript off and reload the page, then click on the completion tickbox twice to verify that it correctly ticks and then unticks (to test the non-JS version).

      Show
      0. Ensure completion tracking is enabled in advanced features. 1. Create a new empty course with completion tracking enabled (leave other settings default). 2. Add a new label to the first week of the course. Set label name to: argh'"s label 3. Turn editing off. 4. View source. Search for all instances of the string "argh" so as to find the 'argh'"s label' strings regardless of how they are quoted. Before fixing this issue, some of those instances are invalid HTML code because they contain a single quote inside an attribute that is single-quoted. Firefox displays these in red in its source view. After fixing this issue, any instances that are in attributes should be escaped suitably for the surrounding quotes. 5. Click on the completion tickbox to verify that it correctly ticks. 6. Look at the title (mouse over the tickbox) to verify that it contains the correct module name without extra quoting. (This title is written by JS code which uses the modulename field in the form). 7. Click again to check it unticks. 8. Turn JavaScript off and reload the page, then click on the completion tickbox twice to verify that it correctly ticks and then unticks (to test the non-JS version).
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-36808-master
    • Rank:
      46324

      Description

      The activity completion toggle source code (./course/lib.php ~ line 1713) uses hardcoded HTML markup and single quotes to wrap tag attribute values. This will produce invalid markup when the resource/activity title contains single quotes. This malformed HTML breaks the layout in IE7 and IE8/9 running in compatibility mode.

      The HTML that get produced >>

      <form class='togglecompletion' method='post' action='http://test.dan.hi/course/togglecompletion.php'>
      <div>
      <input type='hidden' name='id' value='48' />
      <input type='hidden' name='modulename' value='Google's Homepage' />
      <input type='hidden' name='sesskey' value='WNC7Frlbip' />
      <input type='hidden' name='completionstate' value='1' />
      <input type='image' src='http://test.dan.hi/theme/image.php?theme=enterprise&image=i%2Fcompletion-manual-n&rev=392' alt='Not completed: Google's Homepage. Select to mark as complete.' title='Mark as complete: Google's Homepage' />
      </div>
      </form>

      << As shown the ALT and TITLE attribute values contain single quotes while being wrapped in single quotes.

      Steps to Reproduce:

      • Go to any course view page.
      • Edit settings of the course and enable completion tracking and save.
      • Turn editing on
      • Add a URL (resource) a section (though any resource or activity will have the same effect).
      • Give it the title > Google's Homepage
      • Give it the URL value > http://www.google.com
      • Click "Save and return to course"
      • View HTML source generated for this feature

      Suggested Solution

      • Replace hardcode HTML markup with HTML generated by the html_writer:: functionality >>

      echo html_writer::start_tag('form', array('class'=>'togglecompletion'.$extraclass, 'method'=>'post', 'action'=>$CFG->wwwroot.'/course/togglecompletion.php'));
      echo html_writer::start_tag('div');
      echo html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'id', 'value'=>$mod->id));
      echo html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'modulename', 'value'=>s($mod->name)));
      echo html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'sesskey', 'value'=>sesskey()));
      echo html_writer::empty_tag('input', array('type'=>'hidden', 'name'=>'completionstate', 'value'=>$newstate));
      echo html_writer::empty_tag('input', array('type'=>'image', 'src'=>$imgsrc, 'alt'=>$imgalt, 'title'=>$imgtitle));
      echo html_writer::end_tag('div'); echo html_writer::end_tag('form');

        Issue Links

          Activity

          Hide
          Sam Marshall added a comment -

          Confirmed in Moodle 2.3.3+, I have written testing instructions (and I agree with proposed fix, this code predates html_writer by quite some time but now is a good point to change it...)

          Show
          Sam Marshall added a comment - Confirmed in Moodle 2.3.3+, I have written testing instructions (and I agree with proposed fix, this code predates html_writer by quite some time but now is a good point to change it...)
          Hide
          Sam Marshall added a comment -

          Updated test script to include some additional issues that could arise from this change.

          Show
          Sam Marshall added a comment - Updated test script to include some additional issues that could arise from this change.
          Hide
          Sam Marshall added a comment -

          I think we'll count this as code review (by me of the reporter's code) since this fix is based on that code - thanks! Aside from whitespace/trivia, the only thing I changed was the escaping of the 'module name' field; this ended up double-escaped.

          Show
          Sam Marshall added a comment - I think we'll count this as code review (by me of the reporter's code) since this fix is based on that code - thanks! Aside from whitespace/trivia, the only thing I changed was the escaping of the 'module name' field; this ended up double-escaped.
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23. Thanks Sam & Dan!

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23. Thanks Sam & Dan!
          Hide
          Rajesh Taneja added a comment -

          Thanks Sam,

          Works Grt.
          No invalid instances found in html source. Was able to change completion status without problem.

          Show
          Rajesh Taneja added a comment - Thanks Sam, Works Grt. No invalid instances found in html source. Was able to change completion status without problem.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many thanks for your effort, the whole Moodle Community will be enjoying your great solutions starting now! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: