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

[SavioBS629]

Summary

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:

• 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.

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) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[SavioBS629]

Claude Code PR Review

PR: #367 • Head: b80824b • Reviewers: stack:code-review

Summary

Makes upload path containment fail-closed (defaulting to the process working directory when MCP_UPLOAD_BASE_DIR is unset, with the default owned by the validator), rejects a defaulted base dir at the filesystem root, differentiates error messages, hardens tests, and documents the new env var — superseding PR #366 and resolving all five findings from its review.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No credentials introduced; error text carries paths only
High	Security	Authentication/authorization checks present	N/A	No auth surface touched
High	Security	Input validation and sanitization	Pass	PR #366's High (containment vacuous at cwd /) verified fixed: defaulted root base dir rejected after realpath (POSIX / and Windows drive roots); explicit root honored as documented opt-in
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	Pass	usingDefaultBaseDir detection sound — all 3 call sites pass appConfig.UPLOAD_BASE_DIR (string | undefined); root guard runs after realpathSync so symlinked cwds can't dodge it
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Defaulted vs configured realpath failures now produce distinct, accurate messages
High	Correctness	No race conditions or concurrency issues	Pass	process.cwd() is process config, not user data — multi-tenant safe; remote wrapper checked, nothing breaks (fail-closed is an improvement there)
Medium	Testing	New code has corresponding tests	Pass	4 new cases: default containment, root rejection, explicit-root opt-in, cwd allow — all mock process.cwd() with proper restore
Medium	Testing	Error paths and edge cases tested	Pass	Right error regexes asserted per case
Medium	Testing	Existing tests still pass (no regressions)	Pass	29 files / 277 tests green at head; tsc --noEmit and eslint clean
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	Env var read only in src/config.ts; validator uses process.cwd(), not process.env — env-var discipline intact
Medium	Quality	Changes are focused (single concern)	Pass	One concern across 4 files incl. docs
Low	Quality	Meaningful names, no dead code	Pass	Single default owner now; no duplication
Low	Quality	Comments explain why, not what	Pass	config.ts comment explains why the default lives in the validator
Low	Quality	No unnecessary dependencies added	Pass	None added

Findings

All five findings from the PR #366 review are verified fixed at this head:

• src/lib/upload-validator.ts High — fail-closed default vacuous when cwd is / Resolved — root-cwd guard at upload-validator.ts:88-92, tested
• src/config.ts Medium — undocumented breaking default Resolved — README "Upload directory containment" section added
• src/lib/upload-validator.ts Low — misleading error when base dir defaulted Resolved — message branched on usingDefaultBaseDir
• src/lib/upload-validator.ts Low — duplicated cwd default Resolved — validator is the single owner; config passes undefined
• tests/tools/upload-validator.test.ts Low — test relied on tmpdir outside vitest cwd Resolved — process.cwd() mocked in all default-path tests

New findings (all Low / non-blocking):

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

• Severity: Low

• Reviewer: stack:code-review

• Issue: The hidden-directory error says "Move the file to a non-hidden location or set MCP_UPLOAD_BASE_DIR", but the hidden-segment check is unconditional — the env var never bypasses it. Preexisting on main, but this PR standardizes these messages, so it's the right moment.

• Suggestion: Drop the "or set MCP_UPLOAD_BASE_DIR" clause.

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

• Severity: Low

• Reviewer: stack:code-review

• Issue: Line exceeds the repo's Prettier width (prettier --check flags it). The build gate passes only because npm run format targets src/**/*.ts; a future format sweep over tests will churn this line.

• Suggestion: Reflow: vi.spyOn(process, "cwd") then .mockReturnValue(path.parse(workDir).root) on the next line.

• File: README.md:293

• Severity: Low

• Reviewer: stack:code-review

• Issue: MCP_UPLOAD_BASE_DIR is documented in the README but absent from the checked-in env-flag catalog (.claude/knowledge/docs/product/FEATURE-FLAGS.md).

• Suggestion: Add a short entry there.

• File: src/config.ts:56

• Severity: Low

• Reviewer: stack:code-review

• Issue: A whitespace-only MCP_UPLOAD_BASE_DIR=" " is treated as configured and fails as "does not exist ( )" — fail-closed, so harmless, but the error is ugly.

• Suggestion: .trim() in the length check.

Info (no change requested): the remote MCP wrapper was checked — it disables uploadProductRequirementFile, keeps app-upload flows, and stages no files outside cwd, so the new default breaks nothing there and is a multi-tenant improvement. Worth a release-note heads-up so the wrapper team knows to set MCP_UPLOAD_BASE_DIR if they ever stage files elsewhere.

---

Verdict: PASS — all prior findings genuinely fixed; remaining items are Low-severity polish, none blocking.