diff --git a/cmd/gateway_agents.go b/cmd/gateway_agents.go index 4a8efc30..269b7490 100644 --- a/cmd/gateway_agents.go +++ b/cmd/gateway_agents.go @@ -231,19 +231,34 @@ func buildSubagentToolsRegistry( ) (*tools.Registry, *tools.ExecTool) { reg := parentReg.Clone() var execTool *tools.ExecTool + var readTool *tools.ReadFileTool + var writeTool *tools.WriteFileTool + var listTool *tools.ListFilesTool if sandboxMgr != nil { - reg.Register(tools.NewSandboxedReadFileTool(workspace, restrict, sandboxMgr)) - reg.Register(tools.NewSandboxedWriteFileTool(workspace, restrict, sandboxMgr)) - reg.Register(tools.NewSandboxedListFilesTool(workspace, restrict, sandboxMgr)) + readTool = tools.NewSandboxedReadFileTool(workspace, restrict, sandboxMgr) + writeTool = tools.NewSandboxedWriteFileTool(workspace, restrict, sandboxMgr) + listTool = tools.NewSandboxedListFilesTool(workspace, restrict, sandboxMgr) execTool = tools.NewSandboxedExecTool(workspace, restrict, sandboxMgr) - reg.Register(execTool) } else { - reg.Register(tools.NewReadFileTool(workspace, restrict)) - reg.Register(tools.NewWriteFileTool(workspace, restrict)) - reg.Register(tools.NewListFilesTool(workspace, restrict)) + readTool = tools.NewReadFileTool(workspace, restrict) + writeTool = tools.NewWriteFileTool(workspace, restrict) + listTool = tools.NewListFilesTool(workspace, restrict) execTool = tools.NewExecTool(workspace, restrict) - reg.Register(execTool) } + + // These four tools are built fresh, so they start with none of the hardening the + // gateway applied to the parent's instances at startup: exec path denials and their + // exemptions, shell deny-group toggles, the command keyword allowlist, and the + // read/write/list deny prefixes covering config.json, the databases, and delegate/. + // Without this, spawning a subagent widened reach — the parent could not touch the + // data dir, the subagent could. Inherit from the live parent instances so there is + // one source of truth and a later config reload cannot leave subagents behind. + inheritParentPathPolicy(parentReg, readTool, writeTool, listTool, execTool) + + reg.Register(readTool) + reg.Register(writeTool) + reg.Register(listTool) + reg.Register(execTool) // Red Team F3: subagent ExecTool must enforce the secure-CLI gate // (and env scrub on fall-through) — without this, a parent agent // can spawn a subagent to bypass the gate via host-inherited env. @@ -253,6 +268,42 @@ func buildSubagentToolsRegistry( return reg, execTool } +// inheritParentPathPolicy copies the parent registry's tool hardening onto the freshly +// built subagent tools. A tool missing from the parent registry, or registered there +// under an unexpected concrete type, is skipped: the subagent then has no policy to +// inherit for it, which matches the parent having none to give. +func inheritParentPathPolicy( + parentReg *tools.Registry, + readTool *tools.ReadFileTool, + writeTool *tools.WriteFileTool, + listTool *tools.ListFilesTool, + execTool *tools.ExecTool, +) { + if parentReg == nil { + return + } + if pt, ok := parentReg.Get("read_file"); ok { + if parent, ok := pt.(*tools.ReadFileTool); ok { + readTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("write_file"); ok { + if parent, ok := pt.(*tools.WriteFileTool); ok { + writeTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("list_files"); ok { + if parent, ok := pt.(*tools.ListFilesTool); ok { + listTool.InheritPathPolicy(parent) + } + } + if pt, ok := parentReg.Get("exec"); ok { + if parent, ok := pt.(*tools.ExecTool); ok { + execTool.InheritSecurityPolicy(parent) + } + } +} + // setupTTS creates the TTS manager from config and registers providers. // Edge TTS is always registered (free, no API key required). // Always returns a non-nil manager with at least one provider. diff --git a/cmd/gateway_subagent_policy_inherit_test.go b/cmd/gateway_subagent_policy_inherit_test.go new file mode 100644 index 00000000..69185694 --- /dev/null +++ b/cmd/gateway_subagent_policy_inherit_test.go @@ -0,0 +1,127 @@ +package cmd + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/tools" +) + +// A subagent's file and exec tools are constructed fresh, so they start with none of the +// hardening the gateway applies to the parent's instances at startup. Before this was +// wired, spawning a subagent widened reach: the parent could not read config.json or +// exec against the data dir, the subagent could. These tests pin the inheritance. +func TestSubagentToolsInheritParentPolicy(t *testing.T) { + workspace := t.TempDir() + dataDir := t.TempDir() + + // The denied files must exist. Without them a read or a `cat` fails because the file + // is missing, the assertion sees IsError and passes — for the wrong reason. Verified + // by disabling the fix: only the write_file assertion went red until these existed. + if err := os.WriteFile(filepath.Join(workspace, "config.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "config.json"), []byte("{}"), 0o644); err != nil { + t.Fatal(err) + } + + parent := tools.NewRegistry() + parentRead := tools.NewReadFileTool(workspace, true) + parentRead.DenyPaths("config.json", "memory.db") + parentWrite := tools.NewWriteFileTool(workspace, true) + parentWrite.DenyPaths("config.json", "delegate/") + parentList := tools.NewListFilesTool(workspace, true) + parentList.DenyPaths("config.json") + parentExec := tools.NewExecTool(workspace, true) + parentExec.DenyPaths(dataDir) + parent.Register(parentRead) + parent.Register(parentWrite) + parent.Register(parentList) + parent.Register(parentExec) + + reg, execTool := buildSubagentToolsRegistry(parent, workspace, true, nil, nil) + if reg == nil || execTool == nil { + t.Fatal("buildSubagentToolsRegistry returned nil") + } + + ctx := context.Background() + + // exec: a command referencing the parent's denied data dir must be refused. + res := execTool.Execute(ctx, map[string]any{"command": "cat " + dataDir + "/config.json"}) + if res == nil { + t.Fatal("exec returned nil result") + } + if !res.IsError { + t.Errorf("subagent exec reached the parent's denied data dir: %+v", res) + } + + // read_file: the parent's denied prefixes must apply. + rf, ok := reg.Get("read_file") + if !ok { + t.Fatal("read_file missing from subagent registry") + } + res = rf.Execute(ctx, map[string]any{"path": "config.json"}) + if res == nil || !res.IsError { + t.Errorf("subagent read_file reached config.json: %+v", res) + } + + // write_file: same. + wf, ok := reg.Get("write_file") + if !ok { + t.Fatal("write_file missing from subagent registry") + } + res = wf.Execute(ctx, map[string]any{"path": "config.json", "content": "x"}) + if res == nil || !res.IsError { + t.Errorf("subagent write_file reached config.json: %+v", res) + } +} + +// Inheriting denials without the parent's exemptions would leave the subagent unable to +// read the skills it is told to use: the skills store sits under the denied data dir. +func TestSubagentExecInheritsParentPathExemptions(t *testing.T) { + workspace := t.TempDir() + dataDir := t.TempDir() + skillsStore := dataDir + "/skills-store/" + + if err := os.MkdirAll(filepath.Join(skillsStore, "demo"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(skillsStore, "demo", "SKILL.md"), []byte("# demo\n"), 0o644); err != nil { + t.Fatal(err) + } + + parent := tools.NewRegistry() + parentExec := tools.NewExecTool(workspace, true) + parentExec.DenyPaths(dataDir) + parentExec.AllowPathExemptions(skillsStore) + parent.Register(parentExec) + + _, execTool := buildSubagentToolsRegistry(parent, workspace, true, nil, nil) + + res := execTool.Execute(context.Background(), map[string]any{"command": "cat " + skillsStore + "demo/SKILL.md"}) + if res == nil { + t.Fatal("exec returned nil result") + } + if res.IsError { + t.Errorf("exemption did not carry over; reading the skills store was refused: %+v", res) + } + if !strings.Contains(res.ForLLM, "# demo") { + t.Errorf("expected the skill file contents, got %q", res.ForLLM) + } +} + +// A parent registry without the tools, or with nothing configured, must not panic. +func TestSubagentToolsInheritTolerantOfMissingParentTools(t *testing.T) { + workspace := t.TempDir() + + reg, execTool := buildSubagentToolsRegistry(tools.NewRegistry(), workspace, true, nil, nil) + if reg == nil || execTool == nil { + t.Fatal("expected a usable registry from an empty parent") + } + if _, ok := reg.Get("read_file"); !ok { + t.Error("read_file should still be registered") + } +} diff --git a/internal/agent/loop_history_skills.go b/internal/agent/loop_history_skills.go index 012f0688..43547e66 100644 --- a/internal/agent/loop_history_skills.go +++ b/internal/agent/loop_history_skills.go @@ -2,6 +2,8 @@ package agent import ( "context" + + "github.com/nextlevelbuilder/goclaw/internal/skills" ) // Hybrid skill thresholds: when skill count and total token estimate are below @@ -29,26 +31,39 @@ func (l *Loop) resolveSkillsSummary(ctx context.Context, skillFilter []string) s } filtered := l.skillsLoader.FilterSkills(ctx, allowList) - if len(filtered) == 0 { + if !shouldInlineSkills(filtered) { + // Search mode: no XML in prompt, agent uses skill_search tool return "" } + return l.skillsLoader.BuildSummary(ctx, allowList) +} - // Estimate tokens: ~1 token per 4 chars for name+description. - // Cap description length to match BuildSummary() truncation (skillDescMaxLen=200 runes). +// shouldInlineSkills decides between inline mode and search mode for a set of skills. +// +// Both the live prompt builder and the system-prompt preview endpoint must call this. +// They used to decide separately and disagree: the preview counted tokens with +// tokencount.NewFallbackCounter() over the fully rendered XML — tags, paths +// and all, at roughly runes/2 — while the runtime estimated name+description at chars/4. +// On the same 18 skills that was 3087 against 944, so the preview reported search mode +// for an agent that was actually running inline. A preview whose whole purpose is to show +// the real prompt cannot use a different rule than the real prompt. +// +// The estimate deliberately mirrors BuildSummary()'s truncation (skillDescMaxLen=200 +// runes) rather than measuring the rendered string, so the decision costs no rendering. +func shouldInlineSkills(filtered []skills.Info) bool { + if len(filtered) == 0 { + return false + } + if len(filtered) > skillInlineMaxCount { + return false + } + // ~1 token per 4 chars for name+description, +10 for the XML tag overhead per entry. totalChars := 0 for _, s := range filtered { descLen := min(len(s.Description), 200) - totalChars += len(s.Name) + descLen + 10 // +10 for XML tags overhead + totalChars += len(s.Name) + descLen + 10 } - estimatedTokens := totalChars / 4 - - if len(filtered) <= skillInlineMaxCount && estimatedTokens <= skillInlineMaxTokens { - // Inline mode: build full XML summary - return l.skillsLoader.BuildSummary(ctx, allowList) - } - - // Search mode: no XML in prompt, agent uses skill_search tool - return "" + return totalChars/4 <= skillInlineMaxTokens } // resolvePinnedSkillsSummary builds XML for pinned skills only (always inline). diff --git a/internal/agent/preview_prompt.go b/internal/agent/preview_prompt.go index 26e4e45a..6e13e965 100644 --- a/internal/agent/preview_prompt.go +++ b/internal/agent/preview_prompt.go @@ -12,8 +12,8 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/bootstrap" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" - "github.com/nextlevelbuilder/goclaw/internal/tokencount" "github.com/nextlevelbuilder/goclaw/internal/tools" ) @@ -57,6 +57,9 @@ type PreviewDeps struct { SkillsLoader interface { BuildPinnedSummary(ctx context.Context, names []string) string BuildSummary(ctx context.Context, allowList []string) string + // FilterSkills feeds shouldInlineSkills, so the preview reaches the same + // inline-vs-search verdict as the live prompt builder. + FilterSkills(ctx context.Context, allowList []string) []skills.Info } // MCPLister provides store-based MCP tool info for preview (nil = skip). // When set, MCP tool descriptions are populated from configured servers @@ -304,13 +307,10 @@ func BuildPreviewPrompt(ctx context.Context, ag *store.AgentData, mode PromptMod } } - summary := deps.SkillsLoader.BuildSummary(ctx, skillAllowList) - if summary != "" { - tokens := tokencount.NewFallbackCounter().Count("claude-3", summary) - if tokens <= skillInlineMaxTokens { - skillsSummary = summary - } - // Over threshold → search-only mode (skillsSummary stays empty) + // Same decision function as the live prompt builder — see shouldInlineSkills. + // Over threshold → search-only mode (skillsSummary stays empty). + if shouldInlineSkills(deps.SkillsLoader.FilterSkills(ctx, skillAllowList)) { + skillsSummary = deps.SkillsLoader.BuildSummary(ctx, skillAllowList) } } diff --git a/internal/agent/preview_prompt_test.go b/internal/agent/preview_prompt_test.go index e860e4e3..403e113b 100644 --- a/internal/agent/preview_prompt_test.go +++ b/internal/agent/preview_prompt_test.go @@ -11,6 +11,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" ) @@ -42,6 +43,7 @@ func TestBuildPreviewPrompt_SkillsInline(t *testing.T) { r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{ SkillsLoader: &mockSkillsLoader{ summary: "\nGit operations\n", + infos: skillInfoN(1, 20), }, }) if !strings.Contains(r.Prompt, "") { @@ -51,14 +53,56 @@ func TestBuildPreviewPrompt_SkillsInline(t *testing.T) { func TestBuildPreviewPrompt_SkillsSearchMode(t *testing.T) { bigSummary := strings.Repeat("x", 10000) + // 100 skills at the 200-rune description cap: ~5300 estimated tokens, past the 3000 + // inline ceiling. The estimate is taken from the skill list, not from the rendered + // summary, so the count and description length are what push it over. r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{ - SkillsLoader: &mockSkillsLoader{summary: bigSummary}, + SkillsLoader: &mockSkillsLoader{summary: bigSummary, infos: skillInfoN(100, 200)}, }) if strings.Contains(r.Prompt, bigSummary) { t.Error("expected large summary to be excluded (search-only mode)") } } +// The preview endpoint exists to show the prompt the agent actually gets. It used to +// decide inline-vs-search with tokencount.NewFallbackCounter() over the rendered XML +// while the runtime estimated name+description at chars/4 — on 18 real skills that read +// 3087 against 944, so the preview claimed search mode for an agent running inline. +// Both paths now call shouldInlineSkills; this pins them to the same verdict. +func TestPreviewInlineDecisionMatchesRuntime(t *testing.T) { + for _, tc := range []struct { + name string + count int + descLen int + wantInline bool + }{ + {"empty", 0, 0, false}, + {"one small skill", 1, 20, true}, + // Count gate, isolated: descriptions stay tiny so the token estimate is far + // under the ceiling and only the count can decide. + {"at the count ceiling", skillInlineMaxCount, 10, true}, + {"one past the count ceiling", skillInlineMaxCount + 1, 10, false}, + // Token gate, isolated: the count stays under the ceiling, so a false verdict + // can only come from the token estimate. 58 x (8 + 200 + 10) / 4 = 3161 > 3000. + {"under the count ceiling, under the token ceiling", 40, 200, true}, + {"under the count ceiling, over the token ceiling", 58, 200, false}, + } { + t.Run(tc.name, func(t *testing.T) { + infos := skillInfoN(tc.count, tc.descLen) + if got := shouldInlineSkills(infos); got != tc.wantInline { + t.Fatalf("shouldInlineSkills = %v, want %v", got, tc.wantInline) + } + + loader := &mockSkillsLoader{summary: "marker", infos: infos} + r := BuildPreviewPrompt(context.Background(), baseAgent(), PromptFull, "", PreviewDeps{SkillsLoader: loader}) + gotInPreview := strings.Contains(r.Prompt, "marker") + if gotInPreview != tc.wantInline { + t.Fatalf("preview inlined = %v, want %v (must match shouldInlineSkills)", gotInPreview, tc.wantInline) + } + }) + } +} + func TestBuildPreviewPrompt_PinnedSkillsHybrid(t *testing.T) { ag := baseAgent() ag.OtherConfig = []byte(`{"pinned_skills":["deploy"]}`) @@ -77,6 +121,7 @@ func TestBuildPreviewPrompt_SkillAllowList(t *testing.T) { ag := baseAgent() loader := &mockSkillsLoader{ summary: "ok", + infos: []skills.Info{{Slug: "allowed-skill", Name: "allowed", Description: "ok"}}, } r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "user1", PreviewDeps{ SkillsLoader: loader, @@ -96,6 +141,7 @@ func TestBuildPreviewPrompt_SkillAccessStoreError(t *testing.T) { ag := baseAgent() loader := &mockSkillsLoader{ summary: "desc", + infos: skillInfoN(3, 20), } r := BuildPreviewPrompt(context.Background(), ag, PromptFull, "user1", PreviewDeps{ SkillsLoader: loader, @@ -104,8 +150,14 @@ func TestBuildPreviewPrompt_SkillAccessStoreError(t *testing.T) { if r.Prompt == "" { t.Fatal("expected non-empty prompt on SkillAccessStore error") } - if loader.capturedAllow == nil || len(loader.capturedAllow) != 0 { - t.Errorf("expected empty (non-nil) allow list on error, got %v", loader.capturedAllow) + // Fail closed: an access-store error must not fall back to showing every skill. + // The tag itself appears in the skill-loading protocol text regardless, so match on + // the summary body. + if strings.Contains(r.Prompt, "") { + t.Error("expected no skills in the prompt when the access store errors") + } + if loader.capturedAllow != nil { + t.Errorf("BuildSummary should not run for an empty allow list, got %v", loader.capturedAllow) } } diff --git a/internal/agent/preview_prompt_test_helpers_test.go b/internal/agent/preview_prompt_test_helpers_test.go index c0e68958..c5456134 100644 --- a/internal/agent/preview_prompt_test_helpers_test.go +++ b/internal/agent/preview_prompt_test_helpers_test.go @@ -2,7 +2,10 @@ package agent import ( "context" + "fmt" + "github.com/nextlevelbuilder/goclaw/internal/skills" "sort" + "strings" "github.com/google/uuid" @@ -46,16 +49,17 @@ type mockTool struct { desc string } -func (t *mockTool) Name() string { return t.name } -func (t *mockTool) Description() string { return t.desc } -func (t *mockTool) Parameters() map[string]any { return nil } +func (t *mockTool) Name() string { return t.name } +func (t *mockTool) Description() string { return t.desc } +func (t *mockTool) Parameters() map[string]any { return nil } func (t *mockTool) Execute(_ context.Context, _ map[string]any) *tools.Result { return nil } // mockSkillsLoader implements the widened SkillsLoader interface. type mockSkillsLoader struct { - pinned string // pre-built pinned XML - summary string // pre-built full summary - capturedAllow []string // set by BuildSummary for test assertions + pinned string // pre-built pinned XML + summary string // pre-built full summary + infos []skills.Info // what FilterSkills returns; drives inline-vs-search + capturedAllow []string // set by BuildSummary for test assertions } func (m *mockSkillsLoader) BuildPinnedSummary(_ context.Context, _ []string) string { @@ -67,6 +71,42 @@ func (m *mockSkillsLoader) BuildSummary(_ context.Context, allowList []string) s return m.summary } +// FilterSkills mirrors the real loader's allowList convention: nil means everything, +// an empty slice means nothing, a populated slice selects by slug. +func (m *mockSkillsLoader) FilterSkills(_ context.Context, allowList []string) []skills.Info { + if allowList == nil { + return m.infos + } + if len(allowList) == 0 { + return nil + } + allowed := make(map[string]bool, len(allowList)) + for _, s := range allowList { + allowed[s] = true + } + var out []skills.Info + for _, s := range m.infos { + if allowed[s.Slug] { + out = append(out, s) + } + } + return out +} + +// skillInfoN builds n skills whose combined name+description length puts the estimate +// either side of the inline threshold, depending on descLen. +func skillInfoN(n, descLen int) []skills.Info { + out := make([]skills.Info, 0, n) + for i := range n { + out = append(out, skills.Info{ + Slug: fmt.Sprintf("skill-%d", i), + Name: fmt.Sprintf("skill-%d", i), + Description: strings.Repeat("x", descLen), + }) + } + return out +} + // mockMCPLister implements MCPPreviewLister for testing. type mockMCPLister struct { tools []MCPToolPreviewInfo diff --git a/internal/agent/skill_slash_command_access_test.go b/internal/agent/skill_slash_command_access_test.go new file mode 100644 index 00000000..c5c14ebf --- /dev/null +++ b/internal/agent/skill_slash_command_access_test.go @@ -0,0 +1,98 @@ +package agent + +import ( + "context" + "strings" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" + "github.com/nextlevelbuilder/goclaw/internal/skills" +) + +func managedSkill(slug string) skills.Info { + return skills.Info{Slug: slug, Name: slug, Source: "managed", Description: slug + " description"} +} + +func fsSkill(slug, source string) skills.Info { + return skills.Info{Slug: slug, Name: slug, Source: source, Description: slug + " description"} +} + +// The inline block has always been filtered by visibility and agent +// grants, but slash activation read the loader's full list, so / could activate a +// managed skill the agent was never granted. +func TestSkillsReachableBySlash_GatesManagedSkills(t *testing.T) { + all := []skills.Info{managedSkill("granted"), managedSkill("ungranted")} + + if got := len(skillsReachableBySlash(all, nil)); got != 2 { + t.Errorf("nil allow list should not restrict, got %d skills", got) + } + if got := len(skillsReachableBySlash(all, []string{})); got != 0 { + t.Errorf("empty allow list should block every managed skill, got %d", got) + } + + only := skillsReachableBySlash(all, []string{"granted"}) + if len(only) != 1 || only[0].Slug != "granted" { + t.Errorf("expected only the granted skill, got %+v", only) + } +} + +// The allow list comes from a query over the `skills` table, so filesystem-tier skills +// have no row and would be filtered out by slug. Gating them would strand four of the +// loader's five tiers for slash while skill_search still reaches them unfiltered. +func TestSkillsReachableBySlash_LeavesFilesystemTiersAlone(t *testing.T) { + all := []skills.Info{ + managedSkill("managed-ungranted"), + fsSkill("workspace-skill", "workspace"), + fsSkill("project-skill", "agents-project"), + fsSkill("personal-skill", "agents-personal"), + fsSkill("global-skill", "global"), + fsSkill("builtin-skill", "builtin"), + } + + got := skillsReachableBySlash(all, []string{}) + var slugs []string + for _, s := range got { + slugs = append(slugs, s.Slug) + } + if len(got) != 5 { + t.Fatalf("expected the five non-managed skills to survive, got %v", slugs) + } + for _, s := range got { + if s.Source == "managed" { + t.Errorf("ungranted managed skill leaked through: %s", s.Slug) + } + } +} + +// End-to-end through the resolver: a managed skill outside the allow list must not +// activate, and must not be disclosed by the not-found suggestions, /list-skills or +// /help either — blocking activation alone would still leak the name and description. +func TestResolveSkillSlashCommand_ManagedSkillOutsideAllowList(t *testing.T) { + loader := newManagedSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", SuggestNotFound: new(true)} + + granted := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/managed-only build a page") + if granted.Kind != skillSlashCommandActivate { + t.Fatalf("nil allow list should activate, got kind=%v", granted.Kind) + } + + denied := resolveSkillSlashCommand(context.Background(), loader, []string{"something-else"}, cfg, "/managed-only build a page") + if denied.Kind == skillSlashCommandActivate { + t.Fatalf("managed skill outside the allow list must not activate") + } + + suggest := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/managed-onl build") + if strings.Contains(suggest.Guidance, "managed-only") { + t.Errorf("suggestions disclosed an ungranted managed skill: %q", suggest.Guidance) + } + + list := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/list-skills") + if strings.Contains(list.Guidance, "managed-only") { + t.Errorf("/list-skills disclosed an ungranted managed skill: %q", list.Guidance) + } + + help := resolveSkillSlashCommand(context.Background(), loader, []string{}, cfg, "/help managed-only") + if help.Kind == skillSlashCommandHelp { + t.Error("/help described an ungranted managed skill") + } +} diff --git a/internal/agent/skill_slash_command_matching.go b/internal/agent/skill_slash_command_matching.go index 68a4bb1c..c1505310 100644 --- a/internal/agent/skill_slash_command_matching.go +++ b/internal/agent/skill_slash_command_matching.go @@ -3,6 +3,8 @@ package agent import ( "sort" "strings" + "unicode" + "unicode/utf8" "github.com/nextlevelbuilder/goclaw/internal/skills" ) @@ -22,9 +24,7 @@ func parseSkillSlashCommand(message, prefix string) (parsedSkillSlashCommand, bo if after == "" || looksLikePath(after) { return parsedSkillSlashCommand{}, false } - first, rest, _ := strings.Cut(after, " ") - first = strings.TrimSpace(first) - rest = strings.TrimSpace(rest) + first, rest := cutFirstField(after) switch strings.ToLower(first) { case "list-skills": return parsedSkillSlashCommand{verb: "list-skills"}, true @@ -44,16 +44,38 @@ func parseSkillSlashCommand(message, prefix string) (parsedSkillSlashCommand, bo } func looksLikePath(value string) bool { - first, _, _ := strings.Cut(value, " ") + first, _ := cutFirstField(value) return strings.Contains(first, "/") || strings.Contains(first, "\\") || strings.Contains(first, ".") } -func firstWord(value string) (string, string) { - first, rest, ok := strings.Cut(strings.TrimSpace(value), " ") - if !ok { - return strings.TrimSpace(value), "" +// cutFirstField splits value at the first run of whitespace and returns the leading +// field plus the trimmed remainder. +// +// This replaces strings.Cut(value, " "), which split on a literal space only. A user who +// types the slash command and presses Enter before the rest of the message sends +// "/ck:git\nreview the diff"; the old split yielded the target "ck:git\nreview", which +// matches no skill. The reply was "skill not found" followed by near-matches that +// included the skill they had just named — the similarity fallback searched the mangled +// string and still landed beside it. Multi-line messages are normal in Slack and +// Telegram, so this was reachable in ordinary use. +func cutFirstField(value string) (string, string) { + value = strings.TrimSpace(value) + i := strings.IndexFunc(value, unicode.IsSpace) + if i < 0 { + return value, "" } - return strings.TrimSpace(first), strings.TrimSpace(rest) + return value[:i], strings.TrimSpace(value[i:]) +} + +// hasFieldPrefix reports whether raw begins with value followed by whitespace, i.e. +// value occupies a whole leading field rather than merely being a string prefix. +// Both arguments are expected to be lowercased by the caller. +func hasFieldPrefix(raw, value string) bool { + if !strings.HasPrefix(raw, value) || len(raw) == len(value) { + return false + } + r, _ := utf8.DecodeRuneInString(raw[len(value):]) + return unicode.IsSpace(r) } func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skills.Info, bool, string) { @@ -69,7 +91,7 @@ func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skill } var matches []candidate lowerRaw := strings.ToLower(raw) - partialTarget, partialRemainder := firstWord(raw) + partialTarget, partialRemainder := cutFirstField(raw) lowerPartialTarget := strings.ToLower(partialTarget) for _, skill := range all { for _, value := range []string{skill.Slug, skill.Name} { @@ -82,7 +104,7 @@ func matchSkillCommandTarget(all []skills.Info, raw string, partial bool) (skill matches = append(matches, candidate{info: skill, matchText: value, score: len([]rune(value))}) continue } - if strings.HasPrefix(lowerRaw, lowerValue+" ") { + if hasFieldPrefix(lowerRaw, lowerValue) { remainder := trimMatchedSkillCommandPrefix(raw, value) matches = append(matches, candidate{info: skill, matchText: value, remainder: remainder, score: len([]rune(value))}) continue diff --git a/internal/agent/skill_slash_command_parsing_test.go b/internal/agent/skill_slash_command_parsing_test.go new file mode 100644 index 00000000..98b97076 --- /dev/null +++ b/internal/agent/skill_slash_command_parsing_test.go @@ -0,0 +1,95 @@ +package agent + +import ( + "context" + "testing" + + "github.com/nextlevelbuilder/goclaw/internal/config" +) + +// The tokenizer used to split the slash command off with strings.Cut(after, " ") — a +// literal space. Typing the command and pressing Enter before the rest of the message +// produced the target "ck:git\nreview", which matches no skill, and the user was told the +// skill did not exist while it sat in the suggestion list right below. Multi-line +// messages are ordinary in Slack and Telegram, so this was reachable in normal use. +func TestParseSkillSlashCommand_SeparatorIsAnyWhitespace(t *testing.T) { + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + for _, tc := range []struct { + name string + message string + wantTarget string + wantRest string + }{ + {"space", "/ck:git review the diff", "ck:git", "review the diff"}, + {"newline", "/ck:git\nreview the diff", "ck:git", "review the diff"}, + {"crlf", "/ck:git\r\nreview the diff", "ck:git", "review the diff"}, + {"blank line between", "/ck:git\n\nreview the diff", "ck:git", "review the diff"}, + {"tab", "/ck:git\treview the diff", "ck:git", "review the diff"}, + {"no arguments", "/ck:git", "ck:git", ""}, + {"trailing newline only", "/ck:git\n", "ck:git", ""}, + } { + t.Run(tc.name, func(t *testing.T) { + parsed, ok := parseSkillSlashCommand(tc.message, cfg.EffectivePrefix()) + if !ok { + t.Fatalf("parse failed for %q", tc.message) + } + if parsed.target != tc.wantTarget { + t.Errorf("target = %q, want %q", parsed.target, tc.wantTarget) + } + if parsed.rest != tc.wantRest { + t.Errorf("rest = %q, want %q", parsed.rest, tc.wantRest) + } + }) + } +} + +// The verb forms split on the same separator. +func TestParseSkillSlashCommand_VerbsAcceptNewline(t *testing.T) { + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + parsed, ok := parseSkillSlashCommand("/help\nck:git", cfg.EffectivePrefix()) + if !ok || parsed.verb != "help" || parsed.target != "ck:git" { + t.Fatalf("help: got verb=%q target=%q ok=%v", parsed.verb, parsed.target, ok) + } + + parsed, ok = parseSkillSlashCommand("/use\nck:git and then stop", cfg.EffectivePrefix()) + if !ok || parsed.verb != "use" || parsed.rest != "ck:git and then stop" { + t.Fatalf("use: got verb=%q rest=%q ok=%v", parsed.verb, parsed.rest, ok) + } +} + +// A newline-separated command must reach the same skill as the space-separated one. +func TestResolveSkillSlashCommand_NewlineActivatesSameSkill(t *testing.T) { + loader := newSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + spaced := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design build a landing page") + newlined := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design\nbuild a landing page") + + if spaced.Kind != skillSlashCommandActivate { + t.Fatalf("space form did not activate: kind=%v", spaced.Kind) + } + if newlined.Kind != spaced.Kind { + t.Fatalf("newline form kind = %v, want %v", newlined.Kind, spaced.Kind) + } + if newlined.Skill.Slug != spaced.Skill.Slug { + t.Fatalf("newline form matched %q, want %q", newlined.Skill.Slug, spaced.Skill.Slug) + } + if newlined.RemainingPrompt != spaced.RemainingPrompt { + t.Fatalf("newline remainder = %q, want %q", newlined.RemainingPrompt, spaced.RemainingPrompt) + } +} + +// hasFieldPrefix must not treat a longer skill name as a match for a shorter one. +func TestMatchSkillCommandTarget_RequiresWholeField(t *testing.T) { + loader := newSlashTestLoader(t) + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} + + // "frontend-design-extra" is not a skill; without partial matching this must not + // resolve to "frontend-design" merely because that is a string prefix of it. + res := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/frontend-design-extra do a thing") + if res.Kind == skillSlashCommandActivate { + t.Fatalf("string-prefix match should not activate, got %q", res.Skill.Slug) + } +} diff --git a/internal/agent/skill_slash_commands.go b/internal/agent/skill_slash_commands.go index b9da5d97..6fb07291 100644 --- a/internal/agent/skill_slash_commands.go +++ b/internal/agent/skill_slash_commands.go @@ -29,7 +29,13 @@ type skillSlashCommandResult struct { } func (l *Loop) applySkillSlashCommand(ctx context.Context, req *RunRequest, message, extraPrompt string, skillFilter []string) (string, string, []string) { - result := resolveSkillSlashCommand(ctx, l.skillsLoader, l.resolveSkillSlashCommandConfig(ctx), message) + // l.skillAllowList is the visibility + grant filter computed when the agent was + // resolved (internal/agent/resolver.go). The inline block already + // honours it; the slash path did not, so / activated any skill the loader could + // see — including internal skills never granted to this agent, and skills belonging to + // another tenant's store if the loader had them. Filtering here also stops the + // not-found suggestions from disclosing that such a skill exists. + result := resolveSkillSlashCommand(ctx, l.skillsLoader, l.skillAllowList, l.resolveSkillSlashCommandConfig(ctx), message) if result.Kind == skillSlashCommandNone { return message, extraPrompt, skillFilter } @@ -76,7 +82,10 @@ func (l *Loop) resolveSkillSlashCommandConfig(ctx context.Context) config.SkillS return cfg } -func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg config.SkillSlashCommandConfig, message string) skillSlashCommandResult { +// resolveSkillSlashCommand matches a slash command against the skills this agent may +// use. allowList follows the loader's convention: nil means every skill, an empty slice +// means none, and a populated slice is an explicit set of slugs. +func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, allowList []string, cfg config.SkillSlashCommandConfig, message string) skillSlashCommandResult { if loader == nil || !cfg.EffectiveEnabled() { return skillSlashCommandResult{Kind: skillSlashCommandNone} } @@ -84,7 +93,7 @@ func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg co if !ok { return skillSlashCommandResult{Kind: skillSlashCommandNone} } - all := loader.ListSkills(ctx) + all := skillsReachableBySlash(loader.ListSkills(ctx), allowList) switch parsed.verb { case "list-skills": return skillSlashCommandResult{Kind: skillSlashCommandList, Guidance: buildSkillSlashListGuidance(all)} @@ -101,6 +110,40 @@ func resolveSkillSlashCommand(ctx context.Context, loader *skills.Loader, cfg co } } +// skillsReachableBySlash applies the agent's allow list to the skills the DB governs, +// and lets the rest through. +// +// The allow list comes from SkillAccessStore.ListAccessible, which queries the `skills` +// table only (internal/store/pg/skills_grants.go:410). Filesystem-tier skills — the +// workspace, .agents and ~/.agents/~/.goclaw directories of the five-tier loader — have +// no row there, so filtering every skill against the list would make those four tiers +// unreachable by slash while `skill_search` still finds them +// (internal/tools/skill_search.go:81 calls ListSkills unfiltered). Gating only the +// managed tier closes the grant bypass without stranding skills an operator placed on +// disk deliberately. +// +// Builtin skills are seeded into the table with is_system = true and ListAccessible +// returns those unconditionally, so they stay reachable either way. +// +// A nil allow list means no restriction; an empty one means no managed skill is allowed. +func skillsReachableBySlash(all []skills.Info, allowList []string) []skills.Info { + if allowList == nil { + return all + } + allowed := make(map[string]bool, len(allowList)) + for _, slug := range allowList { + allowed[slug] = true + } + out := make([]skills.Info, 0, len(all)) + for _, s := range all { + if s.Source == "managed" && !allowed[s.Slug] { + continue + } + out = append(out, s) + } + return out +} + func resolveSkillActivation(ctx context.Context, loader *skills.Loader, all []skills.Info, raw string, cfg config.SkillSlashCommandConfig) skillSlashCommandResult { skill, matched, remainder := matchSkillCommandTarget(all, raw, cfg.EffectivePartialMatching()) if !matched { diff --git a/internal/agent/skill_slash_commands_test.go b/internal/agent/skill_slash_commands_test.go index d67c0d7b..be911cfc 100644 --- a/internal/agent/skill_slash_commands_test.go +++ b/internal/agent/skill_slash_commands_test.go @@ -9,11 +9,12 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/config" "github.com/nextlevelbuilder/goclaw/internal/skills" + "github.com/nextlevelbuilder/goclaw/internal/store" ) func TestResolveSkillSlashCommandExactSlug(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/frontend-design build a landing page") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/frontend-design build a landing page") if result.Kind != skillSlashCommandActivate { t.Fatalf("kind = %v, want activate", result.Kind) @@ -31,7 +32,7 @@ func TestResolveSkillSlashCommandExactSlug(t *testing.T) { func TestResolveSkillSlashCommandExactNameUseSyntax(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/use Frontend Design build a landing page") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/use Frontend Design build a landing page") if result.Kind != skillSlashCommandActivate { t.Fatalf("kind = %v, want activate", result.Kind) @@ -47,12 +48,12 @@ func TestResolveSkillSlashCommandExactNameUseSyntax(t *testing.T) { func TestResolveSkillSlashCommandPartialMatchRequiresUniqueEnabled(t *testing.T) { loader := newSlashTestLoader(t) - disabled := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, "/front build") + disabled := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, "/front build") if disabled.Kind != skillSlashCommandUnknown { t.Fatalf("disabled partial kind = %v, want unknown", disabled.Kind) } - enabled := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/", PartialMatching: true}, "/front build") + enabled := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", PartialMatching: true}, "/front build") if enabled.Kind != skillSlashCommandActivate { t.Fatalf("enabled partial kind = %v, want activate", enabled.Kind) } @@ -64,7 +65,7 @@ func TestResolveSkillSlashCommandPartialMatchRequiresUniqueEnabled(t *testing.T) func TestResolveSkillSlashCommandFalsePositives(t *testing.T) { loader := newSlashTestLoader(t) for _, msg := range []string{"/home/user/project", "/etc/config.yaml", "https://example.com/path", "regular prompt"} { - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"}, msg) + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"}, msg) if result.Kind != skillSlashCommandNone { t.Fatalf("%q kind = %v, want none", msg, result.Kind) } @@ -73,9 +74,9 @@ func TestResolveSkillSlashCommandFalsePositives(t *testing.T) { func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { loader := newSlashTestLoader(t) - cfg := config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/"} + cfg := config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/"} - list := resolveSkillSlashCommand(context.Background(), loader, cfg, "/list-skills") + list := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/list-skills") if list.Kind != skillSlashCommandList { t.Fatalf("list kind = %v, want list", list.Kind) } @@ -83,7 +84,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { t.Fatalf("list guidance missing skills: %s", list.Guidance) } - help := resolveSkillSlashCommand(context.Background(), loader, cfg, "/help frontend-design") + help := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/help frontend-design") if help.Kind != skillSlashCommandHelp { t.Fatalf("help kind = %v, want help", help.Kind) } @@ -91,7 +92,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { t.Fatalf("unexpected help result: %#v", help) } - helpByName := resolveSkillSlashCommand(context.Background(), loader, cfg, "/help Frontend Design") + helpByName := resolveSkillSlashCommand(context.Background(), loader, nil, cfg, "/help Frontend Design") if helpByName.Kind != skillSlashCommandHelp { t.Fatalf("help by name kind = %v, want help", helpByName.Kind) } @@ -102,7 +103,7 @@ func TestResolveSkillSlashCommandListAndHelp(t *testing.T) { func TestResolveSkillSlashCommandSuggestsUnknown(t *testing.T) { loader := newSlashTestLoader(t) - result := resolveSkillSlashCommand(context.Background(), loader, config.SkillSlashCommandConfig{Enabled: boolPtr(true), Prefix: "/", SuggestNotFound: boolPtr(true)}, "/fronted build") + result := resolveSkillSlashCommand(context.Background(), loader, nil, config.SkillSlashCommandConfig{Enabled: new(true), Prefix: "/", SuggestNotFound: new(true)}, "/fronted build") if result.Kind != skillSlashCommandUnknown { t.Fatalf("kind = %v, want unknown", result.Kind) @@ -112,8 +113,9 @@ func TestResolveSkillSlashCommandSuggestsUnknown(t *testing.T) { } } +//go:fix inline func boolPtr(v bool) *bool { - return &v + return new(v) } func newSlashTestLoader(t *testing.T) *skills.Loader { @@ -125,6 +127,21 @@ func newSlashTestLoader(t *testing.T) *skills.Loader { return skills.NewLoader("", root, "") } +// newManagedSlashTestLoader builds a loader whose skills come from the managed +// (DB-backed) tier, so the allow-list gate applies to them. The filesystem tiers are +// deliberately left empty: skillsReachableBySlash only gates Source == "managed". +func newManagedSlashTestLoader(t *testing.T) *skills.Loader { + t.Helper() + t.Setenv("GOCLAW_DISABLE_PERSONAL_SKILLS", "1") + dataDir := t.TempDir() + storeDir := config.TenantSkillsStoreDir(dataDir, store.MasterTenantID, "") + // Managed layout is ///SKILL.md. + writeSkill(t, filepath.Join(storeDir, "managed-only"), "1", "managed-only", "A managed skill.", "Body.") + loader := skills.NewLoader("", "", "") + loader.SetManagedDir(dataDir) + return loader +} + func writeSkill(t *testing.T, root, slug, name, description, body string) { t.Helper() dir := filepath.Join(root, slug) diff --git a/internal/http/agents.go b/internal/http/agents.go index 8356ea34..5debc69b 100644 --- a/internal/http/agents.go +++ b/internal/http/agents.go @@ -20,6 +20,7 @@ import ( "github.com/nextlevelbuilder/goclaw/internal/i18n" "github.com/nextlevelbuilder/goclaw/internal/permissions" "github.com/nextlevelbuilder/goclaw/internal/providers" + "github.com/nextlevelbuilder/goclaw/internal/skills" "github.com/nextlevelbuilder/goclaw/internal/store" "github.com/nextlevelbuilder/goclaw/internal/tools" "github.com/nextlevelbuilder/goclaw/pkg/protocol" @@ -106,6 +107,7 @@ type ToolPreviewLister interface { type SkillPreviewBuilder interface { BuildPinnedSummary(ctx context.Context, names []string) string BuildSummary(ctx context.Context, allowList []string) string + FilterSkills(ctx context.Context, allowList []string) []skills.Info } // SetPreviewDeps attaches optional dependencies for system prompt preview. diff --git a/internal/http/skills_import.go b/internal/http/skills_import.go index 57744701..166195df 100644 --- a/internal/http/skills_import.go +++ b/internal/http/skills_import.go @@ -83,6 +83,66 @@ type SkillsImportSummary struct { } // doSkillsImport parses the skills tar.gz and creates skills + writes files + applies grants. +// writeImportedSkillAuxFiles writes a skill's non-SKILL.md files under skillDir, +// preserving their directory structure. Paths are sanitised per segment, so an archive +// entry such as "../../etc/passwd" collapses to "etc/passwd" and stays inside skillDir. +// A file that cannot be written is logged and skipped: a missing reference degrades the +// skill, but it should not abort an import that is otherwise sound. +// reservedSkillFiles are handled by name earlier in the import and must never be +// written from the auxiliary set. sanitizeRelPath collapses "./SKILL.md" to "SKILL.md", +// and that name does not match the switch above (which compares the raw archive path), +// so without this guard a crafted entry would land in aux and then overwrite the +// SKILL.md that GuardSkillContent had already scanned — the scan would pass on one file +// while a different one reached disk. +var reservedSkillFiles = map[string]bool{ + "SKILL.md": true, + "metadata.json": true, + "grants.jsonl": true, +} + +// maxImportedAuxFiles bounds how many extra files one skill may carry. Real skills hold +// a handful of references; a four-figure count is an archive doing something else. +const maxImportedAuxFiles = 512 + +func writeImportedSkillAuxFiles(skillDir, slug string, aux map[string][]byte) { + written := 0 + skipped := 0 + for rel, data := range aux { + // Directory entries carry no content and a trailing separator. Writing them as + // empty files would, depending on map iteration order, occupy the name a real + // directory needs and silently drop everything beneath it. + if strings.HasSuffix(rel, "/") { + continue + } + clean := sanitizeRelPath(rel) + if clean == "" { + continue + } + if reservedSkillFiles[clean] { + slog.Warn("security.skills.import_reserved_path_rejected", "slug", slug, "entry", rel, "resolved", clean) + skipped++ + continue + } + if written >= maxImportedAuxFiles { + skipped++ + continue + } + dest := filepath.Join(skillDir, filepath.FromSlash(clean)) + if err := os.MkdirAll(filepath.Dir(dest), 0755); err != nil { + slog.Warn("skills.import: mkdir for skill file", "slug", slug, "path", clean, "error", err) + continue + } + if err := os.WriteFile(dest, data, 0644); err != nil { + slog.Warn("skills.import: write skill file", "slug", slug, "path", clean, "error", err) + continue + } + written++ + } + if skipped > 0 { + slog.Warn("skills.import: skill files skipped", "slug", slug, "written", written, "skipped", skipped, "limit", maxImportedAuxFiles) + } +} + func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID string, progressFn func(ProgressEvent)) (*SkillsImportSummary, error) { entries, err := readTarGzEntries(r) if err != nil { @@ -94,6 +154,9 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID metadata []byte skillMD []byte grants []byte + // aux holds every other file under skills//, keyed by its path relative + // to the skill root — references/errors.md, scripts/run.sh, assets/, and so on. + aux map[string][]byte } bySlug := make(map[string]*skillEntry) @@ -121,6 +184,16 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID bySlug[slug].skillMD = data case "grants.jsonl": bySlug[slug].grants = data + default: + // Everything else in the skill directory. The export side archives the whole + // tree (addSkillDirectoryToArchive), but import used to recognise only the + // three names above and silently discard the rest — so a skill whose SKILL.md + // cites references/*.md arrived without them and failed at the first read. + // The switch had no default, so nothing logged and the import reported success. + if bySlug[slug].aux == nil { + bySlug[slug].aux = make(map[string][]byte) + } + bySlug[slug].aux[file] = data } } @@ -190,6 +263,7 @@ func (h *SkillsHandler) doSkillsImport(ctx context.Context, r io.Reader, userID slog.Warn("skills.import: write SKILL.md", "slug", slug, "error", err) } } + writeImportedSkillAuxFiles(skillDir, slug, entry.aux) skillID = uuid.Must(uuid.NewV7()) visibility := meta.Visibility diff --git a/internal/http/skills_import_aux_files_test.go b/internal/http/skills_import_aux_files_test.go new file mode 100644 index 00000000..319499ee --- /dev/null +++ b/internal/http/skills_import_aux_files_test.go @@ -0,0 +1,197 @@ +package http + +import ( + "fmt" + "os" + "path/filepath" + "strings" + "testing" +) + +// Export archives the whole skill directory, but import used to recognise only +// metadata.json, SKILL.md and grants.jsonl and silently discard everything else — the +// switch had no default. A skill whose SKILL.md cites references/*.md arrived without +// them, the import reported success, and the skill failed at the first read. +func TestWriteImportedSkillAuxFiles_PreservesTree(t *testing.T) { + dir := t.TempDir() + + aux := map[string][]byte{ + "references/errors.md": []byte("# Errors"), + "references/nested/deep.md": []byte("# Deep"), + "scripts/run.sh": []byte("#!/bin/sh\n"), + "assets/logo.svg": []byte(""), + } + writeImportedSkillAuxFiles(dir, "demo", aux) + + for rel, want := range aux { + got, err := os.ReadFile(filepath.Join(dir, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("%s: %v", rel, err) + } + if string(got) != string(want) { + t.Errorf("%s: content = %q, want %q", rel, got, want) + } + } +} + +// Archive entry names are attacker-controlled. Each segment is sanitised, so a traversal +// attempt collapses to a relative path and the write stays inside the skill directory. +func TestWriteImportedSkillAuxFiles_ContainsTraversal(t *testing.T) { + root := t.TempDir() + skillDir := filepath.Join(root, "skill") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatal(err) + } + outside := filepath.Join(root, "outside.txt") + + writeImportedSkillAuxFiles(skillDir, "demo", map[string][]byte{ + "../outside.txt": []byte("escaped"), + "../../outside.txt": []byte("escaped"), + "references/../../x.md": []byte("escaped"), + "./references/ok.md": []byte("fine"), + }) + + if _, err := os.Stat(outside); !os.IsNotExist(err) { + t.Fatalf("traversal escaped the skill directory: %v", err) + } + + // The sanitised forms land inside skillDir instead. + if _, err := os.Stat(filepath.Join(skillDir, "outside.txt")); err != nil { + t.Errorf("expected the sanitised path inside the skill dir: %v", err) + } + if got, err := os.ReadFile(filepath.Join(skillDir, "references", "ok.md")); err != nil || string(got) != "fine" { + t.Errorf("ordinary path broke: got %q err %v", got, err) + } + + // Nothing may sit above skillDir. + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + for _, e := range entries { + if e.Name() != "skill" { + t.Errorf("unexpected entry written above the skill dir: %s", e.Name()) + } + } +} + +// A path that sanitises to nothing is skipped rather than written as the directory itself. +func TestWriteImportedSkillAuxFiles_SkipsEmptyPaths(t *testing.T) { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "..": []byte("x"), + ".": []byte("x"), + "/": []byte("x"), + "///": []byte("x"), + }) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + names := make([]string, 0, len(entries)) + for _, e := range entries { + names = append(names, e.Name()) + } + t.Errorf("expected nothing written, got %s", strings.Join(names, ", ")) + } +} + +// A nil or empty map is a no-op, not a panic — most skills have no extra files. +func TestWriteImportedSkillAuxFiles_NoFiles(t *testing.T) { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", nil) + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{}) + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Errorf("expected an empty directory, got %d entries", len(entries)) + } +} + +// SKILL.md is scanned by GuardSkillContent before anything is written. The switch that +// routes archive entries compares the raw path, so "./SKILL.md" does not match it and +// lands in the auxiliary set — where sanitizeRelPath collapses it back to "SKILL.md". +// Writing that would replace the scanned file with an unscanned one. +func TestWriteImportedSkillAuxFiles_RefusesReservedNames(t *testing.T) { + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, "SKILL.md"), []byte("scanned"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "metadata.json"), []byte(`{"ok":true}`), 0o644); err != nil { + t.Fatal(err) + } + + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "./SKILL.md": []byte("unscanned"), + "SKILL.md/": []byte("unscanned"), + ".//SKILL.md": []byte("unscanned"), + "./metadata.json": []byte(`{"ok":false}`), + "./grants.jsonl": []byte("forged"), + "references/ok.md": []byte("fine"), + }) + + got, err := os.ReadFile(filepath.Join(dir, "SKILL.md")) + if err != nil || string(got) != "scanned" { + t.Errorf("SKILL.md was overwritten from the auxiliary set: got %q err %v", got, err) + } + got, err = os.ReadFile(filepath.Join(dir, "metadata.json")) + if err != nil || string(got) != `{"ok":true}` { + t.Errorf("metadata.json was overwritten: got %q err %v", got, err) + } + if _, err := os.Stat(filepath.Join(dir, "grants.jsonl")); !os.IsNotExist(err) { + t.Errorf("grants.jsonl was written from the auxiliary set: %v", err) + } + // An ordinary path in the same batch must still land. + if got, err := os.ReadFile(filepath.Join(dir, "references", "ok.md")); err != nil || string(got) != "fine" { + t.Errorf("ordinary path broke: got %q err %v", got, err) + } +} + +// A tar directory entry has a trailing separator and no content. Written as a file it +// would take the name a real directory needs, and whether that happens depends on map +// iteration order — so the failure would be intermittent. +func TestWriteImportedSkillAuxFiles_IgnoresDirectoryEntries(t *testing.T) { + for range 20 { + dir := t.TempDir() + writeImportedSkillAuxFiles(dir, "demo", map[string][]byte{ + "references/": {}, + "references/errors.md": []byte("# Errors"), + "assets/": {}, + "assets/logo.svg": []byte(""), + }) + + got, err := os.ReadFile(filepath.Join(dir, "references", "errors.md")) + if err != nil || string(got) != "# Errors" { + t.Fatalf("directory entry blocked a real file: got %q err %v", got, err) + } + info, err := os.Stat(filepath.Join(dir, "references")) + if err != nil || !info.IsDir() { + t.Fatalf("references should be a directory: %v", err) + } + } +} + +// An archive with an implausible number of extra files is capped, and the cap is logged +// rather than applied silently. +func TestWriteImportedSkillAuxFiles_CapsFileCount(t *testing.T) { + dir := t.TempDir() + aux := make(map[string][]byte, maxImportedAuxFiles+50) + for i := range maxImportedAuxFiles + 50 { + aux[fmt.Sprintf("references/f%d.md", i)] = []byte("x") + } + writeImportedSkillAuxFiles(dir, "demo", aux) + + entries, err := os.ReadDir(filepath.Join(dir, "references")) + if err != nil { + t.Fatal(err) + } + if len(entries) > maxImportedAuxFiles { + t.Errorf("wrote %d files, cap is %d", len(entries), maxImportedAuxFiles) + } + if len(entries) != maxImportedAuxFiles { + t.Errorf("expected exactly the cap to be written, got %d", len(entries)) + } +} diff --git a/internal/tools/security_policy_inherit.go b/internal/tools/security_policy_inherit.go new file mode 100644 index 00000000..099693e5 --- /dev/null +++ b/internal/tools/security_policy_inherit.go @@ -0,0 +1,75 @@ +package tools + +// Security policy inheritance for spawned subagents. +// +// A subagent gets a cloned registry, but the file and exec tools inside it are +// constructed fresh (cmd/gateway_agents.go). A fresh tool carries none of the hardening +// the gateway applied to the parent's instances at startup — no path denials, no +// exemptions, no shell deny-group toggles, no command keyword allowlist. The result is +// that spawning a subagent widens what the agent can reach: the parent is blocked from +// the data dir and the internal databases, the subagent is not. +// +// These methods copy that policy across so the subagent starts no wider than its parent. +// Inheriting from the live parent instance keeps one source of truth — a deny path added +// at startup, or reloaded later via config pub/sub, reaches subagents without a second +// wiring site that can drift. +// +// Allow-prefixes are inherited too: they are what make the parent's deny roots usable at +// all (the skills store sits under the denied data dir), so copying denials without them +// would leave the subagent unable to read the skills it is told to use. + +import "maps" + +// InheritSecurityPolicy copies path denials, deny exemptions, global shell deny-group +// toggles and the command keyword allowlist from parent onto t. +func (t *ExecTool) InheritSecurityPolicy(parent *ExecTool) { + if t == nil || parent == nil || t == parent { + return + } + // DenyPaths rebuilds the compiled patterns from the raw roots, so pass the roots + // rather than copying pathDenyPatterns — that keeps the slash-variant expansion and + // the pathDenyRoots bookkeeping in one place. + t.DenyPaths(parent.pathDenyRoots...) + t.AllowPathExemptions(parent.denyExemptions...) + + parent.policyMu.RLock() + groups := parent.globalDenyGroups + rules := slicesClone(parent.commandKeywordAllowlist) + parent.policyMu.RUnlock() + + if len(groups) > 0 { + copied := make(map[string]bool, len(groups)) + maps.Copy(copied, groups) + t.SetGlobalShellDenyGroups(copied) + } + if len(rules) > 0 { + t.SetCommandKeywordAllowlist(rules) + } +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *ReadFileTool) InheritPathPolicy(parent *ReadFileTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *WriteFileTool) InheritPathPolicy(parent *WriteFileTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +} + +// InheritPathPolicy copies allowed and denied path prefixes from parent onto t. +func (t *ListFilesTool) InheritPathPolicy(parent *ListFilesTool) { + if t == nil || parent == nil || t == parent { + return + } + t.AllowPaths(parent.allowedPrefixes...) + t.DenyPaths(parent.deniedPrefixes...) +}