Skip to content

fix(api): reject environment folder paths outside the workspace volume #651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pikann merged 1 commit into from
Oct 28, 2026
Merged

fix(api): reject environment folder paths outside the workspace volume #651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions apps/web/src/components/projects/environments/folder-create-dialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,11 @@ export function FolderCreateDialog({
});

const trimmedPath = path.trim();
const isValidPath = trimmedPath.startsWith("/");
const isValidPath = trimmedPath.startsWith(ENVIRONMENT_HOME_ROOT);

// isValidPath already blocks a relative path, so
// EnvironmentFolderPathInvalid mainly guards a stale check;
// isValidPath is the same root check the server now enforces (folders
// outside the mounted workspace vanish on the next Pod recreation and

@pullfrog pullfrog Bot Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not quite "the same root check the server now enforces": a raw startsWith(ENVIRONMENT_HOME_ROOT) also accepts prefix siblings like /home/paca/workspaces-other and uncleaned .. segments, both of which the server rejects after path.Clean. The server is authoritative so nothing breaks, but the comment overstates the equivalence and EnvironmentFolderPathInvalid is reachable from a client-valid input — consider softening the wording rather than tightening the check.

// break every later attach), so EnvironmentFolderPathInvalid is rare;
// EnvironmentFolderPathTaken is the genuinely common case — the same
// path added twice.
const addErrorMessage = addMutation.isError
Expand Down
2 changes: 1 addition & 1 deletion services/api/internal/domain/environment/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var (
var (
ErrFolderNotFound = errors.New("environment folder not found")
ErrFolderPathTaken = errors.New("a folder with this path already exists in this environment")
ErrFolderPathInvalid = errors.New("folder path must be an absolute path")
ErrFolderPathInvalid = errors.New("folder path must be inside the environment's workspace (/home/paca/workspaces)")
)

// SSH key errors
Expand Down
18 changes: 16 additions & 2 deletions services/api/internal/service/environment/environment_service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"io"
"net/http"
"net/url"
pathpkg "path"
"regexp"
"strings"
"time"
Expand Down Expand Up @@ -981,6 +982,11 @@ func (s *Service) ListFolders(ctx context.Context, projectID, environmentID uuid
return s.repo.ListFolders(ctx, env.ID, filter, cursorAfter, limit)
}

// environmentHomeRoot is the one directory that survives a container/Pod
// recreation — the mounted workspace volume every environment folder must
// live under. Same value agent-runner uses for its own Browse/mkdir guards.
const environmentHomeRoot = "/home/paca/workspaces"

// AddFolder creates the folder row and asks agent-runner to mkdir it into
// place inside the environment's container. Folders are path-only — no
// name/repo-clone/branch fields (dropped before ever shipping; see
Expand All @@ -991,8 +997,16 @@ func (s *Service) AddFolder(ctx context.Context, projectID, environmentID uuid.U
return nil, err
}

path := strings.TrimSpace(in.Path)
if !strings.HasPrefix(path, "/") {
path := pathpkg.Clean(strings.TrimSpace(in.Path))
// Every folder must live under the mounted workspace volume. A path
// outside it is accepted by the mkdir below but written to the
// container's own writable layer, so it disappears on the next
// Pod/container recreation and every later attach fails with ACP
// "session/new: Invalid params: invalid directory path".
// Mirrors agent-runner's environmentHomeRoot
// (internal/acpbridge/environment_handlers.go), which scopes Browse and
// the mkdir/delete guards to the same root.
if path != environmentHomeRoot && !strings.HasPrefix(path, environmentHomeRoot+"/") {

@pullfrog pullfrog Bot Oct 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allowing the workspace root here is inconsistent with agent-runner, which refuses to mkdir it: forbiddenFolderTargets lists environmentHomeRoot itself, so the POST /internal/environments/{id}/folders this triggers returns 400. Because the folder row is created first, a running environment ends up with the row persisted while the request errors, and a retry then reports path taken — and the dialog's default path is the root, so this is the default submission. If root-as-folder is intended, agent-runner needs to permit it (or the mkdir should be skipped when path == environmentHomeRoot); otherwise drop the path != environmentHomeRoot carve-out and the "root" test case.

Technical details 
# Root carve-out vs. agent-runner's mkdir guard

## Affected sites
- `services/api/internal/service/environment/environment_service.go:1009` — accepts `path == environmentHomeRoot`.
- `services/api/internal/service/environment/environment_service.go:1033-1041` — for a `StatusRunning` env the row is committed (`CreateFolder`, line 1022) *before* the agent-runner call, so the 400 leaves a persisted row.
- `services/agent-runner/internal/acpbridge/environment_handlers.go:597-618` — `forbiddenFolderTargets` includes `environmentHomeRoot`.
- `services/agent-runner/internal/acpbridge/environment_handlers.go:675-679` — `handleCreateEnvironmentFolder` calls `safeFolderTarget` unconditionally, which refuses it.
- `services/api/internal/service/environment/environment_service_test.go:1622` — pins `"root"` as valid; the test's env has `BackendRef == nil`, so the agent-runner leg is never exercised.

## Required outcome
- Root is either rejected consistently at the api boundary, or the full create flow (including the mkdir leg for a running env) succeeds for it.

## Also in this comment block
- `environment_service.go:1006-1008` states agent-runner "scopes Browse and the mkdir/delete guards to the same root." `safeFolderTarget` only refuses a fixed list of top-level directories; it does not scope mkdir to `environmentHomeRoot`. And there is no delete guard/endpoint — `DeleteFolder` is row-only (`environment_service.go:1110`).

## Open questions for the human
- Is a folder whose path is the workspace root a supported object (e.g. as a conversation workdir), or is the dialog default meant to force a subfolder?

return nil, environmentdom.ErrFolderPathInvalid
}

Expand Down
41 changes: 41 additions & 0 deletions services/api/internal/service/environment/environment_service_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1600,3 +1600,44 @@ func TestRestartEnvironment_SurvivesCallerContextCancellation(t *testing.T) {
require.NotNil(t, gotBackendRef, "the fresh backend_ref must still be persisted despite the caller's ctx cancellation")
assert.Equal(t, "container-new", *gotBackendRef)
}

// TestAddFolder_PathMustBeUnderWorkspaceRoot pins the guard against folder
// paths outside the mounted workspace volume: mkdir happily creates them in
// the container's writable layer, they disappear on the next Pod
// recreation, and every later attach then fails with ACP "session/new:
// Invalid params: invalid directory path".
func TestAddFolder_PathMustBeUnderWorkspaceRoot(t *testing.T) {
env := &environmentdom.Environment{ID: uuid.New(), Status: environmentdom.StatusRunning}
repo := &mockRepo{
findVisibleEnvironmentInProject: func(_ context.Context, _, _ uuid.UUID) (*environmentdom.Environment, error) {
return env, nil
},
}
svc := New(repo, "", "")

cases := []struct {
name, path, want string
wantErr bool
}{
{"root", environmentHomeRoot, environmentHomeRoot, false},
{"subfolder", environmentHomeRoot + "/api", environmentHomeRoot + "/api", false},
{"subfolder with trailing slash", environmentHomeRoot + "/api/", environmentHomeRoot + "/api", false},
{"outside the volume", "/home/paca-dev/workspaces", "", true},
{"sibling with the root as prefix", "/home/paca/workspaces-other", "", true},
{"traversal out of the root", environmentHomeRoot + "/../../etc", "", true},
{"relative", "workspaces/api", "", true},
{"empty", "", "", true},
}

for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
folder, err := svc.AddFolder(context.Background(), uuid.New(), env.ID, environmentdom.AddFolderInput{Path: tc.path})
if tc.wantErr {
assert.ErrorIs(t, err, environmentdom.ErrFolderPathInvalid)
return
}
require.NoError(t, err)
assert.Equal(t, tc.want, folder.Path)
})
}
}
Loading