Contract installation during macos-setup.sh reports vulnerabilities

[lispmeister]

During a clean installation run of macos-setup.sh the contract installation section reported the following:

Warnings:

Installing contracts/solidity npm and requirements...
npm WARN deprecated tar.gz@1.0.7: ⚠️  WARNING ⚠️ tar.gz module has been deprecated and your application is vulnerable. Please use tar module instead: https://npmjs.com/tar
npm WARN deprecated to-iso-string@0.0.2: to-iso-string has been deprecated, use @segment/to-iso-string instead.
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated minimatch@0.3.0: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue

Contract installation terminated with:

added 339 packages from 750 contributors and audited 3751 packages in 62.941s
found 282 vulnerabilities (280 low, 1 high, 1 critical)
run `npm audit fix` to fix them, or `npm audit` for details

Running npm audit in the contract sub directory reports:

found 282 vulnerabilities (280 low, 1 high, 1 critical) in 3751 scanned packages
run `npm audit fix` to fix 279 of them.
3 vulnerabilities require manual review. See the full report for details.

The three vulnerabilities that require manual intervention are dependencies of the solidity-parser library that is no longer maintained. Root cause for this dependency is the reference of a point release of truffle-compile by DigixGlobal:

https://github.com/keep-network/keep-core/blob/3e1adcebfa70ca7354fad4948abde1eea6465162/contracts/solidity/package-lock.json#L20

Recommended action:

• [ ] Migrate from solidity-parser to solidity-parser-antlr
• [ ] Fix deprecated dependencies
• [ ] Patch vulnerabilities
• [ ] Automate keeping the dependencies clean an patched

[Shadowfiend]

Fwiw one of the annoyances with Solidity using npm is that there are a lot of false-positive vulnerabilities, since many vulnerabilities are JS libraries in the browser, and the dependencies to them are transitive and irrelevant.

But, naturally, I'm down to keep these up to date. https://github.com/thesis/heimdall/pull/20#issuecomment-415399730 is one note from last time we looked at automating dependency audits (some further conversation above that as well).

[pschlump]

On a clean system I am seeing the same results - my normal dev system has different results because I already have a "solc" compiler installed - the path finds the solc compiler (brew finds it) and it is not re-installed.

[pschlump]

When I change the dependency in package.json to solc 0.4.25 it fixes the "npm WARN" errors. I am not ready to upgrade to a new version of the compiler just yet - Last time took 3 days to sort out all of the unexpected details.

[pschlump]

What we have determined. A newer version of solc in the package.json file (0.4.25) fixes the "npm WARN" stuff at the top of this issue. That would be good. Upgrading to truffle 4.1.14 (current version as of Oct 15) causes us a problem with CircleCI and running tests - so it is not an option to upgrade. We believe that it will fix some of the security issues being reported by npm install. Upgrading to a newer version of solc usually takes some time and needs to be a coordinated effort. This means that we are not going to upgrade today.

[lispmeister]

PR #343 contains just the solc compiler upgrade.

[Shadowfiend]

So to go through the original todos here… We're not updating solidity-parser because that's not our direct dependency, right? And 343 bumped the vulnerabilities?

So is the only check remaining here automating dependency updates? Do we want to do that? Dependency updates are not necessarily something you want to automate given the potential for breakage, but they do need to be flagged.

[lispmeister]

I still think it makes sense to upgrade to a more "official" version of truffle. Our truffle-compile dependency points to a patched version we have no control over. To be more specific this fork is over two years old.

[Shadowfiend]

Got it! So we're not looking to update solidity-parser directly, but to bump our truffle dependency, right?

[Shadowfiend]

I can also look at enabling GitHub's security vulnerability analysis for our NPM packages, which will give us notifications for vulnerabilities.

[lispmeister]

I think it will pay of to be a bit more paranoid in managing our npm dependencies. If we need to use "unofficial" resources that should be documented and needs to be maintained.

[lispmeister]

If you go to the DigixGlobal repository where we source our truffle-compile artifact you'll see this: This branch is 139 commits behind trufflesuite:master. And that's just their fork of the upstream project. It's a dead end.

[Shadowfiend]

Sure… But we also depend on that for doc generation, not for our code builds. That's also our fork, so we can be in the driver's seat regarding when new code gets pulled in. We can poke at it @ https://github.com/keep-network/doxity .

I think we need double paranoia and a dose of skepticism to boot. What I mean is:

• We need to be paranoid about our npm dependencies being insecure.
• We need to be paranoid about our npm dependencies breaking in minor version updates, particularly when our smart contracts are managed by these dependencies.
• We need to be skeptical about security flags because JS packages are used across frontends, backends, and builds, and we only use them for builds. Some (many?) security flags have more to do with frontend usage, for example.

It's the down side of trying to shovel the whole JS package ecosystem into our build system.

So my thought: we should be aggressive about opening up PRs that bump our versions of npm packages to secure versions, but if that doesn't work out (as was the case with the Truffle version update, for example), we just need to be pragmatic about figuring out if the “vulnerabilities” we're trying to deal with are vulnerabilities for our usage and how urgent they are. We should also probably add vulnerability flagging to the repos we are pinning like the doxity repo.

We also probably need to refork our doxity fork from the original upstream and apply @ngrinkevich's commits to our own fork, rather than the current fork-of-a-fork situation we've got going on.

[Shadowfiend]

we only use them for builds

Ironically I typed this then later realized doxity is the issue and doxity is our doc generator, which generates frontend code. So yeah, bit more complicated than that. Of course :p

[lispmeister]

I think a policy of "keep it patched" unless there's a documented reason to fall behind that can be reviewed at the next milestone should work. The flagged vulnerabilities are all solidity parser related and we should not desensitize ourselves to issues in the immediate toolchain.

[Shadowfiend]

Ok, @keep-network/developers now has access to keep-network/doxity, which is a direct fork of DigixGlobal/doxity instead of ngrinkevich/global. keep-network/doxity in turn has a doxity-latest branch that should have the right dependencies, I think. Doxity is also pinning their dependencies, so that could get interesting.

Meantime, I think changing our reference from keep-network/doxity#master to keep-network/doxity#doxity-latest should do the trick.

@ngrinkevich I do need you to check the docs that we generate once we switch to that latest branch to see if they're behaving as you expect. Your commits from your fork did not apply cleanly to that branch, and it's not immediately clear to me what their impact was and whether that branch fixes the same issues a different way, so we'll need to see if things are behaving.

[lispmeister]

Progress!

[Shadowfiend]

Patch by default is fair, completely agree.