diff --git a/src/infra/access-policy.test.ts b/src/infra/access-policy.test.ts index d9d0970630a..839a4c9216a 100644 --- a/src/infra/access-policy.test.ts +++ b/src/infra/access-policy.test.ts @@ -104,6 +104,18 @@ describe("validateAccessPolicyConfig", () => { expect(errs[0]).toMatch(/auto-expanded/); }); + it("auto-expands non-existent versioned directory names (v1.0, app-2.3) in deny[]", () => { + // Versioned names like "v1.0" or "pkg-1.0.0" look like files via naive dot-detection + // but are almost always directories. The tightened heuristic requires the extension + // to contain at least one letter — digits-only extensions (like ".0") are treated as + // directory-like and expanded to /**. + const base = os.tmpdir(); + const versionedDir = path.join(base, `openclaw-test-v1.0-${Date.now()}`); + const config: AccessPolicyConfig = { deny: [versionedDir] }; + validateAccessPolicyConfig(config); + expect(config.deny?.[0]).toBe(`${versionedDir}/**`); // must be expanded + }); + it("does NOT auto-expand a non-existent deny[] path that looks like a file (has extension)", () => { // "~/future-secrets.key" doesn't exist yet but the extension heuristic should // prevent expansion to "~/future-secrets.key/**" — the user intends to protect @@ -133,6 +145,32 @@ describe("validateAccessPolicyConfig", () => { expect(checkAccessPolicy(file, "read", config)).toBe("deny"); }); + it("validates scripts[].rules perm strings and emits diagnostics for bad ones", () => { + // A typo like "rwX" in a script's rules must produce a diagnostic, not silently + // fail closed (which would deny exec with no operator-visible error). + const config: AccessPolicyConfig = { + scripts: { + "/usr/local/bin/deploy.sh": { + rules: { "~/deploy/**": "rwX" }, // invalid: uppercase X + }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("rwX") && e.includes("scripts"))).toBe(true); + }); + + it("validates scripts[].deny entries and emits diagnostics for empty patterns", () => { + const config: AccessPolicyConfig = { + scripts: { + "/usr/local/bin/deploy.sh": { + deny: ["", "~/.secrets/**"], // first entry is invalid empty string + }, + }, + }; + const errs = validateAccessPolicyConfig(config); + expect(errs.some((e) => e.includes("scripts") && e.includes("deny"))).toBe(true); + }); + it("accepts valid 'rwx' and '---' perm strings", () => { expect(validateAccessPolicyConfig({ default: "rwx" })).toEqual([]); expect(validateAccessPolicyConfig({ default: "---" })).toEqual([]); diff --git a/src/infra/access-policy.ts b/src/infra/access-policy.ts index 3a15374e3d4..bdf9dcb6210 100644 --- a/src/infra/access-policy.ts +++ b/src/infra/access-policy.ts @@ -75,6 +75,29 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] } } + if (config.scripts) { + for (const [scriptPath, entry] of Object.entries(config.scripts)) { + if (entry.rules) { + for (const [pattern, perm] of Object.entries(entry.rules)) { + if (!PERM_STR_RE.test(perm)) { + errors.push( + `access-policy.scripts["${scriptPath}"].rules["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`, + ); + } + } + } + if (entry.deny) { + for (let i = 0; i < entry.deny.length; i++) { + if (!entry.deny[i]) { + errors.push( + `access-policy.scripts["${scriptPath}"].deny[${i}] must be a non-empty glob pattern`, + ); + } + } + } + } + } + if (config.deny) { for (let i = 0; i < config.deny.length; i++) { const pattern = config.deny[i]; @@ -110,8 +133,11 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[] .replace(/[/\\]$/, "") .split(/[/\\]/) .pop() ?? ""; - // Has a dot that is not the leading dot (dotfile), and has chars after the dot. - looksLikeFile = /[^.]\.[^/\\]+$/.test(lastName); + // Has a dot that is not the leading dot (dotfile), and the extension + // contains at least one letter — this excludes version-like suffixes + // (.0, .3, -1.0, app-2.3) which look like versioned directory names. + // Examples: "secrets.key" → file; "v1.0" → directory; ".ssh" → directory. + looksLikeFile = /[^.]\.[a-zA-Z][^/\\]*$/.test(lastName); } if (!isExistingFile && !looksLikeFile) { const fixed = `${pattern}/**`; diff --git a/src/infra/exec-sandbox-bwrap.test.ts b/src/infra/exec-sandbox-bwrap.test.ts index dae956bdcff..458e209bb28 100644 --- a/src/infra/exec-sandbox-bwrap.test.ts +++ b/src/infra/exec-sandbox-bwrap.test.ts @@ -115,6 +115,17 @@ describe("generateBwrapArgs", () => { expect(tmpfsMounts).toContain("/tmp"); }); + it("does not add --tmpfs /tmp in restrictive mode even with no explicit /tmp rule", () => { + // Regression guard: the defaultAllowsRead guard on the /tmp block must prevent + // a writable tmpfs being mounted under default:"---" when no /tmp rule exists. + // explicitTmpPerm === null is true (no rule), but defaultAllowsRead is false, + // so the entire /tmp block must be skipped. + const config: AccessPolicyConfig = { default: "---" }; + const args = generateBwrapArgs(config, HOME); + const tmpfsMounts = args.map((a, i) => (a === "--tmpfs" ? args[i + 1] : null)).filter(Boolean); + expect(tmpfsMounts).not.toContain("/tmp"); + }); + it("skips --tmpfs /tmp in permissive mode when policy explicitly restricts /tmp writes", () => { // A rule "/tmp/**": "r--" means the user wants /tmp read-only; the base --ro-bind / / // already makes it readable. Adding --tmpfs /tmp would silently grant write access. diff --git a/src/infra/exec-sandbox-bwrap.ts b/src/infra/exec-sandbox-bwrap.ts index 452e06d44a6..17f3162f0a6 100644 --- a/src/infra/exec-sandbox-bwrap.ts +++ b/src/infra/exec-sandbox-bwrap.ts @@ -143,14 +143,6 @@ export function generateBwrapArgs( // mount /proc so programs that read /proc/self/*, /proc/cpuinfo, etc. work // correctly inside the sandbox (shells, Python, most build tools need this). args.push("--proc", "/proc"); - // Add writable /tmp tmpfs unless the policy has an explicit rule that denies - // write on /tmp. Without an explicit rule, /tmp is writable by default (needed - // by most processes for temp files). With an explicit "r--" or "---" rule on - // /tmp/**, respect it — /tmp remains read-only via the --ro-bind / / base. - const explicitTmpPerm = findBestRule("/tmp/.", config.rules ?? {}, homeDir); - if (explicitTmpPerm === null || explicitTmpPerm[1] === "w") { - args.push("--tmpfs", "/tmp"); - } args.push("--dev", "/dev"); } else { // Restrictive base: only bind system paths needed to run processes. @@ -165,6 +157,18 @@ export function generateBwrapArgs( // the enclosed process genuinely needs it. } + // Writable /tmp tmpfs — only in permissive mode AND only when the policy does not + // explicitly restrict writes on /tmp. Keeping this outside the if/else block above + // makes the defaultAllowsRead guard self-evident and not implicit from nesting. + // In restrictive mode (default:"---"), /tmp is intentionally omitted so rules + // control tmpfs access explicitly (e.g. "/tmp/**":"rwx" is handled by the rules loop). + if (defaultAllowsRead) { + const explicitTmpPerm = findBestRule("/tmp/.", config.rules ?? {}, homeDir); + if (explicitTmpPerm === null || explicitTmpPerm[1] === "w") { + args.push("--tmpfs", "/tmp"); + } + } + // Apply rules: upgrade paths with w bit to read-write binds. // Sort by concrete path length ascending so less-specific mounts are applied // first — bwrap applies mounts in order, and later mounts win for overlapping