Merge pull request #23 from HarperFast/fix/sitemap-fetch-guard-staging

feat(plugin): guard sitemap fetch, route via staging edge, identifiable UAs; v0.5.0

Author: harper-joseph

package-lock.json

@@ -1,6 +1,6 @@
 {
 	"name": "@harperfast/prerender",
-	"version": "0.4.0",
+	"version": "0.5.0",
 	"type": "module",
 	"description": "Configurable Harper plugin for prerendering pages for bots and crawlers",
 	"license": "Apache-2.0",

packages/plugin/package.json

@@ -119,6 +119,11 @@ const defaultConfig = () => ({
 	// The cache key does not include the header, so cache HITS always return the normal
 	// cached page regardless of it. Empty `ip` disables the feature — production is
 	// unaffected unless a staging IP is explicitly configured.
+	//
+	// The sitemap refresh reuses this `ip` too, but unconditionally (no toggle header — it has
+	// no incoming request): whenever `ip` is set, every sitemap fetch is pinned to it, so all
+	// Harper→origin traffic hits the same edge. The security token often only authenticates
+	// against the staging edge, so a direct prod sitemap fetch is bounced with a 403.
 	staging: {
 		ip: '',
 		header: 'x-harper-staging',
@@ -189,16 +194,25 @@ const defaultConfig = () => ({
 		maxClaimLimit: 25,
 	},
 
-	// Per-device-type User-Agent strings sent to the origin.
+	// Per-device-type User-Agent strings sent to the origin on the proxy (cache-miss
+	// passthrough) fetch. Each carries a `HarperProxy/1.0` product token so Harper's
+	// proxy traffic is identifiable in origin/CDN logs while still presenting a real,
+	// device-appropriate browser UA (the origin serves device-specific HTML off it).
 	userAgents: {
 		mobile:
-			'Mozilla/5.0 (Linux; Android 11; Pixel 5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/W.X.Y.Z Mobile Safari/537.36 HarperPrerender/1.0',
+			'Mozilla/5.0 (Linux; Android 11; Pixel 5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/W.X.Y.Z Mobile Safari/537.36 HarperProxy/1.0',
 		tablet:
-			'Mozilla/5.0 (Linux; Android 7.0; Pixel C Build/NRD90M; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/52.0.2743.98 Safari/537.36 HarperPrerender/1.0',
+			'Mozilla/5.0 (Linux; Android 7.0; Pixel C Build/NRD90M; wv) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/52.0.2743.98 Safari/537.36 HarperProxy/1.0',
 		desktop:
-			'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/W.X.Y.Z Safari/537.36 HarperPrerender/1.0',
+			'Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) HeadlessChrome/W.X.Y.Z Safari/537.36 HarperProxy/1.0',
 	},
 
+	// User-Agent for Harper's sitemap crawler fetch (Sitemap.refresh). Unlike the proxy
+	// fetch above, a sitemap fetch isn't a device render, so it sends a single self-identifying
+	// UA rather than a spoofed browser one — makes Harper's sitemap traffic obvious in
+	// origin/CDN logs and separable from the proxy traffic.
+	sitemapUserAgent: 'HarperSitemapCrawler/1.0',
+
 	// Pages whose normalized URL contains any of these substrings are never
 	// auto-scheduled for rendering.
 	excludePathPatterns: ['/search/'],

packages/plugin/src/config.js

@@ -1,8 +1,9 @@
-import { XMLParser } from 'fast-xml-parser';
 import { config } from '../config.js';
 import { RenderTarget } from './RenderTarget.js';
 import { CacheKey } from '../util/cacheKey.js';
 import { currentMinuteMs, getNextSitemapRefreshTime } from '../util/time.js';
+import { parseSitemap } from '../util/sitemap.js';
+import { configuredStagingIp, dispatcherFor } from '../util/upstream.js';
 
 class Sitemap extends databases.sitemaps.Sitemap {
 	static directURLMapping = true;
@@ -202,32 +203,56 @@ function getTtlFromChangeFreq(changefreq, { minTtl, defaultTtl }) {
 }
 
 async function fetchLatestSitemap(url) {
+	// Route every Harper→origin sitemap fetch through the same edge as the render/origin-fetch
+	// path: whenever a staging IP is configured, pin the TCP connection to it (Host/SNI stay the
+	// real origin, exactly like upstream.js). The security token typically only authenticates
+	// against the staging edge, so a direct prod fetch is bounced with a 403 "Access Denied".
+	// Empty staging.ip → normal direct fetch (production, once the token is valid at the origin).
+	const stagingIp = configuredStagingIp();
+	const via = stagingIp ? ` (via staging ${stagingIp})` : '';
+
 	const res = await fetch(url, {
 		method: 'GET',
 		redirect: 'follow',
-		headers: { 'User-Agent': 'harper-bot/1.0', [config.securityToken.header]: config.securityToken.value },
+		headers: { 'User-Agent': config.sitemapUserAgent, [config.securityToken.header]: config.securityToken.value },
+		dispatcher: dispatcherFor(stagingIp),
 	});
 	const xml = await res.text();
 
-	const parser = new XMLParser({
-		isArray: (tagName) => ['sitemap', 'url'].some((value) => value === tagName),
-	});
-
-	const data = parser.parse(xml);
-
-	const parsed = { url, lastRefreshed: new Date(), entries: [], entryCount: 0 };
+	// A blocked/errored fetch returns an HTML error page with a 4xx/5xx status. Guard the
+	// status AND the parsed shape so it fails loudly instead of being silently treated as an
+	// empty sitemap (which used to return a misleading `created: 0` success).
+	if (!res.ok) {
+		throw new Error(`Sitemap fetch failed for ${url}${via}: ${res.status} ${res.statusText} — ${snippet(xml)}`);
+	}
 
-	if (Array.isArray(data?.urlset?.url)) {
-		parsed.isIndex = false;
-		parsed.entries = data.urlset.url;
-	} else if (Array.isArray(data?.sitemapindex?.sitemap)) {
-		parsed.isIndex = true;
-		parsed.entries = data.sitemapindex.sitemap;
+	let parsed;
+	try {
+		parsed = parseSitemap(xml);
+	} catch (e) {
+		const contentType = res.headers.get('content-type') ?? 'unknown';
+		throw new Error(
+			`Sitemap fetch for ${url}${via} returned a non-sitemap response (status ${res.status}, content-type ${contentType}): ${e.message} — ${snippet(xml)}`
+		);
 	}
 
-	parsed.entryCount = parsed.entries.length;
+	return {
+		url,
+		lastRefreshed: new Date(),
+		isIndex: parsed.isIndex,
+		entries: parsed.entries,
+		entryCount: parsed.entries.length,
+	};
+}
 
-	return parsed;
+// A short, single-line excerpt of a response body for error messages. Slice before the
+// whitespace-collapse so a large body (a full sitemap can be >1 MB) doesn't run the regex
+// over the whole string.
+function snippet(body, max = 200) {
+	const raw = String(body ?? '');
+	const truncated = raw.length > max * 2 ? raw.slice(0, max * 2) : raw;
+	const text = truncated.replace(/\s+/g, ' ').trim();
+	return text.length > max ? `${text.slice(0, max)}…` : text;
 }
 
 let sitemapSchedulerStarted = false;

packages/plugin/src/resources/Sitemap.js

@@ -0,0 +1,38 @@
+import { XMLParser } from 'fast-xml-parser';
+
+const parser = new XMLParser({
+	isArray: (tagName) => tagName === 'sitemap' || tagName === 'url',
+});
+
+/**
+ * Parse sitemap XML into `{ isIndex, entries }`.
+ *
+ * Throws if the document is neither a `<urlset>` nor a `<sitemapindex>` — e.g. an HTML
+ * error/challenge page (a 403 "Access Denied" from the CDN, a login wall, a 404 page). The
+ * old code silently treated any such body as an empty sitemap, so a blocked fetch looked
+ * like a successful no-op refresh (`created: 0, updated: 0, …`). Failing loudly here surfaces
+ * the real problem to the caller.
+ *
+ * A valid but empty `<urlset/>` / `<sitemapindex/>` parses to `entries: []` WITHOUT throwing:
+ * fast-xml-parser renders an empty element as `''`, so presence is checked with `in`, not
+ * truthiness (`data.urlset` is falsy for an empty sitemap). The `typeof data === 'object'`
+ * guard keeps `in` off a non-object result — fast-xml-parser v5 always returns an object
+ * (plain text parses to `{}`), but this stays safe if that ever changes.
+ */
+export function parseSitemap(xml) {
+	const data = parser.parse(xml);
+
+	if (data && typeof data === 'object') {
+		if ('urlset' in data) {
+			return { isIndex: false, entries: Array.isArray(data.urlset?.url) ? data.urlset.url : [] };
+		}
+		if ('sitemapindex' in data) {
+			return { isIndex: true, entries: Array.isArray(data.sitemapindex?.sitemap) ? data.sitemapindex.sitemap : [] };
+		}
+	}
+
+	const rootTags = data && typeof data === 'object' ? Object.keys(data).filter((key) => key !== '?xml') : [];
+	throw new Error(
+		`expected a <urlset> or <sitemapindex> root, got ${rootTags.length ? `<${rootTags.join('>, <')}>` : 'a non-XML or empty document'}`
+	);
+}

packages/plugin/src/util/sitemap.js

@@ -18,14 +18,24 @@ export const stagingTargetIp = (headers) => {
 	return headers?.get(header) ? ip : undefined;
 };
 
+/**
+ * The configured staging IP if it is set and valid, else undefined — regardless of any
+ * request header. For callers that opt into staging out-of-band rather than via a per-request
+ * toggle header (e.g. the sitemap refresh, which has no incoming request to carry a header).
+ */
+export const configuredStagingIp = () => {
+	const { ip } = config.staging;
+	return ip && isIP(ip) ? ip : undefined;
+};
+
 // Dispatchers that pin DNS resolution to a fixed IP (staging passthrough), one per IP.
 // Only the connect address is overridden — the origin (so Host header + TLS SNI + cert
 // validation) stays the real origin host, the server-side equivalent of Chrome's
 // --host-resolver-rules=MAP host ip. In practice there is at most one entry (the single
 // configured staging IP); the map just keeps it stable across requests and across a
 // config reload that changes the IP.
 const pinnedDispatchers = new Map();
-const dispatcherFor = (ip) => {
+export const dispatcherFor = (ip) => {
 	if (!ip) return agent;
 	let dispatcher = pinnedDispatchers.get(ip);
 	if (!dispatcher) {

packages/plugin/src/util/upstream.js

@@ -31,6 +31,20 @@ test('applyOptions rejects type-mismatched values and keeps defaults', () => {
 	assert.equal(config.page.ttl, 24 * 60 * 60 * 1000);
 });
 
+test('sitemapUserAgent defaults to the Harper sitemap crawler UA and is overridable', () => {
+	applyOptions({});
+	assert.equal(config.sitemapUserAgent, 'HarperSitemapCrawler/1.0');
+	applyOptions({ sitemapUserAgent: 'AcmeBot/2.0' });
+	assert.equal(config.sitemapUserAgent, 'AcmeBot/2.0');
+});
+
+test('proxy device UAs carry the HarperProxy product token', () => {
+	applyOptions({});
+	for (const ua of Object.values(config.userAgents)) {
+		assert.match(ua, /HarperProxy\/1\.0$/);
+	}
+});
+
 test('applyOptions exposes analytics + url defaults', () => {
 	applyOptions({});
 	assert.equal(config.analytics.enabled, true);

packages/plugin/test/config.test.js

@@ -0,0 +1,62 @@
+import { test } from 'node:test';
+import assert from 'node:assert/strict';
+import { parseSitemap } from '../src/util/sitemap.js';
+
+const xmlDecl = '<?xml version="1.0" encoding="UTF-8"?>';
+
+test('parses a <urlset> with multiple <url> entries', () => {
+	const { isIndex, entries } = parseSitemap(
+		`${xmlDecl}<urlset xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">` +
+			`<url><loc>https://x/a</loc></url><url><loc>https://x/b</loc></url></urlset>`
+	);
+	assert.equal(isIndex, false);
+	assert.equal(entries.length, 2);
+	assert.equal(entries[0].loc, 'https://x/a');
+});
+
+test('normalizes a single <url> to a one-element array', () => {
+	const { isIndex, entries } = parseSitemap(`${xmlDecl}<urlset><url><loc>https://x/a</loc></url></urlset>`);
+	assert.equal(isIndex, false);
+	assert.equal(entries.length, 1);
+	assert.equal(entries[0].loc, 'https://x/a');
+});
+
+test('parses a <sitemapindex> as an index', () => {
+	const { isIndex, entries } = parseSitemap(
+		`${xmlDecl}<sitemapindex><sitemap><loc>https://x/s1.xml</loc></sitemap></sitemapindex>`
+	);
+	assert.equal(isIndex, true);
+	assert.equal(entries.length, 1);
+	assert.equal(entries[0].loc, 'https://x/s1.xml');
+});
+
+test('a valid but empty <urlset> yields no entries WITHOUT throwing', () => {
+	const { isIndex, entries } = parseSitemap(`${xmlDecl}<urlset></urlset>`);
+	assert.equal(isIndex, false);
+	assert.deepEqual(entries, []);
+});
+
+test('a self-closing empty <urlset/> yields no entries without throwing', () => {
+	const { entries } = parseSitemap(`${xmlDecl}<urlset/>`);
+	assert.deepEqual(entries, []);
+});
+
+test('an empty <sitemapindex> yields no entries without throwing', () => {
+	const { isIndex, entries } = parseSitemap(`${xmlDecl}<sitemapindex></sitemapindex>`);
+	assert.equal(isIndex, true);
+	assert.deepEqual(entries, []);
+});
+
+test('throws on an HTML error/challenge page (the Akamai 403 case)', () => {
+	const html = '<HTML><HEAD><TITLE>Access Denied</TITLE></HEAD><BODY><H1>Access Denied</H1></BODY></HTML>';
+	assert.throws(() => parseSitemap(html), /expected a <urlset> or <sitemapindex> root, got <HTML>/);
+});
+
+test('throws on an empty document', () => {
+	assert.throws(() => parseSitemap(''), /got a non-XML or empty document/);
+});
+
+test('throws on a plain-text (non-XML) response — e.g. a bare "Access Denied"', () => {
+	// fast-xml-parser parses plain text to {}, so this must NOT crash on `'urlset' in data`.
+	assert.throws(() => parseSitemap('Access Denied'), /got a non-XML or empty document/);
+});

packages/plugin/test/sitemap.test.js

@@ -1,7 +1,12 @@
 import { test } from 'node:test';
 import assert from 'node:assert/strict';
 import { applyOptions, config } from '../src/config.js';
-import { resolveUpstreamHeaders, sanitizeOriginResponseHeaders, stagingTargetIp } from '../src/util/upstream.js';
+import {
+	configuredStagingIp,
+	resolveUpstreamHeaders,
+	sanitizeOriginResponseHeaders,
+	stagingTargetIp,
+} from '../src/util/upstream.js';
 
 // Minimal stand-in for the request headers object (only `.get` is used here).
 const headersWith = (present) => ({ get: (name) => (present.includes(name) ? '1' : null) });
@@ -53,6 +58,22 @@ test('stagingTargetIp supports an IPv6 staging address', () => {
 	assert.equal(stagingTargetIp(headersWith(['x-harper-staging'])), '2606:2800:220:1:248:1893:25c8:1946');
 });
 
+test('configuredStagingIp returns the configured ip regardless of any request header', () => {
+	applyOptions({ staging: { ip: '23.50.51.27' } });
+	// No header argument at all — the sitemap refresh opts in out-of-band, not via a header.
+	assert.equal(configuredStagingIp(), '23.50.51.27');
+});
+
+test('configuredStagingIp is undefined when no staging ip is configured', () => {
+	applyOptions({});
+	assert.equal(configuredStagingIp(), undefined);
+});
+
+test('configuredStagingIp ignores an invalid configured ip', () => {
+	applyOptions({ staging: { ip: 'not-an-ip' } });
+	assert.equal(configuredStagingIp(), undefined);
+});
+
 test('ignoredHeaders defaults to an empty list', () => {
 	applyOptions({});
 	assert.deepEqual(config.ignoredHeaders, []);