Low severity vulnerability affecting this repo

[snyk-community]

Hi there,

We noticed that your repo has a low severity vulnerability:

• Regular Expression Denial of Service
• Affected package: uglify-js <2.6.0

Here is the test report for this repo.
If you’d like to fix this vulnerability, Snyk lets you generate a pull request that recommends the best upgrade path - there’s a link to fix this vulnerability on the test report.

Stay secure :-)
Snyk Community

[binarymist]

Thanks Snyk. We could turn this into a feature. My guess is that if and when NodeGoat depends on some tooling for mitigating the issues around consuming free and open source, we'd probably go for something like NSP which is free and more mature than Snyk?

[ckarande]

Thanks Snyk. We appreciate notifying us. @binarymist I like the idea of converting this vulnerability into a feature. However, it could be tricky as uglify-js is not directly used by the application code.

[guypod]

@binarymist FWIW, Snyk is actually far more comprehensive and mature than NSP, both in DB coverage and in feature set, especially around GitHub integration. Entirely true NSP was around as a project sooner, but hasn't progressed much in a long while.

I'd encourage to try the two tools out before picking ;)

[snyk-community]

@ckarande @binarymist that's a really good idea! It's possible require a bit of a research to see how easy it to exploit this vulnerability through swig.

[lirantal]

Hey guys,

Just dropping by here as I accidentally saw this thread which is actually the exact topic for my PR with regards to the 'Insecure Components' section: #83

I understand Snyk's open issue here but since the point is that this is a vulnerable app in purpose there's little sense in trying to fix and make this repo secure.

Hopefully my #83 PR satisfies the feedback here.

[binarymist]

Swig 1.4.2 is what we're using and is the latest but no longer maintained package, which depends on uglify-js 2.4.24. So in order to get out of this situation, an alternative to Swig is required.

swig is mentioned in a3.html, mentioned in package.json obviously, and is used in server.js. Someone needs to find a suitable alternative and replace.

[lirantal]

@binarymist I think the point of the issue that was opened here was to alert for possible insecure code with swig but that's kind of irrelevant because the whole project is supposed to be insecure as it is an educational resource. Do you think it's still required to move from swig to something else?

[binarymist]

Yeah, in order for a project to be maintained, it needs to have up to date dependencies, otherwise maintainers will be forever struggling. I think it's intent is to be purposely vulnerable, but I think that means the vulnerabilities should be intentional and ideally documented, otherwise it ends up depending on old packages to never be updated. This would make for maintenance hell. Correct me if I'm wrong here @ckarande ?

[lirantal]

Yep, makes sense.

[ckarande]

@binarymist , yes that makes sense. That's why I didn't close this issue.

We should keep the dependencies up to date as much as possible. At least there shouldn't be any dependencies that have known vulnerabilities unless we explicitly demonstrate those.

[binarymist]

When Swig is replaced, we should probably implement a defense in depth solution using:

1. Validation
2. Filtering
3. Sanitisation

It would be good to see this across the entire app or as much of it as possible, just thinking of the #84 which does provide a commented solution in code, but it is pretty minimal, not that that's bad, but it may lead devs to think "Oh, that's all I need to do", and we really want to lead them down the path of success and know how apply security in depth. I'll have a think about the best way to provide comprehensive mitigation across all inputs. Of course the fix will need to be commented out by default.
I think we just need to be careful to think about the mitigations and make sure we're applying the same style across all components, rather than all doing our own thing, this is more of a "we need to step back and take a holistic approach" across the entire app.