Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 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
41 changes: 36 additions & 5 deletions src/core/Cline.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,16 @@ type UserContent = Array<
Anthropic.TextBlockParam | Anthropic.ImageBlockParam | Anthropic.ToolUseBlockParam | Anthropic.ToolResultBlockParam
>

// Add near the top of the file, after imports:
const ALLOWED_AUTO_EXECUTE_COMMANDS = [
'npm',
'npx',
'tsc',
'git log',
'git diff',
'list'
] as const

export class Cline {
readonly taskId: string
api: ApiHandler
Expand Down Expand Up @@ -124,6 +134,25 @@ export class Cline {
}
}

protected isAllowedCommand(command?: string): boolean {
if (!command) {
return false;
}
// Check for command chaining characters
if (command.includes('&&') ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential false positive: Commands with arguments containing blocked characters\n\nThe command validation checks the entire command string for blocked characters, which could reject legitimate commands:\n\ntypescript\nif (command.includes('&&') ||\n command.includes(';') ||\n command.includes('||') ||\n command.includes('|') ||\n command.includes('$(') ||\n command.includes('`')) {\n return false;\n}\n\n\nScenario: A user wants to run:\nbash\ngit log --format='%h | %s'\ngit log --grep='fix && update'\nnpm run build -- --env.API_URL='https://api.example.com?foo=1&bar=2'\n\n\nThese commands contain |, &&, or & within quoted strings or as part of URLs, but they are NOT command injection vectors. However, the current implementation would block them.\n\nImpact: Medium - May block legitimate use cases\n\nRecommendation: Consider more sophisticated parsing that:\n1. Respects quoted strings (single and double quotes)\n2. Only checks for operators outside of quotes\n3. Or document this limitation and suggest users use environment variables or config files for complex arguments

command.includes(';') ||
command.includes('||') ||
command.includes('|') ||
command.includes('$(') ||
command.includes('`')) {
return false;
}
const trimmedCommand = command.trim().toLowerCase();
return ALLOWED_AUTO_EXECUTE_COMMANDS.some(prefix =>
trimmedCommand.startsWith(prefix.toLowerCase())
);
}

// Storing task to disk for history

private async ensureTaskDirectoryExists(): Promise<string> {
Expand Down Expand Up @@ -555,8 +584,8 @@ export class Cline {
: [{ type: "text", text: lastMessage.content }]
if (previousAssistantMessage && previousAssistantMessage.role === "assistant") {
const assistantContent = Array.isArray(previousAssistantMessage.content)
? previousAssistantMessage.content
: [{ type: "text", text: previousAssistantMessage.content }]
? previousAssistantMessage.content
: [{ type: "text", text: previousAssistantMessage.content }]

const toolUseBlocks = assistantContent.filter(
(block) => block.type === "tool_use"
Expand Down Expand Up @@ -839,7 +868,7 @@ export class Cline {
// (have to do this for partial and complete since sending content in thinking tags to markdown renderer will automatically be removed)
// Remove end substrings of <thinking or </thinking (below xml parsing is only for opening tags)
// (this is done with the xml parsing below now, but keeping here for reference)
// content = content.replace(/<\/?t(?:h(?:i(?:n(?:k(?:i(?:n(?:g)?)?)?)?)?)?)?$/, "")
// content = content.replace(/<\/?t(?:h(?:i(?:n(?:k(?:i(?:n(?:g)?)?)?)?)?$/, "")
// Remove all instances of <thinking> (with optional line break after) and </thinking> (with optional line break before)
// - Needs to be separate since we dont want to remove the line break before the first tag
// - Needs to happen before the xml parsing below
Expand Down Expand Up @@ -1503,7 +1532,7 @@ export class Cline {
const command: string | undefined = block.params.command
try {
if (block.partial) {
if (this.alwaysAllowExecute) {
if (this.alwaysAllowExecute && this.isAllowedCommand(command)) {
await this.say("command", command, undefined, block.partial)
} else {
await this.ask("command", removeClosingTag("command", command), block.partial).catch(
Expand All @@ -1520,7 +1549,9 @@ export class Cline {
break
}
this.consecutiveMistakeCount = 0
const didApprove = this.alwaysAllowExecute || (await askApproval("command", command))

const didApprove = (this.alwaysAllowExecute && this.isAllowedCommand(command)) ||
(await askApproval("command", command))
if (!didApprove) {
break
}
Expand Down
90 changes: 89 additions & 1 deletion src/core/__tests__/Cline.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,5 +318,93 @@ describe('Cline', () => {
expect(writeDisabledCline.alwaysAllowWrite).toBe(false);
// The write operation would require approval in actual implementation
});
});
});

describe('isAllowedCommand', () => {
let cline: any

beforeEach(() => {
// Create a more complete mock provider
const mockProvider = {
context: {
globalStorageUri: { fsPath: '/mock/path' }
},
postStateToWebview: jest.fn(),
postMessageToWebview: jest.fn(),
updateTaskHistory: jest.fn()
}

// Mock the required dependencies
const mockApiConfig = {
getModel: () => ({
id: 'claude-3-sonnet',
info: { supportsComputerUse: true }
})
}

// Create test instance with mocked constructor params
cline = new Cline(
mockProvider as any,
mockApiConfig as any,
undefined, // customInstructions
false, // alwaysAllowReadOnly
false, // alwaysAllowWrite
false, // alwaysAllowExecute
'test task' // task
)

// Mock internal methods that are called during initialization
cline.initiateTaskLoop = jest.fn()
cline.say = jest.fn()
cline.addToClineMessages = jest.fn()
cline.overwriteClineMessages = jest.fn()
cline.addToApiConversationHistory = jest.fn()
cline.overwriteApiConversationHistory = jest.fn()
})

test('returns true for allowed commands', () => {
expect(cline.isAllowedCommand('npm install')).toBe(true)
expect(cline.isAllowedCommand('npx create-react-app')).toBe(true)
expect(cline.isAllowedCommand('tsc --watch')).toBe(true)
expect(cline.isAllowedCommand('git log --oneline')).toBe(true)
expect(cline.isAllowedCommand('git diff main')).toBe(true)
})

test('returns true regardless of case or whitespace', () => {
expect(cline.isAllowedCommand('NPM install')).toBe(true)
expect(cline.isAllowedCommand(' npm install')).toBe(true)
expect(cline.isAllowedCommand('GIT DIFF')).toBe(true)
})

test('returns false for non-allowed commands', () => {
expect(cline.isAllowedCommand('rm -rf /')).toBe(false)
expect(cline.isAllowedCommand('git push')).toBe(false)
expect(cline.isAllowedCommand('git commit')).toBe(false)
expect(cline.isAllowedCommand('curl http://example.com')).toBe(false)
})

test('returns false for undefined or empty commands', () => {
expect(cline.isAllowedCommand()).toBe(false)
expect(cline.isAllowedCommand('')).toBe(false)
expect(cline.isAllowedCommand(' ')).toBe(false)
})

test('returns false for commands with chaining operators', () => {
const maliciousCommands = [
'npm install && rm -rf /',
'git status; dangerous-command',
'git log || evil-script',
'git status | malicious-pipe',
'git log $(evil-command)',
'git status `rm -rf /`',
'npm install && echo "malicious"',
'git status; curl http://evil.com',
'tsc --watch || wget malware',
];

maliciousCommands.forEach(cmd => {
expect(cline.isAllowedCommand(cmd)).toBe(false);
});
});
})
});