Skip to content

fix(utils): Support crypto.getRandomValues in old Chromium versions #9251

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

Conversation

jghinestrosa
Copy link
Contributor

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

Fixes #9250

Here is my proposal to fix getRandomByte function throwing an error due to crypto.getRandomValues returning undefined in old browser engines like Chromium 23.

This error could be fixed as well by using a polyfill in every project that imports and uses Sentry but, since the change of this PR only involved keeping the Uint8Array reference in a variable, I thought it would be worth it to give it a try.

Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Hi @jghinestrosa thanks for opening this PR! Looks good to me.
Quite odd that this function seems to be only semi-implemented in these versions but after reading the MDN docs, I think your change makes sense and should work.

Can you still add a test for this case? We already have tests for this function here:

describe('uuid4 generation', () => {
const uuid4Regex = /^[0-9A-F]{12}[4][0-9A-F]{3}[89AB][0-9A-F]{15}$/i;
// Jest messes with the global object, so there is no global crypto object in any node version
// For this reason we need to create our own crypto object for each test to cover all the code paths
it('returns valid uuid v4 ids via Math.random', () => {
for (let index = 0; index < 1_000; index++) {
expect(uuid4()).toMatch(uuid4Regex);
}
});
it('returns valid uuid v4 ids via crypto.getRandomValues', () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const cryptoMod = require('crypto');
(global as any).crypto = { getRandomValues: cryptoMod.getRandomValues };
for (let index = 0; index < 1_000; index++) {
expect(uuid4()).toMatch(uuid4Regex);
}
});
it('returns valid uuid v4 ids via crypto.randomUUID', () => {
// eslint-disable-next-line @typescript-eslint/no-var-requires
const cryptoMod = require('crypto');
(global as any).crypto = { randomUUID: cryptoMod.randomUUID };
for (let index = 0; index < 1_000; index++) {
expect(uuid4()).toMatch(uuid4Regex);
}
});
it("return valid uuid v4 even if crypto doesn't exists", () => {
(global as any).crypto = { getRandomValues: undefined, randomUUID: undefined };
for (let index = 0; index < 1_000; index++) {
expect(uuid4()).toMatch(uuid4Regex);
}
});
it('return valid uuid v4 even if crypto invoked causes an error', () => {
(global as any).crypto = {
getRandomValues: () => {
throw new Error('yo');
},
randomUUID: () => {
throw new Error('yo');
},
};
for (let index = 0; index < 1_000; index++) {
expect(uuid4()).toMatch(uuid4Regex);
}
});
});

@Lms24
Copy link
Member

Lms24 commented Oct 23, 2023

@jghinestrosa thanks for adding tests! It seems like the tests fail for Node versions <18. Would you mind taking another look?

@jghinestrosa
Copy link
Contributor Author

jghinestrosa commented Oct 28, 2023

@Lms24

@jghinestrosa thanks for adding tests! It seems like the tests fail for Node versions <18. Would you mind taking another look?

Done!

It looks like crypto.getRandomValues is supported since Node v17.4.0: https://nodejs.org/api/crypto.html#cryptogetrandomvaluestypedarray

Node.js documentation specifying that crypto.getRandomValues is supported since Node v17.4.0

Because of this, when Node version is < v17.4.0, the unit tests that are intended to test crypto.getRandomValues might not work as expected. I added a check before calling the function.

@jghinestrosa
Copy link
Contributor Author

Hi @Lms24, is there anything else missing in this PR that I didn't notice? Please, feel free to let me know if you need something else to finish the review process.

@AbhiPrasad AbhiPrasad merged commit e128936 into getsentry:develop Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto.getRandomValues returns undefined in Chromium 23
3 participants