mirror of https://github.com/openclaw/openclaw.git
fix: validation completeness and consumer invariant consistency across all enforcement layers
- permAllowsWrite (bwrap), permToOps/deniedOps (seatbelt): guard all positional perm accesses with VALID_PERM_RE - catchAllPerm/tmpPerm (seatbelt): validate rawPerm before positional access; fail closed to '---' - hasScriptOverride (exec-runtime): check entry shape (non-null object, not array) before setting bypass flag - scripts["policy"] merged into overrideRules in applyScriptPolicyOverride (was silently dropped) - mergeAccessPolicy: reject non-object script entries before propagating - validateAccessPolicyFileStructure: recurse into per-script entries to catch removed deny/default fields - validateAccessPolicyConfig: reject non-object entries, validate sha256 format, emit mid-path wildcard diagnostics for scripts["policy"] AND per-script policy blocks (previously only config.policy) - env-prefix regex: handle escaped quotes in double-quoted values ((?:[^"\\]|\\.)*) - _resetBwrapAvailableCacheForTest: export added for test isolation - Tests added for all of the above
This commit is contained in:
parent
eb40d49d44
commit
65946937a0
|
|
@ -390,9 +390,17 @@ export async function runExecProcess(opts: {
|
|||
// symlink keys ("/usr/bin/python" → /usr/bin/python3.12) match argv0, which
|
||||
// is always the realpathSync result from resolveArgv0.
|
||||
const _scripts = opts.permissions.scripts ?? {};
|
||||
// Skip the reserved "policy" key — it holds shared rules, not a per-script entry.
|
||||
const hasScriptOverride = Object.keys(_scripts).some(
|
||||
(k) => k !== "policy" && path.normalize(resolveScriptKey(k)) === path.normalize(argv0),
|
||||
// Skip the reserved "policy" key and malformed (non-object) entries — only a
|
||||
// validated ScriptPolicyEntry object counts as a legitimate script override.
|
||||
// A truthy primitive (true, "oops") would otherwise bypass the base exec gate
|
||||
// even though applyScriptPolicyOverride already rejected the entry.
|
||||
const hasScriptOverride = Object.entries(_scripts).some(
|
||||
([k, v]) =>
|
||||
k !== "policy" &&
|
||||
v != null &&
|
||||
typeof v === "object" &&
|
||||
!Array.isArray(v) &&
|
||||
path.normalize(resolveScriptKey(k)) === path.normalize(argv0),
|
||||
);
|
||||
if (!hasScriptOverride && checkAccessPolicy(argv0, "exec", effectivePermissions) === "deny") {
|
||||
throw new Error(`exec denied by access policy: ${argv0}`);
|
||||
|
|
|
|||
|
|
@ -66,6 +66,15 @@ export function mergeAccessPolicy(
|
|||
const baseEntry = base.scripts?.[key] as
|
||||
| import("../config/types.tools.js").ScriptPolicyEntry
|
||||
| undefined;
|
||||
// Reject non-object entries — a primitive (true, "rwx") is not a valid ScriptPolicyEntry.
|
||||
// validateAccessPolicyConfig also catches this, but the merge layer must not propagate it.
|
||||
if (
|
||||
overrideEntry == null ||
|
||||
typeof overrideEntry !== "object" ||
|
||||
Array.isArray(overrideEntry)
|
||||
) {
|
||||
continue;
|
||||
}
|
||||
const overrideScriptEntry =
|
||||
overrideEntry as import("../config/types.tools.js").ScriptPolicyEntry;
|
||||
if (!baseEntry) {
|
||||
|
|
@ -123,7 +132,27 @@ function validateAccessPolicyFileStructure(filePath: string, parsed: unknown): s
|
|||
if (typeof block !== "object" || block === null || Array.isArray(block)) {
|
||||
errors.push(`${filePath}: agents["${agentId}"] must be an object`);
|
||||
} else {
|
||||
checkRemovedKeys(block as Record<string, unknown>, `agents["${agentId}"]`);
|
||||
const agentBlock = block as Record<string, unknown>;
|
||||
checkRemovedKeys(agentBlock, `agents["${agentId}"]`);
|
||||
// Recurse into per-script entries so old "deny"/"default" fields are caught there too.
|
||||
const scripts = agentBlock["scripts"];
|
||||
if (scripts != null && typeof scripts === "object" && !Array.isArray(scripts)) {
|
||||
for (const [scriptKey, scriptEntry] of Object.entries(
|
||||
scripts as Record<string, unknown>,
|
||||
)) {
|
||||
if (
|
||||
scriptKey !== "policy" &&
|
||||
scriptEntry != null &&
|
||||
typeof scriptEntry === "object" &&
|
||||
!Array.isArray(scriptEntry)
|
||||
) {
|
||||
checkRemovedKeys(
|
||||
scriptEntry as Record<string, unknown>,
|
||||
`agents["${agentId}"].scripts["${scriptKey}"]`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -298,7 +327,9 @@ export function resolveAccessPolicyForAgent(agentId?: string): AccessPolicyConfi
|
|||
// auto-expand diagnostics ("rule auto-expanded to ...") are informational
|
||||
// and the footer would mislead operators into thinking the policy is broken.
|
||||
if (errors.some((e) => !e.includes("auto-expanded") && !e.includes("mid-path wildcard"))) {
|
||||
console.error(`[access-policy] Bad permission strings are treated as "---" (deny all).`);
|
||||
console.error(
|
||||
`[access-policy] Bad permission strings are treated as "---" (deny all) at the tool layer and OS sandbox layer.`,
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -70,6 +70,73 @@ describe("validateAccessPolicyConfig", () => {
|
|||
expect(checkAccessPolicy(file, "read", config)).toBe("deny");
|
||||
});
|
||||
|
||||
it("rejects non-object script entries (e.g. a bare string or boolean)", () => {
|
||||
// A primitive entry like "/deploy.sh": "rwx" or "/deploy.sh": true would bypass
|
||||
// the exec gate — validateAccessPolicyConfig must reject it at load time.
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
"/deploy.sh": "rwx" as unknown as import("../config/types.tools.js").ScriptPolicyEntry,
|
||||
},
|
||||
};
|
||||
const errs = validateAccessPolicyConfig(config);
|
||||
expect(errs.some((e) => e.includes("/deploy.sh") && e.includes("must be an object"))).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects a sha256 value with wrong length", () => {
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
"/deploy.sh": { sha256: "abc123" },
|
||||
},
|
||||
};
|
||||
const errs = validateAccessPolicyConfig(config);
|
||||
expect(errs.some((e) => e.includes("sha256") && e.includes("64-character"))).toBe(true);
|
||||
});
|
||||
|
||||
it("rejects a sha256 value with non-hex characters", () => {
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
"/deploy.sh": { sha256: "z".repeat(64) },
|
||||
},
|
||||
};
|
||||
const errs = validateAccessPolicyConfig(config);
|
||||
expect(errs.some((e) => e.includes("sha256") && e.includes("64-character"))).toBe(true);
|
||||
});
|
||||
|
||||
it("accepts a valid 64-char hex sha256", () => {
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
"/deploy.sh": { sha256: "a".repeat(64) },
|
||||
},
|
||||
};
|
||||
expect(validateAccessPolicyConfig(config)).toEqual([]);
|
||||
});
|
||||
|
||||
it("emits mid-path wildcard diagnostic for scripts['policy'] entries", () => {
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
policy: { "/home/*/workspace/**": "r--" },
|
||||
},
|
||||
};
|
||||
const errs = validateAccessPolicyConfig(config);
|
||||
expect(
|
||||
errs.some((e) => e.includes("mid-path wildcard") && e.includes('scripts["policy"]')),
|
||||
).toBe(true);
|
||||
});
|
||||
|
||||
it("emits mid-path wildcard diagnostic for per-script policy entries", () => {
|
||||
const config: AccessPolicyConfig = {
|
||||
scripts: {
|
||||
"/deploy.sh": { policy: { "/home/*/workspace/**": "r--" } },
|
||||
},
|
||||
};
|
||||
const errs = validateAccessPolicyConfig(config);
|
||||
expect(errs.some((e) => e.includes("mid-path wildcard") && e.includes("/deploy.sh"))).toBe(
|
||||
true,
|
||||
);
|
||||
});
|
||||
|
||||
it("validates scripts[].policy perm strings and emits diagnostics for bad ones", () => {
|
||||
// A typo like "rwX" in a script's policy must produce a diagnostic, not silently
|
||||
// fail closed (which would deny exec with no operator-visible error).
|
||||
|
|
@ -717,6 +784,21 @@ describe("resolveArgv0", () => {
|
|||
expect(result).toMatch(/sh$/);
|
||||
});
|
||||
|
||||
it("handles env assignment with escaped quote inside double-quoted value", () => {
|
||||
// MYVAR="a\"b" /usr/bin/python script.py — the \" inside the value must not
|
||||
// truncate the match, which would leave `b"` as the next token and misidentify
|
||||
// it as argv0 instead of /usr/bin/python.
|
||||
const result = resolveArgv0('MYVAR="a\\"b" /bin/sh -c echo');
|
||||
expect(result).not.toBeNull();
|
||||
expect(result).toMatch(/sh$/);
|
||||
});
|
||||
|
||||
it("handles multiple env assignments with escaped quotes in values", () => {
|
||||
const result = resolveArgv0('A="x\\"y" B="p\\"q" /bin/sh -c echo');
|
||||
expect(result).not.toBeNull();
|
||||
expect(result).toMatch(/sh$/);
|
||||
});
|
||||
|
||||
// The following tests use /bin/sh and Unix env behaviour — skip on Windows where
|
||||
// /bin/sh doesn't exist and env resolves to env.EXE with different semantics.
|
||||
const itUnix = it.skipIf(process.platform === "win32");
|
||||
|
|
|
|||
|
|
@ -132,6 +132,15 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[]
|
|||
`access-policy.scripts["policy"]["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`,
|
||||
);
|
||||
}
|
||||
if (
|
||||
hasMidPathWildcard(pattern) &&
|
||||
!_midPathWildcardWarned.has(`scripts:policy:${pattern}`)
|
||||
) {
|
||||
_midPathWildcardWarned.add(`scripts:policy:${pattern}`);
|
||||
errors.push(
|
||||
`access-policy.scripts["policy"]["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`,
|
||||
);
|
||||
}
|
||||
autoExpandBareDir(sharedPolicy, pattern, perm, errors);
|
||||
}
|
||||
}
|
||||
|
|
@ -139,14 +148,41 @@ export function validateAccessPolicyConfig(config: AccessPolicyConfig): string[]
|
|||
if (scriptPath === "policy") {
|
||||
continue; // handled above
|
||||
}
|
||||
const scriptEntry = entry as import("../config/types.tools.js").ScriptPolicyEntry | undefined;
|
||||
if (scriptEntry?.policy) {
|
||||
// Reject non-object entries (e.g. true, "rwx") — a truthy primitive would
|
||||
// bypass the exec gate in hasScriptOverride and applyScriptPolicyOverride.
|
||||
if (entry == null || typeof entry !== "object" || Array.isArray(entry)) {
|
||||
errors.push(
|
||||
`access-policy.scripts["${scriptPath}"] must be an object (e.g. { sha256: "...", policy: {...} }), got ${JSON.stringify(entry)}`,
|
||||
);
|
||||
continue;
|
||||
}
|
||||
const scriptEntry = entry as import("../config/types.tools.js").ScriptPolicyEntry;
|
||||
// Validate sha256 format when present — a typo causes silent exec denial at runtime.
|
||||
if (scriptEntry.sha256 !== undefined) {
|
||||
if (!/^[0-9a-f]{64}$/i.test(scriptEntry.sha256)) {
|
||||
errors.push(
|
||||
`access-policy.scripts["${scriptPath}"].sha256 "${scriptEntry.sha256}" is invalid: must be a 64-character lowercase hex string`,
|
||||
);
|
||||
}
|
||||
}
|
||||
if (scriptEntry.policy) {
|
||||
for (const [pattern, perm] of Object.entries(scriptEntry.policy)) {
|
||||
if (!PERM_STR_RE.test(perm)) {
|
||||
errors.push(
|
||||
`access-policy.scripts["${scriptPath}"].policy["${pattern}"] "${perm}" is invalid: must be exactly 3 chars (e.g. "rwx", "r--", "---")`,
|
||||
);
|
||||
}
|
||||
// Emit mid-path wildcard diagnostic for per-script policy blocks,
|
||||
// matching the same warning emitted for config.policy entries.
|
||||
if (
|
||||
hasMidPathWildcard(pattern) &&
|
||||
!_midPathWildcardWarned.has(`scripts:${scriptPath}:${pattern}`)
|
||||
) {
|
||||
_midPathWildcardWarned.add(`scripts:${scriptPath}:${pattern}`);
|
||||
errors.push(
|
||||
`access-policy.scripts["${scriptPath}"].policy["${pattern}"] contains a mid-path wildcard — OS-level enforcement uses prefix match (file-type filter is tool-layer only).`,
|
||||
);
|
||||
}
|
||||
autoExpandBareDir(scriptEntry.policy, pattern, perm, errors);
|
||||
}
|
||||
}
|
||||
|
|
@ -396,11 +432,14 @@ export function resolveArgv0(command: string, cwd?: string, _depth = 0): string
|
|||
// FOO='a b') prevents misparse when a quoted env value contains spaces — a naive
|
||||
// whitespace-split would break FOO='a b' /script.sh into ["FOO='a", "b'", "/script.sh"]
|
||||
// and incorrectly treat "b'" as the argv0, bypassing script policy lookups.
|
||||
const envPrefixRe = /^[A-Za-z_][A-Za-z0-9_]*=(?:"[^"]*"|'[^']*'|\S*)\s*/;
|
||||
// Double-quoted values: allow backslash-escaped characters (e.g. "a\"b") so the
|
||||
// regex doesn't truncate at the escaped quote and misidentify the next token as argv0.
|
||||
// Single-quoted values: no escaping in POSIX sh single quotes, so [^']* is correct.
|
||||
const envPrefixRe = /^[A-Za-z_][A-Za-z0-9_]*=(?:"(?:[^"\\]|\\.)*"|'[^']*'|\S*)\s*/;
|
||||
let rest = trimmed;
|
||||
while (envPrefixRe.test(rest)) {
|
||||
// Capture a literal PATH= override; skip if the value contains $ (unexpandable).
|
||||
const pathM = rest.match(/^PATH=(?:"([^"]*)"|'([^']*)'|(\S+))\s*/);
|
||||
const pathM = rest.match(/^PATH=(?:"((?:[^"\\]|\\.)*)"|'([^']*)'|(\S+))\s*/);
|
||||
if (pathM) {
|
||||
const val = pathM[1] ?? pathM[2] ?? pathM[3] ?? "";
|
||||
if (!val.includes("$")) {
|
||||
|
|
|
|||
|
|
@ -2,9 +2,11 @@ import os from "node:os";
|
|||
import { describe, expect, it, vi } from "vitest";
|
||||
import type { AccessPolicyConfig } from "../config/types.tools.js";
|
||||
import {
|
||||
_resetBwrapAvailableCacheForTest,
|
||||
_resetBwrapFileDenyWarnedPathsForTest,
|
||||
_warnBwrapFileDenyOnce,
|
||||
generateBwrapArgs,
|
||||
isBwrapAvailable,
|
||||
wrapCommandWithBwrap,
|
||||
} from "./exec-sandbox-bwrap.js";
|
||||
|
||||
|
|
@ -428,6 +430,18 @@ describe("wrapCommandWithBwrap", () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe("_resetBwrapAvailableCacheForTest", () => {
|
||||
it("clears the availability cache so isBwrapAvailable re-probes", async () => {
|
||||
// Prime the cache with one result, then reset and verify the next call re-checks.
|
||||
await isBwrapAvailable(); // populates cache
|
||||
_resetBwrapAvailableCacheForTest();
|
||||
// After reset, isBwrapAvailable must re-probe (result may differ in test env — just
|
||||
// verify it returns a boolean without throwing, proving the cache was cleared).
|
||||
const result = await isBwrapAvailable();
|
||||
expect(typeof result).toBe("boolean");
|
||||
});
|
||||
});
|
||||
|
||||
describe("_resetBwrapFileDenyWarnedPathsForTest", () => {
|
||||
it("clears the warned-paths set so the same path can warn again", () => {
|
||||
const spy = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
|
|
|
|||
|
|
@ -36,6 +36,10 @@ const _bwrapFileDenyWarnedPaths = new Set<string>();
|
|||
export function _resetBwrapFileDenyWarnedPathsForTest(): void {
|
||||
_bwrapFileDenyWarnedPaths.clear();
|
||||
}
|
||||
/** Reset the bwrap availability cache. Only for use in tests. */
|
||||
export function _resetBwrapAvailableCacheForTest(): void {
|
||||
bwrapAvailableCache = undefined;
|
||||
}
|
||||
export function _warnBwrapFileDenyOnce(filePath: string): void {
|
||||
if (_bwrapFileDenyWarnedPaths.has(filePath)) {
|
||||
return;
|
||||
|
|
@ -116,8 +120,11 @@ function patternToPath(pattern: string, homeDir: string, perm?: PermStr): string
|
|||
return parentDir || "/";
|
||||
}
|
||||
|
||||
// Keep in sync with VALID_PERM_RE in access-policy.ts and exec-sandbox-seatbelt.ts.
|
||||
const VALID_PERM_RE = /^[r-][w-][x-]$/;
|
||||
|
||||
function permAllowsWrite(perm: PermStr): boolean {
|
||||
return perm[1] === "w";
|
||||
return VALID_PERM_RE.test(perm) && perm[1] === "w";
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -143,7 +150,9 @@ export function generateBwrapArgs(
|
|||
): string[] {
|
||||
const args: string[] = [];
|
||||
// Determine base stance from the "/**" catch-all rule (replaces the removed `default` field).
|
||||
const catchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---";
|
||||
const rawCatchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---";
|
||||
// Validate format before positional access — malformed perm falls back to "---" (fail closed).
|
||||
const catchAllPerm = VALID_PERM_RE.test(rawCatchAllPerm) ? rawCatchAllPerm : "---";
|
||||
const defaultAllowsRead = catchAllPerm[0] === "r";
|
||||
|
||||
if (defaultAllowsRead) {
|
||||
|
|
|
|||
|
|
@ -135,7 +135,13 @@ function patternToSbplMatcher(pattern: string, homeDir: string, perm?: PermStr):
|
|||
return { matcher: sbplLiteral(base), approximate: false };
|
||||
}
|
||||
|
||||
// Keep in sync with VALID_PERM_RE in access-policy.ts and exec-sandbox-bwrap.ts.
|
||||
const VALID_PERM_RE = /^[r-][w-][x-]$/;
|
||||
|
||||
function permToOps(perm: PermStr): string[] {
|
||||
if (!VALID_PERM_RE.test(perm)) {
|
||||
return [];
|
||||
}
|
||||
const ops: string[] = [];
|
||||
if (perm[0] === "r") {
|
||||
ops.push(SEATBELT_READ_OPS);
|
||||
|
|
@ -150,6 +156,10 @@ function permToOps(perm: PermStr): string[] {
|
|||
}
|
||||
|
||||
function deniedOps(perm: PermStr): string[] {
|
||||
// Malformed perm — deny everything (fail closed).
|
||||
if (!VALID_PERM_RE.test(perm)) {
|
||||
return [SEATBELT_READ_OPS, SEATBELT_WRITE_OPS, SEATBELT_EXEC_OPS];
|
||||
}
|
||||
const ops: string[] = [];
|
||||
if (perm[0] !== "r") {
|
||||
ops.push(SEATBELT_READ_OPS);
|
||||
|
|
@ -184,7 +194,9 @@ export function generateSeatbeltProfile(
|
|||
lines.push("");
|
||||
|
||||
// Determine base stance from the "/**" catch-all rule (replaces the removed `default` field).
|
||||
const catchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---";
|
||||
const rawCatchAllPerm = findBestRule("/**", config.policy ?? {}, homeDir) ?? "---";
|
||||
// Validate format before positional access — malformed perm falls back to "---" (fail closed).
|
||||
const catchAllPerm = VALID_PERM_RE.test(rawCatchAllPerm) ? rawCatchAllPerm : "---";
|
||||
const defaultPerm = catchAllPerm; // alias for readability below
|
||||
const defaultAllowsAnything =
|
||||
catchAllPerm[0] === "r" || catchAllPerm[1] === "w" || catchAllPerm[2] === "x";
|
||||
|
|
@ -223,7 +235,10 @@ export function generateSeatbeltProfile(
|
|||
// unconditionally granting /tmp access when default: "---".
|
||||
// findBestRule probes both the path and path+"/" internally, so "/tmp" correctly
|
||||
// matches glob rules like "/tmp/**" without needing the "/tmp/." workaround.
|
||||
const tmpPerm = findBestRule("/tmp", config.policy ?? {}, homeDir) ?? "---";
|
||||
const rawTmpPerm = findBestRule("/tmp", config.policy ?? {}, homeDir) ?? "---";
|
||||
// Validate before positional access — malformed perm falls back to "---" (fail closed),
|
||||
// consistent with permToOps/deniedOps and the tool-layer permAllows guard.
|
||||
const tmpPerm = VALID_PERM_RE.test(rawTmpPerm) ? rawTmpPerm : "---";
|
||||
// Emit read and write allowances independently so a read-only policy like
|
||||
// "/tmp/**": "r--" does not accidentally grant write access to /tmp.
|
||||
if (tmpPerm[0] === "r") {
|
||||
|
|
|
|||
Loading…
Reference in New Issue