Skip to content

Fix security vulnerabilities and code quality issues - #38

Open
HaardenShah wants to merge 1 commit into
mainfrom
claude/gifted-goldberg
Open

Fix security vulnerabilities and code quality issues#38
HaardenShah wants to merge 1 commit into
mainfrom
claude/gifted-goldberg

Conversation

@HaardenShah

Copy link
Copy Markdown
Owner

Summary

  • Fix broken player delete by adding missing CSRF token to fetch request
  • Fix XSS vulnerabilities via innerHTML in live.php and schedule-match.php
  • Add session.cookie_secure flag to prevent cookie leakage over HTTP
  • Validate custom date range inputs with DateTime::createFromFormat
  • Replace insecure mt_rand with random_bytes/random_int for UUID and player code generation
  • Cache redundant method calls in Database::getStats()
  • Escape $messageType in HTML class attributes across admin pages

Test plan

  • Verify player delete works correctly in admin panel (was previously silently broken)
  • Verify live score error messages display correctly
  • Verify scheduled matches list renders properly
  • Verify custom date filtering works on stats pages
  • Verify new player registration generates valid UUIDs

🤖 Generated with Claude Code

- Fix broken player delete by adding missing CSRF token to fetch request
- Fix XSS via innerHTML in live.php error display (use textContent/DOM API)
- Fix XSS via innerHTML in schedule-match.php match list (escape user data)
- Add session.cookie_secure flag to prevent session cookie leakage over HTTP
- Validate custom date range inputs with DateTime::createFromFormat
- Replace insecure mt_rand UUID/code generation with random_bytes/random_int
- Cache redundant getTables()/getDatabaseSize() calls in Database::getStats()
- Escape $messageType in HTML class attributes across admin pages

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@HaardenShah

Copy link
Copy Markdown
Owner Author

Code review

Found 1 issue:

  1. session.cookie_secure set unconditionally to 1 will break authentication on HTTP deployments. The app supports local/LAN HTTP installs (the README and setup wizard assume this), and the rest of the codebase already checks HTTPS conditionally (e.g., InstallCheck.php uses isset($_SERVER['HTTPS']) && $_SERVER['HTTPS'] === 'on'). With cookie_secure=1, the session cookie is never sent over HTTP, silently breaking all admin logins. Fix: make it conditional on HTTPS detection.

ini_set('session.cookie_samesite', 'Lax');
ini_set('session.cookie_secure', '1');

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@HaardenShah HaardenShah self-assigned this Feb 26, 2026
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.

1 participant