Moodle
  1. Moodle
  2. MDL-5223

buggy htmlentities call in mediaplugin filter

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 1.6, 1.9.5
    • Fix Version/s: 1.9.6
    • Component/s: Filters
    • Labels:
      None
    • Environment:
      All
    • Affected Branches:
      MOODLE_16_STABLE, MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      28205

      Description

      In \filter\mediaplugin\filter.php there is a buggy call to htmlentities which causes the $THEME->filter_mediaplugin_colors parameters to never reach their intended destination, i.e. flashvars.

      Removing line 33: $c = htmlentities($c);

      solves the problem.

      Joseph

        Activity

        Hide
        Martin Dougiamas added a comment -

        From Petr Skoda (skodak at centrum.cz) Tuesday, 18 April 2006, 05:22 AM:

        Hi!

        I am not using flash or mediaplugin myself, you will have to assign it to somebody else

        I guess you might try Jamie Pratt...

        From Joseph R?zeau (joseph.rezeau at uhb.fr) Tuesday, 18 April 2006, 05:39 AM:

        Martin, could you re-assign this bug please?

        Joseph

        From Martin Dougiamas (martin at moodle.com) Thursday, 20 April 2006, 04:26 PM:

        It might fix your problem, but does it cause more problems with different input?

        What is the exact case you are using there?

        I've checked this into CVS on the assumption you're right, but this might need more testing.

        From Joseph R?zeau (joseph.rezeau at uhb.fr) Friday, 21 April 2006, 12:35 AM:

        Hi Martin!

        >It might fix your problem, but does it cause more problems with different input?

        Well, I don't think there is another input going through there...

        >What is the exact case you are using there?

        Sorry, what do you mean by case ? UPPERCASE vs lower case?

        Anyway, I have tested this bug again, and I can confirm that it has been there since at least version 1.5. In fact, NO CHANGES take place at all in the colors of the Flash player!!! This is because the faulty call to htmlentities changes the correct string, e.g.

        bgColour=000000&btnColour=ffffff&btnBorderColour=cccccc&iconColour=000000&iconOverColour=FF0000&trackColour=cccccc&handleColour=ffffff&loaderColour=ffffff&waitForPlay=yes&

        into this garbage:

        flashvars=bgColour=000000&btnColour=ffffff&btnBorderColour=cccccc&iconColour=000000&iconOverColour=FF0000&trackColour=cccccc&handleColour=ffffff&loaderColour=ffffff&waitForPlay=yes&

        And I suspect that this bug has gone un-noticed all those years simply because no-one has ever taken the trouble to change the colors of the Flash player! I've noticed that ALL the themes which are currently distributed with Moodle simply copy exactly the same parameters as those from \filter\mediaplugin\filter.php. And it also goes un-noticed because the colors coded in filter.php are the same as those hard-coded in the Flash file! (i.e. black and green).

        So I recommend deleting the faulty line

        $c = htmlentities($c);

        so everything works as expected.

        Joseph

        From Martin Dougiamas (martin at moodle.com) Friday, 21 April 2006, 12:26 PM:

        OK, great, thanks for the thining on this, fixed in 1.5.x and HEAD

        Show
        Martin Dougiamas added a comment - From Petr Skoda (skodak at centrum.cz) Tuesday, 18 April 2006, 05:22 AM: Hi! I am not using flash or mediaplugin myself, you will have to assign it to somebody else I guess you might try Jamie Pratt... From Joseph R?zeau (joseph.rezeau at uhb.fr) Tuesday, 18 April 2006, 05:39 AM: Martin, could you re-assign this bug please? Joseph From Martin Dougiamas (martin at moodle.com) Thursday, 20 April 2006, 04:26 PM: It might fix your problem, but does it cause more problems with different input? What is the exact case you are using there? I've checked this into CVS on the assumption you're right, but this might need more testing. From Joseph R?zeau (joseph.rezeau at uhb.fr) Friday, 21 April 2006, 12:35 AM: Hi Martin! >It might fix your problem, but does it cause more problems with different input? Well, I don't think there is another input going through there... >What is the exact case you are using there? Sorry, what do you mean by case ? UPPERCASE vs lower case? Anyway, I have tested this bug again, and I can confirm that it has been there since at least version 1.5. In fact, NO CHANGES take place at all in the colors of the Flash player!!! This is because the faulty call to htmlentities changes the correct string, e.g. bgColour=000000&btnColour=ffffff&btnBorderColour=cccccc&iconColour=000000&iconOverColour=FF0000&trackColour=cccccc&handleColour=ffffff&loaderColour=ffffff&waitForPlay=yes& into this garbage: flashvars=bgColour=000000&btnColour=ffffff&btnBorderColour=cccccc&iconColour=000000&iconOverColour=FF0000&trackColour=cccccc&handleColour=ffffff&loaderColour=ffffff&waitForPlay=yes& And I suspect that this bug has gone un-noticed all those years simply because no-one has ever taken the trouble to change the colors of the Flash player! I've noticed that ALL the themes which are currently distributed with Moodle simply copy exactly the same parameters as those from \filter\mediaplugin\filter.php. And it also goes un-noticed because the colors coded in filter.php are the same as those hard-coded in the Flash file! (i.e. black and green). So I recommend deleting the faulty line $c = htmlentities($c); so everything works as expected. Joseph From Martin Dougiamas (martin at moodle.com) Friday, 21 April 2006, 12:26 PM: OK, great, thanks for the thining on this, fixed in 1.5.x and HEAD
        Hide
        Michael Blake added a comment -

        assign to a valid user

        Show
        Michael Blake added a comment - assign to a valid user
        Hide
        Ashley Holman added a comment -

        The htmlentities line has crept back in, and is breaking the flash vars as the original author stated.

        I came across this because the waitForPlay option was doing nothing. Removing the htmlentities() call fixes this.

        I guess the real solution is to apply htmlentities() to the each of the parameter values but keep the & separators literal.

        Show
        Ashley Holman added a comment - The htmlentities line has crept back in, and is breaking the flash vars as the original author stated. I came across this because the waitForPlay option was doing nothing. Removing the htmlentities() call fixes this. I guess the real solution is to apply htmlentities() to the each of the parameter values but keep the & separators literal.
        Hide
        Gordon Bateson added a comment -

        > I guess the real solution is to apply htmlentities() to the each of the parameter values
        > but keep the & separators literal.

        Good idea. Here is a suggested fix for "filter/mediaplugin/filter.php":

        OLD: $c = htmlentities($c);
        NEW: $c = implode('&', array_map('htmlentities', explode('&', $c)));

        Show
        Gordon Bateson added a comment - > I guess the real solution is to apply htmlentities() to the each of the parameter values > but keep the & separators literal. Good idea. Here is a suggested fix for "filter/mediaplugin/filter.php": OLD: $c = htmlentities($c); NEW: $c = implode('&', array_map('htmlentities', explode('&', $c)));
        Hide
        Ashley Holman added a comment -

        Now fixed in the latest 19_STABLE using the fix that the original reporter suggested.

        Gordon, I did try your patch however it looks like htmlentities() is not appropriate, we actually want urlencode() to escape chars in URL format. The problem is, the splitting on '&' is not enough since urlencode will also escape the '=' sign as well. Ideally we would have a proper array containing flash vars which then get escaped through urlencode(), however this change would impact all theme config.php files so not something we should introduce into 19_STABLE.

        Show
        Ashley Holman added a comment - Now fixed in the latest 19_STABLE using the fix that the original reporter suggested. Gordon, I did try your patch however it looks like htmlentities() is not appropriate, we actually want urlencode() to escape chars in URL format. The problem is, the splitting on '&' is not enough since urlencode will also escape the '=' sign as well. Ideally we would have a proper array containing flash vars which then get escaped through urlencode(), however this change would impact all theme config.php files so not something we should introduce into 19_STABLE.
        Hide
        Martin Dougiamas added a comment -

        Petr could you just quickly check this for injections/XSS ?

        Show
        Martin Dougiamas added a comment - Petr could you just quickly check this for injections/XSS ?
        Hide
        Petr Škoda added a comment -

        looks ok, the data is not coming from user, so the worst alternative is that it would not validate as xhtml strict

        Show
        Petr Škoda added a comment - looks ok, the data is not coming from user, so the worst alternative is that it would not validate as xhtml strict
        Hide
        Ashley Holman added a comment -

        Also just checked this into HEAD.

        Note to theme designers: you should now be able to customise mp3 player settings/colours as documented in: http://docs.moodle.org/en/MP3_player

        If you need to use special characters in flash vars, use appropriate URL encoding when setting $THEME->resource_mp3player_colors and $THEME->filter_mediaplugin_colors

        See http://www.w3schools.com/TAGS/ref_urlencode.asp

        Show
        Ashley Holman added a comment - Also just checked this into HEAD. Note to theme designers: you should now be able to customise mp3 player settings/colours as documented in: http://docs.moodle.org/en/MP3_player If you need to use special characters in flash vars, use appropriate URL encoding when setting $THEME->resource_mp3player_colors and $THEME->filter_mediaplugin_colors See http://www.w3schools.com/TAGS/ref_urlencode.asp
        Hide
        Martin Dougiamas added a comment -

        Just specify the first version it was fixed in. Thanks

        Show
        Martin Dougiamas added a comment - Just specify the first version it was fixed in. Thanks
        Hide
        Dongsheng Cai added a comment -

        thanks everyone, closing

        Show
        Dongsheng Cai added a comment - thanks everyone, closing

          People

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

            Dates

            • Created:
              Updated:
              Resolved: