Skip to content

security: use argv for host-side helper calls #2476

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.

Sign up for GitHub

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

Closed
WilliamK112 wants to merge 3 commits into from
Closed

security: use argv for host-side helper calls #2476

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69dc6a9
security: prefer argv for host-side helper calls
WilliamK112 Apr 25, 2026
bac60ab
test: convert argv callsites suite to ESM imports
WilliamK112 Apr 28, 2026
7d5a678
test: fail fast on unexpected argv stub calls
WilliamK112 Apr 28, 2026
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
43 changes: 31 additions & 12 deletions src/lib/onboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2595,17 +2595,33 @@ function destroyGateway() {
}
// openshell gateway destroy doesn't remove Docker volumes, which leaves
// corrupted cluster state that breaks the next gateway start. Clean them up.
// Shell required: pipe (|), && chaining, || fallback.
run(
`docker volume ls -q --filter "name=openshell-cluster-${GATEWAY_NAME}" | grep . && docker volume ls -q --filter "name=openshell-cluster-${GATEWAY_NAME}" | xargs docker volume rm || true`,
const volumeIds = runCapture(
["docker", "volume", "ls", "-q", "--filter", `name=openshell-cluster-${GATEWAY_NAME}`],
{ ignoreError: true },
);
)
.split("\n")
.map((line) => line.trim())
.filter(Boolean);
if (volumeIds.length > 0) {
run(["docker", "volume", "rm", ...volumeIds], {
ignoreError: true,
suppressOutput: true,
});
}
}

function getGatewayClusterContainerState(): string {
const containerName = getGatewayClusterContainerName();
const state = runCapture(
`docker inspect --type container --format '{{.State.Status}}{{if .State.Health}} {{.State.Health.Status}}{{end}}' ${shellQuote(containerName)} 2>/dev/null`,
[
"docker",
"inspect",
"--type",
"container",
"--format",
"{{.State.Status}}{{if .State.Health}} {{.State.Health.Status}}{{end}}",
containerName,
],
{ ignoreError: true },
)
.trim()
Expand Down Expand Up @@ -2700,12 +2716,12 @@ fi

function runGatewayClusterCapture(script: string, opts: RunnerOptions = {}) {
const containerName = getGatewayClusterContainerName();
return runCapture(`docker exec ${shellQuote(containerName)} sh -lc ${shellQuote(script)}`, opts);
return runCapture(["docker", "exec", containerName, "sh", "-lc", script], opts);
}

function runGatewayCluster(script: string, opts: RunnerOptions = {}) {
const containerName = getGatewayClusterContainerName();
return run(`docker exec ${shellQuote(containerName)} sh -lc ${shellQuote(script)}`, opts);
return run(["docker", "exec", containerName, "sh", "-lc", script], opts);
}

function listMissingGatewayBootstrapSecrets() {
Expand Down Expand Up @@ -3289,14 +3305,14 @@ async function preflight(): Promise<ReturnType<typeof nim.detectGpu>> {
// tunnels the user may have set up on the same port. (#1950)
if (port === DASHBOARD_PORT && portCheck.process === "ssh" && portCheck.pid) {
// Use `ps` to get the command line — works on Linux, macOS, and WSL.
const cmdline = runCapture(`ps -p ${portCheck.pid} -o args= 2>/dev/null`, {
const cmdline = runCapture(["ps", "-p", String(portCheck.pid), "-o", "args="], {
ignoreError: true,
}).trim();
if (cmdline.includes("openshell")) {
console.log(
` Cleaning up orphaned SSH port-forward on port ${port} (PID ${portCheck.pid})...`,
);
run(`kill ${portCheck.pid} 2>/dev/null || true`, { ignoreError: true });
run(["kill", String(portCheck.pid)], { ignoreError: true });
sleep(1);
portCheck = await checkPortAvailable(port);
if (portCheck.ok) {
Expand Down Expand Up @@ -4598,8 +4614,7 @@ async function setupNim(gpu: ReturnType<typeof nim.detectGpu>): Promise<{
let preferredInferenceApi: string | null = null;

// Detect local inference options
// "command -v" is a shell builtin — must go through bash.
const hasOllama = !!runCapture("command -v ollama", { ignoreError: true });
const hasOllama = !!runCapture(["which", "ollama"], { ignoreError: true });
const ollamaRunning = !!runCapture(["curl", "-sf", `http://127.0.0.1:${OLLAMA_PORT}/api/tags`], {
ignoreError: true,
});
Expand Down Expand Up @@ -7090,7 +7105,11 @@ function printDashboard(

const token = fetchGatewayAuthTokenFromSandbox(sandboxName);
const chatUiUrl = process.env.CHAT_UI_URL || `http://127.0.0.1:${CONTROL_UI_PORT}`;
const wslAddr = isWsl() ? (String(runCapture("hostname -I 2>/dev/null", { ignoreError: true }) || "").trim().split(/\s+/)[0] || null) : null;
const wslAddr = isWsl()
? (String(runCapture(["hostname", "-I"], { ignoreError: true }) || "")
.trim()
.split(/\s+/)[0] || null)
: null;
const chain = buildChain({ chatUiUrl, isWsl: isWsl(), wslHostAddress: wslAddr });

// Build access info inline — uses chain instead of re-deriving from env
Expand Down
61 changes: 33 additions & 28 deletions src/lib/preflight.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ import { DASHBOARD_PORT } from "./ports";

// runner.ts still uses CommonJS-style exports — use require here.
// eslint-disable-next-line @typescript-eslint/no-require-imports
const { runCapture } = require("./runner");
const { run, runCapture } = require("./runner");

type CaptureCommand = string | readonly string[];

// ── Types ────────────────────────────────────────────────────────

Expand Down Expand Up @@ -120,17 +122,17 @@ export interface AssessHostOpts {
dockerInfoOutput?: string;
dockerInfoError?: string;
readFileImpl?: (filePath: string, encoding: BufferEncoding) => string;
runCaptureImpl?: (command: string, options?: { ignoreError?: boolean }) => string;
runCaptureImpl?: (command: CaptureCommand, options?: { ignoreError?: boolean }) => string;
commandExistsImpl?: (commandName: string) => boolean;
gpuProbeImpl?: () => boolean;
}

function commandExists(
commandName: string,
runCaptureImpl: (command: string, options?: { ignoreError?: boolean }) => string,
runCaptureImpl: (command: CaptureCommand, options?: { ignoreError?: boolean }) => string,
): boolean {
try {
const output = runCaptureImpl(`command -v ${commandName}`, { ignoreError: true });
const output = runCaptureImpl(["which", commandName], { ignoreError: true });
return Boolean(String(output || "").trim());
} catch {
return false;
Expand Down Expand Up @@ -204,16 +206,16 @@ function isHeadlessLikely(env: NodeJS.ProcessEnv): boolean {
}

function detectNvidiaGpu(
runCaptureImpl: (command: string, options?: { ignoreError?: boolean }) => string,
runCaptureImpl: (command: CaptureCommand, options?: { ignoreError?: boolean }) => string,
): boolean {
if (!commandExists("nvidia-smi", runCaptureImpl)) {
return false;
}
return Boolean(String(runCaptureImpl("nvidia-smi -L", { ignoreError: true }) || "").trim());
return Boolean(String(runCaptureImpl(["nvidia-smi", "-L"], { ignoreError: true }) || "").trim());
}

function detectPackageManager(
runCaptureImpl: (command: string, options?: { ignoreError?: boolean }) => string,
runCaptureImpl: (command: CaptureCommand, options?: { ignoreError?: boolean }) => string,
): PackageManager {
if (commandExists("apt-get", runCaptureImpl)) return "apt";
if (commandExists("dnf", runCaptureImpl)) return "dnf";
Expand Down Expand Up @@ -245,7 +247,7 @@ export function assessHost(opts: AssessHostOpts = {}): HostAssessment {
const env = opts.env ?? process.env;
const runCaptureImpl =
opts.runCaptureImpl ??
((command: string, options?: { ignoreError?: boolean }) =>
((command: CaptureCommand, options?: { ignoreError?: boolean }) =>
runCapture(command, { ignoreError: options?.ignoreError ?? false }));
const readFileImpl = opts.readFileImpl ?? fs.readFileSync;
const dockerInstalled =
Expand All @@ -261,7 +263,7 @@ export function assessHost(opts: AssessHostOpts = {}): HostAssessment {
let dockerReachable = false;
let dockerRunning = false;
if (dockerInstalled && dockerInfoOutput === undefined) {
dockerInfoOutput = runCaptureImpl("docker info --format '{{json .}}' 2>/dev/null", {
dockerInfoOutput = runCaptureImpl(["docker", "info", "--format", "{{json .}}"], {
ignoreError: true,
});
}
Expand Down Expand Up @@ -290,11 +292,11 @@ export function assessHost(opts: AssessHostOpts = {}): HostAssessment {
const dockerDefaultCgroupnsMode = readDockerDefaultCgroupnsMode(readFileImpl);
const dockerServiceActive =
platform === "linux" && systemctlAvailable && dockerInstalled
? parseSystemctlState(runCaptureImpl("systemctl is-active docker", { ignoreError: true }))
? parseSystemctlState(runCaptureImpl(["systemctl", "is-active", "docker"], { ignoreError: true }))
: null;
const dockerServiceEnabled =
platform === "linux" && systemctlAvailable && dockerInstalled
? parseSystemctlState(runCaptureImpl("systemctl is-enabled docker", { ignoreError: true }))
? parseSystemctlState(runCaptureImpl(["systemctl", "is-enabled", "docker"], { ignoreError: true }))
: null;
const assessment: HostAssessment = {
platform,
Expand Down Expand Up @@ -521,8 +523,7 @@ export async function checkPortAvailable(
if (typeof o.lsofOutput === "string") {
lsofOut = o.lsofOutput;
} else {
// "command -v" is a shell builtin — must go through bash.
const hasLsof = runCapture("command -v lsof", { ignoreError: true });
const hasLsof = runCapture(["which", "lsof"], { ignoreError: true });
if (hasLsof) {
lsofOut = runCapture(["lsof", "-i", `:${p}`, "-sTCP:LISTEN", "-P", "-n"], {
ignoreError: true,
Expand Down Expand Up @@ -661,11 +662,10 @@ function getExistingSwapResult(mem: MemoryInfo): SwapResult | null {

function checkSwapDiskSpace(): SwapResult | null {
try {
// Pipe requires a shell: df ... | tail -1
const dfOut = runCapture("df / --output=avail -k 2>/dev/null | tail -1", {
const dfOut = runCapture(["df", "/", "--output=avail", "-k"], {
ignoreError: true,
});
const freeKB = parseInt((dfOut || "").trim(), 10);
const freeKB = parseInt((dfOut || "").trim().split(/\r?\n/).pop() || "", 10);
if (!isNaN(freeKB) && freeKB < 5000000) {
return {
ok: false,
Expand Down Expand Up @@ -712,11 +712,17 @@ function createSwapfile(mem: MemoryInfo): SwapResult {
runCapture(["sudo", "chmod", "600", "/swapfile"], { ignoreError: false });
runCapture(["sudo", "mkswap", "/swapfile"], { ignoreError: false });
runCapture(["sudo", "swapon", "/swapfile"], { ignoreError: false });
// Shell required: grep || echo | tee pipeline
runCapture(
"grep -q '/swapfile' /etc/fstab || echo '/swapfile none swap sw 0 0' | sudo tee -a /etc/fstab",
{ ignoreError: false },
);
const existingFstabEntry =
run(["grep", "-q", "/swapfile", "/etc/fstab"], {
ignoreError: true,
suppressOutput: true,
}).status === 0;
if (!existingFstabEntry) {
runCapture(["sudo", "tee", "-a", "/etc/fstab"], {
ignoreError: false,
input: "/swapfile none swap sw 0 0\n",
});
Comment on lines +715 to +724

@coderabbitai coderabbitai Bot Apr 25, 2026

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.

⚠️ Potential issue | 🟡 Minor

Match only active /swapfile entries in /etc/fstab.

Line 716 uses a plain substring grep, so a commented entry or an unrelated path containing /swapfile will suppress the append. The swap file still works for the current boot, but it will not be re-enabled after reboot.

Suggested fix 
     const existingFstabEntry =
-      run(["grep", "-q", "/swapfile", "/etc/fstab"], {
+      run(["grep", "-qE", "^[[:space:]]*/swapfile[[:space:]]", "/etc/fstab"], {
         ignoreError: true,
         suppressOutput: true,
       }).status === 0;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const existingFstabEntry =
run(["grep", "-q", "/swapfile", "/etc/fstab"], {
ignoreError: true,
suppressOutput: true,
}).status === 0;
if (!existingFstabEntry) {
runCapture(["sudo", "tee", "-a", "/etc/fstab"], {
ignoreError: false,
input: "/swapfile none swap sw 0 0\n",
});
const existingFstabEntry =
run(["grep", "-qE", "^[[:space:]]*/swapfile[[:space:]]", "/etc/fstab"], {
ignoreError: true,
suppressOutput: true,
}).status === 0;
if (!existingFstabEntry) {
runCapture(["sudo", "tee", "-a", "/etc/fstab"], {
ignoreError: false,
input: "/swapfile none swap sw 0 0\n",
});
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@src/lib/preflight.ts` around lines 715 - 724, The current grep call in
existingFstabEntry uses a plain substring search which can match commented lines
or unrelated paths; update the check so it only matches active (non-commented)
fstab entries for /swapfile by changing the command invoked by run([...]) to use
an anchored regex (e.g., grep -qE with a pattern like
'^[[:space:]]*[^#].*/swapfile\b') so only uncommented lines containing /swapfile
are detected; keep the subsequent runCapture([... "sudo", "tee", "-a",
"/etc/fstab"]) logic unchanged so the append happens only when no active entry
exists.

}
writeManagedSwapMarker();

return { ok: true, totalMB: mem.totalMB + 4096, swapCreated: true };
Expand Down Expand Up @@ -818,12 +824,12 @@ export interface DnsProbeResult {

export interface ProbeContainerDnsOpts {
/** Override the docker run command. */
command?: string;
command?: CaptureCommand;
/** Inject captured output (bypasses shell). */
outputOverride?: string | null;
/** Override runCapture. */
runCaptureImpl?: (
command: string,
command: CaptureCommand,
opts?: { ignoreError?: boolean; timeout?: number },
) => string | null;
}
Expand All @@ -844,15 +850,15 @@ const PROBE_TIMEOUT_MS = 20_000;
* `172.17.0.1`.
*/
export function getDockerBridgeGatewayIp(
runCaptureImpl: (command: string, opts?: { ignoreError?: boolean }) => string | null = (
runCaptureImpl: (command: CaptureCommand, opts?: { ignoreError?: boolean }) => string | null = (
cmd,
o,
) => runCapture(cmd, { ignoreError: o?.ignoreError ?? false }),
): string | null {
let raw: string | null;
try {
raw = runCaptureImpl(
"docker network inspect bridge --format '{{range .IPAM.Config}}{{.Gateway}}{{end}}' 2>/dev/null",
["docker", "network", "inspect", "bridge", "--format", "{{range .IPAM.Config}}{{.Gateway}}{{end}}"],
{ ignoreError: true },
);
} catch {
Expand Down Expand Up @@ -896,15 +902,14 @@ export function probeContainerDns(opts: ProbeContainerDnsOpts = {}): DnsProbeRes
// ignoreError, and we fall through to the `no_output` branch.
const command =
opts.command ??
"docker run --rm --pull=missing busybox:latest " +
"nslookup registry.npmjs.org 2>&1";
["docker", "run", "--rm", "--pull=missing", "busybox:latest", "nslookup", "registry.npmjs.org"];

let output: string | null | undefined = opts.outputOverride;
if (output === undefined) {
try {
const runCaptureImpl =
opts.runCaptureImpl ??
((cmd: string, o?: { ignoreError?: boolean; timeout?: number }) =>
((cmd: CaptureCommand, o?: { ignoreError?: boolean; timeout?: number }) =>
runCapture(cmd, {
ignoreError: o?.ignoreError ?? false,
timeout: o?.timeout,
Expand Down
Loading