diff --git a/.coderabbit.yaml b/.coderabbit.yaml index ed7dcbc..4a8d06d 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -3,6 +3,13 @@ # quota on generated docs, bot PRs, and per-push incremental re-reviews. reviews: + # Go has no docstrings — godoc comments (enforced by the linters we + # already run) are the Go idiom. The Python-centric docstring-coverage + # gate false-positives on every Go diff, so it is disabled. + pre_merge_checks: + docstrings: + mode: "off" + auto_review: enabled: true # Review once when the PR opens; no re-review on every push. diff --git a/docs/security/ENVIRONMENT_HARDENING.md b/docs/security/ENVIRONMENT_HARDENING.md index 9d4f655..f9228fa 100644 --- a/docs/security/ENVIRONMENT_HARDENING.md +++ b/docs/security/ENVIRONMENT_HARDENING.md @@ -39,13 +39,13 @@ but absent from the process environment entirely. └───────────────────────────────────────────────────────────┘ │ ▼ - internal/shell/shell.go → NewShell() + pkg/shell/shell.go → NewShell() (the single enforcement point for all shell surfaces) ``` ## Layer 2: Environment Filtering -**Location:** `internal/shell/env.go`, `internal/shell/shell.go` +**Location:** `pkg/shell/env.go`, `pkg/shell/shell.go` When `NewShell` is called without an explicit `Env` slice, the process environment (`os.Environ()`) is filtered through an allowlist. Only variables @@ -122,7 +122,7 @@ sandbox. ## Layer 3: Command Deny List -**Location:** `internal/shell/run.go` (blockHandler), `internal/agent/tools/bash.go` (blockFuncs) +**Location:** `pkg/shell/run.go` (blockHandler), `pkg/agent/tools/bash.go` (blockFuncs) The `blockHandler` middleware intercepts external command execution in the mvdan/sh interpreter chain. Each `BlockFunc` in the list is tested against @@ -243,7 +243,7 @@ built-in list. Built-in entries cannot be removed via configuration. ## Layer 1: Sandbox Markers -**Location:** `internal/shell/shell.go` (`PhosphorEnvMarkers`) +**Location:** `pkg/shell/shell.go` (`PhosphorEnvMarkers`) Every shell spawned by Phosphor receives the following environment markers: @@ -438,8 +438,8 @@ echoing them reveals nothing. ## Testing -Comprehensive tests are in `internal/shell/env_test.go` and -`internal/agent/tools/bash_test.go`: +Comprehensive tests are in `pkg/shell/env_test.go` and +`pkg/agent/tools/bash_test.go`: - `TestFilterEnv_Basic` — basic allowlist filtering - `TestFilterEnv_CaseInsensitive` — Windows case-insensitive matching @@ -454,5 +454,5 @@ Comprehensive tests are in `internal/shell/env_test.go` and Run with: ```bash -go test ./internal/shell/ ./internal/agent/tools/ -run TestFilterEnv|TestSafeDefaultEnv|TestPhosphorEnvMarkers|TestNewShell|TestBashTool_ConfigAwareBannedCommands +go test ./pkg/shell/ ./pkg/agent/tools/ -run 'TestFilterEnv|TestSafeDefaultEnv|TestPhosphorEnvMarkers|TestNewShell|TestBashTool_ConfigAwareBannedCommands' ``` diff --git a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md index 7ad0cc0..dd2846a 100644 --- a/docs/security/SECURITY_VALIDATION_TEST_SUITE.md +++ b/docs/security/SECURITY_VALIDATION_TEST_SUITE.md @@ -53,7 +53,7 @@ Tests map to the six-layer defense architecture documented in [WORKSPACE_HARDENI ## Test Files -### `internal/agent/tools/workspace_hardening_test.go` +### `pkg/agent/tools/workspace_hardening_test.go` Validates that every filesystem-touching tool blocks out-of-workspace paths with a `Security violation: path ... is outside workspace` error. @@ -72,7 +72,7 @@ Validates that every filesystem-touching tool blocks out-of-workspace paths with | `TestWorkspaceHardening_BashWorkingDirBlocksOutsidePaths` | bash | out-of-workspace `working_dir` parameter | | `TestWorkspaceHardening_AllToolsAllowInWorkspace` | all | positive control — in-workspace paths are allowed | -### `internal/shell/environment_hardening_test.go` +### `pkg/shell/environment_hardening_test.go` Validates shell-level security: command deny list, environment filtering, and sandbox markers. @@ -86,7 +86,7 @@ Validates shell-level security: command deny list, environment filtering, and sa | `TestEnvironmentHardening_FilterEnvWithAllowedEnv` | Env filtering | AWS_SECRET_ACCESS_KEY invisible when AllowedEnv=[PATH,HOME]; PATH visible | | `TestEnvironmentHardening_EmptyAllowedEnv_FiltersEverything` | Env filtering | Empty allowlist filters ALL environment variables including PATH | -### `internal/agent/tools/compositional_chain_test.go` +### `pkg/agent/tools/compositional_chain_test.go` Validates that chaining individually allowed actions cannot produce forbidden outcomes. @@ -97,7 +97,7 @@ Validates that chaining individually allowed actions cannot produce forbidden ou | `TestCompositionalChains_WorkingDirEscapeInChain` | Out-of-workspace working_dir blocked even in chains | | `TestCompositionalChains_ChainTriggersPermission` | Plain `ls` does NOT trigger permission; `ls && echo` DOES trigger permission | -### `internal/agent/tools/agentjacking_test.go` +### `pkg/agent/tools/agentjacking_test.go` Validates that data from "trusted" sources (MCP servers, context files) cannot be used to smuggle malicious instructions or bypass security. @@ -109,7 +109,7 @@ Validates that data from "trusted" sources (MCP servers, context files) cannot b | `TestAgentjacking_ZerWidthCharsInPath` | Zero-width characters (\u200B, \uFEFF, \u2060) in paths do not bypass bounds checks | | `TestAgentjacking_HTMLCommentsInPath` | HTML comments (``) in paths do not bypass bounds checks | -### `internal/agent/tools/path_obfuscation_test.go` +### `pkg/agent/tools/path_obfuscation_test.go` Validates that IsInside and SmartJoin handle complex path resolutions robustly. @@ -137,7 +137,7 @@ Validates network egress hardening: the `securityTransport` interceptor, IP-bloc | `TestWebFetchHardeningUserPromptVerified` | Unknown host triggers TUI prompt — user granted access | | `TestWebFetchHardeningUserPromptDenyVerified` | Unknown host triggers TUI prompt — user denied access → request blocked | -### `internal/agent/tools/memory_poisoning_test.go` +### `pkg/agent/tools/memory_poisoning_test.go` Validates that persistent state (context files like AGENTS.md, PHOSPHOR.md) cannot override hardcoded security policies. @@ -149,7 +149,7 @@ Validates that persistent state (context files like AGENTS.md, PHOSPHOR.md) cann | `TestMemoryPoisoning_BashToolSecurity` | Bash tool still blocks banned commands and out-of-workspace working_dir even with malicious GEMINI.md | | `TestMemoryPoisoning_EditToolSecurity` | Edit tool still blocks out-of-workspace edits even with malicious AGENTS.md | -### `internal/agent/tools/fuzzing_test.go` +### `pkg/agent/tools/fuzzing_test.go` Validates that all security guards **fail-closed** (reject input) rather than **fail-open** (panic/crash). A crash in a security check is itself a vulnerability — if `IsInside` panics, the caller may silently proceed. These tests feed random garbage into every guard and assert `NotPanics`, ensuring robustness under adversarial or malformed input. @@ -165,16 +165,16 @@ Validates that all security guards **fail-closed** (reject input) rather than ** ```bash # Individual test suites -go test ./internal/agent/tools/ -run 'TestWorkspaceHardening' -count=1 -go test ./internal/shell/ -run 'TestEnvironmentHardening' -count=1 -go test ./internal/agent/tools/ -run 'TestCompositionalChains' -count=1 -go test ./internal/agent/tools/ -run 'TestAgentjacking' -count=1 -go test ./internal/agent/tools/ -run 'TestPathObfuscation' -count=1 -go test ./internal/agent/tools/ -run 'TestMemoryPoisoning' -count=1 -go test ./internal/agent/tools/ -run 'TestFuzzing' -count=1 +go test ./pkg/agent/tools/ -run 'TestWorkspaceHardening' -count=1 +go test ./pkg/shell/ -run 'TestEnvironmentHardening' -count=1 +go test ./pkg/agent/tools/ -run 'TestCompositionalChains' -count=1 +go test ./pkg/agent/tools/ -run 'TestAgentjacking' -count=1 +go test ./pkg/agent/tools/ -run 'TestPathObfuscation' -count=1 +go test ./pkg/agent/tools/ -run 'TestMemoryPoisoning' -count=1 +go test ./pkg/agent/tools/ -run 'TestFuzzing' -count=1 # All new security tests together -go test ./internal/agent/tools/ ./internal/shell/ \ +go test ./pkg/agent/tools/ ./pkg/shell/ \ -run 'TestWorkspaceHardening|TestEnvironmentHardening|TestCompositionalChains|TestAgentjacking|TestPathObfuscation|TestMemoryPoisoning|TestFuzzing' \ -count=1 ``` @@ -199,5 +199,5 @@ go test ./internal/agent/tools/ ./internal/shell/ \ - Error message assertions use substring matching (`Contains`) to catch wording regressions without being fragile. - The `securityViolationMsg` constant (`"Security violation: path"`) is shared across workspace hardening tests. - When adding new tools, follow the existing pattern: resolve path with `SmartJoin`, call `IsInside`, return `fantasy.NewTextErrorResponse` on failure. -- When adding new banned commands, update both `internal/agent/tools/bash.go:bannedCommands` and the relevant environment hardening tests. +- When adding new banned commands, update both `pkg/agent/tools/bash.go:bannedCommands` and the relevant environment hardening tests. - **Fail-closed principle**: Security guards must never panic. Fuzzing tests (`TestFuzzing_*`) use `require.NotPanics()` to verify that garbage input is rejected (returns error/false) rather than crashing. A crash in a security check is a fail-open vulnerability. diff --git a/docs/security/WORKSPACE_HARDENING.md b/docs/security/WORKSPACE_HARDENING.md index 9c2ffde..7dc2ad4 100644 --- a/docs/security/WORKSPACE_HARDENING.md +++ b/docs/security/WORKSPACE_HARDENING.md @@ -85,10 +85,10 @@ Every tool that touches the filesystem resolves its target path to an absolute p | Tool | Location | Behavior | |------|----------|----------| -| `write.go` | `internal/agent/tools/write.go:74` | After `SmartJoin`, resolves absolute path and checks `IsInside`. Rejects with security error. | -| `edit.go` | `internal/agent/tools/edit.go:85` | Same pattern — validates before any content replacement or file creation. | -| `multiedit.go` | `internal/agent/tools/multiedit.go:88` | Same pattern — validates before any edit operations. | -| `append.go` | `internal/agent/tools/append.go:80` | Previously had its own inline bounds check. Refactored to use `IsInside` for consistency. | +| `write.go` | `pkg/agent/tools/write.go:74` | After `SmartJoin`, resolves absolute path and checks `IsInside`. Rejects with security error. | +| `edit.go` | `pkg/agent/tools/edit.go:85` | Same pattern — validates before any content replacement or file creation. | +| `multiedit.go` | `pkg/agent/tools/multiedit.go:88` | Same pattern — validates before any edit operations. | +| `append.go` | `pkg/agent/tools/append.go:80` | Previously had its own inline bounds check. Refactored to use `IsInside` for consistency. | ### File-read tools (view, download) @@ -96,8 +96,8 @@ These previously used a **permission-request pattern** — they would detect out | Tool | Location | Behavior | |------|----------|----------| -| `view.go` | `internal/agent/tools/view.go:122` | Replaced permission gate with hard block. The old `isSkillFile` workaround (which allowed reading skill files outside the workspace) is removed. Builtin skills via `phosphor://skills/...` prefix are handled separately before the bounds check and remain accessible. | -| `download.go` | `internal/agent/tools/download.go:90` | Added bounds check after `SmartJoin`. Removed unused `cmp` import that was only needed for the old relative-path formatting. | +| `view.go` | `pkg/agent/tools/view.go:122` | Replaced permission gate with hard block. The old `isSkillFile` workaround (which allowed reading skill files outside the workspace) is removed. Builtin skills via `phosphor://skills/...` prefix are handled separately before the bounds check and remain accessible. | +| `download.go` | `pkg/agent/tools/download.go:90` | Added bounds check after `SmartJoin`. Removed unused `cmp` import that was only needed for the old relative-path formatting. | ### Search and listing tools (ls, glob, grep, structural_search) @@ -105,10 +105,10 @@ These previously used permission requests for out-of-workspace directories. All | Tool | Location | Behavior | |------|----------|----------| -| `ls.go` | `internal/agent/tools/ls.go:93` | Replaced permission gate with hard block after path resolution. | -| `glob.go` | `internal/agent/tools/glob.go:68` | Bounds check after `ResolveSearchPath`. | -| `grep.go` | `internal/agent/tools/grep.go:146` | Same — bounds check after `ResolveSearchPath`. | -| `structural_search.go` | `internal/agent/tools/structural_search.go:302` | Same — bounds check after `ResolveSearchPath`. | +| `ls.go` | `pkg/agent/tools/ls.go:93` | Replaced permission gate with hard block after path resolution. | +| `glob.go` | `pkg/agent/tools/glob.go:68` | Bounds check after `ResolveSearchPath`. | +| `grep.go` | `pkg/agent/tools/grep.go:146` | Same — bounds check after `ResolveSearchPath`. | +| `structural_search.go` | `pkg/agent/tools/structural_search.go:302` | Same — bounds check after `ResolveSearchPath`. | ### Cross-reference tools (references, diagnostics) @@ -116,12 +116,12 @@ These tools accept user-supplied paths and previously had no workspace validatio | Tool | Location | Behavior | |------|----------|----------| -| `references.go` | `internal/agent/tools/references.go:38` | Accepts `workingDir` parameter. Validates `params.Path` against workspace via `IsInside` before calling `searchFiles`. Prevents symbol search in files outside workspace (e.g., `/etc/passwd`). | -| `diagnostics.go` | `internal/agent/tools/diagnostics.go:29` | Accepts `workingDir` parameter. Validates `params.FilePath` against workspace via `IsInside` before triggering LSP analysis. Prevents LSP side-effects on files outside workspace. | +| `references.go` | `pkg/agent/tools/references.go:38` | Accepts `workingDir` parameter. Validates `params.Path` against workspace via `IsInside` before calling `searchFiles`. Prevents symbol search in files outside workspace (e.g., `/etc/passwd`). | +| `diagnostics.go` | `pkg/agent/tools/diagnostics.go:29` | Accepts `workingDir` parameter. Validates `params.FilePath` against workspace via `IsInside` before triggering LSP analysis. Prevents LSP side-effects on files outside workspace. | ## Layer 2: Bash Working Directory Guard -**Location:** `internal/agent/tools/bash.go:217` +**Location:** `pkg/agent/tools/bash.go:217` The bash tool accepts an optional `working_dir` parameter. Previously, the agent could pass any absolute path (e.g. `C:\Windows`) as `working_dir` and commands would execute there with no restriction. @@ -139,7 +139,7 @@ This covers absolute paths, relative paths that resolve outside the workspace, a ## Layer 3: Shell CWD Lockdown -**Location:** `internal/shell/shell.go:68` (Shell struct), `shell.go:280` (`updateShellFromRunner`) +**Location:** `pkg/shell/shell.go:68` (Shell struct), `shell.go:280` (`updateShellFromRunner`) Even with the working_dir guard, a command like `cd /some/outside/path && ls` could change the shell's internal working directory mid-execution. The POSIX shell interpreter (`mvdan.cc/sh/v3`) tracks `runner.Dir` and updates the Shell's `cwd` after each command via `updateShellFromRunner`. @@ -163,7 +163,7 @@ If a `cd` command would move the working directory outside the workspace, it is The background shell manager passes the workspace root through to each new Shell: -**Location:** `internal/shell/background.go:89` — `Start(ctx, workingDir, workspace, blockFuncs, command, description)` +**Location:** `pkg/shell/background.go:89` — `Start(ctx, workingDir, workspace, blockFuncs, command, description)` ### Why the post-command reset is not sufficient @@ -235,7 +235,7 @@ directory. ## Layer 4: Agent Prompt Instructions -**Location:** `internal/agent/tools/bash.md.tpl:19` +**Location:** `pkg/agent/tools/bash.md.tpl:19` The bash tool's system prompt now includes an explicit sandbox warning: @@ -245,9 +245,16 @@ This is defense-in-depth — it tells the LLM the rules so it is less likely to ## Layer 5: Startup Symlink Resolution -**Location:** `internal/config/load.go:51-55` +**Location:** `pkg/config/load.go:63-68` -Before config initialization, the workspace root is resolved via `filepath.EvalSymlinks` to prevent bypass through symlink traversal in bounds checking. If the workspace doesn't exist on disk (e.g., during tests), the original path is used as-is. +The workspace root is resolved via `filepath.EvalSymlinks` after the initial +config files are parsed but before the `ConfigStore` is constructed. The +workspace config is merged afterward (after `ConfigStore` construction, at +the highest priority); either way, resolution happens before the store is +built, so every consumer of the workspace path sees the resolved target +and bounds checking cannot be bypassed through symlink traversal. If the +workspace doesn't exist on disk (e.g., during tests), the original path is +used as-is. ```go if resolvedWorkingDir, err := filepath.EvalSymlinks(workingDir); err == nil { @@ -263,9 +270,9 @@ The following components intentionally operate outside the workspace sandbox. Th | Component | Reason for bypass | |-----------|-------------------| -| Hook runner (`internal/hooks/`) | Runs user-authored shell commands from `phosphor.json`, not agent-generated ones. The cwd comes from the hook config, not the agent. | +| Hook runner (`pkg/hooks/`) | Runs user-authored shell commands from `phosphor.json`, not agent-generated ones. The cwd comes from the hook config, not the agent. | | `getGitStatus` in prompt loading | Internal utility that runs git commands against the project directory. Not agent-facing. | -| LSP client (`internal/lsp/`) | Uses file paths for diagnostics but does not write files. Path filtering already exists (`fsext.HasPrefix`). | +| LSP client (`pkg/lsp/`) | Uses file paths for diagnostics but does not write files. Path filtering already exists (`fsext.HasPrefix`). | | Config loading | Reads `phosphor.json` and context files (AGENTS.md, etc.) from the working directory. | ### Skills and MCP Servers @@ -280,9 +287,9 @@ The following components use `UnsafeSmartJoin` instead of `SmartJoin` because th | Component | Location | Reason | |-----------|----------|--------| -| MCP resource listing | `internal/agent/tools/list_mcp_resources.go:55` | MCP servers may reference resources outside the workspace. | -| MCP resource reading | `internal/agent/tools/read_mcp_resource.go:61` | MCP servers may serve content from arbitrary locations. | -| Shell script dispatch | `internal/shell/dispatch.go:58` | Hook scripts may be located outside the workspace. | +| MCP resource listing | `pkg/agent/tools/list_mcp_resources.go:55` | MCP servers may reference resources outside the workspace. | +| MCP resource reading | `pkg/agent/tools/read_mcp_resource.go:61` | MCP servers may serve content from arbitrary locations. | +| Shell script dispatch | `pkg/shell/dispatch.go:58` | Hook scripts may be located outside the workspace. | These are explicitly marked with `UnsafeSmartJoin` to make the intentional bypass visible in code review and static analysis. @@ -303,8 +310,8 @@ If future features need the agent to read configuration files or other resources All bounds-check logic is covered by unit tests: - `internal/filepathext/bounds_test.go` — `IsInside` correctness (7 cases + real-path integration test). -- `internal/agent/tools/append_test.go` — existing append tests pass with the refactored bounds check. -- `internal/shell/background_test.go` — background shell manager tests pass with the updated `Start` signature. +- `pkg/agent/tools/append_test.go` — existing append tests pass with the refactored bounds check. +- `pkg/shell/background_test.go` — background shell manager tests pass with the updated `Start` signature. - `internal/pathguard/pathguard_test.go` — `TestCommandEscapesWorkspace_CDCommandValidated` pins that `cd` targets are bounds-checked (absolute, `~`, and compound `cd X && cmd` forms are refused; in-tree `cd` is allowed). - `internal/pathguard/heredoc_test.go` — `TestHeredocBodyIsData` pins that heredoc bodies, stacked heredocs, `<<-` bodies, and herestring operands are treated as data while real operands on the introducer line are still flagged; `TestHeredocBodyNotRewritten` pins that `CorrectCommandPaths` leaves heredoc data byte-for-byte intact. - `pkg/shell/confinement_vectors_test.go` — `TestMidCommandCDEscapeBlocked` pins the red-team vectors (`cd && cat `, `cd && ls`, `cd ~ && ls .ssh`, `cd ; echo > file`) are refused; `TestMidCommandCDInTreeAllowed` and `TestCDEscapeHarmlessWithTrustedTempRoots` pin that legitimate in-tree and trusted-temp directory changes still work. diff --git a/internal/cmd/schema.go b/internal/cmd/schema.go index 771f9bf..7b580ea 100644 --- a/internal/cmd/schema.go +++ b/internal/cmd/schema.go @@ -3,20 +3,65 @@ package cmd import ( "encoding/json" "fmt" + "reflect" + "strings" "github.com/hackafterdark/phosphor/pkg/config" "github.com/invopop/jsonschema" "github.com/spf13/cobra" ) +// newSchemaReflector returns a reflector configured for phosphor's +// configuration types. csync.Map is a locked wrapper around a plain map; +// Map instances carry a sync.RWMutex, so they cannot expose a value-receiver +// JSONSchemaAlias without tripping go vet's copylocks check. The schema is +// mapped here instead. +func newSchemaReflector() *jsonschema.Reflector { + reflector := new(jsonschema.Reflector) + reflector.Mapper = func(t reflect.Type) *jsonschema.Schema { + if t.Kind() == reflect.Struct && t.PkgPath() == "github.com/hackafterdark/phosphor/pkg/csync" && + strings.HasPrefix(t.Name(), "Map[") { + return &jsonschema.Schema{ + Type: "object", + AdditionalProperties: &jsonschema.Schema{ + Ref: "#/$defs/ProviderConfig", + }, + } + } + return nil + } + return reflector +} + +// newConfigSchema reflects config.Config and adds the ProviderConfig +// definition that the providers mapper references, promoting any nested +// definitions it pulls in to the root so their $refs resolve too. +func newConfigSchema() *jsonschema.Schema { + root := newSchemaReflector().Reflect(&config.Config{}) + + expanded := newSchemaReflector() + expanded.ExpandedStruct = true + providerSchema := expanded.Reflect(&config.ProviderConfig{}) + providerSchema.ID = jsonschema.EmptyID + providerSchema.Version = "" + + root.Definitions["ProviderConfig"] = providerSchema + for name, def := range providerSchema.Definitions { + if _, exists := root.Definitions[name]; !exists { + root.Definitions[name] = def + } + } + providerSchema.Definitions = nil + return root +} + var schemaCmd = &cobra.Command{ Use: "schema", Short: "Generate JSON schema for configuration", Long: "Generate JSON schema for the phosphor configuration file", Hidden: true, RunE: func(cmd *cobra.Command, args []string) error { - reflector := new(jsonschema.Reflector) - bts, err := json.MarshalIndent(reflector.Reflect(&config.Config{}), "", " ") + bts, err := json.MarshalIndent(newConfigSchema(), "", " ") if err != nil { return fmt.Errorf("failed to marshal schema: %w", err) } diff --git a/internal/cmd/schema_test.go b/internal/cmd/schema_test.go index 891ac2e..d2c9e4c 100644 --- a/internal/cmd/schema_test.go +++ b/internal/cmd/schema_test.go @@ -5,16 +5,13 @@ import ( "strings" "testing" - "github.com/hackafterdark/phosphor/pkg/config" - "github.com/invopop/jsonschema" "github.com/stretchr/testify/require" ) func TestSchemaNoBrokenRefs(t *testing.T) { t.Parallel() - reflector := new(jsonschema.Reflector) - bts, err := json.Marshal(reflector.Reflect(&config.Config{})) + bts, err := json.Marshal(newConfigSchema()) require.NoError(t, err) var schema struct { @@ -31,8 +28,7 @@ func TestSchemaNoBrokenRefs(t *testing.T) { func TestSchemaProvidersHasAdditionalProperties(t *testing.T) { t.Parallel() - reflector := new(jsonschema.Reflector) - bts, err := json.Marshal(reflector.Reflect(&config.Config{})) + bts, err := json.Marshal(newConfigSchema()) require.NoError(t, err) var schema struct { @@ -56,4 +52,6 @@ func TestSchemaProvidersHasAdditionalProperties(t *testing.T) { require.Equal(t, "object", providers.Type) require.True(t, strings.Contains(string(providers.AdditionalProperties), "ProviderConfig"), "providers should use additionalProperties with a ProviderConfig ref, got: %s", string(providers.AdditionalProperties)) + _, ok = schema.Defs["ProviderConfig"] + require.True(t, ok, "providers $ref target ProviderConfig must exist in $defs") } diff --git a/pkg/agent/common_test.go b/pkg/agent/common_test.go index 5310653..b217690 100644 --- a/pkg/agent/common_test.go +++ b/pkg/agent/common_test.go @@ -168,10 +168,10 @@ func coderAgent(r *vcr.Recorder, env fakeEnv, large, small fantasy.LanguageModel allTools := []fantasy.AgentTool{ tools.NewBashTool(env.permissions, env.workingDir, cfg.Config().Tools.Bash, cfg.Config().Options.Attribution, modelName, nil), - tools.NewDownloadTool(env.permissions, env.workingDir, r.GetDefaultClient()), + tools.NewDownloadTool(env.permissions, env.workingDir, cfg.Config(), r.GetDefaultClient()), tools.NewEditTool(nil, env.permissions, env.history, *env.filetracker, env.workingDir, nil), tools.NewMultiEditTool(nil, env.permissions, env.history, *env.filetracker, env.workingDir, nil), - tools.NewFetchTool(env.permissions, env.workingDir, r.GetDefaultClient()), + tools.NewFetchTool(env.permissions, env.workingDir, cfg.Config(), r.GetDefaultClient()), tools.NewGlobTool(env.workingDir), tools.NewGrepTool(env.workingDir, cfg.Config().Tools.Grep, nil), tools.NewStructuralSearchTool(env.workingDir, nil), diff --git a/pkg/agent/coordinator.go b/pkg/agent/coordinator.go index 78bdf90..60040f9 100644 --- a/pkg/agent/coordinator.go +++ b/pkg/agent/coordinator.go @@ -1141,10 +1141,10 @@ func (c *coordinator) buildTools(ctx context.Context, agent config.Agent, isSubA allTools, tools.NewJobOutputTool(c.redactionPolicy), tools.NewJobKillTool(), - tools.NewDownloadTool(c.permissions, c.cfg.WorkingDir(), networkClient), + tools.NewDownloadTool(c.permissions, c.cfg.WorkingDir(), c.cfg.Config(), networkClient), tools.NewEditTool(c.lspManager, c.permissions, c.history, c.filetracker, c.cfg.WorkingDir(), c.redactionPolicy), tools.NewMultiEditTool(c.lspManager, c.permissions, c.history, c.filetracker, c.cfg.WorkingDir(), c.redactionPolicy), - tools.NewFetchTool(c.permissions, c.cfg.WorkingDir(), networkClient), + tools.NewFetchTool(c.permissions, c.cfg.WorkingDir(), c.cfg.Config(), networkClient), tools.NewGlobTool(c.cfg.WorkingDir()), tools.NewGrepTool(c.cfg.WorkingDir(), c.cfg.Config().Tools.Grep, c.redactionPolicy), tools.NewScanSecretsTool(c.cfg.WorkingDir(), c.redactionPolicy), diff --git a/pkg/agent/tools/download.go b/pkg/agent/tools/download.go index cd21a71..d4d313f 100644 --- a/pkg/agent/tools/download.go +++ b/pkg/agent/tools/download.go @@ -14,6 +14,7 @@ import ( "charm.land/fantasy" "github.com/hackafterdark/phosphor/internal/filepathext" + "github.com/hackafterdark/phosphor/pkg/config" "github.com/hackafterdark/phosphor/pkg/egress" "github.com/hackafterdark/phosphor/pkg/otel" "github.com/hackafterdark/phosphor/pkg/permission" @@ -53,9 +54,9 @@ func downloadDescription() string { }) } -func NewDownloadTool(permissions permission.Service, workingDir string, client *http.Client) fantasy.AgentTool { +func NewDownloadTool(permissions permission.Service, workingDir string, cfg *config.Config, client *http.Client) fantasy.AgentTool { if client == nil { - client = NewSecuredHTTPClient(nil, nil) + client = NewSecuredHTTPClient(cfg, nil) client.Timeout = 5 * time.Minute } else { client = egress.WrapClient(client) diff --git a/pkg/agent/tools/fetch.go b/pkg/agent/tools/fetch.go index 0d79b93..c6067b0 100644 --- a/pkg/agent/tools/fetch.go +++ b/pkg/agent/tools/fetch.go @@ -12,6 +12,7 @@ import ( "unicode/utf8" "charm.land/fantasy" + "github.com/hackafterdark/phosphor/pkg/config" "github.com/hackafterdark/phosphor/pkg/egress" "github.com/hackafterdark/phosphor/pkg/otel" "github.com/hackafterdark/phosphor/pkg/permission" @@ -48,9 +49,9 @@ func fetchDescription() string { }) } -func NewFetchTool(permissions permission.Service, workingDir string, client *http.Client) fantasy.AgentTool { +func NewFetchTool(permissions permission.Service, workingDir string, cfg *config.Config, client *http.Client) fantasy.AgentTool { if client == nil { - client = NewSecuredHTTPClient(nil, nil) + client = NewSecuredHTTPClient(cfg, nil) } else { client = egress.WrapClient(client) } diff --git a/pkg/agent/tools/url_secret_guard_test.go b/pkg/agent/tools/url_secret_guard_test.go index 56e060e..e9c14c2 100644 --- a/pkg/agent/tools/url_secret_guard_test.go +++ b/pkg/agent/tools/url_secret_guard_test.go @@ -33,7 +33,7 @@ func TestFetchTool_RefusesRegisteredSecretInQuery(t *testing.T) { })) defer srv.Close() - tool := NewFetchTool(&mockPermissionService{}, t.TempDir(), nil) + tool := NewFetchTool(&mockPermissionService{}, t.TempDir(), nil, nil) input, err := json.Marshal(FetchParams{ URL: srv.URL + "/log?token=" + guardToken, Format: "text", @@ -54,7 +54,7 @@ func TestFetchTool_AllowsCleanQuery(t *testing.T) { })) defer srv.Close() - tool := NewFetchTool(&mockPermissionService{}, t.TempDir(), nil) + tool := NewFetchTool(&mockPermissionService{}, t.TempDir(), nil, nil) input, err := json.Marshal(FetchParams{URL: srv.URL + "/search?q=hello&page=2", Format: "text"}) require.NoError(t, err) @@ -121,7 +121,7 @@ func TestDownloadTool_RefusesRegisteredSecretInQuery(t *testing.T) { })) defer srv.Close() - tool := NewDownloadTool(&mockPermissionService{}, t.TempDir(), nil) + tool := NewDownloadTool(&mockPermissionService{}, t.TempDir(), nil, nil) input, err := json.Marshal(DownloadParams{ URL: srv.URL + "/artifact.bin?token=" + guardToken, FilePath: "artifact.bin", @@ -171,7 +171,7 @@ func TestFetchDownloadToolsRouteThroughSecuredClient(t *testing.T) { perm := &mockPermissionService{} workingDir := t.TempDir() - fetchTool := NewFetchTool(perm, workingDir, nil) + fetchTool := NewFetchTool(perm, workingDir, nil, nil) input, err := json.Marshal(FetchParams{URL: "https://trusted.example.com/x", Format: "text"}) require.NoError(t, err) _, err = fetchTool.Run(guardSessionCtx(), fantasy.ToolCall{ @@ -180,7 +180,7 @@ func TestFetchDownloadToolsRouteThroughSecuredClient(t *testing.T) { require.Error(t, err, "fetch must fail closed on an unapproved host") require.Contains(t, err.Error(), "no prompt handler configured") - dlTool := NewDownloadTool(perm, workingDir, nil) + dlTool := NewDownloadTool(perm, workingDir, nil, nil) dlInput, err := json.Marshal(DownloadParams{URL: "https://trusted.example.com/x", FilePath: "x.bin"}) require.NoError(t, err) _, err = dlTool.Run(guardSessionCtx(), fantasy.ToolCall{ diff --git a/pkg/agent/tools/workspace_hardening_test.go b/pkg/agent/tools/workspace_hardening_test.go index 602f675..5677fa4 100644 --- a/pkg/agent/tools/workspace_hardening_test.go +++ b/pkg/agent/tools/workspace_hardening_test.go @@ -200,7 +200,7 @@ func TestWorkspaceHardening_DownloadToolBlocksOutsidePaths(t *testing.T) { workingDir := t.TempDir() ctx := context.WithValue(context.Background(), SessionIDContextKey, "test-session") - tool := NewDownloadTool(nil, workingDir, nil) + tool := NewDownloadTool(nil, workingDir, nil, nil) tests := []struct { name string diff --git a/pkg/csync/maps.go b/pkg/csync/maps.go index 549cda3..b2dc9cc 100644 --- a/pkg/csync/maps.go +++ b/pkg/csync/maps.go @@ -131,14 +131,6 @@ var ( _ json.Marshaler = &Map[string, any]{} ) -// JSONSchemaAlias returns the underlying map type for JSON schema generation. -// Value receiver is required because github.com/invopop/jsonschema checks -// interface satisfaction on the non-pointer type after stripping pointers. -func (Map[K, V]) JSONSchemaAlias() any { //nolint - m := map[K]V{} - return m -} - // UnmarshalJSON implements json.Unmarshaler. func (m *Map[K, V]) UnmarshalJSON(data []byte) error { m.mu.Lock() diff --git a/pkg/phosphordocs/content/security/ENVIRONMENT_HARDENING.md b/pkg/phosphordocs/content/security/ENVIRONMENT_HARDENING.md index 9d4f655..f9228fa 100644 --- a/pkg/phosphordocs/content/security/ENVIRONMENT_HARDENING.md +++ b/pkg/phosphordocs/content/security/ENVIRONMENT_HARDENING.md @@ -39,13 +39,13 @@ but absent from the process environment entirely. └───────────────────────────────────────────────────────────┘ │ ▼ - internal/shell/shell.go → NewShell() + pkg/shell/shell.go → NewShell() (the single enforcement point for all shell surfaces) ``` ## Layer 2: Environment Filtering -**Location:** `internal/shell/env.go`, `internal/shell/shell.go` +**Location:** `pkg/shell/env.go`, `pkg/shell/shell.go` When `NewShell` is called without an explicit `Env` slice, the process environment (`os.Environ()`) is filtered through an allowlist. Only variables @@ -122,7 +122,7 @@ sandbox. ## Layer 3: Command Deny List -**Location:** `internal/shell/run.go` (blockHandler), `internal/agent/tools/bash.go` (blockFuncs) +**Location:** `pkg/shell/run.go` (blockHandler), `pkg/agent/tools/bash.go` (blockFuncs) The `blockHandler` middleware intercepts external command execution in the mvdan/sh interpreter chain. Each `BlockFunc` in the list is tested against @@ -243,7 +243,7 @@ built-in list. Built-in entries cannot be removed via configuration. ## Layer 1: Sandbox Markers -**Location:** `internal/shell/shell.go` (`PhosphorEnvMarkers`) +**Location:** `pkg/shell/shell.go` (`PhosphorEnvMarkers`) Every shell spawned by Phosphor receives the following environment markers: @@ -438,8 +438,8 @@ echoing them reveals nothing. ## Testing -Comprehensive tests are in `internal/shell/env_test.go` and -`internal/agent/tools/bash_test.go`: +Comprehensive tests are in `pkg/shell/env_test.go` and +`pkg/agent/tools/bash_test.go`: - `TestFilterEnv_Basic` — basic allowlist filtering - `TestFilterEnv_CaseInsensitive` — Windows case-insensitive matching @@ -454,5 +454,5 @@ Comprehensive tests are in `internal/shell/env_test.go` and Run with: ```bash -go test ./internal/shell/ ./internal/agent/tools/ -run TestFilterEnv|TestSafeDefaultEnv|TestPhosphorEnvMarkers|TestNewShell|TestBashTool_ConfigAwareBannedCommands +go test ./pkg/shell/ ./pkg/agent/tools/ -run 'TestFilterEnv|TestSafeDefaultEnv|TestPhosphorEnvMarkers|TestNewShell|TestBashTool_ConfigAwareBannedCommands' ``` diff --git a/pkg/phosphordocs/content/security/WORKSPACE_HARDENING.md b/pkg/phosphordocs/content/security/WORKSPACE_HARDENING.md index 9c2ffde..7dc2ad4 100644 --- a/pkg/phosphordocs/content/security/WORKSPACE_HARDENING.md +++ b/pkg/phosphordocs/content/security/WORKSPACE_HARDENING.md @@ -85,10 +85,10 @@ Every tool that touches the filesystem resolves its target path to an absolute p | Tool | Location | Behavior | |------|----------|----------| -| `write.go` | `internal/agent/tools/write.go:74` | After `SmartJoin`, resolves absolute path and checks `IsInside`. Rejects with security error. | -| `edit.go` | `internal/agent/tools/edit.go:85` | Same pattern — validates before any content replacement or file creation. | -| `multiedit.go` | `internal/agent/tools/multiedit.go:88` | Same pattern — validates before any edit operations. | -| `append.go` | `internal/agent/tools/append.go:80` | Previously had its own inline bounds check. Refactored to use `IsInside` for consistency. | +| `write.go` | `pkg/agent/tools/write.go:74` | After `SmartJoin`, resolves absolute path and checks `IsInside`. Rejects with security error. | +| `edit.go` | `pkg/agent/tools/edit.go:85` | Same pattern — validates before any content replacement or file creation. | +| `multiedit.go` | `pkg/agent/tools/multiedit.go:88` | Same pattern — validates before any edit operations. | +| `append.go` | `pkg/agent/tools/append.go:80` | Previously had its own inline bounds check. Refactored to use `IsInside` for consistency. | ### File-read tools (view, download) @@ -96,8 +96,8 @@ These previously used a **permission-request pattern** — they would detect out | Tool | Location | Behavior | |------|----------|----------| -| `view.go` | `internal/agent/tools/view.go:122` | Replaced permission gate with hard block. The old `isSkillFile` workaround (which allowed reading skill files outside the workspace) is removed. Builtin skills via `phosphor://skills/...` prefix are handled separately before the bounds check and remain accessible. | -| `download.go` | `internal/agent/tools/download.go:90` | Added bounds check after `SmartJoin`. Removed unused `cmp` import that was only needed for the old relative-path formatting. | +| `view.go` | `pkg/agent/tools/view.go:122` | Replaced permission gate with hard block. The old `isSkillFile` workaround (which allowed reading skill files outside the workspace) is removed. Builtin skills via `phosphor://skills/...` prefix are handled separately before the bounds check and remain accessible. | +| `download.go` | `pkg/agent/tools/download.go:90` | Added bounds check after `SmartJoin`. Removed unused `cmp` import that was only needed for the old relative-path formatting. | ### Search and listing tools (ls, glob, grep, structural_search) @@ -105,10 +105,10 @@ These previously used permission requests for out-of-workspace directories. All | Tool | Location | Behavior | |------|----------|----------| -| `ls.go` | `internal/agent/tools/ls.go:93` | Replaced permission gate with hard block after path resolution. | -| `glob.go` | `internal/agent/tools/glob.go:68` | Bounds check after `ResolveSearchPath`. | -| `grep.go` | `internal/agent/tools/grep.go:146` | Same — bounds check after `ResolveSearchPath`. | -| `structural_search.go` | `internal/agent/tools/structural_search.go:302` | Same — bounds check after `ResolveSearchPath`. | +| `ls.go` | `pkg/agent/tools/ls.go:93` | Replaced permission gate with hard block after path resolution. | +| `glob.go` | `pkg/agent/tools/glob.go:68` | Bounds check after `ResolveSearchPath`. | +| `grep.go` | `pkg/agent/tools/grep.go:146` | Same — bounds check after `ResolveSearchPath`. | +| `structural_search.go` | `pkg/agent/tools/structural_search.go:302` | Same — bounds check after `ResolveSearchPath`. | ### Cross-reference tools (references, diagnostics) @@ -116,12 +116,12 @@ These tools accept user-supplied paths and previously had no workspace validatio | Tool | Location | Behavior | |------|----------|----------| -| `references.go` | `internal/agent/tools/references.go:38` | Accepts `workingDir` parameter. Validates `params.Path` against workspace via `IsInside` before calling `searchFiles`. Prevents symbol search in files outside workspace (e.g., `/etc/passwd`). | -| `diagnostics.go` | `internal/agent/tools/diagnostics.go:29` | Accepts `workingDir` parameter. Validates `params.FilePath` against workspace via `IsInside` before triggering LSP analysis. Prevents LSP side-effects on files outside workspace. | +| `references.go` | `pkg/agent/tools/references.go:38` | Accepts `workingDir` parameter. Validates `params.Path` against workspace via `IsInside` before calling `searchFiles`. Prevents symbol search in files outside workspace (e.g., `/etc/passwd`). | +| `diagnostics.go` | `pkg/agent/tools/diagnostics.go:29` | Accepts `workingDir` parameter. Validates `params.FilePath` against workspace via `IsInside` before triggering LSP analysis. Prevents LSP side-effects on files outside workspace. | ## Layer 2: Bash Working Directory Guard -**Location:** `internal/agent/tools/bash.go:217` +**Location:** `pkg/agent/tools/bash.go:217` The bash tool accepts an optional `working_dir` parameter. Previously, the agent could pass any absolute path (e.g. `C:\Windows`) as `working_dir` and commands would execute there with no restriction. @@ -139,7 +139,7 @@ This covers absolute paths, relative paths that resolve outside the workspace, a ## Layer 3: Shell CWD Lockdown -**Location:** `internal/shell/shell.go:68` (Shell struct), `shell.go:280` (`updateShellFromRunner`) +**Location:** `pkg/shell/shell.go:68` (Shell struct), `shell.go:280` (`updateShellFromRunner`) Even with the working_dir guard, a command like `cd /some/outside/path && ls` could change the shell's internal working directory mid-execution. The POSIX shell interpreter (`mvdan.cc/sh/v3`) tracks `runner.Dir` and updates the Shell's `cwd` after each command via `updateShellFromRunner`. @@ -163,7 +163,7 @@ If a `cd` command would move the working directory outside the workspace, it is The background shell manager passes the workspace root through to each new Shell: -**Location:** `internal/shell/background.go:89` — `Start(ctx, workingDir, workspace, blockFuncs, command, description)` +**Location:** `pkg/shell/background.go:89` — `Start(ctx, workingDir, workspace, blockFuncs, command, description)` ### Why the post-command reset is not sufficient @@ -235,7 +235,7 @@ directory. ## Layer 4: Agent Prompt Instructions -**Location:** `internal/agent/tools/bash.md.tpl:19` +**Location:** `pkg/agent/tools/bash.md.tpl:19` The bash tool's system prompt now includes an explicit sandbox warning: @@ -245,9 +245,16 @@ This is defense-in-depth — it tells the LLM the rules so it is less likely to ## Layer 5: Startup Symlink Resolution -**Location:** `internal/config/load.go:51-55` +**Location:** `pkg/config/load.go:63-68` -Before config initialization, the workspace root is resolved via `filepath.EvalSymlinks` to prevent bypass through symlink traversal in bounds checking. If the workspace doesn't exist on disk (e.g., during tests), the original path is used as-is. +The workspace root is resolved via `filepath.EvalSymlinks` after the initial +config files are parsed but before the `ConfigStore` is constructed. The +workspace config is merged afterward (after `ConfigStore` construction, at +the highest priority); either way, resolution happens before the store is +built, so every consumer of the workspace path sees the resolved target +and bounds checking cannot be bypassed through symlink traversal. If the +workspace doesn't exist on disk (e.g., during tests), the original path is +used as-is. ```go if resolvedWorkingDir, err := filepath.EvalSymlinks(workingDir); err == nil { @@ -263,9 +270,9 @@ The following components intentionally operate outside the workspace sandbox. Th | Component | Reason for bypass | |-----------|-------------------| -| Hook runner (`internal/hooks/`) | Runs user-authored shell commands from `phosphor.json`, not agent-generated ones. The cwd comes from the hook config, not the agent. | +| Hook runner (`pkg/hooks/`) | Runs user-authored shell commands from `phosphor.json`, not agent-generated ones. The cwd comes from the hook config, not the agent. | | `getGitStatus` in prompt loading | Internal utility that runs git commands against the project directory. Not agent-facing. | -| LSP client (`internal/lsp/`) | Uses file paths for diagnostics but does not write files. Path filtering already exists (`fsext.HasPrefix`). | +| LSP client (`pkg/lsp/`) | Uses file paths for diagnostics but does not write files. Path filtering already exists (`fsext.HasPrefix`). | | Config loading | Reads `phosphor.json` and context files (AGENTS.md, etc.) from the working directory. | ### Skills and MCP Servers @@ -280,9 +287,9 @@ The following components use `UnsafeSmartJoin` instead of `SmartJoin` because th | Component | Location | Reason | |-----------|----------|--------| -| MCP resource listing | `internal/agent/tools/list_mcp_resources.go:55` | MCP servers may reference resources outside the workspace. | -| MCP resource reading | `internal/agent/tools/read_mcp_resource.go:61` | MCP servers may serve content from arbitrary locations. | -| Shell script dispatch | `internal/shell/dispatch.go:58` | Hook scripts may be located outside the workspace. | +| MCP resource listing | `pkg/agent/tools/list_mcp_resources.go:55` | MCP servers may reference resources outside the workspace. | +| MCP resource reading | `pkg/agent/tools/read_mcp_resource.go:61` | MCP servers may serve content from arbitrary locations. | +| Shell script dispatch | `pkg/shell/dispatch.go:58` | Hook scripts may be located outside the workspace. | These are explicitly marked with `UnsafeSmartJoin` to make the intentional bypass visible in code review and static analysis. @@ -303,8 +310,8 @@ If future features need the agent to read configuration files or other resources All bounds-check logic is covered by unit tests: - `internal/filepathext/bounds_test.go` — `IsInside` correctness (7 cases + real-path integration test). -- `internal/agent/tools/append_test.go` — existing append tests pass with the refactored bounds check. -- `internal/shell/background_test.go` — background shell manager tests pass with the updated `Start` signature. +- `pkg/agent/tools/append_test.go` — existing append tests pass with the refactored bounds check. +- `pkg/shell/background_test.go` — background shell manager tests pass with the updated `Start` signature. - `internal/pathguard/pathguard_test.go` — `TestCommandEscapesWorkspace_CDCommandValidated` pins that `cd` targets are bounds-checked (absolute, `~`, and compound `cd X && cmd` forms are refused; in-tree `cd` is allowed). - `internal/pathguard/heredoc_test.go` — `TestHeredocBodyIsData` pins that heredoc bodies, stacked heredocs, `<<-` bodies, and herestring operands are treated as data while real operands on the introducer line are still flagged; `TestHeredocBodyNotRewritten` pins that `CorrectCommandPaths` leaves heredoc data byte-for-byte intact. - `pkg/shell/confinement_vectors_test.go` — `TestMidCommandCDEscapeBlocked` pins the red-team vectors (`cd && cat `, `cd && ls`, `cd ~ && ls .ssh`, `cd ; echo > file`) are refused; `TestMidCommandCDInTreeAllowed` and `TestCDEscapeHarmlessWithTrustedTempRoots` pin that legitimate in-tree and trusted-temp directory changes still work.