High Severity Vulnerability in html-minifier

[PavelBurya]

Hello, our security check has found a high severity vulnerability in html-minifier, which is a dependency of mjml.

Dependency hierarchy:

• mjml-4.13.0.tgz (Root Library)
• mjml-cli-4.13.0.tgz
• ❌ html-minifier-4.0.0.tgz (Vulnerable Library)

Vulnerability description:
A Regular Expression Denial of Service (ReDoS) flaw was found in kangax html-minifier 4.0.0 via the candidate variable in htmlminifier.js.

Here is a link to a similar issue in html-minifier. It does not seem to be worked on.

kangax/html-minifier#1135

Can you update your repository to get rid of this vulnerability?

[iRyusa]

There's nothing we can do to mitigate this on our end. Just wait for them to patch it.

[PavelBurya]

Unfortunately, this is a high severity vulnerability, so we can't wait years for them to fix it. Since html-minifier's last update was back on October 2021, I have concerns if that package is still supported and if anything will be fixed on their side.

And if neither your team nor theirs will work on this, we'll probably have to replace mjml with other libraries.

[iRyusa]

You can just not minify the code with the option we provide to be totally safe. This exploit is a concern if you're using MJML server-side.

It's FOSS at the end of the day, so you can also work on it. html-minifier was forked in html-minifier-terser and we're totally fine to replace it.

Edit: there's a huge drawback to use the terser version it's a way bigger dep that html-minifier tho.

[IsaaX]

Hello, same concern here. I've been trying ways to address this vuln but seems I couldn't do a quick turn around. I'm also in the camp of having to remove mjml if we cannot use the alternative html-minifier-terser. I have PCI compliance that I have to abide by and have to address security issues. As of now I plan on disabling minification.

Just to add more context to this discussion

Comparing html-minifier vs html-minifier-terser

Unpacked size
96.8kb vs 4.55MB

Weekly Downloads
3.7M vs 12.2M

Last publish
4 years ago vs 18 days ago

There are pro and cons to each implementation.

[juansedo]

Any update about this?
Is there any mjml's fork that solve the issue?

[ArielPrevu3D]

You guys need to replace html-minifier, the dependency is abandoned, vulnerable, slow and has popular alternatives which don't have these issues.

I propose html-minifier-terser or minify for a replacement

[iRyusa]

html-minifer-terser is just way too big for now to embed consider as a replacement for now.

[eddeee888]

Hello 👋 Since there are drawbacks in both options, would you consider letting the consumer pass in the minifier implementation?

For example, the mjml-core is currently importing html-minifier and calling it here. Could it take htmlMinify as an option? e.g.

// Remove import { minify as htmlMinify } from 'html-minifier' at the top
export default function mjml2html(mjml, options = {}) {
  // ... all the stuff

  content = options.htmlMinify(content, {
      collapseWhitespace: true,
      minifyCSS: false,
      caseSensitive: true,
      removeEmptyAttributes: true,
      ...minifyOptions,
    })
}

From what I can see, most options like collapseWhitespace, minifyCSS, etc. exist in html-minifier-terser too. So, in theory this could work?

[morphar]

If html-minifier-terser is a fork of html-minifier, would it work to just override it in your own package.json?
Something like:

{
  ...
  "overrides": {
    "html-minifier": "npm:html-minifier-terser@7.1.0"
  }
}

[iRyusa]

I was wondering if you could replace by another package on package json via overrides too but it seems you can only override the version.

[eddeee888]

I was wondering if you could replace by another package on package json via overrides too but it seems you can only override the version.

This feature seems to be available for PNPM. I cannot find anything that does this for NPM or Yarn 1.x. 🤔 (Could be wrong)

Summarising the thread so far, here are the mentioned solutions:

• ❌ Replace html-minifier with html-minifier-terser: works as a drop-in replacement but is not great because html-minifier-terser is a lot bigger
• ❌ Using overrides or solutions: works on some package managers but not others
• ❓ Let the caller pass in the minifier implementation through options: This'll remove the dependency but changes the implementation, probably a major version bump. Any other drawbacks?

Happy to discuss more. I'm keen to help in any way possible :)

[iRyusa]

In a future release we want to remove both minifier/beautify by default and let the user install. Maybe we should just do that now and if html-minifier is installed the option still works in mjml-core but if not then it just skips it totally. So the dep become fully optionnal.

Most of MJML users use it locally on their machine so this vulnerability doesn't really impact them. It's more an issue for dynamic template generated server side with user input.

[khitrenovich]

This feature seems to be available for PNPM. I cannot find anything that does this for NPM or Yarn 1.x. 🤔 (Could be wrong)

You can do that with Yarn 1 and forced resolutions -

  "resolutions": {
    "html-minifier": "npm:html-minifier-terser@7.1.0"
  },

Unfortunately, looks like the security check I'm evaluating right now does not know how to process that, but yours might.

[eddeee888]

Something to note is that html-minifier-terser returns a Promise whereas html-minifier returns a string.

So for the package switch to work, one would have to add await or .then() accordingly

[eddeee888]

In a future release we want to remove both minifier/beautify by default and let the user install. Maybe we should just do that now and if html-minifier is installed the option still works in mjml-core but if not then it just skips it totally. So the dep become fully optional.

Yea, this seems great as it solves this security issue and trims this library down further.
In your mind, would the packages be in peerDeps, or does it take a function?