[security-fix] Fix unhandled error in mcp_inspect.go HTTP server startup (Alert #406)

[github-actions]

Security Fix: Unhandled Error in HTTP Server Startup Error Path

Alert Number: #406
Severity: Low (Warning)
Rule: G104 - Errors unhandled
Tool: gosec (Golang security checks)
Location: pkg/cli/mcp_inspect.go:525

Vulnerability Description

Gosec detected an unhandled error from os.RemoveAll(tmpDir) at line 525. The G104 rule flags situations where errors from function calls are silently ignored, which can lead to:

• Unnoticed failures in critical operations
• Silent resource leaks or incomplete cleanup
• Difficulty in debugging issues
• Violation of Go best practices for error handling

While this specific case is a cleanup operation in an error path (cleaning up a temporary directory after failing to start the HTTP server), proper error handling demonstrates defensive programming and helps maintain code quality.

Fix Applied

Added error handling for the os.RemoveAll(tmpDir) call in the HTTP server startup error path:

Before:

serverCmd, err := startSafeInputsHTTPServer(tmpDir, port, verbose)
if err != nil {
    os.RemoveAll(tmpDir)
    return nil, nil, "", fmt.Errorf("failed to start safe-inputs HTTP server: %w", err)
}

After:

serverCmd, err := startSafeInputsHTTPServer(tmpDir, port, verbose)
if err != nil {
    // Clean up temporary directory on error
    if rmErr := os.RemoveAll(tmpDir); rmErr != nil && verbose {
        mcpInspectLog.Printf("Warning: failed to clean up temporary directory %s: %v", tmpDir, rmErr)
    }
    return nil, nil, "", fmt.Errorf("failed to start safe-inputs HTTP server: %w", err)
}

Security Best Practices Applied

✅ Error Handling: Properly checks and handles error return values
✅ User Visibility: Logs warning when cleanup fails (in verbose mode)
✅ Non-Breaking: Cleanup failure doesn't affect the main error return path
✅ Defensive Programming: Follows Go best practices for error handling
✅ G104 Compliance: Satisfies gosec security scanner requirements

Testing

✅ Build succeeded: go build ./pkg/cli/... passes without errors
✅ No breaking changes: Normal operation flow unchanged
✅ Error visibility: Cleanup failures are logged in verbose mode
✅ Non-critical handling: Cleanup failure doesn't interrupt the error return path

Impact Assessment

Risk: Minimal
Breaking Changes: None
Backwards Compatibility: Full
Performance: No impact

The fix only adds error checking for a cleanup operation in an error path. The main function behavior remains unchanged - it still returns an error when the HTTP server fails to start, whether or not the cleanup succeeds. The warning message is only shown in verbose mode.

Why This Fix Is Important

While this specific error is non-critical (cleanup in error path), properly handling errors:

1. Improves debugging: Helps diagnose filesystem permission issues or disk space problems
2. Follows Go conventions: Error values should never be silently ignored
3. Maintains code quality: Demonstrates proper defensive programming
4. Satisfies security scanners: Eliminates gosec G104 alert
5. Sets good precedent: Shows correct error handling pattern for similar cases

Files Modified

• pkg/cli/mcp_inspect.go:
  • Line 525-528: Added error handling with warning message for cleanup failure
  • Maintains existing error return flow

References

• CodeQL Alert: https://github.com/githubnext/gh-aw/security/code-scanning/406
• Gosec G104: Audit errors not checked
• Go Error Handling: [Effective Go - Errors]((redacted)

---

🤖 Generated by Security Fix Agent in workflow run 20543747667

AI generated by Security Fix PR