mcpctl/src/mcpd/tests/security.test.ts

/**
 * Security tests for mcpd.
 *
 * Tests for identified security issues:
 * 1. audit-events endpoint bypasses RBAC (mapUrlToPermission returns 'skip')
 * 2. x-service-account header impersonation (any authenticated user can set it)
 * 3. MCP proxy maps to wrong RBAC action (POST → 'create' instead of 'run')
 * 4. externalUrl has no scheme/destination restriction (SSRF)
 * 5. MCP proxy has no input validation on method/serverId
 * 6. RBAC list filtering only checks 'name' field
 */
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import Fastify from 'fastify';
import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify';
import { registerMcpProxyRoutes } from '../src/routes/mcp-proxy.js';
import type { McpProxyRouteDeps } from '../src/routes/mcp-proxy.js';
import { registerAuditEventRoutes } from '../src/routes/audit-events.js';
import { AuditEventService } from '../src/services/audit-event.service.js';
import type { IAuditEventRepository } from '../src/repositories/interfaces.js';
import { errorHandler } from '../src/middleware/error-handler.js';
import { CreateMcpServerSchema } from '../src/validation/mcp-server.schema.js';

// ─────────────────────────────────────────────────────────
// § 1  audit-events endpoint bypasses RBAC
// ─────────────────────────────────────────────────────────

/**
 * Reproduce mapUrlToPermission from main.ts to test which URLs
 * get RBAC checks and which are skipped.
 */
type PermissionCheck =
  | { kind: 'resource'; resource: string; action: string; resourceName?: string }
  | { kind: 'operation'; operation: string }
  | { kind: 'skip' };

function mapUrlToPermission(method: string, url: string): PermissionCheck {
  const match = url.match(/^\/api\/v1\/([a-z-]+)/);
  if (!match) return { kind: 'skip' };

  const segment = match[1] as string;

  if (segment === 'backup') return { kind: 'operation', operation: 'backup' };
  if (segment === 'restore') return { kind: 'operation', operation: 'restore' };
  if (segment === 'audit-logs' && method === 'DELETE') return { kind: 'operation', operation: 'audit-purge' };

  const resourceMap: Record<string, string | undefined> = {
    'servers': 'servers',
    'instances': 'instances',
    'secrets': 'secrets',
    'projects': 'projects',
    'templates': 'templates',
    'users': 'users',
    'groups': 'groups',
    'rbac': 'rbac',
    'audit-logs': 'rbac',
    'mcp': 'servers',
    'prompts': 'prompts',
    'promptrequests': 'promptrequests',
  };

  const resource = resourceMap[segment];
  if (resource === undefined) return { kind: 'skip' };

  let action: string;
  switch (method) {
    case 'GET':
    case 'HEAD':
      action = 'view';
      break;
    case 'POST':
      action = 'create';
      break;
    case 'DELETE':
      action = 'delete';
      break;
    default:
      action = 'edit';
      break;
  }

  const nameMatch = url.match(/^\/api\/v1\/[a-z-]+\/([^/?]+)/);
  const resourceName = nameMatch?.[1];
  const check: PermissionCheck = { kind: 'resource', resource, action };
  if (resourceName !== undefined) (check as { resourceName: string }).resourceName = resourceName;
  return check;
}

describe('Security: RBAC coverage gaps in mapUrlToPermission', () => {
  it('audit-events endpoint is NOT in resourceMap — bypasses RBAC', () => {
    // This documents a known security issue: any authenticated user can query
    // all audit events regardless of their RBAC permissions
    const check = mapUrlToPermission('GET', '/api/v1/audit-events');
    // Currently returns 'skip' — this is the bug
    expect(check.kind).toBe('skip');
  });

  it('audit-events POST (batch insert) also bypasses RBAC', () => {
    const check = mapUrlToPermission('POST', '/api/v1/audit-events');
    expect(check.kind).toBe('skip');
  });

  it('audit-events by ID also bypasses RBAC', () => {
    const check = mapUrlToPermission('GET', '/api/v1/audit-events/some-cuid');
    expect(check.kind).toBe('skip');
  });

  it('all known resource endpoints DO have RBAC coverage', () => {
    const coveredEndpoints = [
      'servers', 'instances', 'secrets', 'projects', 'templates',
      'users', 'groups', 'rbac', 'audit-logs', 'prompts', 'promptrequests',
    ];

    for (const endpoint of coveredEndpoints) {
      const check = mapUrlToPermission('GET', `/api/v1/${endpoint}`);
      expect(check.kind, `${endpoint} should have RBAC check`).not.toBe('skip');
    }
  });

  it('MCP proxy maps POST to servers:create instead of servers:run', () => {
    // /api/v1/mcp/proxy is a POST that executes tools — semantically this is
    // a 'run' action, but mapUrlToPermission maps POST → 'create'
    const check = mapUrlToPermission('POST', '/api/v1/mcp/proxy');
    expect(check.kind).toBe('resource');
    if (check.kind === 'resource') {
      expect(check.resource).toBe('servers');
      // BUG: should be 'run' for executing tools, not 'create'
      expect(check.action).toBe('create');
    }
  });

  it('non-api URLs correctly return skip', () => {
    expect(mapUrlToPermission('GET', '/healthz').kind).toBe('skip');
    expect(mapUrlToPermission('GET', '/health').kind).toBe('skip');
    expect(mapUrlToPermission('GET', '/').kind).toBe('skip');
  });
});

// ─────────────────────────────────────────────────────────
// § 2  x-service-account header impersonation
// ─────────────────────────────────────────────────────────

describe('Security: x-service-account header impersonation', () => {
  // This test documents that any authenticated user can impersonate service accounts
  // by setting the x-service-account header. The RBAC service trusts this header
  // and adds the service account's permissions to the user's permissions.

  it('x-service-account header is passed to RBAC without verification', () => {
    // The RBAC service's getPermissions() accepts serviceAccountName directly.
    // In main.ts, the value comes from: request.headers['x-service-account']
    // There is no validation that the authenticated user IS the service account,
    // or that the user is authorized to act as that service account.
    //
    // Attack scenario:
    // 1. Attacker authenticates as regular user (low-privilege)
    // 2. Sends request with header: x-service-account: project:admin
    // 3. RBAC service treats them as having the service account's bindings
    // 4. Attacker gets elevated permissions

    // We verify this by examining the RBAC service code path:
    // In rbac.service.ts line 144:
    //   if (s.kind === 'ServiceAccount') return serviceAccountName !== undefined && s.name === serviceAccountName;
    // This matches ANY request with the right header value — no ownership check.
    expect(true).toBe(true); // Structural documentation test
  });
});

// ─────────────────────────────────────────────────────────
// § 3  MCP proxy input validation
// ─────────────────────────────────────────────────────────

describe('Security: MCP proxy input validation', () => {
  let app: FastifyInstance;

  afterEach(async () => {
    if (app) await app.close();
  });

  function buildApp() {
    const mcpProxyService = {
      execute: vi.fn(async () => ({
        jsonrpc: '2.0' as const,
        id: 1,
        result: { tools: [] },
      })),
    };
    const auditLogService = {
      create: vi.fn(async () => ({ id: 'log-1' })),
    };
    const authDeps = {
      findSession: vi.fn(async () => ({
        userId: 'user-1',
        expiresAt: new Date(Date.now() + 3600_000),
      })),
    };

    app = Fastify({ logger: false });
    app.setErrorHandler(errorHandler);
    registerMcpProxyRoutes(app, {
      mcpProxyService,
      auditLogService,
      authDeps,
    } as unknown as McpProxyRouteDeps);
    return { mcpProxyService, auditLogService };
  }

  it('accepts arbitrary method strings (no allowlist)', async () => {
    // Any JSON-RPC method is forwarded to upstream servers without validation.
    // An attacker could send methods like 'shutdown', 'admin/reset', etc.
    const { mcpProxyService } = buildApp();

    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/mcp/proxy',
      payload: {
        serverId: 'srv-1',
        method: 'dangerous/admin_shutdown',
        params: {},
      },
      headers: { authorization: 'Bearer valid-token' },
    });

    // Request succeeds — method is forwarded without validation
    expect(res.statusCode).toBe(200);
    expect(mcpProxyService.execute).toHaveBeenCalledWith({
      serverId: 'srv-1',
      method: 'dangerous/admin_shutdown',
      params: {},
    });
  });

  it('accepts empty method string', async () => {
    const { mcpProxyService } = buildApp();

    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/mcp/proxy',
      payload: {
        serverId: 'srv-1',
        method: '',
        params: {},
      },
      headers: { authorization: 'Bearer valid-token' },
    });

    expect(res.statusCode).toBe(200);
    expect(mcpProxyService.execute).toHaveBeenCalledWith(
      expect.objectContaining({ method: '' }),
    );
  });

  it('no Zod schema validation on request body', async () => {
    // The route destructures body without schema validation.
    // Extra fields are silently accepted.
    const { mcpProxyService } = buildApp();

    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/mcp/proxy',
      payload: {
        serverId: 'srv-1',
        method: 'tools/list',
        params: {},
        __proto__: { isAdmin: true },
        extraField: 'injected',
      },
      headers: { authorization: 'Bearer valid-token' },
    });

    expect(res.statusCode).toBe(200);
  });
});

// ─────────────────────────────────────────────────────────
// § 4  externalUrl SSRF validation
// ─────────────────────────────────────────────────────────

describe('Security: externalUrl SSRF via CreateMcpServerSchema', () => {
  it('accepts internal IP addresses (SSRF risk)', () => {
    // externalUrl uses z.string().url() which validates format but not destination
    const internalUrls = [
      'http://169.254.169.254/latest/meta-data/',    // AWS metadata
      'http://metadata.google.internal/',              // GCP metadata
      'http://100.100.100.200/latest/meta-data/',      // Alibaba Cloud metadata
      'http://10.0.0.1/',                              // Private network
      'http://192.168.1.1/',                           // Private network
      'http://172.16.0.1/',                            // Private network
      'http://127.0.0.1:3100/',                        // Localhost (mcpd itself!)
      'http://[::1]:3100/',                            // IPv6 localhost
      'http://0.0.0.0/',                               // All interfaces
    ];

    for (const url of internalUrls) {
      const result = CreateMcpServerSchema.safeParse({
        name: 'test-server',
        externalUrl: url,
      });
      // All currently pass validation — this is the SSRF vulnerability
      expect(result.success, `${url} should be flagged but currently passes`).toBe(true);
    }
  });

  it('accepts file:// URLs', () => {
    const result = CreateMcpServerSchema.safeParse({
      name: 'test-server',
      externalUrl: 'file:///etc/passwd',
    });
    // z.string().url() validates format, and file:// is a valid URL scheme
    // Whether this passes or fails depends on the Zod version's url() validator
    // This test documents the current behavior
    if (result.success) {
      // If this passes, it's an additional SSRF vector
      expect(result.data.externalUrl).toBe('file:///etc/passwd');
    }
  });

  it('correctly validates URL format', () => {
    const invalid = CreateMcpServerSchema.safeParse({
      name: 'test-server',
      externalUrl: 'not-a-url',
    });
    expect(invalid.success).toBe(false);
  });
});

// ─────────────────────────────────────────────────────────
// § 5  Audit events route — unauthenticated batch insert
// ─────────────────────────────────────────────────────────

describe('Security: audit-events batch insert has no auth in route definition', () => {
  let app: FastifyInstance;
  let repo: IAuditEventRepository;

  beforeEach(async () => {
    app = Fastify({ logger: false });
    app.setErrorHandler(errorHandler);
    repo = {
      findAll: vi.fn(async () => []),
      findById: vi.fn(async () => null),
      createMany: vi.fn(async (events: unknown[]) => events.length),
      count: vi.fn(async () => 0),
    };
    const service = new AuditEventService(repo);
    registerAuditEventRoutes(app, service);
    await app.ready();
  });

  afterEach(async () => {
    if (app) await app.close();
  });

  it('batch insert accepts events without authentication at route level', async () => {
    // The route itself has no preHandler auth middleware (unlike mcp-proxy).
    // Auth is only applied via the global hook in main.ts.
    // If registerAuditEventRoutes is used outside of main.ts's global hook setup,
    // audit events can be inserted without auth.
    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/audit/events',
      payload: [
        {
          timestamp: new Date().toISOString(),
          sessionId: 'fake-session',
          projectName: 'injected-project',
          eventKind: 'gate_decision',
          source: 'attacker',
          verified: true,   // Attacker can claim verified=true
          payload: { trigger: 'fake', intent: 'malicious' },
        },
      ],
    });

    // Without global auth hook, this succeeds
    expect(res.statusCode).toBe(201);
    expect(repo.createMany).toHaveBeenCalled();
  });

  it('attacker can inject events with verified=true (no server-side enforcement)', async () => {
    // The verified flag is accepted from the client without validation.
    // mcplocal (which runs on untrusted user devices) sends verified=true for its events.
    // An attacker could inject fake "verified" events to pollute the audit trail.
    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/audit/events',
      payload: [
        {
          timestamp: new Date().toISOString(),
          sessionId: 'attacker-session',
          projectName: 'target-project',
          eventKind: 'gate_decision',
          source: 'mcpd',      // Impersonate mcpd as source
          verified: true,       // Claim it's verified
          payload: { trigger: 'begin_session', intent: 'legitimate looking' },
        },
      ],
    });

    expect(res.statusCode).toBe(201);

    // Verify the event was stored with attacker-controlled values
    const storedEvents = (repo.createMany as ReturnType<typeof vi.fn>).mock.calls[0]![0] as Array<Record<string, unknown>>;
    expect(storedEvents[0]).toMatchObject({
      source: 'mcpd',
      verified: true,
    });
  });

  it('attacker can inject events for any project', async () => {
    const res = await app.inject({
      method: 'POST',
      url: '/api/v1/audit/events',
      payload: [
        {
          timestamp: new Date().toISOString(),
          sessionId: 'attacker-session',
          projectName: 'production-sensitive-project',
          eventKind: 'tool_call_trace',
          source: 'mcplocal',
          verified: true,
          payload: { toolName: 'legitimate_tool' },
        },
      ],
    });

    expect(res.statusCode).toBe(201);
  });
});

// ─────────────────────────────────────────────────────────
// § 6  RBAC list filtering only checks 'name' field
// ─────────────────────────────────────────────────────────

describe('Security: RBAC list filtering gaps', () => {
  it('preSerialization hook only filters by name field', () => {
    // From main.ts lines 390-397:
    // The hook filters array responses by checking item['name'].
    // Resources without a 'name' field pass through unfiltered.
    //
    // Affected resources:
    // - AuditEvent (has no 'name' field → never filtered)
    // - AuditLog (has no 'name' field → never filtered)
    // - Any future resource without a 'name' field

    // Simulate the filtering logic
    const payload = [
      { id: '1', name: 'allowed-server', description: 'visible' },
      { id: '2', name: 'forbidden-server', description: 'should be hidden' },
      { id: '3', description: 'no name field — passes through' },
    ];

    const rbacScope = { wildcard: false, names: new Set(['allowed-server']) };

    // Apply the filtering logic from main.ts
    const filtered = payload.filter((item) => {
      const name = item['name' as keyof typeof item];
      return typeof name === 'string' && rbacScope.names.has(name);
    });

    // Items with matching name are included
    expect(filtered).toHaveLength(1);
    expect(filtered[0]!.name).toBe('allowed-server');

    // BUG: Items without a name field are EXCLUDED, not leaked through.
    // Actually re-reading: typeof undefined === 'undefined', so the filter
    // returns false for items without name. This means nameless items are
    // EXCLUDED when rbacScope is active — which may cause audit events to
    // disappear from filtered responses. Not a leak, but a usability issue.
  });

  it('wildcard scope bypasses all filtering', () => {
    const rbacScope = { wildcard: true, names: new Set<string>() };

    // When wildcard is true, the hook returns payload as-is
    // This is correct behavior — wildcard means "see everything"
    expect(rbacScope.wildcard).toBe(true);
  });
});