Skip to content

feat(security): restrict Engineer Bash via sandbox instead of allowlist (Closes #180) - #188

Merged
ytnobody merged 1 commit into
developfrom
hermit/ytnobody/issue-180
Jul 28, 2026
Merged

feat(security): restrict Engineer Bash via sandbox instead of allowlist (Closes #180)#188
ytnobody merged 1 commit into
developfrom
hermit/ytnobody/issue-180

Conversation

@ytnobody

@ytnobody ytnobody commented Jul 28, 2026

Copy link
Copy Markdown
Owner

Summary

Implements Issue #180: hermit init / hermit doctor / README changes to constrain the Engineer's Bash(*) access via Claude Code's sandbox feature instead of an allowlist (allowlist approach already rejected per #138 — every new tool/command shape breaks it).

  • hermit init now generates a sandbox block in .claude/settings.json:
    • enabled: true, allowUnsandboxedCommands: false (Claude Code defaults this to true, which would make the block a no-op if left unset)
    • network.tlsTerminate: {} + allowedDomains: ["*.github.com", "proxy.golang.org", "sum.golang.org", "storage.googleapis.com"] (Go toolchain hosts included so go build/go test keep working under the sandbox)
    • credentials.files: ~/.ssh and ~/.aws/credentials denied
    • credentials.envVars: GITHUB_TOKEN uses mode: "mask" + injectHosts: ["api.github.com"], never "deny" (deny would break gh, which the Engineer needs for gh pr create etc.)
  • Re-running hermit init on an already-initialized project (internal/permissions.MergeDefaultSettings) preserves every existing top-level key in .claude/settings.json — most importantly permissions — and only fills in keys that are entirely absent (e.g. sandbox on a pre-Engineer の Bash を allowlist ではなくサンドボックスで制限する(hermit init / doctor / README) #180 project). Verified manually: ran hermit init against this repo's own real .claude/settings.json and confirmed the existing (much longer, hand-grown) permissions.allow list survived byte-for-byte while sandbox was added.
  • hermit doctor (cmd/hermit/doctor.go) gained three warning-level checks (checkSandboxSettings): sandbox.enabled false/missing, allowUnsandboxedCommands true/missing, sandbox.excludedCommands non-empty. These are warnings, not failures, so doctor still passes cleanly on projects that predate the sandbox recommendation.
  • README gained a "Sandboxing the Engineer" section documenting the scope/precedence caveat called out in the Issue: boolean keys (sandbox.enabled etc.) resolve by scope precedence, but array keys merge additively across every scope (sandbox.excludedCommands, permissions.allow, ...), so nothing in a project-scoped .claude/settings.json stops the Engineer from proposing a PR that widens or bypasses it. Real enforcement requires managed settings plus allowManagedReadPathsOnly / allowManagedDomainsOnly, whose automated generation is explicitly out of scope here and tracked in 制御面(risk/permissions/readiness/harness.toml/.claude)への変更を HIGH に昇格し、自動マージから隔離する #179.
  • REQUIREMENTS.md gained REQ-018 with REQ-ID-named tests.

Assumption (ambiguous point, no human reply expected mid-run)

The Issue didn't specify whether hermit doctor's new sandbox checks should be hard failures (non-zero exit) or warnings. I made them warnings (warn: true, passed: true), matching the existing pattern for the snap-gh check, so that hermit doctor doesn't start failing on every project that predates this feature. The acceptance criteria only say doctor should "detect and warn" (検出して警告する), which is consistent with this choice.

Test plan

  • go build ./...
  • go test ./... (full suite, all packages pass)
  • gofmt -l . clean
  • Manually ran go run ./cmd/hermit init against this repo's real (pre-existing) .claude/settings.json and confirmed: existing permissions.allow preserved verbatim, sandbox block added with the exact structure from the Issue
  • Manually ran go run ./cmd/hermit doctor against the real (still sandbox-less until this PR merges and init is rerun) .claude/settings.json and confirmed the three new checks warn without failing the overall run

🤖 Generated with Claude Code

@ytnobody

Copy link
Copy Markdown
Owner Author

Superintendentによるレビュー(PR #188, Issue #180)

evaluate_risk の判定は HIGH(500行以上、かつ cmd/hermit/doctor.gocmd/hermit/main.go が高リスクパス)。自動マージ対象外のため、人間の判断を待ちます。以下、実質的なレビュー内容です。

差分の内容確認

  • internal/permissions/permissions.go: DefaultSandboxSettings() はIssue本文の指定通り allowUnsandboxedCommands: falseGITHUB_TOKENmask+injectHosts(denyではない)、Goツールチェーン向けドメインを含む実装
  • MergeDefaultSettings(path): 既存の .claude/settings.json のトップレベルキー(特にpermissions)を保持したまま、欠けているキー(sandbox等)のみを補完する設計。テストで「既存permissionsが変更されずに残る」ことを確認しており、破壊的変更ではないという受け入れ条件を満たす
  • cmd/hermit/doctor.gocheckSandboxSettings: sandbox.enabled未設定/false、allowUnsandboxedCommands未設定/true、excludedCommandsが空でない、の3種を検出。いずれも警告のみでdoctor全体のpass/failには影響しない設計は、Issue本文の「検出して警告する」という要求と整合し、既存プロジェクトの後方互換性も壊さない
  • README「Sandboxing the Engineer」セクションは、boolean/配列キーのprecedenceの違い、project scope配置ではEngineer自身がPR経由で無効化・迂回しうる旨、managed settingsによる実効的な強制とallowManagedReadPathsOnly/allowManagedDomainsOnlyの必要性、それらの自動生成は別Issue #179のスコープである旨を明記しており、Issue本文の要求を満たしている
  • CIは全て成功(Test/Lint/Security Scan)。Engineerの報告によれば、実際にこのリポジトリ自身の.claude/settings.jsonに対してhermit initを再実行し、既存のpermissions.allowが保持されることを手動検証済み

懸念点

推奨

実装はIssueの受け入れ条件を満たしており、危険な既定値(GITHUB_TOKENのdeny化など)も適切に回避されています。人間による承認後のマージを推奨します。マージ時はREQ-015番号衝突の解消をお願いします。

@ytnobody
ytnobody force-pushed the hermit/ytnobody/issue-180 branch 2 times, most recently from a479b93 to 46611e9 Compare July 28, 2026 02:57
@ytnobody

Copy link
Copy Markdown
Owner Author

⚠️ HERMIT: HIGH risk detected.
Reasons: [500 or more lines changed cmd/hermit/doctor.go is in a high-risk path cmd/hermit/main.go is in a high-risk path]

…st (Closes #180)

hermit init now generates a `sandbox` block in .claude/settings.json
(enabled, allowUnsandboxedCommands=false, network.allowedDomains for
GitHub + the Go module proxy, GITHUB_TOKEN masked+injectHosts rather
than denied) instead of relying solely on the Bash(*) allowlist, which
Issue #138 already showed breaks whenever a new tool/command shape
shows up. Re-running `hermit init` on an already-initialized project
merges in only missing top-level keys (e.g. sandbox) and never
destroys existing permissions customization.

hermit doctor gained three warning-level checks for the most common
ways this block ends up inert: sandbox.enabled false/missing,
allowUnsandboxedCommands true/missing, and a non-empty
sandbox.excludedCommands.

README documents the settings.json scope/precedence caveat this
relies on: boolean keys resolve by scope precedence, but array keys
(including sandbox.excludedCommands and permissions.allow) merge
additively across scopes, so a project-scope sandbox is a strong
default rather than an enforcement boundary against the Engineer
itself — real enforcement needs managed settings plus
allowManagedReadPathsOnly/allowManagedDomainsOnly, tracked separately
in Issue #179.
@ytnobody
ytnobody force-pushed the hermit/ytnobody/issue-180 branch from 46611e9 to 878b87a Compare July 28, 2026 03:01
@ytnobody

Copy link
Copy Markdown
Owner Author

⚠️ HERMIT: HIGH risk detected.
Reasons: [500 or more lines changed cmd/hermit/doctor.go is in a high-risk path cmd/hermit/main.go is in a high-risk path]

@ytnobody
ytnobody merged commit 1af8fd3 into develop Jul 28, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant