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

[gitJoe42]

Hello, I deleted the node_modules folder in my meteor app and made meteor npm install. After that the app can't be started and ends up with the following error:

=> Started proxy.
/Users/jochen/.meteor/packages/semantic_ui/.2.2.6_5.h77ycc.hbbb4++os+web.browser+web.cordova/plugin.generateSemanticUi.os/npm/node_modules/meteor/promise/node_modules/meteor-promise/promise_server.js:165
      throw error;
      ^

SyntaxError: Unexpected end of JSON input
    at JSON.parse (<anonymous>)
    at /Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/optimistic.ts:321:17
    at wrap.makeCacheKey (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/optimistic.ts:36:15)
    at recomputeNewValue (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:198:31)
    at Slot.withValue (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/@wry/context/lib/context.esm.js:69:29)
    at reallyRecompute (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:181:19)
    at Entry.recompute (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:91:9)
    at optimistic (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/index.ts:150:25)
    at /Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/optimistic.ts:366:19
    at recomputeNewValue (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:198:31)
    at Slot.withValue (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/@wry/context/lib/context.esm.js:69:29)
    at reallyRecompute (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:181:19)
    at Entry.recompute (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/entry.ts:91:9)
    at optimistic (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/optimism/src/index.ts:150:25)
    at find (/tools/isobuild/package-source.js:1344:30)
    at /tools/isobuild/package-source.js:1400:27
    at Array.forEach (<anonymous>)
    at find (/tools/isobuild/package-source.js:1379:22)
    at /tools/isobuild/package-source.js:1400:27
    at Array.forEach (<anonymous>)
    at find (/tools/isobuild/package-source.js:1379:22)
    at /tools/isobuild/package-source.js:1400:27
    at Array.forEach (<anonymous>)
    at find (/tools/isobuild/package-source.js:1379:22)
    at /tools/isobuild/package-source.js:1400:27
    at Array.forEach (<anonymous>)
    at find (/tools/isobuild/package-source.js:1379:22)
    at find (/tools/isobuild/package-source.js:1411:25)
    at /tools/isobuild/package-source.js:1423:34
    at Object.withCache (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/files.ts:1662:18)
    at PackageSource._findSources (/tools/isobuild/package-source.js:1423:18)
    at SourceArch.getFiles (/tools/isobuild/package-source.js:965:32)
    at /tools/isobuild/compiler.js:406:23
    at /tools/isobuild/compiler.js:186:28
    at Object.withCache (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/files.ts:1662:18)
    at /tools/isobuild/compiler.js:185:11
    at Function.each (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/underscore/underscore-node-f-pre.js:1316:7)
    at Object.compile (/tools/isobuild/compiler.js:180:5)
    at /tools/isobuild/bundler.js:3291:24
    at Object.capture (/tools/utils/buildmessage.js:283:5)
    at bundle (/tools/isobuild/bundler.js:3237:31)
    at /tools/isobuild/bundler.js:3180:32
    at Slot.withValue (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/dev_bundle/lib/node_modules/@wry/context/lib/context.esm.js:69:29)
    at Object.withCache (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/fs/tools/fs/files.ts:1662:39)
    at Object.bundle (/tools/isobuild/bundler.js:3180:16)
    at /tools/runners/run-app.js:581:24
    at Function.run (/Users/jochen/.meteor/packages/meteor-tool/.2.4.0_1.1npmc7f.35nt++os.osx.x86_64+web.browser+web.browser.legacy+web.cordova/mt-os.osx.x86_64/tools/tool-env/tools/tool-env/profile.ts:289:14)
    at bundleApp (/tools/runners/run-app.js:580:34)
    at AppRunner._runOnce (/tools/runners/run-app.js:627:35)
    at AppRunner._fiber (/tools/runners/run-app.js:949:28)
    at /tools/runners/run-app.js:410:12

When I changed resolve/test/resolver/malformed_package_json/package.json to a well formed package.json by adding a '}' everything is fine.

[ljharb]

This seems like a bug in meteor; there’s almost no reason to ever be reading a non-package-level package.json file in node_modules, and nothing in node_modules should be expected to be well-formed.

See #264.

[ljharb]

Actually it looks like a bug in meteor-tool specifically, based on the stack trace. Please file an issue and link it here.

[gitJoe42]

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

Good point.

[ljharb]

(to be clear: the only reason is to look for "main" or "type": "module" when doing resolution, and in either case, the only correct thing to do is ignore unparseable files, just like node itself does)

[ngraef]

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.

[ljharb]

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

Good npm citizens publish their tests, in my opinion.

[ngraef]

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.

[andyedwardsibm]

Another scenario is where Twistlock/Prisma scans of images that contain resolve report scan errors due to the malformed package.json files. In that case the scanner is hunting through the entire image looking for package.json files and scanning those "test" files.

Would a possible "solution" be to not have package.json files in the test folder but to mock out fs.readFile() in each test? That was you still get to have the tests included in the module but you don't then have deliberately-invalid package.json files on disk

[ljharb]

@andyedwardsibm thats still a bug in that tool - both because if it’s scanning for dependency info, it should only be scanning package.json files that can possibly install them (at the root of packages), and also because anything scanning third-party code should never be able to assume it’s well-formed. Otherwise, it’d be a pretty trivial attack to add a malformed nested package.json file to a malicious package.

Yes, i could probably mock things out rather than have real fixtures, but then I’ve made my tests more brittle only so other tools’ bugs and security vulnerabilities can avoid getting fixed.

[fernandopasik]

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

[robertfoobar]

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]

@robertfoobar it's not really possible to have "sensitive information or functions" in test files for a language like javascript; that CWE doesn't apply here.

[robertfoobar]

@ljharb That's not the point. My point is that this approach violates general best practices, namely

Remove test code before deploying the application into production.

and, in doing so, causes additional problems for others. 😕

[ljharb]

@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.

[alansikora]

Having this file, named package.json, inside a test directory, on a dist build made me dig through for 2 days, I was on a very specific env.

Test code on dist versions does not make sense, I trust the authors already tested it when it was shipped.

[ljharb]

@alansikora node versions are released after it's shipped, and nothing's tested on every possible runtime environment. deps shipping the tests means i can easily make sure those deps are behaving as expected, even on my idiosyncratic setups.

[EvHaus]

@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?

[ljharb]

It has zero impact on bundle size, since only imported files end up in the bundle. Issues like this only happen when tools do incorrect things - namely, looking at package.json filed naively instead of only directly inside package boundaries.

The only impact it has (módulo broken tools) is on install size, and very little on that.

[SystemStack]

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

[humarkx]

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"

[ljharb]

@humarkx whatever's trying to parse that is broken. I assume it's meteor since you're posting on this issue, but either way this repo isn't the place to post about it. See upthread.

[mirek]

vscode with flow extension fails with "Unexpected end of input, expected the token }" as it digs into this test file.

Just drop test from being published in npm, tests should live in source control (github) not in published package.

[ljharb]

@mirek why would vscode - or any tool - ever be digging into test files inside node_modules?

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

[spflinux]

Hope to fix this problem earlier， this problem due to just one "{" in the package.json and can not be parse correctly.

[JakeThurman]

My org uses (Sonatype)[https://www.sonatype.com/products/sonatype-repository-firewall] for scanning dependencies for vulnerabilities. It parses package.json files under node-modules during this process to confirm what packages are going to be in play.

It fails reading node_modules\resolve\test\resolver\malformed_package_json\package.json. I'm able to ignore all errors but not this one specifically, and I'd rather not have to in case a new error appeared that I did want to address.

@ljharb I would really like to push back on the need to run tests on a dependency installed from npm. Maybe I am missing the use case, but clearly the current state is causing the lot of pain. Webpack used to have this problem from a similar tests and I filed a bug report, and they fixed it! Webpack does not ship their tests.

Since I do see you feel strongly, would you consider ignoring just the test/resolver/malformed_package_json directory from the package? That is the test case that causes issues with many scanners and may be a good compromise.

[ljharb]

Then that’s a bug in sonatype; I’d suggest filing it with them.

no, i won’t ignore that test case in the package. I see this “pain” as a good thing since it’s exposed how many scanning tools have fundamentally broken and naive heuristics.

[JakeThurman]

@ljharb The check against all installed files is legit. Because you ship the test files, I imagine it ends up directly on many production servers, or at least is using during the build of production code, which we want to be safe as well. While it may seem like it at first, this is not naive.

The scanner checks EVERY file that is part of our production build for things like source and similarity to known malicious code. Just because it's labeled as "tests", that is not a reason to trust it. How many zero day vulnerabilities are due to not starting from a point of zero-trust? To make up an example, for all we know this could be a binary for a virus called "package.json" to avoid detection, by being under tests/, being a "known safe file name", etc.

Just to emphasize, it's not that this file is malicious, but we have no reason to trust it until we check it.

Am I missing something about what makes checking all installed naive beyond that? I am open to changing my mind here but I'm just not understanding why we wouldn't check all code we install from the internet. Is there some other heuristic you are expecting?

[ljharb]

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

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.

[JakeThurman]

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.

[ljharb]

No, it's not a hard problem to solve - it's the way bundlers work. You can simply traverse the dependency graph.

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.