From 33111e46ffa2dd660ef8f7ed7467c91fd19039ae Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Tue, 9 Jun 2026 19:14:13 +0200 Subject: [PATCH] fix: harden local file URL guard --- .../is-external-url-allowed.ts | 29 +++++++++++++-- .../task-attachment-list.component.spec.ts | 3 ++ src/app/ui/is-external-url-allowed.spec.ts | 22 +++++++++++ src/app/ui/marked-options-factory.spec.ts | 37 +++++++++++-------- 4 files changed, 72 insertions(+), 19 deletions(-) diff --git a/electron/shared-with-frontend/is-external-url-allowed.ts b/electron/shared-with-frontend/is-external-url-allowed.ts index 0f071792af..b78e0b7720 100644 --- a/electron/shared-with-frontend/is-external-url-allowed.ts +++ b/electron/shared-with-frontend/is-external-url-allowed.ts @@ -13,7 +13,21 @@ */ export const ALLOWED_EXTERNAL_URL_SCHEMES = ['http:', 'https:', 'mailto:', 'file:']; +const LOCAL_FILE_URL_PREFIX = 'file:///'; +const ASCII_CONTROL_CHAR_RE = /[\u0000-\u001f\u007f]/; const _isSlash = (char: string): boolean => char === '/' || char === '\\'; +const _isEncodedSlashAt = (value: string, index: number): boolean => + value.startsWith('%2f', index) || value.startsWith('%5c', index); +const _hasUncLikeLocalFilePathStart = (value: string): boolean => { + const firstLocalPathCharIndex = LOCAL_FILE_URL_PREFIX.length; + return ( + value[firstLocalPathCharIndex] === '/' || + value[firstLocalPathCharIndex] === '\\' || + _isEncodedSlashAt(value, firstLocalPathCharIndex) + ); +}; +const _isLocalFileUrlWithoutUncPrefix = (value: string): boolean => + value.startsWith(LOCAL_FILE_URL_PREFIX) && !_hasUncLikeLocalFilePathStart(value); /** * True for a UNC / network path such as `\\host\share` or `//host/share` — i.e. @@ -60,11 +74,18 @@ export const isExternalUrlSchemeAllowed = (url: unknown): boolean => { if (parsed.protocol === 'file:') { // Allow ONLY the canonical local form `file:///`. 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. + // remote SMB reference and leaks the user's NTLM hash. Gate on both the RAW + // string and the parser-normalized href: raw catches parser differences, + // normalized catches dot-segment/control-character rewrites. const lower = trimmed.toLowerCase(); - return lower.startsWith('file:///') && lower[8] !== '/' && lower[8] !== '\\'; + if (ASCII_CONTROL_CHAR_RE.test(lower)) { + return false; + } + const normalizedLower = parsed.href.toLowerCase(); + return ( + _isLocalFileUrlWithoutUncPrefix(lower) && + _isLocalFileUrlWithoutUncPrefix(normalizedLower) + ); } return true; }; diff --git a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts index 8dcfea509e..1bd4376979 100644 --- a/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts +++ b/src/app/features/tasks/task-attachment/task-attachment-list/task-attachment-list.component.spec.ts @@ -52,6 +52,9 @@ describe('TaskAttachmentListComponent', () => { [ 'file://192.168.1.100/share/pixel.png', 'file:////host/share/pixel.png', + 'file:///%5C%5Chost/share/pixel.png', + 'file:///%2F%2Fhost/share/pixel.png', + 'file:///%2e%2e/%2F%2Fhost/share/pixel.png', '\\\\host\\share\\pixel.png', '//host/share/pixel.png', ].forEach((path) => { diff --git a/src/app/ui/is-external-url-allowed.spec.ts b/src/app/ui/is-external-url-allowed.spec.ts index 7e80b31852..9a40428ea0 100644 --- a/src/app/ui/is-external-url-allowed.spec.ts +++ b/src/app/ui/is-external-url-allowed.spec.ts @@ -50,6 +50,17 @@ describe('isExternalUrlSchemeAllowed', () => { 'file://192.168.1.100/share/x', 'file:////host/share', // path-based UNC, empty host 'file://///host/share', + 'file:///%5C%5Chost/share', // percent-encoded \\host/share + 'file:///%5c%5chost/share', + 'file:///%5Chost/share', + 'file:///%2F%2Fhost/share', // percent-encoded //host/share + 'file:///%2f%2fhost/share', + 'file:///%2Fhost/share', + 'file:///%2e//host/share', // dot-segment normalizes to file:////host/share + 'file:///%2e%2e/%2F%2Fhost/share', + 'file:///a/..//host/share', + 'file:///./%2F%2Fhost/share', + 'file:///\t//host/share', 'file:\\\\host\\share', 'FILE://HOST/share', // case-insensitive ]; @@ -96,6 +107,17 @@ describe('isExternalUrlSchemeAllowed', () => { 'file://host/share', 'file://192.168.1.100/share/x', 'file:////host/share', + 'file:///%5C%5Chost/share', + 'file:///%5c%5chost/share', + 'file:///%5Chost/share', + 'file:///%2F%2Fhost/share', + 'file:///%2f%2fhost/share', + 'file:///%2Fhost/share', + 'file:///%2e//host/share', + 'file:///%2e%2e/%2F%2Fhost/share', + 'file:///a/..//host/share', + 'file:///./%2F%2Fhost/share', + 'file:///\t//host/share', '', undefined, null, diff --git a/src/app/ui/marked-options-factory.spec.ts b/src/app/ui/marked-options-factory.spec.ts index 0b9d5eff70..0c4f217253 100644 --- a/src/app/ui/marked-options-factory.spec.ts +++ b/src/app/ui/marked-options-factory.spec.ts @@ -361,21 +361,25 @@ describe('markedOptionsFactory', () => { 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(' { + 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(' { const linkRenderer = options.renderer!.link.bind({ parser: mockParser }); @@ -531,6 +535,9 @@ describe('markedOptionsFactory', () => { [ 'file://192.168.1.100/share/pixel.png', 'file:////host/share/pixel.png', + 'file:///%5C%5Chost/share/pixel.png', + 'file:///%2F%2Fhost/share/pixel.png', + 'file:///%2e%2e/%2F%2Fhost/share/pixel.png', '\\\\host\\share\\pixel.png', '//host/share/pixel.png', ].forEach((href) => {