Security, stability & performance quick wins from codebase audit - #184
Open
Sparksx wants to merge 1 commit into
Open
Security, stability & performance quick wins from codebase audit#184Sparksx wants to merge 1 commit into
Sparksx wants to merge 1 commit into
Conversation
Security: - Make gold/diamonds/essence/forgeLevel tamper-resistant: server only accepts values that decreased (spending) or forge level that increased, preventing clients from inflating currency - Fix refund gold clawback to use atomic transaction with decrement instead of racy read-then-write pattern - Fix admin gold/essence/diamonds adjustments to use atomic transactions Stability: - Remove competing SIGINT/SIGTERM handlers from prisma.js (index.js handles shutdown in the correct order: Socket.IO → HTTP → Prisma) - Fix trashItem() to actually clear the equipment slot, removing the need for home.js to directly mutate state internals - Add negative amount guard to spendGold() Performance: - Add Cache-Control header to monster templates endpoint (matches equipment and player template endpoints) - Create lightweight LIST_CLAN_INCLUDE for clan list endpoint, avoiding loading full gameState JSON blobs for 25 clans × N members Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PakRpNc4HcG1sRQwy3h249
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.
Summary
Full codebase audit across security, performance, stability, and functionality — 44 findings identified, 8 quick wins implemented in this PR.
Security (Critical + High fixes)
PUT /api/game/statenow validates gold/diamonds/essence can only decrease (client spending) and forgeLevel can only increase. Clients can no longer inflate currency by sending arbitrary values.$transactionwithdecrementinstead of a racy read-then-write that could duplicate or lose gold under concurrent access.increment/decrementinstead of read-then-write.Stability
prisma.jsthat raced withindex.jsshutdown sequence, potentially callingprocess.exit(0)before HTTP/Socket.IO cleanup.trashItem()state mutation —trashItem()now correctly setsequipment[type] = null, removing the need forhome.jsto directly mutate state internals (violating model/view separation).spendGold()against negative amounts — Passing a negative amount no longer increases gold.Performance
Cache-Control: public, max-age=3600header (matching equipment and player template endpoints).LIST_CLAN_INCLUDEforGET /api/clansthat skips loading fullplayerandupdatedAtdata for every member of every clan in the top-25 list.Test plan
npm test— 524 tests passnpm run lint— 0 errors (pre-existing warnings only)npm run build— clean production buildGenerated by Claude Code