Repository navigation
Prevent secret masking from MCP servers - #50746
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 95fbf928-8f60-4b41-8aae-175460945b91
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. 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. No ADR enforcement needed: PR #50746 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ 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 completed test quality analysis. 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. |
There was a problem hiding this comment.
Pull request overview
Separates MCP-safe Git authentication from GitHub Actions secret masking and blocks accidental masking in MCP bundles.
Changes:
- Adds an MCP-safe Git authentication environment builder.
- Moves Actions-only masking into
git_auth_helpers.cjs. - Adds deployment guards against direct
core.setSecretcalls.
Show a summary per file
| File | Description |
|---|---|
.changeset/patch-prevent-mcp-secret-masking.md |
Records the security fix. |
actions/setup/setup.sh |
Updates MCP deployment file lists. |
actions/setup/js/shim.cjs |
Makes shimmed setSecret throw. |
actions/setup/js/shim.test.cjs |
Tests throwing behavior. |
actions/setup/js/setup_sh_file_lists.test.cjs |
Guards MCP bundles against masking calls. |
actions/setup/js/git_auth_env.cjs |
Adds MCP-safe auth environment construction. |
actions/setup/js/git_helpers.cjs |
Uses the MCP-safe builder. |
actions/setup/js/git_helpers.test.cjs |
Verifies masking is omitted. |
actions/setup/js/git_auth_helpers.cjs |
Adds Actions-only masking wrapper. |
actions/setup/js/git_auth_helpers.test.cjs |
Verifies credential masking. |
actions/setup/js/push_repo_memory.cjs |
Uses the Actions-only wrapper. |
actions/setup/js/push_repo_memory.test.cjs |
Updates import assertions. |
actions/setup/js/push_experiment_state.cjs |
Uses the Actions-only wrapper. |
actions/setup/js/check_workflow_recompile_needed.cjs |
Uses the Actions-only wrapper. |
actions/setup/js/check_workflow_recompile_needed.test.cjs |
Extends the mocked core API. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 15/15 changed files
- Comments generated: 1
- Review effort level: Balanced
| const setSecret = /** @param {string} _value */ _value => { | ||
| throw new Error("core.setSecret is unavailable outside the github-script runtime"); |
There was a problem hiding this comment.
setSecret should never be called from a node context.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
The approach is clean and correct. Separating buildGitAuthEnv (MCP-safe, no secret masking) from getGitAuthEnv in git_auth_helpers.cjs (Actions context, calls core.setSecret) is a sound architectural split. The shim now throws on accidental setSecret calls outside the GitHub Actions runtime, the deploy-list test (setup_sh_file_lists.test.cjs) enforces that core.setSecret files are never bundled into MCP/safe-outputs deployments, and the test coverage validates both sides. No blocking issues found.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 43.5 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — commenting with suggestions, no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Naming ambiguity: Both
git_helpers.cjsandgit_auth_helpers.cjsexportgetGitAuthEnvwith opposite masking semantics — the wrong import is a silent security regression. - Optional parameter encodes a mandatory policy choice:
buildGitAuthEnv(token, setSecret?)is callable without masking; making this explicit (two functions or a discriminant option) removes an invisible footgun. - Regex guard coverage:
filesCallingCoreSetSecretmisses bracket-notation calls; worth noting the assumption or extending the pattern.
Positive Highlights
- ✅ Clean separation of MCP-safe env construction from Actions-only masking — the design intent is clear and well-executed.
- ✅ The shim now throws instead of silently no-oping; this converts an invisible failure into an immediate, debuggable error.
- ✅ Automated enforcement test in
setup_sh_file_lists.test.cjsis an excellent regression guard. - ✅ Callers migrated consistently; no stale imports left behind.
- ✅ Changeset file included.
| return buildGitAuthEnv(authToken); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
[/codebase-design] Both git_helpers.cjs and git_auth_helpers.cjs export getGitAuthEnv but with opposite secret-masking semantics — the wrong import either leaks credentials silently or crashes in an MCP process. Consider renaming the git_helpers.cjs version to something like buildGitAuthEnvNoMask so the boundary is unmistakable at the call site.
💡 Suggested rename
// git_helpers.cjs - before
function getGitAuthEnv(token) { ... }
module.exports = { getGitAuthEnv };
// git_helpers.cjs - after
function buildGitAuthEnvNoMask(token) { ... }
module.exports = { buildGitAuthEnvNoMask };The masking version in git_auth_helpers.cjs keeps the getGitAuthEnv name as the canonical Actions entry point.
@copilot please address this.
| * @param {(value: string) => void} [setSecret] | ||
| * @returns {Object} | ||
| */ | ||
| function buildGitAuthEnv(authToken, setSecret) { |
There was a problem hiding this comment.
[/codebase-design] The setSecret parameter is optional ([setSecret]), which keeps the function callable from MCP contexts — but it silently does nothing when omitted, making it easy to call without masking by accident. Since buildGitAuthEnv is always invoked either with masking (Actions path) or without (MCP path), making the intent explicit via two thin wrappers or a mandatory discriminant would remove the ambiguity.
💡 Example: explicit discriminant
// Callers pass false to be explicit about skipping masking:
function buildGitAuthEnv(authToken, { mask = false } = {}) {
const tokenBase64 = ...;
if (mask) setSecret(tokenBase64); // setSecret is the required Actions global here
return { ... };
}Or keep two exported functions: buildGitAuthEnvMasked and buildGitAuthEnvUnmasked.
@copilot please address this.
| } | ||
|
|
||
| function filesCallingCoreSetSecret(files) { | ||
| return files.filter(file => /\bcore\.setSecret\s*\(/.test(readFileSync(resolve(__dirname, file), "utf8"))); |
There was a problem hiding this comment.
[/tdd] The regex only catches core.setSecret( and misses bracket notation (core['setSecret'](...)). A future refactor using bracket notation could silently bypass this MCP safety guard.
Consider adding a second pattern for bracket notation, or at minimum documenting this limitation as a known assumption in the test.
@copilot please address this.
|
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 Report✅ Test Quality Score: 86/100 — Excellent
📊 Metrics (7 tests)
Verdict
This PR introduces strong security-invariant tests. The
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot update setup.sh with the new file |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in c6cf2e2. The Actions-side plain Node callers now use an Actions masking helper instead of the shim’s unavailable |
|
🎉 This pull request is included in a new release. Release: |
Warning
Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.
What happened
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Overview
Prevents MCP server processes from calling
core.setSecret(a GitHub Actions workflow command), which is unsafe when invoked from an MCP server context outside thegithub-scriptruntime. Secret masking logic is separated from MCP-safe git authentication environment construction.Key Changes
actions_secret_masking.cjs: introducesmaskSecret, which callscore.setSecretwhen available or falls back to emitting a raw::add-mask::workflow command — without requiring the real@actions/coremodule.git_auth_env.cjs: introducesbuildGitAuthEnv, which builds git authentication environment variables without callingcore.setSecret, making it safe to use from MCP server processes.git_auth_helpers.cjs/git_helpers.cjs:getGitAuthEnvnow delegates environment construction tobuildGitAuthEnvand appliesmaskSecretfor credential masking only where safe (Actions context), not inside the MCP-safe path.shim.cjs:setSecretnow throws when called outside thegithub-scriptruntime instead of silently emitting an escapedadd-maskworkflow command;escapeCommandDatahelper was removed as no longer needed.action_setup_otlp.cjs,copilot_harness.cjs,dynamic_checkout.cjs) to usemaskSecretinstead of callingcore.setSecretdirectly.setup.sh:generate_mcp_scripts_config.cjsremoved fromMCP_SCRIPTS_FILES;git_auth_env.cjsadded toSAFE_OUTPUTS_FILES.actions_secret_masking.test.cjscoveringmaskSecretfallback and MCP shim placeholder handling; newsetup_sh_file_lists.test.cjsguard (filesCallingCoreSetSecret) ensuring MCP script files never callcore.setSecret; updated tests forshim.cjs,git_auth_helpers.cjs,git_helpers.cjs,push_repo_memory.cjs, andcheck_workflow_recompile_needed.cjs.patch-prevent-mcp-secret-masking.md.Impact
setSecretfrom the MCP shim (outsidegithub-script) now throws instead of no-op/masking, surfacing misuse early.Commits
ecebecd27fix: prevent secret masking from MCP servers3999b0ba8fix: keep Actions secret masking outside MCP shim02ee8c199fix: bind Actions setSecret masking call6d6760709docs: clarify shim setSecret sentinelc6cf2e272test: cover shim setSecret fallback masking> Generated by PR Description Updater for Prevent secret masking from MCP servers #50746 · auto · 43.3 AIC · ⊞ 6.8K · ◷