Skip to content

Commit f024982

Browse files
13ernkastel13ernkastel
committed
fix(security): harden sandbox command execution
Rebase the sandbox command-hardening changes onto current main. Add argv-based sandbox command checks and align the onboarding harness with the hardened dashboard and DNS helper calls.
1 parent 3f4d6fe commit f024982

File tree

7 files changed

+274
-18
lines changed

7 files changed

+274
-18
lines changed

‎bin/lib/onboard.js‎

Lines changed: 25 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,16 @@ function envInt(name, fallback) {
1818
const n = Number(raw);
1919
return Number.isFinite(n) ? Math.max(0, Math.round(n)) : fallback;
2020
}
21-
const { ROOT, SCRIPTS, redact, run, runCapture, shellQuote } = require("./runner");
21+
const {
22+
ROOT,
23+
SCRIPTS,
24+
redact,
25+
run,
26+
runCapture,
27+
runFile,
28+
shellQuote,
29+
validateName,
30+
} = require("./runner");
2231
const {
2332
getDefaultOllamaModel,
2433
getBootstrapOllamaModelOptions,
@@ -2389,15 +2398,14 @@ async function promptValidatedSandboxName() {
23892398
"NEMOCLAW_SANDBOX_NAME",
23902399
"my-assistant",
23912400
);
2392-
const sandboxName = (nameAnswer || "my-assistant").trim().toLowerCase();
2401+
const sandboxName = (nameAnswer || "my-assistant").trim();
23932402

2394-
// Validate: RFC 1123 subdomain — lowercase alphanumeric and hyphens,
2395-
// must start and end with alphanumeric (required by Kubernetes/OpenShell)
2396-
if (/^[a-z0-9]([a-z0-9-]*[a-z0-9])?$/.test(sandboxName)) {
2397-
return sandboxName;
2403+
try {
2404+
return validateName(sandboxName, "sandbox name");
2405+
} catch (error) {
2406+
console.error(` ${error.message}`);
23982407
}
23992408

2400-
console.error(` Invalid sandbox name: '${sandboxName}'`);
24012409
console.error(" Names must be lowercase, contain only letters, numbers, and hyphens,");
24022410
console.error(" and must start and end with a letter or number.");
24032411

@@ -2424,7 +2432,10 @@ async function createSandbox(
24242432
) {
24252433
step(5, 7, "Creating sandbox");
24262434

2427-
const sandboxName = sandboxNameOverride || (await promptValidatedSandboxName());
2435+
const sandboxName = validateName(
2436+
sandboxNameOverride ?? (await promptValidatedSandboxName()),
2437+
"sandbox name",
2438+
);
24282439
const chatUiUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${CONTROL_UI_PORT}`;
24292440

24302441
// Reconcile local registry state with the live OpenShell gateway state.
@@ -2583,11 +2594,11 @@ async function createSandbox(
25832594
// or seeing 502/503 errors during initial load.
25842595
console.log(" Waiting for NemoClaw dashboard to become ready...");
25852596
for (let i = 0; i < 15; i++) {
2586-
const readyMatch = runCapture(
2587-
`openshell sandbox exec ${sandboxName} curl -sf http://localhost:18789/ 2>/dev/null || echo "no"`,
2597+
const readyMatch = runCaptureOpenshell(
2598+
["sandbox", "exec", sandboxName, "curl", "-sf", `http://localhost:${CONTROL_UI_PORT}/`],
25882599
{ ignoreError: true },
25892600
);
2590-
if (readyMatch && !readyMatch.includes("no")) {
2601+
if (readyMatch) {
25912602
console.log(" ✓ Dashboard is live");
25922603
break;
25932604
}
@@ -2612,10 +2623,9 @@ async function createSandbox(
26122623
// DNS proxy — run a forwarder in the sandbox pod so the isolated
26132624
// sandbox namespace can resolve hostnames (fixes #626).
26142625
console.log(" Setting up sandbox DNS proxy...");
2615-
run(
2616-
`bash "${path.join(SCRIPTS, "setup-dns-proxy.sh")}" ${GATEWAY_NAME} "${sandboxName}" 2>&1 || true`,
2617-
{ ignoreError: true },
2618-
);
2626+
runFile("bash", [path.join(SCRIPTS, "setup-dns-proxy.sh"), GATEWAY_NAME, sandboxName], {
2627+
ignoreError: true,
2628+
});
26192629

26202630
console.log(` ✓ Sandbox '${sandboxName}' created`);
26212631
return sandboxName;

‎bin/lib/runner.js‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,30 @@ function runInteractive(cmd, opts = {}) {
5757
return result;
5858
}
5959

60+
/**
61+
* Run a program directly with argv-style arguments, bypassing shell parsing.
62+
* Exits the process on failure unless opts.ignoreError is true.
63+
*/
64+
function runFile(file, args = [], opts = {}) {
65+
const stdio = opts.stdio ?? ["ignore", "pipe", "pipe"];
66+
const normalizedArgs = args.map((arg) => String(arg));
67+
const result = spawnSync(file, normalizedArgs, {
68+
...opts,
69+
stdio,
70+
cwd: ROOT,
71+
env: { ...process.env, ...opts.env },
72+
});
73+
if (!opts.suppressOutput) {
74+
writeRedactedResult(result, stdio);
75+
}
76+
if (result.status !== 0 && !opts.ignoreError) {
77+
const rendered = [shellQuote(file), ...normalizedArgs.map((arg) => shellQuote(arg))].join(" ");
78+
console.error(` Command failed (exit ${result.status}): ${redact(rendered).slice(0, 80)}`);
79+
process.exit(result.status || 1);
80+
}
81+
return result;
82+
}
83+
6084
/**
6185
* Run a shell command and return its stdout as a trimmed string.
6286
* Throws a redacted error on failure, or returns '' when opts.ignoreError is true.
@@ -200,6 +224,7 @@ module.exports = {
200224
redact,
201225
run,
202226
runCapture,
227+
runFile,
203228
runInteractive,
204229
shellQuote,
205230
validateName,

‎src/lib/onboard-session.test.ts‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,20 @@ describe("onboard session", () => {
120120
expect(loaded.metadata.token).toBeUndefined();
121121
});
122122

123+
it("persists and clears web search config through safe session updates", () => {
124+
session.saveSession(session.createSession());
125+
session.markStepComplete("provider_selection", {
126+
webSearchConfig: { fetchEnabled: true },
127+
});
128+
129+
let loaded = session.loadSession();
130+
expect(loaded.webSearchConfig).toEqual({ fetchEnabled: true });
131+
132+
session.completeSession({ webSearchConfig: null });
133+
loaded = session.loadSession();
134+
expect(loaded.webSearchConfig).toBeNull();
135+
});
136+
123137
it("does not clear existing metadata when updates omit whitelisted metadata fields", () => {
124138
session.saveSession(session.createSession({ metadata: { gatewayName: "nemoclaw" } }));
125139
session.markStepComplete("provider_selection", {

‎test/onboard.test.js‎

Lines changed: 101 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1315,10 +1315,20 @@ runner.run = (command, opts = {}) => {
13151315
commands.push({ command, env: opts.env || null });
13161316
return { status: 0 };
13171317
};
1318+
runner.runFile = (file, args = [], opts = {}) => {
1319+
commands.push({ type: "runFile", file, args, env: opts.env || null });
1320+
return { status: 0 };
1321+
};
13181322
runner.runCapture = (command) => {
13191323
if (command.includes("'sandbox' 'get' 'my-assistant'")) return "";
13201324
if (command.includes("'sandbox' 'list'")) return "my-assistant Ready";
1321-
if (command.includes("sandbox exec my-assistant curl -sf http://localhost:18789/")) return "ok";
1325+
if (
1326+
command.includes(
1327+
"'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'",
1328+
)
1329+
) {
1330+
return "ok";
1331+
}
13221332
return "";
13231333
};
13241334
registry.registerSandbox = () => true;
@@ -1390,6 +1400,74 @@ const { createSandbox } = require(${onboardPath});
13901400
);
13911401
});
13921402

1403+
it("rejects a whitespace-only sandboxNameOverride before any shell helpers run", async () => {
1404+
const repoRoot = path.join(import.meta.dirname, "..");
1405+
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-blank-override-"));
1406+
const scriptPath = path.join(tmpDir, "blank-sandbox-override.js");
1407+
const onboardPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "onboard.js"));
1408+
const runnerPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "runner.js"));
1409+
const registryPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "registry.js"));
1410+
const preflightPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "preflight.js"));
1411+
const credentialsPath = JSON.stringify(path.join(repoRoot, "bin", "lib", "credentials.js"));
1412+
1413+
const script = String.raw`
1414+
const runner = require(${runnerPath});
1415+
const registry = require(${registryPath});
1416+
const preflight = require(${preflightPath});
1417+
const credentials = require(${credentialsPath});
1418+
1419+
const commands = [];
1420+
runner.run = (command, opts = {}) => {
1421+
commands.push({ type: "run", command, env: opts.env || null });
1422+
return { status: 0 };
1423+
};
1424+
runner.runFile = (file, args = [], opts = {}) => {
1425+
commands.push({ type: "runFile", file, args, env: opts.env || null });
1426+
return { status: 0 };
1427+
};
1428+
runner.runCapture = (command) => {
1429+
commands.push({ type: "runCapture", command });
1430+
return "";
1431+
};
1432+
registry.registerSandbox = () => true;
1433+
registry.removeSandbox = () => true;
1434+
preflight.checkPortAvailable = async () => ({ ok: true });
1435+
credentials.prompt = async () => {
1436+
throw new Error("prompt should not be reached for an explicit override");
1437+
};
1438+
1439+
const { createSandbox } = require(${onboardPath});
1440+
1441+
(async () => {
1442+
try {
1443+
await createSandbox(null, "gpt-5.4", "nvidia-prod", null, " ");
1444+
console.log(JSON.stringify({ ok: true, commands }));
1445+
} catch (error) {
1446+
console.log(JSON.stringify({ ok: false, message: error.message, commands }));
1447+
}
1448+
})().catch((error) => {
1449+
console.error(error);
1450+
process.exit(1);
1451+
});
1452+
`;
1453+
fs.writeFileSync(scriptPath, script);
1454+
1455+
const result = spawnSync(process.execPath, [scriptPath], {
1456+
cwd: repoRoot,
1457+
encoding: "utf-8",
1458+
env: {
1459+
...process.env,
1460+
HOME: tmpDir,
1461+
},
1462+
});
1463+
1464+
assert.equal(result.status, 0, result.stderr);
1465+
const payload = JSON.parse(result.stdout.trim().split("\n").pop());
1466+
assert.equal(payload.ok, false);
1467+
assert.match(payload.message, /Invalid sandbox name/);
1468+
assert.deepEqual(payload.commands, []);
1469+
});
1470+
13931471
it("binds the dashboard forward to 0.0.0.0 when CHAT_UI_URL points to a remote host", async () => {
13941472
const repoRoot = path.join(import.meta.dirname, "..");
13951473
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-onboard-remote-forward-"));
@@ -1419,10 +1497,20 @@ runner.run = (command, opts = {}) => {
14191497
commands.push({ command, env: opts.env || null });
14201498
return { status: 0 };
14211499
};
1500+
runner.runFile = (file, args = [], opts = {}) => {
1501+
commands.push({ type: "runFile", file, args, env: opts.env || null });
1502+
return { status: 0 };
1503+
};
14221504
runner.runCapture = (command) => {
14231505
if (command.includes("'sandbox' 'get' 'my-assistant'")) return "";
14241506
if (command.includes("'sandbox' 'list'")) return "my-assistant Ready";
1425-
if (command.includes("sandbox exec my-assistant curl -sf http://localhost:18789/")) return "ok";
1507+
if (
1508+
command.includes(
1509+
"'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'",
1510+
)
1511+
) {
1512+
return "ok";
1513+
}
14261514
return "";
14271515
};
14281516
registry.registerSandbox = () => true;
@@ -1510,13 +1598,23 @@ runner.run = (command, opts = {}) => {
15101598
commands.push({ command, env: opts.env || null });
15111599
return { status: 0 };
15121600
};
1601+
runner.runFile = (file, args = [], opts = {}) => {
1602+
commands.push({ type: "runFile", file, args, env: opts.env || null });
1603+
return { status: 0 };
1604+
};
15131605
runner.runCapture = (command) => {
15141606
if (command.includes("'sandbox' 'get' 'my-assistant'")) return "";
15151607
if (command.includes("'sandbox' 'list'")) {
15161608
sandboxListCalls += 1;
15171609
return sandboxListCalls >= 2 ? "my-assistant Ready" : "my-assistant Pending";
15181610
}
1519-
if (command.includes("sandbox exec my-assistant curl -sf http://localhost:18789/")) return "ok";
1611+
if (
1612+
command.includes(
1613+
"'sandbox' 'exec' 'my-assistant' 'curl' '-sf' 'http://localhost:18789/'",
1614+
)
1615+
) {
1616+
return "ok";
1617+
}
15201618
return "";
15211619
};
15221620
registry.registerSandbox = () => true;

‎test/registry.test.js‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,25 @@ describe("registry", () => {
107107
expect(data.defaultSandbox).toBe("persist");
108108
});
109109

110+
it("clearAll removes persisted sandboxes and the default pointer", () => {
111+
registry.registerSandbox({ name: "alpha", model: "m1" });
112+
registry.registerSandbox({ name: "beta", model: "m2" });
113+
registry.setDefault("beta");
114+
115+
registry.clearAll();
116+
117+
expect(registry.listSandboxes()).toEqual({
118+
sandboxes: [],
119+
defaultSandbox: null,
120+
});
121+
expect(registry.getDefault()).toBe(null);
122+
expect(registry.getSandbox("alpha")).toBe(null);
123+
expect(JSON.parse(fs.readFileSync(regFile, "utf-8"))).toEqual({
124+
sandboxes: {},
125+
defaultSandbox: null,
126+
});
127+
});
128+
110129
it("handles corrupt registry file gracefully", () => {
111130
fs.mkdirSync(path.dirname(regFile), { recursive: true });
112131
fs.writeFileSync(regFile, "NOT JSON");

‎test/runner.test.js‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,55 @@ describe("runner helpers", () => {
5656
expect(calls[0][2].stdio).toEqual(["ignore", "pipe", "pipe"]);
5757
expect(calls[1][2].stdio).toEqual(["inherit", "pipe", "pipe"]);
5858
});
59+
it("runs argv-style commands without going through bash -c", () => {
60+
const calls = [];
61+
const originalSpawnSync = childProcess.spawnSync;
62+
// @ts-expect-error — intentional partial mock for testing
63+
childProcess.spawnSync = (...args) => {
64+
calls.push(args);
65+
return { status: 0, stdout: "", stderr: "" };
66+
};
67+
68+
try {
69+
delete require.cache[require.resolve(runnerPath)];
70+
const { runFile } = require(runnerPath);
71+
runFile("bash", ["/tmp/setup.sh", "safe;name", "$(id)"]);
72+
} finally {
73+
childProcess.spawnSync = originalSpawnSync;
74+
delete require.cache[require.resolve(runnerPath)];
75+
}
76+
77+
expect(calls).toHaveLength(1);
78+
expect(calls[0][0]).toBe("bash");
79+
expect(calls[0][1]).toEqual(["/tmp/setup.sh", "safe;name", "$(id)"]);
80+
expect(calls[0][2].stdio).toEqual(["ignore", "pipe", "pipe"]);
81+
});
82+
83+
it("honors suppressOutput for argv-style commands", () => {
84+
const originalSpawnSync = childProcess.spawnSync;
85+
const stdoutSpy = vi.spyOn(process.stdout, "write").mockImplementation(() => true);
86+
const stderrSpy = vi.spyOn(process.stderr, "write").mockImplementation(() => true);
87+
// @ts-expect-error — intentional partial mock for testing
88+
childProcess.spawnSync = () => ({
89+
status: 0,
90+
stdout: "safe stdout\n",
91+
stderr: "safe stderr\n",
92+
});
93+
94+
try {
95+
delete require.cache[require.resolve(runnerPath)];
96+
const { runFile } = require(runnerPath);
97+
runFile("bash", ["/tmp/setup.sh"], { suppressOutput: true });
98+
} finally {
99+
childProcess.spawnSync = originalSpawnSync;
100+
stdoutSpy.mockRestore();
101+
stderrSpy.mockRestore();
102+
delete require.cache[require.resolve(runnerPath)];
103+
}
104+
105+
expect(stdoutSpy).not.toHaveBeenCalled();
106+
expect(stderrSpy).not.toHaveBeenCalled();
107+
});
59108
});
60109

61110
describe("runner env merging", () => {

0 commit comments

Comments
 (0)