Fix template injection risk in copilot-session-insights workflow #4001
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Template injection security fix complete (commit 4cc2542). Fixed:
Verified:
The fix ensures step outputs are passed through environment variables before use in bash scripts, preventing potential code execution via template expansion. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a template injection prevention pattern in the copilot-session-insights workflow by using environment variables instead of direct template expressions in bash scripts. While the implementation follows security best practices, there's a significant documentation issue that needs to be addressed.
Key Changes:
- Modified
.github/workflows/copilot-session-insights.mdto pass step outputs through environment variables rather than using template expressions directly in bash - Recompiled
.github/workflows/copilot-session-insights.lock.ymlwith the security improvements - Added new documentation in
specs/template-injection-prevention.mdexplaining the vulnerability pattern and fix
Critical Finding:
The PR contradicts an existing security review (specs/SECURITY_REVIEW_TEMPLATE_INJECTION.md from 2025-11-11) which explicitly analyzed this exact code and concluded it was a FALSE POSITIVE with NO SECURITY RISK. The documentation needs to either reconcile these conflicting assessments or clarify that this is a preventive best practice rather than a security vulnerability fix.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
.github/workflows/copilot-session-insights.md |
Added environment variable indirection for step output with security comments (preventive pattern, not fixing an active vulnerability) |
.github/workflows/copilot-session-insights.lock.yml |
Compiled workflow correctly reflects source changes with environment variable and updated bash script |
specs/template-injection-prevention.md |
New security documentation with several accuracy issues: contradicts existing security review, overstates vulnerability risk, contains incorrect issue reference (#3945 should be #3984), and could better clarify this is a preventive measure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: My Step | ||
| run: | | ||
| echo "Value: ${{ steps.previous.outputs.value }}" |
There was a problem hiding this comment.
[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-session-insights.md | ||
| **Issue:** Template expression used directly in bash echo statement | ||
| - **Line:** 115 |
There was a problem hiding this comment.
[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.ymlThis helps readers understand the relationship between source and compiled files.
| - **Line:** 115 | |
| - **Line (source):** 115 in copilot-session-insights.md | |
| - **Line (compiled):** ~207 in copilot-session-insights.lock.yml |
|
|
||
| - [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) |
There was a problem hiding this comment.
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).
| - 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) |
| # Template Injection Prevention in Workflows | ||
|
|
||
| ## 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 }}" | ||
| ``` | ||
| 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 | ||
| - **Risk:** While using step output (controlled), the pattern could lead to injection if changed to use untrusted data | ||
| **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) | ||
|
|
||
| ## 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 | ||
| ``` |
There was a problem hiding this comment.
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:
- Either remove this PR entirely (if the previous security review is correct)
- Or update the documentation to explain why the previous security review's conclusion was incorrect
- 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.
| 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 |
There was a problem hiding this comment.
[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 riskThis makes it clear that it's a preventive pattern, not a fix for an existing exploitable vulnerability.
| # 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 |
| 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 |
There was a problem hiding this comment.
[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 bashThis maintains the security awareness while being more accurate about the actual risk level.
| # Security: Use environment variable instead of template expression in bash script | |
| # Security: Use environment variable as best practice to avoid template expressions in bash |
| - **Line:** 115 | ||
| - **Risk:** While using step output (controlled), the pattern could lead to injection if changed to use untrusted data |
There was a problem hiding this comment.
[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.
| - **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) |
Template Injection Security Fix - Complete ✅
Analysis Complete
Issues Found and Fixed
1. copilot-session-insights.md (line 115) - FIXED ✅
Vulnerable code:
Risk: Template expression inside bash script string. While currently using a step output (controlled), this pattern is risky and could lead to injection if changed to use untrusted data.
Fix Applied:
EXTENSION_INSTALLEDenvironment variable at step level$EXTENSION_INSTALLEDinstead of template expression2. mcp-inspector.md - NO ISSUES FOUND ✅
Status: Reviewed and confirmed no template injection vulnerabilities
Documentation Added
Created comprehensive security documentation at
specs/template-injection-prevention.mdincluding:Validation Results
✅ Both workflows compile successfully:
Files Modified
.github/workflows/copilot-session-insights.md- Fixed template injection vulnerability.github/workflows/copilot-session-insights.lock.yml- Recompiled with security fixspecs/template-injection-prevention.md- New security documentation (3.9 KB)Security Impact
Prevents potential code injection attacks via template expansion by ensuring all step outputs are passed through environment variables rather than directly interpolated in bash script contexts.
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.