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

change package name on test to avoid scanning issue #313

Closed
wants to merge 2 commits into from

Conversation

cichelero
Copy link

@cichelero cichelero commented Sep 4, 2023

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

@ljharb
Copy link
Member

ljharb commented Sep 4, 2023

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.

@ljharb ljharb marked this pull request as draft September 4, 2023 15:51
@@ -1,5 +1,4 @@
{
"name": "monorepo-symlink-test",
Copy link
Member

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

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.

Copy link
Member

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.

Copy link
Author

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 :-)

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!

@cichelero cichelero marked this pull request as ready for review September 8, 2023 12:59
@cichelero cichelero changed the title remove package name on test to avoid scanning issue change package name on test to avoid scanning issue Sep 11, 2023
@dotintegral
Copy link

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.

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 🙏

@ljharb
Copy link
Member

ljharb commented Oct 10, 2023

@dotintegral see #312 (comment)

@ljharb ljharb marked this pull request as draft October 10, 2023 20:08
ljharb added a commit that referenced this pull request 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 ljharb closed this in 9e6f936 Oct 10, 2023
ljharb added a commit that referenced this pull request 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
Copy link
Member

ljharb commented Oct 10, 2023

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.

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

Successfully merging this pull request may close these issues.

monorepo-symlink-test
6 participants