Uh oh!
There was an error while loading. Please reload this page.
Harden sandbox.mcp.env key encoding at emission and launch boundaries - #53171
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
✅ PR Code Quality Reviewer completed the code quality review.
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (82 additions detected).
|
✅ Ponytail Reviewer completed successfully!
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
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.
|
There was a problem hiding this comment.
Verdict: comment
No blocking issues stood out in the changed lines. This is a narrowly scoped defense-in-depth hardening pass, and the compiler/runtime checks stay aligned.
Highlights
- The compiler now derives marker emission, override suppression, and transport slot emission from the same sanitized name set, which closes the obvious "validation bypass" gap.
- The launcher-side duplicate and reserved-name rejection is a sensible backstop and is covered by targeted tests.
- I did not find a changed-line correctness, crash, or regression issue strong enough to block merge.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 4.75 AIC · ⌖ 6.35 AIC · ⊞ 4.5K
Comment /review to run again
There was a problem hiding this comment.
Pull request overview
Hardens MCP gateway environment-variable handling at compiler emission and runtime launch boundaries.
Changes:
- Filters unsafe or reserved custom environment names during compilation.
- Rejects reserved and duplicate names at runtime.
- Adds Go and JavaScript security coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/mcp_setup_gateway.go | Adds emission-boundary filtering. |
pkg/workflow/mcp_gateway_env_security_test.go | Tests filtering and marker omission. |
actions/setup/js/start_mcp_gateway.cjs | Adds launcher validation. |
actions/setup/js/start_mcp_gateway.test.cjs | Tests reserved and duplicate names. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| if (names.some(name => name === customGatewayEnvNamesVar || name.startsWith(customGatewayEnvTransportPrefix))) { | ||
| throw new Error(`${customGatewayEnvNamesVar} must not contain names reserved by the ${customGatewayEnvTransportPrefix} transport namespace`); |
| func sanitizedGatewayEnvNames(gatewayEnvVars map[string]string) []string { | ||
| names := make([]string, 0, len(gatewayEnvVars)) | ||
| for _, name := range sliceutil.SortedKeys(gatewayEnvVars) { | ||
| if isReservedMCPGatewayTransportEnvVar(name) { |
There was a problem hiding this comment.
Review: Harden sandbox.mcp.env key encoding at emission and launch boundaries
The PR correctly centralises the name-sanitisation logic in sanitizedGatewayEnvNames and applies it at both the YAML emission boundary and the marker-injection boundary. The JS-side validation additions (reserved-name rejection, duplicate rejection) and the new tests are thorough and well-structured.
One bug found
buildAddedGatewayEnvVarSet (Go, line 715) does not use the sanitised name list.
The loop at lines 715–717 still iterates over gatewayConfig.Env (all raw names), so reserved and invalid names remain in the addedEnvVars deduplication set. This inconsistency means an HTTP MCP env var whose name coincidentally matches a user-supplied invalid/reserved gateway env var name would be silently suppressed from docker run -e flags, even though the gateway var itself was already filtered out by sanitizedGatewayEnvNames. Inline comment filed.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 29.2 AIC · ⌖ 7.22 AIC · ⊞ 5.6K
Comments that could not be inline-anchored
pkg/workflow/mcp_setup_gateway.go:716
Bug: buildAddedGatewayEnvVarSet uses raw (unsanitized) gatewayConfig.Env keys
This loop (lines 715–717) iterates over gatewayConfig.Env directly, so reserved names (GH_AW_MCP_GATEWAY_CUSTOM_ENV_NAMES, GH_AW_MCP_GATEWAY_ENV_*) and structurally invalid names are still added to addedEnvVars. Those names are already filtered out by sanitizedGatewayEnvNames before emission, but remain in the "already added" deduplication set used to suppress HTTP MCP env var forwarding.
Consequen…
There was a problem hiding this comment.
Ponytail review (over-engineering only).
One finding — see inline comment on pkg/workflow/mcp_setup_gateway.go.
net: -3 lines possible.
Generated by ✂️ Ponytail Reviewer for #53171 · auto · 34.5 AIC · ⌖ 2.81 AIC · ⊞ 7.2K
Comment /ponytail to run again
| } | ||
| yaml.WriteString(" env:\n") | ||
| customEnvVarNames := sliceutil.SortedKeys(gatewayEnvVars) | ||
| overriddenNames := make(map[string]struct{}, len(customEnvVarNames)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three focused issues; the security logic itself is solid.
📋 Key Themes & Highlights
Key Themes
- Hardcoded constant values in JS test (
start_mcp_gateway.test.cjs:86):it.eachuses string literals instead of the constantscustomGatewayEnvNamesVar/customGatewayEnvTransportPrefix, so a rename will silently leave the test passing with stale expectations. - Double-call to
sanitizedGatewayEnvNames(mcp_setup_gateway.go:657): the marker-guard call and the emission path independently re-derive the sanitized slice from the same map. No correctness bug today, but a future mutation between the two calls could produce a marker↔env-var mismatch that is hard to diagnose. - Overly broad assertion in
TestMCPGatewayCustomEnvMarkerOmittedWhenAllNamesFiltered:assert.Emptyon the whole builder output obscures intent and could mask a regression if HTTP-MCP env flags are added to the same builder.
Positive Highlights
- ✅ Defense-in-depth approach is well-conceived: re-validating names at the final emission boundary before Docker args are constructed is exactly the right place.
- ✅
sanitizedGatewayEnvNamesis clean, well-documented, and properly covers both the pattern check and the reserved-namespace check. - ✅ Go contract test (
TestMCPGatewayCustomEnvCommandContract) is an excellent technique for keeping compiler and launcher constants in sync. - ✅ Injection-payload names in
TestMCPGatewayCustomEnvNamesAreFilteredAtEmissionBoundaryprovide strong regression protection.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 54 AIC · ⌖ 8.84 AIC · ⊞ 7.7K
Comment /matt to run again
| ).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 => { |
There was a problem hiding this comment.
[/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 constantsmodule.exports.customGatewayEnvNamesVar=customGatewayEnvNamesVar;module.exports.customGatewayEnvTransportPrefix=customGatewayEnvTransportPrefix;// test fileit.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.
| 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 { |
There was a problem hiding this comment.
[/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)
iflen(sanitized) >0 {
containerCmd.WriteString(" "+mcpGatewayCustomEnvMarker)
}
// pass sanitized into writeMCPGatewayStepEnv instead of re-deriving itThis also makes it easier to write a test that asserts marker presence ↔ non-empty sanitized names in one place.
@copilot please address this.
| gatewayEnv := map[string]string{"BAD-NAME": "shell-unsafe"} | ||
| var yaml strings.Builder | ||
| writeMCPGatewayStepEnv(&yaml, nil, nil, gatewayEnv) |
There was a problem hiding this comment.
[/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.
gh-aw-bot
commented
Aug 16, 2026
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Failed checks to fix: Please refresh the branch state if needed, rerun the relevant tests, and run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in 29b8ce8 and 5eb4495: custom-name filtering is computed once for compiler emission and marker generation, filtered keys no longer suppress HTTP MCP forwarding, launcher tests use exported transport constants, and marker/step-env omission coverage is focused. Local validation passed; the listed CI jobs failed before tests on a missing cache entry. |
gh-aw-bot
commented
Aug 16, 2026
@copilot thanks for the follow-up. Please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Resolved the remaining full-namespace gap in |
…x-mcp-env-encoding Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
gh-aw-bot
commented
Aug 16, 2026
@copilot thanks for the follow-up. This PR looks close. Please run the No new failed checks were listed in the compact candidate data for this run.
|
Uh oh!
There was an error while loading. Please reload this page.
🎉 This pull request is included in a new release. Release: |
Review of the
sandbox.mcp.envencoding path found that values are already injection-safe by construction (they travel through compiler-controlledGH_AW_MCP_GATEWAY_ENV_<n>step env vars and are reassembled into argv-e NAME=VALUEentries by a shell-lessspawn), but keys relied entirely on upstream schema/frontmatter validation — nothing re-checked a name at the point where it becomes a Docker argument.Encoding path reviewed
patternProperties^[A-Z_][A-Z0-9_]*$, string values,additionalProperties: falsevalidateSandboxConfig: rejects the reservedGH_AW_MCP_GATEWAY_*namespace and non-conforming nameswriteMCPGatewayStepEnv: values written only under indexed transport names, YAML-escaped via%q; user names never become runner env var names__GH_AW_MCP_GATEWAY_CUSTOM_ENV__marker — no key or value is interpolated into therun:block or the serialized commandstart_mcp_gateway.cjs: rebuilds atomic-e NAME=VALUEargv entries,spawnwithout a shellChanges
pkg/workflow/mcp_setup_gateway.go): newsanitizedGatewayEnvNamesfilters custom env names against^[A-Z_][A-Z0-9_]*$and the reserved transport namespace. The names manifest, indexed transport values, override suppression, and the Docker marker now all derive from this single sanitized set, so a name carrying shell/Docker metacharacters cannot be emitted even if an earlier validation path is bypassed. Theenv:block is omitted when nothing valid remains.actions/setup/js/start_mcp_gateway.cjs):injectCustomGatewayEnvArgsadditionally rejects reserved-namespace names and duplicates; the manifest variable and transport prefix are now shared constants used in lookups and error messages.