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.

Sign up for GitHub

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

Jump to bottom

fix(worker): replace Math.random() with crypto.randomUUID() #1762

Merged
merged 1 commit into from
Oct 10, 2023
Merged

fix(worker): replace Math.random() with crypto.randomUUID() #1762

merged 1 commit into from
Oct 10, 2023

Conversation

laryro
Copy link
Contributor

@laryro laryro commented Oct 6, 2023

Replace Math.random() with with crypto.randomUUID() in mockServiceWorker.js file.

It's known that Math.Random isn't secure (see https://v8.dev/blog/math-random). Even if this file isn't supposed to be deployed in production, some static code analysis tools may flag this as a critical issue, breaking CI pipelines.

kettanaito
kettanaito reviewed Oct 7, 2023
View reviewed changes
src/mockServiceWorker.js Outdated Show resolved Hide resolved
@kettanaito
Copy link
Member

Hi, @laryro! Thanks for opening this, I think it's a great improvement. One thing I may add, MSW treats any changes to the worker file is breaking changes. Would you mind rebasing this pull request against the feat/standard-api branch? We would get the change released in the next major version then. Thanks!

@kettanaito kettanaito mentioned this pull request Oct 9, 2023
Merged
34 tasks
refactor(mockserviceworker.js): replace math.random with crypto.rando…
84e2e26 
…muuid in service worker file

It's known that Math.Random isn't secure (see https://v8.dev/blog/math-random). Even if this file
isn't supposed to be deployed in production, some static code analysis tools may flag this as a
critical issue, breaking CI pipelines.
@laryro laryro force-pushed the refactor/replace-math-random branch from bc2ebe0 to 84e2e26 Compare October 9, 2023 22:16
@laryro laryro changed the base branch from main to feat/standard-api October 9, 2023 22:17
@laryro
Copy link
Contributor Author

laryro commented Oct 9, 2023

Hi, @laryro! Thanks for opening this, I think it's a great improvement. One thing I may add, MSW treats any changes to the worker file is breaking changes. Would you mind rebasing this pull request against the feat/standard-api branch? We would get the change released in the next major version then. Thanks!

@kettanaito I rebased my branch and pointed it to feat/standard-api, as well as removed the slice method per our convo.
Lemme know if you need anything else 😄

@laryro laryro requested a review from kettanaito October 9, 2023 22:19
kettanaito
kettanaito approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

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

This looks fantastic! Thank you for working on this, @laryro, and welcome to contributors! 👏

@kettanaito kettanaito changed the title refactor(mockserviceworker): replace Math.random() with crypto.randomUUID() fix(worker): replace Math.random() with crypto.randomUUID() Oct 10, 2023
@kettanaito kettanaito merged commit 078e541 into mswjs:feat/standard-api Oct 10, 2023
8 checks passed
@laryro laryro deleted the refactor/replace-math-random branch October 10, 2023 13:05
@zyyv zyyv mentioned this pull request Dec 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kettanaito kettanaito kettanaito approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@laryro @kettanaito