fix(telemetry): stop shipping raw CLI args in ao.cli.usage_errors (#2813)

backend/internal/adapters/telemetry/posthog.go

@@ -244,18 +244,56 @@ func sanitizeRemotePayload(name string, payload map[string]any) map[string]any {
 		if !ok {
 			continue
 		}
-		if safe, ok := sanitizeRemoteValue(value); ok {
+		if safe, ok := sanitizeRemoteValue(key, value); ok {
 			sanitized[key] = safe
 		}
 	}
 	return sanitized
 }
 
-func sanitizeRemoteValue(v any) (any, bool) {
+// maxRemoteStringLen bounds any allowlisted string property. No legitimate value
+// here is large; a big one means an emitter leaked free text, so we truncate.
+const maxRemoteStringLen = 256
+
+// commandShapedKeys hold a command name or command path (e.g. "status",
+// "ao status"), never free text. Values that don't match the command shape are
+// replaced with a stable non-reversible token so raw args (URLs, paths, prose)
+// can never leave the machine even if a future emitter reintroduces the leak
+// (defense-in-depth for #2813; the primary fix is in cli.usageErrorCommand).
+var commandShapedKeys = map[string]bool{"command": true, "command_path": true}
+
+// isCommandShaped reports whether s looks like a command name/path: a short
+// string of letters/digits/-/_/space plus the "<unknown>" sentinel. URLs (":",
+// "/"), file paths ("/"), and prose (punctuation, non-ASCII) fail it.
+func isCommandShaped(s string) bool {
+	if s == "" || len(s) > 48 {
+		return false
+	}
+	for _, r := range s {
+		switch {
+		case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9',
+			r == '-', r == '_', r == ' ', r == '<', r == '>':
+		default:
+			return false
+		}
+	}
+	return true
+}
+
+func sanitizeRemoteValue(key string, v any) (any, bool) {
 	switch value := v.(type) {
 	case string:
 		value = strings.TrimSpace(value)
-		return value, value != ""
+		if value == "" {
+			return nil, false
+		}
+		if commandShapedKeys[key] && !isCommandShaped(value) {
+			return "sha256:" + sha256String(value)[:16], true
+		}
+		if len(value) > maxRemoteStringLen {
+			value = value[:maxRemoteStringLen]
+		}
+		return value, true
 	case bool:
 		return value, true
 	case int:

backend/internal/adapters/telemetry/sanitize_command_test.go

@@ -0,0 +1,61 @@
+package telemetry
+
+import (
+	"strings"
+	"testing"
+)
+
+// Defense-in-depth: even if an emitter puts raw free text in a command-shaped
+// property, the sink must not ship it — command/command_path values that aren't
+// command-shaped are replaced with a stable non-reversible token (#2813).
+func TestSanitizeRemoteValueRedactsNonCommandShapedCommandKeys(t *testing.T) {
+	leaky := []string{
+		"https://gitlab.com/org/repo/-/merge_requests/9",
+		"/Users/fora/Sites/internal-tool",
+		"ao Review this PR and merge if CI is green; ping @security",
+		"严格审查必须由当前主会话单独完成",
+	}
+	for _, key := range []string{"command", "command_path"} {
+		for _, raw := range leaky {
+			got, ok := sanitizeRemoteValue(key, raw)
+			if !ok {
+				t.Fatalf("%s=%q: dropped, expected a redacted token", key, raw)
+			}
+			s, _ := got.(string)
+			if !strings.HasPrefix(s, "sha256:") {
+				t.Fatalf("%s=%q: got %q, want a sha256: token", key, raw, s)
+			}
+			if strings.Contains(s, raw) || strings.Contains(s, "gitlab") || strings.Contains(s, "/Users/") {
+				t.Fatalf("%s: raw text leaked through: %q", key, s)
+			}
+		}
+	}
+}
+
+func TestSanitizeRemoteValueAllowsRealCommands(t *testing.T) {
+	cases := map[string]string{
+		"command":      "status",
+		"command_path": "ao status",
+	}
+	for key, val := range cases {
+		got, ok := sanitizeRemoteValue(key, val)
+		if !ok || got != val {
+			t.Fatalf("%s=%q: got (%v,%v), want it passed through unchanged", key, val, got, ok)
+		}
+	}
+	// The "<unknown>" sentinel the client emits must pass the shape check.
+	if got, ok := sanitizeRemoteValue("command_path", "ao list <unknown>"); !ok || got != "ao list <unknown>" {
+		t.Fatalf(`"ao list <unknown>" should pass, got (%v,%v)`, got, ok)
+	}
+}
+
+func TestSanitizeRemoteValueLengthBackstop(t *testing.T) {
+	// Non-command string keys are truncated to the backstop cap, never unbounded.
+	got, ok := sanitizeRemoteValue("error_kind", strings.Repeat("x", maxRemoteStringLen+500))
+	if !ok {
+		t.Fatal("long value dropped, expected truncation")
+	}
+	if s := got.(string); len(s) != maxRemoteStringLen {
+		t.Fatalf("length backstop not applied: got len %d, want %d", len(s), maxRemoteStringLen)
+	}
+}

backend/internal/cli/root.go

@@ -30,7 +30,7 @@ func executeWithDeps(deps Deps, args []string) error {
 	cmd.SetArgs(args)
 	err := cmd.Execute()
 	if err != nil && ExitCode(err) == 2 {
-		(&commandContext{deps: deps}).emitCLIUsageError(context.Background(), args, err)
+		(&commandContext{deps: deps}).emitCLIUsageError(context.Background(), knownCommandNames(cmd), args, err)
 	}
 	return err
 }
@@ -224,8 +224,8 @@ func (c *commandContext) emitCLIInvoked(ctx context.Context, cmd *cobra.Command)
 	})
 }
 
-func (c *commandContext) emitCLIUsageError(ctx context.Context, args []string, err error) {
-	command, commandPath := usageErrorCommand(args)
+func (c *commandContext) emitCLIUsageError(ctx context.Context, known map[string]bool, args []string, err error) {
+	command, commandPath := usageErrorCommand(known, args)
 	reqCtx, cancel := context.WithTimeout(ctx, probeTimeout)
 	defer cancel()
 	_ = c.postLoopbackJSON(reqCtx, "/internal/telemetry/cli-usage-error", map[string]string{
@@ -235,18 +235,51 @@ func (c *commandContext) emitCLIUsageError(ctx context.Context, args []string, e
 	})
 }
 
-func usageErrorCommand(args []string) (string, string) {
-	tokens := []string{"ao"}
+// knownCommandNames collects every registered command name (and alias) in the
+// tree, so usageErrorCommand can tell a real subcommand from arbitrary free text.
+func knownCommandNames(root *cobra.Command) map[string]bool {
+	known := make(map[string]bool)
+	var walk func(*cobra.Command)
+	walk = func(c *cobra.Command) {
+		for _, sub := range c.Commands() {
+			known[sub.Name()] = true
+			for _, alias := range sub.Aliases {
+				known[alias] = true
+			}
+			walk(sub)
+		}
+	}
+	walk(root)
+	return known
+}
+
+// usageErrorCommand derives a SAFE command / command_path for the usage-error
+// telemetry event. Parsing has already failed, so args may contain arbitrary user
+// text (URLs, absolute paths, multi-paragraph instructions). Only tokens that are
+// registered command names are propagated; the first non-flag token that is not a
+// known command is recorded as the sentinel "<unknown>" and the scan stops, so
+// raw positional text never leaves the machine (#2813). Flags (leading "-") end
+// the scan as before.
+func usageErrorCommand(known map[string]bool, args []string) (string, string) {
+	command := "ao"
+	commandPath := "ao"
+	sawUnknown := false
 	for _, arg := range args {
 		if strings.HasPrefix(arg, "-") {
 			break
 		}
-		tokens = append(tokens, arg)
+		if known[arg] {
+			command = arg
+			commandPath += " " + arg
+			continue
+		}
+		// Free text / URL / path — record a sentinel, never the raw token.
+		commandPath += " <unknown>"
+		sawUnknown = true
+		break
 	}
-	commandPath := strings.Join(tokens, " ")
-	command := "ao"
-	if len(tokens) > 1 {
-		command = tokens[len(tokens)-1]
+	if sawUnknown && command == "ao" {
+		command = "<unknown>"
 	}
 	return command, commandPath
 }

backend/internal/cli/usage_error_test.go

@@ -0,0 +1,56 @@
+package cli
+
+import (
+	"strings"
+	"testing"
+)
+
+// usageErrorCommand must never propagate raw positional text (URLs, paths, prose)
+// into the telemetry command / command_path — only registered command names.
+func TestUsageErrorCommandRedactsFreeText(t *testing.T) {
+	known := map[string]bool{"list": true, "status": true, "spawn": true}
+
+	cases := []struct {
+		name     string
+		args     []string
+		wantCmd  string
+		wantPath string
+	}{
+		{"url", []string{"https://gitlab.com/org/repo/-/merge_requests/9"}, "<unknown>", "ao <unknown>"},
+		{"abs path", []string{"/Users/fora/Sites/internal-tool"}, "<unknown>", "ao <unknown>"},
+		{"prose", []string{"Review this PR and merge if CI is green; ping @security first"}, "<unknown>", "ao <unknown>"},
+		{"non-ascii", []string{"严格审查必须由当前主会话单独完成"}, "<unknown>", "ao <unknown>"},
+		{"known then free text", []string{"list", "some-secret-project"}, "list", "ao list <unknown>"},
+		{"known then bad flag", []string{"status", "--bogus"}, "status", "ao status"},
+		{"no args", nil, "ao", "ao"},
+		{"leading flag", []string{"--help"}, "ao", "ao"},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd, path := usageErrorCommand(known, tc.args)
+			if cmd != tc.wantCmd || path != tc.wantPath {
+				t.Fatalf("usageErrorCommand(%v) = (%q, %q), want (%q, %q)", tc.args, cmd, path, tc.wantCmd, tc.wantPath)
+			}
+			// Hard guarantee: no raw positional token appears anywhere in the output.
+			for _, arg := range tc.args {
+				if strings.HasPrefix(arg, "-") {
+					continue
+				}
+				if !known[arg] && (strings.Contains(cmd, arg) || strings.Contains(path, arg)) {
+					t.Fatalf("raw arg %q leaked into (%q, %q)", arg, cmd, path)
+				}
+			}
+		})
+	}
+}
+
+func TestKnownCommandNamesIncludesSubcommandsAndAliases(t *testing.T) {
+	known := knownCommandNames(NewRootCommand(DefaultDeps().withDefaults()))
+	// A few commands that must exist in the tree; guards against the collector
+	// silently returning empty (which would make everything "<unknown>").
+	for _, name := range []string{"status", "spawn", "list"} {
+		if !known[name] {
+			t.Fatalf("known command set missing %q; got %d entries", name, len(known))
+		}
+	}
+}