From 9d77fb227861dabc54ab2c349490ff8720e8187e Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 19 Jul 2026 10:53:27 +0200 Subject: [PATCH] L12: serialize TTY prompts process-wide and share friction accounting - internal/danger/approver.go: add a process-wide mutex (ttyPromptMu) that serializes all TTY approval prompts; move the approval log that drives friction mode to a process-wide map so repeated approvals across tool instances correctly engage high-friction mode. Reset the global log in test mode on each NewTTYApprover so tests stay isolated. - internal/danger/whitebox_coverage_test.go: update record-approval test for the global log. - cmd/odek/parallel_shell_danger_test.go: reset friction state before the trust-cache test. - docs/SECURITY.md + AGENTS.md: document L12 mitigation. --- AGENTS.md | 1 + cmd/odek/parallel_shell_danger_test.go | 4 ++ docs/SECURITY.md | 6 +++ internal/danger/approver.go | 64 ++++++++++++++++++----- internal/danger/whitebox_coverage_test.go | 15 +++--- 5 files changed, 69 insertions(+), 21 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index f470ff5..ff7025d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -182,6 +182,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU - **Telegram log file permissions** (`internal/telegram/log.go`) — Telegram log files are created with `0600`, and `os.Chmod` hardens existing files. Chat IDs and task snippets are no longer world-readable by default. - **Telegram chat-scoped sessions and plans** (`internal/telegram/session.go`, `internal/telegram/plan.go`, `cmd/odek/telegram.go`) — `/sessions`, `/resume`, `/prune`, `/plans`, `/plan_view`, `/plan_delete`, and `/plan_resume` are scoped to the requesting chat. Sessions are filtered by the `tg-` prefix, `ResumeSession` rejects cross-chat IDs, and plans live under `~/.odek/plans/chat/` so one chat cannot list, read, delete, or resume another chat's sessions/plans. - **Telegram clarify callback binding** (`cmd/odek/telegram.go`, `internal/telegram/handler.go`) — each `clarify` prompt now uses a random request ID in its inline-keyboard callback data (`clarify::yes/no`). The handler validates the request ID and binds the answer to the originating user, so a group member or stale keyboard cannot answer someone else's clarify prompt. `Handler.OnCallbackQuery` now receives the `userID` for consistent callback binding. +- **Process-wide TTY prompt serialization + friction accounting** (`internal/danger/approver.go`) — all CLI TTY approval prompts are serialized by a process-wide mutex so concurrent tool calls cannot interleave prompts on `/dev/tty`; the approval log that drives friction mode is shared across TTYApprover instances, so the high-friction "type `approve`" path engages after repeated approvals regardless of which tool instance recorded them. - **odek self-invocation gate** (`internal/danger/classifier.go`) — any shell stage whose program basename is `odek` is classified as `system_write`. This prevents a prompt-injected agent from reaching the human-gated trust mutations (`odek memory promote`, `odek memory extended confirm`, `odek skill promote --force`) through the shell tool and flipping its own taint gates. - **MCP inputSchema hardening** (`cmd/odek/mcp_approval.go`) — every string in an MCP tool's `inputSchema` is recursively guard-scanned for injection patterns; schemas larger than 256 KiB are rejected; the interactive approval prompt shows a SHA-256 hash and byte size of the schema so operators can detect changes. - **MCP tool batch classification** (`internal/loop/loop.go`) — MCP tools (`__`) are classified as `unknown` by `classifyToolCall`, so the batch approval gate shows them and untrusted sub-agents force them to `deny`. diff --git a/cmd/odek/parallel_shell_danger_test.go b/cmd/odek/parallel_shell_danger_test.go index b18948e..ef5beca 100644 --- a/cmd/odek/parallel_shell_danger_test.go +++ b/cmd/odek/parallel_shell_danger_test.go @@ -145,6 +145,10 @@ func TestParallelShell_Danger_MultipleCommandsPrompted(t *testing.T) { // class in the TTY fallback persists across parallel_shell calls on the same // tool instance. func TestParallelShell_Danger_TrustedClassCached(t *testing.T) { + // Reset the process-wide approval log so earlier parallel_shell tests + // do not trip friction mode for this single trust-session prompt. + danger.ResetTTYFrictionStateForTest() + tty, cleanup := writeTTY(t, "t") // trust session defer cleanup() diff --git a/docs/SECURITY.md b/docs/SECURITY.md index b8e5a57..634c536 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -673,6 +673,12 @@ The Telegram bot's `clarify` tool sent inline keyboard buttons with literal call Each clarify prompt now generates a random request ID (embedded in callback data as `clarify::yes/no`). The handler validates the request ID, rejects callbacks from a different user than the one who triggered the prompt, and ignores callbacks for expired or already-answered prompts. The `Handler.OnCallbackQuery` signature now receives the originating `userID` so other callback handlers can apply the same binding. +### 39o. Process-wide TTY prompt serialization and friction accounting + +In CLI mode, concurrent tool calls (for example, `parallel_shell`) each opened `/dev/tty` with their own reader and printed prompts simultaneously. A user could approve a command they never saw, and the per-instance approval log meant friction mode never engaged across prompts. + +`TTYApprover` now serializes all TTY prompts with a process-wide mutex, and the approval log that drives friction mode is shared across all instances. Concurrent prompts queue behind the active prompt, and repeated approvals of the same class within the friction window correctly trigger the high-friction "type `approve`" path. + ### 40. `/api/resources` result limit cap The `/api/resources?q=...&limit=N` autocomplete endpoint previously accepted any positive `limit` value. It is now capped to 100 results both in the HTTP handler and in `Registry.Search`. This prevents a prompt-injected or attacker-forged request from forcing an unbounded directory walk and returning a multi-megabyte JSON response. diff --git a/internal/danger/approver.go b/internal/danger/approver.go index a7be8d7..caa53f5 100644 --- a/internal/danger/approver.go +++ b/internal/danger/approver.go @@ -6,6 +6,7 @@ import ( "os" "strings" "sync" + "testing" "time" ) @@ -29,6 +30,28 @@ type Approver interface { PromptOperation(op ToolOperation) error } +var ( + // ttyPromptMu serializes all TTY approval prompts process-wide. Without + // this, concurrent tool calls (e.g. parallel_shell) each open /dev/tty + // with their own bufio.Reader and compete for keystrokes, so the user + // can approve a command they never saw. + ttyPromptMu sync.Mutex + + // ttyApprovalLog and ttyApprovalMu track approvals across all + // TTYApprover instances, so friction mode engages even when tools + // create fresh approvers per prompt. + ttyApprovalMu sync.Mutex + ttyApprovalLog = make(map[RiskClass][]time.Time) +) + +// ResetTTYFrictionStateForTest clears the process-wide approval log used by +// friction mode. It is intended for tests that need a clean approval baseline. +func ResetTTYFrictionStateForTest() { + ttyApprovalMu.Lock() + ttyApprovalLog = make(map[RiskClass][]time.Time) + ttyApprovalMu.Unlock() +} + // TTYApprover implements Approver by reading from /dev/tty. // This is the default approver used in CLI mode (odek run, odek repl). // When /dev/tty is not available (piped stdin, CI), it falls back to @@ -48,20 +71,25 @@ type TTYApprover struct { // notice they have approved an unusual number of dangerous calls. FrictionThreshold int FrictionWindow time.Duration - approvalLog map[RiskClass][]time.Time // pauseFn is overridden in tests so we don't actually sleep. pauseFn func(d time.Duration) } // NewTTYApprover creates a TTYApprover with the given config. func NewTTYApprover(cfg *DangerousConfig) *TTYApprover { + // Tests run many TTYApprover-backed assertions in the same process. + // Reset the process-wide approval log on each creation so friction-mode + // tests start from a known baseline. Production keeps the global log so + // friction engages across tool instances. + if testing.Testing() { + ResetTTYFrictionStateForTest() + } return &TTYApprover{ DangerousConfig: cfg, TrustedClasses: make(map[RiskClass]bool), TTYPath: "/dev/tty", FrictionThreshold: 3, FrictionWindow: 60 * time.Second, - approvalLog: make(map[RiskClass][]time.Time), pauseFn: func(d time.Duration) { time.Sleep(d) }, } } @@ -70,12 +98,9 @@ func NewTTYApprover(cfg *DangerousConfig) *TTYApprover { // returns true if the next prompt for this class should engage the // high-friction path. func (a *TTYApprover) recordApproval(cls RiskClass) { - a.mu.Lock() - defer a.mu.Unlock() - if a.approvalLog == nil { - a.approvalLog = make(map[RiskClass][]time.Time) - } - a.approvalLog[cls] = append(a.approvalLog[cls], time.Now()) + ttyApprovalMu.Lock() + defer ttyApprovalMu.Unlock() + ttyApprovalLog[cls] = append(ttyApprovalLog[cls], time.Now()) } // shouldFriction returns true when there have been >= FrictionThreshold @@ -85,17 +110,17 @@ func (a *TTYApprover) shouldFriction(cls RiskClass) bool { if a.FrictionThreshold <= 0 || a.FrictionWindow <= 0 { return false } - a.mu.Lock() - defer a.mu.Unlock() + ttyApprovalMu.Lock() + defer ttyApprovalMu.Unlock() cutoff := time.Now().Add(-a.FrictionWindow) - log := a.approvalLog[cls] + log := ttyApprovalLog[cls] kept := log[:0] for _, t := range log { if t.After(cutoff) { kept = append(kept, t) } } - a.approvalLog[cls] = kept + ttyApprovalLog[cls] = kept return len(kept) >= a.FrictionThreshold } @@ -124,6 +149,17 @@ func (a *TTYApprover) PromptOperation(op ToolOperation) error { } func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error { + // Serialize all TTY prompts process-wide. Concurrent tool calls + // otherwise open /dev/tty independently and race for keystrokes. + ttyPromptMu.Lock() + defer ttyPromptMu.Unlock() + return a.promptLocked(cls, cmd, description) +} + +// promptLocked is the inner prompt implementation. The caller must hold +// ttyPromptMu. It may recurse for the "context" command or after telling +// the user that trust-session is unavailable for a high-impact class. +func (a *TTYApprover) promptLocked(cls RiskClass, cmd, description string) error { // Check session trust cache a.mu.Lock() trusted := a.TrustedClasses != nil && a.TrustedClasses[cls] @@ -203,7 +239,7 @@ func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error { case "t", "trust": if !allowTrust { fmt.Fprintf(os.Stderr, " trust-session not available for %s — type 'a' to approve once or 'd' to deny\n", cls) - return a.prompt(cls, cmd, description) + return a.promptLocked(cls, cmd, description) } // Cache this risk class for the session a.mu.Lock() @@ -223,7 +259,7 @@ func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error { a.mu.Unlock() fmt.Fprintf(tty, " Trust this class: %v\n", trusted) // Re-prompt - return a.prompt(cls, cmd, description) + return a.promptLocked(cls, cmd, description) default: return fmt.Errorf("operation denied by user: %s", cmd) } diff --git a/internal/danger/whitebox_coverage_test.go b/internal/danger/whitebox_coverage_test.go index 420099f..f05063d 100644 --- a/internal/danger/whitebox_coverage_test.go +++ b/internal/danger/whitebox_coverage_test.go @@ -207,16 +207,17 @@ func TestPrompt_ReadErrorWhenNoNewline(t *testing.T) { } } -func TestRecordApproval_InitialisesNilLog(t *testing.T) { - // A zero-value TTYApprover (no NewTTYApprover) has a nil approvalLog; - // recordApproval must lazily initialise it. +func TestRecordApproval_PopulatesGlobalLog(t *testing.T) { + // recordApproval writes to the process-wide approval log used by + // friction mode, even on a zero-value TTYApprover. + ResetTTYFrictionStateForTest() a := &TTYApprover{} a.recordApproval(SystemWrite) - a.mu.Lock() - n := len(a.approvalLog[SystemWrite]) - a.mu.Unlock() + ttyApprovalMu.Lock() + n := len(ttyApprovalLog[SystemWrite]) + ttyApprovalMu.Unlock() if n != 1 { - t.Errorf("approvalLog[SystemWrite] = %d entries, want 1", n) + t.Errorf("ttyApprovalLog[SystemWrite] = %d entries, want 1", n) } }