-
Notifications
You must be signed in to change notification settings - Fork 960
fix(desktop): allowlist URL schemes before shell.openExternal #3650
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||||||||||||||||||||||
| import { EventEmitter } from "node:events"; | ||||||||||||||||||||||||||
| import { clipboard, Menu, shell, webContents } from "electron"; | ||||||||||||||||||||||||||
| import { clipboard, Menu, webContents } from "electron"; | ||||||||||||||||||||||||||
| import { safeOpenExternal } from "main/lib/safe-url"; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| interface ConsoleEntry { | ||||||||||||||||||||||||||
| level: "log" | "warn" | "error" | "info" | "debug"; | ||||||||||||||||||||||||||
|
|
@@ -126,7 +127,9 @@ class BrowserManager extends EventEmitter { | |||||||||||||||||||||||||
| menuItems.push( | ||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| label: "Open Link in Default Browser", | ||||||||||||||||||||||||||
| click: () => shell.openExternal(linkURL), | ||||||||||||||||||||||||||
| click: () => { | ||||||||||||||||||||||||||
| void safeOpenExternal(linkURL); | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
|
Comment on lines
+130
to
133
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/main/lib/browser/browser-manager.ts
Line: 130-133
Comment:
**Unhandled promise rejection from `safeOpenExternal`**
`void safeOpenExternal(linkURL)` discards the returned promise. If `shell.openExternal` throws internally (e.g., OS error), the rejection is silently swallowed and becomes an unhandled promise rejection. A `.catch()` or `try/catch` inside the click handler would make failure more observable. The same applies to line 200 for `Open Page in Default Browser`.
```suggestion
click: () => {
safeOpenExternal(linkURL).catch((err) => {
console.warn("[BrowserManager] openExternal failed:", err);
});
},
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||
| label: "Open Link as New Split", | ||||||||||||||||||||||||||
|
|
@@ -194,7 +197,7 @@ class BrowserManager extends EventEmitter { | |||||||||||||||||||||||||
| label: "Open Page in Default Browser", | ||||||||||||||||||||||||||
| click: () => { | ||||||||||||||||||||||||||
| if (pageURL && pageURL !== "about:blank") { | ||||||||||||||||||||||||||
| shell.openExternal(pageURL); | ||||||||||||||||||||||||||
| void safeOpenExternal(pageURL); | ||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Same unhandled promise rejection issue: Prompt for AI agents
Suggested change
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
198
to
201
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Same pattern as the "Open Link" case above — errors from
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/main/lib/browser/browser-manager.ts
Line: 198-201
Comment:
**Same unhandled rejection applies to `Open Page in Default Browser`**
Same pattern as the "Open Link" case above — errors from `shell.openExternal` will become unhandled promise rejections.
```suggestion
click: () => {
if (pageURL && pageURL !== "about:blank") {
safeOpenExternal(pageURL).catch((err) => {
console.warn("[BrowserManager] openExternal failed:", err);
});
}
},
```
How can I resolve this? If you propose a fix, please make it concise. |
||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||
| enabled: !!pageURL && pageURL !== "about:blank", | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| export { | ||
| externalUrlLogLabel, | ||
| isSafeExternalUrl, | ||
| safeOpenExternal, | ||
| } from "./safe-url"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| import { describe, expect, it } from "bun:test"; | ||
| import { externalUrlLogLabel, isSafeExternalUrl } from "./safe-url"; | ||
|
|
||
| describe("isSafeExternalUrl", () => { | ||
| it("allows http, https, and mailto URLs", () => { | ||
| expect(isSafeExternalUrl("http://example.com")).toBe(true); | ||
| expect(isSafeExternalUrl("https://example.com/path?q=1")).toBe(true); | ||
| expect(isSafeExternalUrl("mailto:user@example.com")).toBe(true); | ||
| expect(isSafeExternalUrl("HTTPS://EXAMPLE.COM")).toBe(true); | ||
| }); | ||
|
|
||
| it("blocks file, javascript, data, and custom-scheme URLs", () => { | ||
| expect( | ||
| isSafeExternalUrl("file:///System/Applications/Calculator.app"), | ||
| ).toBe(false); | ||
| expect(isSafeExternalUrl("file:///etc/passwd")).toBe(false); | ||
| expect(isSafeExternalUrl("javascript:alert(1)")).toBe(false); | ||
| expect(isSafeExternalUrl("data:text/html,<script>alert(1)</script>")).toBe( | ||
| false, | ||
| ); | ||
| expect(isSafeExternalUrl("vscode://open?url=evil")).toBe(false); | ||
| expect(isSafeExternalUrl("ssh://user@host")).toBe(false); | ||
| expect(isSafeExternalUrl("ftp://example.com")).toBe(false); | ||
| }); | ||
|
|
||
| it("blocks malformed input", () => { | ||
| expect(isSafeExternalUrl("")).toBe(false); | ||
| expect(isSafeExternalUrl("not a url")).toBe(false); | ||
| expect(isSafeExternalUrl("/etc/passwd")).toBe(false); | ||
| }); | ||
| }); | ||
|
Comment on lines
+1
to
+31
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test suite only exercises
This gap means a future refactor that accidentally calls Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/desktop/src/main/lib/safe-url/safe-url.test.ts
Line: 1-31
Comment:
**`safeOpenExternal` is untested**
The test suite only exercises `isSafeExternalUrl`. `safeOpenExternal` (the public-facing wrapper used in `browser-manager.ts`) is not covered at all. While it is a thin wrapper, testing it with a mocked `shell` would verify that:
1. A safe URL results in `shell.openExternal` being called and `true` returned.
2. A blocked URL results in `shell.openExternal` NOT being called and `false` returned.
This gap means a future refactor that accidentally calls `openExternal` before the guard would not be caught by tests.
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| describe("externalUrlLogLabel", () => { | ||
| it("returns only the scheme, never the full URL", () => { | ||
| expect(externalUrlLogLabel("https://example.com/path?token=secret")).toBe( | ||
| "https:", | ||
| ); | ||
| expect(externalUrlLogLabel("file:///etc/passwd")).toBe("file:"); | ||
| expect(externalUrlLogLabel("mailto:user@example.com")).toBe("mailto:"); | ||
| }); | ||
|
|
||
| it("returns sentinels for empty and malformed input", () => { | ||
| expect(externalUrlLogLabel("")).toBe("empty"); | ||
| expect(externalUrlLogLabel("not a url")).toBe("malformed"); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| import { shell } from "electron"; | ||
|
|
||
| /** | ||
| * Schemes safe to hand to Electron's `shell.openExternal`. | ||
| * Anything else (file:, javascript:, custom handlers, etc.) can execute | ||
| * binaries or scripts via the OS URL handler registry. | ||
| */ | ||
| const ALLOWED_SCHEMES = new Set(["http:", "https:", "mailto:"]); | ||
|
|
||
| export function isSafeExternalUrl(url: string): boolean { | ||
| if (typeof url !== "string" || url.length === 0) return false; | ||
| try { | ||
| return ALLOWED_SCHEMES.has(new URL(url).protocol); | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| export function externalUrlLogLabel(url: string): string { | ||
| if (typeof url !== "string" || url.length === 0) return "empty"; | ||
| try { | ||
| return new URL(url).protocol || "unknown:"; | ||
| } catch { | ||
| return "malformed"; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Wraps `shell.openExternal` with a scheme allowlist. Returns false and | ||
| * refuses to dispatch when the URL is not http(s)/mailto. Catches | ||
| * `shell.openExternal` rejections so callers can fire-and-forget without | ||
| * risking an unhandled rejection in the Electron main process. | ||
| */ | ||
| export async function safeOpenExternal(url: string): Promise<boolean> { | ||
| if (!isSafeExternalUrl(url)) { | ||
| console.warn( | ||
| "[safeOpenExternal] blocked unsafe URL scheme:", | ||
| externalUrlLogLabel(url), | ||
| ); | ||
| return false; | ||
| } | ||
| try { | ||
| await shell.openExternal(url); | ||
| return true; | ||
| } catch (error) { | ||
| console.error( | ||
| "[safeOpenExternal] openExternal failed:", | ||
| externalUrlLogLabel(url), | ||
| error, | ||
| ); | ||
| return false; | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.
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.
P2: Unhandled promise rejection:
void safeOpenExternal(linkURL)discards the promise. Ifshell.openExternalthrows an OS error, the rejection is unhandled, which can crash the Electron main process. Add a.catch()handler to log the failure.Prompt for AI agents