critical build issue

[bradymiller]

hi @d3sandoval ,

We are having a critical issue with the build process, and all development builds are breaking. Seems to stem from following error: npm ERR! code E404 npm ERR! 404 Not Found: flatmap-stream@https://registry.npmjs.org/flatmap-stream/-/flatmap-stream-0.1.1.tgz (appears this package contained malicious code)

Any thoughts on best way to quickly fix this?

Thanks! -brady

[DreaminDani]

Try rm package-lock.json && rm -rf node_modules on your local to clear out all your local copies of things. Then npm install to regenerate the package-lock.

If that doesn't work, happy to hop on a call to help fix it.

[bradymiller]

Isn't the issue though that flatmap-stream-0.1.1.tgz no longer exists since it was removed secondary to malicious code: https://github.com/dominictarr/event-stream/issues/116

[TheToolbox]

Hello again everyone! Sorry I have been inactive, and I apologize in advance for my resulting ignorance. Please let me know if any of my thoughts here is unreasonable or infeasible!

flatmap-stream was found to be a malicious package targeting cryptocurrencies. I've been trying to remove it from our dependency tree, but I can't even convince npm to install all the packages. In addition to this vuln (marked CRITICAL), npm audit has 5 marked HIGH and 234 marked LOW. I'd be happy to start pruning things, but I'm not sure what will break if I do. Napa also scares me a bit from a security perspective, and I'd love to see if we could try to pull it out.

My goals in fixing this: 1) Remove flatmap-stream and its upstream dependency npm-run-all 2) Address all HIGH security issues 3) Address all LOW security issues 4) Add npm audit to the CI to prevent glaring issues like this in the future 5) On the devops/installation side of things, add the --prod flag to npm install for production uses to reduce attack surface 6) Work to reduce the number of dependencies overall, as the amount of code pulled in is too much to audit (are angular, react, and jquery-ui all necessary?)

Is there anyone (probably @d3sandoval) that can walk me a bit through the npm dependency graph so we can figure out what can be pruned and what can be safely updated?

[DreaminDani]

@TheToolbox all of your points are reasonable. Napa is a hack to help us move off of bower. You can find most of the details in that change here: https://github.com/openemr/openemr/pull/1767

If you can think of another way to pull in non-npm dependencies without using napa, i'm all for it :)

[bradymiller]

Looks like suggestions by @d3sandoval will get openemr working again :)

https://github.com/openemr/openemr/commit/02b2accf67fb466b67ea307361f52671cb5dcd90

Agree on working on hardening all the other stuff (note we can't do anything though about the older dependencies since those require work on the codebase; ie. for example, removing the outdated jquery libraries)

[bradymiller]

Also, Here's where the build process is happening in the docker: https://github.com/openemr/openemr-devops/blob/master/docker/openemr/5.0.2/Dockerfile#L22-L32

[bradymiller]

We also need to sort out where the flatmap-stream package was actually used in openemr (I am guessing used in the pipeline to produce the theme scripts) and ensure no vulnerability exists in folks using the development codebase (note we have not released a production version with this building scheme, but some folks do run off the development codebase) -brady

[DreaminDani]

@bradymiller it appeared to only affect people with working on cryptocurrency stuff: https://github.com/dominictarr/event-stream/issues/116

[TheToolbox]

@bradymiller It looks like flatmap-stream was used by npm-run-all (a devDependency), which looks like it was a development ergonomics addition. I don't see it used anywhere in either a build process or actual code. First glance to me suggests it can just be removed.

[bradymiller]

Looks like the new npm build doesn't include event-stream or flatmap-stream anymore (they were removed in the updated lock file).

for a light laugh: https://github.com/dominictarr/event-stream/issues/121

[TheToolbox]

w.r.t. napa, npm allows installation of github urls directly (so npm i --save github:user/dependency works), so we could do that for any of the github-hosted ones

[DreaminDani]

@TheToolbox IIRC that only works if the dependency has a package.json file :(

[TheToolbox]

:'( You're completely right. I guess we'd need to knock them out one at a time then. At least much of it seems to be various (exceedingly old) jQuery versions. Do we know where those are currently being used? Most of them have vulnerabilities tagged HIGH for XSS.

In higher-priority news, I think I was able to rip out npm-run-all, although in the process npm seems to think that all sub-dependencies should be changed from x.y.z to ^x.y.z. Are there any thoughts about the risk/reward of that move?

[DreaminDani]

unpinning dependencies isn't the worst thing in the world. more strict orgs have explicit policies to always pin: https://before-you-ship.18f.gov/infrastructure/pinning-dependencies/

It'd be up to the maintainers if we should have a similar policy. The only reason I had them pinned was because I was moving them over from their bower versions and didn't want to break things by accidentally using a higher version.

also, glad to hear you were able to remove npm-run-all from the dependency tree! Just make sure npm run dev-build still works :)

[bradymiller]

@TheToolbox and @d3sandoval ,

Sounds reasonable (assuming these changes are in https://github.com/openemr/openemr/pull/1996 ) on the sub-dependencies. I would still keep the main dependencies pinned, which you kept in the PR.

Looks like the above PR will need to be rebuilt on the most recent codebase to prevent the merge conflict in the package-lock.json file

btw, here's an issue regarding goal to consolidate jquery: https://github.com/openemr/openemr/issues/842

-brady