fix(security): require admin auth for /metrics and stop logging the API key (SEC-07)

docs/features/observability.md

@@ -56,18 +56,43 @@ OTLP collector.
 Invalid values are repaired on load (unknown protocol → `http`, out-of-range
 sample rate → `0.1`), so a partial block never breaks startup.
 
-> **Note:** `/metrics` is served on the same listener as the REST API. The
-> REST API requires an API key, but the `/metrics` endpoint follows the same
-> rules as the MCP endpoints. Keep `listen` bound to a trusted interface (the
-> default is localhost) or scrape via a sidecar/network policy in clustered
-> deployments.
+## Authentication
+
+`/metrics` is served on the same listener as the REST API and **requires the
+global API key** — the exporter carries fleet-wide tool, server and request
+topology, so it is treated as admin-only data. Present the key as either:
+
+- `X-API-Key: <api key>`, or
+- `Authorization: Bearer <api key>`, or
+- `?apikey=<api key>` query parameter (same precedence as the rest of the
+  REST API — see [rest-api.md](../api/rest-api.md)).
+
+> **Caution:** prefer a header over the `?apikey=` query parameter for
+> scrapers. Query strings are the credential form most likely to be copied
+> into an intermediary or reverse-proxy's access logs.
+
+Agent tokens (`mcp_agt_`) are rejected with `403`: they are scope-restricted
+and must not read fleet-wide aggregates. The tray's Unix-socket connection is
+trusted by OS-level permissions and needs no key, as everywhere else.
+
+The liveness and readiness probes (`/healthz`, `/livez`, `/health`, `/readyz`,
+`/ready`) stay unauthenticated by design.
+
+> **Changed:** before this release `/metrics` answered unauthenticated
+> requests. Existing scrapers must be updated to send the key. Keep `listen`
+> bound to a trusted interface (the default is localhost) as a second layer.
 
 ## Prometheus scrape config
 
 ```yaml
 scrape_configs:
   - job_name: mcpproxy
     metrics_path: /metrics
+    authorization:
+      # Prometheus defaults the type to Bearer; mcpproxy accepts the global
+      # API key as the bearer credential.
+      credentials: "<mcpproxy api key>"
+      # or: credentials_file: /etc/mcpproxy/api-key
     static_configs:
       - targets: ["mcpproxy:8080"]
 ```

frontend/src/services/api.ts

@@ -73,7 +73,7 @@
       this.apiKey = apiKeyFromURL
       // Store the new API key for future navigation/refreshes
       localStorage.setItem('mcpproxy-api-key', apiKeyFromURL)
-      console.log('API key from URL (updating storage):', this.apiKey.substring(0, 8) + '...')
+      // SEC-07: never log key material (not even a prefix) to the devtools console.
       // Clean the URL by removing the API key parameter for security
       urlParams.delete('apikey')
       const newURL = window.location.pathname + (urlParams.toString() ? '?' + urlParams.toString() : '')
@@ -83,9 +83,6 @@
       const storedApiKey = localStorage.getItem('mcpproxy-api-key')
       if (storedApiKey) {
         this.apiKey = storedApiKey
-        console.log('API key from localStorage:', this.apiKey.substring(0, 8) + '...')
-      } else {
-        console.log('No API key found in URL or localStorage')
       }
     }
   }
@@ -117,8 +114,8 @@
   public setAPIKey(key: string): void {
     this.apiKey = key
     if (key) {
       localStorage.setItem('mcpproxy-api-key', key)
-      console.log('API key set and stored:', key.substring(0, 8) + '...')
+      // SEC-07: no key material in the console, not even a prefix.
     } else {
       localStorage.removeItem('mcpproxy-api-key')
       console.log('API key cleared')
@@ -202,11 +199,11 @@
       // Add API key header if available
       if (this.apiKey) {
         headers['X-API-Key'] = this.apiKey
-        console.log(`API request to ${endpoint} with API key: ${this.getAPIKeyPreview()}`)
       } else {
+        // SEC-07: log only that the request is unauthenticated. The previous
+        // lines echoed the key prefix, window.location.search (which can still
+        // carry ?apikey=) and the localStorage value on every single call.
         console.log(`API request to ${endpoint} without API key - initialized: ${this.initialized}`)
-        console.log('Current URL search params:', window.location.search)
-        console.log('LocalStorage API key:', localStorage.getItem('mcpproxy-api-key')?.substring(0, 8) + '...')
       }
 
       const response = await fetch(`${this.baseUrl}${endpoint}`, {
@@ -501,10 +498,11 @@
       ? `${this.baseUrl}/events?apikey=${encodeURIComponent(this.apiKey)}`
       : `${this.baseUrl}/events`
 
+    // SEC-07: the "redacted" URL used to be redacted WITH the key preview, so it
+    // leaked the first 8 characters anyway. Redact fully and drop the preview.
     console.log('Creating EventSource:', {
       hasApiKey: !!this.apiKey,
-      apiKeyPreview: this.getAPIKeyPreview(),
-      url: this.apiKey ? url.replace(this.apiKey, this.getAPIKeyPreview()) : url
+      url: this.apiKey ? url.replace(encodeURIComponent(this.apiKey), '[redacted]') : url
     })
 
     return new EventSource(url)

frontend/src/stores/system.ts

@@ -170,10 +170,9 @@ export const useSystemStore = defineStore('system', () => {
     }
 
     console.log('Attempting to connect EventSource...')
-    console.log('API key status:', {
-      hasApiKey: api.hasAPIKey(),
-      apiKeyPreview: api.getAPIKeyPreview()
-    })
+    // SEC-07: log only whether a key is present. This used to include
+    // api.getAPIKeyPreview(), i.e. the first 8 characters of the admin key.
+    console.log('API key status:', { hasApiKey: api.hasAPIKey() })
 
     const es = api.createEventSource()
     eventSource.value = es
@@ -385,9 +384,13 @@ export const useSystemStore = defineStore('system', () => {
       }
     })
 
-    es.onerror = (event) => {
+    es.onerror = () => {
       connected.value = false
-      console.error('EventSource error occurred:', event)
+      // SEC-07: do NOT log the error event. Its `target` is the EventSource,
+      // whose `url` carries the API key as a ?apikey= query parameter, so
+      // logging the event puts the WHOLE key in the devtools console. The
+      // event itself carries no diagnostic detail beyond readyState anyway.
+      console.error('EventSource error occurred; readyState:', es.readyState)
 
       // Check if this might be an authentication error
       if (es.readyState === EventSource.CLOSED) {

frontend/tests/unit/api-key-not-logged.spec.ts

@@ -0,0 +1,140 @@
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'
+
+// SEC-07: the API service used to console.log the first 8 characters of the
+// admin key on import, on setAPIKey, on EVERY request and when opening the SSE
+// stream. Devtools console history is readable by anything with a debugger
+// attached and ends up pasted into bug reports, so no key material — not even
+// a prefix — may reach the console.
+
+const SECRET = 'SUPERSECRETKEY-abcdef0123456789'
+const STORAGE_KEY = 'mcpproxy-api-key'
+
+type ConsoleMethod = 'log' | 'info' | 'debug' | 'warn' | 'error'
+const CONSOLE_METHODS: ConsoleMethod[] = ['log', 'info', 'debug', 'warn', 'error']
+
+/** Every argument passed to any console method during the spy's lifetime. */
+function collectConsoleOutput(spies: Record<ConsoleMethod, ReturnType<typeof vi.spyOn>>): string {
+  return CONSOLE_METHODS.flatMap((method) =>
+    spies[method].mock.calls.flatMap((args: unknown[]) =>
+      args.map((arg) => {
+        if (typeof arg === 'string') return arg
+        try {
+          return JSON.stringify(arg)
+        } catch {
+          return String(arg)
+        }
+      })
+    )
+  ).join('\n')
+}
+
+describe('api service does not log key material', () => {
+  let spies: Record<ConsoleMethod, ReturnType<typeof vi.spyOn>>
+
+  beforeEach(() => {
+    localStorage.clear()
+    vi.resetModules()
+    spies = {} as Record<ConsoleMethod, ReturnType<typeof vi.spyOn>>
+    for (const method of CONSOLE_METHODS) {
+      spies[method] = vi.spyOn(console, method).mockImplementation(() => {})
+    }
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+    vi.unstubAllGlobals()
+    localStorage.clear()
+    // Always clear a ?apikey= left on the jsdom URL by the query-parameter
+    // test below, even if that test's assertions threw first — otherwise the
+    // secret lingers in window.location for whatever spec runs next in this
+    // module.
+    window.history.replaceState({}, '', '/')
+  })
+
+  it('never writes the key or its prefix to the console', async () => {
+    // The singleton runs initializeAPIKey() in its constructor at import time,
+    // so seed storage before importing.
+    localStorage.setItem(STORAGE_KEY, SECRET)
+
+    // jsdom has no EventSource; stub one so createEventSource() can run.
+    class FakeEventSource {
+      constructor(public url: string) {}
+      close() {}
+    }
+    vi.stubGlobal('EventSource', FakeEventSource as unknown as typeof EventSource)
+    vi.stubGlobal(
+      'fetch',
+      vi.fn(async () => new Response(JSON.stringify({ success: true, data: {} }), {
+        status: 200,
+        headers: { 'Content-Type': 'application/json' }
+      }))
+    )
+
+    const api = (await import('@/services/api')).default
+
+    // Exercise every path that used to echo the key.
+    api.reinitializeAPIKey()
+    api.setAPIKey(SECRET)
+    await api.getStatus()
+    api.createEventSource()
+
+    const logged = collectConsoleOutput(spies)
+    expect(logged).not.toContain(SECRET)
+    expect(logged).not.toContain(SECRET.substring(0, 8))
+  })
+
+  // The service is not the only consumer: the system store used to log
+  // api.getAPIKeyPreview() every time it opened the SSE stream, so a spec that
+  // only drives APIService would miss the leak that actually fires on app boot.
+  it('never leaks the key through the system store SSE connect path', async () => {
+    localStorage.setItem(STORAGE_KEY, SECRET)
+
+    // A faithful stand-in: the real EventSource carries the key in its `url`
+    // (SSE cannot send headers), and the error event's `target` is the
+    // EventSource itself — which is how logging the raw event leaked the whole
+    // key, not just a prefix.
+    const created: FakeEventSource[] = []
+    class FakeEventSource {
+      static readonly CLOSED = 2
+      readonly CLOSED = 2
+      readyState = 0
+      onopen: (() => void) | null = null
+      onerror: ((event: unknown) => void) | null = null
+      constructor(public url: string) {
+        created.push(this)
+      }
+      addEventListener() {}
+      close() {}
+    }
+    vi.stubGlobal('EventSource', FakeEventSource as unknown as typeof EventSource)
+
+    const { createPinia, setActivePinia } = await import('pinia')
+    setActivePinia(createPinia())
+
+    const { useSystemStore } = await import('@/stores/system')
+    useSystemStore().connectEventSource()
+
+    // Drive the failure path too: a dropped stream is routine, and its handler
+    // used to log the credential-bearing event object.
+    const es = created.at(-1)
+    expect(es).toBeDefined()
+    expect(es!.url).toContain(encodeURIComponent(SECRET))
+    es!.readyState = 1
+    es!.onerror?.({ type: 'error', target: es })
+
+    const logged = collectConsoleOutput(spies)
+    expect(logged).not.toContain(SECRET)
+    expect(logged).not.toContain(SECRET.substring(0, 8))
+  })
+
+  it('does not log the key when it arrives via the URL parameter', async () => {
+    window.history.replaceState({}, '', `/?apikey=${encodeURIComponent(SECRET)}`)
+
+    const api = (await import('@/services/api')).default
+    expect(api.hasAPIKey()).toBe(true)
+
+    const logged = collectConsoleOutput(spies)
+    expect(logged).not.toContain(SECRET)
+    expect(logged).not.toContain(SECRET.substring(0, 8))
+  })
+})

internal/config/config.go

@@ -1515,6 +1515,8 @@ type ObservabilityConfig struct {
 // MetricsExporterConfig controls the Prometheus /metrics endpoint (MCP-32).
 type MetricsExporterConfig struct {
 	// Enabled exposes /metrics on the existing HTTP listener when true.
+	// The endpoint is admin-authenticated (SEC-07): scrapers must present the
+	// global API key, via X-API-Key or an Authorization: Bearer header.
 	Enabled bool `json:"enabled" mapstructure:"enabled"`
 }