fix(shell): address rcFile security issues

- Only source rcFile if the workspace is a trusted folder

- Properly escape the rcFile path to prevent command injection
This commit is contained in:
jacob314
2026-04-21 15:46:16 -07:00
parent 66abea7a0e
commit 413098e03a
2 changed files with 68 additions and 8 deletions
+48 -2
View File
@@ -165,6 +165,7 @@ describe('ShellTool', () => {
addSessionApproval: vi.fn(),
},
getShellToolRcFile: vi.fn().mockReturnValue(undefined),
isTrustedFolder: vi.fn().mockReturnValue(true),
} as unknown as Config;
const bus = createMockMessageBus();
@@ -487,9 +488,10 @@ EOF`;
expect(mockShellExecutionService.mock.calls[0][0]).toMatch(/\nEOF\n\)\n/);
});
it('should source rcfile when shellToolRcFile setting is present', async () => {
it('should source rcfile when shellToolRcFile setting is present and folder is trusted', async () => {
const rcFilePath = '~/.geminirc';
(mockConfig.getShellToolRcFile as Mock).mockReturnValue(rcFilePath);
(mockConfig.isTrustedFolder as Mock).mockReturnValue(true);
const invocation = shellTool.build({ command: 'my-command' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
@@ -497,7 +499,51 @@ EOF`;
await promise;
expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringContaining(`source ${rcFilePath} && my-command`),
expect.stringContaining(
`source '${rcFilePath.replace('~', '/home/user')}'; export PAGER=cat`,
),
expect.any(String),
expect.any(Function),
expect.any(AbortSignal),
false,
expect.any(Object),
);
});
it('should NOT source rcfile when shellToolRcFile setting is present but folder is untrusted', async () => {
const rcFilePath = '~/.geminirc';
(mockConfig.getShellToolRcFile as Mock).mockReturnValue(rcFilePath);
(mockConfig.isTrustedFolder as Mock).mockReturnValue(false);
const invocation = shellTool.build({ command: 'my-command' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;
expect(mockShellExecutionService).not.toHaveBeenCalledWith(
expect.stringContaining(`source '${rcFilePath}'`),
expect.any(String),
expect.any(Function),
expect.any(AbortSignal),
false,
expect.any(Object),
);
});
it('should properly escape quotes in rcFilePath', async () => {
const rcFilePath = "/path/with/'quotes'/rc";
(mockConfig.getShellToolRcFile as Mock).mockReturnValue(rcFilePath);
(mockConfig.isTrustedFolder as Mock).mockReturnValue(true);
const invocation = shellTool.build({ command: 'my-command' });
const promise = invocation.execute({ abortSignal: mockAbortSignal });
resolveShellExecution();
await promise;
expect(mockShellExecutionService).toHaveBeenCalledWith(
expect.stringContaining(
`source '/path/with/'\\''quotes'\\''/rc'; export PAGER=cat`,
),
expect.any(String),
expect.any(Function),
expect.any(AbortSignal),
+20 -6
View File
@@ -50,7 +50,12 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js';
import { getShellDefinition } from './definitions/coreTools.js';
import { resolveToolDeclaration } from './definitions/resolver.js';
import type { AgentLoopContext } from '../config/agent-loop-context.js';
import { toPathKey, isSubpath, resolveToRealPath } from '../utils/paths.js';
import {
homedir,
toPathKey,
isSubpath,
resolveToRealPath,
} from '../utils/paths.js';
import {
getProactiveToolSuggestions,
isNetworkReliantCommand,
@@ -463,11 +468,20 @@ export class ShellToolInvocation extends BaseToolInvocation<
const onAbort = () => combinedController.abort();
let strippedCommandWithRc = strippedCommand;
const rcFilePath = this.context.config.getShellToolRcFile();
if (rcFilePath) {
strippedCommandWithRc = `source ${rcFilePath} && ${strippedCommand}`;
} else if (!isWindows) {
strippedCommandWithRc = `export PAGER=cat GIT_PAGER=cat; more() { cat "$@"; }; less() { cat "$@"; }; ${strippedCommand}`;
if (!isWindows) {
strippedCommandWithRc = `export PAGER=cat GIT_PAGER=cat; ${strippedCommand}`;
const rcFilePath = this.context.config.getShellToolRcFile();
if (rcFilePath && this.context.config.isTrustedFolder()) {
let resolvedRcFilePath = rcFilePath;
if (rcFilePath === '~' || rcFilePath.startsWith('~/')) {
resolvedRcFilePath = homedir() + rcFilePath.substring(1);
}
const escapedRcFilePath = resolvedRcFilePath.replace(
/'/g,
() => "'\\''",
);
strippedCommandWithRc = `source '${escapedRcFilePath}'; ${strippedCommandWithRc}`;
}
}
try {