Merge pull request #42 from happyprime/task/report-hardening

Report hardening: escaping, config validation, modal dialog (MED-001/011 + 5 low)

Author: jeremyfelt

src/config.mjs

@@ -69,6 +69,55 @@ export function normalizeDomain(domain) {
 	return value;
 }
 
+// Path keys and viewport names become part of output filenames (the
+// `${key}-${viewport}` slug), so they must not contain path separators or
+// traversal sequences that could redirect a write outside the output dir.
+const SAFE_SLUG_PART = /^[a-z0-9][a-z0-9._-]*$/i;
+
+/**
+ * Assert a value is safe to use as part of an output filename.
+ *
+ * @param {string} value - The value to check.
+ * @param {string} kind  - A label for the error message (e.g. "path key").
+ */
+function assertSafeSlugPart(value, kind) {
+	if (
+		typeof value !== 'string' ||
+		!SAFE_SLUG_PART.test(value) ||
+		value.includes('..')
+	) {
+		throw new Error(
+			`❌ Invalid ${kind} ${JSON.stringify(value)}: it becomes part of an ` +
+				'output filename, so use only letters, numbers, dashes, dots, ' +
+				'and underscores (no slashes or "..").'
+		);
+	}
+}
+
+/**
+ * Validate a pixelmatch color option is an [r, g, b] triple of 0–255 integers.
+ *
+ * Without this, a non-array value (e.g. `null`) crashes report generation with
+ * a `.join` TypeError at the very end of a compare run, and a crafted array
+ * would be interpolated into the report's CSS.
+ *
+ * @param {*}      color - The configured color value.
+ * @param {string} name  - The option name, for the error message.
+ */
+function validateColor(color, name) {
+	const valid =
+		Array.isArray(color) &&
+		color.length === 3 &&
+		color.every((n) => Number.isInteger(n) && n >= 0 && n <= 255);
+
+	if (!valid) {
+		throw new Error(
+			`❌ Invalid "pixelmatchOptions.${name}": expected [r, g, b] integers 0–255, ` +
+				`got ${JSON.stringify(color)}.`
+		);
+	}
+}
+
 /**
  * Validate the viewports defined in a config, throwing on the first problem.
  *
@@ -105,6 +154,8 @@ export function validateViewports(viewports) {
 			);
 		}
 
+		assertSafeSlugPart(viewport.name, 'viewport name');
+
 		for (const dimension of ['width', 'height']) {
 			const value = viewport[dimension];
 			if (!Number.isInteger(value) || value <= 0) {
@@ -193,6 +244,10 @@ export function loadConfig({ configPath = 'reglance.json', domain } = {}) {
 		throw new Error('No "paths" configured in reglance.json.');
 	}
 
+	for (const key of Object.keys(raw.paths)) {
+		assertSafeSlugPart(key, 'path key');
+	}
+
 	// The domain is only required for capture; control and compare operate on
 	// already-captured files, so a missing domain is allowed here.
 	const resolvedDomain = domain ?? raw.domain;
@@ -210,16 +265,20 @@ export function loadConfig({ configPath = 'reglance.json', domain } = {}) {
 		url: origin ? buildUrl(origin, pathname) : pathname,
 	}));
 
+	const pixelmatchOptions = {
+		...DEFAULT_PIXELMATCH_OPTIONS,
+		...raw.pixelmatchOptions,
+	};
+	validateColor(pixelmatchOptions.diffColor, 'diffColor');
+	validateColor(pixelmatchOptions.diffColorAlt, 'diffColorAlt');
+
 	return {
 		name: raw.name || (origin ? new URL(origin).host : 'reglance'),
 		domain: origin,
 		outputDir,
 		viewports,
 		targets,
-		pixelmatchOptions: {
-			...DEFAULT_PIXELMATCH_OPTIONS,
-			...raw.pixelmatchOptions,
-		},
+		pixelmatchOptions,
 		timeouts: {
 			...DEFAULT_TIMEOUTS,
 			...raw.timeouts,

src/report.mjs

@@ -4,14 +4,25 @@ import { fileURLToPath } from 'node:url';
 
 const TEMPLATES_DIR = fileURLToPath(new URL('../templates', import.meta.url));
 
+const templateCache = new Map();
+
 /**
  * Read a template file that ships with the package.
  *
+ * Memoized: templates don't change during a run, and these are read once per
+ * comparison, so caching avoids hundreds of redundant blocking reads (and
+ * filesystem contention once compare is parallelized).
+ *
  * @param {string} name - The template filename.
  * @returns {string} The template contents.
  */
 function readTemplate(name) {
-	return fs.readFileSync(path.join(TEMPLATES_DIR, name), 'utf8');
+	let cached = templateCache.get(name);
+	if (cached === undefined) {
+		cached = fs.readFileSync(path.join(TEMPLATES_DIR, name), 'utf8');
+		templateCache.set(name, cached);
+	}
+	return cached;
 }
 
 /**
@@ -32,15 +43,31 @@ export function copyAssets(config) {
  * @param {string} str - The string to escape.
  * @returns {string} The escaped string.
  */
-function escapeHtml(str) {
-	return str
+export function escapeHtml(str) {
+	return String(str)
 		.replace(/&/g, '&')
 		.replace(/</g, '&lt;')
 		.replace(/>/g, '&gt;')
 		.replace(/"/g, '&quot;')
 		.replace(/'/g, '&#039;');
 }
 
+/**
+ * Serialize a value for embedding in an inline <script>.
+ *
+ * JSON.stringify does not neutralize `</script>` or the U+2028/U+2029 line
+ * separators, which can break out of the script element, so escape them.
+ *
+ * @param {*} value - The value to serialize.
+ * @returns {string} Script-safe JSON.
+ */
+function jsonForScript(value) {
+	return JSON.stringify(value)
+		.replace(/</g, '\\u003c')
+		.replace(/\u2028/g, '\\u2028')
+		.replace(/\u2029/g, '\\u2029');
+}
+
 /**
  * Build an HTML diff report comparing two HTML snapshots.
  *
@@ -76,9 +103,9 @@ export function generateHtmlDiff(html1, html2, context, diffLines) {
 
 	const template = readTemplate('html-diff.html');
 	const html = template
-		.replaceAll('{name}', name)
-		.replaceAll('{urlKey}', urlKey)
-		.replaceAll('{viewportName}', viewport.name)
+		.replaceAll('{name}', escapeHtml(name))
+		.replaceAll('{urlKey}', escapeHtml(urlKey))
+		.replaceAll('{viewportName}', escapeHtml(viewport.name))
 		.replaceAll('{viewportWidth}', String(viewport.width))
 		.replaceAll('{viewportHeight}', String(viewport.height))
 		.replaceAll('{status}', hasChanges ? 'Changes detected' : 'No changes')
@@ -110,11 +137,11 @@ export function generateReport(config, report) {
 	const overlayAlt = `Original (control) capture of ${where}`;
 
 	template = template
-		.replaceAll('{name}', name)
+		.replaceAll('{name}', escapeHtml(name))
 		.replaceAll('{baseAlt}', baseAlt)
 		.replaceAll('{overlayAlt}', overlayAlt)
-		.replaceAll('{urlKey}', urlKey)
-		.replaceAll('{viewportName}', viewport.name)
+		.replaceAll('{urlKey}', escapeHtml(urlKey))
+		.replaceAll('{viewportName}', escapeHtml(viewport.name))
 		.replaceAll('{viewportWidth}', String(viewport.width))
 		.replaceAll('{viewportHeight}', String(viewport.height))
 		.replaceAll('{originalImage}', rel(controlImage))
@@ -175,38 +202,40 @@ export function generateIndex(config, reports) {
 						? 'medium'
 						: 'low';
 			const htmlDiffClass = report.htmlHasChanges ? 'high' : 'low';
+			const url = escapeHtml(report.url);
+			const viewportName = escapeHtml(report.viewport.name);
 
 			return `
-			<tr data-url="${report.url}" data-viewport="${report.viewport.name}" data-diff="${report.diffPercentage}" data-index="${index}">
-				<td class="url-cell" title="${report.url}">${report.url}</td>
-				<td>${report.viewport.name} (${report.viewport.width}x${report.viewport.height})</td>
+			<tr data-url="${url}" data-viewport="${viewportName}" data-diff="${report.diffPercentage}" data-index="${index}">
+				<td class="url-cell" title="${url}">${url}</td>
+				<td>${viewportName} (${report.viewport.width}x${report.viewport.height})</td>
 				<td class="diff-percentage ${diffClass}"><span class="visually-hidden">${diffClass} difference: </span>${report.diffPercentage.toFixed(2)}%</td>
 				<td class="diff-percentage ${htmlDiffClass}">${report.htmlHasChanges ? 'Yes' : 'No'}</td>
 				<td><a href="${rel(report.reportPath)}">View Report</a></td>
-				<td><a href="#" onclick="openModal(window.diffData, ${index}); return false;">View Diff</a></td>
+				<td><button type="button" class="link-button" onclick="openModal(window.diffData, ${index}, this)">View Diff</button></td>
 				<td><a href="${rel(report.htmlDiffPath)}">View HTML Diff</a></td>
 			</tr>`;
 		})
 		.join('');
 
 	const viewportOptions = viewports
-		.map(
-			(v) =>
-				`<option value="${v.name}">${v.name} (${v.width}x${v.height})</option>`
-		)
+		.map((v) => {
+			const vn = escapeHtml(v.name);
+			return `<option value="${vn}">${vn} (${v.width}x${v.height})</option>`;
+		})
 		.join('\n\t\t\t\t');
 
 	const template = readTemplate('index.html');
 	const indexHtml = template
-		.replaceAll('{name}', name)
+		.replaceAll('{name}', escapeHtml(name))
 		.replaceAll('{threshold}', String(pixelmatchOptions.threshold))
 		.replaceAll('{includeAA}', pixelmatchOptions.includeAA ? 'Yes' : 'No')
 		.replaceAll('{alpha}', String(pixelmatchOptions.alpha))
 		.replaceAll('{diffColor}', pixelmatchOptions.diffColor.join(','))
 		.replaceAll('{viewportOptions}', viewportOptions)
 		.replaceAll('{rows}', rows)
 		.replaceAll('{diffViewer}', diffViewer)
-		.replaceAll('{diffData}', JSON.stringify(diffData));
+		.replaceAll('{diffData}', jsonForScript(diffData));
 
 	const indexPath = path.join(dirs.reports, 'index.html');
 	fs.writeFileSync(indexPath, indexHtml);

templates/assets/index-style.css

@@ -34,6 +34,26 @@ a:hover {
 	text-decoration: underline;
 }
 
+/* A button that performs an in-page action (opening the diff dialog) but
+   reads like the surrounding navigation links. */
+.link-button {
+	background: none;
+	border: 0;
+	padding: 0;
+	font: inherit;
+	color: #0066cc;
+	cursor: pointer;
+}
+
+.link-button:hover {
+	text-decoration: underline;
+}
+
+.link-button:focus-visible {
+	outline: 2px solid #0066cc;
+	outline-offset: 2px;
+}
+
 .diff-percentage {
 	font-weight: bold;
 }