feat: Electron enable context isolation #9358
Conversation
- Enable contextIsolation: true in main window webPreferences - Refactor preload-app.ts to use contextBridge.exposeInMainWorld() - Update all renderer code to use window.electronAPI.* instead of direct window properties - Fix locale module to lazy-load electronAPI to avoid timing issues - Remove path module usage from preload scripts (preparation for sandboxing) - Add documentation: ELECTRON_REMOTE_AUDIT.md and PR_BREAKDOWN.md This is PR 1 of the Electron security hardening effort.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances Electron security by enabling context isolation for the main window, which prevents renderer processes from directly accessing Node.js and Electron APIs. This is a significant security hardening change that isolates contexts and uses the contextBridge API to expose only necessary functionality to the renderer.
Key changes:
- Enabled
contextIsolation: truein main window webPreferences - Refactored preload scripts to use
contextBridge.exposeInMainWorld()for API exposure - Updated all renderer code to access APIs via
window.electronAPInamespace - Enhanced context menu actions to handle webviews correctly by passing webContents IDs
- Added error handling for account removal operations
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| electron/src/main.ts | Enables context isolation in webPreferences |
| electron/src/types/globals.d.ts | Updates Window interface to define electronAPI structure |
| electron/src/preload/preload-app.ts | Exposes APIs via contextBridge; adds webContents ID handling for edit operations |
| electron/src/preload/preload-webview.ts | Updates logger initialization to use string literal |
| electron/src/preload/menu/preload-context.ts | Adds webContents ID parameter to menu actions and direct webContents operations for webviews |
| electron/renderer/src/lib/locale.ts | Adds lazy loading pattern for locale info from electronAPI |
| electron/renderer/src/lib/WindowUrl.ts | Updates to use getWrapperLocale() function |
| electron/renderer/src/components/context/EditAccountMenu.tsx | Updates to use electronAPI and adds error handling for account deletion |
| electron/renderer/src/components/WebView/Webview.tsx | Updates to use electronAPI; adds early return for non-existent webviews; improves error handling |
| electron/renderer/src/actions/index.ts | Updates to use electronAPI for sendBadgeCount |
| .catch(error => { | ||
| console.error('Failed to delete account:', error); | ||
| // Still abort account creation even if deletion fails | ||
| connected.abortAccountCreation(accountId); | ||
| }); |
There was a problem hiding this comment.
Error handling logs to console.error but provides no user feedback. When account deletion fails, the user should be notified through the UI (e.g., a toast or dialog) rather than silently failing with only a console log. This is important for user experience as they may not know the operation failed.
| webview.blur(); | ||
| webview.focus(); |
There was a problem hiding this comment.
The focusWebviewByContentsId function attempts to focus a webview but also calls blur() first. The purpose of calling blur() before focus() is unclear and could be confusing. Consider adding a comment explaining why blur() is necessary before focus(), or remove it if it's not needed.
|
|
||
| logger.info(`Processing deletion of "${truncatedId}"`); | ||
| const viewInstanceId = accountWebview.getWebContentsId(); | ||
| ipcRenderer.on(EVENT_TYPE.ACCOUNT.DATA_DELETED, () => resolve()); |
There was a problem hiding this comment.
The ipcRenderer.on listener for EVENT_TYPE.ACCOUNT.DATA_DELETED is registered every time sendDeleteAccount is called, but never removed. This creates a memory leak where multiple listeners accumulate with each account deletion. The listener should be removed after it fires using ipcRenderer.once instead of ipcRenderer.on, or explicitly removed in the callback.
| ipcRenderer.on(EVENT_TYPE.ACCOUNT.DATA_DELETED, () => resolve()); | |
| ipcRenderer.once(EVENT_TYPE.ACCOUNT.DATA_DELETED, () => resolve()); |
| submitDeepLink: (url: string): void => { | ||
| ipcRenderer.send(EVENT_TYPE.ACTION.DEEP_LINK_SUBMIT, url); | ||
| }; | ||
|
|
||
| window.sendDeleteAccount = (accountId: string, sessionID?: string): Promise<void> => { | ||
| }, |
There was a problem hiding this comment.
Input validation is missing for the submitDeepLink function. The url parameter should be validated to ensure it's a properly formatted URL and matches expected patterns before being sent via IPC to prevent potential injection or processing of malicious URLs.
| sendBadgeCount: (count: number, ignoreFlash: boolean): void => { | ||
| ipcRenderer.send(EVENT_TYPE.UI.BADGE_COUNT, {count, ignoreFlash}); | ||
| }; | ||
|
|
||
| window.submitDeepLink = (url: string): void => { | ||
| }, |
There was a problem hiding this comment.
Input validation is missing for sendBadgeCount. The count parameter should be validated to ensure it's a non-negative integer to prevent unexpected behavior or display issues with badge counts.
| ipcRenderer.send(EVENT_TYPE.ACCOUNT.DELETE_DATA, viewInstanceId, accountId, sessionID); | ||
| } catch (error) { | ||
| // eslint-disable-next-line prefer-promise-reject-errors | ||
| reject(`Failed to get webContents ID for account "${truncatedId}": ${error}`); |
There was a problem hiding this comment.
The error variable should be included in the rejection message. Currently the template string includes ${error} but 'error' is the catch parameter name. Consider using String(error) or error.message to ensure proper error information is passed in the rejection.
| reject(`Failed to get webContents ID for account "${truncatedId}": ${error}`); | |
| const errorMessage = error instanceof Error ? error.message : String(error); | |
| reject(`Failed to get webContents ID for account "${truncatedId}": ${errorMessage}`); |
| .catch(error => { | ||
| console.error('Failed to delete account:', error); | ||
| // Still abort account creation even if deletion fails | ||
| abortAccountCreation(account.id); | ||
| }); |
There was a problem hiding this comment.
Error handling logs to console.error but provides no user feedback. When account deletion fails, the user should be notified through the UI (e.g., a toast or dialog) rather than silently failing with only a console log. This is important for user experience as they may not know the operation failed.
EnricoSchw
left a comment
EnricoSchw
left a comment
There was a problem hiding this comment.
Hi @adamlow-wire ,
thx for this PR, we need to test this change thoroughly. Because If we place the app in an isolated context, this change will affect our calling.
Please note that audio and video streams are singletons. The isolated context may make them inaccessible in parts of our app. Therefore, we need to test this changes.
Furthermore, our app runs in a sandbox on Mac, which maybe makes things more complicated.
That's why I built the app locally, signed it, and launched it in a sandbox. Unfortunately, I couldn't get past the login screen. Here the error I see.
Unfortunately, it's not very informative, but maybe you can make something of it.
3 participants
Pull Request
Summary
This PR enables Electron context isolation for the main window.
What Changed
contextIsolation: true) in main window webPreferencescontextBridge.exposeInMainWorld()instead of direct window property assignmentswindow.electronAPI.*instead of direct window propertiesTesting
Manual testing of context menu's, account addition/removal, can't test SSO as don't have the setup to do so.
Security Checklist (required)
Standards Acknowledgement (required)
Screenshots or demo (if the user interface changed)
No UI changes - this is a security hardening change that maintains existing functionality.
Notes for reviewers
Testing Checklist