Skip to content

[security-fix] Fix unhandled errors in mcp_inspect.go cleanup operations#7969

Merged
pelikhan merged 1 commit into
mainfrom
main-310d1c583f079db9
Dec 28, 2025
Merged

[security-fix] Fix unhandled errors in mcp_inspect.go cleanup operations#7969
pelikhan merged 1 commit into
mainfrom
main-310d1c583f079db9

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Security Fix: Unhandled Errors in Temporary Directory Cleanup

Alert Number: #407
Severity: Low (Warning)
Rule: G104 - Errors unhandled

Vulnerability Description

The security scan identified two instances in pkg/cli/mcp_inspect.go where os.RemoveAll(tmpDir) is called without checking for errors. This violates proper error handling practices and can lead to:

  1. Silent failures: Cleanup failures go unnoticed, potentially leaving temporary directories behind
  2. Resource leaks: Uncleaned temporary directories can accumulate over time
  3. Debugging difficulties: Errors during cleanup are not logged, making it harder to diagnose issues

The unhandled errors were found at:

  • Line 514: When failing to find an available port for the HTTP server
  • Line 537: When the safe-inputs HTTP server fails to start within timeout

Fix Applied

Added proper error handling for both os.RemoveAll(tmpDir) calls:

if err := os.RemoveAll(tmpDir); err != nil && verbose {
    mcpInspectLog.Printf("Warning: failed to clean up temporary directory %s: %v", tmpDir, err)
}

This approach:

  • Checks the error returned by os.RemoveAll()
  • Logs a warning message when cleanup fails (in verbose mode)
  • Follows the same pattern used elsewhere in the file (see lines 505-507, 526-528)
  • Maintains backward compatibility - cleanup failures don't block the error flow

Security Best Practices

  1. Always handle errors: Even in cleanup/error paths, errors should be checked
  2. Consistent error handling: Applied the same pattern used elsewhere in the function
  3. Non-critical operations: Since this is cleanup in an error path, we log warnings rather than returning errors
  4. Informative logging: Includes the directory path and error details for debugging

Testing Considerations

  • ✅ Build succeeds without errors
  • ✅ No breaking changes to existing functionality
  • ✅ Error handling is consistent with other cleanup operations in the same file
  • ✅ Minimal, surgical changes - only added error checking

AI generated by Security Fix PR

Added proper error handling for os.RemoveAll(tmpDir) calls at:
- Line 514: When failing to find available port
- Line 537: When server fails to start within timeout

This addresses security alert #407 (G104 - Errors unhandled).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@pelikhan pelikhan marked this pull request as ready for review December 28, 2025 15:58
@pelikhan pelikhan merged commit ec1d5e4 into main Dec 28, 2025
4 checks passed
@pelikhan pelikhan deleted the main-310d1c583f079db9 branch December 28, 2025 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant