Skip to content

Commit 2b40ae6

Browse files
committed
Make the security regressions behavioural
1 parent 2c4c99c commit 2b40ae6

5 files changed

Lines changed: 156 additions & 21 deletions

File tree

‎packages/plugins/mcp/src/sdk/appserver-connector.test.ts‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -298,24 +298,23 @@ describe("codex app-server bridge", () => {
298298
),
299299
);
300300

301-
it.effect("keeps a prototype-shaped argument as data, not as a prototype", () =>
301+
it.effect("carries a prototype-named argument through to the program as data", () =>
302302
Effect.scoped(
303303
Effect.gen(function* () {
304-
// Embedded as an object literal, `__proto__` sets the prototype rather
305-
// than an own key: the argument would disappear from `args.__proto__`
306-
// and reappear as inherited properties on every lookup the generated
307-
// program makes.
304+
// Parsed, not written as a literal: `{ __proto__: … }` in source sets
305+
// the prototype, so a literal fixture would assert nothing. This is
306+
// the shape that arrives over the wire.
307+
// oxlint-disable-next-line executor/no-json-parse -- an object literal would assign the PROTOTYPE; parsing produces the own key that actually arrives over the wire
308+
const args = JSON.parse('{"url":"https://example.com/","__proto__":{"polluted":"yes"}}');
308309
const connection = yield* withConnection(browserInput());
309310

310311
const result = yield* Effect.promise(() =>
311-
connection.client.callTool({
312-
name: "navigate",
313-
arguments: { url: "https://example.com/", __proto__: { polluted: "yes" } },
314-
}),
312+
connection.client.callTool({ name: "navigate", arguments: args }),
315313
);
316314
const program = (result.content as readonly { readonly text: string }[])[0]!.text;
317-
expect(program, "arguments are parsed as data").toContain("JSON.parse(");
318-
expect(program, "never emitted as an object literal").not.toContain("= {");
315+
316+
expect(program, "embedded as parsed data").toContain("JSON.parse(");
317+
expect(program, "with the key preserved rather than dropped").toContain("__proto__");
319318
}),
320319
),
321320
);

‎packages/plugins/mcp/src/sdk/codex-plugin-presets.test.ts‎

Lines changed: 39 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { describe, expect, it } from "@effect/vitest";
22

33
import { CURATED_CODEX_PLUGINS } from "./codex-plugin-presets";
4-
import { APPROVAL_TERM_KEYS } from "./invoke";
4+
import { approvalTerms } from "./invoke";
55
import { mcpPresets } from "./presets";
66

77
// ---------------------------------------------------------------------------
@@ -41,13 +41,43 @@ describe("codex catalog presets", () => {
4141
});
4242

4343
describe("approval terms", () => {
44-
// The projection lives at the MCP boundary (`invoke.ts`), but the vocabulary
45-
// it recognises is the contract these plugins speak, so pin it here.
46-
it("names only the keys that describe what accepting means", () => {
47-
// Codex's browser approval says the grant persists, and for which origin.
48-
// Everything else a server puts in `_meta` — progress tokens, internal
49-
// ids, opaque state — is not a term the user is agreeing to, and must not
50-
// be rendered as one.
51-
expect(APPROVAL_TERM_KEYS).toEqual(["persist", "origin", "connector_name", "connector_id"]);
44+
// `_meta` is an open map: servers put progress tokens, internal ids, and
45+
// opaque state in it. Rendering all of it as "approval terms" would both
46+
// mislead and risk surfacing something private, so the projection is a
47+
// closed vocabulary.
48+
it("keeps the keys that say what accepting means", () => {
49+
expect(
50+
approvalTerms({
51+
persist: "always",
52+
origin: "https://example.com",
53+
connector_name: "Browser use",
54+
connector_id: "browser-use",
55+
}),
56+
).toEqual({
57+
meta: {
58+
persist: "always",
59+
origin: "https://example.com",
60+
connector_name: "Browser use",
61+
connector_id: "browser-use",
62+
},
63+
});
64+
});
65+
66+
it("drops everything else a server attached", () => {
67+
expect(
68+
approvalTerms({
69+
persist: "always",
70+
progressToken: "tok_123",
71+
codex_approval_kind: "mcp_tool_call",
72+
internalSessionId: "sess_abc",
73+
nested: { secret: "do not render" },
74+
}),
75+
).toEqual({ meta: { persist: "always" } });
76+
});
77+
78+
it("ignores non-string values and contributes nothing when no term applies", () => {
79+
expect(approvalTerms({ persist: { always: true }, origin: 42 })).toEqual({});
80+
expect(approvalTerms({ progressToken: "tok" })).toEqual({});
81+
expect(approvalTerms(undefined)).toEqual({});
5282
});
5383
});
Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
import { describe, expect, it } from "@effect/vitest";
2+
3+
import { jsLiteral, jsString, writeJsonResult } from "./codex-repl";
4+
5+
// ---------------------------------------------------------------------------
6+
// Caller arguments are embedded into a JavaScript source text that runs in
7+
// Codex's REPL. The encoding is the whole security boundary, so it is tested
8+
// on the values that break naive embeddings rather than on happy text.
9+
// ---------------------------------------------------------------------------
10+
11+
/** The value the emitted expression produces, without evaluating JS: the
12+
* expression is `JSON.parse(<js string literal>)`, so unwrap the literal and
13+
* parse it the same way the REPL would. */
14+
const evaluate = (expression: string): unknown => {
15+
expect(expression.startsWith("JSON.parse(")).toBe(true);
16+
const literal = expression.slice("JSON.parse(".length, -1);
17+
// oxlint-disable-next-line executor/no-json-parse -- the subject under test IS the JSON embedding; decoding it exactly as the REPL would is the assertion, and a schema decode would hide the prototype behaviour being checked
18+
return JSON.parse(JSON.parse(literal) as string);
19+
};
20+
21+
describe("jsLiteral", () => {
22+
it("keeps a prototype-named key as an own key", () => {
23+
// Built by parsing, not as an object literal: writing `{ __proto__: … }`
24+
// in source sets the prototype, so a literal-based fixture would test
25+
// nothing. This is the shape that actually arrives over the wire.
26+
// oxlint-disable-next-line executor/no-json-parse -- an object literal would assign the PROTOTYPE, so parsing is the only way to build the own key this test is about
27+
const args: unknown = JSON.parse('{"__proto__":{"polluted":"yes"},"url":"https://x/"}');
28+
29+
const value = evaluate(jsLiteral(args)) as Record<string, unknown>;
30+
31+
expect(Object.hasOwn(value, "__proto__"), "carried as data").toBe(true);
32+
expect(Object.getPrototypeOf(value), "and not onto the prototype chain").toBe(Object.prototype);
33+
expect((value as { polluted?: unknown }).polluted).toBeUndefined();
34+
});
35+
36+
it("survives quotes, newlines, and JS line terminators", () => {
37+
// U+2028/U+2029 are legal raw inside a JSON string but END A STATEMENT in
38+
// a JS source text — the case that silently truncates a program.
39+
const args = { text: 'he said "hi"\n\u2028\u2029 and left', path: "C:\\tmp" };
40+
41+
expect(evaluate(jsLiteral(args))).toEqual(args);
42+
});
43+
44+
it("renders an absent value as null rather than a hole", () => {
45+
expect(evaluate(jsLiteral(undefined))).toBeNull();
46+
});
47+
});
48+
49+
describe("jsString", () => {
50+
it("escapes line terminators that would end the statement", () => {
51+
expect(jsString("a\u2028b")).toBe('"a\\u2028b"');
52+
// oxlint-disable-next-line executor/no-json-parse -- decoding the emitted JS string literal is the assertion
53+
expect(JSON.parse(jsString('quote " here')) as string).toBe('quote " here');
54+
});
55+
});
56+
57+
describe("writeJsonResult", () => {
58+
it("scopes the per-call body so a reused REPL session does not redeclare", () => {
59+
const program = writeJsonResult(["const a = 1;"], "await f(a)");
60+
61+
expect(program.startsWith("await (async () => {")).toBe(true);
62+
expect(program).toContain("nodeRepl.write(JSON.stringify(result ?? null));");
63+
});
64+
});

‎packages/plugins/mcp/src/sdk/connection-pool-key.test.ts‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,48 @@ describe("MCP connection-pool key", () => {
208208
},
209209
);
210210

211+
// Two connections under the SAME owner are still distinct sessions: a
212+
// grant made through one must not answer a prompt raised by the other.
213+
const sameOwnerOtherConnection = yield* connectionPoolKey(
214+
recipe,
215+
"none",
216+
{},
217+
{
218+
owner: "org",
219+
connection: "second",
220+
},
221+
);
222+
223+
expect(theirs).not.toBe(mine);
224+
expect(sameOwnerOtherConnection).not.toBe(mine);
225+
}),
226+
);
227+
228+
it.effect("separates remote connections that differ only by identity", () =>
229+
Effect.gen(function* () {
230+
// Hashed on the remote arm too: an open server with no credential would
231+
// otherwise pool one session across every connection.
232+
const input = remoteInput({ headers: {} });
233+
234+
const mine = yield* connectionPoolKey(
235+
input,
236+
"none",
237+
{},
238+
{
239+
owner: "org",
240+
connection: "default",
241+
},
242+
);
243+
const theirs = yield* connectionPoolKey(
244+
input,
245+
"none",
246+
{},
247+
{
248+
owner: "user:someone-else",
249+
connection: "default",
250+
},
251+
);
252+
211253
expect(theirs).not.toBe(mine);
212254
}),
213255
);

‎packages/plugins/mcp/src/sdk/invoke.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ const decodeElicitContent = Schema.decodeUnknownSync(
129129
* server contributes nothing rather than noise. */
130130
export const APPROVAL_TERM_KEYS = ["persist", "origin", "connector_name", "connector_id"] as const;
131131

132-
const approvalTerms = (meta: Record<string, unknown> | undefined) => {
132+
export const approvalTerms = (meta: Record<string, unknown> | undefined) => {
133133
if (meta === undefined) return {};
134134
const terms = Object.fromEntries(
135135
APPROVAL_TERM_KEYS.flatMap((key) => {

0 commit comments

Comments
 (0)