Skip to content

npm bom incorrectly lists PURLs for linked/file deps #1697

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

Open
ljharb opened this issue Mar 24, 2025 · 14 comments
Open

npm bom incorrectly lists PURLs for linked/file deps #1697

ljharb opened this issue Mar 24, 2025 · 14 comments

Comments

@ljharb
Copy link

ljharb commented Mar 24, 2025

On https://github.com/ljharb/list-exports, i ran npx @cyclonedx/cdxgen. After pretty-printing the resulting JSON file, I see, for example:

{   
    "authors": [
        {
            "name": "ForbesLindesay"
        }
    ],  
    "group": "",
    "name": "is-promise-2.2.2",
    "version": "2.2.2",
    "description": "Test whether an object looks like a promises-a+ promise",
    "scope": "optional",
    "licenses": [
        {
            "license": {
                "id": "MIT",
                "url": "https://opensource.org/licenses/MIT"
            }
        }
    ],
    "purl": "pkg:npm/[email protected]",
    "externalReferences": [
        {
            "type": "vcs",
            "url": "https://github.com/then/is-promise#readme"
        },
        {
            "type": "vcs",
            "url": "git+https://github.com/then/is-promise.git"
        }
    ],
    "type": "library",
    "bom-ref": "pkg:npm/[email protected]",
    "properties": [
        {
            "name": "SrcFile",
            "value": "node_modules/@fixtures/is-promise-2.2.2/package.json"
        }
    ],
    "evidence": {
        "identity": [
            {
                "field": "purl",
                "confidence": 0.7,
                "methods": [
                    {
                        "technique": "manifest-analysis",
                        "confidence": 0.7,
                        "value": "node_modules/@fixtures/is-promise-2.2.2/package.json"
                    }
                ],
                "concludedValue": "node_modules/@fixtures/is-promise-2.2.2/package.json"
            }
        ]
    }
},

Note the purl field which says pkg:npm/[email protected].

That's certainly where this package.json file came from at one point - but it's vendored into the repo, as a test fixture, and depended on with file:. The hidden npm lockfile also has link: true in it.

Thus, it should not have a PURL at all because it's not a package that's installed from anywhere. A vulnerability in pkg:npm/[email protected] has no guarantee of applying to this code or not, because it's just a local package.json file that I could have changed the "name" of from or to anything else.

@prabhu
Copy link
Collaborator

prabhu commented Mar 25, 2025

Interesting. purl can be generated for local linked packages too. I have users who track CVEs for local private packages, so I don't think this is a bug. Perhaps we can reduce the confidence a bit?

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

I'm not sure why - if it's not ever installed from a registry, then it should never have the purl of the public package.

Certainly I'd also reduce the confidence; for an npm package, you can even checksum the contents and compare that to the contents in the published tarball, if you want to be absolutely certain.

@dwelch2344
Copy link

@ljharb It's a good question. On one hand, if you've got a local fork of something, I'd argue you should probably change the package name accordingly. On the other, perhaps adding a qualifier to linked packages a la ?type=local or ?type=linked could be a solution?

@dwelch2344
Copy link

One other thought: could there be a way to specify adapting the record accordingly? a la one of the config/rc files (I know there's some existing options, but don't think anything that'd solve this) or worst case providing some sort of extension point/visitor pattern/etc.

I know there's always post-processing the whole sbom, but have wanted the chance to drop a simple map function that could edit the generated component/record before moving on

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

It's a test fixture and a dev dep, it's not actually used in any code, and the name in package.json that points to it is changed - it's just the local name in the package.json file that's unchanged, and that local name doesn't actually mean anything, and cdxgen probably shouldn't be relying on it in any case (the lockfile, not the package.json, tells you where a package came from)

I'm not sure what the best approach is, but I think that false positives in security are far worse than false negatives, and while anything that's linked and/or vendored should be included, they shouldn't in any way risk falsely implying that they're the same package as something installed from the registry - because they're not, in the majority case.

@prabhu
Copy link
Collaborator

prabhu commented Mar 25, 2025

I think we can use one of the attributes such as scope=excluded instead of a custom purl qualifier. I want to understand how big of a problem is this? Anyone else have issues with linked packages? Also what if a real public package gets used in a link manner?

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

The latter is the job of cdxgen to determine, and if it looks at the available lockfiles instead of naively trusting the package.json file, then it can get that information pretty easily.

@prabhu
Copy link
Collaborator

prabhu commented Mar 25, 2025

Would you be interested in contributing a PR? We do look at the lock files and parse using arborist and babel.

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

Possibly - can you give me some pointers where to look, and where test cases should go?

@prabhu
Copy link
Collaborator

prabhu commented Mar 25, 2025

export async function parsePkgLock(pkgLockFile, options = {}) {

Use an IDE with a working debugger.

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

thanks, where would test go? those aren't run in the IDE :-)

@prabhu
Copy link
Collaborator

prabhu commented Mar 25, 2025

That's your homework. Figure it out :)

@ljharb
Copy link
Author

ljharb commented Mar 25, 2025

lol i'm not sure i have time for that level of contribution, then.

@prabhu
Copy link
Collaborator

prabhu commented Mar 26, 2025

"scope": "optional"

Looks like cdxgen is working correctly as designed. The scope and the evidence.identity.concludedValue correctly captures how the component was identified. Moving this to the discussion.

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

No branches or pull requests

3 participants