Skip to content

Commit 2b58387

Browse files
juergen-kcclaude
andcommitted
fix(mcp): wire APIKey through HTTP transport (Bugbot PR #17)
Bugbot flagged (high severity): the help text advertised --api-key for the http transport but the code passed an empty APIKey to SSEConfig, so users following the security guidance would have ended up with an unauthenticated server they believed was protected. - http case now reads config.APIKey() and passes it through, matching the sse case's behavior (but auth stays optional for http, not required, so basic-host and local MCP Apps dev still work without a key) - Startup log now discloses auth state and warns when binding a non-loopback address without auth (tunnel-like exposure) - Help text reworded to point at the actual ways to configure the key ('jc auth login', JC_API_KEY env var, --api-key global flag) rather than vaguely saying "use --api-key" Three regression tests added: - TestHTTP_AuthRejectsUnauthenticated — no header → 401 when key configured - TestHTTP_AuthAcceptsCorrectKey — correct x-api-key → 200 - TestHTTP_NoAuthWhenNoKey — permissive default preserved for local dev Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 1cfc267 commit 2b58387

2 files changed

Lines changed: 131 additions & 4 deletions

File tree

internal/cmd/mcp.go

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -94,8 +94,9 @@ Streamable HTTP Examples (for Claude Desktop custom connectors and MCP Apps):
9494
Security: the http transport is stateless and permissive by default (wide-open
9595
CORS, cross-origin checks disabled) so browser-based MCP clients like basic-host
9696
and MCP Apps UIs can connect. When exposing the server via a tunnel (cloudflared,
97-
ngrok, etc.), use --api-key to prevent unauthenticated tool calls from anyone
98-
who discovers the URL.
97+
ngrok, etc.), configure an API key — via 'jc auth login', the JC_API_KEY env
98+
var, or the --api-key global flag — so the auth middleware rejects
99+
unauthenticated tool calls from anyone who discovers the URL.
99100
100101
Use JC_PROFILE environment variable to select which JumpCloud org to use.`,
101102
RunE: func(cmd *cobra.Command, args []string) error {
@@ -231,14 +232,32 @@ func runMcpServe(rateLimit int, readOnly bool, transport, addr string, port int,
231232

232233
case "http":
233234
// Streamable HTTP transport for Claude Desktop custom connectors and MCP Apps.
234-
// No auth by default on loopback — designed for local use with cloudflared tunnels.
235+
// Auth is optional here (unlike sse) so browser-based MCP clients like
236+
// basic-host can connect during local development. When the operator has
237+
// configured an API key (via `jc auth login`, JC_API_KEY, or --api-key),
238+
// we pass it through so the server's auth middleware rejects unauth'd
239+
// calls — critical when exposing via a cloudflared tunnel.
235240
listenAddr := resolveSSEAddr(addr, port)
241+
apiKey := config.APIKey()
236242

237-
fmt.Fprintf(os.Stderr, "jc: starting MCP server on Streamable HTTP transport at http://%s/mcp\n", listenAddr)
243+
scheme := "http"
244+
fmt.Fprintf(os.Stderr, "jc: starting MCP server on Streamable HTTP transport at %s://%s/mcp\n", scheme, listenAddr)
245+
if apiKey == "" {
246+
// Warn if binding beyond loopback without auth — that's the
247+
// combination a tunnel creates, too.
248+
if host, _, err := net.SplitHostPort(listenAddr); err == nil {
249+
if host != "127.0.0.1" && host != "::1" && host != "localhost" {
250+
fmt.Fprintln(os.Stderr, "jc: WARNING: HTTP transport running without an API key. Anyone who reaches the server can call all tools.")
251+
}
252+
}
253+
} else {
254+
fmt.Fprintln(os.Stderr, "jc: HTTP transport requires x-api-key or Authorization: Bearer header for all requests.")
255+
}
238256

239257
return server.RunStreamableHTTP(ctx, mcp.SSEConfig{
240258
Addr: listenAddr,
241259
CORSOrigin: "*",
260+
APIKey: apiKey,
242261
})
243262

244263
default:

internal/mcp/sse_test.go

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -597,3 +597,111 @@ func generateTestCert(t *testing.T) (certFile, keyFile string) {
597597

598598
return certFile, keyFile
599599
}
600+
601+
// startHTTPStreamServer mirrors startSSEServer but for the Streamable HTTP
602+
// transport. Returns the base URL (including the /mcp path).
603+
func startHTTPStreamServer(t *testing.T, cfg SSEConfig) (*Server, string) {
604+
t.Helper()
605+
setupTest(t)
606+
607+
if cfg.Addr == "" {
608+
cfg.Addr = ":0"
609+
}
610+
611+
server := NewServer(Options{
612+
RateLimit: 60,
613+
AuditLogPath: filepath.Join(t.TempDir(), "audit.log"),
614+
})
615+
616+
ctx, cancel := context.WithCancel(context.Background())
617+
t.Cleanup(cancel)
618+
619+
errCh := make(chan error, 1)
620+
go func() { errCh <- server.RunStreamableHTTP(ctx, cfg) }()
621+
622+
var addr net.Addr
623+
for i := 0; i < 50; i++ {
624+
addr = server.Listener()
625+
if addr != nil {
626+
break
627+
}
628+
time.Sleep(10 * time.Millisecond)
629+
}
630+
if addr == nil {
631+
t.Fatal("HTTP stream server did not start listening")
632+
}
633+
634+
return server, "http://" + addr.String() + "/mcp"
635+
}
636+
637+
// TestHTTP_AuthRejectsUnauthenticated is the regression guard for the high-
638+
// severity Bugbot finding: when APIKey is set on the http transport, requests
639+
// without credentials must be rejected.
640+
func TestHTTP_AuthRejectsUnauthenticated(t *testing.T) {
641+
_, baseURL := startHTTPStreamServer(t, SSEConfig{
642+
APIKey: "test-key",
643+
CORSOrigin: "*",
644+
})
645+
646+
body := strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"t","version":"1"}}}`)
647+
req, _ := http.NewRequest(http.MethodPost, baseURL, body)
648+
req.Header.Set("Content-Type", "application/json")
649+
req.Header.Set("Accept", "application/json, text/event-stream")
650+
651+
resp, err := http.DefaultClient.Do(req)
652+
if err != nil {
653+
t.Fatalf("POST: %v", err)
654+
}
655+
defer resp.Body.Close()
656+
657+
if resp.StatusCode != http.StatusUnauthorized {
658+
t.Errorf("expected 401 Unauthorized without x-api-key, got %d", resp.StatusCode)
659+
}
660+
}
661+
662+
// TestHTTP_AuthAcceptsCorrectKey confirms auth is actually checked, not bypassed.
663+
func TestHTTP_AuthAcceptsCorrectKey(t *testing.T) {
664+
_, baseURL := startHTTPStreamServer(t, SSEConfig{
665+
APIKey: "test-key",
666+
CORSOrigin: "*",
667+
})
668+
669+
body := strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"t","version":"1"}}}`)
670+
req, _ := http.NewRequest(http.MethodPost, baseURL, body)
671+
req.Header.Set("Content-Type", "application/json")
672+
req.Header.Set("Accept", "application/json, text/event-stream")
673+
req.Header.Set("x-api-key", "test-key")
674+
675+
resp, err := http.DefaultClient.Do(req)
676+
if err != nil {
677+
t.Fatalf("POST: %v", err)
678+
}
679+
defer resp.Body.Close()
680+
681+
if resp.StatusCode != http.StatusOK {
682+
t.Errorf("expected 200 with correct x-api-key, got %d", resp.StatusCode)
683+
}
684+
}
685+
686+
// TestHTTP_NoAuthWhenNoKey asserts the permissive default: no API key → any
687+
// client can connect (needed for basic-host and local MCP Apps dev).
688+
func TestHTTP_NoAuthWhenNoKey(t *testing.T) {
689+
_, baseURL := startHTTPStreamServer(t, SSEConfig{
690+
CORSOrigin: "*",
691+
})
692+
693+
body := strings.NewReader(`{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"t","version":"1"}}}`)
694+
req, _ := http.NewRequest(http.MethodPost, baseURL, body)
695+
req.Header.Set("Content-Type", "application/json")
696+
req.Header.Set("Accept", "application/json, text/event-stream")
697+
698+
resp, err := http.DefaultClient.Do(req)
699+
if err != nil {
700+
t.Fatalf("POST: %v", err)
701+
}
702+
defer resp.Body.Close()
703+
704+
if resp.StatusCode != http.StatusOK {
705+
t.Errorf("expected 200 without auth when no API key configured, got %d", resp.StatusCode)
706+
}
707+
}

0 commit comments

Comments
 (0)