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

Youtube iframe 'src' parameter stripped out after saving an entry #278

Closed
sidm1983 opened this issue Oct 13, 2020 · 7 comments
Closed

Youtube iframe 'src' parameter stripped out after saving an entry #278

sidm1983 opened this issue Oct 13, 2020 · 7 comments
Assignees
Labels

Comments

@sidm1983
Copy link

sidm1983 commented Oct 13, 2020

Description

So I was just testing the default HTML Purifier config that now allows embedding of youtube videos properly and doesn't strip it out on saving of an entry, however it is still not working properly. I believe this is because the regex used to determine whether the iframe src URL is either youtube or vimeo is not quite right and doesn't account for an edge case in youtube urls that omit the https: part at the beginning of the URL.

Steps to reproduce

  1. Go to youtube and grab any video URL (e.g. https://www.youtube.com/watch?v=roY6H75d9wE)
  2. Use the video embed icon in redactor to add the above youtube video to the editor.
  3. Save the entry
  4. Come back and edit the same entry
  5. You will see that the youtube video thumbnail has disappeared. If you look at the HTML in the editor, the iframe itself is there, it is just missing the 'src' parameter.

Solution

This is happening because when you first embed the youtube video, the resulting HTML looks like this:

<figure><iframe style="width: 500px; height: 281px;" src="//www.youtube.com/embed/roY6H75d9wE" frameborder="0" allowfullscreen=""></iframe></figure>

As you can see above, the src parameter starts with //, instead of https://

As a result, the above src value gets stripped out because the current regex being used doesn't account for the possibility of a missing scheme:

'URI.SafeIframeRegexp' => '%^https?://(www.youtube.com/embed/|player.vimeo.com/video/)%',

Here is a regex I've used in the past that works with this edge case:

'URI.SafeIframeRegexp' => '%^(https?:)?//(www\.youtube(?:-nocookie)?\.com/embed/|player\.vimeo\.com/video/)%',

Please note the additional brackets and question mark around the https?: part marking it as optional. It also accounts for youtube's nocookie URLs. Lastly, the . (dot) characters have also been escaped as we want it to match an actual dot and not just any character.

Additional info

  • Craft version: 3.5.12.1
  • PHP version: 7.3
  • Database driver & version: pgsql 11
  • Plugins & versions: redactor 2.8.1
@sidm1983 sidm1983 added the bug label Oct 13, 2020
@sidm1983
Copy link
Author

Also, I think those . (dot) characters in the regex need to be escaped properly as the intent is to match a literal . character and not any character. I have updated the regex solution above to escape them.

@sidm1983
Copy link
Author

@andris-sevcenko just checking in to see if you have seen this issue. Thank you.

@andris-sevcenko
Copy link
Contributor

@sidm1983 I have. Are you willing to submit a PR for this?

@brandonkelly brandonkelly self-assigned this Oct 19, 2020
@brandonkelly
Copy link
Member

Just released 2.8.3 which fixes this.

brandonkelly added a commit to craftcms/craft that referenced this issue Oct 19, 2020
@brandonkelly
Copy link
Member

Just updated the default HTML Purifier config for new Craft projects as well. You will need to make this change manually if you have a config/htmlpurifier/Default.json file: craftcms/craft@2e37109

@sidm1983
Copy link
Author

Thanks for that @andris-sevcenko & @brandonkelly. Apologies I didn't submit a PR for this, as I didn't get a chance at the time. Thanks for sorting it out though.

@brandonkelly, quick question about the change you made in the above commit. It looks like the dot characters are not being escaped, which means they will match any character and not just a single dot. Is this intentional?

@sidm1983
Copy link
Author

@brandonkelly just wanted to check if you've seen my comment above about escaping the dot character in the regex. Thank you. 😊🙏🏽

khalwat pushed a commit to nystudio107/craft that referenced this issue Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants