Skip to content

Commit

Permalink
(enh) prevent rehighlighting of an element
Browse files Browse the repository at this point in the history
  • Loading branch information
@joshgoebel
joshgoebel committed Apr 29, 2023
1 parent f4bf02a commit 3987abe
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 0 deletions.
9 changes: 9 additions & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,12 @@
## Version 11.9.0

Parser:

- (enh) prevent rehighlighting of an element [joshgoebel][]

[Josh Goebel]: https://github.com/joshgoebel


## Version 11.8.0

Parser engine:
Expand Down
6 changes: 6 additions & 0 deletions src/highlight.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,11 @@ const HLJS = function(hljs) {
fire("before:highlightElement",
{ el: element, language });

if (element.dataset.highlighted) {
console.log("Element previously highlighted. To highlight again, first unset `dataset.highlighted`.", element);
return;
}

// we should be all text, no child nodes (unescaped HTML) - this is possibly
// an HTML injection attack - it's likely too late if this is already in
// production (the code has likely already done its damage by the time
Expand All @@ -769,6 +774,7 @@ const HLJS = function(hljs) {
const result = language ? highlight(text, { language, ignoreIllegals: true }) : highlightAuto(text);

element.innerHTML = result.value;
element.dataset.highlighted = "yes";
updateClassName(element, language, result.language);
element.result = {
language: result.language,
Expand Down

6 comments on commit 3987abe

@verhovsky
Copy link
Contributor

@verhovsky verhovsky commented on 3987abe Oct 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshgoebel dataset.highlighted should be mentioned in the README.

This is a pretty big change for anyone that ever replaces the contents or the language of an element with highlighted code. It means that you now have a mandatory step of clearing the property before you call hljs.highlightElement:

codeElement.removeAttribute('data-highlighted');
hljs.highlightElement(codeElement)

This broke how I was using highlight.js, which was letting users type code and re-highlighting it on every keystroke.

@joshgoebel
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure about the README, but definitely worth a mention in the docs I'd say.

But this is absolutely a breaking change and should not have made it into a minor release. You have my apology there. At this point (2 weeks after release) with no other reports though, I think it may be best to leave it in and just document it vs issuing a patch release to rip it out.

@allejo Any opinion here on document vs revert?

@allejo
Copy link
Member

@allejo allejo commented on 3987abe Oct 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, it's already made it into a release and has been out for a bit already. Since this is the first complaint, I vote to document it and append the release notes/changelog describing this accidental breaking change.

If we receive more complaints about this breaking people's workflows, I'd say we can have a patch release with this reverted (or being opt-in behavior, which ever makes the most sense).

@ITrunsDE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have the same problem. When I remove the attribute, I get the following message

[Warning] One of your code blocks includes unescaped HTML. This is a potentially serious security risk. (highlight.min.js, line 266)
[Warning] https://github.com/highlightjs/highlight.js/wiki/security (highlight.min.js, line 267)
[Warning] The element with unescaped HTML: (highlight.min.js, line 268)

When I reload the page, the code is formatted normally and the message does not appear.

I am definitely in favour of an opt-in.

@joshgoebel
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I remove the attribute, I get the following message

Because it includes HTML. If you wanted to re-highlight it (without a warning) you need to replace it with just raw text first - no HTML.

@figuerom16
Copy link

@figuerom16 figuerom16 commented on 3987abe Nov 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@verhovsky @ITrunsDE
Dynamic behavior can be faked with an editor mask. The mask will be transparent showing invisible text, but will show the caret and highlighting. As the user edits the invisible text the highlightjs will display behind the mask correctly. For all intensive purposes the user should be unaware of this.

Example:

<style>.stack {position: absolute; left:21px; right: 21px; min-height: 500px;}</style>
<pre class="stack"><code id="code"></code></pre>
<pre class="stack" style="background-color: transparent;"><code id="editor" contenteditable spellcheck="false" style="min-height: 500px; color:transparent; caret-color: red;"></code></pre>
<script>document.getElementById("editor").addEventListener("input", e => {document.getElementById("code").innerHTML = hljs.highlightAuto(e.target.innerText, ["python", "go", "bash", "javascript"]).value})</script>

Please sign in to comment.