-
Notifications
You must be signed in to change notification settings - Fork 376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow story body to use html tag iframe. #6844
Conversation
Some security context on allowing users to embed arbitrary
To support embedding videos, one safer solution might be to deny embedding cc @steve9164 |
Example of how to restrict |
@na9da @steve9164 Could you please take another look? The iframe tag is allowed only if the source is from Youtube. |
@steve9164 to check iframe attributes... |
false, | ||
{ | ||
ADD_TAGS: ["iframe"], | ||
ALLOWED_ATTR: ["src", "width", "height"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will restrict attributes for all tags to only these 3. This will stop e.g. embedded svgs from rendering, but also many other examples. See second scene of http://ci.terria.io/issue-6843/#share=s-ejPZyk2EJitArJp0T2naTpnok5Y&playStory=1 compared to the second scene of http://ci.terria.io/main/#share=s-ejPZyk2EJitArJp0T2naTpnok5Y&playStory=1 (the blue svg is not rendered any more).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other examples include text color added using scene editor being ignored
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why svg
tag is removed. Needs investigation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This restriction ALLOWED_ATTR: ["src", "width", "height"]
does not behave as expected. It did not remove the svg tag. Instead, it removes all properties of svg.g.path
.
Removing this restriction will work.
* No explict restrictions on attributes. * Update unit test.
What this PR does
Fixes #6843
The
dompurity
module thinksiframe
tag is unsafe and is ignored by its default configuration. This PR addiframe
as a safe tag inStoryBody
if only the video clips to be embedded are from the following three media source domains with specified paths:Note
The story editor allows user to insert a picture url but it will not be played back -- a different issue.
Test me
Try to create and SAVE a story with embedded video, then PLAY the story, using before and after the fix.
Checklist
doc/
.