Uh oh!
There was an error while loading. Please reload this page.
feat(plugin): unify URL normalization + the canonical verdict — RFC core hardcoded, site-contingent rules configurable (v0.45.0 / browser v1.17.0) - #94
Conversation
…config; v0.45.0 Two halves of `canonicalizeUrl` were the wrong way round: we decoded three RESERVED characters (`:` `,` `@`) unconditionally — a claim about how one origin parses URLs — while skipping the UNRESERVED decode that RFC 3986 §6.2.2.2 says is equivalent for every origin that exists. So `/%68ello` and `/hello` were two cache keys, two targets and two renders of one resource, on any site, while a deployment whose API reads `?ids=1,2,3` as a list had no way to stop us folding `%2C` into it. Now: - unreserved (`ALPHA / DIGIT / - . _ ~`) always decodes. Not configurable — there is no site for which it is wrong, and it is what Cloudflare/Akamai/Fastly do. - `cacheKey.decodeReserved` (default `[':', ',', '@']`, i.e. today's behavior) carries the contingent half, with structural characters refused by the schema: `&` in that list would decode a separator into every key and reparse the URL. - escape-hex upper-casing stays (§6.2.2.1); `+`/`%20` folding and duplicate-slash collapsing stay out — no standard supports either. Migration: none. Measured across 87,983 sitemap locs (37,983 catalog + 50,000 product), the escapes present are `%20 %26 %27 %2F %24 %3A %2C %2B` and NOT ONE is unreserved, so no existing key changes. The default `decodeReserved` reproduces current behavior exactly. `%2E` needed no special case, which is worth recording because it looks like it should: WHATWG `new URL()` resolves dot segments in their encoded spellings too, so `/%2E%2E/x` is already `/x` before this runs and a surviving `%2E` is inside a real segment. The browser's mirrored copy pins the default set rather than reading config: it never BUILDS a key, only compares two URLs it normalized itself, so a deployment that changes the set does not have to mirror it there. An option that changes which URLs are the SAME key (folding `+` and `%20`, #92) is the opposite and must be mirrored — noted in both. Adds `itemEnum` to the schema validator: `enum` for a list, whole-list rejection, for options where a rogue entry corrupts rather than merely misconfigures. Refs #92
…ent; v1.17.0 A faceted origin resolves `+`, `%2B` and `%20` between facet values to one page and canonicalizes all of them to the `+` spelling — but each spelling is its own cache key. The self-canonical test ran on `normalizeCanonicalUrl`, whose form-encoding round-trip collapses `%20` and `+`, so a `%20`-for-`+` re-spelling read as self-canonical and became a second recurring target rendering the same bytes ~2x/day forever. Every other re-spelling in that family was already caught; this one could not be, by construction. Take the verdict on `canonicalizeUrl` instead. The consequence of a canonical verdict is `Target.suppress`, and a Target IS a cache key, so "same key" is the question worth asking, and asking it through the key-building function means the answer cannot drift from what the plugin stores. `canonicalAllowsIndex` becomes `canonicalVerdict` returning self | variant | elsewhere; 'variant' posts the new reason `canonical-variant` so a wave of duplicate spellings stays legible next to genuine mismatches. The rendered url's own param names are the allowlist for both sides, so a param the route drops can never manufacture a mismatch. NOT solved in the cache key on purpose: `+` and `%2B` are interchangeable only in separator position. In value position they are different values (measured against a live faceted origin: `Brand:ACME%2BCO` returns the brand's 52 products, `Brand:ACME+CO` returns an empty page canonicalizing elsewhere), so folding them would serve one facet's HTML under another facet's url — and re-keying the corpus orphans every cached page. Blast radius, verified in code and pinned by a new test: a sitemap-listed url is serialized even when non-indexable, so its result posts with content and `rendered` wins the outcome — the declared corpus cannot be retired by any canonical verdict, only discovered urls can. Measured on a live faceted origin: 103/103 real sitemap locs (60 catalog facet urls from 3 sitemaps, 30 PDPs, the homepage, 12 re-probed with a mobile UA) are self-canonical under the strict comparison, i.e. zero would-be suppressions. Plugin change is comment-only (the reason string flows through as data) — no release needed. Refs #84
… config
This plugin serves websites of all types, so a rule that depends on how one origin
parses its query cannot be hardcoded. Split the verdict along that line:
- A canonical naming a DIFFERENT document disowns the page. Invariable, every site,
not configurable.
- A canonical naming this very document RE-SPELLED as another cache key ('variant')
is a duplicate only if that origin's parser cannot tell the spellings apart —
a form-decoding origin cannot, an RFC-3986 one can. So: `canonical.strict`,
default false, which reproduces the historical lenient reading byte-for-byte.
The doc comment carries the one-request discriminator that settles it for a given
parameter across every URL (ask for `?f=A%2BB`, then `?f=A+B`; if the origin resolves
the second as a space it form-decodes), so an operator can decide without a crawl.
Tests pin both directions plus the invariable half under either setting.There was a problem hiding this comment.
Code Review
This pull request introduces a configurable decodeReserved option to normalize percent-encoded reserved characters in URLs, allowing different spellings of the same logical URL to resolve to a single cache key. It also ensures unreserved characters are unconditionally decoded per RFC 3986. The feedback suggests optimizing performance by caching the decodeReserved Set at the module level to avoid garbage collection overhead on the hot path. Additionally, the new itemEnum property should be added to the serialization list in describeConfigSchema to ensure the admin UI and management API receive the validation constraints.
| const normalizeEscapes = (s, extra) => | ||
| s.replace(/%[0-9A-Fa-f]{2}/g, (m) => { | ||
| const char = String.fromCharCode(parseInt(m.slice(1), 16)); | ||
| if (UNRESERVED.test(char)) return char; | ||
| return extra.has(char) ? char : m.toUpperCase(); | ||
| }); |
There was a problem hiding this comment.
Creating a new Set on every single call to canonicalizeUrl introduces unnecessary allocation and garbage collection overhead, especially during large-scale operations like sitemap ingestion (which can process millions of URLs). We can optimize this by caching the Set at the module level and updating it dynamically whenever the configuration is reloaded using onConfigApplied.
import{onConfigApplied}from'../config.js';letdecodeReservedSet=newSet(config.cacheKey.decodeReserved);onConfigApplied((fresh)=>{decodeReservedSet=newSet(fresh.cacheKey.decodeReserved);});constnormalizeEscapes=(s)=>s.replace(/%[0-9A-Fa-f]{2}/g,(m)=>{constchar=String.fromCharCode(parseInt(m.slice(1),16));if(UNRESERVED.test(char))returnchar;returndecodeReservedSet.has(char) ? char : m.toUpperCase();});References
- Cache normalized configuration values or computed arrays that are accessed frequently on hot paths to avoid CPU overhead and allocation churn, recomputing them only when the configuration is applied.
There was a problem hiding this comment.
Applied. The set is built once at module scope and rebuilt by onConfigApplied — which runs after enforceSchemaConstraints, so it caches the enforced value rather than the raw one. I kept it as an explicit parameter to normalizeEscapes instead of closing over the module variable (the function stays pure and directly testable), with a ??= for the window before the first applyOptions, since this module can be imported before config is applied. Added a test that a live decodeReserved change still takes effect, so the cache can't outlive a config apply unnoticed.
| const extra = new Set(config.cacheKey.decodeReserved); | ||
| let half = normalizeEscapes(`${parsed.protocol}//${parsed.host}${path}${query}`, extra); |
There was a problem hiding this comment.
Use the cached decodeReservedSet instead of creating a new Set on every invocation of canonicalizeUrl.
| constextra=newSet(config.cacheKey.decodeReserved); | |
| lethalf=normalizeEscapes(`${parsed.protocol}//${parsed.host}${path}${query}`,extra); | |
| lethalf=normalizeEscapes(`${parsed.protocol}//${parsed.host}${path}${query}`); |
References
- Cache normalized configuration values or computed arrays that are accessed frequently on hot paths to avoid CPU overhead and allocation churn, recomputing them only when the configuration is applied.
There was a problem hiding this comment.
Applied — the call site uses the cached set.
| 'the URL into a different shape. Beware list-valued params — an API that reads `?ids=1,2,3` ' + | ||
| 'as three values and `%2C` as a literal comma inside one is a site where `,` must be removed ' + | ||
| 'from this list.', | ||
| { itemType: 'string', itemEnum: [':', ',', '@', ';', '$', "'", '(', ')', '!', '*'] } |
There was a problem hiding this comment.
The newly introduced itemEnum property is missing from the serialization list in describeConfigSchema(). Without adding it to the list of keys copied to the serialized schema, the management API and admin UI will not receive this validation constraint, preventing the UI from displaying or validating the allowed options for decodeReserved.
Please update describeConfigSchema in packages/plugin/src/configSchema.js to include 'itemEnum' in the serialization array.
for(constkeyof['enum','unit','min','max','nonEmpty','itemType','secret','movedFrom','itemEnum']){There was a problem hiding this comment.
Good catch — applied. itemEnum is now in describeConfigSchema's serialization list. It matters more than the usual field here: decodeReserved is one of the few options where a rogue entry corrupts every cache key rather than merely misconfiguring a feature, so the admin UI needs to be able to show what is allowed.
Both from review on #94. The set was rebuilt per canonicalizeUrl call — one allocation per URL on the bot read path and per URL on sitemap ingestion. Cached at module scope and rebuilt by onConfigApplied (which runs after enforceSchemaConstraints, so it caches the enforced value), with a lazy init for the window before the first apply. Test pins that a live change still takes effect. itemEnum was missing from describeConfigSchema's serialization list, so the management API and admin UI would not have seen the constraint on decodeReserved.
…s query From review on #90 — `current` is already a URL, so `new URLSearchParams(current.search)` re-parsed a query string that was parsed a line earlier.
# Conflicts: # package-lock.json # packages/browser/package.json
…ce, mirrored; v0.45.0 Completes the URL/normalization story in one place, so the invariable rules and the site-contingent ones are declared side by side instead of arriving in three releases. cacheKey.trailingSlash: strip|preserve (default strip = today). No standard makes `/a/` and `/a` one resource, and it differs per ROUTE on one site: measured, this deployment's origin 200s a slashed catalog URL and 403s a slashed product URL — so stripping has us answer, on the origin's behalf, with a page it refused. cacheKey.plusIsSpace: false (default). Folds `%20` to `+` in the QUERY only, for an origin that form-decodes — where `+` IS a space and the spellings cannot name different resources. `%2B` is never folded (a literal plus in a value is a different value). The fold runs BEFORE the sort, or it defeats itself: `%20` and `+` sort to different positions, so folding afterwards gives two spellings of one query two keys. Both are mirrored in the browser (`cacheKey` in its config), because they change WHICH URLS ARE THE SAME KEY. A renderer left unfolded compares a folded job URL against a canonical spelled the plugin's way, calls it a duplicate, and retires a healthy page — pinned by a test. The plugin's `decodeReserved` is deliberately NOT mirrored: it changes a key's bytes, but the browser only ever compares two URLs it normalized itself. Shared vectors carry an optional `options` block so both suites assert the same policies.
Everything about how a URL becomes a cache key, in one change: the normalization a CDN does unconditionally, the normalization that is a claim about one site, and the canonical verdict that reads a page against the result. Supersedes #90 and implements the code for #92 and #93.
The organising rule
We serve on behalf of the origin, so for every URL
Uwe accept,origin(N(U))must be the same resource asorigin(U)— we render the representative and serve its bytes for everything that maps to it. Each rule is therefore a claim about origin semantics. The ones that follow from a standard or a parser property are hardcoded; the ones contingent on one site's behavior are config, defaulting to today's behavior.ALPHA / DIGIT / - . _ ~, §6.2.2.2):,@cacheKey.decodeReserved— was hardcoded/a/≡/acacheKey.trailingSlash(defaultstrip= today)%20≡+in the querycacheKey.plusIsSpace(defaultfalse= today)canonical.strict(browser, defaultfalse= today)What was actually wrong
The two halves were reversed. We decoded three reserved characters unconditionally — a claim about one origin's parser — and skipped the unreserved decode that RFC 3986 defines as equivalent for every origin. So
/%68elloand/hellowere two keys, two targets and two renders of one resource on every site, while a deployment whose API reads?ids=1,2,3as a list had no way to stop us folding%2Cinto it.The canonical was read against the wrong unit. The self-canonical test ran on a form-encoding comparison that collapses
%20and+, so a re-spelling read as self-canonical and became a second recurring target for bytes the first key already had. It now reads againstcanonicalizeUrl— the consequence of the verdict isTarget.suppress, and a Target is a cache key, so "same key" is the question worth asking. Verdict split three ways (self/variant/elsewhere), withvariantreported as its own reason,canonical-variant.Trailing-slash folding answers for URLs the origin refuses. Measured on the deployment origin, same minute:
/product/prd-…/…jsp→ 200,/product/prd-…/…jsp/→ 403; the catalog route 200s both. Stripping has us serve a cached 200 for a URL the origin refused, which is exactly what the organising rule forbids — hence a policy, and this deployment should setpreserve.Measured, on the live origin and the production cluster
%20 %26 %27 %2F %24 %3A %2C %2Band not one unreserved escape → the RFC decode re-keys nothing. Migration-free.canonical-mismatch), 85% of it/product/prd-N/product.jspplaceholder slugs — so this mechanism is load-bearing, not theoretical.read_audit_logwindow: 4.7% of discovered catalog URLs write a space as+where the sitemap writes%20. Probing all 27: 19 were self-canonical under the old comparison, i.e. permanent duplicate targets. ~900/day.explainon one pair: the+spelling is a live recurring target; its%20twin has no target at all.+and%2Bare not interchangeable:?CN=Brand:BLACK%2BDECKER→ 52 products, self-canonical;?CN=Brand:BLACK+DECKER→ 0 products, canonicalBrand:BLACK%20DECKER. The origin read a raw+as a space — it form-decodes, which is what makesplusIsSpacesound here and why it is off by default everywhere else.Every contingent option carries the experiment that settles it
plusIsSpaceandcanonical.strictboth hinge on one question — does the origin form-decode its query? — and both doc comments give the discriminator: request a value with a literal plus (?f=A%2BB), then the same value with a raw+(?f=A+B); if the second resolves as a space, it form-decodes. One request per allowlisted parameter, valid for every URL on the site. An operator decides without crawling anything.Safety
renderedwins inRenderJob.outcome— the plugin's suppression branch is unreachable for it. Only discovered URLs are retirable. Pinned by a test so a refactor can't quietly remove it.decodeURIcollapses. Everything structural is reserved underdecodeURIand inert; in this corpus that leaves%20(the target class) and%27(sampled,self).decodeReservedrefuses structural characters via a new genericitemEnumconstraint —&in that list would decode a separator into every key. Whole-list rejection, because a half-applied key policy is worse than the default one.Mirroring, and the trap in it
cacheKey.plusIsSpaceandcacheKey.trailingSlashare mirrored in the browser because they change which URLs are the same key. A renderer left unfolded compares a folded job URL against a canonical spelled the plugin's way, calls it a duplicate and retires a healthy page — enabling the fold on one side alone would retire the folded corpus. Test pins it; deploy the two packages together.decodeReservedis deliberately not mirrored: it changes a key's bytes, but the browser never builds a key — it only compares two URLs it normalized itself, and both sides go through the same function.Tests
Shared vectors extended to 26 and now carry an optional
optionsblock, so both suites assert the same policies. Plugin 658 pass, browser 120 pass, lint and format clean.Closes#90. Implements the code for #92 and #93 — what remains there is operational: decide
trailingSlash: preserve, and schedule theplusIsSpacere-key (~20k catalog URLs, cached pages orphaned).