Replace Math Random by Using Crypto Random#126
Conversation
- [+] refactor(random): replace math/rand with crypto/rand for secure random number generation - [+] feat(random): add GetRandomString and GetRandomNumberString functions for secure string generation - [+] fix(random): update GenerateKey function to use crypto/rand for random character selection
WalkthroughReplaced math/rand/time seeding with crypto/rand + math/big for all random utilities. GenerateKey now constructs a 48-char key (16 crypto-rand chars + 32 UUID-derived chars). Added GetRandomString and GetRandomNumberString. RandRange now uses crypto/rand. Added tests for uniqueness and distribution. Changes
Sequence Diagram(s)sequenceDiagram
actor Caller
participant Random as random package
participant CRand as crypto/rand
participant UUID as GetUUID
Caller->>Random: GenerateKey()
activate Random
loop 16 times
Random->>CRand: rand.Int(big.NewInt(len(keyChars)))
CRand-->>Random: index
Random->>Random: append keyChars[index]
end
Random->>UUID: GetUUID()
UUID-->>Random: uuid (no hyphens)
Random->>Random: take/transform 32 UUID chars (uppercase for even indices)
Random-->>Caller: 48-char key
deactivate Random
sequenceDiagram
actor Caller
participant Random as random package
participant CRand as crypto/rand
Caller->>Random: GetRandomString(n) / GetRandomNumberString(n)
activate Random
loop n times
Random->>CRand: rand.Int(big.NewInt(len(charset)))
CRand-->>Random: index
Random->>Random: append charset[index]
end
Random-->>Caller: generated string
deactivate Random
Caller->>Random: RandRange(min, max)
activate Random
Random->>CRand: rand.Int(big.NewInt(max-min))
CRand-->>Random: offset
Random-->>Caller: min + offset
deactivate Random
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
common/random/main.go (3)
11-12: Docstring LGTM; minor: prefer strings.ReplaceAll over Replace with -1.Small readability nit: Replace
strings.Replace(..., -1)withstrings.ReplaceAll(...).- code = strings.Replace(code, "-", "", -1) + code = strings.ReplaceAll(code, "-", "")Also applies to: 15-15
31-31: Consider avoiding panics in a library package; return errors instead.Panic on RNG failures will crash the process (e.g., during request handling). Exposing
(string, error)or(int, error)returns, or providingMustXxxwrappers, would be more ergonomic and safer in many contexts.If you want, I can draft a non-breaking
Must-style API alongside error-returning variants.Also applies to: 55-55, 71-71, 83-83
46-49: Go naming: consider dropping the “Get” prefix.Idiomatic Go would name these
RandomStringandRandomNumberString. Not blocking, but worth considering if API stability permits.Also applies to: 62-65
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
common/random/main.go(3 hunks)
🔇 Additional comments (1)
common/random/main.go (1)
22-24: Doc clarity looks good.The description matches the actual construction (16 random chars + 32 UUID chars => 48 total).
- [+] test(crypto_rand): add unit tests for random value generation and distribution
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
common/random/crypto_rand_test.go (1)
53-53: Consider adjusting the duplicate rate threshold.The 0.1% (0.001) duplicate rate for
GetRandomNumberString(10)seems overly strict. With 10 digits and 10 positions, there are 10^10 possible combinations, making duplicates extremely unlikely in 10,000 iterations. The birthday paradox suggests you'd need approximately √(10^10) ≈ 100,000 samples to see meaningful collision probability.Consider either:
- Increasing the duplicate rate threshold to be more realistic
- Reducing iterations for this test case
- Or simply expect strict uniqueness like the 15-character version
- allowDupsRate: 0.001, // Allow 0.1% duplicates due to limited numeric space + expectedUniq: true, // 10^10 possible combinations make duplicates extremely unlikely
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
common/random/crypto_rand_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
common/random/crypto_rand_test.go (1)
common/random/main.go (5)
GetUUID(13-17)GenerateKey(25-44)GetRandomString(49-60)GetRandomNumberString(65-76)RandRange(79-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (5)
common/random/crypto_rand_test.go (5)
1-7: LGTM! Clean test package structure.The test package follows Go conventions with proper import organization and clear separation from the implementation package.
9-63: Comprehensive uniqueness testing with proper statistical considerations.The table-driven test structure effectively covers all random generation functions with appropriate expectations for each. The distinction between strict uniqueness for UUID/GenerateKey and allowable duplicate rates for numeric strings shows good understanding of the mathematical constraints.
65-97: Excellent test execution with clear error reporting.The test logic correctly tracks duplicates, calculates rates, and provides informative error messages and logging. The conditional checking based on
expectedUniqandallowDupsRatehandles both strict and probabilistic uniqueness requirements appropriately.
99-136: Well-designed distribution test with proper statistical validation.The distribution test effectively validates that
RandRangeproduces evenly distributed results within the expected range. The 10% tolerance is reasonable for statistical variation, and the logging provides valuable debugging information.
112-115: RandRange boundary behavior is correct and tests align with implementation
TheRandRangefunction incommon/random/main.go(lines 79–86) usesrand.Int(rand.Reader, big.NewInt(int64(max-min))), which returns a value in [0, max-min). Addingminyields a result in [min, max), exactly matching the test’s check incommon/random/crypto_rand_test.go. No changes are needed.
- [+] test(crypto_rand): increase iterations from 10,000 to 100,000 for uniqueness tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
common/random/crypto_rand_test.go (5)
9-63: Cut test runtime: gate heavy iterations behind testing.Short and reuse a baseIterSix generators × 100k iterations each triggers millions of crypto/rand draws and large maps, which can slow CI. Make iterations configurable and honor -short.
Apply this diff:
func TestUniqueness(t *testing.T) { - // Table-driven test structure for uniqueness testing + // Table-driven test structure for uniqueness testing + baseIter := 100_000 + if testing.Short() { + baseIter = 10_000 + } tests := []struct { name string generator func() string iterations int expectedUniq bool allowDupsRate float64 // Allow this rate of duplicates (for functions with limited output space) }{ { name: "GetUUID should always generate unique values", generator: random.GetUUID, - iterations: 100000, + iterations: baseIter, expectedUniq: true, }, { name: "GenerateKey should always generate unique values", generator: random.GenerateKey, - iterations: 100000, + iterations: baseIter, expectedUniq: true, }, { name: "GetRandomString(10) should generate unique values", generator: func() string { return random.GetRandomString(10) }, - iterations: 100000, + iterations: baseIter, expectedUniq: true, }, { name: "GetRandomString(20) should generate unique values", generator: func() string { return random.GetRandomString(20) }, - iterations: 100000, + iterations: baseIter, expectedUniq: true, }, { name: "GetRandomNumberString(10) should generate mostly unique values", generator: func() string { return random.GetRandomNumberString(10) }, - iterations: 100000, + iterations: baseIter, expectedUniq: false, - allowDupsRate: 0.001, // Allow 0.1% duplicates due to limited numeric space + allowDupsRate: 0.0001, // Allow 0.01% duplicates; generous vs. birthday bound (~0.0005%) }, { name: "GetRandomNumberString(15) should generate mostly unique values", generator: func() string { return random.GetRandomNumberString(15) }, - iterations: 100000, + iterations: baseIter, expectedUniq: true, }, }
51-54: Tighten duplicate threshold for 10-digit numeric stringsFor 100k samples over 10^10 possibilities, the expected duplicate count is ~0.5 (birthday bound), i.e., ~0.0005% rate. The current 0.1% threshold (≈100 duplicates) is so lax it would miss serious regressions. Suggest 0.01% (≈10 duplicates) to stay extremely safe while still non-flaky.
If you prefer not to touch base iterations, you can apply just this narrower change:
- allowDupsRate: 0.001, // Allow 0.1% duplicates due to limited numeric space + allowDupsRate: 0.0001, // Allow 0.01% duplicates; still far above expectation
67-79: Minor memory win: use map[string]struct{} as a setAvoids storing booleans and saves a word per entry in large maps.
- // Use a map to track unique values - seen := make(map[string]bool, tt.iterations) + // Use a map as a set to track unique values + seen := make(map[string]struct{}, tt.iterations) duplicates := 0 // Generate values and check for duplicates for i := 0; i < tt.iterations; i++ { val := tt.generator() - if seen[val] { + if _, ok := seen[val]; ok { duplicates++ } else { - seen[val] = true + seen[val] = struct{}{} } }
99-136: Honor -short and keep the distribution test crispThis test is solid and non-flaky with a wide 10% tolerance. Still, 100k iterations are heavy. Make it scale down for -short to speed CI.
func TestRandRangeDistribution(t *testing.T) { // For RandRange, we're more interested in distribution than uniqueness min, max := 1, 10 - iterations := 100000 + iterations := 100_000 + if testing.Short() { + iterations = 10_000 + }Optional: if you want the test to assert distribution more strictly without flakiness, reduce tolerance to 5% only when iterations >= 100k.
9-97: Add negative tests for RandRange(min,max) invalid inputsRandRange uses crypto/rand.Int with modulus (max-min); when max <= min this panics via the error path. Add tests to document and lock this behavior.
Proposed additions:
func TestRandRangeInvalidInputs(t *testing.T) { t.Parallel() cases := [][2]int{ {0, 0}, {5, 5}, {10, 3}, } for _, c := range cases { min, max := c[0], c[1] func() { defer func() { if r := recover(); r == nil { t.Fatalf("expected panic for RandRange(%d,%d), got none", min, max) } }() _ = random.RandRange(min, max) }() } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
common/random/crypto_rand_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
common/random/crypto_rand_test.go (1)
common/random/main.go (5)
GetUUID(13-17)GenerateKey(25-44)GetRandomString(49-60)GetRandomNumberString(65-76)RandRange(79-86)
🔇 Additional comments (2)
common/random/crypto_rand_test.go (2)
1-7: External tests look goodUsing package random_test and importing the module path exercises only the public API and prevents accidental reliance on internals. LGTM.
9-63: Fix invalidfor … rangeloops in common/random/main.goTwo loops in
common/random/main.goattempt to range over integer literals, which will not compile. Update them to standard index-based loops:• In
GenerateKey()(around line 27):- for i := range 16 { + for i := 0; i < 16; i++ {• In the UUID-to-byte conversion (around line 36):
- for i := range 32 { + for i := 0; i < 32; i++ {Alternatively, if you meant to loop over the length of a slice (e.g.
keyoruuid), usefor i := range key { … }orfor i := range uuid { … }. These changes are required to resolve compile errors.Likely an incorrect or invalid review comment.
- [+] docs(random.go): update comments to include references for uuid and crypto/rand packages
2 participants
This PR Related #125
Summary by CodeRabbit
New Features
Refactor
Tests