fix(P0): close 2 audit findings from independent review
1. P0 SECURITY: git/index.ts run_git used execSync(`git ${args.join(' ')}`)
with LLM-controlled args (commit messages, branch names, ranges) —
classic command injection. Replaced with execFileSync('git', args, ...)
which uses argv array (no shell parsing).
2. P1 CORRECTNESS: RuntimeApp constructor created TWO Scheduler instances:
- Line 39: Scheduler({...}) without worker_manager
- Line 55: Scheduler({...}, worker_manager) replacing the first
First instance was leaked (allocated then overwritten). Removed the
duplicate, kept only the wired version.
Verification:
- 169/169 tests pass
- tsc --noEmit: 0 errors
- depcruise: 0 violations
- grep 'new Scheduler' RuntimeApp.ts → 1 match (was 2)
- grep 'execSync' git/index.ts → 0 matches (was 1, with LLM-controlled args)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -36,11 +36,6 @@ export class RuntimeApp {
|
||||
constructor(config: RuntimeAppConfig) {
|
||||
this.config = config
|
||||
this.logger = new Logger(config.log_dir || join(config.project_root, '.air', 'logs'))
|
||||
this.scheduler = new Scheduler({
|
||||
session_id: config.session_id,
|
||||
project_id: config.project_id,
|
||||
project_root: config.project_root
|
||||
})
|
||||
this.worker_manager = new WorkerManager()
|
||||
this.context_assembler = new ContextAssembler()
|
||||
this.doctor = new DoctorService(config.project_root)
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
* @module packages/runtime/src/tools/git
|
||||
*/
|
||||
|
||||
import { execSync } from 'child_process'
|
||||
import { execFileSync } from 'child_process'
|
||||
import { existsSync } from 'fs'
|
||||
import { join, dirname } from 'path'
|
||||
import type { ToolDefinition, ToolCall, ToolResultEnvelope, ISOTimeString } from '@aircoding/contracts'
|
||||
@@ -119,7 +119,8 @@ export function createGitExecutor(project_root: string) {
|
||||
|
||||
const run_git = (repo_path: string, ...args: string[]): string => {
|
||||
try {
|
||||
return execSync(`git ${args.join(' ')}`, {
|
||||
// SECURITY: use execFileSync with args array — never string-interpolate user-controlled args
|
||||
return execFileSync('git', args, {
|
||||
cwd: repo_path,
|
||||
encoding: 'utf-8',
|
||||
stdio: ['pipe', 'pipe', 'pipe']
|
||||
|
||||
Reference in New Issue
Block a user