search

avajs / ava

Node.js test runner that lets you develop with confidence 🚀
MIT License
20.74k stars 1.41k forks source link

Unhealthy dependency - `trim-off-newlines` #2822

Closed KillyMXI closed 3 years ago

KillyMXI commented 3 years ago

I've got a security warning on my projects. Turns out it's trim-off-newlines package and it only gets into my node_modules via ava dependencies.

If I understand the prevention strategy correctly, the regex /^[\r\n]+|[\r\n]+$/g is sufficient for the job and avoids the catastrophic backtracking. I write it here because I think it might be better to have an utility function locally rather than link a junk package.

I think this requires 3.15.1 patch update.

novemberborn commented 3 years ago

These regular expression denial of service vulnerabilities are only vulnerabilities if you're passing external strings into the regular expressions.

That's not the case with AVA. Maybe the test run a little slower, but I doubt that.

A PR is in progress to implement this within AVA itself, see #2825. That'll be included in AVA 4.

I recommend marking your project as not vulnerable to this CVE.

KillyMXI commented 3 years ago

I do realize this falls into the common category of issues not exploitable in dev tools. I'm also in the camp of those who think issues like this are easier to fix once than being an annoyance for countless number of people. It makes better optics for the project. It is bad issues like this propagate mindlessly but if there is a single point to stop the propagation then why not to use it.

I'm unhappy about the lack of backport without close full release of 4.0. There seem to be a lot of open issues still marked for 4.0 milestone - a lot of work yet to be done.

vladimiry commented 3 years ago

These regular expression denial of service vulnerabilities are only vulnerabilities if you're passing external strings into the regular expressions.

It's not just about being actually affected but also about the build workflow of projects that use @avajs might get blocked by security audit warnings. The workflow of some projects simply doesn't alloy known security warnings to present. In my experience normally the issues like this get handled relatively quickly upstream and only very rarely the manual exclusion/patching/effort is required. Unfortunately, so far it looks like the recent stable version of the project is abandoned.

I'm unhappy about the lack of backport without close full release of 4.0.

The trim-off-newlines is used as a direct dependency in v3, which is currently the recent stable release. So, in my opinion, in the case, if the dependency itself is not going to be updated anytime soon upstream and especially when the issue has already been handled in the alpha version, complete resolving the CVE like this is the responsibility of the project that uses the dependency as a direct one.