fix(sse): crypto-secure RNG for combo/deck load-balancing selection (CodeQL #665) - #4455
Merged
Merged
Conversation
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…on (CodeQL #665) CodeQL js/insecure-randomness (#665) flagged Math.random() used for combo target selection (weighted / random / power-of-two-choices), the credential shuffle deck, and shadow-routing sampling as "randomness in a security context". These are provider load-balancing decisions — not secrets, tokens, nonces or session ids — so the finding is a false positive, and the reported sink (targetExhaustion.ts:59, a plain identifier, zero data flow) is degenerate. The cleanest durable fix is to remove the Math.random sources rather than dismiss. New leaf helper src/shared/utils/secureRandom.ts (secureRandomInt / secureRandomFloat) backed by node:crypto, a drop-in for Math.floor(Math.random()*n) / Math.random() with identical ranges. Replaces all 8 selection-path call sites in targetSorters.ts, shadowRouting.ts and shuffleDeck.ts. A test-only _setSecureRandomFloatSource seam (mirroring the existing _resetAllDecks export) lets the deterministic selection tests inject a fixed RNG; secureRandomInt derives from the same float source, so a given injected value selects the exact same index Math.floor(Math.random()*n) would have — the migrated assertions are unchanged. Regression guard: tests/unit/secure-random-routing.test.ts pins the helper ranges and a static check that the selection sources contain no Math.random().
diegosouzapw
force-pushed
the
fix/codeql-insecure-random-combo
branch
from
June 20, 2026 23:51
fb6c0cb to
f52af73
Compare
Contributor
CI Coverage Report
Coverage artifact was not available for this run. |
Merged
tkgo11
pushed a commit
to tkgo11/OmniRoute
that referenced
this pull request
Sep 23, 2026
…iegosouzapw#4455) Replaces Math.random with crypto-secure RNG in combo/deck load-balancing selection (CodeQL diegosouzapw#665). Integrated into release/v3.8.33.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Closes CodeQL alert #665 (
js/insecure-randomness, HIGH).CodeQL flagged
Math.random()used for combo target selection (weighted / random /power-of-two-choices), the credential shuffle deck, and shadow-routing sampling as
"randomness in a security context". These are provider load-balancing decisions —
not secrets, tokens, nonces or session ids — so the finding is a false positive, and
the reported sink (
targetExhaustion.ts:59, a plain destructured identifier with zerodata flow) is degenerate. Rather than dismiss, this removes the
Math.randomsources sothe panel stays clean and the rule cannot re-fire.
How
src/shared/utils/secureRandom.ts—secureRandomInt(n)/secureRandomFloat()backed bynode:crypto, drop-in forMath.floor(Math.random()*n)/Math.random()(identical ranges).targetSorters.ts,shadowRouting.ts,shuffleDeck.ts._setSecureRandomFloatSourceseam (mirrors the existing_resetAllDecksexport) lets the deterministic selection tests inject a fixed RNG.
secureRandomIntderives from the same float source, so a given injected value picks the exact same
index
Math.floor(Math.random()*n)would have — the 5 migrated tests keep theirassertions unchanged (no masking).
Tests (Hard Rule #18 — TDD)
tests/unit/secure-random-routing.test.ts: helper range/distribution + a staticguard that the selection sources contain no
Math.random(). Proven RED→GREEN(stash sources → guard fails; restore → passes).
combo-routing-engine.test.ts(123) + 5 otherMath.random-override suites (40) +new test (5) all green.
typecheck:core,eslint,check:cyclesclean.test:unit16445/16461 andtest:vitest192/193 — the handful of reds arepre-existing timing/env flakes (chatCore-timeout, rate-limit-semaphore,
stream-readiness, quota-recorder, opencode dist-not-built, mcp
auditsqlite-binding);none import this diff and all pass in isolation.
Cherry-pick to
release/v3.8.32follows in a separate PR.