MP3 player not shown when use external YUI enabled

Details

Type: Bug
Status: Closed
Priority: Major
Resolution: Fixed
Affects Version/s: 2.2, 2.3
Fix Version/s: 2.2.5, 2.3.2
Component/s: Filters
Labels:
- patch
- triaged
Environment:
Linux and Solaris servers with apache and mysql

Testing Instructions:
Hide

Enable the site setting 'useexternalyui'

Enable the Multimedia filter (and mark the 'filter_mediaplugin_enable_mp3' checkbox if the option is available in your moodle version, otherwise is enabled)

Add a link to a MP3 file (one of the user private files for example) in any HTML text area

View the page with the HTML text displayed, the link SHOULD BE replaced by the MP3 player and there SHOULD NOT BE a JS error
Show
Enable the site setting 'useexternalyui' Enable the Multimedia filter (and mark the 'filter_mediaplugin_enable_mp3' checkbox if the option is available in your moodle version, otherwise is enabled) Add a link to a MP3 file (one of the user private files for example) in any HTML text area View the page with the HTML text displayed, the link SHOULD BE replaced by the MP3 player and there SHOULD NOT BE a JS error
Workaround:

Hide

Access to the style sheets wrapped in a try & catch. This solution does not replicate the same exact funcionality with 'useexternalyui' enabled, but I'm afraid that it's not possible, is a JS security restriction.

Solution tested on Firefox, Chrome and Opera; IE is, as always, another world, it doesn't replace the player with 'userexternalyui' enabled nor disabled, but it seems that there is no security violation when accessing cross domain style sheets, since it's another issue I think that you prefer to have it on another tracker issue.

Grepping around moodle core code to find more 'document.styleSheets' to prevent other JS security violations when online YUI libraries is enabled, it seems that this is the only place where the styles sheets are accessed this way; the other apperance is in the jquery lib of xhprof which I suppose is stand-alone and not dependent of the moodle core YUI libraries.

Show
Access to the style sheets wrapped in a try & catch. This solution does not replicate the same exact funcionality with 'useexternalyui' enabled, but I'm afraid that it's not possible, is a JS security restriction. Solution tested on Firefox, Chrome and Opera; IE is, as always, another world, it doesn't replace the player with 'userexternalyui' enabled nor disabled, but it seems that there is no security violation when accessing cross domain style sheets, since it's another issue I think that you prefer to have it on another tracker issue. Grepping around moodle core code to find more 'document.styleSheets' to prevent other JS security violations when online YUI libraries is enabled, it seems that this is the only place where the styles sheets are accessed this way; the other apperance is in the jquery lib of xhprof which I suppose is stand-alone and not dependent of the moodle core YUI libraries.
Affected Branches:
MOODLE_22_STABLE, MOODLE_23_STABLE
Fixed Branches:
MOODLE_22_STABLE, MOODLE_23_STABLE
Pull from Repository:
https://github.com/dmonllao/moodle
Pull Master Branch:
~~MDL-30876~~_master
Pull Master Diff URL:
https://github.com/dmonllao/moodle/compare/master...MDL-30876_master

Description

With the multimedia filter enabled and MP3 checked, if the YUI libraries are obtained from another domain (useexternalyui site setting enabled) the MP3 player is not shown

Reproduction steps:

Enable the site setting 'useexternalyui'
Enable the Multimedia filter and mark the 'filter_mediaplugin_enable_mp3' checkbox
In any HTML editor click the Media Button and link to a MP3 file
View the page with the HTML text displayed, the link is not replaced by the MP3 player

It throws a Javascript 'Security error" code: "1000' error (is trying to access the contents of another domain's stylesheet) and stops the Javascritp execution, therefore the link to the MP3 file is not replaced by the MP3 player.

Options
- Sort By Name
- Sort By Date
- Ascending
- Descending
- Download All

Attachments

YUIonline_exception.png

194 kB

16/Jul/12 10:03 AM

Issue Links

is duplicated by

MDL-32310 mediaplugin mp3 player not shown in chrome

will help resolve

MDL-32310 mediaplugin mp3 player not shown in chrome

MDL-32582 MP3 Flow Player doesn't work in Chrome if empty stylesheets are present

Activity

Ascending order - Click to sort in descending order

Hide

Permalink

Michael de Raadt added a comment - 25/Jan/12 11:18 AM

I'm finally catching up with the pre-christmas issues.

Thanks for reporting that and providing a solution.

Show

Michael de Raadt added a comment - 25/Jan/12 11:18 AM I'm finally catching up with the pre-christmas issues. Thanks for reporting that and providing a solution.

Hide

Permalink

Renaat Debleu added a comment - 10/Mar/12 8:23 PM

The file /lib/javascript-static.js is also generating security errors when using web fonts - some chrome plugins....
I modified the file (try catch block & extra null checking) and the mp3 player is showing up again in Firefox-Chrome-IE :

javascript-static.js

for (var j=0; j < document.styleSheets.length; j++) {
   var allrules = new Array();
   try {
      if (typeof (document.styleSheets[j].rules) != 'undefined') {
         allrules = document.styleSheets[j].rules;
      } else if (typeof (document.styleSheets[j].cssRules) != 'undefined') {
         allrules = document.styleSheets[j].cssRules;
      } else {
         // why??
         continue;
      }
      //chrome plugins - web fonts - ...
      if (allrules == null) {
         continue;
      }
              
      for(var i=0; i<allrules.length; i++) {
         rule = '';
         if (/^\.mp3flowplayer_.*Color$/.test(allrules[i].selectorText)) {
           if (typeof(allrules[i].cssText) != 'undefined') {
              rule = allrules[i].cssText;
           } else if (typeof(allrules[i].style.cssText) != 'undefined') {
              rule = allrules[i].style.cssText;
           }
           if (rule != '' && /.*color\s*:\s*([^;]+).*/gi.test(rule)) {
              rule = rule.replace(/.*color\s*:\s*([^;]+).*/gi, '$1');
              var colprop = allrules[i].selectorText.replace(/^\.mp3flowplayer_/, '');
              controls[colprop] = rule;
           }
         }
      }
      allrules = false;
   }
   catch (err) {
      //Houston, we have a security problem
      continue;
   }
}

Show

Renaat Debleu added a comment - 10/Mar/12 8:23 PM The file /lib/javascript-static.js is also generating security errors when using web fonts - some chrome plugins.... I modified the file (try catch block & extra null checking) and the mp3 player is showing up again in Firefox-Chrome-IE : javascript-static.js for ( var j=0; j < document.styleSheets.length; j++) { var allrules = new Array(); try { if (typeof (document.styleSheets[j].rules) != 'undefined') { allrules = document.styleSheets[j].rules; } else if (typeof (document.styleSheets[j].cssRules) != 'undefined') { allrules = document.styleSheets[j].cssRules; } else { // why?? continue ; } //chrome plugins - web fonts - ... if (allrules == null ) { continue ; } for ( var i=0; i<allrules.length; i++) { rule = ''; if (/^\.mp3flowplayer_.*Color$/.test(allrules[i].selectorText)) { if (typeof(allrules[i].cssText) != 'undefined') { rule = allrules[i].cssText; } else if (typeof(allrules[i].style.cssText) != 'undefined') { rule = allrules[i].style.cssText; } if (rule != '' && /.*color\s*:\s*([^;]+).*/gi.test(rule)) { rule = rule.replace(/.*color\s*:\s*([^;]+).*/gi, '$1'); var colprop = allrules[i].selectorText.replace(/^\.mp3flowplayer_/, ''); controls[colprop] = rule; } } } allrules = false ; } catch (err) { //Houston, we have a security problem continue ; } }

Hide

Permalink

David Monllaó added a comment - 06/Jul/12 10:08 AM

Hi Renaat,

The attached patches https://github.com/dmonllao/moodle/compare/master...MDL-30876_master (for example) should solve your problem as well, can you please verify it or provide steps to reproduce your problem? Thanks in advance

Show

David Monllaó added a comment - 06/Jul/12 10:08 AM Hi Renaat, The attached patches https://github.com/dmonllao/moodle/compare/master...MDL-30876_master (for example) should solve your problem as well, can you please verify it or provide steps to reproduce your problem? Thanks in advance

Hide

Permalink

David Monllaó added a comment - 09/Jul/12 9:40 AM

The ~~MDL-30876~~ patch solves ~~MDL-32310~~ and ~~MDL-32582~~, it includes the "allrules = null" checking

Show

David Monllaó added a comment - 09/Jul/12 9:40 AM The MDL-30876 patch solves MDL-32310 and MDL-32582 , it includes the "allrules = null" checking

Hide

Permalink

Rajesh Taneja added a comment - 11/Jul/12 2:23 PM

Thanks David,

Patch looks good to me, pushing it for integration review.

Show

Rajesh Taneja added a comment - 11/Jul/12 2:23 PM Thanks David, Patch looks good to me, pushing it for integration review.

Hide

Permalink

Dan Poltawski added a comment - 12/Jul/12 6:03 PM

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

TIA and ciao

Show

Dan Poltawski added a comment - 12/Jul/12 6:03 PM The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao

Hide

Permalink

Sam Hemelryk added a comment - 16/Jul/12 7:42 AM

This issue looked very familiar.
I integrated a fix for this earlier today ~~MDL-32310~~.

David can you tell me, the try...catch that has been introduced here, is it required (do any browsers throw exceptions)?
If so then certainly this fix needs to be merged into the existing fix. If not then I think the current fix should be enough.

I'll leave this in integration until I've heard back from you David, and will ping Dan as well to get his thoughts.

Cheers
Sam

Show

Sam Hemelryk added a comment - 16/Jul/12 7:42 AM This issue looked very familiar. I integrated a fix for this earlier today MDL-32310 . David can you tell me, the try...catch that has been introduced here, is it required (do any browsers throw exceptions)? If so then certainly this fix needs to be merged into the existing fix. If not then I think the current fix should be enough. I'll leave this in integration until I've heard back from you David, and will ping Dan as well to get his thoughts. Cheers Sam

Hide

Permalink

David Monllaó added a comment - 16/Jul/12 10:03 AM - edited

Hi Sam,

Yes, Firefox reports the exception and stops the JS execution, this patch includes the other one. Attaching exception thrown with the ~~MDL-32310~~ patch applied

Show

David Monllaó added a comment - 16/Jul/12 10:03 AM - edited Hi Sam, Yes, Firefox reports the exception and stops the JS execution, this patch includes the other one. Attaching exception thrown with the MDL-32310 patch applied

Hide

Permalink

Sam Hemelryk added a comment - 17/Jul/12 8:22 AM

Awesome thanks for clarifying that David, I've merged these changes in now

Show

Sam Hemelryk added a comment - 17/Jul/12 8:22 AM Awesome thanks for clarifying that David, I've merged these changes in now

Hide

Permalink

Ankit Agarwal added a comment - 18/Jul/12 12:51 PM

works as expected.
Thanks

Show

Ankit Agarwal added a comment - 18/Jul/12 12:51 PM works as expected. Thanks

Hide

Permalink

Dan Poltawski added a comment - 19/Jul/12 2:43 PM

*Notice*: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26

Congratulations

{tracker.user.name}

You've made into Moodle

{tracker.fixversion-1}

I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world.

cheers!

{tracker.friendlyintegrator}

Show

Dan Poltawski added a comment - 19/Jul/12 2:43 PM * Notice *: Undefined variable: friendlyintegrator in /Users/danp/git/tokenintegrationthanks.php on line 26 Congratulations {tracker.user.name} ! You've made into Moodle {tracker.fixversion-1} + I would like to personally thank you for this contribution on behalf of all Moodle users throughout the world. cheers! {tracker.friendlyintegrator}

People

Assignee:

David Monllaó

Reporter:

David Monllaó

Peer reviewer:

Rajesh Taneja

Integrator:

Sam Hemelryk

Tester:

Ankit Agarwal

Participants:

Ankit Agarwal, Dan Poltawski, David Monllaó, Michael de Raadt, Rajesh Taneja, Renaat Debleu, Sam Hemelryk

Vote (2)

Watch (2)

Dates

Created:

22/Dec/11 11:24 PM

Updated:

19/Jul/12 2:43 PM

Resolved:

19/Jul/12 2:43 PM

Agile

View on Board