Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions actions/setup/js/mount_mcp_as_cli.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Copy link
Copy Markdown
Contributor

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.

try {
fs.writeFileSync(toolsFile, JSON.stringify(tools, null, 2), { mode: 0o644 });
} catch (err) {
Expand All @@ -619,7 +621,10 @@ async function main() {
// Write the CLI wrapper script using the container-accessible URL
const scriptPath = path.join(CLI_BIN_DIR, name);
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).
fs.writeFileSync(scriptPath, generateCLIWrapperScript(name, containerUrl, toolsFile, apiKey, bridgeScript), { mode: 0o700 });
mountedServers.push(name);
mountedServerTools.push({ name, tools });
core.info(` ✓ Mounted as: ${scriptPath}`);
Expand Down
93 changes: 92 additions & 1 deletion actions/setup/js/mount_mcp_as_cli.test.cjs
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";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice — importing afterEach and vi from vitest here sets up the test harness cleanly. The new HTTP server test coverage for file permissions is a great addition.

import fs from "fs";
import http from "http";
import os from "os";
import path from "path";

Expand Down Expand Up @@ -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", () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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;
vi.resetModules();
});

it("writes the CLI wrapper script with owner-only permissions (0o700), not world-readable (0o755)", 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"
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnosing-bugs] global.core is assigned after the dynamic import() call — this works because the module reads core lazily at call time, but the assumption is implicit and fragile.

💡 Move assignment before import()

Assign global.core before vi.resetModules() and the import() so the order is self-documenting:

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 core at the top level, the current order causes a silent failure — the test passes setup but then crashes inside main() with a confusing undefined-property error.

@copilot please address this.

vi.resetModules();
const mod = await import("./mount_mcp_as_cli.cjs?t=" + Date.now());

const infos = [];
const warnings = [];
global.core = {
info: msg => infos.push(msg),
warning: msg => warnings.push(msg),
addPath: () => {},
setOutput: () => {},
};

await mod.main();

const scriptPath = path.join(tempDir, "gh-aw/mcp-cli/bin/testserver");
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");

delete global.core;
});
});
Loading