Sanitize extractor HTML (#326)

* Sanitize site-extractor HTML output (GHSA-jg4p-g6xj-4qmf)

Site extractors build their output from template-literal strings,
interpolating attacker-controlled DOM attribute values (image alt/src,
og:image, video description) without escaping. That output is returned
through buildExtractorResponse() which previously only resolved URLs,
bypassing the DOM-based sanitization the main pipeline applies — so an
injected event handler or javascript: URL could reach consumers that
render contentHtml.

Fix, defense in depth:
- Route all extractor output through _sanitizeExtractorHtml(), which
  parses into a DOM and runs the same _stripUnsafeElements() pass used
  elsewhere (and resolves relative URLs in the same pass). This covers
  every extractor, present and future.
- escapeHtml() the interpolated values at the confirmed sinks in
  x-article, substack, and youtube extractors. This is complementary,
  not redundant: it also blocks injection of attributes the central
  pass does not strip (style/class/etc.) and preserves data integrity.

Add a regression test that re-parses extractor output and asserts no
event-handler attributes or javascript: URLs survive.

* Update CLAUDE.md

Author: kepano

CLAUDE.md

@@ -95,6 +95,7 @@ Debug mode preserves class/id/data-* attributes and skips div flattening. Use `c
 
 - **Never use `innerHTML` directly.** Always use `parseHTML()` from `src/utils/dom.ts` which parses via `<template>` elements (no script execution, no resource loading).
 - **Sanitize URLs** — `javascript:` and `data:text/html` must be stripped from `href`/`src` attributes. `srcdoc` must be stripped from iframes. `on*` event handler attributes must be removed. See `sanitizeContent()` in `src/defuddle.ts`.
+- **Never interpolate page-derived values into HTML strings unescaped.** Extractors that build HTML from template literals (e.g. `` `<img src="${src}" alt="${alt}">` ``) must wrap every interpolated value in `escapeHtml()` from `src/utils/dom.ts`, or build the node via `createElement`/`setAttribute` + `serializeHTML()`. An unescaped `"` lets attacker-controlled values (image `alt`/`src`, `og:image`, descriptions) inject new attributes (XSS, e.g. GHSA-jg4p-g6xj-4qmf). Downstream sanitization is a backstop, not a substitute — escape at the point of interpolation. Cover new extractors with a poisoned-attribute case in `tests/extractor-xss.test.ts`.
 
 ### Common pitfalls
 - **UMD exports**: `export: 'default'` in webpack means named exports must be static properties on the default class (see `src/index.full.ts`).

src/defuddle.ts

@@ -1678,7 +1678,7 @@ export class Defuddle {
 		extractor: BaseExtractor,
 		pageMetaTags: MetaTagItem[]
 	): DefuddleResponse {
-		const contentHtml = this.resolveContentUrls(extracted.contentHtml);
+		const contentHtml = this._sanitizeExtractorHtml(extracted.contentHtml);
 		const variables = this.getExtractorVariables(extracted.variables);
 		return {
 			content: contentHtml,
@@ -1692,14 +1692,37 @@ export class Defuddle {
 			author: extracted.variables?.author || metadata.author,
 			site: extracted.variables?.site || metadata.site,
 			schemaOrgData: metadata.schemaOrgData,
-			wordCount: this.countHtmlWords(extracted.contentHtml),
+			wordCount: this.countHtmlWords(contentHtml),
 			parseTime: Math.round(Date.now() - startTime),
 			extractorType: extractor.constructor.name.replace('Extractor', '').toLowerCase(),
 			metaTags: pageMetaTags,
 			...(variables ? { variables } : {}),
 		};
 	}
 
+	/**
+	 * Sanitize and finalize HTML produced by site extractors.
+	 *
+	 * Extractors build their output from template-literal strings, so unlike the
+	 * main pipeline their output never passes through the DOM-based attribute
+	 * sanitizer. Attacker-controlled attribute values (e.g. an image `alt` or
+	 * `src` read straight off the page) could otherwise close an attribute and
+	 * inject an event handler or a `javascript:` URL. Parsing the output into a
+	 * DOM and running the same `_stripUnsafeElements` pass used elsewhere
+	 * neutralizes any such injection regardless of which extractor produced it.
+	 *
+	 * Relative-URL resolution runs on the same parsed DOM so extractor output is
+	 * parsed and serialized only once (resolveRelativeUrls no-ops without a URL).
+	 */
+	private _sanitizeExtractorHtml(html: string): string {
+		if (!html) return html;
+		const container = this.doc.createElement('div');
+		container.appendChild(parseHTML(this.doc, html));
+		this._stripUnsafeElements(container);
+		this.resolveRelativeUrls(container);
+		return serializeHTML(container);
+	}
+
 	/**
 	 * Filter extractor variables to only include custom ones
 	 * (exclude standard fields that are already mapped to top-level properties)

src/extractors/substack.ts

@@ -1,6 +1,6 @@
 import { BaseExtractor } from './_base';
 import { ExtractorResult } from '../types/extractors';
-import { parseHTML } from '../utils/dom';
+import { parseHTML, escapeHtml } from '../utils/dom';
 
 const INJECTED_ATTR = 'data-defuddle-substack-post';
 
@@ -178,12 +178,12 @@ export class SubstackExtractor extends BaseExtractor {
 		if (!this.noteImage) return '';
 
 		const ogImage = this.document.querySelector('meta[property="og:image"]')?.getAttribute('content');
-		if (ogImage) return `<img src="${ogImage}" alt="" />`;
+		if (ogImage) return `<img src="${escapeHtml(ogImage)}" alt="" />`;
 
 		const img = this.noteImage.querySelector('img');
 		if (!img) return '';
 		const src = this.getLargestSrc(img);
-		return src ? `<img src="${src}" alt="" />` : '';
+		return src ? `<img src="${escapeHtml(src)}" alt="" />` : '';
 	}
 
 	private getLargestSrc(img: Element): string {

src/extractors/x-article.ts

@@ -1,6 +1,6 @@
 import { BaseExtractor } from './_base';
 import { ExtractorResult } from '../types/extractors';
-import { serializeHTML } from '../utils/dom';
+import { serializeHTML, escapeHtml } from '../utils/dom';
 
 const SELECTORS = {
 	ARTICLE_READ_VIEW: '[data-testid="twitterArticleReadView"]',
@@ -111,7 +111,7 @@ export class XArticleExtractor extends BaseExtractor {
 
 		const alt = headerPhoto.getAttribute('alt')?.replace(/\s+/g, ' ').trim() || 'Image';
 
-		return `<img src="${this.upgradeImageSrc(src)}" alt="${alt}">`;
+		return `<img src="${escapeHtml(this.upgradeImageSrc(src))}" alt="${escapeHtml(alt)}">`;
 	}
 
 	private cleanContent(container: HTMLElement): void {

src/extractors/youtube.ts

@@ -366,7 +366,7 @@ export class YoutubeExtractor extends BaseExtractor {
 	}
 
 	private formatDescription(description: string): string {
-		return `<p>${description.replace(/\n/g, '<br>')}</p>`;
+		return `<p>${escapeHtml(description).replace(/\n/g, '<br>')}</p>`;
 	}
 
 	private getVideoData(): any {

tests/extractor-xss.test.ts

@@ -0,0 +1,63 @@
+import { describe, test, expect } from 'vitest';
+import { Defuddle } from '../src/node';
+import { parseLinkedomHTML } from '../src/utils/linkedom-compat';
+
+// Regression test for GHSA-jg4p-g6xj-4qmf: XSS via unescaped attribute
+// interpolation in site extractors. Extractor output is built from template
+// strings and previously bypassed DOM-based sanitization, so attacker-
+// controlled attribute values (e.g. an image alt read off the page) could
+// close the attribute and inject an event handler or a javascript: URL.
+
+const X_URL = 'https://x.com/testuser/article/123456789';
+
+// Header image lives in the read view but OUTSIDE the article container, so
+// extractHeaderImage() emits it via a template string.
+function makeXArticleHTML(headerImgAttrs: string): string {
+	return `
+		<html><head><title>Test Article</title></head>
+		<body>
+			<div data-testid="twitterArticleReadView">
+				<div data-testid="tweetPhoto"><img ${headerImgAttrs}></div>
+				<div data-testid="twitterArticleRichTextView">
+					<h1 data-testid="twitter-article-title">Test Article</h1>
+					<div class="public-DraftStyleDefault-block">Body text</div>
+				</div>
+			</div>
+		</body></html>
+	`;
+}
+
+// Re-parse the extractor output and assert no element carries an executable
+// attribute. This is the true security property: an escaped "onerror" living
+// inside an alt value is harmless; a real onerror attribute is not.
+function assertNoExecutableAttributes(html: string) {
+	const doc = parseLinkedomHTML(`<body>${html}</body>`, X_URL);
+	for (const el of Array.from(doc.querySelectorAll('*'))) {
+		for (const attr of Array.from((el as Element).attributes)) {
+			expect(attr.name.toLowerCase().startsWith('on')).toBe(false);
+			if (['src', 'href'].includes(attr.name.toLowerCase())) {
+				expect(attr.value.toLowerCase().replace(/\s+/g, '')).not.toContain('javascript:');
+			}
+		}
+	}
+}
+
+describe('Extractor output XSS sanitization (GHSA-jg4p-g6xj-4qmf)', () => {
+	test('does not emit an event handler attribute from X header image alt', async () => {
+		// alt contains a double-quote that, unescaped, would close the attribute
+		// and turn the rest into a real onerror handler.
+		const html = makeXArticleHTML('src="https://example.com/img.jpg" alt=\'x" onerror="alert(1)\'');
+		const doc = parseLinkedomHTML(html, X_URL);
+		const response = await Defuddle(doc, X_URL);
+
+		assertNoExecutableAttributes(response.content);
+	});
+
+	test('strips javascript: URL injected via X header image src', async () => {
+		const html = makeXArticleHTML('src="javascript:alert(1)" alt="ok"');
+		const doc = parseLinkedomHTML(html, X_URL);
+		const response = await Defuddle(doc, X_URL);
+
+		assertNoExecutableAttributes(response.content);
+	});
+});