search

showdownjs / ng-showdown

Angular integration for Showdown
BSD 3-Clause "New" or "Revised" License
105 stars 33 forks source link

Directive seems to be broken #14

Closed phw closed 9 years ago

phw commented 9 years ago

I think there is some conflict with the directives.

The current documentation says one should use 

<p markdown-to-html="mdScopeVariable"></p>

For me this fails, having a look at the code the model value in https://github.com/showdownjs/ng-showdown/blob/master/src/ng-showdown.js#L121 is always undefined.

Looks like the model is expected in sd-model-to-html. However that doesn't work, either. Using both makes Angular complain about two directives requesting an isolated scope, using only sd-model-to-html strangely also shows an undefined model.

Using angular 1.3.15

phw commented 9 years ago

See PR #15

tivie commented 9 years ago

Seems the latest release is broken. I will have a look.

@SyntaxRules What was the reason behind changing the API?

SyntaxRules commented 9 years ago

As it was in beta, I determined it was the perfect time to clear up the names.

old: sdStripHtml new: stripHtml reason for change: The user just imported the ng-showdown model. I don't think they need to be reminded they are using that module. In my opinion the sd prefix is redundant.

old: sdModelToHtml new: markdownToHtml reason for change: Ignoring the prefix. the name modelToHtml implies we are expecting the user to pass in a model. What this module expects is a markdown string to be passed in. I think the name is more accurately stating what this directive does.

tivie commented 9 years ago

@SyntaxRules Seem sensible reason. =)

The prefix was to safeguard directive name clash.

The issue with $sce and the directive is easily fixed, although we will need to support, at least for a while, the old directives names for backwards compatibility.

Btw, did you figure why travis chokes with node 0.8?

SyntaxRules commented 9 years ago

Yup, see #12 and #13 Bower doesn't support/work with node 0.8 link

tivie commented 9 years ago

@SyntaxRules Ah! nice catch :+1:

I'm playing with $sce and it seems $sce.trustAsHtml does not sanitize anything. You can check it on http://showdownjs.github.io/demo/ with this md text:

# foo
<script>alert('xss');</script>

the script tag is left unchanged

tivie commented 9 years ago

this should fix it as well as giving users a way to easily sanitize showdown's output.

The default behavior cannot be sanitation since $sanitize is very agressive and strips thinks like ids and stuff.

phw commented 9 years ago

See my comments on #17