From c578585195e456b9509bc54a51cdf34bc043abbb Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 10 Aug 2026 13:20:33 +0530 Subject: [PATCH] =?UTF-8?q?fix(lib):=20fail-closed=20upload=20containment?= =?UTF-8?q?=20=E2=80=94=20default=20UPLOAD=5FBASE=5FDIR=20to=20CWD?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security retest on PMAA-107 found containment was skipped entirely when MCP_UPLOAD_BASE_DIR was unset (the default), leaving arbitrary-path exfiltration open for allowlisted extensions. Containment now defaults to the process working directory at both layers: config.ts falls back to process.cwd(), and validateUploadPath() itself defaults a missing allowedBaseDir to process.cwd() so no caller can re-open the gap. Co-Authored-By: Claude Fable 5 --- src/config.ts | 4 +++- src/lib/upload-validator.ts | 36 ++++++++++++++-------------- tests/tools/upload-validator.test.ts | 28 +++++++++++++++++++++- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/src/config.ts b/src/config.ts index 18d3d853..efde5b0d 100644 --- a/src/config.ts +++ b/src/config.ts @@ -49,9 +49,11 @@ const config = new Config( browserstackLocalOptions, process.env.USE_OWN_LOCAL_BINARY_PROCESS === "true", process.env.REMOTE_MCP === "true", + // Fail-closed: uploads are contained to the working directory unless the + // user explicitly widens the boundary via MCP_UPLOAD_BASE_DIR (PMAA-107). process.env.MCP_UPLOAD_BASE_DIR && process.env.MCP_UPLOAD_BASE_DIR.length > 0 ? process.env.MCP_UPLOAD_BASE_DIR - : undefined, + : process.cwd(), ); export default config; diff --git a/src/lib/upload-validator.ts b/src/lib/upload-validator.ts index d516d129..dc21a87f 100644 --- a/src/lib/upload-validator.ts +++ b/src/lib/upload-validator.ts @@ -17,7 +17,8 @@ export interface UploadValidationOptions { * - File extension is in `allowedExtensions` (case-insensitive) * - No path segment is a hidden dir/file (starts with `.`); blocks ~/.ssh, * ~/.aws, .env, etc. even after symlink resolution - * - If `allowedBaseDir` is set, the canonical path must live inside it + * - The canonical path must live inside `allowedBaseDir` (defaults to the + * process working directory when not provided — containment is fail-closed) */ export function validateUploadPath( filePath: string, @@ -71,23 +72,22 @@ export function validateUploadPath( ); } - if (options.allowedBaseDir) { - let baseCanonical: string; - try { - baseCanonical = fs.realpathSync(path.resolve(options.allowedBaseDir)); - } catch { - throw new Error( - `Upload rejected: configured MCP_UPLOAD_BASE_DIR does not exist (${options.allowedBaseDir}).`, - ); - } - const baseWithSep = baseCanonical.endsWith(path.sep) - ? baseCanonical - : baseCanonical + path.sep; - if (canonical !== baseCanonical && !canonical.startsWith(baseWithSep)) { - throw new Error( - `Upload rejected: file must be located inside ${baseCanonical}.`, - ); - } + const allowedBaseDir = options.allowedBaseDir ?? process.cwd(); + let baseCanonical: string; + try { + baseCanonical = fs.realpathSync(path.resolve(allowedBaseDir)); + } catch { + throw new Error( + `Upload rejected: configured MCP_UPLOAD_BASE_DIR does not exist (${allowedBaseDir}).`, + ); + } + const baseWithSep = baseCanonical.endsWith(path.sep) + ? baseCanonical + : baseCanonical + path.sep; + if (canonical !== baseCanonical && !canonical.startsWith(baseWithSep)) { + throw new Error( + `Upload rejected: file must be located inside ${baseCanonical}. Set MCP_UPLOAD_BASE_DIR to allow uploads from a different directory.`, + ); } return canonical; diff --git a/tests/tools/upload-validator.test.ts b/tests/tools/upload-validator.test.ts index 51eed475..444aa4cd 100644 --- a/tests/tools/upload-validator.test.ts +++ b/tests/tools/upload-validator.test.ts @@ -1,7 +1,7 @@ import fs from "fs"; import os from "os"; import path from "path"; -import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { validateUploadPath, APP_BINARY_EXTENSIONS, @@ -32,10 +32,35 @@ describe("validateUploadPath", () => { const resolved = validateUploadPath(file, { allowedExtensions: APP_BINARY_EXTENSIONS, maxSizeBytes: MAX_APP_UPLOAD_BYTES, + allowedBaseDir: workDir, }); expect(resolved).toBe(fs.realpathSync(file)); }); + it("defaults containment to the process working directory (fail-closed)", () => { + const outside = write("app.apk"); + expect(() => + validateUploadPath(outside, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + }), + ).toThrow(/must be located inside/); + }); + + it("allows files inside the working directory when no base dir is set", () => { + const cwdSpy = vi.spyOn(process, "cwd").mockReturnValue(workDir); + try { + const file = write("app.apk"); + const resolved = validateUploadPath(file, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + }); + expect(resolved).toBe(fs.realpathSync(file)); + } finally { + cwdSpy.mockRestore(); + } + }); + it("rejects an empty path", () => { expect(() => validateUploadPath(" ", { @@ -185,6 +210,7 @@ describe("validateUploadPath", () => { const resolved = validateUploadPath(file, { allowedExtensions: APP_BINARY_EXTENSIONS, maxSizeBytes: MAX_APP_UPLOAD_BYTES, + allowedBaseDir: workDir, }); expect(resolved).toBe(fs.realpathSync(file)); });