fix: audit nitpicks - bugs, security, type safety, race conditions#54
Open
lacymorrow wants to merge 1 commit into
Open
fix: audit nitpicks - bugs, security, type safety, race conditions#54lacymorrow wants to merge 1 commit into
lacymorrow wants to merge 1 commit into
Conversation
Backport of fixes applied to downstream cinematic project.
Debug logging:
- stopEvent.ts (main and utils): remove console.warn on every event
- sounds.ts: remove console.warn on preload, console.info on play
- ColorPickerInput.tsx: fix onChange={console.log} -> onChange={handleChange}
- InputComboboxForm.tsx: remove console.log(items)
- global-context.tsx: remove debug console.log calls for APP_UPDATED
Bugs:
- dialog.ts: fix copyright overwrite bug - when both options.copyright
and options.text were set, the second block overwrote the first
instead of combining them; fixed to join with filter(Boolean)
Security:
- protocol.ts: add path traversal guard to the protocol handler.
Without it, a crafted URL could escape the resources directory.
Race conditions:
- global-context.tsx: PLAY_SOUND listener was registered inside a
.then() callback, creating a window where unmounting before the
promise resolved left the listener un-cleaned. Moved registration
to synchronous code.
Type safety:
- dialog.ts: replace any types on options and aboutPanelOptions with
proper types (AboutWindowOptions interface, Electron.AboutPanelOptionsOptions)
- protocol.ts: replace any type on request param with Request
- preload.ts: once() returned void while on() returned a cleanup
function - inconsistent API. Fixed once() to also return cleanup fn.
- global-context.tsx: replace any types on notification and sound
callbacks with proper RendererNotificationOptions/string casts
- menu.ts: fix inconsistent checked type for quitOnWindowClose
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Backport of fixes from the downstream Cinematic audit pass. All bugs and nitpicks found in equivalent files have been applied here.
Bugs fixed
dialog.ts: Copyright overwrite bug — when bothoptions.copyrightandoptions.textwere set, the second block overwrote instead of combining. Fixed with a single join.Security
protocol.ts: Protocol handler had no path traversal guard — a crafted URL like `app://../../etc/passwd` could escape `__resources`. Added explicit `path.resolve` check and 403 response.Race condition
global-context.tsx: `PLAY_SOUND` listener was registered inside a `.then()` callback, creating a window where unmounting before the promise resolved would leave the listener un-cleaned. Moved registration to synchronous setup code.Type safety
dialog.ts: Replaced `any` on `showAboutWindow` options and `aboutPanelOptions` with proper types (`AboutWindowOptions` interface, `Electron.AboutPanelOptionsOptions`).protocol.ts: Replaced `any` on request param with `Request`.preload.ts: `once()` returned `void` while `on()` returned a cleanup function — inconsistent. Fixed `once` to also return `(() => void) | undefined`.global-context.tsx: Replaced `any` on notification and sound callbacks with proper types.menu.ts: Fixed inconsistent `checked` type cast on `quitOnWindowClose`.Debug logging
Test plan