Skip to content

Commit 193fdd6

Browse files
eleqtrizitdavid-luz-silvadrobison00
authored
fix(policy): preserve restrictive tool allowlists (#58476)
* fix(policy): preserve restrictive tool allowlists Co-authored-by: David Silva <david.silva@gendigital.com> * fix(policy): address review follow-ups * fix(policy): restore additive alsoAllow semantics * fix(policy): preserve optional tool opt-ins for allow-all configs * fix(policy): narrow plugin-only allowlist warnings * fix(policy): add changelog entry * Revert "fix(policy): add changelog entry" This reverts commit 4a996bf4caedfe8c9ff3a7f190816e657ead5d10. * chore: add changelog for restrictive tool allowlists --------- Co-authored-by: David Silva <david.silva@gendigital.com> Co-authored-by: Devin Robison <drobison@nvidia.com>
1 parent 9f85595 commit 193fdd6

8 files changed

Lines changed: 240 additions & 49 deletions

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ Docs: https://docs.openclaw.ai
2626
- Exec/Windows: prefer strict-inline-eval denial over generic allowlist prompts for interpreter carriers, while keeping persisted Windows allow-always approvals argv-bound. (#59780) Thanks @luoyanglang.
2727
- Gateway/connect: omit admin-scoped config and auth metadata from lower-privilege `hello-ok` snapshots while preserving those fields for admin reconnects. (#58469) Thanks @eleqtrizit.
2828
- iOS/canvas: restrict A2UI bridge trust to the bundled scaffold and exact capability-backed remote canvas URLs, so generic `canvas.navigate` and `canvas.present` loads no longer gain action-dispatch authority. (#58471) Thanks @eleqtrizit.
29+
- Agents/tool policy: preserve restrictive plugin-only allowlists instead of silently widening access to core tools, and keep allowlist warnings aligned with the enforced policy. (#58476) Thanks @eleqtrizit.
2930

3031
## 2026.4.2
3132

‎src/agents/sandbox-tool-policy.test.ts‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
import { describe, expect, it } from "vitest";
2+
import type { OpenClawConfig } from "../config/config.js";
3+
import { resolveEffectiveToolPolicy } from "./pi-tools.policy.js";
4+
import { pickSandboxToolPolicy } from "./sandbox-tool-policy.js";
5+
import { resolveEffectiveToolFsRootExpansionAllowed } from "./tool-fs-policy.js";
6+
7+
describe("pickSandboxToolPolicy", () => {
8+
it("returns undefined when neither allow nor deny is configured", () => {
9+
expect(pickSandboxToolPolicy({})).toBeUndefined();
10+
});
11+
12+
it("keeps alsoAllow without allow additive", () => {
13+
expect(
14+
pickSandboxToolPolicy({
15+
alsoAllow: ["web_search"],
16+
}),
17+
).toEqual({
18+
allow: ["*", "web_search"],
19+
deny: undefined,
20+
});
21+
});
22+
23+
it("merges allow and alsoAllow when both are present", () => {
24+
expect(
25+
pickSandboxToolPolicy({
26+
allow: ["read"],
27+
alsoAllow: ["write"],
28+
}),
29+
).toEqual({
30+
allow: ["read", "write"],
31+
deny: undefined,
32+
});
33+
});
34+
35+
it("preserves allow-all semantics for allow: [] plus alsoAllow", () => {
36+
expect(
37+
pickSandboxToolPolicy({
38+
allow: [],
39+
alsoAllow: ["web_search"],
40+
}),
41+
).toEqual({
42+
allow: ["*", "web_search"],
43+
deny: undefined,
44+
});
45+
});
46+
47+
it("passes deny through unchanged", () => {
48+
expect(
49+
pickSandboxToolPolicy({
50+
deny: ["exec"],
51+
}),
52+
).toEqual({
53+
allow: undefined,
54+
deny: ["exec"],
55+
});
56+
});
57+
58+
it("keeps global alsoAllow additive in effective tool policy resolution", () => {
59+
const cfg: OpenClawConfig = {
60+
tools: {
61+
profile: "coding",
62+
alsoAllow: ["lobster"],
63+
},
64+
};
65+
66+
const resolved = resolveEffectiveToolPolicy({ config: cfg, agentId: "main" });
67+
expect(resolved.globalPolicy).toEqual({ allow: ["*", "lobster"], deny: undefined });
68+
expect(resolved.profileAlsoAllow).toEqual(["lobster"]);
69+
});
70+
71+
it("does not block fs root expansion when only global alsoAllow is configured", () => {
72+
const cfg: OpenClawConfig = {
73+
tools: {
74+
alsoAllow: ["lobster"],
75+
},
76+
};
77+
78+
expect(resolveEffectiveToolFsRootExpansionAllowed({ cfg, agentId: "main" })).toBe(true);
79+
});
80+
});

‎src/agents/sandbox-tool-policy.ts‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,10 @@ function unionAllow(base?: string[], extra?: string[]): string[] | undefined {
1010
if (!Array.isArray(extra) || extra.length === 0) {
1111
return base;
1212
}
13-
// If the user is using alsoAllow without an allowlist, treat it as additive on top of
14-
// an implicit allow-all policy.
15-
if (!Array.isArray(base) || base.length === 0) {
13+
if (!Array.isArray(base)) {
14+
return Array.from(new Set(["*", ...extra]));
15+
}
16+
if (base.length === 0) {
1617
return Array.from(new Set(["*", ...extra]));
1718
}
1819
return Array.from(new Set([...base, ...extra]));

‎src/agents/tool-policy-pipeline.test.ts‎

Lines changed: 74 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ describe("tool-policy-pipeline", () => {
3636
resetToolPolicyWarningCacheForTest();
3737
});
3838

39-
test("strips allowlists that would otherwise disable core tools", () => {
39+
test("preserves plugin-only allowlists instead of silently stripping them", () => {
4040
const tools = [{ name: "exec" }, { name: "plugin_tool" }] as unknown as DummyTool[];
4141
const filtered = applyToolPolicyPipeline({
4242
// oxlint-disable-next-line typescript/no-explicit-any
@@ -53,7 +53,7 @@ describe("tool-policy-pipeline", () => {
5353
],
5454
});
5555
const names = filtered.map((t) => (t as unknown as DummyTool).name).toSorted();
56-
expect(names).toEqual(["exec", "plugin_tool"]);
56+
expect(names).toEqual(["plugin_tool"]);
5757
});
5858

5959
test("warns about unknown allowlist entries", () => {
@@ -109,6 +109,7 @@ describe("tool-policy-pipeline", () => {
109109
expect(warnings[0]).toContain(
110110
"shipped core tools but unavailable in the current runtime/provider/model/config",
111111
);
112+
expect(warnings[0]).not.toContain("Allowlist contains only plugin entries");
112113
expect(warnings[0]).not.toContain("unless the plugin is enabled");
113114
});
114115

@@ -175,6 +176,58 @@ describe("tool-policy-pipeline", () => {
175176
expect(warnings).toHaveLength(258);
176177
});
177178

179+
test("evicts the oldest warning when the dedupe cache is full", () => {
180+
const warnings: string[] = [];
181+
const tools = [{ name: "exec" }] as unknown as DummyTool[];
182+
183+
for (let i = 0; i < 256; i += 1) {
184+
applyToolPolicyPipeline({
185+
// oxlint-disable-next-line typescript/no-explicit-any
186+
tools: tools as any,
187+
// oxlint-disable-next-line typescript/no-explicit-any
188+
toolMeta: () => undefined,
189+
warn: (msg: string) => warnings.push(msg),
190+
steps: [
191+
{
192+
policy: { allow: [`unknown_${i}`] },
193+
label: "tools.allow",
194+
stripPluginOnlyAllowlist: true,
195+
},
196+
],
197+
});
198+
}
199+
200+
warnings.length = 0;
201+
202+
applyToolPolicyPipeline({
203+
// oxlint-disable-next-line typescript/no-explicit-any
204+
tools: tools as any,
205+
// oxlint-disable-next-line typescript/no-explicit-any
206+
toolMeta: () => undefined,
207+
warn: (msg: string) => warnings.push(msg),
208+
steps: [
209+
{
210+
policy: { allow: ["unknown_256"] },
211+
label: "tools.allow",
212+
stripPluginOnlyAllowlist: true,
213+
},
214+
],
215+
});
216+
applyToolPolicyPipeline({
217+
// oxlint-disable-next-line typescript/no-explicit-any
218+
tools: tools as any,
219+
// oxlint-disable-next-line typescript/no-explicit-any
220+
toolMeta: () => undefined,
221+
warn: (msg: string) => warnings.push(msg),
222+
steps: [
223+
{ policy: { allow: ["unknown_0"] }, label: "tools.allow", stripPluginOnlyAllowlist: true },
224+
],
225+
});
226+
227+
expect(warnings).toHaveLength(2);
228+
expect(warnings[1]).toContain("unknown_0");
229+
});
230+
178231
test("applies allowlist filtering when core tools are explicitly listed", () => {
179232
const tools = [{ name: "exec" }, { name: "process" }] as unknown as DummyTool[];
180233
const filtered = applyToolPolicyPipeline({
@@ -193,4 +246,23 @@ describe("tool-policy-pipeline", () => {
193246
});
194247
expect(filtered.map((t) => (t as unknown as DummyTool).name)).toEqual(["exec"]);
195248
});
249+
250+
test("applies deny filtering after allow filtering", () => {
251+
const tools = [{ name: "exec" }, { name: "process" }] as unknown as DummyTool[];
252+
const filtered = applyToolPolicyPipeline({
253+
// oxlint-disable-next-line typescript/no-explicit-any
254+
tools: tools as any,
255+
// oxlint-disable-next-line typescript/no-explicit-any
256+
toolMeta: () => undefined,
257+
warn: () => {},
258+
steps: [
259+
{
260+
policy: { allow: ["exec", "process"], deny: ["process"] },
261+
label: "tools.allow",
262+
stripPluginOnlyAllowlist: true,
263+
},
264+
],
265+
});
266+
expect(filtered.map((t) => (t as unknown as DummyTool).name)).toEqual(["exec"]);
267+
});
196268
});

‎src/agents/tool-policy-pipeline.ts‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,29 @@ import { filterToolsByPolicy } from "./pi-tools.policy.js";
22
import type { AnyAgentTool } from "./pi-tools.types.js";
33
import { isKnownCoreToolId } from "./tool-catalog.js";
44
import {
5+
analyzeAllowlistByToolType,
56
buildPluginToolGroups,
67
expandPolicyWithPluginGroups,
78
normalizeToolName,
8-
stripPluginOnlyAllowlist,
99
type ToolPolicyLike,
1010
} from "./tool-policy.js";
1111

1212
const MAX_TOOL_POLICY_WARNING_CACHE = 256;
1313
const seenToolPolicyWarnings = new Set<string>();
14+
const toolPolicyWarningOrder: string[] = [];
1415

1516
function rememberToolPolicyWarning(warning: string): boolean {
1617
if (seenToolPolicyWarnings.has(warning)) {
1718
return false;
1819
}
1920
if (seenToolPolicyWarnings.size >= MAX_TOOL_POLICY_WARNING_CACHE) {
20-
const oldest = seenToolPolicyWarnings.values().next().value;
21+
const oldest = toolPolicyWarningOrder.shift();
2122
if (oldest) {
2223
seenToolPolicyWarnings.delete(oldest);
2324
}
2425
}
2526
seenToolPolicyWarnings.add(warning);
27+
toolPolicyWarningOrder.push(warning);
2628
return true;
2729
}
2830

@@ -114,22 +116,22 @@ export function applyToolPolicyPipeline(params: {
114116

115117
let policy: ToolPolicyLike | undefined = step.policy;
116118
if (step.stripPluginOnlyAllowlist) {
117-
const resolved = stripPluginOnlyAllowlist(policy, pluginGroups, coreToolNames);
119+
const resolved = analyzeAllowlistByToolType(policy, pluginGroups, coreToolNames);
118120
if (resolved.unknownAllowlist.length > 0) {
119121
const entries = resolved.unknownAllowlist.join(", ");
120122
const gatedCoreEntries = resolved.unknownAllowlist.filter((entry) =>
121123
isKnownCoreToolId(entry),
122124
);
123125
const otherEntries = resolved.unknownAllowlist.filter((entry) => !isKnownCoreToolId(entry));
124126
if (
125-
!shouldSuppressUnavailableCoreToolWarning({
126-
suppressUnavailableCoreToolWarning: step.suppressUnavailableCoreToolWarning === true,
127+
shouldWarnAboutUnknownAllowlist({
128+
suppressUnavailableCoreToolWarning: step.suppressUnavailableCoreToolWarning ?? false,
127129
hasGatedCoreEntries: gatedCoreEntries.length > 0,
128130
hasOtherEntries: otherEntries.length > 0,
129131
})
130132
) {
131133
const suffix = describeUnknownAllowlistSuffix({
132-
strippedAllowlist: resolved.strippedAllowlist,
134+
pluginOnlyAllowlist: resolved.pluginOnlyAllowlist,
133135
hasGatedCoreEntries: gatedCoreEntries.length > 0,
134136
hasOtherEntries: otherEntries.length > 0,
135137
});
@@ -148,7 +150,7 @@ export function applyToolPolicyPipeline(params: {
148150
return filtered;
149151
}
150152

151-
function shouldSuppressUnavailableCoreToolWarning(params: {
153+
function shouldWarnAboutUnknownAllowlist(params: {
152154
suppressUnavailableCoreToolWarning: boolean;
153155
hasGatedCoreEntries: boolean;
154156
hasOtherEntries: boolean;
@@ -158,18 +160,18 @@ function shouldSuppressUnavailableCoreToolWarning(params: {
158160
!params.hasGatedCoreEntries ||
159161
params.hasOtherEntries
160162
) {
161-
return false;
163+
return true;
162164
}
163-
return true;
165+
return false;
164166
}
165167

166168
function describeUnknownAllowlistSuffix(params: {
167-
strippedAllowlist: boolean;
169+
pluginOnlyAllowlist: boolean;
168170
hasGatedCoreEntries: boolean;
169171
hasOtherEntries: boolean;
170172
}): string {
171-
const preface = params.strippedAllowlist
172-
? "Ignoring allowlist so core tools remain available."
173+
const preface = params.pluginOnlyAllowlist
174+
? "Allowlist contains only plugin entries; core tools will not be available."
173175
: "";
174176
const detail =
175177
params.hasGatedCoreEntries && params.hasOtherEntries
@@ -182,4 +184,5 @@ function describeUnknownAllowlistSuffix(params: {
182184

183185
export function resetToolPolicyWarningCacheForTest(): void {
184186
seenToolPolicyWarnings.clear();
187+
toolPolicyWarningOrder.length = 0;
185188
}

0 commit comments

Comments
 (0)