Skip to content

Conversation

hotpineapple
Copy link
Contributor

@hotpineapple hotpineapple commented Oct 12, 2025

this PR resolves todo comment about RFC7517 section 4.3 recommendation
added print warning with unrelated key combination

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Oct 12, 2025
print warning with unrelated key combination
Copy link

codecov bot commented Oct 12, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.54%. Comparing base (b13f24c) to head (48b5949).
⚠️ Report is 36 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/crypto/util.js 66.66% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #60221    +/-   ##
========================================
  Coverage   88.53%   88.54%            
========================================
  Files         703      704     +1     
  Lines      207997   208139   +142     
  Branches    40015    40007     -8     
========================================
+ Hits       184150   184287   +137     
- Misses      15864    15865     +1     
- Partials     7983     7987     +4     
Files with missing lines Coverage Δ
lib/internal/crypto/util.js 94.79% <66.66%> (-0.64%) ⬇️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@panva
Copy link
Member

panva commented Oct 12, 2025

Is there a test case scenario that fails in any of the browser implementations but works in Node.js related to JWK key_ops?

@hotpineapple
Copy link
Contributor Author

Is there a test case scenario that fails in any of the browser implementations but works in Node.js related to JWK key_ops?

I'll add test code. Thanks for your review!

@tniessen tniessen added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 13, 2025
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

The reference spec says about the permitted values of key_ops:

Other values MAY be used.
(...)
Multiple unrelated key operations SHOULD NOT be specified for a key because of the potential vulnerabilities associated with using the same key with multiple algorithms. Thus, the combinations "sign" with "verify", "encrypt" with "decrypt", and "wrapKey" with "unwrapKey" are permitted, but other combinations SHOULD NOT be used.

SHOULD NOT in this case means:

This phrase, or the phrase "NOT RECOMMENDED" mean that there may exist valid reasons in particular circumstances when the particular behavior is acceptable or even useful, but the full implications should be understood and the case carefully weighed before implementing any behavior described with this label.

This makes me wonder if there is a chance that someone may want to allow JWK objects with a set of key_ops that this PR would consider invalid, in which case we might not want to throw unconditionally.

@panva
Copy link
Member

panva commented Oct 13, 2025

@tniessen I've asked for a test case that can be checked for in other implementations exactly because I'm uncertain about a rejection being appropriate here. Especially since during import the user specifies the expected keyUsages, and if the user just echoes JWK key_ops as the keyUsages the WebCryptoAPI steps already make it so that only valid usages/ops can be used for a given algorithm.

So the question is, really, if the following WebCryptoAPI jwk import clauses

If the key_ops field of jwk is present, and is invalid according to the requirements of JSON Web Key [JWK] or does not contain all of the specified usages values, then throw a DataError.

Include rejecting such combinations of key_ops in other implementations.

In case it isn't rejected or is inconsistent across implementations I believe the only TODO would be to remove the TODO comment.

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. 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.

4 participants