fix(desktop): allowlist URL schemes before shell.openExternal#3650
Merged
saddlepaddle merged 2 commits intoApr 22, 2026
Merged
Conversation
The browser-pane context menu and `external.openUrl` tRPC procedure
forwarded attacker-controlled URLs directly to `shell.openExternal`,
letting a `file://` href right-clicked inside a pane launch arbitrary
applications via OS URL handlers.
Adds `main/lib/safe-url` with an `{http, https, mailto}` allowlist and
routes both sinks through it. Rejects disallowed schemes on `openUrl`
with BAD_REQUEST.
Contributor
📝 WalkthroughWalkthroughAdds an allowlist-based URL safety module and integrates it into the TRPC external URL handler and BrowserManager context-menu handlers; unsafe URLs are rejected (logged via a label) and opening external links goes through a guarded Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer (TRPC client)
participant TRPC as TRPC Router
participant Main as Main Process (BrowserManager / safe-url)
participant Shell as Electron Shell
Renderer->>TRPC: request openUrl(input)
TRPC->>Main: validate via isSafeExternalUrl(input)
alt invalid
Main-->>TRPC: externalUrlLogLabel(input)
TRPC-->>Renderer: TRPCError BAD_REQUEST ("URL scheme not allowed")
else valid
Main->>Shell: safeOpenExternal(input) -> shell.openExternal(input)
alt shell succeeds
Shell-->>Main: success
Main-->>TRPC: success
TRPC-->>Renderer: success
else shell throws
Shell-->>Main: error
Main-->>TRPC: logs label and returns failure (TRPC INTERNAL error)
TRPC-->>Renderer: TRPCError INTERNAL_SERVER_ERROR
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR hardens the Electron desktop app against URL-scheme injection attacks by introducing a Key changes:
Confidence Score: 5/5Safe to merge — the security fix is correct and well-scoped; remaining comments are non-blocking P2 style suggestions The core allowlist logic using No files require special attention;
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/lib/safe-url/safe-url.ts | New module implementing the URL scheme allowlist (http:, https:, mailto:); logic is correct — uses new URL().protocol (WHATWG-compliant, handles case-normalization and malformed input) and an explicit Set allowlist |
| apps/desktop/src/main/lib/safe-url/safe-url.test.ts | Good coverage for isSafeExternalUrl (allow/block/malformed cases), but safeOpenExternal itself has no test coverage |
| apps/desktop/src/main/lib/browser/browser-manager.ts | Correctly replaces shell.openExternal with safeOpenExternal in both context-menu paths; void usage means errors from the underlying shell.openExternal call become unhandled promise rejections |
| apps/desktop/src/lib/trpc/routers/external/index.ts | Adds isSafeExternalUrl guard at the tRPC boundary and returns a BAD_REQUEST TRPCError on blocked URLs — correct placement and error code |
| apps/desktop/src/main/lib/safe-url/index.ts | Trivial barrel re-export; correct |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Untrusted browser-pane content"] -->|"right-click link/page"| B["BrowserManager context menu"]
A -->|"tRPC call"| C["external.openUrl mutation"]
B --> D["safeOpenExternal(url)"]
C --> E{"isSafeExternalUrl(input)?"}
E -->|"false"| F["throw TRPCError BAD_REQUEST\n+ console.error"]
E -->|"true"| G["shell.openExternal(input)"]
D --> H{"isSafeExternalUrl(url)?"}
H -->|"false"| I["console.warn\nreturn false"]
H -->|"true"| J["shell.openExternal(url)\nreturn true"]
subgraph isSafeExternalUrl
K["new URL(url).protocol"] --> L{"in ALLOWED_SCHEMES?\nhttp: / https: / mailto:"}
L -->|"yes"| M["return true"]
L -->|"no / throws"| N["return false"]
end
H --> K
E --> K
Prompt To Fix All With AI
This 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.
---
This 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.
---
This 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.Reviews (1): Last reviewed commit: "fix(desktop): allowlist URL schemes befo..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+130
to
133
| click: () => { | ||
| void safeOpenExternal(linkURL); | ||
| }, | ||
| }, |
There was a problem hiding this comment.
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.
Suggested change
| click: () => { | |
| void safeOpenExternal(linkURL); | |
| }, | |
| }, | |
| click: () => { | |
| safeOpenExternal(linkURL).catch((err) => { | |
| console.warn("[BrowserManager] openExternal failed:", err); | |
| }); | |
| }, |
Prompt To Fix With AI
This 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.
Comment on lines
198
to
201
| click: () => { | ||
| if (pageURL && pageURL !== "about:blank") { | ||
| shell.openExternal(pageURL); | ||
| void safeOpenExternal(pageURL); | ||
| } |
There was a problem hiding this comment.
Open Page in Default Browser
Same pattern as the "Open Link" case above — errors from shell.openExternal will become unhandled promise rejections.
Suggested change
| click: () => { | |
| if (pageURL && pageURL !== "about:blank") { | |
| shell.openExternal(pageURL); | |
| void safeOpenExternal(pageURL); | |
| } | |
| click: () => { | |
| if (pageURL && pageURL !== "about:blank") { | |
| safeOpenExternal(pageURL).catch((err) => { | |
| console.warn("[BrowserManager] openExternal failed:", err); | |
| }); | |
| } | |
| }, |
Prompt To Fix With AI
This 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.
Comment on lines
+1
to
+31
| import { describe, expect, it } from "bun:test"; | ||
| import { 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); | ||
| }); | ||
| }); |
There was a problem hiding this 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:
- A safe URL results in
shell.openExternalbeing called andtruereturned. - A blocked URL results in
shell.openExternalNOT being called andfalsereturned.
This gap means a future refactor that accidentally calls openExternal before the guard would not be caught by tests.
Prompt To Fix With AI
This 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.![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues found across 5 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/browser/browser-manager.ts">
<violation number="1" location="apps/desktop/src/main/lib/browser/browser-manager.ts:131">
P2: Unhandled promise rejection: `void safeOpenExternal(linkURL)` discards the promise. If `shell.openExternal` throws an OS error, the rejection is unhandled, which can crash the Electron main process. Add a `.catch()` handler to log the failure.</violation>
<violation number="2" location="apps/desktop/src/main/lib/browser/browser-manager.ts:200">
P2: Same unhandled promise rejection issue: `void safeOpenExternal(pageURL)` discards the promise. Add a `.catch()` to prevent a potential Electron main-process crash on OS errors.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| click: () => { | ||
| if (pageURL && pageURL !== "about:blank") { | ||
| shell.openExternal(pageURL); | ||
| void safeOpenExternal(pageURL); |
Contributor
There was a problem hiding this comment.
P2: Same unhandled promise rejection issue: void safeOpenExternal(pageURL) discards the promise. Add a .catch() to prevent a potential Electron main-process crash on OS errors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/browser/browser-manager.ts, line 200:
<comment>Same unhandled promise rejection issue: `void safeOpenExternal(pageURL)` discards the promise. Add a `.catch()` to prevent a potential Electron main-process crash on OS errors.</comment>
<file context>
@@ -194,7 +197,7 @@ class BrowserManager extends EventEmitter {
click: () => {
if (pageURL && pageURL !== "about:blank") {
- shell.openExternal(pageURL);
+ void safeOpenExternal(pageURL);
}
},
</file context>
Suggested change
| void safeOpenExternal(pageURL); | |
| safeOpenExternal(pageURL).catch((err) => { | |
| console.warn("[BrowserManager] openExternal failed:", err); | |
| }); |
| label: "Open Link in Default Browser", | ||
| click: () => shell.openExternal(linkURL), | ||
| click: () => { | ||
| void safeOpenExternal(linkURL); |
Contributor
There was a problem hiding this comment.
P2: Unhandled promise rejection: void safeOpenExternal(linkURL) discards the promise. If shell.openExternal throws 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
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/browser/browser-manager.ts, line 131:
<comment>Unhandled promise rejection: `void safeOpenExternal(linkURL)` discards the promise. If `shell.openExternal` throws an OS error, the rejection is unhandled, which can crash the Electron main process. Add a `.catch()` handler to log the failure.</comment>
<file context>
@@ -126,7 +127,9 @@ class BrowserManager extends EventEmitter {
label: "Open Link in Default Browser",
- click: () => shell.openExternal(linkURL),
+ click: () => {
+ void safeOpenExternal(linkURL);
+ },
},
</file context>
Suggested change
| void safeOpenExternal(linkURL); | |
| safeOpenExternal(linkURL).catch((err) => { | |
| console.warn("[BrowserManager] openExternal failed:", err); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
97-109:⚠️ Potential issue | 🟠 MajorRedact untrusted URLs before logging.
Both the blocked and failure paths log
inputverbatim. For rejectedfile:,data:,mailto:, or tokenizedhttps:URLs, this can leak local paths, email addresses, or secrets into main-process logs. Log only a sanitized label such as the parsed protocol or"malformed".🛡️ Proposed redaction pattern
+function externalUrlLogLabel(url: string): string { + try { + return new URL(url).protocol || "unknown:"; + } catch { + return "malformed"; + } +} + export const createExternalRouter = () => { return router({ openUrl: publicProcedure.input(z.string()).mutation(async ({ input }) => { if (!isSafeExternalUrl(input)) { - console.error("[external/openUrl] Blocked unsafe URL scheme:", input); + console.warn( + "[external/openUrl] Blocked unsafe URL scheme:", + externalUrlLogLabel(input), + ); throw new TRPCError({ code: "BAD_REQUEST", message: "URL scheme not allowed", }); } try { await shell.openExternal(input); } catch (error) { const errorMessage = error instanceof Error ? error.message : "Unknown error"; - console.error("[external/openUrl] Failed to open URL:", input, error); + console.error( + "[external/openUrl] Failed to open URL:", + externalUrlLogLabel(input), + error, + ); throw new TRPCError({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 97 - 109, The logs in the external/openUrl handler leak the raw input; update the console.error calls (the branch that checks isSafeExternalUrl and the catch block around shell.openExternal) to never log the full input value — instead parse the input with the URL constructor (or fallback to "malformed" if parsing fails) and log only the URL.protocol (or "malformed") and any errorMessage; adjust the messages around isSafeExternalUrl, TRPCError, and shell.openExternal to use the sanitized label so paths, emails, or tokenized data are not emitted to main-process logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/lib/safe-url/safe-url.ts`:
- Around line 23-29: safeOpenExternal currently calls shell.openExternal without
catching rejections and logs the full untrusted URL; wrap the call in a
try/catch around shell.openExternal(url) inside safeOpenExternal (use
isSafeExternalUrl to gate first), return false on any exception, and log only a
sanitized representation (e.g., origin/host or a redacted string) rather than
the raw URL to avoid leaking paths/secrets; ensure the function still returns
true on success and false on validation failure or caught errors.
---
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 97-109: The logs in the external/openUrl handler leak the raw
input; update the console.error calls (the branch that checks isSafeExternalUrl
and the catch block around shell.openExternal) to never log the full input value
— instead parse the input with the URL constructor (or fallback to "malformed"
if parsing fails) and log only the URL.protocol (or "malformed") and any
errorMessage; adjust the messages around isSafeExternalUrl, TRPCError, and
shell.openExternal to use the sanitized label so paths, emails, or tokenized
data are not emitted to main-process logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 675b58e3-d361-4aff-9d80-d7fe2ebbfb25
📒 Files selected for processing (5)
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/main/lib/browser/browser-manager.tsapps/desktop/src/main/lib/safe-url/index.tsapps/desktop/src/main/lib/safe-url/safe-url.test.tsapps/desktop/src/main/lib/safe-url/safe-url.ts
Addresses two defense-in-depth findings from PR superset-sh#3650 review: - safeOpenExternal now try/catches shell.openExternal so fire-and-forget callers (browser-manager context menu) can't leak an unhandled rejection into the main process. - Log only the parsed URL scheme (via externalUrlLogLabel) instead of the raw untrusted URL in both sinks, avoiding path/email/token spill into electron-log files.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/lib/trpc/routers/external/index.ts (1)
109-120:⚠️ Potential issue | 🟡 MinorRaw
errorobject anderror.messagecan still leak the URL.You've redacted
inputin the log viaexternalUrlLogLabel, but on line 115 the fullerroris logged and on line 119error.messageis propagated back through theTRPCError. Electron/Chromium error messages fromshell.openExternalcommonly embed the offending URL (e.g.,"Failed to open URL: <full-url>"), which defeats the redaction goal described in the PR for paths, emails, and tokens.Consider scrubbing or dropping the error details, e.g.:
🛡️ Proposed redaction of error payload
- } catch (error) { - const errorMessage = - error instanceof Error ? error.message : "Unknown error"; - console.error( - "[external/openUrl] Failed to open URL:", - externalUrlLogLabel(input), - error, - ); - throw new TRPCError({ - code: "INTERNAL_SERVER_ERROR", - message: errorMessage, - }); - } + } catch (error) { + console.error( + "[external/openUrl] Failed to open URL:", + externalUrlLogLabel(input), + error instanceof Error ? error.name : "UnknownError", + ); + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to open external URL", + }); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/external/index.ts` around lines 109 - 120, The catch block in the external/openUrl handler currently logs the raw error and returns error.message in the TRPCError, which can leak the full URL; update the handler around the catch (the code using externalUrlLogLabel and throwing new TRPCError) to stop logging the raw error object and to never propagate error.message to clients: log only a redacted/sanitized message (e.g., use externalUrlLogLabel(input) plus a fixed text) and throw a TRPCError with a generic message like "Failed to open external URL" (no error details); ensure references to external/openUrl, externalUrlLogLabel, and the TRPCError creation are updated accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/lib/trpc/routers/external/index.ts`:
- Around line 109-120: The catch block in the external/openUrl handler currently
logs the raw error and returns error.message in the TRPCError, which can leak
the full URL; update the handler around the catch (the code using
externalUrlLogLabel and throwing new TRPCError) to stop logging the raw error
object and to never propagate error.message to clients: log only a
redacted/sanitized message (e.g., use externalUrlLogLabel(input) plus a fixed
text) and throw a TRPCError with a generic message like "Failed to open external
URL" (no error details); ensure references to external/openUrl,
externalUrlLogLabel, and the TRPCError creation are updated accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c543e353-a503-4b68-9a39-596624ad4fce
📒 Files selected for processing (4)
apps/desktop/src/lib/trpc/routers/external/index.tsapps/desktop/src/main/lib/safe-url/index.tsapps/desktop/src/main/lib/safe-url/safe-url.test.tsapps/desktop/src/main/lib/safe-url/safe-url.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/lib/safe-url/index.ts
- apps/desktop/src/main/lib/safe-url/safe-url.test.ts
- apps/desktop/src/main/lib/safe-url/safe-url.ts
4 tasks
13 tasks
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shell.openExternalcall reachable from untrusted content behind an{http, https, mailto}scheme allowlistOpen Link / Page in Default Browser) so afile://href on a page loaded in the pane can no longer launch arbitrary appsexternal.openUrltRPC boundary withBAD_REQUESTTest plan
bun test src/main/lib/safe-url/— 3 passed, covers http/https/mailto allow + file/javascript/data/custom-scheme/malformed blockbun run typecheckinapps/desktop— cleanbun run lint:fix— cleanexploit_0.html(anchor href =file:///System/Applications/Calculator.app) in a browser pane, right-click → "Open Link in Default Browser", confirm Calculator does not launch and a warning is loggedSummary by cubic
Allowlisted
http,https, andmailtobeforeshell.openExternalto block unsafe URL handlers. Preventsfile://links from launching apps, rejects disallowed schemes inexternal.openUrlwithBAD_REQUEST, and logs only the URL scheme.main/lib/safe-urlwithisSafeExternalUrl,safeOpenExternal, andexternalUrlLogLabel; unit tests cover allowlist, blocked schemes, and redacted logging.safeOpenExternalto avoid unhandled rejections.external.openUrl; disallowed schemes returnBAD_REQUEST.Written for commit bc5ce11. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests