kangax / html-minifier

Javascript-based HTML compressor/minifier (with Node.js support)
http://kangax.github.io/html-minifier/
MIT License
4.94k stars 571 forks source link

Added a missing null check to collapseWhitespace #1069

Closed dlinning-jockey closed 2 years ago

dlinning-jockey commented 4 years ago

This was breaking a script, as there was no null checking in place. str.replace() was the specific breaking point.

alexlamsl commented 4 years ago

It's an internal function, so it shouldn't be passed anything other than a String.

Please demonstrate the use case which you encounter the error, so I can determine where exactly the problem lies.

dlinning-jockey commented 4 years ago

So it actually breaks pretty easily, assuming the user passes a null value to minify():

Package.json shows "html-minifier": "^4.0.0" for the version.

const minify = require("html-minifier").minify;

var result = minify(null);

console.log("Result:", result);
// Expected output: Either null or ""
// Actual output: TypeError: Cannot read property 'replace' of null

While yes, the end-user should have done a null check already, I don't think that calling the function with a null value should break like this. Since the return of collapseWhitespace() is return lineBreakBefore + str + lineBreakAfter;, and lineBreakBefore/After are initially set to an empty string, I don't think that the early return of an empty string is too breaking.

alexlamsl commented 4 years ago

minify() should definitely gives an error if you don't pass in correct parameters − papering over user error is undesirable, and also makes it hard to debug down the line.

DanielRuf commented 4 years ago

So in this case the code should thow an Error. Can you change this @dlinning-jockey? Would be great to also have a PR for https://github.com/DanielRuf/html-minifier-terser if possible and a test which covers this.

dlinning-jockey commented 4 years ago

@alexlamsl I've updated it to throw a SyntaxError, and it does it within the minify() function now, instead of collapseWhitespace().