fix: reject out-of-range timestamps in isValid#133
Open
spokodev wants to merge 1 commit into
Open
Conversation
isValid only checked the length and the Crockford alphabet, so it accepted
ULID strings whose leading character is above 7 — i.e. a timestamp beyond
the 48-bit maximum. decodeTime and ULID_REGEX both reject those, so isValid
disagreed with the rest of the library:
isValid("Z".repeat(26)) // true
decodeTime("Z".repeat(26)) // throws "Malformed ULID: timestamp too large"
Validate against the existing ULID_REGEX, which also constrains the leading
character to 0-7.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
isValidonly checks the length (26) and that every character is in the Crockford base32 alphabet. It does not constrain the leading character, so it accepts strings whose timestamp component exceeds the 48-bit maximum (TIME_MAX). The rest of the library rejects those —decodeTimethrows andULID_REGEX(/^[0-7]…/) does not match — soisValiddisagrees withdecodeTime:Per the spec the largest valid ULID is
7ZZZZZZZZZZZZZZZZZZZZZZZZZ— a leading character above7overflows the timestamp. Over a fuzz of random 26-char Crockford strings, the currentisValiddisagrees with "doesdecodeTimesucceed?" on ~74% of inputs.Fix
Validate against the existing
ULID_REGEX(already exported fromconstantsand used byulidToUUID), which additionally constrains the leading character to0-7. After the changeisValid(x)agrees withdecodeTime(x)succeeding on 100% of fuzzed inputs, and the regex's character class preserves the existing case-insensitive behaviour.Tests
isValidhad no test coverage; added adescribe("isValid", …)block covering a generated ULID (both cases), wrong length / out-of-alphabet rejection, the 48-bit timestamp bound, and agreement withdecodeTime. The two bound/agreement assertions fail before the change and pass after. Full suite green (25/25),prettier --checkclean.