From 17e6503ccdeee0597dafef98a139b5ab078cb8af Mon Sep 17 00:00:00 2001 From: g1nation Date: Wed, 6 May 2026 12:31:58 +0900 Subject: [PATCH] refactor: fix security hardcode, dead code, resource leaks, operator bugs --- src/bridge.ts | 23 +++++++++++++++++++--- src/config.ts | 3 +-- src/core/conflict.ts | 8 +++++--- src/core/dataProcessor.ts | 37 +++++++++--------------------------- src/core/health.ts | 36 +++++++++++++++++++++++++++-------- src/core/lock.ts | 40 +++++++++++++++++++++++++++++++++------ src/extension.ts | 15 +++++++++------ src/security.ts | 31 +++++++++++++++++++++++++++--- 8 files changed, 134 insertions(+), 59 deletions(-) diff --git a/src/bridge.ts b/src/bridge.ts index 0996400..8c6e20c 100644 --- a/src/bridge.ts +++ b/src/bridge.ts @@ -135,9 +135,26 @@ export class BridgeServer { } const result = await this.aiService.call(`Analyze chat history for metrics (JSON):\n${historyText.slice(-6000)}`); - const jsonMatch = result.match(/\{[\s\S]*?\}/); - res.writeHead(jsonMatch ? 200 : 500, { 'Content-Type': 'application/json' }); - res.end(jsonMatch ? jsonMatch[0] : JSON.stringify({ error: "Parse failed", raw: result })); + + // Try to extract valid JSON from the AI response + let parsed: any = null; + try { + parsed = JSON.parse(result); + } catch { + // AI may wrap JSON in markdown code fences — try to extract + const fenceMatch = result.match(/```(?:json)?\s*([\s\S]*?)```/); + if (fenceMatch) { + try { parsed = JSON.parse(fenceMatch[1].trim()); } catch { /* fall through */ } + } + } + + if (parsed) { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify(parsed)); + } else { + res.writeHead(500, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ error: "Failed to parse AI response as JSON", raw: summarizeText(result, 500) })); + } } private async processBrainInject(data: any, res: http.ServerResponse) { diff --git a/src/config.ts b/src/config.ts index 58c9d17..2da4619 100644 --- a/src/config.ts +++ b/src/config.ts @@ -147,10 +147,9 @@ export const SECURITY_POLICY = { maxContextFiles: 200, }; -export const MAX_CONTEXT_SIZE = 12_000; export const EXCLUDED_DIRS = new Set([ 'node_modules', '.git', '.vscode', 'out', 'dist', 'build', '.next', '.cache', '__pycache__', '.DS_Store', 'coverage', - '.turbo', '.nuxt', '.output', 'vendor', 'target' + '.turbo', '.nuxt', '.output', 'vendor', 'target', '.astra' ]); diff --git a/src/core/conflict.ts b/src/core/conflict.ts index c982476..2227552 100644 --- a/src/core/conflict.ts +++ b/src/core/conflict.ts @@ -9,18 +9,20 @@ export class ConflictResolver { * Analyzes proposed actions and returns a warning message if conflicts are found. */ public static analyze(actions: any[]): string | null { + if (!actions || actions.length === 0) return null; + // 1. Resource Conflict: Multiple edits to sensitive core files const securityFiles = actions.filter(a => a.path && (a.path.includes('auth') || a.path.includes('security') || a.path.includes('config'))); const performanceChanges = actions.filter(a => a.content && (a.content.includes('async') || a.content.includes('parallel') || a.content.includes('cache'))); if (securityFiles.length > 0 && performanceChanges.length > 0) { - return "⚠️ Goal Conflict Detected: You are attempting to modify security-sensitive files while introducing performance optimizations (async/parallel). This might introduce race conditions or security vulnerabilities. Should I proceed with both, or prioritize Security?"; + return "Goal Conflict Detected: You are attempting to modify security-sensitive files while introducing performance optimizations (async/parallel). This might introduce race conditions or security vulnerabilities. Should I proceed with both, or prioritize Security?"; } // 2. Structural Conflict: Modifying both interface and implementation in a way that might break contracts - const interfaces = actions.filter(a => a.path && a.path.endsWith('.d.ts') || a.path.includes('interface')); + const interfaces = actions.filter(a => a.path && (a.path.endsWith('.d.ts') || a.path.includes('interface'))); if (interfaces.length > 1 && actions.length > 10) { - return "⚠️ Structural Complexity Warning: This task involves massive changes to both interfaces and multiple implementations. This may lead to unexpected side effects. Would you like a step-by-step review?"; + return "Structural Complexity Warning: This task involves massive changes to both interfaces and multiple implementations. This may lead to unexpected side effects. Would you like a step-by-step review?"; } return null; diff --git a/src/core/dataProcessor.ts b/src/core/dataProcessor.ts index 174645b..f100e27 100644 --- a/src/core/dataProcessor.ts +++ b/src/core/dataProcessor.ts @@ -1,5 +1,5 @@ /** - * 집계 결과 타입 정의 + * Aggregate result type definition */ export interface AggregateResult { key: string; @@ -14,14 +14,14 @@ interface AggregateOptions { /** * DataProcessor: - * 시스템의 알고리즘적 효율성과 유지보수성을 극대화하기 위한 핵심 집계 엔진. - * O(N) 복잡도를 보장하며 데이터 분포 민감도를 고려한 최적화 전략을 포함합니다. + * Core aggregation engine for maximizing algorithmic efficiency and maintainability. + * Guarantees O(N) complexity with data distribution-sensitive optimization strategy. */ export class DataProcessor { /** - * 핵심 데이터 집계 함수 (Optimized O(N)) - * @param data 집계할 데이터 배열 - * @param keyPath 집계 기준이 될 속성 경로 + * Core data aggregation function (Optimized O(N)) + * @param data Array to aggregate + * @param keyPath Property path to group by */ public static aggregate(data: any[], keyPath: string, options: AggregateOptions = {}): AggregateResult[] { if (!data || data.length === 0) return []; @@ -31,10 +31,6 @@ export class DataProcessor { const pathSegments = this.parseKeyPath(keyPath); const collectValues = options.collectValues !== false; - - // 1. 성능 상충 관계 (Sweet Spot) 고려: - // 데이터가 매우 작을 때는(예: N < 10) 해시 맵 생성 오버헤드가 더 클 수 있으나, - // 일반적인 성능 보장을 위해 해시 기반 단일 패스(Single-Pass) 방식을 기본으로 채택합니다. const map = new Map(); @@ -59,15 +55,9 @@ export class DataProcessor { if (collectValues) { entry.values.push(item); } - - // 수치형 데이터인 경우 평균 계산을 위한 로직 (예시) - if (typeof item.value === 'number') { - // 점진적 평균 계산 등 추가 가능 - } } catch (error) { - // 오류 처리 정밀도 (Error Handling Granularity) - // 특정 아이템 처리 실패가 전체 집계 중단으로 이어지지 않도록 격리 + // Isolate item processing failures to prevent entire aggregation from aborting console.warn(`[DataProcessor] Skip item due to error: ${error}`); } } @@ -76,17 +66,8 @@ export class DataProcessor { } /** - * 데이터 분포 민감성(Data Distribution Sensitivity)을 고려한 고도화된 집계 (Trie 기반) - * 키가 매우 길거나 계층적인 경우 메모리 및 검색 속도 최적화를 위해 사용합니다. - */ - public static aggregateByTrie(data: any[], keyPath: string, options: AggregateOptions = {}): AggregateResult[] { - // TODO: 복잡한 키 구조를 위한 Trie 인덱싱 로직 구현 (Phase 2 확장 예정) - return this.aggregate(data, keyPath, options); - } - - /** - * 중첩된 객체 속성 접근 (Safety handling). - * keyPath는 루프 밖에서 한 번만 파싱하여 대규모 데이터 처리 시 반복 split 비용을 피합니다. + * Nested object property accessor (safety handling). + * keyPath is parsed once outside the loop to avoid repeated split costs on large datasets. */ private static getNestedValue(obj: any, pathSegments: string[]): any { return pathSegments.reduce((prev, curr) => { diff --git a/src/core/health.ts b/src/core/health.ts index f7c10ca..ca8e507 100644 --- a/src/core/health.ts +++ b/src/core/health.ts @@ -6,28 +6,32 @@ import { logInfo, logWarn, logError } from '../utils'; /** * HealthCheckMonitor: Periodically monitors the environment * (Ollama, Disk, API) to ensure the agent stays functional. + * + * Properly tracks the interval timer for cleanup on deactivation. */ export class HealthCheckMonitor { + private static intervalHandle: ReturnType | null = null; + public static async runAllChecks(): Promise<{ ok: boolean; reports: string[] }> { const reports: string[] = []; const config = getConfig(); - // 1. Ollama Connectivity Check + // 1. AI Engine Connectivity Check try { const res = await fetch(`${config.ollamaUrl}/api/tags`, { signal: AbortSignal.timeout(3000) }); if (res.ok) { - logInfo('Health Check: Ollama connectivity OK.'); + logInfo('Health Check: AI engine connectivity OK.'); } else { - reports.push('⚠️ AI Server (Ollama) is reachable but returned an error.'); + reports.push('AI Server is reachable but returned an error.'); } } catch { - reports.push('❌ AI Server (Ollama) is NOT reachable. Please check if it is running.'); + reports.push('AI Server is NOT reachable. Please check if it is running.'); } // 2. Workspace Validation const workspaceFolders = vscode.workspace.workspaceFolders; if (!workspaceFolders || workspaceFolders.length === 0) { - reports.push('⚠️ No workspace folder open. Agent capabilities will be limited.'); + reports.push('No workspace folder open. Agent capabilities will be limited.'); } // 3. Simple Disk/Permissions Check @@ -39,7 +43,7 @@ export class HealthCheckMonitor { logInfo('Health Check: Write permissions OK.'); } } catch { - reports.push('❌ Write permissions denied in the current workspace.'); + reports.push('Write permissions denied in the current workspace.'); } if (reports.length > 0) { @@ -53,7 +57,23 @@ export class HealthCheckMonitor { }; } - public static startInterval(ms: number = 300000) { // Default 5 mins - setInterval(() => this.runAllChecks(), ms); + public static startInterval(ms: number = 300000) { + // Prevent duplicate intervals + if (this.intervalHandle !== null) { + clearInterval(this.intervalHandle); + } + this.intervalHandle = setInterval(() => this.runAllChecks(), ms); + } + + /** + * Stops periodic health checks and releases resources. + * Should be called from extension.deactivate(). + */ + public static dispose(): void { + if (this.intervalHandle !== null) { + clearInterval(this.intervalHandle); + this.intervalHandle = null; + logInfo('Health Check: Interval stopped.'); + } } } diff --git a/src/core/lock.ts b/src/core/lock.ts index 786ab49..555dee0 100644 --- a/src/core/lock.ts +++ b/src/core/lock.ts @@ -3,15 +3,22 @@ import { logInfo } from '../utils'; /** * AsyncLockManager: Prevents race conditions by ensuring only one task * can access a specific resource (e.g., a file path) at a time. + * + * Includes timeout protection to prevent indefinite lock-waiting, + * and proper cleanup on acquisition failure. */ export class AsyncLockManager { private locks: Map> = new Map(); + private static readonly DEFAULT_TIMEOUT_MS = 30_000; /** * Acquires a lock for a specific resource. - * If the resource is already locked, it waits until the previous task finishes. + * If the resource is already locked, waits until the previous task finishes. + * Times out after `timeoutMs` to prevent deadlocks. + * + * @returns A release function that MUST be called when the work is done (use try/finally). */ - public async acquire(resourceId: string): Promise<() => void> { + public async acquire(resourceId: string, timeoutMs: number = AsyncLockManager.DEFAULT_TIMEOUT_MS): Promise<() => void> { const previousLock = this.locks.get(resourceId) || Promise.resolve(); let release: () => void; @@ -21,17 +28,38 @@ export class AsyncLockManager { this.locks.set(resourceId, previousLock.then(() => newLock)); - await previousLock; + // Wait for previous lock with a timeout to prevent deadlocks + const timeoutPromise = new Promise((_, reject) => { + setTimeout(() => reject(new Error(`Lock acquisition timed out for resource: ${resourceId}`)), timeoutMs); + }); + + try { + await Promise.race([previousLock, timeoutPromise]); + } catch (error) { + // Clean up the dangling lock on timeout + if (this.locks.get(resourceId) === previousLock.then(() => newLock)) { + this.locks.delete(resourceId); + } + release!(); + throw error; + } + logInfo(`Lock acquired for: ${resourceId}`); return () => { logInfo(`Lock released for: ${resourceId}`); release(); - if (this.locks.get(resourceId) === newLock) { - this.locks.delete(resourceId); - } + // Clean up the Map entry if this is the latest lock + this.locks.delete(resourceId); }; } + + /** + * Returns the number of currently held locks (for diagnostics). + */ + public getActiveLockCount(): number { + return this.locks.size; + } } // Export as a singleton for the entire agent process diff --git a/src/extension.ts b/src/extension.ts index b26bb1a..6f44c8e 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -85,7 +85,9 @@ export async function activate(context: vscode.ExtensionContext) { } } -export function deactivate() {} +export function deactivate() { + HealthCheckMonitor.dispose(); +} async function runInitialSetup(context: vscode.ExtensionContext) { try { @@ -138,7 +140,9 @@ async function _ensureBrainDir(context: vscode.ExtensionContext): Promise normalizedTarget.startsWith(root)); + if (!isTrusted) { throw new Error(`Security Violation: Path traversal detected! Attempted to access ${absolutePath} which is outside allowed boundaries.`); }