highlightjs / highlightjs/highlight.js

Chore: (v12) turn on HTMLInjection errors by default

Open
#3,528 0 comments 0 reactions 0 assignees View on GitHub
bug help welcome parser
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.