Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 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
99 changes: 99 additions & 0 deletions src/lib/state/config-io.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,105 @@
}
});

it("readConfigFile repairs a 755 parent directory to 700", () => {
const root = makeTempDir();
const dir = path.join(root, "loose-dir");
fs.mkdirSync(dir, { mode: 0o755 });
const file = path.join(dir, "config.json");
fs.writeFileSync(file, JSON.stringify({ repaired: true }), { mode: 0o600 });

const result = readConfigFile(file, null);

expect(result).toEqual({ repaired: true });
expect(fs.statSync(dir).mode & 0o777).toBe(0o700);
});

it("readConfigFile repairs a 644 file to 600", () => {
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const file = path.join(dir, "config.json");
fs.writeFileSync(file, JSON.stringify({ tight: true }), { mode: 0o644 });

const result = readConfigFile(file, null);

expect(result).toEqual({ tight: true });
expect(fs.statSync(file).mode & 0o777).toBe(0o600);
});

it("ensureConfigDir heals every root-level file in the dir, not just the one being read (#4546)", () => {
// #4546 expects auto-repair across all root-level files. Most of those
// files (onboard-session.json, ollama-proxy-token, etc.) are written by
// code paths that don't flow through readConfigFile, so the read-time
// per-file heal alone misses them. The dir walk in ensureConfigDir is
// what covers them — verify by writing several siblings at 644 and
// confirming a single read tightens all of them to 600.
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });

const siblings = [
"onboard-session.json",
"ollama-proxy-token",
"ollama-auth-proxy.pid",
"usage-notice.json",
];
for (const name of siblings) {
fs.writeFileSync(path.join(dir, name), "stale", { mode: 0o644 });
}

readConfigFile(target, null);

for (const name of siblings) {
const mode = fs.statSync(path.join(dir, name)).mode & 0o777;
expect(mode, `${name} should be tightened to 600`).toBe(0o600);
}
});

it("ensureConfigDir skips symlinks during the root-level heal", () => {
// A chmod on a symlink follows to the target — if ~/.nemoclaw/X is a
// symlink to /etc/passwd, healing must NOT chmod /etc/passwd. lstat
// before chmod keeps the heal scoped to real files inside the dir.
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });

const outside = path.join(os.tmpdir(), `nemoclaw-symlink-target-${String(process.pid)}`);
fs.writeFileSync(outside, "outside", { mode: 0o644 });
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
const linkPath = path.join(dir, "rogue-link");
fs.symlinkSync(outside, linkPath);

try {
readConfigFile(target, null);
expect(fs.statSync(outside).mode & 0o777).toBe(0o644);
} finally {
fs.unlinkSync(linkPath);
fs.unlinkSync(outside);
}
});

it("readConfigFile does not chmod through a symlink even via the per-file heal", () => {
// Defensive duplicate of the symlink check, this time for the per-file
// heal in readConfigFile itself (not the dir walk in ensureConfigDir).
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);

const outside = path.join(os.tmpdir(), `nemoclaw-symlink-readtarget-${String(process.pid)}`);
fs.writeFileSync(outside, JSON.stringify({ outside: true }), { mode: 0o644 });
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
const symlinkPath = path.join(dir, "config.json");
fs.symlinkSync(outside, symlinkPath);

try {
// Reading through the symlink should not chmod the target file.
readConfigFile(symlinkPath, null);
expect(fs.statSync(outside).mode & 0o777).toBe(0o644);
} finally {
fs.unlinkSync(symlinkPath);
fs.unlinkSync(outside);
}
});
Comment on lines +165 to +253

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.

Please add regression coverage for the scope boundary. The important guarantee is: root-level sibling healing happens under the host ~/.nemoclaw root, but not in arbitrary config directories that may have a different permission contract.

Suggested change
it("ensureConfigDir heals every root-level file in the dir, not just the one being read (#4546)", () => {
// #4546 expects auto-repair across all root-level files. Most of those
// files (onboard-session.json, ollama-proxy-token, etc.) are written by
// code paths that don't flow through readConfigFile, so the read-time
// per-file heal alone misses them. The dir walk in ensureConfigDir is
// what covers them — verify by writing several siblings at 644 and
// confirming a single read tightens all of them to 600.
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });
const siblings = [
"onboard-session.json",
"ollama-proxy-token",
"ollama-auth-proxy.pid",
"usage-notice.json",
];
for (const name of siblings) {
fs.writeFileSync(path.join(dir, name), "stale", { mode: 0o644 });
}
readConfigFile(target, null);
for (const name of siblings) {
const mode = fs.statSync(path.join(dir, name)).mode & 0o777;
expect(mode, `${name} should be tightened to 600`).toBe(0o600);
}
});
it("ensureConfigDir skips symlinks during the root-level heal", () => {
// A chmod on a symlink follows to the target — if ~/.nemoclaw/X is a
// symlink to /etc/passwd, healing must NOT chmod /etc/passwd. lstat
// before chmod keeps the heal scoped to real files inside the dir.
//
// Positive control: a regular sibling at 0o644 proves the walker
// actually ran (it should be tightened to 0o600). Without the
// control, this test would pass vacuously if the walker were a no-op.
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });
const sibling = path.join(dir, "should-be-healed.json");
fs.writeFileSync(sibling, "stale", { mode: 0o644 });
// Use mkdtempSync (via makeTempDir) for an unguessable outside path —
// a predictable os.tmpdir()+pid path is a CodeQL "insecure temporary
// file" pattern and lets a coresident attacker pre-create the target.
const outsideDir = makeTempDir();
const outside = path.join(outsideDir, "target");
fs.writeFileSync(outside, "outside", { mode: 0o644 });
const linkPath = path.join(dir, "rogue-link");
fs.symlinkSync(outside, linkPath);
readConfigFile(target, null);
expect(
fs.statSync(sibling).mode & 0o777,
"positive control: walker tightened the regular sibling",
).toBe(0o600);
expect(
fs.statSync(outside).mode & 0o777,
"symlink target must not be chmodded through the link",
).toBe(0o644);
// Cleanup of linkPath and outside happens via afterEach (both live
// inside dirs in tmpDirs).
});
it("readConfigFile does not chmod through a symlink even via the per-file heal", () => {
// Defensive duplicate of the symlink check, this time for the per-file
// heal in readConfigFile itself (not the dir walk in ensureConfigDir).
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const outsideDir = makeTempDir();
const outside = path.join(outsideDir, "target.json");
fs.writeFileSync(outside, JSON.stringify({ outside: true }), { mode: 0o644 });
const symlinkPath = path.join(dir, "config.json");
fs.symlinkSync(outside, symlinkPath);
// Reading through the symlink should not chmod the target file.
readConfigFile(symlinkPath, null);
expect(fs.statSync(outside).mode & 0o777).toBe(0o644);
// Cleanup via afterEach (both dirs are tracked in tmpDirs).
});
function withHome<T>(home: string, fn: () => T): T {
const previous = process.env.HOME;
process.env.HOME = home;
try {
return fn();
} finally {
if (previous === undefined) {
delete process.env.HOME;
} else {
process.env.HOME = previous;
}
}
}
it("ensureConfigDir heals every root-level file in the host state root, not just the one being read (#4546)", () => {
// #4546 expects auto-repair across all root-level files. Most of those
// files (onboard-session.json, ollama-proxy-token, etc.) are written by
// code paths that don't flow through readConfigFile, so the read-time
// per-file heal alone misses them. The dir walk in ensureConfigDir is
// what covers them - verify by writing several siblings at 644 and
// confirming a single read tightens all of them to 600.
const home = makeTempDir();
const dir = path.join(home, ".nemoclaw");
fs.mkdirSync(dir, { mode: 0o700 });
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });
const siblings = [
"onboard-session.json",
"ollama-proxy-token",
"ollama-auth-proxy.pid",
"usage-notice.json",
];
for (const name of siblings) {
fs.writeFileSync(path.join(dir, name), "stale", { mode: 0o644 });
}
withHome(home, () => readConfigFile(target, null));
for (const name of siblings) {
const mode = fs.statSync(path.join(dir, name)).mode & 0o777;
expect(mode, `${name} should be tightened to 600`).toBe(0o600);
}
});
it("does not heal sibling files outside the host state root", () => {
const home = makeTempDir();
const dir = path.join(home, "other-config");
fs.mkdirSync(dir, { mode: 0o700 });
const target = path.join(dir, "config.json");
const sibling = path.join(dir, "group-writable.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });
fs.writeFileSync(sibling, "keep-mutable", { mode: 0o660 });
fs.chmodSync(sibling, 0o660);
withHome(home, () => readConfigFile(target, null));
expect(fs.statSync(sibling).mode & 0o777).toBe(0o660);
});
it("ensureConfigDir skips symlinks during the root-level heal", () => {
// A chmod on a symlink follows to the target - if ~/.nemoclaw/X is a
// symlink to /etc/passwd, healing must NOT chmod /etc/passwd. lstat
// before chmod keeps the heal scoped to real files inside the dir.
//
// Positive control: a regular sibling at 0o644 proves the walker
// actually ran (it should be tightened to 0o600). Without the
// control, this test would pass vacuously if the walker were a no-op.
const home = makeTempDir();
const dir = path.join(home, ".nemoclaw");
fs.mkdirSync(dir, { mode: 0o700 });
const target = path.join(dir, "config.json");
fs.writeFileSync(target, JSON.stringify({ ok: true }), { mode: 0o600 });
const sibling = path.join(dir, "should-be-healed.json");
fs.writeFileSync(sibling, "stale", { mode: 0o644 });
const outsideDir = makeTempDir();
const outside = path.join(outsideDir, "target");
fs.writeFileSync(outside, "outside", { mode: 0o644 });
fs.chmodSync(outside, 0o644);
const linkPath = path.join(dir, "rogue-link");
fs.symlinkSync(outside, linkPath);
withHome(home, () => readConfigFile(target, null));
expect(
fs.statSync(sibling).mode & 0o777,
"positive control: walker tightened the regular sibling",
).toBe(0o600);
expect(
fs.statSync(outside).mode & 0o777,
"symlink target must not be chmodded through the link",
).toBe(0o644);
});
it("readConfigFile does not chmod through a symlink even via the per-file heal", () => {
// Defensive duplicate of the symlink check, this time for the per-file
// heal in readConfigFile itself (not the dir walk in ensureConfigDir).
const dir = makeTempDir();
fs.chmodSync(dir, 0o700);
const outsideDir = makeTempDir();
const outside = path.join(outsideDir, "target.json");
fs.writeFileSync(outside, JSON.stringify({ outside: true }), { mode: 0o644 });
fs.chmodSync(outside, 0o644);
const symlinkPath = path.join(dir, "config.json");
fs.symlinkSync(outside, symlinkPath);
// Reading through the symlink should not chmod the target file.
readConfigFile(symlinkPath, null);
expect(fs.statSync(outside).mode & 0o777).toBe(0o644);
});


it("supports both rich and legacy constructor forms", () => {
const rich = new ConfigPermissionError("test error", "/some/path");
expect(rich.name).toBe("ConfigPermissionError");
Expand Down
69 changes: 68 additions & 1 deletion src/lib/state/config-io.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,39 @@ export function rejectSymlinksOnPath(dirPath: string): void {
}
}

/**
* Tighten group/world bits on every regular file directly inside `dirPath`.
* Symlinks are skipped (we use `lstat`; a chmod on a symlink follows to the
* target, which would mutate something outside the config dir). Subdirectories
* are skipped — this is intentionally root-level only, matching the issue's
* acceptance criteria (#4546). Best-effort: a single file's chmod failure
* does not abort the walk.
*/
function healRootLevelFiles(dirPath: string): void {

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.

Scope the host-state permission contract explicitly before the helper. This gives us a cheap guardrail so the 700/600 healer does not become a generic permission normalizer for mutable sandbox config paths.

Suggested change
function healRootLevelFiles(dirPath: string): void {
function hostNemoclawDir(): string {
const home = process.env.HOME ?? os.homedir();
return path.resolve(home, ".nemoclaw");
}
function isHostNemoclawRoot(dirPath: string): boolean {
return path.resolve(dirPath) === hostNemoclawDir();
}
function isMutableSandboxConfigPath(targetPath: string): boolean {
const resolved = path.resolve(targetPath);
return resolved === "/sandbox/.openclaw" || resolved.startsWith("/sandbox/.openclaw/");
}
function healRootLevelFiles(dirPath: string): void {

let entries: fs.Dirent[];
try {
entries = fs.readdirSync(dirPath, { withFileTypes: true });
} catch {
// If we can't list (e.g. dir does not exist), nothing to heal.
return;
}
for (const entry of entries) {
if (entry.isSymbolicLink() || !entry.isFile()) continue;
const full = path.join(dirPath, entry.name);
try {
// lstat (not stat) so a TOCTOU-swapped symlink between readdir and
// chmod doesn't trick us into chmodding a target outside the dir.
const st = fs.lstatSync(full);
if (!st.isFile()) continue;
if ((st.mode & 0o077) !== 0) {
fs.chmodSync(full, 0o600);
}
} catch {
// Best effort — keep walking.
}
}
}

export function ensureConfigDir(dirPath: string): void {
// SECURITY: Block symlink attacks before creating or writing to the directory.
rejectSymlinksOnPath(dirPath);

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.

Track whether this call is touching the mutable OpenClaw sandbox tree. That path has its own permission contract and should not be normalized to host-state defaults.

Suggested change
rejectSymlinksOnPath(dirPath);
rejectSymlinksOnPath(dirPath);
const mutableSandboxPath = isMutableSandboxConfigPath(dirPath);

Expand Down Expand Up @@ -181,11 +214,45 @@ export function ensureConfigDir(dirPath: string): void {
}
throw error;
}

// Heal every root-level file in the dir, not just the one being read.
// Issue #4546 expects auto-repair across all root-level files
// (sandboxes.json, onboard-session.json, ollama-auth-proxy.pid,
// ollama-proxy-token, usage-notice.json, etc.) — most of which are written
// by code paths that don't flow through readConfigFile/writeConfigFile.
// Doing the walk here means every nemoclaw invocation that touches the
// config dir restores the entire root level to mode 600.
healRootLevelFiles(dirPath);

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.

Please scope the root-level sibling sweep to the exact host NemoClaw state root. This preserves #4546 while avoiding another #4538-style regression if a future caller routes a mutable sandbox config directory through ensureConfigDir(). When applying this, also update the directory-mode chmod above to skip mutableSandboxPath.

Suggested change
// Heal every root-level file in the dir, not just the one being read.
// Issue #4546 expects auto-repair across all root-level files
// (sandboxes.json, onboard-session.json, ollama-auth-proxy.pid,
// ollama-proxy-token, usage-notice.json, etc.) — most of which are written
// by code paths that don't flow through readConfigFile/writeConfigFile.
// Doing the walk here means every nemoclaw invocation that touches the
// config dir restores the entire root level to mode 600.
healRootLevelFiles(dirPath);
if (isHostNemoclawRoot(dirPath)) {
// Heal every root-level file in the host NemoClaw state dir, not just the
// one being read. Issue #4546 expects auto-repair across root-level files
// such as sandboxes.json, onboard-session.json, and ollama-proxy-token.
//
// Keep this scoped to ~/.nemoclaw. Mutable sandbox config paths such as
// /sandbox/.openclaw deliberately use group-writable permissions.
healRootLevelFiles(dirPath);
}

}

export function readConfigFile<T>(filePath: string, fallback: T): T {
try {
return parseJson<T>(fs.readFileSync(filePath, "utf-8"));
ensureConfigDir(path.dirname(filePath));
} catch (error) {
if (error instanceof ConfigPermissionError) {
throw error;
}
// Directory doesn't exist and can't be created — fall through to let
// readFileSync produce the appropriate ENOENT / fallback path.
}

try {
const content = parseJson<T>(fs.readFileSync(filePath, "utf-8"));

// Heal file-level permission drift: tighten group/world bits.
// lstat (not stat) so we don't chmod through a symlink to a target
// outside the config dir — defensive duplicate of healRootLevelFiles
// for read paths whose dirname differs from what ensureConfigDir saw.
try {
const st = fs.lstatSync(filePath);
if (st.isFile() && (st.mode & 0o077) !== 0) {

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.

Same boundary on the per-file heal: do not chmod through the generic read path when the file is a mutable sandbox OpenClaw config file.

Suggested change
if (st.isFile() && (st.mode & 0o077) !== 0) {
if (!isMutableSandboxConfigPath(filePath) && st.isFile() && (st.mode & 0o077) !== 0) {

fs.chmodSync(filePath, 0o600);
}
} catch {
// Best effort — don't fail the read if we can't heal permissions.
}

return content;
} catch (error) {
const errnoError = error instanceof Error ? error : null;
if (isPermissionError(errnoError)) {
Expand Down
Loading