Skip to content

node:crypto: name rejected input key encoding options key.<option> like Node - #38415

Closed
robobun wants to merge 1 commit into
mainfrom
farm/4680be35/crypto-key-encoding-error-prefix
Closed

robobun wants to merge 1 commit into
mainfrom
farm/4680be35/crypto-key-encoding-error-prefix

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • createPrivateKey({ key, format: "bogus" }) throws ERR_INVALID_ARG_VALUE: The property 'options.format' is invalid. Received 'bogus'. Node v26.3.0 (the version Bun reports) says 'key.format'. Same for key.type and key.passphrase, and for every API that takes a { key, ... } object: createPublicKey, crypto.sign/verify, Verify#verify, publicEncrypt/publicDecrypt, Sign#sign, privateEncrypt/privateDecrypt.
  • KeyObject::prepareAsymmetricKey (src/jsc/bindings/node/crypto/KeyObject.cpp:2008, :2023, :2037) calls parseKeyEncoding with a null object name, and makeOptionString (src/jsc/bindings/node/crypto/CryptoUtil.cpp:800) renders a null name as options.<option>. Both mirror Node's option() helper as it was before Node v26.1.0; crypto: accept key data in crypto.diffieHellman() and cleanup DH jobs nodejs/node#62527 changed it to take a full prefix and made prepareAsymmetricKey pass key.
  • The adapted copy of test/js/node/test/parallel/test-crypto-key-objects.js pinned the old options.type wording (upstream changed that line to key.type in the same Node PR).

Fix

  • makeOptionString(prefix, option) now renders <prefix>.<option>, with a null prefix still meaning the bare options.<option> form, exactly like Node's current option().
  • prepareAsymmetricKey passes "key"; generateKeyPair passes "options.publicKeyEncoding" / "options.privateKeyEncoding" (previously the bare names, which makeOptionString prepended options. to); KeyObject#export keeps passing null. So the only messages that change are the input-key ones, which is also the only thing crypto: accept key data in crypto.diffieHellman() and cleanup DH jobs nodejs/node#62527 changed.
  • Correct because it matches Node v26.1+ for every consumer whose Node-side name is key (createPrivateKey, createPublicKey, sign, verify, Verify#verify, publicEncrypt, publicDecrypt); a 93-line matrix of these calls now produces byte-identical output between this build and Node v26.3.0. Sign#sign, privateEncrypt and privateDecrypt now say key.format where Node 26.1+ says privateKey.format; that root-name difference already applies to all of their other messages (key.key, key.asymmetricKeyType, ...) and is tracked separately, so this change only brings them in line with the rest of their own messages.
  • Verified with test/js/node/crypto/crypto.key-objects.test.ts (new encoding option names block: 7 consumers x string/Buffer/ArrayBuffer key material x format/type/missing type/passphrase, plus KeyObject#export and generateKeyPairSync cases pinning the unchanged options.* prefixes; 22 tests fail on the released binary, all pass with this build).
  • test/js/node/test/parallel/test-crypto-key-objects.js updated to the upstream wording; it and the 38 other vendored test-crypto-key*/keygen*/sign-verify/rsa-dsa/dh-stateless tests pass with this build.
  • test/js/node/crypto/ (node-crypto, crypto-rsa, crypto-oneshot, crypto-pqc, sign-jwk-ieee-p1363, crypto-sign-regression) and test/js/third_party/jsonwebtoken/validateAsymmetricKey.test.js pass with this build.

Background

  • prepareAsymmetricKey is the shared entry point that turns the key argument of createPublicKey/createPrivateKey/sign/verify/publicEncrypt/... into key material. When the argument is an object ({ key, format, type, passphrase, ... }), its encoding options are validated by parseKeyEncoding, the same function KeyObject#export and generateKeyPair use for output encodings.
  • parseKeyEncoding takes an object-name argument purely for error messages. Node derives the reported property path from it with option(name, prefix): options.<name> when no prefix is given (the encoding object is the options argument itself, as in export), otherwise <prefix>.<name>. makeOptionString is Bun's port of that helper.
  • ERR_INVALID_ARG_VALUE renders The property 'a.b' is invalid when the name contains a dot and The argument 'a' is invalid otherwise, so the prefix decides the whole shape of the message.

…ke Node

prepareAsymmetricKey passed a null object name to parseKeyEncoding, so a
bad format/type/passphrase inside a { key, format, ... } argument was
reported as options.format etc. Node v26.1.0 (nodejs/node#62527) reports
these as key.format, key.type and key.passphrase, and keeps options.*
for KeyObject#export and options.<public|private>KeyEncoding.* for
generateKeyPair.

makeOptionString now takes the full prefix like Node's option() helper:
prepareAsymmetricKey passes "key", generateKeyPair passes
"options.publicKeyEncoding" / "options.privateKeyEncoding", and export
keeps passing null for the bare options.* form.
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: fe233539-958c-419d-8562-bf07493dbe61

📥 Commits

Reviewing files that changed from the base of the PR and between 01aa7cd and 98d4e21.

📒 Files selected for processing (5)
  • src/jsc/bindings/node/crypto/CryptoGenKeyPair.cpp
  • src/jsc/bindings/node/crypto/CryptoUtil.cpp
  • src/jsc/bindings/node/crypto/KeyObject.cpp
  • test/js/node/crypto/crypto.key-objects.test.ts
  • test/js/node/test/parallel/test-crypto-key-objects.js

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix pushed, self-review in progress.

Reproduced on the released binary (bun 1.4.0) and on main (6855f522e2) against Node v26.3.0:

crypto.createPrivateKey({ key, format: "bogus" })
  node: The property 'key.format' is invalid. Received 'bogus'
  bun:  The property 'options.format' is invalid. Received 'bogus'

Same for key.type / key.passphrase and for every API that accepts a { key, ... } object. With this branch, a 93-case matrix (createPrivateKey, createPublicKey, sign, verify, Verify#verify, publicEncrypt, publicDecrypt x string/Buffer/ArrayBuffer x four bad option sets, plus export and generateKeyPairSync) produces output identical to Node v26.3.0.

The new block in test/js/node/crypto/crypto.key-objects.test.ts fails on the released binary (22 tests) and passes with this build; the vendored test-crypto-key*, keygen*, sign-verify, rsa-dsa and dh-stateless tests pass with this build.

Out of scope, tracked separately: Sign#sign / privateEncrypt / privateDecrypt use the privateKey root name in Node 26.1+ (affects their ERR_INVALID_ARG_TYPE messages too), and the raw-format key.key message says "of type" where Node says "an instance of".

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — error-message-only Node compat fix with all call sites accounted for.

What was reviewed

  • makeOptionString semantics change: verified the only three call-site families (prepareAsymmetricKey, generateKeyPair's parseKeyEncodingConfig, KeyObject#export) and confirmed each now passes the right prefix so export and generateKeyPair messages are unchanged while input-key messages become key.<option>.
  • New test block pins all three prefixes (7 consumers × 3 key-material shapes × 4 options, plus export and generateKeyPairSync); vendored test-crypto-key-objects.js updated to the upstream key.type wording.
  • No success-path behavior touched; the passphrase check for createPublicKey still gates on isPublic != true, so the key.passphrase case in the matrix is reachable for both public and private consumers.
Extended reasoning...

Overview

This PR changes the property-path prefix in ERR_INVALID_ARG_VALUE messages produced by node:crypto's key-encoding validation to match Node v26.1+ (nodejs/node#62527). The native change is five lines: makeOptionString now treats its first argument as a full prefix (<prefix>.<option>) instead of a sub-name under options., the three parseKeyEncoding calls in prepareAsymmetricKey pass "key" instead of null, and the two generateKeyPair callers pass the already-prefixed "options.publicKeyEncoding" / "options.privateKeyEncoding" to preserve their existing output. KeyObject#export keeps passing null, which still renders as options.<option>.

Security risks

None. This is a change to the text of validation error messages; no parsing, allocation, key-material handling, or control flow on the success path is affected.

Level of scrutiny

Low. The only functional risk with a helper-semantics change like this is missing a caller. I grepped for every use of makeOptionString, parseKeyEncoding, parsePublicKeyEncoding, and parsePrivateKeyEncoding — there are exactly three call-site families, all touched or verified unchanged in this PR, and the new tests explicitly pin the output of all three (input-key → key.*, export → options.*, keygen → options.<pub|priv>KeyEncoding.*). The vendored Node parallel test is updated to the upstream wording rather than weakened.

Other factors

Test coverage is thorough: a describe.each × test.each matrix over 7 consumers and 3 key-material shapes exercises the string / JSArrayBufferView / JSArrayBuffer branches of prepareAsymmetricKey for format, type, missing type, and passphrase; separate tests pin export and generateKeyPairSync messages so a future refactor of makeOptionString can't silently regress the unchanged paths. The PR description documents the known residual divergence (Sign#sign/privateEncrypt/privateDecrypt say key.* where Node says privateKey.*) as a pre-existing root-name difference tracked separately, which is consistent with Bun's other messages for those APIs. No CODEOWNERS cover these files.

@robobun

robobun commented Aug 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:05 AM PT - Aug 14th, 2026

❌ @robobun, your commit 98d4e21 has some failures in Build #95659 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38415

That installs a local version of the PR into your bun-38415 executable, so you can run:

bun-38415 --bun

@robobun

robobun commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator Author

Closing: #45335 contains this change. It passes the name of the caller to makeOptionString(prefix, option), so the messages read key.format, privateKey.format and options.privateKey.format, as in Node.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant