Merge pull request #14 from HarperFast/fix/plugin-cachekey-normalization

fix(plugin): normalize cache-key URL encoding so equivalent URLs share one key; v0.3.3

Author: harper-joseph

package-lock.json

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

packages/plugin/package.json

@@ -3,7 +3,7 @@ import { Readable } from 'node:stream';
 import { CacheKey } from '../util/cacheKey.js';
 import { getBotName } from '../util/userAgent.js';
 import { isPrerenderCandidate } from '../util/indexSignals.js';
-import { normalizeUrl } from '../util/url.js';
+import { normalizeUrl, cacheKeyUrl } from '../util/url.js';
 import { config } from '../config.js';
 import { sanitizeDeviceType } from '../util/device_type.js';
 import { isForwardedMode, resolveForwardedRequest } from '../util/ingress.js';
@@ -53,7 +53,7 @@ export async function handleBotRequest(request) {
 				body: request._nodeRequest,
 			});
 		} else {
-			const cacheKey = CacheKey.toCacheKey({ url: url.href, deviceType });
+			const cacheKey = CacheKey.toCacheKey({ url: cacheKeyUrl(url), deviceType });
 
 			const page = await PrerenderedPage.get(cacheKey);
 
@@ -77,11 +77,14 @@ export async function handleBotRequest(request) {
 async function handlePageScheduling(resource) {
 	try {
 		if (isPrerenderCandidate(resource)) {
-			const existingNonIndexable = await databases.signals.NonIndexable.get({ id: resource.url, select: 'url' });
+			const existingNonIndexable = await databases.signals.NonIndexable.get({
+				id: cacheKeyUrl(resource.url),
+				select: 'url',
+			});
 
 			if (!existingNonIndexable) {
 				for (const deviceType of config.deviceTypes.default) {
-					const cacheKey = CacheKey.toCacheKey({ url: resource.url, deviceType });
+					const cacheKey = CacheKey.toCacheKey({ url: cacheKeyUrl(resource.url), deviceType });
 					const existingTarget = await RenderTarget.get({ id: cacheKey, select: 'cacheKey' });
 					if (!existingTarget) {
 						await RenderTarget.put(cacheKey, {

packages/plugin/src/http_handlers/bot_request.js

@@ -3,6 +3,7 @@ import { config } from '../config.js';
 import { currentMinuteMs, getNextRenderTime } from '../util/time.js';
 import { QueueState } from './QueueState.js';
 import { CacheKey } from '../util/cacheKey.js';
+import { cacheKeyUrl } from '../util/url.js';
 import { RenderTarget } from './RenderTarget.js';
 
 const protocol = server.hostname === 'localhost' ? 'http' : 'https';
@@ -74,7 +75,11 @@ export class RenderQueue extends Resource {
 
 			const { deviceType } = CacheKey.parse(result.id);
 
-			cacheKey = CacheKey.toCacheKey({ deviceType, url: result.redirectedTo });
+			// 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 (the renderer's
+			// redirectedTo is decodeURI'd and skips the query allowlist — normalizeUrl re-encodes
+			// and re-applies the policy for a consistent key).
+			cacheKey = CacheKey.toCacheKey({ deviceType, url: cacheKeyUrl(result.redirectedTo) });
 		}
 
 		try {

packages/plugin/src/resources/RenderQueue.js

@@ -28,3 +28,25 @@ export const normalizeUrl = (url, returnObject = false, queryParams = config.url
 
 	return returnObject ? parsed : parsed.href;
 };
+
+/**
+ * The cache-key form of an ALREADY-NORMALIZED URL: presents `:` decoded so keys read like
+ * the canonical/sitemap form (`?CN=Gender:Mens`) instead of the form-encoded `%3A`.
+ *
+ * This is a pure encoding transform — it does NOT re-apply the query allowlist or re-sort.
+ * The caller must pass an already-normalized URL (from `normalizeUrl` or the forwarded-mode
+ * ingress resolution), because in forwarded mode that normalization used the per-ROUTE
+ * allowlist, which this function can't know — re-normalizing here with the global policy
+ * would wrongly drop route params like `CN`.
+ *
+ * Decodes only query-legal, non-separator characters — `:` (facets), `,` (lists), `@` — so
+ * `:` and `%3A` (etc.) address the same resource and the keys read like the canonical. Anything
+ * structural stays encoded (`&`/`=`/`+`/`#`, and `;` which some parsers treat as a separator),
+ * so the URL can't reparse into a different shape. (Edit `DECODE` to change the policy in one
+ * place — every cache-key site calls this.)
+ */
+const DECODE = { '%3a': ':', '%2c': ',', '%40': '@' };
+export const cacheKeyUrl = (url) => {
+	const href = typeof url === 'string' ? url : (url?.href ?? String(url));
+	return href.replace(/%(?:3a|2c|40)/gi, (m) => DECODE[m.toLowerCase()]);
+};

packages/plugin/src/util/url.js

@@ -1,7 +1,7 @@
 import { test, beforeEach } from 'node:test';
 import assert from 'node:assert/strict';
 import { applyOptions } from '../src/config.js';
-import { normalizeUrl } from '../src/util/url.js';
+import { normalizeUrl, cacheKeyUrl } from '../src/util/url.js';
 
 beforeEach(() => applyOptions({}));
 
@@ -40,3 +40,39 @@ test('an explicit empty allowlist drops all params regardless of global policy',
 	applyOptions({ url: { queryParams: ['*'] } });
 	assert.equal(normalizeUrl('https://x.com/a?page=2&CN=foo', false, []), 'https://x.com/a');
 });
+
+// cacheKeyUrl presents the cache-key form of an ALREADY-NORMALIZED url: it decodes `:` so
+// keys read like the canonical form (`?CN=Gender:Mens`). Pure transform — it must NOT
+// re-normalize or drop params (serving already applied the route/global allowlist).
+test('cacheKeyUrl decodes ":" and preserves the (already-normalized) query verbatim', () => {
+	assert.equal(
+		cacheKeyUrl('https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender%3AMens+Department%3AClothing'),
+		'https://www.kohls.com/catalog/mens-clothing.jsp?CN=Gender:Mens+Department:Clothing'
+	);
+});
+
+test('cacheKeyUrl does not drop params (no allowlist re-applied)', () => {
+	// even a param the global allowlist would drop is preserved — the caller already normalized
+	assert.equal(cacheKeyUrl('https://x.com/a?CN=x%3Ay&foo=1'), 'https://x.com/a?CN=x:y&foo=1');
+});
+
+test('cacheKeyUrl decodes safe query sub-delimiters (: , @) but leaves separators encoded', () => {
+	assert.equal(cacheKeyUrl('https://x.com/a?CN=A%3AB%2CC%40D'), 'https://x.com/a?CN=A:B,C@D');
+	// %26 (&), %3D (=), %3B (;) stay encoded — decoding them could shift how the URL parses.
+	assert.equal(cacheKeyUrl('https://x.com/a?q=a%26b%3Dc%3Bd'), 'https://x.com/a?q=a%26b%3Dc%3Bd');
+});
+
+test('cacheKeyUrl accepts a URL object and never throws', () => {
+	assert.equal(cacheKeyUrl(new URL('https://x.com/a?CN=x%3Ay')), 'https://x.com/a?CN=x:y');
+	assert.doesNotThrow(() => cacheKeyUrl('::::not a url'));
+});
+
+// The encoded/decoded COLLAPSE comes from normalizeUrl (sort re-encodes to %3A) then
+// cacheKeyUrl (decode) — the pipeline the serving + scheduling paths use.
+test('normalizeUrl -> cacheKeyUrl collapses encoded and decoded to one key', () => {
+	applyOptions({ url: { queryParams: ['*'] } });
+	const enc = cacheKeyUrl(normalizeUrl('https://x.com/a?CN=Gender%3AMens'));
+	const dec = cacheKeyUrl(normalizeUrl('https://x.com/a?CN=Gender:Mens'));
+	assert.equal(enc, dec);
+	assert.equal(enc, 'https://x.com/a?CN=Gender:Mens');
+});