-
-
Notifications
You must be signed in to change notification settings - Fork 184
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
change package name on test to avoid scanning issue #313
Conversation
I don't wish to change my repo solely so naive/broken/incompetent security scanners can more easily hide the fact that they are so. Snyk has been made aware of the problem and will surely correct it soon. |
@@ -1,5 +1,4 @@ | |||
{ | |||
"name": "monorepo-symlink-test", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the "name" field is always required in package.json, so this can't just be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be involved to rename the package to something like resolve-monorepo-symlink-test? In saying that, I think the posibility of a malicous person would simply deploy a new package with the new name, so this would be a temporary fix potentially. Unfortunately the scanner I used was AWS Inspector which I would think is widely used, so this is probably affecting many people as indicated with all the previous open issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I certainly could - but then people might not realize that their scanner doesn't actually understand how npm packages work, and might make the mistake of thinking it's helping their security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think people might understand but a simple rename might save them lots of time explaining it's a false positive if they work in a medium/big company where such scans are heavily used.
I adapted the PR with the renaming. Up to you @ljharb :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ljharb i totally get where you are coming from. However it's not the internal discussion that is difficult, but it's to the 30 different customers, many from large enterprises who are using our docker packages.
We dont have the access to the right people, nor the bandwidth to explain they should ignore their security scanner. This means they are not deploying at all. It's quite a difficult situation where we are contemplating forking this package to address the issue. Obviously this is insane!
Hi @ljharb. From what I understand, the package in question is a test package. I would like to kindly challenge the existence of a test package in releases. 🤔 Seem like this piece of code is not used in the released code at all. Therefore, does it make sense for it to be there in the first place? Unless it is used and I just missed that, then please let me know. Thank you 🙏 |
I went ahead and published v1.22.8 and v2.0.0-next.5 without a malformed package.json, and with a renamed test project from one coincidental name to another. I still strongly suggest you all find a better security tool that doesn't create noisy false positives; a security tool needs to be trusted, and naïveté is not a secure trait. |
There is a coincidence where a private test package with name
monorepo-symlink-test
This ends up being mixed by security scanners scanners with https://security.snyk.io/vuln/SNYK-JS-MONOREPOSYMLINKTEST-5865510
This should fix #312