fix: fixed embedded video not showing in about page#37698
Conversation
|
Thanks for the pull request, @Asespinel! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
22c8d2a to
7e841c8
Compare
|
Friendly ping on this, @felipemontoya |
|
Hi @Asespinel @felipemontoya! Is this still in progress? |
ChrisChV
left a comment
There was a problem hiding this comment.
@Asespinel Thanks for this work! I left some comments
| 'player.vimeo.com', | ||
| 'dailymotion.com', | ||
| 'www.dailymotion.com', | ||
| ] |
There was a problem hiding this comment.
It is missing geo.dailymotion.com: https://developers.dailymotion.com/docs/iframe-web
| # HTML Sanitization Settings | ||
| # Trusted domains for embedded content (iframes) in course about pages and similar content | ||
| # These domains are considered safe for embedding videos and other interactive content | ||
| ALLOWED_EMBED_HOSTS = [ |
There was a problem hiding this comment.
I think it would be better to define this setting in openedx/envs/common.py. What do you think?
|
Also, could you add some tests to this? |
Description
Fixes issue where embedded videos (YouTube iframes) appear correctly in Studio course about page preview but are stripped out and don't render in the LMS course about page.
The issue was introduced by the lxml upgrade from 4.9.4 to 5.3.2.
In lxml 5.0+, the
lxml.html.cleanmodule was deprecated and moved to theseparate
lxml-html-cleanpackage, which has different default behaviors.Problem
The
clean_dangerous_html()function inopenedx/core/djangolib/markup.pyuseslxml.html.clean.Cleanerwhich by default removes<iframe>elements for security reasons. This causes embedded videos to be stripped from course about pages when viewed in the LMS, even though they appear correctly in Studio preview.Observed behavior:
Solution
Use
host_whitelistwith configurable allowed domains via theALLOWED_EMBED_HOSTSDjango setting.Implementation:
ALLOWED_EMBED_HOSTSsetting inlms/envs/common.pyclean_dangerous_html()to read allowed hosts from settings onlyALLOWED_EMBED_HOSTSis not configured or empty, all iframes are blockedDefault configuration (in
lms/envs/common.py):Administrators can override this in production settings or via Tutor configuration.
Testing
The HTML used for the test is:
Before
After
Related issue:
#37655