fix: reject out-of-range timestamps in isValid - #133
Open
spokodev wants to merge 1 commit into
Open
Conversation
Copy link
Copy Markdown
Sorry, something went wrong.
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
1 participant
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
isValid only 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 — decodeTime throws and ULID_REGEX (/^[0-7]…/) does not match — so isValid disagrees with decodeTime:
Per the spec the largest valid ULID is 7ZZZZZZZZZZZZZZZZZZZZZZZZZ — a leading character above 7 overflows the timestamp. Over a fuzz of random 26-char Crockford strings, the current isValid disagrees with "does decodeTime succeed?" on ~74% of inputs.
Fix
Validate against the existing ULID_REGEX (already exported from constants and used by ulidToUUID), which additionally constrains the leading character to 0-7. After the change isValid(x) agrees with decodeTime(x) succeeding on 100% of fuzzed inputs, and the regex's character class preserves the existing case-insensitive behaviour.
Tests
isValid had no test coverage; added a describe("isValid", …) block covering a generated ULID (both cases), wrong length / out-of-alphabet rejection, the 48-bit timestamp bound, and agreement with decodeTime. The two bound/agreement assertions fail before the change and pass after. Full suite green (25/25), prettier --check clean.