mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
* fix(electron): open file:// URLs with special characters (#8695) Local file:// URLs with non-ASCII names or spaces (e.g. Grüne, "Another One") failed to open on Windows: both open sinks handed the URL to shell.openExternal, which percent-encodes the path (ü → %C3%BC, space → %20) before ShellExecute, so the OS then searched for a literally-named folder and failed. Pure-ASCII paths worked because there was nothing to encode. Route local file: URLs through shell.openPath with a decoded filesystem path (fileURLToPath) instead. openPath takes a raw path, so Unicode names and spaces open correctly. This also applies the executable-extension guard to file: URLs, which previously only ran on the OPEN_PATH sink (GHSA-hr87-735w-hfq3). Shared openLocalPath()/isLocalFileUrl() cover both the OPEN_EXTERNAL IPC handler and the navigation-interception path in main-window. * test(electron): cover NTLM/exec vectors at the un-pre-gated openPath sink Follow-up to the #8695 review. Add regression tests locking in that openLocalPath alone (OPEN_PATH has no scheme pre-gate) blocks the path-based UNC file: URL (file:////host/share, GHSA-hr87-735w-hfq3) and an executable hidden behind a percent-encoded dot (evil%2Ebat). Document that the decode tests run on Linux and therefore don't exercise the Windows drive-letter/backslash conversion, and clarify why isLocalFileUrl is intentionally broader than the renderer/scheme-allowlist checks.
This commit is contained in:
parent
3d952fb157
commit
4e0e2d9ffd
4 changed files with 204 additions and 21 deletions
|
|
@ -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);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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('//', '/');
|
||||
|
|
|
|||
131
electron/open-url.test.cjs
Normal file
131
electron/open-url.test.cjs
Normal file
|
|
@ -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, []);
|
||||
});
|
||||
49
electron/open-url.ts
Normal file
49
electron/open-url.ts
Normal file
|
|
@ -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:///<path>`.) 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);
|
||||
};
|
||||
Loading…
Add table
Add a link
Reference in a new issue