npm vulnerability

[petriaarnio]

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High │ Arbitrary Code Execution │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package │ underscore │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in │ >=1.12.1 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ @mapbox/mapbox-gl-draw │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path │ @mapbox/mapbox-gl-draw > @mapbox/geojsonhint > │
│ │ jsonlint-lines > nomnom > underscore │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info │ https://npmjs.com/advisories/1674 │
└───────────────┴──────────────────────────────────────────────────────────────┘

[davidbeers]

The root of the problem is in jsonlint: zaach/jsonlint#133.
There is an open PR on jsonlint to fix it by using the maintained fork of nomnom instead of the deprecated one, but the maintainer may need some encouragement to test and accept the PR: zaach/jsonlint#120

[ThisIsMissEm]

dupe of #1020

[davidbeers]

Well, no, it's not a dupe of #1020. This one is due to geojson-hint > json-lint > nomnom > underscore.

[ThisIsMissEm]

Well, no, it's not a dupe of #1020. This one is due to geojson-hint > json-lint > nomnom > underscore.

Well, it is in a way, as #1020 covers all vulnerabilities current (look at the screenshot, it lists both geojson-hint and geojson-flattern)

Edited: noticed that geojson-hint has multiple vulnerabilities (minimist and underscore). I've already opened an issue suggesting removing geojson-hint as that package is entirely deprecated, see #1051

Either way, upgrading packages to resolve vulnerabilities because downstream packages depend on minimist will close both these tickets

[murdocha]

We really like the mapbox-gl tools, but this audit issue is causing us some problems at the moment.

The NPM audit issue is preventing us (because of our agency security policies) from deploying an app using mapbox-gl-draw into our production environments. Bad timing because we just wrapped up development and testing about the time that NPM audit began to flag this issue as a "high" severity finding.

options now include:

• stripping out mapbox-gl-draw related features (probably what we'll do short term)

and then medium term:

• rewriting our custom draw features to not use mapbox-gl-draw
  or
• wait for a patch to get deployed for this from the mapbox-gl-draw maintainer team.

Do you have any other suggestions for us as a potential work around, or (better) any planned ETA on a fix?

[ThisIsMissEm]

@murdocha maybe fork the package? (so clone the repo then change the package name to publish it to your own internal packages registry (e.g., github packages) — but to remove the security issues in the chain it's not particularly difficult, just needs someone from the mapbox team to say yes.

[murdocha]

We do need to do that (have an internal packages registry) so we have that option going forward (in a pinch), but we haven't set that up yet. That sounds like a solid work-around.

So in that scenario, we would clone the root cause library (jsonlint)? which has an open PR fix that never got merged here: zaach/jsonlint#120 ?
and merge that PR fix into our cloned, private, repo that we then publish to a private package registry?

[ThisIsMissEm]

You could also maybe try the patch-package package, or audit the code yourself — I think the exploits are in the CLI dependencies, not the runtime dependencies.

…

On 17. May 2021, at 22:10, Andrew Murdoch ***@***.***> wrote: We do need to do that (have an internal packages registry) so we have that option going forward (in a pinch), but we haven't set that up yet. That sounds like a solid work-around. So in that scenario, we would clone the root cause library (jsonlint)? which has an open PR fix that never got merged here: zaach/jsonlint#120 ? and merge that PR fix into our cloned, private, repo that we then publish to a private package registry? — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[mourner]

No worries — I'm looking into this today, sorry for the delay! I'll see if we can fix the transitive dependencies now, but eventually we should probably remove GeoJSON validation from the package altogether.

[mourner]

Looking into this further, we should just remove the validation — it isn't very useful for its added size, it's applied inconsistently as @ThisIsMissEm mentioned in #1051, relies on an unmaintained library, and removing it is technically not too breaking since the code that worked before will continue working after the upgrade.