skohub-io / skohub.io

https://skohub.io
3 stars 1 forks source link

Improve dependency warning setup #16

Closed acka47 closed 2 years ago

acka47 commented 2 years ago

This is a generic issue for all SkoHub modules.

I just read that npm audit is broken by design which also applies to GitHub dependabot for npm projects as both rely on the same database .

This matches our experience so far with dependabot. We should discuss how to improve our setup to filter out vulnerability warnings for things that are actually non-vulnerabilities. Here are my initial ideas:

  1. Move dependencies that are not relevant for production to devDependencies.
  2. Add audit --production tests to the CI GitHub Actions.
  3. Turn of dependabot warnings (e.g. here for SkoHub Vocabs).

What do you think?

fsteeg commented 2 years ago
  1. Move dependencies that are not relevant for production to devDependencies.
  2. Add npm audit --production tests to the CI GitHub Actions.
  3. Turn off dependabot warnings (e.g. here for SkoHub Vocabs).

Sounds good! In particular for our Gatsby-based projects many dependencies are probably not required in production but only used to generate the static sites.

acka47 commented 2 years ago

We discussed today to implement it as suggested in the original comment. @literarymachine , @dr0i and @sroertgen, I leave you assigned until we start with implementation in case you want to add your two cents.

dr0i commented 2 years ago

I read the article. Honestly, it's about my scope of understanding, e.g. it's stated that creating that react-app :

converts it into a static HTML+JS+CSS folder. Notably, it does not produce a Node.js app.

I did that, and it produced a static/js/787.d3befce1.chunk.js, javascript. As stated here using javascript may result in a static site but also may be dynamically. I would have to analyze the code to qualify it).

... and as far as I have understand it correctly, following proposals are no valid ways in context of getting rid of "absurd security warnings by dependabot":

  1. Move dependencies that are not relevant for production to devDependencies.
  2. Add audit --production tests to the CI GitHub Actions.

It's stated in the artile: "Move dependency to devDependencies ... is flawed."

  1. Turn off dependabot warnings (e.g. here for SkoHub Vocabs).

This is good for all our static sites. It's already done, I checked. (is it really good? If the gatsby static site generator is fed with data from outside (e.g. vocabs- is this really totally harmless or could this be an attack vector exploiting gatsby vulnerabilities? Oh boy - I guess the latter. So in my view, as not having analyzed or understand this remotely accurate, this is to be treated as a potential risk. => I am going to reenable dependabot for these modules.)

Leaving these other modules (linked to security warnings sorted by severity):

These are modules that are running at our servers (pubsub & editor) or at the users computer as browser plugin (extension). Both are are potentially dangerous, to our servers and/or to the users computer. Problem is, I am not a node/js dev and cannot properly determine (in adequate work time) what is potentially really dangerous, if there is an attack vector in the used code, etc.

So I unassign myself here.

My advice, considering the three mentioned modules (as long as there is no better qualified argument), is to:

check the warnings and take them seriously

Also, I would like to escalate this as there are a number of open high security warnings (as linked above). (please do understand that I like skohub and would found it bizarre to no more host it, but also understand to keep harm away)

acka47 commented 2 years ago

First, some general thoughts:

  1. We MUST maintain SkoHub Vocabs properly as it is used in production by different parties.
  2. We CAN and probably SHOULD switch off SkoHub Editor and SkoHub PubSub for now as we do not have the capacities for proper maintenance and security updates.

With regard to SkoHub Vocabs:

Here is my suggestion on how to move forward:

dr0i commented 2 years ago

Assigning also @acka47 to announce temporary stop of maintenance and hosting of Editor and PubSub.

acka47 commented 2 years ago

Assigning also @acka47 to announce temporary stop of maintenance and hosting of Editor and PubSub.

Done in https://github.com/skohub-io/skohub-blog/commit/5f7e842794b36ad4626c2cd2ff6aab53ed80e28e

dr0i commented 2 years ago

So, just using devDependencies is left.

  1. I still want to point out that devDependencies is typed as flawed in the text you gave as reference. But, as you stated:

The build service is password protected so currently only people we trust can send data to skohub.io/build

so I put at least all the gatsby stuff under devDependencies, now:

$ npm audit --production found 0 vulnerabilities

  1. Funny thing is: devDependencies will not be taken into account by dependabot (see here and here ).

  2. A normal npm i will also still give security issues for all packages.

So: dump the idea of devDependencies doing us any good (at least in conjunction with dependabot)

However, we got many sec issues fixed with #163, so we may just go on to fix the severest issues anyway (one critical left in gatsby-plugin-sharp) and don't turn off dependabot (although it will still gives us 22 vulnerabilities (5 moderate, 17 high)).

dr0i commented 2 years ago

npm audit now reveals:

25 vulnerabilities (5 moderate, 18 high, 2 critical)

Fixing these automatically with npm audit fix --force doesn't work.

Most of them have to do with gatsby besides:

node-fetch <2.6.7 Severity: high node-fetch is vulnerable to Exposure of Sensitive Information to an Unauthorized Actor - https://github.com/advisories/GHSA-r683-j2x4-v87g fix available via npm audit fix node_modules/cross-fetch/node_modules/node-fetch cross-fetch <=2.2.3 || 2.2.5 || 3.0.0 - 3.1.4 || >=3.2.0-alpha.0 Depends on vulnerable versions of node-fetch node_modules/cross-fetch @graphql-tools/url-loader <=7.4.3-alpha-9f8b9c45.0 Depends on vulnerable versions of cross-fetch Depends on vulnerable versions of ws node_modules/@graphql-tools/url-loader

Don't know how to update this, seems to be a bit of work. I give up and just ACK npm audit is broken by design.