Skip to content

Harden security, performance, and stability (quick wins) - #192

Open
Sparksx wants to merge 1 commit into
mainfrom
claude/focused-curie-yght5p
Open

Harden security, performance, and stability (quick wins)#192
Sparksx wants to merge 1 commit into
mainfrom
claude/focused-curie-yght5p

Conversation

@Sparksx

@Sparksx Sparksx commented Jul 30, 2026

Copy link
Copy Markdown
Owner

Summary

Comprehensive codebase audit across security, performance, stability, and functionality — with immediate implementation of the highest-priority quick wins (low difficulty, high impact).

Security fixes

  • Enable Content Security Policy (CSP) — was fully disabled (contentSecurityPolicy: false), now configured with proper directives for self, Stripe JS/API, WebSocket, and dev localhost
  • Distinct JWT dev secretsJWT_SECRET and JWT_REFRESH_SECRET shared the same fallback value, meaning an access token could be verified as a refresh token in dev
  • Pin JWT algorithm to HS256 — all jwt.verify() calls now explicitly restrict to HS256, preventing algorithm confusion attacks
  • Rate limit /refresh endpoint — was the only auth endpoint without rate limiting, enabling unlimited token generation from a stolen refresh token
  • Limit settings payload sizePUT /api/auth/settings accepted arbitrary JSON with no size cap; now limited to 20 keys / 4KB
  • Username character-set validation — registration and username change now enforce [A-Za-z0-9_-] to prevent homoglyph impersonation and unicode abuse

Performance fixes

  • Stripe singletonnew Stripe(...) was called on every payment request; now cached as a lazy singleton
  • Database index on RefreshToken.expiresAt — the 24h cleanup query was doing a full table scan
  • Cache-Control on monster templates — was missing, unlike equipment and player templates which already had max-age=3600
  • Immutable cache headers for hashed assets — Vite-built /assets/* files now served with max-age=1y, immutable
  • Remove unnecessary save() from trashItem — the function emitted events but triggered a wasted localStorage write + server save despite not modifying state

Audit findings (remaining backlog)

The full audit identified additional improvements not implemented in this PR, ordered by priority:

Priority Category Issue Difficulty
HIGH Security Client can set arbitrary gold/combat/forge values (anti-cheat gap) High
HIGH Security Banned users can still use all HTTP API routes Medium
HIGH Performance Clan listing N+1: loads all members + gameState for 25 clans Medium
HIGH Performance No application-level caching for templates/leaderboard Medium
MEDIUM Performance Sequential DB updates in expedition resolve loop Medium
MEDIUM Performance No compression middleware on Express Low
MEDIUM Stability Race condition on completeMission (double-complete possible) Medium
MEDIUM Performance PvP opponent pool fetches 100 users with full state Medium
MEDIUM Security Socket admin events use cached role (stale after demotion) Low
LOW Security Refresh tokens stored as plaintext in DB Medium
LOW Performance broadcastConversation iterates all connected sockets Medium
LOW Stability Math.random() in transaction (non-deterministic on retry) Low

Test plan

  • All 524 tests pass (npm test)
  • ESLint: 0 errors (53 pre-existing warnings unchanged)
  • Vite production build succeeds (npm run build)
  • CSP directives allow Stripe checkout, WebSocket connections, and inline styles
  • JWT verification still works with pinned HS256 algorithm
  • Rate limiter on /refresh uses same 5/min window as other auth endpoints
  • Username validation rejects special characters while allowing _ and -

Generated by Claude Code

Security:
- Enable Content Security Policy (CSP) with directives for self, Stripe, and WebSocket
- Use distinct JWT dev fallback secrets to prevent access/refresh token confusion
- Pin JWT verification to HS256 algorithm (defense-in-depth)
- Add rate limiting on the /refresh endpoint to prevent token abuse
- Limit settings payload size (4KB max, 20 keys max)
- Add username character-set validation (alphanumeric + underscore/dash only)

Performance:
- Cache Stripe SDK instance as singleton instead of re-creating per request
- Add database index on RefreshToken.expiresAt for periodic cleanup queries
- Add Cache-Control header on monster templates (matching equipment/player templates)
- Serve Vite hashed assets with immutable cache headers (1 year max-age)
- Remove unnecessary save() call from trashItem (no state mutation occurs)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0184XFRfwnkhdecjbHTgvMwV
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