🛡️ Sentinel: [MEDIUM] Fix weak random number generation#320
🛡️ Sentinel: [MEDIUM] Fix weak random number generation#320
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
WalkthroughThis PR replaces insecure Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a medium-severity security vulnerability by replacing instances of Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the use of weak random number generation by replacing all instances of Math.random() with cryptographically secure alternatives from the node:crypto module. The implementation for generating jitter delays and uniqueness nonces is sound and effectively mitigates the described security vulnerability. The changes are well-executed and no issues were found.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| const min = Math.max(0, HYPIXEL_RETRY_DELAY_MIN_MS); | ||
| const max = Math.max(min, HYPIXEL_RETRY_DELAY_MAX_MS); | ||
| return min + Math.random() * (max - min); | ||
| return max > min ? randomInt(min, max + 1) : min; |
There was a problem hiding this comment.
SUGGESTION: Behavioral change in jitter range
The original code used Math.random() * (max - min) which returns a float in [0, max-min), making the result [min, max) - exclusive on max.
The new code uses randomInt(min, max + 1) which returns integers in [min, max] - inclusive on max.
This means the configured maximum retry delay is now achievable (previously it was never exactly reached). This is a minor behavioral difference and arguably more correct, but worth noting.
Code Review SummaryStatus: 1 Suggestion Found | Recommendation: Merge Overview
Issue Details (click to expand)SUGGESTION
Other Observations (not in diff)No other issues found. The fix is complete - verified no remaining Files Reviewed (3 files)
Summary: The security fix is sound. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/src/services/hypixel.ts`:
- Around line 168-171: The jitterDelay function can call randomInt with non-safe
or out-of-range values from HYPIXEL_RETRY_DELAY_MIN_MS /
HYPIXEL_RETRY_DELAY_MAX_MS causing a RangeError; validate and sanitize those
env-derived bounds before calling randomInt. In jitterDelay, coerce both
HYPIXEL_RETRY_DELAY_MIN_MS and HYPIXEL_RETRY_DELAY_MAX_MS to integers, ensure
Number.isSafeInteger for min and max, clamp them to a safe non-negative range
(e.g., at least 0 and no greater than Number.MAX_SAFE_INTEGER or a reasonable
upper limit), ensure max >= min (swap or set max = min), and only then call
randomInt(min, max + 1); if validation fails, fall back to a safe default delay
or return the sanitized min.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 08d9fd00-e9ff-47b2-b9e0-ee4823c6ce47
📒 Files selected for processing (3)
.jules/sentinel.mdbackend/src/services/hypixel.tsbackend/src/services/hypixelTracker.ts
| function jitterDelay(): number { | ||
| const min = Math.max(0, HYPIXEL_RETRY_DELAY_MIN_MS); | ||
| const max = Math.max(min, HYPIXEL_RETRY_DELAY_MAX_MS); | ||
| return min + Math.random() * (max - min); | ||
| return max > min ? randomInt(min, max + 1) : min; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify retry delay sources and lack/presence of bounds before randomInt usage.
rg -n -C3 'function jitterDelay|randomInt\(' backend/src/services/hypixel.ts
rg -n -C3 'HYPIXEL_RETRY_DELAY_MIN_MS|HYPIXEL_RETRY_DELAY_MAX_MS|parseIntEnv' backend/src/config.ts
Repository: beenycool/bedwars-level-head
Length of output: 6768
🏁 Script executed:
# Get the full parseIntEnv implementation
sed -n '221,240p' backend/src/config.ts
Repository: beenycool/bedwars-level-head
Length of output: 510
🏁 Script executed:
# Check what randomInt is and where it's imported from
rg -n 'import.*randomInt|from.*randomInt' backend/src/services/hypixel.ts
Repository: beenycool/bedwars-level-head
Length of output: 114
🏁 Script executed:
# See if there are any existing bounds/validation applied to HYPIXEL_RETRY_DELAY values elsewhere
rg -n -B5 -A5 'HYPIXEL_RETRY_DELAY' backend/src/config.ts
Repository: beenycool/bedwars-level-head
Length of output: 762
🌐 Web query:
Node.js crypto.randomInt constraints RangeError limits
💡 Result:
The Node.js crypto.randomInt([min,] max[, callback]) function generates a cryptographically secure random integer n such that min <= n < max (min defaults to 0 if omitted). It throws a RangeError in these cases: - min or max is not a safe integer (Number.isSafeInteger(min/max) must be true). - max <= min. Error: "The value of 'max' is out of range. It must be greater than the value of 'min' (X). Received Y." - The range (max - min) exceeds 2^48 - 1 (0xFFFFFFFFFFFF, or 281474976710655). Error: "The value of 'max-min' is out of range. It must be <= 281474976710655. Received Z." These constraints ensure uniform distribution without modulo bias, using up to 6 random bytes internally (max 48 bits). Added in Node.js v14.10.0 / v12.19.0. Current as of Node.js v25.x (2026). For larger ranges, use BigInt version or crypto.randomBytes manually.
Citations:
- 1: https://github.com/nodejs/node/blob/main/lib/internal/crypto/random.js
- 2: https://docs.deno.com/api/node/crypto/~/randomInt
- 3: https://runebook.dev/en/articles/node/crypto/cryptorandomintmin-max-callback
- 4: https://www.geeksforgeeks.org/node-js/node-js-crypto-randomint-method/
- 5: crypto: improve randomInt out-of-range error message nodejs/node#35088
- 6: crypto: fix randomInt range check nodejs/node#35052
Guard randomInt inputs to prevent runtime RangeError on misconfigured env values.
At Line 171, randomInt(min, max + 1) throws RangeError if env-derived bounds are not safe integers (Number.isSafeInteger() must be true) or exceed the allowed range. The current code applies Math.max(0, ...) but lacks validation for safe integer bounds, turning misconfigured env values into hard crashes instead of graceful retry delays.
🔧 Proposed hardening
function jitterDelay(): number {
+ const MAX_TIMEOUT_MS = 2_147_483_647; // setTimeout upper bound
+ const toSafeDelay = (value: number): number => {
+ if (!Number.isSafeInteger(value)) return 0;
+ return Math.min(MAX_TIMEOUT_MS, Math.max(0, value));
+ };
+
- const min = Math.max(0, HYPIXEL_RETRY_DELAY_MIN_MS);
- const max = Math.max(min, HYPIXEL_RETRY_DELAY_MAX_MS);
+ const min = toSafeDelay(HYPIXEL_RETRY_DELAY_MIN_MS);
+ const max = Math.max(min, toSafeDelay(HYPIXEL_RETRY_DELAY_MAX_MS));
return max > min ? randomInt(min, max + 1) : min;
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/src/services/hypixel.ts` around lines 168 - 171, The jitterDelay
function can call randomInt with non-safe or out-of-range values from
HYPIXEL_RETRY_DELAY_MIN_MS / HYPIXEL_RETRY_DELAY_MAX_MS causing a RangeError;
validate and sanitize those env-derived bounds before calling randomInt. In
jitterDelay, coerce both HYPIXEL_RETRY_DELAY_MIN_MS and
HYPIXEL_RETRY_DELAY_MAX_MS to integers, ensure Number.isSafeInteger for min and
max, clamp them to a safe non-negative range (e.g., at least 0 and no greater
than Number.MAX_SAFE_INTEGER or a reasonable upper limit), ensure max >= min
(swap or set max = min), and only then call randomInt(min, max + 1); if
validation fails, fall back to a safe default delay or return the sanitized min.
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability:
Math.random()was being used for generating a uniqueness nonce in Redis and jitter offsets for retry delays.Math.random()is not cryptographically secure and produces predictable pseudo-random sequences.🎯 Impact: Attackers could potentially predict nonces or calculate exact retry intervals for timing analysis.
🔧 Fix: Replaced
Math.random()withrandomBytesandrandomIntfrom the built-innode:cryptolibrary to enforce CSPRNG.✅ Verification: Run
cd backend && pnpm test. All tests should pass andMath.random()should no longer be used for these security or infrastructure logic points.PR created automatically by Jules for task 4636760693768330986 started by @beenycool
Summary by CodeRabbit