fix: stop destroying anchors that carry data-* attributes

docs/ARCHITECTURE.md

@@ -145,8 +145,17 @@ When extending the renderer or its CSS, preserve these rules:
     per-element gate so a host's injected markup (e.g. its artifact `<img>`) survives
     sanitization. The core allowlist stays the security gate; keep additions narrow.
 
-  A host emitting attributes outside the escape/sink allowlists must also widen
-  `SAFE_OUTER_TAG_RE` (`escape.ts`) to match.
+  A host emitting attributes outside the sink allowlist widens it via
+  `sanitizeExtension`; that is the only allowlist a host has to touch. **`data-*`
+  needs no widening at all** — custom data attributes pass both gates
+  generically (`data-attributes.ts`), matching DOMPurify's `ALLOW_DATA_ATTR`
+  default so the two shipped backends stay interchangeable. A host on a page
+  running htmx/Alpine/Stimulus, where `data-*` is not inert, re-narrows with
+  `sanitizeExtension.onElement`. The pre-sink escape gate (`SAFE_OUTER_TAG_RE`,
+  `escape.ts`) is not configurable and matches decorator anchors by shape; an
+  anchor it does not recognise degrades to its allowlisted attributes
+  (`narrowAnchor`) rather than being escaped whole, which would leave the
+  matching `</a>` behind as a stray close tag.
 - **Valid block HTML.** Block elements (`<ul>`, `<ol>`, `<h3>`, `<h4>`, `<pre>`, `<table>`,
   `<hr>`) must never end up inside `<p>`. Mixed single-newline blocks (heading → subheading → list)
   are common in LLM output; split at block boundaries before wrapping paragraphs.

scripts/bundle-size-budget.json

@@ -33,7 +33,7 @@
     },
     "./sanitizers/browser": {
       "file": "dist/sanitize-browser.js",
-      "gzipBudget": 590
+      "gzipBudget": 640
     },
     "./diagrams/mermaid": {
       "file": "dist/mermaid-mermaidjs.js",

src/data-attributes.test.ts

@@ -0,0 +1,39 @@
+// `DATA_ATTR_NAME_SOURCE` is the sink walk's definition of a custom data
+// attribute; `escape.ts` spells the same shape as a regex *literal* so it
+// tree-shakes out of entries that want only `escapeHtml` (see the note there).
+// Two spellings can drift — two copies of an attribute allowlist drifting is
+// what #146 left behind — so pin them against each other behaviourally: the
+// escape gate must carry through exactly the names the sink calls `data-*`.
+import { describe, it } from 'node:test'
+import assert from 'node:assert/strict'
+import { renderMarkdownUnsafe } from './renderer.ts'
+import { DATA_ATTR_NAME_RE } from './data-attributes.ts'
+
+const CANDIDATES = [
+  'data-x',
+  'data-workspace-link',
+  'data-browser-link',
+  'data-footnote-ref',
+  'data-foo-bar-baz',
+  'data-a1',
+  'data-1',
+  'data-UPPER',
+  // Not custom data attributes, and not otherwise allowlisted on an anchor:
+  'data-',
+  'datax',
+  'data_x',
+  'datum-x',
+  'xdata-x',
+]
+
+describe('data-* shape parity between the escape gate and the sink', () => {
+  for (const name of CANDIDATES) {
+    it(`agrees about \`${name}\``, () => {
+      const html = renderMarkdownUnsafe(`<a href="https://example.com" ${name}="v">y</a>`, {
+        htmlPolicy: 'escape',
+      })
+      // The gate carried the attribute through iff the sink calls it `data-*`.
+      assert.equal(html.includes(`${name}="v"`), DATA_ATTR_NAME_RE.test(name), html)
+    })
+  }
+})

src/data-attributes.ts

@@ -0,0 +1,24 @@
+/**
+ * The one definition of a custom `data-*` attribute name, shared by the two
+ * gates that have to agree about them: the pre-sink escape gate
+ * (`SAFE_OUTER_TAG_RE` in `escape.ts`) and the sink allowlist walk
+ * (`enforceSanitizerAllowlist` in `sanitize-browser.ts`).
+ *
+ * They are shared rather than written twice because two copies of an attribute
+ * allowlist drifting apart is exactly what #146 left behind: it dropped the
+ * host-specific `data-browser-link` / `data-workspace-link` names from both
+ * gates but gave only the sink a replacement hook, so a host `linkDecorator`
+ * emitting them had its whole `<a …>` escaped to literal text.
+ *
+ * `data-*` is allowed generically rather than name-by-name because it carries no
+ * behavioural surface in HTML — no script, no URL, no navigation, no form
+ * control — which is why DOMPurify's own `ALLOW_DATA_ATTR` defaults to `true`.
+ * Matching that default keeps the two shipped sanitizer backends interchangeable
+ * and spares hosts (and future core markers) an allowlist edit per attribute.
+ * See the caveat on `SanitizeExtension` in `sanitize.ts` for the one environment
+ * where `data-*` is *not* inert.
+ */
+export const DATA_ATTR_NAME_SOURCE = 'data-[a-z0-9-]+'
+
+/** {@link DATA_ATTR_NAME_SOURCE} as a whole-name test. */
+export const DATA_ATTR_NAME_RE = /* @__PURE__ */ new RegExp(`^${DATA_ATTR_NAME_SOURCE}$`, 'i')

src/escape.ts

@@ -20,8 +20,24 @@ export function escapeHtml(text: string): string {
   return text.replace(/[&<>"']/g, (ch) => HTML_ESCAPES[ch] ?? ch)
 }
 
+/**
+ * Renderer-generated tags that survive the text-escaping pass, matched by SHAPE.
+ * The `<a>` arm admits any `data-*` attribute (value optional, as in the
+ * renderer's own valueless `data-footnote-ref`) rather than a list of names:
+ * #146 evicted the host-specific `data-browser-link` / `data-workspace-link`
+ * from both allowlists but gave only the sink a replacement hook, so a host
+ * `linkDecorator` emitting them had its whole `<a …>` escaped to literal text
+ * while the matching `</a>` (a separate arm) survived, leaving a stray close
+ * tag. `isSanctionedRendererTag` re-validates content either way, and anything
+ * this misses degrades through {@link narrowAnchor} instead of being destroyed.
+ */
+// Regex *literals*, not `new RegExp` over a shared source string: this module is
+// pulled in by entries that want only `escapeHtml` (the shiki highlighter, say),
+// and a literal tree-shakes out of those bundles where a constructed one does
+// not. `data-attributes.test.ts` pins these against `DATA_ATTR_NAME_SOURCE` so
+// the duplicated `data-*` shape still cannot drift.
 const SAFE_OUTER_TAG_RE =
-  /^(?:<a(?:\s+href="[^"]*")(?:\s+(?:title|target|rel|class)="[^"]*")*\s*>|<\/(?:a|code|em|strong)>|<(?:code|em|strong)\b[^>]*>|<img\b[^>]*\bdata-md-rendered="1"[^>]*\/?>)$/i
+  /^(?:<a(?:\s+href="[^"]*")(?:\s+(?:(?:title|target|rel|class)="[^"]*"|data-[a-z0-9-]+(?:="[^"]*")?))*\s*>|<\/(?:a|code|em|strong)>|<(?:code|em|strong)\b[^>]*>|<img\b[^>]*\bdata-md-rendered="1"[^>]*\/?>)$/i
 
 /**
  * Benign raw inline HTML models emit in prose (strikethrough, sub/superscript,
@@ -81,21 +97,79 @@ function isSanctionedRendererTag(tag: string): boolean {
  */
 const PASSTHROUGH_TAG_RE = /^<\/?[a-zA-Z][a-zA-Z0-9-]*(?:\s[^<>]*)?\/?>$/
 
-function keepRawTag(part: string, policy: HtmlPolicy): boolean {
-  if (policy === 'passthrough') return PASSTHROUGH_TAG_RE.test(part)
+/** Whole-name test for the attributes {@link narrowAnchor} keeps. */
+const SAFE_ANCHOR_ATTR_NAME_RE = /^(?:href|title|target|rel|class|data-[a-z0-9-]+)$/i
+
+/** One attribute inside an open tag: a name, optionally with a quoted value. */
+const TAG_ATTR_RE = /([a-zA-Z_:][-a-zA-Z0-9_:.]*)(?:\s*=\s*"([^"]*)")?/g
+
+/** An open `<a>` tag carrying at least one attribute, captured for narrowing. */
+const ANCHOR_OPEN_TAG_RE = /^<a\s+([^>]*?)\s*>$/i
+
+/** A double-quoted `href`, the shape every renderer/decorator anchor has. */
+const QUOTED_HREF_RE = /\bhref\s*=\s*"/i
+
+/**
+ * Fail-safe for an anchor {@link SAFE_OUTER_TAG_RE} does not recognise: re-emit
+ * it carrying only the allowlisted attributes instead of escaping it whole.
+ *
+ * The all-or-nothing test this backstops fails *badly*, not safely — `</a>` is a
+ * separate arm and survives on its own, so one unrecognised attribute turns a
+ * link into escaped source text followed by a stray unbalanced close tag (#146's
+ * `data-*` fallout was one instance; any attribute the core or a host adds later
+ * is the next). Degrading to a narrowed anchor keeps the markup well-formed and
+ * keeps the decision conservative: the tag is rebuilt from the allowlist, so an
+ * unknown attribute is dropped rather than passed on to the sink, and
+ * `isSanctionedRendererTag` still rejects event handlers and dangerous schemes.
+ *
+ * Requires a quoted `href` — the shape the renderer and every decorator emit,
+ * and the only one `isSanctionedRendererTag` can read a scheme out of. An
+ * unquoted `<a href=javascript:…>` therefore still escapes whole, as before.
+ */
+function narrowAnchor(tag: string): string | null {
+  const body = ANCHOR_OPEN_TAG_RE.exec(tag)?.[1]
+  if (body === undefined || !QUOTED_HREF_RE.test(body)) return null
+  if (!isSanctionedRendererTag(tag)) return null
+  const kept: string[] = []
+  let hasHref = false
+  for (const [, rawName = '', value] of body.matchAll(TAG_ATTR_RE)) {
+    const name = rawName.toLowerCase()
+    if (!SAFE_ANCHOR_ATTR_NAME_RE.test(name)) continue
+    if (name === 'href') {
+      // The guard above can be satisfied by a `href="` *inside* another
+      // attribute's value, so confirm a real one survived the scan: this
+      // salvages links, and an `<a>` with no href is not one.
+      if (value === undefined) continue
+      hasHref = true
+    }
+    // Values came out of a `<[^>]+>` split inside double quotes, so they carry
+    // no `<`, `>` or `"` and need no re-escaping to be re-emitted.
+    kept.push(value === undefined ? name : `${name}="${value}"`)
+  }
+  return hasHref ? `<a ${kept.join(' ')}>` : null
+}
+
+/** The HTML to emit for a raw tag, or `null` to escape it as literal text. */
+function safeRawTag(part: string, policy: HtmlPolicy): string | null {
+  if (policy === 'passthrough') return PASSTHROUGH_TAG_RE.test(part) ? part : null
   // Renderer-generated tags (re-validated for forged content) always survive —
-  // this escaper runs over the renderer's own output.
-  if (SAFE_OUTER_TAG_RE.test(part) && isSanctionedRendererTag(part)) return true
+  // this escaper runs over the renderer's own output. Verbatim, so output for
+  // everything this arm already matched stays byte-identical.
+  if (SAFE_OUTER_TAG_RE.test(part) && isSanctionedRendererTag(part)) return part
+  // Only then: salvage an anchor the shape test missed rather than mangle it.
+  const narrowed = narrowAnchor(part)
+  if (narrowed !== null) return narrowed
   // Escape policy keeps the benign attribute-less inline allowlist;
   // escape-all literalizes everything but the void <br>.
-  return policy === 'escape-all' ? BR_TAG_RE.test(part) : BENIGN_RAW_INLINE_TAG_RE.test(part)
+  const keep = policy === 'escape-all' ? BR_TAG_RE.test(part) : BENIGN_RAW_INLINE_TAG_RE.test(part)
+  return keep ? part : null
 }
 
 function escapeHtmlOutsideSafeTags(html: string): string {
   const policy = getHtmlPolicy()
   return html
     .split(/(<[^>]+>)/g)
-    .map((part) => (part.startsWith('<') && keepRawTag(part, policy) ? part : escapeHtml(part)))
+    .map((part) => (part.startsWith('<') ? (safeRawTag(part, policy) ?? escapeHtml(part)) : escapeHtml(part)))
     .join('')
 }
 

src/raw-html-escape.test.ts

@@ -69,4 +69,104 @@ describe('raw-HTML escaping boundary (htmlPolicy: escape)', () => {
   it('preserves benign attribute-less inline HTML', () => {
     assert.match(escaped('<sub>2</sub> and <kbd>Esc</kbd>'), /<sub>2<\/sub> and <kbd>Esc<\/kbd>/)
   })
+
+  // #146 dropped the host-specific `data-*` link attributes from the core
+  // allowlists, but only the sink got a replacement hook — this gate was left
+  // with none, so a host `linkDecorator` emitting them had its `<a …>` open tag
+  // escaped to literal text while the matching `</a>` survived. The gate now
+  // matches the generic `data-*` shape.
+  describe('anchors carrying decorator data-* attributes', () => {
+    const decorated = (attrs: string, policy: 'escape' | 'escape-all' = 'escape') =>
+      renderMarkdownUnsafe('[the docs](https://example.com/page)', {
+        htmlPolicy: policy,
+        linkDecorator: () => attrs,
+      })
+
+    it('preserves a host decorator\u2019s browser-link anchor', () => {
+      const html = decorated(' target="_blank" rel="noopener noreferrer" data-browser-link="true"')
+      assert.match(html, /<a href="https:\/\/example\.com\/page" target="_blank" rel="noopener noreferrer" data-browser-link="true">/)
+      assert.doesNotMatch(html, /&lt;a /)
+    })
+
+    it('preserves a host decorator\u2019s workspace-link anchor', () => {
+      const html = decorated(' class="workspace-markdown-link" data-workspace-link="true"')
+      assert.match(html, /<a href="[^"]*" class="workspace-markdown-link" data-workspace-link="true">/)
+      assert.doesNotMatch(html, /&lt;a /)
+    })
+
+    it('preserves them under escape-all too', () => {
+      const html = decorated(' data-browser-link="true"', 'escape-all')
+      assert.match(html, /<a href="[^"]*" data-browser-link="true">/)
+    })
+
+    it('leaves no stray unbalanced </a> behind', () => {
+      const html = decorated(' data-browser-link="true"')
+      assert.equal((html.match(/<a /g) ?? []).length, (html.match(/<\/a>/g) ?? []).length)
+    })
+
+    it('accepts a valueless data attribute and interleaved ordering', () => {
+      assert.match(decorated(' data-x class="c" data-y="1" title="t"'), /<a href="[^"]*" data-x class="c" data-y="1" title="t">/)
+    })
+
+    it('still escapes a raw anchor whose data-* rides alongside an event handler', () => {
+      const html = escaped('<a href="https://example.com" data-browser-link="true" onclick="alert(1)">x</a>')
+      assert.doesNotMatch(html, /<a[^>]*onclick/i)
+      assert.match(html, /&lt;a /i)
+    })
+
+    it('still escapes a raw javascript: anchor wearing a data-* attribute', () => {
+      const html = escaped('<a href="javascript:alert(1)" data-workspace-link="true">x</a>')
+      assert.doesNotMatch(html, /<a href="javascript:/i)
+      assert.match(html, /&lt;a /i)
+    })
+
+    it('drops a non-data unknown attribute but keeps the anchor', () => {
+      assert.match(escaped('<a href="https://example.com" datax="1">x</a>'), /<a href="https:\/\/example\.com">/)
+    })
+  })
+
+  // The shape test above is all-or-nothing and fails badly, not safely: `</a>`
+  // is a separate arm and survives on its own, so one unrecognised attribute
+  // used to yield escaped source text plus a stray unbalanced close tag.
+  describe('unrecognised anchor attributes degrade instead of mangling', () => {
+    it('drops the unknown attribute and keeps a well-formed anchor', () => {
+      const html = escaped('<a href="https://example.com" style="position:fixed" datax="1">x</a>')
+      assert.match(html, /<a href="https:\/\/example\.com">x<\/a>/)
+      assert.doesNotMatch(html, /style=/i)
+    })
+
+    it('leaves no unbalanced </a> for an unrecognised attribute', () => {
+      const html = escaped('<a href="https://example.com" style="x">y</a>')
+      assert.equal((html.match(/<a /g) ?? []).length, (html.match(/<\/a>/g) ?? []).length)
+    })
+
+    it('keeps the allowlisted attributes while dropping the rest', () => {
+      const html = escaped('<a href="https://example.com" style="x" class="c" data-k="v" title="t">y</a>')
+      assert.match(html, /<a href="https:\/\/example\.com" class="c" data-k="v" title="t">/)
+    })
+
+    it('still refuses a dangerous scheme rather than narrowing to it', () => {
+      const html = escaped('<a href="javascript:alert(1)" style="x">y</a>')
+      assert.doesNotMatch(html, /<a href="javascript:/i)
+      assert.match(html, /&lt;a /i)
+    })
+
+    it('still refuses an event handler rather than dropping just that attribute', () => {
+      const html = escaped('<a href="https://example.com" onclick="alert(1)">y</a>')
+      assert.doesNotMatch(html, /<a /i)
+      assert.match(html, /&lt;a /i)
+    })
+
+    it('leaves an unquoted href escaped whole (no scheme to validate)', () => {
+      assert.match(escaped('<a href=javascript:alert(1) style="x">y</a>'), /&lt;a /i)
+    })
+
+    it('does not turn a non-anchor unknown tag into markup', () => {
+      assert.match(escaped('<div class="x">y</div>'), /&lt;div /i)
+    })
+
+    it('does not mint an href-less anchor from a href-shaped attribute value', () => {
+      assert.match(escaped('<a data-x="href=">y</a>'), /&lt;a /i)
+    })
+  })
 })

src/sanitize-backend.test.ts

@@ -91,12 +91,21 @@ describe('enforceSanitizerAllowlist (native backend narrowing)', () => {
     assert.equal(root.innerHTML, '<p>keep</p>bare text')
   })
 
-  it('strips attributes outside the allowlist', () => {
-    const root = parse('<a href="/x" class="c" onclick="evil()" data-x="1">link</a>')
+  it('strips attributes outside the allowlist, event handlers included', () => {
+    const root = parse('<a href="/x" class="c" onclick="evil()" style="position:fixed">link</a>')
     enforceSanitizerAllowlist(root, config)
     assert.equal(root.innerHTML, '<a href="/x" class="c">link</a>')
   })
 
+  // `data-*` is the deliberate exception: it passes generically, matching
+  // DOMPurify's `ALLOW_DATA_ATTR` default so the two backends agree
+  // (sanitize-data-attributes.test.ts pins that parity).
+  it('keeps data-* without an allowlist entry', () => {
+    const root = parse('<a href="/x" data-x="1" data-footnote-ref>link</a>')
+    enforceSanitizerAllowlist(root, config)
+    assert.equal(root.innerHTML, '<a href="/x" data-x="1" data-footnote-ref="">link</a>')
+  })
+
   it('drops content of dangerous containers rather than unwrapping them', () => {
     const root = parse('<p>ok</p><style>.x{color:red}</style>')
     enforceSanitizerAllowlist(root, config)

src/sanitize-browser.ts

@@ -1,3 +1,4 @@
+import { DATA_ATTR_NAME_RE } from './data-attributes.ts'
 import type { SanitizerBackend, SanitizerConfig } from './sanitize.ts'
 
 // Zero-dependency sanitizer backend built on the native Sanitizer API
@@ -6,7 +7,8 @@ import type { SanitizerBackend, SanitizerConfig } from './sanitize.ts'
 // scripts, event-handler attributes, and unsafe URLs); a strict allowlist walk
 // then narrows the result to exactly the tags/attributes the renderer produces
 // and runs the core/host per-element gate — identical posture to the DOMPurify
-// backend.
+// backend, `data-*` included (that walk once diverged by stripping every data
+// attribute DOMPurify's `ALLOW_DATA_ATTR` default kept).
 
 // `setHTML` is a recent addition and may be missing from the ambient DOM lib.
 // The options arg carries a Sanitizer config; `elements`/`attributes` are
@@ -65,7 +67,12 @@ export function enforceSanitizerAllowlist(root: ParentNode, config: SanitizerCon
       continue
     }
     for (const attr of Array.from(el.attributes)) {
-      if (!allowedAttr.has(attr.name.toLowerCase())) el.removeAttribute(attr.name)
+      // `data-*` passes generically, matching DOMPurify's `ALLOW_DATA_ATTR`
+      // default so the two shipped backends stay interchangeable — without it
+      // this walk silently stripped every host/renderer data attribute the
+      // DOMPurify backend kept (see DATA_ATTR_NAME_SOURCE for why generically).
+      const name = attr.name.toLowerCase()
+      if (!allowedAttr.has(name) && !DATA_ATTR_NAME_RE.test(name)) el.removeAttribute(attr.name)
     }
     config.onElement?.(el, tag)
   }