Repository navigation
fix(mitm): graceful sudo degradation in slim Docker / non-root containers - #4513
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to gracefully degrade when sudo is not available (such as in slim Docker images running as a non-root user) by checking for its presence on the PATH and stripping the sudo prefix if missing. It also adds corresponding unit tests to verify this behavior. The review feedback highlights two key performance improvements: replacing the synchronous shell-out in isSudoAvailable with a faster traversal of PATH using fs.accessSync, and short-circuiting the isSudoAvailable check in execFileWithPassword so it only runs when the command is actually 'sudo' and the user is not root.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function isSudoAvailable(): boolean { | ||
| if (process.platform === "win32") return false; | ||
| try { | ||
| // nosemgrep: javascript.lang.security.detect-child-process.detect-child-process | ||
| execFileSync("sh", ["-c", "command -v sudo"], { stdio: "ignore" }); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
Using execFileSync to spawn a shell process (sh -c "command -v sudo") synchronously blocks the Node.js event loop. Spawning a process is a relatively heavy operation (taking tens of milliseconds), which can degrade performance when executing commands. Since we only need to check if sudo exists in the system's PATH, we can perform this check much faster and without blocking the event loop by traversing the PATH directories using fs.accessSync.
export function isSudoAvailable(): boolean {
if (process.platform === "win32") return false;
const pathEnv = process.env.PATH || "";
const dirs = pathEnv.split(path.delimiter);
for (const dir of dirs) {
if (!dir) continue;
try {
fs.accessSync(path.join(dir, "sudo"), fs.constants.X_OK);
return true;
} catch {
// Continue searching
}
}
return false;
}| const root = isRoot(); | ||
| const needsPassword = !root || command !== "sudo"; | ||
| const sudoAvailable = isSudoAvailable(); | ||
| const stripSudo = command === "sudo" && (root || !sudoAvailable); | ||
| const needsPassword = !stripSudo && command === "sudo"; |
There was a problem hiding this comment.
The isSudoAvailable() check is currently called on every invocation of execFileWithPassword, even if the command is not 'sudo' or if the process is already running as root. Since isSudoAvailable() performs synchronous operations, this introduces unnecessary overhead for non-sudo commands. We can short-circuit this check so it is only executed when command === 'sudo' and the user is not root.
| const root = isRoot(); | |
| const needsPassword = !root || command !== "sudo"; | |
| const sudoAvailable = isSudoAvailable(); | |
| const stripSudo = command === "sudo" && (root || !sudoAvailable); | |
| const needsPassword = !stripSudo && command === "sudo"; | |
| const root = isRoot(); | |
| const stripSudo = command === "sudo" && (root || !isSudoAvailable()); | |
| const needsPassword = !stripSudo && command === "sudo"; |
…ners
OmniRoute's runtime Docker image (`node:24-trixie-slim` + `USER node`) does
not ship `sudo`, so `execFileWithPassword("sudo", ...)` failed with
`spawn sudo ENOENT` for any MITM operation triggered from inside the
container (cert install, DNS host-file write).
Adds `isSudoAvailable()` probe and extends the existing root-skip branch in
`execFileWithPassword` to also strip the `sudo -S` wrapper when sudo is
missing — the underlying command then runs directly under the current user
(no elevation). Privileged operations that genuinely need elevation (system
trust store, `/etc/hosts`) still error explicitly so operators can mount
the CA / hosts file from the host side.
TDD: 4 new unit tests in `tests/unit/mitm-sudo-graceful-degrade.test.ts`
cover the probe shape, the sudo-missing fallback, the empty-PATH fallback,
and the no-regression on non-sudo commands. Existing
`mitm-system-commands-prefix-3641` and `mitm-manager-*` tests stay green.
Inspired-by: decolua/9router#381
Co-authored-by: lokinh <locnh@uniultra.xyz>
d62b6f3 to
e0f9220
Compare
…ners (cheaper-inference#4513) Rebuilt onto release/v3.8.33 (squash-base-stale). Integrated into release/v3.8.33.
Summary
OmniRoute's runtime Docker image (
node:24-trixie-slim+USER node) does not shipsudo, soexecFileWithPassword("sudo", …)failed withspawn sudo ENOENTfor any MITM operation triggered from inside the container (cert install, DNS host-file write).This PR adds
isSudoAvailable()and extends the existing root-skip branch inexecFileWithPasswordto also strip thesudo -Swrapper when sudo is missing — the underlying command then runs directly under the current user (no elevation, same path already taken for the root case).What does not change
assembleStandalone; the upstream PR's COPY ofsrc/mitm/node-forgeis N/A here (OmniRoute uses Node's built-incrypto, notnode-forge).runElevatedPowerShellpath is untouched./etc/hosts) still error explicitly. The container operator is expected to mount those from the host side.Test plan
Validated via TDD (Hard Rule #18):
tests/unit/mitm-sudo-graceful-degrade.test.ts— 4 cases (probe shape, sudo-missing fallback, empty-PATH fallback, non-sudo command unaffected). Confirmed RED → GREEN locally.tests/unit/mitm-system-commands-prefix-3641.test.ts— still GREEN (no regression on the doubled-prefix fix).tests/unit/mitm-manager-{stub,repair,bypass-json}.test.ts— still GREEN.npm run typecheck:core— clean.npm run check:any-budget:t11— PASS (explicit-any budget respected).npx eslint src/mitm/systemCommands.ts tests/unit/mitm-sudo-graceful-degrade.test.ts— clean.VPS validation note: not required for this PR — the failure mode is reproducible and asserted in TDD (test #3 explicitly clears
PATHto simulate the Dockernode:*-slimenvironment).