Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions electron/main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,30 @@ if (hasSingleInstanceLock) {
app.quit();
}

// Forward renderer console output to the main process so it appears in the
// terminal where `npm run dev` is running. Without this, `console.info` /
// `console.warn` / `console.error` calls in the renderer only show up in
// DevTools, which is invisible when the user is following terminal
// instructions (see issue #8 and PR #11).
app.on("browser-window-created", (_event, window) => {
window.webContents.on("console-message", (details) => {
// New API: details.level is "info" | "warning" | "error" | "debug".
// Skip debug to keep the terminal readable; the user can still inspect
// DevTools for that.
const { level, message } = details;
if (level === "debug") return;
const tag = level.toUpperCase();
const line = `[renderer ${tag}] ${message}`;
if (level === "error") {
console.error(line);
} else if (level === "warning") {
console.warn(line);
} else {
console.log(line);
}
});
Comment on lines +134 to +150

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Gate renderer-console forwarding behind an explicit diagnostic flag.

On Line 134, this is enabled unconditionally for every window. Since renderer logs include user paths/state (e.g., export diagnostics), this can leak sensitive data into terminal/system logs in normal runs. Make forwarding opt-in (or dev-only) and keep it disabled by default in packaged builds.

Suggested patch
- app.on("browser-window-created", (_event, window) => {
- 	window.webContents.on("console-message", (details) => {
+const shouldForwardRendererConsole =
+	!app.isPackaged || process.env.OPENSCREEN_ENABLE_RENDERER_CONSOLE_FORWARDING === "1";
+
+if (shouldForwardRendererConsole) {
+	app.on("browser-window-created", (_event, window) => {
+		window.webContents.on("console-message", (details) => {
 		// New API: details.level is "info" | "warning" | "error" | "debug".
 		// Skip debug to keep the terminal readable; the user can still inspect
 		// DevTools for that.
 		const { level, message } = details;
 		if (level === "debug") return;
 		const tag = level.toUpperCase();
 		const line = `[renderer ${tag}] ${message}`;
 		if (level === "error") {
 			console.error(line);
 		} else if (level === "warning") {
 			console.warn(line);
 		} else {
 			console.log(line);
 		}
-	});
-});
+		});
+	});
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
app.on("browser-window-created", (_event, window) => {
window.webContents.on("console-message", (details) => {
// New API: details.level is "info" | "warning" | "error" | "debug".
// Skip debug to keep the terminal readable; the user can still inspect
// DevTools for that.
const { level, message } = details;
if (level === "debug") return;
const tag = level.toUpperCase();
const line = `[renderer ${tag}] ${message}`;
if (level === "error") {
console.error(line);
} else if (level === "warning") {
console.warn(line);
} else {
console.log(line);
}
});
const shouldForwardRendererConsole =
!app.isPackaged || process.env.OPENSCREEN_ENABLE_RENDERER_CONSOLE_FORWARDING === "1";
if (shouldForwardRendererConsole) {
app.on("browser-window-created", (_event, window) => {
window.webContents.on("console-message", (details) => {
// New API: details.level is "info" | "warning" | "error" | "debug".
// Skip debug to keep the terminal readable; the user can still inspect
// DevTools for that.
const { level, message } = details;
if (level === "debug") return;
const tag = level.toUpperCase();
const line = `[renderer ${tag}] ${message}`;
if (level === "error") {
console.error(line);
} else if (level === "warning") {
console.warn(line);
} else {
console.log(line);
}
});
});
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@electron/main.ts` around lines 134 - 150, The renderer console message
listener in the "browser-window-created" event handler is enabled
unconditionally for all windows, which can leak sensitive user data (paths,
state, diagnostics) into terminal and system logs during normal operation. Wrap
the window.webContents.on("console-message") listener registration inside a
conditional check that only enables it when an explicit diagnostic flag is set,
and ensure this flag is disabled by default in packaged builds. This makes
console forwarding opt-in rather than always-on.

});

function isEditorWindow(window: BrowserWindow) {
return window.webContents.getURL().includes("windowType=editor");
}
Expand Down
60 changes: 60 additions & 0 deletions src/components/video-editor/VideoEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,31 @@ function buildSaveDiagnosticMessage(formatLabel: "GIF" | "Video", reason?: strin
return `${formatLabel} export save failed${reason ? `\nReason: ${reason}` : ""}`;
}

function getPreviewVideoDiagnostics(video: HTMLVideoElement | null) {
if (!video) {
return { present: false };
}

return {
present: true,
src: video.currentSrc || video.src,
readyState: video.readyState,
networkState: video.networkState,
error: video.error
? {
code: video.error.code,
message: video.error.message,
}
: null,
currentTime: Number.isFinite(video.currentTime) ? video.currentTime : null,
duration: Number.isFinite(video.duration) ? video.duration : null,
paused: video.paused,
ended: video.ended,
videoWidth: video.videoWidth,
videoHeight: video.videoHeight,
};
}

const CAPTION_WORD_CHOICES = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12] as const;

export default function VideoEditor() {
Expand Down Expand Up @@ -1855,6 +1880,33 @@ export default function VideoEditor() {
setExportError(null);
setExportedFilePath(null);

const previewBeforeExport = getPreviewVideoDiagnostics(video);
console.info(
`[VideoEditor] export started ${JSON.stringify({
format: settings.format,
sourcePath: videoSourcePath ?? videoPath,
videoPath,
webcamVideoPath,
preview: previewBeforeExport,
settings: {
exportQuality: settings.quality || exportQuality,
aspectRatio,
padding,
borderRadius,
shadowIntensity,
showBlur,
motionBlurAmount,
cropRegion,
zoomRegions: zoomRegions.length,
trimRegions: trimRegions.length,
speedRegions: speedRegions.length,
annotations: annotationRegions.length,
effectiveShowCursor,
cursorSize,
},
})}`,
);
Comment on lines +1883 to +1908

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Avoid logging raw source file paths in export traces.

These diagnostics include raw sourcePath / videoPath / webcamVideoPath, and previewBefore / previewAfter also carry src. Because these traces are meant to be shared, they can leak local filesystem details. Please reuse getFileNameForDiagnostics() (or similar redaction) before serializing.

Also applies to: 2135-2142

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/video-editor/VideoEditor.tsx` around lines 1883 - 1908, The
export trace in VideoEditor’s export logging is serializing raw filesystem paths
and preview metadata with src values, which can leak local file details. Update
the console.info diagnostics in the export flow (including the
previewBeforeExport/previewAfter data and the export payload) to redact
path-like fields using getFileNameForDiagnostics() or equivalent before
JSON.stringify. Apply the same sanitization in the other export trace block
referenced by the comment so both logs consistently omit raw sourcePath,
videoPath, and webcamVideoPath values.


try {
const wasPlaying = isPlaying;
if (wasPlaying) {
Expand Down Expand Up @@ -2080,6 +2132,14 @@ export default function VideoEditor() {
toast.error(t("errors.exportFailedWithError", { error: message }));
}
} finally {
console.info(
`[VideoEditor] export finished ${JSON.stringify({
sourcePath: videoSourcePath ?? videoPath,
videoPath,
previewBefore: previewBeforeExport,
previewAfter: getPreviewVideoDiagnostics(videoPlaybackRef.current?.video ?? null),
})}`,
);
setIsExporting(false);
exporterRef.current = null;
// Reset so the next export can reopen the dialog (second export
Expand Down
41 changes: 40 additions & 1 deletion src/components/video-editor/VideoPlayback.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,26 @@ function enableAllPreviewAudioTracks(video: HTMLVideoElement) {
}
}

function getVideoElementDiagnostics(video: HTMLVideoElement) {
return {
src: video.currentSrc || video.src,
readyState: video.readyState,
networkState: video.networkState,
error: video.error
? {
code: video.error.code,
message: video.error.message,
}
: null,
currentTime: Number.isFinite(video.currentTime) ? video.currentTime : null,
duration: Number.isFinite(video.duration) ? video.duration : null,
paused: video.paused,
ended: video.ended,
videoWidth: video.videoWidth,
videoHeight: video.videoHeight,
};
}

const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
(
{
Expand Down Expand Up @@ -345,6 +365,18 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
const speedRegionsRef = useRef<SpeedRegion[]>([]);
const motionBlurAmountRef = useRef(motionBlurAmount);
const cursorOverlayRef = useRef<PixiCursorOverlay | null>(null);

const logPreviewVideoEvent = useCallback(
(event: React.SyntheticEvent<HTMLVideoElement>) => {
console.info(
`[VideoPlayback] preview video ${event.type} ${JSON.stringify({
videoPath,
diagnostics: getVideoElementDiagnostics(event.currentTarget),
})}`,
Comment on lines +369 to +375

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Redact local paths from preview diagnostics.

This payload currently emits both videoPath and diagnostics.src verbatim. For local file:// sources that exposes usernames/home-directory paths in terminal logs that users may paste into bug reports. Log a basename or stable redacted token instead.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/components/video-editor/VideoPlayback.tsx` around lines 369 - 375, The
preview logging in logPreviewVideoEvent is leaking local file paths by emitting
videoPath and diagnostics.src verbatim. Update the VideoPlayback preview event
logging to redact local file:// sources before calling console.info, replacing
the raw path with a basename or a stable token while keeping the rest of
getVideoElementDiagnostics output intact. Ensure any reference to videoPath or
diagnostics.src in this callback is sanitized so terminal logs do not expose
user home-directory details.

);
},
[videoPath],
);
const showCursorRef = useRef(showCursor);
const cursorSizeRef = useRef(cursorSize);
const cursorSmoothingRef = useRef(cursorSmoothing);
Expand Down Expand Up @@ -2152,7 +2184,14 @@ const VideoPlayback = forwardRef<VideoPlaybackRef, VideoPlaybackProps>(
forceResolveDuration(e.currentTarget);
}
}}
onError={() => onError("Failed to load video")}
onEmptied={logPreviewVideoEvent}
onStalled={logPreviewVideoEvent}
onSuspend={logPreviewVideoEvent}
onAbort={logPreviewVideoEvent}
onError={(event) => {
logPreviewVideoEvent(event);
onError("Failed to load video");
}}
/>
{supplementalAudioPath && (
<audio ref={supplementalAudioRef} src={supplementalAudioPath} preload="auto" />
Expand Down
21 changes: 21 additions & 0 deletions src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,27 @@ import App from "./App.tsx";
import { I18nProvider } from "./contexts/I18nContext";
import "./index.css";

// === One-shot uncaught trap — capture stack traces for the .geometry null deref in #8.
// Delete after the diagnostic run that closes out the issue's root cause.
const __errTrap = (tag: string) => (e: ErrorEvent | PromiseRejectionEvent) => {
const isRej = "reason" in e;
const err = isRej ? (e as PromiseRejectionEvent).reason : (e as ErrorEvent).error;
const evt = e as ErrorEvent;
console.error(
`[VideoEditor] ${tag}`,
JSON.stringify({
message: err?.message ?? String(err),
source: isRej ? null : evt.filename,
lineno: isRej ? null : evt.lineno,
colno: isRej ? null : evt.colno,
stack: err?.stack ?? null,
Comment on lines +16 to +20

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Fall back to ErrorEvent.message for non-rejection errors.

On window "error" events, event.error is often missing even when event.message, filename, and line info are populated. The current fallback logs "undefined"/"null" in that case, which defeats the diagnostic value of this trap for a common browser error shape.

Suggested fix
 		JSON.stringify({
-			message: err?.message ?? String(err),
+			message: isRej
+				? err?.message ?? String(err)
+				: err?.message ?? evt.message ?? String(err),
 			source: isRej ? null : evt.filename,
 			lineno: isRej ? null : evt.lineno,
 			colno: isRej ? null : evt.colno,
 			stack: err?.stack ?? null,
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
message: err?.message ?? String(err),
source: isRej ? null : evt.filename,
lineno: isRej ? null : evt.lineno,
colno: isRej ? null : evt.colno,
stack: err?.stack ?? null,
message: isRej
? err?.message ?? String(err)
: err?.message ?? evt.message ?? String(err),
source: isRej ? null : evt.filename,
lineno: isRej ? null : evt.lineno,
colno: isRej ? null : evt.colno,
stack: err?.stack ?? null,
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main.tsx` around lines 16 - 20, The window error handler in main.tsx is
using err?.message as the only message source, which logs undefined/null when
event.error is missing on non-rejection ErrorEvent cases. Update the error-trap
logic around the event handling to fall back to evt.message for non-rejection
errors before String(err), while keeping the existing source, lineno, colno, and
stack behavior in the same handler.

}),
);
};
window.addEventListener("error", __errTrap("uncaught"));
window.addEventListener("unhandledrejection", __errTrap("unhandledrejection"));
// === end trap ===

const windowType = new URLSearchParams(window.location.search).get("windowType") || "";
if (
windowType === "hud-overlay" ||
Expand Down
Loading