fix: swap isomorphic-fetch for node-fetch for security issue

[shazron]

See #95

[alexkli]

I believe there is no fundamental reason to use any special cross-platform fetch implementation here, this can actually just use a standard node-fetch (or in future the new built-in node fetch module)... unless fetch-retry does not work with a latest node-fetch (v2) for some reason. But this comment says v2 should work fine: jonbern/fetch-retry#64 (comment)

isomorphic-fetch came in in this commit #12 when updating dependencies... previously fetch-retry came with isomorphic-fetch but in newer versions one had to explicitly depend on it.

IMO node-fetch is a better dependency to have, so I would suggest to try out node-fetch instead of moving to another library cross-fetch that at least I have no experience with.

[shazron]

@alexkli ok, I will update with node-fetch. I didn't delve into why isomorphic-fetch was used in this lib (perhaps there is an in-browser use case I reckoned) and cross-fetch seemed a drop-in replacement.

[shazron]

@alexkli PR updated

[alexkli]

Thanks @shazron. Code change LGTM. However, the CI build is failing in all nodejs versions (here is node 14). I can't find the actual error at quick glance...

[shazron]

@alexkli all tests pass except this ngrok thing that I have no idea how to fix:

The command '/bin/sh -c wskdebug --ngrok myaction 2>&1 | grep -A 10 "ngrok dependency required for --ngrok is not installed"' returned a non-zero code: 1

[alexkli]

Hmm, it looks unrelated, but it worked in the last PR #92 that got merged.

This error happens in the posttest script which you can run using npm run posttest.

The test logic is in here: https://github.com/apache/openwhisk-wskdebug/tree/master/test/install

It spins up a container and tries to install wskdebug from scratch in there and see that it runs. As part of that it checks for the ngrok installation check we have in wskdebug: https://github.com/apache/openwhisk-wskdebug/blob/master/test/install/Dockerfile#L34-L35

You could run this locally and change that line to just wskdebug --ngrok myaction to see what the actual output is... assuming it is reproduceable locally. If not, commit this change (temporarily) and see how it looks like when run in Travis CI.

[shazron]

The error message I get when making that change and running npm run posttest is:

 => ERROR [ 8/10] RUN wskdebug --ngrok myaction                                             12.7s
------
 > [ 8/10] RUN wskdebug --ngrok myaction:
#13 12.66
#13 12.66 Error: Unknown Error From API: socket hang up
------
executor failed running [/bin/sh -c wskdebug --ngrok myaction]: exit code: 1
"docker rmi" requires at least 1 argument.
See 'docker rmi --help'.

Usage:  docker rmi [OPTIONS] IMAGE [IMAGE...]

Remove one or more images

ERROR: Installation test failed!

As you can see the error message has changed from what is expected. I double checked I don't have ngrok installed globally before running it.

I'll check in the change so the CI can run.

[shazron]

I'll debug why it doesn't go to

openwhisk-wskdebug/src/agentmgr.js

Line 94 in 1ec715d

throw new Error("ngrok dependency required for --ngrok is not installed. Please install it using:\n\n npm install -g ngrok --unsafe-perm=true\n");

[shazron]

@alexkli it's pretty clear now that there is a bug in the existing code, and nothing related to this PR. This line:

openwhisk-wskdebug/src/agentmgr.js

Line 23 in 1ec715d

NgrokAgent = require('./agents/ngrok');

the result is always defined so this logic will always be false:

openwhisk-wskdebug/src/agentmgr.js

Line 93 in 1ec715d

if (this.argv.ngrok && !NgrokAgent) {

(if (true && !true) => if (false))

There should be a MODULE_NOT_FOUND error thrown, but I'm not seeing it -- npm ls ngrok doesn't show anything either:

❯ npm ls ngrok
@openwhisk/wskdebug@1.3.0 /Users/shazron/Documents/git/work/apache/openwhisk-wskdebug
└── (empty)

[shazron]

Solved it! npm@7 is default in the latest node-lts, and by default will install peer dependencies. I added the flag --legacy-peer-deps to the RUN npm install -g openwhisk-wskdebug.tgz in the Dockerfile and now it works. Checking the change in.

[shazron]

@rabbah @alexkli all issues resolved. Perhaps this can be merged and a release started? Thanks!

[alexkli]

@shazron Thanks for digging deeper on this!

npm 7 by default will install peer dependencies

That is a problem. ngrok must NOT be installed by default as it conflicts with the Apache licensing of openwhisk. That's why it was made a peer dependency - so that it does not get installed by default and that a developer must actively choose to install it themselves. While still working with the nodejs code and linters (that otherwise complain on missing require() dependencies).

Now if npm 7 has changed the peer behavior, we need to find a different way where a simple npm install would not install it. I think this is probably best to be fixed in a separate PR that we need to come up with and merge before this one (it essentially blocks any other changes to this repo).

Any smart ideas on how to achieve that? cc @rabbah

Sorry that you run into this with your change.

[alexkli]

And also this was a reason: #22

Not sure if that has improved as well.

[shazron]

Looks like it can be turned off in .npmrc:

legacy-peer-deps=true

However, that only works when you are doing npm i inside the repo root, and does not apply when installing it from a tarball like here:

openwhisk-wskdebug/test/install/Dockerfile

Line 30 in 204f4e3

RUN npm install -g openwhisk-wskdebug.tgz

... at least according to my testing.

Not sure what the workaround is here, besides requiring before install:

npm config set legacy-peer-deps true --global

I prepared a PR where you can see that the ngrok test still fails: #97

[alexkli]

@shazron There is a peerDependenciesMeta with an optional: true flag that can be put in the package.json, which seems to be restoring the old behavior? Worth trying. If it works it should also be backwards compatible for users who still have npm < 7.

[shazron]

@alexkli #97 now passes with your suggestion, once that is approved and merged, I will rebase from master, and amend this PR to remove --legacy-peer-deps from the Dockerfile

[shazron]

removed --legacy-peer-deps from Dockerfile. Needs to be rebased with master after #97 is merged (it was approved)