avoid escaping highlighted code even if identical to source

[devoto13]

Marked version: 1.0.0

Markdown flavor: n/a

Expectation

The code processed by highlight function is not escaped.

Result

The code processed by highlight function is not escaped only if result is different from the input.

What was attempted

This is a bit controversial change, but I think the behaviour from this PR is more consistent and predictable. Current logic will escape highlight output if no changes were made by the highlighter.

This PR changes the behaviour to be more consistent and also clearly documents all aspects of highlight function. If you agree that this change of behaviour is the correct direction, I'll also add tests for the asynchronous highlighter.

Contributor

• Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR);

Committer

In most cases, this should be a different person than the contributor.

• Draft GitHub release notes have been updated.
• CI is green (no forced merge required).
• Merge PR

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/markedjs/markedjs/2b7kahppt
✅ Preview: https://markedjs-git-fork-devoto13-consistent-highlight.markedjs.now.sh

[UziTech]

If the highlighter doesn't highlight anything (i.e. returns the same text) I would assume it should be escaped otherwise it seems like it could open up an xss security hole.

The async highlighter was broken in v1.0.0 but should be fixed in #1664

[devoto13]

If the highlighter doesn't highlight anything (i.e. returns the same text) I would assume it should be escaped otherwise it seems like it could open up an xss security hole.

IMO this would be a good point if opposite is true, i.e. if input has been modified, then it is escaped. But this is not the case. It may be better to add escaping to the function contract instead.

[UziTech]

This would definitely be a breaking change and anyone who relied on code being escaped by the highlighter or marked would need to add their own way of escaping if the highlighter doesn't.

I feel like it would be easier for people to create there own renderer or always change the text in their highlighter function if they want the code to not be escaped.

Here is code to prevent escaping code that isn't changed by the highlighter by extending marked:

const marked = require('marked');

// Override renderer function
const renderer = {
  code(code, infostring, escaped) {
    const lang = (infostring || '').match(/\S*/)[0];
    if (this.options.highlight) {
      const out = this.options.highlight(code, lang);
      if (out != null) {
        escaped = true;
        code = out;
      }
    }
    if (!lang) {
      return '<pre><code>'
        + (escaped ? code : escape(code, true))
        + '</code></pre>\n';
    }
    return '<pre><code class="'
      + this.options.langPrefix
      + escape(lang, true)
      + '">'
      + (escaped ? code : escape(code, true))
      + '</code></pre>\n';
  }
};

marked.use({ renderer });

marked(...)

[UziTech]

It may be better to add escaping to the function contract instead.

This might be a good thing to do when we start removing options and replacing them with extensions but that will probably be v3.0

Right now the plan is:

• v1: make marked easier to extend and create extensions for any options that we can.
• v2: deprecate options and recommend using extensions instead.
• v3: remove/change options.

[devoto13]

You're right that it is not important enough to warrant a breaking change. Although I still think that it is more consistent to always not escape as using highlighter didn't modify the code as a condition for escaping is pretty arbitrary. So as you say, it is better to revisit this when the time is right.

I was updating a documentation, which used an ancient fork of marked to the latest marked. And my motivation for this change was to minimise the diff to a manageable size. I'll follow your advice and use a custom renderer instead.