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: |
971560c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://9d5ae356.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 35748149989 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
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