Skip to content

Fix 9 critical code review issues: validation, error handling, and security#42

Merged
pardeike merged 3 commits intomainfrom
copilot/fix-41
Sep 14, 2025
Merged

Fix 9 critical code review issues: validation, error handling, and security#42
pardeike merged 3 commits intomainfrom
copilot/fix-41

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Sep 14, 2025

This PR addresses 9 critical issues identified in the AI code review, implementing minimal surgical fixes to improve robustness and security without breaking existing functionality.

Issues Fixed

1. Non-interactive game validation failure

  • Modified GameConfig.Validate() to allow empty Target for DirectPath mode in automated environments
  • Enables gabs games add to work in CI/CD pipelines and scripts
  • Updated tests to reflect new validation logic

2. CLI references non-existent command

  • Removed reference to unimplemented gabs games edit command in success messages
  • Updated message to guide users to manual configuration instead

3. HTTP server startup error handling

  • Fixed ServeHTTP to properly propagate startup errors instead of swallowing them
  • Server now exits with proper error codes when HTTP binding fails
  • Prevents silent failures that could confuse users

4. Connection leak in writer cleanup

  • Fixed writer slice cleanup logic that could leak connections when multiple clients disconnect
  • Changed from index-based removal to value-based search to handle concurrent disconnections safely

5. Memory safety in config access

  • Updated GetGame() to return a safe copy instead of a pointer to map value
  • Prevents accidental modifications that wouldn't persist to the underlying map

6. Input validation for backoff parameters

  • Added validation for negative durations and min > max ordering in parseBackoff
  • Prevents configuration errors that could cause unexpected behavior

7. Error visibility in process restart

  • Modified Controller.Restart() to log stop errors while continuing with restart
  • Improves debugging without breaking restart functionality when processes are already dead

8. HTTP request body size limits

  • Added 1MB size limit to HTTP MCP request bodies using http.MaxBytesReader
  • Prevents potential memory exhaustion attacks via large request bodies

9. Documentation clarity for Steam/Epic games

  • Enhanced README to emphasize that stopProcessName is required for Steam/Epic games
  • Makes it clear this is critical for proper game termination, not optional

Testing

  • All existing tests pass
  • Added new test for empty Target validation
  • Manual testing confirms:
    • Backoff validation rejects invalid inputs
    • Non-interactive game addition works correctly
    • HTTP server errors are properly reported
    • CLI help no longer references non-existent commands

Impact

These changes improve the robustness and security of GABS while maintaining full backward compatibility. The fixes are minimal and surgical, reducing regression risk while addressing real issues that could affect production deployments.

Fixes #41.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI and others added 2 commits September 14, 2025 19:41
- Fix Issue 1: Allow empty Target for DirectPath in automated environments
- Fix Issue 2: Remove reference to non-existent 'gabs games edit' command
- Fix Issue 3: Propagate HTTP server startup errors properly
- Fix Issue 4: Fix writer cleanup to avoid connection leaks
- Fix Issue 5: Return GameConfig copy (safer than map reference)
- Fix Issue 6: Add validation for negative durations and min > max in parseBackoff
- Fix Issue 7: Log stop errors in Controller.Restart but continue
- Fix Issue 8: Add 1MB size limit to HTTP request body reads

Co-authored-by: pardeike <853584+pardeike@users.noreply.github.com>
Fix Issue 9: Enhanced README documentation to make stopProcessName requirement more prominent for Steam/Epic games, emphasizing it's critical for proper game termination.

Co-authored-by: pardeike <853584+pardeike@users.noreply.github.com>
Copilot AI changed the title [WIP] Code review Fix 9 critical code review issues: validation, error handling, and security Sep 14, 2025
Copilot AI requested a review from pardeike September 14, 2025 19:44
@pardeike pardeike marked this pull request as ready for review September 14, 2025 19:44
@pardeike pardeike merged commit da430f2 into main Sep 14, 2025
3 checks passed
@pardeike pardeike deleted the copilot/fix-41 branch September 14, 2025 19:44
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.

Code review

2 participants