Mask secrets read by the JavaScript runtime#50737
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ 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 #50737 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
Hardens JavaScript runtimes against credential leakage by registering environment-based secrets with GitHub Actions masking.
Changes:
- Adds
readSecretEnv()and standalonecore.setSecret()support. - Routes credential reads through the masking helper.
- Packages and tests the helper across runtime bundles.
Show a summary per file
| File | Description |
|---|---|
actions/setup/setup.sh |
Bundles the secret helper. |
actions/setup/index.js |
Masks the OTLP input token. |
actions/setup/js/action_setup_otlp.cjs |
Masks OTLP credentials. |
actions/setup/js/apply_samples.cjs |
Masks repository tokens. |
actions/setup/js/artifact_client.cjs |
Masks artifact runtime tokens. |
actions/setup/js/assign_to_agent.cjs |
Masks assignment tokens. |
actions/setup/js/build_checkout_manifest.cjs |
Masks checkout tokens. |
actions/setup/js/check_daily_aic_workflow_guardrail.cjs |
Masks guardrail tokens. |
actions/setup/js/check_workflow_recompile_needed.cjs |
Masks maintenance tokens. |
actions/setup/js/codex_harness.cjs |
Masks API keys. |
actions/setup/js/copilot_sdk_driver.cjs |
Masks SDK credentials. |
actions/setup/js/create_agent_session.cjs |
Masks session tokens. |
actions/setup/js/create_issue.cjs |
Masks agent-assignment tokens. |
actions/setup/js/create_pull_request.cjs |
Masks PR and checkout tokens. |
actions/setup/js/extra_empty_commit.cjs |
Masks CI trigger tokens. |
actions/setup/js/git_helpers.cjs |
Masks fallback Git credentials. |
actions/setup/js/mount_mcp_as_cli.cjs |
Masks gateway API keys. |
actions/setup/js/pi_agent_core_driver.cjs |
Masks provider credentials. |
actions/setup/js/pi_provider.cjs |
Masks configured provider tokens. |
actions/setup/js/push_experiment_state.cjs |
Masks push credentials. |
actions/setup/js/push_repo_memory.cjs |
Masks repository push tokens. |
actions/setup/js/push_signed_commits.test.cjs |
Updates the core test mock. |
actions/setup/js/read_secret_env.cjs |
Adds the masking helper. |
actions/setup/js/read_secret_env.test.cjs |
Tests helper behavior. |
actions/setup/js/redact_evals_results.cjs |
Masks evaluation secrets. |
actions/setup/js/redact_secrets.cjs |
Masks redaction inputs. |
actions/setup/js/run_operation_update_upgrade.cjs |
Masks update push tokens. |
actions/setup/js/safe_output_handler_manager.cjs |
Masks project credentials. |
actions/setup/js/safe_outputs_config.cjs |
Masks secret placeholders. |
actions/setup/js/shim.cjs |
Adds standalone masking commands. |
actions/setup/js/shim.test.cjs |
Tests command escaping. |
actions/setup/js/start_mcp_gateway.cjs |
Masks gateway credentials. |
actions/setup/js/test-live-github-api.cjs |
Masks live-test tokens. |
actions/setup/js/update_project.cjs |
Masks project token checks. |
actions/setup/js/validate_lockdown_requirements.cjs |
Masks validation token reads. |
actions/setup/js/validate_secrets.cjs |
Masks validated credentials. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 36/36 changed files
- Comments generated: 2
- Review effort level: Balanced
| } | ||
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match); | ||
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => { | ||
| const envValue = /(?:TOKEN|SECRET|PASSWORD|KEY|CREDENTIAL|AUTH)/.test(envName) ? readSecretEnv(envName) : process.env[envName]; |
There was a problem hiding this comment.
Handled in 6449784f07: ${WRITE_PROJECT_PAT} now matches the shared secret-env-name heuristic (PAT as an env segment), so safe-output placeholders with PAT values route through readSecretEnv() and are covered by a masking assertion.
|
|
||
| /** @param {string} secret */ | ||
| const setSecret = secret => { | ||
| process.stderr.write(`::add-mask::${escapeCommandData(secret)}\n`); |
There was a problem hiding this comment.
Handled in 6449784f07: start_mcp_gateway.cjs now registers likely secret environment values for masking in the parent Actions step before launching the detached gateway; the shim fallback remains for standalone child processes.
|
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 ReportPR: #50737 — "Mask secrets read by the JavaScript runtime" SummaryThis PR adds comprehensive tests for secret masking functionality in JavaScript actions. The test suite covers happy-path and edge-case scenarios with strong integration test coverage using subprocess spawning. Coverage AnalysisNew Test Functions: 6 Test Files & Metrics
Test Classification
Quality Signals✅ Strengths:
Guidelines Check✅ No Go tests (repo uses JavaScript for this feature) Next Steps: No action required. Tests are ready for merge.
|
![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.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on a correctness issue and two quality gaps.
📋 Key Themes & Highlights
Key Themes
- Implicit global reference in
readSecretEnv(correctness): after callingensureCoreSetSecret(), the code calls the barecore.setSecret(value)which relies onglobal.corebeing accessible — if it is cleared between the two lines the function throws. Use the returned object instead. - Whitespace-only secret edge case (test gap): a value like
" "is truthy and gets masked, but callers downstream skip it after trimming; the asymmetry should be covered by a test. - Shim patching duplication (maintainability): the full shim construction block and the partial-
else ifpatch block diverge; future method additions require touching both.
Positive Highlights
- ✅ Excellent breadth of coverage — all known token/key reads across 36 files consistently migrated.
- ✅ Standalone subprocess tests in
shim.test.cjsandread_secret_env.test.cjsare exactly the right technique for verifying::add-mask::output. - ✅
escapeCommandDatacorrectly mirrors@actions/core's percent/CR/LF encoding. - ✅ Graceful no-op when the secret is empty or undefined.
| const value = process.env[name]; | ||
| if (value) { | ||
| const { ensureCoreSetSecret } = require("./shim.cjs"); | ||
| ensureCoreSetSecret(); |
There was a problem hiding this comment.
[/diagnosing-bugs] readSecretEnv calls ensureCoreSetSecret() (which sets global.core.setSecret) and then immediately calls the bare core.setSecret(value) on line 16 — relying on the implicit global. If global.core is ever cleared between those two lines (e.g. in test isolation), this will throw ReferenceError: core is not defined.
💡 Suggested fix
Use the returned core object directly:
const c = ensureCoreSetSecret();
c.setSecret(value);This removes the implicit global dependency and makes the masking path safe under any global mutation.
@copilot please address this.
There was a problem hiding this comment.
Handled in 6449784f07: readSecretEnv() now captures the object returned by ensureCoreSetSecret() and calls coreShim.setSecret(value) instead of relying on the implicit global.
| const __dirname = dirname(fileURLToPath(import.meta.url)); | ||
| const originalCore = global.core; | ||
| const setSecret = vi.fn(); | ||
| global.core = { setSecret }; |
There was a problem hiding this comment.
[/tdd] global.core is replaced at module load time (line 11), before readSecretEnv is required. This means tests that rely on shim fallback behaviour cannot be exercised within the same module — they need a subprocess (spawnSync) as the standalone test at the bottom does. The comment at line 6-12 should document this constraint so future maintainers understand why the mock must be set before the require call.
@copilot please address this.
There was a problem hiding this comment.
Handled in 6449784f07: added a test comment documenting that the core mock must be installed before requiring read_secret_env.cjs, with shim fallback coverage kept in the subprocess test.
| expect(readSecretEnv("TEST_SECRET")).toBeUndefined(); | ||
| expect(setSecret).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[/tdd] Missing edge case: a secret value that is whitespace-only (e.g. " ") will pass the if (value) truthy check in readSecretEnv and get masked, but callers like redact_evals_results.cjs later trim-and-skip it with value.trim() !== "". Consider adding a test for the whitespace-only case so the masking-vs-usage asymmetry is explicit and intentional.
@copilot please address this.
There was a problem hiding this comment.
Handled in 6449784f07: added a whitespace-only secret test that asserts the value is returned and registered for masking.
| @@ -40,7 +61,10 @@ if (!global.core) { | |||
| setOutput: /** @param {string} name @param {unknown} value */ (name, value) => { | |||
There was a problem hiding this comment.
[/codebase-design] The existing if (!global.core) block now constructs a full shim object including setSecret, while the new else if branch only patches in setSecret. This means the two code paths have different shapes — a future reader adding a new core method must remember to add it in both the initial construction block and any patching branches. Consider extracting a makeShimCore() factory so both paths share a single source of truth.
@copilot please address this.
There was a problem hiding this comment.
Handled in 6449784f07: refactored shim core construction through makeShimCore() and applyMissingShimCoreMethods() so full and partial-core paths share one source of truth.
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.
Review: Mask secrets read by the JavaScript runtime
Good, well-scoped PR. The approach — centralizing secret reads through readSecretEnv() and wiring ::add-mask:: via the shim — is clean and the coverage across 36 files appears thorough. Tests for both shim.cjs and read_secret_env.cjs are a welcome addition.
One non-blocking suggestion (see inline): read_secret_env.cjs references bare core inside readSecretEnv() relying on global.core being implicitly in scope. The code works because CJS resolves bare identifiers against global, but ensureCoreSetSecret() already returns global.core — capturing that return value would make the dependency explicit and easier to reason about.> 🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 91.1 AIC · ⊞ 5.3K
| */ | ||
| function readSecretEnv(name) { | ||
| const value = process.env[name]; | ||
| if (value) { |
There was a problem hiding this comment.
The core identifier is referenced here without an explicit declaration. ensureCoreSetSecret() sets global.core and Node.js resolves bare identifiers against global in CJS — so this works — but it is implicit and fragile.
Consider capturing the return value of ensureCoreSetSecret() which already returns global.core:
if (value) {
const { ensureCoreSetSecret } = require('./shim.cjs');
const coreShim = ensureCoreSetSecret();
coreShim.setSecret(value);
}This makes the contract explicit and avoids silent reliance on global resolution. @copilot please address this.
There was a problem hiding this comment.
Handled in 6449784f07: readSecretEnv() now uses the returned core shim directly before calling setSecret().
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
JavaScript runtime components could read authentication values from environment variables without registering them for GitHub Actions log masking.
Changes
Secret access
readSecretEnv()to mask every non-empty value throughcore.setSecret().Runtime compatibility
::add-mask::support toshim.cjsfor standalone Node.js and MCP processes.core.setSecret()behavior ingithub-script.Packaging