From 5010c65679eb96de8a769a10c0387ff46fd4a4b8 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 10 Aug 2026 13:20:33 +0530 Subject: [PATCH 1/3] =?UTF-8?q?fix(lib):=20fail-closed=20upload=20containm?= =?UTF-8?q?ent=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 Upload path containment was skipped entirely when MCP_UPLOAD_BASE_DIR was unset (the default), leaving arbitrary-path uploads 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..b8466e76 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. 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)); }); From b80824b3bb2194f206479bdbc05596ce92c5c39f Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 10 Aug 2026 14:45:28 +0530 Subject: [PATCH 2/3] =?UTF-8?q?fix(lib):=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20root-cwd=20guard,=20single=20default=20owner,=20doc?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject a defaulted base dir that resolves to the filesystem root: with cwd "/" (how some MCP clients launch stdio servers) every absolute path passes the startsWith containment check, silently allowing the whole filesystem. An explicitly configured root is honored as a deliberate opt-out. - Make the validator the single owner of the CWD default; config.ts passes MCP_UPLOAD_BASE_DIR through as undefined when unset so the validator can tell a defaulted base dir from a configured one. - Branch the realpath-failure error message: blame the working directory when nothing was configured, MCP_UPLOAD_BASE_DIR when it was. - Harden the default-containment test to mock process.cwd() instead of relying on tmpdir being outside the vitest cwd; add root-cwd rejection and explicit-root opt-out tests. - Document upload containment and MCP_UPLOAD_BASE_DIR in the README. Co-Authored-By: Claude Fable 5 --- README.md | 16 ++++++++++ src/config.ts | 8 +++-- src/lib/upload-validator.ts | 13 ++++++-- tests/tools/upload-validator.test.ts | 46 +++++++++++++++++++++++----- 4 files changed, 71 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index c34a8c47..0e95e0fa 100644 --- a/README.md +++ b/README.md @@ -290,6 +290,22 @@ Select the “Installed” tab. Click the “Configure MCP Servers” button at } ``` +### 📂 Upload directory containment + +File-upload tools (app uploads, Test Management attachments) only read files inside the directory the MCP server was launched from (usually your project root). This is a security default: it stops a misbehaving agent from uploading arbitrary files on your machine. + +To upload files from elsewhere (e.g. `~/Downloads`), set `MCP_UPLOAD_BASE_DIR` to the directory your files live in: + + ```json + "env": { + "BROWSERSTACK_USERNAME": "", + "BROWSERSTACK_ACCESS_KEY": "", + "MCP_UPLOAD_BASE_DIR": "/Users/you/Downloads" + } + ``` + +If the server is launched with the filesystem root as its working directory (some MCP clients do this), uploads are rejected until `MCP_UPLOAD_BASE_DIR` is set. + ### 💡 List of BrowserStack MCP Tools As of now we support 44 tools. diff --git a/src/config.ts b/src/config.ts index b8466e76..0e56a928 100644 --- a/src/config.ts +++ b/src/config.ts @@ -49,11 +49,13 @@ 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. + // Undefined when MCP_UPLOAD_BASE_DIR is unset; validateUploadPath() then + // falls back to the process working directory (fail-closed containment). + // The default lives in the validator — the enforcement point — so it can + // distinguish a defaulted base dir from a configured one. process.env.MCP_UPLOAD_BASE_DIR && process.env.MCP_UPLOAD_BASE_DIR.length > 0 ? process.env.MCP_UPLOAD_BASE_DIR - : process.cwd(), + : undefined, ); export default config; diff --git a/src/lib/upload-validator.ts b/src/lib/upload-validator.ts index dc21a87f..980d8afc 100644 --- a/src/lib/upload-validator.ts +++ b/src/lib/upload-validator.ts @@ -18,7 +18,8 @@ export interface UploadValidationOptions { * - No path segment is a hidden dir/file (starts with `.`); blocks ~/.ssh, * ~/.aws, .env, etc. even after symlink resolution * - The canonical path must live inside `allowedBaseDir` (defaults to the - * process working directory when not provided — containment is fail-closed) + * process working directory when not provided — containment is fail-closed; + * a defaulted base dir that is the filesystem root is rejected outright) */ export function validateUploadPath( filePath: string, @@ -72,13 +73,21 @@ export function validateUploadPath( ); } + const usingDefaultBaseDir = options.allowedBaseDir === undefined; 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}).`, + usingDefaultBaseDir + ? `Upload rejected: the process working directory could not be resolved (${allowedBaseDir}). Set MCP_UPLOAD_BASE_DIR to a valid directory.` + : `Upload rejected: configured MCP_UPLOAD_BASE_DIR does not exist (${allowedBaseDir}).`, + ); + } + if (usingDefaultBaseDir && path.parse(baseCanonical).root === baseCanonical) { + throw new Error( + "Upload rejected: the process working directory is the filesystem root, so upload containment cannot be enforced. Set MCP_UPLOAD_BASE_DIR to the directory uploads may come from.", ); } const baseWithSep = baseCanonical.endsWith(path.sep) diff --git a/tests/tools/upload-validator.test.ts b/tests/tools/upload-validator.test.ts index 444aa4cd..133e3d8f 100644 --- a/tests/tools/upload-validator.test.ts +++ b/tests/tools/upload-validator.test.ts @@ -38,13 +38,45 @@ describe("validateUploadPath", () => { }); 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/); + const otherDir = fs.mkdtempSync(path.join(os.tmpdir(), "other-cwd-")); + const cwdSpy = vi.spyOn(process, "cwd").mockReturnValue(otherDir); + try { + const outside = write("app.apk"); + expect(() => + validateUploadPath(outside, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + }), + ).toThrow(/must be located inside/); + } finally { + cwdSpy.mockRestore(); + fs.rmSync(otherDir, { recursive: true, force: true }); + } + }); + + it("rejects a defaulted base dir that is the filesystem root", () => { + const cwdSpy = vi.spyOn(process, "cwd").mockReturnValue(path.parse(workDir).root); + try { + const file = write("app.apk"); + expect(() => + validateUploadPath(file, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + }), + ).toThrow(/filesystem root/); + } finally { + cwdSpy.mockRestore(); + } + }); + + it("honors an explicitly configured filesystem-root base dir", () => { + const file = write("app.apk"); + const resolved = validateUploadPath(file, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + allowedBaseDir: path.parse(workDir).root, + }); + expect(resolved).toBe(fs.realpathSync(file)); }); it("allows files inside the working directory when no base dir is set", () => { From 6df2c1e76836ee9d6eb901856f010489cefdf3ec Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Mon, 10 Aug 2026 16:05:16 +0530 Subject: [PATCH 3/3] Update tests/tools/upload-validator.test.ts --- tests/tools/upload-validator.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/tools/upload-validator.test.ts b/tests/tools/upload-validator.test.ts index 133e3d8f..965a0ece 100644 --- a/tests/tools/upload-validator.test.ts +++ b/tests/tools/upload-validator.test.ts @@ -55,7 +55,9 @@ describe("validateUploadPath", () => { }); it("rejects a defaulted base dir that is the filesystem root", () => { - const cwdSpy = vi.spyOn(process, "cwd").mockReturnValue(path.parse(workDir).root); + const cwdSpy = vi + .spyOn(process, "cwd") + .mockReturnValue(path.parse(workDir).root); try { const file = write("app.apk"); expect(() =>