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

feat: improved random UID generation through crypto module #218

Closed

Conversation

Jurredr
Copy link
Contributor

@Jurredr Jurredr commented Jan 8, 2025

Problem

Even though cryptographic safety is not really relevant to anything we're doing with IDs, it's good practice to make use of Node's built-in crypto package anyways, over implementing our own less optimized solution. See https://github.com/aws/mynah-ui/security/code-scanning/8.

Solution

Use crypto's random UUID function.

Tests

  • I have tested this change on VSCode
  • I have tested this change on JetBrains

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@Jurredr Jurredr added the improvement Improvement of an existing feature label Jan 8, 2025
@Jurredr Jurredr self-assigned this Jan 8, 2025
@Jurredr Jurredr requested a review from a team as a code owner January 8, 2025 11:12
@Jurredr Jurredr linked an issue Jan 8, 2025 that may be closed by this pull request
1 task
@dogusata
Copy link
Collaborator

dogusata commented Jan 8, 2025

Does it require a third party dependency?

Also, we're not using the UUIDs for specific identification and not storing them in the client. I'm not so sure about changing the current ID structure.

@Jurredr
Copy link
Contributor Author

Jurredr commented Jan 8, 2025

Does it require a third party dependency?

Also, we're not using the UUIDs for specific identification and not storing them in the client. I'm not so sure about changing the current ID structure.

I think it's better for the future to use the standard, instead of our own implementation. I don't see a need to reinvent the wheel. crypto is part of NodeJS, it's not like a separate new third party dependency, and it's fully stable and never changed (https://developer.mozilla.org/en-US/docs/Web/API/Crypto/randomUUID).

@Jurredr
Copy link
Contributor Author

Jurredr commented Jan 8, 2025

Closing this. Unnecessary hassle, I'm seeing that it has some ifs and buts like only working properly with https possibly. Also it's already failing on the UI testing package.

@Jurredr Jurredr closed this Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix code scanning alert - Insecure randomness
2 participants