diff --git a/backend/internal/adapters/telemetry/posthog.go b/backend/internal/adapters/telemetry/posthog.go index 5b09337fd0..d624c0f4df 100644 --- a/backend/internal/adapters/telemetry/posthog.go +++ b/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 "" 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: diff --git a/backend/internal/adapters/telemetry/sanitize_command_test.go b/backend/internal/adapters/telemetry/sanitize_command_test.go new file mode 100644 index 0000000000..59c1070827 --- /dev/null +++ b/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 "" sentinel the client emits must pass the shape check. + if got, ok := sanitizeRemoteValue("command_path", "ao list "); !ok || got != "ao list " { + t.Fatalf(`"ao list " 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) + } +} diff --git a/backend/internal/cli/root.go b/backend/internal/cli/root.go index 631696a8f5..7a11d71f2e 100644 --- a/backend/internal/cli/root.go +++ b/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 "" 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 += " " + sawUnknown = true + break } - commandPath := strings.Join(tokens, " ") - command := "ao" - if len(tokens) > 1 { - command = tokens[len(tokens)-1] + if sawUnknown && command == "ao" { + command = "" } return command, commandPath } diff --git a/backend/internal/cli/usage_error_test.go b/backend/internal/cli/usage_error_test.go new file mode 100644 index 0000000000..d7e7c7a89f --- /dev/null +++ b/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"}, "", "ao "}, + {"abs path", []string{"/Users/fora/Sites/internal-tool"}, "", "ao "}, + {"prose", []string{"Review this PR and merge if CI is green; ping @security first"}, "", "ao "}, + {"non-ascii", []string{"严格审查必须由当前主会话单独完成"}, "", "ao "}, + {"known then free text", []string{"list", "some-secret-project"}, "list", "ao list "}, + {"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 ""). + for _, name := range []string{"status", "spawn", "list"} { + if !known[name] { + t.Fatalf("known command set missing %q; got %d entries", name, len(known)) + } + } +}