search

mitmedialab / unhangout-old

RETIRED
https://unhangout.media.mit.edu
MIT License
155 stars 39 forks source link

allow iframe embed and admin video controls redesign #468

Closed srish closed 8 years ago

yourcelf commented 8 years ago

Looks like this is working, good job!

Some code questions: what's the purpose of the video.IframeVideo view? It appears to be an empty div. It gets initialized by the embedIframePlayer with the ifCode from the model, but then that is never used. Instead, loadIframePlayer embeds the iframe directly, replacing anythin video.IframeVideo would've added. I recommend either (1) remove the video.IframeVideo view entirely (including its empty template) and just insert iframe code directly, or (2) making the video.IframeVideo view responsible for rendering and removing the iframe itself. As it is, the view is doing nothing and cluttering up the code.

What's the purpose of the swfobject import? Please remove if it's not there for a reason anymore.

What's the purpose of the LIVESTREAM_API_KEY? Please remove if it's not there for a reason anymore.

srish commented 8 years ago

@yourcelf I have removed the iframeVideo view, and now inserting iframe code directly. I have also removed other variables which aren't required anymore for this implementation.

yourcelf commented 8 years ago

I added a commit to sanitize iframe code server-side: https://github.com/unhangout/unhangout/pull/468/commits/9949488273180e9151079d6e36988250436ec06f Please check it out and see what you think; I can revert if you prefer a different solution.

The client-side check is a good start (checking that the string starts with <iframe and ends with </iframe>), but it's not complete. For example, <iframe src='http://asdf.com'></iframe><script src='bad.js'></script><iframe src='http://asdf.com'></iframe> would get through that check. I would remove the || iframeCode.endsWith('>') bit, as that allows basically anything through: https://github.com/unhangout/unhangout/pull/468/files#diff-698f301f8c399cc49892153625361a6eR717

Regardless, client-side checks are only a sanity check admins that flub the copy/paste. To protect users from malicious XSS, we need to sanitize server-side before propagating the code to other clients. There are still plenty of ways an arbitrary iframe embed could do badness (CSRF stuff, clickjacking, etc), and we can't really take advantage of iframe sandboxing if we want plugins like flash to be able tor un inside them. But at least we should prevent anything other than a single iframe tag with no child elements from getting through.

srish commented 8 years ago

looks good to me!