Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .github/workflows/copilot-session-insights.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 4 additions & 1 deletion .github/workflows/copilot-session-insights.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ steps:
continue-on-error: true
env:
GH_TOKEN: ${{ secrets.GH_AW_COPILOT_TOKEN || secrets.GH_AW_GITHUB_TOKEN }}
# Security: Pass step output through environment variable to prevent template injection
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The security comment mentions "prevent template injection" but the actual risk here is lower than described. The steps.install-extension.outputs.EXTENSION_INSTALLED is a controlled step output (boolean value set by the workflow itself), not untrusted user input.

While the fix is technically correct and follows best practices by using environment variables, the comment should clarify that this is a preventive measure rather than fixing an active vulnerability. The original code was not vulnerable to template injection because step outputs from the same workflow are trusted.

Consider updating the comment to:

# Security: Use environment variable as best practice to prevent potential future template injection risk

This makes it clear that it's a preventive pattern, not a fix for an existing exploitable vulnerability.

Suggested change
# Security: Pass step output through environment variable to prevent template injection
# Security: Use environment variable as best practice to prevent potential future template injection risk

Copilot uses AI. Check for mistakes.
EXTENSION_INSTALLED: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}
run: |
# Create output directory
mkdir -p /tmp/gh-aw/agent-sessions
Expand All @@ -112,7 +114,8 @@ steps:
# Check if gh agent-task extension is installed
if ! gh agent-task --help &> /dev/null; then
echo "::warning::gh agent-task extension is not installed"
echo "::warning::Extension installation status from previous step: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}"
# Security: Use environment variable instead of template expression in bash script
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Similar to the environment variable declaration, this security comment should clarify that this is a preventive best practice rather than fixing an active vulnerability. The EXTENSION_INSTALLED variable contains a controlled boolean value set by the workflow itself.

Consider updating to:

# Security: Use environment variable as best practice to avoid template expressions in bash

This maintains the security awareness while being more accurate about the actual risk level.

Suggested change
# Security: Use environment variable instead of template expression in bash script
# Security: Use environment variable as best practice to avoid template expressions in bash

Copilot uses AI. Check for mistakes.
echo "::warning::Extension installation status from previous step: $EXTENSION_INSTALLED"
echo "::warning::This workflow requires GitHub Enterprise Copilot access"
echo "SESSIONS_AVAILABLE=false" >> "$GITHUB_OUTPUT"
exit 1
Expand Down
119 changes: 119 additions & 0 deletions specs/template-injection-prevention.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
# Template Injection Prevention in Workflows
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot move to top level specs folder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation moved to specs/template-injection-prevention.md (commit 4c6de86).


## Overview

This document explains the template injection security fix applied to workflows in this repository to prevent potential code injection attacks via GitHub Actions template expansion.

## What is Template Injection?

Template injection occurs when untrusted data flows into GitHub Actions template expressions (`${{ }}`) that are evaluated during workflow execution. This can lead to:

- Code execution in workflow steps
- Information disclosure
- Privilege escalation

## The Vulnerability Pattern

**Unsafe Pattern:**
```yaml
steps:
- name: My Step
run: |
echo "Value: ${{ steps.previous.outputs.value }}"
Comment on lines +20 to +22
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The documentation example shows ${{ steps.previous.outputs.value }} as the unsafe pattern, but the actual fix applied uses ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}.

For consistency and clarity, consider updating the example to match the actual fix:

**Unsafe Pattern:**
```yaml
steps:
  - name: Check extension status
    run: |
      echo "Status: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}"

Why concerning: While step outputs are controlled, using template expressions inside bash strings violates security best practices and creates maintenance risk.


This makes it clearer that the concern is about the **pattern** rather than the specific data being untrusted.
```suggestion
  - name: Check extension status
    run: |
      echo "Status: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}"

Copilot uses AI. Check for mistakes.
```
If the output value contains malicious content, it could be executed when the template is expanded.
## The Fix
**Safe Pattern:**
```yaml
steps:
- name: My Step
env:
MY_VALUE: ${{ steps.previous.outputs.value }}
run: |
echo "Value: $MY_VALUE"
```
By passing the value through an environment variable, the content is treated as data, not executable code.
## Changes Made
### copilot-session-insights.md
**Issue:** Template expression used directly in bash echo statement
- **Line:** 115
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The description states the original issue was at "line 205" but the actual fix was applied at line 115 in the markdown file (which becomes line 207 in the lock.yml after adding the environment variable).

The PR description appears to reference line 205 from the .lock.yml file (the compiled output) rather than the .md source file. This is confusing since developers work with the .md files.

Consider clarifying in the documentation:

**Issue:** Template expression used directly in bash echo statement
- **Line (source):** 115 in copilot-session-insights.md
- **Line (compiled):** ~207 in copilot-session-insights.lock.yml

This helps readers understand the relationship between source and compiled files.

Suggested change
- **Line:** 115
- **Line (source):** 115 in copilot-session-insights.md
- **Line (compiled):** ~207 in copilot-session-insights.lock.yml

Copilot uses AI. Check for mistakes.
- **Risk:** While using step output (controlled), the pattern could lead to injection if changed to use untrusted data
Comment on lines +46 to +47
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The documentation states "While currently using a step output (controlled), this pattern is risky and could lead to injection if changed to use untrusted data."

This description could be misleading. The actual security concern should be stated more clearly:

The issue is not a current vulnerability, but rather a code pattern that violates security best practices. Using template expressions inside bash script strings (even with trusted data) creates a maintenance hazard - if someone later changes the code to use untrusted data, they might not realize they're creating a vulnerability.

Consider revising lines 45-47 to:

- **Line:** 115  
- **Risk:** Code pattern that could become vulnerable if modified to use untrusted data in the future
- **Severity:** Preventive best practice (not an active vulnerability)

This accurately reflects that it's a preventive security measure rather than fixing an exploitable bug.

Suggested change
- **Line:** 115
- **Risk:** While using step output (controlled), the pattern could lead to injection if changed to use untrusted data
- **Line:** 115
- **Risk:** Code pattern that could become vulnerable if modified to use untrusted data in the future
- **Severity:** Preventive best practice (not an active vulnerability)

Copilot uses AI. Check for mistakes.
**Fix Applied:**
```diff
- name: List and download Copilot agent sessions
id: download-sessions
continue-on-error: true
env:
GH_TOKEN: ${{ secrets.GH_AW_COPILOT_TOKEN || secrets.GH_AW_GITHUB_TOKEN }}
+ # Security: Pass step output through environment variable to prevent template injection
+ EXTENSION_INSTALLED: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}
run: |
# ...
if ! gh agent-task --help &> /dev/null; then
echo "::warning::gh agent-task extension is not installed"
- echo "::warning::Extension installation status from previous step: ${{ steps.install-extension.outputs.EXTENSION_INSTALLED }}"
+ # Security: Use environment variable instead of template expression in bash script
+ echo "::warning::Extension installation status from previous step: $EXTENSION_INSTALLED"
echo "::warning::This workflow requires GitHub Enterprise Copilot access"
# ...
```

### mcp-inspector.md

**Status:** No template injection vulnerabilities found
- The "Setup MCPs" step name is static text
- Environment variables use secrets, which are safe
- No untrusted data flows into template expressions

## Security Best Practices

When writing GitHub Actions workflows:

1. **Never use untrusted inputs directly in template expressions:**
-`${{ github.event.issue.title }}`
-`${{ github.event.issue.body }}`
-`${{ github.event.comment.body }}`
-`${{ github.head_ref }}` (can be controlled by PR authors)

2. **Use sanitized context instead:**
-`${{ needs.activation.outputs.text }}` (sanitized by gh-aw)

3. **Pass data through environment variables:**
```yaml
env:
UNTRUSTED_VALUE: ${{ github.event.issue.title }}
run: |
echo "Title: $UNTRUSTED_VALUE"
```
4. **Safe context variables (always safe to use):**
- `${{ github.actor }}`
- `${{ github.repository }}`
- `${{ github.run_id }}`
- `${{ github.run_number }}`
- `${{ github.sha }}`

## References

- [GitHub Actions Security Hardening](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions)
- [Understanding the risk of script injections](https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#understanding-the-risk-of-script-injections)
- Issue #3945 - Static Analysis Report (November 14, 2025)
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reference to "Issue #3945" should be updated to reflect the correct issue number mentioned in the PR description. According to the PR metadata, this fixes issue #3984, not #3945.

Update line 108 to:

- Issue githubnext/gh-aw#3984 - [task] Fix template injection risks in workflows
- Discussion githubnext/gh-aw#3945 - Static Analysis Report (November 14, 2025)

This correctly distinguishes between the issue being fixed (#3984) and the discussion that identified it (#3945).

Suggested change
- Issue #3945 - Static Analysis Report (November 14, 2025)
- Issue githubnext/gh-aw#3984 - [task] Fix template injection risks in workflows
- Discussion githubnext/gh-aw#3945 - Static Analysis Report (November 14, 2025)

Copilot uses AI. Check for mistakes.

## Validation

Both workflows compile successfully after the fix:
- ✅ `copilot-session-insights.md` - Template injection fixed
- ✅ `mcp-inspector.md` - No vulnerabilities found

```bash
./gh-aw compile copilot-session-insights --validate
./gh-aw compile mcp-inspector --validate
```
Comment on lines +1 to +119
Copy link

Copilot AI Nov 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Critical Issue: This PR contradicts an existing security review.

The repository contains specs/SECURITY_REVIEW_TEMPLATE_INJECTION.md (dated 2025-11-11) which explicitly analyzed this exact same code pattern and concluded it is a FALSE POSITIVE with NO SECURITY RISK.

From that security review:

Verdict: ✅ FALSE POSITIVE - NO SECURITY RISK

Rationale:

  • The template expansion references a step output that is set by the workflow itself
  • The value is always one of two hardcoded strings: "true" or "false"
  • No user-controlled data flows into this template expansion

Recommended Action:

  1. Either remove this PR entirely (if the previous security review is correct)
  2. Or update the documentation to explain why the previous security review's conclusion was incorrect
  3. Or clarify that this is a preventive best practice change (not a security fix) and update both this PR's documentation and the existing security review accordingly

The new documentation in this PR should reference the existing security review and explain the relationship between the two analyses.

Copilot uses AI. Check for mistakes.
Loading