[security-fix] Fix unhandled error in mcp_inspect.go cleanup (Alert #407)

[github-actions]

Security Fix: Unhandled Error in Temporary Directory Cleanup

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

Vulnerability Description

Gosec detected an unhandled error from os.RemoveAll(tmpDir) at line 504. 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 write files), proper error handling demonstrates defensive programming and helps maintain code quality.

Fix Applied

Added error handling for the os.RemoveAll(tmpDir) call at line 504:

Before:

if err := writeSafeInputsFiles(tmpDir, safeInputsConfig, verbose); err != nil {
    os.RemoveAll(tmpDir)
    return nil, nil, "", fmt.Errorf("failed to write safe-inputs files: %w", err)
}

After:

if err := writeSafeInputsFiles(tmpDir, safeInputsConfig, verbose); err != nil {
    // Clean up temporary directory on error
    if err := os.RemoveAll(tmpDir); err != nil && verbose {
        mcpInspectLog.Printf("Warning: failed to clean up temporary directory %s: %v", tmpDir, err)
    }
    return nil, nil, "", fmt.Errorf("failed to write safe-inputs files: %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 writing safe-inputs files fails, 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 504-507: 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/407
• Gosec G104: Audit errors not checked
• Go Error Handling: [Effective Go - Errors]((redacted)

---

🤖 Generated by Security Fix Agent in workflow run 20534157143

AI generated by Security Fix PR