Keep safe-output token placeholders out of runtime config.json#36353
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the safe-outputs workflow by ensuring secret- and input-derived values are not expanded into the on-disk safeoutputs/config.json at runtime; placeholders remain literal on disk and are substituted only in memory when the JS runtime loads the config, with token-bearing fields redacted from debug/info logs.
Changes:
- Compiler now always writes
safeoutputs/config.jsonusing a single-quoted heredoc and replaces secret/context/input expressions with${ENV_VAR}placeholders, passing required env vars through to the server step. - JS runtime loader resolves
${ENV_VAR}placeholders fromprocess.envbefore consumers use the config, and redacts*token*fields from config debug logs. - Adds regression tests covering placeholder preservation on disk and in-memory resolution for both workflow inputs and secrets.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_outputs_dynamic_allowed_repos_test.go | Adds/updates compiler tests to assert placeholders remain literal on disk and env vars are available for runtime resolution. |
| pkg/workflow/mcp_setup_generator.go | Adjusts safe-outputs setup generation to write quoted heredocs, build placeholder/env mappings, and pass env vars into the server step. |
| actions/setup/js/safe_outputs_config.cjs | Adds in-memory ${VAR} placeholder substitution and redacted debug logging for token keys. |
| actions/setup/js/safe_outputs_config.test.cjs | Adds a focused test ensuring placeholders resolve at runtime and token values are not logged. |
| actions/setup/js/safe_output_handler_manager.cjs | Redacts token-bearing fields when logging handler config loaded from env. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
| safeOutputsConfigEnvKeys, safeOutputsConfigEnvValues := buildSafeOutputsConfigRuntimeEnvVars(safeOutputConfig) | ||
| writeStepEnvVars(yaml, safeOutputsConfigEnvKeys, safeOutputsConfigEnvValues) | ||
| yaml.WriteString(" run: |\n") | ||
| yaml.WriteString(" # Environment variables are set above to prevent template injection\n") | ||
| yaml.WriteString(" export DEBUG\n") |
| function isSensitiveConfigKey(key) { | ||
| return /token/i.test(key); | ||
| } | ||
|
|
||
| function redactSensitiveConfig(value) { | ||
| if (Array.isArray(value)) { | ||
| return value.map(redactSensitiveConfig); | ||
| } | ||
| if (value && typeof value === "object") { | ||
| return Object.fromEntries( | ||
| Object.entries(value).map(([key, nestedValue]) => [ | ||
| key, | ||
| isSensitiveConfigKey(key) ? "***REDACTED***" : redactSensitiveConfig(nestedValue), | ||
| ]), | ||
| ); | ||
| } | ||
| return value; | ||
| } |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (>100 new lines in 📄 Draft ADR committed:
📋 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 MatterADRs create a searchable, permanent record of why the codebase looks the way it does. This change makes a security-relevant architectural choice — keeping secret placeholders off disk and resolving them in memory — that future contributors will want the rationale for. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 90/100 — Excellent
📊 Metrics & Test Classification (3 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §26787220232
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — solid security fix with a few follow-up suggestions.
📋 Key Themes & Highlights
Key Themes
- Duplication:
redactSensitiveConfigis copy-pasted into bothsafe_outputs_config.cjsandsafe_output_handler_manager.cjs. A shared helper module would make future pattern changes (e.g. addingpassword) atomic. - Silent resolution failures:
resolveEnvPlaceholdersuses?? matchas a fallback — if an env var is absent at runtime the placeholder passes through silently, making misconfiguration hard to diagnose. - Test coverage gap: The new redaction in
safe_output_handler_manager.cjs:287has no test; the parallel test added insafe_outputs_config.test.cjsis a good template to follow.
Positive Highlights
- ✅ Root cause properly addressed: always-quoted heredoc + in-memory resolution is the right two-layer fix.
- ✅ Existing assertions were inverted rather than new tests bolted on — the test now encodes the correct security invariant.
- ✅ Clean Go refactor:
buildSafeOutputsConfigRuntimeData/buildSafeOutputsConfigRuntimeEnvVars/writeStepEnvVarsremove meaningful duplication without changing semantics. - ✅ Redaction added to both log paths in JS — good defensive depth.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M
| return /token/i.test(key); | ||
| } | ||
|
|
||
| function redactSensitiveConfig(value) { |
There was a problem hiding this comment.
[/diagnose] redactSensitiveConfig is defined here and again at safe_output_handler_manager.cjs:260 — identical logic in two files.
💡 Suggestion
Extract to a shared helper module (e.g. config_utils.cjs) and require it from both files. A future change to the redaction pattern (e.g. adding password or secret) would then need to be made in only one place, and a single test suite covers both call-sites.
| if (typeof value !== "string") { | ||
| return value; | ||
| } | ||
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match); |
There was a problem hiding this comment.
[/diagnose] The regex [A-Z_][A-Z0-9_]* matches only uppercase env-var names, so a placeholder like ${my_secret} would silently pass through unresolved.
💡 Details
All GitHub Actions secret/input env vars generated by the Go compiler (WRITE_PROJECT_PAT, GH_AW_INPUT_*) are uppercase, so this isn't a current bug. But the restriction is invisible to callers and would cause a silent runtime failure if a lowercase or mixed-case placeholder ever appeared in a config.
Either document the uppercase-only contract with a comment, or broaden the regex to [A-Za-z_][A-Za-z0-9_]* to be defensive:
return value.replace(/\$\{([A-Za-z_][A-Za-z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match);| if (typeof value !== "string") { | ||
| return value; | ||
| } | ||
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match); |
There was a problem hiding this comment.
[/tdd] The ?? match fallback silently leaves an unresolved ${VARNAME} literal in the config when the env var isn't set — a missing secret would be passed as a placeholder string to the safe-output handler rather than causing an early, clear failure.
💡 Suggestion
Consider logging a warning (or throwing) when a placeholder cannot be resolved:
return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => {
if (process.env[envName] === undefined) {
server.warning?.(`Placeholder ${match} has no matching env var — it will be passed literally`);
}
return process.env[envName] ?? match;
});A companion test case for the unresolved-placeholder path would help pin the intended behaviour.
| try { | ||
| const config = JSON.parse(process.env.GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG); | ||
| core.info(`Loaded config from GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ${JSON.stringify(config)}`); | ||
| core.info(`Loaded config from GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG: ${JSON.stringify(redactSensitiveConfig(config))}`); |
There was a problem hiding this comment.
[/tdd] The new redactSensitiveConfig call here has no test coverage — the existing loadConfig tests don't exercise the GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG path.
💡 Suggestion
Add a test in safe_output_handler_manager.test.cjs (or a new file) that:
- Sets
GH_AW_SAFE_OUTPUTS_HANDLER_CONFIGto a JSON object containing agithub-tokenfield. - Calls
loadConfig(). - Asserts the logged output contains
***REDACTED***and does not contain the real token value.
This mirrors the test added in safe_outputs_config.test.cjs and closes the coverage gap for this second log path.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.7M
| } | ||
|
|
||
| function redactSensitiveConfig(value) { | ||
| if (Array.isArray(value)) { |
There was a problem hiding this comment.
Narrow redaction regex leaks non-token sensitive fields into debug logs. Any config key named password, secret, api_key, private_key, or credential will be logged in plaintext to the runner debug log.
💡 Suggested fix
Broaden the pattern in isSensitiveConfigKey (and mirror it in safe_output_handler_manager.cjs):
function isSensitiveConfigKey(key) {
return /token|secret|key|password|credential|private/i.test(key);
}Without this, a handler config like { "github_app_private_key": "...", "client_secret": "..." } will appear verbatim in Actions runner debug logs, potentially captured by log-forwarding or audit infrastructure. Token-only redaction is the most obvious case but far from the only one.
| } | ||
|
|
||
| function redactSensitiveConfig(value) { | ||
| if (Array.isArray(value)) { |
There was a problem hiding this comment.
redactSensitiveConfig is duplicated verbatim across two modules. The two copies will diverge silently when the sensitive-key pattern is updated — one file gets patched, the other doesn't.
💡 Suggested fix
Extract into a shared config_redaction.cjs and require it from both files:
// config_redaction.cjs
function isSensitiveConfigKey(key) {
return /token|secret|key|password|credential|private/i.test(key);
}
function redactSensitiveConfig(value) { ... }
module.exports = { redactSensitiveConfig };// In both safe_outputs_config.cjs and safe_output_handler_manager.cjs
const { redactSensitiveConfig } = require('./config_redaction.cjs');This is already a two-file inconsistency: safe_outputs_config.cjs extracts isSensitiveConfigKey as a named helper while safe_output_handler_manager.cjs inlines the regex. Centralising removes the surface area for divergence.
| return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => process.env[envName] ?? match); | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
Unresolved placeholder silently passes through as a literal string, causing confusing downstream failures. When a referenced env var is not set, process.env[envName] ?? match returns the raw ${VAR} string, which is then used as the token/URL value.
💡 Suggested fix
Emit a warning (or throw for required fields) when a placeholder remains unresolved:
return value.replace(/\$\{([A-Z_][A-Z0-9_]*)\}/g, (match, envName) => {
const resolved = process.env[envName];
if (resolved === undefined) {
// Warn so operators see which variable was missing
server?.warning?.(`Config placeholder ${match} is unset — passing through literally`);
}
return resolved ?? match;
});Without this, a misconfigured workflow (missing secret binding) produces no startup error — the safe-outputs server starts, then fails later when it tries to authenticate with the literal string ${WRITE_PROJECT_PAT} as a token, which is extremely hard to diagnose.
There was a problem hiding this comment.
Smoke test reply: threading check passed here.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 4.1M
|
@copilot review all comments and reviews from copilot and the github-actions actor. Apply the copilot review skill. |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Prefix secret-derived env var names with GH_AW_SECRET_ in buildSafeOutputsConfigRuntimeEnvVars to prevent accidental collision with reserved step env vars (e.g. DEBUG, GH_AW_SAFE_OUTPUTS_CONFIG_PATH). - Extract redactSensitiveConfig/isSensitiveConfigKey into a shared safe_outputs_config_redact.cjs module; both safe_outputs_config.cjs and safe_output_handler_manager.cjs now import from that single source. - Update tests to match the new GH_AW_SECRET_ prefix. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
🎬 THE END — Smoke Claude MISSION: ACCOMPLISHED! The hero saves the day! ✨ |
|
🚀 Smoke Pi MISSION COMPLETE! Pi delivered. 🥧 |
|
🚀 Smoke Antigravity MISSION COMPLETE! Antigravity has spoken. ✨ |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
🚀 Smoke Gemini MISSION COMPLETE! Gemini has spoken. ✨ |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
|
Smoke Test: Gemini - 26789674019
Overall Status: FAIL Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
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. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
🤖 Smoke Test: Claude — Run 26789674010 — PARTIAL
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · opus48 1M
| * @param {string} key | ||
| * @returns {boolean} | ||
| */ | ||
| function isSensitiveConfigKey(key) { |
There was a problem hiding this comment.
Nice scoping of the sensitive-key check to token. Consider also matching secret/password if future config fields carry those. (smoke test #15)
| return value; | ||
| } | ||
|
|
||
| module.exports = { isSensitiveConfigKey, redactSensitiveConfig }; |
There was a problem hiding this comment.
Clean module export — both helpers are reusable and well-documented. (smoke test #15)
|
Smoke test agent was here - leaving a tiny comet trail across the discussion board. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
|
Keep safe-output token placeholders out of runtime config.json Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test review completed: inline comments posted and overall automation checks recorded.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · gpt54 4.1M
| mkdir -p /tmp/gh-aw/safeoutputs | ||
| mkdir -p /tmp/gh-aw/mcp-logs/safeoutputs | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << GH_AW_SAFE_OUTPUTS_CONFIG_c15b09357217befe_EOF | ||
| cat > "${RUNNER_TEMP}/gh-aw/safeoutputs/config.json" << 'GH_AW_SAFE_OUTPUTS_CONFIG_c15b09357217befe_EOF' |
There was a problem hiding this comment.
Smoke test inline note: validating automation flow on .github/workflows/agentic-token-audit.lock.yml line 610.
| GH_AW_SAFE_OUTPUTS_TOOLS_PATH: ${{ runner.temp }}/gh-aw/safeoutputs/tools.json | ||
| GH_AW_SAFE_OUTPUTS_CONFIG_PATH: ${{ runner.temp }}/gh-aw/safeoutputs/config.json | ||
| GH_AW_MCP_LOG_DIR: /tmp/gh-aw/mcp-logs/safeoutputs | ||
| GITHUB_WORKFLOW: ${{ github.workflow }} |
There was a problem hiding this comment.
Smoke test inline note: validating automation flow on .github/workflows/agentic-token-audit.lock.yml line 780.
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
…safe_outputs_config.test.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed — ran Prettier on |
|
|
|
|
Safe-output compilation was expanding
github-tokenexpressions into${RUNNER_TEMP}/gh-aw/safeoutputs/config.json, materializing secret values on disk at runtime. This change keeps secret-bearing fields as placeholders in the on-disk config and resolves them only in memory when the safe-outputs runtime loads the file.Compiler: preserve placeholders on disk
safeoutputs/config.jsonwith a quoted heredoc so${VAR}placeholders are not expanded by the shell.GH_AW_SECRET_prefix (e.g.secrets.WRITE_PROJECT_PAT→GH_AW_SECRET_WRITE_PROJECT_PAT) to prevent accidental collision with reserved step env vars likeDEBUGorGH_AW_SAFE_OUTPUTS_CONFIG_PATH.Runtime: resolve config placeholders in memory
config.json, then substitute${VAR}fromprocess.envbefore handlers consume the config.Logging: redact token-bearing config fields
*token*fields from safe-output config debug logs in the JS loaders to avoid moving exposure from disk into logs.redactSensitiveConfigextracted into a sharedsafe_outputs_config_redact.cjsmodule imported by bothsafe_outputs_config.cjsandsafe_output_handler_manager.cjs, ensuring consistent redaction behavior across both paths.Regression coverage
config.jsonExample of the compiled config change:
Instead of writing the expanded secret value to disk, the runtime now resolves
${GH_AW_SECRET_WRITE_PROJECT_PAT}from the step environment in memory.✨ PR Review Safe Output Test - Run 26789674010
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.comSee Network Configuration for more information.