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

crypto: adjust types for getRandomValues #41481

Closed

Conversation

LiviaMedeiros
Copy link
Contributor

Fixes: #41480

Prevents Web Crypto API's getRandomValues from accepting DataView

WIP/draft at this moment.

I'm not sure if new DataView(buf.buffer) in this test was intentional:

{
const buf = new Uint16Array(10);
const before = Buffer.from(buf).toString('hex');
getRandomValues(new DataView(buf.buffer));
const after = Buffer.from(buf).toString('hex');
assert.notStrictEqual(before, after);
}

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto

@nodejs-github-bot nodejs-github-bot added crypto Issues and PRs related to the crypto subsystem. needs-ci PRs that need a full CI run. labels Jan 11, 2022
@tniessen tniessen requested a review from jasnell January 11, 2022 17:58
@lpinca
Copy link
Member

lpinca commented Jan 11, 2022

@LiviaMedeiros If this is ready (it seems so) please remove the draft status, otherwise CI does not run. Thank you.

@LiviaMedeiros LiviaMedeiros force-pushed the adjust-getRandomValues branch from c3076ab to e074f4b Compare January 11, 2022 23:04
@LiviaMedeiros LiviaMedeiros marked this pull request as ready for review January 11, 2022 23:05
@LiviaMedeiros
Copy link
Contributor Author

Sure, now it's ready.

Also I should explicitly mention that this is a potentially breaking change, if someone actually uses this method with DataView.
Such usage is unlikely; but it's a part of public API and current documentation allows that.

@lpinca
Copy link
Member

lpinca commented Jan 12, 2022

The Web Crypto API is experimental so I do not think we should label this as "semver-major". Can you please also update the doc to remove the no longer supported types?

@LiviaMedeiros
Copy link
Contributor Author

That's great.
Updated the doc.

Raw ArrayBuffer from previous version wasn't acceptable and didn't work:

crypto.getRandomValues(new ArrayBuffer(8)); // DOMException [TypeMismatchError]

@LiviaMedeiros LiviaMedeiros force-pushed the adjust-getRandomValues branch 2 times, most recently from 6b067ef to c108ea1 Compare January 12, 2022 22:41
@LiviaMedeiros
Copy link
Contributor Author

Rebased to make linter happy, updated list of jsGlobalTypes to make type parser happy.
If the union type looks too pedantic and ugly, it may be reduced to just {Buffer|TypedArray}.
But technically even Buffer is redundant, since its prototype is Uint8Array; and it would require an explicit warning about Float32Array and Float64Array anyway.

Copy link
Contributor

@DerekNonGeneric DerekNonGeneric left a comment

Choose a reason for hiding this comment

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

@LiviaMedeiros, thank you so much for this PR!

Did a light review, so pre-approving for now, but hope that we can get a more thorough review on this by other collaborators more involved in these parts — would like to see if further improvements can be made here. :)

@DerekNonGeneric DerekNonGeneric added experimental Issues and PRs related to experimental features. typings webcrypto labels Jan 14, 2022
doc/api/webcrypto.md Outdated Show resolved Hide resolved
@LiviaMedeiros LiviaMedeiros force-pushed the adjust-getRandomValues branch from c108ea1 to 1ee830b Compare January 14, 2022 08:34
@LiviaMedeiros LiviaMedeiros force-pushed the adjust-getRandomValues branch from 81a5a97 to 1dc73af Compare January 16, 2022 22:12
doc/api/webcrypto.md Outdated Show resolved Hide resolved
@LiviaMedeiros LiviaMedeiros force-pushed the adjust-getRandomValues branch from 1dc73af to 213ea91 Compare January 17, 2022 08:35
tools/doc/type-parser.mjs Outdated Show resolved Hide resolved
@tniessen
Copy link
Member

Thanks @LiviaMedeiros :)

@nodejs-github-bot
Copy link
Collaborator

@panva
Copy link
Member

panva commented Jan 22, 2022

Landed in b8de7aa

panva pushed a commit that referenced this pull request Jan 22, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
@panva panva closed this Jan 22, 2022
BethGriggs pushed a commit that referenced this pull request Jan 25, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Linkgoron pushed a commit to Linkgoron/node that referenced this pull request Jan 31, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: nodejs#41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: nodejs#41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
mrbbot added a commit to cloudflare/miniflare that referenced this pull request Feb 25, 2022
This was previously a `DataView`, not an integer-type TypedArray,
throwing in Node 17.5.0: nodejs/node#41481
danielleadams pushed a commit that referenced this pull request Feb 28, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
prevents Web Crypto API's getRandomValues from accepting DataView

Fixes: #41480
Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

PR-URL: #41481
Reviewed-By: Derek Lewis <DerekNonGeneric@inf.is>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Zeyu Yang <himself65@outlook.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
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. experimental Issues and PRs related to experimental features. needs-ci PRs that need a full CI run. typings webcrypto
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webcrypto getRandomValues accepts DataView
8 participants