diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d5bd81477d..521368ffd16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ Docs: https://docs.openclaw.ai - Security/macOS beta onboarding: remove Anthropic OAuth sign-in and the legacy `oauth.json` onboarding path that exposed the PKCE verifier via OAuth `state`; this impacted the macOS beta onboarding path only. Anthropic subscription auth is now setup-token-only and will ship in the next npm release (`2026.2.25`). Thanks @zdi-disclosures for reporting. - Security/Nextcloud Talk: drop replayed signed webhook events with persistent per-account replay dedupe across restarts, and reject unexpected webhook backend origins when account base URL is configured. Thanks @aristorechina for reporting. - Security/Gateway: harden `agents.files` path handling to block out-of-workspace symlink targets for `agents.files.get`/`agents.files.set`, keep in-workspace symlink targets supported, and add gateway regression coverage for both blocked escapes and allowed in-workspace symlinks. Thanks @tdjackey for reporting. +- Security/Browser temp paths: harden trace/download output-path handling against symlink-root and symlink-parent escapes with realpath-based write-path checks plus secure fallback tmp-dir validation that fails closed on unsafe fallback links. This ships in the next npm release (`2026.2.25`). Thanks @tdjackey for reporting. - Gateway/Message media roots: thread `agentId` through gateway `send` RPC and prefer explicit `agentId` over session/default resolution so non-default agent workspace media sends no longer fail with `LocalMediaAccessError`; added regression coverage for agent precedence and blank-agent fallback. (#23249) Thanks @Sid-Qin. - Cron/Model overrides: when isolated `payload.model` is no longer allowlisted, fall back to default model selection instead of failing the job, while still returning explicit errors for invalid model strings. (#26717) Thanks @Youyou972. - Security/Gateway auth: require pairing for operator device-identity sessions authenticated with shared token auth so unpaired devices cannot self-assign operator scopes. Thanks @tdjackey for reporting. diff --git a/src/browser/paths.test.ts b/src/browser/paths.test.ts index 1599c3895b2..0fe27fe1e4e 100644 --- a/src/browser/paths.test.ts +++ b/src/browser/paths.test.ts @@ -7,6 +7,7 @@ import { resolvePathsWithinRoot, resolvePathWithinRoot, resolveStrictExistingPathsWithinRoot, + resolveWritablePathWithinRoot, } from "./paths.js"; async function createFixtureRoot(): Promise<{ baseDir: string; uploadsDir: string }> { @@ -245,6 +246,45 @@ describe("resolvePathWithinRoot", () => { }); }); +describe("resolveWritablePathWithinRoot", () => { + it("accepts a writable path under root when parent is a real directory", async () => { + await withFixtureRoot(async ({ uploadsDir }) => { + const result = await resolveWritablePathWithinRoot({ + rootDir: uploadsDir, + requestedPath: "safe.txt", + scopeLabel: "uploads directory", + }); + expect(result).toEqual({ + ok: true, + path: path.resolve(uploadsDir, "safe.txt"), + }); + }); + }); + + it.runIf(process.platform !== "win32")( + "rejects write paths routed through a symlinked parent directory", + async () => { + await withFixtureRoot(async ({ baseDir, uploadsDir }) => { + const outsideDir = path.join(baseDir, "outside"); + await fs.mkdir(outsideDir, { recursive: true }); + const symlinkDir = path.join(uploadsDir, "escape-link"); + await fs.symlink(outsideDir, symlinkDir); + + const result = await resolveWritablePathWithinRoot({ + rootDir: uploadsDir, + requestedPath: "escape-link/pwned.txt", + scopeLabel: "uploads directory", + }); + + expect(result.ok).toBe(false); + if (!result.ok) { + expect(result.error).toContain("must stay within uploads directory"); + } + }); + }, + ); +}); + describe("resolvePathsWithinRoot", () => { it("resolves all valid in-root paths", () => { const result = resolvePathsWithinRoot({ diff --git a/src/browser/paths.ts b/src/browser/paths.ts index 88a541b75dc..34e927f8c5b 100644 --- a/src/browser/paths.ts +++ b/src/browser/paths.ts @@ -1,6 +1,7 @@ import fs from "node:fs/promises"; import path from "node:path"; import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js"; +import { isNotFoundPathError, isPathInside } from "../infra/path-guards.js"; import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js"; export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir(); @@ -30,6 +31,67 @@ export function resolvePathWithinRoot(params: { return { ok: true, path: resolved }; } +export async function resolveWritablePathWithinRoot(params: { + rootDir: string; + requestedPath: string; + scopeLabel: string; + defaultFileName?: string; +}): Promise<{ ok: true; path: string } | { ok: false; error: string }> { + const lexical = resolvePathWithinRoot(params); + if (!lexical.ok) { + return lexical; + } + + const invalid = (): { ok: false; error: string } => ({ + ok: false, + error: `Invalid path: must stay within ${params.scopeLabel}`, + }); + + const rootDir = path.resolve(params.rootDir); + let rootRealPath: string; + try { + const rootLstat = await fs.lstat(rootDir); + if (!rootLstat.isDirectory() || rootLstat.isSymbolicLink()) { + return invalid(); + } + rootRealPath = await fs.realpath(rootDir); + } catch { + return invalid(); + } + + const requestedPath = lexical.path; + const parentDir = path.dirname(requestedPath); + try { + const parentLstat = await fs.lstat(parentDir); + if (!parentLstat.isDirectory() || parentLstat.isSymbolicLink()) { + return invalid(); + } + const parentRealPath = await fs.realpath(parentDir); + if (!isPathInside(rootRealPath, parentRealPath)) { + return invalid(); + } + } catch { + return invalid(); + } + + try { + const targetLstat = await fs.lstat(requestedPath); + if (targetLstat.isSymbolicLink() || !targetLstat.isFile()) { + return invalid(); + } + const targetRealPath = await fs.realpath(requestedPath); + if (!isPathInside(rootRealPath, targetRealPath)) { + return invalid(); + } + } catch (err) { + if (!isNotFoundPathError(err)) { + return invalid(); + } + } + + return lexical; +} + export function resolvePathsWithinRoot(params: { rootDir: string; requestedPaths: string[]; diff --git a/src/browser/routes/agent.act.ts b/src/browser/routes/agent.act.ts index 78fa2f6856c..42ea8444f53 100644 --- a/src/browser/routes/agent.act.ts +++ b/src/browser/routes/agent.act.ts @@ -1,3 +1,4 @@ +import fs from "node:fs/promises"; import type { BrowserFormField } from "../client-actions-core.js"; import type { BrowserRouteContext } from "../server-context.js"; import { @@ -15,14 +16,17 @@ import { import { DEFAULT_DOWNLOAD_DIR, DEFAULT_UPLOAD_DIR, - resolvePathWithinRoot, + resolveWritablePathWithinRoot, resolveExistingPathsWithinRoot, } from "./path-output.js"; import type { BrowserResponse, BrowserRouteRegistrar } from "./types.js"; import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js"; -function resolveDownloadPathOrRespond(res: BrowserResponse, requestedPath: string): string | null { - const downloadPathResult = resolvePathWithinRoot({ +async function resolveDownloadPathOrRespond( + res: BrowserResponse, + requestedPath: string, +): Promise { + const downloadPathResult = await resolveWritablePathWithinRoot({ rootDir: DEFAULT_DOWNLOAD_DIR, requestedPath, scopeLabel: "downloads directory", @@ -466,9 +470,10 @@ export function registerBrowserAgentActRoutes( targetId, feature: "wait for download", run: async ({ cdpUrl, tab, pw }) => { + await fs.mkdir(DEFAULT_DOWNLOAD_DIR, { recursive: true }); let downloadPath: string | undefined; if (out.trim()) { - const resolvedDownloadPath = resolveDownloadPathOrRespond(res, out); + const resolvedDownloadPath = await resolveDownloadPathOrRespond(res, out); if (!resolvedDownloadPath) { return; } @@ -504,7 +509,8 @@ export function registerBrowserAgentActRoutes( targetId, feature: "download", run: async ({ cdpUrl, tab, pw }) => { - const downloadPath = resolveDownloadPathOrRespond(res, out); + await fs.mkdir(DEFAULT_DOWNLOAD_DIR, { recursive: true }); + const downloadPath = await resolveDownloadPathOrRespond(res, out); if (!downloadPath) { return; } diff --git a/src/browser/routes/agent.debug.ts b/src/browser/routes/agent.debug.ts index fab517d9589..b3b6ee0946c 100644 --- a/src/browser/routes/agent.debug.ts +++ b/src/browser/routes/agent.debug.ts @@ -8,7 +8,7 @@ import { resolveTargetIdFromQuery, withPlaywrightRouteContext, } from "./agent.shared.js"; -import { DEFAULT_TRACE_DIR, resolvePathWithinRoot } from "./path-output.js"; +import { DEFAULT_TRACE_DIR, resolveWritablePathWithinRoot } from "./path-output.js"; import type { BrowserRouteRegistrar } from "./types.js"; import { toBoolean, toStringOrEmpty } from "./utils.js"; @@ -122,7 +122,7 @@ export function registerBrowserAgentDebugRoutes( const id = crypto.randomUUID(); const dir = DEFAULT_TRACE_DIR; await fs.mkdir(dir, { recursive: true }); - const tracePathResult = resolvePathWithinRoot({ + const tracePathResult = await resolveWritablePathWithinRoot({ rootDir: dir, requestedPath: out, scopeLabel: "trace directory", diff --git a/src/browser/server.agent-contract-form-layout-act-commands.test.ts b/src/browser/server.agent-contract-form-layout-act-commands.test.ts index 0328736eade..e96193e5995 100644 --- a/src/browser/server.agent-contract-form-layout-act-commands.test.ts +++ b/src/browser/server.agent-contract-form-layout-act-commands.test.ts @@ -1,7 +1,9 @@ +import fs from "node:fs/promises"; +import os from "node:os"; import path from "node:path"; import { fetch as realFetch } from "undici"; import { describe, expect, it } from "vitest"; -import { DEFAULT_UPLOAD_DIR } from "./paths.js"; +import { DEFAULT_DOWNLOAD_DIR, DEFAULT_TRACE_DIR, DEFAULT_UPLOAD_DIR } from "./paths.js"; import { installAgentContractHooks, postJson, @@ -16,6 +18,23 @@ import { const state = getBrowserControlServerTestState(); const pwMocks = getPwMocks(); +async function withSymlinkPathEscape(params: { + rootDir: string; + run: (relativePath: string) => Promise; +}): Promise { + const outsideDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-route-escape-")); + const linkName = `escape-link-${Date.now()}-${Math.random().toString(16).slice(2)}`; + const linkPath = path.join(params.rootDir, linkName); + await fs.mkdir(params.rootDir, { recursive: true }); + await fs.symlink(outsideDir, linkPath); + try { + return await params.run(`${linkName}/pwned.zip`); + } finally { + await fs.unlink(linkPath).catch(() => {}); + await fs.rm(outsideDir, { recursive: true, force: true }).catch(() => {}); + } +} + describe("browser control server", () => { installAgentContractHooks(); @@ -268,6 +287,58 @@ describe("browser control server", () => { expect(pwMocks.downloadViaPlaywright).not.toHaveBeenCalled(); }); + it.runIf(process.platform !== "win32")( + "trace stop rejects symlinked write path escape under trace dir", + async () => { + const base = await startServerAndBase(); + await withSymlinkPathEscape({ + rootDir: DEFAULT_TRACE_DIR, + run: async (pathEscape) => { + const res = await postJson<{ error?: string }>(`${base}/trace/stop`, { + path: pathEscape, + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.traceStopViaPlaywright).not.toHaveBeenCalled(); + }, + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "wait/download rejects symlinked write path escape under downloads dir", + async () => { + const base = await startServerAndBase(); + await withSymlinkPathEscape({ + rootDir: DEFAULT_DOWNLOAD_DIR, + run: async (pathEscape) => { + const res = await postJson<{ error?: string }>(`${base}/wait/download`, { + path: pathEscape, + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.waitForDownloadViaPlaywright).not.toHaveBeenCalled(); + }, + }); + }, + ); + + it.runIf(process.platform !== "win32")( + "download rejects symlinked write path escape under downloads dir", + async () => { + const base = await startServerAndBase(); + await withSymlinkPathEscape({ + rootDir: DEFAULT_DOWNLOAD_DIR, + run: async (pathEscape) => { + const res = await postJson<{ error?: string }>(`${base}/download`, { + ref: "e12", + path: pathEscape, + }); + expect(res.error).toContain("Invalid path"); + expect(pwMocks.downloadViaPlaywright).not.toHaveBeenCalled(); + }, + }); + }, + ); + it("wait/download accepts in-root relative output path", async () => { const base = await startServerAndBase(); const res = await postJson<{ ok?: boolean; download?: { path?: string } }>( diff --git a/src/infra/tmp-openclaw-dir.test.ts b/src/infra/tmp-openclaw-dir.test.ts index 0424e5e0223..f3e3fe36299 100644 --- a/src/infra/tmp-openclaw-dir.test.ts +++ b/src/infra/tmp-openclaw-dir.test.ts @@ -8,24 +8,54 @@ function fallbackTmp(uid = 501) { return path.join("/var/fallback", `openclaw-${uid}`); } +function nodeErrorWithCode(code: string) { + const err = new Error(code) as Error & { code?: string }; + err.code = code; + return err; +} + +function secureDirStat(uid = 501) { + return { + isDirectory: () => true, + isSymbolicLink: () => false, + uid, + mode: 0o40700, + }; +} + function resolveWithMocks(params: { lstatSync: NonNullable; + fallbackLstatSync?: NonNullable; accessSync?: NonNullable; uid?: number; tmpdirPath?: string; }) { + const uid = params.uid ?? 501; + const fallbackPath = fallbackTmp(uid); const accessSync = params.accessSync ?? vi.fn(); + const wrappedLstatSync = vi.fn((target: string) => { + if (target === POSIX_OPENCLAW_TMP_DIR) { + return params.lstatSync(target); + } + if (target === fallbackPath) { + if (params.fallbackLstatSync) { + return params.fallbackLstatSync(target); + } + return secureDirStat(uid); + } + return secureDirStat(uid); + }) as NonNullable; const mkdirSync = vi.fn(); - const getuid = vi.fn(() => params.uid ?? 501); + const getuid = vi.fn(() => uid); const tmpdir = vi.fn(() => params.tmpdirPath ?? "/var/fallback"); const resolved = resolvePreferredOpenClawTmpDir({ accessSync, - lstatSync: params.lstatSync, + lstatSync: wrappedLstatSync, mkdirSync, getuid, tmpdir, }); - return { resolved, accessSync, lstatSync: params.lstatSync, mkdirSync, tmpdir }; + return { resolved, accessSync, lstatSync: wrappedLstatSync, mkdirSync, tmpdir }; } describe("resolvePreferredOpenClawTmpDir", () => { @@ -45,24 +75,12 @@ describe("resolvePreferredOpenClawTmpDir", () => { }); it("prefers /tmp/openclaw when it does not exist but /tmp is writable", () => { - const lstatSyncMock = vi.fn>(() => { - const err = new Error("missing") as Error & { code?: string }; - err.code = "ENOENT"; - throw err; - }); - - // second lstat call (after mkdir) should succeed - lstatSyncMock.mockImplementationOnce(() => { - const err = new Error("missing") as Error & { code?: string }; - err.code = "ENOENT"; - throw err; - }); - lstatSyncMock.mockImplementationOnce(() => ({ - isDirectory: () => true, - isSymbolicLink: () => false, - uid: 501, - mode: 0o40700, - })); + const lstatSyncMock = vi + .fn>() + .mockImplementationOnce(() => { + throw nodeErrorWithCode("ENOENT"); + }) + .mockImplementationOnce(() => secureDirStat(501)); const { resolved, accessSync, mkdirSync, tmpdir } = resolveWithMocks({ lstatSync: lstatSyncMock, @@ -84,7 +102,7 @@ describe("resolvePreferredOpenClawTmpDir", () => { const { resolved, tmpdir } = resolveWithMocks({ lstatSync }); expect(resolved).toBe(fallbackTmp()); - expect(tmpdir).toHaveBeenCalledTimes(1); + expect(tmpdir).toHaveBeenCalled(); }); it("falls back to os.tmpdir()/openclaw when /tmp is not writable", () => { @@ -94,9 +112,7 @@ describe("resolvePreferredOpenClawTmpDir", () => { } }); const lstatSync = vi.fn(() => { - const err = new Error("missing") as Error & { code?: string }; - err.code = "ENOENT"; - throw err; + throw nodeErrorWithCode("ENOENT"); }); const { resolved, tmpdir } = resolveWithMocks({ accessSync, @@ -104,7 +120,7 @@ describe("resolvePreferredOpenClawTmpDir", () => { }); expect(resolved).toBe(fallbackTmp()); - expect(tmpdir).toHaveBeenCalledTimes(1); + expect(tmpdir).toHaveBeenCalled(); }); it("falls back when /tmp/openclaw is a symlink", () => { @@ -118,7 +134,7 @@ describe("resolvePreferredOpenClawTmpDir", () => { const { resolved, tmpdir } = resolveWithMocks({ lstatSync }); expect(resolved).toBe(fallbackTmp()); - expect(tmpdir).toHaveBeenCalledTimes(1); + expect(tmpdir).toHaveBeenCalled(); }); it("falls back when /tmp/openclaw is not owned by the current user", () => { @@ -132,7 +148,7 @@ describe("resolvePreferredOpenClawTmpDir", () => { const { resolved, tmpdir } = resolveWithMocks({ lstatSync }); expect(resolved).toBe(fallbackTmp()); - expect(tmpdir).toHaveBeenCalledTimes(1); + expect(tmpdir).toHaveBeenCalled(); }); it("falls back when /tmp/openclaw is group/other writable", () => { @@ -145,6 +161,51 @@ describe("resolvePreferredOpenClawTmpDir", () => { const { resolved, tmpdir } = resolveWithMocks({ lstatSync }); expect(resolved).toBe(fallbackTmp()); - expect(tmpdir).toHaveBeenCalledTimes(1); + expect(tmpdir).toHaveBeenCalled(); + }); + + it("throws when fallback path is a symlink", () => { + const lstatSync = vi.fn(() => ({ + isDirectory: () => true, + isSymbolicLink: () => true, + uid: 501, + mode: 0o120777, + })); + const fallbackLstatSync = vi.fn(() => ({ + isDirectory: () => true, + isSymbolicLink: () => true, + uid: 501, + mode: 0o120777, + })); + + expect(() => + resolveWithMocks({ + lstatSync, + fallbackLstatSync, + }), + ).toThrow(/Unsafe fallback OpenClaw temp dir/); + }); + + it("creates fallback directory when missing, then validates ownership and mode", () => { + const lstatSync = vi.fn(() => ({ + isDirectory: () => true, + isSymbolicLink: () => true, + uid: 501, + mode: 0o120777, + })); + const fallbackLstatSync = vi + .fn>() + .mockImplementationOnce(() => { + throw nodeErrorWithCode("ENOENT"); + }) + .mockImplementationOnce(() => secureDirStat(501)); + + const { resolved, mkdirSync } = resolveWithMocks({ + lstatSync, + fallbackLstatSync, + }); + + expect(resolved).toBe(fallbackTmp()); + expect(mkdirSync).toHaveBeenCalledWith(fallbackTmp(), { recursive: true, mode: 0o700 }); }); }); diff --git a/src/infra/tmp-openclaw-dir.ts b/src/infra/tmp-openclaw-dir.ts index 1e8250b3210..2897d69e48a 100644 --- a/src/infra/tmp-openclaw-dir.ts +++ b/src/infra/tmp-openclaw-dir.ts @@ -95,12 +95,53 @@ export function resolvePreferredOpenClawTmpDir( } }; + const resolveFallbackState = ( + fallbackPath: string, + requireWritableAccess: boolean, + ): "available" | "missing" | "invalid" => { + try { + const candidate = lstatSync(fallbackPath); + if (!isTrustedPreferredDir(candidate)) { + return "invalid"; + } + if (requireWritableAccess) { + accessSync(fallbackPath, fs.constants.W_OK | fs.constants.X_OK); + } + return "available"; + } catch (err) { + if (isNodeErrorWithCode(err, "ENOENT")) { + return "missing"; + } + return "invalid"; + } + }; + + const ensureTrustedFallbackDir = (): string => { + const fallbackPath = fallback(); + const state = resolveFallbackState(fallbackPath, true); + if (state === "available") { + return fallbackPath; + } + if (state === "invalid") { + throw new Error(`Unsafe fallback OpenClaw temp dir: ${fallbackPath}`); + } + try { + mkdirSync(fallbackPath, { recursive: true, mode: 0o700 }); + } catch { + throw new Error(`Unable to create fallback OpenClaw temp dir: ${fallbackPath}`); + } + if (resolveFallbackState(fallbackPath, true) !== "available") { + throw new Error(`Unsafe fallback OpenClaw temp dir: ${fallbackPath}`); + } + return fallbackPath; + }; + const existingPreferredState = resolvePreferredState(true); if (existingPreferredState === "available") { return POSIX_OPENCLAW_TMP_DIR; } if (existingPreferredState === "invalid") { - return fallback(); + return ensureTrustedFallbackDir(); } try { @@ -108,10 +149,10 @@ export function resolvePreferredOpenClawTmpDir( // Create with a safe default; subsequent callers expect it exists. mkdirSync(POSIX_OPENCLAW_TMP_DIR, { recursive: true, mode: 0o700 }); if (resolvePreferredState(true) !== "available") { - return fallback(); + return ensureTrustedFallbackDir(); } return POSIX_OPENCLAW_TMP_DIR; } catch { - return fallback(); + return ensureTrustedFallbackDir(); } }