diff --git a/CHANGELOG.md b/CHANGELOG.md index 2174973744cd..f77e90227b9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ Docs: https://docs.openclaw.ai ### Changes +- Exec approvals: remove the old `cat SKILL.md && printf ... && ` allowlist compatibility path so skill files must be loaded with the read tool and only the real skill executable is auto-allowed. - 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. - 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. - Dependencies: bump the bundled Codex harness to `@openai/codex` `0.132.0` and refresh the app-server model-list docs for the new catalog. diff --git a/src/agents/bash-tools.exec.approval-id.test.ts b/src/agents/bash-tools.exec.approval-id.test.ts index c89394dbc5b1..6e4f1aefb4b7 100644 --- a/src/agents/bash-tools.exec.approval-id.test.ts +++ b/src/agents/bash-tools.exec.approval-id.test.ts @@ -1228,28 +1228,25 @@ describe("exec approvals", () => { expect(calls).toContain("exec.approval.request"); }); - it("runs a skill wrapper chain without prompting when the wrapper is allowlisted", async () => { + it("runs a direct skill wrapper command without prompting when the wrapper is allowlisted", async () => { if (process.platform === "win32") { return; } const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-wrapper-")); try { - const skillDir = path.join(tempDir, ".openclaw", "skills", "gog"); - const skillPath = path.join(skillDir, "SKILL.md"); const binDir = path.join(tempDir, "bin"); const wrapperPath = path.join(binDir, "gog-wrapper"); - await fs.mkdir(skillDir, { recursive: true }); await fs.mkdir(binDir, { recursive: true }); - await fs.writeFile(skillPath, "# gog skill\n"); await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n"); await fs.chmod(wrapperPath, 0o755); + const trustedWrapperPath = await fs.realpath(wrapperPath); await writeExecApprovalsConfig({ version: 1, defaults: { security: "allowlist", ask: "off", askFallback: "deny" }, agents: { main: { - allowlist: [{ pattern: wrapperPath }], + allowlist: [{ pattern: trustedWrapperPath }], }, }, }); @@ -1265,7 +1262,7 @@ describe("exec approvals", () => { }); const result = await tool.execute("call-skill-wrapper", { - command: `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`, + command: `${JSON.stringify(wrapperPath)} calendar events primary --today --json`, workdir: tempDir, }); @@ -1277,6 +1274,65 @@ describe("exec approvals", () => { } }); + it("requires approval for the legacy skill display prelude even when the wrapper is allowlisted", async () => { + if (process.platform === "win32") { + return; + } + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-skill-prelude-")); + try { + const skillDir = path.join(tempDir, ".openclaw", "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + const binDir = path.join(tempDir, "bin"); + const wrapperPath = path.join(binDir, "gog-wrapper"); + await fs.mkdir(skillDir, { recursive: true }); + await fs.mkdir(binDir, { recursive: true }); + await fs.writeFile(skillPath, "# gog skill\n"); + await fs.writeFile(wrapperPath, "#!/bin/sh\necho '{\"events\":[]}'\n"); + await fs.chmod(wrapperPath, 0o755); + const trustedWrapperPath = await fs.realpath(wrapperPath); + + await writeExecApprovalsConfig({ + version: 1, + defaults: { security: "allowlist", ask: "on-miss", askFallback: "deny" }, + agents: { + main: { + allowlist: [{ pattern: trustedWrapperPath }], + }, + }, + }); + + const calls: string[] = []; + vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { + calls.push(method); + if (method === "exec.approval.request") { + return acceptedApprovalResponse(params); + } + if (method === "exec.approval.waitDecision") { + return { decision: "deny" }; + } + return { ok: true }; + }); + + const tool = createExecTool({ + host: "gateway", + ask: "on-miss", + security: "allowlist", + approvalRunningNoticeMs: 0, + }); + + const command = `cat ${JSON.stringify(skillPath)} && printf '\\n---CMD---\\n' && ${JSON.stringify(wrapperPath)} calendar events primary --today --json`; + const result = await tool.execute("call-skill-prelude", { + command, + workdir: tempDir, + }); + + expectPendingCommandText(result, command); + expect(calls).toContain("exec.approval.request"); + } finally { + await fs.rm(tempDir, { recursive: true, force: true }); + } + }); + it("shows full chained node commands in approval-pending message", async () => { const calls: string[] = []; vi.mocked(callGatewayTool).mockImplementation(async (method, _opts, params) => { diff --git a/src/infra/exec-approvals-allowlist.ts b/src/infra/exec-approvals-allowlist.ts index cdbde02df2e6..95d6c48ecbd0 100644 --- a/src/infra/exec-approvals-allowlist.ts +++ b/src/infra/exec-approvals-allowlist.ts @@ -27,7 +27,6 @@ import { type ExecCommandAnalysis, type ExecCommandSegment, type ExecutableResolution, - type ShellChainOperator, } from "./exec-approvals-analysis.js"; import type { ExecAllowlistEntry } from "./exec-approvals.types.js"; import { @@ -132,13 +131,7 @@ export type ExecAllowlistEvaluation = { segmentSatisfiedBy: ExecSegmentSatisfiedBy[]; }; -export type ExecSegmentSatisfiedBy = - | "allowlist" - | "safeBins" - | "inlineChain" - | "skills" - | "skillPrelude" - | null; +export type ExecSegmentSatisfiedBy = "allowlist" | "safeBins" | "inlineChain" | "skills" | null; export type SkillBinTrustEntry = { name: string; resolvedPath: string; @@ -232,163 +225,6 @@ function isSkillAutoAllowedSegment(params: { return Boolean(params.skillBinTrust.get(executableName)?.has(resolvedPath)); } -function resolveSkillPreludePath(rawPath: string, cwd?: string): string { - const expanded = rawPath.startsWith("~") ? expandHomePrefix(rawPath) : rawPath; - if (path.isAbsolute(expanded)) { - return path.resolve(expanded); - } - return path.resolve(cwd?.trim() || process.cwd(), expanded); -} - -function isSkillMarkdownPreludePath(filePath: string): boolean { - const normalized = filePath.replace(/\\/g, "/"); - const lowerNormalized = normalizeLowercaseStringOrEmpty(normalized); - if (!lowerNormalized.endsWith("/skill.md")) { - return false; - } - const parts = lowerNormalized.split("/").filter(Boolean); - if (parts.length < 2) { - return false; - } - for (let index = parts.length - 2; index >= 0; index -= 1) { - if (parts[index] !== "skills") { - continue; - } - const segmentsAfterSkills = parts.length - index - 1; - if (segmentsAfterSkills === 1 || segmentsAfterSkills === 2) { - return true; - } - } - return false; -} - -function resolveSkillMarkdownPreludeId(filePath: string): string | null { - const normalized = filePath.replace(/\\/g, "/"); - const lowerNormalized = normalizeLowercaseStringOrEmpty(normalized); - if (!lowerNormalized.endsWith("/skill.md")) { - return null; - } - const parts = lowerNormalized.split("/").filter(Boolean); - if (parts.length < 3) { - return null; - } - for (let index = parts.length - 2; index >= 0; index -= 1) { - if (parts[index] !== "skills") { - continue; - } - if (parts.length - index - 1 !== 2) { - continue; - } - const skillId = parts[index + 1]?.trim(); - return skillId || null; - } - return null; -} - -function isSkillPreludeReadSegment(segment: ExecCommandSegment, cwd?: string): boolean { - const execution = resolveExecutionTargetResolution(segment.resolution); - if (normalizeLowercaseStringOrEmpty(execution?.executableName) !== "cat") { - return false; - } - // Keep the display-prelude exception narrow: only a plain `cat <...>/SKILL.md` - // qualifies, not extra argv forms or arbitrary file reads. - if (segment.argv.length !== 2) { - return false; - } - const rawPath = segment.argv[1]?.trim(); - if (!rawPath) { - return false; - } - return isSkillMarkdownPreludePath(resolveSkillPreludePath(rawPath, cwd)); -} - -function isSkillPreludeMarkerSegment(segment: ExecCommandSegment): boolean { - const execution = resolveExecutionTargetResolution(segment.resolution); - if (normalizeLowercaseStringOrEmpty(execution?.executableName) !== "printf") { - return false; - } - if (segment.argv.length !== 2) { - return false; - } - const marker = segment.argv[1]; - return marker === "\\n---CMD---\\n" || marker === "\n---CMD---\n"; -} - -function isSkillPreludeSegment(segment: ExecCommandSegment, cwd?: string): boolean { - return isSkillPreludeReadSegment(segment, cwd) || isSkillPreludeMarkerSegment(segment); -} - -function isSkillPreludeOnlyEvaluation( - segments: ExecCommandSegment[], - cwd: string | undefined, -): boolean { - return segments.length > 0 && segments.every((segment) => isSkillPreludeSegment(segment, cwd)); -} - -function resolveSkillPreludeIds( - segments: ExecCommandSegment[], - cwd: string | undefined, -): ReadonlySet { - const skillIds = new Set(); - for (const segment of segments) { - if (!isSkillPreludeReadSegment(segment, cwd)) { - continue; - } - const rawPath = segment.argv[1]?.trim(); - if (!rawPath) { - continue; - } - const skillId = resolveSkillMarkdownPreludeId(resolveSkillPreludePath(rawPath, cwd)); - if (skillId) { - skillIds.add(skillId); - } - } - return skillIds; -} - -function resolveAllowlistedSkillWrapperId(segment: ExecCommandSegment): string | null { - const execution = resolveExecutionTargetResolution(segment.resolution); - const executableName = normalizeExecutableToken( - execution?.executableName ?? segment.argv[0] ?? "", - ); - if (!executableName.endsWith("-wrapper")) { - return null; - } - const skillId = executableName.slice(0, -"-wrapper".length).trim(); - return skillId || null; -} - -function resolveTrustedSkillExecutionIds(params: { - analysis: ExecCommandAnalysis; - evaluation: ExecAllowlistEvaluation; -}): ReadonlySet { - const skillIds = new Set(); - if (!params.evaluation.allowlistSatisfied) { - return skillIds; - } - for (const [index, segment] of params.analysis.segments.entries()) { - const satisfiedBy = params.evaluation.segmentSatisfiedBy[index]; - if (satisfiedBy === "skills") { - const execution = resolveExecutionTargetResolution(segment.resolution); - const executableName = normalizeExecutableToken( - execution?.executableName ?? execution?.rawExecutable ?? segment.argv[0] ?? "", - ); - if (executableName) { - skillIds.add(executableName); - } - continue; - } - if (satisfiedBy !== "allowlist") { - continue; - } - const wrapperSkillId = resolveAllowlistedSkillWrapperId(segment); - if (wrapperSkillId) { - skillIds.add(wrapperSkillId); - } - } - return skillIds; -} - const MAX_SHELL_WRAPPER_INLINE_EVAL_DEPTH = 3; type InlineChainAllowlistEvaluation = { @@ -1305,7 +1141,7 @@ export function evaluateShellAllowlist( }; } - const chainEvaluations = chainParts.map(({ part, opToNext }) => { + const chainEvaluations = chainParts.map(({ part }) => { const analysis = analyzeShellCommand({ command: part, cwd: params.cwd, @@ -1318,7 +1154,6 @@ export function evaluateShellAllowlist( return { analysis, evaluation: evaluateExecAllowlist({ analysis, ...allowlistContext }), - opToNext, }; }); if (chainEvaluations.some((entry) => entry === null)) { @@ -1328,60 +1163,18 @@ export function evaluateShellAllowlist( const finalizedEvaluations = chainEvaluations as Array<{ analysis: ExecCommandAnalysis; evaluation: ExecAllowlistEvaluation; - opToNext: ShellChainOperator | null; }>; - const allowSkillPreludeAtIndex = new Set(); - const reachableSkillIds = new Set(); - // Only allow the `cat SKILL.md && printf ...` display prelude when it sits on a - // contiguous `&&` chain that actually reaches a later trusted skill-wrapper execution. - for (let index = finalizedEvaluations.length - 1; index >= 0; index -= 1) { - const { analysis, evaluation, opToNext } = finalizedEvaluations[index]; - const trustedSkillIds = resolveTrustedSkillExecutionIds({ - analysis, - evaluation, - }); - if (trustedSkillIds.size > 0) { - for (const skillId of trustedSkillIds) { - reachableSkillIds.add(skillId); - } - continue; - } - - const isPreludeOnly = - !evaluation.allowlistSatisfied && isSkillPreludeOnlyEvaluation(analysis.segments, params.cwd); - const preludeSkillIds = isPreludeOnly - ? resolveSkillPreludeIds(analysis.segments, params.cwd) - : new Set(); - const reachesTrustedSkillExecution = - opToNext === "&&" && - (preludeSkillIds.size === 0 - ? reachableSkillIds.size > 0 - : [...preludeSkillIds].some((skillId) => reachableSkillIds.has(skillId))); - if (isPreludeOnly && reachesTrustedSkillExecution) { - allowSkillPreludeAtIndex.add(index); - continue; - } - - reachableSkillIds.clear(); - } const allowlistMatches: ExecAllowlistEntry[] = []; const segments: ExecCommandSegment[] = []; const segmentAllowlistEntries: Array = []; const segmentSatisfiedBy: ExecSegmentSatisfiedBy[] = []; - for (const [index, { analysis, evaluation }] of finalizedEvaluations.entries()) { - const effectiveSegmentSatisfiedBy = allowSkillPreludeAtIndex.has(index) - ? analysis.segments.map(() => "skillPrelude" as const) - : evaluation.segmentSatisfiedBy; - const effectiveSegmentAllowlistEntries = allowSkillPreludeAtIndex.has(index) - ? analysis.segments.map(() => null) - : evaluation.segmentAllowlistEntries; - + for (const { analysis, evaluation } of finalizedEvaluations) { segments.push(...analysis.segments); allowlistMatches.push(...evaluation.allowlistMatches); - segmentAllowlistEntries.push(...effectiveSegmentAllowlistEntries); - segmentSatisfiedBy.push(...effectiveSegmentSatisfiedBy); - if (!evaluation.allowlistSatisfied && !allowSkillPreludeAtIndex.has(index)) { + segmentAllowlistEntries.push(...evaluation.segmentAllowlistEntries); + segmentSatisfiedBy.push(...evaluation.segmentSatisfiedBy); + if (!evaluation.allowlistSatisfied) { return { analysisOk: true, allowlistSatisfied: false, diff --git a/src/infra/exec-approvals-analysis.test.ts b/src/infra/exec-approvals-analysis.test.ts index 35bd88924d29..b7f5362cedca 100644 --- a/src/infra/exec-approvals-analysis.test.ts +++ b/src/infra/exec-approvals-analysis.test.ts @@ -27,21 +27,14 @@ function expectAnalyzedShellCommand( return res; } -function createSkillPreludeFixture(options: { withWrapper?: boolean } = {}) { +function createSkillWrapperFixture() { const skillRoot = makeTempDir(); - const skillDir = path.join(skillRoot, "skills", "gog"); - const skillPath = path.join(skillDir, "SKILL.md"); const wrapperPath = path.join(skillRoot, "bin", "gog-wrapper"); - fs.mkdirSync(skillDir, { recursive: true }); - fs.writeFileSync(skillPath, "# gog\n"); + fs.mkdirSync(path.dirname(wrapperPath), { recursive: true }); + fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 }); - if (options.withWrapper) { - fs.mkdirSync(path.dirname(wrapperPath), { recursive: true }); - fs.writeFileSync(wrapperPath, "#!/bin/sh\n", { mode: 0o755 }); - } - - return { skillRoot, skillPath, wrapperPath }; + return { skillRoot, wrapperPath }; } describe("exec approvals shell analysis", () => { @@ -613,16 +606,14 @@ describe("exec approvals shell analysis", () => { expect(result.allowlistSatisfied).toBe(testCase.expectedAllowlistSatisfied); }); - it("allows the skill display prelude when a later skill wrapper is allowlisted", () => { + it("allows a direct skill wrapper command when the wrapper is allowlisted", () => { if (process.platform === "win32") { return; } - const { skillRoot, skillPath, wrapperPath } = createSkillPreludeFixture({ - withWrapper: true, - }); + const { skillRoot, wrapperPath } = createSkillWrapperFixture(); const result = evaluateShellAllowlist({ - command: `cat ${skillPath} && printf '\\n---CMD---\\n' && ${wrapperPath} calendar events primary --today --json`, + command: `${wrapperPath} calendar events primary --today --json`, allowlist: [{ pattern: wrapperPath }], safeBins: new Set(), cwd: skillRoot, @@ -630,55 +621,21 @@ describe("exec approvals shell analysis", () => { expect(result.analysisOk).toBe(true); expect(result.allowlistSatisfied).toBe(true); - expect(result.segmentSatisfiedBy).toEqual(["skillPrelude", "skillPrelude", "allowlist"]); + expect(result.segmentSatisfiedBy).toEqual(["allowlist"]); }); - it("does not treat arbitrary allowlisted binaries as trusted skill wrappers", () => { + it("rejects the legacy skill display prelude when only the wrapper is allowlisted", () => { if (process.platform === "win32") { return; } - const { skillRoot, skillPath } = createSkillPreludeFixture(); + const { skillRoot, wrapperPath } = createSkillWrapperFixture(); + const skillDir = path.join(skillRoot, "skills", "gog"); + const skillPath = path.join(skillDir, "SKILL.md"); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(skillPath, "# gog\n"); const result = evaluateShellAllowlist({ - command: `cat ${skillPath} && printf '\\n---CMD---\\n' && /bin/echo calendar events primary --today --json`, - allowlist: [{ pattern: "/bin/echo" }], - safeBins: new Set(), - cwd: skillRoot, - }); - - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(false); - expect(result.segmentSatisfiedBy).toEqual([null]); - }); - - it("still rejects the skill display prelude when no trusted skill command follows", () => { - if (process.platform === "win32") { - return; - } - const { skillRoot, skillPath } = createSkillPreludeFixture(); - - const result = evaluateShellAllowlist({ - command: `cat ${skillPath} && printf '\\n---CMD---\\n'`, - allowlist: [], - safeBins: new Set(), - cwd: skillRoot, - }); - - expect(result.analysisOk).toBe(true); - expect(result.allowlistSatisfied).toBe(false); - expect(result.segmentSatisfiedBy).toEqual([null]); - }); - - it("rejects the skill display prelude when a trusted wrapper is not reachable", () => { - if (process.platform === "win32") { - return; - } - const { skillRoot, skillPath, wrapperPath } = createSkillPreludeFixture({ - withWrapper: true, - }); - - const result = evaluateShellAllowlist({ - command: `cat ${skillPath} && printf '\\n---CMD---\\n' && false && ${wrapperPath} calendar events primary --today --json`, + command: `cat ${skillPath} && printf '\\n---CMD---\\n' && ${wrapperPath} calendar events primary --today --json`, allowlist: [{ pattern: wrapperPath }], safeBins: new Set(), cwd: skillRoot, @@ -1011,7 +968,7 @@ describe("exec approvals shell analysis", () => { const printfPath = binPath("printf"); const gogPath = binPath("gog-wrapper"); const result = evaluateShellAllowlist({ - command: `${shellPath} -c "cat SKILL.md && printf '---CMD---' && gog-wrapper calendar events"`, + command: `${shellPath} -c "cat README.md && printf ready && gog-wrapper calendar events"`, allowlist: [{ pattern: catPath }, { pattern: printfPath }, { pattern: gogPath }], safeBins: new Set(), cwd: dir, @@ -1036,7 +993,7 @@ describe("exec approvals shell analysis", () => { const catPath = binPath("cat"); const gogPath = binPath("gog-wrapper"); const result = evaluateShellAllowlist({ - command: `${shellPath} -c "cat SKILL.md && rm -rf / && gog-wrapper calendar events"`, + command: `${shellPath} -c "cat README.md && rm -rf / && gog-wrapper calendar events"`, allowlist: [{ pattern: catPath }, { pattern: gogPath }], safeBins: new Set(), cwd: dir, diff --git a/src/infra/exec-approvals-analysis.ts b/src/infra/exec-approvals-analysis.ts index 289f15d8d90c..0979b138acee 100644 --- a/src/infra/exec-approvals-analysis.ts +++ b/src/infra/exec-approvals-analysis.ts @@ -1104,14 +1104,7 @@ function renderInlineChainSegmentArgv(params: { export function buildSafeBinsShellCommand(params: { command: string; segments: ExecCommandSegment[]; - segmentSatisfiedBy: ( - | "allowlist" - | "safeBins" - | "inlineChain" - | "skills" - | "skillPrelude" - | null - )[]; + segmentSatisfiedBy: ("allowlist" | "safeBins" | "inlineChain" | "skills" | null)[]; cwd?: string; env?: NodeJS.ProcessEnv; platform?: string | null;