diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index 4b6c4528..22277e5f 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -359,9 +359,15 @@ coding-context -p issue_number=123 -p issue_title="Bug" normal-task # Output will contain: Issue: 123 and Title: Bug ``` -### Parameter Substitution +### Content Expansion -Use `${parameter_name}` syntax for dynamic values. +Task and command content supports three types of dynamic expansion, processed in a single pass to prevent injection attacks. + +#### Parameter Expansion + +Use `${parameter_name}` syntax to substitute parameter values from `-p` flags. + +**Syntax:** `${parameter_name}` **Example:** ```markdown @@ -384,6 +390,81 @@ coding-context \ /fix-bug ``` +**Behavior:** If a parameter is not found, the placeholder remains unchanged (e.g., `${missing}` stays as `${missing}`) and a warning is logged. + +#### Command Expansion + +Use `` !`command` `` syntax to execute shell commands and include their output. + +**Syntax:** `` !`command` `` + +**Example:** +```markdown +--- +task_name: system-info +--- +# System Information + +Current date: !`date +%Y-%m-%d` +Current user: !`whoami` +Git branch: !`git rev-parse --abbrev-ref HEAD` +``` + +**Output:** +``` +Current date: 2025-12-11 +Current user: alex +Git branch: main +``` + +**Behavior:** +- Command output is included as-is (including any trailing newlines) +- If the command fails, the original syntax remains unchanged (e.g., `` !`false` `` stays as `` !`false` ``) and a warning is logged +- Commands are executed using `sh -c` + +**Security Note:** Only use with trusted task files, as commands are executed with your user permissions. + +#### Path Expansion + +Use `@path` syntax to include the contents of a file. + +**Syntax:** `@path` (delimited by whitespace; use `\ ` to escape spaces in filenames) + +**Example:** +```markdown +--- +task_name: include-config +--- +# Current Configuration + +@config.yaml + +# API Documentation + +@docs/api.md +``` + +**With spaces in filenames:** +```markdown +Content from file: @my\ file\ with\ spaces.txt +``` + +**Behavior:** +- File content is included verbatim +- If the file is not found, the original syntax remains unchanged (e.g., `@missing.txt` stays as `@missing.txt`) and a warning is logged +- Path can be absolute or relative to the current directory + +#### Security: Single-Pass Expansion + +All three expansion types are processed in a **single pass, rune-by-rune** to prevent injection attacks: + +- Expanded content is **never re-processed** for further expansions +- Command output containing `${param}` will not be expanded +- File content containing `` !`command` `` will not be executed +- Parameter values containing `@path` will not be read as files + +This prevents command injection where expanded content could trigger further, unintended expansions. + ### File Location Task files must be in one of these directories: diff --git a/integration_test.go b/integration_test.go index 91e4288a..abeafd7f 100644 --- a/integration_test.go +++ b/integration_test.go @@ -292,6 +292,118 @@ Please work on ${component} and fix ${issue}. } } +func TestExpanderIntegration(t *testing.T) { + tmpDir := t.TempDir() + tasksDir := filepath.Join(tmpDir, ".agents", "tasks") + + if err := os.MkdirAll(tasksDir, 0o755); err != nil { + t.Fatalf("failed to create tasks dir: %v", err) + } + + // Create a test file for path expansion + dataFile := filepath.Join(tmpDir, "data.txt") + if err := os.WriteFile(dataFile, []byte("file content"), 0o644); err != nil { + t.Fatalf("failed to write data file: %v", err) + } + + // Create a task file with all three expansion types + taskFile := filepath.Join(tasksDir, "test-expander.md") + taskContent := fmt.Sprintf(`--- +task_name: test-expander +--- +# Test Expander + +Parameter: ${component} +Command: !`+"`echo hello`"+` +Path: @%s +Combined: ${component} !`+"`echo world`"+` +`, dataFile) + if err := os.WriteFile(taskFile, []byte(taskContent), 0o644); err != nil { + t.Fatalf("failed to write task file: %v", err) + } + + // Run the program with parameters + output := runTool(t, "-C", tmpDir, "-p", "component=auth", "test-expander") + + // Check parameter expansion + if !strings.Contains(output, "Parameter: auth") { + t.Errorf("parameter expansion failed. Output:\n%s", output) + } + + // Check command expansion + if !strings.Contains(output, "Command: hello") { + t.Errorf("command expansion failed. Output:\n%s", output) + } + + // Check path expansion + if !strings.Contains(output, "Path: file content") { + t.Errorf("path expansion failed. Output:\n%s", output) + } + + // Check combined expansion + if !strings.Contains(output, "Combined: auth world") { + t.Errorf("combined expansion failed. Output:\n%s", output) + } +} + +func TestExpanderSecurityIntegration(t *testing.T) { + tmpDir := t.TempDir() + tasksDir := filepath.Join(tmpDir, ".agents", "tasks") + + if err := os.MkdirAll(tasksDir, 0o755); err != nil { + t.Fatalf("failed to create tasks dir: %v", err) + } + + // Create a file that contains expansion syntax (should not be re-expanded) + dataFile := filepath.Join(tmpDir, "injection.txt") + if err := os.WriteFile(dataFile, []byte("${injected} and !`echo hacked`"), 0o644); err != nil { + t.Fatalf("failed to write data file: %v", err) + } + + // Create a task file that tests security (no re-expansion) + taskFile := filepath.Join(tasksDir, "test-security.md") + taskContent := fmt.Sprintf(`--- +task_name: test-security +--- +# Test Security + +File content: @%s +Param with command: ${evil} +Command with param: !`+"`echo '${secret}'`"+` +`, dataFile) + if err := os.WriteFile(taskFile, []byte(taskContent), 0o644); err != nil { + t.Fatalf("failed to write task file: %v", err) + } + + // Run the program with parameters that contain expansion syntax + output := runTool(t, "-C", tmpDir, "-p", "evil=!`echo INJECTED`", "-p", "secret=TOPSECRET", "test-security") + + // Check that file content with expansion syntax is NOT re-expanded + if !strings.Contains(output, "File content: ${injected} and !`echo hacked`") { + t.Errorf("file content was re-expanded (security issue). Output:\n%s", output) + } + + // Check that parameter value with command syntax is NOT executed + if !strings.Contains(output, "Param with command: !`echo INJECTED`") { + t.Errorf("parameter with command syntax was executed (security issue). Output:\n%s", output) + } + + // Check that command output with parameter syntax is NOT re-expanded + if !strings.Contains(output, "Command with param: ${secret}") { + t.Errorf("command output was re-expanded (security issue). Output:\n%s", output) + } + + // Verify that sensitive data is NOT in output (unless it's part of literal text) + if strings.Contains(output, "TOPSECRET") { + t.Errorf("parameter was re-expanded from command output (security issue). Output:\n%s", output) + } + // Check that the literal command syntax is preserved (not executed) + // The word "hacked" appears in the literal text, so we check for the full context + if !strings.Contains(output, "!`echo hacked`") { + t.Errorf("file content was re-expanded (security issue). Output:\n%s", output) + } +} + func TestMdcFileSupport(t *testing.T) { dirs := setupTestDirs(t) diff --git a/pkg/codingcontext/context.go b/pkg/codingcontext/context.go index e4360b2c..f4159976 100644 --- a/pkg/codingcontext/context.go +++ b/pkg/codingcontext/context.go @@ -270,19 +270,23 @@ func (cc *Context) findCommand(commandName string, params map[string]string) (st return *content, nil } -// expandParams substitutes parameter placeholders in the given content. +// expandParams performs all types of content expansion: +// - Parameter expansion: ${param_name} +// - Command expansion: !`command` +// - Path expansion: @path +// If params is provided, it is merged with cc.params (with params taking precedence). func (cc *Context) expandParams(content string, params map[string]string) string { - return os.Expand(content, func(key string) string { - if val, ok := params[key]; ok { - return val - } - // If not in params map, check cc.params - if val, ok := cc.params[key]; ok { - return val - } - // Return original placeholder if not found - return fmt.Sprintf("${%s}", key) - }) + // Merge params with cc.params + mergedParams := make(map[string]string) + for k, v := range cc.params { + mergedParams[k] = v + } + for k, v := range params { + mergedParams[k] = v + } + + // Use the expand function to handle all expansion types + return expand(content, mergedParams, cc.logger) } // shouldExpandParams returns true if parameter expansion should occur based on the expandParams field. diff --git a/pkg/codingcontext/expander.go b/pkg/codingcontext/expander.go new file mode 100644 index 00000000..e696e161 --- /dev/null +++ b/pkg/codingcontext/expander.go @@ -0,0 +1,150 @@ +package codingcontext + +import ( + "fmt" + "log/slog" + "os" + "os/exec" + "strings" +) + +// expand performs all types of expansion on the content in a single pass: +// 1. Parameter expansion: ${param_name} +// 2. Command expansion: !`command` +// 3. Path expansion: @path +// SECURITY: Processes rune-by-rune to prevent injection attacks where expanded +// content contains further expansion sequences (e.g., command output with ${param}). +func expand(content string, params map[string]string, logger *slog.Logger) string { + var result strings.Builder + result.Grow(len(content)) + runes := []rune(content) + i := 0 + + for i < len(runes) { + // Check for parameter expansion: ${...} + if i+2 < len(runes) && runes[i] == '$' && runes[i+1] == '{' { + // Find the closing } + end := i + 2 + for end < len(runes) && runes[end] != '}' { + end++ + } + if end < len(runes) { + // Extract parameter name + paramName := string(runes[i+2 : end]) + if val, ok := params[paramName]; ok { + result.WriteString(val) + } else { + logger.Warn("parameter not found", "param", paramName) + result.WriteString(string(runes[i : end+1])) + } + i = end + 1 + continue + } + } + + // Check for command expansion: !`...` + if i+2 < len(runes) && runes[i] == '!' && runes[i+1] == '`' { + // Find the closing ` + end := i + 2 + for end < len(runes) && runes[end] != '`' { + end++ + } + if end < len(runes) { + // Extract command + command := string(runes[i+2 : end]) + cmd := exec.Command("sh", "-c", command) + output, err := cmd.CombinedOutput() + if err != nil { + logger.Warn("command expansion failed", "command", command, "error", err) + } + // Write command output (even if command failed, output may contain error info) + result.WriteString(string(output)) + i = end + 1 + continue + } + } + + // Check for path expansion: @path + if runes[i] == '@' && (i == 0 || isWhitespaceRune(runes[i-1])) { + // Found potential path expansion at start or after whitespace + pathStart := i + 1 + pathEnd := pathStart + + // Scan for the end of the path (whitespace or end of string) + // Handle escaped spaces + for pathEnd < len(runes) { + if runes[pathEnd] == '\\' && pathEnd+1 < len(runes) && runes[pathEnd+1] == ' ' { + // Escaped space, skip both characters + pathEnd += 2 + } else if isWhitespaceRune(runes[pathEnd]) { + // Unescaped whitespace marks end of path + break + } else { + pathEnd++ + } + } + + if pathEnd > pathStart { + // Extract and unescape the path + path := unescapePath(string(runes[pathStart:pathEnd])) + + // Validate the path + if err := validatePath(path); err != nil { + logger.Warn("path validation failed", "path", path, "error", err) + // Return the original @path if validation fails + result.WriteString(string(runes[i:pathEnd])) + i = pathEnd + continue + } + + // Read the file + fileContent, err := os.ReadFile(path) + if err != nil { + logger.Warn("path expansion failed", "path", path, "error", err) + // Return the original @path if file doesn't exist + result.WriteString(string(runes[i:pathEnd])) + } else { + // Expand to file content + result.Write(fileContent) + } + + i = pathEnd + continue + } + } + + // No expansion found, write the current rune + result.WriteRune(runes[i]) + i++ + } + + return result.String() +} + +// isWhitespaceRune checks if a rune is whitespace (space, tab, newline, carriage return) +func isWhitespaceRune(r rune) bool { + return r == ' ' || r == '\t' || r == '\n' || r == '\r' +} + +// unescapePath removes escape sequences from a path (specifically \) +func unescapePath(path string) string { + return strings.ReplaceAll(path, "\\ ", " ") +} + +// validatePath validates a file path for basic safety checks. +// Note: This tool is designed to work with user-created markdown files in their +// workspace and grants read access to files the user can read. The primary +// defense is that users should only use trusted markdown files. +func validatePath(path string) error { + // Check for null bytes which are never valid in file paths + if strings.Contains(path, "\x00") { + return fmt.Errorf("path contains null byte") + } + + // We intentionally allow paths with .. components as they may be + // legitimate references to files in parent directories within the + // user's workspace. The security model is that users should only + // use trusted markdown files, similar to running trusted shell scripts. + + return nil +} diff --git a/pkg/codingcontext/expander_test.go b/pkg/codingcontext/expander_test.go new file mode 100644 index 00000000..663e00e5 --- /dev/null +++ b/pkg/codingcontext/expander_test.go @@ -0,0 +1,431 @@ +package codingcontext + +import ( + "log/slog" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestExpandParameters(t *testing.T) { + tests := []struct { + name string + params Params + content string + expected string + }{ + { + name: "single parameter expansion", + params: Params{"name": "Alice"}, + content: "Hello, ${name}!", + expected: "Hello, Alice!", + }, + { + name: "multiple parameter expansions", + params: Params{"first": "John", "last": "Doe"}, + content: "${first} ${last}", + expected: "John Doe", + }, + { + name: "parameter not found - returns unchanged with warning", + params: Params{}, + content: "Value: ${missing}", + expected: "Value: ${missing}", + }, + { + name: "mixed found and not found parameters", + params: Params{"found": "yes"}, + content: "${found} and ${notfound}", + expected: "yes and ${notfound}", + }, + { + name: "no parameters to expand", + params: Params{"key": "value"}, + content: "Plain text without parameters", + expected: "Plain text without parameters", + }, + { + name: "parameter with special characters", + params: Params{"path": "/tmp/file.txt"}, + content: "File: ${path}", + expected: "File: /tmp/file.txt", + }, + { + name: "unclosed parameter - treated as literal", + params: Params{"name": "value"}, + content: "Text ${name and more", + expected: "Text ${name and more", + }, + { + name: "empty parameter name - expands to empty", + params: Params{"": "value"}, + content: "Text ${} more", + expected: "Text value more", + }, + { + name: "parameter at end of string", + params: Params{"end": "final"}, + content: "Start ${end}", + expected: "Start final", + }, + { + name: "nested braces - outer takes precedence", + params: Params{"outer": "value"}, + content: "${outer{inner}}", + expected: "${outer{inner}}", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expand(tt.content, tt.params, slog.New(slog.NewTextHandler(os.Stderr, nil))) + if result != tt.expected { + t.Errorf("expand() = %q, want %q", result, tt.expected) + } + }) + } +} + +func TestExpandCommands(t *testing.T) { + tests := []struct { + name string + content string + expected string + contains string // Use contains for commands with variable output + }{ + { + name: "simple echo command", + content: "Output: !`echo hello`", + expected: "Output: hello\n", + }, + { + name: "command with multiple words", + content: "!`echo hello world`", + expected: "hello world\n", + }, + { + name: "multiple commands in content", + content: "!`echo foo` and !`echo bar`", + expected: "foo\n and bar\n", + }, + { + name: "command that fails - returns output (empty for false)", + content: "!`false` failed", + expected: " failed", + }, + { + name: "command with pipes", + content: "!`echo test | tr a-z A-Z`", + expected: "TEST\n", + }, + { + name: "no commands to expand", + content: "Plain text without commands", + expected: "Plain text without commands", + }, + { + name: "command with newline in output", + content: "!`printf 'line1\\nline2'`", + expected: "line1\nline2", + }, + { + name: "command output not trimmed", + content: "!`echo -n hello` world", + expected: "hello world", + }, + { + name: "unclosed backtick - treated as literal", + content: "Text !`echo test more", + expected: "Text !`echo test more", + }, + { + name: "empty command", + content: "Text !`` more", + expected: "Text more", + }, + { + name: "command at end of string", + content: "Start !`echo end`", + expected: "Start end\n", + }, + { + name: "command with error output", + content: "Error: !`cat /nonexistent/file 2>&1`", + contains: "No such file or directory", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expand(tt.content, Params{}, slog.New(slog.NewTextHandler(os.Stderr, nil))) + if tt.contains != "" { + if !strings.Contains(result, tt.contains) { + t.Errorf("expand() = %q, should contain %q", result, tt.contains) + } + } else { + if result != tt.expected { + t.Errorf("expand() = %q, want %q", result, tt.expected) + } + } + }) + } +} + +func TestExpandPaths(t *testing.T) { + // Create a temporary directory for test files + tmpDir := t.TempDir() + + // Create test files + testFile1 := filepath.Join(tmpDir, "test1.txt") + if err := os.WriteFile(testFile1, []byte("content1"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + testFile2 := filepath.Join(tmpDir, "test2.txt") + if err := os.WriteFile(testFile2, []byte("content2"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + testFileWithSpace := filepath.Join(tmpDir, "test file.txt") + if err := os.WriteFile(testFileWithSpace, []byte("spaced content"), 0644); err != nil { + t.Fatalf("failed to create test file with space: %v", err) + } + + tests := []struct { + name string + content string + expected string + }{ + { + name: "single file expansion", + content: "File content: @" + testFile1, + expected: "File content: content1", + }, + { + name: "multiple file expansions", + content: "First: @" + testFile1 + " Second: @" + testFile2, + expected: "First: content1 Second: content2", + }, + { + name: "file not found - returns unchanged", + content: "Missing: @/nonexistent/file.txt", + expected: "Missing: @/nonexistent/file.txt", + }, + { + name: "file path with escaped space", + content: "Content: @" + strings.ReplaceAll(testFileWithSpace, " ", "\\ "), + expected: "Content: spaced content", + }, + { + name: "no paths to expand", + content: "Plain text without @ paths", + expected: "Plain text without @ paths", + }, + { + name: "@ not at start or after whitespace is not expanded", + content: "email@example.com", + expected: "email@example.com", + }, + { + name: "@ after newline", + content: "line1\n@" + testFile1, + expected: "line1\ncontent1", + }, + { + name: "path at end without trailing whitespace", + content: "End: @" + testFile1, + expected: "End: content1", + }, + { + name: "lone @ with no path", + content: "Text @ more", + expected: "Text @ more", + }, + { + name: "multiple consecutive @ symbols", + content: "Text @@ more", + expected: "Text @@ more", + }, + { + name: "path with backslash not escaping space - whole path not found", + content: "Path: @" + testFile1 + "\\notspace", + expected: "Path: @" + testFile1 + "\\notspace", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expand(tt.content, Params{}, slog.New(slog.NewTextHandler(os.Stderr, nil))) + if result != tt.expected { + t.Errorf("expand() = %q, want %q", result, tt.expected) + } + }) + } +} + +func TestExpandCombined(t *testing.T) { + // Create a temporary directory for test files + tmpDir := t.TempDir() + + testFile := filepath.Join(tmpDir, "data.txt") + if err := os.WriteFile(testFile, []byte("file-${param}"), 0644); err != nil { + t.Fatalf("failed to create test file: %v", err) + } + + tests := []struct { + name string + params Params + content string + expected string + }{ + { + name: "combined expansions - command, path, parameter", + params: Params{"name": "World"}, + content: "!`echo Hello` ${name} from @" + testFile, + expected: "Hello\n World from file-${param}", + }, + { + name: "file content NOT re-expanded (security fix)", + params: Params{"param": "replaced"}, + content: "@" + testFile, + expected: "file-${param}", // Changed: file content is not re-expanded + }, + { + name: "command output NOT re-expanded (security fix)", + params: Params{"dynamic": "value"}, + content: "!`echo '${dynamic}'`", + expected: "${dynamic}\n", // Changed: command output is not re-expanded + }, + { + name: "all expansion types together", + params: Params{"x": "X", "y": "Y"}, + content: "${x} !`echo middle` ${y}", + expected: "X middle\n Y", + }, + { + name: "no expansions needed", + params: Params{}, + content: "plain text", + expected: "plain text", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expand(tt.content, tt.params, slog.New(slog.NewTextHandler(os.Stderr, nil))) + if result != tt.expected { + t.Errorf("expand() = %q, want %q", result, tt.expected) + } + }) + } +} + +func TestExpandBasic(t *testing.T) { + // Test basic expansion functionality + content := "Hello ${name}!" + params := Params{"name": "World"} + logger := slog.New(slog.NewTextHandler(os.Stderr, nil)) + + result := expand(content, params, logger) + expected := "Hello World!" + if result != expected { + t.Errorf("expand() = %q, want %q", result, expected) + } +} + +func TestValidatePath(t *testing.T) { + tests := []struct { + name string + path string + wantErr bool + }{ + { + name: "simple filename", + path: "file.txt", + wantErr: false, + }, + { + name: "absolute path", + path: "/tmp/file.txt", + wantErr: false, + }, + { + name: "relative path with subdirectory", + path: "subdir/file.txt", + wantErr: false, + }, + { + name: "path with null byte - rejected", + path: "file\x00.txt", + wantErr: true, + }, + { + name: "path with directory traversal - allowed (legitimate use case)", + path: "../../../etc/passwd", + wantErr: false, + }, + { + name: "path with .. - allowed", + path: "dir/../file.txt", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validatePath(tt.path) + if (err != nil) != tt.wantErr { + t.Errorf("validatePath() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} + +func TestExpandSecurityNoReExpansion(t *testing.T) { + tests := []struct { + name string + params Params + content string + expected string + desc string + }{ + { + name: "parameter value with command syntax not expanded", + params: Params{"evil": "!`echo INJECTED`"}, + content: "Value: ${evil}", + expected: "Value: !`echo INJECTED`", + desc: "Parameter containing command syntax should not be executed", + }, + { + name: "parameter value with path syntax not expanded", + params: Params{"path": "@/etc/passwd"}, + content: "Path: ${path}", + expected: "Path: @/etc/passwd", + desc: "Parameter containing path syntax should not be read", + }, + { + name: "command output with parameter syntax not expanded", + params: Params{"secret": "SECRET"}, + content: "!`echo '${secret}'`", + expected: "${secret}\n", + desc: "Command output containing parameter syntax should not be expanded", + }, + { + name: "command output with path syntax not expanded", + params: Params{}, + content: "!`echo '@/etc/passwd'`", + expected: "@/etc/passwd\n", + desc: "Command output containing path syntax should not be read", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := expand(tt.content, tt.params, slog.New(slog.NewTextHandler(os.Stderr, nil))) + if result != tt.expected { + t.Errorf("Security test failed: %s\nexpand() = %q, want %q", tt.desc, result, tt.expected) + } + }) + } +}