fix(security): close remaining path validation gaps (#77)

Harden the SSH agent-state and skill-removal paths to match the local security model, and avoid rejecting valid local workspace skill removals.

Made-with: Cursor

Co-authored-by: iamlukethedev <lucas.guilherme@smartwayslfl.com>

src/app/api/gateway/agent-state/route.ts

@@ -63,6 +63,9 @@ export async function POST(request: Request) {
       message.includes("agentId is required") ||
       message.includes("trashDir is required") ||
       message.includes("Invalid agentId") ||
+      message.includes("trashDir does not exist") ||
+      message.includes("trashDir is not under") ||
+      message.includes("Refusing to restore over existing path") ||
       message.includes("Gateway URL is missing") ||
       message.includes("Invalid gateway URL") ||
       message.includes("require OPENCLAW_GATEWAY_SSH_TARGET")

src/app/api/gateway/skills/remove/route.ts

@@ -76,6 +76,7 @@ export async function POST(request: Request) {
       message.includes("Unsupported skill source") ||
       message.includes("Refusing to remove") ||
       message.includes("not a directory") ||
+      message.includes("Remote workspace skill removal is not supported over SSH") ||
       message.includes("Gateway URL is missing") ||
       message.includes("Invalid gateway URL") ||
       message.includes("require OPENCLAW_GATEWAY_SSH_TARGET")

src/lib/skills/remove-local.ts

@@ -2,7 +2,7 @@ import fs from "node:fs";
 import os from "node:os";
 import path from "node:path";
 
-import { resolveStateDir, resolveUserPath } from "@/lib/clawdbot/paths";
+import { resolveUserPath } from "@/lib/clawdbot/paths";
 import type { RemovableSkillSource, SkillRemoveRequest, SkillRemoveResult } from "@/lib/skills/types";
 
 const resolveComparablePath = (input: string): string => {
@@ -57,18 +57,6 @@ export const removeSkillLocally = (params: SkillRemoveRequest): SkillRemoveResul
   const workspaceDir = normalizeRequiredPath(params.workspaceDir, "workspaceDir");
   const managedSkillsDir = normalizeRequiredPath(params.managedSkillsDir, "managedSkillsDir");
 
-  // Security: validate that workspaceDir and managedSkillsDir are under the
-  // server-side state directory. Without this check, an attacker can supply
-  // arbitrary values for these fields, effectively controlling the "allowed root"
-  // and bypassing the isPathInside containment check below.
-  const stateDir = resolveStateDir();
-  if (!isPathInside(stateDir, workspaceDir)) {
-    throw new Error(`workspaceDir is not under the state directory: ${workspaceDir}`);
-  }
-  if (!isPathInside(stateDir, managedSkillsDir)) {
-    throw new Error(`managedSkillsDir is not under the state directory: ${managedSkillsDir}`);
-  }
-
   const allowedRoot = resolveAllowedRoot({
     source,
     workspaceDir,

src/lib/ssh/agent-state.ts

@@ -74,6 +74,7 @@ if not trash_dir_raw:
   raise SystemExit("trashDir is required.")
 
 base = pathlib.Path.home() / ".openclaw"
+trash_root = base / "trash" / "studio-delete-agent"
 trash_dir = pathlib.Path(trash_dir_raw).expanduser()
 
 try:
@@ -81,9 +82,9 @@ try:
 except FileNotFoundError:
   raise SystemExit(f"trashDir does not exist: {trash_dir_raw}")
 
-resolved_base = base.resolve(strict=False)
-if resolved_base not in resolved_trash.parents:
-  raise SystemExit(f"trashDir is not under {base}: {trash_dir_raw}")
+resolved_trash_root = trash_root.resolve(strict=False)
+if resolved_trash != resolved_trash_root and resolved_trash_root not in resolved_trash.parents:
+  raise SystemExit(f"trashDir is not under {trash_root}: {trash_dir_raw}")
 
 moves = []
 

src/lib/ssh/skills-remove.ts

@@ -35,13 +35,13 @@ if source not in allowed_sources:
   raise SystemExit(f"Unsupported skill source for removal: {source}")
 
 base_dir = pathlib.Path(base_dir_raw).expanduser().resolve(strict=False)
-workspace_dir = pathlib.Path(workspace_dir_raw).expanduser().resolve(strict=False)
-managed_skills_dir = pathlib.Path(managed_skills_dir_raw).expanduser().resolve(strict=False)
+state_dir = (pathlib.Path.home() / ".openclaw").resolve(strict=False)
+managed_skills_root = (state_dir / "skills").resolve(strict=False)
 
 if source == "openclaw-managed":
-  allowed_root = managed_skills_dir
+  allowed_root = managed_skills_root
 else:
-  allowed_root = (workspace_dir / "skills").resolve(strict=False)
+  raise SystemExit("Remote workspace skill removal is not supported over SSH.")
 
 try:
   base_dir.relative_to(allowed_root)

tests/unit/agentStateExecutor.test.ts

@@ -55,5 +55,8 @@ describe("agent state ssh executor", () => {
         input: expect.stringContaining('python3 - "$1" "$2"'),
       })
     );
+    const call = mockedRunSshJson.mock.calls[0]?.[0];
+    expect(call?.input).toContain('trash_root = base / "trash" / "studio-delete-agent"');
+    expect(call?.input).toContain('trashDir is not under {trash_root}');
   });
 });

tests/unit/agentStateLocal.test.ts

@@ -39,5 +39,18 @@ describe("agent state local", () => {
     expect(fs.existsSync(agentDir)).toBe(true);
     expect(fs.readFileSync(path.join(workspace, "hello.txt"), "utf8")).toBe("hi");
   });
+
+  it("rejects restore paths outside the agent-state trash root", () => {
+    const stateDir = mkTmpStateDir();
+    process.env.OPENCLAW_STATE_DIR = stateDir;
+
+    const agentId = "test-agent";
+    const fakeTrashDir = path.join(stateDir, "agents", agentId);
+    fs.mkdirSync(fakeTrashDir, { recursive: true });
+
+    expect(() => restoreAgentStateLocally({ agentId, trashDir: fakeTrashDir })).toThrow(
+      "trashDir is not under"
+    );
+  });
 });
 

tests/unit/skillsRemoveExecutor.test.ts

@@ -54,5 +54,8 @@ describe("skills remove ssh executor", () => {
         input: expect.stringContaining('python3 - "$1" "$2" "$3" "$4" "$5"'),
       })
     );
+    const call = mockedRunSshJson.mock.calls[0]?.[0];
+    expect(call?.input).toContain('managed_skills_root = (state_dir / "skills").resolve(strict=False)');
+    expect(call?.input).toContain("Remote workspace skill removal is not supported over SSH.");
   });
 });

tests/unit/skillsRemoveLocal.test.ts

@@ -2,14 +2,23 @@ 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 { removeSkillLocally } from "@/lib/skills/remove-local";
 
 const mkTmpDir = () => fs.mkdtempSync(path.join(os.tmpdir(), "claw3d-skill-remove-"));
 
 describe("skills remove local", () => {
+  const originalStateDir = process.env.OPENCLAW_STATE_DIR;
+
+  afterEach(() => {
+    if (originalStateDir === undefined) delete process.env.OPENCLAW_STATE_DIR;
+    else process.env.OPENCLAW_STATE_DIR = originalStateDir;
+  });
+
   it("removes a workspace skill directory", () => {
+    process.env.OPENCLAW_STATE_DIR = mkTmpDir();
+
     const workspaceDir = mkTmpDir();
     const managedSkillsDir = mkTmpDir();
     const skillDir = path.join(workspaceDir, "skills", "github");

tests/unit/skillsRemoveRoute.test.ts

@@ -147,4 +147,31 @@ describe("skills remove route", () => {
     });
     expect(fs.existsSync(skillDir)).toBe(false);
   });
+
+  it("rejects remote workspace skill removal over ssh", async () => {
+    writeStudioSettings("ws://example.test:18789");
+
+    mockedSpawnSync.mockReturnValueOnce({
+      status: 1,
+      stdout: "",
+      stderr: "Remote workspace skill removal is not supported over SSH.",
+      error: undefined,
+    } as never);
+
+    const response = await POST(
+      new Request("http://localhost/api/gateway/skills/remove", {
+        method: "POST",
+        headers: { "content-type": "application/json" },
+        body: JSON.stringify({
+          skillKey: "github",
+          source: "openclaw-workspace",
+          baseDir: "/home/ubuntu/workspace-main/skills/github",
+          workspaceDir: "/home/ubuntu/workspace-main",
+          managedSkillsDir: "/home/ubuntu/.openclaw/skills",
+        }),
+      })
+    );
+
+    expect(response.status).toBe(400);
+  });
 });