From 84240baa4ff16c39dca2aecf09ea76828ff221f6 Mon Sep 17 00:00:00 2001 From: beyondkmp Date: Wed, 22 Apr 2026 14:46:55 +0800 Subject: [PATCH] fix(security): allow vscode/cursor/zed deep-links in openExternal (#14437) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### What this PR does fix #14433 Before this PR: - The external-app config in `packages/shared/externalApp/config/index.ts` exposes **Visual Studio Code / Cursor / Zed** as code-editor targets, and the renderer has "Open in " buttons on the Agent chat navbar (`OpenExternalAppButton.tsx`) and inline file paths in agent messages (`ClickableFilePath.tsx`) that call `window.open(buildEditorUrl(app, path))`. - These eventually hit `mainWindow.webContents.setWindowOpenHandler` in `src/main/services/WindowService.ts`, which checks the URL against `isSafeExternalUrl()` before calling `shell.openExternal`. - `isSafeExternalUrl()`'s allowlist only contained `http:`, `https:`, `mailto:`, `obsidian:`, so `vscode://…`, `cursor://…`, `zed://…` all fell through to the `else` branch and were silently denied with a `Blocked shell.openExternal for untrusted URL scheme: vscode://…` warning. Clicking "Open in VS Code" did nothing. After this PR: - `ALLOWED_EXTERNAL_PROTOCOLS` also includes `vscode:`, `vscode-insiders:`, `cursor:`, `zed:`, so the deep-links produced by `buildEditorUrl()` pass the safety check and actually open the file in the chosen editor. - `security.test.ts` gains an `allows code-editor deep-link protocols` case covering all four schemes (including the exact URL shape reported in the logs: `vscode://file/C%3A/…?windowId=_blank`). Fixes # ### Why we need it and why it was done in this way Users already see and click the editor buttons, so the current behavior (silent failure + warning log) is a functional regression, not a security-hardening feature. We just forgot to sync the main-process allowlist when those editor buttons were introduced. The following tradeoffs were made: - Kept the fix minimal — only the allowlist and its test are touched — per the `main`-branch code-freeze rule that hotfix PRs must not include refactoring. - Added `vscode-insiders:` proactively so VS Code Insiders users won't hit the same dead-button bug even though the current config doesn't list Insiders as a separate editor. The following alternatives were considered: - Deriving the allowlist dynamically from `EXTERNAL_APPS` in `packages/shared/externalApp/config` so new editors are auto-covered. Rejected for now because it's a refactor and `main` is frozen; suitable for the `v2` branch instead. Links to places where the discussion took place: N/A ### Breaking changes None. ### Special notes for your reviewer - Security-sensitive surface. The added schemes are all documented editor deep-links (`vscode://`, `vscode-insiders://`, `cursor://`, `zed://`) that the app already offers UI for. No `file:`, `javascript:`, `ms-msdt:`, etc. are permitted — the existing negative tests still pass. - The same `isSafeExternalUrl()` is also used by `will-navigate` and `WebviewService` / `ipc.ts` `openExternal`, so the newly-allowed schemes are consistent across entry points (and that's the behavior we want — opening a file in VS Code from a message link should work everywhere). ### Checklist - [x] PR: The PR description is expressive enough and will help future contributors - [x] Code: [Write code that humans can understand](https://en.wikiquote.org/wiki/Martin_Fowler#code-for-humans) and [Keep it simple](https://en.wikipedia.org/wiki/KISS_principle) - [x] Refactor: You have [left the code cleaner than you found it (Boy Scout Rule)](https://learning.oreilly.com/library/view/97-things-every/9780596809515/ch08.html) - [x] Upgrade: Impact of this change on upgrade flows was considered and addressed if required - [x] Documentation: A [user-guide update](https://docs.cherry-ai.com) was considered and is present (link) or not required. Check this only when the PR introduces or changes a user-facing feature or behavior. - [x] Self-review: I have reviewed my own code (e.g., via [`/gh-pr-review`](/.claude/skills/gh-pr-review/SKILL.md), `gh pr diff`, or GitHub UI) before requesting review from others ### Release note ```release-note Fix "Open in VS Code / Cursor / Zed" buttons on the Agent chat navbar and inline file-path menus being silently blocked by the main-process URL allowlist. Clicking an editor button now actually opens the file in the selected editor. ``` --------- Signed-off-by: beyondkmp Co-authored-by: Claude Opus 4.7 (1M context) --- src/main/services/__tests__/security.test.ts | 34 +++++++++++ src/main/services/security.ts | 61 ++++++++++++++++++-- 2 files changed, 90 insertions(+), 5 deletions(-) diff --git a/src/main/services/__tests__/security.test.ts b/src/main/services/__tests__/security.test.ts index 76c09982cf..e49480dd1a 100644 --- a/src/main/services/__tests__/security.test.ts +++ b/src/main/services/__tests__/security.test.ts @@ -21,6 +21,40 @@ describe('isSafeExternalUrl', () => { expect(isSafeExternalUrl('obsidian://new?file=test&vault=myvault&clipboard')).toBe(true) }) + it('allows code-editor file-open deep-links on Unix paths', () => { + expect(isSafeExternalUrl('vscode://file/C%3A/Users/foo/bar.ts?windowId=_blank')).toBe(true) + expect(isSafeExternalUrl('vscode-insiders://file/C%3A/Users/foo/bar.ts')).toBe(true) + expect(isSafeExternalUrl('cursor://file/C%3A/Users/foo/bar.ts?windowId=_blank')).toBe(true) + expect(isSafeExternalUrl('zed://file/Users/foo/bar.ts')).toBe(true) + }) + + it('allows Zed file-open deep-links for Windows absolute paths', () => { + // buildEditorUrl() for Zed emits `zed://file` without a slash, so a + // Windows path like C:\Users\foo\bar.ts produces zed://fileC%3A/... + expect(isSafeExternalUrl('zed://fileC%3A/Users/foo/bar.ts')).toBe(true) + expect(isSafeExternalUrl('zed://filed%3a/data/foo.ts')).toBe(true) + }) + + it('rejects editor deep-links with non-file authorities', () => { + // command authority runs registered VS Code commands + expect(isSafeExternalUrl('vscode://command/workbench.action.terminal.sendSequence?text=rm')).toBe(false) + // extension URL handlers + expect(isSafeExternalUrl('vscode://ms-python.python/do-something')).toBe(false) + expect(isSafeExternalUrl('cursor://settings')).toBe(false) + expect(isSafeExternalUrl('zed://extension/evil')).toBe(false) + // missing authority entirely + expect(isSafeExternalUrl('vscode:command/foo')).toBe(false) + }) + + it('rejects Zed deep-links that do not match the file-open shape', () => { + // host starts with "file" but is not file or file + expect(isSafeExternalUrl('zed://filename/path')).toBe(false) + expect(isSafeExternalUrl('zed://files.evil.com/cmd')).toBe(false) + // userinfo smuggling: "file" in userinfo, real host is attacker-controlled + expect(isSafeExternalUrl('zed://file@evil.com/path')).toBe(false) + expect(isSafeExternalUrl('vscode://file:pw@evil.com/path')).toBe(false) + }) + it('rejects file:// protocol', () => { expect(isSafeExternalUrl('file:///etc/passwd')).toBe(false) expect(isSafeExternalUrl('file://localhost/tmp')).toBe(false) diff --git a/src/main/services/security.ts b/src/main/services/security.ts index ea59f289e1..21639d4309 100644 --- a/src/main/services/security.ts +++ b/src/main/services/security.ts @@ -2,22 +2,73 @@ * Security utility functions for the main process. */ -const ALLOWED_EXTERNAL_PROTOCOLS = new Set(['http:', 'https:', 'mailto:', 'obsidian:']) +const ALLOWED_EXTERNAL_PROTOCOLS = new Set([ + 'http:', + 'https:', + 'mailto:', + 'obsidian:', + 'vscode:', + 'vscode-insiders:', + 'cursor:', + 'zed:' +]) + +/** + * Editor deep-link schemes. For these we only accept the "open a file" shape + * produced by `buildEditorUrl()`, so that attacker-supplied links cannot + * reach other authorities such as `vscode://command/...` (runs registered + * commands) or `vscode://./...` (invokes extension URL + * handlers). + */ +const EDITOR_DEEP_LINK_PROTOCOLS = new Set(['vscode:', 'vscode-insiders:', 'cursor:', 'zed:']) + +/** + * Zed's deep-link format is `zed://file` (no slash separator before + * the path — Zed strips the `zed://file` prefix and treats the rest as a + * filesystem path). That means on Unix the URL is `zed://file/abs/path` + * (host parses as `file`), but on Windows it is `zed://fileC%3A/abs/path` + * (host parses as `fileC%3A`), so a plain `host === 'file'` check is + * insufficient. Match the two exact shapes buildEditorUrl() can emit: a + * slash, or a single-letter encoded drive followed by a slash. + */ +const ZED_FILE_URL_RE = /^zed:\/\/file(\/|[A-Za-z]%3[Aa]\/)/i /** * Check whether a URL is safe to open via shell.openExternal(). * - * Only http(s) and mailto links are allowed. This prevents attackers from - * abusing custom protocol handlers (e.g. file://, ms-msdt:, calculator:) - * to execute local files or launch arbitrary applications. + * Only an explicit allowlist of schemes is permitted (web links, mail, and + * known code-editor deep-links used by the app). Editor schemes are further + * restricted to the "open a file" URL shape emitted by `buildEditorUrl()` so + * that attackers cannot smuggle in `vscode://command/...` command URIs, + * extension URL handlers, or userinfo tricks like `zed://file@evil/...`. * * @see https://benjamin-altpeter.de/shell-openexternal-dangers/ */ export function isSafeExternalUrl(url: string): boolean { try { const parsed = new URL(url) - return ALLOWED_EXTERNAL_PROTOCOLS.has(parsed.protocol) + if (!ALLOWED_EXTERNAL_PROTOCOLS.has(parsed.protocol)) { + return false + } + if (EDITOR_DEEP_LINK_PROTOCOLS.has(parsed.protocol)) { + return isFileOpenEditorUrl(parsed, url) + } + return true } catch { return false } } + +function isFileOpenEditorUrl(parsed: URL, rawUrl: string): boolean { + // Reject userinfo in any form to foil `zed://file@evil/path`-style tricks + // where "file" ends up as the username and the real host is attacker-chosen. + if (parsed.username !== '' || parsed.password !== '') { + return false + } + if (parsed.protocol === 'zed:') { + return ZED_FILE_URL_RE.test(rawUrl) + } + // vscode / vscode-insiders / cursor all produce ://file/, + // where the URL authority is exactly "file". + return parsed.host === 'file' +}