-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
fix(utils): Support crypto.getRandomValues in old Chromium versions #9251
Conversation
There was a problem hiding this 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:
sentry-javascript/packages/utils/test/misc.test.ts
Lines 292 to 346 in b036647
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); | |
} | |
}); | |
}); |
@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 ![]() Because of this, when Node version is < v17.4.0, the unit tests that are intended to test |
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. |
yarn lint
) & (yarn test
).Fixes #9250
Here is my proposal to fix
getRandomByte
function throwing an error due tocrypto.getRandomValues
returningundefined
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.