gjtorikian / commonmarker

Ruby wrapper for the comrak (CommonMark parser) Rust crate
MIT License
416 stars 80 forks source link

unsafe filters `<script>` tags? #250

Closed fliiiix closed 10 months ago

fliiiix commented 10 months ago
irb(main):002:0> Commonmarker.to_html("<script>alert('test')</script>", options:{render: {unsafe:true}})
=> "&lt;script>alert('test')&lt;/script>\n"

If i understand the spec correctly that should embed the script as plain html. (Im using commommarker 1.0.0.pre10)

Is that a bug or is there a config option i missed to allow script tags to be rendered as html?

gjtorikian commented 10 months ago

I believe you're right, that should've emitted the HTML directly. I'll try to make some time this week to investigate the source—I'm not sure if it's this library here or a bug in https://github.com/kivikakk/comrak, which this gem wraps.

Of the top off my head, it almost looks like escape is being incorrectly applied. You can either scrub HTML out (unsafe: false), or, the HTML can be escaped:

irb(main):006:0> Commonmarker.to_html("<script>alert('test')</script>", options:{render: {escape:true}})
=> "&lt;script&gt;alert('test')&lt;/script&gt;\n"

It seems as if the escaping is happening on the first tag char? (<)

Less dangerous tags do not have this behavior:

irb(main):007:0> Commonmarker.to_html("<b>alert('test')</b>", options:{render: {unsafe:true}})
=> "<p><b>alert('test')</b></p>\n"
irb(main):008:0> Commonmarker.to_html("<b>alert('test')</b>", options:{render: {unsafe:false}})
=> "<p><!-- raw HTML omitted -->alert('test')<!-- raw HTML omitted --></p>\n"
fliiiix commented 10 months ago

I wasn't able to reproduce this issue with the comrak cli

~/.local/bin/comrak --unsafe test.md 
<script>alert('test')</script>

~/.local/bin/comrak --unsafe --escape test.md
&lt;script&gt;alert('test')&lt;/script&gt;

So maybe its just a lib problem or how the gem uses the lib. But yeah it looks weird because it only escapes the first < of script which makes no sense to me yet.

kivikakk commented 10 months ago

This is the tagfilter extension from the GFM spec. Disable that and it should be as you expect.

gjtorikian commented 10 months ago

Wow, TIL. Setting tagfilter: false works as expected. Thanks @kivikakk 🙇‍♂️

fliiiix commented 10 months ago

just when i thought i had enough experience with markdown options i learn something new :partying_face:

gjtorikian commented 10 months ago

buddy, same

kivikakk commented 10 months ago

Honestly, that "extension" might well be one of my least proud moments as a software engineer. Live and learn 😊