Skip to content

node:assert: compare KeyObject and CryptoKey material in deep equality - #41500

Closed
robobun wants to merge 5 commits into
mainfrom
robobun/b7864dd1/deep-equal-key-material
Closed

robobun wants to merge 5 commits into
mainfrom
robobun/b7864dd1/deep-equal-key-material

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • util.isDeepStrictEqual, assert.deepStrictEqual, assert.deepEqual and Bun.deepEquals report any two KeyObjects or CryptoKeys as equal. assert.deepStrictEqual(createSecretKey(a), createSecretKey(b)) passes for any a and b. Node throws ERR_ASSERTION. A test that asserts two keys are equal can never fail.
  • The cause is the own-property walk in Bun__deepEquals (src/jsc/bindings/bindings.cpp). Both classes keep their state in C++ and have no own enumerable properties, so the walk compares nothing.

Fix

  • A new arm in the ObjectType tail of specialObjectsDequal, next to the Temporal arm. It follows node's comparisons.js: a KeyObject compares by type and key material. A CryptoKey compares by type, extractable, algorithm (deep, with the caller's mode), usages and key material. Both then compare own enumerable properties as before.
  • The key material compare throws ERR_CRYPTO_UNSUPPORTED_OPERATION when OpenSSL cannot compare the keys, the same as KeyObject.prototype.equals and node's handle equals.
  • The native assert.partialDeepStrictEqual port (src/jsc/modules/NodeUtilTypesModule.cpp) shares the helpers. It already handled KeyObject through a JS call to equals. It now also handles CryptoKey.
  • Verified: test/js/node/assert/deep-equal.test.ts (22 matrix cases, 20 fail on 1.4.2) and test/js/bun/bun-object/deep-equals.test.ts (fails on 1.4.2). Also test/js/node/assert/, node's test-assert*.js, crypto.key-objects.test.ts, test/js/bun/test/expect*.

Background

  • specialObjectsDequal is the per-type dispatch inside the deep equality walk. It returns false to reject a pair, true to accept it, and std::nullopt to continue with the generic own-property compare. All four entry points (node strict, node loose, Bun.deepEquals, expect().toEqual) share this template, so expect(keyA).toEqual(keyB) now compares key material too. The new bun-object test pins that.
  • JSKeyObject and JSCryptoKey both use the ObjectType cell type, so the arm lives in the c1Type == ObjectType tail, not in the switch.
  • KeyObject::create(CryptoKey&) already converts WebCrypto key material to a node KeyObject handle. The CryptoKey arm uses it for the material compare.
  • webcrypto: RSA-PSS saltLength range errors, key-material deep equality, supports() parity (+5 tests) #35374 (draft, stacked) carries an older version of this arm in its bindings.cpp hunk 2. This PR supersedes that hunk with two deliberate differences: it throws on an unsupported compare instead of falling through, and it deep-compares the algorithm dictionary instead of the identifier only.
Notes

Node v26.3.0 reference: lib/internal/util/comparisons.js, the isKeyObject and isCryptoKey arms of objectComparisonStart. Both run in every mode (strict, loose, partial) and fall through to keyCheck afterwards.

Repro on 1.4.2 (node v26.3.0 in the comment):

import assert from "node:assert";
import util from "node:util";
import { createSecretKey, generateKeyPairSync } from "node:crypto";
const a = createSecretKey(Buffer.from("secret-A")), b = createSecretKey(Buffer.from("secret-B"));
util.isDeepStrictEqual(a, b);         // true   (node: false)
assert.deepStrictEqual(a, b);         // passes (node: ERR_ASSERTION)
assert.deepEqual({ k: a }, { k: b }); // passes (node: ERR_ASSERTION)
const p1 = generateKeyPairSync("ec", { namedCurve: "P-256" }).publicKey;
const p2 = generateKeyPairSync("ec", { namedCurve: "P-256" }).publicKey;
assert.deepStrictEqual(p1, p2);       // passes (node: ERR_ASSERTION)
const k1 = await crypto.subtle.importKey("raw", Buffer.from("k1k1k1k1k1k1k1k1"), { name: "HMAC", hash: "SHA-256" }, true, ["sign"]);
const k2 = await crypto.subtle.importKey("raw", Buffer.from("k2k2k2k2k2k2k2k2"), { name: "HMAC", hash: "SHA-256" }, true, ["sign"]);
assert.deepStrictEqual(k1, k2);       // passes (node: ERR_ASSERTION)
assert.partialDeepStrictEqual(k1, k2); // passes (node: ERR_ASSERTION)

algorithm is read through the JS getter. JSCryptoKey caches the dictionary object in m_algorithm, so the read is cheap and the compared objects are plain objects with Object.prototype.

The ObjectType tail of specialObjectsDequalSlow is not reachable for ObjectType cells (the dispatch only sends the listed cell types there), so the arm is added to the templated function only.

Suites run with the debug build: test/js/node/assert/ (7 files), test/js/node/test/parallel/test-assert{,-checktag,-class,-deep-with-error,-typedarray-deepequal}.js, test/js/node/crypto/crypto.key-objects.test.ts, test/js/node/util/test-util-types.test.js, test/js/bun/test/expect.test.ts, test/js/bun/test/expect/, test/js/bun/bun-object/.

Self-reviewed: 3 concerns raised, 3 addressed (reference #35374 and state the semantic differences, add a Bun.deepEquals / toEqual test, fold the duplicated CryptoKey arm into one helper).


no test proof · iteration 1 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/bun-object/deep-equals.test.ts

util.isDeepStrictEqual, assert.deepStrictEqual, assert.deepEqual and
Bun.deepEquals walked the own enumerable properties of a KeyObject or a
CryptoKey. Both keep their state in C++ and have no own properties, so
any two keys compared equal.

Follow node's comparisons.js. A KeyObject compares by type and key
material. A CryptoKey compares by type, extractable, algorithm, usages
and key material. Both then compare own enumerable properties as
before. assert.partialDeepStrictEqual now covers CryptoKey through the
same helpers.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.

Or wait 5 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: cddf43de-48fe-46d3-8b81-f55f6aa996d6

📥 Commits

Reviewing files that changed from the base of the PR and between 4593030 and 74187b6.

📒 Files selected for processing (6)
  • src/jsc/bindings/bindings.cpp
  • src/jsc/bindings/node/crypto/KeyObject.cpp
  • src/jsc/bindings/node/crypto/KeyObject.h
  • src/jsc/modules/NodeUtilTypesModule.cpp
  • test/js/bun/bun-object/deep-equals.test.ts
  • test/js/node/assert/deep-equal.test.ts

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

@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:23 AM PT - Sep 6th, 2026

❌ @robobun, your commit 74187b6 has 3 failures in Build #110854 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41500

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

bun-41500 --bun

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

On the usages comparison: node v26.3.0 compares the usages mask for exact equality in every mode, including partial. lib/internal/util/comparisons.js:441 reads getCryptoKeyUsagesMask(val1) !== getCryptoKeyUsagesMask(val2). Only algorithm goes through innerDeepEqual with the mode (line 440). The bitmap check in cryptoKeysDeepEqual matches that.

Checked against node v26.3.0:

const k = createSecretKey(Buffer.from("secret-A"));
const a = k.toCryptoKey({ name: "HMAC", hash: "SHA-256" }, true, ["sign", "verify"]);
const b = k.toCryptoKey({ name: "HMAC", hash: "SHA-256" }, true, ["sign"]);
assert.partialDeepStrictEqual(a, b); // node: ERR_ASSERTION

No change needed.

Comment thread src/jsc/bindings/bindings.cpp Resolved
Comment thread src/jsc/bindings/bindings.cpp Resolved
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: closed. #47567 (89fbb78) landed this comparison on main. The closing comment below has the check against main.

@Jarred-Sumner Jarred-Sumner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there some symbol property for deep equality we can use instead?

@robobun

robobun commented Sep 6, 2026

Copy link
Copy Markdown
Collaborator Author

There is no such symbol today. Node's comparisons.js uses isKeyObject / isCryptoKey type checks and internal slot getters, not a protocol. Jest has no equality symbol either (only asymmetricMatch for matchers). In Bun the closest precedent is Bun.inspect.custom for util.inspect.

I can add one. Two shapes:

  1. Internal: a private name (not visible to JS) that KeyObject.prototype and CryptoKey.prototype hold as a native method. The ObjectType tail of the deep equality walk looks it up once and calls it. Nothing changes for users. The C++ type checks in this PR move behind the lookup, and Temporal, URL and Headers could move to the same hook later.
  2. Public: Bun.deepEquals.custom (a registered symbol, like Bun.inspect.custom). Same mechanism, but user classes can implement it too. That is a new API and needs a decision on semantics (which modes call it, what the return value means, whether expect().toEqual honors it).

One detail for either shape: the CryptoKey hook needs to recurse into the algorithm dictionary with the caller's mode (strict, loose or partial). So the hook would return the values to compare next, or receive a compare callback, instead of returning a plain boolean.

Which shape do you want? I will rework the PR to match.

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

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Nov 1, 2026

Copy link
Copy Markdown
Collaborator Author

This PR is covered by #47567 (89fbb78), which is on main. It landed the comparison of KeyObject and CryptoKey in deep equality for assert.deepStrictEqual, assert.deepEqual, util.isDeepStrictEqual, assert.partialDeepStrictEqual, Bun.deepEquals and expect().toEqual / toStrictEqual.

I checked that on main (3f3a9ff). I added the test blocks of this PR to main's versions of the two test files and ran them on a debug build, with no source change from this PR.

  • test/js/node/assert/deep-equal.test.ts: 524 pass, 0 fail. The 15 matrix rows of this PR pass through all three entry points. Its assert.partialDeepStrictEqual test passes too.
  • test/js/bun/bun-object/deep-equals.test.ts: 68 pass, 2 skip, 0 fail. The test of this PR passes in both modes.
  • Control: on 1.4.2, 28 of those 46 test runs fail, and so does the Bun.deepEquals test.

Main answers no row of this PR differently. The source differs in two points that no row observes:

  • When EVP_PKEY_cmp cannot compare two keys, main reports them as not equal. This PR threw ERR_CRYPTO_UNSUPPORTED_OPERATION. I found no key pair that reaches that path.
  • Main compares the algorithm name and the hash in C++. This PR deep-compared the algorithm dictionary.

#47567 uses type checks in the same place as this PR. The symbol-based hook discussed above is not implemented anywhere.

@robobun robobun closed this Nov 1, 2026
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.

2 participants