From 0da13d76995011ec0ba85bf19d444710f04fdf88 Mon Sep 17 00:00:00 2001 From: vorticalbox Date: Wed, 29 Apr 2026 18:14:00 +0100 Subject: [PATCH] fix(tools/touch): gate outside-workingDir paths via permission prompt Mirrors the view.go sanitizer pattern (filepath.Abs + filepath.Rel + ".." check) so CodeQL no longer flags os.Stat/MkdirAll/OpenFile in the touch tool as uncontrolled path expressions. --- internal/agent/tools/touch.go | 64 ++++++++++++++++++------ internal/agent/tools/touch_test.go | 80 ++++++++++++++++++++++++++++++ 2 files changed, 129 insertions(+), 15 deletions(-) diff --git a/internal/agent/tools/touch.go b/internal/agent/tools/touch.go index fb3934b2cca31b361389dc6073a0bd13a59af02e..3e94e4e381efe097fd31e45a810fadae59b58d2f 100644 --- a/internal/agent/tools/touch.go +++ b/internal/agent/tools/touch.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "charm.land/fantasy" "github.com/charmbracelet/crush/internal/filepathext" @@ -57,17 +58,50 @@ func NewTouchTool( filePath := filepathext.SmartJoin(workingDir, params.FilePath) - fileInfo, err := os.Stat(filePath) + absWorkingDir, err := filepath.Abs(workingDir) + if err != nil { + return fantasy.ToolResponse{}, fmt.Errorf("error resolving working directory: %w", err) + } + absFilePath, err := filepath.Abs(filePath) + if err != nil { + return fantasy.ToolResponse{}, fmt.Errorf("error resolving file path: %w", err) + } + relPath, relErr := filepath.Rel(absWorkingDir, absFilePath) + isOutsideWorkDir := relErr != nil || strings.HasPrefix(relPath, "..") + + if isOutsideWorkDir { + granted, permReqErr := permissions.Request(ctx, + permission.CreatePermissionRequest{ + SessionID: sessionID, + Path: absFilePath, + ToolCallID: call.ID, + ToolName: TouchToolName, + Action: "write", + Description: fmt.Sprintf("Create empty file outside working directory: %s", absFilePath), + Params: TouchPermissionsParams{ + FilePath: absFilePath, + }, + }, + ) + if permReqErr != nil { + return fantasy.ToolResponse{}, permReqErr + } + if !granted { + return NewPermissionDeniedResponse(), nil + } + } + + fileInfo, err := os.Stat(absFilePath) if err == nil { if fileInfo.IsDir() { - return fantasy.NewTextErrorResponse(fmt.Sprintf("Path is a directory, not a file: %s", filePath)), nil + return fantasy.NewTextErrorResponse(fmt.Sprintf("Path is a directory, not a file: %s", absFilePath)), nil } - return fantasy.NewTextErrorResponse(fmt.Sprintf("File already exists: %s", filePath)), nil + return fantasy.NewTextErrorResponse(fmt.Sprintf("File already exists: %s", absFilePath)), nil } else if !os.IsNotExist(err) { return fantasy.ToolResponse{}, fmt.Errorf("error checking file: %w", err) } - dir := filepath.Dir(filePath) + dir := filepath.Dir(absFilePath) if err = os.MkdirAll(dir, 0o755); err != nil { return fantasy.ToolResponse{}, fmt.Errorf("error creating directory: %w", err) } @@ -75,13 +109,13 @@ func NewTouchTool( p, err := permissions.Request(ctx, permission.CreatePermissionRequest{ SessionID: sessionID, - Path: fsext.PathOrPrefix(filePath, workingDir), + Path: fsext.PathOrPrefix(absFilePath, absWorkingDir), ToolCallID: call.ID, ToolName: TouchToolName, Action: "write", - Description: fmt.Sprintf("Create empty file %s", filePath), + Description: fmt.Sprintf("Create empty file %s", absFilePath), Params: TouchPermissionsParams{ - FilePath: filePath, + FilePath: absFilePath, OldContent: "", NewContent: "", }, @@ -94,10 +128,10 @@ func NewTouchTool( return NewPermissionDeniedResponse(), nil } - file, err := os.OpenFile(filePath, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o644) + file, err := os.OpenFile(absFilePath, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o644) if err != nil { if os.IsExist(err) { - return fantasy.NewTextErrorResponse(fmt.Sprintf("File already exists: %s", filePath)), nil + return fantasy.NewTextErrorResponse(fmt.Sprintf("File already exists: %s", absFilePath)), nil } return fantasy.ToolResponse{}, fmt.Errorf("error creating file: %w", err) } @@ -105,21 +139,21 @@ func NewTouchTool( return fantasy.ToolResponse{}, fmt.Errorf("error closing file: %w", err) } - _, err = files.Create(ctx, sessionID, filePath, "") + _, err = files.Create(ctx, sessionID, absFilePath, "") if err != nil { return fantasy.ToolResponse{}, fmt.Errorf("error creating file history: %w", err) } - filetracker.RecordRead(ctx, sessionID, filePath) + filetracker.RecordRead(ctx, sessionID, absFilePath) - notifyLSPs(ctx, lspManager, filePath) + notifyLSPs(ctx, lspManager, absFilePath) - result := fmt.Sprintf("Empty file successfully created: %s", filePath) + result := fmt.Sprintf("Empty file successfully created: %s", absFilePath) result = fmt.Sprintf("\n%s\n", result) - result += getDiagnostics(filePath, lspManager) + result += getDiagnostics(absFilePath, lspManager) return fantasy.WithResponseMetadata(fantasy.NewTextResponse(result), TouchResponseMetadata{ - FilePath: filePath, + FilePath: absFilePath, }, ), nil }) diff --git a/internal/agent/tools/touch_test.go b/internal/agent/tools/touch_test.go index 81cb1569498729a7f575388b594e3a3cd3407086..feacbeb4616814a942ffb1836e171abb6580cbe5 100644 --- a/internal/agent/tools/touch_test.go +++ b/internal/agent/tools/touch_test.go @@ -5,13 +5,39 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" "charm.land/fantasy" + "github.com/charmbracelet/crush/internal/permission" + "github.com/charmbracelet/crush/internal/pubsub" "github.com/stretchr/testify/require" ) +// recordingPermissionService captures permission requests and answers them +// according to a configurable response. +type recordingPermissionService struct { + *pubsub.Broker[permission.PermissionRequest] + requests []permission.CreatePermissionRequest + grant bool +} + +func (m *recordingPermissionService) Request(ctx context.Context, req permission.CreatePermissionRequest) (bool, error) { + m.requests = append(m.requests, req) + return m.grant, nil +} + +func (m *recordingPermissionService) Grant(req permission.PermissionRequest) {} +func (m *recordingPermissionService) Deny(req permission.PermissionRequest) {} +func (m *recordingPermissionService) GrantPersistent(req permission.PermissionRequest) {} +func (m *recordingPermissionService) AutoApproveSession(sessionID string) {} +func (m *recordingPermissionService) SetSkipRequests(skip bool) {} +func (m *recordingPermissionService) SkipRequests() bool { return false } +func (m *recordingPermissionService) SubscribeNotifications(ctx context.Context) <-chan pubsub.Event[permission.PermissionNotification] { + return make(<-chan pubsub.Event[permission.PermissionNotification]) +} + type mockFileTrackerService struct{} func (m mockFileTrackerService) RecordRead(ctx context.Context, sessionID, path string) {} @@ -60,6 +86,60 @@ func TestTouchToolRefusesExistingFile(t *testing.T) { require.Equal(t, "content", string(content)) } +func TestTouchToolStaysInsideWorkingDir(t *testing.T) { + t.Parallel() + + workingDir := t.TempDir() + perms := &recordingPermissionService{grant: true} + tool := NewTouchTool(nil, perms, &mockHistoryService{}, mockFileTrackerService{}, workingDir) + ctx := context.WithValue(context.Background(), SessionIDContextKey, "test-session") + + resp := runTouchTool(t, tool, ctx, TouchParams{FilePath: "inside.txt"}) + require.False(t, resp.IsError) + + for _, req := range perms.requests { + require.NotContains(t, req.Description, "outside working directory", + "inside-workingDir touch should not trigger an outside-workingDir permission prompt") + } + + _, err := os.Stat(filepath.Join(workingDir, "inside.txt")) + require.NoError(t, err) +} + +func TestTouchToolOutsideWorkingDirRequiresPermission(t *testing.T) { + t.Parallel() + + parent := t.TempDir() + workingDir := filepath.Join(parent, "wd") + require.NoError(t, os.MkdirAll(workingDir, 0o755)) + + // Denied: file outside workingDir must not be created. + deny := &recordingPermissionService{grant: false} + tool := NewTouchTool(nil, deny, &mockHistoryService{}, mockFileTrackerService{}, workingDir) + ctx := context.WithValue(context.Background(), SessionIDContextKey, "test-session") + + resp := runTouchTool(t, tool, ctx, TouchParams{FilePath: "../escape.txt"}) + require.True(t, resp.IsError) + + require.Len(t, deny.requests, 1) + require.True(t, strings.Contains(deny.requests[0].Description, "outside working directory"), + "expected outside-working-directory permission prompt, got %q", deny.requests[0].Description) + + _, err := os.Stat(filepath.Join(parent, "escape.txt")) + require.True(t, os.IsNotExist(err), "denied permission should not create the file") + + // Granted: same path now succeeds. + grant := &recordingPermissionService{grant: true} + tool = NewTouchTool(nil, grant, &mockHistoryService{}, mockFileTrackerService{}, workingDir) + resp = runTouchTool(t, tool, ctx, TouchParams{FilePath: "../escape.txt"}) + require.False(t, resp.IsError) + require.GreaterOrEqual(t, len(grant.requests), 1) + require.Contains(t, grant.requests[0].Description, "outside working directory") + + _, err = os.Stat(filepath.Join(parent, "escape.txt")) + require.NoError(t, err) +} + func TestWriteToolEmptyContentPointsToTouch(t *testing.T) { t.Parallel()