Fix for checklist rendering problem

[Jackymancs4]

Trying to address #694, #314, #681

Hello,
after some investigation I found that the problem was indeed the sanitization of html performed by https://github.com/leizongmin/js-xss through https://github.com/VisionistInc/showdown-xss-filter.

js-xss provides powerful filtering API, but showdown-xss-filter forces only the default behavior that kills every type of input and form.

Since showdown-xss-filter has pretty old dependencies, I'm going to maintain (mainly for personal use) an updated fork that enables all of js-xss configurations.

This PR includes a whitelisting configuration that allows only checklist rendering, while performing xss validation on everything else.

Test:

Test1

- [ ] Test
- [ ] Test
- [ ]Test
- [x] Test

[ ] Test
[x] Test

[ ]
[x]

<script>alert('xss!')</script>

Before	After

[dmsnell]

Thanks for the patch @Jackymancs4!

If there another way we can accomplish this with the existing showdown-xss-filter? Should we be looking at js-xss instead? Could your forked-patch apply to the upstream project?

[dmsnell]

we would definitely want to lock this down. right now it appears like if the input is a checkbox then we'll also allow XSS attacks in other unrelated fields of the same HTML

[Jackymancs4]

Can you articulate more? I'm very interested if I missed something.
Without whitelisting this, rendering checkboxes is near impossible.
Also, strictly allowing only checkbox type input seems pretty harmless..

[Jackymancs4]

I can actually improve sanitization of <input>, stripping it from all the non-type attributes, if that was the problem.

Like, enforcing that only <input type="checkbox disabled> are returned.

[Jackymancs4]

If there another way we can accomplish this with the existing showdown-xss-filter?

Nope, I already extensively studied the code and it seems impossible to me, because of this line
https://github.com/VisionistInc/showdown-xss-filter/blob/e26046384e266be5646ef0eb58816ff4029da565/showdown-xss-filter.js#L20

Should we be looking at js-xss instead?

Well, I think it's a possible way, but I'm not able to see any real gain doing so.
Showdown is working on expanding is extension capability, and I think it's pretty cool.
showdown-xss-filter and showdown-xss-config simply pipe showdown output in a js-xss instance, but more can be done in the future.

Could your forked-patch apply to the upstream project?

Actually, my fork is not a patch, but a complete rewrite of the code according to the more recent showdown extension interface. And I'm planning to improve testing coverage. Also, the original repo has been unmantained for almost three years. I can try, of course.

[roundhill]

Just tried this out, thanks for the fix @Jackymancs4. We are indeed allowing any input with a type="checkbox" now. I'm wondering if we could limit it to only allow if it is within a <li> element? Otherwise I can add it in note content and it gets rendered:

There's also a small conflict with package.json now. If we can fix those two things then I think we can .

[roundhill]

Could your forked-patch apply to the upstream project?

We should also see about this, or maybe put the fork in one of our repos.

[dmsnell]

Closing this due to #721 and #724