Skip to content

Commit 3af2578

Browse files
committed
Enforce director allowlist inside the exec tool promoter
The promoter filtered outside-allow names only because its one call site pre-filtered in the activate closure; a second caller wiring activate raw would get no gate. The promoter now takes isAllowed and filters itself, with the exec call site passing the overlay check and the product-default test path passing allow-all. An allow list that deny empties now throws at overlay resolution, matching the deny-without-allow behavior, instead of locking the director out silently.
1 parent 0ae22a8 commit 3af2578

3 files changed

Lines changed: 46 additions & 9 deletions

File tree

‎src/exec/runner.test.ts‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,14 @@ describe("exec director allowlist", () => {
4444
]);
4545
});
4646

47+
test("an allow that deny empties is rejected loudly", () => {
48+
const pkg = {
49+
...DIRECTOR_REGISTRY.explorer,
50+
tools: { allow: ["run_shell"], deny: ["run_shell"] },
51+
};
52+
expect(() => resolveExecDirectorOverlayForPackage(pkg)).toThrow(/empty/);
53+
});
54+
4755
test("a deny-only package config is rejected loudly", () => {
4856
const pkg = {
4957
...DIRECTOR_REGISTRY.explorer,
@@ -63,10 +71,8 @@ describe("exec director allowlist", () => {
6371
builtInPrefix: overlay.advertisedAllow,
6472
});
6573
const promote = createExecToolPromoter({
66-
activate: (names) =>
67-
activated.activate(
68-
names.filter((name) => isExecOverlayToolAllowed(overlay, name)),
69-
),
74+
activate: (names) => activated.activate(names),
75+
isAllowed: (name) => isExecOverlayToolAllowed(overlay, name),
7076
currentDefinitions: () => [],
7177
computeAdvertised,
7278
updateDirectorTools: () => undefined,
@@ -79,6 +85,30 @@ describe("exec director allowlist", () => {
7985
).toBe(false);
8086
});
8187

88+
test("the promoter gates allow itself — a raw activate caller gets no bypass", () => {
89+
const overlay = resolveExecDirectorOverlay("explorer");
90+
const { activated, computeAdvertised } = createAdvertisedToolset({
91+
sessionMode: "orchestrator",
92+
toolAvailability: { languageServerAvailable: true },
93+
getProvider: () => ({ providerName: "test", model: "test-model" }),
94+
builtInPrefix: overlay.advertisedAllow,
95+
});
96+
let advertisedCount = 0;
97+
const promote = createExecToolPromoter({
98+
activate: (names) => activated.activate(names),
99+
isAllowed: (name) => isExecOverlayToolAllowed(overlay, name),
100+
currentDefinitions: () => [],
101+
computeAdvertised,
102+
updateDirectorTools: () => {
103+
advertisedCount += 1;
104+
},
105+
});
106+
promote([OUTSIDE_ALLOW, "read_file"]);
107+
expect(activated.has(OUTSIDE_ALLOW)).toBe(false);
108+
expect(activated.has("read_file")).toBe(true);
109+
expect(advertisedCount).toBe(1);
110+
});
111+
82112
test("skywalker overlay leaves every tool allowed", () => {
83113
const overlay = resolveExecDirectorOverlay("skywalker");
84114
expect(isExecOverlayToolAllowed(overlay, OUTSIDE_ALLOW)).toBe(true);

‎src/exec/runner.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,13 @@ export function resolveExecDirectorOverlayForPackage(
299299
allow !== undefined && allow.length > 0
300300
? allow.filter((name) => !deny.includes(name))
301301
: undefined;
302+
if (allowed !== undefined && allowed.length === 0) {
303+
throw new Error(
304+
`Director package "${pkg.id}" tools.allow minus tools.deny is empty — ` +
305+
"exec overlays enforce a closed allow list, so no tool would be " +
306+
"advertised. Keep an allow entry outside tools.deny.",
307+
);
308+
}
302309
const advertisedAllow =
303310
allowed !== undefined
304311
? pkg.spawn.maySpawn
@@ -391,13 +398,14 @@ export function createExecToolCallGate(
391398

392399
export function createExecToolPromoter(args: {
393400
activate: (names: readonly string[]) => boolean;
401+
isAllowed: (name: string) => boolean;
394402
currentDefinitions: () => readonly ToolDefinition[];
395403
computeAdvertised: (all: readonly ToolDefinition[]) => ToolDefinition[];
396404
updateDirectorTools: (defs: ToolDefinition[]) => void;
397405
persist?: () => void;
398406
}): (names: string[]) => void {
399407
return (names) => {
400-
if (!args.activate(names)) return;
408+
if (!args.activate(names.filter((name) => args.isAllowed(name)))) return;
401409
args.updateDirectorTools(args.computeAdvertised(args.currentDefinitions()));
402410
args.persist?.();
403411
};
@@ -880,10 +888,8 @@ export async function runExec(config: Config): Promise<ExecResult> {
880888
// names, so outside-allow tools can never become advertised or callable.
881889
agentToolset.setToolPromoter(
882890
createExecToolPromoter({
883-
activate: (names) =>
884-
activatedToolNames.activate(
885-
names.filter((name) => isExecOverlayToolAllowed(overlay, name)),
886-
),
891+
activate: (names) => activatedToolNames.activate(names),
892+
isAllowed: (name) => isExecOverlayToolAllowed(overlay, name),
887893
currentDefinitions: () =>
888894
agentToolset.dynamicRunner.currentDefinitions(),
889895
computeAdvertised,

‎tests/unit/exec/runner.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -784,6 +784,7 @@ describe("exec tool call gate and promoter", () => {
784784
const directorNames: string[][] = [];
785785
const promote = createExecToolPromoter({
786786
activate: (names) => activated.activate(names),
787+
isAllowed: () => true,
787788
currentDefinitions: () => runner.currentDefinitions(),
788789
computeAdvertised,
789790
updateDirectorTools: (defs) => {

0 commit comments

Comments
 (0)