Implement crypto.getRandomValues(array). #3352
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Are failures like |
sjrd
force-pushed
the
crypto-getrandomvalues
branch
from
73da75a
to
3fbf038
Compare
|
Rebased on top of master to get 4c7eed1. The CI is green now. 🙂 |
domenic
reviewed
Apr 27, 2022
|
Thanks for the review. I added commit that should address your comments. Let me know if you'd like me to squash the commits instead. |
domenic
approved these changes
Apr 27, 2022
There was a problem hiding this comment.
LGTM. Will try and do a merge and release this weekend, although I'm worried about how apparently jsdom is really broken on Node v18 right now and might have trouble fitting this into my weekend open-source time... #3358 (comment)
|
|
||
| // https://w3c.github.io/webcrypto/#Crypto-method-getRandomValues | ||
| getRandomValues(array) { | ||
| // Note: this rejects Float32Array and Float64Array. |
sjrd
force-pushed
the
crypto-getrandomvalues
branch
from
6221abc
to
3546aeb
Compare
|
Would love to see this in a minor release before dropping node 12 if possible (even though 12 is EOL later this week) 🙏 |
dbjorge
added a commit
to microsoft/accessibility-insights-web
that referenced
this pull request
Apr 28, 2022
* chore(deps-dev): bump jest from 27.5.1 to 28.0.0 Bumps [jest](https://github.com/facebook/jest/tree/HEAD/packages/jest) from 27.5.1 to 28.0.0. - [Release notes](https://github.com/facebook/jest/releases) - [Changelog](https://github.com/facebook/jest/blob/main/CHANGELOG.md) - [Commits](https://github.com/facebook/jest/commits/v28.0.0/packages/jest) --- updated-dependencies: - dependency-name: jest dependency-type: direct:development update-type: version-update:semver-major ... --- This required some manual updates. The easy ones were: * Jest 28 no longer includes `jest-environment-jsdom` as an implicit dependency. Our unit and e2e tests both use this, so I've added an explicit dependency for it. * Jest 28 adds a new `github-actions` reporter available by default. This seems like an obvious win, so I went ahead and enabled it. * Jest 28 adds native support for a `--shard` argument; I've replaced the janky manual version our GitHub CI build was using with the new native support. I fixed the off-by-one issue with our sharded job naming while I was here. I'll update the `main` branch policy accordingly once this PR merges. The other three were more interesting; I've given them separate comments for ease of linking them. --- #### Jest 28 tries to use an ESM version of `uuid` by default Jest 28 attempts to respect `package.json` `exports` fields based on the test environment you're using. This means that for packages that export separate entry points for node vs browser environments, Jest will attempt to use the browser entry point when you're using `jest-environment-jsdom` and the node entry point when you're using `jest-environment-node`. This is a good change that we want to use in most cases; it keeps our test environment closer to a real browser environment. Unfortunately, one of our dependencies (`uuid`) only provides an ESM implementation (not CommonJS) for browsers, though it provides both ESM and CommonJS versions for node. [This comment on uuid#620 summarizes their exports matrix](uuidjs/uuid#620 (comment)), and [uuid#616's discussion](uuidjs/uuid#616) has some context on the issue. `uuid`'s `package.json` indicates that its ESM+browser entry point is what *all* browser cases should use, so Jest ends up trying to use that even though Jest tries to use a CommonJS+browser entry point where available. This produces hundreds of errors that look something like this: <details> <summary>Full example of an error</summary> ``` ● Test suite failed to run Jest encountered an unexpected token Jest failed to parse a file. This happens e.g. when your code or its dependencies use non-standard JavaScript syntax, or when Jest is not configured to support such syntax. Out of the box Jest supports Babel, which will be used to transform your files into valid JS based on your Babel configuration. By default "node_modules" folder is ignored by transformers. Here's what you can do: • If you are trying to use ECMAScript Modules, see https://jestjs.io/docs/ecmascript-modules for how to enable it. • If you are trying to use TypeScript, see https://jestjs.io/docs/getting-started#using-typescript • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config. • If you need a custom transformation specify a "transform" option in your config. • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option. You'll find more details and examples of these config options in the docs: https://jestjs.io/docs/configuration For information about custom transformations, see: https://jestjs.io/docs/code-transformation Details: C:\repos\accessibility-insights-web\node_modules\uuid\dist\esm-browser\index.js:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,jest){export { default as v1 } from './v1.js'; ^^^^^^ SyntaxError: Unexpected token 'export' 1 | // Copyright (c) Microsoft Corporation. All rights reserved. 2 | // Licensed under the MIT License. > 3 | import { v4 } from 'uuid'; | ^ 4 | 5 | export type UUIDGenerator = () => string; 6 | at Runtime.createScriptFromCode (node_modules/jest-runtime/build/index.js:1773:14) at Object.<anonymous> (src/common/uid-generator.ts:3:26) ``` </details> I considered a few options for addressing this: * We could wait and hope that `uuid` starts including a CommonJS + browser entry point, like uuidjs/uuid#616 proposes. Jest would start picking this up without any changes, but this would fail because such a `uuid` entry point would assume the presence of a browser `crypto.getRandomValues` API. We would then have to either wait for JSDOM to start providing this (probably via jsdom/jsdom#3352) or provide a polyfill as part of our test environment/test setup. This is probably not a good solution because it seems unlikely that `uuid` will add a CommonJS browser build; the most recent word I saw from `uuid`'s maintainers was that they were ["convinced that adding a CommonJS browser build would be fundamentally wrong at this point in time"](uuidjs/uuid#620 (comment)) * We could try to make ESM+Jest work (this would fix the immediate issue but would still require a `crypto` implementation). [Jest's native support for this is currently blocked on node/v8 issues](jestjs/jest#9430), but it's possible we could work around that with something like https://github.com/nicolo-ribaudo/jest-light-runner. * We could override the `exports` conditions that `jest-environment-jsdom` passes, per [the suggestion in the Jest v27 to v28 upgrade guide](https://jestjs.io/docs/upgrading-to-jest28#packagejson-exports). As I understand it, this would amount to creating our own custom Jest Test Environment wrapping `jest-environment-jsdom` and overriding its [`exportConditions`](https://github.com/facebook/jest/blob/v28.0.0/packages/jest-environment-jsdom/src/index.ts#L160) property to look for use `node` instead of `browser`. This is something we could do ourselves immediately and much less work than moving to ESM, but I didn't love this solution because I wanted to keep the proper export conditions in the cases where they aren't broken * We could use a Jest `moduleNameMapper` entry like `'^uuid$': require.resolve('uuid')` to force the use of a Node+CommonJS version of `uuid`. This works and is more scoped than overriding `exports` conditions, but has the downside that it is essentially a silent `yarn resolution`; it forces *every* dependency chain through `uuid` to use whatever version of `uuid` happens to be hoisted, even if some chains want older versions. This would make future `uuid` upgrades very dangerous. See uuidjs/uuid#616 (comment) for context. * The option I chose: create a [custom Jest resolver](9ad4e61) which overrides only the specific behavior of how Jest resolves the `uuid` dependency, forcing it use the CommonJS+node version despite `jest-environment-jsdom`'s export conditions. --- #### `jsdom` v17 assumes the availability of a global `TextEncoder`, which `jest-environment-jsdom` does not provide Because we updated how we pull in `jest-environment-jsdom`, we ended up updating the version of `jsdom` that we use in practice, so we started hitting this not-entirely-new issue. This one is described by jsdom/jsdom#2524 (comment) (the original issue is for a separate-but-related feature request, which was unfortunately hijacked). The symptom is an error of form `ReferenceError: TextEncoder is not defined` on a line importing `jsdom`. <details> <summary>Full example of an error</summary> ``` FAIL unit tests src/tests/unit/tests/injected/visualization/drawer.test.ts ● Test suite failed to run ReferenceError: TextEncoder is not defined 1 | // Copyright (c) Microsoft Corporation. All rights reserved. 2 | // Licensed under the MIT License. > 3 | import { JSDOM } from 'jsdom'; | ^ 4 | 5 | export class TestDocumentCreator { 6 | public static createTestDocument(html: string = ''): Document { at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/encoding.js:2:21) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/url-state-machine.js:5:34) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/URL-impl.js:2:13) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/lib/URL.js:442:14) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/webidl2js-wrapper.js:3:13) at Object.<anonymous> (node_modules/jsdom/node_modules/whatwg-url/index.js:3:34) at Object.<anonymous> (node_modules/jsdom/lib/api.js:7:19) at Object.<anonymous> (src/tests/unit/common/test-document-creator.ts:3:30) at Object.<anonymous> (src/tests/unit/tests/injected/visualization/drawer.test.ts:15:78) ``` </details> The issue is that we're trying to import `jsdom` from within a `jest-environment-jsdom` environment. `jsdom` uses a global `TextEncoder` as part of its implementation, but doesn't export one globally. The suggested workaround from the Jest maintainer is to not import `jsdom` reentrantly, and to instead use `jest-environment-node` for tests that import `jsdom` themselves. We don't want to do that; we have some tests that want to import `jsdom` to create a fresh DOM environment to test in isolation with, but some of our transitive dependencies assume the availability of a global `document`/`window` for reasons unimportant to those specific tests. Instead, I worked around this by just re-exporting Node `util`'s `TextEncoder` as `window.TextEncoder` in our unit tests' `jest-setup`. --- #### Jest 28 forbids `describe(SomeThing, ...)` for `SomeThing`s without `name`s In Jest 27, specifying `describe(SomeThing, ...)` when `SomeThing` does not have a name (eg, an arrow function) was not an error; it just produced tests where the "SomeThing" that was intended to be filled in at the beginning of a test name was actually filled in as an empty string. Jest 28 has become more strict about this. Passing a named function or a named class is now explicitly supported, but passing an unnamed thing now produces an error of the form `Invalid first argument, <stringified version of Thing>. It must be a named class, named function, number, or string.` In practice, this error message is very challenging to parse because <stringified version of Thing> is the entire definition of a function, complete with code coverage markers: <details> <summary>Example of one of these errors</summary> ``` FAIL unit tests src/tests/unit/tests/DetailsView/details-view-content.test.tsx ● Test suite failed to run Invalid first argument, function (props) { /* istanbul ignore next */ cov_7cparukpf().f[1]++; var selectedDetailsViewSwitcherNavConfiguration = /* istanbul ignore next */ (cov_7cparukpf().s[20]++, props.deps.getDetailsSwitcherNavConfiguration({ selectedDetailsViewPivot: props.storeState.visualizationStoreData.selectedDetailsViewPivot })); /* istanbul ignore next */ cov_7cparukpf().s[21]++; var renderHeader = function () { /* istanbul ignore next */ cov_7cparukpf().f[2]++; var storeState = /* istanbul ignore next */ (cov_7cparukpf().s[22]++, props.storeState); var visualizationStoreData = /* istanbul ignore next */ (cov_7cparukpf().s[23]++, storeState.visualizationStoreData); /* istanbul ignore next */ cov_7cparukpf().s[24]++; return /*#__PURE__*/React.createElement(_interactiveHeader.InteractiveHeader, { deps: props.deps, selectedPivot: visualizationStoreData.selectedDetailsViewPivot, featureFlagStoreData: storeState.featureFlagStoreData, tabClosed: props.storeState.tabStoreData.isClosed, navMenu: selectedDetailsViewSwitcherNavConfiguration.leftNavHamburgerButton, isSideNavOpen: props.isSideNavOpen, setSideNavOpen: props.setSideNavOpen, narrowModeStatus: props.narrowModeStatus }); }; /* istanbul ignore next */ cov_7cparukpf().s[25]++; var renderOverlay = function () { /* istanbul ignore next */ cov_7cparukpf().f[3]++; var deps = /* istanbul ignore next */ (cov_7cparukpf().s[26]++, props.deps), storeState = /* istanbul ignore next */ (cov_7cparukpf().s[27]++, props.storeState); /* istanbul ignore next */ cov_7cparukpf().s[28]++; return /*#__PURE__*/React.createElement(_detailsViewOverlay.DetailsViewOverlay, { deps: deps, previewFeatureFlagsHandler: props.deps.previewFeatureFlagsHandler, scopingActionMessageCreator: props.deps.scopingActionMessageCreator, inspectActionMessageCreator: props.deps.inspectActionMessageCreator, detailsViewStoreData: storeState.detailsViewStoreData, scopingStoreData: storeState.scopingPanelStateStoreData, featureFlagStoreData: storeState.featureFlagStoreData, userConfigurationStoreData: storeState.userConfigurationStoreData }); }; /* istanbul ignore next */ cov_7cparukpf().s[29]++; var renderDetailsView = function () { /* istanbul ignore next */ cov_7cparukpf().f[4]++; var deps = /* istanbul ignore next */ (cov_7cparukpf().s[30]++, props.deps), storeState = /* istanbul ignore next */ (cov_7cparukpf().s[31]++, props.storeState); var selectedDetailsRightPanelConfiguration = /* istanbul ignore next */ (cov_7cparukpf().s[32]++, props.deps.getDetailsRightPanelConfiguration({ selectedDetailsViewPivot: storeState.visualizationStoreData.selectedDetailsViewPivot, detailsViewRightContentPanel: storeState.detailsViewStoreData.detailsViewRightContentPanel })); var selectedTest = /* istanbul ignore next */ (cov_7cparukpf().s[33]++, selectedDetailsViewSwitcherNavConfiguration.getSelectedDetailsView(storeState)); var automatedChecksCardsViewData = /* istanbul ignore next */ (cov_7cparukpf().s[34]++, props.deps.getCardViewData(props.storeState.unifiedScanResultStoreData.rules, props.storeState.unifiedScanResultStoreData.results, props.deps.getCardSelectionViewData(props.storeState.cardSelectionStoreData, props.storeState.unifiedScanResultStoreData, props.deps.isResultHighlightUnavailable))); var tabStopRequirementData = /* istanbul ignore next */ (cov_7cparukpf().s[35]++, props.storeState.visualizationScanResultStoreData.tabStops.requirements); var needsReviewCardsViewData = /* istanbul ignore next */ (cov_7cparukpf().s[[36](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:36)]++, props.deps.getCardViewData(props.storeState.needsReviewScanResultStoreData.rules, props.storeState.needsReviewScanResultStoreData.results, props.deps.getCardSelectionViewData(props.storeState.needsReviewCardSelectionStoreData, props.storeState.needsReviewScanResultStoreData, props.deps.isResultHighlightUnavailable))); var targetAppInfo = /* istanbul ignore next */ (cov_7cparukpf().s[[37](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:37)]++, { name: props.storeState.tabStoreData.title, url: props.storeState.tabStoreData.url }); var scanDate = /* istanbul ignore next */ (cov_7cparukpf().s[[38](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:38)]++, props.deps.getDateFromTimestamp(props.storeState.unifiedScanResultStoreData.timestamp)); var scanMetadata = /* istanbul ignore next */ (cov_7cparukpf().s[[39](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:39)]++, { timespan: { scanComplete: scanDate }, targetAppInfo: targetAppInfo, toolData: props.storeState.unifiedScanResultStoreData.toolInfo }); /* istanbul ignore next */ cov_7cparukpf().s[[40](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:40)]++; return /*#__PURE__*/React.createElement(_detailsViewBody.DetailsViewBody, { deps: deps, tabStoreData: storeState.tabStoreData, tabStopsViewStoreData: storeState.tabStopsViewStoreData, assessmentStoreData: storeState.assessmentStoreData, pathSnippetStoreData: storeState.pathSnippetStoreData, featureFlagStoreData: storeState.featureFlagStoreData, selectedTest: selectedTest, detailsViewStoreData: storeState.detailsViewStoreData, visualizationStoreData: storeState.visualizationStoreData, visualizationScanResultData: storeState.visualizationScanResultStoreData, visualizationConfigurationFactory: props.deps.visualizationConfigurationFactory, assessmentsProvider: props.deps.assessmentsProvider, dropdownClickHandler: props.deps.dropdownClickHandler, clickHandlerFactory: props.deps.clickHandlerFactory, assessmentInstanceTableHandler: props.deps.assessmentInstanceTableHandler, issuesTableHandler: props.deps.issuesTableHandler, rightPanelConfiguration: selectedDetailsRightPanelConfiguration, switcherNavConfiguration: selectedDetailsViewSwitcherNavConfiguration, userConfigurationStoreData: storeState.userConfigurationStoreData, automatedChecksCardsViewData: automatedChecksCardsViewData, needsReviewCardsViewData: needsReviewCardsViewData, scanIncompleteWarnings: storeState.unifiedScanResultStoreData.scanIncompleteWarnings, scanMetadata: scanMetadata, isSideNavOpen: props.isSideNavOpen, setSideNavOpen: props.setSideNavOpen, narrowModeStatus: props.narrowModeStatus, tabStopRequirementData: tabStopRequirementData }); }; /* istanbul ignore next */ cov_7cparukpf().s[[41](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:41)]++; return /*#__PURE__*/React.createElement(React.Fragment, null, renderHeader(), renderDetailsView(), renderOverlay()); }. It must be a named class, named function, number, or string. [49](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:49) | import { StoreMocks } from './store-mocks'; [50](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:50) | > [51](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:51) | describe(DetailsViewContent, () => { | ^ [52](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:52) | const pageTitle = 'DetailsViewContainerTest title'; [53](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:53) | const pageUrl = 'http://detailsViewContainerTest/url/'; [54](https://github.com/microsoft/accessibility-insights-web/runs/6167262184?check_suite_focus=true#step:7:54) | let detailsViewActionMessageCreator: IMock<DetailsViewActionMessageCreator>; at Object.describe (src/tests/unit/tests/DetailsView/details-view-content.test.tsx:51:1) ``` </details> 18 of our test files trigger this issue. In every case, the unnamed component in question is a `NamedFC` React component. The `displayName` that `NamedFC` gives these components is just for the benefit of React debugging messages, and doesn't help Jest - these really are 18 test files with test names like ` renders normally` instead of `ThingUnderTest renders normally`, with an empty string where the component name belongs. I think the ideal way to resolve this would be to completely remove our `NamedFC` wrapper and replace it with `eslint-plugin-react`'s [display-name](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/docs/rules/display-name.md) and/or [function-component-definition](https://github.com/jsx-eslint/eslint-plugin-react/blob/HEAD/docs/rules/function-component-definition.md) rules (with similar settings to [what AirBnB uses as of this issue's resolution](airbnb/javascript#2505)). That would be a huge change, though; we have hundreds of components that use `NamedFC`. I'd want to do a separate PR for that. Instead, this PR just changes the 18 tests in question to use `TheNamedFC.displayName`; this has the advantage that it will break obviously if we ever do the bigger change. --- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Dan Bjorge <danielbj@microsoft.com>
This is the only member of `crypto` that is available even in non-secure contexts.
domenic
force-pushed
the
crypto-getrandomvalues
branch
from
3546aeb
to
7f98b6c
Compare
|
Thank you for merging and for the v20.0.0 release. :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the only member of
cryptothat is available even in non-secure contexts.Motivation: Since we fixed a security in Scala.js related to
java.util.UUID.randomUUID()(see GHSA-j2f9-w8wh-9ww4), the need forcrypto.getRandomValueshas become quite high.jsdomis one of the main testing environments used by the Scala.js community. The lack ofgetRandomValuesis now causing several maintainers and project authors to use insecure variants because ofjsdom. This in turn risks exposing them to security vulnerabilities of their own. I hope that by actually providingcrypto.getRandomValuesinjsdom, we will avoid this problematic situation.This is my first contribution to
jsdom. Don't hesitate to tell me if anything should be done differently. Also, if things should change, please tell me whether you prefer amending the commits and force-pushing on the branch, or whether you prefer to add fixup commits to the branch.