fix(lib): fail-closed upload containment — default UPLOAD_BASE_DIR to CWD

[SavioBS629]

Summary

Security retest on PMAA-107 found upload path 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:

• src/config.ts — UPLOAD_BASE_DIR falls back to process.cwd() when the env var is unset
• src/lib/upload-validator.ts — validateUploadPath() defaults a missing allowedBaseDir to process.cwd(), so no caller can re-open the gap

Behavior change

Uploads from outside the working directory now fail unless MCP_UPLOAD_BASE_DIR is set. The rejection message tells the user how to widen the boundary.

Not included

Extension-allowlist narrowing (.json/.txt/.csv) — pushed back on in the ticket, left as an open question.

Testing

npm run build green — lint, format, 275/275 tests (2 new: outside-CWD rejected by default, inside-CWD accepted), tsc.

🤖 Generated with Claude Code

[SavioBS629]

Claude Code Review (automated) — 4 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[SavioBS629]

[High] Fail-closed default is vacuous when cwd is /

With cwd /, baseCanonical is "/", baseWithSep is "/", and every absolute path passes startsWith("/") — containment silently allows the entire filesystem. Claude Desktop on macOS launches stdio MCP servers with cwd /, so the fail-closed guarantee fails in one of the most common deployments this fix targets.

Suggestion: after canonicalizing, if the base dir was defaulted (not explicitly configured) and equals the filesystem root (path.parse(baseCanonical).root === baseCanonical), reject with "working directory is the filesystem root; set MCP_UPLOAD_BASE_DIR to enable uploads". Add a cwd=/ test.

Reviewer: stack:code-review

[SavioBS629]

[Medium] Undocumented breaking default; MCP_UPLOAD_BASE_DIR documented nowhere

This flips the default from "no containment" to "cwd-only": users uploading from ~/Downloads while the server runs with a project cwd (Cursor/VS Code) will start getting rejections, and MCP_UPLOAD_BASE_DIR appears in no README or feature-flag doc.

Suggestion: document the env var (README env table + FEATURE-FLAGS doc) and call out the behavior change in the next release notes.

Reviewer: stack:code-review

[SavioBS629]

[Low] Misleading error when the base dir was defaulted

If the env var is unset and realpathSync fails (e.g. deleted cwd), this blames "configured MCP_UPLOAD_BASE_DIR" even though the user configured nothing.

Suggestion: branch the message on options.allowedBaseDir !== undefined ("configured MCP_UPLOAD_BASE_DIR does not exist" vs "working directory could not be resolved").

Reviewer: stack:code-review

[SavioBS629]

[Low] Test relies on tmpdir being outside vitest's cwd

This passes only because os.tmpdir() happens to be outside the repo cwd; on a runner with TMPDIR inside the workspace it fails spuriously, and with cwd / it passes vacuously.

Suggestion: mock process.cwd() to a sibling temp dir, as the companion test below already does.

Reviewer: stack:code-review

[SavioBS629]

Claude Code PR Review

PR: #366 • Head: c578585 • Reviewers: stack:code-review

Summary

Makes upload path containment fail-closed: UPLOAD_BASE_DIR defaults to the process working directory (in config.ts and as a fallback inside validateUploadPath()) instead of skipping containment entirely when MCP_UPLOAD_BASE_DIR is unset, closing the arbitrary-path upload gap for allowlisted extensions.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials introduced; error messages carry paths only
High	Security	Authentication/authorization checks present	N/A	No auth surface touched
High	Security	Input validation and sanitization	Fail	Containment is vacuous when cwd is / — the exact Claude Desktop deployment this fix targets (finding 1)
High	Security	No IDOR — resource ownership validated	N/A	No resource-ownership surface
High	Security	No SQL injection (parameterized queries)	N/A	No queries
High	Correctness	Logic is correct, handles edge cases	Fail	Filesystem-root base dir edge case defeats the guarantee (finding 1)
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Throws are explicit; call-site try/catch at all 3 tool handlers confirmed
High	Correctness	No race conditions or concurrency issues	Pass	realpath-before-containment ordering preserved; no shared mutable state (process.cwd() is process config, not user data — multi-tenant safe)
Medium	Testing	New code has corresponding tests	Pass	Both new behaviors covered (16/16 pass at head)
Medium	Testing	Error paths and edge cases tested	Pass	Rejection paths covered; cwd=/ edge case untested (falls under finding 1); finding 5 notes one fragile assumption
Medium	Testing	Existing tests still pass (no regressions)	Pass	Reviewer ran validator suite + adjacent upload suites (93 tests) at head — green
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Config-singleton pattern respected; see finding 4 for a drift risk
Medium	Quality	Changes are focused (single concern)	Pass	Single concern, 3 files
Low	Quality	Meaningful names, no dead code	Pass	Finding 4 notes the validator fallback is near-dead code by design — needs an intent comment
Low	Quality	Comments explain why, not what	Pass	New comment in config.ts explains the why
Low	Quality	No unnecessary dependencies added	Pass	None added

Findings

• File: src/lib/upload-validator.ts:75

• Severity: High

• Reviewer: stack:code-review

• Issue: Fail-closed default is vacuous when cwd is /. With baseCanonical = "/", baseWithSep = "/" and every absolute path passes startsWith("/") — containment silently allows the entire filesystem. Claude Desktop on macOS launches stdio MCP servers with cwd /, one of the most common deployments this fix targets, so the stated guarantee fails exactly where it matters most while signalling safety.

• Suggestion: After canonicalizing, if the base dir was defaulted (not explicitly configured) and equals the filesystem root (path.parse(baseCanonical).root === baseCanonical), reject with "working directory is the filesystem root; set MCP_UPLOAD_BASE_DIR to enable uploads" (or at minimum log a loud warning). Add a cwd=/ test.

• File: src/config.ts:56

• Severity: Medium

• Reviewer: stack:code-review

• Issue: Deliberate breaking default (no containment → cwd-only) with MCP_UPLOAD_BASE_DIR documented nowhere — no README entry, no FEATURE-FLAGS doc, and a fix(lib) commit type that will bury it in release notes. Users uploading from ~/Downloads with a project cwd (Cursor/VS Code) will start seeing rejections.

• Suggestion: Document MCP_UPLOAD_BASE_DIR in the README env table and FEATURE-FLAGS doc; call out the behavior change explicitly in the next release notes.

• File: src/lib/upload-validator.ts:81

• Severity: Low

• Reviewer: stack:code-review

• Issue: When the base dir was defaulted (env unset) and realpathSync fails (e.g. deleted cwd), the error blames "configured MCP_UPLOAD_BASE_DIR" even though the user configured nothing.

• Suggestion: Branch the message on options.allowedBaseDir !== undefined ("configured MCP_UPLOAD_BASE_DIR does not exist" vs "working directory could not be resolved").

• File: src/lib/upload-validator.ts:75

• Severity: Low

• Reviewer: stack:code-review

• Issue: The cwd default now lives in two places — config.ts snapshots process.cwd() at module load; the validator re-reads it at call time. All production call sites pass the config value, so the validator fallback only fires for direct library callers; two sources of truth that can diverge, and ambient process state in src/lib cuts against the repo's config-discipline rule.

• Suggestion: If the fallback is intentional defense-in-depth for library consumers (the remote wrapper imports this package), keep it and say so in a comment; otherwise make allowedBaseDir required and keep the default only in config.ts.

• File: tests/tools/upload-validator.test.ts:40

• Severity: Low

• Reviewer: stack:code-review

• Issue: The "defaults containment to the process working directory" test only passes because os.tmpdir() happens to be outside vitest's cwd. On a runner with TMPDIR inside the workspace it fails spuriously; with cwd / it passes vacuously.

• Suggestion: Mock process.cwd() to a sibling temp dir (as the companion test at line 51 already does) so the test controls both sides of the containment check.

---

Verdict: FAIL — the cwd=/ edge (finding 1) defeats the fail-closed guarantee in the primary deployment this fix targets; address it (plus the docs callout) before merge.

[SavioBS629]

Duplicate PR