Skip to content

Commit 807e61c

Browse files
authored
fix(git): use env vars for safe commit message handling (#6)
## Summary Fixed shell expansion issues in `git-commit` workflow by adopting the GitHub Actions security pattern for handling user-provided content. ## Problem Commit messages containing special characters were being corrupted: - ❌ Backticks (`` ` ``) were stripped or executed as command substitution - ❌ Dollar signs (`$`) triggered variable expansion - ❌ Quotes could break shell parsing Example of broken message: ``` fix: handle `backticks` and $variables ``` Would become: ``` fix: handle and ``` ## Background: Variable Syntax Migration This fix builds on commit c234599 which migrated all workflow variables from `${...}` to `{{...}}` syntax: - **Previous syntax**: `${inputs.commit_message}` (ambiguous with shell variables) - **New syntax**: `{{inputs.commit_message}}` (clear separation from shell) - **Benefits**: Matches Jinja2 standard, eliminates shell expansion ambiguity - **Scope**: Complete migration of 32 template workflows, 25 test workflows, all docs However, even with `{{...}}` syntax, the variable content was still vulnerable to shell expansion when resolved into command strings. ## Root Cause The `select_message` block passed commit message content directly in shell command strings with double quotes: ```yaml command: | printf '%s' "{{inputs.commit_message}}" > "$SCRATCH/message.txt" ``` **Execution flow:** 1. Workflow engine resolves `{{inputs.commit_message}}` → `"fix: handle \`backticks\` here"` 2. String inserted into command: `printf '%s' "fix: handle \`backticks\` here" > ...` 3. Shell executes backticks as command substitution → content stripped The `{{...}}` syntax change separated workflow vars from shell vars syntactically, but didn't prevent shell expansion of the **content** itself. ## Solution Adopted the **GitHub Actions security pattern**: Pass sensitive content via environment variables instead of inline shell substitution. **Before:** ```yaml command: | if [ -n "{{inputs.commit_message}}" ]; then printf '%s' "{{inputs.commit_message}}" > "$SCRATCH/message.txt" else printf '%s' "{{blocks.llm_generate_message.outputs.response}}" > "$SCRATCH/message.txt" fi ``` **After:** ```yaml env: MANUAL_MESSAGE: "{{inputs.commit_message}}" LLM_MESSAGE: "{{blocks.llm_generate_message.outputs.response}}" command: | # Use environment variables to safely pass content (GitHub Actions pattern) # This prevents shell expansion of special chars (backticks, $, quotes) if [ -n "$MANUAL_MESSAGE" ]; then printf '%s' "$MANUAL_MESSAGE" > "$SCRATCH/message.txt" else printf '%s' "$LLM_MESSAGE" > "$SCRATCH/message.txt" fi ``` ## How It Works 1. **Workflow engine** resolves `{{...}}` variables at YAML level (from commit c234599) 2. **Python** sets environment variables through the OS (no shell interpretation) 3. **Shell** reads `$VAR` from environment safely without expanding content This completes the security model: - ✅ **Syntax level**: `{{...}}` (workflow) vs `${...}` (shell) - clear separation - ✅ **Content level**: Environment variables prevent shell expansion of user content ## How Industry Standards Handle This **GitHub Actions:** ```yaml # UNSAFE: Content in command string run: echo "${{ github.event.issue.title }}" # SAFE: Content via environment variable env: TITLE: ${{ github.event.issue.title }} run: echo "$TITLE" ``` **GitLab CI:** Uses same pattern with `variables:` block **Argo Workflows:** Uses `env:` fields for parameter passing This is the industry-standard security practice for CI/CD systems. ## Testing Tested with commit message containing all special characters: ``` fix(git): use env vars for safe message handling Changed `select_message` block with `backticks`, $variables, and user's quotes. Pattern: set `$MESSAGE` via env, read $VAR safely. ``` **Result**: All special characters preserved correctly in the final commit! ✅ Both commits in this PR were created using the fixed workflow, demonstrating it works correctly. ## Changes - Modified `src/workflows_mcp/templates/git/git-commit.yaml`: - Added `env` field to `select_message` block with `MANUAL_MESSAGE` and `LLM_MESSAGE` - Moved message content from command string to environment variables - Updated shell command to reference `$MANUAL_MESSAGE` and `$LLM_MESSAGE` instead of inline substitution - Added comments explaining the GitHub Actions security pattern ## Benefits - ✅ Prevents shell expansion of backticks, dollar signs, and quotes - ✅ Matches industry-standard CI/CD security practices (GitHub Actions, GitLab CI) - ✅ Uses existing Shell executor capabilities (no new executors needed) - ✅ Maintains the file-based commit approach (`git commit -F`) - ✅ Completes the security model started by commit c234599 ## References - Variable syntax migration: commit c234599 - GitHub Actions Security: [Preventing script injection attacks](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-an-intermediate-environment-variable) - Shell executor env support: `src/workflows_mcp/engine/executors_core.py` lines 276-279
2 parents 54f41c4 + 0e2b476 commit 807e61c

92 files changed

Lines changed: 2188 additions & 2161 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

README.md

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -133,10 +133,10 @@ blocks:
133133
- id: greet
134134
type: Shell
135135
inputs:
136-
command: printf "Hello, ${inputs.name}!"
136+
command: printf "Hello, {{inputs.name}}!"
137137

138138
outputs:
139-
greeting: "${blocks.greet.outputs.stdout}"
139+
greeting: "{{blocks.greet.outputs.stdout}}"
140140
```
141141
142142
### Key Features
@@ -147,9 +147,9 @@ Reference inputs, block outputs, and metadata anywhere in your workflow.
147147
148148
Examples:
149149
150-
- [Using workflow inputs](tests/workflows/core/variable-resolution/inputs.yaml) - `${inputs.field_name}`
151-
- [Using block outputs](tests/workflows/core/variable-resolution/block-outputs.yaml) - `${blocks.block_id.outputs.field}`
152-
- [Using metadata](tests/workflows/core/variable-resolution/metadata.yaml) - `${metadata.workflow_name}`
150+
- [Using workflow inputs](tests/workflows/core/variable-resolution/inputs.yaml) - `{{inputs.field_name}}`
151+
- [Using block outputs](tests/workflows/core/variable-resolution/block-outputs.yaml) - `{{blocks.block_id.outputs.field}}`
152+
- [Using metadata](tests/workflows/core/variable-resolution/metadata.yaml) - `{{metadata.workflow_name}}`
153153
- [Variable shortcuts](tests/workflows/core/variable-resolution/shortcuts.yaml) - convenient shorthand syntax
154154

155155
**Conditionals:**
@@ -167,9 +167,9 @@ Use simple shortcuts to check if blocks succeeded, failed, or were skipped.
167167

168168
Examples:
169169

170-
- [Success detection](tests/workflows/core/block-status/success-detection.yaml) - `${blocks.id.succeeded}`
171-
- [Failure detection](tests/workflows/core/block-status/failure-detection.yaml) - `${blocks.id.failed}`
172-
- [Skip detection](tests/workflows/core/block-status/skip-detection.yaml) - `${blocks.id.skipped}`
170+
- [Success detection](tests/workflows/core/block-status/success-detection.yaml) - `{{blocks.id.succeeded}}`
171+
- [Failure detection](tests/workflows/core/block-status/failure-detection.yaml) - `{{blocks.id.failed}}`
172+
- [Skip detection](tests/workflows/core/block-status/skip-detection.yaml) - `{{blocks.id.skipped}}`
173173

174174
**Parallel Execution:**
175175

schema.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,7 +650,7 @@
650650
},
651651
"WorkflowInput": {
652652
"additionalProperties": false,
653-
"description": "Input model for Workflow executor.\n\nSupports variable references from parent context:\n- ${inputs.field}: Parent workflow inputs\n- ${blocks.block_id.outputs.field}: Parent block outputs\n- ${metadata.field}: Parent workflow metadata\n\nVariable resolution happens in parent context before passing to child.",
653+
"description": "Input model for Workflow executor.\n\nSupports variable references from parent context:\n- {{inputs.field}}: Parent workflow inputs\n- {{blocks.block_id.outputs.field}}: Parent block outputs\n- {{metadata.field}}: Parent workflow metadata\n\nVariable resolution happens in parent context before passing to child.",
654654
"properties": {
655655
"workflow": {
656656
"description": "Workflow name to execute",

src/workflows_mcp/engine/executors_interactive.py

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ class PromptExecutor(BlockExecutor):
8585
type: Shell
8686
inputs:
8787
command: "./deploy.sh"
88-
condition: ${blocks.confirm_deploy.outputs.response} == 'yes'
88+
condition: "{{blocks.confirm_deploy.outputs.response}} == 'yes'"
8989
depends_on: [confirm_deploy]
9090
9191
Example YAML - Multiple Choice:
@@ -106,7 +106,7 @@ class PromptExecutor(BlockExecutor):
106106
type: Shell
107107
inputs:
108108
command: "./deploy.sh dev"
109-
condition: ${blocks.select_env.outputs.response} == '1'
109+
condition: "{{blocks.select_env.outputs.response}} == '1'"
110110
depends_on: [select_env]
111111
112112
Example YAML - Free-form Input:
@@ -124,7 +124,7 @@ class PromptExecutor(BlockExecutor):
124124
- id: create_commit
125125
type: Shell
126126
inputs:
127-
command: git commit -m "${blocks.get_commit_msg.outputs.response}"
127+
command: git commit -m "{{blocks.get_commit_msg.outputs.response}}"
128128
depends_on: [get_commit_msg]
129129
130130
Benefits of Simplified Design:

src/workflows_mcp/engine/executors_workflow.py

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ class WorkflowInput(BlockInput):
2424
Input model for Workflow executor.
2525
2626
Supports variable references from parent context:
27-
- ${inputs.field}: Parent workflow inputs
28-
- ${blocks.block_id.outputs.field}: Parent block outputs
29-
- ${metadata.field}: Parent workflow metadata
27+
- {{inputs.field}}: Parent workflow inputs
28+
- {{blocks.block_id.outputs.field}}: Parent block outputs
29+
- {{metadata.field}}: Parent workflow metadata
3030
3131
Variable resolution happens in parent context before passing to child.
3232
"""
@@ -53,7 +53,7 @@ class WorkflowExecutor(BlockExecutor):
5353
Architecture:
5454
- Returns full child Execution (includes child's blocks!)
5555
- Orchestrator stores child Execution in parent.blocks[block_id]
56-
- Enables deep access: ${blocks.run_tests.blocks.pytest.outputs.exit_code}
56+
- Enables deep access: {{blocks.run_tests.blocks.pytest.outputs.exit_code}}
5757
- Uses ExecutionContext for workflow registry access
5858
- Uses WorkflowRunner for child workflow execution
5959
- Recursion depth limiting via context.check_recursion_depth()
@@ -72,8 +72,8 @@ class WorkflowExecutor(BlockExecutor):
7272
}
7373
7474
Variable Access:
75-
${blocks.run_tests.outputs.test_passed} # Child's workflow output
76-
${blocks.run_tests.blocks.pytest.outputs.exit_code} # Drill down!
75+
{{blocks.run_tests.outputs.test_passed}} # Child's workflow output
76+
{{blocks.run_tests.blocks.pytest.outputs.exit_code}} # Drill down!
7777
7878
Pause Propagation:
7979
If child workflow pauses (Prompt block), ExecutionPaused exception
@@ -86,7 +86,7 @@ class WorkflowExecutor(BlockExecutor):
8686
inputs:
8787
workflow: python-ci-pipeline
8888
inputs:
89-
project_path: "${inputs.project_path}"
89+
project_path: "{{inputs.project_path}}"
9090
"""
9191

9292
type_name: ClassVar[str] = "Workflow"

src/workflows_mcp/engine/schema.py

Lines changed: 27 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
- Required fields and types
1414
- Block type existence in registry
1515
- Dependency validity (no cycles, valid references)
16-
- Variable substitution syntax (${block_id.field})
16+
- Variable substitution syntax ({{block_id.field}})
1717
"""
1818

1919
import re
@@ -40,7 +40,7 @@ class ValueType(str, Enum):
4040
- Output parsing logic
4141
4242
These match Python's built-in types for consistency with:
43-
- isinstance() checks in conditions: isinstance(${inputs.name}, str)
43+
- isinstance() checks in conditions: isinstance({{inputs.name}}, str)
4444
- Type annotations in executor models: Field(default="", description="...")
4545
- Variable resolution return types
4646
@@ -293,14 +293,14 @@ class BlockDefinition(BaseModel):
293293
type: CreateWorktree
294294
description: "Create isolated git worktree for feature development"
295295
inputs:
296-
branch: "feature/${issue_number}"
296+
branch: "feature/{{issue_number}}"
297297
base_branch: "main"
298298
299299
- id: create_file
300300
type: CreateFile
301301
description: "Create initial README file in worktree"
302302
inputs:
303-
path: "${create_worktree.worktree_path}/README.md"
303+
path: "{{create_worktree.worktree_path}}/README.md"
304304
content: "# Feature"
305305
depends_on:
306306
- create_worktree
@@ -321,7 +321,7 @@ class BlockDefinition(BaseModel):
321321
description: "Deploy to production if tests pass"
322322
inputs:
323323
command: "echo 'Deploying...'"
324-
condition: "${run_tests.exit_code} == 0"
324+
condition: "{{run_tests.exit_code}} == 0"
325325
depends_on:
326326
- run_tests
327327
"""
@@ -407,18 +407,18 @@ class WorkflowOutputSchema(BaseModel):
407407
expressions that reference block outputs.
408408
409409
Attributes:
410-
value: Expression (e.g., "${block.outputs.field}" or "${block.exit_code}")
410+
value: Expression (e.g., "{{block.outputs.field}}" or "{{block.exit_code}}")
411411
type: Output type (str, int, float, bool, json)
412412
description: Optional human-readable output description
413413
414414
Example:
415415
outputs:
416416
test_results:
417-
value: "${run_tests.outputs.test_results}"
417+
value: "{{run_tests.outputs.test_results}}"
418418
type: json
419419
description: "Test execution results"
420420
success:
421-
value: "${run_tests.exit_code}"
421+
value: "{{run_tests.exit_code}}"
422422
type: int
423423
description: "Test exit code"
424424
"""
@@ -466,17 +466,17 @@ class WorkflowSchema(BaseModel):
466466
- id: block1
467467
type: Shell
468468
inputs:
469-
command: 'printf "Hello ${input_name}"'
469+
command: 'printf "Hello {{input_name}}"'
470470
471471
- id: block2
472472
type: Shell
473473
inputs:
474-
command: 'printf "Output: ${blocks.block1.outputs.stdout}"'
474+
command: 'printf "Output: {{blocks.block1.outputs.stdout}}"'
475475
depends_on:
476476
- block1
477477
478478
outputs:
479-
final_message: "${blocks.block2.outputs.stdout}"
479+
final_message: "{{blocks.block2.outputs.stdout}}"
480480
"""
481481

482482
# Metadata fields (flattened for YAML convenience)
@@ -602,8 +602,8 @@ def validate_no_cyclic_dependencies(self) -> "WorkflowSchema":
602602
@model_validator(mode="after")
603603
def validate_variable_substitution_syntax(self) -> "WorkflowSchema":
604604
"""Validate variable substitution syntax in all string values."""
605-
# Pattern: ${namespace.path.to.field} - captures full dotted path
606-
var_pattern = re.compile(r"\$\{([a-z_][a-z0-9_.]*)\}")
605+
# Pattern: {{namespace.path.to.field}} - captures full dotted path
606+
var_pattern = re.compile(r"\{\{([a-z_][a-z0-9_.]*)\}\}")
607607

608608
block_ids = {block.id for block in self.blocks}
609609
input_names = set(self.inputs.keys())
@@ -614,38 +614,38 @@ def validate_string_value(value: str, context: str) -> None:
614614
for var_path in matches:
615615
parts = var_path.split(".")
616616

617-
# ${inputs.field} - workflow input
617+
# {{inputs.field}} - workflow input
618618
if parts[0] == "inputs":
619619
if len(parts) < 2:
620620
raise ValueError(
621-
f"{context}: Invalid variable reference '${{{var_path}}}'. "
622-
f"Input reference must include field name: ${{inputs.field_name}}"
621+
f"{context}: Invalid variable reference '{{{{{var_path}}}}}'. "
622+
f"Input reference must include field name: {{{{inputs.field_name}}}}"
623623
)
624624
field_name = parts[1]
625625
if field_name not in input_names:
626626
raise ValueError(
627-
f"{context}: Invalid variable reference '${{{var_path}}}'. "
627+
f"{context}: Invalid variable reference '{{{{{var_path}}}}}'. "
628628
f"Input '{field_name}' does not exist. "
629629
f"Available inputs: {sorted(input_names)}"
630630
)
631631

632-
# ${blocks.block_id.namespace.field} - block outputs/metadata
633-
# Also supports shortcut: ${blocks.block_id.field} -> outputs.field
632+
# {{blocks.block_id.namespace.field}} - block outputs/metadata
633+
# Also supports shortcut: {{blocks.block_id.field}} -> outputs.field
634634
elif parts[0] == "blocks":
635635
if len(parts) < 3:
636636
raise ValueError(
637-
f"{context}: Invalid variable reference '${{{var_path}}}'. "
637+
f"{context}: Invalid variable reference '{{{{{var_path}}}}}'. "
638638
f"Block reference must be: "
639-
f"${{blocks.block_id.outputs.field}}, "
640-
f"${{blocks.block_id.metadata.field}}, or "
641-
f"${{blocks.block_id.field}} (shortcut for outputs)"
639+
f"{{{{blocks.block_id.outputs.field}}}}, "
640+
f"{{{{blocks.block_id.metadata.field}}}}, or "
641+
f"{{{{blocks.block_id.field}}}} (shortcut for outputs)"
642642
)
643643
block_id = parts[1]
644644

645645
# Check if block exists
646646
if block_id not in block_ids:
647647
raise ValueError(
648-
f"{context}: Invalid variable reference '${{{var_path}}}'. "
648+
f"{context}: Invalid variable reference '{{{{{var_path}}}}}'. "
649649
f"Block '{block_id}' does not exist. "
650650
f"Available blocks: {sorted(block_ids)}"
651651
)
@@ -654,18 +654,18 @@ def validate_string_value(value: str, context: str) -> None:
654654
# - Standard namespaces: outputs, metadata, inputs
655655
# - Custom output fields: any valid identifier
656656
# Both are valid, so no validation needed beyond block existence
657-
# If 3 parts, it's shortcut form ${blocks.block_id.field}
657+
# If 3 parts, it's shortcut form {{blocks.block_id.field}}
658658
# This is valid and will be auto-expanded to outputs.field
659659

660-
# ${metadata.field} - workflow metadata
660+
# {{metadata.field}} - workflow metadata
661661
elif parts[0] == "metadata":
662662
# Metadata references are valid (read-only workflow metadata)
663663
pass
664664

665665
# Unknown namespace
666666
else:
667667
raise ValueError(
668-
f"{context}: Invalid variable reference '${{{var_path}}}'. "
668+
f"{context}: Invalid variable reference '{{{{{var_path}}}}}'. "
669669
f"Unknown namespace '{parts[0]}'. "
670670
f"Valid namespaces: 'inputs', 'blocks', 'metadata'"
671671
)

0 commit comments

Comments
 (0)