From 23b3bc13a061c7fdf2b68505ce5101a030b38783 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Tue, 15 Sep 2026 18:48:51 +0530 Subject: [PATCH 1/2] fix(security): make MCP_UPLOAD_BASE_DIR mandatory for all upload tools MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Directory containment in validateUploadPath was opt-in: the base-dir prefix check only ran when allowedBaseDir was set, and MCP_UPLOAD_BASE_DIR is unset by default. With it unset, any caller-supplied absolute path to a non-hidden, allow-listed-extension file was accepted and streamed to BrowserStack — an arbitrary file read/exfiltration on a default install (GHSA-j4xm-vw5v-c87v). Only the upload-PRD tool (upload-file.ts) had its own refuse gate; the App Automate (uploadApp, uploadEspressoApp) and App Live (app upload) paths went through the same opt-in validator and remained exposed. Make containment mandatory in validateUploadPath itself: refuse the upload unless allowedBaseDir is configured, then enforce that the canonical path lives inside it. This covers every upload tool uniformly via the single shared validator. Behavior change: App Automate app upload, Espresso app upload, and App Live sessions now require MCP_UPLOAD_BASE_DIR to be set (same requirement already in place for the upload-PRD tool). Co-Authored-By: Claude Opus 4.8 --- src/lib/upload-validator.ts | 46 +++++++++++++++++----------- tests/tools/upload-validator.test.ts | 23 ++++++++++++++ 2 files changed, 51 insertions(+), 18 deletions(-) diff --git a/src/lib/upload-validator.ts b/src/lib/upload-validator.ts index d516d129..4114bd57 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 + * - `allowedBaseDir` is mandatory: uploads are refused unless it is configured + * (via MCP_UPLOAD_BASE_DIR), and the canonical path must live inside it */ export function validateUploadPath( filePath: string, @@ -71,23 +72,32 @@ 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}.`, - ); - } + // Directory containment is mandatory. Without a configured base dir there is + // nothing confining the (possibly absolute) path, so any readable file on the + // host could be streamed off it — refuse rather than fall back to "no check". + if (!options.allowedBaseDir) { + throw new Error( + "Upload rejected: file uploads are disabled because MCP_UPLOAD_BASE_DIR is not set. " + + "Set MCP_UPLOAD_BASE_DIR to a directory containing the files you want to upload, then " + + "restart the MCP server. Uploads are restricted to that directory.", + ); + } + + 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}.`, + ); } return canonical; diff --git a/tests/tools/upload-validator.test.ts b/tests/tools/upload-validator.test.ts index 51eed475..f423f194 100644 --- a/tests/tools/upload-validator.test.ts +++ b/tests/tools/upload-validator.test.ts @@ -32,10 +32,32 @@ describe("validateUploadPath", () => { const resolved = validateUploadPath(file, { allowedExtensions: APP_BINARY_EXTENSIONS, maxSizeBytes: MAX_APP_UPLOAD_BYTES, + allowedBaseDir: workDir, }); expect(resolved).toBe(fs.realpathSync(file)); }); + it("refuses when no base directory is configured (MCP_UPLOAD_BASE_DIR unset)", () => { + const file = write("app.apk"); + expect(() => + validateUploadPath(file, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + }), + ).toThrow(/MCP_UPLOAD_BASE_DIR is not set/); + }); + + it("refuses when the configured base directory does not exist", () => { + const file = write("app.apk"); + expect(() => + validateUploadPath(file, { + allowedExtensions: APP_BINARY_EXTENSIONS, + maxSizeBytes: MAX_APP_UPLOAD_BYTES, + allowedBaseDir: path.join(os.tmpdir(), "no-such-base-dir-xyz"), + }), + ).toThrow(/does not exist/); + }); + it("rejects an empty path", () => { expect(() => validateUploadPath(" ", { @@ -185,6 +207,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 82339575e788662ca043ef396ab1335c89b78800 Mon Sep 17 00:00:00 2001 From: Savio Dias Date: Wed, 16 Sep 2026 12:13:24 +0530 Subject: [PATCH 2/2] refactor(security): drop redundant comment and shorten the upload gate message Co-Authored-By: Claude Opus 4.8 --- src/lib/upload-validator.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/lib/upload-validator.ts b/src/lib/upload-validator.ts index 4114bd57..86fa0aa9 100644 --- a/src/lib/upload-validator.ts +++ b/src/lib/upload-validator.ts @@ -72,14 +72,10 @@ export function validateUploadPath( ); } - // Directory containment is mandatory. Without a configured base dir there is - // nothing confining the (possibly absolute) path, so any readable file on the - // host could be streamed off it — refuse rather than fall back to "no check". if (!options.allowedBaseDir) { throw new Error( - "Upload rejected: file uploads are disabled because MCP_UPLOAD_BASE_DIR is not set. " + - "Set MCP_UPLOAD_BASE_DIR to a directory containing the files you want to upload, then " + - "restart the MCP server. Uploads are restricted to that directory.", + "Upload rejected: MCP_UPLOAD_BASE_DIR is not set. Set it to the directory " + + "containing the files to upload, then restart the MCP server.", ); }