resolve: The test/resolver/malformed_package_json/package.json leads to an fatal error when starting meteor

Found this problem as well when running flow (https://github.com/flowtype/flow-bin) (latest version)

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/resolve/test/resolver/malformed_package_json/package.json:2:1

Unexpected end of input, expected the token }

     1│ {
     2│

Found 1 error
error Command failed with exit code 2.

As a workaround I just added the file to the ignore list in the flowconfig file

[ignore]
<PROJECT_ROOT>/node_modules/resolve/test/resolver/malformed_package_json/package.json

Can I ask why resolve publishes the test directory in its package in the first place? In my opinion, test/ should be in .npmignore.

I found this issue because I was using a different tool that scans all package.json files under node_modules, and it crashed on test/resolver/malformed_package_json/package.json. Yes, that tool needs to be fixed to ignore malformed input; however resolve could also be a good NPM citizen by cleaning up its releases.

because npm explore foo && npm install && npm test should always work.

I don’t agree with that statement, because in many cases it would require publishing nearly the entire contents of the project in the package. I believe making it easy to develop on the module is the job of the source repo, not the release artifacts. Regardless, this is not my package to maintain. Thank you for explaining the reasoning behind the decision, and thank you for all the maintenance work you do.

@robertfoobar that’s not been a best practice in the npm ecosystem, historically.

What I see is that it’s not caused problems, it’s exposed flaws in existing tools - flaws that have made them run MUCH slower than they need to, because they’re scanning WAY more files than they need to.

This issue unfortunately seems to cause a lot of avoidable hassle. It cost us almost one day to analyse.

Good npm citizens publish their tests, in my opinion.

I also don’t agree. Shipping test code is considered a weakness in software development. There’s even a CWE for that:

Accessible test applications can pose a variety of security risks. Since developers or administrators rarely consider that someone besides themselves would even know about the existence of these applications, it is common for them to contain sensitive information or functions.

Although you’re IMO right that affected third party apps should be implemented more resiliently, this probably is not going to happen soon.

@ljharb Thank you.

Tests should always be in the published package, so that npm install x && npm explore x && npm install && npm test always works.

I saw this statement repeated several times in this thread. Where does this comes from? Is it recommended somewhere in the Npm documentation?

It has zero impact on bundle size, since only imported files end up in the bundle.

This is an opinion strongly focused on frontend. The backend applications do not use bundles; bundling do not bring any benefit to them. These tests use space, trigger security issues and all the annoyances discussed here and do not bring any benefit.

The contents of a file that is never accessed, accessible, or executed is irrelevant.

This statement cannot be proved without running the code and taking all possible paths. The security scanning tools are correct. All files included in the application must be checked.

You can simply traverse the dependency graph.

It does! But like I mentioned this is a subpackage of a package that is imported. So as far as the package tree is concerned, this is included.

It’s a JSON file. It’s literally impossible to have any malice coming from it, because all it can do is parse, or fail to parse. The only thing that makes “package.json” special is when it’s at a package boundary, which this is not. I would be hesitant to trust a scanning tool that failed to understand this.

Subpackages are importable still though. Here’s an article talking about them. And from a vulnerability perspective, it would leave a hole in the protection of the scanner if it just allowed anything to be in subpackages.

To be clear it’s using the package.json file as one of many inputs to evaluation. It doesn’t need it to be there and you can ignore parsing errors, but I would rather not so that I see any other issues that come up. Which, seems to be the concern of others on this thread as well

---

I also just want to take a moment to thank you for your work on this package. I know that Open Source development can be pretty thankless, and I just want to say I appreciate the time and care put into these packages that make the whole ecosystem better for everyone.

The contents of a file that is never accessed, accessible, or executed is irrelevant.

While true, to know what files will be executed at runtime is a hard problem to solve, no?

Separately, is sonatype checking all JSON files? or is it merely checking package.json files? If the former, then a malformed JSON file shouldn’t kill the overall check; if the latter, this isn’t a package’s package.json, so it shouldn’t be checked at all.

To my knowledge, all package.json files. And it does because you can import this sub-package using require("resolve/test/resolver/malformed_package_json"). Just in case we do, it gets scanned.

failed to parse node_modules/resolve/test/resolver/malformed_package_json/package.json failed to parse node_modules/eslint-plugin-react/node_modules/resolve/test/resolver/malformed_package_json/package.json

"eslint-plugin-react": "^7.30.1"

This was absolutely hilarious, spent half an hour on it. Thanks for the laughs, added to my comedy gold bookmarks 👍🏽

@ngraef because npm explore foo && npm install && npm test should always work. Good npm citizens publish their tests, in my opinion.

@ljharb I’d love to understand your position on this a bit better. I can I see the benefit in the sense that users could run tests against the package locally to get confidence of its code quality, but it seems like it comes with some serious tradeoffs, namely increased bundle size and issues like this one. In my view it’s very easy to get the tests for the package by cloning the repo instead. I would imagine the number of people who would prefer a small npm bundle size would be much higher than the number of people who would appreciate tests to be included. Am I missing some other angle?

@ngraef because npm explore foo && npm install && npm test should always work.

Good npm citizens publish their tests, in my opinion.

there’s almost no reason to ever be reading a non-package-level package.json file in node_modules

Good point.