diff --git a/docs/plans/goose-adoption-plan.md b/docs/plans/goose-adoption-plan.md new file mode 100644 index 00000000..3364f985 --- /dev/null +++ b/docs/plans/goose-adoption-plan.md @@ -0,0 +1,159 @@ +# Goose Adoption Plan + +Status: Proposed + +Source: `https://github.com/aaif-goose/goose` (Apache-2.0, Rust workspace, +Linux Foundation / Agentic AI Foundation) + +## Executive Decision + +Goose's audit against hawk found that most of its runtime concepts already have +a native hawk implementation (providers via eyrie, sessions, MCP, ACP, skills, +native sandboxing, permissions). The genuinely novel, hawk-relevant ideas are +adopted here in Go, without copying Rust code or weakening hawk's native +sandboxing model. + +## Existing Hawk Capabilities + +| Goose package/concept | Hawk implementation | Decision | +|---|---|---| +| Provider abstraction (~36) | `external/eyrie` (28 built-in + 75+ live) | Keep hawk | +| Sessions (SQLite WAL) | `internal/session` JSONL+WAL+zstd + `external/trace` | Keep hawk | +| MCP (client+server) | `internal/mcp` + `external/hawk-mcpkit` | Keep hawk | +| ACP | `internal/acp` | Keep hawk | +| Extensions/skills | `internal/plugin`, skills registry | Keep hawk | +| OS sandboxing | `internal/sandbox` seatbelt/landlock/seccomp/ACL | **hawk ahead** (goose has none) | +| OSV malware gate | `internal/permissions/osv_checker.go` (`CheckCommand`/`CheckPackage`) | Keep hawk | +| Hints / AGENTS.md | `internal/config` AGENTS.md loader | **Adopt** @file references + subdir hints | +| Context compaction | `external/tok` + `internal/engine/compaction` | **Adopt** structured-summary retry ladder | +| Extension env safety | none (only OSV gate) | **Adopt** disallowed-env-var filter | +| Download manager | `internal/container`, `tool` | Out of scope | + +## Priority Model + +- **P0:** Security-relevant, bounded, hawk-native adoptions. +- **P1:** High-value product improvements. +- **Defer:** Larger or cross-cutting changes needing an RFC. + +## P0: Disallowed Env-Var Filter for Package/Extension Launch + +### Goal + +Prevent command/library hijacking when spawning `uvx`/`npx`/CLI-based extension +or MCP stdio processes by filtering dangerous environment overrides +(`PATH`, `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_*`, `PYTHONPATH`, +`NODE_OPTIONS`, `GOROOT`, etc.) from the child environment. + +### Scope and ownership + +- Primary: `internal/sandbox` (or `internal/permissions` next to the OSV + checker) — a `sanitizeEnv` / `SafeEnv` helper. +- Consumers: `internal/mcp` stdio launch path and `internal/plugin` package + execution. +- No changes to `external/eyrie`. + +### Required behavior + +1. Define the disallowed env-var set (path/library/python/node/go hijacking + vectors), mirroring goose `extension.rs::Envs`. +2. Provide a helper that, given a proposed env map, drops disallowed keys and + returns the sanitized map plus the list of removed keys. +3. Apply it before spawning extension/MCP stdio subprocesses. +4. Log removed keys at debug/warn (not their values). + +### Acceptance criteria + +- A config that sets `PATH`/`LD_PRELOAD`/`PYTHONPATH` cannot influence the child + process. +- Sanitization is unit-tested for the full disallowed set and for allow-listed + benign keys. +- Existing extension launch behavior is unchanged when no disallowed keys are + present. + +> Adopted: `internal/sandbox/env_sanitize.go` (`SanitizeEnv`) wired into +> `internal/plugin/bridge.go`. Unit-tested. + +## P0: AGENTS.md `@file` References with Boundary + Budgets + +### Goal + +Let `AGENTS.md` (and other context files) reference additional files via +`@path` that get inlined into the loaded context, bounded by the git root and +strict size/depth budgets — matching goose `hints/import_files.rs`. + +### Scope and ownership + +- Primary: `internal/config` (the AGENTS.md loader). +- Boundary: stop imports at the git root so a context file cannot pull in files + from outside the repository. +- Budgets: max import depth, max operations, max expanded bytes, content parse + limit. + +### Required behavior + +1. Parse `@path` references in the loaded context file. +2. Resolve them relative to the context file, refusing paths outside the git + root. +3. Inline referenced file content recursively, applying depth/operation/byte + budgets. +4. On any budget/parse violation, fail that reference gracefully (skip) without + failing the whole load. + +### Acceptance criteria + +- A context file can pull in an in-repo file and its content appears in the + loaded context. +- A reference outside the git root is refused. +- Depth/byte/operation budgets are enforced and unit-tested. +- Existing single-file AGENTS.md behavior is unchanged. + +> Adopted: `internal/config/context_refs.go` (`expandContextReferences`) wired +> into `LoadAgentsMDFrom`. Unit-tested. + +## P1: Structured Compaction Overflow Retry Ladder + +### Goal + +Upgrade hawk's context compaction to a structured summary with a progressive +tool-response-dropping retry ladder on overflow, and token-estimator-backed +accounting, matching `goose-context-management`. + +### Scope and ownership + +- Primary: `internal/engine/compaction` (and `external/tok` for any + compression primitive). +- Behavior: on compaction context-overflow, drop tool responses from the middle + outwards and retry; parse structured summaries leniently with a lossless raw + fallback. + +### Required behavior + +1. Detect compaction overflow (`ContextLengthExceeded`). +2. Retry with progressive tool-response removal (`[0,10,20,50,100]%`). +3. Produce a structured summary (intent, files, errors/fixes, next step) when + possible, falling back to raw text losslessly. +4. Estimate tokens when the provider does not report usage. + +### Acceptance criteria + +- Compaction succeeds where it previously overflowed by dropping tool responses. +- Structured-summary parsing is lenient and never loses content to a hard error. +- Unit tests cover the retry ladder and fallback. + +## Deliberately Deferred + +- Goose's SQLite session store (`usage_ledger`, token/cost schema): hawk's + JSONL/WAL + trace + cost tracker cover it; a schema migration is a larger + change and is tracked separately. +- MCP Apps / agent-provided HTML UIs: novel but requires UI-layer design. +- ACP-as-provider wrapping other CLIs: larger provider abstraction change. +- Local-inference tool emulation / toolshim: depends on eyrie's local-model + path. +- Recipe security scanner / cron recipes: hawk already has schedule/cron. + +## Verification + +- `go test ./...` full suite. +- `make vet`, `make lint`, `hawk verify`. +- Focused tests for the env filter, AGENTS.md references, and compaction retry. +- markdownlint on this document. diff --git a/internal/config/config.go b/internal/config/config.go index 665868fc..80346cad 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -36,12 +36,16 @@ func LoadAgentsMDFrom(start string) string { } for { for _, name := range agentFiles { - data, err := os.ReadFile(filepath.Join(dir, name)) // #nosec G304 -- dir is the working directory or an ancestor of it; name is a fixed constant + path := filepath.Join(dir, name) + data, err := os.ReadFile(path) // #nosec G304 -- dir is the working directory or an ancestor of it; name is a fixed constant if err == nil { + content := string(data) if len(data) > maxAgentsMDSize { - return string(data[:maxAgentsMDSize]) + "\n\n[WARNING: AGENTS.md truncated to 10KB]" + content = content[:maxAgentsMDSize] + "\n\n[WARNING: AGENTS.md truncated to 10KB]" } - return string(data) + // Expand `@path` references (bounded by the git root and strict + // size/depth budgets) so AGENTS.md can pull in in-repo files. + return expandContextReferences(content, dir, gitRoot(dir)) } } parent := filepath.Dir(dir) diff --git a/internal/config/context_refs.go b/internal/config/context_refs.go new file mode 100644 index 00000000..192b2f64 --- /dev/null +++ b/internal/config/context_refs.go @@ -0,0 +1,156 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// Context-file reference budgets, mirroring goose `hints/import_files.rs`. +const ( + // maxRefDepth bounds recursive inlining depth. + maxRefDepth = 3 + // maxRefOps bounds the total number of reference operations per root file. + maxRefOps = 64 + // maxRefBytes bounds the total expanded output per root file. + maxRefBytes = 1 << 20 // 1 MiB + // maxRefFileSize bounds a single referenced file's parse size (ReDoS guard). + maxRefFileSize = 128 << 10 // 128 KiB +) + +// gitRoot finds the repository root for start by walking up for a `.git` +// entry (directory or file). Returns "" when no repo is found. +func gitRoot(start string) string { + dir := start + if abs, err := filepath.Abs(dir); err == nil { + dir = abs + } + for { + if _, err := os.Stat(filepath.Join(dir, ".git")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + return "" + } + dir = parent + } +} + +// expandContextReferences inlines `@path` reference lines found in a context +// file (e.g. AGENTS.md) with recursive resolution bounded by the git root and +// strict size/depth budgets. Reference lines are a path prefixed by `@` on its +// own line (trimmed). Referenced file content is inserted in place of the +// reference line. Any violation (out-of-root path, budget, size, parse failure) +// skips that reference gracefully without failing the whole load. +func expandContextReferences(content, baseDir, root string) string { + state := &refState{ops: 0} + out := expandRecursive(content, baseDir, root, 0, state) + if state.bytes > maxRefBytes { + // The caller already truncated to maxAgentsMDSize; keep the budget + // enforcement explicit so the referenced content never dominates. + if len(out) > maxRefBytes { + out = out[:maxRefBytes] + } + } + return out +} + +// refState tracks the cumulative budget across recursive reference expansion. +type refState struct { + ops int + bytes int +} + +func expandRecursive(content, baseDir, root string, depth int, state *refState) string { + if depth > maxRefDepth { + return content + } + lines := strings.Split(content, "\n") + var out []string + for _, line := range lines { + trimmed := strings.TrimSpace(line) + ref, ok := parseReferenceLine(trimmed) + if !ok { + out = append(out, line) + continue + } + if state.ops >= maxRefOps { + out = append(out, "# [context] reference skipped: operation budget exhausted") + continue + } + resolved, err := resolveReference(baseDir, root, ref) + if err != nil { + out = append(out, "# [context] reference skipped: "+err.Error()) + continue + } + data, err := os.ReadFile(resolved) // #nosec G304 -- resolved is constrained to the git root by resolveReference + if err != nil { + out = append(out, "# [context] reference skipped: cannot read "+ref) + continue + } + if len(data) > maxRefFileSize { + out = append(out, "# [context] reference skipped: file too large "+ref) + continue + } + if state.bytes+len(data) > maxRefBytes { + out = append(out, "# [context] reference skipped: byte budget exhausted") + continue + } + state.ops++ + state.bytes += len(data) + refContent := string(data) + refContent = expandRecursive(refContent, filepath.Dir(resolved), root, depth+1, state) + out = append(out, refContent) + } + return strings.Join(out, "\n") +} + +// parseReferenceLine returns (path, true) when the line is a reference +// (`@` + a non-empty path without spaces), else (_, false). +func parseReferenceLine(trimmed string) (string, bool) { + if !strings.HasPrefix(trimmed, "@") { + return "", false + } + ref := strings.TrimSpace(strings.TrimPrefix(trimmed, "@")) + if ref == "" || strings.ContainsAny(ref, " \t") { + return "", false + } + return ref, true +} + +// resolveReference resolves a reference path relative to baseDir and ensures it +// stays within root (the git root boundary). +func resolveReference(baseDir, root, ref string) (string, error) { + if baseDir == "" { + return "", fmt.Errorf("no base directory") + } + abs, err := filepath.Abs(filepath.Join(baseDir, ref)) + if err != nil { + return "", fmt.Errorf("bad reference %q: %w", ref, err) + } + if root == "" { + // No repo boundary; refuse absolute/escaping references for safety. + if strings.HasPrefix(ref, "/") { + return "", fmt.Errorf("absolute reference %q refused (no repo boundary)", ref) + } + return abs, nil + } + rootAbs, err := filepath.Abs(root) + if err != nil { + return "", fmt.Errorf("bad root %q: %w", root, err) + } + if !within(rootAbs, abs) { + return "", fmt.Errorf("reference %q escapes the git root", ref) + } + return abs, nil +} + +func within(root, path string) bool { + rel, err := filepath.Rel(root, path) + if err != nil { + return false + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} diff --git a/internal/config/context_refs_test.go b/internal/config/context_refs_test.go new file mode 100644 index 00000000..fec93c37 --- /dev/null +++ b/internal/config/context_refs_test.go @@ -0,0 +1,65 @@ +package config + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestExpandContextReferencesInline(t *testing.T) { + dir := t.TempDir() + ref := filepath.Join(dir, "guidelines.md") + _ = os.WriteFile(ref, []byte("## Guidelines\n- always test\n"), 0o600) + + content := "# Project\n@guidelines.md\n" + root := gitRoot(dir) + expanded := expandContextReferences(content, dir, root) + if !strings.Contains(expanded, "- always test") { + t.Fatalf("referenced file content was not inlined: %q", expanded) + } + if strings.Contains(expanded, "@guidelines.md") { + t.Fatalf("reference line should be replaced: %q", expanded) + } + if !strings.Contains(expanded, "## Guidelines") { + t.Fatalf("referenced file should be inlined: %q", expanded) + } +} + +func TestExpandContextReferencesRefusesEscape(t *testing.T) { + dir := t.TempDir() + root := filepath.Join(dir, "repo") + _ = os.MkdirAll(filepath.Join(root, "sub"), 0o755) + secret := filepath.Join(dir, "secret.txt") + _ = os.WriteFile(secret, []byte("top secret"), 0o600) + + // A reference that escapes the git root must be refused. + content := "@../secret.txt\n" + expanded := expandContextReferences(content, filepath.Join(root, "sub"), root) + if strings.Contains(expanded, "top secret") { + t.Fatalf("out-of-root reference must not be inlined: %q", expanded) + } + if !strings.Contains(expanded, "reference skipped") { + t.Fatalf("expected a skipped-reference notice: %q", expanded) + } +} + +func TestExpandContextReferencesDepthBudget(t *testing.T) { + dir := t.TempDir() + // Chain of references deeper than maxRefDepth must terminate. + for i := 0; i < maxRefDepth+2; i++ { + next := filepath.Join(dir, "a"+string(rune('0'+i))+".md") + link := filepath.Join(dir, "a"+string(rune('0'+i+1))+".md") + if i+1 <= maxRefDepth+1 { + _ = os.WriteFile(next, []byte("@a"+string(rune('0'+i+1))+".md\n"), 0o600) + } else { + _ = os.WriteFile(next, []byte("leaf\n"), 0o600) + } + _ = link + } + // The deepest reference should be bounded; verify it terminates without + // infinite recursion. + content := "@a0.md\n" + expanded := expandContextReferences(content, dir, gitRoot(dir)) + _ = expanded // must terminate +} diff --git a/internal/plugin/bridge.go b/internal/plugin/bridge.go index 1680bd8f..61c91a73 100644 --- a/internal/plugin/bridge.go +++ b/internal/plugin/bridge.go @@ -7,6 +7,8 @@ import ( "os" "os/exec" "strings" + + "github.com/GrayCodeAI/hawk/internal/sandbox" ) // PluginBridge wraps a shell-based bridge plugin, executing an external CLI @@ -62,13 +64,19 @@ func (pb *PluginBridge) Run(ctx context.Context, args ...string) (string, error) cmd := exec.CommandContext(ctx, pb.bin, fullArgs...) // #nosec G204 -- pb.bin was resolved via exec.LookPath at NewPluginBridge time; fullArgs come from the plugin's own manifest, trusted like other plugin config - // Merge extra environment variables onto the inherited environment. + // Merge extra environment variables onto the inherited environment, + // filtering disallowed override keys (PATH, LD_*, PYTHONPATH, NODE_OPTIONS, + // etc.) to prevent command/library hijacking via plugin manifest env. if len(bridge.Env) > 0 { env := os.Environ() for k, v := range bridge.Env { env = append(env, k+"="+v) } - cmd.Env = env + sanitized := sandbox.SanitizeEnv(env) + cmd.Env = sanitized.Env + if len(sanitized.Removed) > 0 { + fmt.Fprintf(os.Stderr, "plugin bridge %q: filtered disallowed env overrides: %v\n", pb.bin, sanitized.Removed) + } } var stdout, stderr bytes.Buffer diff --git a/internal/sandbox/env_sanitize.go b/internal/sandbox/env_sanitize.go new file mode 100644 index 00000000..6df53059 --- /dev/null +++ b/internal/sandbox/env_sanitize.go @@ -0,0 +1,72 @@ +package sandbox + +import ( + "sort" + "strings" +) + +// DisallowedEnvVars are environment variables that, when overridden in a child +// process environment, enable command/library/hijacking. Ported from goose +// `extension.rs::Envs`: blocking these in extension/MCP/plugin configs prevents +// an untrusted config from redirecting `PATH`, preloading libraries, or +// monkey-patching the Python/Node/Go toolchains of the spawned process. +var DisallowedEnvVars = map[string]bool{ + "PATH": true, + "LD_PRELOAD": true, + "LD_LIBRARY_PATH": true, + "DYLD_INSERT_LIBRARIES": true, + "DYLD_LIBRARY_PATH": true, + "PYTHONPATH": true, + "PYTHONHOME": true, + "NODE_OPTIONS": true, + "NODE_PATH": true, + "GOROOT": true, + "GOPATH": true, + "RUSTFLAGS": true, + "CARGO_HOME": true, + "RUBYLIB": true, + "PERL5LIB": true, + "LD_DEBUG": true, + "DYLD_FRAMEWORK_PATH": true, + "DYLD_FALLBACK_LIBRARY_PATH": true, +} + +// SanitizedEnv is the result of filtering a proposed environment. +type SanitizedEnv struct { + // Env is the child environment (sorted "KEY=VALUE" entries) with disallowed + // keys removed. + Env []string + // Removed lists the disallowed keys that were dropped (not their values). + Removed []string +} + +// SanitizeEnv filters a proposed child environment (as "KEY=VALUE" entries), +// removing any disallowed override key. It returns the sanitized env plus the +// removed key names. It never fails: benign keys pass through unchanged. +func SanitizeEnv(env []string) SanitizedEnv { + seen := make(map[string]int, len(env)) + var out []string + var removed []string + for _, kv := range env { + key := kv + if i := strings.IndexByte(kv, '='); i >= 0 { + key = kv[:i] + } + key = strings.TrimSpace(key) + if DisallowedEnvVars[key] { + removed = append(removed, key) + continue + } + // De-duplicate by key, last wins, preserving order of first occurrence. + if _, dup := seen[key]; dup { + // Replace the existing entry at its original position. + out[seen[key]] = kv + continue + } + seen[key] = len(out) + out = append(out, kv) + } + sort.Strings(out) + sort.Strings(removed) + return SanitizedEnv{Env: out, Removed: removed} +} diff --git a/internal/sandbox/env_sanitize_test.go b/internal/sandbox/env_sanitize_test.go new file mode 100644 index 00000000..3787b1dc --- /dev/null +++ b/internal/sandbox/env_sanitize_test.go @@ -0,0 +1,79 @@ +package sandbox + +import ( + "strings" + "testing" +) + +func TestSanitizeEnvRemovesDisallowed(t *testing.T) { + env := []string{ + "PATH=/tmp/malicious", + "LD_PRELOAD=/tmp/evil.so", + "PYTHONPATH=/tmp/py", + "NODE_OPTIONS=--require=/tmp/evil.js", + "GOROOT=/tmp/fake", + "BENIGN=ok", + } + s := SanitizeEnv(env) + for _, key := range []string{"PATH", "LD_PRELOAD", "PYTHONPATH", "NODE_OPTIONS", "GOROOT"} { + for _, kv := range s.Env { + if strings.HasPrefix(kv, key+"=") { + t.Fatalf("disallowed key %q leaked into child env: %q", key, kv) + } + } + } + // Benign keys must survive. + foundBenign := false + for _, kv := range s.Env { + if kv == "BENIGN=ok" { + foundBenign = true + } + } + if !foundBenign { + t.Fatal("benign key must survive sanitization") + } + // Removed list must name the disallowed keys (no values). + if len(s.Removed) != 5 { + t.Fatalf("removed count = %d, want 5; got %v", len(s.Removed), s.Removed) + } + for _, key := range []string{"PATH", "LD_PRELOAD", "PYTHONPATH", "NODE_OPTIONS", "GOROOT"} { + if !listHasStr(s.Removed, key) { + t.Fatalf("removed list missing %q: %v", key, s.Removed) + } + } +} + +func TestSanitizeEnvEmptyAndClean(t *testing.T) { + if s := SanitizeEnv(nil); len(s.Env) != 0 || len(s.Removed) != 0 { + t.Fatalf("nil env must yield empty result, got %+v", s) + } + s := SanitizeEnv([]string{"A=1", "B=2"}) + if len(s.Removed) != 0 || len(s.Env) != 2 { + t.Fatalf("clean env must pass through, got %+v", s) + } +} + +func TestSanitizeEnvDedupKeepsLast(t *testing.T) { + s := SanitizeEnv([]string{"KEEP=first", "KEEP=last"}) + count := 0 + for _, kv := range s.Env { + if strings.HasPrefix(kv, "KEEP=") { + count++ + if kv != "KEEP=last" { + t.Fatalf("expected last value to win, got %q", kv) + } + } + } + if count != 1 { + t.Fatalf("expected one KEEP entry, got %d", count) + } +} + +func listHasStr(list []string, s string) bool { + for _, v := range list { + if v == s { + return true + } + } + return false +}