-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
iframes instead of inline videos #469
Comments
@DBounds See its a feature: http://textangular.com/ point 5 :P |
@bastienJS yes, I'm aware. :) I think it could actually be a serious pain point if not made optional. It certainly is in my case. |
Point 5 is actually referring to our use of content editable rather than an iFrame to type in. $http.post('some-url', {html: taApplyCustomRenderers($scope.htmlValueFromTextAngular)}); I chose to do it this way so the user can resize anything that they can create an image placeholder for, it also means you can embed iframes and flash without breaking angular's sanitiser functionality. (I'll add this to the FAQ) Code references: taApplyCustomRenderers, youtube tool |
Thank you @SimeonC. |
@SimeonC I would argue that iFrames are not a security risk any more than any HTML tag is. iFrames have their own security model which makes them secure and usable. They are "different" than other tags in that they give control of a small portion of the view port to another resource but they also exist in another DOM with strict boundaries. If the rationale for not using an iFrame is largely based on security concerns I highly suggest you give some additional thought to this. This particular feature is very relevant right now and many implementors will be influenced by services like Medium when making a decision around what to use. This UX is a serious detractor. Keep in mind I say this with a heavy background in application security which was my career path for 5-6 years. |
We run the html output of textAngular through a careful fork of AngularJS' ng-sanitize module (textAngular-sanitize). As textAngular is designed to be a WYSIWYG editor and has a range of uses from Admin only use to front end users an iFrame is a security concern as it allows content we cannot control, this answer on SO sums up our thoughts nicely:
http://stackoverflow.com/questions/7289139/why-are-iframes-considered-dangerous-and-a-security-risk That said, the main reason I opted for using the placeholder was not so much the restrictions of the sanitiser but the fact that you can't easily catch click events on an iFrame so you couldn't resize the video as you wanted - the easiest way of doing this was to reuse the image resize code and some placeholders. I am curious about your comment about "services like Medium" I had a look around and couldn't seem to make any sense of your comment. |
@SimeonC the data about iFrames is true in an uncontrolled environment. You've added a lay of structure by requiring these are YouTube videos/links thus mitigating the security concern. Yes you still need to trust that YouTube isn't compromised and serving malware, but that's a pretty calculated risk. Regarding Medium (screen shot: http://cl.ly/image/2b2J3u1n2Y06), they're probably the best, most modern example of what a rich text editor could be. The UX is fantastic in every way shape and form. My reference to them and other blogging platforms, all of which have and rely on rich text capabilities, is that they all use iframes for YouTube and Vimeo videos both while composing and in rendering. |
By going about videos in the way you are you're adding a lot of overhead, both in the software you have to write, as well as the implementation costs the user has to bear. When/If you ever add Vimeo or Vine support you'll need to do something entirely different to be able to provide a similar user experience. Taking the iframe path allows for what is essentially the same pattern front to back, for both the user and you as the maintainer. Taking the iframe route is better for everyone involved as well as this great product you're building. |
The security concern is that we have to shim the sanitiser to allow iFrames, therefore any iframe can be inserted without being checked. We only use the placeholder when composing, when you render it using ta-bind it renders as a iFrame. If you want to create a PR that safely and securely allows iFrames for youtube videos feel free. |
Hi, sorry for bumping this again, but I haven't been able to find anything in the documentation or issues. Is there any way to customize textAngular or sanitise in order to allow any embed? My embeds are added only through an admin, so security is not a big concern. Thanks. |
Thank you so much. On Sun, Sep 13, 2015, 14:07 Patrick Bassut notifications@github.com wrote:
eder sánchez | developer edrpls.com |
@SimeonC I applied the "taApplyCustomRenderers" to the value before saving it. But when I want to edit the value again the iframe-tag is not converted to img-tag which can be handled by textAngular. EDIT: Found a solution: #769 (comment) |
Hi @SimeonC.
First, this is a great module and one I use at http://nimble.hr/.
That said I wanted to make a suggestion on one of your greatest features, which I feel is poorly implemented and creates much more complexity than is necessary: YouTube support.
First: By not using iframes and rendering the videos inline you're imposing something unexpected on the implementor; Angular is required to render the textAngularized result. As far as I can tell, everything about textAngulars output can be used in a non-Angular environment except for the YouTube video integration.
Secondly: Embedding videos with iframes is also the norm across the web. Take a look at Medium, Wordpress, Tumblr, etc. IMO what you're doing with textAngular imposes a UX that's unnatural and awkward, unlike the rest of the module.
As far as accommodations for size, you simple enable some simple UX for height and width with some sensible defaults. That's an easy fix, but something I saw brought up in one of the issue threads.
The text was updated successfully, but these errors were encountered: