Skip to content

Commit 96871cc

Browse files
kochj23claude
andcommitted
fix(medium): Fix 10 MEDIUM severity findings from audit
1. Search performance (CodebaseIndexer.swift): Replaced .lowercased() + .contains() with .localizedCaseInsensitiveContains() 2. Occurrence counting (CodebaseIndexer.swift): Replaced array-splitting count with range-based loop using .range(of:options:range:) 3. O(n²) insert (ContextManager.swift): Changed insert(at:0) loop to append + reverse for O(n) message assembly 4. Memory limit (CodebaseIndexer.swift): Added 1MB cap on cached file content to prevent unbounded index memory growth 5. Python paths (MLXService.swift): Added multi-version Python lookup replacing hardcoded 3.9 path 6. Thread safety (MLXService.swift): Changed concurrent+barrier queues to serial queues for simpler, correct synchronization 7. Async logging (CommandValidator.swift): Moved file I/O to serial background queue, delegated to SecureLogger 8. Permission check (CommandValidator.swift): Added isReadableFile() check in validatePythonScript() 9. Regex validation (CommandValidator.swift): Converted Python function and system manipulation checks to regex with word boundaries 10. Debug cleanup: Removed remaining print() in logSecurityEvent() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent f70ca00 commit 96871cc

4 files changed

Lines changed: 122 additions & 58 deletions

File tree

MLX Code/Services/CodebaseIndexer.swift

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -85,16 +85,20 @@ actor CodebaseIndexer {
8585
}
8686

8787
do {
88-
let content = try String(contentsOf: fileURL, encoding: .utf8)
89-
let symbols = extractSymbols(from: content, language: ext)
90-
9188
let attributes = try fileManager.attributesOfItem(atPath: fileURL.path)
9289
let size = attributes[.size] as? Int ?? 0
9390
let modified = attributes[.modificationDate] as? Date ?? Date()
9491

92+
// Only cache file content for files under 1 MB to limit memory usage
93+
let maxCachedContentSize = 1_048_576 // 1 MB
94+
let content = try String(contentsOf: fileURL, encoding: .utf8)
95+
let cachedContent = size <= maxCachedContentSize ? content : ""
96+
97+
let symbols = extractSymbols(from: content, language: ext)
98+
9599
let indexed = IndexedFile(
96100
path: fileURL.path,
97-
content: content,
101+
content: cachedContent,
98102
language: ext,
99103
lastModified: modified,
100104
size: size,
@@ -119,27 +123,25 @@ actor CodebaseIndexer {
119123
/// - limit: Maximum results to return
120124
/// - Returns: Matching files with relevance scores
121125
func search(_ query: String, limit: Int = 10) async -> [(file: IndexedFile, score: Double)] {
122-
let lowercaseQuery = query.lowercased()
123126
var results: [(file: IndexedFile, score: Double)] = []
124127

125128
for file in index.values {
126129
var score = 0.0
127130

128-
// Check file name
129-
if file.path.lowercased().contains(lowercaseQuery) {
131+
// Check file name using case-insensitive comparison (avoids per-file lowercasing)
132+
if file.path.localizedCaseInsensitiveContains(query) {
130133
score += 10.0
131134
}
132135

133-
// Check symbols
136+
// Check symbols using case-insensitive comparison (avoids per-symbol lowercasing)
134137
for symbol in file.symbols {
135-
if symbol.name.lowercased().contains(lowercaseQuery) {
138+
if symbol.name.localizedCaseInsensitiveContains(query) {
136139
score += 5.0
137140
}
138141
}
139142

140-
// Check content
141-
let contentLower = file.content.lowercased()
142-
let occurrences = contentLower.components(separatedBy: lowercaseQuery).count - 1
143+
// Count occurrences using range-based loop (avoids allocating split array)
144+
let occurrences = countOccurrences(of: query, in: file.content)
143145
score += Double(occurrences) * 0.5
144146

145147
if score > 0 {
@@ -153,6 +155,17 @@ actor CodebaseIndexer {
153155
return Array(results.prefix(limit))
154156
}
155157

158+
/// Counts case-insensitive occurrences of a substring without allocating an array
159+
private func countOccurrences(of query: String, in content: String) -> Int {
160+
var count = 0
161+
var searchRange = content.startIndex..<content.endIndex
162+
while let range = content.range(of: query, options: .caseInsensitive, range: searchRange) {
163+
count += 1
164+
searchRange = range.upperBound..<content.endIndex
165+
}
166+
return count
167+
}
168+
156169
/// Finds files similar to a given file
157170
/// - Parameter path: File path
158171
/// - Returns: Similar files

MLX Code/Services/ContextManager.swift

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,11 @@ actor ContextManager {
7676
if recentTokens + msgTokens > budget.recentMessagesBudget {
7777
break
7878
}
79-
recentMessages.insert(message, at: 0)
79+
recentMessages.append(message)
8080
recentTokens += msgTokens
8181
}
82+
// Reverse once after collecting to restore chronological order (avoids O(n^2) inserts at index 0)
83+
recentMessages.reverse()
8284

8385
// 3. If older messages were dropped, create rule-based summary
8486
let droppedCount = nonSystemMessages.count - recentMessages.count

MLX Code/Services/MLXService.swift

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -515,7 +515,10 @@ actor MLXService {
515515

516516
// Use actual Xcode Python binary (not the xcode-select shim at /usr/bin/python3)
517517
// The shim calls xcrun which is forbidden in App Sandbox
518-
let pythonPath = "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9"
518+
guard let pythonPath = getXcodePythonPath() else {
519+
await SecureLogger.shared.error("No Xcode Python binary found", category: "MLXService")
520+
throw MLXServiceError.generationFailed("Python not found. Please install Xcode and Command Line Tools.")
521+
}
519522
process.executableURL = URL(fileURLWithPath: pythonPath)
520523
process.arguments = [scriptPath] // Daemon doesn't need --mode flag
521524
process.standardInput = inputPipe
@@ -783,6 +786,21 @@ actor MLXService {
783786
return "\(homeDir)/Library/Python/3.9/lib/python/site-packages"
784787
}
785788

789+
/// Discovers the Xcode-bundled Python binary by checking multiple versions
790+
/// Falls back through 3.13, 3.12, 3.11, 3.10, 3.9 to find whichever is installed
791+
/// - Returns: Path to the Python binary, or nil if none found
792+
private func getXcodePythonPath() -> String? {
793+
let basePath = "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions"
794+
let pythonVersions = ["3.13", "3.12", "3.11", "3.10", "3.9"]
795+
for version in pythonVersions {
796+
let path = "\(basePath)/\(version)/bin/python\(version)"
797+
if FileManager.default.fileExists(atPath: path) {
798+
return path
799+
}
800+
}
801+
return nil
802+
}
803+
786804
// MARK: - Private Methods
787805

788806
/// Formats chat messages into a prompt string
@@ -985,14 +1003,12 @@ extension MLXService {
9851003

9861004
// Use actual Xcode Python binary (not the xcode-select shim at /usr/bin/python3)
9871005
// The shim calls xcrun which is forbidden in App Sandbox
988-
let pythonPath = "/Applications/Xcode.app/Contents/Developer/Library/Frameworks/Python3.framework/Versions/3.9/bin/python3.9"
989-
let pythonExists = FileManager.default.fileExists(atPath: pythonPath)
990-
await SecureLogger.shared.info("Python exists at \(pythonPath): \(pythonExists)", category: "MLXService")
991-
992-
guard pythonExists else {
993-
await SecureLogger.shared.error("Python binary not found at: \(pythonPath)", category: "MLXService")
994-
throw MLXServiceError.generationFailed("Python 3.9 not found. Please install Xcode and Command Line Tools.")
1006+
// Checks multiple Python versions (3.13 down to 3.9) to find whichever Xcode ships
1007+
guard let pythonPath = getXcodePythonPath() else {
1008+
await SecureLogger.shared.error("No Xcode Python binary found across versions 3.9-3.13", category: "MLXService")
1009+
throw MLXServiceError.generationFailed("Python not found. Please install Xcode and Command Line Tools.")
9951010
}
1011+
await SecureLogger.shared.info("Using Python at: \(pythonPath)", category: "MLXService")
9961012

9971013
// Create process to download
9981014
let process = Process()
@@ -1053,9 +1069,9 @@ extension MLXService {
10531069
throw error
10541070
}
10551071

1056-
// Capture all output in separate buffers (thread-safe with DispatchQueue)
1057-
let outputQueue = DispatchQueue(label: "com.mlxcode.output", attributes: .concurrent)
1058-
let errorQueue = DispatchQueue(label: "com.mlxcode.error", attributes: .concurrent)
1072+
// Capture all output in separate buffers (thread-safe with serial DispatchQueues)
1073+
let outputQueue = DispatchQueue(label: "com.mlxcode.output")
1074+
let errorQueue = DispatchQueue(label: "com.mlxcode.error")
10591075
var allOutput = Data()
10601076
var allErrors = Data()
10611077

@@ -1064,8 +1080,8 @@ extension MLXService {
10641080
outputHandle.readabilityHandler = { handle in
10651081
let data = handle.availableData
10661082
if data.count > 0 {
1067-
// Store all output (thread-safe)
1068-
outputQueue.async(flags: .barrier) {
1083+
// Store all output (thread-safe via serial queue)
1084+
outputQueue.async {
10691085
allOutput.append(data)
10701086
}
10711087

@@ -1091,8 +1107,8 @@ extension MLXService {
10911107
errorHandle.readabilityHandler = { handle in
10921108
let data = handle.availableData
10931109
if data.count > 0 {
1094-
// Store all errors (thread-safe)
1095-
errorQueue.async(flags: .barrier) {
1110+
// Store all errors (thread-safe via serial queue)
1111+
errorQueue.async {
10961112
allErrors.append(data)
10971113
}
10981114

MLX Code/Utilities/CommandValidator.swift

Lines changed: 63 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -133,30 +133,40 @@ enum CommandValidator {
133133
}
134134
}
135135

136-
// 3. Block dangerous functions
137-
let dangerousFunctions = [
138-
"exec(",
139-
"eval(",
140-
"compile(",
141-
"pickle.load(",
142-
"pickle.loads(",
143-
"torch.load(",
144-
"open(", // File access
145-
"input(", // User input
146-
"raw_input("
136+
// 3. Block dangerous functions using regex (consistent with import validation above)
137+
// Each tuple: (regex pattern, human-readable description)
138+
let dangerousFunctionPatterns: [(pattern: String, description: String)] = [
139+
(#"\bexec\s*\("#, "exec()"),
140+
(#"\beval\s*\("#, "eval()"),
141+
(#"\bcompile\s*\("#, "compile()"),
142+
(#"\bpickle\.loads?\s*\("#, "pickle.load(s)"),
143+
(#"\btorch\.load\s*\("#, "torch.load()"),
144+
(#"\bopen\s*\("#, "open()"),
145+
(#"\binput\s*\("#, "input()"),
146+
(#"\braw_input\s*\("#, "raw_input()")
147147
]
148148

149-
for dangerous in dangerousFunctions {
150-
if code.contains(dangerous) {
151-
logSecurityEvent("BLOCKED dangerous Python function: \(dangerous)", level: .critical)
152-
throw SecurityError.dangerousFunction(dangerous)
149+
for (regexPattern, description) in dangerousFunctionPatterns {
150+
if let regex = try? NSRegularExpression(pattern: regexPattern, options: []),
151+
regex.firstMatch(in: uncommentedCode, range: NSRange(uncommentedCode.startIndex..., in: uncommentedCode)) != nil {
152+
logSecurityEvent("BLOCKED dangerous Python function: \(description)", level: .critical)
153+
throw SecurityError.dangerousFunction(description)
153154
}
154155
}
155156

156-
// 4. Block system manipulation
157-
if code.contains("os.") || code.contains("sys.") || code.contains("subprocess.") {
158-
logSecurityEvent("BLOCKED system manipulation in Python code", level: .critical)
159-
throw SecurityError.systemManipulation
157+
// 4. Block system manipulation using regex for consistency
158+
let systemPatterns: [(pattern: String, description: String)] = [
159+
(#"\bos\."#, "os module access"),
160+
(#"\bsys\."#, "sys module access"),
161+
(#"\bsubprocess\."#, "subprocess module access")
162+
]
163+
164+
for (regexPattern, description) in systemPatterns {
165+
if let regex = try? NSRegularExpression(pattern: regexPattern, options: []),
166+
regex.firstMatch(in: uncommentedCode, range: NSRange(uncommentedCode.startIndex..., in: uncommentedCode)) != nil {
167+
logSecurityEvent("BLOCKED system manipulation: \(description)", level: .critical)
168+
throw SecurityError.systemManipulation
169+
}
160170
}
161171

162172
// 5. Log for audit
@@ -184,6 +194,12 @@ enum CommandValidator {
184194
throw SecurityError.fileNotFound(expandedPath)
185195
}
186196

197+
// 2b. Verify file is readable (handles permission errors explicitly)
198+
guard FileManager.default.isReadableFile(atPath: expandedPath) else {
199+
logSecurityEvent("Permission denied reading script: \(expandedPath)", level: .error)
200+
throw SecurityError.invalidPath("Permission denied: cannot read \(expandedPath)")
201+
}
202+
187203
// 3. Verify it's a Python file
188204
guard expandedPath.hasSuffix(".py") else {
189205
throw SecurityError.invalidFileType("Only .py files allowed")
@@ -260,24 +276,41 @@ enum CommandValidator {
260276

261277
// MARK: - Logging
262278

279+
/// Serial queue for async file I/O to avoid blocking the calling thread
280+
private static let logQueue = DispatchQueue(label: "com.mlxcode.commandvalidator.log")
281+
263282
private static func logSecurityEvent(_ message: String, level: SecurityLogLevel) {
264283
let timestamp = ISO8601DateFormatter().string(from: Date())
265284
let logMessage = "[\(timestamp)] [CommandValidator] [\(level.rawValue)] \(message)"
266285

267-
print(logMessage)
286+
// Delegate to SecureLogger for structured logging (non-blocking fire-and-forget)
287+
Task {
288+
switch level {
289+
case .critical:
290+
await SecureLogger.shared.critical(logMessage, category: "CommandValidator")
291+
case .error:
292+
await SecureLogger.shared.error(logMessage, category: "CommandValidator")
293+
case .warning:
294+
await SecureLogger.shared.warning(logMessage, category: "CommandValidator")
295+
case .info:
296+
await SecureLogger.shared.info(logMessage, category: "CommandValidator")
297+
}
298+
}
268299

269-
// Also log to security log file
300+
// Also write to security log file asynchronously (avoids blocking the caller)
270301
let logPath = NSHomeDirectory() + "/Library/Logs/MLXCode/security.log"
271302
if let data = (logMessage + "\n").data(using: .utf8) {
272-
if let fileHandle = FileHandle(forWritingAtPath: logPath) {
273-
fileHandle.seekToEndOfFile()
274-
fileHandle.write(data)
275-
try? fileHandle.close()
276-
} else {
277-
// Create file if doesn't exist
278-
let logDir = (logPath as NSString).deletingLastPathComponent
279-
try? FileManager.default.createDirectory(atPath: logDir, withIntermediateDirectories: true)
280-
try? data.write(to: URL(fileURLWithPath: logPath))
303+
logQueue.async {
304+
if let fileHandle = FileHandle(forWritingAtPath: logPath) {
305+
fileHandle.seekToEndOfFile()
306+
fileHandle.write(data)
307+
try? fileHandle.close()
308+
} else {
309+
// Create file if doesn't exist
310+
let logDir = (logPath as NSString).deletingLastPathComponent
311+
try? FileManager.default.createDirectory(atPath: logDir, withIntermediateDirectories: true)
312+
try? data.write(to: URL(fileURLWithPath: logPath))
313+
}
281314
}
282315
}
283316
}

0 commit comments

Comments
 (0)