Closed gitJoe42 closed 1 year ago
ljharb
commented
2 years ago 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
commented
2 years ago Actually it looks like a bug in meteor-tool specifically, based on the stack trace. Please file an issue and link it here.
gitJoe42
commented
2 years ago there’s almost no reason to ever be reading a non-package-level package.json file in node_modules
Good point.
ljharb
commented
2 years ago (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
commented
2 years ago 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
commented
2 years ago @ngraef because npm explore foo && npm install && npm test should always work.
Good npm citizens publish their tests, in my opinion.
ngraef
commented
2 years ago because
npm explore foo && npm install && npm testshould 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
commented
2 years ago 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
commented
2 years ago @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
commented
2 years ago 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
commented
2 years ago 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
commented
2 years ago @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
commented
2 years ago @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
commented
2 years ago @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
commented
2 years ago 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
commented
2 years ago @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
commented
2 years ago @ngraef because
npm explore foo && npm install && npm testshould 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
commented
2 years ago 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.
LeviBroadnax
commented
2 years ago This was absolutely hilarious, spent half an hour on it. Thanks for the laughs, added to my comedy gold bookmarks 👍🏽
humarkx
commented
2 years ago 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
commented
2 years ago @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
commented
1 year ago 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
commented
1 year ago @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
commented
1 year ago Hope to fix this problem earlier， this problem due to just one "{" in the package.json and can not be parse correctly.
JakeThurman
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago @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
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago 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.
JakeThurman
commented
1 year ago 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.
ljharb
commented
1 year ago 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.
JakeThurman
commented
1 year ago 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.
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.
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.
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!
ljharb
commented
1 year ago 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.
JakeThurman
commented
1 year ago 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.
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.
ljharb
commented
1 year ago Well sure, but blindly serving node_modules is always a massive security hole, and should never be done in the first place.
axiac
commented
1 year ago Tests should always be in the published package, so that
npm install x && npm explore x && npm install && npm testalways 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.
ljharb
commented
1 year ago @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.
axiac
commented
1 year ago @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.