fix(security): fail closed on plan-approval declines; add frontend security headers - #53
Open
nithiink wants to merge 1 commit into
Open
fix(security): fail closed on plan-approval declines; add frontend security headers#53nithiink wants to merge 1 commit into
nithiink wants to merge 1 commit into
Conversation
…curity headers Addresses findings from the 2026-07-21 daily security review (#52). M1 (claude_runner.py): the SDK ExitPlanMode branch approved a plan whenever the answer contained an intent word ("auto"/"manual"/"proceed"/"approve"), even after decide_permission had resolved it to "deny". A decline such as "do not proceed" (contains "proceed") was inverted into approval. Short-circuit on an explicit deny before the intent-substring test. Genuine approvals ("proceed", "manually approve edits", "auto") still resolve to allow. M2 (tmux_runner.py): _classify_plan_choice checked the manual/auto intent substrings before the decline branch, and the decline branch only caught negations at the start of the phrase. "do not approve edits" matched "approve edit" -> manual, and "...auto..." could escalate to auto-approve mode. Evaluate an explicit decline (decide_permission == "deny") first. L1 (next.config.mjs): add X-Frame-Options/CSP frame-ancestors, nosniff, and Referrer-Policy headers to the command-proxying frontend (clickjacking + defense-in-depth). M3 (WebFetch auto-approved) is a product/UX tradeoff and is left for a maintainer decision, not auto-fixed here. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BJwDxJqz2ckLKHUTraLqLf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the auto-fixable findings from the 2026-07-21 daily security review. Refs #52.
Fixes (mapped to findings)
M1 —
backend/claude_runner.py(SDKClaudeRunner._map_decision, ExitPlanMode branch)The branch approved the plan whenever the answer contained an intent word (
auto/manual/proceed/approve), even afterdecide_permissionhad already resolved it todeny. So a decline such as"do not proceed"(which contains"proceed") was inverted intoPermissionResultAllow(), defeating the module's documented fail-closed contract.Fix: short-circuit on an explicit
denybefore the intent-substring test.M2 —
backend/tmux_runner.py(_classify_plan_choice)The
manual/autointent substrings ("manual","approve edit","each edit","review edit","auto") were tested before the decline branch, and that decline branch only caught negations at the start of the phrase."do not approve edits"matched"approve edit"→manual, and a phrase like"please don't switch to auto"matched"auto"→ escalated to auto-approve-everything mode.Fix: evaluate an explicit decline (
decide_permission(c) == "deny") first.L1 —
frontend/next.config.mjsAdded a
headers()block applyingX-Frame-Options: DENY, CSPframe-ancestors 'none',X-Content-Type-Options: nosniff, andReferrer-Policy: no-referrerto all routes — clickjacking protection + defense-in-depth for the command-proxying UI (which binds0.0.0.0in network mode).Verification
decide_permissionwas checked in isolation to confirm the fixes preserve the happy paths:deny(now correctly short-circuited):"do not proceed","don't approve","do not approve edits","please don't switch to auto"."proceed","manually approve edits","auto"(→None, handled by the existing intent path),"yes","approve".No existing test references
_map_decision,_classify_plan_choice, orSAFE_TOOLS, so behavior of the covered paths is unchanged. (The full pytest suite couldn't run in the review sandbox —claude_agent_sdkisn't installed there — so please let CI exercise it.)Left for manual attention (not in this PR)
WebFetch/WebSearchauto-approved assafe(backend/permissions.py:12-17): a real prompt-injection exfiltration/SSRF path (unpromptedWebFetchcan carry local data to an arbitrary host), but removing it fromSAFE_TOOLSis a deliberate product/UX tradeoff, so it's left for a maintainer decision.NODE_TLS_REJECT_UNAUTHORIZED=0indev:network— previously reported (Daily security review — 2026-06-08 #23/Daily security review — 2026-06-10 #36); loopback-capped.🤖 Generated with Claude Code
https://claude.ai/code/session_01BJwDxJqz2ckLKHUTraLqLf
Generated by Claude Code