[CB-14145] npm audit in CI TEST WIP - DO NOT MERGE

[brodycj]

Platforms affected

All

What does this PR do?

• npm audit in Travis CI

FUTURE TBD:

• npm audit in AppVeyor CI

NOTE: This is a TEST WIP PR, based on 2.1.x, expected to fail with npm audit issue. This change is intended to be included in 2.2.x (for patch release) and master for next major release, to verify that any npm audit issues would be spotted and resolved in the future.

What testing has been done on this change?

Check CI results from this PR

Checklist

• Reported an issue in the JIRA database
• Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
• Added automated test coverage as appropriate for this change.

[raphinesse]

Great idea 👍

I see one problem though. Builds might be broken by a PR that is not responsible for it. Just because a new security issue has been discovered in the meantime. So maybe we actually want something that pushes us for any issues. Greenkeeper, David DM or maybe auditing in our nightly builds.

[brodycj]

Greenkeeper, David DM or maybe auditing in our nightly builds.

+1 (+100)

I can take a look sometime next month, wouldn't mind if someone can look into these options more quickly.

While I would agree with the concern that CI build may be broken by a PR that is not responsible for it, I would personally favor the idea that we resolve any npm audit issues before reviewing and integrating any other changes in the future.

[raphinesse]

• https://blog.github.com/2017-11-16-introducing-security-alerts-on-github/
• https://github.com/marketplace/category/security
• Snyk looks good

[raphinesse]

Having given this a little more thought, my stance is that failing our CI tests on a failed audit would be a bad idea. The reasons being:

• We are basically polling for changes, and only so if someone files a PR or commits to master. Thus:
  • We are not notified ASAP
  • PRs might seem to fail the tests when they did not actually cause the problem
• Our tests might be stuck failing for some time without us being able to fix it. Because:
  • Either there's not a fix to the vulnerability yet
  • Or the vulnerability is in a transitive dependency, while our direct dependency (or someone in the chain) hasn't yet updated (Right now, node-sass users might be able to relate)

So I'd say: leave it out of our normal CI and use a dedicated service instead. Snyk seems to be made for this, the GitHub Security Alerts might work fine too. In both cases we have to see how compatible the workflow is with how INFRA works.

PS: Even though not security focused, it might still be nice to have GreenKeeper.

[AhsanAyaz]

Having these vulnerabilities in cordova-android of course. Any plan to fix this? :)

[brodycj]

Having these vulnerabilities in cordova-android of course. Any plan to fix this? :)

Yes I am waiting for a review of apache/cordova-android#451.

Also keep in mind that the npm audit issues would only affect developer command-line tooling, not expected to affect any published apps. But better to be on the safe side, I think.

Closing now.