[code-scanning-fix] Fix go/unsafe-quoting: replace Python network-allowed updater with JavaScript in actions/setup/js#40502
Conversation
Alert: #600 (go/unsafe-quoting, critical)
CWE-116: Improper Encoding or Escaping of Output
The ecosystemJSON value was interpolated directly into a Python raw
triple-single-quoted string (r'''%s''') inside a bash heredoc. If any
ecosystem name or domain string contained three consecutive single quotes
('''), it would break out of the Python string literal and could allow
code injection.
Fix: base64-encode the ecosystem JSON in Go using
encoding/base64.StdEncoding.EncodeToString, then decode it in Python via
base64.b64decode('%s').decode(). Base64 output contains only [A-Za-z0-9+/=]
characters, making injection through the string delimiter impossible.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…s/setup/js 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. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #40502 does not have the 'implementation' label (has_implementation_label=false) and has only 11 new lines of code in business logic directories, below the 100-line threshold (requires_adr_by_default_volume=false). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
This PR addresses CodeQL alert go/unsafe-quoting by removing the Python heredoc updater that embedded Go-marshalled JSON into a quoted script literal, and replacing it with a Node.js implementation under actions/setup/js/ that reads inputs via environment variables.
Changes:
- Replaced the Python-based
workflow_callnetwork allowlist updater with a Node.js script invoked from the generatedrun:command. - Added a new
update_network_allowed.cjsimplementation plus Vitest unit tests for token expansion/deduplication and config write behavior. - Updated Go tests to assert the new Node-based invocation pattern and env var passing.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/awf_helpers.go | Switches generated updater snippet from Python heredoc to Node invocation with env-var-supplied JSON. |
| pkg/workflow/awf_config_test.go | Updates assertions to match the new Node updater command structure. |
| actions/setup/js/update_network_allowed.cjs | Adds the Node implementation that updates network.allowDomains in the AWF config. |
| actions/setup/js/update_network_allowed.test.cjs | Adds unit tests covering token expansion, deduplication, whitespace trimming, and error path behavior. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| // Using node avoids any Python dependency and eliminates the risk of quote | ||
| // injection: the ecosystem map is single-quoted by shellEscapeArg (JSON only | ||
| // contains double-quoted strings so no single quotes can appear). |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 91/100 — Excellent
📊 Metrics & Test Classification (10 tests analyzed)
Go: 1 ( Verdict
Score breakdown: Behavioral coverage 40/40 · Edge/error cases 21/30 · Duplication 20/20 · Proportional growth 10/10 = 91/100
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — requesting changes on one structural issue that may prevent the JS tests from running.
📋 Key Themes & Highlights
Key Issue
- ESM/CJS mismatch in the test file:
update_network_allowed.test.cjsuses ESMimportstatements andimport.meta.url, but the.cjsextension tells Node.js and most bundlers to treat the file as CommonJS. If vitest does not transform.cjsfiles, the 9 unit tests the PR description relies on will fail to parse entirely. This is the main blocker.
Secondary Issues
- Missing
try/catchforJSON.parse(ecosystemMapJSON): falls through to the generic top-level handler instead of the descriptive per-case pattern used elsewhere JSON.stringify(config, null, 0): non-idiomatic;JSON.stringify(config)is the conventional compact form- Test gap: no test for an invalid (malformed)
GH_AW_ECOSYSTEM_MAP_JSONvalue - Exit code not asserted in the
RUNNER_TEMPerror test
Positive Highlights
- ✅ The Go-side fix is clean and principled: structured data moved entirely to an env var, eliminating the injection surface
- ✅ Single-quoting a JSON value (which contains only double-quoted strings) is a sound and well-explained design choice
- ✅ Good breadth of test scenarios: deduplication, whitespace trimming, unknown-token passthrough, compact output
- ✅ The Go test explicitly asserts absence of the old Python pattern — good regression gate
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 83.5 AIC · ⌖ 8.08 AIC · ⊞ 6.9K
| @@ -0,0 +1,162 @@ | |||
| // @ts-check | |||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | |||
There was a problem hiding this comment.
[/tdd] ESM import statement in a .cjs file — this conflicts with the file extension and may prevent tests from running.
Node.js treats .cjs files as CommonJS and refuses to parse import/export syntax. Vitest may transform the file, but import.meta.url on line 8 is ESM-only and unlikely to survive that transformation correctly.
💡 Suggested fix
Rename the test file to .test.mjs to match its ESM syntax, and update any test runner config to pick it up:
actions/setup/js/update_network_allowed.test.mjs
Alternatively, rewrite with CJS-style require() calls throughout (removing import.meta.url too). Either way, run node --input-type=commonjs < update_network_allowed.test.cjs to verify the file parses cleanly as CJS before merging.
| } | ||
|
|
||
| /** @type {Record<string, string[]>} */ | ||
| const ecosystemMap = JSON.parse(ecosystemMapJSON); |
There was a problem hiding this comment.
[/diagnose] JSON.parse(ecosystemMapJSON) is called without a try/catch — a malformed GH_AW_ECOSYSTEM_MAP_JSON produces a generic top-level Error: <message> rather than the descriptive error messages used for the config file elsewhere in this function.
The top-level .catch will handle it (exit 1), but the inconsistency in error reporting could confuse debugging.
💡 Suggested fix
Wrap the parse in its own try/catch, matching the pattern used for the config file:
let ecosystemMap;
try {
ecosystemMap = JSON.parse(ecosystemMapJSON);
} catch (/** `@type` {unknown} */ err) {
const e = /** `@type` {Error} */ err;
process.stderr.write(`Invalid GH_AW_ECOSYSTEM_MAP_JSON: ${e.message}\n`);
process.exit(1);
}| } | ||
|
|
||
| try { | ||
| fs.writeFileSync(configPath, JSON.stringify(config, null, 0) + "\n"); |
There was a problem hiding this comment.
[/diagnose] JSON.stringify(config, null, 0) — passing 0 as the space argument is non-idiomatic. The intent is compact JSON, which is the default when no third argument is provided.
💡 Suggested fix
// Before
fs.writeFileSync(configPath, JSON.stringify(config, null, 0) + "\n");
// After — conventional compact form
fs.writeFileSync(configPath, JSON.stringify(config) + "\n");Both produce identical output, but the two-argument form is the idiomatic way to request compact JSON and removes the misleading null, 0 which could be read as "use a zero-space indented pretty-print".
| expect(raw).not.toMatch(/, /); | ||
| }); | ||
|
|
||
| it("exits 1 when RUNNER_TEMP is not set", async () => { |
There was a problem hiding this comment.
[/tdd] Missing error-path test: GH_AW_ECOSYSTEM_MAP_JSON is set but contains invalid JSON. The implementation correctly guards against a missing value, but a malformed JSON string will fall through to the unhandled top-level catch — and there is no test covering that path.
💡 Suggested test to add
it("exits 1 when GH_AW_ECOSYSTEM_MAP_JSON is invalid JSON", async () => {
const initial = { network: { allowDomains: [] } };
writeFileSync(configPath, JSON.stringify(initial) + "\n");
process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm";
process.env.GH_AW_ECOSYSTEM_MAP_JSON = "{not valid json";
const exitSpy = vi.spyOn(process, "exit").mockImplementation(_code => {
throw new Error("process.exit called");
});
try {
await expect(main()).rejects.toThrow();
} finally {
exitSpy.mockRestore();
}
});| throw new Error("process.exit called"); | ||
| }); | ||
| try { | ||
| await expect(main()).rejects.toThrow(); |
There was a problem hiding this comment.
[/tdd] The test verifies process.exit is called but does not assert which exit code is passed. A future regression that exits with code 0 on a missing RUNNER_TEMP would not be caught.
💡 Suggested assertion
const exitSpy = vi.spyOn(process, "exit").mockImplementation(_code => {
throw new Error("process.exit called");
});
try {
await expect(main()).rejects.toThrow();
expect(exitSpy).toHaveBeenCalledWith(1); // ← add this
} finally {
exitSpy.mockRestore();
}There was a problem hiding this comment.
REQUEST_CHANGES — 3 blocking issues in the new JS implementation
The Python→Node.js migration correctly eliminates the quote-injection risk. The Go side (escaping, test assertions) is solid. Three issues in the new update_network_allowed.cjs need fixing before this can merge.
Blocking issues summary
| # | File | Severity | Issue |
|---|---|---|---|
| 1 | update_network_allowed.cjs:96 |
medium | Non-atomic writeFileSync can corrupt the AWF firewall config on interrupted write |
| 2 | update_network_allowed.cjs:72 |
medium | Unguarded JSON.parse(ecosystemMapJSON) gives an opaque error message on failure; exit-code table is also incomplete |
| 3 | update_network_allowed.test.cjs |
medium | Two error paths (GH_AW_ECOSYSTEM_MAP_JSON unset / invalid JSON) have no test coverage |
Issue 4 (low) is a minor require(fs) vs. ESM import inconsistency in the test file.
🔎 Code quality review by PR Code Quality Reviewer · 177.5 AIC · ⌖ 9.94 AIC · ⊞ 5.1K
| } | ||
|
|
||
| try { | ||
| fs.writeFileSync(configPath, JSON.stringify(config, null, 0) + "\n"); |
There was a problem hiding this comment.
Non-atomic write to security-critical AWF config may corrupt it on interrupted execution.
💡 Suggested fix
writeFileSync overwrites the file in-place. If the process is killed (OOM, SIGKILL) mid-write, awf-config.json is left truncated or partially written — AWF then fails to parse it and either crashes the step or silently skips firewall enforcement.
Use the POSIX-atomic write-then-rename pattern:
const tmpPath = configPath + '.tmp';
try {
fs.writeFileSync(tmpPath, JSON.stringify(config, null, 0) + '\n');
fs.renameSync(tmpPath, configPath);
} catch (/** `@type` {unknown} */ err) {
try { fs.unlinkSync(tmpPath); } catch (_) {}
const e = /** `@type` {NodeJS.ErrnoException} */ (err);
process.stderr.write(`Failed to write AWF config: ${e.message}\n`);
process.exit(1);
}fs.renameSync on Linux is atomic when both paths share the same filesystem (which they do — both live under RUNNER_TEMP), so the live file is never visible in a partial state.
| } | ||
|
|
||
| /** @type {Record<string, string[]>} */ | ||
| const ecosystemMap = JSON.parse(ecosystemMapJSON); |
There was a problem hiding this comment.
Unguarded JSON.parse for ecosystem map produces an opaque error message on failure.
💡 Suggested fix
If GH_AW_ECOSYSTEM_MAP_JSON ever contains malformed JSON (e.g. a marshaling bug or shell quoting regression), the SyntaxError escapes to the outer main().catch() which only emits Error: Unexpected token X at position Y — no indication of which env var is the culprit. The exit-code table in the file header (lines 20–22) doesn't document this failure mode either.
Wrap the parse with a descriptive catch:
let ecosystemMap;
try {
ecosystemMap = /** `@type` {Record<string, string[]>} */ (JSON.parse(ecosystemMapJSON));
} catch (/** `@type` {unknown} */ err) {
const e = /** `@type` {Error} */ (err);
process.stderr.write(`GH_AW_ECOSYSTEM_MAP_JSON contains invalid JSON: ${e.message}\n`);
process.exit(1);
}Also update the exit-code table to list this case.
| await main(); | ||
|
|
||
| const raw = readFileSync(configPath, "utf8"); | ||
| expect(raw.endsWith("\n")).toBe(true); |
There was a problem hiding this comment.
Two error paths reachable in the implementation have no test coverage.
💡 Suggested tests
The test for RUNNER_TEMP missing (which correctly uses vi.spyOn(process, 'exit')) shows the scaffolding is already in place. These two paths are just missing:
1. GH_AW_ECOSYSTEM_MAP_JSON is unset when tokens are present (lines 65-69 of impl):
it('exits 1 when GH_AW_ECOSYSTEM_MAP_JSON is not set', async () => {
writeFileSync(configPath, JSON.stringify({ network: { allowDomains: [] } }) + '\n');
process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = 'npm';
delete process.env.GH_AW_ECOSYSTEM_MAP_JSON;
const exitSpy = vi.spyOn(process, 'exit').mockImplementation(_code => {
throw new Error('process.exit called');
});
try {
await expect(main()).rejects.toThrow();
} finally {
exitSpy.mockRestore();
}
});2. GH_AW_ECOSYSTEM_MAP_JSON contains invalid JSON (line 72 of impl — currently unguarded):
it('exits 1 when GH_AW_ECOSYSTEM_MAP_JSON contains invalid JSON', async () => {
writeFileSync(configPath, JSON.stringify({ network: { allowDomains: [] } }) + '\n');
process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = 'npm';
process.env.GH_AW_ECOSYSTEM_MAP_JSON = '{broken json';
const exitSpy = vi.spyOn(process, 'exit').mockImplementation(_code => {
throw new Error('process.exit called');
});
try {
await expect(main()).rejects.toThrow();
} finally {
exitSpy.mockRestore();
}
});The second test will fail until the unguarded JSON.parse at line 72 is wrapped (see that comment).
| beforeEach(() => { | ||
| tempDir = mkdtempSync(join(tmpdir(), "update-network-allowed-test-")); | ||
| const ghAwDir = join(tempDir, "gh-aw"); | ||
| require("fs").mkdirSync(ghAwDir); |
There was a problem hiding this comment.
require("fs").mkdirSync mixes CJS require with ESM imports already in scope.
💡 Suggested fix
Line 6 already imports the named fs exports via ESM import. The require("fs") call on this line is inconsistent and surprising to readers. Add mkdirSync to the existing import:
import { writeFileSync, readFileSync, mkdtempSync, rmSync, mkdirSync } from "fs";Then replace line 27 with:
mkdirSync(ghAwDir);|
|
||
| if (!config.network || typeof config.network !== "object") { | ||
| config.network = {}; | ||
| } |
There was a problem hiding this comment.
typeof [] === 'object' means the Array guard silently passes and corrupts the config.
💡 Suggested fix
The guard on line 74 is:
if (!config.network || typeof config.network !== 'object') {
config.network = {};
}typeof [] returns 'object' and ![] is false, so if the existing config file has "network": ["something"] (malformed but a realistic file corruption scenario), the guard passes and the code proceeds to assign allowDomains as a non-index property directly onto the array object. Nothing is written to stderr, AWF gets a config where network.allowDomains is an own-property of an array, and the result is silently wrong.
Add an Array check:
if (!config.network || typeof config.network !== 'object' || Array.isArray(config.network)) {
config.network = {};
}|
@copilot run pr-finisher skill |
|
@copilot please rerun the JS typecheck and impacted tests after the fixes land.
|
|
@copilot review all comments and address the blocking review feedback on the atomic write, JSON parse/error handling, and env-input tests.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
|
|
|
|
Testing safeoutputs connectivity |
|
|
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
Smoke test
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information. |
Agent Container Tool Check
Result: 12/12 tools available ✅ Overall Status: PASS
|
Smoke Test: Gemini - 27885935356
Overall Status: FAIL Run URL: https://github.com/github/gh-aw/actions/runs/27885935356 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.
|
💥 Smoke Test: Claude — Run 27885935353Core #1-12: ✅ all pass Overall: PARTIAL (1 skipped, 0 failed) 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.
💥 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 · 70.7 AIC · ⊞ 8.5K
| } | ||
| const allowDomains = toStringArray(network.allowDomains); | ||
| const seen = new Set(allowDomains); | ||
|
|
There was a problem hiding this comment.
Nice use of a Set to dedupe domains efficiently — O(1) membership checks keep this clean.
| // Pass the ecosystem map JSON via an env var and invoke the JavaScript | ||
| // implementation deployed by actions/setup to ${RUNNER_TEMP}/gh-aw/actions/. | ||
| // Using node avoids any Python dependency and eliminates quote-injection risk: | ||
| // shellEscapeArg safely single-quotes and escapes the JSON payload. |
There was a problem hiding this comment.
Good call switching to shellEscapeArg + node — this removes the quote-injection surface from the prior Python heredoc.
|
Smoke Test: Codex - 27885935363\n\n✅ GitHub merged PRs read\n✅ Serena activate_project + find_symbol\n✅ Playwright navigation/title\n❌ Web-fetch MCP\n✅ Temp file write\n✅ Temp file verification\n✅ Build gh-aw\n✅ Comment memory append\n✅ Cache memory write/verify\n✅ Set issue field\n\nOverall: FAIL 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.
|
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.
|
|
PR: [code-scanning-fix] Fix go/unsafe-quoting: replace Python network-allowed updater with JavaScript in actions/setup/js 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.
|
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.
|
There was a problem hiding this comment.
Smoke review. Wires work. Notes inline.
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 · 627.8 AIC · ⊞ 19.2K
|
Smoke 278859: FAIL;1-2✅;3-15❌;by
|
|
`` @copilot review all comments and fix the failed safe_outputs/network-allowed issue, then rerun the workflow.
|
Summary
Replaces the inline Python heredoc used to update
network.allowDomainsin the AWF config with a dedicated JavaScript CommonJS module (update_network_allowed.cjs). This removes the Python runtime dependency from the setup action, eliminates an unsafe shell-quoting injection vector, and ships the logic with a proper test suite.Changes
New:
actions/setup/js/update_network_allowed.cjs(high impact)GH_AW_WORKFLOW_CALL_NETWORK_ALLOWEDandGH_AW_ECOSYSTEM_MAP_JSONenv vars.network.allowDomainslist with proper deduplication, whitespace trimming, missing-field initialisation, and compact JSON output.New:
actions/setup/js/update_network_allowed.test.cjs(medium impact)Modified:
pkg/workflow/awf_helpers.go(high impact)buildWorkflowCallNetworkAllowedUpdateScriptnow emits a singlenode update_network_allowed.cjsshell invocation instead of a multi-line Python heredoc.Modified:
pkg/workflow/awf_config_test.go(medium impact)TestBuildAWFCommand_WorkflowCallNetworkAllowedUpdaterUsesRunnerTempEnvto assert the newnode/update_network_allowed.cjsinvocation and the presence of theGH_AW_ECOSYSTEM_MAP_JSONenv var.Motivation
The Python heredoc approach had two problems:
Risk
Low. Non-breaking change. The Go-side command structure change is covered by updated unit tests. The new JS module has its own test suite. No public API or schema is affected.
Testing
actions/setup/js/update_network_allowed.test.cjs— vitest unit tests for all updater behaviours and error paths.pkg/workflow/awf_config_test.go— Go unit test asserting the correct shell invocation is generated.