fix(security): VProtocol auto-repairs for low findings #69, #77, #78, #79, #81

- cmd/odek/serve.go: server-side cap on WebSocket prompt size (1 MiB) and
  validation of model ID length/characters (#69, #81)
- cmd/odek/subagent.go: parse and cap --timeout (<=3600s) and --max-iter
  (<=100) (#79)
- internal/config/loader.go: refuse to load ~/.odek/secrets.env when it is
  group/world-readable (#78)
- internal/telegram/health.go: warn when health server binds to a non-loopback
  address (#77)
- Add regression tests for all fixes; full suite, race detector, go vet and
  golangci-lint pass

Author: jkyberneees

cmd/odek/serve.go

@@ -14,6 +14,7 @@ import (
 	"os"
 	"os/signal"
 	"path/filepath"
+	"regexp"
 	"strconv"
 	"strings"
 	"sync"
@@ -39,6 +40,18 @@ var uiFS embed.FS
 // multi-gigabyte frame.
 const maxWSMessageBytes = 8 * 1024 * 1024 // 8 MiB
 
+// maxPromptBytes caps the size of the user prompt accepted through the Web UI.
+// Combined with the WebSocket frame cap, this prevents a local client from
+// bloating the session file or exhausting the LLM context budget.
+const maxPromptBytes = 1 * 1024 * 1024 // 1 MiB
+
+// maxModelIDBytes caps the length of a model ID supplied by the Web UI.
+const maxModelIDBytes = 128
+
+// modelIDPattern restricts model IDs to printable ASCII characters commonly
+// used by model providers (alphanumeric, punctuation, and path separators).
+var modelIDPattern = regexp.MustCompile(`^[A-Za-z0-9_.:/@-]+$`)
+
 // maxWSConnections caps the number of concurrent WebSocket clients. Once the
 // limit is reached, further upgrade attempts are rejected with HTTP 503. This
 // prevents a local attacker from spawning unlimited connections, each of which
@@ -830,6 +843,24 @@ func handlePrompt(
 	prompt := msg.Content
 	sessionID := msg.SessionID
 
+	// Server-side cap on prompt size (finding #69). A client can already send
+	// up to the WebSocket frame cap; reject anything above a reasonable prompt
+	// limit before storing it in the session or forwarding it to the LLM.
+	if len(prompt) > maxPromptBytes {
+		writeWSError(conn, "prompt exceeds maximum size")
+		return currSess
+	}
+
+	// Server-side validation of model IDs from the UI (finding #81). Model IDs
+	// are passed through to the LLM client and logged; cap length and reject
+	// control / unusual characters to prevent oversized payloads or injection.
+	if msg.Model != "" {
+		if len(msg.Model) > maxModelIDBytes || !modelIDPattern.MatchString(msg.Model) {
+			writeWSError(conn, "invalid model ID")
+			return currSess
+		}
+	}
+
 	// Resolve @ references
 	refs := resource.ParseRefs(prompt)
 	resolvedRefs := make(map[string]string)

cmd/odek/serve_test.go

@@ -2234,3 +2234,115 @@ func TestServe_CSRF_TokenRequired(t *testing.T) {
 	conn = dialTestWS(t, addr)
 	conn.Close()
 }
+
+// TestServe_E2E_PromptSizeCap verifies that prompts above maxPromptBytes are
+// rejected server-side before they are stored in the session or forwarded to
+// the LLM (finding #69).
+func TestServe_E2E_PromptSizeCap(t *testing.T) {
+	llmSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/models") {
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{}})
+			return
+		}
+		t.Error("LLM must not be called for an oversized prompt")
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer llmSrv.Close()
+
+	envCleanup := setTestEnv(t, llmSrv.URL)
+	defer envCleanup()
+
+	store := newTestSessionStore(t)
+	ln, mux := buildServeMux(t, store)
+	defer ln.Close()
+
+	errCh := make(chan error, 1)
+	go func() { errCh <- serveOnListener(ln, mux) }()
+	waitForHTTP(t, ln.Addr().String())
+
+	conn := dialTestWS(t, ln.Addr().String())
+	defer conn.Close()
+	conn.SetReadDeadline(time.Now().Add(5 * time.Second))
+
+	msg := map[string]any{
+		"type":    "prompt",
+		"content": strings.Repeat("x", maxPromptBytes+1),
+	}
+	payload, _ := json.Marshal(msg)
+	if err := golangws.Message.Send(conn, string(payload)); err != nil {
+		t.Fatalf("Send: %v", err)
+	}
+
+	var raw []byte
+	if err := golangws.Message.Receive(conn, &raw); err != nil {
+		t.Fatalf("expected error event, got receive error: %v", err)
+	}
+	var evt map[string]any
+	if err := json.Unmarshal(raw, &evt); err != nil {
+		t.Fatalf("unmarshal event: %v", err)
+	}
+	if evt["type"] != "error" {
+		t.Fatalf("expected error event for oversized prompt, got %v", evt["type"])
+	}
+}
+
+// TestServe_E2E_InvalidModelIDRejected verifies that model IDs from the Web UI
+// are length- and character-validated before use (finding #81).
+func TestServe_E2E_InvalidModelIDRejected(t *testing.T) {
+	llmSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodGet && strings.HasSuffix(r.URL.Path, "/models") {
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]any{"data": []map[string]any{}})
+			return
+		}
+		t.Error("LLM must not be called for an invalid model ID")
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer llmSrv.Close()
+
+	envCleanup := setTestEnv(t, llmSrv.URL)
+	defer envCleanup()
+
+	store := newTestSessionStore(t)
+	ln, mux := buildServeMux(t, store)
+	defer ln.Close()
+
+	errCh := make(chan error, 1)
+	go func() { errCh <- serveOnListener(ln, mux) }()
+	waitForHTTP(t, ln.Addr().String())
+
+	conn := dialTestWS(t, ln.Addr().String())
+	defer conn.Close()
+	conn.SetReadDeadline(time.Now().Add(5 * time.Second))
+
+	cases := []string{
+		strings.Repeat("x", maxModelIDBytes+1),
+		"model\nwith-newline",
+		"model\x00with-null",
+		"model<script>",
+	}
+	for _, model := range cases {
+		msg := map[string]any{
+			"type":    "prompt",
+			"content": "hello",
+			"model":   model,
+		}
+		payload, _ := json.Marshal(msg)
+		if err := golangws.Message.Send(conn, string(payload)); err != nil {
+			t.Fatalf("Send: %v", err)
+		}
+
+		var raw []byte
+		if err := golangws.Message.Receive(conn, &raw); err != nil {
+			t.Fatalf("expected error event, got receive error: %v", err)
+		}
+		var evt map[string]any
+		if err := json.Unmarshal(raw, &evt); err != nil {
+			t.Fatalf("unmarshal event: %v", err)
+		}
+		if evt["type"] != "error" {
+			t.Fatalf("expected error event for model %q, got %v", model, evt["type"])
+		}
+	}
+}

cmd/odek/subagent.go

@@ -174,20 +174,21 @@ func parseSubagentConfig(data string) subagentConfig {
 //	1 = task error (status: "error" with message)
 //	2 = timeout (killed by parent/context)
 //	3 = internal setup error
-func subagentCmd(args []string) error {
-	// Parse flags
-	var cfg struct {
-		goal          string
-		context       string
-		taskFile      string
-		timeout       int
-		maxIter       int
-		quiet         bool
-		stream        bool
-		parentSession string
-	}
-
-	// Simple flag parser (matches existing pattern in parseRunFlags)
+// subagentFlags holds the parsed flags for `odek subagent`.
+type subagentFlags struct {
+	goal          string
+	context       string
+	taskFile      string
+	timeout       int
+	maxIter       int
+	quiet         bool
+	stream        bool
+	parentSession string
+}
+
+// parseSubagentFlags parses and validates sub-agent CLI flags.
+func parseSubagentFlags(args []string) (subagentFlags, error) {
+	var cfg subagentFlags
 	i := 0
 	for i < len(args) {
 		switch args[i] {
@@ -226,11 +227,34 @@ func subagentCmd(args []string) error {
 				cfg.parentSession = args[i]
 			}
 		default:
-			return fmt.Errorf("unknown flag %q", args[i])
+			return cfg, fmt.Errorf("unknown flag %q", args[i])
 		}
 		i++
 	}
 
+	// Clamp runaway limits (finding #79). Values <= 0 fall through to the
+	// defaults in subagentCmd; explicitly huge values are capped to prevent a
+	// single sub-agent invocation from running forever.
+	const (
+		maxSubagentTimeout = 3600 // 1 hour
+		maxSubagentIter    = 100
+	)
+	if cfg.timeout > maxSubagentTimeout {
+		cfg.timeout = maxSubagentTimeout
+	}
+	if cfg.maxIter > maxSubagentIter {
+		cfg.maxIter = maxSubagentIter
+	}
+
+	return cfg, nil
+}
+
+func subagentCmd(args []string) error {
+	cfg, err := parseSubagentFlags(args)
+	if err != nil {
+		return err
+	}
+
 	// Validate: --goal XOR --task
 	hasGoal := cfg.goal != ""
 	hasTaskFile := cfg.taskFile != ""

cmd/odek/subagent_contract_test.go

@@ -1076,6 +1076,32 @@ func skipIfNoBinary(t *testing.T) {
 	}
 }
 
+// TestParseSubagentFlags_CapsRunawayLimits verifies that --timeout and
+// --max-iter are capped at sensible maximums (finding #79).
+func TestParseSubagentFlags_CapsRunawayLimits(t *testing.T) {
+	cfg, err := parseSubagentFlags([]string{
+		"--goal", "test",
+		"--timeout", "999999",
+		"--max-iter", "99999",
+	})
+	if err != nil {
+		t.Fatalf("parseSubagentFlags error: %v", err)
+	}
+	if cfg.timeout != 3600 {
+		t.Errorf("timeout cap = %d, want 3600", cfg.timeout)
+	}
+	if cfg.maxIter != 100 {
+		t.Errorf("max-iter cap = %d, want 100", cfg.maxIter)
+	}
+}
+
+func TestParseSubagentFlags_UnknownFlag(t *testing.T) {
+	_, err := parseSubagentFlags([]string{"--unknown"})
+	if err == nil {
+		t.Fatal("expected error for unknown flag")
+	}
+}
+
 func isFlagParseError(err error) bool {
 	if err == nil {
 		return false

internal/config/loader.go

@@ -1598,6 +1598,16 @@ func loadSecretsEnv() {
 	}
 	defer f.Close()
 
+	// Refuse to load secrets from a file that is readable by anyone other than
+	// the owner. A world/group-readable secrets.env leaks API keys and tokens
+	// to other local users (finding #78).
+	if info, err := f.Stat(); err == nil {
+		if perm := info.Mode().Perm(); perm&0077 != 0 {
+			fmt.Fprintf(os.Stderr, "odek: WARNING: %s is group/world-readable (%04o); refusing to load secrets\n", path, perm)
+			return
+		}
+	}
+
 	scanner := bufio.NewScanner(f)
 	for scanner.Scan() {
 		line := strings.TrimSpace(scanner.Text())

internal/config/loader_test.go

@@ -1375,3 +1375,42 @@ func TestLoadFile_CapsSizeViaLimitReader(t *testing.T) {
 		t.Fatalf("loadFile should reject oversized file read via LimitReader, got Model=%q", cfg.Model)
 	}
 }
+
+// TestLoadConfig_SecretsEnvPermissionCheck verifies that secrets.env is only
+// loaded when it is owner-readable (finding #78).
+func TestLoadConfig_SecretsEnvPermissionCheck(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	odekDir := filepath.Join(home, ".odek")
+	if err := os.MkdirAll(odekDir, 0755); err != nil {
+		t.Fatal(err)
+	}
+	path := filepath.Join(odekDir, "secrets.env")
+
+	// World/group-readable secrets.env must be ignored.
+	if err := os.WriteFile(path, []byte("ODEK_TEST_SECRET=world-readable\n"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("ODEK_TEST_SECRET", "")
+	LoadConfig(CLIFlags{})
+	if os.Getenv("ODEK_TEST_SECRET") == "world-readable" {
+		t.Error("world-readable secrets.env was loaded")
+	}
+
+	// Owner-only readable secrets.env must be loaded.
+	if err := os.Remove(path); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(path, []byte("ODEK_TEST_SECRET=owner-only\n"), 0600); err != nil {
+		t.Fatal(err)
+	}
+	// Ensure the file really is 0600 even under a permissive umask.
+	if err := os.Chmod(path, 0600); err != nil {
+		t.Fatal(err)
+	}
+	t.Setenv("ODEK_TEST_SECRET", "")
+	LoadConfig(CLIFlags{})
+	if os.Getenv("ODEK_TEST_SECRET") != "owner-only" {
+		t.Errorf("owner-only secrets.env not loaded, got %q", os.Getenv("ODEK_TEST_SECRET"))
+	}
+}

internal/telegram/health.go

@@ -85,6 +85,15 @@ func (hs *HealthServer) Start(ctx context.Context) error {
 	}
 	hs.addr = ln.Addr().String() // store actual bound address (e.g. :0 → :34567)
 
+	// Warn when binding to a non-loopback address. The health endpoint exposes
+	// bot liveness/uptime; under the single-user model it should not be
+	// reachable from the network (finding #77).
+	if host, _, err := net.SplitHostPort(hs.addr); err == nil {
+		if host == "" || (host != "127.0.0.1" && host != "::1" && host != "localhost") {
+			hs.log.Warn("health server binding to non-loopback address", "addr", hs.addr)
+		}
+	}
+
 	srv := &http.Server{Addr: hs.addr, Handler: hs}
 	hs.log.Info("health server started", "addr", ln.Addr().String())
 

internal/telegram/health_test.go

@@ -142,3 +142,49 @@ func (m *mockResponseWriter) Write(b []byte) (int, error) {
 func (m *mockResponseWriter) WriteHeader(statusCode int) {
 	m.statusCode = statusCode
 }
+
+// captureLogger records Warn calls for testing.
+type captureLogger struct {
+	warnings []string
+}
+
+func (c *captureLogger) Debug(_ string, _ ...any) {}
+func (c *captureLogger) Info(_ string, _ ...any)  {}
+func (c *captureLogger) Warn(msg string, _ ...any)  { c.warnings = append(c.warnings, msg) }
+func (c *captureLogger) Error(_ string, _ ...any) {}
+func (c *captureLogger) With(_ ...any) Logger     { return c }
+
+func TestHealthServer_NonLoopbackAddressWarns(t *testing.T) {
+	log := &captureLogger{}
+	hs := NewHealthServer(":0")
+	hs.SetLogger(log)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	errCh := make(chan error, 1)
+	go func() {
+		errCh <- hs.Start(ctx)
+	}()
+
+	time.Sleep(50 * time.Millisecond)
+	cancel()
+
+	select {
+	case err := <-errCh:
+		if err != nil && !strings.Contains(err.Error(), "server closed") {
+			t.Errorf("unexpected shutdown error: %v", err)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("timeout waiting for health server shutdown")
+	}
+
+	found := false
+	for _, w := range log.warnings {
+		if strings.Contains(w, "non-loopback") {
+			found = true
+			break
+		}
+	}
+	if !found {
+		t.Errorf("expected non-loopback binding warning, got %v", log.warnings)
+	}
+}