fix: Critical security vulnerabilities#1312
fix: Critical security vulnerabilities#1312Zeyad-Azima wants to merge 1 commit intoThinkInAIXYZ:devfrom
Conversation
Fixed two critical security vulnerabilities: 1. File System Access Vulnerability (CVSS 9.1 CRITICAL) - Enabled webSecurity: true to prevent file:// access - Enabled sandbox: true for process isolation - Prevents arbitrary file reads (SSH keys, AWS credentials, etc.) 2. Unsafe URL Handler (CVSS 8.8 HIGH) - Replaced direct shell.openExternal() with secure IPC handler - Added protocol whitelist (only http/https allowed) - Added domain whitelist for trusted sites - Added user confirmation for untrusted domains - Prevents malicious app launches and phishing attacks Security improvements: - Blocks file:// protocol access from renderer - Blocks dangerous protocols (smb://, ssh://, etc.) - User confirmation required for untrusted external links - Trusted domains (OpenAI, Anthropic, GitHub) work seamlessly Files changed: - src/main/presenter/windowPresenter/FloatingChatWindow.ts - src/preload/index.ts - src/main/presenter/index.ts
📝 WalkthroughWalkthroughThis PR hardens external URL handling security by implementing a new IPC-based secure handler with protocol and domain whitelisting, user confirmation for untrusted domains, and enables Electron webSecurity and sandbox features in the FloatingChatWindow. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer Process<br/>(FloatingChatWindow)
participant Preload as Preload Script
participant IPC as IPC Channel
participant MainPresenter as Main Presenter
participant Dialog as Dialog
participant Shell as shell.openExternal
Renderer->>Preload: api.openExternal(url)
Preload->>IPC: ipcRenderer.invoke('open-external-secure', url)
IPC->>MainPresenter: open-external-secure handler
MainPresenter->>MainPresenter: Validate protocol (http/https)
MainPresenter->>MainPresenter: Check domain whitelist
alt Domain whitelisted
MainPresenter->>Shell: shell.openExternal(url)
Shell-->>MainPresenter: success
MainPresenter-->>IPC: {success: true}
else Domain not whitelisted
MainPresenter->>Dialog: Show confirmation dialog
Dialog-->>MainPresenter: User response
alt User confirms
MainPresenter->>Shell: shell.openExternal(url)
Shell-->>MainPresenter: success
MainPresenter-->>IPC: {success: true}
else User denies
MainPresenter-->>IPC: {success: false, error: "User denied"}
end
end
IPC-->>Preload: response
Preload-->>Renderer: Promise resolved
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/presenter/windowPresenter/FloatingChatWindow.ts (1)
89-96:⚠️ Potential issue | 🟠 MajorGood security hardening, but critical inconsistencies in other window implementations.
While
webSecurity: trueandsandbox: truecorrectly harden this window, verification reveals insecure configurations elsewhere:
src/main/presenter/windowPresenter/index.tsusessandbox: false(multiple instances)src/main/presenter/floatingButtonPresenter/FloatingButtonWindow.tsuseswebSecurity: falseandsandbox: falsesrc/main/presenter/tabPresenter.tsusessandbox: falseAll BrowserWindow instances must use secure defaults. Disable only when absolutely necessary with documented justification.
🤖 Fix all issues with AI agents
In `@src/preload/index.ts`:
- Around line 45-48: The preload API's openExternal implementation returns
ipcRenderer.invoke('open-external-secure', url) which resolves to {success:
boolean, error?: string} but index.d.ts still types openExternal as
Promise<void>; update the declaration for openExternal in index.d.ts to
Promise<{success: boolean, error?: string}> and then update the caller in
AboutUsSettings.vue (the code invoking window.api.openExternal) to await the
result and check result?.success, handling/logging result?.error when success is
false.
🧹 Nitpick comments (4)
src/main/presenter/index.ts (4)
262-264: Pass the reconstructed URL toshell.openExternalto prevent URL parsing differentials.Using the original
urlstring after validating vianew URL(url)opens a (narrow) window for parsing-differential attacks ifshell.openExternalinterprets the raw string differently from theURLconstructor. Use the canonicalized form instead.Proposed fix
// 4. Safe to open - await shell.openExternal(url) - console.log('✅ SECURITY: Opened external URL:', url, isTrusted ? '(trusted)' : '(user confirmed)') + await shell.openExternal(parsedUrl.toString()) + console.log('✅ SECURITY: Opened external URL:', parsedUrl.toString(), isTrusted ? '(trusted)' : '(user confirmed)')
207-275: Missing handler cleanup indestroy()— will throw on re-registration if singleton is ever reset.
ipcMain.handlethrows if a handler for the same channel is already registered. While the singleton pattern currently prevents this, adding cleanup indestroy()makes the code resilient to future changes and aligns with the cleanup pattern already used for other presenters.Add cleanup in destroy()
destroy() { + ipcMain.removeHandler('open-external-secure') this.floatingButtonPresenter.destroy() this.tabPresenter.destroy()
225-234: Hardcoded trusted domain list — consider making it configurable or centralizing it.The trusted domains list is a static array embedded in the handler. This is fine for the initial security fix, but as the app integrates more providers, this list will need updates. Consider extracting it to a configuration constant or making it configurable via
configPresenterin a follow-up.
240-260: Potential dialog spam from a compromised renderer.A malicious or compromised renderer could repeatedly invoke
open-external-securewith untrusted URLs, flooding the user with confirmation dialogs. Consider adding a simple rate-limiter or debounce (e.g., suppress dialogs if one is already showing).
| openExternal: (url: string) => { | ||
| return shell.openExternal(url) | ||
| // ✅ SECURITY FIX: Use secure IPC instead of direct shell.openExternal | ||
| return ipcRenderer.invoke('open-external-secure', url) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "openExternal" --type=ts --type=vue -C 3 -g '!node_modules/**' -g '!src/preload/**' -g '!src/main/**'Repository: ThinkInAIXYZ/deepchat
Length of output: 92
🏁 Script executed:
rg -n "openExternal" --type ts --type js -C 3 -g '!node_modules/**' -g '!src/preload/**' -g '!src/main/**'Repository: ThinkInAIXYZ/deepchat
Length of output: 4934
🏁 Script executed:
# Read the preload file to confirm return type
head -100 src/preload/index.ts | tail -60Repository: ThinkInAIXYZ/deepchat
Length of output: 1842
🏁 Script executed:
# Search for all openExternal calls without the exclusions
fd -e ts -e tsx -e vue | xargs rg -l "openExternal" | head -20Repository: ThinkInAIXYZ/deepchat
Length of output: 505
🏁 Script executed:
cat src/preload/index.d.tsRepository: ThinkInAIXYZ/deepchat
Length of output: 637
🏁 Script executed:
# Check if AboutUsSettings.vue will fail TypeScript strict checks
grep -A 5 "const openExternalLink" src/renderer/settings/components/AboutUsSettings.vueRepository: ThinkInAIXYZ/deepchat
Length of output: 244
Type definition mismatch: Update index.d.ts to reflect actual return type.
The type definition declares openExternal?(url: string): Promise<void>, but the implementation returns ipcRenderer.invoke('open-external-secure', url), which resolves to an object with {success: boolean, error?: string}. This breaks strict TypeScript type checking.
Update src/preload/index.d.ts to:
openExternal?(url: string): Promise<{success: boolean, error?: string}>
Then update the caller in src/renderer/settings/components/AboutUsSettings.vue (line 259) to handle the new return shape:
const result = await window.api.openExternal(url)
if (!result?.success) {
console.error('Failed to open external link:', result?.error)
}🤖 Prompt for AI Agents
In `@src/preload/index.ts` around lines 45 - 48, The preload API's openExternal
implementation returns ipcRenderer.invoke('open-external-secure', url) which
resolves to {success: boolean, error?: string} but index.d.ts still types
openExternal as Promise<void>; update the declaration for openExternal in
index.d.ts to Promise<{success: boolean, error?: string}> and then update the
caller in AboutUsSettings.vue (the code invoking window.api.openExternal) to
await the result and check result?.success, handling/logging result?.error when
success is false.
|
Thank you for the detailed review and suggestions. At this stage, the current design and relatively permissive capabilities are intentional. The product is positioned as an agent playground and can be understood more like a development tool. We are not planning to introduce additional restrictions that would limit functionality in this area for now. In addition, the IPC restructuring and invocation changes proposed in this PR do not align with the architectural conventions of this project. Considering both functional positioning and codebase consistency, we will not be merging this PR. We appreciate your attention to security and the time you invested in the analysis. We will continue evaluating the direction you suggested and look for more suitable hardening approaches in the future. |
|
Separately, we do plan to address the openExternal handling in a future release by blocking non-HTTP/HTTPS protocols from being opened. |
hey @zerob13, Noted, In addition make sure to inform the user of requests being made to any hosts/domains, Cause if the response not in the chat include any Js code embedded with-in the response could help stealing local files and also clipboard content and other payload I used, It's monitoring and exfiltrating all the data/keylogging and so on. |
2 participants
This PR addresses two critical security vulnerabilities with a combined CVSS score of 9.8 CRITICAL.
Vulnerability 1: File System Access
Issue: `webSecurity: false` allowed arbitrary file reads via `fetch('file://...')`
Impact:
Fix:
Vulnerability 2: Unsafe URL Handler
Issue: Direct `shell.openExternal()` exposure without validation
Impact:
Fix:
PoC
Summary by CodeRabbit
Release Notes
Security