crypto: adjust types for getRandomValues #41481

LiviaMedeiros · 2022-01-11T17:15:46Z

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:

node/test/parallel/test-webcrypto-random.js

Lines 47 to 53 in 6b7b0b7

    
           { 
        
             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 · 2022-01-11T17:15:53Z

Review requested:

@nodejs/crypto

lpinca · 2022-01-11T20:19:21Z

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

LiviaMedeiros · 2022-01-11T23:43:22Z

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 · 2022-01-12T20:01:32Z

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 · 2022-01-12T22:25:08Z

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 · 2022-01-12T22:53:36Z

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.

DerekNonGeneric

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

test/fixtures/wpt/WebCryptoAPI/getRandomValues.any.js

doc/api/webcrypto.md

prevents Web Crypto API's getRandomValues from accepting DataView - Fixes: nodejs#41480 - Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

tools/doc/type-parser.mjs

This reverts commit 5f1c397.

tniessen · 2022-01-17T16:08:46Z

Thanks @LiviaMedeiros :)

nodejs-github-bot · 2022-01-22T12:28:01Z

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

panva · 2022-01-22T14:21:45Z

Landed in b8de7aa

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>

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>

This was previously a `DataView`, not an integer-type TypedArray, throwing in Node 17.5.0: nodejs/node#41481

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>

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

LiviaMedeiros mentioned this pull request Jan 11, 2022

Webcrypto getRandomValues accepts DataView #41480

Closed

tniessen requested a review from jasnell January 11, 2022 17:58

LiviaMedeiros force-pushed the adjust-getRandomValues branch from c3076ab to e074f4b Compare January 11, 2022 23:04

LiviaMedeiros marked this pull request as ready for review January 11, 2022 23:05

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

DerekNonGeneric approved these changes Jan 14, 2022

View reviewed changes

DerekNonGeneric added experimental Issues and PRs related to experimental features. typings webcrypto labels Jan 14, 2022

lpinca reviewed Jan 14, 2022

View reviewed changes

test/fixtures/wpt/WebCryptoAPI/getRandomValues.any.js Outdated Show resolved Hide resolved

lpinca reviewed Jan 14, 2022

View reviewed changes

doc/api/webcrypto.md Outdated Show resolved Hide resolved

LiviaMedeiros force-pushed the adjust-getRandomValues branch from c108ea1 to 1ee830b Compare January 14, 2022 08:34

lpinca approved these changes Jan 14, 2022

View reviewed changes

himself65 approved these changes Jan 15, 2022

View reviewed changes

LiviaMedeiros force-pushed the adjust-getRandomValues branch from 81a5a97 to 1dc73af Compare January 16, 2022 22:12

tniessen reviewed Jan 17, 2022

View reviewed changes

doc/api/webcrypto.md Outdated Show resolved Hide resolved

LiviaMedeiros and others added 5 commits January 17, 2022 16:34

crypto: adjust types for getRandomValues

f6ae133

prevents Web Crypto API's getRandomValues from accepting DataView - Fixes: nodejs#41480 - Refs: https://www.w3.org/TR/WebCryptoAPI/#Crypto-method-getRandomValues

doc: adjust types for webcrypto getRandomValues

a650735

tools: add more TypedArray types

5f1c397

Update doc/api/webcrypto.md

1b43562

Co-authored-by: Luigi Pinca <luigipinca@gmail.com>

doc: adjust types for webcrypto getRandomValues

213ea91

LiviaMedeiros force-pushed the adjust-getRandomValues branch from 1dc73af to 213ea91 Compare January 17, 2022 08:35

tniessen approved these changes Jan 17, 2022

View reviewed changes

tools/doc/type-parser.mjs Outdated Show resolved Hide resolved

jasnell approved these changes Jan 17, 2022

View reviewed changes

tools: revert TypedArray types

2e679a2

This reverts commit 5f1c397.

panva closed this Jan 22, 2022

ruyadorno mentioned this pull request Feb 8, 2022

v17.5.0 release proposal #41897

Merged

mrbbot added a commit to cloudflare/miniflare that referenced this pull request Feb 25, 2022

Pass correctly-typed value to webcrypto.getRandomValues()

b887877

This was previously a `DataView`, not an integer-type TypedArray, throwing in Node 17.5.0: nodejs/node#41481

danielleadams mentioned this pull request Mar 3, 2022

v16.14.1 release proposal #42200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: adjust types for getRandomValues #41481

crypto: adjust types for getRandomValues #41481

LiviaMedeiros commented Jan 11, 2022

nodejs-github-bot commented Jan 11, 2022

lpinca commented Jan 11, 2022 •

edited

LiviaMedeiros commented Jan 11, 2022

lpinca commented Jan 12, 2022

LiviaMedeiros commented Jan 12, 2022

LiviaMedeiros commented Jan 12, 2022

DerekNonGeneric left a comment

tniessen commented Jan 17, 2022

nodejs-github-bot commented Jan 22, 2022

panva commented Jan 22, 2022

	{
	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);
	}

crypto: adjust types for getRandomValues #41481

crypto: adjust types for getRandomValues #41481

Conversation

LiviaMedeiros commented Jan 11, 2022

nodejs-github-bot commented Jan 11, 2022

lpinca commented Jan 11, 2022 • edited

LiviaMedeiros commented Jan 11, 2022

lpinca commented Jan 12, 2022

LiviaMedeiros commented Jan 12, 2022

LiviaMedeiros commented Jan 12, 2022

DerekNonGeneric left a comment

Choose a reason for hiding this comment

tniessen commented Jan 17, 2022

nodejs-github-bot commented Jan 22, 2022

panva commented Jan 22, 2022

lpinca commented Jan 11, 2022 •

edited