Moodle

youtube and vimeo embeding causes warnings on https sites

Details

  • Type: Bug Bug
  • Status: Closed 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
  • Pull Master Diff URL:

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

is blocked by

Bug - A problem which impairs or prevents Moodle from functioning correctly. MDL-28484 We do not have a function to detect if we are being served on https

  • Minor - Minor loss of function where workaround is possible
  • Development in progress - This issue is being actively worked on at the moment by the assignee.

Activity

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

Updated as per your request.

Show
Andrew Nicols added a comment - Updated as per your request.
Hide
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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
Permalink
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

Vote (0)
Watch (4)

Dates

  • Created:
    Updated:
    Resolved: