Conversation
…PI key (SEC-07)
/metrics was registered on the bare chi router, outside the /api/v1 group that
carries apiKeyAuthMiddleware, so any caller that could reach the listener could
scrape fleet-wide tool-usage counters and API topology unauthenticated.
It now carries the same credential chain as /events plus an admin gate:
apiKeyAuthMiddleware alone still admits agent tokens (mcp_agt_), and a
scope-restricted agent must not read fleet-wide aggregates. Scrapers
authenticate with the global API key via X-API-Key or Authorization: Bearer
(the latter is what Prometheus' `authorization: {credentials: ...}` block
sends). The tray keeps its Unix-socket bypass.
The liveness and readiness probes (/healthz, /livez, /health, /readyz, /ready)
stay unauthenticated by design.
On the frontend, the API service logged the first 8 characters of the admin key
to the devtools console on import, on setAPIKey, on every request and when
opening the SSE stream; the system store logged it again on connect, and the
SSE onerror handler logged the raw error event, whose target is the
EventSource, leaking the whole key via its ?apikey= URL. All of those are gone.
getAPIKeyPreview() itself is kept: it still backs an on-screen preview in the
auth modal.
Tests: new internal/httpapi/metrics_auth_test.go (401 without a key, 401 with a
wrong key, 200 with X-API-Key and with Bearer, 403 for a real agent token, all
five health aliases still open) — built on a controller returning a real
*config.Config so it cannot pass vacuously through the middleware's
not-a-config passthrough. internal/server/e2e_metrics_test.go flips from
asserting an unauthenticated 200 to 401-then-authenticated-200. New
frontend/tests/unit/api-key-not-logged.spec.ts asserts no console call
stringifies to the secret or its first 8 characters.
Follow-ups, deliberately not in this change: the admin key is persisted in
localStorage (a session-cookie flow is a larger UX change); the shared HTTP
logging middleware writes r.URL.RawQuery verbatim, so a ?apikey= credential on
any route lands in http.log; Activity.vue logs raw activity payloads.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
29c470b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6f386a33.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-protect-metrics-endpoint.mcpproxy-docs.pages.dev |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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) |
| // 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) |
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35815344798 --repo smart-mcp-proxy/mcpproxy-go
|
…7 review) Second-lens review on PR #1344 found two SHOULD-FIX gaps: - docs/features/observability.md: the Authentication section listed only X-API-Key and Authorization: Bearer as accepted /metrics credentials, omitting the ?apikey= query parameter that authenticateWithPrecedence also accepts (step 3, internal/httpapi/server.go). Documented it, with a caution that query-string credentials are the form most likely to leak into intermediary access logs. - frontend/tests/unit/api-key-not-logged.spec.ts: the URL-parameter test's window.history.replaceState cleanup only ran after its assertions passed, so a failing assertion left ?apikey=<SECRET> on the jsdom URL for whatever spec ran next in the module. Moved the cleanup into afterEach so it always runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
Second-lens review — appliedBoth SHOULD-FIX findings verified against the code and applied:
No MUST-FIX findings on this round. Both changes are docs/test-only — no production behavior change, so no new failing test was needed. Verified: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
/metricswas registered on the bare chi router, outside the/api/v1group that carriesapiKeyAuthMiddleware, so it answered unauthenticated callers with fleet-wide tool-usage counters and API topology. It now sits behind the same credential chain as/events, plus an admin gate.The Web UI's API service also logged the first 8 characters of the admin key to the devtools console — on import, on
setAPIKey, on every request, and when opening the SSE stream. The system store logged it again on connect, and the SSEonerrorhandler logged the raw error event, whosetargetis the EventSource, leaking the whole key through its?apikey=URL. All removed.Why
Security finding SEC-07. Two decisions worth calling out:
apiKeyAuthMiddlewarealone still admits agent tokens (mcp_agt_), and a scope-restricted agent must not read fleet-wide aggregates — the same leak class as the finding. The route addsrequireAdminReadMiddleware, a thin wrapper over the existingrequireAdminReadrule, so one mux keeps one definition of "not admin"./healthz,/livez,/health,/readyz,/readyare liveness/readiness probes and remain unauthenticated.Operator-visible change: existing Prometheus scrapers break until they send the key.
observability.metrics.enableddefaults tofalse, so most installs are untouched.docs/features/observability.mdnow documents the header and a scrape config usingauthorization: { credentials: ... }, which mcpproxy accepts asAuthorization: Bearer <api key>.getAPIKeyPreview()is kept — it still backs the on-screen preview in the auth modal.How verified
internal/httpapi/metrics_auth_test.go: 401 with no credential, 401 with a wrong key, 200 withX-API-Keyand withBearer, 403 for a realmcp_agt_token through the realhandleAgentTokenAuth, and all five health aliases still open. Built on a controller returning a real*config.Configso it cannot pass vacuously through the middleware's not-a-config passthrough.internal/server/e2e_metrics_test.goflips from asserting an unauthenticated 200 to 401-then-authenticated-200 against the real binary.frontend/tests/unit/api-key-not-logged.spec.tsdrives the singleton, the store's connect path and itsonerrorhandler under console spies; each case was confirmed to fail against the pre-fix source./metrics→ 401,X-API-Key→ 200 with the uptime gauge,Bearer→ 200, a real agent token → 403, all five probes → 200 unauthenticated, Unix socket → 200.--build-tags server),internal/httpapi+internal/config+internal/serverunder-race, full frontend suite (1328 tests) andvue-tsc.codex exec --model gpt-5.6-sol, 3 rounds, final verdict APPROVE. Round 1 and 2 each found a genuine surviving console leak (the system store's key preview, then the rawonerrorevent) — both fixed with coverage. I rejected two out-of-scope findings; see below.Follow-ups (deliberately not in this PR)
localStorage; moving to a session-cookie flow is a larger UX change.httpLoggingMiddlewarewritesr.URL.RawQueryverbatim, so an?apikey=credential on any route lands inhttp.log. Pre-existing, shared middleware, affects/eventson every page load today.Activity.vuelogs raw activity payloads, which can carry upstream tool-call credentials. Different subsystem and data class.🤖 Generated with Claude Code