refactor(httpapi): type the controller seams and fail closed without a config - #1346
Merged
Merged
Conversation
…a config
The management service travelled from internal/runtime through
httpapi.ServerController as interface{}, and twelve handler sites in
internal/httpapi re-derived its method set with ad-hoc anonymous interface
assertions. Five of those were unchecked, so a method-set drift became a
panic recovered by chi into an opaque 500 with no log line naming the cause;
the other seven degraded to a runtime ok=false and a 500.
management.Service already describes exactly what those handlers call and
internal/httpapi already imports internal/management, so the seam is typed
to it end to end (runtime field + setter + getter, internal/server.Server,
httpapi.ServerController) and all twelve assertions are deleted. The three
equivalents on the MCP surface in internal/server/mcp.go go with them.
apiKeyAuthMiddleware had two "allow through (testing scenario)" branches on
GetCurrentConfig(). Typing the getter to *config.Config makes the
non-*config.Config branch unrepresentable, and the remaining nil path now
fails CLOSED with 503: no configuration means there is nothing to
authenticate against, so the request cannot be authenticated. In production
Runtime.GetCurrentConfig() wrapped a *config.Config and runtime.New rejects
a nil config, so this was not an exploitable bypass there — but it was a
live one for any other implementor, and 37 test requests authenticated by
not authenticating.
Three security tests (Spec 099 FR-018a's disclosure floor, and
auth.AuthorizeServerOp's unrestricted-on-absent-AuthContext default) reached
their handlers through the deleted branch. They are rewritten to drive the
handler and the subtree gate directly with a no-AuthContext request, keeping
the same assertions, rather than deleted with the branch. index_search_scoped
does the same: the middleware would otherwise replace the AuthContext those
tests inject and turn every scoped fixture into an admin one.
Verified on a live instance: all twelve converted call sites exercised over
REST and MCP, auth still required, no panics.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Deploying mcpproxy-docs with
|
| Latest commit: |
eba0071
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fdd96f12.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-typed-controller-seams.mcpproxy-docs.pages.dev |
Contributor
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 35816416798 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
… seams - remove dead X-API-Key Set in TestAddFromRegistry_NilConfigIsAnError (the first Set was immediately overwritten and asserted nothing) - TestAPIKeyAuth_NilConfigFailsClosed: assert the 503 body text, not just the status code, so an unrelated earlier 503 can't pass the same assertions for the wrong reason - GetCurrentConfig(): guard s.runtime == nil like its sibling GetManagementService(); unreachable in production but latent for a hand-built *Server in tests. Regression test added (get_current_config_test.go) reproducing the prior nil-pointer panic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Member
Author
|
Applied a second-lens review pass to this PR (no MUST FIX findings; three SHOULD FIX findings applied, one left for a follow-up):
Left for a follow-up doc pass rather than fixed here: the stale "kept as interface{} to avoid circular imports" / "middleware's test/no-config passthrough" comments at Verified: |
Dumbris
added a commit
that referenced
this pull request
Sep 24, 2026
#1359) * fix(httpapi): restore the build after the typed controller seam landed #1346 changed ServerController.GetCurrentConfig to return *config.Config, while #1350 (merged just before it) added currentAdminAPIKey with an interface type assertion on that value and a cors_test controller returning any. Each PR was green alone; together main does not compile. * docs(httpapi): fix the stale nil-config note on currentAdminAPIKey
Dumbris
added a commit
that referenced
this pull request
Sep 24, 2026
/metrics now requires the admin API key. This test only passed before because its mock controller handed the auth middleware no config, which the middleware used to forward unauthenticated; #1346 made that fail closed.
Dumbris
added a commit
that referenced
this pull request
Sep 24, 2026
…PI key (SEC-07) (#1344) * fix(security): require admin auth for /metrics and stop logging the API 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> * docs+test: document ?apikey= for /metrics, harden test cleanup (SEC-07 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> * test(httpapi): authenticate the readiness test's /metrics probe (SEC-07) /metrics now requires the admin API key. This test only passed before because its mock controller handed the auth middleware no config, which the middleware used to forward unauthenticated; #1346 made that fail closed. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
2 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
Types two
interface{}seams onhttpapi.ServerControllerand makes the auth middleware fail closed when it has no configuration.ARC-04 — the management service travelled as
interface{}. It leftinternal/runtimeuntyped, crossedServerController, and was re-derived ininternal/httpapiby 12 ad-hoc anonymous interface assertions. Five of those were unchecked (mgmtSvc.(interface{...}).Method(...)), so a method-set drift panicked intochi'sRecovererand surfaced as an opaque 500 with no log line naming the cause; the other seven degraded took=falseand a 500.management.Servicealready describes exactly what those handlers call, andinternal/httpapialready importsinternal/management, so the seam is now typed to it end to end and all 12 assertions are gone. The three equivalents on the MCP surface (internal/server/mcp.go) go with them.SEC-02 — two fail-open branches in
apiKeyAuthMiddleware. TypingGetCurrentConfig()to*config.Configmakes the "not the expected type" branch unrepresentable; the remaining nil path now returns 503 instead of forwarding the request. To be precise about severity: in productionRuntime.GetCurrentConfig()wrapped a*config.Configandruntime.Newrejects a nil config, so this was not an exploitable bypass there — but it was a live one for any other implementor of the interface, and 37 existing test requests were authenticating by not authenticating.Why it is one change
Both defects are the same missing type. The named interface the seam needed already existed; nothing new was designed.
Notable
handleGetGlobalToolskeeps its deliberate graceful fallback tocontroller.GetServerTools; the nil check stays on the interface value.auth.AuthorizeServerOp's unrestricted-on-absent-AuthContextdefault) reached their handlers through the deleted branch. They are rewritten, not deleted: they now drive the handler and the subtree gate directly with a no-AuthContextrequest and keep the same assertions.index_search_scopedmoved for a different reason — the middleware would replace theAuthContextthose tests inject and turn every scoped fixture into an admin one.How verified
TDD: both tests were written and run red first (503-and-handler-not-reached failed 200/true; the compile-time contract failed to build).
go test -raceoninternal/httpapi,internal/runtime,internal/management, andinternal/serverwith theunit-tests.ymlskip regex-tags server(which is whereindex_search_scoped_test.golives — invisible to a bare run)golangci-lintpasses, bare and--build-tags server; the 16/19 findings are pre-existing and untouchedmcpproxy serve—servers,doctor,{id}/tools,enable/disable,restart,restart_all,enable_all,disable_all,logout, globaltools, andupstream_serversrestart/enable/disable over JSON-RPC. Unauthenticated request still 401s; zero panics in the log.Cross-model review:
codex gpt-5.6-sol, one round, no production findings. It raised one P3 — the rewritten tests no longer traverse the full chi route. I verified it and did not change the code: at least five other tests (scope_reveal_test.go:341,contracts_test.go:429,search_tools_test.go:48,security_test.go:57,tenant_allowlist_walk_test.go:129) already drive/api/v1/index/searchthrough the real router, and re-plumbing the rewritten tests would destroy the very property they pin.🤖 Generated with Claude Code