Skip to content

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

Closed
panva wants to merge 2 commits into
nodejs:mainfrom
panva:crypto-objects-equal
Closed

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

Conversation

@panva

@panva panva commented Nov 24, 2023

Copy link
Copy Markdown
Member

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

@panva panva added crypto Issues and PRs related to the crypto subsystem. assert Issues and PRs related to the assert subsystem. webcrypto Issues and PRs related to the Web Crypto API. labels Nov 24, 2023
@nodejs-github-bot 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
Comment thread test/parallel/test-assert-deep.js Outdated
Comment thread test/parallel/test-assert-deep.js Outdated
@panva panva added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 24, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 24, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva panva added the commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. label Nov 25, 2023
@panva
panva force-pushed the crypto-objects-equal branch from 0965ede to 5327861 Compare November 25, 2023 13:20
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@panva
panva force-pushed the crypto-objects-equal branch from 5327861 to ca444db Compare November 25, 2023 13:27
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

@panva panva removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 25, 2023
@panva
panva force-pushed the crypto-objects-equal branch from ca444db to b008047 Compare November 25, 2023 14:33
@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 25, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@panva

panva commented Nov 25, 2023

Copy link
Copy Markdown
Member Author

cc @nodejs/crypto

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the review wanted PRs that need review. label Nov 30, 2023
@panva
panva requested a review from aduh95 December 6, 2023 09:55

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

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.

Comment on lines +1230 to +1231
const a = crypto.createSecretKey(Buffer.alloc(1, 0));
const b = crypto.createSecretKey(Buffer.alloc(1, 1));

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.

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]));

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 17, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. and removed commit-queue PRs queued for automated landing through the Commit Queue. labels Dec 17, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
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

@panva panva added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed PRs whose Commit Queue landing failed and need manual intervention before retrying. labels Dec 17, 2023
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 17, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@panva panva added the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 17, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Dec 17, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 154afbe...0afe731

nodejs-github-bot pushed a commit that referenced this pull request Dec 17, 2023
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
PR-URL: #50897
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@panva
panva deleted the crypto-objects-equal branch December 17, 2023 13:16
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #50897
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
PR-URL: #50897
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50897
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #50897
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
@chharvey

chharvey commented Jun 9, 2024

Copy link
Copy Markdown
Contributor

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.

@aduh95

aduh95 commented Jun 9, 2024

Copy link
Copy Markdown
Contributor

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

@chharvey

Copy link
Copy Markdown
Contributor

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

@panva

panva commented Jun 11, 2024

Copy link
Copy Markdown
Member Author

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

@chharvey

Copy link
Copy Markdown
Contributor

@aduh95 @panva I opened this issue: #53423

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

Labels

assert Issues and PRs related to the assert subsystem. commit-queue-rebase PRs the Commit Queue should land as multiple self-contained commits. crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. review wanted PRs that need review. util Issues and PRs related to the built-in util module. webcrypto Issues and PRs related to the Web Crypto API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants