From fe14e1db85ad1b85405cb7b1d8ab8bcc4ed2eb89 Mon Sep 17 00:00:00 2001 From: DennisYu07 <617072224@qq.com> Date: Thu, 19 Mar 2026 19:54:28 +0800 Subject: [PATCH] reslove comment --- .../prompt-processors/shellProcessor.ts | 16 ++- .../src/extension/claude-converter.test.ts | 75 ---------- .../core/src/extension/extensionManager.ts | 3 +- packages/core/src/extension/variables.test.ts | 49 +++++++ packages/core/src/utils/shell-utils.test.ts | 134 ++++++++++++++++++ packages/core/src/utils/shell-utils.ts | 42 ++++++ 6 files changed, 239 insertions(+), 80 deletions(-) diff --git a/packages/cli/src/services/prompt-processors/shellProcessor.ts b/packages/cli/src/services/prompt-processors/shellProcessor.ts index b4201d2fd..0ebf9fa88 100644 --- a/packages/cli/src/services/prompt-processors/shellProcessor.ts +++ b/packages/cli/src/services/prompt-processors/shellProcessor.ts @@ -12,6 +12,7 @@ import { getShellConfiguration, ShellExecutionService, flatMapTextParts, + checkArgumentSafety, } from '@qwen-code/qwen-code-core'; import type { AnyToolInvocation } from '@qwen-code/qwen-code-core'; @@ -101,6 +102,16 @@ export class ShellProcessor implements IPromptProcessor { const { shell } = getShellConfiguration(); const userArgsEscaped = escapeShellArg(userArgsRaw, shell); + // Check safety of the value that will be used for $ARGUMENTS (after removing outer quotes) + let userArgsForArgumentsPlaceholder = userArgsRaw.replace( + /^'([\s\S]*?)'$/, + '$1', + ); + const argumentSafety = checkArgumentSafety(userArgsForArgumentsPlaceholder); + if (!argumentSafety.isSafe) { + userArgsForArgumentsPlaceholder = userArgsEscaped; + } + const resolvedInjections: ResolvedShellInjection[] = injections.map( (injection) => { const command = injection.content; @@ -111,10 +122,7 @@ export class ShellProcessor implements IPromptProcessor { const resolvedCommand = command .replaceAll(SHORTHAND_ARGS_PLACEHOLDER, userArgsEscaped) // Replace {{args}} - .replaceAll( - '$ARGUMENTS', - userArgsRaw.replace(/^'([\s\S]*?)'$/, '$1'), - ); + .replaceAll('$ARGUMENTS', userArgsForArgumentsPlaceholder); return { ...injection, resolvedCommand }; }, ); diff --git a/packages/core/src/extension/claude-converter.test.ts b/packages/core/src/extension/claude-converter.test.ts index 8a47a2919..5a251ce26 100644 --- a/packages/core/src/extension/claude-converter.test.ts +++ b/packages/core/src/extension/claude-converter.test.ts @@ -511,66 +511,6 @@ describe('convertClaudePluginPackage', () => { // Clean up converted directory fs.rmSync(result.convertedDir, { recursive: true, force: true }); }); - - it('should handle hooks defined directly in marketplace config', async () => { - // Setup: Create a plugin with hooks defined directly in marketplace config - const pluginSourceDir = path.join(testDir, 'direct-hooks-plugin'); - fs.mkdirSync(pluginSourceDir, { recursive: true }); - - // Create marketplace.json with hooks defined directly - const marketplaceDir = path.join(pluginSourceDir, '.claude-plugin'); - fs.mkdirSync(marketplaceDir, { recursive: true }); - - const marketplaceConfig: ClaudeMarketplaceConfig = { - name: 'test-marketplace', - owner: { name: 'Test Owner', email: 'test@example.com' }, - plugins: [ - { - name: 'direct-hooks-plugin', - version: '1.0.0', - source: './', - strict: false, - hooks: { - PreToolUse: [ - { - matcher: '*', // Part of HookDefinition - sequential: true, // Part of HookDefinition - hooks: [ - // HookConfig[] array inside HookDefinition - { - type: HookType.Command, - command: 'npm install', - }, - ], - }, - ], - }, - }, - ], - }; - - fs.writeFileSync( - path.join(marketplaceDir, 'marketplace.json'), - JSON.stringify(marketplaceConfig, null, 2), - 'utf-8', - ); - - // Execute: Convert the plugin - const result = await convertClaudePluginPackage( - pluginSourceDir, - 'direct-hooks-plugin', - ); - - // Verify: The converted config should contain the hooks - expect(result.config.hooks).toBeDefined(); - expect(result.config.hooks!['PreToolUse']).toHaveLength(1); - expect(result.config.hooks!['PreToolUse']![0].hooks![0].command).toBe( - 'npm install', - ); - - // Clean up converted directory - fs.rmSync(result.convertedDir, { recursive: true, force: true }); - }); }); describe('performVariableReplacement for Claude extensions', () => { @@ -586,21 +526,6 @@ describe('performVariableReplacement for Claude extensions', () => { } }); - it('should replace ${CLAUDE_PLUGIN_ROOT} in markdown files', () => { - const extDir = path.join(testDir, 'ext-md'); - fs.mkdirSync(extDir, { recursive: true }); - - const mdContent = `# Test Extension - Run \`\${CLAUDE_PLUGIN_ROOT}/scripts/setup.sh\` to configure.`; - fs.writeFileSync(path.join(extDir, 'README.md'), mdContent, 'utf-8'); - - performVariableReplacement(extDir); - - const result = fs.readFileSync(path.join(extDir, 'README.md'), 'utf-8'); - expect(result).toContain(`${extDir}/scripts/setup.sh`); - expect(result).not.toContain('${CLAUDE_PLUGIN_ROOT}'); - }); - it('should replace .claude with .qwen in shell scripts', () => { const extDir = path.join(testDir, 'ext-sh'); fs.mkdirSync(extDir, { recursive: true }); diff --git a/packages/core/src/extension/extensionManager.ts b/packages/core/src/extension/extensionManager.ts index e26f14962..bd29d3650 100644 --- a/packages/core/src/extension/extensionManager.ts +++ b/packages/core/src/extension/extensionManager.ts @@ -989,7 +989,8 @@ export class ExtensionManager { } // Perform variable replacement in extension files (e.g., ${CLAUDE_PLUGIN_ROOT}) for Claude extensions - if (originSource === 'Claude') { + const hooksDir = path.join(destinationPath, 'hooks'); + if (originSource === 'Claude' || fs.existsSync(hooksDir)) { performVariableReplacement(destinationPath); } diff --git a/packages/core/src/extension/variables.test.ts b/packages/core/src/extension/variables.test.ts index 0f9ec06da..685a70064 100644 --- a/packages/core/src/extension/variables.test.ts +++ b/packages/core/src/extension/variables.test.ts @@ -406,4 +406,53 @@ describe('performVariableReplacement', () => { 'content', ); }); + + describe('regex boundary cases', () => { + it('should not replace incomplete variable syntax (missing brace) in markdown', () => { + const extDir = path.join(testDir, 'ext-incomplete'); + fs.mkdirSync(extDir, { recursive: true }); + + // Note: performVariableReplacement only processes .md files + const content = 'Path: $CLAUDE_PLUGIN_ROOT/config.json'; + fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8'); + + performVariableReplacement(extDir); + + const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8'); + // Should remain unchanged (no braces) + expect(result).toBe('Path: $CLAUDE_PLUGIN_ROOT/config.json'); + }); + + it('should replace double dollar sign but keep first dollar', () => { + const extDir = path.join(testDir, 'ext-double-dollar'); + fs.mkdirSync(extDir, { recursive: true }); + + // Note: performVariableReplacement only processes .md files + // The regex matches ${CLAUDE_PLUGIN_ROOT}, leaving first $ intact + const content = 'Path: $${CLAUDE_PLUGIN_ROOT}/config.json'; + fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8'); + + performVariableReplacement(extDir); + + const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8'); + // First $ is preserved, variable is replaced + expect(result).toBe(`Path: $${extDir}/config.json`); + }); + + it('should replace variable in markdown comments', () => { + const extDir = path.join(testDir, 'ext-comment'); + fs.mkdirSync(extDir, { recursive: true }); + + // Comments in markdown files should be processed + const content = '# TODO: Update ${CLAUDE_PLUGIN_ROOT} later'; + fs.writeFileSync(path.join(extDir, 'test.md'), content, 'utf-8'); + + performVariableReplacement(extDir); + + const result = fs.readFileSync(path.join(extDir, 'test.md'), 'utf-8'); + // Should be replaced (comments in markdown are still processed) + expect(result).toContain(extDir); + expect(result).not.toContain('${CLAUDE_PLUGIN_ROOT}'); + }); + }); }); diff --git a/packages/core/src/utils/shell-utils.test.ts b/packages/core/src/utils/shell-utils.test.ts index b974bfd5a..8cac096a8 100644 --- a/packages/core/src/utils/shell-utils.test.ts +++ b/packages/core/src/utils/shell-utils.test.ts @@ -6,6 +6,7 @@ import { expect, describe, it, beforeEach, vi, afterEach } from 'vitest'; import { + checkArgumentSafety, checkCommandPermissions, escapeShellArg, getCommandRoots, @@ -607,3 +608,136 @@ describe('isCommandNeedPermission', () => { expect(result.reason).toContain('requires permission to execute'); }); }); + +describe('checkArgumentSafety', () => { + describe('command substitution patterns', () => { + it('should detect $() command substitution', () => { + const result = checkArgumentSafety('$(whoami)'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('$() command substitution'); + }); + + it('should detect backtick command substitution', () => { + const result = checkArgumentSafety('`whoami`'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain( + 'backtick command substitution', + ); + }); + + it('should detect <() process substitution', () => { + const result = checkArgumentSafety('<(cat file)'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('<() process substitution'); + }); + + it('should detect >() process substitution', () => { + const result = checkArgumentSafety('>(tee file)'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('>() process substitution'); + }); + }); + + describe('command separators', () => { + it('should detect semicolon separator', () => { + const result = checkArgumentSafety('arg1; rm -rf /'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('; command separator'); + }); + + it('should detect pipe', () => { + const result = checkArgumentSafety('arg1 | cat file'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('| pipe'); + }); + + it('should detect && operator', () => { + const result = checkArgumentSafety('arg1 && ls'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('&& AND operator'); + }); + + it('should detect || operator', () => { + const result = checkArgumentSafety('arg1 || ls'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('|| OR operator'); + }); + }); + + describe('background execution', () => { + it('should detect background operator', () => { + const result = checkArgumentSafety('arg1 & ls'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('& background operator'); + }); + }); + + describe('input/output redirection', () => { + it('should detect output redirection', () => { + const result = checkArgumentSafety('arg1 > file'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('> output redirection'); + }); + + it('should detect input redirection', () => { + const result = checkArgumentSafety('arg1 < file'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('< input redirection'); + }); + + it('should detect append redirection', () => { + const result = checkArgumentSafety('arg1 >> file'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('> output redirection'); + }); + }); + + describe('safe inputs', () => { + it('should accept simple arguments', () => { + const result = checkArgumentSafety('arg1 arg2'); + expect(result.isSafe).toBe(true); + expect(result.dangerousPatterns).toHaveLength(0); + }); + + it('should accept arguments with numbers', () => { + const result = checkArgumentSafety('file123.txt'); + expect(result.isSafe).toBe(true); + }); + + it('should accept arguments with hyphens', () => { + const result = checkArgumentSafety('--flag=value'); + expect(result.isSafe).toBe(true); + }); + + it('should accept arguments with underscores', () => { + const result = checkArgumentSafety('my_file_name'); + expect(result.isSafe).toBe(true); + }); + + it('should accept arguments with dots', () => { + const result = checkArgumentSafety('path/to/file.txt'); + expect(result.isSafe).toBe(true); + }); + + it('should accept empty string', () => { + const result = checkArgumentSafety(''); + expect(result.isSafe).toBe(true); + }); + + it('should accept arguments with spaces (quoted)', () => { + const result = checkArgumentSafety('hello world'); + expect(result.isSafe).toBe(true); + }); + }); + + describe('multiple dangerous patterns', () => { + it('should detect multiple dangerous patterns', () => { + const result = checkArgumentSafety('$(whoami); rm -rf / &'); + expect(result.isSafe).toBe(false); + expect(result.dangerousPatterns).toContain('$() command substitution'); + expect(result.dangerousPatterns).toContain('; command separator'); + expect(result.dangerousPatterns).toContain('& background operator'); + expect(result.dangerousPatterns).toHaveLength(3); + }); + }); +}); diff --git a/packages/core/src/utils/shell-utils.ts b/packages/core/src/utils/shell-utils.ts index 1f0476866..684fa91c1 100644 --- a/packages/core/src/utils/shell-utils.ts +++ b/packages/core/src/utils/shell-utils.ts @@ -913,3 +913,45 @@ export function isCommandNeedsPermission(command: string): { reason: 'Command requires permission to execute.', }; } + +/** + * Checks user arguments for potentially dangerous shell characters. + * This is used to validate arguments before they are substituted into + * shell command templates (e.g., $ARGUMENTS placeholder). + * + * Note: This does NOT remove outer quotes - it validates the raw input. + * Use escapeShellArg() for safe shell argument escaping. + * + * @param args - The raw user arguments string + * @returns Object with isSafe flag and list of dangerous patterns found + */ +export function checkArgumentSafety(args: string): { + isSafe: boolean; + dangerousPatterns: string[]; +} { + const dangerousPatterns: string[] = []; + + // Command substitution patterns + if (/\$\(/.test(args)) dangerousPatterns.push('$() command substitution'); + if (/`/.test(args)) dangerousPatterns.push('backtick command substitution'); + if (/<\(/.test(args)) dangerousPatterns.push('<() process substitution'); + if (/>\(/.test(args)) dangerousPatterns.push('>() process substitution'); + + // Command separators (outside of quotes) + if (/;/.test(args)) dangerousPatterns.push('; command separator'); + if (/\|/.test(args)) dangerousPatterns.push('| pipe'); + if (/&&/.test(args)) dangerousPatterns.push('&& AND operator'); + if (/\|\|/.test(args)) dangerousPatterns.push('|| OR operator'); + + // Background execution (space + &, with optional surrounding) + if (/\s+&/.test(args)) dangerousPatterns.push('& background operator'); + + // Input/Output redirection + if (/>\s+|\d*>/.test(args)) dangerousPatterns.push('> output redirection'); + if (/<\s+|\d*