fix: address oracle review — shell injection, range validation, timestamp robustness

ffmpeg.ts:
- Replace exec() with spawn() for ffmpeg/ffprobe — no shell interpreter,
  no injection risk from filenames with special characters
- Filter invalid ranges in computeIdleSections() after clamping to video
  bounds (handles timestamps exceeding video duration)
- Use path.basename() for timer IDs instead of split('/') for Windows compat

executor.ts:
- Use try/finally around VM execution so execution timestamps are recorded
  even when code throws — the execution still occupied real time that should
  not be sped up in the demo video
- Snapshot recordingStartedAt before try to avoid race if stopRecording()
  is called inside the same execute() clearing the field
This commit is contained in:
Tommy D. Rossi
2026-02-25 12:51:48 +01:00
parent 95bda7acc7
commit 2d0f4ac456
2 changed files with 73 additions and 42 deletions
+17 -10
View File
@@ -1035,18 +1035,25 @@ export class PlaywrightExecutor {
// Track execution timestamps relative to recording start (seconds). // Track execution timestamps relative to recording start (seconds).
// Used to identify idle gaps that can be sped up in demo videos. // Used to identify idle gaps that can be sped up in demo videos.
const execStartSec = this.recordingStartedAt !== null // Captured before try so we can record timing even if execution throws.
? (Date.now() - this.recordingStartedAt) / 1000 const recordingStartSnapshot = this.recordingStartedAt
const execStartSec = recordingStartSnapshot !== null
? (Date.now() - recordingStartSnapshot) / 1000
: -1 : -1
const result = await Promise.race([ let result: unknown
vm.runInContext(wrappedCode, vmContext, { timeout, displayErrors: true }), try {
new Promise((_, reject) => setTimeout(() => reject(new CodeExecutionTimeoutError(timeout)), timeout)), result = await Promise.race([
]) vm.runInContext(wrappedCode, vmContext, { timeout, displayErrors: true }),
new Promise((_, reject) => setTimeout(() => reject(new CodeExecutionTimeoutError(timeout)), timeout)),
if (this.recordingStartedAt !== null && execStartSec >= 0) { ])
const execEndSec = (Date.now() - this.recordingStartedAt) / 1000 } finally {
this.executionTimestamps.push({ start: execStartSec, end: execEndSec }) // Record timestamp even on error — the execution still occupied real time
// that should not be sped up in the demo video.
if (recordingStartSnapshot !== null && execStartSec >= 0 && this.recordingStartedAt !== null) {
const execEndSec = (Date.now() - recordingStartSnapshot) / 1000
this.executionTimestamps.push({ start: execStartSec, end: execEndSec })
}
} }
let responseText = formatConsoleLogs(consoleLogs) let responseText = formatConsoleLogs(consoleLogs)
+56 -32
View File
@@ -6,7 +6,7 @@
* No intermediate files, no multi-pass. * No intermediate files, no multi-pass.
*/ */
import { exec } from 'node:child_process' import { spawn } from 'node:child_process'
import path from 'node:path' import path from 'node:path'
// --------------------------------------------------------------------------- // ---------------------------------------------------------------------------
@@ -67,16 +67,17 @@ export interface VideoInfo {
/** Probe input video for dimensions and frame rate via ffprobe. */ /** Probe input video for dimensions and frame rate via ffprobe. */
export async function probeVideo(filePath: string): Promise<VideoInfo> { export async function probeVideo(filePath: string): Promise<VideoInfo> {
const command = [ const stdout = await runCommand({
'ffprobe', bin: 'ffprobe',
'-v error', args: [
'-select_streams v:0', '-v', 'error',
'-show_entries stream=width,height,r_frame_rate', '-select_streams', 'v:0',
'-of json', '-show_entries', 'stream=width,height,r_frame_rate',
`"${filePath}"`, '-of', 'json',
].join(' ') filePath,
],
})
const stdout = await runCommand({ command })
const parsed = JSON.parse(stdout) const parsed = JSON.parse(stdout)
const stream = parsed.streams?.[0] const stream = parsed.streams?.[0]
if (!stream) { if (!stream) {
@@ -93,28 +94,48 @@ export async function probeVideo(filePath: string): Promise<VideoInfo> {
} }
} }
/** Run a shell command, optionally abortable. Returns stdout. */ /**
* Run a process with argv (no shell). Returns stdout as string.
* Avoids shell injection by never passing through a shell interpreter.
*/
function runCommand({ function runCommand({
command, bin,
args,
signal, signal,
}: { }: {
command: string bin: string
args: string[]
signal?: AbortSignal signal?: AbortSignal
}): Promise<string> { }): Promise<string> {
return new Promise((resolve, reject) => { return new Promise((resolve, reject) => {
const childProcess = exec(command, (error, stdout, stderr) => { const child = spawn(bin, args, { stdio: ['ignore', 'pipe', 'pipe'] })
if (error) { let stdout = ''
reject(new Error(`FFmpeg error: ${stderr}`, { cause: error })) let stderr = ''
} else {
child.stdout.on('data', (data: Buffer) => {
stdout += data.toString()
})
child.stderr.on('data', (data: Buffer) => {
stderr += data.toString()
})
child.on('close', (code) => {
if (code === 0) {
resolve(stdout) resolve(stdout)
} else {
reject(new Error(`FFmpeg error (exit ${code}): ${stderr}`))
} }
}) })
child.on('error', (err) => {
reject(new Error(`Failed to start ${bin}`, { cause: err }))
})
if (signal) { if (signal) {
signal.addEventListener( signal.addEventListener(
'abort', 'abort',
() => { () => {
childProcess.kill() child.kill()
reject( reject(
signal.reason instanceof Error signal.reason instanceof Error
? signal.reason ? signal.reason
@@ -235,12 +256,13 @@ export async function concatenateVideos(
throw new Error('Missing required parameters') throw new Error('Missing required parameters')
} }
const timerId = `concat-${inputFiles.length}-videos-${outputFile.split('/').pop()}` const timerId = `concat-${inputFiles.length}-videos-${path.basename(outputFile)}`
console.time(timerId) console.time(timerId)
const inputArgs = inputFiles.map((file) => { // Build argv: -i file1 -i file2 ... -filter_complex "..." -map "[v_out]" output
return `-i "${file.path}"` const inputArgs = inputFiles.flatMap((file) => {
}).join(' ') return ['-i', file.path]
})
const filterComplexParts: string[] = [] const filterComplexParts: string[] = []
const videoStreamParts: string[] = [] const videoStreamParts: string[] = []
@@ -266,13 +288,12 @@ export async function concatenateVideos(
) )
const filterComplex = filterComplexParts.join('; ') const filterComplex = filterComplexParts.join('; ')
const command = const args = [...inputArgs, '-filter_complex', filterComplex, '-map', '[v_out]', outputFile]
`ffmpeg ${inputArgs} -filter_complex "${filterComplex}" -map "[v_out]" "${outputFile}"`
console.log('Running FFmpeg command:', command) console.log('Running FFmpeg concat:', args.join(' '))
try { try {
await runCommand({ command, signal }) await runCommand({ bin: 'ffmpeg', args, signal })
} finally { } finally {
console.timeEnd(timerId) console.timeEnd(timerId)
} }
@@ -328,7 +349,7 @@ export async function speedUpSections(
const height = dims?.height ?? probed!.height const height = dims?.height ?? probed!.height
const frameRate = fps ?? probed!.frameRate const frameRate = fps ?? probed!.frameRate
const timerId = `speedup-${sections.length}-sections-${outputFile.split('/').pop()}` const timerId = `speedup-${sections.length}-sections-${path.basename(outputFile)}`
console.time(timerId) console.time(timerId)
const segments = buildSegments(sections) const segments = buildSegments(sections)
@@ -352,13 +373,12 @@ export async function speedUpSections(
) )
const filterComplex = filterParts.join('; ') const filterComplex = filterParts.join('; ')
const command = const args = ['-i', inputFile, '-filter_complex', filterComplex, '-map', '[v_out]', outputFile]
`ffmpeg -i "${inputFile}" -filter_complex "${filterComplex}" -map "[v_out]" "${outputFile}"`
console.log('Running FFmpeg command:', command) console.log('Running FFmpeg speedup:', args.join(' '))
try { try {
await runCommand({ command, signal }) await runCommand({ bin: 'ffmpeg', args, signal })
} finally { } finally {
console.timeEnd(timerId) console.timeEnd(timerId)
} }
@@ -422,12 +442,16 @@ export function computeIdleSections({
} }
// Apply buffer: expand each execution range by bufferSeconds on each side, // Apply buffer: expand each execution range by bufferSeconds on each side,
// then merge overlapping ranges into consolidated "active" intervals. // clamp to video bounds, then filter out any ranges that become invalid
// (e.g. timestamps that exceed the video duration).
const buffered = executionTimestamps const buffered = executionTimestamps
.map((t) => ({ .map((t) => ({
start: Math.max(0, t.start - bufferSeconds), start: Math.max(0, t.start - bufferSeconds),
end: Math.min(totalDuration, t.end + bufferSeconds), end: Math.min(totalDuration, t.end + bufferSeconds),
})) }))
.filter((r) => {
return Number.isFinite(r.start) && Number.isFinite(r.end) && r.end > r.start
})
.sort((a, b) => { .sort((a, b) => {
return a.start - b.start return a.start - b.start
}) })