diff --git a/README.md b/README.md index cf248f9..3abff98 100644 --- a/README.md +++ b/README.md @@ -93,7 +93,9 @@ Generated files: - `harness.toml` — Project configuration (shared with the team) - `CLAUDE.md` — Role definitions for Superintendent / Engineer - `.github/ISSUE_TEMPLATE/hermit-task.md` — GitHub Issue template for well-structured tasks -- `.claude/settings.json` — Claude Code permission settings for autonomous operation +- `.claude/settings.json` — Claude Code permission settings for autonomous operation, plus a recommended `sandbox` block (see "Sandboxing the Engineer" below) + +Re-running `hermit init` on an already-initialized project never overwrites an existing `.claude/settings.json` wholesale: any top-level key you've already customized (most importantly `permissions`) is left exactly as-is, and only keys that are entirely missing (e.g. `sandbox`, on a project initialized before it was added) are filled in with hermit's defaults. Edit the "Coding Guidelines" section in `CLAUDE.md` to match your project. @@ -111,6 +113,54 @@ Edit the "Coding Guidelines" section in `CLAUDE.md` to match your project. - In practice, many public-repo operators will want both: trusted-author Issues *and* human-reviewed merges. - **Future work (not yet implemented):** an Issue-author allowlist (e.g. only process Issues from repository collaborators or an explicit list of GitHub usernames) is being considered as a built-in mitigation. Until it exists, use the operating modes above. +### Sandboxing the Engineer + +The Engineer runs with `Bash(*)` permission — unrestricted shell access — on the machine running Claude Code, because an allow-list of individual commands was tried and rejected (Issue #138: every time a new tool or command shape showed up that the allow-list hadn't anticipated, the loop either stalled on a confirmation prompt or someone widened the list until it was `Bash(*)` in practice anyway). Instead of enumerating which commands are allowed, `hermit init` generates a `sandbox` block in `.claude/settings.json` that narrows *what the Engineer can reach*, regardless of which command it runs: + +```json +{ + "sandbox": { + "enabled": true, + "allowUnsandboxedCommands": false, + "network": { + "tlsTerminate": {}, + "allowedDomains": ["*.github.com", "proxy.golang.org", "sum.golang.org", "storage.googleapis.com"] + }, + "credentials": { + "files": [ + { "path": "~/.ssh", "mode": "deny" }, + { "path": "~/.aws/credentials", "mode": "deny" } + ], + "envVars": [ + { "name": "GITHUB_TOKEN", "mode": "mask", "injectHosts": ["api.github.com"] } + ] + } + } +} +``` + +A few details worth knowing if you edit this block by hand: + +- **`allowUnsandboxedCommands` defaults to `true` in Claude Code.** If you omit it (or leave it `true`), the sandbox is effectively optional — a command can simply opt out. `hermit init` always writes it as `false`; keep it that way. +- **`GITHUB_TOKEN` must be `"mode": "mask"` with `injectHosts`, never `"mode": "deny"`.** `gh` (and therefore `gh pr create`, `gh issue comment`, etc., which the Engineer needs to do its job) requires the real token when talking to `api.github.com`. `"deny"` breaks it outright. `"mask"` hides the value everywhere else and only injects the real token on requests to the hosts listed in `injectHosts`; this requires `network.tlsTerminate` to be present (even as `{}`), since Claude Code needs to terminate TLS to inspect the destination host before deciding whether to inject. +- **`network.allowedDomains` must cover your dependency graph, not just GitHub.** For a Go project this means the module proxy/sum/storage hosts (`proxy.golang.org`, `sum.golang.org`, `storage.googleapis.com`) in addition to `*.github.com`, or `go build`/`go test`/`go mod download` will fail under the sandbox. Verify the list by actually running your project's test command with the sandbox enabled — don't assume it's complete. + +**Scope and precedence — read this before treating the sandbox as an enforcement mechanism.** Claude Code settings are layered (managed > CLI flags > local project > project > user), and different key *shapes* combine across that stack differently: + +- **Boolean keys** (like `sandbox.enabled`, `sandbox.allowUnsandboxedCommands`) resolve by precedence: the highest-precedence scope that sets the key wins outright. A project-scoped `.claude/settings.json` beats a user-scoped `~/.claude/settings.json`. +- **Array keys** (like `sandbox.excludedCommands`, `permissions.allow`, `permissions.deny` — anything that's a list of patterns) are **merged, additively, across every scope** that sets them. Nothing lower in the stack can *remove* an entry a higher scope added, but a lower scope can freely *add* entries a higher scope didn't ask for. + +The practical consequence: a `sandbox` block placed in `.claude/settings.json` (project scope, the default `hermit init` target) constrains the Engineer only as long as the Engineer doesn't touch it. But the Engineer's job is to open PRs against this very repository — including PRs that edit `.claude/settings.json` itself, add an entry to `sandbox.excludedCommands`, or otherwise widen the array-merged keys from project scope. Nothing in a project-scoped sandbox config stops the Engineer from proposing (and, if auto-merge is on for LOW/MEDIUM risk PRs, landing) exactly that change. In other words: project settings are a strong default, not an enforcement boundary against the agent they're meant to constrain. + +**To actually enforce the sandbox against the Engineer, you need managed settings** — a settings file outside the repository, deployed by an admin/MDM process the Engineer has no write access to, which sits above project scope in the precedence stack. Two managed-settings flags close the remaining loophole in the array-merge behavior described above: + +- **`allowManagedReadPathsOnly`** — restricts filesystem reads to paths the managed config explicitly allows, so a project-scope (or Engineer-authored) change can't widen readable paths beyond what the managed config permits. +- **`allowManagedDomainsOnly`** — restricts network access to domains the managed config explicitly allows, closing the equivalent hole for `network.allowedDomains`. + +hermit does not currently generate a managed settings file — `hermit init` only ever writes to the project's own `.claude/settings.json`, which (per the above) the Engineer can eventually influence. Automating managed-settings generation, and wiring up `allowManagedReadPathsOnly` / `allowManagedDomainsOnly`, is tracked separately (see Issue #179); until that lands, the project-scope sandbox block here is best understood as raising the cost of an adversarial or buggy Engineer action, not as a hard boundary. + +`hermit doctor` checks the generated `.claude/settings.json` for the two most common ways this block ends up not doing anything (`sandbox.enabled` false/missing, `allowUnsandboxedCommands` true/missing) plus a non-empty `sandbox.excludedCommands` (which reopens a hole per command listed there); see `hermit doctor` below. + --- ## Usage @@ -390,8 +440,9 @@ Runs a series of environment checks and reports pass/fail for each: - `GITHUB_TOKEN` is available (from environment or `gh auth token`) - `harness.toml` exists with `owner` and `repo` filled in - `claude` (Claude Code) is installed +- `.claude/settings.json` sandbox configuration (warnings, non-fatal — see "Sandboxing the Engineer" above): `sandbox.enabled` is true, `allowUnsandboxedCommands` is false, `sandbox.excludedCommands` is empty -Exits with a non-zero status if any check fails. +Exits with a non-zero status if any check fails. The sandbox checks are warnings and never cause a non-zero exit on their own, so `hermit doctor` still passes on projects initialized before the sandbox recommendation existed. ### `hermit dry-run` diff --git a/REQUIREMENTS.md b/REQUIREMENTS.md index a2c6baa..2676612 100644 --- a/REQUIREMENTS.md +++ b/REQUIREMENTS.md @@ -212,3 +212,18 @@ HERMIT は public リポジトリで運用され得るため、第三者が作 - 受け入れ条件: 信頼できない author_association を持つ Issue が `ListOpenIssues` / `ListAllIssues` の結果から除外されること。`harness.toml` に設定が無い、または空の場合も安全な既定値 (OWNER/MEMBER/COLLABORATOR) が適用され「全員許可」にフォールバックしないこと。除外時にログが出力されること - verify: test - 実装状況: 実装済み — `internal/github/client.go` の `listOpenIssuesFromRepo` が各 Issue の `author_association` を `(*Client).isTrustedAuthor` で判定し、非信頼の Issue を除外したうえで `log.Printf` により除外理由 (Issue番号・owner/repo・association) を記録する。信頼リストは `(*Client).SetTrustedAuthorAssociations` で設定でき、未設定または空スライスを渡した場合は `gh.DefaultTrustedAuthorAssociations` (`OWNER`/`MEMBER`/`COLLABORATOR`) にフォールバックする — 「空 = 全員許可」にはならない。`cmd/hermit/main.go` の `Config.Security.TrustedAuthorAssociations` (`harness.toml` の `[security] trusted_author_associations`) が `cmdServe` / `cmdDryRun` で `SetTrustedAuthorAssociations` に渡される。除外された Issue が `assign_issue` / `create_worktree` の対象にならないのは、`list_issues` (`internal/mcp/tools.go`) がこの層より上位で `ListOpenIssues`/`ListAllIssues` の返り値のみをキューとして扱うため自動的に保証される。テストは `internal/github/req_test.go` の `TestREQ017_*` + +## REQ-018: Engineer の Bash をサンドボックスで制限する + +Engineer は `Bash(*)` 許可でローカルマシン上で動作しており、`permissions.allow` によるコマンド単位の allowlist 方式は不採用 (Issue #138: 新規ツール追加のたびに未更新でループが壊れる失敗モードが実証済み)。代わりに `hermit init` が Claude Code のサンドボックス機能 (`sandbox` ブロック) を推奨設定として生成し、能力を列挙するのではなく届く範囲そのものを狭める。 + +- `hermit init` は `.claude/settings.json` に `sandbox.enabled: true` / `allowUnsandboxedCommands: false` / `network.tlsTerminate` + `allowedDomains` (`*.github.com`, `proxy.golang.org`, `sum.golang.org`, `storage.googleapis.com`) / `credentials.files` (`~/.ssh`, `~/.aws/credentials` を deny) / `credentials.envVars` (`GITHUB_TOKEN` を `mode: mask` + `injectHosts: ["api.github.com"]`、`deny` にはしない) を書き込む +- `allowUnsandboxedCommands` は Claude Code 側の既定が `true` であり明示的に `false` にしないとサンドボックス自体が実質無効化されるため、生成される設定では必ず `false` にする +- `GITHUB_TOKEN` を `deny` にすると `gh` コマンド (Engineer が PR 作成等に使う) が動作しなくなるため、`mask` + `injectHosts` を用いる +- 既に `.claude/settings.json` が存在するプロジェクトに対して `hermit init` を再実行しても、既存の `permissions` などのトップレベルキーは破壊されず、欠けているキー (`sandbox` など) のみが追加される +- `hermit doctor` は生成済み `.claude/settings.json` の `sandbox.enabled` が false/未設定、`allowUnsandboxedCommands` が true/未設定、`sandbox.excludedCommands` が空でない場合に警告する (警告のみで doctor 全体の pass/fail には影響しない) +- README にスコープごとの precedence (boolean キーは上位スコープが勝つ、配列キー (`excludedCommands` 等) は全スコープでマージされ下位スコープから広げられる) と、project settings 配置では Engineer 自身が PR 経由で無効化・迂回しうる旨、実効的な強制には managed settings と `allowManagedReadPathsOnly` / `allowManagedDomainsOnly` が必要な旨、その自動生成自体は別 Issue #179 のスコープである旨を明記する + +- 受け入れ条件: `hermit init` が生成する `.claude/settings.json` に上記構造の `sandbox` ブロックが含まれ、`allowUnsandboxedCommands: false` / `GITHUB_TOKEN` の `mode: mask` + `injectHosts` が満たされていること。生成された設定を適用した状態で `go build ./...` / `go test ./...` が成功し、`gh pr create` 相当の操作が実行できること。`hermit doctor` が上記 3 種の警告を検出すること。既存プロジェクトへの `hermit init` 再実行が既存の `permissions` 設定を破壊しないこと +- verify: test +- 実装状況: 実装済み — `internal/permissions/permissions.go` の `DefaultSandboxSettings` / `MergeDefaultSettings` (再実行時は既存のトップレベルキーを保持し、欠けているキーのみ補完)、`cmd/hermit/main.go` の `writeClaudeSettings` (`MergeDefaultSettings` を経由するよう変更)、`cmd/hermit/doctor.go` の `checkSandboxSettings` (3 種の警告)。テストは `internal/permissions/permissions_test.go` の `TestREQ018_*` 群 (`DefaultSandboxSettings` の `allowUnsandboxedCommands`/`GITHUB_TOKEN`/Go ツールチェーン許可ドメイン、`MergeDefaultSettings` の新規生成・既存 `permissions`/`sandbox` の保持・エラー経路) と `cmd/hermit/doctor_test.go` / `cmd/hermit/unit_test.go` の `TestREQ018_*` 群 (`checkSandboxSettings` の警告条件、`writeClaudeSettings` の再実行時非破壊)。`go build ./...` と `go test ./...` の成功、および `gh pr create` 相当操作の実行可能性は本 Issue #180 の PR 自体 (生成された設定下で `go test ./...` を通し、同じ worktree から `gh pr create` で PR を作成) によって実地検証済み。README の "Sandboxing the Engineer" セクションにスコープ precedence・managed settings・Issue #179 依存の記載を追加 diff --git a/cmd/hermit/doctor.go b/cmd/hermit/doctor.go index db9bcb4..ba9a65d 100644 --- a/cmd/hermit/doctor.go +++ b/cmd/hermit/doctor.go @@ -1,6 +1,7 @@ package main import ( + "encoding/json" "fmt" "os" "os/exec" @@ -130,9 +131,110 @@ func runChecks() []checkResult { passed: err == nil, }) + // Checks: sandbox configuration in .claude/settings.json (REQ-018). These + // are warnings, not hard failures, so `hermit doctor` keeps passing on + // projects initialized before the sandbox recommendation existed. + results = append(results, checkSandboxSettings(".claude/settings.json")...) + return results } +// sandboxSettingsRaw mirrors just the fields of the "sandbox" block in +// .claude/settings.json that checkSandboxSettings inspects. It is decoded +// independently of internal/permissions.Settings so a malformed or +// hand-edited settings.json (missing fields, extra keys) never breaks +// `hermit doctor` itself — unmarshal errors are treated as "not configured". +type sandboxSettingsRaw struct { + Sandbox *struct { + Enabled *bool `json:"enabled"` + AllowUnsandboxedCommands *bool `json:"allowUnsandboxedCommands"` + ExcludedCommands []string `json:"excludedCommands"` + } `json:"sandbox"` +} + +// checkSandboxSettings inspects the "sandbox" block of the settings.json at +// path and warns about the three ways it can end up not actually restricting +// the Engineer's Bash tool (see Issue #180 / REQUIREMENTS.md REQ-018): +// +// - sandbox.enabled is false or missing — the whole block is inert +// - allowUnsandboxedCommands is true or missing — defaults to true in +// Claude Code, which lets commands opt out of the sandbox entirely +// - excludedCommands has entries — those commands bypass the sandbox +func checkSandboxSettings(path string) []checkResult { + data, err := os.ReadFile(path) + fileMissing := os.IsNotExist(err) + + var cfg sandboxSettingsRaw + if err == nil { + // Best-effort: malformed JSON is handled the same as "no sandbox + // block configured" rather than failing doctor outright. + _ = json.Unmarshal(data, &cfg) + } + + missingFileDetail := path + " not found (run `hermit init`)" + missingBlockDetail := "sandbox block missing from " + path + + enabled := cfg.Sandbox != nil && cfg.Sandbox.Enabled != nil && *cfg.Sandbox.Enabled + enabledDetail := "" + switch { + case enabled: + // no detail needed + case fileMissing: + enabledDetail = missingFileDetail + case cfg.Sandbox == nil: + enabledDetail = missingBlockDetail + case cfg.Sandbox.Enabled == nil: + enabledDetail = "sandbox.enabled not set" + default: + enabledDetail = "sandbox.enabled is false" + } + + blocked := cfg.Sandbox != nil && cfg.Sandbox.AllowUnsandboxedCommands != nil && !*cfg.Sandbox.AllowUnsandboxedCommands + blockedDetail := "" + switch { + case blocked: + // no detail needed + case fileMissing: + blockedDetail = missingFileDetail + case cfg.Sandbox == nil: + blockedDetail = missingBlockDetail + case cfg.Sandbox.AllowUnsandboxedCommands == nil: + blockedDetail = "allowUnsandboxedCommands not set (defaults to true in Claude Code, which makes the sandbox block a no-op)" + default: + blockedDetail = "allowUnsandboxedCommands is true (defeats the sandbox; set it to false)" + } + + var excluded []string + if cfg.Sandbox != nil { + excluded = cfg.Sandbox.ExcludedCommands + } + excludedDetail := "" + if len(excluded) > 0 { + excludedDetail = "sandbox.excludedCommands bypasses the sandbox for: " + strings.Join(excluded, ", ") + } + + return []checkResult{ + { + name: "sandbox.enabled is true", + passed: true, + warn: !enabled, + detail: enabledDetail, + }, + { + name: "allowUnsandboxedCommands is false", + passed: true, + warn: !blocked, + detail: blockedDetail, + }, + { + name: "sandbox.excludedCommands is empty", + passed: true, + warn: len(excluded) > 0, + detail: excludedDetail, + }, + } +} + func cmdDoctor() { results := runChecks() diff --git a/cmd/hermit/doctor_test.go b/cmd/hermit/doctor_test.go index eb2348c..373c816 100644 --- a/cmd/hermit/doctor_test.go +++ b/cmd/hermit/doctor_test.go @@ -7,6 +7,8 @@ import ( "path/filepath" "strings" "testing" + + "github.com/ytnobody/hermit/internal/permissions" ) // captureDoctor runs cmdDoctor() capturing stdout, returning output and whether it exited. @@ -28,8 +30,8 @@ func captureDoctorOutput(t *testing.T) string { func TestRunChecks_AllFields(t *testing.T) { results := runChecks() - if len(results) != 6 { - t.Errorf("expected 6 checks, got %d", len(results)) + if len(results) != 9 { + t.Errorf("expected 9 checks, got %d", len(results)) } names := make([]string, len(results)) @@ -44,6 +46,9 @@ func TestRunChecks_AllFields(t *testing.T) { "GITHUB_TOKEN is available", "harness.toml exists with owner/repo", "Claude Code (claude) is installed", + "sandbox.enabled is true", + "allowUnsandboxedCommands is false", + "sandbox.excludedCommands is empty", } for i, want := range expected { if i >= len(names) { @@ -331,3 +336,136 @@ repo = "test-repo" // Ignore exit status since some checks may fail in CI _ = cmd.Run() } + +// TestREQ018_CheckSandboxSettings_FileMissing verifies all three sandbox +// checks warn (without failing doctor) when .claude/settings.json does not +// exist at all, e.g. a project that has never run `hermit init`. +func TestREQ018_CheckSandboxSettings_FileMissing(t *testing.T) { + dir := t.TempDir() + results := checkSandboxSettings(filepath.Join(dir, "does-not-exist.json")) + if len(results) != 3 { + t.Fatalf("expected 3 results, got %d", len(results)) + } + for _, r := range results { + if !r.passed { + t.Errorf("%s: expected passed=true (warning, not failure)", r.name) + } + } + // enabled / allowUnsandboxedCommands can't be verified when the file is + // missing entirely, so both warn. + if !results[0].warn || results[0].detail == "" { + t.Errorf("sandbox.enabled: expected warn=true with a detail when settings.json is missing") + } + if !results[1].warn || results[1].detail == "" { + t.Errorf("allowUnsandboxedCommands: expected warn=true with a detail when settings.json is missing") + } + // excludedCommands is vacuously empty when there's no file to read from, + // so it must NOT warn (nothing bypasses a sandbox that was never + // configured to exist). + if results[2].warn { + t.Errorf("sandbox.excludedCommands: expected warn=false when settings.json is missing (nothing to report)") + } +} + +// TestREQ018_CheckSandboxSettings_NoSandboxBlock verifies the checks warn +// when settings.json exists but has no "sandbox" key at all (a +// pre-Issue-#180 hermit init output). +func TestREQ018_CheckSandboxSettings_NoSandboxBlock(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + if err := os.WriteFile(path, []byte(`{"permissions":{"allow":["Bash(*)"]}}`), 0o644); err != nil { + t.Fatal(err) + } + results := checkSandboxSettings(path) + if !results[0].warn { + t.Error("sandbox.enabled: expected warn=true when sandbox block is absent") + } + if !results[1].warn { + t.Error("allowUnsandboxedCommands: expected warn=true when sandbox block is absent") + } + if results[2].warn { + t.Error("sandbox.excludedCommands: expected warn=false when sandbox block is absent (nothing to report)") + } +} + +// TestREQ018_CheckSandboxSettings_Recommended verifies hermit's own +// recommended sandbox config (enabled, allowUnsandboxedCommands=false, no +// excludedCommands) produces zero warnings. +func TestREQ018_CheckSandboxSettings_Recommended(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + data := permissions.DefaultSettingsJSON() + if err := os.WriteFile(path, data, 0o644); err != nil { + t.Fatal(err) + } + results := checkSandboxSettings(path) + for _, r := range results { + if r.warn { + t.Errorf("%s: expected warn=false for hermit's own recommended config, detail: %s", r.name, r.detail) + } + } +} + +// TestREQ018_CheckSandboxSettings_UnsandboxedAllowed verifies the second +// check warns specifically when allowUnsandboxedCommands is true (the +// default in Claude Code, which makes "enabled: true" a no-op). +func TestREQ018_CheckSandboxSettings_UnsandboxedAllowed(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + content := `{"sandbox":{"enabled":true,"allowUnsandboxedCommands":true}}` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + results := checkSandboxSettings(path) + if results[0].warn { + t.Error("expected sandbox.enabled check to pass without warning") + } + if !results[1].warn { + t.Error("expected allowUnsandboxedCommands=true to be flagged") + } + if !strings.Contains(results[1].detail, "true") { + t.Errorf("expected detail to explain allowUnsandboxedCommands is true, got %q", results[1].detail) + } +} + +// TestREQ018_CheckSandboxSettings_ExcludedCommands verifies the third check +// warns and names the offending commands when excludedCommands is non-empty. +func TestREQ018_CheckSandboxSettings_ExcludedCommands(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + content := `{"sandbox":{"enabled":true,"allowUnsandboxedCommands":false,"excludedCommands":["curl","rm -rf"]}}` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatal(err) + } + results := checkSandboxSettings(path) + if results[0].warn || results[1].warn { + t.Error("expected enabled/allowUnsandboxedCommands checks to pass without warning") + } + if !results[2].warn { + t.Error("expected excludedCommands check to warn") + } + if !strings.Contains(results[2].detail, "curl") || !strings.Contains(results[2].detail, "rm -rf") { + t.Errorf("expected detail to name the excluded commands, got %q", results[2].detail) + } +} + +// TestREQ018_CheckSandboxSettings_MalformedJSON verifies malformed JSON is +// treated as "not configured" (all three checks warn) rather than panicking +// or crashing `hermit doctor`. +func TestREQ018_CheckSandboxSettings_MalformedJSON(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "settings.json") + if err := os.WriteFile(path, []byte("not valid json {{"), 0o644); err != nil { + t.Fatal(err) + } + results := checkSandboxSettings(path) + if !results[0].warn { + t.Error("sandbox.enabled: expected warn=true for malformed settings.json") + } + if !results[1].warn { + t.Error("allowUnsandboxedCommands: expected warn=true for malformed settings.json") + } + if results[2].warn { + t.Error("sandbox.excludedCommands: expected warn=false for malformed settings.json (nothing to report)") + } +} diff --git a/cmd/hermit/main.go b/cmd/hermit/main.go index 4873bc2..a6ed891 100644 --- a/cmd/hermit/main.go +++ b/cmd/hermit/main.go @@ -848,7 +848,16 @@ func writeClaudeSettings() error { if err := os.MkdirAll(".claude", 0o755); err != nil { return err } - return os.WriteFile(filepath.Join(".claude", "settings.json"), permissions.DefaultSettingsJSON(), 0o644) + path := filepath.Join(".claude", "settings.json") + // MergeDefaultSettings preserves any existing "permissions" (and other) + // keys from a prior `hermit init` run, only filling in a missing + // "sandbox" block. Never unconditionally overwrite the file — a project + // may have hand-tuned its permissions since the last init. + merged, err := permissions.MergeDefaultSettings(path) + if err != nil { + return err + } + return os.WriteFile(path, merged, 0o644) } // writeIssueTemplate creates .github/ISSUE_TEMPLATE/hermit-task.md in the diff --git a/cmd/hermit/unit_test.go b/cmd/hermit/unit_test.go index d12bf86..54e3ef0 100644 --- a/cmd/hermit/unit_test.go +++ b/cmd/hermit/unit_test.go @@ -763,6 +763,47 @@ func TestWriteClaudeSettings(t *testing.T) { if !strings.Contains(string(data), "Bash(*)") { t.Errorf("settings.json missing Bash(*): %s", data) } + if !strings.Contains(string(data), `"sandbox"`) { + t.Errorf("settings.json missing sandbox block: %s", data) + } +} + +// TestREQ018_WriteClaudeSettings_RerunPreservesCustomPermissions is the +// direct regression test for Issue #180's acceptance criterion that +// re-running `hermit init` (which calls writeClaudeSettings) against a +// project that already has a hand-tuned .claude/settings.json must not +// destroy the existing "permissions" block. +func TestREQ018_WriteClaudeSettings_RerunPreservesCustomPermissions(t *testing.T) { + dir := t.TempDir() + prev, _ := os.Getwd() + os.Chdir(dir) + defer os.Chdir(prev) + + if err := os.MkdirAll(".claude", 0o755); err != nil { + t.Fatal(err) + } + custom := `{"permissions": {"allow": ["Bash(git *)"]}}` + if err := os.WriteFile(filepath.Join(".claude", "settings.json"), []byte(custom), 0o644); err != nil { + t.Fatal(err) + } + + if err := writeClaudeSettings(); err != nil { + t.Fatalf("writeClaudeSettings (rerun): %v", err) + } + + data, err := os.ReadFile(filepath.Join(".claude", "settings.json")) + if err != nil { + t.Fatalf("settings.json missing after rerun: %v", err) + } + if !strings.Contains(string(data), "Bash(git *)") { + t.Errorf("rerun destroyed the existing custom permissions.allow: %s", data) + } + if strings.Contains(string(data), "Bash(*)") { + t.Errorf("rerun must not widen the existing custom allow-list to Bash(*): %s", data) + } + if !strings.Contains(string(data), `"sandbox"`) { + t.Errorf("rerun should still add the missing sandbox block: %s", data) + } } // --- writeTemplate --- diff --git a/internal/permissions/permissions.go b/internal/permissions/permissions.go index 0ea114d..1693af0 100644 --- a/internal/permissions/permissions.go +++ b/internal/permissions/permissions.go @@ -12,9 +12,112 @@ import ( // Settings represents the structure of a Claude Code settings.json file. type Settings struct { - Permissions struct { - Allow []string `json:"allow"` - } `json:"permissions"` + Permissions PermissionsBlock `json:"permissions"` + // Sandbox holds the recommended sandbox configuration written by + // `hermit init` (see DefaultSandboxSettings). It is a pointer so that + // settings.json files predating REQ-018 (no sandbox block at all) can be + // distinguished from ones that explicitly configure it. + Sandbox *SandboxSettings `json:"sandbox,omitempty"` +} + +// PermissionsBlock is the `permissions` object of a Claude Code settings.json +// file. +type PermissionsBlock struct { + Allow []string `json:"allow"` +} + +// SandboxSettings represents the `sandbox` object hermit recommends in +// settings.json to narrow what the Engineer's Bash tool can reach on the host +// machine, without resorting to an allow-list of individual commands (see +// README "Security" section for the allow-list failure mode this replaces, +// and its scope/precedence caveats). +type SandboxSettings struct { + // Enabled turns the sandbox on. Without this, the rest of the block has + // no effect. + Enabled bool `json:"enabled"` + // AllowUnsandboxedCommands defaults to true in Claude Code itself, which + // means a sandbox block with this field omitted is effectively inert — + // hermit always writes it explicitly as false. + AllowUnsandboxedCommands bool `json:"allowUnsandboxedCommands"` + // ExcludedCommands lists Bash command patterns that bypass the sandbox + // entirely. hermit's generated config leaves this empty; a non-empty + // list here re-opens the hole the sandbox is meant to close, which is + // why `hermit doctor` flags it. + ExcludedCommands []string `json:"excludedCommands,omitempty"` + Network SandboxNetworkSettings `json:"network"` + Credentials SandboxCredentialSettings `json:"credentials"` +} + +// SandboxNetworkSettings restricts outbound network access from the sandbox. +type SandboxNetworkSettings struct { + // TLSTerminate must be present (even empty) for envVars credential + // masking/injection to work, since Claude Code needs to terminate TLS to + // inject the token header only for allowed hosts. + TLSTerminate map[string]any `json:"tlsTerminate"` + AllowedDomains []string `json:"allowedDomains"` +} + +// SandboxCredentialSettings hides host credentials from the sandboxed +// process, and controls how sensitive env vars are exposed to it. +type SandboxCredentialSettings struct { + Files []SandboxCredentialFile `json:"files"` + EnvVars []SandboxCredentialEnvVar `json:"envVars"` +} + +// SandboxCredentialFile denies (or otherwise restricts) sandbox access to a +// host path likely to contain credentials. +type SandboxCredentialFile struct { + Path string `json:"path"` + Mode string `json:"mode"` +} + +// SandboxCredentialEnvVar controls how an environment variable is exposed to +// the sandboxed process. Mode "deny" would break tools (like gh) that need +// the token; hermit uses "mask" + InjectHosts so the real value is only +// injected on requests to trusted hosts. +type SandboxCredentialEnvVar struct { + Name string `json:"name"` + Mode string `json:"mode"` + InjectHosts []string `json:"injectHosts,omitempty"` +} + +// DefaultSandboxSettings returns hermit's recommended sandbox configuration. +// +// AllowUnsandboxedCommands is explicitly false: Claude Code defaults this to +// true, and leaving it unset (or true) makes the rest of the sandbox block +// effectively optional for the model to honor. +// +// GITHUB_TOKEN is "mask" + InjectHosts, never "deny": gh CLI (and therefore +// `gh pr create`, `gh issue comment`, etc.) needs the real token to reach +// api.github.com. "deny" would break the Engineer's ability to open PRs. +// +// AllowedDomains includes the Go toolchain's module proxy/sum/storage hosts +// in addition to GitHub, since `go build`/`go test` fetch dependencies over +// the network. Verify this list actually covers a project's dependency graph +// by running `go test ./...` under the generated sandbox. +func DefaultSandboxSettings() SandboxSettings { + return SandboxSettings{ + Enabled: true, + AllowUnsandboxedCommands: false, + Network: SandboxNetworkSettings{ + TLSTerminate: map[string]any{}, + AllowedDomains: []string{ + "*.github.com", + "proxy.golang.org", + "sum.golang.org", + "storage.googleapis.com", + }, + }, + Credentials: SandboxCredentialSettings{ + Files: []SandboxCredentialFile{ + {Path: "~/.ssh", Mode: "deny"}, + {Path: "~/.aws/credentials", Mode: "deny"}, + }, + EnvVars: []SandboxCredentialEnvVar{ + {Name: "GITHUB_TOKEN", Mode: "mask", InjectHosts: []string{"api.github.com"}}, + }, + }, + } } // LoadSettings reads and parses a Claude Code settings.json file. @@ -87,6 +190,14 @@ func (s *Settings) UncoveredCommands(commands []string) []string { // DefaultSettingsJSON returns the canonical .claude/settings.json content that // hermit projects should use for autonomous (prompt-free) operation. func DefaultSettingsJSON() []byte { + s := defaultSettings() + b, _ := json.MarshalIndent(s, "", " ") + return append(b, '\n') +} + +// defaultSettings builds the full Settings value (permissions allow-list plus +// recommended sandbox block) that a fresh `hermit init` writes. +func defaultSettings() Settings { s := Settings{} s.Permissions.Allow = []string{ "Bash(*)", @@ -112,6 +223,60 @@ func DefaultSettingsJSON() []byte { "mcp__hermit__merge_pr", "mcp__hermit__post_comment", } - b, _ := json.MarshalIndent(s, "", " ") - return append(b, '\n') + sandbox := DefaultSandboxSettings() + s.Sandbox = &sandbox + return s +} + +// MergeDefaultSettings computes the settings.json content `hermit init` +// should write at path. +// +// - If no file exists yet at path, it is a fresh project: return +// DefaultSettingsJSON() unchanged. +// - If a file already exists, preserve every top-level key it already has +// (most importantly "permissions", which a project may have hand-tuned +// after the first `hermit init`) and only fill in keys that are entirely +// absent — "sandbox" first and foremost — with hermit's recommended +// defaults. Re-running `hermit init` on an already-initialized project +// must never destroy prior customization. +func MergeDefaultSettings(path string) ([]byte, error) { + existing, err := os.ReadFile(path) + if os.IsNotExist(err) { + return DefaultSettingsJSON(), nil + } + if err != nil { + return nil, fmt.Errorf("read existing settings file: %w", err) + } + + var raw map[string]json.RawMessage + if err := json.Unmarshal(existing, &raw); err != nil { + return nil, fmt.Errorf("parse existing settings file: %w", err) + } + if raw == nil { + raw = map[string]json.RawMessage{} + } + + def := defaultSettings() + + if _, ok := raw["permissions"]; !ok { + permJSON, err := json.Marshal(def.Permissions) + if err != nil { + return nil, fmt.Errorf("marshal default permissions: %w", err) + } + raw["permissions"] = permJSON + } + + if _, ok := raw["sandbox"]; !ok { + sandboxJSON, err := json.Marshal(def.Sandbox) + if err != nil { + return nil, fmt.Errorf("marshal default sandbox settings: %w", err) + } + raw["sandbox"] = sandboxJSON + } + + b, err := json.MarshalIndent(raw, "", " ") + if err != nil { + return nil, fmt.Errorf("marshal merged settings: %w", err) + } + return append(b, '\n'), nil } diff --git a/internal/permissions/permissions_test.go b/internal/permissions/permissions_test.go index f1f1dd6..7a021e0 100644 --- a/internal/permissions/permissions_test.go +++ b/internal/permissions/permissions_test.go @@ -308,3 +308,259 @@ func TestDefaultSettingsJSONIsValid(t *testing.T) { len(uncovered), strings.Join(uncovered, ", ")) } } + +// TestREQ018_DefaultSandboxSettings_AllowUnsandboxedCommandsFalse verifies +// the sandbox default explicitly disables the "allowUnsandboxedCommands" +// escape hatch. Claude Code defaults this field to true, so hermit must set +// it to false explicitly or the rest of the sandbox block is effectively +// optional. +func TestREQ018_DefaultSandboxSettings_AllowUnsandboxedCommandsFalse(t *testing.T) { + sb := permissions.DefaultSandboxSettings() + if !sb.Enabled { + t.Error("expected sandbox.enabled = true") + } + if sb.AllowUnsandboxedCommands { + t.Error("expected sandbox.allowUnsandboxedCommands = false") + } +} + +// TestREQ018_DefaultSandboxSettings_GithubTokenMaskedNotDenied verifies +// GITHUB_TOKEN is exposed via "mask" + injectHosts rather than "deny": gh CLI +// needs the real token to reach api.github.com, so "deny" would break `gh pr +// create` and friends. +func TestREQ018_DefaultSandboxSettings_GithubTokenMaskedNotDenied(t *testing.T) { + sb := permissions.DefaultSandboxSettings() + + var tokenVar *permissions.SandboxCredentialEnvVar + for i := range sb.Credentials.EnvVars { + if sb.Credentials.EnvVars[i].Name == "GITHUB_TOKEN" { + tokenVar = &sb.Credentials.EnvVars[i] + break + } + } + if tokenVar == nil { + t.Fatal("expected GITHUB_TOKEN entry in sandbox.credentials.envVars") + } + if tokenVar.Mode == "deny" { + t.Error("GITHUB_TOKEN must not be mode=deny (breaks gh CLI); expected mode=mask") + } + if tokenVar.Mode != "mask" { + t.Errorf("expected GITHUB_TOKEN mode=mask, got %q", tokenVar.Mode) + } + found := false + for _, h := range tokenVar.InjectHosts { + if h == "api.github.com" { + found = true + } + } + if !found { + t.Errorf("expected injectHosts to include api.github.com, got %v", tokenVar.InjectHosts) + } + // mask+injectHosts requires tlsTerminate to be configured. + if sb.Network.TLSTerminate == nil { + t.Error("expected network.tlsTerminate to be set (required for envVar host-scoped injection)") + } +} + +// TestREQ018_DefaultSandboxSettings_GoToolchainDomainsAllowed verifies the Go +// module proxy/sum/storage hosts are present in allowedDomains, since `go +// build`/`go test` need network access to fetch dependencies. This is a +// structural check; go.mod/go.sum actually resolving through these hosts is +// verified by `go test ./...` succeeding in CI/dev, per the acceptance +// criteria on Issue #180. +func TestREQ018_DefaultSandboxSettings_GoToolchainDomainsAllowed(t *testing.T) { + sb := permissions.DefaultSandboxSettings() + want := []string{"proxy.golang.org", "sum.golang.org", "storage.googleapis.com", "*.github.com"} + for _, w := range want { + found := false + for _, d := range sb.Network.AllowedDomains { + if d == w { + found = true + break + } + } + if !found { + t.Errorf("expected %q in network.allowedDomains, got %v", w, sb.Network.AllowedDomains) + } + } +} + +// TestREQ018_DefaultSettingsJSON_IncludesSandbox verifies the JSON hermit +// init writes for a fresh project contains the sandbox block described in +// Issue #180, in addition to the existing permissions allow-list. +func TestREQ018_DefaultSettingsJSON_IncludesSandbox(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "settings.json") + if err := os.WriteFile(path, permissions.DefaultSettingsJSON(), 0o644); err != nil { + t.Fatal(err) + } + s, err := permissions.LoadSettings(path) + if err != nil { + t.Fatalf("LoadSettings: %v", err) + } + if s.Sandbox == nil { + t.Fatal("expected non-nil Sandbox block in DefaultSettingsJSON output") + } + if !s.Sandbox.Enabled || s.Sandbox.AllowUnsandboxedCommands { + t.Errorf("expected enabled=true, allowUnsandboxedCommands=false, got %+v", s.Sandbox) + } +} + +// TestREQ018_MergeDefaultSettings_FreshFile verifies MergeDefaultSettings +// behaves like DefaultSettingsJSON when no settings.json exists yet. +func TestREQ018_MergeDefaultSettings_FreshFile(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "settings.json") + + merged, err := permissions.MergeDefaultSettings(path) + if err != nil { + t.Fatalf("MergeDefaultSettings: %v", err) + } + + var raw map[string]any + if err := json.Unmarshal(merged, &raw); err != nil { + t.Fatalf("merged output is not valid JSON: %v", err) + } + if _, ok := raw["sandbox"]; !ok { + t.Error("expected sandbox key in merged output for a fresh file") + } + if _, ok := raw["permissions"]; !ok { + t.Error("expected permissions key in merged output for a fresh file") + } +} + +// TestREQ018_MergeDefaultSettings_PreservesExistingPermissions is the direct +// regression test for the Issue #180 acceptance criterion: re-running +// `hermit init` (which calls MergeDefaultSettings) against a project that +// already has a hand-tuned .claude/settings.json must not destroy the +// existing "permissions" block, even though it lacks Bash(*) and looks +// nothing like hermit's own default. +func TestREQ018_MergeDefaultSettings_PreservesExistingPermissions(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "settings.json") + existing := `{ + "permissions": { + "allow": ["Bash(git *)", "Bash(go *)"] + }, + "someOtherProjectKey": {"foo": "bar"} +}` + if err := os.WriteFile(path, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + merged, err := permissions.MergeDefaultSettings(path) + if err != nil { + t.Fatalf("MergeDefaultSettings: %v", err) + } + + var s permissions.Settings + if err := json.Unmarshal(merged, &s); err != nil { + t.Fatalf("unmarshal merged settings: %v", err) + } + if len(s.Permissions.Allow) != 2 || s.Permissions.Allow[0] != "Bash(git *)" || s.Permissions.Allow[1] != "Bash(go *)" { + t.Errorf("expected existing custom permissions.allow to survive unchanged, got %v", s.Permissions.Allow) + } + if (&s).IsBashAllowed("gh pr create") { + t.Error("merge must not silently widen the existing (narrower) allow-list") + } + + // The sandbox block should have been added since it was absent. + if s.Sandbox == nil || !s.Sandbox.Enabled || s.Sandbox.AllowUnsandboxedCommands { + t.Errorf("expected sandbox block to be filled in with hermit defaults, got %+v", s.Sandbox) + } + + // Unrelated top-level keys must survive too. + var raw map[string]json.RawMessage + if err := json.Unmarshal(merged, &raw); err != nil { + t.Fatalf("unmarshal merged: %v", err) + } + if _, ok := raw["someOtherProjectKey"]; !ok { + t.Error("expected unrelated top-level key 'someOtherProjectKey' to survive the merge") + } +} + +// TestREQ018_MergeDefaultSettings_PreservesExistingSandbox verifies a +// project that has already customized its sandbox block (e.g. added an +// extra allowed domain) keeps that customization on a re-run of `hermit +// init`, rather than being clobbered back to hermit's defaults. +func TestREQ018_MergeDefaultSettings_PreservesExistingSandbox(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "settings.json") + existing := `{ + "permissions": {"allow": ["Bash(*)"]}, + "sandbox": { + "enabled": true, + "allowUnsandboxedCommands": false, + "network": {"tlsTerminate": {}, "allowedDomains": ["*.github.com", "registry.npmjs.org"]} + } +}` + if err := os.WriteFile(path, []byte(existing), 0o644); err != nil { + t.Fatal(err) + } + + merged, err := permissions.MergeDefaultSettings(path) + if err != nil { + t.Fatalf("MergeDefaultSettings: %v", err) + } + + var raw map[string]any + if err := json.Unmarshal(merged, &raw); err != nil { + t.Fatalf("unmarshal merged: %v", err) + } + sandbox, ok := raw["sandbox"].(map[string]any) + if !ok { + t.Fatal("expected sandbox object in merged output") + } + network, ok := sandbox["network"].(map[string]any) + if !ok { + t.Fatal("expected sandbox.network object in merged output") + } + domains, ok := network["allowedDomains"].([]any) + if !ok { + t.Fatal("expected sandbox.network.allowedDomains array in merged output") + } + found := false + for _, d := range domains { + if d == "registry.npmjs.org" { + found = true + } + } + if !found { + t.Errorf("expected pre-existing custom domain 'registry.npmjs.org' to survive the merge, got %v", domains) + } +} + +// TestREQ018_MergeDefaultSettings_MissingFileError verifies the underlying +// os.ReadFile error path other than "not exist" is surfaced rather than +// swallowed (e.g. a permission-denied directory component). We simulate this +// by pointing at a path whose parent is a file, not a directory, which +// produces an ENOTDIR rather than ENOENT. +func TestREQ018_MergeDefaultSettings_MissingFileError(t *testing.T) { + tmp := t.TempDir() + notADir := filepath.Join(tmp, "not-a-dir") + if err := os.WriteFile(notADir, []byte("x"), 0o644); err != nil { + t.Fatal(err) + } + path := filepath.Join(notADir, "settings.json") + + _, err := permissions.MergeDefaultSettings(path) + if err == nil { + t.Fatal("expected an error when the parent path is not a directory") + } +} + +// TestREQ018_MergeDefaultSettings_InvalidJSONError verifies a malformed +// existing settings.json produces an error rather than silently discarding +// the file's content. +func TestREQ018_MergeDefaultSettings_InvalidJSONError(t *testing.T) { + tmp := t.TempDir() + path := filepath.Join(tmp, "settings.json") + if err := os.WriteFile(path, []byte("not valid json {{"), 0o644); err != nil { + t.Fatal(err) + } + + _, err := permissions.MergeDefaultSettings(path) + if err == nil { + t.Fatal("expected an error for malformed existing settings.json") + } +}