Fix pre-testing review findings: legal attribution, security hardening, UX bugs#720
Closed
Alfredoalv13 wants to merge 32 commits into
Closed
Fix pre-testing review findings: legal attribution, security hardening, UX bugs#720Alfredoalv13 wants to merge 32 commits into
Alfredoalv13 wants to merge 32 commits into
Conversation
… repo references - README.md: fix LICENSE link to point at LICENSE.md (file was renamed) - README.zh-CN.md: rebrand from Recordly to VybeClip, matching README.md's MVP-focused content and structure (title, features, dev setup, license and attribution section) - CONTRIBUTING.md: point issue reporting at this fork (Alfredoalv13/Recordly) instead of the upstream repo - package.json: add top-level "license": "AGPL-3.0-or-later" field
Adds a small AboutDialog component (Radix Dialog, matching the existing FeedbackDialog/KeyboardShortcutsDialog pattern) reachable from the editor header. Shows the app name/version, Box Creative Studio, an attribution line for Recordly (AGPLv3, webadderall) and OpenScreen (MIT, Siddharth Vaddem), and a link to the full LICENSE.md, satisfying the AGPLv3 user-facing attribution requirement.
CONTRIBUTING.md still told contributors their work would be licensed under a nonexistent "./LICENSE" file under the MIT License, left over from before the project was relicensed to AGPLv3 (LICENSE.md). The prior Legal & Branding commit fixed this same stale link in README.md but missed CONTRIBUTING.md, leaving the docs internally inconsistent with the new package.json "license": "AGPL-3.0-or-later" field and the AboutDialog's AGPLv3 attribution text. Point it at LICENSE.md/AGPLv3 to match.
generate-auto-captions previously trusted any whisperExecutablePath sent from the renderer, gating it only on "is this file executable" before running it via execFileAsync — letting a compromised renderer achieve code execution in the main process via an arbitrary planted binary. Add an approvedWhisperExecutablePaths set (electron/ipc/state.ts), following the existing approvedLocalReadPaths / isTrustedProjectPath / isOwnedExportPath allowlist pattern: - open-whisper-executable-picker now records the user-selected path into the approved set (electron/ipc/register/captions.ts). - resolveWhisperExecutablePath rejects any preferredPath that isn't one of the app's bundled/default whisper binaries or in the approved set, before it's ever passed to execFileAsync (electron/ipc/captions/generate.ts).
webSecurity:false was disabling same-origin policy and mixed-content blocking on the HUD overlay, studio, and editor BrowserWindows. Git history shows it was introduced for an early prototype that played recorded video via raw file:// URLs; the app has since grown a proper local media server (mediaServer.ts, serving allow-listed files over http://127.0.0.1 with range-request support) and a packaged renderer server (rendererServer.ts, also http://127.0.0.1), but webSecurity was never re-tightened after that migration. - Remove webSecurity:false from the HUD overlay, studio, and editor windows in electron/windows.ts (source-selector, countdown, and update-toast windows already ran with the secure default). - Register a Content-Security-Policy in electron/main.ts via session.defaultSession.webRequest.onHeadersReceived, applied to all windows since they share the default session. default-src/script-src are scoped to 'self' plus the actual (randomly-assigned) mediaServer and rendererServer origins; img-src/media-src additionally allow data:/blob:/file: so the editor canvas, cached thumbnails, and user-installed extension icons (already fully-trusted local code, loaded via file:// in ExtensionIcon.tsx) keep working, and getRenderableVideoUrl()'s file:// fallback still functions if the media server is ever unavailable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…extension in localStorage
Extension settings were all stored under one shared plaintext localStorage
key, so any extension could read every other extension's settings via raw
window.localStorage access (extensions run via dynamic import() in the same
main world), bypassing the host API's per-extension scoping. Each extension
now gets its own namespaced key ("recordly.extension-settings.v1:<id>"), with
a one-time best-effort migration that copies existing settings from the old
shared key forward so already-installed extensions don't lose their data.
…uring testing phase Marketplace downloads have no checksum/signature verification and installed extension code runs with full renderer access, so gate the risk down for external testers without redesigning the trust model: - downloadAndInstallExtension() now returns a clean, user-facing "temporarily disabled during the testing phase" result instead of proceeding, gated by isRemoteMarketplaceInstallEnabled() (env-var opt-in, defaults to off). Locally bundled/first-party extensions and "install from folder" are unaffected. - Added a defense-in-depth check that refuses to install any marketplace extension whose reviewStatus isn't "approved", independent of the gate above, so pending/rejected/flagged extensions can never be installed even if remote install is later re-enabled. - Documented/hardened that extensions:install-from-folder only ever accepts the path returned by dialog.showOpenDialog() — no renderer-supplied path parameter exists on that handler — mirroring the whisper-executable-picker pattern in electron/ipc/register/captions.ts. - Updated the ExtensionManager marketplace "Browse" UI to show a clear "Coming soon" / beta notice in place of the Install button and card action, instead of letting users hit a dead-end error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…are saved or confirmed The before-quit handler used to let the app exit immediately, even while an autosave (triggered as fire-and-forget in VideoEditor.tsx) was still in flight or unsaved edits existed — so Cmd+Q could silently drop the user's latest changes with no warning, unlike window-close which already prompts. Extract the existing "unsaved changes" dialog + save-before-close IPC flow (previously inline in the editor window's close handler) into a reusable promptToSaveUnsavedChangesBeforeClosing helper, and call it from before-quit too: if there are unsaved/in-flight changes, block the quit, reuse the same Save & Close / Discard & Close / Cancel dialog, and wait for the renderer's save to finish (falling back to quitting anyway after a 5s timeout so a stuck save can't trap the app open). Guards against re-entrancy so the app.quit() retry doesn't re-prompt.
…ted locations open-project-file-at-path called loadProjectFromPath() on a renderer-supplied path with no directory confinement, unlike save-project-file which validates via isTrustedProjectPath. A compromised/malicious renderer could pass any absolute path and have its JSON contents read back to the UI. Add isTrustedOpenableProjectPath(), which mirrors isAllowedLocalReadPath's canonicalization approach: it only allows paths inside the managed projects directory (getProjectsDir()) or paths already present in the recent-projects list (which is itself only ever populated by trusted dialog-driven save/load flows), and resolves symlinks before the final check so a symlink under a trusted location cannot point outside of it. The IPC handler now rejects any path that fails this check before touching the filesystem.
…allpaper-thumbnail to approved paths resolveReadableLocalFilePath previously only checked realpath + isFile(), letting a compromised renderer read arbitrary files on disk (SSH keys, config files with secrets, etc) via the read-local-file and generate-wallpaper-thumbnail IPC channels. Both call sites only ever need the bundled asset root, the recordings/userData/temp directories, or a path the app has already approved via a real dialog.showOpenDialog result (approveUserPath/rememberApprovedLocalReadPath). Reuse the existing isAllowedLocalReadPath allowlist (already used by the media server and native exports) for both the lexical and canonicalized path, closing the gap without changing any legitimate call site's behavior.
…pp-managed directories The "write-exported-video-to-path" handler only path.resolve()'d the renderer-supplied outputPath before fs.writeFile, allowing an arbitrary absolute path (e.g. from a compromised renderer or the smokeExport URL param used by dev tooling) to overwrite any file on disk. Sibling handlers (finalize-exported-video, discard-exported-temp) already require path validation before touching a path; this path has no tracked save-dialog result to trust instead, so add an isAllowedExportWritePath check (mirroring isAllowedLocalReadPath) that confines writes to the temp, downloads, userData, and recordings directories and resolves symlinks to prevent directory-confinement bypass.
isTrustedOpenableProjectPath (added to confine open-project-file-at-path to the managed projects dir / recent-projects list) broke the RECORDLY_SMOKE_EXPORT dev/CI flow: main.ts launches the editor with a project fixture path from RECORDLY_SMOKE_EXPORT_PROJECT that intentionally lives outside those locations (e.g. a CI temp dir), so it was being rejected as untrusted. Trust that exact path only when RECORDLY_SMOKE_EXPORT=1, read directly from process.env in the main process rather than from any renderer-supplied argument, so a compromised renderer still cannot use this to smuggle in an arbitrary path. This keeps the original security intent (block arbitrary renderer-chosen project paths) intact while restoring the smoke-export tooling.
Homebrew tap PR auto-merge now requires an explicit opt-in via the HOMEBREW_TAP_AUTO_MERGE repository variable instead of defaulting to true, so releases no longer squash-merge cask updates without human review unless a maintainer explicitly enables it.
…g secrets are missing The Windows build previously warned and silently shipped an unsigned .exe when WINDOWS_SIGNING_CERTIFICATE_P12_BASE64/PASSWORD were missing. Add a dedicated "Validate Windows signing secrets" step that hard-fails (exit 1) before signing configuration runs, mirroring the macOS job's "Validate macOS signing secrets" step.
…fore extraction Pin the SHA-256 of the whisper.cpp v1.8.4 source tarball and verify the downloaded archive against it before extracting/compiling. Previously build-whisper-runtime.mjs downloaded the tarball via plain https.get and extracted it with no integrity check, so a compromised CDN, MITM, or a moved/re-tagged GitHub release could silently feed malicious source into a binary this app ships to users. The build now aborts with a clear error on checksum mismatch, and removes the untrusted archive so it can't be reused by a later cached run.
…ommitted These files were mid-edit by a concurrent process and got swept into the previous commit's index by mistake. Reverting them to their pre-commit state here; the in-progress edits are restored as unstaged working-tree changes so that process can continue and commit them separately.
…picker fails The Open Video action returned silently on result.success === false, leaving the user with no feedback on disk/permission errors. Now shows a sonner error toast in that case.
…rever The finalize promise had an unused _reject param and no listener for gif.js's "error"/"abort" events, so an encoder failure left the export promise pending indefinitely. Now both events reject with a descriptive Error so failures surface to the caller instead of silently hanging.
…s instead of silently orphaning temp files Extract the fire-and-forget discardExportedTemp call into a shared discardExportedTempWithWarning helper that console.warns with the temp file path whenever the IPC call rejects or resolves with success:false, so a stuck multi-GB export temp file is discoverable instead of vanishing without a trace. Applied at both call sites (cancelled export cleanup and component unmount cleanup).
…ated export falls back to WebCodecs When the native/NVIDIA static-layout export path throws at runtime, the exporter silently swapped in a generic skip reason and fell back to the WebCodecs path without telling the user their faster hardware-accelerated export failed. Surface a non-blocking sonner toast.warning() so the user knows why the export may be slower, while still letting the fallback proceed as before.
Skip scheduling a new RAF while one is already pending, and skip recomputing/writing the squircle clip-path when the caption box geometry (width/height/radius) hasn't actually changed, to reduce redundant work during rapid caption-layout updates (e.g. active caption editing) without altering rendered output.
…c handlers Several IPC handlers in assets.ts, captions.ts, export.ts, project.ts, and settings.ts returned raw String(error)/error.message to the renderer, which can include stack fragments and local file paths. Log the full error via console.error server-side and return a generic, safe message instead for the highest-traffic handlers (asset/local file reads, whisper model download/delete, native video export session start/probe/capabilities, folder reveal/open/set, and settings persistence). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…r existing entries Restores changes that were accidentally reverted by d0c923a during the review workflow. A hash mismatch against a recorded manifest entry now aborts the build (exit 1) instead of only warning, so a tampered/mismatched native helper binary can't silently ship in a packaged build. A missing manifest/entry (first build) still only warns, as before.
…e verified getMarketplaceExtension() already swallows its own errors and returns null, making the surrounding try/catch dead code. A null listing (network error, deleted extension, non-200) was silently treated as "review status OK" and fell through to install. Now a null listing is rejected explicitly, so the defense-in-depth check actually fails closed instead of fail-open. Found by the adversarial security-diff review at the end of the pretest fix workflow.
Contributor
|
|
Author
|
Opened against the wrong repo by mistake (gh defaulted to the upstream repo instead of my fork). Closing — the correct PR is on my fork, Alfredoalv13/Recordly. Apologies for the noise. |
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (20)
📒 Files selected for processing (100)
📝 WalkthroughWalkthroughThis PR rebrands the application from Recordly to VybeClip across build configuration, documentation, and all locale files, migrates the project file extension to ChangesVybeClip Rebrand
Security Hardening
VybeClip Studio Feature
Editor UI Theming and Export Fixes
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant VybeStudioPreview
participant studioActions
participant ElectronMain
participant HUDOverlay
VybeStudioPreview->>studioActions: startStudioRecording(api)
studioActions->>ElectronMain: check screen recording / accessibility permissions
ElectronMain-->>studioActions: permission status
studioActions->>ElectronMain: showRecordingControls()
ElectronMain->>HUDOverlay: showOrCreateRecordingControls()
studioActions->>ElectronMain: openSourceSelector()
sequenceDiagram
participant Renderer
participant ExportIPC
participant PathValidator
participant FileSystem
Renderer->>ExportIPC: write-exported-video-to-path(outputPath)
ExportIPC->>PathValidator: isAllowedExportWritePath(resolvedPath)
PathValidator->>FileSystem: realpathSync(parentDir)
FileSystem-->>PathValidator: canonical path
PathValidator-->>ExportIPC: allowed / rejected
ExportIPC-->>Renderer: success or failure response
Possibly related PRs
Suggested labels: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
|
|
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
2 participants
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
Addresses every item from the pre-MVP-testing review, in priority order, via a 10-phase fix→verify→repair workflow (each phase re-ran
tsc --noEmit+ the full test suite before moving on) plus an adversarial security-diff review at the end.Legal / branding
./LICENSElink in README.md, rebranded README.zh-CN.md (was still 100% unbranded Recordly content), updated CONTRIBUTING.md repo references, addedlicensefield to package.jsonCritical security
generate-auto-captionsaccepted an arbitrary renderer-suppliedwhisperExecutablePathand executed it — now gated by an approved-path allowlist (bundled binary or a path that actually came from the file-picker dialog)webSecurityand added a real CSP on all windows (waswebSecurity: falseapp-wide with no CSP)write-exported-video-to-path,open-project-file-at-path,read-local-file, andgenerate-wallpaper-thumbnail— all now confined to app-managed directories, consistent with the codebase's existing validated handlersBuild pipeline
UX / reliability
Post-workflow cleanup (found by the adversarial security-diff review):
getMarketplaceExtension()returningnullon error was silently treated as "approved" instead of rejected)What's intentionally NOT in this PR
Test plan
npx tsc --noEmit— cleannpm test— 786/786 tests passing, 92/92 files🤖 Generated with a Claude Code workflow (10 phases, 47 sub-agents)
Summary by CodeRabbit
New Features
Bug Fixes
Documentation