Skip to content

Fix for code scanning alert no. 1: Insecure randomness#11

Draft
feuvan wants to merge 3 commits into
mainfrom
alert-autofix-1
Draft

Fix for code scanning alert no. 1: Insecure randomness#11
feuvan wants to merge 3 commits into
mainfrom
alert-autofix-1

Conversation

@feuvan

@feuvan feuvan commented Mar 28, 2026

Copy link
Copy Markdown
Owner
  • Identified build failure: Node's import * as crypto from 'crypto' doesn't work in this browser-based Vite/Phaser project
  • Replaced Node crypto import with browser Web Crypto API (globalThis.crypto.getRandomValues)
  • Extracted verbose inline expression into a small randomHex(bytes) helper for readability
  • Build passes (tsc && vite build)
  • All 2953 tests pass

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@feuvan

feuvan commented Mar 28, 2026

Copy link
Copy Markdown
Owner Author

@copilot check build and behavior

Copilot AI commented Mar 28, 2026

Copy link
Copy Markdown

@copilot check build and behavior

The original fix used Node's import * as crypto from 'crypto', which caused a build failure (TS2307: Cannot find module 'crypto') because this is a browser-based Phaser/Vite project, not Node.js.

Fixed in ce38087 by replacing it with the browser's Web Crypto API:

uid: `gem_${Date.now()}_${Array.from(globalThis.crypto.getRandomValues(new Uint8Array(4))).map(b => b.toString(16).padStart(2, '0')).join('')}`,
  • ✅ Build passes (tsc && vite build)
  • ✅ All 2953 tests pass
  • ✅ Generates 8 cryptographically secure hex characters (32 bits entropy), same structure as before

Copilot AI changed the title Potential fix for code scanning alert no. 1: Insecure randomness Fix for code scanning alert no. 1: Insecure randomness Mar 28, 2026
Comment thread src/systems/InventorySystem.ts Outdated
const gemBase = getItemBase(gem.gemId);
const gemItem: ItemInstance = {
uid: `gem_${Date.now()}_${Math.random().toString(36).substring(2, 8)}`,
uid: `gem_${Date.now()}_${Array.from(globalThis.crypto.getRandomValues(new Uint8Array(4))).map(b => b.toString(16).padStart(2, '0')).join('')}`,

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot The change looks ugly

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted the logic into a small randomHex helper (05ae3ea), so the UID line is now just:

uid: `gem_${Date.now()}_${randomHex(4)}`,

@feuvan feuvan left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot does phaser or node for browser provide proper random function?

Copilot AI commented Mar 28, 2026

Copy link
Copy Markdown

@copilot check build and behavior

✅ Build passes (tsc && vite build) and all 2953 tests pass with the current changes (commit 05ae3ea).

@copilot does phaser or node for browser provide proper random function?

Neither Phaser nor a Node-for-browser polyfill is needed here. Phaser's RNG (Phaser.Math.RND, Phaser.Math.Between, etc.) is a seeded XorShift128 PRNG — fast and deterministic but not cryptographically secure, so it wouldn't satisfy the security alert.

The correct browser-native solution is exactly what's already in place: globalThis.crypto.getRandomValues (the Web Crypto API). It's built into every modern browser, requires no import or polyfill, and is the same API used by password managers and TLS — genuinely cryptographically secure. There's nothing extra to add.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants