-
Notifications
You must be signed in to change notification settings - Fork 961
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
High Severity Vulnerability in html-minifier #2589
Comments
There's nothing we can do to mitigate this on our end. Just wait for them to patch it. |
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. |
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. Edit: there's a huge drawback to use the terser version it's a way bigger dep that html-minifier tho. |
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 Just to add more context to this discussion Comparing html-minifier vs html-minifier-terser Unpacked size Weekly Downloads Last publish There are pro and cons to each implementation. |
Any update about this? |
You guys need to replace I propose html-minifier-terser or minify for a replacement |
html-minifer-terser is just way too big for now to embed consider as a replacement for now. |
Hello 👋 Since there are drawbacks in both options, would you consider letting the consumer pass in the minifier implementation? For example, the // 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 |
If html-minifier-terser is a fork of html-minifier, would it work to just override it in your own package.json? {
...
"overrides": {
"html-minifier": "npm:[email protected]"
}
} |
I was wondering if you could replace by another package on package json via |
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:
Happy to discuss more. I'm keen to help in any way possible :) |
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 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. |
You can do that with Yarn 1 and forced resolutions -
Unfortunately, looks like the security check I'm evaluating right now does not know how to process that, but yours might. |
Something to note is that So for the package switch to work, one would have to add |
Yea, this seems great as it solves this security issue and trims this library down further. |
Then we'll go to the "optional" way if I'll try to get this done for next month if the situation is better for me. |
@bridgetbarnes I got formal reply from Snyk - resolutions are not supported in Yarn v1. |
Thanks for looking into it @iRyusa!! 🙏 Otherwise, our security tool may still recognise it as a dependency 🙂 |
It won’t be listed as a peer dep. It will be fully optional and only listed in the doc.
|
Hey @iRyusa, wanted to see if you could provide a rough timetable for this rollout and/or if end of this month is still the target? |
And just to confirm, the optional method decided is not the one that eddeee888 provided here right? The optional method would make html-minifier an optional dependency and wouldn't conduct the minify process if require('html-minifier') isn't resolved. So if users wanted to minify with a different minifier, they would have to do it on their own outside of the package, correct? |
Hello @iRyusa, do you have an estimated date of when version will be released with the fix? Thanks. |
Hi @iRyusa any update on html-minifier? |
We're not able to pass our security audit due to this vulnerability - seemed like there was a fairly simple & promising solution, but no progress in ~6mo. Is this still on the table, or do we have to migrate away? :( |
I might be able to find some time to finally test this PR : #2666 let's hope I can release this MJML5.0 before the end of September as a "beta". If you want to help about this feel free to reach out on Slack. We need to test that migration from MJML4.x to 5.x is smooth for user that require to still use html-minifier and js-beautify and not concerned by this CVE. Ensure that the documentation is explaining properly this change to users. The raw output of HTML from MJML without beautify/minify is a bit weird maybe we could add I will commit to this on my free time so any help is welcomed. |
Is this still being looked at? |
I'm still looking for a way to have a somewhat readable output from MJML without any dep but looks really hard to do as I don't have enough time to work on this. html-nano seems to be a good option to replace current html-minifier tho. |
Any updates |
Just copy pasted from your original issue : For what it's worth : this vulnerability isn't anything new. We started to check alternatives as html-minifier doesn't seem to be updated anymore :
You can check https://www.npmjs.com/package/mjml/v/5.0.0-alpha.1 this version remove html-minifier and js-beautify. It relies on htmlnano and prettier instead.
|
I've been using htmlnano* for minifying instead with preset "safe" and minifyCss: false so far so good. * a patched version until this is released |
@santialbo Should be good as we disable it by default too on the alpha https://github.com/mjmlio/mjml/pull/2858/files#diff-17b00ecc0c06136aa56a592b0e9154976c812427d95ca6ce9c0fc3c4d2e05305R407 |
+1 |
@iRyusa I tried updating to use
(does not happen with |
Yeah that's expected as prettier isn't compatible browser side :( Did you check if you can replace prettier by the standalone version in your rollup config https://prettier.io/docs/en/browser.html ? |
Ok, I'm actually using Sveltekit - but adding this to const config = {
// rest of the config
resolve: {
alias: {
prettier: 'prettier/standalone',
},
},
};
export default config; Thanks! |
FYI for anyone using node, to use the alpha just update your package.json to change
then run npm install |
Can confirm that this worked for me, after upgrading my Node version to v22.3.0 (the dependency required a newer version than what I had) |
I get I tried both on node18 and node22 fix :
However the bundlesize increased enough for me to have to fix my project for the lamda max bundlesize constraint |
I've been having the same issue. After upgrading mjml to Here's the error and stack trace I was seeingnode:internal/modules/esm/utils:229 throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG(); ^ TypeError: A dynamic import callback was invoked without --experimental-vm-modules at importModuleDynamicallyCallback .importModuleDynamicallyCallback (node:internal/modules/esm/utils:229:11 undefined) at Object. (root/functions/node_modules/prettier/index.cjs:593:23 undefined) at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined) at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined) at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined) at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined) at Object. (root/functions/node_modules/mjml-core/lib/index.js:87:40 undefined) at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined) at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined) at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined) at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined) at Object. (root/functions/node_modules/mjml/lib/index.js:9:41 undefined) at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined) at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined) at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined) at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined) at Object. (root/functions/src/emails/utils/renderEmailTemplate.ts:5:14 undefined) at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined) at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined) at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined) at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined) at Object. (root/functions/test/utilities/emails/renderEmailTemplate.spec.ts:1:1 undefined) at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined) at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined) at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined) at jestAdapter .jestAdapter (root/functions/node_modules/jest-circus/build/runner.js:95:13 undefined) at runTestInternal .runTestInternal (root/functions/node_modules/jest-runner/build/index.js:262:16 undefined) at runTest .runTest (root/functions/node_modules/jest-runner/build/index.js:326:7 undefined) { code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG' } The stack trace seems to point to an issue with prettier and jest. There's a couple github issues floating around the prettier and jest repo:
However, after testing upgrading to jest 30.0.0-alpha.1 and 30.0.0-alpha.1 pre-release, it still continued to throw that error for me |
Hard to help without any context and callstack.
|
Haven’t seen your reply George sorry, I’ll check to update prettier to fix this but you’ll need to update jest tooOn 23 Jul 2024, at 04:04, George Cartridge ***@***.***> wrote:
I've been having the same issue. After upgrading mjml to 5.0.0-alpha.4, I get the same error as @wilau2
**Here's the error and stack trace I was seeing**
node:internal/modules/esm/utils:229
throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG();
^
TypeError: A dynamic import callback was invoked without --experimental-vm-modules
at importModuleDynamicallyCallback .importModuleDynamicallyCallback (node:internal/modules/esm/utils:229:11 undefined)
at Object. (root/functions/node_modules/prettier/index.cjs:593:23 undefined)
at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined)
at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined)
at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined)
at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined)
at Object. (root/functions/node_modules/mjml-core/lib/index.js:87:40 undefined)
at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined)
at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined)
at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined)
at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined)
at Object. (root/functions/node_modules/mjml/lib/index.js:9:41 undefined)
at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined)
at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined)
at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined)
at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined)
at Object. (root/functions/src/emails/utils/renderEmailTemplate.ts:5:14 undefined)
at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined)
at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined)
at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined)
at Runtime.requireModuleOrMock (root/functions/node_modules/jest-runtime/build/index.js:951:21 undefined)
at Object. (root/functions/test/utilities/emails/renderEmailTemplate.spec.ts:1:1 undefined)
at Runtime._execModule (root/functions/node_modules/jest-runtime/build/index.js:1264:24 undefined)
at Runtime._loadModule (root/functions/node_modules/jest-runtime/build/index.js:931:12 undefined)
at Runtime.requireModule (root/functions/node_modules/jest-runtime/build/index.js:834:12 undefined)
at jestAdapter .jestAdapter (root/functions/node_modules/jest-circus/build/runner.js:95:13 undefined)
at runTestInternal .runTestInternal (root/functions/node_modules/jest-runner/build/index.js:262:16 undefined)
at runTest .runTest (root/functions/node_modules/jest-runner/build/index.js:326:7 undefined) {
code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG'
}
The stack trace seems to point to an issue with prettier and jest. There's a couple github issues floating around the prettier and jest repo:
jestjs/jest#14305
prettier/prettier#15769
However, after testing upgrading to jest 30.0.0-alpha.1 and 30.0.0-alpha.1 pre-release, it still continued to throw that error for me
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
What types to use with |
We don't provide any types from MJML but it's exactly the same option except it's |
While waiting for MJML 5, I did a fork with html-minifier-terser : https://www.npmjs.com/package/mjml-4-terser (for server-side only, the dependencies are too big to use in a browser) |
Can this be backported until 5.0? Can't use mjml because of this 2 year old high security issue. |
It's a breaking change due to minifier going from sync to async. So no it won't be backported to 4.x branch. Use the latest MJML5, it should be released soon anyway. |
I suggest reading my comments and especially kangax/html-minifier#1135 (comment) - because switching to |
We’re not using terser version as a replacement so we should be fine ?On 3 Nov 2024, at 15:01, Daniel Ruf ***@***.***> wrote:
I suggest reading my acomments and especially kangax/html-minifier#1135 (comment) - because switching to html-minifier-terser will not fix this.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>
|
@iRyusa that is good. Anyways, those suggesting to migrate to the |
Hello, our security check has found a high severity vulnerability in html-minifier, which is a dependency of mjml.
Dependency hierarchy:
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?
The text was updated successfully, but these errors were encountered: