Skip to content
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

ngSanitize #5

Closed
xh4n3 opened this issue Mar 13, 2015 · 7 comments · Fixed by #6
Closed

ngSanitize #5

xh4n3 opened this issue Mar 13, 2015 · 7 comments · Fixed by #6

Comments

@xh4n3
Copy link

xh4n3 commented Mar 13, 2015

Sir,
I've been using your code on my blog, but note the example you gave in the README could lead to XSS problems.
Use ngSanitize?

@xh4n3
Copy link
Author

xh4n3 commented Mar 13, 2015

I modified your code as below.
Now it works, but not sure if it is a correct way to do it.

angular.module('markdown', ['ngSanitize'])
    .provider('markdown', function () {
        var opts = {};
        return {
            config: function (newOpts) {
                opts = newOpts;
            },
            $get: function () {
                return new Showdown.converter(opts);
            }
        };
    })
    .filter('markdown', ['markdown', function (markdown) {
        return function (text) {
            if(text == null) text = '';
            var html = markdown.makeHtml(text);
            return html;
        };
    }]);

vpegado added a commit that referenced this issue Mar 17, 2015
@vpegado
Copy link
Owner

vpegado commented Mar 17, 2015

@xh4n3 this should be solved in the angular application module and not in the markdown module.
I've updated https://github.com/vpegado/angular-markdown-filter/blob/master/README.md with more details on how. Thanks for the report!

@xh4n3
Copy link
Author

xh4n3 commented Mar 17, 2015

Thanks for the detailed reply.
While I was testing your new code, I got a error saying 'Attempting to use an unsafe value in a safe context'.
Did you pass your test?
Besides, personally cannot agree with your new code even I haven't successfully tested.
Try set markdown content with "<script>alert(0);</script>" under your test environment. I assume there will be an alert box popping out.
If so, there would be a huge problem with XSS. If that is the case, have a look at $sanitize cause writing our own version of sanitization is a hard jobs. I think this guy is doing the similar thing.
Sorry to bother you again, I just want to help improving your repo. Please forgive me if I said anything wrong, thanks!

@vpegado
Copy link
Owner

vpegado commented Mar 19, 2015

@xh4n3 I'm happy with all feedback and appreciate the discussion.
The only way I could recreate your issue is when I remove the 'ngSanitize' dependency from my main app module. When it is present it sanitizes the input of the directive ngBindHtml.

Here is an example where the injected script is successfully removed:

'use strict';

angular
  .module('appApp', [
    'ngSanitize',
    'markdown'
  ])
  .controller('MainCtrl', function ($scope) {
    $scope.text = '# Subject\n<script>alert(0);</script>\nThis is the first paragraph.\n';
  });
<div ng-bind-html="text | markdown">
<!-- Result -->
<div ng-bind-html="text | markdown" class="ng-binding ng-scope">
  <h1>Subject</h1>
  <p>This is the first paragraph.</p>
</div>

How does this method work for you?

@xh4n3
Copy link
Author

xh4n3 commented Mar 19, 2015

@vpegado Yes, this works like a charm!
I forgot to import ngSanitize as a dependency to my main app.
Well, add 'note: import ngSanitize as well' to your README could be better.
Thanks very much!

@vpegado
Copy link
Owner

vpegado commented Mar 19, 2015

@xh4n3, I've now updated https://github.com/vpegado/angular-markdown-filter/blob/master/README.md
Would you mind checking if it's clear enough? Thanks for the Input!

@xh4n3
Copy link
Author

xh4n3 commented Mar 19, 2015

@vpegado
Can't be better, 👍

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

Successfully merging a pull request may close this issue.

2 participants