assert,crypto: make KeyObject and CryptoKey testable for equality by panva · Pull Request #50897 · nodejs/node · GitHub

This makes CryptoKey and KeyObject instances testable using assert.deepStrictEqual and assert.deepEqual.

The state before was that

deepEqual did not check extractable, algorithm, usages
neither deepEqual or deepStrict equal compared the key material, so two different keys with the other properties matching evaluated as equal

cc @nodejs/crypto

CI: https://ci.nodejs.org/job/node-test-pull-request/55920/

I think the use case makes sense, and I'm not able to able to suggest a better implementation, so let's roll with this.

nit: I find Buffer.from easier to understand (I had to go read the docs to get what the second argument of Buffer.alloc was for)

Suggested change

      
                const a = crypto.createSecretKey(Buffer.alloc(1, 0));
          
                const b = crypto.createSecretKey(Buffer.alloc(1, 1));
          
                const a = crypto.createSecretKey(Buffer.from([0]));
          
                const b = crypto.createSecretKey(Buffer.from([1]));

Commit Queue failed

- Loading data for nodejs/node/pull/50897
✔  Done loading data for nodejs/node/pull/50897
----------------------------------- PR info ------------------------------------
Title      assert,crypto: make KeyObject and CryptoKey testable for equality (#50897)
Author     Filip Skokan  (@panva)
Branch     panva:crypto-objects-equal -> nodejs:main
Labels     crypto, assert, util, needs-ci, review wanted, webcrypto, commit-queue-rebase
Commits    2
 - crypto: update CryptoKey symbol properties
 - assert,crypto: make KeyObject and CryptoKey testable for equality
Committers 1
 - Filip Skokan 
PR-URL: https://github.com/nodejs/node/pull/50897
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/50897
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 24 Nov 2023 16:50:43 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/50897#pullrequestreview-1785408839
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-11-25T19:19:00Z: https://ci.nodejs.org/job/node-test-pull-request/55920/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - crypto: update CryptoKey symbol properties
   ⚠  - assert,crypto: make KeyObject and CryptoKey testable for equality
- Querying data for job/node-test-pull-request/55920/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/7237974363

CI: https://ci.nodejs.org/job/node-test-pull-request/56347/

CI: https://ci.nodejs.org/job/node-test-pull-request/56349/

Landed in 154afbe...0afe731

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

Not sure if this is the right place to comment but I noticed a regression from v18 to v20, and this PR seems vaguely related.

const a = ['a'];
assert.deepStrictEqual(new Set([a, ['b']]), new Set([a, ['b']]));

v18.20.2: pass — no error as expected
v20.0.0: fail — unexpected error:

AssertionError [ERR_ASSERTION]: Values have same structure but are not reference-equal:
Set(2) {
  [
    'a'
  ],
  [
    'b'
  ]
}

The bug seems to occur only when comparing Set objects, with a mix of elements where some elements are compared by reference (===) and other elements are compared by deep equality.

@chharvey can you open a new issue to track this please? (FWIW I'm able to reproduce)

@aduh95 thanks for your reply. I think I found the culprit and left a comment: #46593 (comment)

@chharvey can you open a new issue to track this please?

@aduh95 @panva I opened this issue: #53423

panva added crypto Issues and PRs related to the crypto subsystem. assert Issues and PRs related to the assert subsystem. webcrypto labels Nov 24, 2023

nodejs-github-bot added needs-ci PRs that need a full CI run. util Issues and PRs related to the built-in util module. labels Nov 24, 2023

aduh95 reviewed Nov 24, 2023

View reviewed changes

Comment thread test/parallel/test-assert-deep.js Outdated

aduh95 approved these changes Nov 24, 2023

View reviewed changes

aduh95 reviewed Nov 24, 2023

View reviewed changes

Comment thread test/parallel/test-assert-deep.js Outdated

panva added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 24, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2023

This comment was marked as outdated.

Sign in to view

panva added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Nov 25, 2023

panva force-pushed the crypto-objects-equal branch from 0965ede to 5327861 Compare November 25, 2023 13:20

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023

panva force-pushed the crypto-objects-equal branch from 5327861 to ca444db Compare November 25, 2023 13:27

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023

This comment was marked as outdated.

Sign in to view

panva removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 25, 2023

panva force-pushed the crypto-objects-equal branch from ca444db to b008047 Compare November 25, 2023 14:33

panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023

This comment was marked as outdated.

Sign in to view

panva added the review wanted PRs that need reviews. label Nov 30, 2023

panva requested a review from aduh95 December 6, 2023 09:55

aduh95 approved these changes Dec 17, 2023

View reviewed changes

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 17, 2023

panva added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 17, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2023

panva added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 17, 2023

nodejs-github-bot closed this Dec 17, 2023

nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2023

crypto: update CryptoKey symbol properties

f505a22

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2023

assert,crypto: make KeyObject and CryptoKey testable for equality

0afe731

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

panva deleted the crypto-objects-equal branch December 17, 2023 13:16

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024

crypto: update CryptoKey symbol properties

e5017a5

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024

assert,crypto: make KeyObject and CryptoKey testable for equality

13a1241

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

RafaelGSS mentioned this pull request Jan 2, 2024

v21.6.0 proposal #51342

Merged

richardlau pushed a commit that referenced this pull request Mar 25, 2024

crypto: update CryptoKey symbol properties

adb5d69

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

richardlau pushed a commit that referenced this pull request Mar 25, 2024

assert,crypto: make KeyObject and CryptoKey testable for equality

cbda4e9

PR-URL: #50897 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

richardlau mentioned this pull request Mar 25, 2024

v20.12.0 proposal #52212

Merged

panva mentioned this pull request Jun 11, 2024

assert: add matchObjectStrict and matchObject #53415

Closed

coderabbitai Bot mentioned this pull request Jun 26, 2026

INS-253: Cover machine-auth JWT and OIDC mutation paths zaks-io/insecur#163

Merged

		const a = crypto.createSecretKey(Buffer.alloc(1, 0));
		const b = crypto.createSecretKey(Buffer.alloc(1, 1));

assert,crypto: make KeyObject and CryptoKey testable for equality by panva · Pull Request #50897 · nodejs/node · GitHub

Uh oh!

Uh oh!

assert,crypto: make KeyObject and CryptoKey testable for equality - #50897

assert,crypto: make KeyObject and CryptoKey testable for equality#50897panva wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom panva:crypto-objects-equalpanva/node:crypto-objects-equalCopy head branch name to clipboard

Conversation

panva commented Nov 24, 2023

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

panva commented Nov 25, 2023

Uh oh!

nodejs-github-bot commented Nov 25, 2023

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 Dec 17, 2023

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Dec 17, 2023

Uh oh!

nodejs-github-bot commented Dec 17, 2023

Uh oh!

nodejs-github-bot commented Dec 17, 2023

Uh oh!

nodejs-github-bot commented Dec 17, 2023

Uh oh!

chharvey commented Jun 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Jun 9, 2024

Uh oh!

chharvey commented Jun 10, 2024

Uh oh!

panva commented Jun 11, 2024

Uh oh!

chharvey commented Jun 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

assert,crypto: make KeyObject and CryptoKey testable for equality#50897
panva wants to merge 2 commits into
nodejs:mainfrom
panva:crypto-objects-equal

chharvey commented Jun 9, 2024 •
edited
Loading