Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(ngSanitize): linky does not sanitize when used in ngBindHtml directive #3285

Closed
wants to merge 1 commit into from

Conversation

brooksbp
Copy link

Look at the top comment here: http://docs.angularjs.org/api/ngSanitize.filter:linky

... and here is a demo of the undesirable behavior: http://jsfiddle.net/5uSnj/

The purpose of this pull request is to hopefully push this issue further along and gain some attention.

The attached code creates a new directive which calls the htmlParser with a new argument to 'linkify' URLs that are not already wrapped in an <a> tag. I am not very experienced with AngularJS and JavaScript, so this is probably not The Best approach. There must be some way to simply use the ngBindHtml directive with the linky filter for the desired behavior.

Thanks!
Brian

@petebacondarwin
Copy link
Contributor

It seems that linky should only focus on links and nothing else but as you say this particular implementation is not quite the right way to go.
Going to leave this PR open for a while to discuss the best way forward.

@IgorMinar
Copy link
Contributor

linky filter actually does what it says it does. it takes text and produces html by wrapping all urls into anchor tags.

notice that it doesn't take html as input, but text.

we can't just change the linky to consider the input to be html as that would make it impossible where its undesirable to consider the input to be text.

I think the best course of action would be to write a new filter that takes html as input, parses it into DOM, finds all textNodes, looks for urls in these text nodes and converts the urls into anchor tags.

This is a different kind of filter than the current linky.

Your change piggybacks on the sanitizer and builds the linkyfication into the sanitizer but that's not right. sanitizer is just sanitizer. linkification is a unrelated concern that should not be part of the sanitizer.

simple version of such filter would look like this: http://plnkr.co/edit/IEpLfZ8gO2B9mJcTKuWY?p=preview (it does need more work to make it production ready, but that should be enough to get you started).

Now if you or others care about this functionality, then I suggest that you make it a project of its own and public is in bower so that others can reuse it.

thanks for bringing it this to our attention!

@IgorMinar IgorMinar closed this Aug 1, 2013
@brooksbp
Copy link
Author

brooksbp commented Aug 8, 2013

OK! Thank you for the guidance Igor.

@bibach
Copy link

bibach commented Nov 8, 2013

Igor, thanks for the code for an HTML-enabled linkify. Our project found an issue with the code when there are more than a couple of links. I made the changes (just a couple of lines) in a fork of your code, in case you want to pull them back into yours, for others who may use it. http://plnkr.co/edit/TKcvCNHLOjYqUFGx4tiJ?p=preview

My apologies if I've made this more difficult than it needs to be. I couldn't find a way to do the equivalent of a pull request (or even diffs) on Plunker. Let me know if there's anything I can do to make the process easier.

@goldibex
Copy link

goldibex commented Feb 5, 2015

I realize this thread is old, but I wanted to leave a comment in case anyone ends up here from Google.

The code from the referenced Plunkr snippet has a gross security vulnerability. It imports Angular's $sanitize service, but never uses it. The assignment of a string to the innerHTML property of a DOM element is therefore an instant XSS reflection attack.

The attack is easy to verify; in the textarea, just type <img src=x onerror=console.log("hello")> and you'll see the log message in your console.

Fixing the attack is even easier; at app.js:38, wrap the variable input with a call to $sanitize.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants