highlightjs / highlightjs/highlight.js
Chore: (v12) turn on HTMLInjection errors by default
- Dominant language
- JavaScript
- Stars
- 25k
- Forks
- 3.8k
- Avg merge
- 11h 35m
- Merged PRs (30d)
- 3
Description
I was reviewing the existing setup where we have both `options.ignoreUnescapedHTML` (the warning in console) and also a hard error `options.throwUnescapedHTML`.
Both are defaulted to `false` now which means _by default_:
- unescaped HTML always issues a warning
- but it will NOT raise an error
Since the library purposely does not support this (for security concerns) and this is trivial to add back via a plugin I think in v12 both of these options should be removed and there should be only a single (unchangeable) behavior: any unescaped HTML that isn't dealt with by a plugin is a hard crash with all the existing warning text being spewed out before the crash.
To fix the issue one can:
- use a plugin to pre-process/merge HTML auto-magically (as we used to do long ago)
- use a stub plugin to restore the "ignore" behavior from before (though I don't think we should publish or support such a plugin)
My thinking is if someone doesn't understand enough to write a tiny 2-3 line JS snippet to ONLY strip the HTML (using `textContent`) that they perhaps shouldn't be including HTML in their code blocks, period.
Is this too strong a take?
---
IE, to summarize the two choices I'm currently considering here are:
- hard error, no choice, use a plugin to "work around" this _if you TRULY know what you're doing_
- hard error is a scarily named config option that one can disable, maybe:
- `insecure_allowCodeInjectAttacks_I_PROMISE_I_KNOW_WHAT_IM_DOING: true`
**But I lean against option 2 because if we're just stripping the HTML, why is it there in the first place?**. The only reason might be a legacy website with tons of static files that would be hard to change by hand - but we also are CONFIDENT there are no injection attacks... but to me in that case someone should just write their own 2-3 line plugin to "fix" things (ie, strip the HTML)...
CC @highlightjs/core
Contributor guide
Assessment
This issue has not been assessed yet.