diff --git a/ulid.go b/ulid.go index e07e1a6..9a2ec8d 100644 --- a/ulid.go +++ b/ulid.go @@ -155,24 +155,22 @@ func Make() (id ULID) { // Parse parses an encoded ULID, returning an error in case of failure. // // ErrDataSize is returned if the len(ulid) is different from an encoded -// ULID's length. Invalid encodings produce undefined ULIDs. For a version that -// returns an error instead, see ParseStrict. +// ULID's length. Invalid encodings return ErrInvalidCharacters. func Parse(ulid string) (id ULID, err error) { - return id, parse([]byte(ulid), false, &id) + return id, parse([]byte(ulid), &id) } // ParseStrict parses an encoded ULID, returning an error in case of failure. // -// It is like Parse, but additionally validates that the parsed ULID consists -// only of valid base32 characters. It is slightly slower than Parse. +// It is equivalent to Parse. // // ErrDataSize is returned if the len(ulid) is different from an encoded // ULID's length. Invalid encodings return ErrInvalidCharacters. func ParseStrict(ulid string) (id ULID, err error) { - return id, parse([]byte(ulid), true, &id) + return Parse(ulid) } -func parse(v []byte, strict bool, id *ULID) error { +func parse(v []byte, id *ULID) error { // Check if a base32 encoded ULID is the right length. if len(v) != EncodedSize { return ErrDataSize @@ -180,33 +178,32 @@ func parse(v []byte, strict bool, id *ULID) error { // Check if all the characters in a base32 encoded ULID are part of the // expected base32 character set. - if strict && - (dec[v[0]] == 0xFF || - dec[v[1]] == 0xFF || - dec[v[2]] == 0xFF || - dec[v[3]] == 0xFF || - dec[v[4]] == 0xFF || - dec[v[5]] == 0xFF || - dec[v[6]] == 0xFF || - dec[v[7]] == 0xFF || - dec[v[8]] == 0xFF || - dec[v[9]] == 0xFF || - dec[v[10]] == 0xFF || - dec[v[11]] == 0xFF || - dec[v[12]] == 0xFF || - dec[v[13]] == 0xFF || - dec[v[14]] == 0xFF || - dec[v[15]] == 0xFF || - dec[v[16]] == 0xFF || - dec[v[17]] == 0xFF || - dec[v[18]] == 0xFF || - dec[v[19]] == 0xFF || - dec[v[20]] == 0xFF || - dec[v[21]] == 0xFF || - dec[v[22]] == 0xFF || - dec[v[23]] == 0xFF || - dec[v[24]] == 0xFF || - dec[v[25]] == 0xFF) { + if dec[v[0]] == 0xFF || + dec[v[1]] == 0xFF || + dec[v[2]] == 0xFF || + dec[v[3]] == 0xFF || + dec[v[4]] == 0xFF || + dec[v[5]] == 0xFF || + dec[v[6]] == 0xFF || + dec[v[7]] == 0xFF || + dec[v[8]] == 0xFF || + dec[v[9]] == 0xFF || + dec[v[10]] == 0xFF || + dec[v[11]] == 0xFF || + dec[v[12]] == 0xFF || + dec[v[13]] == 0xFF || + dec[v[14]] == 0xFF || + dec[v[15]] == 0xFF || + dec[v[16]] == 0xFF || + dec[v[17]] == 0xFF || + dec[v[18]] == 0xFF || + dec[v[19]] == 0xFF || + dec[v[20]] == 0xFF || + dec[v[21]] == 0xFF || + dec[v[22]] == 0xFF || + dec[v[23]] == 0xFF || + dec[v[24]] == 0xFF || + dec[v[25]] == 0xFF { return ErrInvalidCharacters } @@ -400,9 +397,9 @@ const EncodedSize = 26 // parsing the data as string encoded ULID. // // ErrDataSize is returned if the len(v) is different from an encoded -// ULID's length. Invalid encodings produce undefined ULIDs. +// ULID's length. Invalid encodings return ErrInvalidCharacters. func (id *ULID) UnmarshalText(v []byte) error { - return parse(v, false, id) + return parse(v, id) } // Time returns the Unix time in milliseconds encoded in the ULID. diff --git a/ulid_test.go b/ulid_test.go index afa194b..a7a8afc 100644 --- a/ulid_test.go +++ b/ulid_test.go @@ -121,7 +121,7 @@ func TestMustParse(t *testing.T) { {"MustParse", ulid.MustParse}, {"MustParseStrict", ulid.MustParseStrict}, } { - t.Run(tc.name, func(t *testing.T) { + t.Run(tc.name+"/ErrDataSize", func(t *testing.T) { defer func() { if got, want := recover(), ulid.ErrDataSize; got != want { t.Errorf("got panic %v, want %v", got, want) @@ -130,6 +130,14 @@ func TestMustParse(t *testing.T) { _ = tc.fn("") }) + t.Run(tc.name+"/ErrInvalidCharacters", func(t *testing.T) { + defer func() { + if got, want := recover(), ulid.ErrInvalidCharacters; got != want { + t.Errorf("got panic %v, want %v", got, want) + } + }() + _ = tc.fn("0LLLLLLLLLLLLLLLLLLLLLLLLL") + }) } } @@ -228,14 +236,71 @@ func TestParseStrictInvalidCharacters(t *testing.T) { for _, tt := range testCases { t.Run(tt.name, func(t *testing.T) { - _, err := ulid.ParseStrict(tt.input) - if err != ulid.ErrInvalidCharacters { + if _, err := ulid.ParseStrict(tt.input); err != ulid.ErrInvalidCharacters { + t.Errorf("ParseStrict(%q): got err %v, want %v", tt.input, err, ulid.ErrInvalidCharacters) + } + if _, err := ulid.Parse(tt.input); err != ulid.ErrInvalidCharacters { t.Errorf("Parse(%q): got err %v, want %v", tt.input, err, ulid.ErrInvalidCharacters) } }) } } +func TestParseInvalidCharacters(t *testing.T) { + t.Parallel() + + // Crockford Base32 excludes I, L, O, U (and lowercase i, l, o, u). + // Strings containing them must be rejected with ErrInvalidCharacters. + issue69Cases := []struct { + name string + input string + }{ + {"uppercase L", "0LLLLLLLLLLLLLLLLLLLLLLLLL"}, + {"uppercase U", "0UUUUUUUUUUUUUUUUUUUUUUUUU"}, + {"uppercase O", "0OOOOOOOOOOOOOOOOOOOOOOOOO"}, + {"uppercase I", "0IIIIIIIIIIIIIIIIIIIIIIIII"}, + {"lowercase l", "0lllllllllllllllllllllllll"}, + {"lowercase u", "0uuuuuuuuuuuuuuuuuuuuuuuuu"}, + {"lowercase o", "0ooooooooooooooooooooooooo"}, + {"lowercase i", "0iiiiiiiiiiiiiiiiiiiiiiiii"}, + {"first char I", "I0000000000000000000000000"}, + {"first char L", "L0000000000000000000000000"}, + {"first char O", "O0000000000000000000000000"}, + {"first char U", "U0000000000000000000000000"}, + {"symbol space", "0000XSNJG0MQJHBF4QX1EFD6Y "}, + {"symbol hyphen", "0000XSNJG0MQ-HBF4QX1EFD6Y3"}, + {"symbol at", "0000XSNJG0MQ@HBF4QX1EFD6Y3"}, + } + + for _, tc := range issue69Cases { + t.Run(tc.name, func(t *testing.T) { + if _, err := ulid.Parse(tc.input); err != ulid.ErrInvalidCharacters { + t.Errorf("Parse(%q): got err %v, want %v", tc.input, err, ulid.ErrInvalidCharacters) + } + if _, err := ulid.ParseStrict(tc.input); err != ulid.ErrInvalidCharacters { + t.Errorf("ParseStrict(%q): got err %v, want %v", tc.input, err, ulid.ErrInvalidCharacters) + } + + var id ulid.ULID + if err := id.UnmarshalText([]byte(tc.input)); err != ulid.ErrInvalidCharacters { + t.Errorf("UnmarshalText(%q): got err %v, want %v", tc.input, err, ulid.ErrInvalidCharacters) + } + }) + } + + // Verify all positions 0..25 reject excluded Crockford Base32 characters. + base := "0000XSNJG0MQJHBF4QX1EFD6Y3" + excluded := []byte{'I', 'L', 'O', 'U', 'i', 'l', 'o', 'u'} + for i := 0; i < ulid.EncodedSize; i++ { + for _, ch := range excluded { + input := base[:i] + string(ch) + base[i+1:] + if _, err := ulid.Parse(input); err != ulid.ErrInvalidCharacters { + t.Errorf("Parse(%q) at index %d: got err %v, want %v", input, i, err, ulid.ErrInvalidCharacters) + } + } + } +} + func TestAlizainCompatibility(t *testing.T) { t.Parallel() @@ -324,8 +389,8 @@ func TestParseRobustness(t *testing.T) { } for _, tc := range cases { - if _, err := ulid.Parse(string(tc)); err != nil { - t.Error(err) + if _, err := ulid.Parse(string(tc)); err != ulid.ErrInvalidCharacters { + t.Errorf("got %v, want %v", err, ulid.ErrInvalidCharacters) } } @@ -337,18 +402,8 @@ func TestParseRobustness(t *testing.T) { } }() - // quick.Check doesn't constrain input, - // so we need to do so artificially. - if s[0] > '7' { - s[0] %= '7' - } - - var err error - if _, err = ulid.Parse(string(s[:])); err != nil { - t.Error(err) - } - - return err == nil + _, _ = ulid.Parse(string(s[:])) + return true } err := quick.Check(prop, &quick.Config{MaxCount: 1e4})