CB-14145: cordova-common update to resolve npm audit & other updates in patch release

[brodycj]

Platforms affected

Android

What does this PR do?

• cordova-common 2.2.5 update (pinned) to resolve ugly npm audit and engine warning messages
• pin other dependencies in this patch fix
• completely reinstalled dependencies in node_modules using npm@6.1.0
• update bundledDependencies, needed to support deprecated Node.js 4 in this patch fix
• update cordova.js from cordova-js@4.2.4 with the following change in this patch fix (using local coho with Improve patch release support cordova-coho#176 for patch release support):
  • CB-9366 log error.stack
• other changes requested by @raphinesse:
  • CB-13923 (android) fix -1 length for compressed files
  • CB-14127: (android) Move google maven repo ahead of jcenter
  • CB-14101 Fix Java version check for Java >= 9 (CB-14101 Fix Java version check for Java >= 9 #446)
  • Emit log event instead of logging directly (Emit log event instead of logging directly #452)
  • Fix unsafe property access in run.js ((android) Add unit tests for run and retryPromise #445)
• enable node 4 (again), needed to support minor release (no need: deprecated Node.js 4 is still allowed by engines rule in package.json; npm install & npm test are still covered by AppVeyor CI on deprecated Node.js 4)

resolves npm audit warnings & other issues in patch release as needed asap (before the next major release is ready)

What testing has been done on this change?

• npm audit with npm@6.1.0 (latest version) shows 0 vulnerabilities
• using cordova platform add brodybits/cordova-android#cordova-common-2-update to add platform to new Cordova project and test on Android device using cordova run android succeeds on the following Node.js versions:
  • deprecated Node.js 4 (npm 2.15.11)
  • Node.js 6 (npm 3.10.10)
  • Node.js 8 (npm 5.6.0)
  • Node.js 10 (npm 6.1.0)
• npm test succeeds (along with some other tasks) on AppVeyor CI & Travis CI

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 (with some exceptions).
• Added automated test coverage as appropriate for this change.

[codecov-io]

Codecov Report

Merging #451 into 7.1.x will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##            7.1.x     #451   +/-   ##
=======================================
  Coverage   43.95%   43.95%           
=======================================
  Files          17       17           
  Lines        1711     1711           
  Branches      318      318           
=======================================
  Hits          752      752           
  Misses        959      959

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bd3309...f909f35. Read the comment docs.

[raphinesse]

Why do we need the dependency update before unbundling the dependencies (and thus the next major release)?

[brodycj]

Now tested as follows (see above for more details):

• passes npm test on Travis CI
• Using cordova platform add https://github.com/brodybits/cordova-android#cordova-common-2-update to add cordova-android with the proposed changes to new Cordova app results in an app that starts properly on Android (device)
• Using cordova platform add https://github.com/brodybits/cordova-android#cordova-common-2-update to add cordova-android with proposed changes to cordova-sqlite-storage test suite results in app that passes cordova-sqlite-storage test suite on Android (device)

The proposed changes resolves the npm audit warning messages on a minor release.

[Menardi]

@brodybits Why does this PR require re-enabling support for Node 4? This would break a number of waiting PRs which depend on Node 6+.

[dpogue]

We can't drop support for node 4 in a minor version update, but I agree that we should keep master moving forward to the next major version.

@brodybits Can we do these updates for the next minor on top of the 7.1.x branch? That way we can simply delete node_modules entirely on master and not worry about introducing more conflicts into the pending PRs

[brodycj]

@brodybits Can we do these updates for the next minor on top of the 7.1.x branch?

Will do.

[raphinesse]

I've cherry-picked all code fix commits from master that I could identify.

[brodycj]

I've cherry-picked all code fix commits from master that I could identify.

Thanks @raphinesse, updated title yet again to reflect what we actually want to do in the patch release.

TBH I have some mixed feelings, though not major. In general I would rather avoid including other fixes when making a security related patch. For a security patch we want the least risk possible that something goes wrong and the "end" user decides to roll back.

I think the actual security risk is very low. In general I would rather keep it this way.

Another really strange thing is that f05e61d seems to have a MacBook-Pro.local address, not linked to any user on GitHub.

What do you think, any comments?

[brodycj]

FYI the delay is because it is taking me much longer than I expected to resolve the npm audit issues on cordova-js which I would consider to be an upstream dependency. My apologies for the confusion.

/cc @jcesarmobile

[raphinesse]

As a user, I'd be annoyed by a patch release not including fixes to my problems even though they are ready to ship.

I see your point however. Since you do the release, decide at your own discretion.

[brodycj]

@raphinesse you convinced me about the first 2 commits you added:

• f05e61d - CB-13923 (android) fix -1 length for compressed files
• 5cd19a3 - CB-14127: (android) Move google maven repo ahead of jcenter

I am still not so convinced about the 3 commits:

• 056fe36 - change from CB-14101 Fix Java version check for Java >= 9 #446 (CB-14101 Fix Java version check for Java >= 9) more drastic than I would like to include in a security audit patch
• 7c278e3 - change from Emit log event instead of logging directly #452 (emit log event) looks straightforward enough, would have liked a JIRA issue number before sticking it into this patch release.
  552e4be - fix unsafe property access (ported from 1a8154c) is even more ugly since it appears to be a side effect of a PR to add more unit test coverage ((android) Add unit tests for run and retryPromise #445).

I think it would be ideal to move the last 3 commits to a followup patch, not sure if it is worth the effort to split them out or not.

[brodycj]

@raphinesse I just updated the last 2 commits to reference the PRs they originated from and show what kind of scripts are affected.

(GitHub links to the PRs from the commits in master, would be nice to have the same kind of thing in the 7.1.x branch. To show what kind of scripts are affected would also be good for the patch release notes.)

I may just include all the fixes in the same patch release to streamline things. I think the npm audit issues will not lead to any real vulnerabilities, at least not yet:)

[raphinesse]

I'd be happy to see these fixes in the upcoming release. I don't see why these changes should break anything. If things break, we'll fix them. I think you need to worry a bit less 😊

[brodycj]

I just pushed a complete rebase, with the following updates (see above for more details):

• Update cordova.js from cordova-js@4.2.4 with the following (using local coho with Improve patch release support cordova-coho#176 for patch release support):
  • CB-9366 log error.stack
• update to cordova-common@2.2.5
• pin other dependencies in package.json
• completely reinstalled dependencies in node_modules using npm@6.1.0
• update bundledDependencies in package.json, needed by the deprecated node 4 version
• include changes that were added by @raphinesse (with PR number added to the commit messages in the last 3 changes)

tested in Cordova project using deprecated Node.js 4 and other Node.js versions as stated above

npm audit with npm@6.1.0 (latest release) shows 0 vulnerabilities (as stated above)

[raphinesse]

I don't see why we need separate commits for removing an adding node_modules contents. But then again I don't care too strongly about it.

It seems you listed each transitive dependency in bundledDependencies. According to this npm test we should not need to do that. Was there something that made this necessary?

Otherwise this looks good to me 👍

[brodycj]

I don't see why we need separate commits for removing an adding node_modules contents.

As I explained in apache/cordova-windows#281 (comment): a combination of updated dependencies and npm from non-deprecated version of Node.js results in such a massive change to node_modules that it seems cleanest to remove old node_modules before making the update.

It seems you listed each transitive dependency in bundledDependencies. According to this npm test we should not need to do that. Was there something that made this necessary?

Yes, also explained in apache/cordova-windows#281 (comment): With node_modules installed by newer version of npm (comes with non-deprecated version of Node.js), additional libraries need to be listed to work on Node.js 4. We know that Node.js 4 is deprecated but should not be dropped in a patch release:-(

Thanks for checking, will probably merge this really soon.