Skip to content

fix(sandbox-container): prevent path traversal in file read/write/delete - #478

Open
NAVEENKUMARKR777 wants to merge 1 commit into
cloudflare:mainfrom
NAVEENKUMARKR777:fix/sandbox-path-traversal
Open

NAVEENKUMARKR777 wants to merge 1 commit into
cloudflare:mainfrom
NAVEENKUMARKR777:fix/sandbox-path-traversal

Conversation

@NAVEENKUMARKR777

Copy link
Copy Markdown

Summary

container_file_write, container_file_read, and container_file_delete — the MCP tools backing the sandbox's file endpoints — took the client-supplied path (a free-form string, no format constraint in FileWrite/FilePathParam) and used it with no containment check:

  • POST /files/contents (write) called fs.writeFile(reqPath, file.text) directly, with no cwd join at all.
  • GET/DELETE /files/contents/* joined it onto the working directory with path.join(process.cwd(), reqPath), which normalizes .. segments but does not stop them from climbing outside the base directory, and does not stop an absolute path from being concatenated straight onto the prefix.

So a write with path: "../../etc/cron.d/x" (or an absolute path) resolved outside the container's workdir, and reads/deletes were exploitable the same way — arbitrary file read/write/delete on the container filesystem via a directly-exposed MCP tool call. Confirmed by tracing the full call path from the container_file_* tool registrations in apps/sandbox-container/server/container-tools.ts through userContainer.ts into apps/sandbox-container/container/sandbox.container.app.ts; none of the intermediate layers (stripProtocolFromFilePath, get_file_name_from_path) do any traversal or absolute-path check.

Fix

Added resolve_in_workdir() in fileUtils.ts: resolves the requested path against process.cwd() and throws PathTraversalError if the resolved path isn't contained in it. All three handlers (GET/POST/DELETE on /files/contents) and list_files_in_directory now go through this single guard instead of the ad hoc path.join/raw-path calls. A rejected path returns 400 instead of touching the filesystem.

Ordinary usage is unaffected — a leading / in the request path (the normal shape, since these routes are matched off /files/contents/*) still resolves relative to the working directory, matching the prior path.join behavior for non-malicious input.

Test plan

  • Added resolve_in_workdir unit tests covering: relative path, leading-slash path (existing shape from the route), ../ traversal, absolute path treated as relative to the working dir, traversal that only escapes after internal .. segments resolve, and the bare .. segment.
  • Added a list_files_in_directory test asserting a traversal path throws PathTraversalError.
  • pnpm test in apps/sandbox-container (both vitest configs) — all passing.
  • pnpm check:types and pnpm check:lint in apps/sandbox-container — clean.

Fixes #401

container_file_write, container_file_read, and container_file_delete
took the client-supplied path and passed it straight into fs.writeFile
(no cwd join at all) or joined it onto the working directory with
path.join, which does not stop `..` segments or a leading `/` from
resolving outside of it. A path like `../../etc/passwd` or an absolute
path such as `/etc/cron.d/x` therefore escaped the sandbox's working
directory entirely, giving arbitrary file read/write/delete on the
container filesystem through an MCP tool call.

Add resolve_in_workdir(), a single guard that resolves the requested
path against process.cwd() and rejects anything that resolves outside
of it, and route all three handlers through it instead of the ad hoc
path.join calls.

Fixes cloudflare#401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Possible unsanitized path in sandbox.container.app.ts

1 participant