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

fix: Test for invalid byte array sizes and ranges in v1(), v4(), and v7() #845

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gaoyia
Copy link

@gaoyia gaoyia commented Dec 24, 2024

Check and throw the following conditions for v1(), v4(), and v7():

  • options.random or options.rng() returning too-short byte arrays
  • range errors involving buf and offset

@gaoyia
Copy link
Author

gaoyia commented Dec 24, 2024

maybe we shoud

  • use random value
  • throw an Error
  • pad with zeros: uuidv4(v4options); // ⇨ '109156be-c4fb-41ea-b1b4-efe1671c0000'

@broofa
Copy link
Member

broofa commented Dec 24, 2024

Sorry, I missed your last comment. I've updated the PR as follows:

  • throw if options.random or options.rng() return insufficient entropy (too-short array)
  • throw if byte indexes over/underflow buffer range
  • Apply changes to v1() and v7() as well

Note: I removed your code that silently switched to rng() if the user-supplied options.random or options.rng() produced a too-short array. My rationale for that is if a user is explicitly providing one of those options, then they would be surprised to find the internal rng() being used. Better to be explicit about the problem rather than make assumptions about the solution.

@broofa broofa changed the title fix: v4 UUID generation with insufficient random bytes should match e… fix: Test for invalid byte array sizes and ranges in v1(), v4(), and v7() Dec 24, 2024
@broofa
Copy link
Member

broofa commented Dec 24, 2024

@ctavan : Okay to push this out as a patch release? I only hesitate because this code may throw where it didn't previously. While that's not strictly a breaking API change, there's a chance it might catch some users unawares. (But those users would see obviously corrupted UUIDs currently, so it's a bit of a self-correcting problem.)

It's a little surprising this hasn't come up previously. I do vaguely remember encountering this in development years ago and thinking that "undefined" appearing in a UUID string would throw for other reasons, so wasn't a concern. But that may have been before buf support was fleshed out? Or before the v8 or v10 refactors? I forget.

@broofa broofa mentioned this pull request Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants