Moodle
  1. Moodle
  2. MDL-28486

youtube and vimeo embeding causes warnings on https sites

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.3
    • Fix Version/s: 2.3.2
    • Component/s: Filters
    • Labels:
    • Environment:
      Internet Explorer
    • Testing Instructions:
      Hide

      General tests

      The following tests apply to MOODLE_23_STABLE and to master:

      • Navigate to Settings -> Site administration -> Appearance -> Media
        • Ensure that the YouTube and Vimeo options are ticked
      • Navigate to a course page
      • Turn editing on
      • Edit a section
      • Set the section Summary to:
        <p><a href="http://www.youtube.com/v/1akyXgddyAY">Example YouTube</a></p>
        <p><a href="http://vimeo.com/1234567">Example Vimeo</a></p>
        <p><a href="http://www.youtube.com/playlist?list=PL9B6AC5E479B1B992">Example Playlist</a></p>
        
      • Save changes
        • Confirm that the Top video (YouTube) is displayed
        • Confirm that the Middle video (Vimeo) is displayed
        • Confirm that the Bottom video (YouTube Playlist) is displayed
        • View the source and confirm that each video is pointed at the correct (https) URL
      • Open the 'Activity Chooser'
        • Confirm that none of the videos sit in front of the activity chooser

      MOODLE_23_STABLE

      The following instructions are in addition to the tests above. Moodle 2.3 still has the XML Strict Headers setting which slightly changes the way that some videos are displayed:

      • Navigate to Site administration -> Development -> Debugging
      • Turn "XML strict headers" to on
      • View the course page again
        • Confirm that the Top video (YouTube) is displayed
        • Confirm that it is not in an iframe
        • Confirm that the Middle video (Vimeo) is displayed
        • Confirm that the Bottom video (YouTube Playlist) is displayed
      • Open the 'Activity Chooser'
        • Confirm that none of the videos sit in front of the activity chooser
      Show
      General tests The following tests apply to MOODLE_23_STABLE and to master: Navigate to Settings -> Site administration -> Appearance -> Media Ensure that the YouTube and Vimeo options are ticked Navigate to a course page Turn editing on Edit a section Set the section Summary to: <p><a href= "http: //www.youtube.com/v/1akyXgddyAY" >Example YouTube</a></p> <p><a href= "http: //vimeo.com/1234567" >Example Vimeo</a></p> <p><a href= "http: //www.youtube.com/playlist?list=PL9B6AC5E479B1B992" >Example Playlist</a></p> Save changes Confirm that the Top video (YouTube) is displayed Confirm that the Middle video (Vimeo) is displayed Confirm that the Bottom video (YouTube Playlist) is displayed View the source and confirm that each video is pointed at the correct (https) URL Open the 'Activity Chooser' Confirm that none of the videos sit in front of the activity chooser MOODLE_23_STABLE The following instructions are in addition to the tests above. Moodle 2.3 still has the XML Strict Headers setting which slightly changes the way that some videos are displayed: Navigate to Site administration -> Development -> Debugging Turn "XML strict headers" to on View the course page again Confirm that the Top video (YouTube) is displayed Confirm that it is not in an iframe Confirm that the Middle video (Vimeo) is displayed Confirm that the Bottom video (YouTube Playlist) is displayed Open the 'Activity Chooser' Confirm that none of the videos sit in front of the activity chooser
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-28486-master-2
    • Rank:
      18145

      Description

      IE will give insecure content warnings if youtube is used with http and the page is httpsed. Youtube support https embeding (see: http://apiblog.youtube.com/2011/02/https-support-for-youtube-embeds.html ), so we can support this.

      Other browsers also give similar warnings

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Patch to achieve this attached - but this depends on MDL-28484

          Show
          Dan Poltawski added a comment - Patch to achieve this attached - but this depends on MDL-28484
          Hide
          Andrew Nicols added a comment -

          Patch no longer works against 2.3 due to complete rewrite of multimedia filter.

          Show
          Andrew Nicols added a comment - Patch no longer works against 2.3 due to complete rewrite of multimedia filter.
          Hide
          Dan Poltawski added a comment -

          Andrew: if you are interested in this patch, I was thinking we could change the youtube embeding to always use https. But I have not had time to look into if doing that gives IE 'mixed https' warnings and doesn't solve the problem. It seems like an integratable solution irrespective of linked issue.

          Show
          Dan Poltawski added a comment - Andrew: if you are interested in this patch, I was thinking we could change the youtube embeding to always use https. But I have not had time to look into if doing that gives IE 'mixed https' warnings and doesn't solve the problem. It seems like an integratable solution irrespective of linked issue.
          Hide
          Andrew Nicols added a comment -

          I considered this too - it should be fine. Not sure whether vimeo does the same.

          Show
          Andrew Nicols added a comment - I considered this too - it should be fine. Not sure whether vimeo does the same.
          Hide
          Andrew Nicols added a comment -

          Following discussion in the developer chat, I'm going to convert both youtube and vimeo to be always https.
          At present, it's not possible to reliably determine the current URL in a filter because the global $PAGE object shouldn't be accessed from a filter.

          Show
          Andrew Nicols added a comment - Following discussion in the developer chat, I'm going to convert both youtube and vimeo to be always https. At present, it's not possible to reliably determine the current URL in a filter because the global $PAGE object shouldn't be accessed from a filter.
          Hide
          Andrew Nicols added a comment -

          I've replaced all Vimeo and YouTube links with their SSL equivalents.

          I've also replaced the YouTube playlist embedding with the iframe version as embed support for youtube playlists seems to be non-existent and I can't get the existing code to work under any circumstances.

          I've also taken the opportunity to add a missing <span> around the iframe for youtube videos.

          Show
          Andrew Nicols added a comment - I've replaced all Vimeo and YouTube links with their SSL equivalents. I've also replaced the YouTube playlist embedding with the iframe version as embed support for youtube playlists seems to be non-existent and I can't get the existing code to work under any circumstances. I've also taken the opportunity to add a missing <span> around the iframe for youtube videos.
          Hide
          Andrew Nicols added a comment -

          These fixes only affect 2.3 and master.

          Show
          Andrew Nicols added a comment - These fixes only affect 2.3 and master.
          Hide
          Dan Poltawski added a comment -

          Thanks for taking this Andrew.

          Some comments:

          • Please could you put a brief mention of why we are forcing https in the commit message. I'm sure there will be some https haters, so worth putting the rationale in.
          • The xml strict headers stuff is removed in master, so you'll need to correct the patch for that
          • It'll be easier for us to integrate this if you state the range of browsers you've tested this change on.

          cheers,
          dan

          Show
          Dan Poltawski added a comment - Thanks for taking this Andrew. Some comments: Please could you put a brief mention of why we are forcing https in the commit message. I'm sure there will be some https haters, so worth putting the rationale in. The xml strict headers stuff is removed in master, so you'll need to correct the patch for that It'll be easier for us to integrate this if you state the range of browsers you've tested this change on. cheers, dan
          Hide
          Andrew Nicols added a comment -

          Updated as per your request.

          Show
          Andrew Nicols added a comment - Updated as per your request.
          Hide
          Andrew Nicols added a comment -

          Tested with non-SSL moodle on:

          • OS X Lion - Chrome 20
          • OS X Lion - Opera 12
          • OS X Lion - Safari 5.1
          • OS X Lion - Safari 6
          • OS X Lion - Firefox 13
          • OS X Lion - Firefox 14
          • Windows 7 - Chrome 20
          • Windows 7 - IE9
          • Windows 7 - Opera 11
          • Windows 7 - Firefox 13

          Tested with SSL moodle on:

          • OS X Lion - Chrome 20
          • OS X Lion - Opera 12
          • OS X Lion - Safari 5.1
          • OS X Lion - Safari 6
          • OS X Lion - Firefox 13
          • OS X Lion - Firefox 14
          • Windows 7 - Chrome 20
          • Windows 7 - IE9
          • Windows 7 - Opera 11
          • Windows 7 - Firefox 13
          Show
          Andrew Nicols added a comment - Tested with non-SSL moodle on: OS X Lion - Chrome 20 OS X Lion - Opera 12 OS X Lion - Safari 5.1 OS X Lion - Safari 6 OS X Lion - Firefox 13 OS X Lion - Firefox 14 Windows 7 - Chrome 20 Windows 7 - IE9 Windows 7 - Opera 11 Windows 7 - Firefox 13 Tested with SSL moodle on: OS X Lion - Chrome 20 OS X Lion - Opera 12 OS X Lion - Safari 5.1 OS X Lion - Safari 6 OS X Lion - Firefox 13 OS X Lion - Firefox 14 Windows 7 - Chrome 20 Windows 7 - IE9 Windows 7 - Opera 11 Windows 7 - Firefox 13
          Hide
          Aparup Banerjee 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
          Aparup Banerjee 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 -

          Hi Andrew,

          If possible could you please produce branches for the stable versions you'd like this to be backported to.
          Looks to me like it would be well worth backporting, but I see there are conflicts when backporting.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, If possible could you please produce branches for the stable versions you'd like this to be backported to. Looks to me like it would be well worth backporting, but I see there are conflicts when backporting. Cheers Sam
          Hide
          Andrew Nicols added a comment -

          Hi Sam,

          Apologies for that - the original change did cherry-pick cleanly into 2.3, but the xmlstrict change last week broke that and I forgot to create an appropriate branch.

          This will revert back to the object embedding method rather than iframes if xmlstrictheaders is set for YouTube Videos only.

          Show
          Andrew Nicols added a comment - Hi Sam, Apologies for that - the original change did cherry-pick cleanly into 2.3, but the xmlstrict change last week broke that and I forgot to create an appropriate branch. This will revert back to the object embedding method rather than iframes if xmlstrictheaders is set for YouTube Videos only.
          Hide
          Andrew Nicols added a comment -

          Sam Marshall: Adding you as a watcher to this issue as I know that you were interested in this.

          Show
          Andrew Nicols added a comment - Sam Marshall: Adding you as a watcher to this issue as I know that you were interested in this.
          Hide
          Dan Poltawski added a comment -

          The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday.

          This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P
          Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao

          Show
          Dan Poltawski added a comment - The integration of this issue has been delayed to next week because the integration period is over (Monday, Tuesday) and testing must happen on Wednesday. This change to a more rigid timeframe on each integration/testing cycle aims to produce a better and clear separation and organization of tasks for everybody. This is a bulk-automated message, so if you want to blame somebody/thing/where, don't do it here (use git instead) :-D :-P Apologies for the inconvenience, this will be integrated next week. Thanks for your collaboration & ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew, this has been integrated now
          Hide
          Dan Poltawski added a comment -

          Failing, this has broken unit tests:

          medialib_testcase::test_embed_url_other_formats
          Failed asserting that false is true.

          Show
          Dan Poltawski added a comment - Failing, this has broken unit tests: medialib_testcase::test_embed_url_other_formats Failed asserting that false is true.
          Hide
          Dan Poltawski added a comment -

          Have integrated a fix for that. I wonder if we could have a more intelligent test there, checking size etc.

          Show
          Dan Poltawski added a comment - Have integrated a fix for that. I wonder if we could have a more intelligent test there, checking size etc.
          Hide
          Andrew Davis added a comment -

          Looks good on both 2.3 and master. Passing.

          Show
          Andrew Davis added a comment - Looks good on both 2.3 and master. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Fixed STOP Closed STOP Thanks STOP

          Yay, imagination! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Fixed STOP Closed STOP Thanks STOP Yay, imagination! Ciao

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: