Add content expander for parameters, commands, and file paths with injection protection by Copilot · Pull Request #151 · kitproj/coding-context-cli

Copilot · 2025-12-10T22:02:10Z

Fix Command Injection Vulnerability and Code Review Feedback

Security Issue (Fixed in commit `3f3f3f3`)

The previous implementation processed expansions in multiple passes which allowed command injection attacks. Fixed by processing rune-by-rune in a single pass.

Code Review Changes

Changes

Test Coverage

Unit Tests (60+ test cases)

Parameter expansion: Basic expansion, missing parameters, special characters, edge cases (unclosed syntax, empty names, nested braces)
Command expansion: Successful commands, failed commands with stderr output, edge cases (unclosed backticks, empty commands, commands at end of string)
Path expansion: File inclusion, escaped spaces, missing files, edge cases (lone @, consecutive @, backslash handling)
Security tests: Verifies no re-expansion of expanded content

Integration Tests

Basic functionality test for all three expansion types
Security test verifying no re-expansion in real-world scenario with file content, parameter values, and command output containing expansion syntax

Test Results

✅ All unit tests pass (60+ test cases)
✅ All integration tests pass including security test
✅ Edge case tests for malformed input
✅ Security tests confirm no re-expansion
✅ Lint clean

Documentation

✅ Added comprehensive "Content Expansion" section to docs/reference/file-formats.md
✅ Documented all three expansion types with examples
✅ Documented security model and injection protection
✅ Included usage guidelines and behavior notes

Original prompt

Implement a new expander to expand content.

Parameter Expansion

${param_name}

Expands to parameter value. The parameter can only contain command parameter names. If the parameter is not found, then it expands to the parameter name (i.e. unchanged) and logs a warning.

Command Expansion
!`command`
Expands to the output of the shell command. If the command fails, then a warning is logged.

Path Expansion
@path
Expand to the contents of the path. If the path is not found, then it expands to the path (i.e. unchanged) and logs a warning. This is delimited by a space. If the filename needs a space in it, it must be escaped.

Original prompt

Implement a new expander to expand content.

Parameter Expansion

${param_name}

Expands to parameter value. The parameter can only contain command parameter names. If the parameter is not found, then it expands to the parameter name (i.e. unchanged) and logs a warning.

Command Expansion
!`command`
Expands to the output of the shell command. If the command fails, then a warning is logged.

Path Expansion
@path
Expand to the contents of the path. If the path is not found, then it expands to the path (i.e. unchanged) and logs a warning. This is delimited by a space. If the filename needs a space in it, it must be escaped.

Original prompt

Implement a new expander to expand content.

Parameter Expansion

${param_name}

Expands to parameter value. The parameter can only contain command parameter names. If the parameter is not found, then it expands to the parameter name (i.e. unchanged) and logs a warning.

Command Expansion
!`command`
Expands to the output of the shell command. If the command fails, then a warning is logged.

Path Expansion
@path
Expand to the contents of the path. If the path is not found, then it expands to the path (i.e. unchanged) and logs a warning. This is delimited by a space. If the filename needs a space in it, it must be escaped.

Original prompt

Implement a new expander to expand content.

Parameter Expansion

${param_name}

Expands to parameter value. The parameter can only contain command parameter names. If the parameter is not found, then it expands to the parameter name (i.e. unchanged) and logs a warning.

Command Expansion
!`command`
Expands to the output of the shell command. If the command fails, then a warning is logged.

Path Expansion
@path
Expand to the contents of the path. If the path is not found, then it expands to the path (i.e. unchanged) and logs a warning. This is delimited by a space. If the filename needs a space in it, it must be escaped.

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

alexec · 2025-12-10T22:35:24Z

@copilot The current solution could expand a parameter which then expands a command, causing command injection attack.

You must process the file rune by rune.

Process expansions in a single pass, rune-by-rune, to prevent command injection where expanded content (from commands or files) contains further expansion sequences that would be re-expanded. Security issue: Previously, expansions were done in multiple passes (commands -> paths -> parameters), allowing: - Command output containing ${param} to be expanded - File content containing ${param} or !`cmd` to be expanded - Parameters containing !`cmd` or @path to be expanded Fix: Process all three expansion types in a single pass by scanning rune-by-rune, which ensures expanded content is never re-processed. Tests updated to verify security fix and ensure no re-expansion occurs. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Copilot · 2025-12-10T22:49:19Z

@copilot The current solution could expand a parameter which then expands a command, causing command injection attack.

You must process the file rune by rune.

Fixed in commit 3f3f3f3. Rewrote the expander to process content rune-by-rune in a single pass instead of multiple passes. This prevents command injection where expanded content (from commands or files) contains further expansion sequences.

Security tests added to verify:

Command output containing ${param} is not re-expanded
File content containing ${param} is not re-expanded
Parameters containing !cmd`` are not executed
Parameters containing @path are not read

All existing tests pass.

- Change Expander to expander (private struct) - Change params field from Params type to map[string]string directly - Update all references to use lowercase expander Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Replace string concatenation with rune slice notation for better efficiency and to preserve exact original text when parameters are not found or commands fail. Changes: - Line 54: "${" + paramName + "}" → string(runes[i : end+1]) - Line 76: "!`" + command + "`" → string(runes[i : end+1]) Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

@alexec

Refactored per @alexec feedback: - Removed expander struct - Removed NewExpander function - Made expand function private (lowercase) - Pass params and logger as function parameters - Updated all tests to use new function signature - Simplified TestNewExpander to TestExpandWithNilLogger This results in a simpler, more functional API with no unnecessary struct wrapper. Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

alexec

@copilot Also update the docs

Copilot

Pull request overview

This PR implements a new content expander that supports three types of expansions: parameter expansion (${param_name}), command expansion (!`command`), and path expansion (@path). The implementation includes a critical security fix that processes expansions in a single rune-by-rune pass to prevent injection attacks where expanded content contains further expansion sequences. The PR also incorporates code review feedback to use a functional approach with a private expand function.

Key Changes

Single-pass rune-by-rune expansion processing to prevent command injection vulnerabilities
Support for three expansion types: parameters, shell commands, and file paths
Integration with existing parameter expansion in context.go

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 10 comments.

File	Description
pkg/codingcontext/expander.go	New file implementing single-pass expansion logic with parameter, command, and path expansion support
pkg/codingcontext/expander_test.go	Comprehensive test suite with 45+ test cases covering all expansion types, edge cases, and security scenarios
pkg/codingcontext/context.go	Updated `expandParams` method to use new `expand` function and merge parameters correctly
integration_test.go	New integration test verifying all three expansion types work together end-to-end

Copilot · 2025-12-11T03:29:52Z

+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",
+		},
+	}
+
+	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)
+			}
+		})
+	}
+}


The parameter expansion tests are missing important edge cases. Consider adding tests for: unclosed parameter syntax like ${name (should be treated as literal text), empty parameter name ${}, nested or malformed patterns like ${${nested}}, and parameters at the very end of the string. These edge cases are important for ensuring the parser handles malformed input gracefully.

@copilot apply changes based on this feedback

Done in commit e637274. Added edge case tests for: unclosed parameter syntax, empty parameter name, parameter at end of string, and nested braces.

@alexec

Code changes per @alexec feedback: - Removed nil logger check in expand function (line 18) - Preserve command output as-is, no longer trim trailing newlines (line 65) - Updated all test expectations to account for newlines in command output - Changed TestExpandWithNilLogger to TestExpandBasic Documentation updates: - Expanded "Parameter Substitution" section to "Content Expansion" - Added documentation for all three expansion types: - Parameter expansion (${param}) - Command expansion (!`cmd`) - Path expansion (@path) - Documented security model (single-pass, no re-expansion) - Added examples and usage notes for each expansion type Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Copilot · 2025-12-11T03:44:33Z

@copilot Also update the docs

Done in commit b3637ac. Added comprehensive "Content Expansion" section to docs/reference/file-formats.md documenting all three expansion types (parameter, command, path) with examples and security notes.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

alexec · 2025-12-11T03:49:24Z

+				if err != nil {
+					logger.Warn("command expansion failed", "command", command, "error", err)
+					// Return the original !`command` if command fails
+					result.WriteString(string(runes[i : end+1]))


@copilot if the command fails, still put the output in place.

Done in commit e637274. Command expansion now outputs the combined stdout/stderr even when the command fails, allowing error messages to be included in the output.

alexec · 2025-12-11T16:12:28Z

+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",
+		},
+	}
+
+	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)
+			}
+		})
+	}
+}


@copilot apply changes based on this feedback

Changes based on code review feedback: - Command expansion now outputs stderr even when command fails - Added edge case tests for parameter expansion: - Unclosed parameter syntax - Empty parameter name - Parameter at end of string - Nested braces - Added edge case tests for command expansion: - Unclosed backtick - Empty command - Command at end of string - Command with error output - Added edge case tests for path expansion: - Path at end without trailing whitespace - Lone @ with no path - Multiple consecutive @ symbols - Path with backslash not escaping space - Added security integration test verifying no re-expansion of: - File content containing expansion syntax - Parameter values containing command syntax - Command output containing parameter syntax Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Initial plan

512361a

Copilot AI assigned Copilot and alexec Dec 10, 2025

Copilot started work on behalf of alexec December 10, 2025 22:02 View session

Copilot AI and others added 3 commits December 10, 2025 22:08

Implement expander with parameter, command, and path expansion

c258063

Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Add security documentation and validation for expander

674c33e

Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Fix code review issues: remove duplicate test and unused import

7fa72cc

Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Copilot AI changed the title ~~[WIP] Add new expander to expand content with parameters and commands~~ Add content expander for parameters, commands, and file paths Dec 10, 2025

Copilot AI requested a review from alexec December 10, 2025 22:20

Copilot finished work on behalf of alexec December 10, 2025 22:20

Copilot started work on behalf of alexec December 10, 2025 22:35 View session

Copilot AI changed the title ~~Add content expander for parameters, commands, and file paths~~ Add content expander for parameters, commands, and file paths with injection protection Dec 10, 2025

Copilot finished work on behalf of alexec December 10, 2025 22:52

alexec requested changes Dec 11, 2025

View reviewed changes

Comment thread pkg/codingcontext/expander.go Outdated

Comment thread pkg/codingcontext/expander.go Outdated

Copilot started work on behalf of alexec December 11, 2025 01:54 View session

alexec requested changes Dec 11, 2025

View reviewed changes

Comment thread pkg/codingcontext/expander.go Outdated

Make Expander private and use map type directly

8d435ad

- Change Expander to expander (private struct) - Change params field from Params type to map[string]string directly - Update all references to use lowercase expander Co-authored-by: alexec <1142830+alexec@users.noreply.github.com>

Copilot AI requested a review from alexec December 11, 2025 02:04

Copilot finished work on behalf of alexec December 11, 2025 02:04

Copilot started work on behalf of alexec December 11, 2025 02:04 View session

Copilot finished work on behalf of alexec December 11, 2025 02:12

alexec requested changes Dec 11, 2025

View reviewed changes

Comment thread pkg/codingcontext/expander.go Outdated

Copilot started work on behalf of alexec December 11, 2025 02:59 View session

alexec marked this pull request as ready for review December 11, 2025 03:24

Copilot AI review requested due to automatic review settings December 11, 2025 03:24

alexec enabled auto-merge (squash) December 11, 2025 03:24

Copilot started reviewing on behalf of alexec December 11, 2025 03:25 View session

alexec requested changes Dec 11, 2025

View reviewed changes

Comment thread pkg/codingcontext/expander.go Outdated

Comment thread pkg/codingcontext/expander.go Outdated

Copilot AI requested a review from alexec December 11, 2025 03:29

Copilot finished work on behalf of alexec December 11, 2025 03:29

Copilot started work on behalf of alexec December 11, 2025 03:29 View session

Copilot AI reviewed Dec 11, 2025

View reviewed changes

auto-merge was automatically disabled December 11, 2025 03:43
Head branch was pushed to by a user without write access

Update expander.go

c9a7725

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot finished work on behalf of alexec December 11, 2025 03:51

Update pkg/codingcontext/expander_test.go

e49fee2

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

alexec requested changes Dec 11, 2025

View reviewed changes

Copilot started work on behalf of alexec December 11, 2025 16:13 View session

Copilot AI requested a review from alexec December 11, 2025 16:21

Copilot finished work on behalf of alexec December 11, 2025 16:21

alexec approved these changes Dec 11, 2025

View reviewed changes

alexec merged commit ff888eb into main Dec 11, 2025
2 checks passed

alexec deleted the copilot/add-new-expander-functionality branch December 11, 2025 16:59

Conversation

Copilot AI commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fix Command Injection Vulnerability and Code Review Feedback

Security Issue (Fixed in commit 3f3f3f3)

Code Review Changes

Changes

Test Coverage

Unit Tests (60+ test cases)

Integration Tests

Test Results

Documentation

Parameter Expansion

Command Expansion

Path Expansion

Parameter Expansion

Command Expansion

Path Expansion

Parameter Expansion

Command Expansion

Path Expansion

Parameter Expansion

Command Expansion

Path Expansion

Uh oh!

alexec commented Dec 10, 2025

Uh oh!

Copilot AI commented Dec 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexec left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

alexec Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Dec 11, 2025

Uh oh!

alexec Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alexec Dec 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Copilot AI commented Dec 10, 2025 •

edited

Loading

Security Issue (Fixed in commit `3f3f3f3`)