Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
6 changes: 5 additions & 1 deletion packages/core/src/sandbox/utils/commandUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ export async function getCommandName(req: SandboxRequest): Promise<string> {
const roots = getCommandRoots(stripped).filter(
(r) => r !== 'shopt' && r !== 'set',
);
if (roots.length > 0) {
// Single-root enforcement: only grant named-command permissions when the
// command is unambiguous. Multi-root chains fall back to basename so that
// a chained command like `git; malicious_cmd` never inherits `git`'s
// sandbox policy for the entire chain.
if (roots.length === 1) {
return roots[0];
}
return path.basename(req.command);
Expand Down
8 changes: 4 additions & 4 deletions packages/core/src/services/shellExecutionService.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -289,7 +289,7 @@ describe('ShellExecutionService', () => {
'bash',
[
'-c',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l',
"trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l",
],
expect.any(Object),
);
Expand Down Expand Up @@ -1049,7 +1049,7 @@ describe('ShellExecutionService', () => {
'bash',
[
'-c',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
],
expect.any(Object),
);
Expand Down Expand Up @@ -1305,7 +1305,7 @@ describe('ShellExecutionService child_process fallback', () => {
'bash',
[
'-c',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l',
"trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l",
],
expect.objectContaining({ shell: false, detached: true }),
);
Expand Down Expand Up @@ -1664,7 +1664,7 @@ describe('ShellExecutionService child_process fallback', () => {
'bash',
[
'-c',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
],
expect.objectContaining({
shell: false,
Expand Down
38 changes: 36 additions & 2 deletions packages/core/src/services/shellExecutionService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
getShellConfiguration,
resolveExecutable,
type ShellType,
BASH_HUP_GUARD,
} from '../utils/shell-utils.js';
import { isBinary, truncateString } from '../utils/textUtils.js';
import pkg from '@xterm/headless';
Expand Down Expand Up @@ -114,6 +115,32 @@ function injectUtf8CodepageForPty(
return command;
}

/**
* Prepends a POSIX SIGHUP-ignore guard to bash commands on non-Windows platforms.
*
* PTY environments such as WSL2, Kitty, and Alacritty aggressively send SIGHUP
* to process groups that lose their controlling terminal. By prepending
* `trap '' HUP;` we apply the same mechanism as the POSIX `nohup` utility:
* SIG_IGN is inherited across exec(), so every child spawned by the command
* also ignores SIGHUP — making the guard genuinely effective even in subshells.
*
* The guard is bash-only and idempotent (won't be doubled if already present).
* It is stripped back out by stripShellWrapper() / stripHupGuard() before any
* sandbox or permission-check logic sees the command, so there is no
* privilege-escalation surface from the preamble itself.
*/
function ensureHupIgnored(command: string, shell: ShellType): string {
if (shell !== 'bash') {
return command;
}
const trimmed = command.trimStart();
const prefix = `${BASH_HUP_GUARD} `;
if (trimmed.startsWith(prefix) || trimmed === BASH_HUP_GUARD) {
return command; // Already guarded — idempotent
}
return `${BASH_HUP_GUARD} ${command}`;
}

/** A structured result from a shell command execution. */
export type ShellExecutionResult = ExecutionResult;

Expand Down Expand Up @@ -450,9 +477,16 @@ export class ShellExecutionService {

const resolvedExecutable = resolveExecutable(executable) ?? executable;

const guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell);
const promptGuarded = ensurePromptvarsDisabled(commandToExecute, shell);
// Prepend the SIGHUP-ignore guard for bash on non-Windows. This uses the
// same mechanism as POSIX `nohup`: SIG_IGN is inherited across exec(), so
// child processes spawned by the command also ignore SIGHUP. The guard is
// stripped by stripShellWrapper() before any sandbox permission checks.
const hupGuarded = !isWindows
? ensureHupIgnored(promptGuarded, shell)
: promptGuarded;
const finalCommand = injectUtf8CodepageForPty(
guardedCommand,
hupGuarded,
shell,
isWindows,
usingPty,
Expand Down
57 changes: 57 additions & 0 deletions packages/core/src/utils/shell-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ import {
parseCommandDetails,
splitCommands,
stripShellWrapper,
stripHupGuard,
BASH_HUP_GUARD,
normalizeCommand,
hasRedirection,
resolveExecutable,
Expand Down Expand Up @@ -646,3 +648,58 @@ describe('resolveExecutable', () => {
expect(resolveExecutable('anything')).toBeUndefined();
});
});

describe('stripHupGuard', () => {
it('should remove our own HUP guard prefix from the start of a command', () => {
expect(stripHupGuard(`${BASH_HUP_GUARD} git status`)).toBe('git status');
});

it('should be idempotent: if no guard present, return command unchanged', () => {
expect(stripHupGuard('git status')).toBe('git status');
});

it('should return empty string when command is exactly the guard itself', () => {
expect(stripHupGuard(BASH_HUP_GUARD)).toBe('');
});

it('should handle leading whitespace before the guard', () => {
expect(stripHupGuard(` ${BASH_HUP_GUARD} git status`)).toBe('git status');
});

// Security regression: must NOT strip user-supplied trap commands
it('should NOT strip a user-supplied trap command (only strips our exact preamble)', () => {
// A user attempting to use trap as a sandbox bypass
const maliciousCmd = `trap 'rm -rf /' EXIT; git status`;
// stripHupGuard looks for the exact BASH_HUP_GUARD prefix (trap '' HUP;), not 'trap'
expect(stripHupGuard(maliciousCmd)).toBe(maliciousCmd);
});

it('should NOT strip a trap command with different arguments', () => {
const cmd = `trap '' TERM; git status`;
expect(stripHupGuard(cmd)).toBe(cmd);
});
});

describe('stripShellWrapper (with HUP guard integration)', () => {
it('should strip bash -c wrapper AND then the HUP guard preamble', () => {
const wrapped = `bash -c "${BASH_HUP_GUARD} git status"`;
expect(stripShellWrapper(wrapped)).toBe('git status');
});

it('should strip the HUP guard alone when no shell wrapper', () => {
expect(stripShellWrapper(`${BASH_HUP_GUARD} git status`)).toBe(
'git status',
);
});

it('should leave user-supplied trap commands intact (security check)', () => {
// This ensures that a user command starting with a different trap
// is not incorrectly stripped by the HUP guard removal logic
const maliciousCmd = `trap 'rm -rf /' EXIT; git status`;
expect(stripShellWrapper(maliciousCmd)).toBe(maliciousCmd);
});

it('should still strip a plain shell wrapper without HUP guard', () => {
expect(stripShellWrapper('sh -c "ls -l"')).toEqual('ls -l');
});
});
47 changes: 43 additions & 4 deletions packages/core/src/utils/shell-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,40 @@ import {
type SpawnOptionsWithoutStdio,
} from 'node:child_process';

/**
* The exact HUP-signal guard preamble injected by ensureHupIgnored().
* Exported so shellExecutionService and stripShellWrapper stay in sync.
*/
export const BASH_HUP_GUARD = `trap '' HUP;`;

/**
* Strips the SIGHUP guard prepended by ensureHupIgnored() from a command.
*
* This is intentionally narrow: it only removes the exact literal string
* `trap '' HUP; ` from the very start of a command. It does NOT
* skip or ignore arbitrary user-supplied `trap` commands, which would be a
* sandbox-bypass vector (e.g., `trap 'rm -rf /' EXIT; git status`).
*/
export function stripHupGuard(command: string): string {
const trimmed = command.trimStart();
const prefix = `${BASH_HUP_GUARD} `;
if (trimmed.startsWith(prefix)) {
return trimmed.slice(prefix.length);
}
// Handle case where there's no trailing space (e.g., guard is the whole command)
if (trimmed === BASH_HUP_GUARD) {
return '';
}
return command;
}

/**
* Extracts the primary command name from a potentially wrapped shell command.
* Strips shell wrappers and handles shopt/set/etc.
* Strips shell wrappers (including our own HUP guard) and handles shopt/set/etc.
*
* Returns the command name only when there is exactly ONE non-builtin root so
* that chained commands (e.g. `git; malicious_cmd`) never silently inherit the
* first command's sandbox permissions.
*
* @param command - The full command string.
* @param args - The arguments for the command.
Expand All @@ -32,7 +63,10 @@ export async function getCommandName(
const roots = getCommandRoots(stripped).filter(
(r) => r !== 'shopt' && r !== 'set',
);
if (roots.length > 0) {
// Single-root enforcement: only grant named-command permissions when the
// command is unambiguous. Multi-root chains fall back to basename so that
// `git; malicious_cmd` never inherits `git`'s sandbox policy.
if (roots.length === 1) {
return roots[0];
}
return path.basename(command);
Expand Down Expand Up @@ -838,6 +872,7 @@ export function stripShellWrapper(command: string): string {
const pattern =
/^\s*(?:(?:(?:\S+\/)?(?:sh|bash|zsh))\s+-c|cmd\.exe\s+\/c|powershell(?:\.exe)?\s+(?:-NoProfile\s+)?-Command|pwsh(?:\.exe)?\s+(?:-NoProfile\s+)?-Command)\s+/i;
const match = command.match(pattern);
let result: string;
if (match) {
let newCommand = command.substring(match[0].length).trim();
if (
Expand All @@ -846,9 +881,13 @@ export function stripShellWrapper(command: string): string {
) {
newCommand = newCommand.substring(1, newCommand.length - 1);
}
return newCommand;
result = newCommand;
} else {
result = command.trim();
}
return command.trim();
// Peel off the SIGHUP guard that ensureHupIgnored() prepends so that
// sandbox managers see the actual user command, not our preamble.
return stripHupGuard(result);
}

/**
Expand Down
Loading