Skip to content
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

Merged
merged 19 commits into from
Sep 26, 2023
Merged

Allow story body to use html tag iframe. #6844

merged 19 commits into from
Sep 26, 2023

Conversation

mwu2018
Copy link
Contributor

@mwu2018 mwu2018 commented Aug 17, 2023

What this PR does

Fixes #6843
The dompurity module thinks iframe tag is unsafe and is ignored by its default configuration. This PR add iframe as a safe tag in StoryBody 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

  • There are unit tests to verify my changes are correct or unit tests aren't applicable (if so, write quick reason why unit tests don't exist)
  • I've updated relevant documentation in doc/.
  • I've updated CHANGES.md with what I changed.
  • I've provided instructions in the PR description on how to test this PR.

@mwu2018 mwu2018 marked this pull request as ready for review August 18, 2023 01:17
@na9da
Copy link
Collaborator

na9da commented Aug 22, 2023

Some security context on allowing users to embed arbitrary iframe tags:

To support embedding videos, one safer solution might be to deny embedding iframes by default and safelist iframes from a few known sites like youtube or vimeo. Additional config options could be provided for the map owner to allow other sites.

cc @steve9164

@na9da
Copy link
Collaborator

na9da commented Aug 22, 2023

Example of how to restrict iframe usage by src.

@mwu2018
Copy link
Contributor Author

mwu2018 commented Sep 6, 2023

@na9da @steve9164 Could you please take another look? The iframe tag is allowed only if the source is from Youtube.

@nf-s
Copy link
Contributor

nf-s commented Sep 13, 2023

@steve9164 to check iframe attributes...

false,
{
ADD_TAGS: ["iframe"],
ALLOWED_ATTR: ["src", "width", "height"]
Copy link
Member

@steve9164 steve9164 Sep 15, 2023

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).

Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Contributor Author

@mwu2018 mwu2018 Sep 19, 2023

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.
@mwu2018 mwu2018 merged commit 5ee0029 into main Sep 26, 2023
5 checks passed
@mwu2018 mwu2018 deleted the issue-6843 branch September 26, 2023 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix broken embedded video link in story panel
4 participants