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

getRandomValues() and BigInt #255

Closed
annevk opened this issue May 3, 2021 · 4 comments · Fixed by #266
Closed

getRandomValues() and BigInt #255

annevk opened this issue May 3, 2021 · 4 comments · Fixed by #266
Labels

Comments

@annevk
Copy link
Member

annevk commented May 3, 2021

As per web-platform-tests/wpt#27920 (comment) this should be clarified whether BigInt64Array and BigUint64Array are to be supported. It should probably also not speak of an "integer type" and instead replace that with the text between parenthesis.

If they are not supported, please turn this into a feature request as it would make sense for them to work here.

cc @bathos

@twiss
Copy link
Member

twiss commented May 3, 2021

They aren't supported anywhere I tested - so I'll make the change you suggested. I believe, as the linked comment does, that the intent was to exclude Float32Array and Float64Array (the issue with those is that the caller might expect some uniform distribution that you don't get if you interpret random bytes as floats). I don't see any issues with BigInt64Array and BigUint64Array, so I agree it would make sense to add support for those, if browsers are interested in implementing this.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 2, 2021
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
@TimothyGu
Copy link
Member

Filed issues:

Tests are in web-platform-tests/wpt#29565. I'm working on implementing this change in Chrome right now, and I will get around to implementing it in Firefox soon. I think this would likely satisfy the implementor interest requirement.

@twiss
Copy link
Member

twiss commented Jul 2, 2021

@TimothyGu Thanks! I've opened #266 to fix this issue, let me know if you have any comments there as well.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 5, 2021
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 6, 2021
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3002049
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#898924}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jul 6, 2021
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3002049
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#898924}
pull bot pushed a commit to Yannic/chromium that referenced this issue Jul 7, 2021
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3002049
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#898924}
@lucacasonato
Copy link
Contributor

Tracking issue for Deno: denoland/deno#11445
Tracking issue for Node: nodejs/node#39442

targos added a commit to targos/node that referenced this issue Jul 19, 2021
targos added a commit to targos/node that referenced this issue Jul 19, 2021
nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Jul 23, 2021
Refs: w3c/webcrypto#255
Fixes: #39442

PR-URL: #39443
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
targos added a commit to nodejs/node that referenced this issue Jul 25, 2021
Refs: w3c/webcrypto#255
Fixes: #39442

PR-URL: #39443
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
richardlau pushed a commit to nodejs/node that referenced this issue Jul 29, 2021
Refs: w3c/webcrypto#255
Fixes: #39442

PR-URL: #39443
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
BethGriggs pushed a commit to nodejs/node that referenced this issue Jul 29, 2021
Refs: w3c/webcrypto#255
Fixes: #39442

PR-URL: #39443
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Zeyu Yang <[email protected]>
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
The check for "integer arrays" is to prevent people from using
getRandomValues with floating-point arrays, which doesn't work as people
might expect. However, the check was not updated when Chrome gained
support for BigInt64Array and BigUint64Array, which do work as expected.
This allows code fragments such as

    const b = new BigInt64Array(10);
    crypto.getRandomValues(b);

See w3c/webcrypto#255 for the spec issue.

Bug: 1225765
Change-Id: I338ecc5594492e6f329580f4e8f04d367f866361
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3002049
Reviewed-by: Mike West <[email protected]>
Commit-Queue: Timothy Gu <[email protected]>
Cr-Commit-Position: refs/heads/master@{#898924}
NOKEYCHECK=True
GitOrigin-RevId: 88825b38ea9859ca06f31aa60bc8e8ea6563f57d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants