From dce19d520948416e3259518fe38ffdc10a8623c9 Mon Sep 17 00:00:00 2001 From: Recordly Reviewer Date: Mon, 4 May 2026 14:21:49 -0400 Subject: [PATCH] fix: tighten local read allowlist and gracefully skip whisper-runtime build without CMake isAllowedLocalReadPath previously returned true for any existing path because of an existsSync fast-path, which made the read-local-file IPC handler and the local media URL policy effectively allow reading arbitrary files on disk. Drop the existsSync bypass so only paths under app-managed directories or paths that have been explicitly approved (via dialogs, exports, recording sessions, etc.) are accepted. Adjust the local media path policy tests to cover the new behaviour. Also make build-whisper-runtime fall back to bundled artifacts when CMake is missing (mirroring build-windows-capture) so npm ci does not fail on machines without a C++ toolchain. --- electron/ipc/project/manager.test.ts | 20 ++++++++++++++++---- electron/ipc/project/manager.ts | 6 +++++- scripts/build-whisper-runtime.mjs | 26 +++++++++++++++++++++++--- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/electron/ipc/project/manager.test.ts b/electron/ipc/project/manager.test.ts index 51fca45d..2fc02df1 100644 --- a/electron/ipc/project/manager.test.ts +++ b/electron/ipc/project/manager.test.ts @@ -47,13 +47,17 @@ describe("local media path policy", () => { } }); - it("allows existing exported media files outside the session directories", async () => { + it("rejects existing media files outside allowed directories until they are approved", async () => { const downloadsPath = path.join(tempRoot, "Downloads"); const exportPath = path.join(downloadsPath, "export-test.mp4"); await fs.mkdir(downloadsPath, { recursive: true }); await fs.writeFile(exportPath, "test-video"); - const { isAllowedLocalMediaPath } = await import("./manager"); + const { isAllowedLocalMediaPath, rememberApprovedLocalReadPath } = await import("./manager"); + + await expect(isAllowedLocalMediaPath(exportPath)).resolves.toBe(false); + + await rememberApprovedLocalReadPath(exportPath); await expect(isAllowedLocalMediaPath(exportPath)).resolves.toBe(true); }); @@ -74,17 +78,25 @@ describe("local media path policy", () => { await expect(isAllowedLocalMediaPath(pendingExportPath)).resolves.toBe(true); }); - it("approves media-server access for existing external files resolved through the URL policy", async () => { + it("approves media-server access for approved external files resolved through the URL policy", async () => { const downloadsPath = path.join(tempRoot, "Downloads"); const videoPath = path.join(downloadsPath, "external-video.mp4"); await fs.mkdir(downloadsPath, { recursive: true }); await fs.writeFile(videoPath, "test-video"); const resolvedVideoPath = await fs.realpath(videoPath); - const { resolveApprovedLocalMediaPath } = await import("./manager"); + const { resolveApprovedLocalMediaPath, rememberApprovedLocalReadPath } = await import( + "./manager" + ); const { isAllowedMediaPath } = await import("../../mediaServer"); + // Unapproved external paths are rejected before they ever reach the media server. expect(isAllowedMediaPath(videoPath)).toBe(false); + await expect(resolveApprovedLocalMediaPath(videoPath)).resolves.toBeNull(); + + // Once the user opts in (via dialog/export/etc.) the path is approved. + await rememberApprovedLocalReadPath(videoPath); + await expect(resolveApprovedLocalMediaPath(videoPath)).resolves.toBe(resolvedVideoPath); expect(isAllowedMediaPath(videoPath)).toBe(true); }); diff --git a/electron/ipc/project/manager.ts b/electron/ipc/project/manager.ts index 9e61605a..c40b6cac 100644 --- a/electron/ipc/project/manager.ts +++ b/electron/ipc/project/manager.ts @@ -54,8 +54,12 @@ export function isAllowedLocalReadPath(candidatePath: string) { const allowedPrefixes = [RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp")]; const normalizedCandidatePath = normalizePath(candidatePath); + // Security: only allow paths under app-managed directories or paths the user + // has explicitly opted into (recording session sources, files chosen via + // dialog, app-produced exports). Previously this returned true for any + // existing file, which made the allowlist a no-op for read-local-file and + // the local media URL handler. return ( - existsSync(normalizedCandidatePath) || allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) || approvedLocalReadPaths.has(normalizedCandidatePath) ); diff --git a/scripts/build-whisper-runtime.mjs b/scripts/build-whisper-runtime.mjs index 13c944e6..b1ebd8ce 100644 --- a/scripts/build-whisper-runtime.mjs +++ b/scripts/build-whisper-runtime.mjs @@ -361,15 +361,35 @@ async function stageRuntimeArtifacts(target, candidateDir, runtimeEntries) { } async function main() { + const targets = getTargetConfigs(); const cmake = findCmake(); + if (!cmake) { - throw new Error( - "[build-whisper-runtime] CMake is required to build the bundled Whisper runtime.", + // Mirror build-windows-capture: if every target already has a staged + // runtime, postinstall is a no-op. This keeps `npm ci` working for + // contributors who do not have CMake installed and only need to run the + // app or tests against the bundled binaries. + const skipChecks = await Promise.all(targets.map((target) => shouldSkipBuild(target))); + if (skipChecks.every(Boolean)) { + console.log( + "[build-whisper-runtime] CMake not found; using bundled whisper runtime artifacts.", + ); + return; + } + + const missing = targets + .filter((_target, index) => !skipChecks[index]) + .map((target) => target.archTag) + .join(", "); + console.warn( + `[build-whisper-runtime] CMake not found and no bundled runtime is staged for: ${missing}. ` + + "Auto-caption features that rely on whisper.cpp will be unavailable until you install CMake " + + "and rerun `npm run build:whisper-runtime`.", ); + return; } const sourceDir = await ensureSourceTree(); - const targets = getTargetConfigs(); console.log( `[build-whisper-runtime] Target architectures for ${process.platform}: ${targets.map((target) => target.archTag).join(", ")}`,