broker: refuse uploads from outside the staging directory

page.upload and a raw DOM.setFileInputFiles through cdp.call now have to name
files whose real path is under /tmp/pi-chrome-uploads (PI_CHROME_UPLOAD_DIR),
on the broker's host. Anyone holding the token could otherwise attach any file
the user can read to a page.

4 files changed, 94 insertions, 1 deletions

diff --git a/broker/broker.mjs b/broker/broker.mjs
index 4af8e9d..36fa302 100644
--- a/broker/broker.mjs
+++ b/broker/broker.mjs
@@ -27,6 +27,7 @@ import { execSync } from "node:child_process";
 import { createHash, timingSafeEqual } from "node:crypto";
 import { dirname, resolve } from "node:path";
 import { fileURLToPath } from "node:url";
+import { assertUploadsStaged } from "./upload-guard.mjs";
 
 const PKG_PATH = resolve(dirname(fileURLToPath(import.meta.url)), "..", "package.json");
 
@@ -236,6 +237,13 @@ async function handleCommand(request, response) {
 		sendJson(response, 400, { ok: false, error: "Missing command action" });
 		return;
 	}
+	try {
+		assertUploadsStaged(body.action, body.params, process.env.PI_CHROME_UPLOAD_DIR?.trim() || undefined);
+	} catch (error) {
+		log("warn", "command", { action: body.action, peer: request.socket.remoteAddress, ok: false, refused: "upload outside the staging directory" });
+		sendJson(response, 403, { ok: false, error: error.message });
+		return;
+	}
 	const timeoutMs = Math.min(Math.max(Number(body.timeoutMs) || DEFAULT_TIMEOUT_MS, 1_000), MAX_TIMEOUT_MS);
 	const started = Date.now();
 	const peer = request.socket.remoteAddress;
diff --git a/broker/upload-guard.mjs b/broker/upload-guard.mjs
new file mode 100644
index 0000000..dde8cdf
--- /dev/null
+++ b/broker/upload-guard.mjs
@@ -0,0 +1,44 @@
+// Which local files a command may hand to the browser's file inputs.
+//
+// The broker runs on the browser's host, so a path in an upload command names a file on that
+// machine, and nothing else checks it: the companion extension passes it to
+// DOM.setFileInputFiles as given. Anyone holding the broker's token could attach any file the
+// user can read to a page. Clients are told to stage uploads in one directory; this enforces it.
+//
+// Covers chrome_upload_file (page.upload) and the same CDP method through chrome_cdp (cdp.call).
+// Paths are resolved with realpath first, so a symlink or ".." cannot lead out of the directory.
+
+import { realpathSync } from "node:fs";
+import { sep } from "node:path";
+
+export const DEFAULT_UPLOAD_DIR = "/tmp/pi-chrome-uploads";
+
+function resolveReal(path) {
+	try {
+		return realpathSync(path);
+	} catch {
+		return undefined; // missing: nothing to upload, and not ours to guess at
+	}
+}
+
+function pathsOf(action, params) {
+	if (action === "page.upload") return Array.isArray(params?.paths) ? params.paths : [];
+	if (action === "cdp.call" && params?.method === "DOM.setFileInputFiles") {
+		const files = params.params?.files;
+		return Array.isArray(files) ? files : [];
+	}
+	return [];
+}
+
+// Throws if a command would upload a file from outside uploadDir.
+export function assertUploadsStaged(action, params, uploadDir = DEFAULT_UPLOAD_DIR) {
+	const paths = pathsOf(action, params);
+	if (paths.length === 0) return;
+	const root = resolveReal(uploadDir);
+	for (const path of paths) {
+		const real = typeof path === "string" && path.startsWith("/") ? resolveReal(path) : undefined;
+		if (root === undefined || real === undefined || !real.startsWith(root + sep)) {
+			throw new Error(`Uploads are limited to files in ${uploadDir} on the browser's host; "${path}" is not one. Copy the file there first.`);
+		}
+	}
+}
diff --git a/package.json b/package.json
index 1e7a9f5..97ac4ea 100644
--- a/package.json
+++ b/package.json
@@ -2,7 +2,7 @@
 	"name": "pi-chrome",
 	"version": "0.15.57",
 	"scripts": {
-		"test": "node test-suite/unit/csp-eval.test.mjs && node test-suite/unit/automation-target.test.mjs && node test-suite/unit/session-cleanup.test.mjs && node test-suite/unit/background-policy.test.mjs && node test-suite/unit/input-reliability.test.mjs && node test-suite/unit/chrome-command.test.mjs && node test-suite/unit/bridge-resilience.test.mjs && node test-suite/unit/cdp-passthrough.test.mjs && node test-suite/unit/type-evidence.test.mjs && node test-suite/unit/hidden-input.test.mjs && node test-suite/unit/console-capture.test.mjs && node test-suite/unit/prompt-section.test.mjs",
+		"test": "node test-suite/unit/csp-eval.test.mjs && node test-suite/unit/automation-target.test.mjs && node test-suite/unit/session-cleanup.test.mjs && node test-suite/unit/background-policy.test.mjs && node test-suite/unit/input-reliability.test.mjs && node test-suite/unit/chrome-command.test.mjs && node test-suite/unit/bridge-resilience.test.mjs && node test-suite/unit/cdp-passthrough.test.mjs && node test-suite/unit/type-evidence.test.mjs && node test-suite/unit/hidden-input.test.mjs && node test-suite/unit/console-capture.test.mjs && node test-suite/unit/prompt-section.test.mjs && node test-suite/unit/upload-guard.test.mjs",
 		"version": "node scripts/sync-manifest-version.js",
 		"prepublishOnly": "node scripts/sync-manifest-version.js"
 	},
diff --git a/test-suite/unit/upload-guard.test.mjs b/test-suite/unit/upload-guard.test.mjs
new file mode 100644
index 0000000..b2a0daa
--- /dev/null
+++ b/test-suite/unit/upload-guard.test.mjs
@@ -0,0 +1,41 @@
+import assert from "node:assert/strict";
+import fs from "node:fs";
+import os from "node:os";
+import path from "node:path";
+import { test } from "node:test";
+import { assertUploadsStaged } from "../../broker/upload-guard.mjs";
+
+const base = fs.realpathSync(fs.mkdtempSync(path.join(os.tmpdir(), "guard-")));
+const stage = path.join(base, "stage");
+fs.mkdirSync(stage);
+fs.writeFileSync(path.join(stage, "ok.txt"), "x");
+fs.writeFileSync(path.join(base, "secret.txt"), "s");
+fs.symlinkSync(path.join(base, "secret.txt"), path.join(stage, "link.txt"));
+
+const upload = (...paths) => () => assertUploadsStaged("page.upload", { paths }, stage);
+
+test("a file in the staging directory is allowed", () => {
+	assert.doesNotThrow(upload(path.join(stage, "ok.txt")));
+});
+test("a file elsewhere, a ../ escape, a symlink out, a relative path and a missing file are refused", () => {
+	assert.throws(upload(path.join(base, "secret.txt")), /limited to files/);
+	assert.throws(upload(path.join(stage, "..", "secret.txt")), /limited to files/);
+	assert.throws(upload(path.join(stage, "link.txt")), /limited to files/);
+	assert.throws(upload("ok.txt"), /limited to files/);
+	assert.throws(upload(path.join(stage, "nope.txt")), /limited to files/);
+});
+test("one bad path among good ones refuses the lot", () => {
+	assert.throws(upload(path.join(stage, "ok.txt"), "/etc/hosts"), /limited to files/);
+});
+test("a missing staging directory refuses everything", () => {
+	assert.throws(() => assertUploadsStaged("page.upload", { paths: [path.join(stage, "ok.txt")] }, path.join(base, "none")), /limited to files/);
+});
+test("raw DOM.setFileInputFiles through cdp.call is held to the same rule", () => {
+	const call = (files) => () => assertUploadsStaged("cdp.call", { method: "DOM.setFileInputFiles", params: { files } }, stage);
+	assert.doesNotThrow(call([path.join(stage, "ok.txt")]));
+	assert.throws(call(["/etc/hosts"]), /limited to files/);
+});
+test("other commands are untouched", () => {
+	assert.doesNotThrow(() => assertUploadsStaged("page.click", { paths: ["/etc/hosts"] }, stage));
+	assert.doesNotThrow(() => assertUploadsStaged("cdp.call", { method: "Runtime.evaluate", params: {} }, stage));
+});