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.

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

Remove crypto.randomUUID dependency in favor of a custom function #1016

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

sejas
Copy link
Collaborator

@sejas sejas commented Feb 8, 2024

What is this PR doing?

What problem is it solving?

crypto is a library that is available, but needs to be imported on node apps.

How is the problem addressed?

It replaces the function that generates a random value with our custom library

Testing Instructions

  • CI tests pass

@sejas sejas changed the title Remove/crypto dependency Remove crypto.randomUUID dependency in favor of a custom function Feb 8, 2024
@sejas sejas requested a review from adamziel February 8, 2024 12:47
@sejas sejas self-assigned this Feb 8, 2024
@@ -33,7 +33,7 @@ export async function installAsset(
const assetNameGuess = zipFileName.replace(/\.zip$/, '');

const wpContent = joinPaths(await playground.documentRoot, 'wp-content');
const tmpDir = joinPaths(wpContent, crypto.randomUUID());
const tmpDir = joinPaths(wpContent, randomString(46));
Copy link
Contributor

Choose a reason for hiding this comment

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

[ Question ] Why we increased the characters we are using in comparison with UUID?
UUID has 36 + 4 characters instead of 46.
Can we change that to be 40 instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the correction. UUID actually is 36 characters long. Example 'd6c556d6-32b8-4bdf-b338-20a16e9860aa'.

A string containing a randomly generated, 36 character long v4 UUID.
https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID

I added a default length of 36 to randomString function.

https://github.com/WordPress/wordpress-playground/pull/1016/files#diff-0a926c4fcfdcd984fa42eaa66f44f2a5aea8bcde6576ee5ba04698402489ad65R1

@adamziel
Copy link
Collaborator

adamziel commented Feb 8, 2024

Thank you @sejas! One caveat would be that randomString() can return a string containing / which has a special meaning in the path. Also, by default it also uses symbols like !@#$%^&*. Perhaps exporting another utility function like randomFilename() would be more appropriate here after all.

@sejas
Copy link
Collaborator Author

sejas commented Feb 8, 2024

@adamziel, Right!, I improved randomString to accept special characters as a parameter, and I created randomFilename to reuse the same function.

@adamziel adamziel merged commit ececcd3 into trunk Feb 8, 2024
5 checks passed
@adamziel adamziel deleted the remove/crypto-dependency branch February 8, 2024 21:55
@dmsnell
Copy link
Member

dmsnell commented Feb 9, 2024

While I understand that the Playground has its own limited risk for security, I'm a bit surprised we removed a cryptographically-secure source of randomness in favor of one without that. Did we perform an audit to ensure that we don't need better randomness in the app? If all we're doing is creating a temporary filename for an in-memory database it seems harmless, but if we're using this for anything related to sensitive content then we may want to ensure we limit the use of this custom generator and warn people that it provides no cryptographic value.

@adamziel
Copy link
Collaborator

adamziel commented Feb 9, 2024

@dmsnell I agree. This PR is strictly about creating temporary files with random names, not cryptographic security. For anything security related we’ll still need to use crypto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants