Skip to content

Conversation

@ChALkeR
Copy link
Member

@ChALkeR ChALkeR commented Oct 24, 2025

Includes #60384
Closes: #60267

This is a draft.
Feedback is welcome though

  1. Not sure if I caught all the places, conversion is scattered across different codepaths
  2. All validation is in JS land. Perhaps some native methods should have assertions
  3. This will throw synchronously - other input validation does the same though

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 24, 2025
@panva panva added crypto Issues and PRs related to the crypto subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 24, 2025
validateStringWellFormed(val);
if (encoding === 'buffer')
encoding = 'utf8';
return Buffer.from(val, encoding);
Copy link
Member Author

@ChALkeR ChALkeR Oct 24, 2025

Choose a reason for hiding this comment

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

This could also cause collisions on well-formed strings but encodings not capable of containing full Unicode
That should be addressed too

validateStringWellFormed(buffer);
if (encoding === 'buffer')
encoding = 'utf8';
return Buffer.from(buffer, encoding);
Copy link
Member Author

Choose a reason for hiding this comment

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

This could also cause collisions on well-formed strings but encodings not capable of containing full Unicode
That should be addressed too

Sign.prototype.update = function update(data, encoding) {
if (typeof data === 'string') {
validateEncoding(data, encoding);
validateStringWellFormed(data);
Copy link
Member Author

@ChALkeR ChALkeR Oct 24, 2025

Choose a reason for hiding this comment

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

see other comments about encoding, this might also need adjustments and/or a check on src/ side


if (typeof data === 'string') {
validateEncoding(data, encoding);
validateStringWellFormed(data);
Copy link
Member Author

Choose a reason for hiding this comment

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

ditto about encoding

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

crypto Issues and PRs related to the crypto subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Equal hashes on non-equal JS strings are dangerous to the ecosystem

3 participants