-
Notifications
You must be signed in to change notification settings - Fork 507
Write MCP CLI wrapper scripts with owner-only permissions (0o700) instead of 0o755 #55587
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
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 |
|---|---|---|
|
|
@@ -609,7 +609,9 @@ async function main() { | |
| } | ||
| core.info(` Found ${tools.length} tool(s)`); | ||
|
|
||
| // Cache the tool list | ||
| // Cache the tool list. This file only contains tool name/description/schema | ||
| // metadata returned by the gateway; it does not contain the API key or any | ||
| // other secret, so world-readable permissions (0o644) are acceptable here. | ||
| try { | ||
| fs.writeFileSync(toolsFile, JSON.stringify(tools, null, 2), { mode: 0o644 }); | ||
| } catch (err) { | ||
|
|
@@ -618,13 +620,31 @@ async function main() { | |
|
|
||
| // Write the CLI wrapper script using the container-accessible URL | ||
| const scriptPath = path.join(CLI_BIN_DIR, name); | ||
| let scriptFd; | ||
| try { | ||
| fs.writeFileSync(scriptPath, generateCLIWrapperScript(name, containerUrl, toolsFile, apiKey, bridgeScript), { mode: 0o755 }); | ||
| // Owner-only permissions: the wrapper script embeds the plaintext gateway API key, | ||
| // so it must not be world- or group-readable (matches chmod 600 used elsewhere for | ||
| // this same credential, e.g. convert_gateway_config_copilot.sh). | ||
| // Note: writeFileSync(mode) only applies when creating a new file; for existing files, | ||
| // force mode with fchmodSync so prior permissive modes (e.g., 0o755) are corrected. | ||
| scriptFd = fs.openSync(scriptPath, "w", 0o700); | ||
| fs.fchmodSync(scriptFd, 0o700); | ||
|
Contributor
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. Security note: The dual openSync + fchmodSync with mode 0o700 is solid defense-in-depth, ensuring owner-only permissions even when the file already existed with permissive modes. This correctly matches chmod 600 semantics used elsewhere for the same credential. Nice fix! |
||
| fs.writeFileSync(scriptFd, generateCLIWrapperScript(name, containerUrl, toolsFile, apiKey, bridgeScript), "utf8"); | ||
| fs.closeSync(scriptFd); | ||
| scriptFd = undefined; | ||
| mountedServers.push(name); | ||
| mountedServerTools.push({ name, tools }); | ||
| core.info(` ✓ Mounted as: ${scriptPath}`); | ||
| } catch (err) { | ||
| core.warning(` Failed to write CLI wrapper for ${name}: ${getErrorMessage(err)}`); | ||
| } finally { | ||
| if (scriptFd !== undefined) { | ||
| try { | ||
| fs.closeSync(scriptFd); | ||
| } catch { | ||
| // Ignore close errors in cleanup path; main error already reported above. | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| // @ts-check | ||
| import { describe, expect, it } from "vitest"; | ||
| import { afterEach, describe, expect, it, vi } from "vitest"; | ||
|
Contributor
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 — importing |
||
| import fs from "fs"; | ||
| import http from "http"; | ||
| import os from "os"; | ||
| import path from "path"; | ||
|
|
||
|
|
@@ -328,3 +329,93 @@ describe("mount_mcp_as_cli.cjs", () => { | |
| expect(warnings[1]).toContain("stopping empty tools/list retries"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("mount_mcp_as_cli.cjs main() file permissions", () => { | ||
|
Contributor
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. actions/setup/js/mount_mcp_as_cli.test.cjs:L333-420: shrink: end-to-end fake server + temp-dir harness for a single permission assertion. A focused unit test around the file-write call is enough.
Contributor
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. The new describe block for file permissions testing is well-structured. The afterEach cleanup that restores write permissions on the bin directory (0o555 -> original) before temp dir removal is a thoughtful detail that prevents cleanup failures on restrictive file systems. |
||
| /** @type {string | undefined} */ | ||
| let tempDir; | ||
| /** @type {http.Server | undefined} */ | ||
| let server; | ||
|
|
||
| afterEach(async () => { | ||
| if (server) { | ||
| await new Promise(resolve => server.close(resolve)); | ||
| server = undefined; | ||
| } | ||
| if (tempDir) { | ||
| // The bin directory is locked to 0o555 by main(); restore write permissions | ||
| // so the temp directory can be removed during cleanup. | ||
| const binDir = path.join(tempDir, "gh-aw/mcp-cli/bin"); | ||
| if (fs.existsSync(binDir)) { | ||
| fs.chmodSync(binDir, 0o755); | ||
| } | ||
| fs.rmSync(tempDir, { recursive: true, force: true }); | ||
| tempDir = undefined; | ||
| } | ||
| delete process.env.RUNNER_TEMP; | ||
| delete process.env.MCP_GATEWAY_API_KEY; | ||
| delete process.env.MCP_GATEWAY_DOMAIN; | ||
| delete process.env.MCP_GATEWAY_PORT; | ||
| delete global.core; | ||
| vi.resetModules(); | ||
| }); | ||
|
|
||
| it("rewrites an existing 0o755 CLI wrapper script to owner-only permissions (0o700)", async () => { | ||
| // Minimal fake MCP server that answers initialize / notifications/initialized / tools/list | ||
| server = http.createServer((req, res) => { | ||
| let data = ""; | ||
| req.on("data", chunk => (data += chunk)); | ||
| req.on("end", () => { | ||
| const parsed = JSON.parse(data); | ||
| if (parsed.method === "tools/list") { | ||
| res.writeHead(200, { "Content-Type": "application/json" }); | ||
| res.end(JSON.stringify({ jsonrpc: "2.0", id: parsed.id, result: { tools: [{ name: "echo" }] } })); | ||
| } else { | ||
| res.writeHead(200, { "Content-Type": "application/json" }); | ||
| res.end(JSON.stringify({ jsonrpc: "2.0", id: parsed.id, result: {} })); | ||
| } | ||
| }); | ||
| }); | ||
| await new Promise(resolve => server.listen(0, "127.0.0.1", resolve)); | ||
| const address = server.address(); | ||
| const port = typeof address === "object" && address ? address.port : 0; | ||
|
|
||
| tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "mcp-cli-mount-test-")); | ||
| process.env.RUNNER_TEMP = tempDir; | ||
| process.env.MCP_GATEWAY_API_KEY = "super-secret-gateway-key"; | ||
| delete process.env.MCP_GATEWAY_DOMAIN; | ||
| delete process.env.MCP_GATEWAY_PORT; | ||
|
|
||
| const manifestDir = path.join(tempDir, "gh-aw/mcp-cli"); | ||
| fs.mkdirSync(manifestDir, { recursive: true }); | ||
| fs.writeFileSync(path.join(manifestDir, "manifest.json"), JSON.stringify({ servers: [{ name: "testserver", url: `http://127.0.0.1:${port}/mcp` }] }), "utf8"); | ||
|
|
||
|
Contributor
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. [/diagnosing-bugs] 💡 Move assignment before import()Assign global.core = { info: ..., warning: ..., addPath: () => {}, setOutput: () => {} };
vi.resetModules();
const mod = await import('./mount_mcp_as_cli.cjs?t=' + Date.now());
await mod.main();If the module is ever refactored to capture @copilot please address this. |
||
| const binDir = path.join(tempDir, "gh-aw/mcp-cli/bin"); | ||
| fs.mkdirSync(binDir, { recursive: true }); | ||
| const scriptPath = path.join(binDir, "testserver"); | ||
| fs.writeFileSync(scriptPath, "#!/usr/bin/env bash\necho preexisting\n", { mode: 0o755 }); | ||
| fs.chmodSync(scriptPath, 0o755); | ||
|
|
||
| const infos = []; | ||
| const warnings = []; | ||
| global.core = { | ||
| info: msg => infos.push(msg), | ||
| warning: msg => warnings.push(msg), | ||
| addPath: () => {}, | ||
| setOutput: () => {}, | ||
| }; | ||
|
|
||
| vi.resetModules(); | ||
| const mod = await import("./mount_mcp_as_cli.cjs?t=" + Date.now()); | ||
|
|
||
| await mod.main(); | ||
|
|
||
| expect(fs.existsSync(scriptPath)).toBe(true); | ||
|
|
||
| const mode = fs.statSync(scriptPath).mode & 0o777; | ||
| expect(mode).toBe(0o700); | ||
| expect(mode).not.toBe(0o755); | ||
|
|
||
| const scriptContent = fs.readFileSync(scriptPath, "utf8"); | ||
| expect(scriptContent).toContain("super-secret-gateway-key"); | ||
| }); | ||
| }); | ||
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.
Good call adding the comment explaining why 0o644 is safe here — the tools file contains only schema metadata, not secrets. Clear and auditable.