fix(security): confine include permission remediation (#103267)

* fix(security): guard include permission scans

* fix(security): avoid include scan variable shadow

* test(security): cover allowed include audit roots

* test(security): track include audit temp dirs

* fix(security): preserve include scan boundaries

---------

Co-authored-by: NianJiuZst <180004567+NianJiuZst@users.noreply.github.com>
Co-authored-by: Peter Steinberger <steipete@gmail.com>
(cherry picked from commit 5ed8121fd1)
This commit is contained in:
NianJiu
2026-07-11 00:14:08 +08:00
committed by Dallin Romney
parent 64843852e3
commit b3d2991b61
7 changed files with 385 additions and 71 deletions
+60 -35
View File
@@ -1,8 +1,15 @@
// Scans included config files and resolves include graphs.
import * as fs from "node:fs/promises";
import fs from "node:fs";
import path from "node:path";
import { parseJsonWithJson5Fallback } from "../utils/parse-json-compat.js";
import { INCLUDE_KEY, MAX_INCLUDE_DEPTH } from "./includes.js";
import {
createConfigIncludeResolutionSession,
INCLUDE_KEY,
MAX_INCLUDE_DEPTH,
readConfigIncludeFileWithGuards,
type IncludeResolver,
} from "./includes.js";
import { resolveIncludeRoots } from "./paths.js";
// Include discovery walks nested config objects because include blocks may be embedded.
function listDirectIncludes(parsed: unknown): string[] {
@@ -39,51 +46,69 @@ function listDirectIncludes(parsed: unknown): string[] {
return out;
}
function resolveIncludePath(baseConfigPath: string, includePath: string): string {
return path.normalize(
path.isAbsolute(includePath)
? includePath
: path.resolve(path.dirname(baseConfigPath), includePath),
);
}
/** Collects recursively referenced config include files without requiring a valid full config. */
export async function collectIncludePathsRecursive(params: {
configPath: string;
parsed: unknown;
env?: NodeJS.ProcessEnv;
allowedRoots?: readonly string[];
}): Promise<string[]> {
const visited = new Set<string>();
const result: string[] = [];
const includedPaths = new Set<string>();
// Canonical paths dedupe permission targets; lexical bases preserve relative
// include contexts and may need revisiting when first reached too deeply.
const walkedDepthByBase = new Map<string, number>();
const allowedRoots = params.allowedRoots ?? resolveIncludeRoots(params.env);
const resolveInclude = createConfigIncludeResolutionSession(params.configPath, allowedRoots);
const walk = async (basePath: string, parsed: unknown, depth: number): Promise<void> => {
if (depth > MAX_INCLUDE_DEPTH) {
const walk = (basePath: string, parsed: unknown, depth: number): void => {
if (depth >= MAX_INCLUDE_DEPTH) {
return;
}
for (const raw of listDirectIncludes(parsed)) {
const resolved = resolveIncludePath(basePath, raw);
if (visited.has(resolved)) {
continue;
}
visited.add(resolved);
result.push(resolved);
for (const includePath of listDirectIncludes(parsed)) {
let openedBasePath: string | undefined;
let nestedInclude: { basePath: string; parsed: unknown } | undefined;
const resolver: IncludeResolver = {
readFile: (candidate) => fs.readFileSync(candidate, "utf-8"),
readFileWithGuards: (readParams) => {
return readConfigIncludeFileWithGuards({
...readParams,
onResolvedPath: (resolvedIncludePath) => {
includedPaths.add(resolvedIncludePath);
const lexicalBasePath = path.normalize(readParams.resolvedPath);
const nextDepth = depth + 1;
const walkedDepth = walkedDepthByBase.get(lexicalBasePath);
if (walkedDepth !== undefined && walkedDepth <= nextDepth) {
return;
}
walkedDepthByBase.set(lexicalBasePath, nextDepth);
openedBasePath = lexicalBasePath;
},
});
},
parseJson: (raw) => {
const nestedParsed = parseJsonWithJson5Fallback(raw);
if (openedBasePath) {
nestedInclude = { basePath: openedBasePath, parsed: nestedParsed };
}
// The scanner owns nested traversal so one malformed sibling cannot
// hide later guarded files. The production resolver still owns each
// path, root, symlink, file-type, hardlink, and byte-limit decision.
return {};
},
};
const rawText = await fs.readFile(resolved, "utf-8").catch(() => null);
if (!rawText) {
continue;
try {
resolveInclude({ [INCLUDE_KEY]: includePath }, basePath, resolver);
} catch {
// Invalid includes are reported by config validation. Permission repair
// only retains files that reached the production guarded-open boundary.
}
const nestedParsed = (() => {
try {
return parseJsonWithJson5Fallback(rawText);
} catch {
return null;
}
})();
if (nestedParsed) {
await walk(resolved, nestedParsed, depth + 1);
if (nestedInclude) {
walk(nestedInclude.basePath, nestedInclude.parsed, depth + 1);
}
}
};
await walk(params.configPath, params.parsed, 0);
return result;
walk(params.configPath, params.parsed, 0);
return [...includedPaths];
}
+146
View File
@@ -4,9 +4,11 @@ import fs from "node:fs/promises";
import path from "node:path";
import { describe, expect, it, vi } from "vitest";
import { withTempDir } from "../test-helpers/temp-dir.js";
import { collectIncludePathsRecursive } from "./includes-scan.js";
import {
CircularIncludeError,
ConfigIncludeError,
MAX_INCLUDE_DEPTH,
MAX_INCLUDE_FILE_BYTES,
MAX_INCLUDE_PATH_LENGTH,
deepMerge,
@@ -339,6 +341,150 @@ describe("resolveConfigIncludes", () => {
});
});
describe("collectIncludePathsRecursive", () => {
it.runIf(process.platform !== "win32")(
"only reports includes the production resolver can safely open",
async () => {
await withTempDir({ prefix: "openclaw-include-scan-" }, async (tempRoot) => {
const configDir = path.join(tempRoot, "config");
const safeIncludePath = path.join(configDir, "safe.json5");
const outsideIncludePath = path.join(tempRoot, "outside.json5");
const symlinkPath = path.join(configDir, "outside-link.json5");
await fs.mkdir(configDir, { recursive: true });
await fs.writeFile(safeIncludePath, "{ safe: true }\n", "utf-8");
await fs.writeFile(outsideIncludePath, "{ outside: true }\n", "utf-8");
await fs.symlink(outsideIncludePath, symlinkPath);
const includePaths = await collectIncludePathsRecursive({
configPath: path.join(configDir, "openclaw.json"),
parsed: {
$include: ["./safe.json5", "../outside.json5", "./outside-link.json5"],
},
});
expect(includePaths).toEqual([await fs.realpath(safeIncludePath)]);
});
},
);
it.runIf(process.platform !== "win32")(
"keeps the original root boundary after a parent symlink swap",
async () => {
await withTempDir({ prefix: "openclaw-include-root-swap-" }, async (tempRoot) => {
const trustedDir = path.join(tempRoot, "trusted");
const outsideDir = path.join(tempRoot, "outside");
const configLink = path.join(tempRoot, "config-link");
await fs.mkdir(trustedDir, { recursive: true });
await fs.mkdir(outsideDir, { recursive: true });
await fs.writeFile(
path.join(trustedDir, "outer.json5"),
'{ "$include": "./nested.json5" }\n',
"utf-8",
);
await fs.writeFile(path.join(trustedDir, "nested.json5"), "{ trusted: true }\n", "utf-8");
await fs.writeFile(path.join(outsideDir, "nested.json5"), "{ escaped: true }\n", "utf-8");
await fs.symlink(trustedDir, configLink);
let swapped = false;
const resolver: IncludeResolver = {
readFile: (filePath) => nodeFs.readFileSync(filePath, "utf-8"),
parseJson: (raw) => {
const parsed = JSON.parse(raw) as unknown;
if (!swapped) {
nodeFs.unlinkSync(configLink);
nodeFs.symlinkSync(outsideDir, configLink);
swapped = true;
}
return parsed;
},
};
expect(() =>
resolveConfigIncludes(
{ $include: "./outer.json5" },
path.join(configLink, "openclaw.json"),
resolver,
),
).toThrow(/resolves outside config directory/);
});
},
);
it("honors explicitly allowed include roots", async () => {
await withTempDir({ prefix: "openclaw-include-scan-roots-" }, async (tempRoot) => {
const configDir = path.join(tempRoot, "config");
const sharedDir = path.join(tempRoot, "shared");
const sharedIncludePath = path.join(sharedDir, "shared.json5");
await fs.mkdir(configDir, { recursive: true });
await fs.mkdir(sharedDir, { recursive: true });
await fs.writeFile(sharedIncludePath, "{ shared: true }\n", "utf-8");
const includePaths = await collectIncludePathsRecursive({
configPath: path.join(configDir, "openclaw.json"),
parsed: { $include: sharedIncludePath },
allowedRoots: [sharedDir],
});
expect(includePaths).toEqual([await fs.realpath(sharedIncludePath)]);
});
});
it("continues past rejected nested includes to later safe siblings", async () => {
await withTempDir({ prefix: "openclaw-include-scan-nested-" }, async (tempRoot) => {
const configDir = path.join(tempRoot, "config");
const nestedDir = path.join(configDir, "nested");
const outerIncludePath = path.join(nestedDir, "outer.json5");
const safeIncludePath = path.join(configDir, "safe.json5");
const escapedIncludePath = path.join(tempRoot, "escaped.json5");
await fs.mkdir(nestedDir, { recursive: true });
await fs.writeFile(
outerIncludePath,
'{ "$include": ["../../escaped.json5", "../safe.json5"] }\n',
"utf-8",
);
await fs.writeFile(safeIncludePath, "{ safe: true }\n", "utf-8");
await fs.writeFile(escapedIncludePath, "{ escaped: true }\n", "utf-8");
const includePaths = await collectIncludePathsRecursive({
configPath: path.join(configDir, "openclaw.json"),
parsed: { $include: "./nested/outer.json5" },
});
expect(includePaths).toEqual([
await fs.realpath(outerIncludePath),
await fs.realpath(safeIncludePath),
]);
});
});
it("revisits an include reached later at a shallower depth", async () => {
await withTempDir({ prefix: "openclaw-include-scan-depth-" }, async (tempRoot) => {
const configDir = path.join(tempRoot, "config");
const sharedIncludePath = path.join(configDir, "shared.json5");
const leafIncludePath = path.join(configDir, "leaf.json5");
await fs.mkdir(configDir, { recursive: true });
for (let index = 0; index < MAX_INCLUDE_DEPTH - 1; index += 1) {
const nextInclude =
index === MAX_INCLUDE_DEPTH - 2 ? "./shared.json5" : `./chain-${index + 1}.json5`;
await fs.writeFile(
path.join(configDir, `chain-${index}.json5`),
`{ "$include": ${JSON.stringify(nextInclude)} }\n`,
"utf-8",
);
}
await fs.writeFile(sharedIncludePath, '{ "$include": "./leaf.json5" }\n', "utf-8");
await fs.writeFile(leafIncludePath, "{ leaf: true }\n", "utf-8");
const includePaths = await collectIncludePathsRecursive({
configPath: path.join(configDir, "openclaw.json"),
parsed: { $include: ["./chain-0.json5", "./shared.json5"] },
});
expect(includePaths).toContain(await fs.realpath(leafIncludePath));
});
});
});
describe("resolveConfigIncludeWritePath", () => {
it.runIf(process.platform !== "win32")(
"canonicalizes missing targets through symlinks into allowed roots",
+61 -33
View File
@@ -15,10 +15,10 @@ import fs from "node:fs";
import path from "node:path";
import { canUseRootFileOpen, openRootFileSync } from "../infra/boundary-file-read.js";
import { resolvePathViaExistingAncestorSync } from "../infra/boundary-path.js";
import { isBlockedObjectKey } from "../infra/prototype-keys.js";
import { isPathInside } from "../security/scan-paths.js";
import { isPlainObject } from "../utils.js";
import { parseJsonWithJson5Fallback } from "../utils/parse-json-compat.js";
import { isBlockedObjectKey } from "../infra/prototype-keys.js";
export const INCLUDE_KEY = "$include";
export const MAX_INCLUDE_DEPTH = 10;
@@ -85,6 +85,16 @@ type IncludeFileReadParams = {
onResolvedPath?: (resolvedPath: string) => void;
};
type IncludeRoot = {
rootDir: string;
rootRealDir: string;
};
type IncludeBoundary = {
readonly configRoot: IncludeRoot;
readonly allowedRoots: ReadonlyArray<IncludeRoot>;
};
type ResolveConfigIncludesOptions = {
/**
* Additional directories outside the config directory that `$include` paths
@@ -95,11 +105,6 @@ type ResolveConfigIncludesOptions = {
allowedRoots?: ReadonlyArray<string>;
};
type IncludeRoot = {
rootDir: string;
rootRealDir: string;
};
// ============================================================================
// Errors
// ============================================================================
@@ -151,26 +156,17 @@ export function deepMerge(target: unknown, source: unknown): unknown {
class IncludeProcessor {
private visited = new Set<string>();
private depth = 0;
private readonly configRoot: IncludeRoot;
private readonly allowedRoots: ReadonlyArray<IncludeRoot>;
constructor(
private basePath: string,
private resolver: IncludeResolver,
rootDir?: string,
allowedRoots?: ReadonlyArray<IncludeRoot>,
private readonly boundary: IncludeBoundary,
) {
this.visited.add(path.normalize(basePath));
const configRootDir = path.normalize(rootDir ?? path.dirname(basePath));
this.configRoot = {
rootDir: configRootDir,
rootRealDir: path.normalize(safeRealpath(configRootDir)),
};
this.allowedRoots = allowedRoots ?? [];
}
private get rootDir(): string {
return this.configRoot.rootDir;
return this.boundary.configRoot.rootDir;
}
process(obj: unknown): unknown {
@@ -326,10 +322,10 @@ class IncludeProcessor {
candidate: string,
field: "rootDir" | "rootRealDir",
): IncludeRoot | null {
if (isPathInside(this.configRoot[field], candidate)) {
return this.configRoot;
if (isPathInside(this.boundary.configRoot[field], candidate)) {
return this.boundary.configRoot;
}
for (const root of this.allowedRoots) {
for (const root of this.boundary.allowedRoots) {
if (isPathInside(root[field], candidate)) {
return root;
}
@@ -389,12 +385,7 @@ class IncludeProcessor {
}
private processNested(resolvedPath: string, parsed: unknown): unknown {
const nested = new IncludeProcessor(
resolvedPath,
this.resolver,
this.rootDir,
this.allowedRoots,
);
const nested = new IncludeProcessor(resolvedPath, this.resolver, this.boundary);
nested.visited = new Set([...this.visited, resolvedPath]);
nested.depth = this.depth + 1;
return nested.process(parsed);
@@ -409,6 +400,26 @@ function safeRealpath(target: string): string {
}
}
/** Capture the lexical and canonical include roots once for a resolver traversal. */
function createConfigIncludeBoundary(
configPath: string,
allowedRoots: ReadonlyArray<string> = [],
): IncludeBoundary {
const configRootDir = path.normalize(path.dirname(configPath));
return {
configRoot: {
rootDir: configRootDir,
rootRealDir: path.normalize(safeRealpath(configRootDir)),
},
allowedRoots: allowedRoots
.filter((entry) => typeof entry === "string" && entry.length > 0 && path.isAbsolute(entry))
.map((entry) => {
const rootDir = path.normalize(entry);
return { rootDir, rootRealDir: path.normalize(safeRealpath(rootDir)) };
}),
};
}
function isNotFoundError(error: unknown): boolean {
return Boolean(
error &&
@@ -474,6 +485,28 @@ const defaultResolver: IncludeResolver = {
parseJson: parseJsonWithJson5Fallback,
};
function resolveConfigIncludesWithinBoundary(
obj: unknown,
configPath: string,
resolver: IncludeResolver,
boundary: IncludeBoundary,
): unknown {
return new IncludeProcessor(configPath, resolver, boundary).process(obj);
}
/**
* Creates a resolver that shares one immutable root snapshot across independent
* include resolutions. Used when callers must isolate malformed sibling graphs.
*/
export function createConfigIncludeResolutionSession(
configPath: string,
allowedRoots: ReadonlyArray<string> = [],
): (obj: unknown, basePath: string, resolver?: IncludeResolver) => unknown {
const boundary = createConfigIncludeBoundary(configPath, allowedRoots);
return (obj, basePath, resolver = defaultResolver) =>
resolveConfigIncludesWithinBoundary(obj, basePath, resolver, boundary);
}
/**
* Resolves all $include directives in a parsed config object.
*/
@@ -483,11 +516,6 @@ export function resolveConfigIncludes(
resolver: IncludeResolver = defaultResolver,
options: ResolveConfigIncludesOptions = {},
): unknown {
const allowedRoots = (options.allowedRoots ?? [])
.filter((entry) => typeof entry === "string" && entry.length > 0 && path.isAbsolute(entry))
.map<IncludeRoot>((entry) => {
const rootDir = path.normalize(entry);
return { rootDir, rootRealDir: path.normalize(safeRealpath(rootDir)) };
});
return new IncludeProcessor(configPath, resolver, undefined, allowedRoots).process(obj);
const boundary = createConfigIncludeBoundary(configPath, options.allowedRoots ?? []);
return resolveConfigIncludesWithinBoundary(obj, configPath, resolver, boundary);
}
@@ -1,14 +1,16 @@
// Covers config include-file permission audit findings.
import fs from "node:fs";
import os from "node:os";
import path from "node:path";
import { describe, expect, it } from "vitest";
import { afterEach, describe, expect, it } from "vitest";
import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js";
import type { ConfigFileSnapshot } from "../config/types.openclaw.js";
import { collectIncludeFilePermFindings } from "./audit-extra.async.js";
describe("security audit config include permissions", () => {
const tempDirs = useAutoCleanupTempDirTracker(afterEach);
it("flags group/world-readable config include files", async () => {
const tmp = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-include-perms-"));
const tmp = tempDirs.make("openclaw-include-perms-");
const stateDir = path.join(tmp, "state");
fs.mkdirSync(stateDir, { recursive: true, mode: 0o700 });
@@ -44,4 +46,46 @@ describe("security audit config include permissions", () => {
}
expect(finding.severity).toBe("critical");
});
it.runIf(process.platform !== "win32")(
"audits include files under explicitly allowed roots",
async () => {
const tmp = tempDirs.make("openclaw-include-perms-allowed-");
const configDir = path.join(tmp, "config");
const sharedDir = path.join(tmp, "shared");
const sharedIncludePath = path.join(sharedDir, "shared.json5");
fs.mkdirSync(configDir, { recursive: true, mode: 0o700 });
fs.mkdirSync(sharedDir, { recursive: true, mode: 0o700 });
fs.writeFileSync(sharedIncludePath, "{}\n", "utf-8");
fs.chmodSync(sharedIncludePath, 0o644);
const configSnapshot: ConfigFileSnapshot = {
path: path.join(configDir, "openclaw.json"),
exists: true,
raw: `{ "$include": ${JSON.stringify(sharedIncludePath)} }\n`,
parsed: { $include: sharedIncludePath },
sourceConfig: {} as ConfigFileSnapshot["sourceConfig"],
resolved: {} as ConfigFileSnapshot["resolved"],
valid: true,
runtimeConfig: {} as ConfigFileSnapshot["runtimeConfig"],
config: {} as ConfigFileSnapshot["config"],
issues: [],
warnings: [],
legacyIssues: [],
};
const findings = await collectIncludeFilePermFindings({
configSnapshot,
env: { OPENCLAW_INCLUDE_ROOTS: sharedDir },
platform: "linux",
});
expect(findings).toEqual([
expect.objectContaining({
checkId: "fs.config_include.perms_world_readable",
detail: expect.stringContaining(sharedIncludePath),
}),
]);
},
);
});
+1
View File
@@ -579,6 +579,7 @@ export async function collectIncludeFilePermFindings(params: {
const includePaths = await collectIncludePathsRecursive({
configPath,
parsed: params.configSnapshot.parsed,
env: params.env,
});
if (includePaths.length === 0) {
return findings;
+69
View File
@@ -314,4 +314,73 @@ describe("security fix", () => {
{ path: transcriptPath, mode: 0o600, require: "file" },
]);
});
it.runIf(process.platform !== "win32")(
"tightens only includes accepted by the config include resolver",
async () => {
const stateDir = await createStateDir("include-boundary");
const configPath = path.join(stateDir, "openclaw.json");
const safeIncludePath = path.join(stateDir, "safe.json5");
const escapedIncludePath = path.join(fixtureRoot, "escaped.json5");
await fs.writeFile(safeIncludePath, "{}\n", "utf-8");
await fs.writeFile(escapedIncludePath, "{}\n", "utf-8");
await fs.chmod(safeIncludePath, 0o644);
await fs.chmod(escapedIncludePath, 0o644);
await fs.writeFile(
configPath,
'{ "$include": ["./safe.json5", "../escaped.json5"] }\n',
"utf-8",
);
const result = await fixSecurityFootguns({
env: createFixEnv(stateDir, configPath),
stateDir,
configPath,
});
expect(result.actions.some((action) => action.path === escapedIncludePath)).toBe(false);
expectPerms((await fs.stat(safeIncludePath)).mode & 0o777, 0o600);
expectPerms((await fs.stat(escapedIncludePath)).mode & 0o777, 0o644);
},
);
it.runIf(process.platform !== "win32")(
"keeps explicitly allowed include roots in the permission target set",
async () => {
const stateDir = await createStateDir("include-allowed-root");
const configPath = path.join(stateDir, "openclaw.json");
const sharedDir = path.join(fixtureRoot, "shared-includes");
const sharedIncludePath = path.join(sharedDir, "shared.json5");
await fs.mkdir(sharedDir, { recursive: true });
await fs.writeFile(sharedIncludePath, "{}\n", "utf-8");
await fs.chmod(sharedIncludePath, 0o644);
const canonicalSharedIncludePath = await fs.realpath(sharedIncludePath);
await fs.writeFile(
configPath,
`{ "$include": ${JSON.stringify(sharedIncludePath)} }\n`,
"utf-8",
);
const result = await fixSecurityFootguns({
env: {
...createFixEnv(stateDir, configPath),
OPENCLAW_INCLUDE_ROOTS: sharedDir,
},
stateDir,
configPath,
});
expect(result.actions).toEqual(
expect.arrayContaining([
expect.objectContaining({
kind: "chmod",
ok: true,
path: canonicalSharedIncludePath,
mode: 0o600,
}),
]),
);
expectPerms((await fs.stat(sharedIncludePath)).mode & 0o777, 0o600);
},
);
});
+1
View File
@@ -449,6 +449,7 @@ export async function fixSecurityFootguns(opts?: {
includePaths = await collectIncludePathsRecursive({
configPath: snap.path,
parsed: snap.parsed,
env,
}).catch(() => []);
}