gateway: harden and simplify MCP gateway code#2133
Conversation
- Remove unnecessary loadCatalog indirection; call catalogOnce() directly - Pass secrets directly to NewGatewayToolset to avoid redundant ServerSpec lookups - Fix temp file leak in writeTempFile on write/close error - Log cleanup warnings only for temp file removal failures, not routine stop errors - Reject secret values containing newlines to prevent injection in secrets file - Use dedicated http.Client for catalog fetches instead of http.DefaultClient - Avoid unconditional MkdirAll in saveToDisk; create directory only when needed - Merge one-liner servers.go into catalog.go - Make catalogJSON an explicit constant instead of fragile string replacement - Make catalog tests network-independent using a self-contained test catalog Assisted-By: docker-agent
There was a problem hiding this comment.
Review Summary
Assessment: 🟢 APPROVE
This PR successfully hardens and simplifies the MCP gateway code with several important improvements:
✅ Security improvements: Newline validation prevents injection attacks in secrets files
✅ Resource management: Fixed temp file cleanup on write/close errors in writeTempFile
✅ Code simplification: Removed unnecessary indirection and merged related code
✅ Test reliability: Network-independent tests via injected catalog
✅ Better error handling: Explicit logging of cleanup warnings in Stop()
Minor Observation
While reviewing the temp file lifecycle in NewGatewayToolset, I noticed that if initialization fails after writeConfigToFile succeeds (e.g., if NewToolsetCommand fails), the fileConfig temp file cleanup depends on the caller invoking Stop() on the returned GatewayToolset. If callers don't call Stop() on initialization failures, temp files could accumulate in long-running processes.
Suggestion: Consider documenting that callers must call Stop() even on initialization failures, or use defer patterns to ensure cleanup happens automatically.
This is a minor edge case and doesn't block the PR. The changes are solid improvements to the codebase.
Review performed by docker-agent PR review pipeline
Summary
Harden and simplify the MCP gateway code across
pkg/gatewayandpkg/tools/mcp.Changes
loadCatalogindirection — it accepted a context but ignored it, just forwarding tocatalogOnce(). CallcatalogOnce()directly.NewGatewayToolset— eliminates a redundantServerSpecround-trip; the caller already has the server spec.writeTempFile— on write or close error, the temp file is now removed. Close errors are no longer silently dropped.GatewayToolset.Stoppreviously logged warnings for routine process-exit errors too.http.Client— isolates catalog fetching from any mutations tohttp.DefaultClient.MkdirAllinsaveToDisk— tryCreateTempfirst; only create the directory onENOENT.servers.gointocatalog.go—ParseServerRefwas the only function; reduces file count.catalogJSONconstant — replaces fragilestrings.Replacederivation.TestMaininstead of hitting the live Docker MCP catalog.