fix(types): remove modulo bias from NewID

Reducing crypto/rand bytes with % over a 62-char alphabet favored the first 8
characters (256 % 62 = 8). Reject bytes >= 248 (the largest multiple of 62 that
fits in a byte) so every character is equally likely. IDs stay opaque and the
persisted format is unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Author: gwleclerc

server/types/id.go

@@ -2,25 +2,36 @@ package types
 
 import "crypto/rand"
 
-// idAlphabet is a URL-safe base62 alphabet. idLength is chosen to keep IDs short (like the
-// previous shortid) while staying collision-resistant: 62^12 ≈ 3.2e21 possibilities.
+// idAlphabet is a URL-safe base62 alphabet. idLength keeps IDs short while staying
+// collision-resistant: 62^12 ≈ 3.2e21 possibilities.
 const (
 	idAlphabet = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
 	idLength   = 12
 )
 
-// NewID returns a short, random, URL-safe identifier. It replaces the unmaintained
-// teris-io/shortid with a zero-dependency crypto/rand generator, keeping IDs short (unlike a
-// UUID). IDs are opaque, so the exact alphabet/length is not part of the persisted format.
+// NewID returns a short, random, URL-safe identifier using crypto/rand with no external
+// dependency. IDs are opaque, so the alphabet/length are not part of the persisted format.
 func NewID() string {
-	b := make([]byte, idLength)
-	if _, err := rand.Read(b); err != nil {
-		// crypto/rand.Read does not fail on supported platforms; mirror the previous
-		// shortid.MustGenerate behavior of not returning an error.
-		panic(err)
+	// Largest multiple of the alphabet size that fits in a byte (62*4 = 248). Rejecting bytes
+	// >= limit removes the modulo bias so every character is equally likely.
+	const limit = 256 - (256 % len(idAlphabet))
+	id := make([]byte, 0, idLength)
+	buf := make([]byte, idLength)
+	for len(id) < idLength {
+		if _, err := rand.Read(buf); err != nil {
+			// crypto/rand.Read does not fail on supported platforms; not returning an error
+			// keeps NewID callable from initializers.
+			panic(err)
+		}
+		for _, b := range buf {
+			if int(b) >= limit {
+				continue
+			}
+			id = append(id, idAlphabet[int(b)%len(idAlphabet)])
+			if len(id) == idLength {
+				break
+			}
+		}
 	}
-	for i := range b {
-		b[i] = idAlphabet[int(b[i])%len(idAlphabet)]
-	}
-	return string(b)
+	return string(id)
 }