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

Double escaping of OPTGROUP labels in choose_from_menu_nested()

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

      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');
      ?>

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            rwijaya 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
            rwijaya 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
            samhemelryk 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
            samhemelryk 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
            rwijaya Rossiani Wijaya added a comment -

            Thanks Sam.

            patch updated.

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

            Awesome thanks Rossi, +1 for commiting that now

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

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

            Show
            rwijaya Rossiani Wijaya added a comment - The patch is created for version 1.9 and the issue has been resolved in HEAD.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            rwijaya Rossiani Wijaya added a comment -

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

            Show
            rwijaya Rossiani Wijaya added a comment - Thanks Sam for fixing the issue according to petr's suggestion and committed to 1.9 Stable.
            Hide
            mnoorenberghe 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
            mnoorenberghe 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
            rwijaya 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
            rwijaya 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
            mnoorenberghe 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
            mnoorenberghe 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
            rwijaya 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
            rwijaya 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
            samhemelryk 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
            samhemelryk 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: