diff --git a/packages/apps-vtex/src/middleware.cacheHeaders.test.ts b/packages/apps-vtex/src/middleware.cacheHeaders.test.ts new file mode 100644 index 00000000..f9ba47f3 --- /dev/null +++ b/packages/apps-vtex/src/middleware.cacheHeaders.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, it } from "vitest"; +import { vtexMiddleware } from "./mod"; + +/** + * The VTEX app middleware wraps the framework's entire edge-cache layer, so it + * is the last writer of Cache-Control on every response — including cache HITs. + * These tests pin the two things that made that dangerous. + */ + +// A response as the framework's cache layer would hand it over: public headers +// resolved from the page's cache profile, plus the CDN header. +function cachedResponse(): Response { + return new Response("page", { + headers: { + "Cache-Control": "public, max-age=120, s-maxage=900, stale-while-revalidate=1800", + "CDN-Cache-Control": "public, max-age=900", + }, + }); +} + +const next = async () => cachedResponse(); + +describe("vtexMiddleware cache headers", () => { + it("leaves the cache layer's headers alone for an anonymous request", async () => { + const res = await vtexMiddleware(new Request("https://store.com/"), next); + // Used to be downgraded to vtexCacheControl's generic `s-maxage=60`, + // throwing away the profile the cache layer had resolved. + expect(res.headers.get("Cache-Control")).toContain("s-maxage=900"); + }); + + it("forces private headers and clears the CDN header when logged in", async () => { + const req = new Request("https://store.com/", { + // extractVtexContext treats any VtexIdclientAutCookie* as authenticated. + headers: { cookie: "VtexIdclientAutCookie_store=abc123" }, + }); + const res = await vtexMiddleware(req, next); + + expect(res.headers.get("Cache-Control")).toContain("no-store"); + // Cloudflare gives CDN-Cache-Control precedence, so leaving the public + // value behind would cache a personalized page at the CDN regardless of + // the private Cache-Control next to it. + expect(res.headers.get("CDN-Cache-Control")).toBeNull(); + }); +}); diff --git a/packages/apps-vtex/src/mod.ts b/packages/apps-vtex/src/mod.ts index 1df39cda..1ccfb893 100644 --- a/packages/apps-vtex/src/mod.ts +++ b/packages/apps-vtex/src/mod.ts @@ -96,10 +96,26 @@ export interface VtexState { // Middleware // ------------------------------------------------------------------------- -const vtexMiddleware: AppMiddleware = async (request, next) => { +export const vtexMiddleware: AppMiddleware = async (request, next) => { const ctx = extractVtexContext(request); const response = await next(); - response.headers.set("Cache-Control", vtexCacheControl(ctx)); + + // This middleware wraps the framework's whole edge-cache layer, so it is the + // LAST writer of Cache-Control — including on a cache HIT. It used to + // overwrite unconditionally, which downgraded a home page the cache layer had + // resolved as `s-maxage=900` to vtexCacheControl's generic `s-maxage=60`. + // + // Now it only speaks up for the case it actually knows better about: a + // personalized request, which must not be cached anywhere. And when it does, + // it clears CDN-Cache-Control too — otherwise a response can go out as + // `Cache-Control: private, no-store` alongside `CDN-Cache-Control: public, + // max-age=300`, and Cloudflare gives the CDN header precedence. Same pairing + // the worker's own bypasses use, and utils/proxy.ts's hardenProxyCacheHeaders. + if (ctx.isLoggedIn || ctx.hasCustomPricing) { + response.headers.set("Cache-Control", vtexCacheControl(ctx)); + response.headers.delete("CDN-Cache-Control"); + } + propagateISCookies(ctx, response); return response; }; diff --git a/packages/blocks/src/sdk/cacheHeaders.test.ts b/packages/blocks/src/sdk/cacheHeaders.test.ts index 037d0865..efecb7de 100644 --- a/packages/blocks/src/sdk/cacheHeaders.test.ts +++ b/packages/blocks/src/sdk/cacheHeaders.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it } from "vitest"; import { canonicalizeServerFnPayloadForCacheKey, detectCacheProfile, + getCacheProfile, + registerCachePattern, + registerPrivatePaths, serverFnPagePath, + setCacheProfile, } from "./cacheHeaders"; const sfn = (payload: unknown): URL => { @@ -113,3 +117,89 @@ describe("canonicalizeServerFnPayloadForCacheKey — variant-param cache key", ( expect(canonicalizeServerFnPayloadForCacheKey("{not json")).toBe("{not json"); }); }); + +describe("detectCacheProfile — private areas", () => { + it.each([ + "/checkout", + "/checkout/cart", + "/cart", + "/carrinho", + "/minha-conta", + "/meus-pedidos", + "/login", + "/myaccount", + ])("keeps the existing private route %s private", (path) => { + expect(detectCacheProfile(path)).toBe("private"); + }); + + it.each([ + // Every one of these used to fall through to the cacheable `listing` + // default. `/listadedesejos` is the one that bit a live store. + "/listadedesejos", + "/lista-de-desejos", + "/wishlist", + "/favoritos", + "/orders", + "/order-placed", + "/profile", + "/perfil", + "/logout", + "/sair", + "/cadastro", + "/signup", + "/register", + "/assinaturas", + "/trocas", + "/devolucao", + ])("treats %s as private", (path) => { + expect(detectCacheProfile(path)).toBe("private"); + }); + + it("matches case-insensitively", () => { + expect(detectCacheProfile("/Checkout")).toBe("private"); + expect(detectCacheProfile("/MINHA-CONTA")).toBe("private"); + }); + + it("matches behind a locale prefix", () => { + expect(detectCacheProfile("/pt/checkout")).toBe("private"); + expect(detectCacheProfile("/pt-br/minha-conta")).toBe("private"); + }); + + it("does not swallow public routes that merely start with two letters", () => { + expect(detectCacheProfile("/pt/tenis")).toBe("listing"); + expect(detectCacheProfile("/carteiras")).toBe("listing"); + expect(detectCacheProfile("/cartoes-presente")).toBe("listing"); + }); + + it("registerPrivatePaths adds site-specific private routes", () => { + expect(detectCacheProfile("/clube-vip")).toBe("listing"); + registerPrivatePaths(["/clube-vip", "sem-barra"]); + expect(detectCacheProfile("/clube-vip")).toBe("private"); + expect(detectCacheProfile("/clube-vip/beneficios")).toBe("private"); + expect(detectCacheProfile("/sem-barra")).toBe("private"); + // prefix match must respect segment boundaries + expect(detectCacheProfile("/clube-vip-publico")).toBe("listing"); + }); +}); + +describe("cache configuration can tighten, not loosen", () => { + it("refuses to make a non-public profile public", () => { + setCacheProfile("private", { isPublic: true }); + expect(getCacheProfile("private").isPublic).toBe(false); + }); + + it("still allows ordinary tuning of a private profile", () => { + setCacheProfile("private", { loader: { fresh: 1_000 } }); + expect(getCacheProfile("private").loader.fresh).toBe(1_000); + expect(getCacheProfile("private").isPublic).toBe(false); + }); + + it("a custom pattern cannot make a private path public", () => { + // A catch-all site pattern used to win over the built-in private check, + // because custom patterns are evaluated first. + registerCachePattern({ test: () => true, profile: "static" }); + expect(detectCacheProfile("/checkout")).toBe("private"); + // ...but it still applies everywhere else. + expect(detectCacheProfile("/tenis")).toBe("static"); + }); +}); diff --git a/packages/blocks/src/sdk/cacheHeaders.ts b/packages/blocks/src/sdk/cacheHeaders.ts index 62777191..9af2fa39 100644 --- a/packages/blocks/src/sdk/cacheHeaders.ts +++ b/packages/blocks/src/sdk/cacheHeaders.ts @@ -149,15 +149,45 @@ export function setCacheProfile( overrides: CacheProfileOverrides, ): void { const current = PROFILES[profile]; + + // `private`/`cart`/`none` are the profiles that keep authenticated pages off + // the shared edge (and, once CDN caching is on, off the CDN). Flipping one to + // public is how a checkout page ends up served to another visitor, so it + // takes more than an `isPublic: true` in a props bag — see + // `allowPublicPrivateProfile`. + let isPublic = overrides.isPublic ?? current.isPublic; + if (isPublic && !current.isPublic && !publicPrivateProfilesAllowed) { + console.warn( + `[deco] setCacheProfile("${profile}", { isPublic: true }) ignored: making a ` + + `non-public profile public would let authenticated pages be shared between ` + + `visitors. Call allowPublicPrivateProfile() first if this is deliberate.`, + ); + isPublic = current.isPublic; + } + PROFILES[profile] = { edge: { ...current.edge, ...overrides.edge }, browser: { ...current.browser, ...overrides.browser }, loader: { ...current.loader, ...overrides.loader }, client: { ...current.client, ...overrides.client }, - isPublic: overrides.isPublic ?? current.isPublic, + isPublic, }; } +let publicPrivateProfilesAllowed = false; + +/** + * Opt out of the guard in `setCacheProfile` that refuses to turn a non-public + * profile (`private`, `cart`, `none`) public. + * + * There is no legitimate storefront reason to call this. It exists so the + * escape hatch has a name you have to type, rather than being a silent side + * effect of passing `isPublic: true`. + */ +export function allowPublicPrivateProfile(): void { + publicPrivateProfilesAllowed = true; +} + // --------------------------------------------------------------------------- // Derivation: Cache-Control headers (browser layer) // --------------------------------------------------------------------------- @@ -262,16 +292,95 @@ interface CachePattern { profile: CacheProfileName; } -// Authenticated / per-user areas that must never be edge-cached. Includes the -// hyphenless `myaccount` (VTEX My Account wrapper path) and common pt-BR routes -// (`minha-conta`, `meus-pedidos`, `pedidos`) — omitting these let account pages -// fall through to the cacheable `listing` default (see decocms/blocks#412). -const PRIVATE_PREFIX_RE = - /^\/(cart|checkout|account|myaccount|my-account|minha-conta|meus-pedidos|pedidos|login)(\/|$)/; +// Authenticated / per-user areas that must never be edge-cached. Anything not +// matched here falls through to the cacheable `listing` default, so a missing +// entry is a live content leak, not a missed optimization (see +// decocms/blocks#412, which added the hyphenless `myaccount` VTEX wrapper path +// and the pt-BR account routes). +// +// Three properties this regex must keep, each of which was a real hole: +// - case-insensitive: `/Checkout` used to fall through to `listing`. +// - optional locale prefix: `/pt/checkout`, `/br/minha-conta` likewise. Only +// matches when followed by a private segment, so a legitimate two-letter +// route can't be swallowed by it. +// - wishlist / profile / signup / returns: absent until the CDN work, which +// is what served a live store's `/listadedesejos` from the shared entry. +const PRIVATE_SEGMENTS = [ + "cart", + "carrinho", + "checkout", + "account", + "myaccount", + "my-account", + "minha-conta", + "meus-pedidos", + "pedidos", + "orders", + "order-placed", + "login", + "logout", + "sair", + "cadastro", + "signup", + "register", + "profile", + "perfil", + "wishlist", + "favoritos", + "listadedesejos", + "lista-de-desejos", + "minha-lista", + "assinaturas", + "subscriptions", + "troca", + "trocas", + "devolucao", + "devolucoes", +]; + +const LOCALE_PREFIX = "(?:\\/[a-z]{2}(?:-[a-z]{2})?)?"; + +const PRIVATE_PREFIX_RE = new RegExp( + `^${LOCALE_PREFIX}\\/(?:${PRIVATE_SEGMENTS.join("|")})(?:\\/|$)`, + "i", +); + +// Site-registered private prefixes (see `registerPrivatePaths`). Kept separate +// from `PRIVATE_SEGMENTS` so a site can only ever ADD to the private set. +const extraPrivatePaths: string[] = []; + +/** + * Mark additional path prefixes as private — never edge-cached, never served + * from the CDN. + * + * This is the safe half of cache configuration: it can only restrict, never + * relax. Prefer it over `registerCachePattern` (which can also make things + * public, and is evaluated before the built-in private check). + * + * @example + * ```ts + * registerPrivatePaths(["/listadedesejos", "/trocas"]); + * ``` + */ +export function registerPrivatePaths(paths: string[]): void { + for (const path of paths) { + const normalized = path.startsWith("/") ? path : `/${path}`; + if (!extraPrivatePaths.includes(normalized)) extraPrivatePaths.push(normalized); + } +} + +function isPrivatePath(pathname: string): boolean { + if (PRIVATE_PREFIX_RE.test(pathname)) return true; + const lower = pathname.toLowerCase(); + return extraPrivatePaths.some((prefix) => { + const p = prefix.toLowerCase(); + return lower === p || lower.startsWith(`${p}/`); + }); +} const builtinPatterns: CachePattern[] = [ { - test: (p) => PRIVATE_PREFIX_RE.test(p), + test: (p) => isPrivatePath(p), profile: "private", }, { @@ -299,7 +408,13 @@ const customPatterns: CachePattern[] = []; /** * Register additional URL-to-profile patterns. Custom patterns are evaluated - * before built-in ones, so they can override defaults. + * before built-in ones, so they can override defaults — with one exception: + * a custom pattern resolving to a PUBLIC profile cannot override the built-in + * private check (see `detectCacheProfile`). A broad site pattern would + * otherwise silently capture `/checkout` and make it cacheable. + * + * To mark routes as private, prefer {@link registerPrivatePaths} — it can only + * restrict, and isn't subject to ordering rules. */ export function registerCachePattern(pattern: CachePattern): void { customPatterns.push(pattern); @@ -324,8 +439,15 @@ export function detectCacheProfile(pathnameOrUrl: string | URL): CacheProfileNam searchParams = url.searchParams; } + // A private path can never be talked into a public profile by a site pattern. + // Custom patterns still win for anything non-public (a site can always make a + // route MORE restricted), and for public-vs-public overrides on other paths. + const privatePath = isPrivatePath(pathname); + for (const pattern of customPatterns) { - if (pattern.test(pathname, searchParams)) return pattern.profile; + if (!pattern.test(pathname, searchParams)) continue; + if (privatePath && PROFILES[pattern.profile].isPublic) return "private"; + return pattern.profile; } for (const pattern of builtinPatterns) { if (pattern.test(pathname, searchParams)) return pattern.profile; diff --git a/packages/tanstack/src/sdk/workerEntry.test.ts b/packages/tanstack/src/sdk/workerEntry.test.ts index b3d15cdb..8c8d9d1f 100644 --- a/packages/tanstack/src/sdk/workerEntry.test.ts +++ b/packages/tanstack/src/sdk/workerEntry.test.ts @@ -736,3 +736,32 @@ describe("draft preview (pull-based)", () => { expect(res.headers.get("X-Robots-Tag")).toBeNull(); }); }); + +describe("CDN-Cache-Control at the single response exit", () => { + it("defaults to no-store on early returns that never reach dressResponse", async () => { + // `?asJson` returns the fully resolved page — loaders run with the caller's + // cookies — and returns before dressResponse, so it expresses no opinion on + // CDN caching. Without a default here it would inherit whatever the CDN + // decides for a 200 with no cache directives. + const w = createDecoWorkerEntry(MOCK_SERVER_ENTRY, { observability: false }); + const res = await w.fetch( + new Request("https://example.com/some-category?asJson"), + EMPTY_ENV, + MOCK_CTX, + ); + expect(res.headers.get("CDN-Cache-Control")).toBe("no-store"); + }); + + it("leaves a value dressResponse already decided", async () => { + // The cacheable path is the one branch that reasoned about whether the CDN + // key matches the worker key — the default must not stomp it. + const w = createDecoWorkerEntry(MOCK_SERVER_ENTRY, { + observability: false, + cdnCacheControl: "match-profile", + deviceSpecificKeys: false, + geoCacheKey: "off", + }); + const res = await w.fetch(new Request("https://example.com/some-category"), EMPTY_ENV, MOCK_CTX); + expect(res.headers.get("CDN-Cache-Control")).toMatch(/^public, max-age=\d+$/); + }); +}); diff --git a/packages/tanstack/src/sdk/workerEntry.ts b/packages/tanstack/src/sdk/workerEntry.ts index 633c4c6c..e0e05c78 100644 --- a/packages/tanstack/src/sdk/workerEntry.ts +++ b/packages/tanstack/src/sdk/workerEntry.ts @@ -777,11 +777,17 @@ function parseCookieNames(response: Response): string[] { /** * Check if ALL cookies in a response are in the safe list. * Returns true if the response has no cookies or only safe cookies. + * + * Fail-closed: a response that HAS a `set-cookie` we couldn't parse into names + * is treated as unsafe. The parser's fallback path (comma-splitting the + * combined header) is documented as unreliable, and the cost of the two + * outcomes is not symmetric — guessing "safe" here caches a personalized + * response into the shared edge entry. */ function hasOnlySafeCookies(response: Response, safeCookieSet: Set): boolean { if (!response.headers.has("set-cookie")) return true; const names = parseCookieNames(response); - if (names.length === 0) return true; + if (names.length === 0) return false; return names.every((name) => safeCookieSet.has(name)); } @@ -1058,7 +1064,24 @@ export function createDecoWorkerEntry( return out; } - const allBypassPaths = [...(bypassPaths ?? DEFAULT_BYPASS_PATHS), ...extraBypassPaths]; + // DEFAULT_BYPASS_PATHS is always included, even when a site passes its own + // `bypassPaths`. It used to be replaced, so a site adding one path silently + // lost `/deco/`, `/live/` and `/.decofile` — framework routes that must never + // be cached. Cache configuration from a site can tighten, never loosen. + const allBypassPaths = [ + ...new Set([...DEFAULT_BYPASS_PATHS, ...(bypassPaths ?? []), ...extraBypassPaths]), + ]; + + // The logged-in bypass below reads `segment.loggedIn`, which only ever exists + // when the site supplies `buildSegment`. Without it, authenticated and + // anonymous visitors share one edge entry. + if (!rawBuildSegment) { + console.warn( + "[deco] createDecoWorkerEntry: no `buildSegment` configured — logged-in " + + "visitors share the anonymous edge cache entry. Required before enabling " + + "CDN caching.", + ); + } // -- Helpers ---------------------------------------------------------------- @@ -1678,6 +1701,27 @@ export function createDecoWorkerEntry( // invoke handlers, etc.) may independently append the same cookie. deduplicateSetCookies(response); + // `CDN-Cache-Control` is decided here, at the single response exit, so no + // branch can forget it. Two cases: + // + // - `X-Cache: BYPASS` — the worker deliberately declined to cache, so + // the CDN must not either. A dozen call sites set this; some deleted + // the header, some didn't, and several still emitted the profile's + // public `Cache-Control` (`public, s-maxage=900`) on the way out. + // - header absent — a branch that returned before `dressResponse` and + // expressed no opinion. `?asJson`, `?renderJson`, proxied responses and + // the redirect paths all land here. Defaulting these to `no-store` is + // what makes the invariant hold: an early return can only ever be + // MORE restrictive than the cache layer, never accidentally public. + // + // A value already set by `dressResponse` (the cacheable path) is left + // alone — that is the one branch that has actually reasoned about whether + // the CDN key matches the worker key. + const bypassed = response.headers.get("X-Cache") === "BYPASS"; + if (bypassed || !response.headers.has("CDN-Cache-Control")) { + response.headers.set("CDN-Cache-Control", "no-store"); + } + let finalResponse = applySecurityHeaders(response); // Echo request.id + trace.id back to the client / tail worker.