Security hardening: injection-study findings — P0 fixes, persistence/unread-exec gates, read_only default, sandbox default-on

cmd/odek/cli_strict_flags_test.go

@@ -0,0 +1,136 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+// ── P0-1: unknown CLI flags must never fold into the task text ──────────
+//
+// Regression bar for the sec-benchmark finding: `odek run --session
+// --no-color --interaction-mode verbose "task"` used to prepend the
+// unknown flag to the prompt — corrupting it silently and handing
+// anything that controls argv (wrapper scripts, CI jobs, Makefile
+// targets) a prompt-injection vector into the CLI itself.
+
+func TestParseRunFlags_UnknownFlagBeforeTask_Errors(t *testing.T) {
+	_, err := parseRunFlags([]string{"--session", "--no-color", "--interaction-mode", "verbose", "Reply with exactly the word OK"})
+	if err == nil {
+		t.Fatal("expected error for unknown flag --interaction-mode, got nil")
+	}
+	if !strings.Contains(err.Error(), "--interaction-mode") {
+		t.Errorf("error should name the offending flag, got: %v", err)
+	}
+}
+
+func TestParseRunFlags_UnknownFlagAfterTask_Errors(t *testing.T) {
+	// Value-flags after the task used to be folded into the prompt too.
+	_, err := parseRunFlags([]string{"do the thing", "--model", "gpt-5"})
+	if err == nil {
+		t.Fatal("expected error for unknown flag after task, got nil")
+	}
+	if !strings.Contains(err.Error(), "--model") {
+		t.Errorf("error should name the offending flag, got: %v", err)
+	}
+}
+
+func TestParseRunFlags_UnknownFlagNeverReachesTask(t *testing.T) {
+	f, err := parseRunFlags([]string{"--no-color", "--bogus-flag", "real task"})
+	if err == nil {
+		t.Fatalf("expected error, got task=%q", f.Task)
+	}
+}
+
+func TestParseRunFlags_DoubleDashPassthrough(t *testing.T) {
+	f, err := parseRunFlags([]string{"--no-color", "--", "--interaction-mode", "verbose", "Reply OK"})
+	if err != nil {
+		t.Fatalf("parseRunFlags error: %v", err)
+	}
+	want := "--interaction-mode verbose Reply OK"
+	if f.Task != want {
+		t.Errorf("Task = %q, want %q (verbatim after --)", f.Task, want)
+	}
+	if f.NoColor == nil || !*f.NoColor {
+		t.Error("--no-color before -- should still parse as a flag")
+	}
+}
+
+func TestParseRunFlags_TrailingStandaloneFlagStillWorks(t *testing.T) {
+	f, err := parseRunFlags([]string{"do the thing", "--deliver"})
+	if err != nil {
+		t.Fatalf("parseRunFlags error: %v", err)
+	}
+	if f.Task != "do the thing" {
+		t.Errorf("Task = %q, want %q", f.Task, "do the thing")
+	}
+	if f.Deliver == nil || !*f.Deliver {
+		t.Error("--deliver after task should still parse as a flag")
+	}
+}
+
+func TestParseRunFlags_TaskStartingWithDashRequiresSeparator(t *testing.T) {
+	if _, err := parseRunFlags([]string{"-42 is the answer"}); err == nil {
+		t.Fatal("expected error for dash-prefixed task without -- separator")
+	}
+	f, err := parseRunFlags([]string{"--", "-42 is the answer"})
+	if err != nil {
+		t.Fatalf("parseRunFlags error: %v", err)
+	}
+	if f.Task != "-42 is the answer" {
+		t.Errorf("Task = %q, want %q", f.Task, "-42 is the answer")
+	}
+}
+
+func TestParseContinueArgs_UnknownFlagErrors(t *testing.T) {
+	_, _, _, err := parseContinueArgs([]string{"--interaction-mode", "verbose", "fix it"})
+	if err == nil {
+		t.Fatal("expected error for unknown flag in continue, got nil")
+	}
+	if !strings.Contains(err.Error(), "--interaction-mode") {
+		t.Errorf("error should name the offending flag, got: %v", err)
+	}
+}
+
+func TestParseContinueArgs_DanglingValueFlagErrors(t *testing.T) {
+	// A dangling --id used to fall through and become task text.
+	if _, _, _, err := parseContinueArgs([]string{"--id"}); err == nil {
+		t.Fatal("expected error for dangling --id, got nil")
+	}
+	if _, _, _, err := parseContinueArgs([]string{"--external-ref"}); err == nil {
+		t.Fatal("expected error for dangling --external-ref, got nil")
+	}
+}
+
+func TestParseContinueArgs_DoubleDashPassthrough(t *testing.T) {
+	_, _, task, err := parseContinueArgs([]string{"--id", "abc", "--", "--weird", "task text"})
+	if err != nil {
+		t.Fatalf("parseContinueArgs error: %v", err)
+	}
+	if task != "--weird task text" {
+		t.Errorf("task = %q, want %q", task, "--weird task text")
+	}
+}
+
+func TestParseReplFlags_UnknownFlagErrors(t *testing.T) {
+	if _, err := parseReplFlags([]string{"--interaction-mode"}); err == nil {
+		t.Fatal("expected error for unknown repl flag, got nil")
+	}
+	if _, err := parseReplFlags([]string{"--stream", "--nope"}); err == nil {
+		t.Fatal("expected error for unknown trailing repl flag, got nil")
+	}
+}
+
+// ── P0-2: `odek --version` must work like `odek version` ────────────────
+
+func TestDispatch_VersionFlagAlias(t *testing.T) {
+	for _, cmd := range []string{"--version", "-v"} {
+		out := captureStdout(func() {
+			if code := dispatch([]string{cmd}); code != 0 {
+				t.Errorf("dispatch(%q) exit = %d, want 0", cmd, code)
+			}
+		})
+		if !strings.Contains(out, "odek ") {
+			t.Errorf("dispatch(%q) output missing version block, got:\n%s", cmd, out)
+		}
+	}
+}

cmd/odek/dispatch.go

@@ -31,7 +31,10 @@ func dispatch(args []string) int {
 	switch cmd {
 	case "run":
 		return runExit(run(rest))
-	case "version":
+	case "version", "--version", "-v":
+		// --version is the form every packaging script, CI preflight, and
+		// support-bundle collector reaches for first; treat it as a full
+		// alias of the version subcommand instead of an unknown command.
 		printVersion()
 		return 0
 	case "init":

cmd/odek/external_ref.go

@@ -73,20 +73,38 @@ func parseExternalRefFlags(specs []string) ([]session.ExternalRef, error) {
 // parseContinueArgs splits `odek continue` arguments into the optional
 // --id / --external-ref flags (front-positioned, repeatable for the
 // latter) and the trailing task text.
+//
+// Unknown flags are a hard error (P0-1): they must never be folded into
+// the task text, where a typo'd or version-drifted flag silently corrupts
+// the prompt. An explicit "--" separator passes everything after it
+// through verbatim.
 func parseContinueArgs(args []string) (sessionID string, refSpecs []string, task string, err error) {
 	i := 0
+loop:
 	for i < len(args) {
-		if args[i] == "--id" && i+1 < len(args) {
+		if args[i] == "--" {
+			i++
+			break
+		}
+		switch args[i] {
+		case "--id":
+			if i+1 >= len(args) {
+				return "", nil, "", fmt.Errorf("--id requires a value")
+			}
 			sessionID = args[i+1]
 			i += 2
-			continue
-		}
-		if args[i] == "--external-ref" && i+1 < len(args) {
+		case "--external-ref":
+			if i+1 >= len(args) {
+				return "", nil, "", fmt.Errorf("--external-ref requires a value")
+			}
 			refSpecs = append(refSpecs, args[i+1])
 			i += 2
-			continue
+		default:
+			if isFlagLike(args[i]) {
+				return "", nil, "", unknownFlagError(args[i])
+			}
+			break loop
 		}
-		break
 	}
 	if i >= len(args) {
 		return "", nil, "", fmt.Errorf("no task provided for continue")

cmd/odek/file_tool.go

@@ -308,6 +308,13 @@ func (t *readFileTool) Call(argsJSON string) (string, error) {
 		return jsonError(fmt.Sprintf("cannot read %q: %v", args.Path, err))
 	}
 
+	// H-6: only a FULL-file read licenses later execution of this file
+	// (review HIGH-001): a partial read (offset/limit window over a longer
+	// file) showed the model a prefix — the payload could ride below.
+	if args.Offset <= 1 && args.Limit >= totalLines {
+		danger.RecordRead(resolvedPath)
+	}
+
 	result := readFileResult{
 		Content:    wrapUntrusted(t.toolCtx(), resolvedPath, content),
 		TotalLines: totalLines,
@@ -395,8 +402,15 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
 	}
 	args.Path = resolved
 
-	// Security: classify and check write operation
-	risk := danger.ClassifyPath(args.Path)
+	// Security: classify and check write operation. Write targets use the
+	// write-aware classifier: deferred-execution targets (shell profiles,
+	// git hooks, CI workflows, cron/systemd/launchd definitions) escalate
+	// to the persistence class (H-5), and content sniffing catches
+	// lifecycle hooks planted into package.json.
+	risk := danger.ClassifyPathWrite(args.Path)
+	if escalated, isHook := danger.LifecycleContentClass(args.Path, args.Content, risk); isHook {
+		risk = escalated
+	}
 	if err := t.dangerousConfig.CheckOperation(danger.ToolOperation{
 		Name: "write_file", Resource: args.Path, Risk: risk,
 	}, t.trustedClasses); err != nil {
@@ -417,6 +431,8 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
 		if err := sandboxWriteFile(t.containerName, args.Path, []byte(args.Content), origMode); err != nil {
 			return jsonError(fmt.Sprintf("cannot write %q via sandbox: %v", args.Path, err))
 		}
+		// Content authored this session is content the agent has seen (H-6).
+		danger.RecordRead(args.Path)
 		return jsonResult(writeFileResult{
 			Success: true,
 			Path:    args.Path,
@@ -462,6 +478,8 @@ func (t *writeFileTool) Call(argsJSON string) (string, error) {
 		return jsonError(fmt.Sprintf("cannot rename %q: %v", args.Path, err))
 	}
 
+	// Content authored this session is content the agent has seen (H-6).
+	danger.RecordRead(args.Path)
 	return jsonResult(writeFileResult{
 		Success: true,
 		Path:    args.Path,
@@ -842,8 +860,14 @@ func (t *patchTool) Call(argsJSON string) (string, error) {
 	}
 	args.Path = resolved
 
-	// Security: classify and check patch operation
-	risk := danger.ClassifyPath(args.Path)
+	// Security: classify and check patch operation. Write-aware classifier
+	// (H-5): deferred-execution targets escalate to persistence, and the
+	// new content is sniffed for lifecycle hooks (package.json scripts,
+	// conftest.py autouse).
+	risk := danger.ClassifyPathWrite(args.Path)
+	if escalated, isHook := danger.LifecycleContentClass(args.Path, args.NewString, risk); isHook {
+		risk = escalated
+	}
 	if err := t.dangerousConfig.CheckOperation(danger.ToolOperation{
 		Name: "patch", Resource: args.Path, Risk: risk,
 	}, t.trustedClasses); err != nil {
@@ -944,6 +968,8 @@ func (t *patchTool) Call(argsJSON string) (string, error) {
 		return jsonError(fmt.Sprintf("cannot write %q: %v", args.Path, err))
 	}
 
+	// Content (re)authored this session is content the agent has seen (H-6).
+	danger.RecordRead(args.Path)
 	return jsonResult(patchResult{
 		Success: true,
 		Diff:    wrapUntrusted(t.toolCtx(), "patch:"+args.Path, diff),
@@ -1440,6 +1466,11 @@ func (t *batchReadTool) readSingle(arg batchReadFileArg) batchReadFileResult {
 		return batchReadFileResult{Path: arg.Path, Error: fmt.Sprintf("cannot read %q: %v", arg.Path, err)}
 	}
 
+	// H-6: full-file reads only (review HIGH-001) — same rationale as
+	// read_file.
+	if arg.Offset <= 1 && arg.Limit >= totalLines {
+		danger.RecordRead(resolvedPath)
+	}
 	return batchReadFileResult{
 		Path:       arg.Path,
 		Content:    wrapUntrusted(t.toolCtx(), resolvedPath, content),