Repository navigation
fix(vtex): key the legacy PDP cache by pageHref and by the props that change it - #11
Merged
Merged
Conversation
… change it The cache key was built from `req.url` and carried only slug/segment/skuId. Two things followed from that. A page render and a `/live/invoke` call for the same product have different request URLs, so each caller got its own entry for identical data — the mobile app paid the full VTEX round trip for a payload the site had just computed. `intelligentSearch/productListingPage` already solves this by preferring `props.pageHref`; this does the same, and adds `pageHref` to Props. And `similars` toggles `withIsSimilarTo` in the loader but was absent from the key, so whoever asked first decided whether `isSimilarTo` existed for everyone else until the entry expired. Verified live: a call with `similars: false` seeded the entry and a later call with `similars: true` got the product back with `isSimilarTo` missing. `indexingSkus` and the two `advancedConfigs` fields have the same problem and are keyed too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
marcoferreiradev
changed the title
…n range Two ways an attacker-controlled value reaches code that throws, both in the app middleware or in a cacheKey — neither of which the runtime wraps in a try, so a throw in either is a 500. `?sc=` went straight into `segment.channel` and from there into the VTEXSC cookie, where Deno's setCookie rejects anything outside US-ASCII by throwing. `/any-page?sc=<emoji>` returned 500 for the whole page, from a plain URL, no cookie needed. A sales channel is a VTEX integer id, so it is now only taken in that shape — fenced at the source rather than sanitized downstream, because the same value feeds both the cookie and the cache key. And `btoa`, which only accepts latin1, was reached with the segment unescaped in both `serialize` and `getSegmentCacheKeyWithoutUTM`. A `\uXXXX` escape inside the vtex_segment cookie survives atob + JSON.parse as a real code point above 0xFF — only the six utm fields went through removeNonLatin1Chars. Escaping to ASCII before btoa keeps the value intact: the round-trip yields the same string, and a segment that was already ASCII serializes byte-for-byte as before, so no cache entry and no cookie is invalidated. Verified live on a storefront: `?sc=<emoji>`, `?sc=abc` and a forged vtex_segment cookie all return 200 across page render, /deco/render and /live/invoke. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…che keys Three ways the same result was landing in different entries, or different results in the same one. `?page=1` and no page at all are the same state. The PLP cacheKey computes `page` from props and then re-appends every allowed param already in the URL, so one yields `page=1&page=1` and the other `page=1` — a measured 1907ms miss where a 12ms hit belonged. Dropping exact duplicate pairs after the sort is neutral by construction; `filter.colors=azul&filter.colors=verde` survives, which is why this is a dedup and not a `set`. Case is left alone on purpose: the loader reads `PS` and `O` uppercase and `q` lowercase, none of them echoed by the props block, so folding those would hand one page size the other's entry. `productList` keyed by the caller's URL, which for the PDP backfill is the PDP path on our own render, `/deco/render` on the partial and `/live/invoke` from the mobile app. It now honours `pageHref`, the same escape hatch the sibling productListingPage has always had, so a caller that is not the page itself can land on the entry the storefront already produced. A facets list is decided entirely by its props, so its key stops reading the URL — otherwise the key is finer than the fetch and mints one entry per `?q=`. With that closed, the blanket `ctx.isInvoke` veto only survives where the URL still decides the search, which is what kept the backfill paying full price on every PDP. Now that the loader cache is keyed by the module, this key is the only thing separating two callers of the same loader — so `priceFacets`, `similars` and the facets list's own `query` had to go in it.
The website sells on trade policy 1 and the mobile app on policy 5, and the two mirror each other — same catalogue, same prices. The app reaches these loaders over /live/invoke carrying its own vtex_segment, so `channel` in the cache key split every shared path into two entries with byte-identical payloads, and whichever side arrived second paid the VTEX fetch again. Measured on a virgin key, same server, same sequence: the website warms the PLP (miss 2736ms) and the app reads it at channel 5 — miss 5007ms before, hit 3ms after. priceTables and regionId still separate, so Prime and a regionalized shopper keep their own entries. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFJdpNZtWBobnhj3CPPuMj
DEFAULT_SEGMENT carries no campaigns, priceTables, regionId or channelPrivacy, so the anonymous SSR bag omits those fields entirely, while a shopper carrying a VTEX-minted vtex_segment sends the first three as null and channelPrivacy spelled out as "public". Same truth, two JSON strings, two cache entries — and the mobile app always carries that cookie, so every entry it was meant to share with the website's anonymous render depended on a shape it never has. Absent, null and "" are now one state, the same predicate the storefront applies on its own half of the key, and "public" folds into absent so only "private" earns an entry of its own. Measured on virgin keys, same server, same sequence: the website warms the PLP with no cookie and the app reads it carrying a full-shaped segment on channel 5 — miss 1543ms before, hit 3ms after. The negatives still hold: priceTables PRIME misses (956ms), regionId misses (789ms) and channelPrivacy private misses (1112ms), each minting its own entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFJdpNZtWBobnhj3CPPuMj
Three defects the module-keyed loader cache makes reachable. The PLP key took the raw `?page` while the loader subtracts `pageOffset` from it, so with `pageOffset=0` the URL's page 1 and its absence — page 1 and page 0 of the catalogue — normalised onto one key, and the dedup that removed the echoed duplicate took away the accident that had been keeping them apart. Both sides now read the same `pageOf`, and that dedup drops to two lines. `includeOriginalAttributes` was joined with a comma, so `["a,b"]` and `["a","b"]` produced one key for payloads carrying different attributes. The whitelist on a facets list's own `query` was gated on `ctx.isInvoke`, which is true only for a loader calling a loader: a top-level POST /live/invoke arrives with it false, so any phrase minted an entry and an IS search from the public endpoint. The gate is gone, and the comment that claimed `isInvoke` cannot tell the two apart went with it — it contradicted the measurement recorded four lines below it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFJdpNZtWBobnhj3CPPuMj
`pageHref` is a prop, so it arrives from a public POST /live/invoke like any other, and it reaches `new URL` in two places: the key, which the runtime evaluates outside its try, and the loader body, where it is the base of every product URL in the payload. `pageHref: "x"` threw in both — measured HTTP 500, now 200 on the IS list and on the legacy PDP, with the payload's product URLs unchanged. `keyUrlOf` is where that fallback lives, next to the other cache-key helpers, so the three loaders stop each spelling it out. The legacy PDP's `pageHref` also picks up the `@hide true` its sibling already had — it is a caller's contract, not a knob for the admin. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFJdpNZtWBobnhj3CPPuMj
`useCollectionName` rewrites the last pageType from the product's collection, so it changes the payload — and it was the one such prop missing from the key. This storefront has it on for one PLP and off for the others, so a call naming that page with the flag flipped wrote its breadcrumb into the entry the page reads. Measured: false warms (miss 1197ms), true now mints its own (miss 1061ms), and false still hits (10ms). Before, true was served false's entry. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YFJdpNZtWBobnhj3CPPuMj
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The cache key of
vtex/loaders/legacy/productDetailsPage.tswas built fromreq.urland carried onlyslug,segmentandskuId. Two things follow from that.1. Every caller got its own entry
A page render and a
/live/invokecall for the same product have different request URLs —/<slug>/pversus/live/invoke/.... The mobile app paid the full VTEX round trip for a payload the site had just computed.intelligentSearch/productListingPagealready solves this by preferringprops.pageHref. This does the same, and addspageHrefto Props.Measured: with the site having warmed the PDP, the app's call drops to 0.019–0.023s (hit).
2.
similarsdecided for everyoneThe prop toggles
withIsSimilarToinside the loader but was absent from the key. Whoever asked first decided whetherisSimilarToexisted for everyone else until the entry expired.Reproduced before the fix:
After the fix the entries separate:
falsereturns it missing,truereturnsisSimilarTo=4.indexingSkus,advancedConfigs.preferDescriptionandadvancedConfigs.includeOriginalAttributeshave exactly the same problem and are keyed alongside.Context
Companion to oficina-dev/deco-runtime#2, which makes
resolver=in the cache key identify the module rather than the path walked to it. Without that one, this fix alone is not enough for sharing; without this one, the PDP stays out even with that one.🤖 Generated with Claude Code