fix(sandbox): confine the daemon's /read route to app root for absolute paths - #7130
Closed
pedrofrxncx wants to merge 1 commit into
Closed
pedrofrxncx wants to merge 1 commit into
pedrofrxncx wants to merge 1 commit into
Conversation
…te paths The /read route special-cased absolute paths to bypass paths.SafePath's containment check entirely, while every other fs route (/write, /edit, /mkdir, /rename, /delete) already runs absolute paths through SafePath. An absolute `path` in a /read request could therefore read any file reachable by the daemon process, not just files under AppRoot. Removes resolveReadPath's IsAbs bypass and routes /read through the same paths.SafePath(appRoot, repoDir, path) call as the write-side routes. Added TestReadRefusesAbsolutePathOutsideRoot, which fails against the old code (200 with the file's content) and passes against the fix (400, no content leaked).
Collaborator
Author
|
Closing as stale: this PR sat past the bot's 48h merge window, main has moved on, and its CI results no longer reflect the current base. This is a housekeeping close, not a rejection of the change — if the underlying problem still exists, the bot will find it again and open a fresh, rebased PR. [studio-bot:stale-close] |
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.
Found while hunting the
studio-fs-abstractionfocus area (the fs abstraction itself,packages/sandbox/daemon-go/internal/routes/fs.go, is the real implementation of issue #2884 — the frontendapps/web/src/lib/filesystem.tshinted at doesn't exist and a prior audit confirmed the abstraction is already mature). Reading the route while auditing that area surfaced a real trust-boundary gap.The bug: every fs mutation route (
/write,/edit,/mkdir,/rename,/delete) confines a caller-supplied path viapaths.SafePath(appRoot, repoDir, path), which checks even an absolute path is insideappRoot./readhad its ownresolveReadPathwrapper that special-cased absolute paths to skip that check entirely and pass them straight toos.Stat/read. Apathin a/readrequest crosses a real trust boundary — it comes from the agent's tool call over the daemon's HTTP API — so an absolute path let a run read any file the daemon process can see (secrets, other repos on the same pod, etc.), not just files under the sandbox's app root.Failure scenario: POST
/readwith{"path": "/etc/passwd"}(or any absolute path outsideappRoot) returned 200 with the file's content instead of the 400 every other fs route already gives for the same shape of request.Fix: deleted
resolveReadPath's bypass and route/readthrough the samepaths.SafePathcall the write-side routes use. Net: -13/+... a small deletion, no new abstraction.Regression test:
TestReadRefusesAbsolutePathOutsideRootinfs_test.go— writes a secret file outsideappRoot(viaos.MkdirTemp, nott.TempDir(), so it's genuinely outside the shared per-test temp root) and asserts/readnow 400s and never echoes the content. Confirmed it fails against the pre-fix code (200 + leaked content) and passes after.A reviewer can confirm with:
Locally ran:
go build ./...,go vet ./internal/routes/...,gofmt -l(clean), and the fullinternal/routespackage test suite (all pass, 2 skipped for missing localrg). Full CI covers the rest.Summary by cubic
Fixes a path traversal in the daemon's
/readroute so an absolute path can no longer read files outside the sandbox's app root. Previously/readspecial-cased absolute paths and passed them straight toos.Stat, while every other fs route already confined them withpaths.SafePath./readnow runs through the samepaths.SafePathcontainment as the write-side routes and returns 400 for out-of-root paths.appRootand asserts/readreturns 400 without leaking its content.Written for commit 9dea9f5. Summary will update on new commits.