Skip to content

gateway: harden and simplify MCP gateway code#2133

Open
dgageot wants to merge 1 commit intodocker:mainfrom
dgageot:board/look-at-the-mcp-gateway-code-and-find-pl-033c022b
Open

gateway: harden and simplify MCP gateway code#2133
dgageot wants to merge 1 commit intodocker:mainfrom
dgageot:board/look-at-the-mcp-gateway-code-and-find-pl-033c022b

Conversation

@dgageot
Copy link
Member

@dgageot dgageot commented Mar 16, 2026

Summary

Harden and simplify the MCP gateway code across pkg/gateway and pkg/tools/mcp.

Changes

  • Remove loadCatalog indirection — it accepted a context but ignored it, just forwarding to catalogOnce(). Call catalogOnce() directly.
  • Pass secrets directly to NewGatewayToolset — eliminates a redundant ServerSpec round-trip; the caller already has the server spec.
  • Fix temp file leak in writeTempFile — on write or close error, the temp file is now removed. Close errors are no longer silently dropped.
  • Log cleanup warnings scoped to temp file errorsGatewayToolset.Stop previously logged warnings for routine process-exit errors too.
  • Reject secret values containing newlines — prevents injection in the line-based secrets file format.
  • Use dedicated http.Client — isolates catalog fetching from any mutations to http.DefaultClient.
  • Lazy MkdirAll in saveToDisk — try CreateTemp first; only create the directory on ENOENT.
  • Merge servers.go into catalog.goParseServerRef was the only function; reduces file count.
  • Explicit catalogJSON constant — replaces fragile strings.Replace derivation.
  • Network-independent tests — inject a self-contained test catalog via TestMain instead of hitting the live Docker MCP catalog.

- 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
@dgageot dgageot requested a review from a team as a code owner March 16, 2026 18:19
Copy link

@docker-agent docker-agent bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

1 participant