Repository navigation
fix: isolate MCP script secrets per tool - #73887
Conversation
Use distinct transport bindings for each tool and variable, build restricted environments for all four handler languages, and keep tool credentials out of agent, gateway, and dependency installer environments. Add compiler/runtime regressions and regenerate affected workflow locks. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality SentinelOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
This comment has been minimized.
This comment has been minimized.
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Ponytail Reviewer completed successfully! Ponytail ReviewerOutcome: The agent completed and reported no action. A no-op output alone does not verify that all required work was completed or that publication was possible. Agent messages are omitted from this comment. The category uses structured run status, not interpretation of agent prose.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (111 new lines in 📄 Draft ADR committed: Inferred decision: each
📋 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. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
Comment MemoryPeek at saved memory (pr-code-quality-reviewer)Note This comment is managed by comment memory.Expand the saved memory block to view or edit the persistent context for this thread.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review
One compatibility issue needs attention: dependency installers now receive a narrowly filtered environment and lose proxy, CA, and private-registry configuration required on enterprise/firewalled runners. I left an inline comment with the impact and suggested remediation. No blocking correctness or security issue was identified in the changed code.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~7 AIC · ⌖ ~1 AIC · ⊞ 1.7K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd. The isolation approach is sound: tool secrets are removed from the agent and gateway env, and each handler gets only its own bindings. Recommending a decision on the allowlist before merge.
📋 Key Themes & Highlights
Key Themes
- Allowlist compatibility:
createToolEnvironmentdrops CA, proxy, registry, andGITHUB_*context variables. A Node probe confirmed this (inline comment onmcp_handler_process.cjs). The change may be intended, but it is a silent behaviour change for tools that rely on those variables. - Test gap: the new isolation test covers overlapping names and sibling exclusion, but does not pin which ambient names are forwarded (inline comment on the test file).
- Docs contradiction:
docs/src/content/docs/specs/mcp-scripts-specification.mdline 458 still says "Shell/Python/Go tools execute in separate containers". The reference doc and the spec's own boundary note say they run on the runner host. Correct line 458. - Cosmetic:
pkg/workflow/mcp_environment.goaround lines 200–206 leaves a blank line inside theIsMCPScriptsEnabledblock after the removedmaps.Copy. Remove it.
Positive Highlights
- ✅ Secret-validation behaviour is unchanged: the base never listed tool secrets in
collectCommonMCPSecrets, so no fail-fast regression. - ✅
applyMCPScriptsSecretEnvhas no remaining callers andGH_AW_GH_TOKENhas no remaining users. - ✅ Transport names are hex-encoded, so
a-b/a_bcollisions are prevented and tested.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · routed: haiku55 xhigh · ~8 AIC · ⌖ ~1 AIC · ⊞ 5.4K
Comment /matt to run again
There was a problem hiding this comment.
🟡 Changes recommended
The environment allowlist removes required repository variables without rebinding them in shared tools, breaking discussion queries and issue/PR queries with since when repo is omitted.
1 open issue
What changed in this PR
This PR addresses MCP-script secret exposure and environment-variable collisions by introducing per-tool bindings and restricting credential propagation across the compiler and runtime.
Changes:
- Generate distinct transport references for each tool/variable pair; stop forwarding tool credentials to agents and gateways.
- Build allowlisted environments for JavaScript, Bash, Python, Go, and dependency installers.
- Add isolation regressions, document migration and security boundaries, and regenerate affected workflow locks.
This pull request requires changes. Two moderate regressions remain:
actions/setup/js/mcp_dependencies_manager.cjs:71: The allowlist drops supported proxy variables, including lowercase equivalents, breaking registry access on proxied runners. Installers cannot recover through toolenv:declarations; handlers also lose proxy configuration unless explicitly rebound. Preserve these settings in the shared runtime allowlist and documentation.actions/setup/js/mcp_handler_process.cjs:22: Filtering outGITHUB_REPOSITORYandGITHUB_REPOSITORY_OWNERbreaks bundled query tools’ default-repository behavior. Explicitly bind the required repository context in the shared tools’env:declarations and regenerate their locks.
| File | Description |
|---|---|
pkg/workflow/testdata/TestWasmGolden_CompileFixtures/smoke-copilot.golden |
Refreshes expected bindings and credential routing. |
pkg/workflow/mcp_scripts_renderer.go |
Collects tool-scoped startup bindings. |
pkg/workflow/mcp_scripts_renderer_test.go |
Tests binding separation and startup placement. |
pkg/workflow/mcp_scripts_http_codex_test.go |
Updates HTTP transport binding expectations. |
pkg/workflow/mcp_scripts_generator.go |
Generates collision-free per-tool transport references. |
pkg/workflow/mcp_scripts_generator_test.go |
Tests tool-specific manifest references. |
pkg/workflow/mcp_environment.go |
Removes tool credentials from gateway environments. |
pkg/workflow/engine_helpers.go |
Removes shared script-secret propagation helpers. |
pkg/workflow/engine_helpers_secrets_test.go |
Verifies server-only credential exclusion. |
pkg/workflow/enclaves_runtime.go |
Removes script bindings from enclave routing. |
pkg/workflow/copilot_engine.go |
Excludes script secrets from forwarding authorization. |
pkg/workflow/copilot_engine_execution.go |
Stops injecting tool credentials into Copilot. |
pkg/workflow/codex_engine.go |
Stops injecting tool credentials into Codex. |
pkg/workflow/codex_engine_test.go |
Tests credential exclusion across engines. |
pkg/workflow/claude_engine.go |
Stops injecting tool credentials into Claude. |
pkg/workflow/behavior_defined_engine.go |
Removes credential injection for declarative engines. |
pkg/workflow/agy_engine_execution.go |
Stops injecting tool credentials into Agy. |
docs/src/content/docs/specs/mcp-scripts-specification.md |
Defines per-tool bindings and environment boundaries. |
docs/src/content/docs/reference/mcp-scripts.md |
Documents allowlisting and recompilation requirements. |
docs/adr/73887-isolate-mcp-script-secrets-per-tool.md |
Records the isolation decision and tradeoffs. |
actions/setup/js/mcp_server_core.cjs |
Passes tool bindings to language handlers. |
actions/setup/js/mcp_scripts_environment.test.cjs |
Adds concurrent, cross-language isolation regressions. |
actions/setup/js/mcp_scripts_config_loader.cjs |
Documents manifest environment references. |
actions/setup/js/mcp_handler_shell.cjs |
Builds tool-local shell environments. |
actions/setup/js/mcp_handler_python.cjs |
Builds tool-local Python environments. |
actions/setup/js/mcp_handler_process.cjs |
Centralizes allowlisting and binding resolution. |
actions/setup/js/mcp_handler_javascript.cjs |
Builds tool-local JavaScript environments. |
actions/setup/js/mcp_handler_go.cjs |
Builds tool-local Go environments. |
actions/setup/js/mcp_dependencies_manager.test.cjs |
Tests installer credential exclusion. |
actions/setup/js/mcp_dependencies_manager.cjs |
Filters dependency-installer environments. |
.github/workflows/smoke-copilot.lock.yml |
Regenerates scoped query-tool bindings. |
.github/workflows/smoke-copilot-arm.lock.yml |
Updates ARM smoke-test credential routing. |
.github/workflows/smoke-copilot-aoai-entra.lock.yml |
Updates Entra smoke-test credential routing. |
.github/workflows/smoke-copilot-aoai-apikey.lock.yml |
Updates API-key smoke-test credential routing. |
.github/workflows/smoke-claude-copilot.lock.yml |
Scopes the probe’s receipt-path binding. |
.github/workflows/github-mcp-structural-analysis.lock.yml |
Scopes label/workflow query credentials. |
.github/workflows/engine-conformance-pydantic-ai.lock.yml |
Scopes conformance state to its tool. |
.github/workflows/engine-conformance-pi.lock.yml |
Scopes conformance state to its tool. |
.github/workflows/engine-conformance-opencode.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-kiro.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-goose.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-gemini.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-deepseek-harness.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-cursor.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-crush.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/engine-conformance-copilot.lock.yml |
Removes conformance state from agent environments. |
.github/workflows/engine-conformance-codex.lock.yml |
Removes conformance state from agent environments. |
.github/workflows/engine-conformance-claude.lock.yml |
Removes conformance state from agent environments. |
.github/workflows/engine-conformance-aider.lock.yml |
Updates conformance-state routing and forwarding exclusions. |
.github/workflows/deep-report.lock.yml |
Scopes reporting query credentials. |
.github/workflows/daily-regulatory.lock.yml |
Regenerates scoped GitHub-query bindings. |
.github/workflows/daily-performance-summary.lock.yml |
Regenerates scoped GitHub-query bindings. |
.github/workflows/daily-mcp-concurrency-analysis.lock.yml |
Scopes the audit run-ID binding. |
.github/workflows/daily-caveman-optimizer.lock.yml |
Scopes the optimizer run-ID binding. |
Review effort: Max (auto)
Keep credential-free proxy, registry and CA settings in a dedicated installer environment; document and test the handler allowlist contract. Explicitly bind repository context for shared tools, refresh generated locks and WASM goldens, and complete the isolation ADR without asserting maintainer approval. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Reply to comment: This is an informational completion notice, so it needs no separate code change. The actionable review feedback was addressed in 88834f0. |
|
Reply to comment: The reported_no_action outcome does not establish validation, so no code change is based on this notice alone. The combined fixes passed the final validation gate, focused environment tests and shared-default/golden checks before being pushed. |
|
Reply to comment: Accepted: the generated ADR was reviewed and completed in 88834f0, and it is now linked in the PR body. Its status remains Proposed because maintainer acceptance has not been recorded. |
|
Reply to comment: The completion notification itself needs no code change. The review's installer compatibility finding was addressed with a dedicated non-credential configuration allowlist and focused regressions in 88834f0. |
|
Reply to comment: This reported_no_action notice does not independently verify implementation or publication, so it needs no separate code change. The combined review fixes were validated and pushed in 88834f0. |
|
Reply to comment: Partially accepted: the ADR's context, decision, alternatives and consequences are complete and linked in the PR body, including migration and trusted-runner limitations. Actual deciders and acceptance remain unrecorded rather than inventing maintainer approval; the ADR stays Proposed. |
|
Reply to comment: The managed metadata needs no independent code change, and its memory fence was left untouched. The referenced installer compatibility theme was addressed in 88834f0 with configuration-preservation and credential-exclusion tests. |
MCP scripts startup merged every tool's environment into a shared map, allowing handlers to read sibling secrets and overwriting bindings when tools declared the same variable name. This addresses HIGH CWE-200 code scanning alert 1058.
Approach
(tool name, variable name)pair a distinct transport reference shared by the generated manifest and startup environment.Compatibility and security boundary
Existing workflows require recompilation. Handlers no longer inherit ambient TLS/proxy/registry settings, PYTHONPATH or GitHub metadata; declare additional configuration in each tool's
env:block. Dependency installers preserve allowlisted non-credential configuration, but do not inherit tool/server credentials or credential-file overrides. Authenticated installer URLs are unsupported; forwarded endpoint paths must also be non-secret because arbitrary path-embedded credentials cannot be detected.Tools still execute trusted code on the runner host: this environment boundary is not a filesystem or network sandbox.
ADR: ADR-73887: Isolate MCP Script Secrets per Tool. The rationale and alternatives are complete; maintainer acceptance and actual deciders remain unrecorded, so the ADR is Proposed.
Validation
make build,make fmt, andmake lint-cjspassed for the isolation change.make agent-report-progress, including lint, type-check, impacted Go/JavaScript tests and workflow drift.make recompilecompiled all 353 workflows and regenerated affected locks, including downstream dispatcher contract hashes.No workflows were dispatched and the alert was not dismissed.