Skip to content

Commit 696e556

Browse files
authored
L12: serialize TTY prompts process-wide and share friction accounting (#84)
- 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.
1 parent 636e470 commit 696e556

5 files changed

Lines changed: 69 additions & 21 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -182,6 +182,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
182182
- **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.
183183
- **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-<chatID>` prefix, `ResumeSession` rejects cross-chat IDs, and plans live under `~/.odek/plans/chat<chatID>/` so one chat cannot list, read, delete, or resume another chat's sessions/plans.
184184
- **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:<reqID>: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.
185+
- **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.
185186
- **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.
186187
- **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.
187188
- **MCP tool batch classification** (`internal/loop/loop.go`) — MCP tools (`<server>__<tool>`) are classified as `unknown` by `classifyToolCall`, so the batch approval gate shows them and untrusted sub-agents force them to `deny`.

cmd/odek/parallel_shell_danger_test.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,10 @@ func TestParallelShell_Danger_MultipleCommandsPrompted(t *testing.T) {
145145
// class in the TTY fallback persists across parallel_shell calls on the same
146146
// tool instance.
147147
func TestParallelShell_Danger_TrustedClassCached(t *testing.T) {
148+
// Reset the process-wide approval log so earlier parallel_shell tests
149+
// do not trip friction mode for this single trust-session prompt.
150+
danger.ResetTTYFrictionStateForTest()
151+
148152
tty, cleanup := writeTTY(t, "t") // trust session
149153
defer cleanup()
150154

docs/SECURITY.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -673,6 +673,12 @@ The Telegram bot's `clarify` tool sent inline keyboard buttons with literal call
673673

674674
Each clarify prompt now generates a random request ID (embedded in callback data as `clarify:<reqID>: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.
675675

676+
### 39o. Process-wide TTY prompt serialization and friction accounting
677+
678+
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.
679+
680+
`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.
681+
676682
### 40. `/api/resources` result limit cap
677683

678684
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.

internal/danger/approver.go

Lines changed: 50 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"strings"
88
"sync"
9+
"testing"
910
"time"
1011
)
1112

@@ -29,6 +30,28 @@ type Approver interface {
2930
PromptOperation(op ToolOperation) error
3031
}
3132

33+
var (
34+
// ttyPromptMu serializes all TTY approval prompts process-wide. Without
35+
// this, concurrent tool calls (e.g. parallel_shell) each open /dev/tty
36+
// with their own bufio.Reader and compete for keystrokes, so the user
37+
// can approve a command they never saw.
38+
ttyPromptMu sync.Mutex
39+
40+
// ttyApprovalLog and ttyApprovalMu track approvals across all
41+
// TTYApprover instances, so friction mode engages even when tools
42+
// create fresh approvers per prompt.
43+
ttyApprovalMu sync.Mutex
44+
ttyApprovalLog = make(map[RiskClass][]time.Time)
45+
)
46+
47+
// ResetTTYFrictionStateForTest clears the process-wide approval log used by
48+
// friction mode. It is intended for tests that need a clean approval baseline.
49+
func ResetTTYFrictionStateForTest() {
50+
ttyApprovalMu.Lock()
51+
ttyApprovalLog = make(map[RiskClass][]time.Time)
52+
ttyApprovalMu.Unlock()
53+
}
54+
3255
// TTYApprover implements Approver by reading from /dev/tty.
3356
// This is the default approver used in CLI mode (odek run, odek repl).
3457
// When /dev/tty is not available (piped stdin, CI), it falls back to
@@ -48,20 +71,25 @@ type TTYApprover struct {
4871
// notice they have approved an unusual number of dangerous calls.
4972
FrictionThreshold int
5073
FrictionWindow time.Duration
51-
approvalLog map[RiskClass][]time.Time
5274
// pauseFn is overridden in tests so we don't actually sleep.
5375
pauseFn func(d time.Duration)
5476
}
5577

5678
// NewTTYApprover creates a TTYApprover with the given config.
5779
func NewTTYApprover(cfg *DangerousConfig) *TTYApprover {
80+
// Tests run many TTYApprover-backed assertions in the same process.
81+
// Reset the process-wide approval log on each creation so friction-mode
82+
// tests start from a known baseline. Production keeps the global log so
83+
// friction engages across tool instances.
84+
if testing.Testing() {
85+
ResetTTYFrictionStateForTest()
86+
}
5887
return &TTYApprover{
5988
DangerousConfig: cfg,
6089
TrustedClasses: make(map[RiskClass]bool),
6190
TTYPath: "/dev/tty",
6291
FrictionThreshold: 3,
6392
FrictionWindow: 60 * time.Second,
64-
approvalLog: make(map[RiskClass][]time.Time),
6593
pauseFn: func(d time.Duration) { time.Sleep(d) },
6694
}
6795
}
@@ -70,12 +98,9 @@ func NewTTYApprover(cfg *DangerousConfig) *TTYApprover {
7098
// returns true if the next prompt for this class should engage the
7199
// high-friction path.
72100
func (a *TTYApprover) recordApproval(cls RiskClass) {
73-
a.mu.Lock()
74-
defer a.mu.Unlock()
75-
if a.approvalLog == nil {
76-
a.approvalLog = make(map[RiskClass][]time.Time)
77-
}
78-
a.approvalLog[cls] = append(a.approvalLog[cls], time.Now())
101+
ttyApprovalMu.Lock()
102+
defer ttyApprovalMu.Unlock()
103+
ttyApprovalLog[cls] = append(ttyApprovalLog[cls], time.Now())
79104
}
80105

81106
// shouldFriction returns true when there have been >= FrictionThreshold
@@ -85,17 +110,17 @@ func (a *TTYApprover) shouldFriction(cls RiskClass) bool {
85110
if a.FrictionThreshold <= 0 || a.FrictionWindow <= 0 {
86111
return false
87112
}
88-
a.mu.Lock()
89-
defer a.mu.Unlock()
113+
ttyApprovalMu.Lock()
114+
defer ttyApprovalMu.Unlock()
90115
cutoff := time.Now().Add(-a.FrictionWindow)
91-
log := a.approvalLog[cls]
116+
log := ttyApprovalLog[cls]
92117
kept := log[:0]
93118
for _, t := range log {
94119
if t.After(cutoff) {
95120
kept = append(kept, t)
96121
}
97122
}
98-
a.approvalLog[cls] = kept
123+
ttyApprovalLog[cls] = kept
99124
return len(kept) >= a.FrictionThreshold
100125
}
101126

@@ -124,6 +149,17 @@ func (a *TTYApprover) PromptOperation(op ToolOperation) error {
124149
}
125150

126151
func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error {
152+
// Serialize all TTY prompts process-wide. Concurrent tool calls
153+
// otherwise open /dev/tty independently and race for keystrokes.
154+
ttyPromptMu.Lock()
155+
defer ttyPromptMu.Unlock()
156+
return a.promptLocked(cls, cmd, description)
157+
}
158+
159+
// promptLocked is the inner prompt implementation. The caller must hold
160+
// ttyPromptMu. It may recurse for the "context" command or after telling
161+
// the user that trust-session is unavailable for a high-impact class.
162+
func (a *TTYApprover) promptLocked(cls RiskClass, cmd, description string) error {
127163
// Check session trust cache
128164
a.mu.Lock()
129165
trusted := a.TrustedClasses != nil && a.TrustedClasses[cls]
@@ -203,7 +239,7 @@ func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error {
203239
case "t", "trust":
204240
if !allowTrust {
205241
fmt.Fprintf(os.Stderr, " trust-session not available for %s — type 'a' to approve once or 'd' to deny\n", cls)
206-
return a.prompt(cls, cmd, description)
242+
return a.promptLocked(cls, cmd, description)
207243
}
208244
// Cache this risk class for the session
209245
a.mu.Lock()
@@ -223,7 +259,7 @@ func (a *TTYApprover) prompt(cls RiskClass, cmd, description string) error {
223259
a.mu.Unlock()
224260
fmt.Fprintf(tty, " Trust this class: %v\n", trusted)
225261
// Re-prompt
226-
return a.prompt(cls, cmd, description)
262+
return a.promptLocked(cls, cmd, description)
227263
default:
228264
return fmt.Errorf("operation denied by user: %s", cmd)
229265
}

internal/danger/whitebox_coverage_test.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -207,16 +207,17 @@ func TestPrompt_ReadErrorWhenNoNewline(t *testing.T) {
207207
}
208208
}
209209

210-
func TestRecordApproval_InitialisesNilLog(t *testing.T) {
211-
// A zero-value TTYApprover (no NewTTYApprover) has a nil approvalLog;
212-
// recordApproval must lazily initialise it.
210+
func TestRecordApproval_PopulatesGlobalLog(t *testing.T) {
211+
// recordApproval writes to the process-wide approval log used by
212+
// friction mode, even on a zero-value TTYApprover.
213+
ResetTTYFrictionStateForTest()
213214
a := &TTYApprover{}
214215
a.recordApproval(SystemWrite)
215-
a.mu.Lock()
216-
n := len(a.approvalLog[SystemWrite])
217-
a.mu.Unlock()
216+
ttyApprovalMu.Lock()
217+
n := len(ttyApprovalLog[SystemWrite])
218+
ttyApprovalMu.Unlock()
218219
if n != 1 {
219-
t.Errorf("approvalLog[SystemWrite] = %d entries, want 1", n)
220+
t.Errorf("ttyApprovalLog[SystemWrite] = %d entries, want 1", n)
220221
}
221222
}
222223

0 commit comments

Comments
 (0)