-
Notifications
You must be signed in to change notification settings - Fork 45
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
fix: error when passing signature without keys #176
Conversation
@wraithgar thoughts on these changes? I made them to line up with the test suite in the cli pr: npm/cli#4827 |
d59b3b5
to
6f75b51
Compare
The way it works now was intentional, and was an attempt at letting us move forward more quickly without friction. With this PR's behavior, if this is a cli flag that someone turned on then every package from every other registry would start erroring immediately. |
I see now that you're addressing a slightly different case than I was originally thinking. If there are signatures in the packument, but we don't have keys that is not the use case considering. I was only considering the absence of public keys at all. I think, in a very narrow sense, that "If we were asked to verify signatures, and there are signatures, but we don't have a public key" should be an error state. Just wanna put it out there though that "If we were asked to verify signatures, and there is no public key" is not, on its own, an error state. |
👍
Agreed! |
Raise an error when the package has a signature, `verifySignatures` is true and there are no registry keys configured. Updated the error message and fixed the undefined name@version in the error message to match the test cases here: npm/cli#4827 Possible anti-pattern: I've attached a bunch of error attributes if the signature is invalid so we can present this in the json output to ease debugging.
8696581
to
23017ba
Compare
@wraithgar I think this PR is now good to go! |
Raise an error when the package has a signature,
verifySignatures
is true and there are no registry keys configured to match an error case we had set up in the CLI.Updated the error message and fixed the undefined
name@version
in the error message to match the test cases here:npm/cli#4827
Possible anti-pattern: I've attached a bunch of error attributes if the signature is invalid so we can present this in the json output to ease debugging.
Related: #170