google / uuid

Go package for UUIDs based on RFC 4122 and DCE 1.1: Authentication and Security Services.
BSD 3-Clause "New" or "Revised" License
5.29k stars 363 forks source link

uuid.Parse Function Does Not Handle Leading/Trailing Spaces in UUIDs #147

Open mahmoudahmedd opened 9 months ago

mahmoudahmedd commented 9 months ago

The uuid.Parse function currently treats UUID strings with leading or trailing spaces as valid.

_, parseError := uuid.Parse(" a3bb189e-8bf9-3888-9912-ace4e6543002 ") when parsing a UUID string like " a3bb189e-8bf9-3888-9912-ace4e6543002 " (with spaces around it), the function does not return an error (parseError is nil). https://go.dev/play/p/8eIO_36dfdD

It would be beneficial to either treat such strings as invalid ?

jensilo commented 2 months ago

Looking at uuid.Parse func and its doc block, this behaviour can be easily explained.

Microsoft's encoding of UUIDs encloses the UUID in { and }. Like this: {xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx}. This UUID then has a length of 38 bytes. To be compatible with Microsoft's UUID encoding uuid.Parse, as the doc block states, only uses the middle 36 bytes of the passed 38 bytes, thereby cutting out the curly braces.

Now, you haven't provided a valid UUID in Microsoft's encoding, however the length is also 38 bytes. Hence, this does only work with ONE space on each side, not multiple. uuid.Parse uses only the length to detect the encoding, which mostly isn't a problem. However, here it leads to uuid.Parse believing your passed in UUID is in Microsoft's encoding. Therefore, it cuts out the first part (37 bytes left to process) and processes the next 36 bytes as a valid UUID. The last byte }, or in your case , is just ignored.

On a funny side note: Due to this behaviour you could enclose your valid UUID in any two bytes. As long as you get to 38 bytes length and have a valid UUID inside. See: https://go.dev/play/p/yIsYOhk4T9Y.

Of course, you could see this as an issue that needs to be fixed, but why? You could have a check that the 38 bytes long UUID in Microsoft's encoding starts with { and ends with } and otherwise have it return an error. I don't see a need for this. The current implementation is faster, as it includes less checks, and works just fine, there are no security-relevant errors to be expected and I couldn't come up with a valid reason for "fixing" this, apart from "it's just right that way".

Btw. your example is weird, you could prove your point with one line: fmt.Println(uuid.MustParse(" a3bb189e-8bf9-3888-9912-ace4e6543002 ")).

jensilo commented 2 months ago

Oh, I just saw this is a duplicate of #60 and has been discussed already.

TL;DR: Changing this behaviour in uuid.Parse could break existing implementations. Therefore, this is so far untouched.

I think this issue can be closed as well.