fix(security): make MCP_UPLOAD_BASE_DIR mandatory for all upload tools - #420
Merged
gaurav-singh-9227 merged 5 commits intoSep 16, 2026
Merged
gaurav-singh-9227 merged 5 commits into
gaurav-singh-9227 merged 5 commits into
Conversation
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 <noreply@anthropic.com>
…e message Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
…oad types Address review feedback on the mandatory upload base-dir change: - README: add an "Enabling File & App Uploads" section explaining MCP_UPLOAD_BASE_DIR — what it is, an example env entry, and the four tools that require it (uploadProductRequirementFile, takeAppScreenshot, runAppTestsOnBrowserStack, runAppLiveSession). Without this the change looks like the upload tools "just broke" on a default install. - upload-validator: make `allowedBaseDir` a required key (value may be undefined) so every call site must consciously thread the configured base dir — a missing one becomes a compile error instead of a silent runtime always-refuse. - App/test path tool descriptions now note the file must live inside MCP_UPLOAD_BASE_DIR, so the model doesn't confidently pass paths that will be refused. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gaurav-singh-9227
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Directory containment in
validateUploadPathwas opt-in: the base-dir prefix check only ranif (options.allowedBaseDir), andMCP_UPLOAD_BASE_DIRis unset by default. With it unset, any caller-supplied absolute path to a non-hidden, allow-listed-extension file was accepted, streamed to BrowserStack, and returned as a signed download URL — an arbitrary file read / exfiltration on a default install (GHSA-j4xm-vw5v-c87v).A previous change added a refuse gate only to the upload-PRD tool (
upload-file.ts). The other three upload paths go through the same shared validator and remained exposed:takeAppScreenshot→uploadAppappautomate.ts:149uploadEspressoApp→uploadFileToBrowserStackappautomate.ts:183runAppLiveSession→ app uploadapplive-utils/upload-app.ts:20Fix
Make containment mandatory in
validateUploadPathitself: refuse the upload unlessallowedBaseDiris configured, then enforce that the canonicalised path lives inside it. Because all four upload tools share this validator, one change closes the gap uniformly (rather than three separate per-tool gates).App Automate app upload, Espresso app upload, and App Live sessions now require
MCP_UPLOAD_BASE_DIRto be set — the same requirement already in place for the upload-PRD tool. Uploads are restricted to that directory.Testing
npm run lint,tsc --noEmit, andvitest runpass. Added validator cases: refuses when no base dir is configured, refuses when the configured base dir doesn't exist, plus the existing containment/extension/hidden-path/size/symlink coverage.buildFailureThemes/tfaRcaCollaborationare present onmainand untouched.🤖 Generated with Claude Code