browserify / resolve

Implements the node.js require.resolve() algorithm
MIT License
775 stars 183 forks source link

monorepo-symlink-test #312

Closed roberthockley closed 1 year ago

roberthockley commented 1 year ago

Hi,

It seems that resolve has a dependancy on monorepo-symlink-test. I see it in node_modules/resolve/test/resolver/multirepo/package.json.

The monorepo-symlink-test module has been identified as having a critical security risk:

https://security.snyk.io/vuln/SNYK-JS-MONOREPOSYMLINKTEST-5865510

Are there plans to remove this dependancy, or swap it out with something else?

spanagiot commented 1 year ago

From what I see and understand they include the source code directly in the test folder and do not depend on NPM. The source code of the included library is here https://github.com/browserify/resolve/blob/main/test/resolver/multirepo/packages/package-a/index.js https://github.com/browserify/resolve/blob/main/test/resolver/multirepo/packages/package-b/index.js and I don't see anything strange. Please correct me if I'm wrong

cichelero commented 1 year ago

It seems this is just a confusion. The monorepo-symlink-test dependency is not being used by this package, but there is a package.json in the test folder that has the same name, which generates the confusion: https://github.com/browserify/resolve/blob/464fde031724c742630eb6653050f4fe1391da1b/test/resolver/multirepo/package.json#L2

I think the simplest solution is to simply remove the name of the test package. However a cleaner solution is not to ship the test folder with the NPM package.

ljharb commented 1 year ago

Because it's a private package that just coincidentally has the same name as the malicious one, it is a false positive - so whatever tool is flagging this repo is broken, and you should strongly reconsider using a tool that is this naive about npm package names.

Duplicate of https://github.com/browserify/resolve/issues/303. Duplicate of https://github.com/browserify/resolve/issues/291. Duplicate of https://github.com/browserify/resolve/issues/288. Duplicate of https://github.com/browserify/resolve/issues/304. Duplicate of https://github.com/browserify/resolve/issues/305. Duplicate of https://github.com/browserify/resolve/issues/306. Duplicate of https://github.com/browserify/resolve/issues/309. Duplicate of #310. Duplicate of #311.

Tests must be shipped with packages so that npm explore foo && npm install && npm test always works.

cichelero commented 1 year ago

Tests must be shipped with packages so that npm explore foo && npm install && npm test always works.

@ljharb I don't understand why this is required for a package. Can you please explain a bit more?

ljharb commented 1 year ago

@cichelero so that i can debug an installed package by running its tests, whether i have internet or not, and even if the github repo has been deleted (for example, substack deleted his github recently and a thousand repos vanished).