fix: address PR review feedback - canonicalize via realpath and gate whisper soft-fail

isAllowedLocalReadPath now resolves the candidate via fs.realpathSync and requires both the lexical and canonical paths to satisfy the policy, so a symlink placed under an allowed prefix that points outside the allowlist is rejected. The redundant 'either resolved or realResolved is allowed' check in read-local-file and generate-wallpaper-thumbnail is removed since the function canonicalizes internally. Adds a regression test that creates such a symlink (skipping when Windows refuses to create it without Developer Mode).

build-whisper-runtime now only soft-fails when invoked from postinstall, in CI, or with WHISPER_RUNTIME_ALLOW_MISSING=1. Direct 'npm run build*' invocations fail loudly when CMake is missing and no bundled runtime is staged so we don't ship release builds with broken auto-captioning.
This commit is contained in:
Recordly Reviewer
2026-05-04 14:31:09 -04:00
parent dce19d5209
commit b0ab0bf184
4 changed files with 85 additions and 21 deletions
+23
View File
@@ -114,6 +114,29 @@ describe("local media path policy", () => {
expect(isAllowedMediaPath(textPath)).toBe(false);
});
it("rejects symlinks under allowed prefixes that point outside the allowlist", async () => {
const outsideTarget = path.join(tempRoot, "outside-secret.mp4");
const symlinkInsideUserData = path.join(userDataPath, "shortcut-to-secret.mp4");
await fs.writeFile(outsideTarget, "secret-bytes");
try {
await fs.symlink(outsideTarget, symlinkInsideUserData);
} catch (error) {
// Windows requires Developer Mode or admin to create file symlinks. If
// we can't create one, the bypass we're guarding against also can't be
// crafted on this machine, so skipping is safe.
if ((error as NodeJS.ErrnoException).code === "EPERM") {
return;
}
throw error;
}
const { isAllowedLocalMediaPath, resolveApprovedLocalMediaPath } = await import("./manager");
await expect(isAllowedLocalMediaPath(symlinkInsideUserData)).resolves.toBe(false);
await expect(resolveApprovedLocalMediaPath(symlinkInsideUserData)).resolves.toBeNull();
});
it("preserves an existing project thumbnail when no replacement is provided", async () => {
const projectPath = path.join(tempRoot, "Projects", "demo.recordly");
const thumbnailDataUrl = `data:image/png;base64,${Buffer.from("png-thumbnail").toString("base64")}`;
+31 -6
View File
@@ -1,5 +1,5 @@
import { constants as fsConstants } from "node:fs";
import { existsSync } from "node:fs";
import { existsSync, realpathSync } from "node:fs";
import fs from "node:fs/promises";
import path from "node:path";
import { app } from "electron";
@@ -54,14 +54,39 @@ export function isAllowedLocalReadPath(candidatePath: string) {
const allowedPrefixes = [RECORDINGS_DIR, USER_DATA_PATH, getAssetRootPath(), app.getPath("temp")];
const normalizedCandidatePath = normalizePath(candidatePath);
// Canonicalize so a symlink placed under an allowed prefix can't smuggle in a
// target that lives outside it. realpathSync throws when the path doesn't
// exist yet (e.g. a pending export approved before the file is written) — in
// that case fall back to the lexical path, which can only succeed via the
// approvedLocalReadPaths check below since no symlink target exists yet.
let canonicalCandidatePath = normalizedCandidatePath;
try {
canonicalCandidatePath = normalizePath(realpathSync(normalizedCandidatePath));
} catch {
// File may not exist yet; keep the lexical path.
}
// 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 (
// dialog, app-produced exports). The lexical path must satisfy the policy
// AND the canonical (real) path must satisfy it too, so a symlink under an
// allowed prefix that points outside the allowlist is rejected. 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.
const lexicalAllowed =
allowedPrefixes.some((prefix) => isPathInsideDirectory(normalizedCandidatePath, prefix)) ||
approvedLocalReadPaths.has(normalizedCandidatePath)
approvedLocalReadPaths.has(normalizedCandidatePath);
if (!lexicalAllowed) {
return false;
}
if (canonicalCandidatePath === normalizedCandidatePath) {
return true;
}
return (
allowedPrefixes.some((prefix) => isPathInsideDirectory(canonicalCandidatePath, prefix)) ||
approvedLocalReadPaths.has(canonicalCandidatePath)
);
}
+5 -5
View File
@@ -19,9 +19,10 @@ export function registerAssetHandlers() {
ipcMain.handle('generate-wallpaper-thumbnail', async (_, filePath: string) => {
try {
const resolved = normalizePath(filePath)
const realResolved = await fs.realpath(resolved).catch(() => resolved)
if (!isAllowedLocalReadPath(resolved) && !isAllowedLocalReadPath(realResolved)) {
// isAllowedLocalReadPath now canonicalizes via realpath internally and
// requires both the lexical and real paths to satisfy the policy, so a
// single check covers symlinks under allowed prefixes.
if (!isAllowedLocalReadPath(resolved)) {
return { success: false, error: 'Access denied' }
}
@@ -106,8 +107,7 @@ export function registerAssetHandlers() {
ipcMain.handle('read-local-file', async (_, filePath: string) => {
try {
const resolved = normalizePath(filePath)
const realResolved = await fs.realpath(resolved).catch(() => resolved)
if (!isAllowedLocalReadPath(resolved) && !isAllowedLocalReadPath(realResolved)) {
if (!isAllowedLocalReadPath(resolved)) {
console.warn(`[read-local-file] Blocked read outside allowed directories: ${resolved}`)
return { success: false, error: 'Access denied: path outside allowed directories' }
}
+26 -10
View File
@@ -365,12 +365,20 @@ async function main() {
const cmake = findCmake();
if (!cmake) {
// 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.
// Soft-fail only when this script runs as part of `npm install`/`npm ci`,
// in CI, or when the developer explicitly opted in. Direct invocations
// (e.g. via `npm run build`, `build:win`, `build:mac`, `build:linux`)
// must still fail loudly so we never ship a release build that is
// missing the whisper runtime and silently ships broken auto-captions.
const isPostinstall = process.env.npm_lifecycle_event === "postinstall";
const isCI = process.env.CI === "true";
const allowMissing = process.env.WHISPER_RUNTIME_ALLOW_MISSING === "1";
const softFailAllowed = isPostinstall || isCI || allowMissing;
const skipChecks = await Promise.all(targets.map((target) => shouldSkipBuild(target)));
if (skipChecks.every(Boolean)) {
const allTargetsStaged = skipChecks.every(Boolean);
if (allTargetsStaged) {
console.log(
"[build-whisper-runtime] CMake not found; using bundled whisper runtime artifacts.",
);
@@ -381,12 +389,20 @@ async function main() {
.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`.",
if (softFailAllowed) {
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;
}
throw new Error(
`[build-whisper-runtime] CMake is required to stage the whisper runtime for: ${missing}. ` +
"Install CMake and retry, or set WHISPER_RUNTIME_ALLOW_MISSING=1 to build without auto-caption support.",
);
return;
}
const sourceDir = await ensureSourceTree();