Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions scripts/openclaw-config-guard.py
Original file line number Diff line number Diff line change
Expand Up @@ -3461,6 +3461,20 @@ def _transition(
raise GuardError(exc.code, exc.path, detail) from exc
raise GuardError("transition-failed", opened.config_path, detail) from exc

if _is_mutable_dir_posture(opened, identity):
# Unlock is idempotent, mirroring the already-locked short-circuits in
# the lock branch above. A config already in the exact mutable posture
# is the unlock target: on some platforms (DGX Spark/Station, macOS) an
# in-sandbox OpenClaw reconciler re-permissions the config back to
# sandbox-owned mutable *after* a host lock returns, and a freshly
# onboarded or snapshot-restored sandbox boots mutable before the lock
# settles. Requiring the locked posture there rejected a legitimate
# `shields down` with config-not-locked even though the config already
# holds the mutable target posture. Verify the exact mutable posture
# and treat the transition as a no-op instead of failing.
pair = _snapshot_raw_pair(opened)
_verify_mutable_posture(opened, pair, identity)
return
Comment on lines +3464 to +3477

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The directory check happens before _snapshot_raw_pair(), but this tree is already sandbox-writable. If a sandbox process changes the parent or config directory during capture, _verify_mutable_files() still passes and this branch returns success without rechecking the directory. I reproduced _transition("unlock", ...) succeeding with final config-directory mode 0700. Capture the pair, then validate the full mutable posture (for example _verify_mutable_posture(opened, pair, identity)) and add a directory-drift regression.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed at exact signed/Verified head 32c1b9698. The already-mutable unlock path now captures with _snapshot_raw_pair() and then calls _verify_mutable_posture(opened, pair, identity) before returning. The regression mutates the config directory to 0700 during capture and requires fail-closed config-not-mutable; the companion rejected-mode coverage also proves a 0600 file remains unchanged. The focused guard suite passes 45/45 and exact-range hooks pass. Leaving this thread open for your re-review.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact head update: aec2516 is now current-main synced and GitHub Verified; the feature diff is unchanged. The full mutable-posture revalidation and directory-drift regression remain in place. Exact-head guard tests pass 45/45 under the repository Python 3.11 environment, exact-range hooks pass, and the fresh WSL, macOS, CLI, CodeQL, and E2E gates are green. Leaving the thread open for your re-review.

pair = _snapshot_pair(opened)
_verify_locked_posture(opened, pair, identity, allow_blocking_flags=True)
snapshots: list[FileSnapshot] = []
Expand Down
69 changes: 39 additions & 30 deletions test/openclaw-config-guard.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ import { afterEach, describe, expect, it } from "vitest";

const GUARD_PATH = path.resolve("scripts/openclaw-config-guard.py");
const fixtures: string[] = [];

const RUN_AS_CURRENT_USER = String.raw`
import importlib.util
import hashlib
import os
import sys
import time

guard_path, action, config_dir, failure, expected_sha256 = sys.argv[1:6]
spec = importlib.util.spec_from_file_location("nemoclaw_openclaw_config_guard", guard_path)
module = importlib.util.module_from_spec(spec)
Expand Down Expand Up @@ -72,14 +70,17 @@ if failure in {"installed-nonroot-no-cap", "installed-nonroot-not-ready"}:
module._pid1_effective_uid = lambda: identity.root_uid + 1
if failure == "startup-owner":
module.os.getppid = lambda: 1
if failure == "pair-race":
if failure in {"pair-race", "mutable-dir-drift"}:
original_snapshot = module._snapshot_file
raced = False
def race_pair(opened, name):
global raced
snapshot = original_snapshot(opened, name)
if name == "openclaw.json" and not raced:
raced = True
if failure == "mutable-dir-drift":
os.chmod(config_dir, 0o700)
return snapshot
updated = b'{"gateway":{"port":19001}}\n'
with open(os.path.join(config_dir, "openclaw.json"), "wb") as stream:
stream.write(updated)
Expand Down Expand Up @@ -257,11 +258,9 @@ type GuardLine = {
function shellQuote(value: string): string {
return `'${value.replaceAll("'", `'\\''`)}'`;
}

function trustedNodePath(configDir: string): string {
return path.join(path.dirname(configDir), ".nemoclaw-test-node");
}

function fixture() {
const created = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-openclaw-config-guard-"));
const root = fs.realpathSync(created);
Expand All @@ -287,7 +286,6 @@ function fixture() {
fs.chmodSync(root, 0o755);
return { root, configDir, configPath, hashPath };
}

type GuardAction =
| "preflight"
| "preflight-restart"
Expand All @@ -299,7 +297,6 @@ type GuardAction =
| "publish-startup-ready"
| "write-config"
| "recover";

function runGuard(
action: GuardAction,
configDir: string,
Expand Down Expand Up @@ -331,11 +328,19 @@ function runGuard(
.map((line) => JSON.parse(line) as GuardLine);
return { ...result, lines };
}

function mode(filePath: string): number {
return fs.lstatSync(filePath).mode & 0o7777;
}

function fileIdentity(filePath: string): [number, Buffer] {
const fd = fs.openSync(filePath, "r");
try {
return [fs.fstatSync(fd).ino, fs.readFileSync(fd)];
} finally {
fs.closeSync(fd);
}
}

function setUserXattr(filePath: string, value: string): boolean {
return (
spawnSync(
Expand Down Expand Up @@ -396,24 +401,40 @@ describe("openclaw-config-guard", () => {
const { root, configDir, configPath, hashPath } = fixture();
const first = runGuard("lock", configDir);
expect(first.status, JSON.stringify(first.lines)).toBe(0);
const configInode = fs.statSync(configPath).ino;
const hashInode = fs.statSync(hashPath).ino;
const configBytes = fs.readFileSync(configPath);
const hashBytes = fs.readFileSync(hashPath);
const fileIdentities = [configPath, hashPath].map(fileIdentity);
expect(mode(root)).toBe(0o1775);

const second = runGuard("lock", configDir);
expect(second.status, JSON.stringify(second.lines)).toBe(0);
expect(mode(root)).toBe(0o1775);
expect(mode(configDir)).toBe(0o755);
expect(mode(configPath)).toBe(0o444);
expect(mode(hashPath)).toBe(0o444);
expect(fs.statSync(configPath).ino).toBe(configInode);
expect(fs.statSync(hashPath).ino).toBe(hashInode);
expect(fs.readFileSync(configPath)).toEqual(configBytes);
expect(fs.readFileSync(hashPath)).toEqual(hashBytes);
expect([configPath, hashPath].map(fileIdentity)).toEqual(fileIdentities);
});
it("unlocks idempotently when the config already holds the mutable posture (#7430)", () => {
const { configDir, configPath, hashPath } = fixture();
const fileIdentities = [configPath, hashPath].map(fileIdentity);
const r = runGuard("unlock", configDir);
expect(r.status, JSON.stringify(r.lines)).toBe(0);
expect([mode(configDir), mode(configPath), mode(hashPath)]).toEqual([0o2770, 0o660, 0o660]);
expect([configPath, hashPath].map(fileIdentity)).toEqual(fileIdentities);
for (const invalidPath of [configPath, hashPath]) {
fs.chmodSync(invalidPath, 0o600);
try {
const invalid = runGuard("unlock", configDir);
expect(invalid.status).not.toBe(0);
expect(invalid.lines).toContainEqual(
expect.objectContaining({ code: "config-not-mutable" }),
);
expect(mode(invalidPath)).toBe(0o600);
expect([configPath, hashPath].map(fileIdentity)).toEqual(fileIdentities);
} finally {
fs.chmodSync(invalidPath, 0o660);
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const drift = runGuard("unlock", configDir, "mutable-dir-drift");
expect(drift.lines).toContainEqual(expect.objectContaining({ code: "config-not-mutable" }));
});

it("fresh-replaces both files on lock and unlock while preserving bytes, times, and xattrs", () => {
const { root, configDir, configPath, hashPath } = fixture();
const preservedTime = new Date("2025-01-02T03:04:05.000Z");
Expand Down Expand Up @@ -475,7 +496,6 @@ describe("openclaw-config-guard", () => {
fs.closeSync(staleHashFd);
}
});

it("rejects external symlink, hardlink, and special-file substitutions", () => {
for (const attack of ["symlink", "hardlink", "fifo"] as const) {
const { root, configDir, configPath, hashPath } = fixture();
Expand Down Expand Up @@ -506,7 +526,6 @@ describe("openclaw-config-guard", () => {
expect(fs.readFileSync(hashPath)).toEqual(beforeHash);
}
});

it("fail-closes a rename-swapped config namespace and leaves the external tree untouched", () => {
const { root, configDir } = fixture();
const realConfig = path.join(root, "real-openclaw");
Expand All @@ -523,7 +542,6 @@ describe("openclaw-config-guard", () => {
expect(mode(configDir)).toBe(0o755);
expect(mode(path.join(configDir, "openclaw.json"))).toBe(0o444);
});

it("canonicalizes a stale mutable hash while strict preflight rejects a bad record path", () => {
const mismatch = fixture();
const mismatchBytes = fs.readFileSync(mismatch.configPath);
Expand Down Expand Up @@ -558,12 +576,9 @@ describe("openclaw-config-guard", () => {
});
expect(runGuard("preflight", absolute.configDir).status).toBe(0);
});

it("retries config and hash as one pair when a writer interleaves their capture", () => {
const { configDir, configPath, hashPath } = fixture();

const result = runGuard("lock", configDir, "pair-race");

expect(result.status).toBe(0);
const updated = Buffer.from('{"gateway":{"port":19001}}\n');
expect(fs.readFileSync(configPath)).toEqual(updated);
Expand All @@ -573,7 +588,6 @@ describe("openclaw-config-guard", () => {
expect(mode(configPath)).toBe(0o444);
expect(mode(hashPath)).toBe(0o444);
});

it("restart preflight accepts a stable parseable config with a stale mutable hash", () => {
const { configDir, hashPath } = fixture();
fs.writeFileSync(hashPath, `${"0".repeat(64)} openclaw.json\n`);
Expand All @@ -588,7 +602,6 @@ describe("openclaw-config-guard", () => {
]),
);
});

it("rolls a failed unlock back to the complete locked parent, config, and file posture", () => {
const { root, configDir, configPath, hashPath } = fixture();
const configBytes = fs.readFileSync(configPath);
Expand All @@ -610,7 +623,6 @@ describe("openclaw-config-guard", () => {
expect(fs.readFileSync(configPath)).toEqual(configBytes);
expect(fs.readFileSync(hashPath)).toEqual(hashBytes);
});

it("clears descriptor-bound immutable flags for replacement and restores them on rollback", () => {
const { root, configDir } = fixture();
const flagLog = path.join(root, "inode-flags.log");
Expand All @@ -632,7 +644,6 @@ describe("openclaw-config-guard", () => {
expect(appliedFlags).toContain(0x10);
expect(mode(configDir)).toBe(0o755);
});

it("enforces the exact production path and bounded config artifact sizes", () => {
const { configDir, hashPath } = fixture();
const noPathOverride = RUN_AS_CURRENT_USER.replace(
Expand Down Expand Up @@ -660,7 +671,6 @@ describe("openclaw-config-guard", () => {
]),
);
});

it("CAS-writes a fresh mutable config/hash pair and revokes stale descriptors", () => {
const { root, configDir, configPath, hashPath } = fixture();
const oldConfig = fs.readFileSync(configPath);
Expand Down Expand Up @@ -705,7 +715,6 @@ describe("openclaw-config-guard", () => {
fs.closeSync(staleHashFd);
}
});

it("safely replaces a sandbox-precreated persistent journal symlink", () => {
const { root, configDir, configPath } = fixture();
const original = fs.readFileSync(configPath);
Expand Down
Loading