diff --git a/cmd/gnoma/main.go b/cmd/gnoma/main.go index bf309fe..9fe2a56 100644 --- a/cmd/gnoma/main.go +++ b/cmd/gnoma/main.go @@ -649,6 +649,7 @@ func main() { // Resolve skill invocations in pipe mode (/skillname args). submitInput := input + var submitOpts engine.TurnOptions if strings.HasPrefix(input, "/") { parts := strings.Fields(input) name := strings.TrimPrefix(parts[0], "/") @@ -666,10 +667,14 @@ func main() { os.Exit(1) } submitInput = rendered + submitOpts = engine.TurnOptions{ + AllowedTools: sk.Frontmatter.AllowedTools, + AllowedPaths: sk.Frontmatter.Paths, + } } } - _, err = eng.Submit(ctx, submitInput, cb) + _, err = eng.SubmitWithOptions(ctx, submitInput, submitOpts, cb) fmt.Println() if err != nil { diff --git a/internal/engine/engine.go b/internal/engine/engine.go index 38ca6fe..06dd8c0 100644 --- a/internal/engine/engine.go +++ b/internal/engine/engine.go @@ -54,6 +54,7 @@ type Turn struct { type TurnOptions struct { ToolChoice provider.ToolChoiceMode // "" = use provider default AllowedTools []string // if non-nil, only these tools are sent (matched by name) + AllowedPaths []string // if non-nil, tool filesystem access is restricted to these paths } // Engine orchestrates the conversation. diff --git a/internal/engine/loop.go b/internal/engine/loop.go index 37d6a9e..3f400c3 100644 --- a/internal/engine/loop.go +++ b/internal/engine/loop.go @@ -505,6 +505,11 @@ func (e *Engine) executeSingleTool(ctx context.Context, call message.ToolCall, t } } + // Path restriction: deny bash and validate fs tool paths against AllowedPaths. + if denied, blocked := checkPathRestriction(call, t, args, e.turnOpts.AllowedPaths); blocked { + return denied + } + e.logger.Debug("executing tool", "name", call.Name, "id", call.ID) result, err := t.Execute(ctx, args) diff --git a/internal/engine/paths.go b/internal/engine/paths.go new file mode 100644 index 0000000..c36db02 --- /dev/null +++ b/internal/engine/paths.go @@ -0,0 +1,86 @@ +package engine + +import ( + "encoding/json" + "fmt" + "os" + "path/filepath" + "strings" + + "somegit.dev/Owlibou/gnoma/internal/message" + "somegit.dev/Owlibou/gnoma/internal/tool" +) + +// isUnderAllowedPaths reports whether target is equal to or a descendant of any +// path in allowed. Both sides are cleaned before comparison. Returns false when +// allowed is empty. +// +// The trailing-separator check prevents "/tmp" from matching "/tmpx/foo". +func isUnderAllowedPaths(target string, allowed []string) bool { + target = filepath.Clean(target) + sep := string(filepath.Separator) + for _, a := range allowed { + a = filepath.Clean(a) + if target == a || strings.HasPrefix(target, a+sep) { + return true + } + } + return false +} + +// checkPathRestriction enforces AllowedPaths on a single tool call. +// +// Rules (in order): +// 1. If allowed is empty, everything is permitted (fast-path). +// 2. "bash" is always denied when path restrictions are active. +// 3. Tools implementing tool.PathSensitiveTool have their extracted paths +// checked against allowed. An empty extracted path is resolved to cwd. +// 4. Tools that do not implement PathSensitiveTool are permitted (they don't +// declare filesystem access). +// +// Returns (denied result, true) when blocked, or (zero, false) when allowed. +func checkPathRestriction(call message.ToolCall, t tool.Tool, args json.RawMessage, allowed []string) (message.ToolResult, bool) { + if len(allowed) == 0 { + return message.ToolResult{}, false + } + + if call.Name == "bash" { + return message.ToolResult{ + ToolCallID: call.ID, + Content: "bash is not permitted when skill path restrictions are active", + IsError: true, + }, true + } + + pt, ok := t.(tool.PathSensitiveTool) + if !ok { + return message.ToolResult{}, false + } + + for _, p := range pt.ExtractPaths(args) { + var resolved string + if p == "" { + cwd, err := os.Getwd() + if err != nil { + return message.ToolResult{ + ToolCallID: call.ID, + Content: fmt.Sprintf("path access denied: cannot determine current directory: %v", err), + IsError: true, + }, true + } + resolved = cwd + } else { + resolved = p + } + + if !isUnderAllowedPaths(resolved, allowed) { + return message.ToolResult{ + ToolCallID: call.ID, + Content: fmt.Sprintf("path access denied: %q is not in allowed paths %v", resolved, allowed), + IsError: true, + }, true + } + } + + return message.ToolResult{}, false +} diff --git a/internal/engine/paths_test.go b/internal/engine/paths_test.go new file mode 100644 index 0000000..c4dc4f6 --- /dev/null +++ b/internal/engine/paths_test.go @@ -0,0 +1,243 @@ +package engine + +import ( + "context" + "encoding/json" + "testing" + + "somegit.dev/Owlibou/gnoma/internal/message" + "somegit.dev/Owlibou/gnoma/internal/provider" + "somegit.dev/Owlibou/gnoma/internal/stream" + "somegit.dev/Owlibou/gnoma/internal/tool" +) + +func TestIsUnderAllowedPaths(t *testing.T) { + tests := []struct { + name string + target string + allowed []string + want bool + }{ + {"exact match", "/tmp/foo", []string{"/tmp/foo"}, true}, + {"under allowed dir", "/tmp/foo/bar.go", []string{"/tmp"}, true}, + {"not under allowed", "/etc/passwd", []string{"/tmp"}, false}, + {"prevents prefix bypass", "/tmpx/foo", []string{"/tmp"}, false}, + {"matches second path", "/home/user/file", []string{"/tmp", "/home/user"}, true}, + {"empty allowed slice", "/tmp/foo", []string{}, false}, + {"nested dir", "/project/src/main.go", []string{"/project"}, true}, + {"sibling dir denied", "/project-evil/foo", []string{"/project"}, false}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := isUnderAllowedPaths(tc.target, tc.allowed) + if got != tc.want { + t.Errorf("isUnderAllowedPaths(%q, %v) = %v, want %v", + tc.target, tc.allowed, got, tc.want) + } + }) + } +} + +// mockPathSensitiveTool is a mock that implements both Tool and PathSensitiveTool. +type mockPathSensitiveTool struct { + mockTool + extractedPaths []string +} + +func (m *mockPathSensitiveTool) ExtractPaths(_ json.RawMessage) []string { + return m.extractedPaths +} + +func TestSubmitWithOptions_AllowedPaths_DeniesBash(t *testing.T) { + called := false + reg := tool.NewRegistry() + reg.Register(&mockTool{ + name: "bash", + execFn: func(_ context.Context, _ json.RawMessage) (tool.Result, error) { + called = true + return tool.Result{Output: "should not reach here"}, nil + }, + }) + + mp := &mockProvider{ + name: "test", + streams: []stream.Stream{ + newEventStream(message.StopToolUse, "", + stream.Event{Type: stream.EventToolCallStart, ToolCallID: "tc1", ToolCallName: "bash"}, + stream.Event{Type: stream.EventToolCallDone, ToolCallID: "tc1", Args: json.RawMessage(`{"command":"cat /etc/passwd"}`)}, + ), + newEventStream(message.StopEndTurn, "", + stream.Event{Type: stream.EventTextDelta, Text: "blocked"}, + ), + }, + } + + e, _ := New(Config{Provider: mp, Tools: reg}) + + _, err := e.SubmitWithOptions(context.Background(), "run bash", + TurnOptions{AllowedPaths: []string{"/tmp"}}, nil) + if err != nil { + t.Fatalf("SubmitWithOptions: %v", err) + } + if called { + t.Error("bash tool should not be executed when AllowedPaths is set") + } +} + +func TestSubmitWithOptions_AllowedPaths_DeniesOutsidePath(t *testing.T) { + called := false + reg := tool.NewRegistry() + reg.Register(&mockPathSensitiveTool{ + mockTool: mockTool{ + name: "fs.read", + execFn: func(_ context.Context, _ json.RawMessage) (tool.Result, error) { + called = true + return tool.Result{Output: "secret content"}, nil + }, + }, + extractedPaths: []string{"/etc/passwd"}, + }) + + mp := &mockProvider{ + name: "test", + streams: []stream.Stream{ + newEventStream(message.StopToolUse, "", + stream.Event{Type: stream.EventToolCallStart, ToolCallID: "tc1", ToolCallName: "fs.read"}, + stream.Event{Type: stream.EventToolCallDone, ToolCallID: "tc1", Args: json.RawMessage(`{"path":"/etc/passwd"}`)}, + ), + newEventStream(message.StopEndTurn, "", + stream.Event{Type: stream.EventTextDelta, Text: "blocked"}, + ), + }, + } + + e, _ := New(Config{Provider: mp, Tools: reg}) + + _, err := e.SubmitWithOptions(context.Background(), "read file", + TurnOptions{AllowedPaths: []string{"/tmp"}}, nil) + if err != nil { + t.Fatalf("SubmitWithOptions: %v", err) + } + if called { + t.Error("fs.read should not be executed when path is outside AllowedPaths") + } +} + +func TestSubmitWithOptions_AllowedPaths_AllowsInsidePath(t *testing.T) { + called := false + reg := tool.NewRegistry() + reg.Register(&mockPathSensitiveTool{ + mockTool: mockTool{ + name: "fs.read", + execFn: func(_ context.Context, _ json.RawMessage) (tool.Result, error) { + called = true + return tool.Result{Output: "ok"}, nil + }, + }, + extractedPaths: []string{"/tmp/allowed/file.txt"}, + }) + + mp := &mockProvider{ + name: "test", + streams: []stream.Stream{ + newEventStream(message.StopToolUse, "", + stream.Event{Type: stream.EventToolCallStart, ToolCallID: "tc1", ToolCallName: "fs.read"}, + stream.Event{Type: stream.EventToolCallDone, ToolCallID: "tc1", Args: json.RawMessage(`{"path":"/tmp/allowed/file.txt"}`)}, + ), + newEventStream(message.StopEndTurn, "", + stream.Event{Type: stream.EventTextDelta, Text: "done"}, + ), + }, + } + + e, _ := New(Config{Provider: mp, Tools: reg}) + + _, err := e.SubmitWithOptions(context.Background(), "read file", + TurnOptions{AllowedPaths: []string{"/tmp/allowed"}}, nil) + if err != nil { + t.Fatalf("SubmitWithOptions: %v", err) + } + if !called { + t.Error("fs.read should be executed when path is inside AllowedPaths") + } +} + +func TestSubmitWithOptions_NilAllowedPaths_NoRestriction(t *testing.T) { + called := false + reg := tool.NewRegistry() + reg.Register(&mockPathSensitiveTool{ + mockTool: mockTool{ + name: "fs.read", + execFn: func(_ context.Context, _ json.RawMessage) (tool.Result, error) { + called = true + return tool.Result{Output: "ok"}, nil + }, + }, + extractedPaths: []string{"/etc/passwd"}, + }) + + mp := &mockProvider{ + name: "test", + streams: []stream.Stream{ + newEventStream(message.StopToolUse, "", + stream.Event{Type: stream.EventToolCallStart, ToolCallID: "tc1", ToolCallName: "fs.read"}, + stream.Event{Type: stream.EventToolCallDone, ToolCallID: "tc1", Args: json.RawMessage(`{"path":"/etc/passwd"}`)}, + ), + newEventStream(message.StopEndTurn, "", + stream.Event{Type: stream.EventTextDelta, Text: "done"}, + ), + }, + } + + e, _ := New(Config{Provider: mp, Tools: reg}) + + // No AllowedPaths → no restriction + _, err := e.SubmitWithOptions(context.Background(), "read file", TurnOptions{}, nil) + if err != nil { + t.Fatalf("SubmitWithOptions: %v", err) + } + if !called { + t.Error("fs.read should be executed when AllowedPaths is not set") + } +} + +func TestSubmitWithOptions_AllowedPaths_NonPathSensitiveToolAllowed(t *testing.T) { + // A tool that doesn't implement PathSensitiveTool should be permitted even + // when AllowedPaths is set — it doesn't access the filesystem directly. + called := false + reg := tool.NewRegistry() + reg.Register(&mockTool{ // plain mockTool, not PathSensitiveTool + name: "sysinfo", + execFn: func(_ context.Context, _ json.RawMessage) (tool.Result, error) { + called = true + return tool.Result{Output: "linux"}, nil + }, + }) + + mp := &mockProvider{ + name: "test", + streams: []stream.Stream{ + newEventStream(message.StopToolUse, "", + stream.Event{Type: stream.EventToolCallStart, ToolCallID: "tc1", ToolCallName: "sysinfo"}, + stream.Event{Type: stream.EventToolCallDone, ToolCallID: "tc1", Args: json.RawMessage(`{}`)}, + ), + newEventStream(message.StopEndTurn, "", + stream.Event{Type: stream.EventTextDelta, Text: "done"}, + ), + }, + } + + // Register arm with tool support + from := provider.Capabilities{ToolUse: true} + _ = from + e, _ := New(Config{Provider: mp, Tools: reg}) + + _, err := e.SubmitWithOptions(context.Background(), "get info", + TurnOptions{AllowedPaths: []string{"/tmp"}}, nil) + if err != nil { + t.Fatalf("SubmitWithOptions: %v", err) + } + if !called { + t.Error("non-path-sensitive tool should be permitted regardless of AllowedPaths") + } +} diff --git a/internal/skill/skill.go b/internal/skill/skill.go index ac99f73..2ba50b9 100644 --- a/internal/skill/skill.go +++ b/internal/skill/skill.go @@ -18,8 +18,8 @@ type Frontmatter struct { Name string `yaml:"name"` Description string `yaml:"description"` WhenToUse string `yaml:"whenToUse"` - AllowedTools []string `yaml:"allowedTools"` // TODO(M8.3): enforce tool restrictions - Paths []string `yaml:"paths"` // TODO(M8.3): enforce path restrictions + AllowedTools []string `yaml:"allowedTools"` + Paths []string `yaml:"paths"` } // Skill is a parsed skill definition ready for invocation. diff --git a/internal/tool/fs/edit.go b/internal/tool/fs/edit.go index 775c934..c98fc4f 100644 --- a/internal/tool/fs/edit.go +++ b/internal/tool/fs/edit.go @@ -45,6 +45,14 @@ func (t *EditTool) Parameters() json.RawMessage { return editParams } func (t *EditTool) IsReadOnly() bool { return false } func (t *EditTool) IsDestructive() bool { return false } +func (t *EditTool) ExtractPaths(args json.RawMessage) []string { + var a editArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} +} + type editArgs struct { Path string `json:"path"` OldString string `json:"old_string"` diff --git a/internal/tool/fs/fs_test.go b/internal/tool/fs/fs_test.go index dccf02c..fe25635 100644 --- a/internal/tool/fs/fs_test.go +++ b/internal/tool/fs/fs_test.go @@ -624,6 +624,64 @@ func TestFormatSize(t *testing.T) { } } +// --- ExtractPaths --- + +func TestExtractPaths_Read(t *testing.T) { + r := NewReadTool() + paths := r.ExtractPaths(mustJSON(t, readArgs{Path: "/foo/bar.txt"})) + if len(paths) != 1 || paths[0] != "/foo/bar.txt" { + t.Errorf("ExtractPaths = %v, want [/foo/bar.txt]", paths) + } +} + +func TestExtractPaths_Write(t *testing.T) { + w := NewWriteTool() + paths := w.ExtractPaths(mustJSON(t, writeArgs{Path: "/foo/out.txt", Content: "x"})) + if len(paths) != 1 || paths[0] != "/foo/out.txt" { + t.Errorf("ExtractPaths = %v, want [/foo/out.txt]", paths) + } +} + +func TestExtractPaths_Edit(t *testing.T) { + e := NewEditTool() + paths := e.ExtractPaths(mustJSON(t, editArgs{Path: "/foo/file.go", OldString: "a", NewString: "b"})) + if len(paths) != 1 || paths[0] != "/foo/file.go" { + t.Errorf("ExtractPaths = %v, want [/foo/file.go]", paths) + } +} + +func TestExtractPaths_Glob_ExplicitPath(t *testing.T) { + g := NewGlobTool() + paths := g.ExtractPaths(mustJSON(t, globArgs{Pattern: "*.go", Path: "/project/src"})) + if len(paths) != 1 || paths[0] != "/project/src" { + t.Errorf("ExtractPaths = %v, want [/project/src]", paths) + } +} + +func TestExtractPaths_Glob_EmptyPathIsCwd(t *testing.T) { + g := NewGlobTool() + paths := g.ExtractPaths(mustJSON(t, globArgs{Pattern: "*.go"})) + if len(paths) != 1 || paths[0] != "" { + t.Errorf("ExtractPaths = %v, want [\"\"] (empty = cwd)", paths) + } +} + +func TestExtractPaths_Grep(t *testing.T) { + g := NewGrepTool() + paths := g.ExtractPaths(mustJSON(t, grepArgs{Pattern: "func", Path: "/project"})) + if len(paths) != 1 || paths[0] != "/project" { + t.Errorf("ExtractPaths = %v, want [/project]", paths) + } +} + +func TestExtractPaths_LS(t *testing.T) { + l := NewLSTool() + paths := l.ExtractPaths(mustJSON(t, lsArgs{Path: "/project/src"})) + if len(paths) != 1 || paths[0] != "/project/src" { + t.Errorf("ExtractPaths = %v, want [/project/src]", paths) + } +} + // --- Helpers --- func writeTestFile(t *testing.T, content string) string { diff --git a/internal/tool/fs/glob.go b/internal/tool/fs/glob.go index 9141c7d..5c3e7ff 100644 --- a/internal/tool/fs/glob.go +++ b/internal/tool/fs/glob.go @@ -40,6 +40,14 @@ func (t *GlobTool) Parameters() json.RawMessage { return globParams } func (t *GlobTool) IsReadOnly() bool { return true } func (t *GlobTool) IsDestructive() bool { return false } +func (t *GlobTool) ExtractPaths(args json.RawMessage) []string { + var a globArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} // empty string = caller resolves to cwd +} + type globArgs struct { Pattern string `json:"pattern"` Path string `json:"path,omitempty"` diff --git a/internal/tool/fs/grep.go b/internal/tool/fs/grep.go index 5e64869..f07a171 100644 --- a/internal/tool/fs/grep.go +++ b/internal/tool/fs/grep.go @@ -51,6 +51,14 @@ func (t *GrepTool) Parameters() json.RawMessage { return grepParams } func (t *GrepTool) IsReadOnly() bool { return true } func (t *GrepTool) IsDestructive() bool { return false } +func (t *GrepTool) ExtractPaths(args json.RawMessage) []string { + var a grepArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} // empty string = caller resolves to cwd +} + type grepArgs struct { Pattern string `json:"pattern"` Path string `json:"path,omitempty"` diff --git a/internal/tool/fs/ls.go b/internal/tool/fs/ls.go index 8a0d9f6..8ef9a4d 100644 --- a/internal/tool/fs/ls.go +++ b/internal/tool/fs/ls.go @@ -34,6 +34,14 @@ func (t *LSTool) Parameters() json.RawMessage { return lsParams } func (t *LSTool) IsReadOnly() bool { return true } func (t *LSTool) IsDestructive() bool { return false } +func (t *LSTool) ExtractPaths(args json.RawMessage) []string { + var a lsArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} // empty string = caller resolves to cwd +} + type lsArgs struct { Path string `json:"path,omitempty"` } diff --git a/internal/tool/fs/read.go b/internal/tool/fs/read.go index 6b18648..e9b757b 100644 --- a/internal/tool/fs/read.go +++ b/internal/tool/fs/read.go @@ -58,6 +58,14 @@ func (t *ReadTool) Parameters() json.RawMessage { return readParams } func (t *ReadTool) IsReadOnly() bool { return true } func (t *ReadTool) IsDestructive() bool { return false } +func (t *ReadTool) ExtractPaths(args json.RawMessage) []string { + var a readArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} +} + type readArgs struct { Path string `json:"path"` Offset int `json:"offset,omitempty"` diff --git a/internal/tool/fs/write.go b/internal/tool/fs/write.go index 9a4c6a1..85e8d4a 100644 --- a/internal/tool/fs/write.go +++ b/internal/tool/fs/write.go @@ -52,6 +52,14 @@ func (t *WriteTool) Parameters() json.RawMessage { return writeParams } func (t *WriteTool) IsReadOnly() bool { return false } func (t *WriteTool) IsDestructive() bool { return false } +func (t *WriteTool) ExtractPaths(args json.RawMessage) []string { + var a writeArgs + if err := json.Unmarshal(args, &a); err != nil { + return nil + } + return []string{a.Path} +} + type writeArgs struct { Path string `json:"path"` Content string `json:"content"` diff --git a/internal/tool/tool.go b/internal/tool/tool.go index 48abac6..262da8c 100644 --- a/internal/tool/tool.go +++ b/internal/tool/tool.go @@ -27,3 +27,11 @@ type Tool interface { type DeferrableTool interface { ShouldDefer() bool } + +// PathSensitiveTool is an optional interface for tools that access the filesystem. +// Engines enforcing skill path restrictions call ExtractPaths to validate each +// invocation before execution. An empty string in the returned slice means the +// tool will default to the current working directory. +type PathSensitiveTool interface { + ExtractPaths(args json.RawMessage) []string +} diff --git a/internal/tui/app.go b/internal/tui/app.go index 2026436..f9f1c13 100644 --- a/internal/tui/app.go +++ b/internal/tui/app.go @@ -1164,7 +1164,11 @@ func (m Model) handleCommand(cmd string) (tea.Model, tea.Cmd) { m.streamBuf.Reset() m.thinkingBuf.Reset() m.streamFilterClose = "" - if err := m.session.Send(rendered); err != nil { + skillOpts := engine.TurnOptions{ + AllowedTools: sk.Frontmatter.AllowedTools, + AllowedPaths: sk.Frontmatter.Paths, + } + if err := m.session.SendWithOptions(rendered, skillOpts); err != nil { m.messages = append(m.messages, chatMessage{role: "error", content: err.Error()}) m.streaming = false return m, nil