-
Notifications
You must be signed in to change notification settings - Fork 18
Feature/electron security modernization #1286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
- Migrate from webpack 4 to electron-vite for modern build system - Replace nodeIntegration with contextBridge/preload for security - Add secure IPC handlers for file system, dialog, and export operations - Remove deprecated Server pairing functionality (secure-comms-api) - Remove pairedServer redux module and related components - Replace object-hash with ohash for browser compatibility - Fix react-router import to use react-router-dom - Update SCSS to use modern Sass syntax - Switch from npm to pnpm for package management Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Move electron-log to dependencies (runtime requirement) - Move electron-devtools-installer to devDependencies (dev-only) - Update submodule references for electron-vite compatibility Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request modernizes the Electron security architecture by removing direct Node.js access from the renderer process and implementing a secure IPC bridge pattern. The changes eliminate legacy build tooling, remove deprecated server pairing functionality, and update the Node.js runtime to version 22.
Changes:
- Replaced direct Node.js access with a secure preload script and IPC-based API (
electronAPI) - Removed server discovery/pairing features and associated MDNS dependencies
- Migrated from custom Webpack configuration to electron-vite build system
- Updated Node.js from 14.21.3 to 22 and modernized dependencies
Reviewed changes
Copilot reviewed 96 out of 99 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/utils/electronAPI/index.js | New secure API abstraction layer for Electron IPC communication |
| src/utils/filesystem.js | Updated to use secure IPC instead of direct Node.js fs access |
| src/utils/Environment.js | Modified to detect Electron via secure API rather than window.require |
| src/utils/remote.js | Updated IPC event handlers to use secure electronAPI |
| src/utils/exportProcess.js | Refactored to use IPC-based export handlers instead of direct module access |
| public/preload/previewPreload.js | New preload script implementing secure IPC bridge with contextBridge |
| public/components/ipcHandlers.js | New main process IPC handlers for secure renderer communication |
| public/components/exportHandler.js | New export-related IPC handlers for main process |
| electron.vite.config.js | New build configuration replacing legacy Webpack setup |
| package.json | Updated dependencies and build scripts for electron-vite |
Comments suppressed due to low confidence (5)
src/utils/networkFormat.js:1
- The
errorMessagevariable is used instead oferrorin the conditional check. Line 136 checkserrorMessage instanceof ErrorbuterrorMessageis only assigned later and should be checkingerror instanceof Error.
src/utils/protocol/getAssetUrl.js:1 - The code checks
isElectron()but then also checkswindow.electronAPI?.envwhich is redundant. IfisElectron()returns true, it already verifieswindow.electronAPIexists. However, the logic incorrectly assumes only Electron can be in development mode - Cordova can also be in development but this always returns false for it.
src/utils/exportProcess.js:1 - The
buildReformattedProtocolsfunction is called every time export starts, butinstalledProtocolsis unlikely to change during export. This could be cached or memoized to avoid recalculating protocol IDs on every export.
src/utils/filesystem.js:1 - The
getBuffer()method creates a new concatenated buffer on every call. For large files with many chunks, this could be memory-intensive. Consider implementing a more efficient streaming approach or caching the result if called multiple times.
src/containers/App.js:1 - Polling for fullscreen state every second is inefficient. Consider adding IPC event listeners in the preload script for 'enter-full-screen' and 'leave-full-screen' events instead of polling, which would be more responsive and efficient.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check if it looks like base64 | ||
| const isBase64 = /^[A-Za-z0-9+/]+=*$/.test(data.substring(0, 100)); | ||
| if (isBase64 && data.length > 1000) { | ||
| return fse.writeFile(filePath, Buffer.from(data, 'base64')); | ||
| } | ||
| } |
Copilot
AI
Jan 12, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base64 detection logic is fragile. It only checks the first 100 characters and requires length > 1000, which could cause incorrect behavior for smaller binary files or text that happens to match the regex pattern. Consider using a more reliable method to differentiate between base64-encoded binary data and plain text, such as explicit encoding metadata.
- Remove protocol-validation git submodule (was v2.0.0) - Add @codaco/protocol-validation npm package (v5.0.2) - Move platform-specific zipValidation.js to utils/protocol/ (uses network-canvas's inEnvironment pattern) - Update imports in parseProtocol.js and extractProtocol.js Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This pull request removes several legacy configuration files and updates the Node.js version used by the project. The main goal is to clean up the codebase by eliminating custom configuration logic that is no longer needed, likely due to updates in the project's build tooling or migration to a newer setup.
The most important changes are:
Node.js version update:
.node-versionfrom14.21.3to22to ensure the project uses a modern, supported Node.js runtime.Configuration cleanup:
config/env.js, which previously managed loading.envfiles and preparing environment variables for Webpack.config/nc-dev-utils.js, which contained utilities for Cordova/Electron builds and development server integration, indicating these features are no longer supported or are handled differently.config/paths.js, which defined and resolved various project paths for the build process.config/webpack.config.dev.js, the custom Webpack development configuration, suggesting a move to a standard or externally managed configuration.