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

fix: error when passing signature without keys #176

Merged
merged 1 commit into from Jun 1, 2022

Conversation

feelepxyz
Copy link
Member

@feelepxyz feelepxyz commented May 23, 2022

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

@feelepxyz feelepxyz requested a review from a team as a code owner May 23, 2022 14:09
@feelepxyz feelepxyz requested a review from wraithgar May 23, 2022 14:09
@feelepxyz
Copy link
Member Author

@wraithgar thoughts on these changes? I made them to line up with the test suite in the cli pr: npm/cli#4827

@feelepxyz feelepxyz force-pushed the feelepxyz/fix-missing-name-keys branch from d59b3b5 to 6f75b51 Compare May 23, 2022 14:30
@feelepxyz feelepxyz changed the title Error when passing signature without keys fix: error when passing signature without keys May 23, 2022
lib/registry.js Outdated Show resolved Hide resolved
@wraithgar
Copy link
Member

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.

@wraithgar
Copy link
Member

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.

@feelepxyz
Copy link
Member Author

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.
@feelepxyz feelepxyz force-pushed the feelepxyz/fix-missing-name-keys branch from 8696581 to 23017ba Compare June 1, 2022 12:34
@feelepxyz
Copy link
Member Author

@wraithgar I think this PR is now good to go!

@wraithgar wraithgar merged commit d69e524 into main Jun 1, 2022
@wraithgar wraithgar deleted the feelepxyz/fix-missing-name-keys branch June 1, 2022 13:45
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

3 participants