fix: address PR review findings

- Fix XSS vulnerability in formatLogContent by escaping HTML before injecting spans
- Use async fs.readFile instead of sync fs.readFileSync to avoid blocking event loop
- Add path sanitization to prevent path traversal attacks in log file APIs
- Add defense-in-depth path validation to ensure resolved paths stay within LOG_BASE_PATH
- Add archiver error handler to properly handle archive generation errors
- Fix event listener ordering: register 'end' handler before calling finalize()
- Add empty zip detection: return 404 error if no log files found on disk
- Fix toolProtocol not being applied for 'other' provider in new-run.tsx

Author: hannesrudolph

apps/web-evals/src/app/api/runs/[id]/logs/[taskId]/route.ts

@@ -1,6 +1,6 @@
 import { NextResponse } from "next/server"
 import type { NextRequest } from "next/server"
-import * as fs from "node:fs"
+import * as fs from "node:fs/promises"
 import * as path from "node:path"
 
 import { findTask, findRun } from "@roo-code/evals"
@@ -9,6 +9,12 @@ export const dynamic = "force-dynamic"
 
 const LOG_BASE_PATH = "/tmp/evals/runs"
 
+// Sanitize path components to prevent path traversal attacks
+function sanitizePathComponent(component: string): string {
+	// Remove any path separators, null bytes, and other dangerous characters
+	return component.replace(/[/\\:\0*?"<>|]/g, "_")
+}
+
 export async function GET(request: NextRequest, { params }: { params: Promise<{ id: string; taskId: string }> }) {
 	const { id, taskId } = await params
 
@@ -31,19 +37,31 @@ export async function GET(request: NextRequest, { params }: { params: Promise<{
 			return NextResponse.json({ error: "Task does not belong to this run" }, { status: 404 })
 		}
 
+		// Sanitize language and exercise to prevent path traversal
+		const safeLanguage = sanitizePathComponent(task.language)
+		const safeExercise = sanitizePathComponent(task.exercise)
+
 		// Construct the log file path
-		const logFileName = `${task.language}-${task.exercise}.log`
+		const logFileName = `${safeLanguage}-${safeExercise}.log`
 		const logFilePath = path.join(LOG_BASE_PATH, String(runId), logFileName)
 
-		// Check if the log file exists
-		if (!fs.existsSync(logFilePath)) {
-			return NextResponse.json({ error: "Log file not found", logContent: null }, { status: 200 })
+		// Verify the resolved path is within the expected directory (defense in depth)
+		const resolvedPath = path.resolve(logFilePath)
+		const expectedBase = path.resolve(LOG_BASE_PATH)
+		if (!resolvedPath.startsWith(expectedBase)) {
+			return NextResponse.json({ error: "Invalid log path" }, { status: 400 })
 		}
 
-		// Read the log file
-		const logContent = fs.readFileSync(logFilePath, "utf-8")
-
-		return NextResponse.json({ logContent })
+		// Check if the log file exists and read it (async)
+		try {
+			const logContent = await fs.readFile(logFilePath, "utf-8")
+			return NextResponse.json({ logContent })
+		} catch (err) {
+			if ((err as NodeJS.ErrnoException).code === "ENOENT") {
+				return NextResponse.json({ error: "Log file not found", logContent: null }, { status: 200 })
+			}
+			throw err
+		}
 	} catch (error) {
 		console.error("Error reading task log:", error)
 

apps/web-evals/src/app/api/runs/[id]/logs/failed/route.ts

@@ -10,6 +10,12 @@ export const dynamic = "force-dynamic"
 
 const LOG_BASE_PATH = "/tmp/evals/runs"
 
+// Sanitize path components to prevent path traversal attacks
+function sanitizePathComponent(component: string): string {
+	// Remove any path separators, null bytes, and other dangerous characters
+	return component.replace(/[/\\:\0*?"<>|]/g, "_")
+}
+
 export async function GET(request: NextRequest, { params }: { params: Promise<{ id: string }> }) {
 	const { id } = await params
 
@@ -43,25 +49,61 @@ export async function GET(request: NextRequest, { params }: { params: Promise<{
 			chunks.push(chunk)
 		})
 
+		// Track archive errors
+		let archiveError: Error | null = null
+		archive.on("error", (err: Error) => {
+			archiveError = err
+		})
+
+		// Set up the end promise before finalizing (proper event listener ordering)
+		const archiveEndPromise = new Promise<void>((resolve, reject) => {
+			archive.on("end", resolve)
+			archive.on("error", reject)
+		})
+
 		// Add each failed task's log file to the archive
 		const logDir = path.join(LOG_BASE_PATH, String(runId))
+		let filesAdded = 0
 
 		for (const task of failedTasks) {
-			const logFileName = `${task.language}-${task.exercise}.log`
+			// Sanitize language and exercise to prevent path traversal
+			const safeLanguage = sanitizePathComponent(task.language)
+			const safeExercise = sanitizePathComponent(task.exercise)
+			const logFileName = `${safeLanguage}-${safeExercise}.log`
 			const logFilePath = path.join(logDir, logFileName)
 
+			// Verify the resolved path is within the expected directory (defense in depth)
+			const resolvedPath = path.resolve(logFilePath)
+			const expectedBase = path.resolve(LOG_BASE_PATH)
+			if (!resolvedPath.startsWith(expectedBase)) {
+				continue // Skip files with suspicious paths
+			}
+
 			if (fs.existsSync(logFilePath)) {
 				archive.file(logFilePath, { name: logFileName })
+				filesAdded++
 			}
 		}
 
+		// Check if any files were actually added
+		if (filesAdded === 0) {
+			archive.abort()
+			return NextResponse.json(
+				{ error: "No log files found - they may have been cleared from disk" },
+				{ status: 404 },
+			)
+		}
+
 		// Finalize the archive
 		await archive.finalize()
 
 		// Wait for all data to be collected
-		await new Promise<void>((resolve) => {
-			archive.on("end", resolve)
-		})
+		await archiveEndPromise
+
+		// Check for archive errors
+		if (archiveError) {
+			throw archiveError
+		}
 
 		// Combine all chunks into a single buffer
 		const zipBuffer = Buffer.concat(chunks)

apps/web-evals/src/app/runs/[id]/run.tsx

@@ -42,12 +42,25 @@ function getToolAbbreviation(toolName: string): string {
 		.join("")
 }
 
-// Format log content with basic highlighting
+// Escape HTML to prevent XSS
+function escapeHtml(text: string): string {
+	return text
+		.replace(/&/g, "&")
+		.replace(/</g, "&lt;")
+		.replace(/>/g, "&gt;")
+		.replace(/"/g, "&quot;")
+		.replace(/'/g, "&#039;")
+}
+
+// Format log content with basic highlighting (XSS-safe)
 function formatLogContent(log: string): React.ReactNode[] {
 	const lines = log.split("\n")
 	return lines.map((line, index) => {
+		// First escape the entire line to prevent XSS
+		let formattedLine = escapeHtml(line)
+
 		// Highlight timestamps [YYYY-MM-DDTHH:MM:SS.sssZ]
-		let formattedLine = line.replace(
+		formattedLine = formattedLine.replace(
 			/\[(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z)\]/g,
 			'<span class="text-blue-400">[$1]</span>',
 		)
@@ -67,7 +80,7 @@ function formatLogContent(log: string): React.ReactNode[] {
 			'<span class="text-purple-400">$1</span>',
 		)
 
-		// Highlight message arrows
+		// Highlight message arrows (escaped as &rarr; after escapeHtml)
 		formattedLine = formattedLine.replace(/→/g, '<span class="text-cyan-400">→</span>')
 
 		return (

apps/web-evals/src/app/runs/new/new-run.tsx

@@ -248,9 +248,10 @@ export function NewRun() {
 							: {}),
 					}
 				} else if (provider === "other" && values.settings) {
-					// For imported settings, merge in experiments
+					// For imported settings, merge in experiments and tool protocol
 					values.settings = {
 						...values.settings,
+						toolProtocol: useNativeToolProtocol ? "native" : "xml",
 						...experimentsSettings,
 					}
 				}