Hotfix/issue 14

[tivie]

3 things in one:

• Fix for Directive seems to be broken #14 (markdownToHtmlDirective)
• Add option to aggressively sanitize showdown's output
• Keep $sce.trustAsHtml as default bheavior

[phw]

I haven't run the code again, but in the non-sanitize case looks like ut is still broken. The old code is unchanged, but using $sce.trustAsHtml doesn't make any sense in this case. It would be used to pass the HTML as trusted to other directives, but that is not the case here. It is directly used in element.html(). IMHO there is no need for $sce at all here.

[phw]

On a second thought: The difference might be that I use the full jQuery instead of the Angular jQuery light implementation. Has Angular's .element() different behavior here?

[phw]

Ok, found the core issue why the code doesn't work for me: I am using full jQuery and not only the JQlite bundled with angular. In it seems element.html behave a bit differently there, which results in jQuery not outputting anything.

In any way, I don't think you need $sce at all here. The point of $sce.trustAsHtml is to be able to pass HTML as trusted to other directives. What's happening here is that the code just gets directly written to the DOM. So either the HTML from $showdown.makeHtml is treated as untrusted and $sanitize is run or you don't do anything. This seems to be finally a sane implementation:

function getLinkFn($showdown, $sanitize, $sce) {
  return function(scope, element) {
    scope.$watch('model', function(newValue) {
      var val;
      if (typeof newValue === 'string') {
        val = $showdown.makeHtml(newValue);
        if ($showdown.getOption('sanitize')) {
          val = $sanitize(val);
        }
      } else {
        val = typeof newValue;
      }
      element.html(val);
    });
  };
}

Does this make sense?

[phw]

One more thing: I just realized that showdown will not treat existing HTML (e.g. by stripping or escaping it). So in order to prevent dangerous user input it would actually make sense to always run $sanitize before passing the value to showdown:

val = $showdown.makeHtml($sanitize(newValue));

Not sure if there is a need for sanitizing the showdown output, but it could stay an option. The only bad thing showdown generates are probably the IDs, and there are already options to deal with that.

[tivie]

Regarding the use of $sce, it enables decorating the directive without concerns of the output being incorrectly sanitized. However, it seems it break when using with jquery. @SyntaxRules thoughts?

[tivie]

$sanitize breaks a lot more stuff than header ids. It parses img and a tags, which break under some edge cases. It also dislikes iframes (used in youtube showdow extension), strips unknown tags, etc...

[phw]

$sanitize breaks a lot more stuff than header ids. It parses img and a tags, which break under some edge cases. It also dislikes iframes (used in youtube showdow extension), strips unknown tags, etc...

Yes, I personally don't see the point in using sanitize for the output. I think there is some misconception here, the whole issue is rather about the directive being broken when using jQuery.

My comment above was about sanitizing the input before passing it to showdown, which would makes lot of sense, but could also be solved by showdownjs/showdown#182 .

[tivie]

@SyntaxRules I think this is ready to merge. What do you think?

[SyntaxRules]

I took a look, and I like it. I want to add in one more fix for line 174. It will include passing the output straight to a directive, instead of using angular.element. Which will fix the problem @phw was having. I'm hoping to have it done tuesday.

[tivie]

@SyntaxRules I'm going to merge this pull as I need this and a couple of improvements. You can then improve over it with the changes you were talking about.

[SyntaxRules]

Thanks!