fix: move sink-visibility to env var to prevent zizmor template-injection - #48589
Conversation
…tion
- Change sinkVisibilityRuntimeExpr from a raw ${{ }} expression in the
run: heredoc to ${GH_AW_SINK_VISIBILITY} shell variable reference
- Add GH_AW_SINK_VISIBILITY env var to Start MCP Gateway step's env:
block, set from ${{ toJSON(steps.determine-automatic-lockdown.outputs.visibility) }}
- Add -e GH_AW_SINK_VISIBILITY to MCP gateway Docker container flags
- Add GH_AW_SINK_VISIBILITY to buildAddedGatewayEnvVarSet to prevent
duplicate emission
- Add --persona auditor to internal zizmor invocation
- Update test to check for GH_AW_SINK_VISIBILITY instead of old expression
- Regenerate 263 lock files via make recompile
Fixes #48565
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The initial fix only added GH_AW_SINK_VISIBILITY to the env block when 'github' was present in mcpTools. For gh-proxy mode workflows, GitHub is excluded from mcpTools (because it's handled via CLI proxy instead of MCP server), but determine-automatic-lockdown still runs and sink-visibility is still emitted in the MCP gateway config JSON. Move the GH_AW_SINK_VISIBILITY env var assignment to check tools["github"] directly (same condition as generateGitHubMCPLockdownDetectionStep), so all workflows with the GitHub tool get the env var regardless of mode. Also update golden test files to reflect the new env var and docker flag. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. |
|
❌ Test Quality Sentinel failed during test quality analysis. |
|
|
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
There was a problem hiding this comment.
Pull request overview
Moves MCP sink visibility through an environment variable to avoid zizmor template-injection findings.
Changes:
- Routes repository visibility through
GH_AW_SINK_VISIBILITY. - Passes the variable into the MCP gateway container.
- Enables zizmor’s auditor persona and regenerates tests/workflows.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_environment.go |
Adds sink-visibility environment value. |
pkg/workflow/mcp_renderer_guard.go |
Uses shell-variable indirection. |
pkg/workflow/mcp_setup_gateway.go |
Forwards the variable into Docker. |
pkg/workflow/non_github_mcp_guard_policy_test.go |
Updates guard-policy assertions. |
pkg/cli/zizmor.go |
Enables auditor persona. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/with-imports.golden |
Updates golden output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Updates golden output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/playwright-cli-mode.golden |
Updates golden output. |
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/basic-copilot.golden |
Updates golden output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/gemini.golden |
Updates Gemini output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/copilot.golden |
Updates Copilot output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/codex.golden |
Updates Codex output. |
pkg/workflow/testdata/TestWasmGolden_AllEngines/claude.golden |
Updates Claude output. |
.github/workflows/unbloat-docs.lock.yml |
Regenerates workflow lock. |
.github/workflows/tidy.lock.yml |
Regenerates workflow lock. |
.github/workflows/step-name-alignment.lock.yml |
Regenerates workflow lock. |
.github/workflows/spec-enforcer.lock.yml |
Regenerates workflow lock. |
.github/workflows/smoke-pi.lock.yml |
Regenerates workflow lock. |
.github/workflows/smoke-github-claude.lock.yml |
Regenerates workflow lock. |
.github/workflows/smoke-copilot-mai.lock.yml |
Regenerates workflow lock. |
.github/workflows/smoke-copilot-auto.lock.yml |
Regenerates workflow lock. |
.github/workflows/smoke-claude-on-copilot.lock.yml |
Regenerates workflow lock. |
.github/workflows/skillet.lock.yml |
Regenerates workflow lock. |
.github/workflows/schema-consistency-checker.lock.yml |
Regenerates workflow lock. |
.github/workflows/refiner.lock.yml |
Regenerates workflow lock. |
.github/workflows/pr-description-caveman.lock.yml |
Regenerates workflow lock. |
.github/workflows/plan.lock.yml |
Regenerates workflow lock. |
.github/workflows/objective-impact-report.lock.yml |
Regenerates workflow lock. |
.github/workflows/lint-monster.lock.yml |
Regenerates workflow lock. |
.github/workflows/jsweep.lock.yml |
Regenerates workflow lock. |
.github/workflows/instructions-janitor.lock.yml |
Regenerates workflow lock. |
.github/workflows/hourly-ci-cleaner.lock.yml |
Regenerates workflow lock. |
.github/workflows/go-logger.lock.yml |
Regenerates workflow lock. |
.github/workflows/eslint-refiner.lock.yml |
Regenerates workflow lock. |
.github/workflows/eslint-monster.lock.yml |
Regenerates workflow lock. |
.github/workflows/eslint-miner.lock.yml |
Regenerates workflow lock. |
.github/workflows/designer-drift-audit.lock.yml |
Regenerates workflow lock. |
.github/workflows/design-decision-gate.lock.yml |
Regenerates workflow lock. |
.github/workflows/delight.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-windows-terminal-integration-builder.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-team-status.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-security-red-team.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-safeoutputs-git-simulator.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-github-docs-seo-optimizer.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-elixir-credo-snippet-audit.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-doc-updater.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-doc-healer.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-code-metrics.lock.yml |
Regenerates workflow lock. |
.github/workflows/daily-choice-test.lock.yml |
Regenerates workflow lock. |
.github/workflows/craft.lock.yml |
Regenerates workflow lock. |
.github/workflows/copilot-session-insights.lock.yml |
Regenerates workflow lock. |
.github/workflows/copilot-pr-merged-report.lock.yml |
Regenerates workflow lock. |
.github/workflows/copilot-agent-analysis.lock.yml |
Regenerates workflow lock. |
.github/workflows/ci-doctor.lock.yml |
Regenerates workflow lock. |
.github/workflows/avenger.lock.yml |
Regenerates workflow lock. |
.github/workflows/approach-validator.lock.yml |
Regenerates workflow lock. |
.github/workflows/agentic-token-optimizer.lock.yml |
Regenerates workflow lock. |
.github/workflows/ab-testing-advisor.lock.yml |
Regenerates workflow lock. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 186/276 changed files
- Comments generated: 1
- Review effort level: Medium
|
@copilot there are encoding issues with the json https://github.com/github/gh-aw/actions/runs/30359620031/job/90276252896?pr=48589 |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
Fixed in commit The issue was that The fix removes |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with two targeted suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Test coverage gap: the
tools["github"] = falseand gh-proxy mode branches ofGH_AW_SINK_VISIBILITYemission are not directly unit-tested inmcp_environment_test.go; the gh-proxy regression from commitde750a8could silently recur without them - Comment clarity:
sinkVisibilityRuntimeExprcomment explains the zizmor motivation but doesn't clarify why theguardExprSentinelis no longer needed, leaving two parallel patterns unexplained side-by-side
Positive Highlights
- ✅ Clean root-cause fix: expression moved to
env:(evaluated outside shell), not just suppressed - ✅
toJSON()removal in the follow-up commit correctly avoids double-encoding; the comment inmcp_environment.goexplains the reasoning clearly - ✅ Condition checks
tools["github"]directly (notmcpTools) to cover gh-proxy mode — good domain knowledge applied - ✅ Dedup set in
buildAddedGatewayEnvVarSetupdated atomically with the docker flags — no partial state - ✅ Lock files regenerated and golden tests updated in the same PR
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 108.6 AIC · ⌖ 5.02 AIC · ⊞ 6.7K
Comment /matt to run again
|
✅ Smoke OTEL completed successfully! |
Agent Container Tool Check
Result: 12/12 tools available ✅ PASS
|
Smoke Test Results
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
|
Smoke test summary: workflow reliability, security/dependency work, and docs updates dominated the last 24h.
|
|
Commit pushed:
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
🤖 Smoke Test Run 30366142424 — PR Review ResultsCore Tests #1-12: ✅✅✅✅✅✅✅✅✅✅✅✅
Overall: PARTIAL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! Run 30366142424 complete.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · sonnet46 · 87.4 AIC · ⌖ 19.3 AIC · ⊞ 6.1K
Comment /smoke-claude to run again
| // the GitHub tool is present in tools (regardless of mode — gh-proxy workflows also get this | ||
| // step even though github is excluded from mcpTools). | ||
| rawGitHubValue, hasGitHubInTools := tools["github"] | ||
| if hasGitHubInTools && rawGitHubValue != false { |
There was a problem hiding this comment.
👍 The condition hasGitHubInTools && rawGitHubValue != false correctly handles both the presence and truthiness of the github key in tools. This ensures GH_AW_SINK_VISIBILITY is set for gh-proxy mode workflows too — solid guard! Consider adding a test case for the rawGitHubValue == false branch to ensure full coverage.
| dockerArgs := []string{ | ||
| "run", | ||
| "--rm", | ||
| "-v", gitRoot + ":/workdir", | ||
| "-w", "/workdir", | ||
| "ghcr.io/zizmorcore/zizmor:latest", | ||
| "--persona", "auditor", |
There was a problem hiding this comment.
✅ Adding --persona auditor is a great improvement — this ensures the internal zizmor scan catches template-injection findings like the one this PR fixes. This makes the CI pipeline self-enforcing for future similar regressions.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
…d policy iteration, and case string handler Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
Rebased in commit
|
|
/smoke-copilot |
|
📰 BREAKING: Smoke Copilot is now investigating this issue comment. Sources say the story is developing... |
|
/smoke-claude |
|
💥 WHOOSH! Smoke Claude springs into action on this issue comment! [Panel 1 begins...] |
Compiling a workflow with GitHub MCP + safe-outputs emitted
${{ toJSON(steps.determine-automatic-lockdown.outputs.visibility) }}directly inside therun:heredoc for thesink-visibilityguard policy field. zizmor--persona=auditorflags any${{ }}expression in a heredoc body as template injection.Changes
Eliminate
${{ }}from therun:heredocsinkVisibilityRuntimeExprchanged from a sentinel-based raw expression to${GH_AW_SINK_VISIBILITY}— a plain shell variable reference expanded by the unquoted heredocMove the expression to
env:collectMCPEnvironmentVariablesnow emitsGH_AW_SINK_VISIBILITY: ${{ toJSON(steps.determine-automatic-lockdown.outputs.visibility) }}on the Start MCP Gateway step'senv:block — safe because GitHub Actions evaluatesenv:expressions outside the shell execution contexttools["github"]directly (notmcpTools) somode: gh-proxyworkflows — where GitHub is excluded frommcpToolsbutdetermine-automatic-lockdownstill runs — also get the env varPass env var into the gateway container
-e GH_AW_SINK_VISIBILITYto thedocker runflags and dedup set inmcp_setup_gateway.goHarden internal zizmor scan
--persona auditorto the zizmor Docker invocation so internal CI catches this class of finding going forwardBefore / After
Before (in the
run:heredoc — flagged by zizmor):After (shell variable in heredoc, value set via
env:):263 lock files regenerated; 8 golden test files updated to reflect the new env var and docker flag.
✨ PR Review Safe Output Test - Run 30366142424
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.