Fix MCP gateway environment injection - #50924
Conversation
Move custom gateway environment values out of generated shell and Docker command strings. Forward them as atomic spawn arguments and validate names defensively. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a7b7c8f-8100-4da5-bf42-21cd531c8b0a
There was a problem hiding this comment.
Pull request overview
Secures MCP gateway environment injection by transporting custom values separately and adding them as atomic Docker arguments.
Changes:
- Validates custom environment variable names.
- Prevents shell and Docker argument injection.
- Adds security regression tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/sandbox_validation.go |
Validates MCP environment names. |
pkg/workflow/sandbox_validation_test.go |
Tests name validation. |
pkg/workflow/mcp_setup_gateway.go |
Adds secure environment transport. |
pkg/workflow/mcp_gateway_env_security_test.go |
Tests injection protections. |
actions/setup/js/start_mcp_gateway.cjs |
Injects atomic Docker arguments. |
actions/setup/js/start_mcp_gateway.test.cjs |
Tests hostile argument handling. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
| transportEnvVarNames := make(map[string]struct{}, len(customEnvVarNames)) | ||
| for i := range customEnvVarNames { | ||
| transportEnvVarNames[mcpGatewayCustomEnvTransportName(i)] = struct{}{} | ||
| } |
There was a problem hiding this comment.
Handled in 0568afe, and reinforced in 4dc11a4 by reserving the entire GH_AW_MCP_GATEWAY_ENV_* namespace during step-env filtering and keeping regression coverage for the collision path.
Add cross-language contract, mapping, malformed metadata, collision, and expression-value regression tests following independent review. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 4a7b7c8f-8100-4da5-bf42-21cd531c8b0a
|
@copilot Please refresh this PR branch if needed, review any outstanding feedback, and run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the outstanding MCP gateway feedback in 0568afe by reserving |
|
/review |
|
/matt |
|
@copilot add integration workflow tests |
|
Hey @pelikhan 👋 — excellent work on this security fix! The MCP gateway environment injection protection is comprehensive and well-tested. What's particularly strong about this PR:
This is ready for review and merge. 🚀
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
|
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 92/100 Excellent
Analysis: This PR fixes CVE GHSA-j77w-g4jj-hp99 with 17 comprehensive security tests covering hostile payload injection (shell, Docker, BASH_ENV), boundary conditions (empty strings, expressions), and cross-layer integration.
Key metrics:
- Design tests: 88% (15/17 tests verify user-facing security contracts)
- Implementation tests: 12% (well below 30% threshold)
- Coding violations: Zero (proper build tags, no mock libraries, descriptive assertions)
- Test inflation ratio: 4.3:1 (justified for security-critical CVE)
No violations detected. All build tags present, no forbidden mocks, error paths comprehensively tested.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting with 3 targeted issues.
📋 Key Themes & Highlights
Key Themes
- Pattern drift risk: The env-name validation regex is duplicated in Go and JS with no cross-language synchronisation test, creating a latent divergence risk.
- Silent collision drop: MCP env vars that collide with reserved transport names are silently dropped; a warning or compile error would improve debuggability.
- Test coverage gap: The newline injection scenario is tested for correct array output but not for array length invariance, leaving room for a future split-on-newline regression.
Positive Highlights
- ✅ The marker-based approach is elegant — hostile values never touch the shell because they bypass the
dockerCommandstring entirely and land as atomicspawn()arguments. - ✅
TestMCPGatewayCustomEnvCommandContractcross-checking the JS file from Go tests is a great pattern for enforcing cross-language contracts. - ✅ Deterministic index mapping (sorted keys →
GH_AW_MCP_GATEWAY_ENV_0..N) makes the transport scheme reproducible and testable. - ✅ Excellent regression coverage: shell metacharacters, backticks,
BASH_ENV, newlines, and Docker argument injection are all exercised. - ✅ Compile-time name validation in
sandbox_validation.gocloses the attack surface before reaching the runtime.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54.7 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot add workflow integration test that ensures the filtering is happening |
|
|
|
|
|
|
|
Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. |
🧪 Test Quality Sentinel ReportScore: 93/100 — ✅ Excellent Executive SummaryPR #50924 demonstrates exceptional test coverage for the MCP gateway environment injection security fix (GHSA-j77w-g4jj-hp99). All 15 new tests are design-level behavioral contracts protecting critical security invariants. 📊 Metrics
Files analyzed:
Test Classification BreakdownSecurity & Behavioral Contracts (12 high-value tests)
Error Handling Tests (2 medium-value tests)
Idempotency & Safety (1 medium-value test)
Compliance & Quality Signals✅ Build Tags: All new Go test files comply with mandatory
✅ Mock Library Policy: No violations detected
✅ Test Inflation (Security-Justified):
Scoring RationaleFailure Conditions: PASS ✅
✅ Recommendation: APPROVE This PR delivers production-quality security tests with comprehensive edge-case coverage. The test-to-code inflation ratio reflects the security-critical nature of environment variable injection prevention, with multiple injection vectors tested independently and cross-component contracts verified.
|
There was a problem hiding this comment.
Security fix review — MCP gateway environment injection (GHSA-j77w-g4jj-hp99)
Overall this is a well-structured fix. The core approach (compiler-controlled transport names, atomic Docker -e NAME=VALUE injection, marker substitution) is correct and the test coverage is thorough.
Two non-blocking issues flagged:
-
Transport name reservation gap (
sandbox_validation.goline 217):GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMESandGH_AW_MCP_GATEWAY_ENV_*satisfy the allowed pattern but are silently dropped instead of triggering a validation error. Should be rejected explicitly. -
Test helper panic risk (
mcp_gateway_env_security_integration_test.goline 213):strings.Indexcan return -1; the prefix slice would panic if the invariant is ever broken. Easily guarded with anidx < 0check.
Neither is a blocker for the security fix itself.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 43.8 AIC · ⊞ 5.3K
Comments that could not be inline-anchored
pkg/workflow/sandbox_validation.go:217
The ^[A-Z_][A-Z0-9_]*$ pattern allows names like GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES and GH_AW_MCP_GATEWAY_ENV_0, which are the transport variable names used internally. A user who sets one of these will have their value silently dropped in writeMCPGatewayStepEnv (filtered by transportEnvVarNames) rather than getting a clear error here.
Consider adding a reservation check:
if name == mcpGatewayCustomEnvNamesVar || strings.HasPrefix(name, "GH_AW_MCP_GATEWAY_ENV_") {
return Ne…
</details>
<details><summary>pkg/workflow/mcp_gateway_env_security_integration_test.go:213</summary>
```go
stepPrefix = line[:strings.Index(line, "- name: ")]strings.Index returns -1 when the substring is not found, which would cause line[:(-1)] to panic. The outer if trimmed != "- name: "+stepName guard ensures - name: is present, but only for the first match line. If the same step name appears twice in the YAML (unlikely but not impossible), the second occurrence could bypass the guard. A defensive check would be:
idx := strings.Index(line, "- name: ")
if idx < 0 { cont…
</details>
Documents the decision to route sandbox.mcp.env values through compiler-controlled transport variables (GH_AW_MCP_GATEWAY_ENV_*) to prevent shell injection and BASH_ENV exploitation (GHSA-j77w-g4jj-hp99). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Design Decision Gate - ADR RequiredThis PR makes significant changes to core business logic (390 new lines in pkg/workflow/) but does not have a linked Architecture Decision Record (ADR). Draft ADR committed: docs/adr/50924-secure-mcp-gateway-env-injection.md - review and complete it before merging. This PR cannot merge until an ADR is linked in the PR body. What to do next:
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness gaps in the transport reservation logic.
📋 Key Themes & Highlights
Key Themes
- Transport namespace not fully reserved (×2):
transportEnvVarNamesinwriteMCPGatewayStepEnvonly reserves the exact N indexed slots in use, so an MCP server env block or a future compiler path could emit aGH_AW_MCP_GATEWAY_ENV_M(M ≥ N) directly into the YAML. The companion gap insandbox_validation.gois that the name-pattern check allows anyGH_AW_MCP_GATEWAY_*name at authoring time. - Silent empty fallback: The JS injector silently converts missing indexed transport vars to empty strings; no warning or assertion makes a compiler/transport mismatch observable.
Positive Highlights
- ✅ Root cause properly addressed — values no longer flow through shell export or Docker command strings
- ✅ Marker-based placeholder is a clean, injection-safe design
- ✅ Atomic Docker
spawn()argument approach is correct - ✅ Excellent regression test coverage: metacharacters, newlines, BASH_ENV, Docker injection, GitHub expressions, override precedence
- ✅
formatYAMLEnvquoting ensures hostile values are inert in YAML before they reach the JS launcher - ✅ Integration test validates end-to-end compiler output, not just unit behaviour
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 68.4 AIC · ⊞ 7.1K
Comment /matt to run again
| transportEnvVarNames := make(map[string]struct{}, len(customEnvVarNames)+1) | ||
| transportEnvVarNames[mcpGatewayCustomEnvNamesVar] = struct{}{} | ||
| for i := range customEnvVarNames { | ||
| transportEnvVarNames[mcpGatewayCustomEnvTransportName(i)] = struct{}{} |
There was a problem hiding this comment.
[/diagnosing-bugs] The existing Copilot comment on line 94 is correct: transportEnvVarNames only reserves the exact GH_AW_MCP_GATEWAY_ENV_${i} slots for the N custom gateway vars. An MCP server's env: block could legitimately expose GH_AW_MCP_GATEWAY_ENV_5 (the name is publicly documented), and if N ≤ 5 that slot would not be reserved and would pass through to the generated YAML, leaking a transport name as a direct env var.
💡 Suggested fix — reserve the whole prefix
Add a prefix-based check alongside the index loop so any name in the GH_AW_MCP_GATEWAY_ENV_* namespace is always blocked:
// Reserve all transport slots, not just the N currently in use
for _, name := range []string{mcpGatewayCustomEnvNamesVar} {
transportEnvVarNames[name] = struct{}{}
}
for name := range mcpEnvVars {
if strings.HasPrefix(name, "GH_AW_MCP_GATEWAY_ENV_") {
transportEnvVarNames[name] = struct{}{}
}
}Alternatively, extend validateSandboxConfig to reject env names starting with GH_AW_MCP_GATEWAY_ outright, which gives a clearer error at authoring time.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 4dc11a4. writeMCPGatewayStepEnv now filters the full GH_AW_MCP_GATEWAY_ENV_* namespace instead of only the currently indexed slots, and the unit/integration tests now cover a colliding GH_AW_MCP_GATEWAY_ENV_5 case.
| name, | ||
| fmt.Sprintf("environment variable names should match %s", mcpGatewayEnvNamePattern), | ||
| "Use uppercase letters, digits, and underscores, starting with a letter or underscore. Example:\n\nsandbox:\n mcp:\n env:\n API_TOKEN: value", | ||
| ) |
There was a problem hiding this comment.
[/diagnosing-bugs] The name-pattern check (^[A-Z_][A-Z0-9_]*$) rejects lowercase names and hyphens, but does not prevent authors from using names in the reserved GH_AW_MCP_GATEWAY_ namespace (e.g. GH_AW_MCP_GATEWAY_ENV_0). Combined with the index-bounded reservation gap in writeMCPGatewayStepEnv, a crafted env name could interfere with the transport mechanism.
💡 Suggested fix
Add an explicit reserved-prefix check to validateSandboxConfig:
if strings.HasPrefix(name, "GH_AW_MCP_GATEWAY_") {
return NewValidationError(
"sandbox.mcp.env."+name,
name,
"environment variable names starting with GH_AW_MCP_GATEWAY_ are reserved for internal transport",
"Choose a different name that does not start with GH_AW_MCP_GATEWAY_.",
)
}This gives authors a clear error at compile time and fully closes the reservation gap.
@copilot please address this.
There was a problem hiding this comment.
Fixed in 4dc11a4. validateSandboxConfig now rejects sandbox.mcp.env names in the reserved GH_AW_MCP_GATEWAY_ namespace, with a targeted validation test for GH_AW_MCP_GATEWAY_ENV_0.
| throw new Error("GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES must be an array of valid environment variable names"); | ||
| } | ||
|
|
||
| const customArgs = names.flatMap((name, index) => ["-e", `${name}=${env[`GH_AW_MCP_GATEWAY_ENV_${index}`] || ""}`]); |
There was a problem hiding this comment.
[/tdd] env[GH_AW_MCP_GATEWAY_ENV_${index}] || "" silently swallows undefined by falling back to the empty string — there's no test covering what happens when GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES and the indexed transport vars are mismatched (e.g. names has 3 entries but only ENV_0 is present). The current test "uses an empty value when transport metadata is missing" covers exactly one missing entry but does not assert that the resulting container env is safe from a Docker perspective (the -e NAME= form sets the var to empty in the container, which may be unexpected).
💡 Suggested improvement
Add a test that verifies the missing-transport case logs or warns, so a misconfigured compiler doesn't silently ship empty secrets:
it("warns when indexed transport var is absent", () => {
const warn = vi.spyOn(console, "warn").mockImplementation(() => {});
injectCustomGatewayEnvArgs(["run", marker, "image"], {
GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: '["SECRET_A","SECRET_B"]',
GH_AW_MCP_GATEWAY_ENV_0: "present",
// ENV_1 intentionally absent
});
expect(warn).toHaveBeenCalledWith(expect.stringContaining("SECRET_B"));
warn.mockRestore();
});Or at minimum document in a comment that an absent transport var is intentionally treated as empty (not an error).
@copilot please address this.
There was a problem hiding this comment.
Addressed in 4dc11a4 by documenting the intentional empty-value fallback in actions/setup/js/start_mcp_gateway.cjs. The runtime contract remains -e NAME= for missing slots, and the existing uses an empty value when transport metadata is missing test continues to cover that behavior.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 966bffc by extending the workflow integration test to collide with both reserved transport env names and assert those extracted |
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the PR-finisher pass in |
|
🎉 This pull request is included in a new release. Release: |
Overview
Fixes a security vulnerability (GHSA-j77w-g4jj-hp99) in the MCP gateway's handling of
sandbox.mcp.envcustom environment variables. Previously, user-supplied values were interpolated directly into the generated GitHub Actions shell script (export NAME=VALUE) and into the serialized Docker command string (-e NAMEflags forwarded from the shell environment). Both paths were exploitable: shell metacharacters (;, backticks, newlines) in a value could inject arbitrary commands into the runner shell before the gateway process started, and a value assigned toBASH_ENVwould be sourced by Bash on the host before the run script even executed.Security fix
Custom env values are now routed through compiler-controlled, indexed transport variables instead of being embedded in shell code or the Docker command string:
sandbox.mcp.envvalue underGH_AW_MCP_GATEWAY_ENV_0,GH_AW_MCP_GATEWAY_ENV_1, ... in the step's YAMLenv:block (safe, non-shell-interpolated) plus a manifest variableGH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES(JSON array of the original names).__GH_AW_MCP_GATEWAY_CUSTOM_ENV__is placed in the serialized Docker command string instead of literal-e NAMEflags.start_mcp_gateway.cjsreplaces the marker with atomic-e NAME=VALUEDocker arguments built directly fromprocess.env, so values never pass through shell parsing or string splitting.^[A-Z_][A-Z0-9_]*$both at compile time (Go,pkg/workflow/sandbox_validation.go) and at runtime (JS,start_mcp_gateway.cjs).GH_AW_MCP_GATEWAY_namespace (includingGH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES) are reserved for internal transport and rejected during validation to prevent collisions/spoofing.Key changes
Runtime launcher —
actions/setup/js/start_mcp_gateway.cjsinjectCustomGatewayEnvArgs(args, env): finds the__GH_AW_MCP_GATEWAY_CUSTOM_ENV__marker in the split Docker args, parsesGH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES, validates names, and replaces the marker with atomic-e NAME=VALUEpairs sourced from indexedGH_AW_MCP_GATEWAY_ENV_Nvariables. Missing indexed values become empty strings (deterministic name→slot mapping).main().Compiler —
pkg/workflow/mcp_setup_gateway.gomcpGatewayCustomEnvNamesVar,mcpGatewayCustomEnvTransportPrefix,mcpGatewayCustomEnvMarker.writeMCPGatewayStepEnvnow takesgatewayEnvVarsand writes them as sorted, indexedGH_AW_MCP_GATEWAY_ENV_Nstep-env entries plus theGH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMESJSON manifest, skipping any other var name that collides with the reserved namespace.export NAME=VALUEemission fromwriteMCPGatewayExports(custom values no longer touch the run script).appendMCPGatewayCustomAndHTTPEnvFlagsnow emits the__GH_AW_MCP_GATEWAY_CUSTOM_ENV__marker instead of literal-e NAMEflags in the Docker command string.isReservedMCPGatewayTransportEnvVarhelper.Validation —
pkg/workflow/sandbox_validation.gomcpGatewayEnvNamePattern(^[A-Z_][A-Z0-9_]*$) and validation invalidateSandboxConfigthat rejectssandbox.mcp.envnames that are malformed or fall in the reservedGH_AW_MCP_GATEWAY_transport namespace, with actionable error messages/examples.Docs —
docs/adr/50924-secure-mcp-gateway-env-injection.mdTesting
actions/setup/js/start_mcp_gateway.test.cjs: new tests forinjectCustomGatewayEnvArgscovering hostile values (Docker flag/shell injection attempts), multi-value index mapping with empty values, missing transport metadata, commands without the marker, malformed JSON, and invalid names.pkg/workflow/mcp_gateway_env_security_test.go(new,!integration): verifies custom env values never appear as literalexportstatements, step-env keys, or in the run script; verifies overriding of generated step env,BASH_ENVand GitHub-expression values are treated as opaque data, reserved-name collision handling, and the marker/transport-variable contract with the JS launcher.pkg/workflow/mcp_gateway_env_security_integration_test.go(new,integrationbuild tag): compiles real workflow markdown and asserts the generated lock file's "Start MCP Gateway" step contains the transport variables (not literal exports) and that a shared-MCP-config name collision with reserved transport names does not leak or duplicate metadata.pkg/workflow/sandbox_validation_test.go: new cases for valid/invalid/reservedsandbox.mcp.envnames.References:> Generated by PR Description Updater for #50924 · auto · 96.7 AIC · ⊞ 6.8K · ◷