From 79b4b6cca0e122a2265bf96be1c2760715eaad13 Mon Sep 17 00:00:00 2001 From: Vincent Janelle Date: Mon, 13 Apr 2026 14:53:12 -0700 Subject: [PATCH] fix(shell): harden terminal output sanitization Respond to PR feedback by tightening how terminal-facing text is sanitized across shell execution, logger formatting, and shared escape helpers. Why: - timeout and error responses could still surface raw terminal control sequences from captured command output - logger component formatting needed to sanitize component text before applying color while preserving the already-sanitized value during named-part rendering - shared escape handling did not yet cover all terminal-relevant control ranges, including C1 controls and private-use code points What changed: - move shell output escaping so captured stdout and stderr are sanitized before timeout and error result handling - split logger field sanitization from final formatting, sanitize component values before colorization, and bypass reformatting for the component part once sanitized - extend termutil.EscapeControlChars to escape C1 controls and private-use characters in both BMP and supplementary planes Tests: - add termutil coverage for C0, C1, bidi, zero-width, and private-use characters - add logger coverage to verify sanitized component formatting is preserved after FormatPrepare - add shell coverage to verify timeout output returns escaped ANSI and bidi sequences --- pkg/logger/logger.go | 29 ++++++++++++--- pkg/logger/logger_test.go | 24 +++++++++++++ pkg/termutil/escape.go | 4 +-- pkg/termutil/escape_test.go | 70 +++++++++++++++++++++++++++++++++++++ pkg/tools/shell.go | 6 ++-- pkg/tools/shell_test.go | 30 ++++++++++++++++ 6 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 pkg/termutil/escape_test.go diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index 411578fcc..73fcc8666 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -57,7 +57,8 @@ func init() { TimeFormat: "15:04:05", // TODO: make it configurable??? // Custom formatter to handle multiline strings and JSON objects - FormatFieldValue: formatFieldValue, + FormatFieldValue: formatFieldValue, + FormatPartValueByName: formatPartValueByName, PartsOrder: []string{ zerolog.TimestampFieldName, zerolog.LevelFieldName, @@ -67,8 +68,11 @@ func init() { }, FieldsExclude: []string{Component}, FormatPrepare: func(fields map[string]any) error { + component := formatComponentValue(fields[Component]) if isTTY { - fields[Component] = fmt.Sprintf("\x1b[33m%v\x1b[0m", fields[Component]) + fields[Component] = fmt.Sprintf("\x1b[33m%s\x1b[0m", component) + } else { + fields[Component] = component } return nil }, @@ -82,6 +86,22 @@ func init() { } func formatFieldValue(i any) string { + s := sanitizeFieldString(i) + return formatSanitizedFieldValue(s) +} + +func formatPartValueByName(i any, name string) string { + if name == Component { + return fmt.Sprintf("%v", i) + } + return formatFieldValue(i) +} + +func formatComponentValue(i any) string { + return sanitizeFieldString(i) +} + +func sanitizeFieldString(i any) string { var s string switch val := i.(type) { @@ -97,12 +117,13 @@ func formatFieldValue(i any) string { s = unquoted } - s = termutil.EscapeControlChars(s) + return termutil.EscapeControlChars(s) +} +func formatSanitizedFieldValue(s string) string { if strings.Contains(s, "\n") { return fmt.Sprintf("\n%s", s) } - if strings.Contains(s, " ") { if (strings.HasPrefix(s, "{") && strings.HasSuffix(s, "}")) || (strings.HasPrefix(s, "[") && strings.HasSuffix(s, "]")) { diff --git a/pkg/logger/logger_test.go b/pkg/logger/logger_test.go index 2c50f1607..e68f5414b 100644 --- a/pkg/logger/logger_test.go +++ b/pkg/logger/logger_test.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "testing" "time" @@ -272,6 +273,29 @@ func TestFormatFieldValue(t *testing.T) { } } +func TestFormatPreparePreservesColoredComponent(t *testing.T) { + fields := map[string]any{ + Component: "safe\u202ecomponent", + } + + if err := consoleWriter.FormatPrepare(fields); err != nil { + t.Fatalf("FormatPrepare() error = %v", err) + } + + component, ok := fields[Component].(string) + if !ok { + t.Fatalf("component field type = %T, want string", fields[Component]) + } + if strings.ContainsRune(component, '\u202e') { + t.Fatalf("expected bidi control to be escaped in component, got %q", component) + } + + formatted := formatPartValueByName(component, Component) + if formatted != component { + t.Fatalf("formatPartValueByName(component) = %q, want %q", formatted, component) + } +} + func TestDefaultLevelIsInfo(t *testing.T) { // The package-level default (before any SetLevel call) should be INFO. // Because earlier tests may have changed it, we just verify the constant is wired correctly. diff --git a/pkg/termutil/escape.go b/pkg/termutil/escape.go index c1bab7c96..07c8bffd1 100644 --- a/pkg/termutil/escape.go +++ b/pkg/termutil/escape.go @@ -17,9 +17,9 @@ func EscapeControlChars(input string) string { switch { case r == '\n' || r == '\r' || r == '\t': sb.WriteRune(r) - case r < 0x20 || r == 0x7f: + case r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f): sb.WriteString(fmt.Sprintf("\\x%02x", r)) - case unicode.Is(unicode.Cf, r): + case unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Co, r): if r <= 0xffff { sb.WriteString(fmt.Sprintf("\\u%04x", r)) } else { diff --git a/pkg/termutil/escape_test.go b/pkg/termutil/escape_test.go new file mode 100644 index 000000000..4c139ad96 --- /dev/null +++ b/pkg/termutil/escape_test.go @@ -0,0 +1,70 @@ +package termutil + +import "testing" + +func TestEscapeControlChars(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "preserves printable text", + input: "hello world", + expected: "hello world", + }, + { + name: "preserves whitespace controls", + input: "a\tb\nc\rd", + expected: "a\tb\nc\rd", + }, + { + name: "escapes C0 controls", + input: "\x1b[31mred", + expected: `\x1b[31mred`, + }, + { + name: "escapes DEL", + input: "a\x7fb", + expected: `a\x7fb`, + }, + { + name: "escapes C1 controls", + input: "a\u009bb", + expected: `a\x9bb`, + }, + { + name: "escapes bidi override", + input: "safe\u202edanger", + expected: `safe\u202edanger`, + }, + { + name: "escapes zero width chars", + input: "a\u200bb", + expected: `a\u200bb`, + }, + { + name: "escapes astral format chars", + input: "a\U000e0001b", + expected: `a\U000e0001b`, + }, + { + name: "escapes BMP private use chars", + input: "a\ue000b", + expected: `a\ue000b`, + }, + { + name: "escapes supplementary private use chars", + input: "a\U000f0000b", + expected: `a\U000f0000b`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := EscapeControlChars(tt.input); got != tt.expected { + t.Fatalf("EscapeControlChars() = %q, want %q", got, tt.expected) + } + }) + } +} diff --git a/pkg/tools/shell.go b/pkg/tools/shell.go index ea3f23e82..a5264b4cd 100644 --- a/pkg/tools/shell.go +++ b/pkg/tools/shell.go @@ -436,6 +436,7 @@ func (t *ExecTool) runSync(ctx context.Context, command, cwd string) *ToolResult if stderr.Len() > 0 { output += "\nSTDERR:\n" + stderr.String() } + output = termutil.EscapeControlChars(output) if err != nil { if errors.Is(cmdCtx.Err(), context.DeadlineExceeded) { @@ -470,8 +471,6 @@ func (t *ExecTool) runSync(ctx context.Context, command, cwd string) *ToolResult output = "(no output)" } - output = termutil.EscapeControlChars(output) - maxLen := 10000 if len(output) > maxLen { output = output[:maxLen] + fmt.Sprintf("\n... (truncated, %d more chars)", len(output)-maxLen) @@ -1205,6 +1204,9 @@ func extractTraversalPathCandidates(command string) []string { continue } + // This is a lexical heuristic over the literal command text, not a full + // shell parser. Paths materialized only after shell expansion are a + // known limitation and are handled by the surrounding trust model. if _, ok := seen[part]; ok { continue } diff --git a/pkg/tools/shell_test.go b/pkg/tools/shell_test.go index 601f441f9..9e13c8eb9 100644 --- a/pkg/tools/shell_test.go +++ b/pkg/tools/shell_test.go @@ -779,6 +779,36 @@ func TestShellTool_OutputEscapesTerminalControlChars(t *testing.T) { } } +func TestShellTool_TimeoutOutputEscapesTerminalControlChars(t *testing.T) { + tool, err := NewExecTool("", false) + if err != nil { + t.Fatalf("unable to configure exec tool: %s", err) + } + + tool.SetTimeout(100 * time.Millisecond) + + result := tool.Execute(context.Background(), map[string]any{ + "action": "run", + "command": "printf '\\033[31mred\\033[0m\\u202E'; sleep 10", + }) + + if !result.IsError { + t.Fatalf("expected timeout error, got success: %q", result.ForLLM) + } + if !strings.Contains(result.ForLLM, "timed out") { + t.Fatalf("expected timeout message, got: %q", result.ForLLM) + } + if strings.Contains(result.ForLLM, "\x1b") { + t.Fatalf("expected ANSI escape to be escaped in timeout output, got: %q", result.ForLLM) + } + if strings.ContainsRune(result.ForLLM, '\u202e') { + t.Fatalf("expected bidi control to be escaped in timeout output, got: %q", result.ForLLM) + } + if !strings.Contains(result.ForLLM, `\x1b[31mred\x1b[0m`) || !strings.Contains(strings.ToLower(result.ForLLM), `\u202e`) { + t.Fatalf("expected escaped control sequence in timeout output, got: %q", result.ForLLM) + } +} + func TestShellTool_Background_ReturnsImmediately(t *testing.T) { tool, err := NewExecTool("", false) require.NoError(t, err)