-
Notifications
You must be signed in to change notification settings - Fork 424
[code-scanning-fix] Fix go/unsafe-quoting: replace Python network-allowed updater with JavaScript in actions/setup/js #40502
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7f4f9e3
8a31bec
5202f10
6195e80
806f167
14304ea
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| // @ts-check | ||
| "use strict"; | ||
|
|
||
| /** | ||
| * update_network_allowed.cjs | ||
| * | ||
| * Updates the AWF config file's network.allowDomains list based on the | ||
| * GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED environment variable. | ||
| * | ||
| * The variable contains a comma-separated list of ecosystem tokens (e.g. "node,python") | ||
| * or raw domain names. Each token is expanded to its known set of domains using the | ||
| * ecosystem map embedded via the GH_AW_ECOSYSTEM_MAP_JSON environment variable. | ||
| * Unknown tokens are treated as raw domain names. | ||
| * | ||
| * Environment variables: | ||
| * RUNNER_TEMP - GitHub Actions runner temp directory | ||
| * GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED - Comma-separated allowed tokens/domains | ||
| * GH_AW_ECOSYSTEM_MAP_JSON - JSON object mapping ecosystem names to domain arrays | ||
| * | ||
| * Exit codes: | ||
| * 0 — Success (including when no tokens are specified) | ||
| * 1 — Fatal error (missing RUNNER_TEMP, unreadable/invalid config file, write failure) | ||
| */ | ||
|
|
||
| const fs = require("fs"); | ||
| const path = require("path"); | ||
|
|
||
| const NETWORK_ALLOWED_ENV_VAR = "GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED"; | ||
| /** @typedef {{allowDomains?: string[]}} AWFNetworkConfig */ | ||
| /** @typedef {Record<string, unknown> & {network?: AWFNetworkConfig | unknown}} AWFConfig */ | ||
|
|
||
| /** | ||
| * @param {any} value | ||
| * @returns {AWFNetworkConfig} | ||
| */ | ||
| function toNetworkConfig(value) { | ||
| return value; | ||
| } | ||
|
|
||
| /** | ||
| * @param {any} value | ||
| * @returns {string[]} | ||
| */ | ||
| function toStringArray(value) { | ||
| return value; | ||
| } | ||
|
|
||
| /** | ||
| * @returns {Promise<void>} | ||
| */ | ||
| async function main() { | ||
| const runnerTemp = process.env.RUNNER_TEMP; | ||
| if (!runnerTemp) { | ||
| process.stderr.write("RUNNER_TEMP is not set\n"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const configPath = path.join(runnerTemp, "gh-aw", "awf-config.json"); | ||
|
|
||
| /** @type {AWFConfig} */ | ||
| let config; | ||
| try { | ||
| config = JSON.parse(fs.readFileSync(configPath, "utf8")); | ||
| } catch (/** @type {unknown} */ err) { | ||
| const errCode = err && typeof err === "object" && "code" in err ? err.code : undefined; | ||
| const errMessage = err instanceof Error ? err.message : String(err); | ||
| if (errCode === "ENOENT") { | ||
| process.stderr.write(`Missing AWF config file at ${configPath}\n`); | ||
| } else if (err instanceof SyntaxError) { | ||
| process.stderr.write(`Invalid AWF config JSON at ${configPath}: ${errMessage}\n`); | ||
| } else { | ||
| process.stderr.write(`Failed to read AWF config file at ${configPath}: ${errMessage}\n`); | ||
| } | ||
| process.exit(1); | ||
| } | ||
|
|
||
| const networkAllowed = process.env[NETWORK_ALLOWED_ENV_VAR] || ""; | ||
| const tokens = networkAllowed | ||
| .split(",") | ||
| .map(t => t.trim()) | ||
| .filter(t => t.length > 0); | ||
|
|
||
| if (tokens.length > 0) { | ||
| const ecosystemMapJSON = process.env.GH_AW_ECOSYSTEM_MAP_JSON; | ||
| if (!ecosystemMapJSON) { | ||
| process.stderr.write("GH_AW_ECOSYSTEM_MAP_JSON is not set\n"); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| /** @type {Record<string, string[]>} */ | ||
| let ecosystemMap; | ||
| try { | ||
| ecosystemMap = JSON.parse(ecosystemMapJSON); | ||
| } catch (/** @type {unknown} */ err) { | ||
| const errMessage = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`Invalid GH_AW_ECOSYSTEM_MAP_JSON: ${errMessage}\n`); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| // Arrays are treated as malformed for this field and reset to an object shape. | ||
| if (!config.network || typeof config.network !== "object" || Array.isArray(config.network)) { | ||
| config.network = {}; | ||
| } | ||
| const network = toNetworkConfig(config.network); | ||
| if (!Array.isArray(network.allowDomains)) { | ||
| network.allowDomains = []; | ||
| } | ||
| const allowDomains = toStringArray(network.allowDomains); | ||
| const seen = new Set(allowDomains); | ||
|
|
||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice use of a Set to dedupe domains efficiently — O(1) membership checks keep this clean. |
||
| for (const token of tokens) { | ||
| const domains = ecosystemMap[token] || [token]; | ||
| for (const domain of domains) { | ||
| if (!seen.has(domain)) { | ||
| allowDomains.push(domain); | ||
| seen.add(domain); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| fs.writeFileSync(configPath, JSON.stringify(config) + "\n"); | ||
| } catch (/** @type {unknown} */ err) { | ||
| const errMessage = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`Failed to write AWF config file at ${configPath}: ${errMessage}\n`); | ||
| process.exit(1); | ||
| } | ||
| } | ||
|
|
||
| module.exports = { main }; | ||
|
|
||
| if (require.main === module) { | ||
| main().catch((/** @type {unknown} */ err) => { | ||
| const errMessage = err instanceof Error ? err.message : String(err); | ||
| process.stderr.write(`Error: ${errMessage}\n`); | ||
| process.exit(1); | ||
| }); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,181 @@ | ||
| // @ts-check | ||
| import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] ESM Node.js treats 💡 Suggested fixRename the test file to Alternatively, rewrite with CJS-style |
||
| import { createRequire } from "module"; | ||
| import { tmpdir } from "os"; | ||
| import { join } from "path"; | ||
| import { writeFileSync, readFileSync, mkdtempSync, rmSync, mkdirSync } from "fs"; | ||
|
|
||
| const req = createRequire(import.meta.url); | ||
| const { main } = req("./update_network_allowed.cjs"); | ||
|
|
||
| const ECOSYSTEM_MAP = { | ||
| npm: ["registry.npmjs.org", "nodejs.org"], | ||
| python: ["pypi.org", "files.pythonhosted.org"], | ||
| }; | ||
|
|
||
| describe("update_network_allowed.cjs", () => { | ||
| /** @type {string} */ | ||
| let tempDir; | ||
| /** @type {string} */ | ||
| let configPath; | ||
| /** @type {Record<string, string | undefined>} */ | ||
| let savedEnv; | ||
|
|
||
| beforeEach(() => { | ||
| tempDir = mkdtempSync(join(tmpdir(), "update-network-allowed-test-")); | ||
| const ghAwDir = join(tempDir, "gh-aw"); | ||
| mkdirSync(ghAwDir); | ||
| configPath = join(ghAwDir, "awf-config.json"); | ||
|
|
||
| savedEnv = { | ||
| RUNNER_TEMP: process.env.RUNNER_TEMP, | ||
| GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED: process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED, | ||
| GH_AW_ECOSYSTEM_MAP_JSON: process.env.GH_AW_ECOSYSTEM_MAP_JSON, | ||
| }; | ||
|
|
||
| process.env.RUNNER_TEMP = tempDir; | ||
| process.env.GH_AW_ECOSYSTEM_MAP_JSON = JSON.stringify(ECOSYSTEM_MAP); | ||
| delete process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| rmSync(tempDir, { recursive: true, force: true }); | ||
|
|
||
| for (const [key, value] of Object.entries(savedEnv)) { | ||
| if (value === undefined) { | ||
| delete process.env[key]; | ||
| } else { | ||
| process.env[key] = value; | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| it("leaves allowDomains unchanged when no tokens are set", async () => { | ||
| const initial = { network: { allowDomains: ["example.com"] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = ""; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(result.network.allowDomains).toEqual(["example.com"]); | ||
| }); | ||
|
|
||
| it("expands an ecosystem token to its domains", async () => { | ||
| const initial = { network: { allowDomains: [] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm"; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(result.network.allowDomains).toContain("registry.npmjs.org"); | ||
| expect(result.network.allowDomains).toContain("nodejs.org"); | ||
| }); | ||
|
|
||
| it("expands multiple ecosystem tokens", async () => { | ||
| const initial = { network: { allowDomains: [] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm,python"; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(result.network.allowDomains).toContain("registry.npmjs.org"); | ||
| expect(result.network.allowDomains).toContain("pypi.org"); | ||
| }); | ||
|
|
||
| it("treats unknown tokens as raw domain names", async () => { | ||
| const initial = { network: { allowDomains: [] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "custom.example.com"; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(result.network.allowDomains).toContain("custom.example.com"); | ||
| }); | ||
|
|
||
| it("does not add duplicate domains", async () => { | ||
| const initial = { network: { allowDomains: ["registry.npmjs.org"] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm"; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| const count = result.network.allowDomains.filter((/** @type {string} */ d) => d === "registry.npmjs.org").length; | ||
| expect(count).toBe(1); | ||
| }); | ||
|
|
||
| it("initialises network.allowDomains when not present", async () => { | ||
| const initial = { apiProxy: { enabled: true } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm"; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(Array.isArray(result.network.allowDomains)).toBe(true); | ||
| expect(result.network.allowDomains).toContain("registry.npmjs.org"); | ||
| }); | ||
|
|
||
| it("trims whitespace around tokens", async () => { | ||
| const initial = { network: { allowDomains: [] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = " npm , python "; | ||
| await main(); | ||
|
|
||
| const result = JSON.parse(readFileSync(configPath, "utf8")); | ||
| expect(result.network.allowDomains).toContain("registry.npmjs.org"); | ||
| expect(result.network.allowDomains).toContain("pypi.org"); | ||
| }); | ||
|
|
||
| it("writes compact JSON with a trailing newline", async () => { | ||
| const initial = { network: { allowDomains: [] } }; | ||
| writeFileSync(configPath, JSON.stringify(initial) + "\n"); | ||
|
|
||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm"; | ||
| await main(); | ||
|
|
||
| const raw = readFileSync(configPath, "utf8"); | ||
| expect(raw.endsWith("\n")).toBe(true); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two error paths reachable in the implementation have no test coverage. 💡 Suggested testsThe test for 1. 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. 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 |
||
| // Compact JSON: no spaces after : or , | ||
| expect(raw).not.toMatch(/: /); | ||
| expect(raw).not.toMatch(/, /); | ||
| }); | ||
|
|
||
| it("exits 1 when RUNNER_TEMP is not set", async () => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] Missing error-path test: 💡 Suggested test to addit("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();
}
}); |
||
| delete process.env.RUNNER_TEMP; | ||
| process.env.GH_AW_WORKFLOW_CALL_NETWORK_ALLOWED = "npm"; | ||
|
|
||
| const exitSpy = vi.spyOn(process, "exit").mockImplementation(_code => { | ||
| throw new Error("process.exit called"); | ||
| }); | ||
| try { | ||
| await expect(main()).rejects.toThrow(); | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The test verifies 💡 Suggested assertionconst 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();
} |
||
| expect(exitSpy).toHaveBeenCalledWith(1); | ||
| } finally { | ||
| exitSpy.mockRestore(); | ||
| } | ||
| }); | ||
|
|
||
| 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(); | ||
| expect(exitSpy).toHaveBeenCalledWith(1); | ||
| } finally { | ||
| exitSpy.mockRestore(); | ||
| } | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -174,43 +174,12 @@ func buildWorkflowCallNetworkAllowedUpdateScript() (string, error) { | |
| return "", fmt.Errorf("marshal network allowed ecosystem map: %w", err) | ||
| } | ||
|
|
||
| return fmt.Sprintf(`python3 - <<'PY' | ||
| import json | ||
| import os | ||
| from pathlib import Path | ||
|
|
||
| runner_temp = os.environ.get("RUNNER_TEMP") | ||
| if not runner_temp: | ||
| raise SystemExit("RUNNER_TEMP is not set") | ||
|
|
||
| config_path = Path(runner_temp) / "gh-aw" / "awf-config.json" | ||
| try: | ||
| config = json.loads(config_path.read_text()) | ||
| except FileNotFoundError as exc: | ||
| raise SystemExit(f"Missing AWF config file at {config_path}") from exc | ||
| except json.JSONDecodeError as exc: | ||
| raise SystemExit(f"Invalid AWF config JSON at {config_path}: {exc}") from exc | ||
| except OSError as exc: | ||
| raise SystemExit(f"Failed to read AWF config file at {config_path}: {exc}") from exc | ||
|
|
||
| network_allowed = os.environ.get(%q, "") | ||
| tokens = [token.strip() for token in network_allowed.split(",") if token.strip()] | ||
|
|
||
| if tokens: | ||
| ecosystem_map = json.loads(r'''%s''') | ||
| allow_domains = config.setdefault("network", {}).setdefault("allowDomains", []) | ||
| seen = set(allow_domains) | ||
| for token in tokens: | ||
| for domain in ecosystem_map.get(token, [token]): | ||
| if domain not in seen: | ||
| allow_domains.append(domain) | ||
| seen.add(domain) | ||
|
|
||
| try: | ||
| config_path.write_text(json.dumps(config, separators=(",", ":"), ensure_ascii=False) + "\n") | ||
| except OSError as exc: | ||
| raise SystemExit(f"Failed to write AWF config file at {config_path}: {exc}") from exc | ||
| PY`, string(WorkflowCallNetworkAllowedEnvVar), string(ecosystemJSON)), nil | ||
| // 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. | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call switching to shellEscapeArg + node — this removes the quote-injection surface from the prior Python heredoc. |
||
| return fmt.Sprintf(`GH_AW_ECOSYSTEM_MAP_JSON=%s node "${RUNNER_TEMP}/gh-aw/actions/update_network_allowed.cjs"`, | ||
| shellEscapeArg(string(ecosystemJSON))), nil | ||
| } | ||
|
|
||
| // BuildAWFCommand builds a complete AWF command with all arguments. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typeof [] === 'object'means the Array guard silently passes and corrupts the config.💡 Suggested fix
The guard on line 74 is:
typeof []returns'object'and![]isfalse, so if the existing config file has"network": ["something"](malformed but a realistic file corruption scenario), the guard passes and the code proceeds to assignallowDomainsas a non-index property directly onto the array object. Nothing is written to stderr, AWF gets a config wherenetwork.allowDomainsis an own-property of an array, and the result is silently wrong.Add an Array check: