Moodle

Double escaping of OPTGROUP labels in choose_from_menu_nested()

Details

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

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

This issue has a non-specific relationship to:
MDL-21126 Icon madness in Main Menu block Major Closed

Activity

Ascending order - Click to sort in descending order
Hide
Permalink
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
Permalink
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
Permalink
Rossiani Wijaya added a comment -

Thanks Sam.

patch updated.

Show
Rossiani Wijaya added a comment - Thanks Sam. patch updated.
Hide
Permalink
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
Permalink
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
Permalink
Petr Škoda (skodak) 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 (skodak) 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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (2)

Dates

  • Created:
    Updated:
    Resolved:
Atlassian JIRA (v4.4.3#663-r165197) | Report a problem
Powered by a free Atlassian JIRA open source license for Moodle. Try JIRA - bug tracking software for your team.