client/hooks.go (1)

34-40: Address modulo bias in random character selection.

The current implementation introduces statistical bias because 256 (byte range) doesn't divide evenly by 62 (character set length). Some characters will be slightly more likely to appear than others.

Consider using rejection sampling for uniform distribution:

func unsafeRandString(n int) string {
	b := make([]byte, n)
-	if _, err := rand.Read(b); err != nil {
-		panic(fmt.Errorf("failed to read random bytes: %w", err))
-	}
-
-	for i := 0; i < n; i++ {
-		b[i] = letterBytes[int(b[i])%len(letterBytes)]
-	}
+	
+	for i := 0; i < n; i++ {
+		for {
+			var randomByte [1]byte
+			if _, err := rand.Read(randomByte[:]); err != nil {
+				panic(fmt.Errorf("failed to read random bytes: %w", err))
+			}
+			// Reject values that would cause bias
+			if randomByte[0] < 248 { // 248 = 62 * 4, largest multiple of 62 <= 255
+				b[i] = letterBytes[randomByte[0]%62]
+				break
+			}
+		}
+	}

	return utils.UnsafeString(b)
}

Alternatively, if perfect uniformity isn't critical for boundary generation, the current approach is acceptable and much simpler.

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 35-35: client/hooks.go#L35
Added line #L35 was not covered by tests

client/hooks_test.go (1)

46-52: Good test addition for character set validation.

The new subtest effectively verifies that all generated characters belong to the expected character set, which is important given the implementation change. The parallel execution is also appropriate.

Consider adding a test for the error handling path to improve coverage:

t.Run("crypto rand error handling", func(t *testing.T) {
	// This would require mocking crypto/rand.Read to return an error
	// or testing in an environment where crypto/rand fails
	// The current panic behavior may be difficult to test directly
})

Note: The static analysis tool flagged that the panic path (line 35 in hooks.go) lacks test coverage.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

client/hooks.go:33

• Update this doc comment to describe the function’s behavior, including its parameter (n), the returned random string, and the possible error condition.

// An error is returned if the random source fails.

client/hooks.go:34

• [nitpick] Since this function now uses a secure source and returns an error, consider renaming unsafeRandString to something like secureRandString or randString for clarity.

func unsafeRandString(n int) (string, error) {

client/hooks.go (1)

34-46: Excellent security improvement with minor modernization opportunity.

The switch to crypto/rand provides cryptographically secure randomness and eliminates potential bias. The pre-computed bound optimization is well-implemented.

Consider modernizing the for loop to address the static analysis hint:

-	for i := 0; i < n; i++ {
-		idx, err := rand.Int(rand.Reader, bound)
-		if err != nil {
-			return "", fmt.Errorf("failed to read random index: %w", err)
-		}
-		b[i] = letterBytes[idx.Int64()]
-	}
+	for i := range n {
+		idx, err := rand.Int(rand.Reader, bound)
+		if err != nil {
+			return "", fmt.Errorf("failed to read random index: %w", err)
+		}
+		b[i] = letterBytes[idx.Int64()]
+	}

Note: The significant performance impact (10ns → 300ns per operation) is an acceptable trade-off for the security benefits provided by crypto/rand.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

client/hooks.go (1)

34-47: Good bias-avoidance implementation, but consider optimizing the reading strategy.

The bias-avoidance logic correctly computes the rejection threshold. However, the current approach of reading n bytes upfront and then re-reading individual bytes could be optimized.

Consider reading bytes one at a time to avoid the overhead of re-reading:

 func unsafeRandString(n int) (string, error) {
 	L := byte(len(letterBytes))
 	max := byte(256 - (256 % int(L)))
 	out := make([]byte, n)
-	buf := make([]byte, n)
-
-	// Read n raw bytes in one shot
-	if _, err := rand.Read(buf); err != nil {
-		return "", fmt.Errorf("rand.Read failed: %w", err)
-	}
 
-	for i, b := range buf {
-		// Reject values ≥ max
-		for b >= max {
-			if _, err := rand.Read(buf[i : i+1]); err != nil {
-				return "", fmt.Errorf("rand.Read failed: %w", err)
-			}
-			b = buf[i]
-		}
-		out[i] = letterBytes[b%L]
+	for i := 0; i < n; i++ {
+		var b byte
+		for {
+			if _, err := rand.Read([]byte{b}); err != nil {
+				return "", fmt.Errorf("rand.Read failed: %w", err)
+			}
+			if b < max {
+				break
+			}
+		}
+		out[i] = letterBytes[b%L]
 	}

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-07-01T03:44:03.672Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:20-23
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in both `EncryptCookie` and `DecryptCookie` functions have been added to ensure robust validation and prevent potential runtime errors.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-07-01T03:33:22.283Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.

Learnt from: gaby
PR: gofiber/fiber#3056
File: middleware/encryptcookie/utils.go:51-54
Timestamp: 2024-10-08T19:06:06.583Z
Learning: Unit tests for key length enforcement in `DecryptCookie` have been added to ensure consistency and security in the encryption processes.