From e11a18646d0b02002c2aa1efb42e64e46c056622 Mon Sep 17 00:00:00 2001 From: Sam Powers <35611153+sam-powers@users.noreply.github.com> Date: Sat, 13 Jun 2026 17:51:20 -0400 Subject: [PATCH] security: restrict link and update-banner URLs to safe schemes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two places trusted a URL more than they should: - normalizeHref (Cmd+K link mark) passed through any explicit scheme, so javascript: and data: could be persisted into the saved .md and later clicked — a stored script-execution vector in a shared document. It now allows only http/https/mailto/tel (plus in-page and relative refs); any other scheme returns empty. - The update banner's "View release" button opened the html_url from GitHub's API response verbatim. It now accepts that URL only if it parses as an https://github.com/ URL, falling back to the hardcoded releases page otherwise, so a spoofed or compromised response can't redirect the user to an arbitrary scheme or host. Adds coverage for the rejected schemes (javascript/data/vbscript/file) and for html_url fallback on a dangerous scheme and a non-github host. Co-Authored-By: Claude Opus 4.8 --- src/components/Toolbar.tsx | 24 ++++++++++++++++++++---- src/hooks/useUpdateCheck.ts | 22 +++++++++++++++++++++- src/test/components/Toolbar.test.ts | 13 +++++++++++++ src/test/hooks/useUpdateCheck.test.ts | 17 +++++++++++++++++ 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/src/components/Toolbar.tsx b/src/components/Toolbar.tsx index dbb5fac..1485761 100644 --- a/src/components/Toolbar.tsx +++ b/src/components/Toolbar.tsx @@ -3,14 +3,30 @@ import type { Editor } from '@tiptap/react'; import { toolbarSelectionStore } from './Editor'; /** - * Make a typed URL usable as an href: anything with an explicit scheme - * (https:, mailto:) or an in-page/relative reference passes through; a bare - * domain like "example.com" gets https://. Empty input stays empty. + * Schemes a link mark is allowed to carry. Anything else — most importantly + * `javascript:` and `data:` — is rejected rather than passed through, because + * the href is persisted into the saved `.md` and is later clickable: an + * untrusted scheme in a shared document would be a stored script-execution + * vector. The list is the set of navigations a Markdown link legitimately needs. + */ +const ALLOWED_LINK_SCHEMES = ['http', 'https', 'mailto', 'tel']; + +/** + * Make a typed URL usable as an href: a value with an allowed explicit scheme + * (https:, mailto:, tel:) or an in-page/relative reference passes through; a + * bare domain like "example.com" gets https://. A value carrying any other + * scheme (e.g. `javascript:`) is rejected and returns empty. Empty input stays + * empty. */ export function normalizeHref(raw: string): string { const url = raw.trim(); if (!url) return ''; - if (/^[a-z][a-z0-9+.-]*:/i.test(url) || /^[#/.]/.test(url)) return url; + // In-page / relative references (#anchor, /path, ./sibling) are always safe. + if (/^[#/.]/.test(url)) return url; + const schemeMatch = url.match(/^([a-z][a-z0-9+.-]*):/i); + if (schemeMatch) { + return ALLOWED_LINK_SCHEMES.includes(schemeMatch[1].toLowerCase()) ? url : ''; + } return `https://${url}`; } diff --git a/src/hooks/useUpdateCheck.ts b/src/hooks/useUpdateCheck.ts index aa6a427..2bfc997 100644 --- a/src/hooks/useUpdateCheck.ts +++ b/src/hooks/useUpdateCheck.ts @@ -5,6 +5,26 @@ const LATEST_RELEASE_API = 'https://api.github.com/repos/sam-powers/quill/releas const RELEASES_PAGE = 'https://github.com/sam-powers/quill/releases/latest'; const DISMISSED_KEY = 'quill.dismissed-update'; +/** + * The "View release" button opens this URL in the user's browser, so it must + * not be trusted just because it came back in the API response. Accept it only + * if it's an `https://github.com/` URL; otherwise fall back to the hardcoded + * releases page. (A compromised or spoofed response can't redirect the user to + * an arbitrary scheme or host.) + */ +function safeReleaseUrl(htmlUrl: string | undefined): string { + if (!htmlUrl) return RELEASES_PAGE; + try { + const parsed = new URL(htmlUrl); + if (parsed.protocol === 'https:' && parsed.hostname === 'github.com') { + return htmlUrl; + } + } catch { + // Not a parseable URL. + } + return RELEASES_PAGE; +} + export interface UpdateInfo { /** Version of the newer release, without the leading "v" (e.g. "0.4.0"). */ version: string; @@ -50,7 +70,7 @@ export function useUpdateCheck({ if (!release.tag_name || !isNewerVersion(release.tag_name, currentVersion)) return; const version = release.tag_name.replace(/^v/, ''); if (localStorage.getItem(DISMISSED_KEY) === version) return; - setUpdate({ version, url: release.html_url ?? RELEASES_PAGE }); + setUpdate({ version, url: safeReleaseUrl(release.html_url) }); } catch { // Offline, rate-limited, or unmounted mid-flight — stay quiet. } finally { diff --git a/src/test/components/Toolbar.test.ts b/src/test/components/Toolbar.test.ts index 285f626..74176f3 100644 --- a/src/test/components/Toolbar.test.ts +++ b/src/test/components/Toolbar.test.ts @@ -24,4 +24,17 @@ describe('normalizeHref', () => { expect(normalizeHref('')).toBe(''); expect(normalizeHref(' ')).toBe(''); }); + + it('passes through tel: links', () => { + expect(normalizeHref('tel:+15551234567')).toBe('tel:+15551234567'); + }); + + it('rejects dangerous schemes by returning empty', () => { + // These would be persisted into the saved .md and later clickable. + expect(normalizeHref('javascript:alert(1)')).toBe(''); + expect(normalizeHref('JavaScript:alert(1)')).toBe(''); + expect(normalizeHref('data:text/html,')).toBe(''); + expect(normalizeHref('vbscript:msgbox(1)')).toBe(''); + expect(normalizeHref('file:///etc/passwd')).toBe(''); + }); }); diff --git a/src/test/hooks/useUpdateCheck.test.ts b/src/test/hooks/useUpdateCheck.test.ts index b04c2ae..e71a8dc 100644 --- a/src/test/hooks/useUpdateCheck.test.ts +++ b/src/test/hooks/useUpdateCheck.test.ts @@ -81,6 +81,23 @@ describe('useUpdateCheck', () => { expect(second.result.current.update).toBeNull(); }); + it('falls back to the releases page when html_url is not a github.com https URL', async () => { + // A spoofed/compromised response must not redirect the user anywhere. + vi.stubGlobal('fetch', mockRelease('v0.4.0', 'javascript:alert(1)')); + const { result } = renderHook(() => useUpdateCheck({ currentVersion: '0.3.0', enabled: true })); + + await waitFor(() => expect(result.current.update).not.toBeNull()); + expect(result.current.update?.url).toBe('https://github.com/sam-powers/quill/releases/latest'); + }); + + it('falls back to the releases page when html_url is a non-github host', async () => { + vi.stubGlobal('fetch', mockRelease('v0.4.0', 'https://evil.example.com/phish')); + const { result } = renderHook(() => useUpdateCheck({ currentVersion: '0.3.0', enabled: true })); + + await waitFor(() => expect(result.current.update).not.toBeNull()); + expect(result.current.update?.url).toBe('https://github.com/sam-powers/quill/releases/latest'); + }); + it('a release newer than a dismissed one still shows', async () => { localStorage.setItem('quill.dismissed-update', '0.4.0'); vi.stubGlobal('fetch', mockRelease('v0.5.0'));