From c3556d2daa50af591a3a34873dc9e2b567d2ea80 Mon Sep 17 00:00:00 2001 From: Luke The Dev <252071647+iamlukethedev@users.noreply.github.com> Date: Fri, 27 Mar 2026 22:21:41 -0500 Subject: [PATCH] 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 --- src/app/api/gateway/agent-state/route.ts | 3 +++ src/app/api/gateway/skills/remove/route.ts | 1 + src/lib/skills/remove-local.ts | 14 +---------- src/lib/ssh/agent-state.ts | 7 +++--- src/lib/ssh/skills-remove.ts | 8 +++---- tests/unit/agentStateExecutor.test.ts | 3 +++ tests/unit/agentStateLocal.test.ts | 13 +++++++++++ tests/unit/skillsRemoveExecutor.test.ts | 3 +++ tests/unit/skillsRemoveLocal.test.ts | 11 ++++++++- tests/unit/skillsRemoveRoute.test.ts | 27 ++++++++++++++++++++++ 10 files changed, 69 insertions(+), 21 deletions(-) diff --git a/src/app/api/gateway/agent-state/route.ts b/src/app/api/gateway/agent-state/route.ts index 94c77af..93369cd 100644 --- a/src/app/api/gateway/agent-state/route.ts +++ b/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") diff --git a/src/app/api/gateway/skills/remove/route.ts b/src/app/api/gateway/skills/remove/route.ts index bcdd4dc..0a80704 100644 --- a/src/app/api/gateway/skills/remove/route.ts +++ b/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") diff --git a/src/lib/skills/remove-local.ts b/src/lib/skills/remove-local.ts index 4db2b32..8773c56 100644 --- a/src/lib/skills/remove-local.ts +++ b/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, diff --git a/src/lib/ssh/agent-state.ts b/src/lib/ssh/agent-state.ts index bc39f66..cd6eeb0 100644 --- a/src/lib/ssh/agent-state.ts +++ b/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 = [] diff --git a/src/lib/ssh/skills-remove.ts b/src/lib/ssh/skills-remove.ts index 97e423d..5ac54b2 100644 --- a/src/lib/ssh/skills-remove.ts +++ b/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) diff --git a/tests/unit/agentStateExecutor.test.ts b/tests/unit/agentStateExecutor.test.ts index 2cb860c..5d36af2 100644 --- a/tests/unit/agentStateExecutor.test.ts +++ b/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}'); }); }); diff --git a/tests/unit/agentStateLocal.test.ts b/tests/unit/agentStateLocal.test.ts index 0153b11..c036ebe 100644 --- a/tests/unit/agentStateLocal.test.ts +++ b/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" + ); + }); }); diff --git a/tests/unit/skillsRemoveExecutor.test.ts b/tests/unit/skillsRemoveExecutor.test.ts index f047bc0..d9d0b35 100644 --- a/tests/unit/skillsRemoveExecutor.test.ts +++ b/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."); }); }); diff --git a/tests/unit/skillsRemoveLocal.test.ts b/tests/unit/skillsRemoveLocal.test.ts index 7b98084..53ef4ef 100644 --- a/tests/unit/skillsRemoveLocal.test.ts +++ b/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"); diff --git a/tests/unit/skillsRemoveRoute.test.ts b/tests/unit/skillsRemoveRoute.test.ts index 03200c4..c474ee6 100644 --- a/tests/unit/skillsRemoveRoute.test.ts +++ b/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); + }); });