fix: replace parse-glob (fixes #664)

[josundt]

This PR replaces the abandoned, security-vulnerable parse-glob package with a custom parse-glob.js module/function and thereby fixes issue #664

The added module exactly mimics the behavior of the function from the parse-glob package, but only includes the subset that is required by HTMLHint in the returned object. It uses is-glob to validate the glob.

Changes:

• Added parse-glob.ts to replace the vulnerable package
• Added a simple unit test to prove that my new module returns the same result as the replaced package for the required properties used by HTMLHint.
• package.json:
  • Removed the @types/parse-glob package from devDependencies
  • Moved parse-glob from dependencies to devDependencies (currently needed for the added unit test)
  • Added is-glob as dependency

This PR is meant as an intermediate step to prove that the parse-glob package has been properly replaced.

As a next step it would make sense to:

• Remove the parse-glob devDependency - and the simple unit test
• Simplify the returned object from the new parseGlob function.

If (hopefully when) this PR is approved; let me know if you would like me to contribute with that.

[nschonni]

Thanks! I tried to inline this, just to see what it was actually trying to do in the code today, but I think there is something missing, since basename is only set with a valid glob, but there are non glob paths testing for non-empty basename. This may be a bug in the current code, or maybe there is an assumption in this patch

[josundt]

@nschonni My bad, I forgot to handle non-globs (path only).
I have updated the PR to handle non-glob paths similar to the parse-glob package, and added tests to prove the mirrored behavior.
(I also made the code more tolerant to using backslashes instead of slashes for paths.)

[nschonni]

I'm playing with this in https://github.com/nschonni/HTMLHint/tree/parse-glob-fix by inlining the code.
Behaviour of a directory with and without the trailing / is different. Without it, it recursively runs and finds errors, but finds the same amount of files. This happens in the current version though as well.

[josundt]

@nschonni I added a couple of more test globs to the unit test on my local machine to try it out;
one (non-glob) directory path with a trailing / and one without the /.

I noticed that while result.base in my code consistently has removed the trailing / in all the tests, the parse-glob package removes it from result.base in all tests except the one for the non-glob directory path with the traling / suffix.

This is the only difference in behavior I could find, and I could argue that the behavior of my replacement is more consistent than in the package that was removed.

I hope and believe this should be good enough to replace the parse-glob package.

Whether you chose to approve the PR or to use the function from the PR some other way, I still hope you will be able to release a new version of HTMLHint without parse-glob as soon as possible.

Let me know if there's anything more I can do to contribute.

[josundt]

@nschonni Any progress on this?

You need to do something to mitigate the "Regular expression denial of service" vulnerability parse-glob=>glob-base=>glob-parent asap. The vulnerability triggers 4 high severity npm audit warnings.

Even if HTMLHint is just a dev tool, this issue causes a lot of noise and ugliness in static application security test reports etc.
The tool deserves better than being abandoned by users due to security audit problems.

I believe my contribution through this PR should make this fairly simple.
Whether you merge my PR or inline the code from it, please prioritize finalizing the task.

[electrovir]

Is it expected that this PR will receive attention or would you recommend instead maintaining personal forks of HTMLHint to eliminate the security issue?

[joeyparrish]

I've created an updated and rebased version of this PR in #927. I also regenerated the binaries so that my fork can be used directly as a dependency in the meantime.

[thedaviddias]

@joeyparrish is this PR still relevant?

[joeyparrish]

No, my PR included everything from this one. The bulk of the work was done by @josundt. I just rebased it and cleaned up some tests. Thank you, @josundt!