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: fix non-multiple of 8 in SubtleCrypto.deriveBits #55296

Closed
wants to merge 1 commit into from

Conversation

panva
Copy link
Member

@panva panva commented Oct 6, 2024

A WPT update made me look into this.

From the Node.js docs:

The Node.js implementation requires that length, when a number, is a multiple
of 8.

This was never true, instead the implementation returned the closest full byte length.

At the moment the browser implementations do the following

  • Chromium aligns with this PR (and the updated WPTs)
  • Firefox throws DataError
  • Safari aligns with Node.js prior to this PR

There's no interop on this in the first place and there's a pending decision around disallowing truncation in ECDH/X25519/X448 altogether in a future spec update.

Given that this is in my opinion a semver-major PRs that contain breaking changes and should be released in the next major version. change I would rather we only have to do one, i.e. disallow truncation when the spec changes in a major, or fix the implementation with this PR in a major. We've got time to figure out what to do in time for v24.x but i'm opening this to ping @nodejs/crypto and @nodejs/web-standards

@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. webcrypto web-standards Issues and PRs related to Web APIs labels Oct 6, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 6, 2024
@panva
Copy link
Member Author

panva commented Oct 6, 2024

@jasnell do you remember what lead to the current implementation which follows neither the docs nor the spec?

@jasnell
Copy link
Member

jasnell commented Nov 5, 2024

@panva ... sorry! I just spotted this one!

do you remember what lead to the current implementation which follows neither the docs nor the spec?

I think it's just a bug.

@panva panva removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 15, 2024
@panva panva marked this pull request as ready for review November 15, 2024 17:40
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 15, 2024
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Nov 15, 2024
Copy link
Contributor

Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/11860937753

@panva panva removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Nov 15, 2024
@nodejs-github-bot
Copy link
Collaborator

@panva
Copy link
Member Author

panva commented Nov 15, 2024

Firefox has updated their implementation, i think it's alright to follow suit with this. Question is whether major or not?

Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.42%. Comparing base (b02cd41) to head (af40b70).
Report is 207 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55296      +/-   ##
==========================================
- Coverage   88.42%   88.42%   -0.01%     
==========================================
  Files         654      654              
  Lines      187852   187862      +10     
  Branches    36134    36136       +2     
==========================================
+ Hits       166102   166109       +7     
- Misses      14989    15001      +12     
+ Partials     6761     6752       -9     
Files with missing lines Coverage Δ
lib/internal/crypto/diffiehellman.js 97.59% <100.00%> (+0.06%) ⬆️

... and 35 files with indirect coverage changes

@panva panva requested a review from Copilot November 19, 2024 18:36

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated no suggestions.

@jasnell
Copy link
Member

jasnell commented Nov 23, 2024

I still consider this a bug fix and not a major change.

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 23, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 23, 2024
@jasnell jasnell closed this Nov 23, 2024
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Nov 26, 2024
aduh95 pushed a commit that referenced this pull request Nov 26, 2024
@panva panva added lts-watch-v20.x PRs that may need to be released in v20.x lts-watch-v22.x PRs that may need to be released in v22.x and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Nov 27, 2024
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
aduh95 pushed a commit that referenced this pull request Dec 13, 2024
@aduh95 aduh95 added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed lts-watch-v22.x PRs that may need to be released in v22.x labels Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported-to-v22.x PRs backported to the v22.x-staging branch. crypto Issues and PRs related to the crypto subsystem. lts-watch-v20.x PRs that may need to be released in v20.x needs-ci PRs that need a full CI run. web-standards Issues and PRs related to Web APIs webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants