Skip to content

node:crypto: move Sign and Verify to c++#17692

Merged
Jarred-Sumner merged 51 commits intomainfrom
dylan/more-crypto
Mar 1, 2025
Merged

node:crypto: move Sign and Verify to c++#17692
Jarred-Sumner merged 51 commits intomainfrom
dylan/more-crypto

Conversation

@dylan-conway
Copy link
Copy Markdown
Member

What does this PR do?

Uses ncrypto for implementing Sign

How did you verify your code works?

@robobun
Copy link
Copy Markdown
Collaborator

robobun commented Feb 26, 2025

Updated 1:56 AM PT - Mar 1st, 2025

@dylan-conway, your commit c863bf8 has some failures in Build #12558


🧪   try this PR locally:

bunx bun-pr 17692

@dylan-conway dylan-conway requested a review from Copilot February 26, 2025 20:20
@dylan-conway dylan-conway marked this pull request as ready for review February 26, 2025 20:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR moves the Sign implementation to C++ by integrating ncrypto, refactors related crypto API functions, and updates error codes.

  • Updates test behavior for missing crypto support and OpenSSL version detection
  • Refactors crypto.ts to create a new Sign class and update key processing functions
  • Adds new error codes in ErrorCode.ts to support the new crypto functionality

Reviewed Changes

File Description
test/js/node/test/common/crypto.js Wraps the crypto test skip logic in braces and adds OpenSSL version helpers
src/js/node/crypto.ts Refactors crypto functions, adds a new Sign class, and updates key handling
src/bun.js/bindings/ErrorCode.ts Introduces new error mappings for crypto sign and key compatibility

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/js/node/crypto.ts:231

  • [nitpick] The optional parameter 'encoding' lacks an explicit type annotation, which may reduce type safety in a TypeScript context. Consider defining its type explicitly (e.g. encoding?: string).
function getArrayBufferOrView(buffer, name, encoding?) {

Comment on lines +132 to +133
const { m, n, p } = process.versions.openssl.match(regexp).groups;
OPENSSL_VERSION_NUMBER = opensslVersionNumber(m, n, p);
Copy link

Copilot AI Feb 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex match on process.versions.openssl does not check for a null result, which could lead to a runtime error if the version string format changes. Consider adding a null check before destructuring the groups.

Suggested change
const { m, n, p } = process.versions.openssl.match(regexp).groups;
OPENSSL_VERSION_NUMBER = opensslVersionNumber(m, n, p);
const match = process.versions.openssl.match(regexp);
if (match) {
const { m, n, p } = match.groups;
OPENSSL_VERSION_NUMBER = opensslVersionNumber(m, n, p);
} else {
throw new Error('Invalid OpenSSL version format');
}

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I need to make this change in other places in this file?

@dylan-conway dylan-conway changed the title node:crypto: move Sign to c++ node:crypto: move Sign and Verify to c++ Feb 28, 2025
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.

4 participants