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

[wasm] Use "node:crypto" to polyfill getRandomValues on older node #78696

Merged
merged 2 commits into from
Nov 23, 2022

Conversation

maraf
Copy link
Member

@maraf maraf commented Nov 22, 2022

  • Use "node:crypto" when globalThis.crypto is undefined or doesn't have getRandomValues.
  • Polyfill getRandomValues from webcrypto when available
  • Polyfill getRandomValues using randomBytes on even older node

Fixes #77927

@maraf maraf added this to the 8.0.0 milestone Nov 22, 2022
@maraf maraf self-assigned this Nov 22, 2022
@ghost
Copy link

ghost commented Nov 22, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Use "node:crypto" when globalThis.crypto is undefined or doesn't have getRandomValues.
  • Polyfill getRandomValues from webcrypto when available
  • Polyfill getRandomValues using randomBytes on even older node

Fixes #77927

Author: maraf
Assignees: maraf
Labels:

arch-wasm, area-System.Runtime.InteropServices.JavaScript

Milestone: 8.0.0

@maraf
Copy link
Member Author

maraf commented Nov 22, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lewing
Copy link
Member

lewing commented Nov 22, 2022

removing the insecure polyfill in the test runner breaks v8

@maraf
Copy link
Member Author

maraf commented Nov 23, 2022

/azp run runtime-wasm

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@maraf maraf merged commit 4e8e5e5 into dotnet:main Nov 23, 2022
@maraf maraf deleted the WasmNodeCrypto branch November 23, 2022 13:09
@maraf
Copy link
Member Author

maraf commented Nov 23, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3532333494

@radical
Copy link
Member

radical commented Dec 2, 2022

@maraf you tested this on node v14, v17, and v19. Our tests should be running with v14 (from emsdk). Do we need to make sure that this doesn't break for 17/19/others? IOW, do we need to run a few tests with these different versions of node?

@maraf
Copy link
Member Author

maraf commented Dec 5, 2022

@maraf you tested this on node v14, v17, and v19. Our tests should be running with v14 (from emsdk). Do we need to make sure that this doesn't break for 17/19/others? IOW, do we need to run a few tests with these different versions of node?

I think this is a first such thing.
Also, node v19 support fetch out of the box, so you could skip installing a polyfill for it.

@pavelsavara do you have an opinion?

@pavelsavara
Copy link
Member

I would stick to v14 for bulk of the testing for Net8.
But having alternate versions in runtime-extra-platforms would be nice I would go for LTS or Current (18.12 and 19.2).

Or should we make the Current version the main and have v14 just for extra platforms?
It would match what we do with latest V8, right ?

@maraf
Copy link
Member Author

maraf commented Dec 5, 2022

Yeah, having Current as primary target makes sense. Than targeting these extras on v14.

@radical
Copy link
Member

radical commented Dec 5, 2022

@maraf Could you please open an issue to track this?

@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants