Do not rely on package.json and package-lock.json

[juj]

Every now and then we get these "CVE vulnerability" reports against Unity, which stem from automated security scanners that are used that run through Unity's Emscripten NPM packages.

Sometimes corporations have a naive approach to security, where they'll run these scanners, and then decide that a piece of software is out of question to use if the scanner finds any CVE reports against it.

Then we get bug reports since they think Unity's web packages are insecure.

Now when I recently looked into a bunch of these bug reports, I find that this so-called scanner seem to be lazily written, and just read the package.json files that it finds, and then raises vulnerabilities if it matches any of the packages there against any known CVE reports - as opposed to actually matching files against known vulnerable code patterns, or checking what actual NPM package files are present on disk.

In Emscripten repo we use NPM's dependencies vs devDependencies split to separate packages for developers who use Emscripten, vs developers who develop Emscripten. At Unity we do not ship any devDependencies, we only ship items in the dependencies list (minus wasm2c, #19367)

These scanners do not understand such distinction, and they assume that all packages are installed, and all packages are shipped on to a live web page.

We could try to race the devDependencies package list to be up to date against all vulnerabilities, but the issue is that even if we update Emscripten to all the latest NPM packages today, in the 2-4 year lifespan of Unity, there will be new CVE reports coming against many of them that we cannot have anticipated. And these packages were never even part of the Unity Editor we shipped in the first place.

So this PR looks at another idea: would it be possible to make Emscripten not rely on package.json file after initial installation, so that we could just delete that file when we are distributing Emscripten at Unity.

This way lazy scanners won't yell at us for CVE vulnerabilities on code that we are not even shipping.

[sbc100]

I guess that idea here is that you want to remove the package.json file from your distribution of emscripten?
Could you avoid this change by instead just emptying it out? i.e. remove all the deps from it, but have it still be there to act as root so that that our node_modules can be located?

Perhaps we should do the same thing for emsdk, so that we know we have good test coverage?

[juj]

Yeah, I want to delete the package.json file in our Unity redistribution, at the root, or possibly even recursively in all subdirectories. Looks like the CI run came back green for when one would just delete the root package.json files, so that seems functionally safe thing to do, if we only change this one line.

I would prefer to change the acorn lookup here to be manual so we don't need to rely on package.json at all, since otherwise I would need to maintain stub package.jsons in our redistributable creator script. Manual lookup here does not look so bad, as terser already is being manually found as well anyways? (on the next line)

I am not proposing that package.jsons would (necessarily) be deleted from the emscripten installations that google's waterfall does, but that the test runs that we are running on Emscripten CircleCI would always run in this mode where these package.json files (either at root, or recursively) are deleted, so that we know that running in this mode will remain safe thing for us to rely on. (I don't anticipate that we would evolve any features that would actually need these)

Perhaps we should do the same thing for emsdk, so that we know we have good test coverage?

Hmm, emsdk does not use node_modules/package.json, or maybe I misunderstood here?

[sbc100]

I think both unity's distribution and emsdk's distribution expect node_modules to be pre-existing and don't expect to ever run npm to add/remove/update stuff.

So in both/all cases I think we can strip the package.json file when we ship.

I would rather ship an empty one so that we can avoid absolute paths.. but maybe there is some other way do that.

[sbc100]

Also this whole thing strikes me as rather crazy.. since we not actually removing any of the insecure code, just making a little harder for the security scanners to find it :-/ I'm not sure that's a good direction to go in.

[sbc100]

BTW wasm2c is being removed right now: #19380

[sbc100]

BTW, when you prepare the emscripten directory for shipping do you use emsdk.py to produce it? Or do you use emscripten's ./tools/install.py? If we go ahead with this change I think we should have install.py be the place where the package.json file is stripped or removed. See

emscripten/tools/install.py

Line 46 in 89c2932

def copy_emscripten(target):

[juj]

Closing old stale PR.