Skip to content

Commit 4381ef9

Browse files
mbelinkymbelinky
committed
fix(browser): block upload symlink escapes openclaw#21972 thanks @mbelinky 
1 parent 774d73b commit 4381ef9

File tree

6 files changed

+157
-8
lines changed

6 files changed

+157
-8
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ Docs: https://docs.openclaw.ai
4949
- iOS/Security: force `https://` for non-loopback manual gateway hosts during iOS onboarding to block insecure remote transport URLs. (#21969) Thanks @mbelinky.
5050
- Shared/Security: reject insecure deep links that use `ws://` non-loopback gateway URLs to prevent plaintext remote websocket configuration. (#21970) Thanks @mbelinky.
5151
- macOS/Security: reject non-loopback `ws://` remote gateway URLs in macOS remote config to block insecure plaintext websocket endpoints. (#21971) Thanks @mbelinky.
52+
- Browser/Security: block upload path symlink escapes so browser upload sources cannot traverse outside the allowed workspace via symlinked paths. (#21972) Thanks @mbelinky.
5253
- CLI/Onboarding: fix Anthropic-compatible custom provider verification by normalizing base URLs to avoid duplicate `/v1` paths during setup checks. (#21336) Thanks @17jmumford.
5354
- Security/Dependencies: bump transitive `hono` usage to `4.11.10` to incorporate timing-safe authentication comparison hardening for `basicAuth`/`bearerAuth` (`GHSA-gq3j-xvxp-8hrf`). Thanks @vincentkoc.
5455

‎src/agents/tools/browser-tool.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ import {
2121
} from "../../browser/client.js";
2222
import { resolveBrowserConfig } from "../../browser/config.js";
2323
import { DEFAULT_AI_SNAPSHOT_MAX_CHARS } from "../../browser/constants.js";
24-
import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js";
24+
import { DEFAULT_UPLOAD_DIR, resolveExistingPathsWithinRoot } from "../../browser/paths.js";
2525
import { applyBrowserProxyPaths, persistBrowserProxyFiles } from "../../browser/proxy-files.js";
2626
import { loadConfig } from "../../config/config.js";
2727
import { wrapExternalContent } from "../../security/external-content.js";
@@ -700,7 +700,7 @@ export function createBrowserTool(opts?: {
700700
if (paths.length === 0) {
701701
throw new Error("paths required");
702702
}
703-
const uploadPathsResult = resolvePathsWithinRoot({
703+
const uploadPathsResult = await resolveExistingPathsWithinRoot({
704704
rootDir: DEFAULT_UPLOAD_DIR,
705705
requestedPaths: paths,
706706
scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`,

‎src/browser/paths.test.ts‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import fs from "node:fs/promises";
2+
import os from "node:os";
3+
import path from "node:path";
4+
import { afterEach, describe, expect, it } from "vitest";
5+
import { resolveExistingPathsWithinRoot } from "./paths.js";
6+
7+
async function createFixtureRoot(): Promise<{ baseDir: string; uploadsDir: string }> {
8+
const baseDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-browser-paths-"));
9+
const uploadsDir = path.join(baseDir, "uploads");
10+
await fs.mkdir(uploadsDir, { recursive: true });
11+
return { baseDir, uploadsDir };
12+
}
13+
14+
describe("resolveExistingPathsWithinRoot", () => {
15+
const cleanupDirs = new Set<string>();
16+
17+
afterEach(async () => {
18+
await Promise.all(
19+
Array.from(cleanupDirs).map(async (dir) => {
20+
await fs.rm(dir, { recursive: true, force: true });
21+
}),
22+
);
23+
cleanupDirs.clear();
24+
});
25+
26+
it("accepts existing files under the upload root", async () => {
27+
const { baseDir, uploadsDir } = await createFixtureRoot();
28+
cleanupDirs.add(baseDir);
29+
30+
const nestedDir = path.join(uploadsDir, "nested");
31+
await fs.mkdir(nestedDir, { recursive: true });
32+
const filePath = path.join(nestedDir, "ok.txt");
33+
await fs.writeFile(filePath, "ok", "utf8");
34+
35+
const result = await resolveExistingPathsWithinRoot({
36+
rootDir: uploadsDir,
37+
requestedPaths: [filePath],
38+
scopeLabel: "uploads directory",
39+
});
40+
41+
expect(result.ok).toBe(true);
42+
if (result.ok) {
43+
expect(result.paths).toEqual([await fs.realpath(filePath)]);
44+
}
45+
});
46+
47+
it("rejects traversal outside the upload root", async () => {
48+
const { baseDir, uploadsDir } = await createFixtureRoot();
49+
cleanupDirs.add(baseDir);
50+
51+
const outsidePath = path.join(baseDir, "outside.txt");
52+
await fs.writeFile(outsidePath, "nope", "utf8");
53+
54+
const result = await resolveExistingPathsWithinRoot({
55+
rootDir: uploadsDir,
56+
requestedPaths: ["../outside.txt"],
57+
scopeLabel: "uploads directory",
58+
});
59+
60+
expect(result.ok).toBe(false);
61+
if (!result.ok) {
62+
expect(result.error).toContain("must stay within uploads directory");
63+
}
64+
});
65+
66+
it("keeps lexical in-root paths when files do not exist yet", async () => {
67+
const { baseDir, uploadsDir } = await createFixtureRoot();
68+
cleanupDirs.add(baseDir);
69+
70+
const result = await resolveExistingPathsWithinRoot({
71+
rootDir: uploadsDir,
72+
requestedPaths: ["missing.txt"],
73+
scopeLabel: "uploads directory",
74+
});
75+
76+
expect(result.ok).toBe(true);
77+
if (result.ok) {
78+
expect(result.paths).toEqual([path.join(uploadsDir, "missing.txt")]);
79+
}
80+
});
81+
82+
it.runIf(process.platform !== "win32")(
83+
"rejects symlink escapes outside upload root",
84+
async () => {
85+
const { baseDir, uploadsDir } = await createFixtureRoot();
86+
cleanupDirs.add(baseDir);
87+
88+
const outsidePath = path.join(baseDir, "secret.txt");
89+
await fs.writeFile(outsidePath, "secret", "utf8");
90+
const symlinkPath = path.join(uploadsDir, "leak.txt");
91+
await fs.symlink(outsidePath, symlinkPath);
92+
93+
const result = await resolveExistingPathsWithinRoot({
94+
rootDir: uploadsDir,
95+
requestedPaths: ["leak.txt"],
96+
scopeLabel: "uploads directory",
97+
});
98+
99+
expect(result.ok).toBe(false);
100+
if (!result.ok) {
101+
expect(result.error).toContain("regular non-symlink file");
102+
}
103+
},
104+
);
105+
});

‎src/browser/paths.ts‎

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import path from "node:path";
2+
import { SafeOpenError, openFileWithinRoot } from "../infra/fs-safe.js";
23
import { resolvePreferredOpenClawTmpDir } from "../infra/tmp-openclaw-dir.js";
34

45
export const DEFAULT_BROWSER_TMP_DIR = resolvePreferredOpenClawTmpDir();
@@ -47,3 +48,45 @@ export function resolvePathsWithinRoot(params: {
4748
}
4849
return { ok: true, paths: resolvedPaths };
4950
}
51+
52+
export async function resolveExistingPathsWithinRoot(params: {
53+
rootDir: string;
54+
requestedPaths: string[];
55+
scopeLabel: string;
56+
}): Promise<{ ok: true; paths: string[] } | { ok: false; error: string }> {
57+
const resolvedPaths: string[] = [];
58+
for (const raw of params.requestedPaths) {
59+
const pathResult = resolvePathWithinRoot({
60+
rootDir: params.rootDir,
61+
requestedPath: raw,
62+
scopeLabel: params.scopeLabel,
63+
});
64+
if (!pathResult.ok) {
65+
return { ok: false, error: pathResult.error };
66+
}
67+
68+
const rootDir = path.resolve(params.rootDir);
69+
const relativePath = path.relative(rootDir, pathResult.path);
70+
let opened: Awaited<ReturnType<typeof openFileWithinRoot>> | undefined;
71+
try {
72+
opened = await openFileWithinRoot({
73+
rootDir,
74+
relativePath,
75+
});
76+
resolvedPaths.push(opened.realPath);
77+
} catch (err) {
78+
if (err instanceof SafeOpenError && err.code === "not-found") {
79+
// Preserve historical behavior for paths that do not exist yet.
80+
resolvedPaths.push(pathResult.path);
81+
continue;
82+
}
83+
return {
84+
ok: false,
85+
error: `Invalid path: must stay within ${params.scopeLabel} and be a regular non-symlink file`,
86+
};
87+
} finally {
88+
await opened?.handle.close().catch(() => {});
89+
}
90+
}
91+
return { ok: true, paths: resolvedPaths };
92+
}

‎src/browser/routes/agent.act.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {
1616
DEFAULT_DOWNLOAD_DIR,
1717
DEFAULT_UPLOAD_DIR,
1818
resolvePathWithinRoot,
19-
resolvePathsWithinRoot,
19+
resolveExistingPathsWithinRoot,
2020
} from "./path-output.js";
2121
import type { BrowserResponse, BrowserRouteRegistrar } from "./types.js";
2222
import { jsonError, toBoolean, toNumber, toStringArray, toStringOrEmpty } from "./utils.js";
@@ -382,7 +382,7 @@ export function registerBrowserAgentActRoutes(
382382
targetId,
383383
feature: "file chooser hook",
384384
run: async ({ cdpUrl, tab, pw }) => {
385-
const uploadPathsResult = resolvePathsWithinRoot({
385+
const uploadPathsResult = await resolveExistingPathsWithinRoot({
386386
rootDir: DEFAULT_UPLOAD_DIR,
387387
requestedPaths: paths,
388388
scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`,

‎src/cli/browser-cli-actions-input/register.files-downloads.ts‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,13 @@
11
import type { Command } from "commander";
2-
import { DEFAULT_UPLOAD_DIR, resolvePathsWithinRoot } from "../../browser/paths.js";
2+
import { DEFAULT_UPLOAD_DIR, resolveExistingPathsWithinRoot } from "../../browser/paths.js";
33
import { danger } from "../../globals.js";
44
import { defaultRuntime } from "../../runtime.js";
55
import { shortenHomePath } from "../../utils.js";
66
import { callBrowserRequest, type BrowserParentOpts } from "../browser-cli-shared.js";
77
import { resolveBrowserActionContext } from "./shared.js";
88

9-
function normalizeUploadPaths(paths: string[]): string[] {
10-
const result = resolvePathsWithinRoot({
9+
async function normalizeUploadPaths(paths: string[]): Promise<string[]> {
10+
const result = await resolveExistingPathsWithinRoot({
1111
rootDir: DEFAULT_UPLOAD_DIR,
1212
requestedPaths: paths,
1313
scopeLabel: `uploads directory (${DEFAULT_UPLOAD_DIR})`,
@@ -81,7 +81,7 @@ export function registerBrowserFilesAndDownloadsCommands(
8181
.action(async (paths: string[], opts, cmd) => {
8282
const { parent, profile } = resolveBrowserActionContext(cmd, parentOpts);
8383
try {
84-
const normalizedPaths = normalizeUploadPaths(paths);
84+
const normalizedPaths = await normalizeUploadPaths(paths);
8585
const { timeoutMs, targetId } = resolveTimeoutAndTarget(opts);
8686
const result = await callBrowserRequest<{ download: { path: string } }>(
8787
parent,

0 commit comments

Comments
 (0)