fix(browser): read the canonical against the cache key, not the document (v1.17.0)

packages/browser/package.json

@@ -1,6 +1,6 @@
 {
 	"name": "@harperfast/prerender-browser",
-	"version": "1.16.0",
+	"version": "1.17.0",
 	"type": "module",
 	"description": "Headless-browser render library for Harper Prerender: claims render jobs from the @harperfast/prerender queue, renders pages in headless Chrome (Puppeteer), and posts the HTML back. Embedded by a render service and configured entirely via startWorker() options.",
 	"keywords": [

packages/browser/src/RenderJob.ts

@@ -113,7 +113,9 @@ export default class RenderJob {
 	/**
 	 * Why this render produced no cacheable content — one slug across every no-content class,
 	 * so the plugin logs/tracks a single field: 'noindex' (robots meta/header),
-	 * 'canonical-mismatch' (page canonicalizes elsewhere), 'http-error' (non-200 document),
+	 * 'canonical-mismatch' (page canonicalizes to a different URL), 'canonical-variant'
+	 * (canonicalizes to the same URL RE-SPELLED — one document, two cache keys), 'http-error'
+	 * (non-200 document),
 	 * 'redirect-loop', 'redirect' (bailed at navigation), or 'error' (renderer threw — details
 	 * ride in the posted `error`). Unset when content was produced; sendResult derives the
 	 * redirect/error fallbacks so callers only set the values they alone can know.

packages/browser/src/config.ts

@@ -198,6 +198,31 @@ export type WaitForRule = {
 	pathPattern?: string;
 };
 
+/**
+ * How to read a page's `<link rel="canonical">` — see `canonicalVerdict` in util/url.ts.
+ *
+ * A canonical that names a DIFFERENT document always makes the page non-indexable; that much is
+ * invariable. The open question is the re-spelling: a canonical that names this very document
+ * under a different cache key ('variant'), which happens when a site writes a space as `+` where
+ * its sitemap writes `%20`, or vice versa. Whether those two spellings are one resource is a fact
+ * about the SITE's query parsing, not about the URLs — a form-decoding origin cannot tell them
+ * apart, an RFC-3986 one can — so it is config, not a hardcoded assumption.
+ *
+ * `strict: false` (default) reproduces the historical reading exactly: a re-spelling counts as
+ * self-canonical and gets its own target. `strict: true` calls it a duplicate key and reports
+ * `canonical-variant`, so the plugin retires it instead of rendering the same bytes twice forever.
+ *
+ * Turn it on only for a site whose origin form-decodes its query. One request settles that for a
+ * given parameter, for every URL: ask for a value containing a literal `+` (`?f=A%2BB`) and then
+ * the same value with a raw `+` (`?f=A+B`). If the origin resolves the second as a SPACE — its
+ * canonical comes back `A%20B`, or it simply serves what `A B` names — it form-decodes, and the
+ * two spellings can never name different resources.
+ */
+export type CanonicalConfig = {
+	/** Treat a re-spelled self-canonical as a duplicate cache key (non-indexable). Default false. */
+	strict: boolean;
+};
+
 export type PrerenderConfig = {
 	/** Device profiles keyed by the job's `deviceType`; unknown types fall back to `defaultDevice`. */
 	devices: Record<string, DeviceProfile>;
@@ -206,6 +231,7 @@ export type PrerenderConfig = {
 	navigation: NavigationConfig;
 	scroll: ScrollConfig;
 	postProcess: PostProcessConfig;
+	canonical: CanonicalConfig;
 	/**
 	 * Optional declarative "wait for content" rules applied after scroll/settle and before the
 	 * snapshot (see {@link WaitForRule}). Absent by default → a complete no-op, so existing
@@ -255,6 +281,7 @@ export const defaultConfig = (): PrerenderConfig => ({
 		stripBlockedResources: false,
 		resolveLazyImages: false,
 	},
+	canonical: { strict: false },
 	injectWebComponentsPolyfill: true,
 	extraHeaders: {},
 });

packages/browser/src/renderOnce.ts

@@ -95,7 +95,7 @@ export interface RenderResult {
 	outcome: RenderOutcome;
 	statusCode: number | undefined;
 	isIndexable: boolean | undefined;
-	/** Why no cacheable content: 'noindex' | 'canonical-mismatch' | 'http-error' | 'redirect-loop' | 'redirect' | 'error'. */
+	/** Why no cacheable content: 'noindex' | 'canonical-mismatch' | 'canonical-variant' | 'http-error' | 'redirect-loop' | 'redirect' | 'error'. */
 	reason: string | undefined;
 	/** Normalized redirect target (job.redirectedTo), if the final URL canonically differs. */
 	redirectedTo: string | undefined;

packages/browser/src/renderer.ts

@@ -3,7 +3,7 @@ import type { RenderTimings } from './RenderJob.js';
 import { settings } from './settings.js';
 import { CACHE_REPLAY_HEADER, getResourceCache } from './ResourceCache.js';
 import type { PostProcessConfig } from './config.js';
-import { canonicalizeUrl, canonicalAllowsIndex } from './util/url.js';
+import { canonicalizeUrl, canonicalVerdict } from './util/url.js';
 import { markRenderPhase } from './util/renderPhase.js';
 
 const noop = () => {};
@@ -450,8 +450,19 @@ const renderer: Renderer = async (page, job) => {
 
 		if (statusCode === 200) {
 			const { canonicalHref, noindex } = await page.evaluate(extractIndexSignals);
-			job.isIndexable = !noindex && canonicalAllowsIndex(canonicalHref, rawPageUrl);
-			if (!job.isIndexable) job.reason = noindex ? 'noindex' : 'canonical-mismatch';
+			// A canonical naming a DIFFERENT document always disowns the page — invariable, every
+			// site. A canonical naming this very document RE-SPELLED as another cache key
+			// ('variant') is only a duplicate if the site's origin cannot tell the two spellings
+			// apart, which is a property of its query parser, not of the URLs — so that half is
+			// config, defaulting to the historical lenient reading. See config.canonical.strict.
+			// Either way the reason slug is distinct, so duplicate spellings stay legible next to
+			// genuine mismatches.
+			const verdict = canonicalVerdict(canonicalHref, rawPageUrl);
+			const disowned = verdict === 'elsewhere' || (verdict === 'variant' && config.canonical.strict);
+			job.isIndexable = !noindex && !disowned;
+			if (!job.isIndexable) {
+				job.reason = noindex ? 'noindex' : verdict === 'variant' ? 'canonical-variant' : 'canonical-mismatch';
+			}
 
 			if (job.isIndexable || job.isFromSitemap) {
 				const ppStart = Date.now();

packages/browser/src/util/url.ts

@@ -79,11 +79,16 @@ export const normalizeUrlForCompare = (url: string | URL): string => {
 };
 
 /**
- * Normalize a URL for canonical self-reference comparison. Like {@link normalizeUrlForCompare}
- * but also drops the hash and a trailing slash — neither changes which document a canonical
- * names. Sorting re-serializes the query with uniform (form) encoding, so a reserved char
- * that is percent-encoded in the request (`%3A`) and literal in the canonical (`:`) compare
- * equal; `decodeURI` alone can't do that, as it leaves reserved-char escapes intact.
+ * Normalize a URL down to the DOCUMENT it names, ignoring how that name is spelled. Like
+ * {@link normalizeUrlForCompare} but also drops the hash and a trailing slash — neither changes
+ * which document a canonical names. Sorting re-serializes the query with uniform (form)
+ * encoding, so a reserved char that is percent-encoded in the request (`%3A`) and literal in
+ * the canonical (`:`) compare equal; `decodeURI` alone can't do that, as it leaves
+ * reserved-char escapes intact.
+ *
+ * That form-encoding round-trip also collapses `%20` and `+` — which makes this the wrong
+ * question for indexability (see {@link canonicalVerdict}) and exactly the right one for
+ * telling a re-spelling of one document apart from a pointer at a different document.
  */
 export const normalizeCanonicalUrl = (url: string | URL): string => {
 	const parsed = new URL(url);
@@ -97,18 +102,54 @@ export const normalizeCanonicalUrl = (url: string | URL): string => {
 	return safeDecodeURI(parsed.href);
 };
 
+/** What a page's `<link rel="canonical">` says about the URL that was rendered. */
+export type CanonicalVerdict = 'self' | 'variant' | 'elsewhere';
+
 /**
- * Whether a page's canonical link permits indexing: true when there is no canonical, or the
- * canonical resolves to the same URL as the page. A relative canonical is resolved against
- * the current URL. Comparison is on the normalized form, so encoding, param order, hash, and
- * trailing-slash differences are not treated as a mismatch. A malformed canonical fails open
- * (does not drop indexability over a broken tag).
+ * Read a page's canonical link in the only unit the plugin can act on: the CACHE KEY.
+ *
+ *   'self'      — no canonical, or it canonicalizes to this very key. Indexable.
+ *   'variant'   — it names the same document RE-SPELLED as a different key: `%20` where the
+ *                 canonical writes `+`, or any other difference that survives the cache key but
+ *                 vanishes under form-encoding. Rendering it fills a second key with bytes
+ *                 identical to the first.
+ *   'elsewhere' — it names a different URL outright (different path, different params, values
+ *                 in a different order). Often still the same page — a faceted origin ignores a
+ *                 decorative slug and reorders facets for you — but nothing about the URLs says
+ *                 so, and the answer is the same either way: don't index this one.
+ *
+ * Both non-self verdicts are non-indexable and the plugin suppresses them identically; they
+ * are separated only so an operator can read "N duplicate spellings" apart from "N pages that
+ * canonicalize to another page". An origin that writes canonicals in a different encoding from
+ * its own sitemap would surface as a wave of 'variant' — that is the signal to investigate,
+ * not a reason to loosen the comparison back. What such an origin CANNOT lose is its declared
+ * corpus: a sitemap-listed url is serialized even when non-indexable, so its result posts with
+ * content and `rendered` wins in {@link RenderJob.outcome}, which keeps it out of the plugin's
+ * suppression branch entirely. Only urls the plugin discovered are retirable this way.
+ *
+ * WHY THE KEY, NOT THE DOCUMENT. The consequence of this verdict is `Target.suppress`, and a
+ * Target IS a cache key: two spellings of one document that key differently are two targets,
+ * two recurring render slots, and two copies of the same bytes, forever. So "does this name
+ * the same document" is not the question worth asking here — "does this name the same key" is.
+ * Asking it through `canonicalizeUrl` also means the answer cannot drift from what the plugin
+ * actually stores, since that is the same function the key is built with.
+ *
+ * The current URL's own param NAMES are the allowlist for both sides. The rendered URL is
+ * already route-filtered (it is a canonical half), so a param the route drops must never be
+ * able to manufacture a mismatch, while a param the route keeps is compared byte-for-byte.
+ *
+ * A relative canonical resolves against the current URL. A malformed one fails open ('self'):
+ * a broken tag must not cost a page its indexability.
  */
-export const canonicalAllowsIndex = (canonicalHref: string | null | undefined, currentUrl: string): boolean => {
-	if (!canonicalHref) return true;
+export const canonicalVerdict = (canonicalHref: string | null | undefined, currentUrl: string): CanonicalVerdict => {
+	if (!canonicalHref) return 'self';
 	try {
-		return normalizeCanonicalUrl(new URL(canonicalHref, currentUrl)) === normalizeCanonicalUrl(currentUrl);
+		const current = new URL(currentUrl);
+		const canonical = new URL(canonicalHref, current);
+		const allowlist = [...current.searchParams.keys()];
+		if (canonicalizeUrl(canonical, allowlist) === canonicalizeUrl(current, allowlist)) return 'self';
+		return normalizeCanonicalUrl(canonical) === normalizeCanonicalUrl(current) ? 'variant' : 'elsewhere';
 	} catch {
-		return true;
+		return 'self';
 	}
 };

packages/browser/test/redirect.test.ts

@@ -78,6 +78,14 @@ before(async () => {
 				return res.end(
 					`<!doctype html><html><head><title>t</title><link rel="canonical" href="/destination"></head><body>x</body></html>`
 				);
+			// Serves one page under any spelling of its query, and canonicalizes to the `+` one —
+			// what a faceted origin does. Requested as `%20`, it is the same document under a
+			// different cache key.
+			case '/canonical-respelled':
+				res.setHeader('content-type', 'text/html');
+				return res.end(
+					`<!doctype html><html><head><title>t</title><link rel="canonical" href="/canonical-respelled?f=A+B"></head><body>x</body></html>`
+				);
 			case '/destination':
 				res.setHeader('content-type', 'text/html');
 				return page('DESTINATION');
@@ -167,3 +175,79 @@ test('reason distinguishes noindex from canonical-mismatch', async () => {
 	assert.equal(canonical.isIndexable, false);
 	assert.equal(canonical.reason, 'canonical-mismatch');
 });
+
+// A canonical that names THIS document under another spelling is still a second cache key, so
+// it is non-indexable — with its own reason, because "you already render this page under a
+// different key" is a different operational story from "this page disowns itself".
+test('a re-spelled self-canonical is non-indexable as canonical-variant — under canonical.strict', async () => {
+	const result = await renderOnce({
+		url: `${base}/canonical-respelled?f=A%20B`,
+		config: { scroll: { enabled: false }, canonical: { strict: true } },
+		resourceCache: { enabled: false },
+		captureNonIndexable: false, // production's gate: a DISCOVERED url, not a sitemap-listed one
+	});
+	await result.close();
+
+	assert.equal(result.isIndexable, false);
+	assert.equal(result.reason, 'canonical-variant');
+	assert.equal(result.outcome, 'non-indexable');
+	assert.equal(result.html, undefined);
+});
+
+// Whether two spellings are one resource is a fact about the SITE's query parser, so the default
+// must be the historical reading: the re-spelling keeps its own target and renders. A site only
+// opts in after establishing that its origin form-decodes (one request, see CanonicalConfig).
+test('by default a re-spelled self-canonical is still indexable (no behavior change on upgrade)', async () => {
+	const result = await renderOnce({
+		url: `${base}/canonical-respelled?f=A%20B`,
+		config: { scroll: { enabled: false } },
+		resourceCache: { enabled: false },
+		captureNonIndexable: false,
+	});
+	await result.close();
+
+	assert.equal(result.isIndexable, true);
+	assert.equal(result.reason, undefined);
+	assert.ok(result.html);
+});
+
+// The invariable half is NOT configurable: a canonical naming a different document disowns the
+// page whatever `strict` says.
+test('a canonical pointing elsewhere is non-indexable regardless of canonical.strict', async () => {
+	for (const strict of [false, true]) {
+		const result = await renderOnce({
+			url: `${base}/canonical-elsewhere`,
+			config: { scroll: { enabled: false }, canonical: { strict } },
+			resourceCache: { enabled: false },
+			captureNonIndexable: false,
+		});
+		await result.close();
+		assert.equal(result.isIndexable, false, `strict=${strict}`);
+		assert.equal(result.reason, 'canonical-mismatch', `strict=${strict}`);
+	}
+});
+
+// THE BLAST-RADIUS GUARANTEE, pinned so a refactor cannot quietly remove it. A sitemap-listed
+// url serializes even when non-indexable, and content wins in RenderJob.outcome — so the result
+// posts as 'rendered' and the plugin's suppression branch is never reached. Whatever a canonical
+// verdict decides, it can only ever retire urls we DISCOVERED; the corpus the site itself
+// declared is structurally out of reach. An origin whose canonicals are spelled differently from
+// its own sitemap therefore loses nothing but the isIndexable flag on the stored page.
+test('a sitemap-listed url survives any canonical verdict (content wins → rendered)', async () => {
+	for (const path of ['/canonical-respelled?f=A%20B', '/canonical-elsewhere']) {
+		const result = await renderOnce({
+			url: `${base}${path}`,
+			// strict on, i.e. the harshest reading available — the guarantee must hold even there.
+			config: { scroll: { enabled: false }, canonical: { strict: true } },
+			resourceCache: { enabled: false },
+			captureNonIndexable: true, // === isFromSitemap in production
+			// The POSTED outcome — what the plugin actually branches on — not renderOnce's own slug.
+			probes: { posted: ({ job }) => job.outcome },
+		});
+		await result.close();
+
+		assert.equal(result.isIndexable, false, `${path}: the verdict itself is unchanged`);
+		assert.ok(result.html, `${path}: but the page is serialized anyway`);
+		assert.equal(result.probes.posted, 'rendered', `${path}: so content wins and nothing suppresses it`);
+	}
+});