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

Check against undefined versions array from registry. #7

Closed
wants to merge 1 commit into from

Conversation

MarieKirya
Copy link

@MarieKirya MarieKirya commented May 11, 2021

What Changed

Checking to make sure that packument has a parameter called version. This has been updated during load and [_testVersion]

Why Change This

A recent change at npmjs' registry now gives unpublished repositories a registry entry and returns 200. However these entries do not have any version arrays, which causes issues with the metavuln-calculator. If you were to use private packages through git, the package name gets matched up to unpublished repositories on npmjs' registry and breaks

It's possible that this is simply a bug on npmjs' side and this will be fixed in a future update, but this patch for now has allowed me to continue working as normal. I could also be missing something in my custom dependency that is confusing npm, so if there's any way to not need this patch, I'm all ears. I'll do my best to monitor this PR.

Reproduction Steps

  • Create in npmjs with a unique name, package-name will be used in this reproduction steps
  • Unpublish package-name
  • Push up a valid package to a git source with the same name
  • Try to install said package (this will also happen with package zips as well, anything with a matching name)

Todo

  • Needs unit tests written if to be merged in.
  • Give a user a warning to know that their package may not have properly been screened
  • Test dependencies of the non-registry package (maybe?)

References

N/A

@jcristovao
Copy link

Hey @isaacs , any chance this can get merged? I was having issues with the audit phase of npm install and this solved it

@wesleytodd
Copy link

I just ran into an error with npm@8.2.0 which I believe is resolved by this PR. The package causing it appears to be https://registry.npmjs.org/nf-iso-properties with a single unpublished version. cc @wraithgar @nlf

TypeError: Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Advisory.load (./8.2.0/node_modules/@npmcli/metavuln-calculator/lib/advisory.js:102:33)
    at Calculator.[calculate] (./8.2.0/node_modules/@npmcli/metavuln-calculator/lib/index.js:59:14)
    at async Promise.all (index 9)
    at async Map.[init] (./8.2.0/node_modules/@npmcli/arborist/lib/audit-report.js:192:7)
    at async Map.run (./8.2.0/node_modules/@npmcli/arborist/lib/audit-report.js:110:7)
    at async Arborist.audit (./8.2.0/node_modules/@npmcli/arborist/lib/arborist/audit.js:37:24)
    at async Audit.exec (./8.2.0/lib/commands/audit.js:49:5)
    at async module.exports (./8.2.0/lib/cli.js:66:5)

@wraithgar
Copy link
Member

We are going to fix this in arborist. It should never be sending that package to this module.

@wraithgar wraithgar closed this Dec 8, 2021
@MarieKirya
Copy link
Author

MarieKirya commented Dec 20, 2021

@wraithgar Is there a way to track this issue further so we know when to remove our patch in our CI/CD? Thanks!

@grassick
Copy link

This is blocking upgrading npm entirely for us, I'm afraid.

@chrisleerate
Copy link

chrisleerate commented Jan 12, 2022

This bug is currently affecting our package that's published to our private registry. We have a package that's named the same as some random person's unpublished package on NPM. At this point we have a few not-so-great workaround options:

  • Resort to needing to rename our package that's published on our private registry to avoid conflicting with an unpublished NPM package
  • run npm i --no-audit every time we npm install

Not sure why the audit is running against NPM's package in the first place.

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

Successfully merging this pull request may close these issues.

None yet

6 participants