The test/resolver/malformed_package_json/package.json leads to an fatal error when starting meteor #271
Comments
|
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. |
|
Actually it looks like a bug in meteor-tool specifically, based on the stack trace. Please file an issue and link it here. |
Good point. |
|
(to be clear: the only reason is to look for |
|
Can I ask why I found this issue because I was using a different tool that scans all package.json files under node_modules, and it crashed on |
|
@ngraef because Good npm citizens publish their tests, in my opinion. |
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. |
|
Another scenario is where Twistlock/Prisma scans of images that contain Would a possible "solution" be to not have |
|
@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. |
|
Found this problem as well when running flow (https://github.com/flowtype/flow-bin) (latest version) As a workaround I just added the file to the ignore list in the |
|
This issue unfortunately seems to cause a lot of avoidable hassle. It cost us almost one day to analyse.
I also don't agree. Shipping test code is considered a weakness in software development. There's even a CWE for that:
Although you're IMO right that affected third party apps should be implemented more resiliently, this probably is not going to happen soon. |
|
@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. |
|
@ljharb That's not the point. My point is that this approach violates general best practices, namely
and, in doing so, causes additional problems for others. 😕 |
|
@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. |
|
Having this file, named Test code on dist versions does not make sense, I trust the authors already tested it when it was shipped. |
|
@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. |
@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? |
|
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. |
|
This was absolutely hilarious, spent half an hour on it. Thanks for the laughs, added to my comedy gold bookmarks 👍🏽 |
|
|
|
@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. |
|
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. |
|
@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 |
|
Hope to fix this problem earlier， this problem due to just one "{" in the package.json and can not be parse correctly. |
|
My org uses (Sonatype)[https://www.sonatype.com/products/sonatype-repository-firewall] for scanning dependencies for vulnerabilities. It parses It fails reading @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 |
|
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. |
|
@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 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? |
|
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. |
While true, to know what files will be executed at runtime is a hard problem to solve, no?
To my knowledge, all package.json files. And it does because you can import this sub-package using |
|
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. |
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.
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. |
|
Subpackages surely can be imported, but they can't run arbitrary scripts or contain dependencies, which are the only security concerns to be worried about. In particular, if a package.json file not at a package boundary doesn't parse, then node will either block importing or use fallback behavior as if it wasn't present - so a non-parseable package.json is the same as it not existing in the first place. In other words, for a security scanner, a nonparseable package.json MUST always be ignored, or else it won't be checking for things the same way node does. |
While true, we also want to produce a full "bill of materials" for our product, including all open source packages used (this is one of the things the scanner outputs and it uses the package.json to get the name, urls, etc.). The scanner checks a CVE database for each package it finds on a variety of keys. A subpackage may very well be an inlined vulnerable or malicious package and we want to scan that all the same. Not just by root package name.
Thanks for explaining. I think maybe this is the bit I was missing. I still disagree about shipping the tests, but I will add some more information to my bug against the scanner. I'll leave it here. Thanks for engaging! |
|
An SBOM that goes more granular than "a root package.json and its deps" is unnecessary, and one that includes unused code wouldn't be accurate anyways. A "subpackage" is just a subdirectory of a package, and in the npm ecosystem "package" is the atom - there's no such thing as a "subpackage" in reality, it's just a mental model one can hold when working with a package. Thanks, I look forward to the link so I can follow progress. |
Even if it's unused, serving a malicious file from our domain by it being on our webserver would be bad. It's not that there aren't other mitigations to this, but we want defense in depth, and not allowing the file to make it to the webserver in the first place is our goal. |
|
Well sure, but blindly serving node_modules is always a massive security hole, and should never be done in the first place. |
I saw this statement repeated several times in this thread. Where does this comes from? Is it recommended somewhere in the Npm documentation?
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.
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. |
|
@axiac it was the way the entire community did it in the early days of npm, and some of us still cleave to the old ways. Disk space is infinite and free, and any security scanner that reports on never-executed files is not one I'd suggest using. The security scanning tools are thus useless because that is exactly what's required - what they're doing now is providing false positives, not actual results, which is the problem you're complaining about. |
|
@ljharb Thank you. |
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:When I changed resolve/test/resolver/malformed_package_json/package.json to a well formed package.json by adding a '}' everything is fine.
The text was updated successfully, but these errors were encountered: