mirror of
https://github.com/openclaw/openclaw.git
synced 2026-02-19 18:39:20 -05:00
fix: harden include confinement edge cases (#18652) (thanks @aether-ai-agent)
This commit is contained in:
@@ -69,6 +69,7 @@ Docs: https://docs.openclaw.ai
|
||||
- BlueBubbles: match outbound message-id fallback recovery by chat identifier as well as account context. Thanks @tyler6204.
|
||||
- BlueBubbles: include sender identifier in untrusted conversation metadata for conversation info payloads. Thanks @tyler6204.
|
||||
- Security/Exec: fix the OC-09 credential-theft path via environment-variable injection. (#18048) Thanks @aether-ai-agent.
|
||||
- Security/Config: confine `$include` resolution to the top-level config directory, harden traversal/symlink checks with cross-platform-safe path containment, and add doctor hints for invalid escaped include paths. (#18652) Thanks @aether-ai-agent.
|
||||
- Providers: improve error messaging for unconfigured local `ollama`/`vllm` providers. (#18183) Thanks @arosstale.
|
||||
- TTS: surface all provider errors instead of only the last error in aggregated failures. (#17964) Thanks @ikari-pl.
|
||||
- CLI/Doctor/Configure: skip gateway auth checks for loopback-only setups. (#18407) Thanks @sggolakiya.
|
||||
|
||||
@@ -2435,7 +2435,7 @@ Split config into multiple files:
|
||||
- Array of files: deep-merged in order (later overrides earlier).
|
||||
- Sibling keys: merged after includes (override included values).
|
||||
- Nested includes: up to 10 levels deep.
|
||||
- Paths: relative (to the including file), absolute, or `../` parent references.
|
||||
- Paths: resolved relative to the including file, but must stay inside the top-level config directory (`dirname` of the main config file). Absolute/`../` forms are allowed only when they still resolve inside that boundary.
|
||||
- Errors: clear messages for missing files, parse errors, and circular includes.
|
||||
|
||||
---
|
||||
|
||||
38
src/commands/doctor-config-flow.include-warning.test.ts
Normal file
38
src/commands/doctor-config-flow.include-warning.test.ts
Normal file
@@ -0,0 +1,38 @@
|
||||
import fs from "node:fs/promises";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { withTempHome } from "../../test/helpers/temp-home.js";
|
||||
|
||||
const { noteSpy } = vi.hoisted(() => ({
|
||||
noteSpy: vi.fn(),
|
||||
}));
|
||||
|
||||
vi.mock("../terminal/note.js", () => ({
|
||||
note: noteSpy,
|
||||
}));
|
||||
|
||||
import { loadAndMaybeMigrateDoctorConfig } from "./doctor-config-flow.js";
|
||||
|
||||
describe("doctor include warning", () => {
|
||||
it("surfaces include confinement hint for escaped include paths", async () => {
|
||||
await withTempHome(async (home) => {
|
||||
const configDir = path.join(home, ".openclaw");
|
||||
await fs.mkdir(configDir, { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(configDir, "openclaw.json"),
|
||||
JSON.stringify({ $include: "/etc/passwd" }, null, 2),
|
||||
"utf-8",
|
||||
);
|
||||
|
||||
await loadAndMaybeMigrateDoctorConfig({
|
||||
options: { nonInteractive: true },
|
||||
confirm: async () => false,
|
||||
});
|
||||
});
|
||||
|
||||
expect(noteSpy).toHaveBeenCalledWith(
|
||||
expect.stringContaining("$include paths must stay under:"),
|
||||
"Doctor warnings",
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -147,6 +147,30 @@ function noteOpencodeProviderOverrides(cfg: OpenClawConfig) {
|
||||
note(lines.join("\n"), "OpenCode Zen");
|
||||
}
|
||||
|
||||
function noteIncludeConfinementWarning(snapshot: {
|
||||
path?: string | null;
|
||||
issues?: Array<{ message: string }>;
|
||||
}): void {
|
||||
const issues = snapshot.issues ?? [];
|
||||
const includeIssue = issues.find(
|
||||
(issue) =>
|
||||
issue.message.includes("Include path escapes config directory") ||
|
||||
issue.message.includes("Include path resolves outside config directory"),
|
||||
);
|
||||
if (!includeIssue) {
|
||||
return;
|
||||
}
|
||||
const configRoot = path.dirname(snapshot.path ?? CONFIG_PATH);
|
||||
note(
|
||||
[
|
||||
`- $include paths must stay under: ${configRoot}`,
|
||||
'- Move shared include files under that directory and update to relative paths like "./shared/common.json".',
|
||||
`- Error: ${includeIssue.message}`,
|
||||
].join("\n"),
|
||||
"Doctor warnings",
|
||||
);
|
||||
}
|
||||
|
||||
type TelegramAllowFromUsernameHit = { path: string; entry: string };
|
||||
|
||||
type TelegramAllowFromListRef = {
|
||||
@@ -758,6 +782,7 @@ export async function loadAndMaybeMigrateDoctorConfig(params: {
|
||||
const fixHints: string[] = [];
|
||||
if (snapshot.exists && !snapshot.valid && snapshot.legacyIssues.length === 0) {
|
||||
note("Config invalid; doctor will run with best-effort config.", "Config");
|
||||
noteIncludeConfinementWarning(snapshot);
|
||||
}
|
||||
const warnings = snapshot.warnings ?? [];
|
||||
if (warnings.length > 0) {
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import fs from "node:fs/promises";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import {
|
||||
@@ -287,6 +289,17 @@ describe("resolveConfigIncludes", () => {
|
||||
/escapes config directory/,
|
||||
);
|
||||
});
|
||||
|
||||
it("allows nested parent traversal when path stays under top-level config directory", () => {
|
||||
const files = {
|
||||
[configPath("sub", "child.json")]: { $include: "../shared/common.json" },
|
||||
[configPath("shared", "common.json")]: { shared: true },
|
||||
};
|
||||
const obj = { $include: "./sub/child.json" };
|
||||
expect(resolve(obj, files)).toEqual({
|
||||
shared: true,
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe("real-world config patterns", () => {
|
||||
@@ -520,12 +533,35 @@ describe("security: path traversal protection (CWE-22)", () => {
|
||||
expect(() => resolve(obj, {})).toThrow(ConfigIncludeError);
|
||||
});
|
||||
|
||||
it("handles config at filesystem root edge case", () => {
|
||||
it("allows child include when config is at filesystem root", () => {
|
||||
const rootConfigPath = path.join(path.parse(process.cwd()).root, "test.json");
|
||||
const files = { [rootConfigPath]: { root: true } };
|
||||
// Even at root, absolute paths to other locations should be rejected
|
||||
const obj = { $include: "/etc/passwd" };
|
||||
expect(() => resolve(obj, files, rootConfigPath)).toThrow(ConfigIncludeError);
|
||||
const childPath = path.join(path.parse(process.cwd()).root, "child.json");
|
||||
const files = { [childPath]: { root: true } };
|
||||
const obj = { $include: childPath };
|
||||
expect(resolve(obj, files, rootConfigPath)).toEqual({ root: true });
|
||||
});
|
||||
|
||||
it("allows include files when the config root path is a symlink", async () => {
|
||||
const tempRoot = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-includes-symlink-"));
|
||||
try {
|
||||
const realRoot = path.join(tempRoot, "real");
|
||||
const linkRoot = path.join(tempRoot, "link");
|
||||
await fs.mkdir(path.join(realRoot, "includes"), { recursive: true });
|
||||
await fs.writeFile(
|
||||
path.join(realRoot, "includes", "extra.json5"),
|
||||
"{ logging: { redactSensitive: 'tools' } }\n",
|
||||
"utf-8",
|
||||
);
|
||||
await fs.symlink(realRoot, linkRoot);
|
||||
|
||||
const result = resolveConfigIncludes(
|
||||
{ $include: "./includes/extra.json5" },
|
||||
path.join(linkRoot, "openclaw.json"),
|
||||
);
|
||||
expect(result).toEqual({ logging: { redactSensitive: "tools" } });
|
||||
} finally {
|
||||
await fs.rm(tempRoot, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -13,6 +13,7 @@
|
||||
import fs from "node:fs";
|
||||
import path from "node:path";
|
||||
import JSON5 from "json5";
|
||||
import { isPathInside } from "../security/scan-paths.js";
|
||||
import { isPlainObject } from "../utils.js";
|
||||
|
||||
export const INCLUDE_KEY = "$include";
|
||||
@@ -75,12 +76,17 @@ export function deepMerge(target: unknown, source: unknown): unknown {
|
||||
class IncludeProcessor {
|
||||
private visited = new Set<string>();
|
||||
private depth = 0;
|
||||
private readonly rootDir: string;
|
||||
private readonly rootRealDir: string;
|
||||
|
||||
constructor(
|
||||
private basePath: string,
|
||||
private resolver: IncludeResolver,
|
||||
rootDir?: string,
|
||||
) {
|
||||
this.visited.add(path.normalize(basePath));
|
||||
this.rootDir = path.normalize(rootDir ?? path.dirname(basePath));
|
||||
this.rootRealDir = path.normalize(safeRealpath(this.rootDir));
|
||||
}
|
||||
|
||||
process(obj: unknown): unknown {
|
||||
@@ -173,10 +179,10 @@ class IncludeProcessor {
|
||||
: path.resolve(configDir, includePath);
|
||||
const normalized = path.normalize(resolved);
|
||||
|
||||
// SECURITY: Reject paths outside config directory (CWE-22: Path Traversal)
|
||||
if (!normalized.startsWith(configDir + path.sep) && normalized !== configDir) {
|
||||
// SECURITY: Reject paths outside top-level config directory (CWE-22: Path Traversal)
|
||||
if (!isPathInside(this.rootDir, normalized)) {
|
||||
throw new ConfigIncludeError(
|
||||
`Include path escapes config directory: ${includePath}`,
|
||||
`Include path escapes config directory: ${includePath} (root: ${this.rootDir})`,
|
||||
includePath,
|
||||
);
|
||||
}
|
||||
@@ -184,9 +190,9 @@ class IncludeProcessor {
|
||||
// SECURITY: Resolve symlinks and re-validate to prevent symlink bypass
|
||||
try {
|
||||
const real = fs.realpathSync(normalized);
|
||||
if (!real.startsWith(configDir + path.sep) && real !== configDir) {
|
||||
if (!isPathInside(this.rootRealDir, real)) {
|
||||
throw new ConfigIncludeError(
|
||||
`Include path resolves outside config directory (symlink): ${includePath}`,
|
||||
`Include path resolves outside config directory (symlink): ${includePath} (root: ${this.rootDir})`,
|
||||
includePath,
|
||||
);
|
||||
}
|
||||
@@ -240,13 +246,21 @@ class IncludeProcessor {
|
||||
}
|
||||
|
||||
private processNested(resolvedPath: string, parsed: unknown): unknown {
|
||||
const nested = new IncludeProcessor(resolvedPath, this.resolver);
|
||||
const nested = new IncludeProcessor(resolvedPath, this.resolver, this.rootDir);
|
||||
nested.visited = new Set([...this.visited, resolvedPath]);
|
||||
nested.depth = this.depth + 1;
|
||||
return nested.process(parsed);
|
||||
}
|
||||
}
|
||||
|
||||
function safeRealpath(target: string): string {
|
||||
try {
|
||||
return fs.realpathSync(target);
|
||||
} catch {
|
||||
return target;
|
||||
}
|
||||
}
|
||||
|
||||
// ============================================================================
|
||||
// Public API
|
||||
// ============================================================================
|
||||
|
||||
Reference in New Issue
Block a user