Skip to content

Commit e964987

Browse files
jesse-merhiclawsweeper[bot]
jesse-merhi
and
clawsweeper[bot]
authored
Remove skill prelude exec allowlist (#84570)
Summary: - The PR removes the legacy `cat SKILL.md && printf ... && <skill-wrapper>` exec-approval allowlist path, updates focused exec-approval tests, and adds a changelog entry. - Reproducibility: yes. Current-main source and tests show the old `cat SKILL.md && printf ... && <wrapper>` c ... ed this by source and test inspection rather than executing tests because the checkout review is read-only. Automerge notes: - PR branch already contained follow-up commit before automerge: Remove skill prelude exec allowlist Validation: - ClawSweeper review passed for head 0ca7f3e. - Required merge gates passed before the squash merge. Prepared head SHA: 0ca7f3e Review: #84570 (comment) Co-authored-by: jesse-merhi <79823012+jesse-merhi@users.noreply.github.com> Co-authored-by: clawsweeper <274271284+clawsweeper[bot]@users.noreply.github.com> Co-authored-by: clawsweeper[bot] <274271284+clawsweeper[bot]@users.noreply.github.com> Approved-by: jesse-merhi
1 parent b79effe commit e964987

5 files changed

Lines changed: 88 additions & 288 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai
66

77
### Changes
88

9+
- Exec approvals: remove the old `cat SKILL.md && printf ... && <skill-wrapper>` allowlist compatibility path so skill files must be loaded with the read tool and only the real skill executable is auto-allowed.
910
- Discord: let voice sessions follow configured Discord users into voice channels, with allowed-channel checks, multi-user handoff, bounded reconciliation, and DAVE recovery preservation. (#84264) Thanks @fuller-stack-dev.
1011
- Discord/voice: include bounded `IDENTITY.md`, `USER.md`, and `SOUL.md` profile context in realtime voice session instructions by default, with `voice.realtime.bootstrapContextFiles: []` available to disable it. (#84499) Thanks @fuller-stack-dev.
1112
- Dependencies: bump the bundled Codex harness to `@openai/codex` `0.132.0` and refresh the app-server model-list docs for the new catalog.

‎src/agents/bash-tools.exec.approval-id.test.ts‎

Lines changed: 63 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1228,28 +1228,25 @@ describe("exec approvals", () => {
12281228
expect(calls).toContain("exec.approval.request");
12291229
});
12301230

1231-
it("runs a skill wrapper chain without prompting when the wrapper is allowlisted", async () => {
1231+
it("runs a direct skill wrapper command without prompting when the wrapper is allowlisted", async () => {
12321232
if (process.platform === "win32") {
12331233
return;
12341234
}
12351235
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-wrapper-"));
12361236
try {
1237-
const skillDir = path.join(tempDir, ".openclaw", "skills", "gog");
1238-
const skillPath = path.join(skillDir, "SKILL.md");
12391237
const binDir = path.join(tempDir, "bin");
12401238
const wrapperPath = path.join(binDir, "gog-wrapper");
1241-
await fs.mkdir(skillDir, { recursive: true });
12421239
await fs.mkdir(binDir, { recursive: true });
1243-
await fs.writeFile(skillPath, "# gog skill\n");
12441240
await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n");
12451241
await fs.chmod(wrapperPath, 0o755);
1242+
const trustedWrapperPath = await fs.realpath(wrapperPath);
12461243

12471244
await writeExecApprovalsConfig({
12481245
version: 1,
12491246
defaults: { security: "allowlist", ask: "off", askFallback: "deny" },
12501247
agents: {
12511248
main: {
1252-
allowlist: [{ pattern: wrapperPath }],
1249+
allowlist: [{ pattern: trustedWrapperPath }],
12531250
},
12541251
},
12551252
});
@@ -1265,7 +1262,7 @@ describe("exec approvals", () => {
12651262
});
12661263

12671264
const result = await tool.execute("call-skill-wrapper", {
1268-
command: `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`,
1265+
command: `${JSON.stringify(wrapperPath)} calendar events primary --today --json`,
12691266
workdir: tempDir,
12701267
});
12711268

@@ -1277,6 +1274,65 @@ describe("exec approvals", () => {
12771274
}
12781275
});
12791276

1277+
it("requires approval for the legacy skill display prelude even when the wrapper is allowlisted", async () => {
1278+
if (process.platform === "win32") {
1279+
return;
1280+
}
1281+
const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-prelude-"));
1282+
try {
1283+
const skillDir = path.join(tempDir, ".openclaw", "skills", "gog");
1284+
const skillPath = path.join(skillDir, "SKILL.md");
1285+
const binDir = path.join(tempDir, "bin");
1286+
const wrapperPath = path.join(binDir, "gog-wrapper");
1287+
await fs.mkdir(skillDir, { recursive: true });
1288+
await fs.mkdir(binDir, { recursive: true });
1289+
await fs.writeFile(skillPath, "# gog skill\n");
1290+
await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n");
1291+
await fs.chmod(wrapperPath, 0o755);
1292+
const trustedWrapperPath = await fs.realpath(wrapperPath);
1293+
1294+
await writeExecApprovalsConfig({
1295+
version: 1,
1296+
defaults: { security: "allowlist", ask: "on-miss", askFallback: "deny" },
1297+
agents: {
1298+
main: {
1299+
allowlist: [{ pattern: trustedWrapperPath }],
1300+
},
1301+
},
1302+
});
1303+
1304+
const calls: string[] = [];
1305+
vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => {
1306+
calls.push(method);
1307+
if (method === "exec.approval.request") {
1308+
return acceptedApprovalResponse(params);
1309+
}
1310+
if (method === "exec.approval.waitDecision") {
1311+
return { decision: "deny" };
1312+
}
1313+
return { ok: true };
1314+
});
1315+
1316+
const tool = createExecTool({
1317+
host: "gateway",
1318+
ask: "on-miss",
1319+
security: "allowlist",
1320+
approvalRunningNoticeMs: 0,
1321+
});
1322+
1323+
const command = `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`;
1324+
const result = await tool.execute("call-skill-prelude", {
1325+
command,
1326+
workdir: tempDir,
1327+
});
1328+
1329+
expectPendingCommandText(result, command);
1330+
expect(calls).toContain("exec.approval.request");
1331+
} finally {
1332+
await fs.rm(tempDir, { recursive: true, force: true });
1333+
}
1334+
});
1335+
12801336
it("shows full chained node commands in approval-pending message", async () => {
12811337
const calls: string[] = [];
12821338
vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => {

‎src/infra/exec-approvals-allowlist.ts‎

Lines changed: 6 additions & 213 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import {
2727
type ExecCommandAnalysis,
2828
type ExecCommandSegment,
2929
type ExecutableResolution,
30-
type ShellChainOperator,
3130
} from "./exec-approvals-analysis.js";
3231
import type { ExecAllowlistEntry } from "./exec-approvals.types.js";
3332
import {
@@ -132,13 +131,7 @@ export type ExecAllowlistEvaluation = {
132131
segmentSatisfiedBy: ExecSegmentSatisfiedBy[];
133132
};
134133

135-
export type ExecSegmentSatisfiedBy =
136-
| "allowlist"
137-
| "safeBins"
138-
| "inlineChain"
139-
| "skills"
140-
| "skillPrelude"
141-
| null;
134+
export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "inlineChain" | "skills" | null;
142135
export type SkillBinTrustEntry = {
143136
name: string;
144137
resolvedPath: string;
@@ -232,163 +225,6 @@ function isSkillAutoAllowedSegment(params: {
232225
return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath));
233226
}
234227

235-
function resolveSkillPreludePath(rawPath: string, cwd?: string): string {
236-
const expanded = rawPath.startsWith("~") ? expandHomePrefix(rawPath) : rawPath;
237-
if (path.isAbsolute(expanded)) {
238-
return path.resolve(expanded);
239-
}
240-
return path.resolve(cwd?.trim() || process.cwd(), expanded);
241-
}
242-
243-
function isSkillMarkdownPreludePath(filePath: string): boolean {
244-
const normalized = filePath.replace(/\\/g, "/");
245-
const lowerNormalized = normalizeLowercaseStringOrEmpty(normalized);
246-
if (!lowerNormalized.endsWith("/skill.md")) {
247-
return false;
248-
}
249-
const parts = lowerNormalized.split("/").filter(Boolean);
250-
if (parts.length < 2) {
251-
return false;
252-
}
253-
for (let index = parts.length - 2; index >= 0; index -= 1) {
254-
if (parts[index] !== "skills") {
255-
continue;
256-
}
257-
const segmentsAfterSkills = parts.length - index - 1;
258-
if (segmentsAfterSkills === 1 || segmentsAfterSkills === 2) {
259-
return true;
260-
}
261-
}
262-
return false;
263-
}
264-
265-
function resolveSkillMarkdownPreludeId(filePath: string): string | null {
266-
const normalized = filePath.replace(/\\/g, "/");
267-
const lowerNormalized = normalizeLowercaseStringOrEmpty(normalized);
268-
if (!lowerNormalized.endsWith("/skill.md")) {
269-
return null;
270-
}
271-
const parts = lowerNormalized.split("/").filter(Boolean);
272-
if (parts.length < 3) {
273-
return null;
274-
}
275-
for (let index = parts.length - 2; index >= 0; index -= 1) {
276-
if (parts[index] !== "skills") {
277-
continue;
278-
}
279-
if (parts.length - index - 1 !== 2) {
280-
continue;
281-
}
282-
const skillId = parts[index + 1]?.trim();
283-
return skillId || null;
284-
}
285-
return null;
286-
}
287-
288-
function isSkillPreludeReadSegment(segment: ExecCommandSegment, cwd?: string): boolean {
289-
const execution = resolveExecutionTargetResolution(segment.resolution);
290-
if (normalizeLowercaseStringOrEmpty(execution?.executableName) !== "cat") {
291-
return false;
292-
}
293-
// Keep the display-prelude exception narrow: only a plain `cat <...>/SKILL.md`
294-
// qualifies, not extra argv forms or arbitrary file reads.
295-
if (segment.argv.length !== 2) {
296-
return false;
297-
}
298-
const rawPath = segment.argv[1]?.trim();
299-
if (!rawPath) {
300-
return false;
301-
}
302-
return isSkillMarkdownPreludePath(resolveSkillPreludePath(rawPath, cwd));
303-
}
304-
305-
function isSkillPreludeMarkerSegment(segment: ExecCommandSegment): boolean {
306-
const execution = resolveExecutionTargetResolution(segment.resolution);
307-
if (normalizeLowercaseStringOrEmpty(execution?.executableName) !== "printf") {
308-
return false;
309-
}
310-
if (segment.argv.length !== 2) {
311-
return false;
312-
}
313-
const marker = segment.argv[1];
314-
return marker === "\\n---CMD---\\n" || marker === "\n---CMD---\n";
315-
}
316-
317-
function isSkillPreludeSegment(segment: ExecCommandSegment, cwd?: string): boolean {
318-
return isSkillPreludeReadSegment(segment, cwd) || isSkillPreludeMarkerSegment(segment);
319-
}
320-
321-
function isSkillPreludeOnlyEvaluation(
322-
segments: ExecCommandSegment[],
323-
cwd: string | undefined,
324-
): boolean {
325-
return segments.length > 0 && segments.every((segment) => isSkillPreludeSegment(segment, cwd));
326-
}
327-
328-
function resolveSkillPreludeIds(
329-
segments: ExecCommandSegment[],
330-
cwd: string | undefined,
331-
): ReadonlySet<string> {
332-
const skillIds = new Set<string>();
333-
for (const segment of segments) {
334-
if (!isSkillPreludeReadSegment(segment, cwd)) {
335-
continue;
336-
}
337-
const rawPath = segment.argv[1]?.trim();
338-
if (!rawPath) {
339-
continue;
340-
}
341-
const skillId = resolveSkillMarkdownPreludeId(resolveSkillPreludePath(rawPath, cwd));
342-
if (skillId) {
343-
skillIds.add(skillId);
344-
}
345-
}
346-
return skillIds;
347-
}
348-
349-
function resolveAllowlistedSkillWrapperId(segment: ExecCommandSegment): string | null {
350-
const execution = resolveExecutionTargetResolution(segment.resolution);
351-
const executableName = normalizeExecutableToken(
352-
execution?.executableName ?? segment.argv[0] ?? "",
353-
);
354-
if (!executableName.endsWith("-wrapper")) {
355-
return null;
356-
}
357-
const skillId = executableName.slice(0, -"-wrapper".length).trim();
358-
return skillId || null;
359-
}
360-
361-
function resolveTrustedSkillExecutionIds(params: {
362-
analysis: ExecCommandAnalysis;
363-
evaluation: ExecAllowlistEvaluation;
364-
}): ReadonlySet<string> {
365-
const skillIds = new Set<string>();
366-
if (!params.evaluation.allowlistSatisfied) {
367-
return skillIds;
368-
}
369-
for (const [index, segment] of params.analysis.segments.entries()) {
370-
const satisfiedBy = params.evaluation.segmentSatisfiedBy[index];
371-
if (satisfiedBy === "skills") {
372-
const execution = resolveExecutionTargetResolution(segment.resolution);
373-
const executableName = normalizeExecutableToken(
374-
execution?.executableName ?? execution?.rawExecutable ?? segment.argv[0] ?? "",
375-
);
376-
if (executableName) {
377-
skillIds.add(executableName);
378-
}
379-
continue;
380-
}
381-
if (satisfiedBy !== "allowlist") {
382-
continue;
383-
}
384-
const wrapperSkillId = resolveAllowlistedSkillWrapperId(segment);
385-
if (wrapperSkillId) {
386-
skillIds.add(wrapperSkillId);
387-
}
388-
}
389-
return skillIds;
390-
}
391-
392228
const MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH = 3;
393229

394230
type InlineChainAllowlistEvaluation = {
@@ -1305,7 +1141,7 @@ export function evaluateShellAllowlist(
13051141
};
13061142
}
13071143

1308-
const chainEvaluations = chainParts.map(({ part, opToNext }) => {
1144+
const chainEvaluations = chainParts.map(({ part }) => {
13091145
const analysis = analyzeShellCommand({
13101146
command: part,
13111147
cwd: params.cwd,
@@ -1318,7 +1154,6 @@ export function evaluateShellAllowlist(
13181154
return {
13191155
analysis,
13201156
evaluation: evaluateExecAllowlist({ analysis, ...allowlistContext }),
1321-
opToNext,
13221157
};
13231158
});
13241159
if (chainEvaluations.some((entry) => entry === null)) {
@@ -1328,60 +1163,18 @@ export function evaluateShellAllowlist(
13281163
const finalizedEvaluations = chainEvaluations as Array<{
13291164
analysis: ExecCommandAnalysis;
13301165
evaluation: ExecAllowlistEvaluation;
1331-
opToNext: ShellChainOperator | null;
13321166
}>;
1333-
const allowSkillPreludeAtIndex = new Set<number>();
1334-
const reachableSkillIds = new Set<string>();
1335-
// Only allow the `cat SKILL.md && printf ...` display prelude when it sits on a
1336-
// contiguous `&&` chain that actually reaches a later trusted skill-wrapper execution.
1337-
for (let index = finalizedEvaluations.length - 1; index >= 0; index -= 1) {
1338-
const { analysis, evaluation, opToNext } = finalizedEvaluations[index];
1339-
const trustedSkillIds = resolveTrustedSkillExecutionIds({
1340-
analysis,
1341-
evaluation,
1342-
});
1343-
if (trustedSkillIds.size > 0) {
1344-
for (const skillId of trustedSkillIds) {
1345-
reachableSkillIds.add(skillId);
1346-
}
1347-
continue;
1348-
}
1349-
1350-
const isPreludeOnly =
1351-
!evaluation.allowlistSatisfied && isSkillPreludeOnlyEvaluation(analysis.segments, params.cwd);
1352-
const preludeSkillIds = isPreludeOnly
1353-
? resolveSkillPreludeIds(analysis.segments, params.cwd)
1354-
: new Set<string>();
1355-
const reachesTrustedSkillExecution =
1356-
opToNext === "&&" &&
1357-
(preludeSkillIds.size === 0
1358-
? reachableSkillIds.size > 0
1359-
: [...preludeSkillIds].some((skillId) => reachableSkillIds.has(skillId)));
1360-
if (isPreludeOnly && reachesTrustedSkillExecution) {
1361-
allowSkillPreludeAtIndex.add(index);
1362-
continue;
1363-
}
1364-
1365-
reachableSkillIds.clear();
1366-
}
13671167
const allowlistMatches: ExecAllowlistEntry[] = [];
13681168
const segments: ExecCommandSegment[] = [];
13691169
const segmentAllowlistEntries: Array<ExecAllowlistEntry | null> = [];
13701170
const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = [];
13711171

1372-
for (const [index, { analysis, evaluation }] of finalizedEvaluations.entries()) {
1373-
const effectiveSegmentSatisfiedBy = allowSkillPreludeAtIndex.has(index)
1374-
? analysis.segments.map(() => "skillPrelude" as const)
1375-
: evaluation.segmentSatisfiedBy;
1376-
const effectiveSegmentAllowlistEntries = allowSkillPreludeAtIndex.has(index)
1377-
? analysis.segments.map(() => null)
1378-
: evaluation.segmentAllowlistEntries;
1379-
1172+
for (const { analysis, evaluation } of finalizedEvaluations) {
13801173
segments.push(...analysis.segments);
13811174
allowlistMatches.push(...evaluation.allowlistMatches);
1382-
segmentAllowlistEntries.push(...effectiveSegmentAllowlistEntries);
1383-
segmentSatisfiedBy.push(...effectiveSegmentSatisfiedBy);
1384-
if (!evaluation.allowlistSatisfied && !allowSkillPreludeAtIndex.has(index)) {
1175+
segmentAllowlistEntries.push(...evaluation.segmentAllowlistEntries);
1176+
segmentSatisfiedBy.push(...evaluation.segmentSatisfiedBy);
1177+
if (!evaluation.allowlistSatisfied) {
13851178
return {
13861179
analysisOk: true,
13871180
allowlistSatisfied: false,

0 commit comments

Comments
 (0)