refactor(security): unify path alias guard policies

src/agents/apply-patch.ts

@@ -2,6 +2,7 @@ import fs from "node:fs/promises";
 import path from "node:path";
 import type { AgentTool } from "@mariozechner/pi-agent-core";
 import { Type } from "@sinclair/typebox";
+import { PATH_ALIAS_POLICIES, type PathAliasPolicy } from "../infra/path-alias-guards.js";
 import { applyUpdateHunk } from "./apply-patch-update.js";
 import { assertSandboxPath, resolveSandboxInputPath } from "./sandbox-paths.js";
 import type { SandboxFsBridge } from "./sandbox/fs-bridge.js";
@@ -154,7 +155,7 @@ export async function applyPatch(
     }
 
     if (hunk.kind === "delete") {
-      const target = await resolvePatchPath(hunk.path, options, "unlink");
+      const target = await resolvePatchPath(hunk.path, options, PATH_ALIAS_POLICIES.unlinkTarget);
       await fileOps.remove(target.resolved);
       recordSummary(summary, seen, "deleted", target.display);
       continue;
@@ -253,7 +254,7 @@ async function ensureDir(filePath: string, ops: PatchFileOps) {
 async function resolvePatchPath(
   filePath: string,
   options: ApplyPatchOptions,
-  purpose: "readWrite" | "unlink" = "readWrite",
+  aliasPolicy: PathAliasPolicy = PATH_ALIAS_POLICIES.strict,
 ): Promise<{ resolved: string; display: string }> {
   if (options.sandbox) {
     const resolved = options.sandbox.bridge.resolvePath({
@@ -265,8 +266,8 @@ async function resolvePatchPath(
         filePath: resolved.hostPath,
         cwd: options.cwd,
         root: options.cwd,
-        allowFinalSymlink: purpose === "unlink",
-        allowFinalHardlink: purpose === "unlink",
+        allowFinalSymlinkForUnlink: aliasPolicy.allowFinalSymlinkForUnlink,
+        allowFinalHardlinkForUnlink: aliasPolicy.allowFinalHardlinkForUnlink,
       });
     }
     return {
@@ -282,8 +283,8 @@ async function resolvePatchPath(
           filePath,
           cwd: options.cwd,
           root: options.cwd,
-          allowFinalSymlink: purpose === "unlink",
-          allowFinalHardlink: purpose === "unlink",
+          allowFinalSymlinkForUnlink: aliasPolicy.allowFinalSymlinkForUnlink,
+          allowFinalHardlinkForUnlink: aliasPolicy.allowFinalHardlinkForUnlink,
         })
       ).resolved
     : resolvePathFromCwd(filePath, options.cwd);

src/agents/sandbox-paths.ts

@@ -1,9 +1,8 @@
-import fs from "node:fs/promises";
 import os from "node:os";
 import path from "node:path";
 import { fileURLToPath, URL } from "node:url";
-import { assertNoHardlinkedFinalPath } from "../infra/hardlink-guards.js";
-import { isNotFoundPathError, isPathInside } from "../infra/path-guards.js";
+import { assertNoPathAliasEscape, type PathAliasPolicy } from "../infra/path-alias-guards.js";
+import { isPathInside } from "../infra/path-guards.js";
 import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
 
 const UNICODE_SPACES = /[\u00A0\u2000-\u200A\u202F\u205F\u3000]/g;
@@ -62,18 +61,19 @@ export async function assertSandboxPath(params: {
   filePath: string;
   cwd: string;
   root: string;
-  allowFinalSymlink?: boolean;
-  allowFinalHardlink?: boolean;
+  allowFinalSymlinkForUnlink?: boolean;
+  allowFinalHardlinkForUnlink?: boolean;
 }) {
   const resolved = resolveSandboxPath(params);
-  await assertNoSymlinkEscape(resolved.relative, path.resolve(params.root), {
-    allowFinalSymlink: params.allowFinalSymlink,
-  });
-  await assertNoHardlinkedFinalPath({
-    filePath: resolved.resolved,
-    root: path.resolve(params.root),
+  const policy: PathAliasPolicy = {
+    allowFinalSymlinkForUnlink: params.allowFinalSymlinkForUnlink,
+    allowFinalHardlinkForUnlink: params.allowFinalHardlinkForUnlink,
+  };
+  await assertNoPathAliasEscape({
+    absolutePath: resolved.resolved,
+    rootPath: path.resolve(params.root),
     boundaryLabel: "sandbox root",
-    allowFinalHardlink: params.allowFinalHardlink,
+    policy,
   });
   return resolved;
 }
@@ -202,62 +202,13 @@ async function assertNoTmpAliasEscape(params: {
   filePath: string;
   tmpRoot: string;
 }): Promise<void> {
-  await assertNoSymlinkEscape(path.relative(params.tmpRoot, params.filePath), params.tmpRoot);
-  await assertNoHardlinkedFinalPath({
-    filePath: params.filePath,
-    root: params.tmpRoot,
+  await assertNoPathAliasEscape({
+    absolutePath: params.filePath,
+    rootPath: params.tmpRoot,
     boundaryLabel: "tmp root",
   });
 }
 
-async function assertNoSymlinkEscape(
-  relative: string,
-  root: string,
-  options?: { allowFinalSymlink?: boolean },
-) {
-  if (!relative) {
-    return;
-  }
-  const rootReal = await tryRealpath(root);
-  const parts = relative.split(path.sep).filter(Boolean);
-  let current = root;
-  for (let idx = 0; idx < parts.length; idx += 1) {
-    const part = parts[idx];
-    const isLast = idx === parts.length - 1;
-    current = path.join(current, part);
-    try {
-      const stat = await fs.lstat(current);
-      if (stat.isSymbolicLink()) {
-        // Unlinking a symlink itself is safe even if it points outside the root. What we
-        // must prevent is traversing through a symlink to reach targets outside root.
-        if (options?.allowFinalSymlink && isLast) {
-          return;
-        }
-        const target = await tryRealpath(current);
-        if (!isPathInside(rootReal, target)) {
-          throw new Error(
-            `Symlink escapes sandbox root (${shortPath(rootReal)}): ${shortPath(current)}`,
-          );
-        }
-        current = target;
-      }
-    } catch (err) {
-      if (isNotFoundPathError(err)) {
-        return;
-      }
-      throw err;
-    }
-  }
-}
-
-async function tryRealpath(value: string): Promise<string> {
-  try {
-    return await fs.realpath(value);
-  } catch {
-    return path.resolve(value);
-  }
-}
-
 function shortPath(value: string) {
   if (value.startsWith(os.homedir())) {
     return `~${value.slice(os.homedir().length)}`;

src/agents/sandbox/fs-bridge.ts

@@ -1,7 +1,8 @@
-import fs from "node:fs/promises";
-import path from "node:path";
-import { assertNoHardlinkedFinalPath } from "../../infra/hardlink-guards.js";
-import { isNotFoundPathError, isPathInside } from "../../infra/path-guards.js";
+import {
+  assertNoPathAliasEscape,
+  PATH_ALIAS_POLICIES,
+  type PathAliasPolicy,
+} from "../../infra/path-alias-guards.js";
 import { execDockerRaw, type ExecDockerRawResult } from "./docker.js";
 import {
   buildSandboxFsMounts,
@@ -21,8 +22,7 @@ type RunCommandOptions = {
 
 type PathSafetyOptions = {
   action: string;
-  allowFinalSymlink?: boolean;
-  allowFinalHardlink?: boolean;
+  aliasPolicy?: PathAliasPolicy;
   requireWritable?: boolean;
 };
 
@@ -152,8 +152,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
     await this.assertPathSafety(target, {
       action: "remove files",
       requireWritable: true,
-      allowFinalSymlink: true,
-      allowFinalHardlink: true,
+      aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget,
     });
     const flags = [params.force === false ? "" : "-f", params.recursive ? "-r" : ""].filter(
       Boolean,
@@ -178,8 +177,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
     await this.assertPathSafety(from, {
       action: "rename files",
       requireWritable: true,
-      allowFinalSymlink: true,
-      allowFinalHardlink: true,
+      aliasPolicy: PATH_ALIAS_POLICIES.unlinkTarget,
     });
     await this.assertPathSafety(to, {
       action: "rename files",
@@ -256,21 +254,16 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
       );
     }
 
-    await assertNoHostSymlinkEscape({
+    await assertNoPathAliasEscape({
       absolutePath: target.hostPath,
       rootPath: lexicalMount.hostRoot,
-      allowFinalSymlink: options.allowFinalSymlink === true,
-    });
-    await assertNoHardlinkedFinalPath({
-      filePath: target.hostPath,
-      root: lexicalMount.hostRoot,
       boundaryLabel: "sandbox mount root",
-      allowFinalHardlink: options.allowFinalHardlink === true,
+      policy: options.aliasPolicy,
     });
 
     const canonicalContainerPath = await this.resolveCanonicalContainerPath({
       containerPath: target.containerPath,
-      allowFinalSymlink: options.allowFinalSymlink === true,
+      allowFinalSymlinkForUnlink: options.aliasPolicy?.allowFinalSymlinkForUnlink === true,
     });
     const canonicalMount = this.resolveMountByContainerPath(canonicalContainerPath);
     if (!canonicalMount) {
@@ -297,7 +290,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
 
   private async resolveCanonicalContainerPath(params: {
     containerPath: string;
-    allowFinalSymlink: boolean;
+    allowFinalSymlinkForUnlink: boolean;
   }): Promise<string> {
     const script = [
       "set -eu",
@@ -318,7 +311,7 @@ class SandboxFsBridgeImpl implements SandboxFsBridge {
       'printf "%s%s\\n" "$canonical" "$suffix"',
     ].join("\n");
     const result = await this.runCommand(script, {
-      args: [params.containerPath, params.allowFinalSymlink ? "1" : "0"],
+      args: [params.containerPath, params.allowFinalSymlinkForUnlink ? "1" : "0"],
     });
     const canonical = result.stdout.toString("utf8").trim();
     if (!canonical.startsWith("/")) {
@@ -361,53 +354,3 @@ function coerceStatType(typeRaw?: string): "file" | "directory" | "other" {
   }
   return "other";
 }
-
-async function assertNoHostSymlinkEscape(params: {
-  absolutePath: string;
-  rootPath: string;
-  allowFinalSymlink: boolean;
-}): Promise<void> {
-  const root = path.resolve(params.rootPath);
-  const target = path.resolve(params.absolutePath);
-  if (!isPathInside(root, target)) {
-    throw new Error(`Sandbox path escapes mount root (${root}): ${params.absolutePath}`);
-  }
-  const relative = path.relative(root, target);
-  if (!relative) {
-    return;
-  }
-  const rootReal = await tryRealpath(root);
-  const parts = relative.split(path.sep).filter(Boolean);
-  let current = root;
-  for (let idx = 0; idx < parts.length; idx += 1) {
-    current = path.join(current, parts[idx] ?? "");
-    const isLast = idx === parts.length - 1;
-    try {
-      const stat = await fs.lstat(current);
-      if (!stat.isSymbolicLink()) {
-        continue;
-      }
-      if (params.allowFinalSymlink && isLast) {
-        return;
-      }
-      const symlinkTarget = await tryRealpath(current);
-      if (!isPathInside(rootReal, symlinkTarget)) {
-        throw new Error(`Symlink escapes sandbox mount root (${rootReal}): ${current}`);
-      }
-      current = symlinkTarget;
-    } catch (error) {
-      if (isNotFoundPathError(error)) {
-        return;
-      }
-      throw error;
-    }
-  }
-}
-
-async function tryRealpath(value: string): Promise<string> {
-  try {
-    return await fs.realpath(value);
-  } catch {
-    return path.resolve(value);
-  }
-}

src/infra/hardlink-guards.ts

@@ -6,9 +6,9 @@ export async function assertNoHardlinkedFinalPath(params: {
   filePath: string;
   root: string;
   boundaryLabel: string;
-  allowFinalHardlink?: boolean;
+  allowFinalHardlinkForUnlink?: boolean;
 }): Promise<void> {
-  if (params.allowFinalHardlink) {
+  if (params.allowFinalHardlinkForUnlink) {
     return;
   }
   let stat: Awaited<ReturnType<typeof fs.stat>>;

src/infra/path-alias-guards.ts

@@ -0,0 +1,89 @@
+import fs from "node:fs/promises";
+import os from "node:os";
+import path from "node:path";
+import { assertNoHardlinkedFinalPath } from "./hardlink-guards.js";
+import { isNotFoundPathError, isPathInside } from "./path-guards.js";
+
+export type PathAliasPolicy = {
+  allowFinalSymlinkForUnlink?: boolean;
+  allowFinalHardlinkForUnlink?: boolean;
+};
+
+export const PATH_ALIAS_POLICIES = {
+  strict: Object.freeze({
+    allowFinalSymlinkForUnlink: false,
+    allowFinalHardlinkForUnlink: false,
+  }),
+  unlinkTarget: Object.freeze({
+    allowFinalSymlinkForUnlink: true,
+    allowFinalHardlinkForUnlink: true,
+  }),
+} as const;
+
+export async function assertNoPathAliasEscape(params: {
+  absolutePath: string;
+  rootPath: string;
+  boundaryLabel: string;
+  policy?: PathAliasPolicy;
+}): Promise<void> {
+  const root = path.resolve(params.rootPath);
+  const target = path.resolve(params.absolutePath);
+  if (!isPathInside(root, target)) {
+    throw new Error(
+      `Path escapes ${params.boundaryLabel} (${shortPath(root)}): ${shortPath(params.absolutePath)}`,
+    );
+  }
+  const relative = path.relative(root, target);
+  if (relative) {
+    const rootReal = await tryRealpath(root);
+    const parts = relative.split(path.sep).filter(Boolean);
+    let current = root;
+    for (let idx = 0; idx < parts.length; idx += 1) {
+      current = path.join(current, parts[idx] ?? "");
+      const isLast = idx === parts.length - 1;
+      try {
+        const stat = await fs.lstat(current);
+        if (!stat.isSymbolicLink()) {
+          continue;
+        }
+        if (params.policy?.allowFinalSymlinkForUnlink && isLast) {
+          return;
+        }
+        const symlinkTarget = await tryRealpath(current);
+        if (!isPathInside(rootReal, symlinkTarget)) {
+          throw new Error(
+            `Symlink escapes ${params.boundaryLabel} (${shortPath(rootReal)}): ${shortPath(current)}`,
+          );
+        }
+        current = symlinkTarget;
+      } catch (error) {
+        if (isNotFoundPathError(error)) {
+          break;
+        }
+        throw error;
+      }
+    }
+  }
+
+  await assertNoHardlinkedFinalPath({
+    filePath: target,
+    root,
+    boundaryLabel: params.boundaryLabel,
+    allowFinalHardlinkForUnlink: params.policy?.allowFinalHardlinkForUnlink,
+  });
+}
+
+async function tryRealpath(value: string): Promise<string> {
+  try {
+    return await fs.realpath(value);
+  } catch {
+    return path.resolve(value);
+  }
+}
+
+function shortPath(value: string) {
+  if (value.startsWith(os.homedir())) {
+    return `~${value.slice(os.homedir().length)}`;
+  }
+  return value;
+}