Skip to content

Harden sandbox.mcp.env key encoding at emission and launch boundaries #53171

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 7 commits into from
Aug 16, 2026
Merged

Harden sandbox.mcp.env key encoding at emission and launch boundaries #53171

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b8c358e
Harden sandbox.mcp.env name encoding at emission and launch boundaries
Copilot Aug 16, 2026
935197e
Use manifest variable constant in launcher error messages
Copilot Aug 16, 2026
893826f
Merge branch 'main' into copilot/review-sandbox-mcp-env-encoding
github-actions[bot] Aug 16, 2026
29b8ce8
Align sanitized MCP gateway env handling
Copilot Aug 16, 2026
9019672
Remove redundant MCP gateway sanitization
Copilot Aug 16, 2026
8ac7256
Reserve full MCP gateway environment namespace
Copilot Aug 16, 2026
13106bb
Merge remote-tracking branch 'origin/main' into copilot/review-sandbo…
Copilot Aug 16, 2026
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
16 changes: 12 additions & 4 deletions actions/setup/js/start_mcp_gateway.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ const { getErrorMessage } = require("./error_helpers.cjs");
let activeGatewayPid = null;
const customGatewayEnvMarker = "__GH_AW_MCP_GATEWAY_CUSTOM_ENV__";
const customGatewayEnvNamePattern = /^[A-Z_][A-Z0-9_]*$/;
const customGatewayEnvNamesVar = "GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES";
const customGatewayEnvTransportPrefix = "GH_AW_MCP_GATEWAY_ENV_";

// ---------------------------------------------------------------------------
// Timing helpers
Expand Down Expand Up @@ -88,18 +90,24 @@ function injectCustomGatewayEnvArgs(args, env = process.env) {

let names;
try {
names = JSON.parse(env.GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES || "[]");
names = JSON.parse(env[customGatewayEnvNamesVar] || "[]");
} catch (err) {
throw new Error(`GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES must be valid JSON: ${getErrorMessage(err)}`, { cause: err });
throw new Error(`${customGatewayEnvNamesVar} must be valid JSON: ${getErrorMessage(err)}`, { cause: err });
}
if (!Array.isArray(names) || !names.every(name => typeof name === "string" && customGatewayEnvNamePattern.test(name))) {
throw new Error("GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES must be an array of valid environment variable names");
throw new Error(`${customGatewayEnvNamesVar} must be an array of valid environment variable names`);
}
if (names.some(name => name === customGatewayEnvNamesVar || name.startsWith(customGatewayEnvTransportPrefix))) {
throw new Error(`${customGatewayEnvNamesVar} must not contain names reserved by the ${customGatewayEnvTransportPrefix} transport namespace`);
}
if (new Set(names).size !== names.length) {
throw new Error(`${customGatewayEnvNamesVar} must not contain duplicate environment variable names`);
}

// Missing indexed transport values intentionally become empty container env vars.
// This preserves deterministic NAME→slot mapping and keeps Docker argument injection
// impossible even if the compiler/runtime metadata ever diverges.
const customArgs = names.flatMap((name, index) => ["-e", `${name}=${env[`GH_AW_MCP_GATEWAY_ENV_${index}`] || ""}`]);
const customArgs = names.flatMap((name, index) => ["-e", `${name}=${env[`${customGatewayEnvTransportPrefix}${index}`] || ""}`]);
return [...args.slice(0, markerIndex), ...customArgs, ...args.slice(markerIndex + 1)];
}

Expand Down
16 changes: 16 additions & 0 deletions actions/setup/js/start_mcp_gateway.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,22 @@ describe("start_mcp_gateway custom environment arguments", () => {
})
).toThrow(/valid environment variable names/);
});

it.each([["GH_AW_MCP_GATEWAY_ENV_0"], ["GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES"]])("rejects the reserved transport name %s", reservedName => {

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] Test uses hardcoded string literals ("GH_AW_MCP_GATEWAY_ENV_0", "GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES") rather than the module constants. If either constant value changes, this test will silently pass while the behaviour diverges.

💡 Suggestion

Export the constants from start_mcp_gateway.cjs and import them in the test:

// start_mcp_gateway.cjs — expose constants
module.exports.customGatewayEnvNamesVar = customGatewayEnvNamesVar;
module.exports.customGatewayEnvTransportPrefix = customGatewayEnvTransportPrefix;

// test file
it.each([[`${customGatewayEnvTransportPrefix}0`], [customGatewayEnvNamesVar]])(
  'rejects the reserved transport name %s',
  reservedName => { /* ... */ }
);

The Go contract test already verifies constant values end-to-end, so this change would make the JS test self-consistent with the same discipline.

@copilot please address this.

expect(() =>
injectCustomGatewayEnvArgs(["run", marker, "gateway-image"], {
GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: JSON.stringify([reservedName]),
})
).toThrow(/reserved/);
});

it("rejects duplicate environment variable names", () => {
expect(() =>
injectCustomGatewayEnvArgs(["run", marker, "gateway-image"], {
GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: '["API_TOKEN","API_TOKEN"]',
})
).toThrow(/duplicate/);
});
});

describe("start_mcp_gateway OTLP if-missing helpers", () => {
Expand Down
47 changes: 46 additions & 1 deletion pkg/workflow/mcp_gateway_env_security_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,5 +157,50 @@ func TestMCPGatewayCustomEnvCommandContract(t *testing.T) {
require.NoError(t, err)
assert.Contains(t, string(launcher), `const customGatewayEnvMarker = "`+mcpGatewayCustomEnvMarker+`"`)
assert.Contains(t, string(launcher), mcpGatewayCustomEnvNamesVar)
assert.Contains(t, string(launcher), "GH_AW_MCP_GATEWAY_ENV_${index}")
assert.Contains(t, string(launcher), `const customGatewayEnvTransportPrefix = "`+mcpGatewayCustomEnvTransportPrefix+`"`)
assert.Contains(t, string(launcher), "${customGatewayEnvTransportPrefix}${index}")
}

func TestMCPGatewayCustomEnvNamesAreFilteredAtEmissionBoundary(t *testing.T) {
gatewayEnv := map[string]string{
"API_TOKEN": "custom-token",
"BAD-NAME": "shell-unsafe",
"lowercase": "shell-unsafe",
"NAME; touch /tmp/pwned": "shell-unsafe",
"$(touch /tmp/pwned)": "shell-unsafe",
mcpGatewayCustomEnvNamesVar: "reserved",
"GH_AW_MCP_GATEWAY_ENV_3": "reserved",
}

assert.Equal(t, []string{"API_TOKEN"}, sanitizedGatewayEnvNames(gatewayEnv))

var yaml strings.Builder
writeMCPGatewayStepEnv(&yaml, nil, nil, gatewayEnv)

output := yaml.String()
assert.Contains(t, output, `GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES: "[\"API_TOKEN\"]"`)
assert.Contains(t, output, `GH_AW_MCP_GATEWAY_ENV_0: "custom-token"`)
assert.NotContains(t, output, "shell-unsafe")
assert.NotContains(t, output, "reserved")
}

func TestMCPGatewayCustomEnvMarkerOmittedWhenAllNamesFiltered(t *testing.T) {
gatewayEnv := map[string]string{"BAD-NAME": "shell-unsafe"}

var yaml strings.Builder
writeMCPGatewayStepEnv(&yaml, nil, nil, gatewayEnv)

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/tdd] TestMCPGatewayCustomEnvMarkerOmittedWhenAllNamesFiltered verifies that the container command string is empty when all names are invalid. However the assertion assert.Empty(t, containerCommand.String()) checks the entire container command output — if appendMCPGatewayCustomAndHTTPEnvFlags ever emits non-custom content before or after the marker check (e.g. HTTP MCP env flags added to the same builder), the test could mask a regression where the marker is still written.

💡 Suggestion

Assert specifically that the marker constant is absent rather than that the whole output is empty:

assert.NotContains(t, containerCommand.String(), mcpGatewayCustomEnvMarker,
    "marker must be omitted when all gateway env names are filtered")

This makes the assertion more intent-revealing and resilient to future additions.

@copilot please address this.

assert.Empty(t, yaml.String())

var containerCommand strings.Builder
appendMCPGatewayCustomAndHTTPEnvFlags(
&containerCommand,
&WorkflowData{},
&MCPGatewayRuntimeConfig{Env: gatewayEnv},
nil,
false,
nil,
nil,
NewCopilotEngine(),
)
assert.Empty(t, containerCommand.String())
}
41 changes: 36 additions & 5 deletions pkg/workflow/mcp_setup_gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,16 +85,20 @@ func generateMCPGatewaySetup(yaml *strings.Builder, tools map[string]any, mcpToo
}

func writeMCPGatewayStepEnv(yaml *strings.Builder, mcpEnvVars map[string]string, safeOutputsInputEnvVars map[string]string, gatewayEnvVars map[string]string) {
if len(mcpEnvVars) == 0 && len(safeOutputsInputEnvVars) == 0 && len(gatewayEnvVars) == 0 {
customEnvVarNames := sanitizedGatewayEnvNames(gatewayEnvVars)
if len(mcpEnvVars) == 0 && len(safeOutputsInputEnvVars) == 0 && len(customEnvVarNames) == 0 {
return
}
yaml.WriteString(" env:\n")
customEnvVarNames := sliceutil.SortedKeys(gatewayEnvVars)
overriddenNames := make(map[string]struct{}, len(customEnvVarNames))

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

L93-96: yagni: builds a map[string]struct{} + setutil.Contains just to test membership in a small slice already in scope. slices.Contains(customEnvVarNames, envVarName) (package already imported), 0 extra lines.

for _, name := range customEnvVarNames {
overriddenNames[name] = struct{}{}
}
// Write MCP env vars first (sorted)
envVarNames := sliceutil.MapKeys(mcpEnvVars)
sort.Strings(envVarNames)
for _, envVarName := range envVarNames {
if _, overridden := gatewayEnvVars[envVarName]; overridden {
if setutil.Contains(overriddenNames, envVarName) {
continue
}
if isReservedMCPGatewayTransportEnvVar(envVarName) {
Expand All @@ -106,7 +110,7 @@ func writeMCPGatewayStepEnv(yaml *strings.Builder, mcpEnvVars map[string]string,
// runner step environment so the docker -e flag can forward them to the container.
inputVarNames := sliceutil.SortedKeys(safeOutputsInputEnvVars)
for _, envVarName := range inputVarNames {
if _, overridden := gatewayEnvVars[envVarName]; overridden {
if setutil.Contains(overriddenNames, envVarName) {
continue
}
if isReservedMCPGatewayTransportEnvVar(envVarName) {
Expand All @@ -128,6 +132,33 @@ func writeMCPGatewayStepEnv(yaml *strings.Builder, mcpEnvVars map[string]string,
}
}

// sanitizedGatewayEnvNames returns the sorted subset of user-supplied gateway
// environment variable names that are safe to emit.
//
// Names are already constrained by the frontmatter schema and by
// validateSandboxConfig, but this is the last boundary before a name is handed
// to the runtime launcher (which turns it into a `docker run -e NAME=value`
// argument). Re-checking here guarantees that a name can never carry shell or
// Docker argument metacharacters, even if an earlier validation path is
// bypassed. Values need no such filtering: they are transported through
// compiler-controlled GH_AW_MCP_GATEWAY_ENV_<n> variables and never appear in
// the generated shell script or in the Docker command string.
func sanitizedGatewayEnvNames(gatewayEnvVars map[string]string) []string {
names := make([]string, 0, len(gatewayEnvVars))
for _, name := range sliceutil.SortedKeys(gatewayEnvVars) {
if isReservedMCPGatewayTransportEnvVar(name) {
mcpSetupGeneratorLog.Printf("Skipping reserved MCP gateway environment variable name: %s", name)
continue
}
if !mcpGatewayEnvNamePattern.MatchString(name) {
mcpSetupGeneratorLog.Printf("Skipping invalid MCP gateway environment variable name: %s", name)
continue
}
names = append(names, name)
}
return names
}

func mcpGatewayCustomEnvTransportName(index int) string {
return fmt.Sprintf("%s%d", mcpGatewayCustomEnvTransportPrefix, index)
}
Expand Down Expand Up @@ -623,7 +654,7 @@ func appendMCPGatewaySafeOutputsInputEnvFlags(containerCmd *strings.Builder, saf
}

func appendMCPGatewayCustomAndHTTPEnvFlags(containerCmd *strings.Builder, workflowData *WorkflowData, gatewayConfig *MCPGatewayRuntimeConfig, mcpEnvVars map[string]string, hasGitHub bool, githubTool map[string]any, tools map[string]any, engine CodingAgentEngine) {
if len(gatewayConfig.Env) > 0 {
if len(sanitizedGatewayEnvNames(gatewayConfig.Env)) > 0 {

@github-actions github-actions Bot Aug 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[/diagnosing-bugs] sanitizedGatewayEnvNames is called twice for every gateway config that has env vars: once here to decide whether to write the marker, and once inside writeMCPGatewayStepEnv (indirectly through appendMCPGatewayCustomAndHTTPEnvFlags → writeMCPGatewayStepEnv). The double-call is cheap now but creates a subtle maintenance trap — a future caller that mutates the map between calls could produce a marker with no corresponding env vars (or vice-versa).

💡 Suggestion

Consider caching the sanitized slice on MCPGatewayRuntimeConfig (or threading it as a parameter) so the guard and the emission path share a single computation:

// In the calling site:
sanitized := sanitizedGatewayEnvNames(gatewayConfig.Env)
if len(sanitized) > 0 {
    containerCmd.WriteString(" " + mcpGatewayCustomEnvMarker)
}
// pass sanitized into writeMCPGatewayStepEnv instead of re-deriving it

This also makes it easier to write a test that asserts marker presence ↔ non-empty sanitized names in one place.

@copilot please address this.

containerCmd.WriteString(" " + mcpGatewayCustomEnvMarker)
}
if len(mcpEnvVars) == 0 {
Expand Down
Loading