From 383c010173019f30bf352db91ca50b12e8c9febc Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sun, 23 Aug 2026 04:57:08 -0700 Subject: [PATCH] fix(files): allowlist the schemes a markdown link may target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `normalizeLinkHref` rejected only `file` for a `scheme://` target, so any other scheme was returned unchanged. `scheme://` is well-formed for every scheme, so the check let through spellings that are not navigable targets at all. - Keep a scheme only when it is http(s), ftp(s), mailto, or tel; drop the rest - Leave an existing link alone when a committed target normalizes away, rather than unsetting it — the editor seeds that field with the current href, so committing an untouched one previously removed the link Detection is unchanged for relative, anchor, protocol-relative, and bare-domain targets. A document's stored markdown is untouched: normalization runs on the render and edit paths, never on parse or serialize, so a target that is refused still round-trips verbatim. --- .../rich-markdown-editor/markdown-fidelity.ts | 21 ++++--- .../menus/link-editing.test.ts | 46 ++++++++++++++ .../menus/link-editing.tsx | 12 +++- .../rich-markdown-editor/round-trip.test.ts | 62 +++++++++++++++++++ 4 files changed, 128 insertions(+), 13 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts index 4470187fefa..fd46d579527 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/markdown-fidelity.ts @@ -76,21 +76,24 @@ export function applyFrontmatter(frontmatter: string, body: string): string { return frontmatter + body } -/** A leading `scheme://` URL (network protocol). */ -const SCHEME_URL = /^([a-z][a-z0-9+.-]*):\/\//i /** A leading `scheme:` token (per the URL grammar). */ const HAS_SCHEME = /^[a-z][a-z0-9+.-]*:/i /** A bare `host:port` (digits after the colon) — looks scheme-like but is really a domain. */ const HOST_PORT = /^[a-z0-9.-]+:\d+(?:[/?#]|$)/i +/** + * The only schemes a document link may target — an allowlist, because `scheme://` is well-formed for + * every scheme: rejecting just the ones known to be dangerous leaves the next one through, and + * `javascript://…` is a valid URL whose `//` run is merely a comment. + */ +const SAFE_SCHEME = /^(?:(?:https?|ftps?):\/\/|(?:mailto|tel):)/i + /** * Normalize a user-entered link target: prefix a bare domain with `https://` so it doesn't resolve * as an in-app relative URL, while leaving already-qualified, relative (`./other.md`, `../doc.md`), and - * protocol-relative URLs intact. Dangerous schemes are rejected outright rather than trusted or mangled: - * any `scheme:` without `//` other than `mailto:`/`tel:` (so `javascript:`, `data:`, `vbscript:`, - * `blob:`, …), and `file://` (local file access). Other network `scheme://` URLs (`http(s)`, `ftp`, …) - * pass through. A bare `host:port` (digits after the colon) is a domain, not a scheme, so it still gets - * the `https://` prefix. + * protocol-relative URLs intact. A scheme is kept only when {@link SAFE_SCHEME} matches; every other + * one is dropped to `''`, which callers render as inert text rather than a link. A bare `host:port` + * (digits after the colon) is a domain, not a scheme, so it still gets the `https://` prefix. */ export function normalizeLinkHref(href: string): string { const trimmed = href.trim() @@ -99,9 +102,7 @@ export function normalizeLinkHref(href: string): string { if (trimmed.startsWith('//')) return `https:${trimmed}` if (trimmed.startsWith('/')) return trimmed if (trimmed.startsWith('./') || trimmed.startsWith('../')) return trimmed - if (/^(?:mailto|tel):/i.test(trimmed)) return trimmed - const schemed = trimmed.match(SCHEME_URL) - if (schemed) return /^file$/i.test(schemed[1]) ? '' : trimmed + if (SAFE_SCHEME.test(trimmed)) return trimmed if (HAS_SCHEME.test(trimmed) && !HOST_PORT.test(trimmed)) return '' return `https://${trimmed}` } diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts new file mode 100644 index 00000000000..e66652f8e54 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.test.ts @@ -0,0 +1,46 @@ +import type { ChainedCommands } from '@tiptap/core' +import { describe, expect, it, vi } from 'vitest' +import { applyLink } from './link-editing' + +function chainSpy() { + const calls: string[] = [] + const chain = { + extendMarkRange: vi.fn(() => chain), + setLink: vi.fn(({ href }: { href: string }) => { + calls.push(`setLink:${href}`) + return chain + }), + unsetLink: vi.fn(() => { + calls.push('unsetLink') + return chain + }), + run: vi.fn(() => true), + } + return { chain: chain as unknown as ChainedCommands, calls } +} + +describe('applyLink', () => { + it('sets a link for a target that survives normalization', () => { + const { chain, calls } = chainSpy() + applyLink(chain, ' sim.ai ') + expect(calls).toEqual(['setLink:https://sim.ai']) + }) + + it('removes the link when the field is cleared', () => { + const { chain, calls } = chainSpy() + applyLink(chain, ' ') + expect(calls).toEqual(['unsetLink']) + }) + + /** + * The field is seeded with the raw href, so committing one untouched must not be read as "remove". + * Dropping an unsafe target is a refusal to link, not an instruction to delete what is already there. + */ + it('leaves the existing link untouched when the target normalizes away', () => { + for (const target of ['javascript://%0aalert(1)', 'customproto://host/path']) { + const { chain, calls } = chainSpy() + applyLink(chain, target) + expect(calls).toEqual([]) + } + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx index f294e88a950..8cbd0dc7b6d 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/menus/link-editing.tsx @@ -4,11 +4,17 @@ import { normalizeLinkHref } from '../markdown-fidelity' /** * Applies a link to the chain's current selection: normalizes `rawHref`, expands to the full link - * mark, and sets it — or removes the link when the href is empty/unsafe. The caller supplies a chain - * already focused with the target selection (the captured bubble-menu range / the hovered link range). + * mark, and sets it. Clearing the field removes the link; a target that survives normalization + * replaces it. A target that normalizes away is neither set nor removed — the editor seeds this field + * with the raw href, so committing an untouched one would otherwise delete a link the user only + * opened, and dropping an unsafe target is not the same instruction as "remove this link". The + * caller supplies a chain already focused with the target selection (the captured bubble-menu range / + * the hovered link range). */ export function applyLink(chain: ChainedCommands, rawHref: string): void { - const href = normalizeLinkHref(rawHref.trim()) + const trimmed = rawHref.trim() + const href = normalizeLinkHref(trimmed) + if (!href && trimmed) return chain.extendMarkRange('link') if (href) chain.setLink({ href }) else chain.unsetLink() diff --git a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts index c8745e5e861..961fd1d86df 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/files/components/file-viewer/rich-markdown-editor/round-trip.test.ts @@ -5,6 +5,7 @@ * be idempotent (a second pass changes nothing) so autosave never churns. Mirrors the exact * pipeline the editor uses: split frontmatter out, serialize the body, re-attach + clean up. */ +import type { JSONContent } from '@tiptap/core' import { Editor } from '@tiptap/core' import { afterEach, describe, expect, it } from 'vitest' import { createMarkdownContentExtensions } from './extensions' @@ -14,6 +15,7 @@ import { postProcessSerializedMarkdown, splitFrontmatter, } from './markdown-fidelity' +import { parseMarkdownToDoc } from './markdown-parse' let editor: Editor | null = null @@ -126,6 +128,66 @@ describe('markdown-fidelity utils', () => { expect(normalizeLinkHref('blob:https://x.com/uuid')).toBe('') expect(normalizeLinkHref('vbscript:msgbox(1)')).toBe('') expect(normalizeLinkHref('localhost:3000/path')).toBe('https://localhost:3000/path') + // Adding `//` doesn't make a scheme safe, and an unknown scheme is dropped rather than trusted — + // the allowlist is the whole rule. + expect(normalizeLinkHref('javascript://%0aalert(1)')).toBe('') + expect(normalizeLinkHref('customproto://host/path')).toBe('') + }) + + /** + * The property that matters, stated over the spellings a browser collapses before it resolves a + * scheme: whatever comes back must not be executable. Padding and interior tabs/newlines are the + * usual way a blocked scheme is smuggled past a matcher that only reads the literal text. + */ + it('never returns a target that resolves to an executable scheme', () => { + const tab = String.fromCharCode(9) + const lf = String.fromCharCode(10) + const nbsp = String.fromCharCode(160) + const inputs = [ + 'javascript://%0aalert(1)', + 'javascript:alert(1)', + 'JAVASCRIPT://x', + ' javascript:alert(1) ', + `${nbsp}javascript:alert(1)`, + `java${tab}script://alert(1)`, + `java${lf}script:alert(1)`, + 'data://text/html,