fix: open links in external browser instead of in-app#254
Merged
Conversation
- Use parsed.href instead of raw URL string in openIfAllowed to eliminate parser-differential risk - Handle rejected shell.openExternal promise instead of voiding it - Update stale preload JSDoc to reflect expanded protocol allowlist - Add afterEach cleanup for desktopBridge mock in tests - Add test case for data: URI scheme blocking
📝 WalkthroughWalkthroughAdds URL allowlist validation and centralized external-open handling; prevents renderer-initiated new windows and blocks cross-origin navigations in the desktop main process. Web markdown links now route through the desktop bridge when available, falling back to window.open. Changes
Sequence DiagramssequenceDiagram
participant User
participant Renderer as Renderer<br/>(web content)
participant Main as Desktop Main<br/>Process
participant Security as openIfAllowed
participant Shell as System Browser
User->>Renderer: Click link or window.open()
Renderer->>Main: (window.open blocked) send IPC or let handler run
Main->>Security: Parse & validate URL (https/http/mailto)
alt Allowed
Security->>Shell: shell.openExternal(url)
else Disallowed
Security-->>Main: ignore / block
end
sequenceDiagram
participant User
participant WebApp as Web App<br/>(MarkdownContent)
participant ClickH as Click Handler
participant Bridge as Desktop Bridge
participant Browser as System Browser
User->>WebApp: Click markdown link
WebApp->>ClickH: onClick (preventDefault, get safeHref)
alt Bridge available
ClickH->>Bridge: openExternalUrl(safeHref)
Bridge->>Browser: openExternal(safeHref)
else Bridge absent
ClickH->>Browser: window.open(safeHref, "_blank", "noopener,noreferrer")
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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/main.ts`:
- Around line 200-206: The openIfAllowed function currently swallows rejections
from shell.openExternal; update openIfAllowed(url: string) so that when
parsed.protocol is allowed and shell.openExternal(parsed.href) rejects it logs
the error (use processLogger or console.error) with context including the URL
and protocol; ensure callers (setWindowOpenHandler, the 'will-navigate' event
handler, and the IPC bridge handler) rely on this function unchanged but benefit
from the surfaced errors for observability.
In `@apps/web/src/__tests__/MarkdownContent.test.tsx`:
- Around line 20-22: The test suite currently deletes window.desktopBridge in
afterEach but leaves global stubs like the one created by vi.stubGlobal('open',
...) inside a test; move the global cleanup call vi.unstubAllGlobals() into the
afterEach block so that afterEach both removes (window as unknown as
Record<string, unknown>).desktopBridge and calls vi.unstubAllGlobals(), ensuring
any vi.stubGlobal('open', ...) created in tests (or other global stubs) are
always unstubbed regardless of test outcome.
🪄 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: 4b7d4fab-6fbc-4eac-a0f8-1fdb53433f19
📒 Files selected for processing (4)
apps/desktop/src/main/main.tsapps/desktop/src/main/preload.tsapps/web/src/__tests__/MarkdownContent.test.tsxapps/web/src/components/chat/MarkdownContent.tsx
- Replace swallowed .catch(() => {}) with console.error logging in
openIfAllowed so failures (e.g. no default browser) are observable
- Move vi.unstubAllGlobals() into afterEach so global stubs are always
cleaned up regardless of test outcome
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/main.ts`:
- Around line 205-206: The catch handler on shell.openExternal is currently
logging the full user-controlled URL (parsed.href); change the error log to
avoid leaking the full target by logging parsed.protocol and a redacted target
instead (e.g., use parsed.hostname or a fixed "<redacted>" placeholder or a
masked version that strips query and fragment) in the catch for
shell.openExternal inside openIfAllowed, ensuring the console.error no longer
includes parsed.href but only protocol + redactedTarget along with the error
object.
🪄 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: c9cdd93b-029c-474f-9993-96180f1f3a27
📒 Files selected for processing (2)
apps/desktop/src/main/main.tsapps/web/src/__tests__/MarkdownContent.test.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/web/src/tests/MarkdownContent.test.tsx
Comment on lines
+205
to
+206
| shell.openExternal(parsed.href).catch((err: unknown) => { | ||
| console.error(`[openIfAllowed] Failed to open ${parsed.protocol} URL: ${parsed.href}`, err); |
There was a problem hiding this comment.
Redact the URL before logging external-open failures.
Line 206 logs the full parsed.href. These values come from user-controlled links and can include signed query params, mailto: recipients, or message bodies, so a failed open currently leaks that data into app logs. Log only the protocol plus a redacted target.
🛠️ Minimal fix
if (EXTERNAL_PROTOCOLS.has(parsed.protocol)) {
shell.openExternal(parsed.href).catch((err: unknown) => {
- console.error(`[openIfAllowed] Failed to open ${parsed.protocol} URL: ${parsed.href}`, err);
+ const target =
+ parsed.protocol === "mailto:"
+ ? "mailto:[redacted]"
+ : `${parsed.protocol}//${parsed.host}`;
+ console.error("[openIfAllowed] Failed to open external URL", {
+ protocol: parsed.protocol,
+ target,
+ err,
+ });
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/main.ts` around lines 205 - 206, The catch handler on
shell.openExternal is currently logging the full user-controlled URL
(parsed.href); change the error log to avoid leaking the full target by logging
parsed.protocol and a redacted target instead (e.g., use parsed.hostname or a
fixed "<redacted>" placeholder or a masked version that strips query and
fragment) in the catch for shell.openExternal inside openIfAllowed, ensuring the
console.error no longer includes parsed.href but only protocol + redactedTarget
along with the error object.
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
1 participant
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.
What
External links clicked inside the Electron app (markdown chat messages,
target="_blank"anchors) now open in the system default browser instead of navigating within the app window.Why
Electron's default behavior for
<a target="_blank">is to open a new BrowserWindow, causing links to render inside the app rather than the user's browser. This was confusing and broke the expected UX.Key Changes
onClickhandler to markdown<a>elements that routes throughdesktopBridge.openExternalUrl()with awindow.open()fallback for web modesetWindowOpenHandler(denies all new windows, opens URL externally) andwill-navigatelistener (blocks cross-origin navigation) as defense-in-depth safety netsopen-external-urlprotocol allowlist fromhttps:only tohttps:,http:, andmailto:openIfAllowed()to deduplicate protocol validation across all three call sites; uses canonicalizedparsed.hrefto avoid parser-differential risk🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
Tests