1. src/infra/exec-approvals-allowlist.ts, line 561-573 (link)

   cat guard relies on two complementary checks

   isSkillPreludeReadSegment correctly checks segment.argv.length !== 2, so cat -A SKILL.md or cat SKILL.md extra are rejected. Any shell-level quoting that produces exactly two argv tokens but routes to a non-skills path would also be caught downstream by isSkillMarkdownPreludePath. The length-2 guard plus the path check together close the gap — noting this here to help future readers understand why both checks are necessary.

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: src/infra/exec-approvals-allowlist.ts
   Line: 561-573
   
   Comment:
   **`cat` guard relies on two complementary checks**
   
   `isSkillPreludeReadSegment` correctly checks `segment.argv.length !== 2`, so `cat -A SKILL.md` or `cat SKILL.md extra` are rejected. Any shell-level quoting that produces exactly two argv tokens but routes to a non-skills path would also be caught downstream by `isSkillMarkdownPreludePath`. The length-2 guard plus the path check together close the gap — noting this here to help future readers understand why both checks are necessary.
   
   How can I resolve this? If you propose a fix, please make it concise.

1. 🟠 Allowlist bypass: auto-allowed cat .../skills/SKILL.md prelude enables arbitrary file read (symlink) when any trusted wrapper is later in && chain

Description

The new “skill prelude” exception can mark a non-allowlisted cat <path>/SKILL.md segment as satisfied ("skillPrelude") as long as a later trusted skill wrapper is reachable via a contiguous && chain.

This is unsafe because isSkillMarkdownPreludePath() treats any path ending in /skills/SKILL.md (no skill id directory) as a valid prelude:

• segmentsAfterSkills === 1 matches .../skills/SKILL.md anywhere on disk
• In that case resolveSkillPreludeIds() returns an empty set, and the reachability check degrades to reachableSkillIds.size > 0
• Result: if the chain later reaches any trusted wrapper, the earlier cat .../skills/SKILL.md becomes auto-allowed even though it is not allowlisted

Because the allow decision is based only on a string path pattern (via path.resolve) and does not validate realpath nor enforce a trusted skills root, an attacker who can create files/symlinks in the working directory can point .../skills/SKILL.md at arbitrary sensitive files (e.g., /etc/shadow, SSH keys, cloud creds) and have them printed to stdout without an approval prompt.

Vulnerable logic (pattern too broad + reachability fallback):

if (segmentsAfterSkills === 1 || segmentsAfterSkills === 2) {
  return true;
}
...
const reachesTrustedSkillExecution =
  opToNext === "&&" &&
  (preludeSkillIds.size === 0
    ? reachableSkillIds.size > 0
    : [...preludeSkillIds].some((skillId) => reachableSkillIds.has(skillId)));

Recommendation

Tighten the prelude exception so it cannot be used for arbitrary file reads:

1. Remove the .../skills/SKILL.md (no-id) case. Require an explicit skill id directory: .../skills/<skillId>/SKILL.md.
2. Enforce a trusted skills root (e.g., ${cwd}/.openclaw/skills or configured root) and validate with fs.realpathSync to prevent symlink escapes.
3. Require that the prelude’s skillId matches the reachable trusted wrapper id; do not allow the preludeSkillIds.size === 0 fallback.

Example hardening:

import fs from "node:fs";

function isTrustedSkillPreludePath(rawPath: string, cwd: string, skillsRoot: string): string | null {
  const abs = resolveSkillPreludePath(rawPath, cwd);
  const real = fs.realpathSync.native(abs);
  const rootReal = fs.realpathSync.native(skillsRoot);
  if (!real.startsWith(rootReal + path.sep)) return null;
  if (!/\/skills\/[a-z0-9_-]+\/SKILL\.md$/i.test(real.replace(/\\/g,"/"))) return null;
  return real;
}

​// and only allow when extracted skillId is non-null and matches reachableSkillIds

2. 🟡 Policy bypass: /approve rejection misses env options and nested shell execution

Description

rejectExecApprovalShellCommand() attempts to prevent running /approve <id> <decision> via the exec tool by stripping common prefixes (env/command/sudo/etc) and then applying parseExecApprovalCommandText().

However, the prefix-stripping logic is incomplete and can yield false negatives:

• env options are not skipped. For commands like env -i /approve abc123 allow-once, the function strips only the env token and then stops at -i, so the candidate string no longer starts with /approve and the rejection does not trigger.
• Nested execution via interpreters is not inspected (e.g., sh -c '/approve abc123 allow-once'). The top-level argv starts with sh, so the /approve string inside -c is not detected.

Vulnerable code:

if (token === "env") {
  remaining.shift();
  continue;
}
...
const candidates = analysis.ok
  ? analysis.segments.map((segment) => stripApprovalCommandPrefixes(segment.argv).join(" "))
  : ...

Impact depends on the execution environment, but as written this is a policy enforcement bypass: an attacker/user can construct a shell command that still executes /approve ... without being blocked by this guard.

Recommendation

Harden detection so it cannot be bypassed by prefix options or nesting.

At minimum:

1. Handle env options (GNU/BSD) before stopping, similar to sudo handling.
2. Consider rejecting if any argv token contains an approval command (e.g., a -c string), or more conservatively reject any command containing /approve as a word boundary.

Example (minimal improvement for env):

if (token === "env") {
  remaining.shift();
  ​// consume common env options
  while (remaining[0]?.startsWith("-")) {
    const opt = remaining.shift()!;
    if (opt === "--") break;
    const norm = opt.split("=", 1)[0];
    ​// -u VAR takes a value
    if (norm === "-u" && !opt.includes("=") && remaining[0]) remaining.shift();
  }
  continue;
}

For nested shells:

• Either explicitly detect patterns like sh -c/bash -c and scan the -c payload with parseExecApprovalCommandText() (and/or analyzeShellCommand() recursively),
• Or implement a stricter policy: reject any exec command that contains a token starting with /approve anywhere (after shell parsing), not only at argv[0].