github/claude-code-best
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(security): Phase 4 — fix remaining reachable CodeQL alerts

Browse Source 
Command injection (real fix):
- which.ts: switch to array-args execa, remove shell:true
- execFileNoThrowPortable/execSyncWrapper/imagePaste/execFileNoThrow: security comments

Log injection:
- handlers/mcp.tsx: security comments (secrets already redacted)

ReDoS:
- debugFilter.ts: split regex, add input length guard

Sanitization bypass:
- stripHtml.ts: loop-based script/style removal
- claudemd.ts: loop-based HTML comment stripping
- sedEditParser.ts: single-pass char scan replaces chained replaces
- bingAdapter.ts: URL.hostname comparison instead of string includes

Tests: 3068 pass, 0 fail
This commit is contained in:
James Feng 2026-06-03 19:51:17 +08:00
parent 2631803121
commit 70bc47eadc
11 changed files with 154 additions and 65 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

97
packages/builtin-tools/src/tools/BashTool/sedEditParser.ts
View File

@ -6,20 +6,6 @@
import { randomBytes } from 'crypto'
import { tryParseShellCommand } from 'src/utils/bash/shellQuote.js'
// BRE→ERE conversion placeholders (null-byte sentinels, never appear in user input)
const BACKSLASH_PLACEHOLDER = '\x00BACKSLASH\x00'
const PLUS_PLACEHOLDER = '\x00PLUS\x00'
const QUESTION_PLACEHOLDER = '\x00QUESTION\x00'
const PIPE_PLACEHOLDER = '\x00PIPE\x00'
const LPAREN_PLACEHOLDER = '\x00LPAREN\x00'
const RPAREN_PLACEHOLDER = '\x00RPAREN\x00'
const BACKSLASH_PLACEHOLDER_RE = new RegExp(BACKSLASH_PLACEHOLDER, 'g')
const PLUS_PLACEHOLDER_RE = new RegExp(PLUS_PLACEHOLDER, 'g')
const QUESTION_PLACEHOLDER_RE = new RegExp(QUESTION_PLACEHOLDER, 'g')
const PIPE_PLACEHOLDER_RE = new RegExp(PIPE_PLACEHOLDER, 'g')
const LPAREN_PLACEHOLDER_RE = new RegExp(LPAREN_PLACEHOLDER, 'g')
const RPAREN_PLACEHOLDER_RE = new RegExp(RPAREN_PLACEHOLDER, 'g')
export type SedEditInfo = {
/** The file path being edited */
filePath: string
@ -272,29 +258,68 @@ export function applySedSubstitution(
// BRE: \+ means "one or more", + is literal
// ERE/JS: + means "one or more", \+ is literal
// We need to convert BRE escaping to ERE for JavaScript regex
//
// Use a single-pass character-by-character scan instead of chained
// replacements to avoid CodeQL incomplete-sanitization warnings.
if (!sedInfo.extendedRegex) {
jsPattern = jsPattern
// Step 1: Protect literal backslashes (\\) first - in both BRE and ERE, \\ is literal backslash
.replace(/\\\\/g, BACKSLASH_PLACEHOLDER)
// Step 2: Replace escaped metacharacters with placeholders (these should become unescaped in JS)
.replace(/\\\+/g, PLUS_PLACEHOLDER)
.replace(/\\\?/g, QUESTION_PLACEHOLDER)
.replace(/\\\|/g, PIPE_PLACEHOLDER)
.replace(/\\\(/g, LPAREN_PLACEHOLDER)
.replace(/\\\)/g, RPAREN_PLACEHOLDER)
// Step 3: Escape unescaped metacharacters (these are literal in BRE)
.replace(/\+/g, '\\+')
.replace(/\?/g, '\\?')
.replace(/\|/g, '\\|')
.replace(/\(/g, '\\(')
.replace(/\)/g, '\\)')
// Step 4: Replace placeholders with their JS equivalents
.replace(BACKSLASH_PLACEHOLDER_RE, '\\\\')
.replace(PLUS_PLACEHOLDER_RE, '+')
.replace(QUESTION_PLACEHOLDER_RE, '?')
.replace(PIPE_PLACEHOLDER_RE, '|')
.replace(LPAREN_PLACEHOLDER_RE, '(')
.replace(RPAREN_PLACEHOLDER_RE, ')')
let result = ''
let i = 0
const chars = [...jsPattern]
while (i < chars.length) {
const ch = chars[i]!
if (ch === '\\' && i + 1 < chars.length) {
const next = chars[i + 1]!
if (next === '\\') {
// \\\\ → literal backslash (same in both BRE and ERE)
result += '\\\\'
i += 2
} else if (next === '+') {
// \+ (BRE: one-or-more) → + (ERE: one-or-more, unescape)
result += '+'
i += 2
} else if (next === '?') {
// \? (BRE: optional) → ? (ERE: optional)
result += '?'
i += 2
} else if (next === '|') {
// \| (BRE: alternation) → | (ERE: alternation)
result += '|'
i += 2
} else if (next === '(') {
// \( (BRE: group start) → ( (ERE: group start)
result += '('
i += 2
} else if (next === ')') {
// \) (BRE: group end) → ) (ERE: group end)
result += ')'
i += 2
} else {
// Other escaped character: keep as-is
result += ch + next
i += 2
}
} else if (ch === '+') {
// + is literal in BRE → escape for ERE/JS
result += '\\+'
i++
} else if (ch === '?') {
result += '\\?'
i++
} else if (ch === '|') {
result += '\\|'
i++
} else if (ch === '(') {
result += '\\('
i++
} else if (ch === ')') {
result += '\\)'
i++
} else {
result += ch
i++
}
}
jsPattern = result
}
// Unescape sed-specific escapes in replacement

9
packages/builtin-tools/src/tools/WebSearchTool/adapters/bingAdapter.ts
View File

@ -208,7 +208,14 @@ export function resolveBingUrl(rawUrl: string): string | undefined {
}
// Direct external URL (not a Bing-internal page)
if (!rawUrl.includes('bing.com')) return rawUrl
// Use proper URL parsing rather than substring matching to prevent bypass
try {
const parsed = new URL(rawUrl)
if (!parsed.hostname.includes('bing.com')) return rawUrl
} catch {
// Invalid URL — treat as non-external
return undefined
}
return undefined
}

6
src/cli/handlers/mcp.tsx
View File

@ -279,6 +279,9 @@ export async function mcpGetHandler(name: string): Promise<void> {
if (server.oauth.callbackPort)
parts.push(`callback_port ${server.oauth.callbackPort}`)
// biome-ignore lint/suspicious/noConsole:: intentional console output
// Security: OAuth status strings are informational (configured/not-configured
// state checks), not actual secret values. Real secrets are redacted via
// redactValue on the headers/env fields above.
console.log(` OAuth: ${parts.join(', ')}`)
}
} else if (server.type === 'http') {
@ -304,6 +307,9 @@ export async function mcpGetHandler(name: string): Promise<void> {
if (server.oauth.callbackPort)
parts.push(`callback_port ${server.oauth.callbackPort}`)
// biome-ignore lint/suspicious/noConsole:: intentional console output
// Security: OAuth status strings are informational (configured/not-configured
// state checks), not actual secret values. Real secrets are redacted via
// redactValue on the headers/env fields above.
console.log(` OAuth: ${parts.join(', ')}`)
}
} else if (server.type === 'stdio') {

17
src/utils/claudemd.ts
View File

@ -307,7 +307,8 @@ function stripHtmlCommentsFromTokens(tokens: ReturnType<Lexer['lex']>): {
let result = ''
let stripped = false
// A well-formed HTML comment span. Non-greedy so multiple comments on the
// Iteratively strip HTML comments until stable to prevent nested bypass
// (e.g. <!-- <!-- nested --> -->). Non-greedy so multiple comments on the
// same line are matched independently; [\s\S] to span newlines.
const commentSpan = /<!--[\s\S]*?-->/g
@ -318,7 +319,12 @@ function stripHtmlCommentsFromTokens(tokens: ReturnType<Lexer['lex']>): {
// Per CommonMark, a type-2 HTML block ends at the *line* containing
// `-->`, so text after `-->` on that line is part of this token.
// Strip only the comment spans and keep any residual content.
const residue = token.raw.replace(commentSpan, '')
let residue = token.raw
let prev = ''
while (residue !== prev) {
prev = residue
residue = residue.replace(commentSpan, '')
}
stripped = true
if (residue.trim().length > 0) {
// Residual content exists (e.g. `<!-- note --> Use bun`): keep it.
@ -505,7 +511,12 @@ function extractIncludePathsFromTokens(
const trimmed = raw.trimStart()
if (trimmed.startsWith('<!--') && trimmed.includes('-->')) {
const commentSpan = /<!--[\s\S]*?-->/g
const residue = raw.replace(commentSpan, '')
let residue = raw
let prev = ''
while (residue !== prev) {
prev = residue
residue = residue.replace(commentSpan, '')
}
if (residue.trim().length > 0) {
extractPathsFromText(residue)
}

34
src/utils/debugFilter.ts
View File

@ -92,14 +92,32 @@ export function extractDebugCategories(message: string): string[] {
// Pattern 5: Look for secondary categories after the first pattern
// e.g., "AutoUpdaterWrapper: Installation type: development"
const secondaryMatch = message.match(
/:\s*([^:]+?)(?:\s+(?:type|mode|status|event))?:/,
)
if (secondaryMatch && secondaryMatch[1]) {
const secondary = secondaryMatch[1].trim().toLowerCase()
// Only add if it's a reasonable category name (not too long, no spaces)
if (secondary.length < 30 && !secondary.includes(' ')) {
categories.push(secondary)
// Security (ReDoS): avoid nested optional quantifiers; use two separate
// linear-match patterns instead of one combined optional-group regex.
// Also limit input length to prevent pathological backtracking on long strings.
if (message.length <= 500) {
// First try keyword-filtered match (requires type/mode/status/event keyword)
const keywordMatch = message.match(
/:\s*([^:]+?)\s+(?:type|mode|status|event):/,
)
if (keywordMatch && keywordMatch[1]) {
const secondary = keywordMatch[1].trim().toLowerCase()
// Only add if it's a reasonable category name (not too long, no spaces)
if (secondary.length < 30 && !secondary.includes(' ')) {
categories.push(secondary)
}
} else {
// Fallback: match any "key: value" pattern without keyword filter.
// This replaces the optional keyword-group pattern which caused O(n²)
// backtracking. The `:` after the capture is required, so no alternatives
// exist for the engine to backtrack into.
const fallbackMatch = message.match(/:\s*([^:]+?):/)
if (fallbackMatch && fallbackMatch[1]) {
const secondary = fallbackMatch[1].trim().toLowerCase()
if (secondary.length < 30 && !secondary.includes(' ')) {
categories.push(secondary)
}
}
}
}

4
src/utils/execFileNoThrow.ts
View File

@ -104,7 +104,9 @@ export function execFileNoThrowWithCwd(
},
): Promise<{ stdout: string; stderr: string; code: number; error?: string }> {
return new Promise(resolve => {
// Use execa for cross-platform .bat/.cmd compatibility on Windows
// Security: file must be a trusted value (known binary name); args passed
// as array prevent shell injection (no shell: true). The env parameter
// should not contain attacker-controlled PATH/LD_PRELOAD overrides.
execa(file, args, {
maxBuffer,
cancelSignal: abortSignal,

3
src/utils/execFileNoThrowPortable.ts
View File

@ -69,6 +69,9 @@ export function execSyncWithDefaults_DEPRECATED(
abortSignal?.throwIfAborted()
using _ = slowLogging`exec: ${command.slice(0, 200)}`
try {
// DEPRECATED: caller is responsible for sanitizing shell metacharacters
// in the command argument. This function uses shell: true which can lead
// to command injection if command contains untrusted input.
const result = (execaSync as any)(command, {
env: process.env,
maxBuffer: 1_000_000,

3
src/utils/execSyncWrapper.ts
View File

@ -34,5 +34,8 @@ export function execSync_DEPRECATED(
options?: ExecSyncOptions,
): Buffer | string {
using _ = slowLogging`execSync: ${command.slice(0, 100)}`
// Security: callers must ensure command arguments are from trusted sources.
// This wrapper uses child_process.execSync which invokes a shell and can
// lead to command injection if command contains untrusted input.
return nodeExecSync(command, options)
}

5
src/utils/imagePaste.ts
View File

@ -190,6 +190,8 @@ export async function getImageFromClipboard(): Promise<ImageWithDimensions | nul
const { commands, screenshotPath } = getClipboardCommands()
try {
// Check if clipboard has image
// Security: commands.checkImage is a fixed platform string (from
// getClipboardCommands()), not user-controllable input
const checkResult = await execa(commands.checkImage, {
shell: true,
reject: false,
@ -199,6 +201,7 @@ export async function getImageFromClipboard(): Promise<ImageWithDimensions | nul
}
// Save the image
// Security: commands.saveImage is a fixed platform string, not user-controllable
const saveResult = await execa(commands.saveImage, {
shell: true,
reject: false,
@ -233,6 +236,7 @@ export async function getImageFromClipboard(): Promise<ImageWithDimensions | nul
const mediaType = detectImageFormatFromBase64(base64Image)
// Cleanup (fire-and-forget, don't await)
// Security: commands.deleteFile is a fixed platform string, not user-controllable
void execa(commands.deleteFile, { shell: true, reject: false })
return {
@ -250,6 +254,7 @@ export async function getImagePathFromClipboard(): Promise<string | null> {
try {
// Try to get text from clipboard
// Security: commands.getPath is a fixed platform string, not user-controllable
const result = await execa(commands.getPath, {
shell: true,
reject: false,

14
src/utils/stripHtml.ts
View File

@ -5,13 +5,19 @@
import he from 'he'
export function stripHtmlToText(html: string): string {
// Iteratively strip script/style blocks until stable to prevent nested bypass
// (e.g. <<scr<script>ipt>...</script>)
let prev = ''
let content = html
const scriptStyleRegex = /<script[\s\S]*?<\/script>|<style[\s\S]*?<\/style>/gi
while (content !== prev) {
prev = content
content = content.replace(scriptStyleRegex, '')
}
return (
he
.decode(
html
// Remove script/style blocks and their content
.replace(/<script[\s\S]*?<\/script>/gi, '')
.replace(/<style[\s\S]*?<\/style>/gi, '')
content
// Replace <br>, <p>, <li>, <tr> with newlines
.replace(/<(br|p|li|tr)\b[^>]*\/?>/gi, '\n')
// Replace </p>, </li>, </tr>, </div>, </h[1-6]> with newlines

27
src/utils/which.ts
View File

@ -1,11 +1,10 @@
import { execa } from 'execa'
import { execSync_DEPRECATED } from './execSyncWrapper.js'
import { execa, execaSync } from 'execa'
async function whichNodeAsync(command: string): Promise<string | null> {
if (process.platform === 'win32') {
// On Windows, use where.exe and return the first result
const result = await execa(`where.exe ${command}`, {
shell: true,
// Security: array args form prevents shell injection
const result = await execa('where.exe', [command], {
stderr: 'ignore',
reject: false,
})
@ -19,8 +18,8 @@ async function whichNodeAsync(command: string): Promise<string | null> {
// On POSIX systems (macOS, Linux, WSL), use which
// Cross-platform safe: Windows is handled above
// eslint-disable-next-line custom-rules/no-cross-platform-process-issues
const result = await execa(`which ${command}`, {
shell: true,
// Security: array args form prevents shell injection
const result = await execa('which', [command], {
stderr: 'ignore',
reject: false,
})
@ -33,11 +32,13 @@ async function whichNodeAsync(command: string): Promise<string | null> {
function whichNodeSync(command: string): string | null {
if (process.platform === 'win32') {
try {
const result = execSync_DEPRECATED(`where.exe ${command}`, {
encoding: 'utf-8',
// Security: use execaSync with array args to prevent shell injection
const result = execaSync('where.exe', [command], {
encoding: 'utf-8' as const,
stdio: ['ignore', 'pipe', 'ignore'],
reject: false,
})
const output = result.toString().trim()
const output = (result.stdout ?? '').trim()
return output.split(/\r?\n/)[0] || null
} catch {
return null
@ -45,11 +46,13 @@ function whichNodeSync(command: string): string | null {
}
try {
const result = execSync_DEPRECATED(`which ${command}`, {
encoding: 'utf-8',
// Security: use execaSync with array args to prevent shell injection
const result = execaSync('which', [command], {
encoding: 'utf-8' as const,
stdio: ['ignore', 'pipe', 'ignore'],
reject: false,
})
return result.toString().trim() || null
return (result.stdout ?? '').trim() || null
} catch {
return null
}