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

crypto: unify validation of checkPrime checks #47165

Conversation

tniessen
Copy link
Member

Previously, the JS layer would validate that the value of the checks option was an unsigned 32-bit integer, otherwise throwing an appropriate error but with a slightly misleading error message. Then the C++ layer would validate that the value was an unsigned 31-bit integer, otherwise throwing an appropriate error, but with a different (and even less helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no validation in C++ is necessary and so that the error message always matches the exact requirement.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Mar 19, 2023
@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 19, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

test/parallel/test-crypto-prime.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment was marked as outdated.

Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.
@tniessen tniessen force-pushed the crypto-unify-checkprime-checks-range-check branch from c96994b to e2a709b Compare March 20, 2023 13:10
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Mar 20, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 20, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit 7d84d67 into nodejs:main Mar 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 7d84d67

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.

PR-URL: #47165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.

PR-URL: #47165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Previously, the JS layer would validate that the value of the 'checks'
option was an unsigned 32-bit integer, otherwise throwing an appropriate
error but with a slightly misleading error message. Then the C++ layer
would validate that the value was an unsigned 31-bit integer, otherwise
throwing an appropriate error, but with a different (and even less
helpful) error message.

Instead, make the JS layer aware of the 31-bit restriction so that no
validation in C++ is necessary and so that the error message always
matches the exact requirement.

PR-URL: #47165
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants