Merge pull request #28 from HarperFast/fix/cache-key-consistency

fix: one canonical cache key across the whole render flow (plugin v0.7.0, browser v1.11.0)

Author: harper-joseph

package-lock.json

@@ -1,6 +1,6 @@
 {
 	"name": "@harperfast/prerender-browser",
-	"version": "1.10.0",
+	"version": "1.11.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/package.json

@@ -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 { normalizeUrlForCompare, canonicalAllowsIndex } from './util/url.js';
+import { canonicalizeUrl, canonicalAllowsIndex } from './util/url.js';
 
 const noop = () => {};
 
@@ -288,16 +288,17 @@ const renderer: Renderer = async (page, job) => {
 		};
 		const statusCode = job.httpResponse.statusCode;
 
-		const pageUrl = normalizeUrlForCompare(page.url());
-		const jobUrl = normalizeUrlForCompare(job.url);
-
-		if (pageUrl !== jobUrl) {
-			job.redirectedTo = pageUrl;
+		// Redirect detection uses the SHARED canonical form (matches the plugin), so encoding,
+		// param order, hash, and trailing-slash differences never trip a false redirect. Post
+		// back the RAW final URL — the plugin canonicalizes it with the real route allowlist.
+		const rawPageUrl = page.url();
+		if (canonicalizeUrl(rawPageUrl) !== canonicalizeUrl(job.url)) {
+			job.redirectedTo = rawPageUrl;
 		}
 
 		if (statusCode === 200) {
 			const { canonicalHref, noindex } = await page.evaluate(extractIndexSignals);
-			job.isIndexable = !noindex && canonicalAllowsIndex(canonicalHref, pageUrl);
+			job.isIndexable = !noindex && canonicalAllowsIndex(canonicalHref, rawPageUrl);
 
 			if (job.isIndexable || job.isFromSitemap) {
 				const ppStart = Date.now();

packages/browser/src/renderer.ts

@@ -22,6 +22,56 @@ const safeDecodeURI = (s: string): string => {
 	}
 };
 
+/**
+ * Canonical URL-half of a cache key. This MUST stay byte-for-byte identical to the plugin's
+ * `canonicalizeUrl` (packages/plugin/src/util/url.js) — the two are pinned by the shared test
+ * vector at repo root (`test-vectors/canonicalize-url.json`), asserted by both packages' test
+ * suites, so the copies cannot drift. The browser uses it ONLY to detect a genuine redirect
+ * (does the final page URL canonicalize to a different key than the job URL?); it forms no
+ * cache key itself and posts the RAW page URL back for the plugin to canonicalize with the
+ * route allowlist. See that file for the full rule list.
+ *
+ * `queryParams` defaults to `['*']` (keep all) — the browser has no route config, and for
+ * redirect detection keeping every param is the conservative choice; the plugin re-keys with
+ * the real per-route allowlist.
+ */
+const FIXED_DECODE: Record<string, string> = { '%3a': ':', '%2c': ',', '%40': '@' };
+
+export const canonicalizeUrl = (url: string | URL, queryParams: string[] = ['*']): string => {
+	const parsed = url instanceof URL ? new URL(url.href) : new URL(url);
+	parsed.hash = '';
+
+	const rawQuery = parsed.search.startsWith('?') ? parsed.search.slice(1) : parsed.search;
+	let query = '';
+	if (rawQuery) {
+		const keepAll = queryParams.includes('*');
+		const keep = keepAll ? null : new Set(queryParams);
+		const segments = rawQuery.split('&').filter((seg) => {
+			if (seg === '') return false;
+			if (keepAll) return true;
+			const rawKey = seg.split('=')[0];
+			let key: string;
+			try {
+				key = decodeURIComponent(rawKey);
+			} catch {
+				key = rawKey;
+			}
+			return keep!.has(key);
+		});
+		segments.sort();
+		if (segments.length) query = `?${segments.join('&')}`;
+	}
+
+	const path =
+		parsed.pathname !== '/' && parsed.pathname.endsWith('/') ? parsed.pathname.slice(0, -1) : parsed.pathname;
+
+	let half = `${parsed.protocol}//${parsed.host}${path}${query}`;
+	half = half.replace(/%(?:3a|2c|40)/gi, (m) => FIXED_DECODE[m.toLowerCase()]);
+	half = half.replace(/%[0-9a-f]{2}/gi, (m) => m.toUpperCase());
+	half = half.replace(/\|/g, '%7C');
+	return half;
+};
+
 export const normalizeUrlForCompare = (url: string | URL): string => {
 	const parsed = new URL(url);
 	parsed.searchParams.sort();

packages/browser/src/util/url.ts

@@ -1,52 +1,83 @@
 import { test } from 'node:test';
 import assert from 'node:assert/strict';
-import { normalizeUrlForCompare, normalizeCanonicalUrl, canonicalAllowsIndex } from '../dist/util/url.js';
+import { readFileSync } from 'node:fs';
+import {
+	canonicalizeUrl,
+	normalizeUrlForCompare,
+	normalizeCanonicalUrl,
+	canonicalAllowsIndex,
+} from '../dist/util/url.js';
 
-// The reported bug: the request carries %3A (encoded colon) while the page's canonical uses
-// the literal ':'. They name the same resource, so the page is self-canonical → indexable.
+// The single shared vector, asserted by BOTH the browser (this file) and the plugin suite,
+// so the two canonicalizeUrl copies (TS here, JS in the plugin) cannot drift.
+const VECTORS: { url: string; allowlist: string[]; expected: string }[] = JSON.parse(
+	readFileSync(new URL('../../../test-vectors/canonicalize-url.json', import.meta.url), 'utf8')
+);
+
+test('canonicalizeUrl matches every shared cache-key vector (must equal the plugin)', () => {
+	for (const { url, allowlist, expected } of VECTORS) {
+		assert.equal(canonicalizeUrl(url, allowlist), expected, `vector: ${url} @ ${JSON.stringify(allowlist)}`);
+	}
+});
+
+// Redirect detection compares canonicalizeUrl(page.url()) vs canonicalizeUrl(job.url). A page
+// that did not redirect must NOT be flagged just because Chrome's page.url() re-encodes, adds a
+// trailing slash, or reorders params relative to the stored key.
+test('canonicalizeUrl does not flag a non-redirect (encoding / trailing slash / order)', () => {
+	const jobUrl = 'https://example.com/p?b=2&a=1';
+	assert.equal(canonicalizeUrl('https://example.com/p/?a=1&b=2'), canonicalizeUrl(jobUrl)); // slash + order
+	assert.equal(canonicalizeUrl('https://example.com/p?a=1&b=2#frag'), canonicalizeUrl(jobUrl)); // hash
+	// ':' literal (Chrome's form) matches a stored key that decoded '%3A' to ':'
+	assert.equal(canonicalizeUrl('https://example.com/a?f=X:Y'), canonicalizeUrl('https://example.com/a?f=X%3AY'));
+});
+
+// --- indexability (self-canonical) comparison — separate from the cache key ---
+
+// The request carries %3A (encoded colon) while the page's canonical uses the literal ':'.
+// They name the same resource, so the page is self-canonical → indexable.
 test('canonical with a decoded reserved char (: vs %3A) counts as self-canonical', () => {
-	const requested = 'https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender%3AMens+Department%3AClothing';
-	const canonical = 'https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender:Mens+Department:Clothing';
+	const requested = 'https://example.com/c/page.jsp?f=A%3AB+g%3AC';
+	const canonical = 'https://example.com/c/page.jsp?f=A:B+g:C';
 	assert.equal(canonicalAllowsIndex(canonical, requested), true);
 });
 
 test('a genuinely different canonical is NOT self-canonical (stays non-indexable)', () => {
-	const requested = 'https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender%3AMens';
-	const canonical = 'https://www.kohls.com/catalog/womens-clothing.jsp?CN=Gender%3AWomens';
+	const requested = 'https://example.com/c/page-a.jsp?f=A%3AB';
+	const canonical = 'https://example.com/c/page-b.jsp?f=X%3AY';
 	assert.equal(canonicalAllowsIndex(canonical, requested), false);
 });
 
 test('no canonical → indexable', () => {
-	assert.equal(canonicalAllowsIndex(null, 'https://www.kohls.com/x'), true);
-	assert.equal(canonicalAllowsIndex(undefined, 'https://www.kohls.com/x'), true);
-	assert.equal(canonicalAllowsIndex('', 'https://www.kohls.com/x'), true);
+	assert.equal(canonicalAllowsIndex(null, 'https://example.com/x'), true);
+	assert.equal(canonicalAllowsIndex(undefined, 'https://example.com/x'), true);
+	assert.equal(canonicalAllowsIndex('', 'https://example.com/x'), true);
 });
 
 test('param order, trailing slash, and hash differences do not break self-canonical', () => {
-	const current = 'https://www.kohls.com/p?b=2&a=1';
-	assert.equal(canonicalAllowsIndex('https://www.kohls.com/p?a=1&b=2', current), true); // order
-	assert.equal(canonicalAllowsIndex('https://www.kohls.com/p/?b=2&a=1', current), true); // trailing slash
-	assert.equal(canonicalAllowsIndex('https://www.kohls.com/p?b=2&a=1#frag', current), true); // hash
+	const current = 'https://example.com/p?b=2&a=1';
+	assert.equal(canonicalAllowsIndex('https://example.com/p?a=1&b=2', current), true); // order
+	assert.equal(canonicalAllowsIndex('https://example.com/p/?b=2&a=1', current), true); // trailing slash
+	assert.equal(canonicalAllowsIndex('https://example.com/p?b=2&a=1#frag', current), true); // hash
 });
 
 test('a relative canonical resolves against the current URL', () => {
-	const current = 'https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender%3AMens';
-	assert.equal(canonicalAllowsIndex('/catalog/mens-clothing.jsp?CN=Gender:Mens', current), true);
+	const current = 'https://example.com/c/page.jsp?f=A%3AB';
+	assert.equal(canonicalAllowsIndex('/c/page.jsp?f=A:B', current), true);
 });
 
 test('a malformed canonical fails open (does not drop indexability)', () => {
-	assert.equal(canonicalAllowsIndex('http://[bad', 'https://www.kohls.com/x'), true);
+	assert.equal(canonicalAllowsIndex('http://[bad', 'https://example.com/x'), true);
 });
 
 test('normalizeCanonicalUrl canonicalizes encoding, param order, hash, and trailing slash', () => {
 	assert.equal(
-		normalizeCanonicalUrl('https://x.com/a/?c=2&b=Gender:Mens#h'),
-		normalizeCanonicalUrl('https://x.com/a?b=Gender%3AMens&c=2')
+		normalizeCanonicalUrl('https://x.com/a/?c=2&b=A:B#h'),
+		normalizeCanonicalUrl('https://x.com/a?b=A%3AB&c=2')
 	);
 });
 
 test('normalizeCanonicalUrl is idempotent', () => {
-	const once = normalizeCanonicalUrl('https://www.kohls.com/p/?b=2&a=Gender:Mens#h');
+	const once = normalizeCanonicalUrl('https://example.com/p/?b=2&a=A:B#h');
 	assert.equal(normalizeCanonicalUrl(once), once);
 });
 
@@ -60,6 +91,7 @@ test('malformed percent-encoding falls back to raw instead of throwing', () => {
 	for (const bad of ['https://x.com/%E0%A0/a', 'https://x.com/p?x=%E0%A0']) {
 		assert.doesNotThrow(() => normalizeUrlForCompare(bad));
 		assert.doesNotThrow(() => normalizeCanonicalUrl(bad));
+		assert.doesNotThrow(() => canonicalizeUrl(bad));
 	}
 	assert.doesNotThrow(() => canonicalAllowsIndex('https://x.com/a', 'https://x.com/%E0%A0/a'));
 });

packages/browser/test/url.test.ts

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

packages/plugin/package.json

@@ -2,7 +2,7 @@ import { setTimeout as sleep } from 'node:timers/promises';
 import { CacheKey } from '../util/cacheKey.js';
 import { getBotName } from '../util/userAgent.js';
 import { isPrerenderCandidate } from '../util/indexSignals.js';
-import { normalizeUrl, cacheKeyUrl } from '../util/url.js';
+import { canonicalizeUrl } from '../util/url.js';
 import { config } from '../config.js';
 import { sanitizeDeviceType } from '../util/device_type.js';
 import { isForwardedMode, resolveForwardedRequest } from '../util/ingress.js';
@@ -22,7 +22,7 @@ export async function handleBotRequest(request) {
 		if (!target) {
 			return { headers: {}, status: 400 };
 		}
-		const { url, deviceType, noCache, route } = target;
+		const { url, cacheUrl, deviceType, noCache, route } = target;
 
 		request.botName = getBotName(request.headers);
 		if (config.analytics.enabled && (request.botName !== 'other' || config.analytics.recordUnmatched)) {
@@ -34,7 +34,7 @@ export async function handleBotRequest(request) {
 		// `noCache` marks an unrecognized forwarded path we proxy but never cache.
 		const info = { route, noCache };
 
-		const resource = await resolveResource({ request, url, deviceType, info });
+		const resource = await resolveResource({ request, url, cacheUrl, deviceType, info });
 		maybeSchedule(resource, noCache);
 
 		return deliverResource(resource, request, info);
@@ -55,11 +55,20 @@ function resolveBotTarget(request) {
 	if (isForwardedMode()) {
 		const target = request._prerenderTarget ?? resolveForwardedRequest(request);
 		if (!target) return null;
-		return { url: target.url, deviceType: target.deviceType, noCache: !!target.noCache, route: target.route };
+		return {
+			url: target.url,
+			cacheUrl: target.cacheUrl,
+			deviceType: target.deviceType,
+			noCache: !!target.noCache,
+			route: target.route,
+		};
 	}
 
+	// Native/prefix mode: the request path (minus the bot prefix) IS the absolute target URL.
+	const cacheUrl = canonicalizeUrl(request.url.slice(config.botPathPrefix.length), config.url.queryParams);
 	return {
-		url: normalizeUrl(request.url.slice(config.botPathPrefix.length), true),
+		url: new URL(cacheUrl),
+		cacheUrl,
 		deviceType: sanitizeDeviceType(request.headers.get(config.ingress.deviceTypeHeader)),
 		noCache: false,
 		route: undefined,
@@ -69,7 +78,7 @@ function resolveBotTarget(request) {
 // Resolve the resource to serve: an origin proxy for non-GET/HEAD, else a fresh cache hit,
 // an on-demand render, or an origin proxy per the miss mode. Populates the debug `info`
 // (cacheKey/url/cacheStatus/source/renderNowStatus) as a side effect.
-async function resolveResource({ request, url, deviceType, info }) {
+async function resolveResource({ request, url, cacheUrl, deviceType, info }) {
 	if (request.method !== 'GET' && request.method !== 'HEAD') {
 		logger.warn(`Unexpected Request ${request.method} ${url}`);
 		info.source = 'origin';
@@ -82,12 +91,11 @@ async function resolveResource({ request, url, deviceType, info }) {
 		});
 	}
 
-	// `url.href` re-serializes on each access; read it once and reuse for the cache key
-	// and the debug info.
-	const href = url.href;
-	const cacheKey = CacheKey.toCacheKey({ url: cacheKeyUrl(href), deviceType });
+	// `cacheUrl` is the canonical URL-half (already computed at ingress); it IS the cache-key
+	// url component, so no re-normalization here.
+	const cacheKey = CacheKey.toCacheKey({ url: cacheUrl, deviceType });
 	info.cacheKey = cacheKey;
-	info.url = href;
+	info.url = cacheUrl;
 
 	// On-demand levers apply only to an authorized GET for a non-excluded URL.
 	const authorized = request.method === 'GET' && isRenderNowAuthorized(request.headers) && !isExcludedUrl(url);
@@ -198,14 +206,18 @@ async function renderNow({ url, deviceType, cacheKey, request }) {
 async function handlePageScheduling(resource) {
 	try {
 		if (isPrerenderCandidate(resource)) {
+			// resource.url is the origin-fetch URL built from the canonical half, so it is
+			// already route-filtered; canonicalize idempotently ('*' keeps it as-is) so this
+			// url-half equals CacheKey.extractUrl of the render key (and the NonIndexable id).
+			const canonicalUrl = canonicalizeUrl(resource.url, ['*']);
 			const existingNonIndexable = await databases.signals.NonIndexable.get({
-				id: cacheKeyUrl(resource.url),
+				id: canonicalUrl,
 				select: 'url',
 			});
 
 			if (!existingNonIndexable) {
 				for (const deviceType of config.deviceTypes.default) {
-					const cacheKey = CacheKey.toCacheKey({ url: cacheKeyUrl(resource.url), deviceType });
+					const cacheKey = CacheKey.toCacheKey({ url: canonicalUrl, deviceType });
 					const existingTarget = await RenderTarget.get({ id: cacheKey, select: 'cacheKey' });
 					if (!existingTarget) {
 						// No explicit time → RenderTarget.put jitters the first render across the

packages/plugin/src/http_handlers/bot_request.js

@@ -3,7 +3,8 @@ import { config } from '../config.js';
 import { currentMinuteMs } from '../util/time.js';
 import { QueueState } from './QueueState.js';
 import { CacheKey } from '../util/cacheKey.js';
-import { cacheKeyUrl, normalizeUrl } from '../util/url.js';
+import { canonicalizeUrl } from '../util/url.js';
+import { queryAllowlistFor } from '../util/ingress.js';
 import { RenderTarget } from './RenderTarget.js';
 
 const protocol = server.hostname === 'localhost' ? 'http' : 'https';
@@ -68,19 +69,25 @@ export class RenderQueue extends Resource {
 		const url = result.redirectedTo || result.url;
 
 		if (result.redirectedTo) {
-			if (result.redirectedTo !== result.url) {
+			const { deviceType } = CacheKey.parse(result.id);
+
+			// The browser posts the RAW final page URL as `redirectedTo`. Canonicalize it the
+			// same way serving does — with the allowlist a bot READ of that target would use
+			// (route-aware) — so the rendered content is stored under the key that read computes.
+			const redirectKey = CacheKey.toCacheKey({
+				deviceType,
+				url: canonicalizeUrl(result.redirectedTo, queryAllowlistFor(result.redirectedTo)),
+			});
+
+			// Only treat it as a redirect when the final URL canonicalizes to a DIFFERENT key.
+			// A target whose page URL collapses back to the same key (trailing slash, param
+			// reorder, encoding) is not a redirect — keep it under result.id and, crucially,
+			// do NOT delete its RenderTarget (which would drop it from the recurring rotation).
+			if (redirectKey !== result.id) {
 				logger.warn(`Skipped prerendered url due to redirect: ${result.id} redirected to ${result.redirectedTo}`);
 				await RenderTarget.delete(result.id);
+				cacheKey = redirectKey;
 			}
-
-			const { deviceType } = CacheKey.parse(result.id);
-
-			// Normalize the redirect target the same way serving does, so the rendered content is
-			// stored under the key a bot request for that URL will look up. redirectedTo is the
-			// renderer's decodeURI'd form; normalizeUrl re-sorts and standardizes the encoding
-			// (['*'] keeps all params — the matched route, hence its allowlist, isn't known here),
-			// then cacheKeyUrl applies the shared cache-key encoding.
-			cacheKey = CacheKey.toCacheKey({ deviceType, url: cacheKeyUrl(normalizeUrl(result.redirectedTo, false, ['*'])) });
 		}
 
 		try {