Moodle
  1. Moodle
  2. MDL-30876

MP3 player not shown when use external YUI enabled

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.2, 2.3
    • Fix Version/s: 2.2.5, 2.3.2
    • Component/s: Filters
    • Labels:
    • Environment:
      Linux and Solaris servers with apache and mysql
    • Testing Instructions:
      Hide
      1. Enable the site setting 'useexternalyui'
      2. Enable the Multimedia filter (and mark the 'filter_mediaplugin_enable_mp3' checkbox if the option is available in your moodle version, otherwise is enabled)
      3. Add a link to a MP3 file (one of the user private files for example) in any HTML text area
      4. 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:
    • Pull Master Branch:
      MDL-30876_master
    • Rank:
      33883

      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:

      1. Enable the site setting 'useexternalyui'
      2. Enable the Multimedia filter and mark the 'filter_mediaplugin_enable_mp3' checkbox
      3. In any HTML editor click the Media Button and link to a MP3 file
      4. 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.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          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 - I'm finally catching up with the pre-christmas issues. Thanks for reporting that and providing a solution.
          Hide
          Renaat Debleu added a comment -

          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 - 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
          David Monllaó added a comment -

          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 - 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
          David Monllaó added a comment -

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

          Show
          David Monllaó added a comment - The MDL-30876 patch solves MDL-32310 and MDL-32582 , it includes the "allrules = null" checking
          Hide
          Rajesh Taneja added a comment -

          Thanks David,

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

          Show
          Rajesh Taneja added a comment - Thanks David, Patch looks good to me, pushing it for integration review.
          Hide
          Dan Poltawski added a comment -

          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 - 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
          Sam Hemelryk added a comment -

          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 - 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
          David Monllaó added a comment - - 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 - - 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
          Sam Hemelryk added a comment -

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

          Show
          Sam Hemelryk added a comment - Awesome thanks for clarifying that David, I've merged these changes in now
          Hide
          Ankit Agarwal added a comment -

          works as expected.
          Thanks

          Show
          Ankit Agarwal added a comment - works as expected. Thanks
          Hide
          Dan Poltawski added a comment -

          *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 - * 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

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

              Dates

              • Created:
                Updated:
                Resolved: