diff --git a/internal/httpapi/scope_reveal_harness_test.go b/internal/httpapi/scope_reveal_harness_test.go index f9bc1aced..659133715 100644 --- a/internal/httpapi/scope_reveal_harness_test.go +++ b/internal/httpapi/scope_reveal_harness_test.go @@ -131,6 +131,26 @@ func (m *scopeMgmtService) ListServers(context.Context) ([]*contracts.Server, *c return out, stats, nil } +// Doctor mirrors the production management service (internal/management.Doctor), +// including its #1166 scope gate: a server the caller may not enumerate +// contributes nothing to the diagnosis it is handed. Reporting one upstream +// error per surviving server lets a scoped-caller test confirm the hidden +// server is absent. +func (m *scopeMgmtService) Doctor(ctx context.Context) (*contracts.Diagnostics, error) { + diag := &contracts.Diagnostics{} + for i := range m.servers { + if !auth.CanEnumerateServer(ctx, m.servers[i].Name) { + continue + } + diag.UpstreamErrors = append(diag.UpstreamErrors, contracts.UpstreamError{ + ServerName: m.servers[i].Name, + ErrorMessage: "fixture error for " + m.servers[i].Name, + }) + } + diag.TotalIssues = len(diag.UpstreamErrors) + return diag, nil +} + func (m *scopeMgmtService) GetServerTools(_ context.Context, name string) ([]map[string]interface{}, error) { return []map[string]interface{}{ {"name": name + "_tool", "server_name": name, "description": "tool of " + name}, diff --git a/internal/httpapi/scope_route_table_guard_test.go b/internal/httpapi/scope_route_table_guard_test.go new file mode 100644 index 000000000..b08d19fd9 --- /dev/null +++ b/internal/httpapi/scope_route_table_guard_test.go @@ -0,0 +1,303 @@ +package httpapi + +import ( + "net/http" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/scanner" +) + +// Issue #1178. Fixing #1166/#1167 took three passes because the /api/v1 GET +// surface was enumerated by hand and each pass missed a door (/servers/{id}/logs +// carrying upstream stdout/stderr, /activity carrying tool-call args). This test +// drops the hand-enumeration. It walks the PRODUCTION chi route table, finds +// every GET under /api/v1, and requires each one to carry a scope classification +// below. A new GET route added without an entry fails the test the moment it is +// registered, so a door cannot ship unclassified. +// +// Each route is one of: +// +// scopeRefused: a scoped agent token gets the fixed 403 (a deployment-wide +// document it may not read). Driven live, it must be 403 AND carry the +// scope-layer denial body, so an unrelated 403 does not satisfy it. +// scopeFiltered: reachable, but per-server content is narrowed to the caller's +// grant downstream. The `why` names the scoped-caller test that proves the +// narrowing; this file only checks the coarse verdict (never the fixed 403). +// scopeOpen: reachable, and carries nothing about a server outside the +// caller's grant. Allowlisted with a written reason. +// +// Filtered and open both reach the handler for the probe token — a 2xx, never +// the fixed 403. They are kept as separate classes so the `why` on each says +// which it is. A handful reach a documented 404/400/503 BEFORE the gate on the +// synthetic path params the walk uses (no such record, dependency not wired); +// those are enumerated in shortCircuitCodes and their real scoped-caller oracle +// is the named test in `why`, not this walk. + +type scopeClass int + +const ( + scopeRefused scopeClass = iota + scopeFiltered + scopeOpen +) + +type routeScope struct { + class scopeClass + why string +} + +// getRouteScopes classifies every GET route under /api/v1. Keys are chi route +// patterns exactly as chi.Walk reports them (trailing slashes and all). The test +// asserts this map and the walked surface are the same set in BOTH directions: +// an unclassified new route fails, and a stale entry for a removed route fails. +var getRouteScopes = map[string]routeScope{ + // --- Refused: deployment-wide documents an agent token cannot read --- + "/api/v1/config": {scopeRefused, "whole config document; TestGetConfig_AgentTokenForbidden_AdminUnaffected"}, + "/api/v1/code/scripts": {scopeRefused, "stored-script listing is admin-only (scriptsListingDenialMessage)"}, + "/api/v1/onboarding/state": {scopeRefused, "inventory size + client ids; TestOnboardingState_DeniedToScopedCaller"}, + "/api/v1/secrets/refs": {scopeRefused, "credential inventory; TestSecretsInventory_DeniedToScopedCaller"}, + "/api/v1/secrets/config": {scopeRefused, "credential inventory; TestSecretsInventory_DeniedToScopedCaller"}, + "/api/v1/security/overview": {scopeRefused, "fleet scan/finding counts; TestSecurityFleetDoors_DeniedToScopedCaller"}, + "/api/v1/security/queue": {scopeRefused, "names every queued server; TestSecurityFleetDoors_DeniedToScopedCaller"}, + "/api/v1/sessions": {scopeRefused, "MCP client/session history; TestSessions_DeniedToAgentTokens"}, + "/api/v1/sessions/{id}": {scopeRefused, "MCP client/session history; TestSessions_DeniedToAgentTokens"}, + "/api/v1/stats/tokens": {scopeRefused, "deployment-wide token stats; TestTokenStats_DeniedToAgentTokens"}, + "/api/v1/telemetry/payload": {scopeRefused, "fleet heartbeat counts; TestTelemetryPayload_DeniedToScopedCaller"}, + "/api/v1/tokens/": {scopeRefused, "agent tokens cannot manage tokens (requireManageTokens)"}, + "/api/v1/tokens/{name}/": {scopeRefused, "agent tokens cannot manage tokens (requireManageTokens)"}, + + // --- Filtered: reachable, per-server content narrowed to the grant --- + "/api/v1/servers": {scopeFiltered, "visibleServers; TestGetServers_AgentTokenSeesOnlyAllowedSubset_ManagementPath"}, + "/api/v1/tools": {scopeFiltered, "scoped to grant; TestGetGlobalTools_ScopedToAllowedServers"}, + "/api/v1/index/search": {scopeFiltered, "filtered before the cut; TestIndexSearch_HiddenHighRankerDisplacesEntitledHit"}, + "/api/v1/status": {scopeFiltered, "upstream_stats scoped; TestGetStatus_UpstreamStatsScoped"}, + "/api/v1/info": {scopeFiltered, "admin api key withheld; TestInfo_ScopedCallerNeverReceivesTheAdminAPIKey"}, + "/api/v1/annotations/coverage": {scopeFiltered, "one row per server, canSeeServer gate (#1166)"}, + "/api/v1/activity": {scopeFiltered, "scopeAllowedServers; TestListActivity_ScopedToAllowedServers"}, + "/api/v1/activity/summary": {scopeFiltered, "scopeAllowedServers; TestActivitySummary_ScopedToAllowedServers"}, + "/api/v1/activity/usage": {scopeFiltered, "scopeAllowedServers; TestActivityUsage_ScopedToAllowedServers"}, + "/api/v1/activity/export": {scopeFiltered, "scopeAllowedServers; TestActivityExport_ScopedToAllowedServers"}, + "/api/v1/activity/{id}": {scopeFiltered, "unentitled == absent (404); TestActivityDetail_UnentitledIsIndistinguishableFromAbsent"}, + "/api/v1/tool-calls": {scopeFiltered, "scopeAllowedServers; TestToolCalls_ScopedToAllowedServers"}, + "/api/v1/tool-calls/{id}": {scopeFiltered, "unentitled == absent (404); TestToolCallDetail_UnentitledIsIndistinguishableFromAbsent"}, + "/api/v1/security/scans": {scopeFiltered, "canSeeServer per row (handleListScanHistory)"}, + "/api/v1/security/scans/{jobId}/report": {scopeFiltered, "canSeeServer on report.ServerName; TestSecurityScanReportByJobID_ScopedTo404"}, + + // /servers/{id}/**: the whole subtree is gated by scopedServerSubtree, so an + // unentitled server reads exactly as an absent one (404), never the fixed 403. + // TestServerSubtree_UnentitledIsIndistinguishableFromAbsent / + // TestServerLogs_ScopedTokenNeverSeesAnotherServersStderr. + "/api/v1/servers/{id}/tools": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/logs": {scopeFiltered, "scopedServerSubtree gate (stdout/stderr, the #1166 leak)"}, + "/api/v1/servers/{id}/diagnostics": {scopeFiltered, "scopedServerSubtree gate; TestServerDiagnostics_UnentitledIsIndistinguishableFromAbsent"}, + "/api/v1/servers/{id}/tool-calls": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/tools/export": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/tools/{tool}/diff": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/scan/status": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/scan/report": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/scan/files": {scopeFiltered, "scopedServerSubtree gate"}, + "/api/v1/servers/{id}/integrity": {scopeFiltered, "scopedServerSubtree gate"}, + + // --- Open: no per-server identity a scoped token could learn beyond its grant --- + "/api/v1/profiles": {scopeOpen, "profile names, not server inventory"}, + "/api/v1/profiles/active": {scopeOpen, "active profile name only"}, + "/api/v1/routing": {scopeOpen, "available routing modes, no server data"}, + "/api/v1/docker/status": {scopeOpen, "Docker daemon availability, no server data"}, + "/api/v1/registries": {scopeOpen, "configured registry sources, not upstream servers"}, + "/api/v1/registries/{id}/servers": {scopeOpen, "searches a remote registry, not the local inventory"}, + "/api/v1/security/scanners": {scopeOpen, "scanner plugin inventory, not per-server data"}, + "/api/v1/security/scanners/{id}/status": {scopeOpen, "scanner plugin status, not per-server data"}, + "/api/v1/servers/import/paths": {scopeOpen, "host MCP-client config file locations, no upstream identity"}, + "/api/v1/connect": {scopeOpen, "MCP client connect status; reads stay open (server.go connect block)"}, + "/api/v1/connect/{client}": {scopeOpen, "MCP client connect status; reads stay open"}, + "/api/v1/connect/{client}/preview": {scopeOpen, "MCP client config preview; reads stay open"}, + + // /diagnostics and its /doctor alias aggregate per-server health, on two + // paths with two separate gates: the management path delegates to + // internal/management.Doctor, whose CanEnumerateServer drop is proven by + // TestDoctor_ScopedTokenSeesOnlyAllowedServers (internal/management); the + // legacy fallback filters through visibleServers in handleGetDiagnostics + // itself, proven here by TestDiagnostics_LegacyPathScopedCallerDoesNotSeeHiddenServer. + "/api/v1/diagnostics": {scopeFiltered, "management Doctor (TestDoctor_ScopedTokenSeesOnlyAllowedServers) / legacy visibleServers (TestDiagnostics_LegacyPathScopedCallerDoesNotSeeHiddenServer) drop unentitled servers (#1166)"}, + "/api/v1/doctor": {scopeFiltered, "alias of /diagnostics; same scope gate"}, +} + +// minGetRoutes is the vacuity floor the issue asks for: a chi.Walk that matches +// nothing (or almost nothing, after a router refactor changes the mount prefix) +// must not pass forever. The map/route set is pinned exactly by the two-way +// check below; this is the additional "a non-trivial number were found" guard. +// Lower it deliberately, with a diff, if routes are intentionally removed. +const minGetRoutes = 40 + +// routeParamSubstitutions fills chi path params. {id} on the /servers/{id} +// subtree is the entitled server, so where a record exists the scope gate lets +// the request through to the handler (a 2xx below). Where no record exists for +// the synthetic value — /servers/{id}/scan/{status,report,files} (no scan for +// alpha), the /{id} detail routes (no such activity/tool-call/job), a scanner id +// that is not a configured plugin — the handler answers 404/400/503 BEFORE the +// scope gate runs. Those are enumerated in shortCircuitCodes so the walk asserts +// the exact pre-gate code instead of a loose "not 403", and their real +// scoped-caller oracle is the named test in each getRouteScopes `why`. +var routeParamSubstitutions = map[string]string{ + "{id}": "alpha", + "{name}": "alpha", + "{tool}": "alpha_tool", + "{client}": "claude-desktop", + "{jobId}": "job-1", +} + +// shortCircuitCodes are the non-refused routes that, driven with the probe token +// and the SYNTHETIC params above, answer before the scope gate is reached. The +// live walk asserts each returns exactly this code — not 2xx — because a scoped +// token never exercises the gate on these inputs; deleting the gate would leave +// the code here unchanged. The gate for each is proven by the test cited in its +// getRouteScopes `why`. A route that STOPS short-circuiting (e.g. a handler that +// begins serving the synthetic id) trips this and must be re-examined. +var shortCircuitCodes = map[string]int{ + "/api/v1/index/search": http.StatusBadRequest, // no ?q= on the probe request + "/api/v1/connect": http.StatusServiceUnavailable, // connect manager not wired in the fixture + "/api/v1/connect/{client}": http.StatusServiceUnavailable, + "/api/v1/connect/{client}/preview": http.StatusServiceUnavailable, + "/api/v1/activity/{id}": http.StatusNotFound, // synthetic id absent → 404 before canSeeServer + "/api/v1/tool-calls/{id}": http.StatusNotFound, // synthetic id absent → 404 before canSeeServer + "/api/v1/security/scans/{jobId}/report": http.StatusNotFound, // synthetic jobId absent → 404 before canSeeServer + "/api/v1/servers/{id}/scan/status": http.StatusNotFound, // no scan record for alpha in the fixture + "/api/v1/servers/{id}/scan/report": http.StatusNotFound, // no scan record for alpha in the fixture + "/api/v1/servers/{id}/scan/files": http.StatusNotFound, // no scan record for alpha in the fixture + "/api/v1/security/scanners/{id}/status": http.StatusNotFound, // "alpha" is not a configured scanner plugin +} + +func fillRouteParams(pattern string) string { + out := pattern + for placeholder, value := range routeParamSubstitutions { + out = strings.ReplaceAll(out, placeholder, value) + } + return out +} + +// scopeDenialMarker is the fixed substring every scope-layer 403 body carries +// (config, activity, sessions, secrets, telemetry, tokens, and so on). Asserting on it +// pins the refusal to the scope layer, so an unrelated 403 cannot satisfy a +// scopeRefused route. +const scopeDenialMarker = "Agent tokens cannot" + +// TestScopeRouteTableGuard walks the production /api/v1 GET surface and holds +// every route to its classification for a real, read-only agent token scoped to +// one server. +func TestScopeRouteTableGuard(t *testing.T) { + sec := &mockSecurityController{ + scanners: []*scanner.ScannerPlugin{{ID: "ramparts", Name: "Ramparts", Status: scanner.ScannerStatusConfigured}}, + overview: &scanner.SecurityOverview{}, + queueProgress: &scanner.QueueProgress{}, + } + srv, token := scopeRound10Server(t, sec) + + seen := map[string]bool{} + err := chi.Walk(srv.Router(), func(method, route string, _ http.Handler, _ ...func(http.Handler) http.Handler) error { + if method != http.MethodGet || !strings.HasPrefix(route, "/api/v1") { + return nil + } + seen[route] = true + return nil + }) + require.NoError(t, err) + require.NotEmpty(t, seen, "chi.Walk found no /api/v1 GET routes — the route table changed shape") + require.GreaterOrEqualf(t, len(seen), minGetRoutes, + "walk found only %d GET routes under /api/v1 (< %d) — the walk likely matched a stale prefix; a near-empty walk must not pass", + len(seen), minGetRoutes) + + // Every walked route must be classified. This is the guard: a new GET route + // added without an entry above fails here until someone classifies it. + for route := range seen { + if _, ok := getRouteScopes[route]; !ok { + t.Errorf("GET %s is a new /api/v1 route with no scope classification — add it to "+ + "getRouteScopes as scopeRefused / scopeFiltered / scopeOpen, with a written reason "+ + "(and a scoped-caller test if it exposes per-server data)", route) + } + } + + // Every classified route must still exist, so a removed route's stale entry + // (and its now-meaningless reason) does not linger. + for route := range getRouteScopes { + require.Truef(t, seen[route], "getRouteScopes lists %s but the walk did not find it — remove the stale entry", route) + } + + // Drive each route with the scoped token and hold it to its verdict. + for route := range seen { + rc := getRouteScopes[route] + path := fillRouteParams(route) + t.Run(route, func(t *testing.T) { + rec := scopeGet(t, srv, path, token) + if rc.class == scopeRefused { + require.Equalf(t, http.StatusForbidden, rec.Code, + "%s is classified scopeRefused (%s) but did not answer 403: got %d body=%s", + route, rc.why, rec.Code, rec.Body.String()) + require.Containsf(t, rec.Body.String(), scopeDenialMarker, + "%s answered 403 but not the scope-layer denial body — is this the right refusal?", route) + return + } + // Routes that short-circuit before the gate on synthetic params: + // hold the exact pre-gate code, so a route that starts reaching the + // handler (and thus the gate) is caught and re-examined. + if want, ok := shortCircuitCodes[route]; ok { + require.Equalf(t, want, rec.Code, + "%s is a documented pre-gate short-circuit (%s) expected to answer %d, got %d body=%s — "+ + "if it now reaches the handler, drop it from shortCircuitCodes and require 2xx", + route, rc.why, want, rec.Code, rec.Body.String()) + return + } + // Everything else must actually reach the handler: a 2xx, never the + // fixed 403 and never a 4xx/5xx that died before the gate. "not 403" + // alone let a 404/503 masquerade as a reached-and-filtered route. + require.GreaterOrEqualf(t, rec.Code, http.StatusOK, + "%s is classified %v (%s) — a scoped token must reach the handler (2xx, filtered downstream), got %d body=%s", + route, rc.class, rc.why, rec.Code, rec.Body.String()) + require.Lessf(t, rec.Code, http.StatusMultipleChoices, + "%s is classified %v (%s) — a scoped token must reach the handler (2xx, filtered downstream), got %d body=%s. "+ + "If this is a legitimate pre-gate short-circuit, add it to shortCircuitCodes with a reason", + route, rc.class, rc.why, rec.Code, rec.Body.String()) + }) + } +} + +// TestDiagnostics_LegacyPathScopedCallerDoesNotSeeHiddenServer backs the +// scopeFiltered classification of /diagnostics (and /doctor) on the LEGACY path. +// +// withManagement:false selects the fallback branch of handleGetDiagnostics, +// whose own visibleServers() call is the gate under test — deleting that call +// leaks the hidden server here. The management path is a separate gate in +// internal/management.Doctor, proven by TestDoctor_ScopedTokenSeesOnlyAllowedServers; +// driving the httpapi handler with the management mock (as the earlier version of +// this test did) only exercised the mock's own filter loop, so dropping the real +// CanEnumerateServer check left it green. The legacy branch had no scoped-caller +// coverage before this. +func TestDiagnostics_LegacyPathScopedCallerDoesNotSeeHiddenServer(t *testing.T) { + // Both servers must carry an error so each surfaces in the legacy + // upstream-errors list when visible: beta already has one; give alpha one in + // this local copy so "alpha still present" is a real positive control and the + // beta assertion cannot pass against an empty report. The shared fixture is + // left untouched. + servers := scopeFixtureServers() + for i := range servers { + if servers[i].LastError == "" { + servers[i].LastError = "fixture error for " + servers[i].Name + } + } + ctrl := &scopeController{cfg: scopeFixtureConfig(false), servers: servers, withManagement: false} + srv, token := scopedAgentServer(t, ctrl, []string{"alpha"}) + + admin := scopeGet(t, srv, "/api/v1/diagnostics", scopeAdminAPIKey) + require.Equal(t, http.StatusOK, admin.Code) + require.Containsf(t, admin.Body.String(), "beta", + "positive control: an admin must see the hidden server on the legacy path, or this proves nothing about who was filtered: %s", admin.Body.String()) + + scoped := scopeGet(t, srv, "/api/v1/diagnostics", token) + require.Equal(t, http.StatusOK, scoped.Code, "diagnostics stays readable for a scoped caller, only narrowed") + body := scoped.Body.String() + require.Contains(t, body, "alpha", "the caller's own entitled server must still appear") + require.NotContainsf(t, body, "beta", "visibleServers must drop a server outside the token's grant from the legacy diagnostics path: %s", body) +}