diff --git a/electron/ipc-handlers/system.ts b/electron/ipc-handlers/system.ts index 6733f075bc..866d52d879 100644 --- a/electron/ipc-handlers/system.ts +++ b/electron/ipc-handlers/system.ts @@ -1,30 +1,16 @@ import { app, dialog, ipcMain, shell } from 'electron'; import { IPC } from '../shared-with-frontend/ipc-events.const'; -import { - hasExecutableFileExtension, - isExternalUrlSchemeAllowed, - isPathSafeToOpen, -} from '../shared-with-frontend/is-external-url-allowed'; +import { isExternalUrlSchemeAllowed } from '../shared-with-frontend/is-external-url-allowed'; +import { isLocalFileUrl, openLocalPath } from '../open-url'; import { getWin } from '../main-window'; export const initSystemIpc = (): void => { ipcMain.on(IPC.OPEN_PATH, (ev, path: string) => { - // Block UNC / network paths and remote file:// URLs: shell.openPath would - // resolve \\host\share (and file://host/share) to an SMB connection and leak - // the user's NTLM hash. FILE-type task-attachment paths are synced and thus - // attacker-controllable. See GHSA-hr87-735w-hfq3. - if (!isPathSafeToOpen(path)) { - return; - } - // Never launch an executable/script. shell.openPath runs .bat/.cmd/.vbs/... - // via the OS handler (ShellExecute on Windows needs no exec bit), so a - // renderer that drops a file into a writable dir + calls openPath — or a - // malicious synced FILE attachment clicked by the user — would get native - // code execution that bypasses the nodeExecution consent gate. - if (hasExecutableFileExtension(path)) { - return; - } - shell.openPath(path); + // openLocalPath enforces the guards this sink needs: reject UNC / remote + // file:// paths (NTLM-hash leak) and never launch an executable/script. + // FILE-type task-attachment paths are synced and thus attacker-controllable. + // See GHSA-hr87-735w-hfq3. + openLocalPath(path); }); ipcMain.on(IPC.OPEN_EXTERNAL, (ev, url: string) => { // Defense in depth: never hand an unsafe scheme to the OS handler, even if @@ -32,6 +18,14 @@ export const initSystemIpc = (): void => { if (!isExternalUrlSchemeAllowed(url)) { return; } + // A local file: URL opens reliably via openPath, not openExternal: + // openExternal percent-encodes the path on Windows and then can't resolve + // non-ASCII names or spaces. openLocalPath decodes it and re-applies the + // openPath guards. See issue #8695. + if (isLocalFileUrl(url)) { + openLocalPath(url); + return; + } shell.openExternal(url); }); diff --git a/electron/main-window.ts b/electron/main-window.ts index c32ec6f057..8dd7e5f106 100644 --- a/electron/main-window.ts +++ b/electron/main-window.ts @@ -14,6 +14,7 @@ import * as path from 'path'; import { join, normalize } from 'path'; import { IPC } from './shared-with-frontend/ipc-events.const'; import { isExternalUrlSchemeAllowed } from './shared-with-frontend/is-external-url-allowed'; +import { isLocalFileUrl, openLocalPath } from './open-url'; import { readFileSync, stat, writeFileSync } from 'fs'; import { error, log } from 'electron-log/main'; import { IS_MAC, IS_GNOME_WAYLAND } from './common.const'; @@ -437,6 +438,14 @@ function initWinEventListeners(app: Electron.App): void { error('Refused to open URL with disallowed scheme via openExternal'); return; } + // A local file: URL (a folder/file linked from a task) must open via + // openPath, not openExternal: openExternal percent-encodes the path and + // Windows' ShellExecute then can't resolve non-ASCII names or spaces. + // See openLocalPath / issue #8695. + if (isLocalFileUrl(url)) { + openLocalPath(url); + return; + } // needed for mac; especially for jira urls we might have a host like this www.host.de// const urlObj = new URL(url); urlObj.pathname = urlObj.pathname.replace('//', '/'); diff --git a/electron/open-url.test.cjs b/electron/open-url.test.cjs new file mode 100644 index 0000000000..b7b4c5c239 --- /dev/null +++ b/electron/open-url.test.cjs @@ -0,0 +1,131 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const Module = require('node:module'); + +require('ts-node/register/transpile-only'); + +const openUrlPath = path.resolve(__dirname, 'open-url.ts'); +const originalModuleLoad = Module._load; + +let openPathCalls = []; + +const installMocks = () => { + openPathCalls = []; + Module._load = function patchedLoad(request, parent, isMain) { + if (request === 'electron') { + return { + shell: { + openPath: (p) => { + openPathCalls.push(p); + return Promise.resolve(''); + }, + openExternal: () => Promise.resolve(), + }, + }; + } + return originalModuleLoad.call(this, request, parent, isMain); + }; +}; + +const restoreMocks = () => { + Module._load = originalModuleLoad; +}; + +const loadModule = () => { + delete require.cache[openUrlPath]; + installMocks(); + try { + return require(openUrlPath); + } finally { + restoreMocks(); + } +}; + +test('isLocalFileUrl detects local file: URLs (case-insensitive, leading space)', () => { + const { isLocalFileUrl } = loadModule(); + assert.equal(isLocalFileUrl('file:///D:/x'), true); + assert.equal(isLocalFileUrl('FILE:///D:/x'), true); + assert.equal(isLocalFileUrl(' file:///D:/x'), true); + assert.equal(isLocalFileUrl('https://example.com'), false); + assert.equal(isLocalFileUrl('C:\\Projects\\x'), false); + assert.equal(isLocalFileUrl('/home/x'), false); +}); + +// NOTE: these decode tests run on Linux CI, where fileURLToPath returns POSIX +// form ("/D:/Projects/Grüne") and does NOT do the drive-letter + backslash +// conversion ("D:\\Projects\\Grüne") that is the actual Windows behavior. They +// verify percent-decoding (the mechanism that was broken); the Windows-specific +// path conversion relies on Node's documented cross-platform fileURLToPath +// contract and can only be confirmed on-device. +test('openLocalPath decodes non-ASCII chars in a file: URL (issue #8695)', () => { + const { openLocalPath } = loadModule(); + // The umlaut arrives percent-encoded from the WHATWG URL parser (ü → %C3%BC). + openLocalPath('file:///D:/Projects/Gr%C3%BCne'); + assert.equal(openPathCalls.length, 1); + // openPath must receive the DECODED name, never the literal %-escape that + // ShellExecute would treat as the folder name. + assert.ok(openPathCalls[0].includes('Grüne'), openPathCalls[0]); + assert.ok(!openPathCalls[0].includes('%C3%BC'), openPathCalls[0]); +}); + +test('openLocalPath decodes spaces in a file: URL (issue #8695)', () => { + const { openLocalPath } = loadModule(); + openLocalPath('file:///D:/Projects/Another%20One'); + assert.equal(openPathCalls.length, 1); + assert.ok(openPathCalls[0].includes('Another One'), openPathCalls[0]); + assert.ok(!openPathCalls[0].includes('%20'), openPathCalls[0]); +}); + +test('openLocalPath accepts a raw (unencoded) file: URL with backslashes', () => { + const { openLocalPath } = loadModule(); + openLocalPath('file:///D:\\Projects\\Grüne'); + assert.equal(openPathCalls.length, 1); + assert.ok(openPathCalls[0].includes('Grüne'), openPathCalls[0]); +}); + +test('openLocalPath passes a plain filesystem path through unchanged', () => { + const { openLocalPath } = loadModule(); + openLocalPath('/home/user/Grüne Ordner/notes.txt'); + assert.deepEqual(openPathCalls, ['/home/user/Grüne Ordner/notes.txt']); +}); + +test('openLocalPath blocks executable file: URLs', () => { + const { openLocalPath } = loadModule(); + openLocalPath('file:///C:/tmp/evil.exe'); + openLocalPath('file:///C:/tmp/evil.bat'); + assert.deepEqual(openPathCalls, []); +}); + +test('openLocalPath blocks UNC paths', () => { + const { openLocalPath } = loadModule(); + openLocalPath('\\\\host\\share\\file.txt'); + openLocalPath('//host/share/file.txt'); + assert.deepEqual(openPathCalls, []); +}); + +test('openLocalPath blocks a path-based UNC file: URL (four slashes)', () => { + const { openLocalPath } = loadModule(); + // The OPEN_PATH sink has no isExternalUrlSchemeAllowed pre-gate, so + // openLocalPath alone must block this NTLM-leak vector (GHSA-hr87-735w-hfq3): + // file:////host/share decodes to //host/share, which isUncPath rejects. + openLocalPath('file:////host/share'); + assert.deepEqual(openPathCalls, []); +}); + +test('openLocalPath blocks an executable hidden behind a percent-encoded dot', () => { + const { openLocalPath } = loadModule(); + // fileURLToPath decodes %2E → "." so the executable guard sees the real + // ".bat" extension. (Stricter than the old openExternal path, where the + // encoded dot hid the extension.) + openLocalPath('file:///C:/tmp/evil%2Ebat'); + assert.deepEqual(openPathCalls, []); +}); + +test('openLocalPath rejects a file: URL with a remote authority', () => { + const { openLocalPath } = loadModule(); + // fileURLToPath throws for a remote authority on POSIX; on Windows it yields a + // UNC path that isPathSafeToOpen then rejects. Either way: never opened. + openLocalPath('file://host/share'); + assert.deepEqual(openPathCalls, []); +}); diff --git a/electron/open-url.ts b/electron/open-url.ts new file mode 100644 index 0000000000..7cfb7275c1 --- /dev/null +++ b/electron/open-url.ts @@ -0,0 +1,49 @@ +import { shell } from 'electron'; +import { fileURLToPath } from 'node:url'; +import { + hasExecutableFileExtension, + isPathSafeToOpen, +} from './shared-with-frontend/is-external-url-allowed'; + +/** + * True for anything shaped like a `file:` URL. Intentionally broad: at the + * OPEN_PATH sink there is no `isExternalUrlSchemeAllowed` pre-gate, so this must + * catch every `file:`-shape value and hand it to `openLocalPath`, which + * re-validates the decoded path. (At the OPEN_EXTERNAL / navigation sinks, + * `isExternalUrlSchemeAllowed` has already narrowed the input to a canonical + * `file:///`.) Note this is deliberately looser than that check and than + * the renderer's `startsWith('file://')` guard — the safety comes from the + * post-decode guards in `openLocalPath`, not from this test. + */ +export const isLocalFileUrl = (value: string): boolean => /^\s*file:/i.test(value); + +/** + * Open a local filesystem path — or a local `file:` URL — with the OS default + * handler. + * + * A `file:` URL is decoded to a real filesystem path first: on Windows, + * `shell.openExternal` hands ShellExecute a Chromium-percent-encoded URL + * (`ü` → `%C3%BC`, space → `%20`), which then searches for a literally-named + * folder and fails to open it. `fileURLToPath` decodes those escapes and + * converts `/C:/…` → `C:\…`, so folders/files with non-ASCII names or spaces + * open correctly. See issue #8695. + * + * Enforces the two guards required at every `openPath` sink: reject UNC / + * remote paths (they make the OS reach a remote SMB host and leak the user's + * NTLM hash) and never launch an executable/script. See GHSA-hr87-735w-hfq3. + */ +export const openLocalPath = (pathOrFileUrl: string): void => { + let fsPath = pathOrFileUrl; + if (isLocalFileUrl(pathOrFileUrl)) { + try { + fsPath = fileURLToPath(pathOrFileUrl.trim()); + } catch { + // Malformed file: URL (e.g. a remote authority fileURLToPath rejects). + return; + } + } + if (!isPathSafeToOpen(fsPath) || hasExecutableFileExtension(fsPath)) { + return; + } + shell.openPath(fsPath); +};