Skip to content

Commit c23d198

Browse files
committed
fix(wsapprover): non-blocking approval response relay
Previously HandleResponse did a blocking send on a buffer-1 channel. A duplicate or late approval_response could wedge the WebSocket read goroutine and exhaust the global connection semaphore. - Use select/default so surplus responses are dropped. - Add regression test verifying the second HandleResponse returns promptly without blocking. - Document the relay behaviour in SECURITY.md and AGENTS.md. Closes L-3 in sec_findings.md.
1 parent f7637f7 commit c23d198

4 files changed

Lines changed: 54 additions & 4 deletions

File tree

AGENTS.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ Layered prompt-injection / approval-fatigue defenses. Full reference: [docs/SECU
145145
- **write_file content cap** (`cmd/odek/file_tool.go`) — the `content` argument is capped at 1 MiB to prevent disk exhaustion and memory pressure from a single enormous tool call.
146146
- **file_info confinement + wrapping** (`cmd/odek/file_tool.go`) — `file_info` respects the same `restrictToCWD` path confinement as `write_file`/`patch`, and the returned path is wrapped as untrusted content.
147147
- **WebSocket message-size cap** (`cmd/odek/serve.go`) — `odek serve` sets `MaxPayloadBytes` on every WebSocket connection so a local client cannot OOM the server with a huge frame.
148+
- **WebSocket approval relay non-blocking** (`cmd/odek/wsapprover.go`) — `HandleResponse` uses a non-blocking send on the pending response channel, so duplicate or late approval responses are dropped instead of blocking the WebSocket read goroutine and exhausting the global connection semaphore.
148149
- **Session file size cap** (`internal/session/session.go`) — session files larger than 32 MiB are rejected by `Load()` to prevent OOM from tampered or corrupted transcripts.
149150
- **Skill file size cap** (`internal/skills/loader.go`) — `SKILL.md` files larger than 1 MiB are skipped so a malicious project cannot OOM the process at startup or bloat the system prompt.
150151
- **Serve sandbox default-on**`odek serve` enables `--sandbox` automatically unless `--no-sandbox` is passed.

cmd/odek/wsapprover.go

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,12 +201,20 @@ func (a *wsApprover) PromptOperation(op danger.ToolOperation) error {
201201
// HandleResponse processes a response from the browser.
202202
// Called by the WebSocket read loop when an approval_response arrives.
203203
// Returns true if the response matched a pending request.
204+
//
205+
// The send to the response channel is non-blocking: the channel has capacity
206+
// 1, so if a duplicate or late response races with the first one, or if the
207+
// request has already timed out and the reader is gone, the send drops the
208+
// response instead of wedging the WebSocket read goroutine.
204209
func (a *wsApprover) HandleResponse(id, action string) bool {
205210
a.mu.Lock()
206211
resp, ok := a.pending[id]
207212
a.mu.Unlock()
208213
if ok {
209-
resp <- action
214+
select {
215+
case resp <- action:
216+
default:
217+
}
210218
}
211219
return ok
212220
}

cmd/odek/wsapprover_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,49 @@ func TestWSApprover_HandleResponse_NonMatching(t *testing.T) {
7676
}
7777
}
7878

79+
// TestWSApprover_HandleResponse_DoesNotBlock verifies that a duplicate or
80+
// late approval response is dropped instead of blocking the WebSocket read
81+
// goroutine. This is a regression test for the L3 relay race: previously the
82+
// second response could wedge on a buffer-1 channel and exhaust the connection
83+
// semaphore.
84+
func TestWSApprover_HandleResponse_DoesNotBlock(t *testing.T) {
85+
a := newWSApprover(func(v any) error { return nil })
86+
id := "test-id-block"
87+
88+
a.mu.Lock()
89+
a.pending[id] = make(chan string, 1)
90+
a.mu.Unlock()
91+
92+
// First response fills the buffer-1 channel.
93+
if !a.HandleResponse(id, "approve") {
94+
t.Fatal("first HandleResponse should match")
95+
}
96+
97+
// Second response must return promptly even though the channel is full.
98+
done := make(chan struct{})
99+
go func() {
100+
a.HandleResponse(id, "approve")
101+
close(done)
102+
}()
103+
104+
select {
105+
case <-done:
106+
// expected
107+
case <-time.After(2 * time.Second):
108+
t.Fatal("second HandleResponse blocked on a full response channel")
109+
}
110+
111+
// Only one value should have been delivered.
112+
select {
113+
case action := <-a.pending[id]:
114+
if action != "approve" {
115+
t.Errorf("expected 'approve', got %q", action)
116+
}
117+
default:
118+
t.Fatal("expected exactly one buffered response")
119+
}
120+
}
121+
79122
func TestWSApprover_PromptCommand_TrustedClass(t *testing.T) {
80123
callCount := 0
81124
a := newWSApprover(func(v any) error {

docs/SECURITY.md

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -133,9 +133,7 @@ The `guard` section is operator-controlled: project-level `./odek.json` cannot s
133133
When a classification is set to `prompt`, an approver pauses the agent until the user decides. Two implementations:
134134

135135
- **TTYApprover** (CLI / REPL) — reads from `/dev/tty`.
136-
- **WSApprover** (Web UI) — sends `approval_request` over WebSocket; the browser shows a modal.
137-
138-
Both:
136+
- **WSApprover** (Web UI) — sends `approval_request` over WebSocket; the browser shows a modal. Responses are relayed to the pending prompt via a non-blocking send on a capacity-1 channel, so a duplicate, late, or raced response cannot block the WebSocket read goroutine and exhaust the global connection semaphore.
139137

140138
- Disable the "Trust class for session" shortcut for `destructive` and `blocked`. A forged or stale UI that sends `"trust"` for those classes is coerced to a single approve.
141139
- Engage **friction mode** after 3 approvals of the same class in 60 s: require typing the literal word `approve` (no single-letter / button shortcut) and impose a 1.5 s pause before accepting input. This breaks reflex click-through under sustained LLM-driven approval pressure.

0 commit comments

Comments
 (0)