Merge pull request 'test(#244): Part B backlog — editor-ext/mcp/client/server unit+contract tests + findBreadcrumbPath mutation fix' (#257) from test/244-part-b into develop

Reviewed-on: #257
2026-06-30 01:36:00 +03:00
parent d38a39e3e5 c4ab03d387
commit a8a7fad850
23 changed files with 2103 additions and 78 deletions
@@ -0,0 +1,166 @@
+import { McpClientsService } from './mcp-clients.service';
+
+/**
+ * Unit tests for the two security-critical surfaces of McpClientsService that the
+ * sibling specs (ssrf-guard / validate-resolved-addresses / lease) do NOT cover:
+ *
+ *  1. `decryptHeaders` (private) — FAIL-OPEN behavior. A decrypt/parse failure
+ *     (e.g. APP_SECRET rotated, tampered blob) must NEVER throw and must NEVER
+ *     log the blob: it returns `undefined` so the connect proceeds WITHOUT the
+ *     now-unreadable auth headers (which then 401s and the server is skipped),
+ *     rather than crashing the whole turn.
+ *
+ *  2. `this.guardedFetch` (private, bound to the SSRF-pinned dispatcher) — the
+ *     per-request DNS-rebinding guard. A blocked host (private/loopback/metadata
+ *     IP literal, or an unparseable URL) must REJECT before any socket is opened;
+ *     a public host is allowed through to the real `fetch` with the pinned
+ *     dispatcher attached.
+ *
+ * No network and no DB: the repo + secretBox deps are stubbed, and global `fetch`
+ * is mocked for the single allow-path assertion.
+ */
+
+// Build the service with a SecretBoxService stub whose decryptSecret is supplied
+// per-test. The repo dep is unused by the methods under test.
+function buildService(decryptSecret: (blob: string) => string) {
+  const secretBox = { decryptSecret: jest.fn(decryptSecret) };
+  const service = new McpClientsService({} as never, secretBox as never);
+  return { service, secretBox };
+}
+
+describe('McpClientsService.decryptHeaders', () => {
+  // Reach the private method via the as-any pattern common in these NestJS specs.
+  const callDecrypt = (
+    service: McpClientsService,
+    blob: string | null,
+  ): Record<string, string> | undefined =>
+    (
+      service as unknown as {
+        decryptHeaders: (b: string | null) => Record<string, string> | undefined;
+      }
+    ).decryptHeaders(blob);
+
+  it('returns undefined for a null blob without decrypting', () => {
+    const { service, secretBox } = buildService(() => '{}');
+    expect(callDecrypt(service, null)).toBeUndefined();
+    expect(secretBox.decryptSecret).not.toHaveBeenCalled();
+  });
+
+  it('decrypts a valid blob and keeps only string-valued headers', () => {
+    const { service } = buildService(() =>
+      JSON.stringify({
+        Authorization: 'Bearer abc',
+        'X-Api-Key': 'k',
+        // Non-string values must be dropped, not coerced.
+        count: 5,
+        flag: true,
+        nested: { a: 1 },
+      }),
+    );
+    expect(callDecrypt(service, 'cipher')).toEqual({
+      Authorization: 'Bearer abc',
+      'X-Api-Key': 'k',
+    });
+  });
+
+  it('returns undefined when the decrypted object has no string headers', () => {
+    const { service } = buildService(() => JSON.stringify({ count: 5 }));
+    // No usable headers -> undefined (connect with no auth header), not {}.
+    expect(callDecrypt(service, 'cipher')).toBeUndefined();
+  });
+
+  it('FAILS OPEN: a decrypt error returns undefined instead of throwing', () => {
+    const { service } = buildService(() => {
+      throw new Error('Failed to decrypt secret — APP_SECRET may have changed');
+    });
+    const warnSpy = jest
+      .spyOn(
+        (service as unknown as { logger: { warn: (...a: unknown[]) => void } })
+          .logger,
+        'warn',
+      )
+      .mockImplementation(() => undefined);
+
+    let result: unknown;
+    expect(() => {
+      result = callDecrypt(service, 'tampered-blob');
+    }).not.toThrow();
+    expect(result).toBeUndefined();
+    // It warns (so ops sees degradation) but never logs the blob itself.
+    expect(warnSpy).toHaveBeenCalledTimes(1);
+    expect(String(warnSpy.mock.calls[0]?.[0])).not.toContain('tampered-blob');
+  });
+
+  it('FAILS OPEN: malformed JSON (decrypts to non-JSON) returns undefined', () => {
+    const { service } = buildService(() => 'not-json{');
+    jest
+      .spyOn(
+        (service as unknown as { logger: { warn: (...a: unknown[]) => void } })
+          .logger,
+        'warn',
+      )
+      .mockImplementation(() => undefined);
+    expect(callDecrypt(service, 'cipher')).toBeUndefined();
+  });
+});
+
+describe('McpClientsService.guardedFetch (SSRF per-request guard)', () => {
+  // The bound guardedFetch closure lives on the instance as a private field.
+  const guardedFetchOf = (service: McpClientsService) =>
+    (service as unknown as { guardedFetch: typeof fetch }).guardedFetch;
+
+  let fetchSpy: jest.SpiedFunction<typeof fetch>;
+
+  beforeEach(() => {
+    // Any reachable real fetch would be a network call; assert per-test that the
+    // blocked paths never reach it, and stub a Response for the allow path.
+    fetchSpy = jest
+      .spyOn(global, 'fetch')
+      .mockResolvedValue(new Response('ok', { status: 200 }));
+  });
+
+  afterEach(() => {
+    jest.restoreAllMocks();
+  });
+
+  const blocked: Array<[string, string]> = [
+    ['loopback IPv4', 'http://127.0.0.1/mcp'],
+    ['private 10/8', 'http://10.0.0.5/mcp'],
+    ['private 192.168/16', 'http://192.168.1.1/mcp'],
+    ['cloud metadata link-local', 'http://169.254.169.254/latest/meta-data/'],
+    ['loopback IPv6 (bracketed)', 'http://[::1]:8080/mcp'],
+  ];
+
+  it.each(blocked)(
+    'rejects a request to %s without opening a socket',
+    async (_label, url) => {
+      const { service } = buildService(() => '{}');
+      await expect(guardedFetchOf(service)(url)).rejects.toThrow(
+        /blocked request/,
+      );
+      expect(fetchSpy).not.toHaveBeenCalled();
+    },
+  );
+
+  it('rejects an unparseable URL as a blocked request', async () => {
+    const { service } = buildService(() => '{}');
+    await expect(
+      guardedFetchOf(service)('::: not a url :::'),
+    ).rejects.toThrow('blocked request: invalid URL');
+    expect(fetchSpy).not.toHaveBeenCalled();
+  });
+
+  it('allows a public IP literal and forwards through the pinned dispatcher', async () => {
+    const { service } = buildService(() => '{}');
+    const res = await guardedFetchOf(service)('http://8.8.8.8/mcp');
+
+    expect(res.status).toBe(200);
+    expect(fetchSpy).toHaveBeenCalledTimes(1);
+    // The init MUST carry the SSRF-pinned undici dispatcher (the rebinding pin);
+    // dropping it would let undici do a second, unchecked DNS resolution.
+    const init = fetchSpy.mock.calls[0][1] as RequestInit & {
+      dispatcher?: unknown;
+    };
+    expect(init.dispatcher).toBeDefined();
+  });
+});
@@ -0,0 +1,124 @@
+import { z } from 'zod';
+import { AiChatToolsService } from './ai-chat-tools.service';
+import * as loader from './docmost-client.loader';
+import type { DocmostClientLike } from './docmost-client.loader';
+// The real zod-agnostic registry, imported from source so the contract is checked
+// against exactly what the @docmost/mcp package ships (no hand-stub).
+import { SHARED_TOOL_SPECS } from '../../../../../../packages/mcp/src/tool-specs';
+
+/**
+ * CONTRACT: SHARED_TOOL_SPECS <-> in-app tool wiring parity.
+ *
+ * `packages/mcp/src/tool-specs.ts` is the single source of truth for the tools
+ * that are intentionally IDENTICAL across the standalone MCP server (zod v3) and
+ * the in-app AI-SDK service (zod v4). The in-app service builds each one via
+ * `sharedTool(sharedToolSpecs.<key>, execute)`, keyed by the spec's `inAppKey`.
+ *
+ * This test fails the build if a spec is added to the registry but never wired
+ * in-app, if an `inAppKey` is renamed without updating the service, if the
+ * description drifts between the registry and the exposed tool, if the
+ * snake_case `mcpName` <-> camelCase `inAppKey` convention is broken, or if the
+ * exposed tool's input-schema keys diverge from the spec's `buildShape`.
+ *
+ * It does NOT need @docmost/mcp built: the registry is imported from TS source,
+ * and the ESM loader is mocked so `forUser()` never dynamically imports the
+ * package.
+ */
+describe('SHARED_TOOL_SPECS contract parity', () => {
+  // Empty fake client: no tool is executed here — every assertion is on tool
+  // presence / metadata / schema, so the client methods are never called.
+  const fakeClient: Partial<DocmostClientLike> = {};
+  const tokenServiceStub = {
+    generateAccessToken: jest.fn().mockResolvedValue('access-token'),
+    generateCollabToken: jest.fn().mockResolvedValue('collab-token'),
+  };
+
+  let tools: Record<string, unknown>;
+
+  beforeAll(async () => {
+    jest.spyOn(loader, 'loadDocmostMcp').mockResolvedValue({
+      DocmostClient: function () {
+        return fakeClient as DocmostClientLike;
+      } as unknown as loader.DocmostClientCtor,
+      // Feed the service the SAME registry this test asserts against.
+      sharedToolSpecs: SHARED_TOOL_SPECS as unknown as Record<
+        string,
+        loader.SharedToolSpec
+      >,
+    });
+    const service = new AiChatToolsService(
+      tokenServiceStub as never,
+      {} as never,
+      {} as never,
+      {} as never,
+      {} as never,
+      { asSink: () => ({ put: jest.fn(), has: jest.fn(), evict: jest.fn() }) } as never,
+    );
+    tools = (await service.forUser(
+      { id: 'user-1', email: 'u@example.com', workspaceId: 'ws-1' } as never,
+      'session-1',
+      'ws-1',
+      'chat-1',
+    )) as unknown as Record<string, unknown>;
+  });
+
+  afterAll(() => jest.restoreAllMocks());
+
+  // camelCase -> snake_case, matching the registry's mcpName convention.
+  const toSnake = (s: string) =>
+    s.replace(/[A-Z]/g, (c) => `_${c.toLowerCase()}`);
+
+  // Type as the (optional-buildShape) SharedToolSpec; the `satisfies` literal
+  // above otherwise narrows to a union where some members lack buildShape.
+  const specEntries = Object.entries(SHARED_TOOL_SPECS) as Array<
+    [string, loader.SharedToolSpec]
+  >;
+
+  // Sanity: the registry is non-empty, so the per-spec table below is not vacuous.
+  it('registry is non-empty', () => {
+    expect(specEntries.length).toBeGreaterThan(0);
+  });
+
+  describe.each(specEntries)('spec "%s"', (registryKey, spec) => {
+    it('registry key equals its inAppKey', () => {
+      // The service indexes the registry by property name; a key != inAppKey
+      // would wire the wrong (or no) tool.
+      expect(spec.inAppKey).toBe(registryKey);
+    });
+
+    it('mcpName is the snake_case form of inAppKey', () => {
+      expect(spec.mcpName).toBe(toSnake(spec.inAppKey));
+    });
+
+    it('is exposed in-app under its inAppKey', () => {
+      // Fails if a spec is added to the registry but never wired in forUser().
+      expect(tools[spec.inAppKey]).toBeDefined();
+    });
+
+    it("exposed tool's description matches the registry description", () => {
+      const tool = tools[spec.inAppKey] as { description: string };
+      expect(tool.description).toBe(spec.description);
+    });
+
+    it("exposed tool's input-schema keys match buildShape (incl. required)", () => {
+      const tool = tools[spec.inAppKey] as {
+        inputSchema: { jsonSchema: { properties?: Record<string, unknown>; required?: string[] } };
+      };
+      const json = tool.inputSchema.jsonSchema;
+      const actualKeys = Object.keys(json.properties ?? {}).sort();
+
+      // Derive the spec's declared shape with THIS layer's zod (v4) — the same
+      // call the service makes — then compare key sets and required-ness.
+      const shape = spec.buildShape ? spec.buildShape(z) : {};
+      const expectedKeys = Object.keys(shape).sort();
+      expect(actualKeys).toEqual(expectedKeys);
+
+      // A non-.optional() field must surface as required in the advertised schema.
+      const expectedRequired = Object.entries(shape)
+        .filter(([, field]) => !(field as z.ZodTypeAny).isOptional?.())
+        .map(([k]) => k)
+        .sort();
+      expect((json.required ?? []).slice().sort()).toEqual(expectedRequired);
+    });
+  });
+});