-
Notifications
You must be signed in to change notification settings - Fork 3
Description
Summary
During daily compliance review of commit 981ead84c, found one gap in the /close endpoint implementation that deviates from the MCP Gateway Specification v1.7.0.
Recent Changes Reviewed
Commit: 981ead84c373f93d448c1a4f5cf76ccb7cd696a2
PR: #419 - "Sanitize credentials in Docker command logs"
Files Modified:
- ✅
internal/logger/sanitize/sanitize.go- Added credential sanitization (COMPLIANT with spec section 7) - ✅
internal/logger/sanitize/sanitize_test.go- Test coverage for sanitization - ✅
internal/launcher/launcher.go- Sanitized args in logs - ✅
internal/mcp/connection.go- Sanitized args in logs
Note: The PR #419 changes are compliant and improve security. The issue below is a pre-existing gap discovered during systematic review.
Important Issue (MUST violation)
Issue: Close Endpoint Does Not Reject New Requests with 503
Specification Section: 5.1.3 Close Endpoint Behavior
Deep Link: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#513-close-endpoint-behavior
Requirement:
"The gateway MUST perform the following actions when the
/closeendpoint is called:
- Stop Accepting New Requests: Immediately reject any new RPC requests to
/mcp/{server-name}endpoints with HTTP 503 Service Unavailable"
Current State:
In internal/server/handlers.go:26-81, the /close endpoint implementation:
- ✅ Correctly implements idempotency (returns 410 Gone on subsequent calls)
- ✅ Correctly terminates all containers
- ✅ Correctly requires authentication
- ✅ Correctly performs graceful shutdown
- ❌ Does NOT reject new RPC requests with 503 during/after shutdown
The current implementation:
- Sets shutdown state via
InitiateShutdown()(line 55) - Returns 200 OK on first call
- Returns 410 Gone on subsequent calls to
/close - BUT does not prevent or reject requests to
/mcp/{server-name}endpoints with 503
Gap:
After /close is called, the gateway should immediately start rejecting new requests to /mcp/{server-name} endpoints with HTTP 503 Service Unavailable. The current implementation does not have this behavior.
Severity: Important (MUST violation per spec section 5.1.3)
File References:
internal/server/handlers.go:26-81- Close endpoint handlerinternal/server/routed.go:160-168- Close endpoint routinginternal/server/unified.go:850-870- InitiateShutdown implementation- Missing: Middleware or routing logic to check shutdown state and return 503 for
/mcp/*endpoints
Suggested Remediation
Task: Add 503 Response for New Requests During Shutdown
Description: Update the gateway to reject new RPC requests to /mcp/{server-name} endpoints with HTTP 503 Service Unavailable after /close endpoint is called.
Implementation Approach:
-
Add shutdown state check middleware:
- Create middleware in
internal/server/that checksIsShutdown()state - If shutdown is initiated, return HTTP 503 with JSON error response
- Apply middleware to
/mcp/{server-name}route handlers (NOT to/healthor/close)
- Create middleware in
-
Example implementation:
// In internal/server/routed.go or unified.go
func (us *UnifiedServer) rejectIfShutdown(next http.Handler) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if us.IsShutdown() {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusServiceUnavailable)
json.NewEncoder(w).Encode(map[string]string{
"error": "Gateway is shutting down",
})
return
}
next.ServeHTTP(w, r)
})
}
// Apply to MCP endpoints:
mux.Handle("/mcp/", us.rejectIfShutdown(mcpHandler))- Update tests:
- Add test case in
internal/server/to verify 503 response after close - Test that
/healthstill works during shutdown (exempted) - Test that
/closereturns 410 on subsequent calls (existing behavior)
- Add test case in
Specification Reference: https://github.com/githubnext/gh-aw/blob/main/docs/src/content/docs/reference/mcp-gateway.md#513-close-endpoint-behavior
Estimated Effort: Small (2-3 hours)
Compliance Status
| Section | Status | Notes |
|---|---|---|
| ✅ Configuration Format (4.1) | Compliant | All required fields validated |
| ✅ Variable Expansion (4.2) | Compliant | Fail-fast on undefined variables |
| ✅ Containerization (3.2.1) | Compliant | All stdio servers containerized |
| ✅ Protocol Translation (5.2) | Compliant | Stdio ↔ HTTP translation working |
| ✅ Server Isolation (6) | Compliant | Container isolation enforced |
| ✅ Authentication (7) | Compliant | API key auth + credential sanitization |
| Partial | Idempotency works, but missing 503 response | |
| ✅ Health Monitoring (8) | Compliant | /health endpoint with version fields |
| ✅ Error Handling (9) | Compliant | JSON-RPC error format |
| ✅ Custom Server Types (4.1.4) | Compliant | customSchemas field implemented |
| ✅ Volume Mounts (4.1.5) | Compliant | Mount format validated |
References
- MCP Gateway Specification v1.7.0
- Previous review: 2026-01-21 (commit
e039701) - Commits reviewed:
981ead84c(PR Sanitize credentials in Docker command logs #419)
Notes
Positive Findings:
- PR Sanitize credentials in Docker command logs #419 adds excellent credential sanitization that fully complies with spec section 7 requirement "NOT log API keys in plaintext"
- 10 regex patterns covering GitHub PATs, JWT, OAuth, Bearer tokens, hex keys
- Comprehensive test coverage (23 test cases) including leak detection
- No new regressions introduced by recent changes
Known Non-Issues:
MCPGatewaySpecVersionconstant is1.5.0but spec is1.7.0- This was previously reported and marked as "not_planned" by the teamcustomSchemasfield - Previously flagged as missing but now confirmed as implemented ininternal/config/config.go:50
AI generated by Daily Compliance Checker