Skip to content
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

Critical Vulnerability with monorepo-symlink-test #318

Closed
bbuck opened this issue Sep 19, 2023 · 8 comments
Closed

Critical Vulnerability with monorepo-symlink-test #318

bbuck opened this issue Sep 19, 2023 · 8 comments

Comments

@bbuck
Copy link

bbuck commented Sep 19, 2023

https://security.snyk.io/vuln/SNYK-JS-MONOREPOSYMLINKTEST-5865510

Let me start by saying I know your stand, I've read the other threads and your comments. I absolutely disagree with your opinion on the matter though. Let me explain:

I could have copied the malicious package wholesale into a larger PR I made against this project. I could have easily added "private": true. And now these tools are trying to warn folks there may be (not all flags mean there are which seems to be the misunderstanding) an issue with a package. So we investigate, these seems like a coincidence. So it's not an issue with my system -- but it is an issue with this package.

I can't fathom why you would maintain that you refuse to change the name of an internal, private, test-only package because "the scanners are broken" when the fix is as simple as just renaming it to something else. You then reduce the influx that you have, and most likely will, continue to get with this package being flagged. It also seems to be heavily depended on so there is a wide breadth of projects this is used in.

If the change was gargantuan and required weeks to pull off, sure I could get it. But it's a private, test-only package so how integrated is the specific is the name in the project?

Would you even except a pull request to address the issue? The tools should be overly sensitive and flag false positives (obviously not flagging everything, that's not what I'm saying) or else it's going to end up missing actual issues that could have been caught.

@ljharb
Copy link
Member

ljharb commented Sep 19, 2023

If you've read them, why are you opening yet another duplicate issue about it?

You are exactly right that the private: true doesn't alone guarantee it's safe, but the package name provides ZERO SIGNAL WHATSOEVER, and thus relying on it is incompetent.

No, I will quite literally never change the name of the package. If a tool reports on it, it's a bad tool and you should stop using it.

Tools should DEFINITELY not be overly sensitive; false positives in security are MUCH worse than false negatives, because too much noise undermines users' confidence in the concept of security tooling itself. Missing an actual issue is BETTER than bothering people with nonsense.

Duplicate of #317.

@ljharb ljharb closed this as not planned Won't fix, can't repro, duplicate, stale Sep 19, 2023
@bbuck
Copy link
Author

bbuck commented Sep 19, 2023

I opened a new ticket to continue the influx that your refusal to change is/has/will cause. And to continue discourse.

I don't understand what the harm is in changing the name of the internal package, we (as users/other folks winding up here) must not see something critical important that you see that prevents a solution so simple from being possible or even considered. I wanted to try and understand this rigid stance more and also provide rebuttal against reasonings that I read in the other issues.

edit

Also you're the first person I've interacted with that actively encouraged the non-identification of security vulnerabilities as being a good thing. That's wild.

@bbuck
Copy link
Author

bbuck commented Sep 19, 2023

I suppose that doesn't read as a question and just an answer --

To continue, I asked if you'd even consider a PR to address the issue. Since you don't seem interested in addressing the issue. I assume your response:

No, I will quite literally never change the name of the pacakage.

Is intended to answer that question but you did say "I," so I'm just clarifying.

@ljharb
Copy link
Member

ljharb commented Sep 19, 2023

Sorry, to clarify, I would not accept such a PR.

Most CVEs in the JS ecosystem are false positives, and the noise from that causes far more harm to security than a few false negatives would. I work professionally in a security capacity as well as with my open source work, so this is not an uninformed opinion.

@bbuck
Copy link
Author

bbuck commented Sep 19, 2023

I'm trying to recognize that you have an informed opinion, and trying to ask what the information is that's causing the issue? Do you work on a competitor to all the other security providers out there? Is there a monetary benefit for you to claim they're all broken and should be avoided? What's the reason for being so stubborn about this?

At this point, the insistent refusal to change is contributing to false positives, and it could easily be fixed!

@ljharb
Copy link
Member

ljharb commented Sep 19, 2023

lol no, there's no monetary benefit, and i'm not even fulltime employed at the moment. They're also not all broken; snyk itself doesn't report on this, for example.

The reason is because I shouldn't have to make any change to a project solely because a security scanning tool did something wrong. When things are broken, they shouldn't be papered over solely for convenience - that makes everything worse.

In other words, while the noise caused by this false positive is annoying and inconvenient, it serves a beneficial purpose: highlighting which tools need to fix their wildly flawed heuristics, and, highlighting when company policies foolishly block engineering progress on a CVE warning (since most of them are false positives, they shouldn't ever block progress).

ljharb added a commit that referenced this issue Oct 10, 2023
…d security scanners

Fixes #319.
Fixes #318.
Fixes #317.
Fixes #314.
Closes #313.
Fixes #312.
Fixes #311.
Fixes #310.
Fixes #309.
Fixes #306.
Fixes #305.
Fixes #304.
Fixes #303.
Fixes #291.
Fixes #288.
ljharb added a commit that referenced this issue Oct 10, 2023
    Fixes #319.
    Fixes #318.
    Fixes #317.
    Fixes #314.
    Closes #313.
    Fixes #312.
    Fixes #311.
    Fixes #310.
    Fixes #309.
    Fixes #306.
    Fixes #305.
    Fixes #304.
    Fixes #303.
    Fixes #291.
    Fixes #288.
ljharb added a commit that referenced this issue Oct 10, 2023
    Fixes #319.
    Fixes #318.
    Fixes #317.
    Fixes #314.
    Closes #313.
    Fixes #312.
    Fixes #311.
    Fixes #310.
    Fixes #309.
    Fixes #306.
    Fixes #305.
    Fixes #304.
    Fixes #303.
    Fixes #291.
    Fixes #288.
@rraj77
Copy link

rraj77 commented Jan 23, 2024

I found the solution
add this in package.json

  "resolutions": {
    "resolve": "^1.22.8"
  }

@ljharb
Copy link
Member

ljharb commented Jan 24, 2024

That’s not necessary; you can just update your lockfile, no overrides required. However, you should still stop using any broken tool that’s complaining about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants