Moodle
  1. Moodle
  2. MDL-20725

Double escaping of OPTGROUP labels in choose_from_menu_nested()

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.9.6
    • Fix Version/s: None
    • Component/s: Libraries
    • Labels:
      None
    • Difficulty:
      Easy
    • Affected Branches:
      MOODLE_19_STABLE
    • Rank:
      12978

      Description

      In lib/weblib.php around line 1011, inside function choose_from_menu_nested(), there is the following:

      $output .= ' <optgroup label="'. s(format_string($section)) .'">'."\n";

      The double escaping of the $section string is causing optgroup labels to display HTML codes such as "&"

      To reproduce the issue, simply put this code in a test page:

      <?php
      require_once 'config.php';

      $options['One & two'][1] = 'One';
      $options['One & two'][2] = 'Two';
      print choose_from_menu_nested($options, 'number');
      ?>

        Issue Links

          Activity

          Hide
          Rossiani Wijaya added a comment -

          Creating patch to fix the issue.

          the quickfix for this is to remove the format_string function.

          replace this:
          $output .= ' <optgroup label="'. s(format_string($section)) .'">'."\n";
          To:
          $output .= ' <optgroup label="'. s($section) .'">'."\n";

          Show
          Rossiani Wijaya added a comment - Creating patch to fix the issue. the quickfix for this is to remove the format_string function. replace this: $output .= ' <optgroup label="'. s(format_string($section)) .'">'."\n"; To: $output .= ' <optgroup label="'. s($section) .'">'."\n";
          Hide
          Sam Hemelryk added a comment -

          Hi Rossi,
          In this circumstance we should use format_string() rather than s().
          The reason for this is that format_string() cleans a string of all unwanted and potentially hazardous content, and in this case $section is potentially entered by the user.
          I would recommend changing to:
          $output .= ' <optgroup label="'. format_string($section) .'">'."\n";

          Show
          Sam Hemelryk added a comment - Hi Rossi, In this circumstance we should use format_string() rather than s(). The reason for this is that format_string() cleans a string of all unwanted and potentially hazardous content, and in this case $section is potentially entered by the user. I would recommend changing to: $output .= ' <optgroup label="'. format_string($section) .'">'."\n";
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam.

          patch updated.

          Show
          Rossiani Wijaya added a comment - Thanks Sam. patch updated.
          Hide
          Sam Hemelryk added a comment -

          Awesome thanks Rossi, +1 for commiting that now

          Show
          Sam Hemelryk added a comment - Awesome thanks Rossi, +1 for commiting that now
          Hide
          Rossiani Wijaya added a comment -

          The patch is created for version 1.9 and the issue has been resolved in HEAD.

          Show
          Rossiani Wijaya added a comment - The patch is created for version 1.9 and the issue has been resolved in HEAD.
          Hide
          Petr Škoda added a comment -

          reopening, this does not seem correct to me

          1/ the format_string()'s main purpose is to convert multilang strings to html, the XSS cleaning works for html texts only, not for individual parts of other html tags (such as label here)
          2/ the s() is used to get rid of "' and <> - in this particular case strings with " will break pretty badly and in fact if they come from student it would cause XSS

          So we have to use s(), we do not support html tags there - strip_tags() should deal with them. Now the question is do we want multilag support there? I suppose we need it there for BC reasons, so my +1 for:

           
          s(strip_tags(format_string($section)))
          
          Show
          Petr Škoda added a comment - reopening, this does not seem correct to me 1/ the format_string()'s main purpose is to convert multilang strings to html, the XSS cleaning works for html texts only, not for individual parts of other html tags (such as label here) 2/ the s() is used to get rid of "' and <> - in this particular case strings with " will break pretty badly and in fact if they come from student it would cause XSS So we have to use s(), we do not support html tags there - strip_tags() should deal with them. Now the question is do we want multilag support there? I suppose we need it there for BC reasons, so my +1 for: s(strip_tags(format_string($section)))
          Hide
          Rossiani Wijaya added a comment -

          Thanks Sam for fixing the issue according to petr's suggestion and committed to 1.9 Stable.

          Show
          Rossiani Wijaya added a comment - Thanks Sam for fixing the issue according to petr's suggestion and committed to 1.9 Stable.
          Hide
          Matthew N added a comment -

          This commit by Rossiani caused a regression with the site main menu block in edit mode. It now displays the resource's icon between all of the edit buttons. See the screenshot to see what I mean.

          Commit: http://cvs.moodle.org/moodle/lib/weblib.php?r1=1.970.2.152&r2=1.970.2.153&pathrev=MOODLE_19_WEEKLY&sortby=date

          The hunk in that commit which modified print_side_block cause the regression. It seems like only the first hunk was supposed to be committed when comparing to Rossiani's patch in this issue. The other changes seem mostly unrelated. I think someone just needs to backout all the hunks except for the first.

          Show
          Matthew N added a comment - This commit by Rossiani caused a regression with the site main menu block in edit mode. It now displays the resource's icon between all of the edit buttons. See the screenshot to see what I mean. Commit: http://cvs.moodle.org/moodle/lib/weblib.php?r1=1.970.2.152&r2=1.970.2.153&pathrev=MOODLE_19_WEEKLY&sortby=date The hunk in that commit which modified print_side_block cause the regression. It seems like only the first hunk was supposed to be committed when comparing to Rossiani's patch in this issue. The other changes seem mostly unrelated. I think someone just needs to backout all the hunks except for the first.
          Hide
          Rossiani Wijaya added a comment -

          Hi Matthew,

          thank you for testing the issue. The weblib.php i committed was not the latest version. Sam committed file would be the latest version for the issue. Please update the your version and let me know if you still having the issue.

          as for the other changes, it is used to fix other issue thats not related to this tracker.

          Show
          Rossiani Wijaya added a comment - Hi Matthew, thank you for testing the issue. The weblib.php i committed was not the latest version. Sam committed file would be the latest version for the issue. Please update the your version and let me know if you still having the issue. as for the other changes, it is used to fix other issue thats not related to this tracker.
          Hide
          Matthew N added a comment -

          Hello Rossiani,

          I was using the weekly build from Dec. 9, 2009 so it includes Sam's commit but my issue is not related to this bug but with the other changes (to print_side_block) that you committed with the patch for this issue.

          Have you tried to reproduce the issue for yourself by editing the site main menu? When I revert the additional changes committed and leave only the one related to this issue then the bug goes away.

          Do you have an issue number for the print_side_block changes so I can comment there instead as that code is the part that causes the regression.

          Thanks

          Show
          Matthew N added a comment - Hello Rossiani, I was using the weekly build from Dec. 9, 2009 so it includes Sam's commit but my issue is not related to this bug but with the other changes (to print_side_block) that you committed with the patch for this issue. Have you tried to reproduce the issue for yourself by editing the site main menu? When I revert the additional changes committed and leave only the one related to this issue then the bug goes away. Do you have an issue number for the print_side_block changes so I can comment there instead as that code is the part that causes the regression. Thanks
          Hide
          Rossiani Wijaya added a comment -

          Hi Matthew,

          the issue number for print_side_block change is MDL-6820. I submitted the patch to address the issue.

          please take a look.

          Thanks.

          Show
          Rossiani Wijaya added a comment - Hi Matthew, the issue number for print_side_block change is MDL-6820 . I submitted the patch to address the issue. please take a look. Thanks.
          Hide
          Sam Hemelryk added a comment -

          Have linked to an issue that I created specifically for the icon madness. Thank you for spotting that Matthew

          Show
          Sam Hemelryk added a comment - Have linked to an issue that I created specifically for the icon madness. Thank you for spotting that Matthew

            People

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

              Dates

              • Created:
                Updated:
                Resolved: