mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(electron): validate URL scheme before shell.openExternal/openPath (GHSA-hr87-735w-hfq3) (#8210)
* fix(electron): validate URL scheme before shell.openExternal (GHSA-hr87-735w-hfq3)
Task-note Markdown links had their href passed verbatim to shell.openExternal
on click, letting note content (sync, imported backups, issue-provider data)
silently invoke any OS-registered protocol handler — ms-msdt: (Follina),
search-ms:, \\host\share (NTLM hash capture), etc. — with one click and no
prompt. Angular's sanitizer does not help: its SAFE_URL_PATTERN blocks only
javascript:, so these schemes pass.
Add a shared scheme allowlist (http/https/mailto/file) enforced at both layers:
- marked renderer (marked-options-factory): blocked-scheme links render as
inert <span> text instead of an anchor (covers all note render paths + web).
- Electron main process, both shell.openExternal sinks: the OPEN_EXTERNAL IPC
handler (system.ts) and openUrlInBrowser (main-window.ts, the
will-navigate/window-open path used by the fullscreen/archived dialogs).
The allowlist lives in electron/shared-with-frontend so renderer and main agree
on one policy. Blocked links are styled muted/non-interactive with an
explanatory tooltip.
* fix(electron): harden openExternal/openPath against SMB & attribute-injection (GHSA-hr87-735w-hfq3)
Follow-up hardening from multi-agent review of the initial fix:
- file:// SMB bypass: `file:` was allow-listed unconditionally, so
`file://host/share` / `file:////host` reached shell.openExternal — the same
NTLM-leak vector the raw `\\host\share` check blocked. Restrict `file:` to the
canonical local form `file:///<path>`. Gated on the raw string because
Chromium (renderer) and Node (main) parse file: hosts/paths differently.
- IPC.OPEN_PATH was unguarded: a synced FILE-attachment path of `\\host\share`
leaked NTLM via shell.openPath. Reject UNC paths (new isUncPath helper).
- Attribute-injection XSS: the marked link renderer interpolated href/title
raw; marked's angle-bracket link form could inject an event handler that runs
under disableSanitizer. HTML-escape href/title.
- Remove the now-dead/misleading explicit backslash check and redundant
toLowerCase (new URL already throws on raw UNC; protocol is spec-lowercase).
Adds cross-engine-verified tests for file:// SMB forms, isUncPath, and the
href quote-injection case.
* fix(electron): block remote file:// in OPEN_PATH too (GHSA-hr87-735w-hfq3)
Re-review found the OPEN_PATH guard was incomplete: isUncPath only rejects raw
slash-prefixed paths, but shell.openPath('file://host/share') resolves to
\\host\share on Windows — the same NTLM leak. A synced FILE-attachment path is
attacker-controllable.
Centralize the path policy in isPathSafeToOpen() (rejects UNC paths and remote
file:// URLs, allows local paths and file:///) so the two sinks can't drift.
* fix(electron): gate image src against remote file:// / UNC (GHSA-hr87-735w-hfq3)
Markdown image src auto-loads on render (no click), so a remote
file://host/share or UNC src silently triggers an SMB connection and
leaks the user's NTLM hash just by viewing a synced/imported note — the
same vector the link/openExternal gate already blocks, but with no user
interaction. CSP (img-src ... file:) does not block it.
Gate image href through isPathSafeToOpen: remote file:// and UNC srcs
render as inert (HTML-escaped) alt text instead of an <img>; local
file://, http(s), data: and blob: images are unaffected.
Also drop a dead eslint-disable directive on ALLOWED_EXTERNAL_URL_SCHEMES
(the name does not trip naming-convention).
* fix(tasks): gate IMG attachment src against remote file:// / UNC (GHSA-hr87-735w-hfq3)
An IMG-type task attachment renders <img [src]="resolvedOriginalPath">,
which auto-loads on render with no click. A synced/imported remote
file://host or UNC path there silently triggers an SMB fetch and leaks
the user's NTLM hash just by opening the task — the same no-click vector
the markdown-image gate already blocks, but via Angular template binding
(Angular's URL sanitizer does not help; SAFE_URL_PATTERN allows file:).
Drop unsafe srcs via isPathSafeToOpen so they never reach the binding;
local file://, http(s), data: and blob: images are unaffected.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This commit is contained in:
parent
04e6abdf71
commit
cd86ba2c0d
9 changed files with 428 additions and 12 deletions
|
|
@ -1,10 +1,30 @@
|
|||
import { app, dialog, ipcMain, shell } from 'electron';
|
||||
import { IPC } from '../shared-with-frontend/ipc-events.const';
|
||||
import {
|
||||
isExternalUrlSchemeAllowed,
|
||||
isPathSafeToOpen,
|
||||
} from '../shared-with-frontend/is-external-url-allowed';
|
||||
import { getWin } from '../main-window';
|
||||
|
||||
export const initSystemIpc = (): void => {
|
||||
ipcMain.on(IPC.OPEN_PATH, (ev, path: string) => shell.openPath(path));
|
||||
ipcMain.on(IPC.OPEN_EXTERNAL, (ev, url: string) => shell.openExternal(url));
|
||||
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;
|
||||
}
|
||||
shell.openPath(path);
|
||||
});
|
||||
ipcMain.on(IPC.OPEN_EXTERNAL, (ev, url: string) => {
|
||||
// Defense in depth: never hand an unsafe scheme to the OS handler, even if
|
||||
// the renderer-side guard is bypassed. See GHSA-hr87-735w-hfq3.
|
||||
if (!isExternalUrlSchemeAllowed(url)) {
|
||||
return;
|
||||
}
|
||||
shell.openExternal(url);
|
||||
});
|
||||
|
||||
ipcMain.on(
|
||||
IPC.SHOW_EMOJI_PANEL,
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ import { errorHandlerWithFrontendInform } from './error-handler-with-frontend-in
|
|||
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 { readFileSync, stat, writeFileSync } from 'fs';
|
||||
import { error, log } from 'electron-log/main';
|
||||
import { IS_MAC, IS_GNOME_DESKTOP } from './common.const';
|
||||
|
|
@ -414,6 +415,14 @@ export const setWasMaximizedBeforeHide = (value: boolean): void => {
|
|||
// eslint-disable-next-line prefer-arrow/prefer-arrow-functions
|
||||
function initWinEventListeners(app: Electron.App): void {
|
||||
const openUrlInBrowser = (url: string): void => {
|
||||
// Defense in depth: never hand an unsafe scheme to the OS handler, even if
|
||||
// a renderer-side guard is bypassed (e.g. a link click that falls through
|
||||
// to navigation rather than the explicit openExternalUrl IPC). The blocked
|
||||
// schemes are OS protocol handlers / UNC paths. See GHSA-hr87-735w-hfq3.
|
||||
if (!isExternalUrlSchemeAllowed(url)) {
|
||||
error('Refused to open URL with disallowed scheme via openExternal');
|
||||
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('//', '/');
|
||||
|
|
|
|||
96
electron/shared-with-frontend/is-external-url-allowed.ts
Normal file
96
electron/shared-with-frontend/is-external-url-allowed.ts
Normal file
|
|
@ -0,0 +1,96 @@
|
|||
/**
|
||||
* URL schemes that are permitted to reach the OS handler via shell.openExternal.
|
||||
*
|
||||
* Task notes render Markdown links whose href would otherwise be passed verbatim
|
||||
* to the OS handler on click. Without this gate, anyone who can populate note
|
||||
* content (multi-device sync, shared/imported backups, issue-provider content)
|
||||
* could make a single click silently invoke any OS-registered protocol —
|
||||
* `ms-msdt:`, `search-ms:`, etc.
|
||||
* See GHSA-hr87-735w-hfq3.
|
||||
*
|
||||
* Shared between the Angular renderer (link rendering) and the Electron main
|
||||
* process (shell.openExternal call sites) so both layers enforce one policy.
|
||||
*/
|
||||
export const ALLOWED_EXTERNAL_URL_SCHEMES = ['http:', 'https:', 'mailto:', 'file:'];
|
||||
|
||||
const _isSlash = (char: string): boolean => char === '/' || char === '\\';
|
||||
|
||||
/**
|
||||
* True for a UNC / network path such as `\\host\share` or `//host/share` — i.e.
|
||||
* any path whose first two characters are slashes (in either direction).
|
||||
*
|
||||
* Opening such a path (`shell.openPath` / `shell.openExternal('file://host/…')`)
|
||||
* makes the OS reach out to a remote SMB host, leaking the user's NTLM hash.
|
||||
* Local absolute paths (`/home/x`, `C:\x`) and POSIX/Windows roots are NOT UNC.
|
||||
*/
|
||||
export const isUncPath = (path: unknown): boolean => {
|
||||
if (typeof path !== 'string') {
|
||||
return false;
|
||||
}
|
||||
const trimmed = path.trim();
|
||||
return trimmed.length >= 2 && _isSlash(trimmed[0]) && _isSlash(trimmed[1]);
|
||||
};
|
||||
|
||||
/**
|
||||
* Returns true only for URLs whose scheme is in ALLOWED_EXTERNAL_URL_SCHEMES.
|
||||
* Schemeless/relative input and any string that fails URL parsing are rejected.
|
||||
* `file:` is restricted to LOCAL files — a `file:` URL with a remote authority
|
||||
* (`file://host/share`, or a path-based UNC like `file:////host/share`) is the
|
||||
* same NTLM-hash-leak vector as a raw `\\host\share` path and is rejected.
|
||||
*/
|
||||
export const isExternalUrlSchemeAllowed = (url: unknown): boolean => {
|
||||
if (typeof url !== 'string') {
|
||||
return false;
|
||||
}
|
||||
const trimmed = url.trim();
|
||||
if (!trimmed) {
|
||||
return false;
|
||||
}
|
||||
let parsed: URL;
|
||||
try {
|
||||
// Raw UNC paths (`\\host\share`, `//host`) and schemeless input throw here.
|
||||
parsed = new URL(trimmed);
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
// `protocol` is always lower-cased by the WHATWG URL parser.
|
||||
if (!ALLOWED_EXTERNAL_URL_SCHEMES.includes(parsed.protocol)) {
|
||||
return false;
|
||||
}
|
||||
if (parsed.protocol === 'file:') {
|
||||
// Allow ONLY the canonical local form `file:///<path>`. Anything with an
|
||||
// authority (`file://host/…`) or a path-based UNC (`file:////host`) is a
|
||||
// remote SMB reference and leaks the user's NTLM hash. Gate on the RAW
|
||||
// string: Chromium (renderer) and Node (main) parse file: hosts/paths
|
||||
// differently, so `parsed.host` / `parsed.pathname` are not portable.
|
||||
const lower = trimmed.toLowerCase();
|
||||
return lower.startsWith('file:///') && lower[8] !== '/' && lower[8] !== '\\';
|
||||
}
|
||||
return true;
|
||||
};
|
||||
|
||||
/**
|
||||
* Returns true if a value is safe to hand to `shell.openPath` (which opens a
|
||||
* filesystem path, not a URL). Rejects UNC paths AND `file:`-scheme values with
|
||||
* a remote authority — `shell.openPath('file://host/share')` resolves to
|
||||
* `\\host\share` on Windows, the same NTLM-leak `isUncPath` blocks for the raw
|
||||
* form. Plain local paths (`/home/x`, `C:\x`, `./rel`) are allowed.
|
||||
* See GHSA-hr87-735w-hfq3.
|
||||
*/
|
||||
export const isPathSafeToOpen = (path: unknown): boolean => {
|
||||
if (typeof path !== 'string') {
|
||||
return false;
|
||||
}
|
||||
const trimmed = path.trim();
|
||||
if (!trimmed) {
|
||||
return false;
|
||||
}
|
||||
if (isUncPath(trimmed)) {
|
||||
return false;
|
||||
}
|
||||
// A file:-scheme value must be a LOCAL file URL, not a remote authority.
|
||||
if (trimmed.toLowerCase().startsWith('file:')) {
|
||||
return isExternalUrlSchemeAllowed(trimmed);
|
||||
}
|
||||
return true;
|
||||
};
|
||||
|
|
@ -40,6 +40,38 @@ describe('TaskAttachmentListComponent', () => {
|
|||
snackService = TestBed.inject(SnackService) as jasmine.SpyObj<SnackService>;
|
||||
});
|
||||
|
||||
describe('resolvedAttachments img src safety (GHSA-hr87-735w-hfq3)', () => {
|
||||
const imgAttachment = (path: string): TaskAttachment => ({
|
||||
id: 'a',
|
||||
type: 'IMG',
|
||||
title: 'x',
|
||||
path,
|
||||
});
|
||||
|
||||
it('drops remote file:// / UNC src so the <img> cannot auto-load and leak NTLM', () => {
|
||||
[
|
||||
'file://192.168.1.100/share/pixel.png',
|
||||
'file:////host/share/pixel.png',
|
||||
'\\\\host\\share\\pixel.png',
|
||||
'//host/share/pixel.png',
|
||||
].forEach((path) => {
|
||||
fixture.componentRef.setInput('attachments', [imgAttachment(path)]);
|
||||
expect(component.resolvedAttachments()[0].resolvedOriginalPath).toBeUndefined();
|
||||
});
|
||||
});
|
||||
|
||||
it('keeps safe srcs (local file://, http(s), data:)', () => {
|
||||
[
|
||||
'file:///home/user/img.png',
|
||||
'https://example.com/img.png',
|
||||
'data:image/png;base64,iVBORw0KGgo=',
|
||||
].forEach((path) => {
|
||||
fixture.componentRef.setInput('attachments', [imgAttachment(path)]);
|
||||
expect(component.resolvedAttachments()[0].resolvedOriginalPath).toBe(path);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('copy', () => {
|
||||
let attachment: TaskAttachment;
|
||||
let originalClipboardDescriptor: PropertyDescriptor | undefined;
|
||||
|
|
|
|||
|
|
@ -19,6 +19,7 @@ import { MatIcon } from '@angular/material/icon';
|
|||
import { EnlargeImgDirective } from '../../../../ui/enlarge-img/enlarge-img.directive';
|
||||
import { MatAnchor, MatButton } from '@angular/material/button';
|
||||
import { ClipboardImageService } from '../../../../core/clipboard-image/clipboard-image.service';
|
||||
import { isPathSafeToOpen } from '../../../../../../electron/shared-with-frontend/is-external-url-allowed';
|
||||
|
||||
interface ResolvedAttachment extends TaskAttachment {
|
||||
resolvedPath?: string;
|
||||
|
|
@ -66,9 +67,15 @@ export class TaskAttachmentListComponent {
|
|||
: att.path;
|
||||
|
||||
const imgPath = att.originalImgPath || att.path;
|
||||
const resolvedOriginalPath = imgPath?.startsWith('indexeddb://clipboard-images/')
|
||||
const rawOriginalPath = imgPath?.startsWith('indexeddb://clipboard-images/')
|
||||
? urlMap.get(imgPath) || imgPath
|
||||
: imgPath;
|
||||
// The <img> src auto-loads on render (no click), so a synced remote
|
||||
// file://host / UNC path would silently leak the user's NTLM hash. Drop
|
||||
// such srcs so they never reach the binding. See GHSA-hr87-735w-hfq3.
|
||||
const resolvedOriginalPath = isPathSafeToOpen(rawOriginalPath)
|
||||
? rawOriginalPath
|
||||
: undefined;
|
||||
|
||||
const isLoading =
|
||||
att.path?.startsWith('indexeddb://clipboard-images/') &&
|
||||
|
|
|
|||
125
src/app/ui/is-external-url-allowed.spec.ts
Normal file
125
src/app/ui/is-external-url-allowed.spec.ts
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
import {
|
||||
ALLOWED_EXTERNAL_URL_SCHEMES,
|
||||
isExternalUrlSchemeAllowed,
|
||||
isPathSafeToOpen,
|
||||
isUncPath,
|
||||
} from '../../../electron/shared-with-frontend/is-external-url-allowed';
|
||||
|
||||
describe('isExternalUrlSchemeAllowed', () => {
|
||||
describe('allowed schemes', () => {
|
||||
const allowed = [
|
||||
'http://example.com',
|
||||
'https://example.com/path?q=1#frag',
|
||||
'HTTPS://EXAMPLE.COM', // scheme is case-insensitive
|
||||
'mailto:someone@example.com',
|
||||
'file:///home/user/notes.txt',
|
||||
' https://example.com ', // surrounding whitespace tolerated
|
||||
];
|
||||
allowed.forEach((url) => {
|
||||
it(`allows "${url}"`, () => {
|
||||
expect(isExternalUrlSchemeAllowed(url)).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
it('keeps the allowlist in sync with expectations', () => {
|
||||
expect(ALLOWED_EXTERNAL_URL_SCHEMES).toEqual([
|
||||
'http:',
|
||||
'https:',
|
||||
'mailto:',
|
||||
'file:',
|
||||
]);
|
||||
});
|
||||
});
|
||||
|
||||
describe('blocked schemes (GHSA-hr87-735w-hfq3)', () => {
|
||||
const blocked = [
|
||||
'ms-calculator:',
|
||||
'ms-msdt:/id PCWDiagnostic', // Follina (CVE-2022-30190)
|
||||
'search-ms:query=foo',
|
||||
'ms-officecmd:{}',
|
||||
'javascript:alert(1)',
|
||||
'data:text/html,<script>alert(1)</script>',
|
||||
'vbscript:msgbox(1)',
|
||||
'ftp://example.com',
|
||||
'ssh://example.com',
|
||||
'tel:+123456789',
|
||||
'\\\\192.168.1.100\\share', // UNC / SMB — NTLM hash capture
|
||||
'/\\192.168.1.100\\share',
|
||||
// file: with a remote authority is the same SMB / NTLM-leak vector and
|
||||
// must be blocked even though `file:` is allow-listed for local files.
|
||||
'file://192.168.1.100/share/x',
|
||||
'file:////host/share', // path-based UNC, empty host
|
||||
'file://///host/share',
|
||||
'file:\\\\host\\share',
|
||||
'FILE://HOST/share', // case-insensitive
|
||||
];
|
||||
blocked.forEach((url) => {
|
||||
it(`blocks "${url}"`, () => {
|
||||
expect(isExternalUrlSchemeAllowed(url)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it('still allows LOCAL file: URLs (Windows drive + POSIX paths)', () => {
|
||||
expect(isExternalUrlSchemeAllowed('file:///home/user/notes.txt')).toBe(true);
|
||||
expect(isExternalUrlSchemeAllowed('file:///C:/Users/me/doc.pdf')).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isUncPath', () => {
|
||||
it('flags UNC / network paths', () => {
|
||||
['\\\\host\\share', '//host/share', '/\\host', '\\/host', ' \\\\host\\s'].forEach(
|
||||
(p) => expect(isUncPath(p)).toBe(true),
|
||||
);
|
||||
});
|
||||
|
||||
it('does not flag local absolute paths or non-strings', () => {
|
||||
['/home/user/x', 'C:\\Users\\me', './rel', '', '/', 'x', undefined, null].forEach(
|
||||
(p) => expect(isUncPath(p as unknown as string)).toBe(false),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('isPathSafeToOpen (shell.openPath gate)', () => {
|
||||
it('allows local filesystem paths and local file:// URLs', () => {
|
||||
[
|
||||
'/home/user/doc.pdf',
|
||||
'C:\\Users\\me\\doc.pdf',
|
||||
'./rel/x',
|
||||
'file:///home/x',
|
||||
].forEach((p) => expect(isPathSafeToOpen(p)).toBe(true));
|
||||
});
|
||||
|
||||
it('blocks UNC paths AND remote file:// URLs (NTLM leak via shell.openPath)', () => {
|
||||
[
|
||||
'\\\\host\\share',
|
||||
'//host/share',
|
||||
'file://host/share',
|
||||
'file://192.168.1.100/share/x',
|
||||
'file:////host/share',
|
||||
'',
|
||||
undefined,
|
||||
null,
|
||||
].forEach((p) => expect(isPathSafeToOpen(p as unknown as string)).toBe(false));
|
||||
});
|
||||
});
|
||||
|
||||
describe('malformed / non-string input', () => {
|
||||
it('blocks empty and whitespace-only strings', () => {
|
||||
expect(isExternalUrlSchemeAllowed('')).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed(' ')).toBe(false);
|
||||
});
|
||||
|
||||
it('blocks schemeless / relative input', () => {
|
||||
expect(isExternalUrlSchemeAllowed('example.com')).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed('//example.com')).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed('./relative/path')).toBe(false);
|
||||
});
|
||||
|
||||
it('blocks non-string input', () => {
|
||||
expect(isExternalUrlSchemeAllowed(undefined)).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed(null)).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed(42)).toBe(false);
|
||||
expect(isExternalUrlSchemeAllowed({ href: 'https://example.com' })).toBe(false);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -352,6 +352,57 @@ describe('markedOptionsFactory', () => {
|
|||
expect(result).toContain('href="http://x" onmouseover="alert(1)"');
|
||||
expect(result).toContain('title="" onfocus="alert(2)"');
|
||||
});
|
||||
|
||||
// GHSA-hr87-735w-hfq3: the rendered href is passed verbatim to
|
||||
// shell.openExternal on click, so unsafe schemes must never become anchors.
|
||||
describe('unsafe URL schemes (GHSA-hr87-735w-hfq3)', () => {
|
||||
const mockParser = {
|
||||
parseInline: (tokens: any[]) =>
|
||||
tokens.map((t: any) => t.raw || t.text || '').join(''),
|
||||
};
|
||||
|
||||
['ms-calculator:', 'javascript:alert(1)', 'ssh://h', '\\\\host\\share'].forEach(
|
||||
(href) => {
|
||||
it(`renders "${href}" as inert text, not an anchor`, () => {
|
||||
const linkRenderer = options.renderer!.link.bind({ parser: mockParser });
|
||||
const result = linkRenderer({
|
||||
href,
|
||||
title: 'x',
|
||||
tokens: [{ type: 'text', raw: 'Click here', text: 'Click here' }],
|
||||
} as any);
|
||||
expect(result).not.toContain('<a ');
|
||||
expect(result).not.toContain(`href="${href}"`);
|
||||
expect(result).toContain('Click here');
|
||||
});
|
||||
},
|
||||
);
|
||||
|
||||
it('still renders allowed schemes (mailto:, file:) as anchors', () => {
|
||||
const linkRenderer = options.renderer!.link.bind({ parser: mockParser });
|
||||
['mailto:a@b.com', 'file:///tmp/x', 'https://example.com'].forEach((href) => {
|
||||
const result = linkRenderer({
|
||||
href,
|
||||
title: '',
|
||||
tokens: [{ type: 'text', raw: 'L', text: 'L' }],
|
||||
} as any);
|
||||
expect(result).toContain(`href="${href}"`);
|
||||
});
|
||||
});
|
||||
|
||||
it('escapes a quote-injection href so it cannot break out of the attribute', () => {
|
||||
const linkRenderer = options.renderer!.link.bind({ parser: mockParser });
|
||||
const result = linkRenderer({
|
||||
href: 'https://example.com/" onmouseover="alert(1)',
|
||||
title: 'a"b',
|
||||
tokens: [{ type: 'text', raw: 'L', text: 'L' }],
|
||||
} as any);
|
||||
expect(result).not.toContain('onmouseover="alert(1)"');
|
||||
expect(result).toContain('"');
|
||||
expect(result).toContain(
|
||||
'href="https://example.com/" onmouseover="alert(1)"',
|
||||
);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('image renderer', () => {
|
||||
|
|
@ -472,6 +523,56 @@ describe('markedOptionsFactory', () => {
|
|||
);
|
||||
});
|
||||
});
|
||||
|
||||
// GHSA-hr87-735w-hfq3: an image src auto-loads on render (no click), so a
|
||||
// remote file:// / UNC src would silently leak the user's NTLM hash. Such
|
||||
// srcs must never reach the `src` attribute.
|
||||
describe('unsafe image src (GHSA-hr87-735w-hfq3)', () => {
|
||||
[
|
||||
'file://192.168.1.100/share/pixel.png',
|
||||
'file:////host/share/pixel.png',
|
||||
'\\\\host\\share\\pixel.png',
|
||||
'//host/share/pixel.png',
|
||||
].forEach((href) => {
|
||||
it(`blocks remote/UNC src "${href}" (renders no <img>)`, () => {
|
||||
const result = options.renderer!.image({
|
||||
href,
|
||||
title: null,
|
||||
text: 'Alt text',
|
||||
} as any);
|
||||
expect(result).not.toContain('<img');
|
||||
expect(result).not.toContain(`src="${href}"`);
|
||||
expect(result).toContain('Alt text');
|
||||
});
|
||||
});
|
||||
|
||||
it('still renders local file:// and remote web/data images', () => {
|
||||
[
|
||||
'file:///home/user/img.png',
|
||||
'http://example.com/img.png',
|
||||
'https://example.com/img.png',
|
||||
'data:image/png;base64,iVBORw0KGgo=',
|
||||
'blob:https://example.com/abc',
|
||||
].forEach((href) => {
|
||||
const result = options.renderer!.image({
|
||||
href,
|
||||
title: null,
|
||||
text: 'L',
|
||||
} as any);
|
||||
expect(result).toContain(`src="${href}"`);
|
||||
});
|
||||
});
|
||||
|
||||
it('escapes the blocked-image alt text so it cannot inject markup', () => {
|
||||
const result = options.renderer!.image({
|
||||
href: 'file://host/share/x.png',
|
||||
title: null,
|
||||
text: '<img src=x onerror=alert(1)>',
|
||||
} as any);
|
||||
expect(result).not.toContain('<img');
|
||||
expect(result).toContain('<img src=x onerror=alert(1)>');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('paragraph renderer', () => {
|
||||
|
|
|
|||
|
|
@ -1,14 +1,15 @@
|
|||
import { MarkedOptions, MarkedRenderer } from 'ngx-markdown';
|
||||
import { Hooks, Token } from 'marked';
|
||||
import {
|
||||
isExternalUrlSchemeAllowed,
|
||||
isPathSafeToOpen,
|
||||
} from '../../../electron/shared-with-frontend/is-external-url-allowed';
|
||||
|
||||
/**
|
||||
* Escape a string for safe interpolation into a double-quoted HTML attribute.
|
||||
*
|
||||
* Defense-in-depth: the note render surfaces run this renderer's output through
|
||||
* Angular's SecurityContext.HTML sanitizer (they no longer set disableSanitizer),
|
||||
* which already strips event-handler attributes. Escaping here additionally keeps
|
||||
* the raw renderer output non-injectable on its own, so a future sanitizer bypass
|
||||
* can't turn an attacker-controlled image href/title/alt into a new attribute.
|
||||
* Defense-in-depth: note renders are sanitized as HTML before display, but the
|
||||
* raw renderer output still must not be attribute-injectable on its own.
|
||||
* `&` must be replaced first.
|
||||
*/
|
||||
export const escapeHtmlAttr = (value: string): string =>
|
||||
|
|
@ -117,10 +118,12 @@ export const markedOptionsFactory = (): MarkedOptions => {
|
|||
tokens: Token[];
|
||||
}) {
|
||||
const text = tokens ? this.parser.parseInline(tokens) : '';
|
||||
// Escape href/title for parity with renderer.image — the raw renderer output
|
||||
// must not be attribute-injectable on its own (text is already-parsed inline
|
||||
// HTML, so it is not re-escaped). rel="noopener noreferrer" matches the
|
||||
// hardening in render-links.pipe.ts and avoids reverse-tabnabbing.
|
||||
// Block unsafe URL schemes from rendering as clickable links. On click the
|
||||
// href is passed verbatim to shell.openExternal (Electron), which would let
|
||||
// note content silently invoke OS protocol handlers. See GHSA-hr87-735w-hfq3.
|
||||
if (!isExternalUrlSchemeAllowed(href)) {
|
||||
return `<span class="markdown-blocked-link" title="Link blocked: unsafe URL scheme">${text}</span>`;
|
||||
}
|
||||
return `<a target="_blank" rel="noopener noreferrer" href="${escapeHtmlAttr(href)}" title="${escapeHtmlAttr(title || '')}">${text}</a>`;
|
||||
};
|
||||
|
||||
|
|
@ -136,6 +139,17 @@ export const markedOptionsFactory = (): MarkedOptions => {
|
|||
title: string | null;
|
||||
text: string;
|
||||
}) => {
|
||||
// Unlike links, an image src auto-loads on render (no click). A remote
|
||||
// `file://host/share` or UNC src would silently make the OS open an SMB
|
||||
// connection and leak the user's NTLM hash just by viewing the note, so
|
||||
// such srcs must never reach the `src` attribute. Remote web images
|
||||
// (http/https/data/blob) are unaffected. See GHSA-hr87-735w-hfq3.
|
||||
if (!isPathSafeToOpen(href)) {
|
||||
return `<span class="markdown-blocked-link" title="Image blocked: unsafe URL">${escapeHtmlAttr(
|
||||
text || '',
|
||||
)}</span>`;
|
||||
}
|
||||
|
||||
const { width, height } = parseImageDimensionsFromTitle(title);
|
||||
|
||||
// Build width and height attributes (not style, as Angular sanitizer strips inline styles)
|
||||
|
|
|
|||
|
|
@ -136,4 +136,16 @@
|
|||
.checkbox-label {
|
||||
cursor: pointer;
|
||||
}
|
||||
|
||||
// A markdown link with a disallowed URL scheme, or an image with an unsafe
|
||||
// (remote file:// / UNC) src, renders as inert text instead of an anchor/img
|
||||
// (security: GHSA-hr87-735w-hfq3, see marked-options-factory.ts). Style it as
|
||||
// muted, non-interactive text with a help cursor so blocked content reads as
|
||||
// intentionally disabled, not body copy (the title attribute explains why on
|
||||
// hover).
|
||||
.markdown-blocked-link {
|
||||
color: var(--text-color-muted);
|
||||
text-decoration: underline dotted;
|
||||
cursor: help;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue