fix(security): restrict renderer top-level navigation to trusted origins - #368
khawarahemad wants to merge 1 commit into
Conversation
|
I'll review this PR later, it's already quite late over here. |
aiko-chan-ai
added
the
AI
There was a problem hiding this comment.
🟡 Changes recommended
The navigation hardening is incomplete for redirects (needs will-redirect handling) and should avoid delegating non-http(s) schemes via shell.openExternal().
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aims to harden the Electron renderer navigation surface by preventing top-level in-frame navigations from escaping the app’s trusted origins, mitigating exposure when webSecurity: false and privileged preload APIs are present.
Changes:
- Add a
webContentsnavigation guard intended to block top-level navigation to untrusted origins and delegate external URLs to the OS browser. - Make Vencord extension loading optional by checking for existence and logging failures/missing path instead of always attempting to load.
- Adjust Express
Requesttype augmentation to use globalnamespace Expressmerging.
File summaries
| File | Description |
|---|---|
| src/AppCore/index.ts | Adds will-navigate handling for navigation restriction; also changes Vencord extension loading behavior and logging. |
| src/overrides.d.ts | Updates Express Request typing augmentation approach (and removes the file header). |
Review details
- Files reviewed: 1/2 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // WebContents Event | ||
| this.win.webContents | ||
| .on("will-navigate", (event, navigationUrl) => { | ||
| try { | ||
| const parsed = new URL(navigationUrl); | ||
| const isAllowedHost = | ||
| parsed.hostname === Constants.CustomDiscordDomain || | ||
| parsed.hostname === "localhost" || | ||
| parsed.hostname === "127.0.0.1"; | ||
| const isAllowedProtocol = parsed.protocol === "https:" || (parsed.protocol === "http:" && parsed.hostname !== Constants.CustomDiscordDomain); | ||
| if (!isAllowedHost || !isAllowedProtocol) { | ||
| event.preventDefault(); | ||
| shell.openExternal(navigationUrl); | ||
| } | ||
| } catch { | ||
| event.preventDefault(); | ||
| } | ||
| }) |
| const isAllowedProtocol = parsed.protocol === "https:" || (parsed.protocol === "http:" && parsed.hostname !== Constants.CustomDiscordDomain); | ||
| if (!isAllowedHost || !isAllowedProtocol) { | ||
| event.preventDefault(); | ||
| shell.openExternal(navigationUrl); | ||
| } |
| // Load Vencord-Web Extension | ||
| const extension = await this.session.extensions.loadExtension(Constants.VencordExtensionPath); | ||
| this.logger.info(`Loaded Vencord Extension v${extension.version} from ${Constants.VencordExtensionPath}`); | ||
| if (fs.existsSync(Constants.VencordExtensionPath)) { | ||
| try { | ||
| const extension = await this.session.extensions.loadExtension(Constants.VencordExtensionPath); | ||
| this.logger.info(`Loaded Vencord Extension v${extension.version} from ${Constants.VencordExtensionPath}`); | ||
| } catch (err) { | ||
| this.logger.error("Failed to load Vencord extension:", err); | ||
| } | ||
| } else { | ||
| this.logger.warn(`Vencord extension not found at ${Constants.VencordExtensionPath}, running without extension.`); | ||
| } |
aiko-chan-ai
left a comment
aiko-chan-ai
left a comment
There was a problem hiding this comment.
Isn't this a bit overly defensive? Running the Discord client inside this app mostly opens popups to handle external URLs anyway, and in-app redirects are only reachable if DevTools is toggled on (location.href = "<url>")
I doubt the users of this app don't know that; they're all Discord bot devs after all, right?
| this.logger.error("Failed to load Vencord extension:", err); | ||
| } | ||
| } else { | ||
| this.logger.warn(`Vencord extension not found at ${Constants.VencordExtensionPath}, running without extension.`); |
There was a problem hiding this comment.
Technically, this app is entirely unusable without Vencord.
Summary
This PR adds a
will-navigateevent listener tothis.win.webContentsto restrict in-frame top-level navigation to trusted application origins.Impact
While popup windows (
setWindowOpenHandler) were handled, in-frame navigations (location.href,target="_self", or redirects) to external domains were unconstrained. Navigating the top-level frame to an external URL allowed untrusted web content to execute inside aBrowserWindowwithwebSecurity: falseand access privilegedwindow.BotClientNativeandwindow.protoAPIpreload bindings.Root Cause
this.win.webContentslacked awill-navigatehandler to intercept and cancel top-level navigation events targeting external domains.Fix
Added a
will-navigatehandler that verifies the navigation target's hostname (Constants.CustomDiscordDomain,localhost, or127.0.0.1) and protocol (https:, orhttp:for local hosts). External URLs are prevented from navigating in-frame and are delegated to the OS default browser viashell.openExternal().Validation
npm run test:typescriptandnpm run build:ts.window.location.href = "https://google.com"in the renderer is blocked in-frame and opened safely in the external browser.