Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CryptoKey string tag imprecise for detection #45987

Closed
brettz9 opened this issue Dec 27, 2022 · 3 comments · Fixed by #46042
Closed

CryptoKey string tag imprecise for detection #45987

brettz9 opened this issue Dec 27, 2022 · 3 comments · Fixed by #46042
Labels
crypto Issues and PRs related to the crypto subsystem. webcrypto

Comments

@brettz9
Copy link

brettz9 commented Dec 27, 2022

Version

v18.12.1

Platform

Darwin MacBook-Air-2.local 22.2.0 Darwin Kernel Version 22.2.0: Fri Nov 11 02:04:44 PST 2022; root:xnu-8792.61.2~4/RELEASE_ARM64_T8103 arm64

Subsystem

No response

What steps will reproduce the bug?

import {webcrypto as crypto} from 'node:crypto';
const key = await crypto.subtle.generateKey(
  {
    name: 'HMAC',
    hash: {name: 'SHA-512'}
  },
  true, // Extractable
  ['sign', 'verify']
);
const stringTag = Object.prototype.toString.call(key).slice(8, -1);
console.log(stringTag);

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

On the browser, this logs CryptoKey and allows for detection of object type and we expect it here.

What do you see instead?

Object

Additional information

This can be readily remedied by adding Symbol.toStringTag to the CryptoKey class returning the value, "CryptoKey".

brettz9 added a commit to brettz9/typeson-registry that referenced this issue Dec 27, 2022
@bnoordhuis
Copy link
Member

Pull request welcome, but...

We do WPT conformance testing so this is probably not a spec requirement. It'd be best to update upstream WPT first before making changes to node. There's a CryptoKey validator you could add a check to.

@brettz9
Copy link
Author

brettz9 commented Dec 28, 2022

Thanks for the info. IIRC, there are some separate WebIDL-based tests in WPT which might cover class string (string tag) checks for interfaces (and these tests weren't always contained in the same part of the directory structure).

As far as a spec basis for this, there is some mention of class strings in https://webidl.spec.whatwg.org/ , though I only see it mentioned there for interface prototypes, not necessarily interface objects. I wonder if it is implied elsewhere within WebIDL though.

@panva panva added the crypto Issues and PRs related to the crypto subsystem. label Jan 1, 2023
panva added a commit to panva/node that referenced this issue Jan 1, 2023
panva added a commit to panva/node that referenced this issue Jan 1, 2023
@panva panva added the webcrypto label Jan 1, 2023
panva added a commit to panva/node that referenced this issue Jan 1, 2023
@panva
Copy link
Member

panva commented Jan 1, 2023

I am not sure about this being a spec requirement too but if all existing implementations adhere, I don't see the point of not adding this to Node.js and WPT.

PR for the WPT repository web-platform-tests/wpt#37716.

brettz9 added a commit to brettz9/typeson-registry that referenced this issue Jan 1, 2023
brettz9 added a commit to brettz9/typeson-registry that referenced this issue Jan 1, 2023
brettz9 added a commit to brettz9/typeson-registry that referenced this issue Jan 6, 2023
feat: use built-in webcrypto and update for latest Node
brettz9 added a commit to brettz9/typeson-registry that referenced this issue Jan 6, 2023
feat: use built-in webcrypto and update for latest Node
nodejs-github-bot pushed a commit that referenced this issue Jan 18, 2023
closes #45987

PR-URL: #46042
Fixes: #45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
brettz9 added a commit to brettz9/typeson-registry that referenced this issue Jan 18, 2023
…e#45987

feat: use built-in webcrypto and update for latest Node
RafaelGSS pushed a commit that referenced this issue Jan 20, 2023
closes #45987

PR-URL: #46042
Fixes: #45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
panva added a commit to panva/node that referenced this issue Jan 24, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
panva added a commit to panva/node that referenced this issue Jan 24, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Backport-PR-URL: nodejs#46340
panva added a commit to panva/node that referenced this issue Jan 24, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Backport-PR-URL: nodejs#46340
juanarbol pushed a commit that referenced this issue Jan 26, 2023
closes #45987

PR-URL: #46042
Fixes: #45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
juanarbol pushed a commit to panva/node that referenced this issue Jan 28, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Backport-PR-URL: nodejs#46340
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
juanarbol pushed a commit that referenced this issue Jan 31, 2023
closes #45987

PR-URL: #46042
Fixes: #45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
panva added a commit to panva/node that referenced this issue Mar 31, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Backport-PR-URL: nodejs#47336
panva added a commit to panva/node that referenced this issue Mar 31, 2023
closes nodejs#45987

PR-URL: nodejs#46042
Fixes: nodejs#45987
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Backport-PR-URL: nodejs#47336
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. webcrypto
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants