fix: constrain local upload paths to configured directories

[artiom]

Summary

• Limit stdio file uploads to the download directory and explicitly configured additional upload directories.
• Resolve paths and symlinks before checking directory membership, and provide actionable errors for rejected paths.
• Preserve HTTP staging, stored-handle uploads, and the existing file-size limit.

Configuration

BROWSERLESS_UPLOAD_DIRS adds upload directories to the default download directory. Separate entries with : on macOS/Linux or ; on Windows. Unset or empty values keep the download-directory-only default.

Model-facing file-transfer guidance now describes these requirements.

Test plan

• npm test — 1,054 passing, including build and regression tests.
• npm run lint
• Prettier check for all changed files.
• Checked that the HTTP path-rejection and stored-handle branches are unchanged.

Coverage includes allowed paths, explicit additional directories, canonical paths and symlinks, rejected paths, default configuration, the upload size cap, and download-to-upload reuse. No live-service smoke test was run; the changed behavior is local stdio file handling.

No new dependencies.

Summary by CodeRabbit

• New Features

  • Added configurable upload directories through BROWSERLESS_UPLOAD_DIRS.
  • Local file uploads now validate paths against approved directories, including symlink targets.
  • Added clearer guidance when upload paths are rejected, including HTTP staging alternatives.

• Bug Fixes

  • Prevented uploads from accessing files outside approved directories or bypassing restrictions through path traversal and symlinks.

• Tests

  • Added coverage for default directories, configured upload locations, path validation, rejected paths, and stored downloads.

[coderabbitai]

Walkthrough

The change adds configurable upload roots, canonical path validation, and local upload enforcement. Paths must remain within the download directory or configured BROWSERLESS_UPLOAD_DIRS directories, including through symlinks. Tests cover configuration, rejection cases, and re-upload flows.

Changes

Upload path security

Layer / File(s)	Summary
Upload roots and path guard src/config.ts, src/lib/download-store.ts, src/lib/upload-path-guard.ts, test/lib/config.spec.ts, test/lib/upload-path-guard.spec.ts	getConfig now returns resolved upload directories. downloadsDir is exported. assertAllowedUploadPath validates canonical paths and rejects invalid or disallowed paths.
Local upload enforcement src/tools/agent.ts, src/tools/schemas.ts, src/skills/*, test/tools/agent.spec.ts, test/integration/server.spec.ts	Local uploads validate paths before reading files. Tool guidance documents the allowlist and symlink rules. Tests cover allowed files, rejected paths, size limits, and stored download handles.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant normalizeUploadCommand
  participant getConfig
  participant assertAllowedUploadPath
  participant Filesystem
  Client->>normalizeUploadCommand: submit local upload path
  normalizeUploadCommand->>getConfig: load uploadDirs
  normalizeUploadCommand->>assertAllowedUploadPath: validate path
  assertAllowedUploadPath->>Filesystem: resolve path and roots
  Filesystem-->>assertAllowedUploadPath: canonical paths
  assertAllowedUploadPath-->>normalizeUploadCommand: allowed path or validation error
  normalizeUploadCommand->>Filesystem: read allowed file

Loading

Suggested reviewers: andymrtnzp

Merge Risk: 🟡 Moderate · up to b3ef2

A process able to modify an allowed upload directory can race the path check and cause files outside the configured roots to be uploaded. Bind validation to the opened file before merging.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 10 files. (1 skipped: …	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main change: restricting local upload paths to configured directories.
Description check	✅ Passed	The description clearly explains the change, configuration behavior, preserved behavior, test coverage, and validation results. It omits the Related issues and Checklist sections, but the required cha…

Full details: Docstring Coverage

Explanation

Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 10 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

_{A rabbit checks each upload gate
Safe roots keep every file in place
Symlinks cannot lead paths astray
Tests guard the upload path each day
Configured roots show the way}

_{Comment @coderabbitai help to get the list of available commands.}

[artiom]

Verification complete: all six reported checks pass (tests on Node 24, package hygiene, Prettier, title validation, CodeRabbit, and Devin Review). Local build/full tests passed with 1,054 tests; lint and changed-file formatting checks also passed. Both reviewers raised the same filesystem-isolation concern; the threads document why descriptor-relative confinement is outside this canonical directory-selection change for an operator-managed filesystem. CodeRabbit explicitly withdrew its finding; both bot threads are resolved. This guard is not an OS sandbox. The docstring-coverage warning is advisory, not a failing repository check. The branch is mergeable and ready for human review. No merge or release performed.

[devin-ai-integration]

Devin Review found 1 potential issue.