clone/gemini-cli
1
0
Fork 0
mirror of https://github.com/google-gemini/gemini-cli.git synced 2026-05-25 13:44:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

Revert "fix(core): prevent SIGHUP kills in PTY environments" (#27401)

Browse Source
This commit is contained in:
Billy Biggs
2026-05-23 16:52:45 -04:00
committed by GitHub
parent a00d03efc6
commit 3cc7e5b096
5 changed files with 11 additions and 145 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
packages/core/src/sandbox/utils/commandUtils.ts
View File

@@ -56,11 +56,7 @@ export async function getCommandName(req: SandboxRequest): Promise<string> {
const roots = getCommandRoots(stripped).filter(
(r) => r !== 'shopt' && r !== 'set',
);
// 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) {
if (roots.length > 0) {
return roots[0];
}
return path.basename(req.command);

8
packages/core/src/services/shellExecutionService.test.ts
View File

@@ -289,7 +289,7 @@ describe('ShellExecutionService', () => {
'bash',
[
'-c',
"trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l",
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l',
],
expect.any(Object),
);
@@ -1057,7 +1057,7 @@ describe('ShellExecutionService', () => {
'bash',
[
'-c',
'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
],
expect.objectContaining({
handleFlowControl: true,
@@ -1315,7 +1315,7 @@ describe('ShellExecutionService child_process fallback', () => {
'bash',
[
'-c',
"trap '' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l",
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls -l',
],
expect.objectContaining({ shell: false, detached: true }),
);
@@ -1674,7 +1674,7 @@ describe('ShellExecutionService child_process fallback', () => {
'bash',
[
'-c',
'trap \'\' HUP; shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
'shopt -u promptvars nullglob extglob nocaseglob dotglob; ls "foo bar"',
],
expect.objectContaining({
shell: false,

38
packages/core/src/services/shellExecutionService.ts
View File

@@ -17,7 +17,6 @@ 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';
@@ -115,32 +114,6 @@ 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;
@@ -477,16 +450,9 @@ export class ShellExecutionService {
const resolvedExecutable = resolveExecutable(executable) ?? executable;
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 guardedCommand = ensurePromptvarsDisabled(commandToExecute, shell);
const finalCommand = injectUtf8CodepageForPty(
hupGuarded,
guardedCommand,
shell,
isWindows,
usingPty,

57
packages/core/src/utils/shell-utils.test.ts
View File

@@ -21,8 +21,6 @@ import {
parseCommandDetails,
splitCommands,
stripShellWrapper,
stripHupGuard,
BASH_HUP_GUARD,
normalizeCommand,
hasRedirection,
resolveExecutable,
@@ -668,58 +666,3 @@ 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
packages/core/src/utils/shell-utils.ts
View File

@@ -14,40 +14,9 @@ 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 (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.
* Strips shell wrappers and handles shopt/set/etc.
*
* @param command - The full command string.
* @param args - The arguments for the command.
@@ -63,10 +32,7 @@ export async function getCommandName(
const roots = getCommandRoots(stripped).filter(
(r) => r !== 'shopt' && r !== 'set',
);
// 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) {
if (roots.length > 0) {
return roots[0];
}
return path.basename(command);
@@ -877,7 +843,6 @@ 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 (
@@ -886,13 +851,9 @@ export function stripShellWrapper(command: string): string {
) {
newCommand = newCommand.substring(1, newCommand.length - 1);
}
result = newCommand;
} else {
result = command.trim();
return newCommand;
}
// Peel off the SIGHUP guard that ensureHupIgnored() prepends so that
// sandbox managers see the actual user command, not our preamble.
return stripHupGuard(result);
return command.trim();
}
/**