From 7d84e4ee03673ff602eeb2e243650fe211503678 Mon Sep 17 00:00:00 2001 From: "shaggy (mosaic-dev box)" Date: Mon, 10 Aug 2026 16:58:07 -0500 Subject: [PATCH] fix(gateway): sanitize raw exceptions in /mcp status + /reload sources (P3 re-review#5 blocker) --- .../src/mcp-client/mcp-client.service.spec.ts | 44 +++++++++++ .../src/mcp-client/mcp-client.service.ts | 2 +- .../gateway/src/reload/reload.service.spec.ts | 79 +++++++++++++++++++ apps/gateway/src/reload/reload.service.ts | 6 +- 4 files changed, 128 insertions(+), 3 deletions(-) create mode 100644 apps/gateway/src/mcp-client/mcp-client.service.spec.ts diff --git a/apps/gateway/src/mcp-client/mcp-client.service.spec.ts b/apps/gateway/src/mcp-client/mcp-client.service.spec.ts new file mode 100644 index 00000000..531299c9 --- /dev/null +++ b/apps/gateway/src/mcp-client/mcp-client.service.spec.ts @@ -0,0 +1,44 @@ +import { Logger } from '@nestjs/common'; +import { Client } from '@modelcontextprotocol/sdk/client/index.js'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { McpClientService } from './mcp-client.service.js'; + +const MCP_LEAK_MARKER = 'MCP_LEAK_MARKER /srv/secret'; + +describe('McpClientService — failed connect error sanitization', () => { + const originalMcpServers = process.env['MCP_SERVERS']; + + beforeEach(() => { + process.env['MCP_SERVERS'] = JSON.stringify([ + { name: 'leaky-server', url: 'http://localhost:9999/mcp' }, + ]); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (originalMcpServers === undefined) { + delete process.env['MCP_SERVERS']; + } else { + process.env['MCP_SERVERS'] = originalMcpServers; + } + }); + + it('stores a generic serverEntry.error while logging the raw exception server-side', async () => { + vi.spyOn(Client.prototype, 'connect').mockRejectedValue(new Error(MCP_LEAK_MARKER)); + const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + + const service = new McpClientService(); + await service.onModuleInit(); + + const statuses = service.getServerStatuses(); + expect(statuses).toHaveLength(1); + expect(statuses[0]?.connected).toBe(false); + expect(statuses[0]?.error).toBe('Connection failed (see server logs).'); + expect(statuses[0]?.error).not.toContain(MCP_LEAK_MARKER); + + const loggedRawMarker = errorSpy.mock.calls.some((call) => + call.some((arg) => typeof arg === 'string' && arg.includes(MCP_LEAK_MARKER)), + ); + expect(loggedRawMarker).toBe(true); + }); +}); diff --git a/apps/gateway/src/mcp-client/mcp-client.service.ts b/apps/gateway/src/mcp-client/mcp-client.service.ts index 28910e0d..6c304f70 100644 --- a/apps/gateway/src/mcp-client/mcp-client.service.ts +++ b/apps/gateway/src/mcp-client/mcp-client.service.ts @@ -189,7 +189,7 @@ export class McpClientService implements OnModuleInit, OnModuleDestroy { ); } catch (err) { const message = err instanceof Error ? err.message : String(err); - serverEntry.error = message; + serverEntry.error = 'Connection failed (see server logs).'; serverEntry.connected = false; this.logger.error(`Failed to connect to MCP server "${config.name}": ${message}`); } diff --git a/apps/gateway/src/reload/reload.service.spec.ts b/apps/gateway/src/reload/reload.service.spec.ts index 25696941..f939872a 100644 --- a/apps/gateway/src/reload/reload.service.spec.ts +++ b/apps/gateway/src/reload/reload.service.spec.ts @@ -1,5 +1,8 @@ +import { Logger } from '@nestjs/common'; import { describe, expect, it, vi } from 'vitest'; +import type { SlashCommandPayload, SystemReloadPayload } from '@mosaicstack/types'; import { ReloadService } from './reload.service.js'; +import { CommandExecutorService } from '../commands/command-executor.service.js'; function createMockCommandRegistry() { return { @@ -104,3 +107,79 @@ describe('ReloadService', () => { expect(() => service.registerPlugin('my-plugin', {})).not.toThrow(); }); }); + +describe('ReloadService — /reload command sanitizes plugin errors', () => { + it('generic per-plugin errors reach the chat surface while raw markers stay server-side only', async () => { + const registry = { + getManifest: vi.fn().mockReturnValue({ + version: 1, + commands: [ + { name: 'reload', aliases: [], scope: 'core', execution: 'socket', available: true }, + ], + skills: [], + }), + }; + const reloadService = new ReloadService(registry as never); + + const RELOAD_LOAD_LEAK_MARKER = 'RELOAD_LOAD_LEAK_MARKER /srv/load-secret'; + const RELOAD_UNLOAD_LEAK_MARKER = 'RELOAD_UNLOAD_LEAK_MARKER /srv/unload-secret'; + + reloadService.registerPlugin('unload-fails', { + pluginName: 'unload-fails', + onLoad: vi.fn().mockResolvedValue(undefined), + onUnload: vi.fn().mockRejectedValue(new Error(RELOAD_UNLOAD_LEAK_MARKER)), + }); + reloadService.registerPlugin('load-fails', { + pluginName: 'load-fails', + onLoad: vi.fn().mockRejectedValue(new Error(RELOAD_LOAD_LEAK_MARKER)), + onUnload: vi.fn().mockResolvedValue(undefined), + }); + + const errorSpy = vi.spyOn(Logger.prototype, 'error').mockImplementation(() => undefined); + const broadcastReload = vi.fn(); + const mockChatGateway = { broadcastReload }; + const mockAgentService = { getSession: vi.fn(), applyAgentConfig: vi.fn() }; + const mockSystemOverride = { set: vi.fn(), get: vi.fn(), clear: vi.fn() }; + const mockSessionGC = { sweepOrphans: vi.fn() }; + const mockBrain = { agents: { findByName: vi.fn(), findById: vi.fn(), create: vi.fn() } }; + + const executor = new CommandExecutorService( + registry as never, + mockAgentService as never, + mockSystemOverride as never, + mockSessionGC as never, + null, + mockBrain as never, + reloadService, + mockChatGateway as never, + null, + ); + + const payload: SlashCommandPayload = { command: 'reload', conversationId: 'conv-1' }; + const result = await executor.execute(payload, { userId: 'user-1', tenantId: 'user-1' }); + + expect(result.success).toBe(true); + expect(result.message).toContain('unload-fails: unload failed (internal error)'); + expect(result.message).toContain('load-fails: load failed (internal error)'); + expect(result.message).not.toContain(RELOAD_UNLOAD_LEAK_MARKER); + expect(result.message).not.toContain(RELOAD_LOAD_LEAK_MARKER); + + expect(broadcastReload).toHaveBeenCalledOnce(); + const broadcastPayload = broadcastReload.mock.calls[0]?.[0] as SystemReloadPayload; + expect(broadcastPayload.message).toContain('unload-fails: unload failed (internal error)'); + expect(broadcastPayload.message).toContain('load-fails: load failed (internal error)'); + expect(broadcastPayload.message).not.toContain(RELOAD_UNLOAD_LEAK_MARKER); + expect(broadcastPayload.message).not.toContain(RELOAD_LOAD_LEAK_MARKER); + + const loggedUnloadMarker = errorSpy.mock.calls.some((call) => + call.some((arg) => typeof arg === 'string' && arg.includes(RELOAD_UNLOAD_LEAK_MARKER)), + ); + const loggedLoadMarker = errorSpy.mock.calls.some((call) => + call.some((arg) => typeof arg === 'string' && arg.includes(RELOAD_LOAD_LEAK_MARKER)), + ); + expect(loggedUnloadMarker).toBe(true); + expect(loggedLoadMarker).toBe(true); + + errorSpy.mockRestore(); + }); +}); diff --git a/apps/gateway/src/reload/reload.service.ts b/apps/gateway/src/reload/reload.service.ts index 1d84e27b..b6b1f269 100644 --- a/apps/gateway/src/reload/reload.service.ts +++ b/apps/gateway/src/reload/reload.service.ts @@ -58,7 +58,8 @@ export class ReloadService implements OnApplicationBootstrap, OnApplicationShutd await plugin.onUnload(); reloaded.push(name); } catch (err) { - errors.push(`${name}: unload failed — ${err}`); + this.logger.error(`Plugin "${name}" failed during onUnload: ${err}`); + errors.push(`${name}: unload failed (internal error)`); } } } @@ -69,7 +70,8 @@ export class ReloadService implements OnApplicationBootstrap, OnApplicationShutd try { await plugin.onLoad(); } catch (err) { - errors.push(`${name}: load failed — ${err}`); + this.logger.error(`Plugin "${name}" failed during onLoad: ${err}`); + errors.push(`${name}: load failed (internal error)`); } } }