Conversation
🦋 Changeset detectedLatest commit: 6ed7315 The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Size Report 1Affected Products
Test Logs |
Changeset File Check
|
Size Analysis Report 1Affected Products
Test Logs |
What's the effect of this, if you deploy an app with App Check to a site and call it with HTTP, do you get an error? |
The uuidv4 generator in util used `Math.random()`, which does not provide strong uniqueness guarantees (https://www.bocoup.com/blog/random-numbers). The places where the uuidv4 generator were used didn't require strong uniqueness guarantees (nothing security related), but I think it's good to move away from this from util in case we try to use it in the future. A better built-in alternative is `crypto.randomUUID()`, which does provide strong uniqueness guarantees. Since this is a more modern JS built-in, it's only [defined in secure contexts](https://blog.mozilla.org/security/2018/01/15/secure-contexts-everywhere/). Is this something we're concerned about? Are there any App Check users with apps running in non-secure environments?
@hsubox76 If you call It's worth mentioning that sites served locally on HTTP ( Also, in App Check, |
There was a problem hiding this comment.
Seems only used in tests in data-connect and database too. I support this change for these reasons
- It's really a small change touching just a few lines of code so the risk is low
- We can avoid rolling our own UUID implementation
I don't think the security considerations apply to our use cases. When we do need a secure implementation in the future, we'll likely have to do a security review anyway.
The uuidv4 generator in util uses
Math.random(), which does not provide strong uniqueness guarantees (https://www.bocoup.com/blog/random-numbers).The places where the uuidv4 generator are used don't require strong uniqueness guarantees (nothing security related), but I think it's good to move away from this from util in case we try to use it in the future.
A better built-in alternative is
crypto.randomUUID(), which does provide strong uniqueness guarantees. I believe the reason this wasn't used originally was because it wasn't supported in Node <15. Since this is a more modern JS built-in, it's only defined in secure contexts. Is this something we're concerned about? Are there any App Check users with apps running in non-secure environments?Fixes #6462
Also closes b/283984828