Fix review round 1 of the wait list and its hooks (issues 1629, 1743, 1744)
A session id reaching the Stop and SessionStart hooks was only checked for blank, so "../../x" or "a/b" could read or write outside the waits directory; both hooks now hold every session id to waitSessionID before building any path, and allow (saying why) rather than fail on a bad one. claude_code_wait's list/watched/done/said held only add to that same check, letting a malformed id through as an ordinary, always-empty key; they all refuse it now. The Stop hook's three-strikes cap could be bypassed forever if its counter file could not be written on a refusal: it now allows instead, saying so, so a wait can never hold a turn open indefinitely. Its advice offered mesh_wait on a wait's event family unconditionally, ignoring that the session's tool runner may not be granted it (ADR 0367 §2); it now offers mesh_wait only "if this session's tool runner is granted it, otherwise start a watcher", and drops the family entirely when a wait has none. Both hooks' fail-open and let-through notices were said only on stderr, which Claude Code does not show the operator on an allowed (exit 0) outcome; they now also print the same words as a "systemMessage" JSON object on stdout, which Claude Code does show.
This commit is contained in:
@@ -4,6 +4,8 @@ package main
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
@@ -72,6 +74,45 @@ type panicReader struct{}
|
||||
|
||||
func (panicReader) Read([]byte) (int, error) { panic("boom") }
|
||||
|
||||
// Review round 1, item 1 (1743/1744, ADR 0489 §5): a session id that is not a plain one is never used to
|
||||
// build a file path — reading outside the waits dir never happens — and the hook prints only a line saying
|
||||
// the id could not be used.
|
||||
func TestSessionStartRejectsAnUnsafeSessionID(t *testing.T) {
|
||||
root := t.TempDir()
|
||||
waitsDir := filepath.Join(root, "agent", "waits")
|
||||
if err := os.MkdirAll(waitsDir, 0o700); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
// What "../../x" resolves to from waitsDir: root/x.json, outside waitsDir, with content that must never be
|
||||
// read through it.
|
||||
outside := filepath.Join(waitsDir, "../../x.json")
|
||||
writeWaitsFixture(t, outside, WaitsFile{Session: "x", At: time.Now(),
|
||||
Waits: []Wait{{ID: "w-secret", What: "SECRET-DATA", Bound: 10, At: time.Now()}}})
|
||||
t.Cleanup(func() { _ = os.Remove(outside) })
|
||||
|
||||
for _, id := range []string{"../../x", "a/b"} {
|
||||
var out bytes.Buffer
|
||||
stdin := strings.NewReader(fmt.Sprintf(`{"session_id":%q}`, id))
|
||||
if code := runSessionStart(waitsDir, stdin, &out); code != 0 {
|
||||
t.Fatalf("id %q: code %d", id, code)
|
||||
}
|
||||
if strings.Contains(out.String(), "SECRET-DATA") {
|
||||
t.Fatalf("id %q: read a file outside the waits dir: %s", id, out.String())
|
||||
}
|
||||
if !strings.Contains(out.String(), "could not be used") {
|
||||
t.Fatalf("id %q: did not say the id could not be used: %s", id, out.String())
|
||||
}
|
||||
// Item (a): the notice reaches the screen as the one JSON object Claude Code shows.
|
||||
if !strings.Contains(out.String(), "systemMessage") {
|
||||
t.Fatalf("id %q: the notice is not a systemMessage: %s", id, out.String())
|
||||
}
|
||||
}
|
||||
var out bytes.Buffer
|
||||
if code := runSessionStart(waitsDir, strings.NewReader(`{"session_id":""}`), &out); code != 0 || out.Len() != 0 {
|
||||
t.Fatalf("blank id: code %d, out %q", code, out.String())
|
||||
}
|
||||
}
|
||||
|
||||
// 1744/AC4 is checked together with 1743/AC7 in terminal_only_test.go's
|
||||
// TestThePluginsOnlyHooksAreTheGuardTheSessionStartHookAndTheStopHook.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user