fix(app): keep client trace metadata out of shared HTML cache entries - #2755
fix(app): keep client trace metadata out of shared HTML cache entries#2755NathanDrake2406 wants to merge 8 commits into
Conversation
`experimental.clientTraceMetadata` renders request-scoped OpenTelemetry propagation entries (traceparent, baggage) as `<meta>` tags in the App Router SSR head. Runtime cacheable responses are teed into the App Page HTML cache verbatim, and reading the OTel context is not recorded as dynamic usage, so the generating request's trace context was stored and replayed to every later cache HIT — leaking one user's trace/baggage correlation data (tenant, user id) into another user's HTML. Pages Router already gates emission on route staticness (`shouldEmitPagesClientTraceMetadata`); App Router had no equivalent. Strip the emitted tags at the single point where runtime HTML becomes a shared cache entry, and stop the ISR/PPR cache-artifact render from emitting them at all. The live response still carries the generating request's trace context; the cache entry carries none, which is correct — a cache HIT has no request whose trace it could correlate.
commit: |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1d786bf33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Performance benchmarksCompared 1 improved · 0 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Statically importing client-trace-metadata pulled the feature-specific module into the shared App Router RSC server graph even when experimental.clientTraceMetadata is unset. Gate on the allow-list and import the stripping helper only for configured cache writes.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: client trace metadata cache isolation
The security invariant — HTML stored in a shared cache must contain no request-scoped trace metadata — is correctly enforced at all three storage boundaries:
- Origin-managed ISR: emit-with-marker on the live response + strip-by-marker on the tee'd cache copy. The random per-render UUID marker makes it unguessable, so framework-injected tags are removed without touching application-authored
<meta name="baggage">tags. Verified the marked block is spliced into the head as one contiguous raw string (createTickBufferedTransformenqueuesinjectHTML()unescaped) and the finalizer strips against the fully-bufferedreadStreamAsText, soindexOfreliably finds both markers. - Edge-managed adapters + dev: suppress emission before render for anything that can enter a shared cache.
renderAppPageCacheArtifacts(ISR/PPR fill) no longer receives the allow-list at all — correct, since its output is a shared artifact and, on SWR, the served-from-cache HTML. - Known no-store retention is conservatively gated by
!middlewareMayControlSharedCaching, so middleware that can promote to a shared-cache policy forces suppression.
The lazy import("./client-trace-metadata.js") behind the marker check keeps the feature module out of the RSC graph for the common unconfigured case (addressing Codex's P1). Tests cover the emit/suppress/mark matrix well, including the middleware-promotion and force-static-edge edge cases, and the regression asserts the stored entry is clean while the app-authored tag survives.
No blocking issues. A few minor, non-blocking observations below.
| canSanitizeSeparateOriginCacheCopy && | ||
| clientTraceMetadata && | ||
| clientTraceMetadata.length > 0 | ||
| ? crypto.randomUUID() |
There was a problem hiding this comment.
Nit: crypto.randomUUID() is used here against the global crypto, whereas shims/headers.ts:1086 guards with if (crypto && typeof crypto.randomUUID === "function") before calling it. Global crypto is present in all currently-supported runtimes (Node 18+, Workers), so this is not a correctness problem — just an inconsistency worth noting. node:crypto's randomUUID (already imported in dev-server.ts / app-rsc-entry.ts) would match the repo's prevailing pattern.
| if (options.clientTraceMetadataMarker) { | ||
| const { stripClientTraceMetadataBlock } = await import("./client-trace-metadata.js"); | ||
| cachedHtml = stripClientTraceMetadataBlock(cachedHtml, options.clientTraceMetadataMarker); | ||
| } |
There was a problem hiding this comment.
Minor: the dynamic import + strip runs before the dynamic-usage skip check (lines 158-164) and the cache-policy check (171-174). When the write is ultimately skipped, this does the module load and string scan for nothing. The marker is only set in the origin-adapter production path where a write is usually expected, so the impact is small — but moving the strip below the skip checks would avoid the work on the skip paths. Not blocking.
| const middlewareMayControlSharedCaching = | ||
| options.middlewareContext.headers?.has("cache-control") === true || | ||
| options.middlewareContext.headers?.has("cdn-cache-control") === true || | ||
| options.middlewareContext.headers?.has("cloudflare-cdn-cache-control") === true; |
There was a problem hiding this comment.
The middlewareMayControlSharedCaching gate checks cache-control, cdn-cache-control, and cloudflare-cdn-cache-control. Standard CDN cache-control also includes vercel-cdn-cache-control (Next.js honors it in its cache-control resolution). Since vinext targets Cloudflare this is a low-priority gap, but if middleware sets only Vercel-CDN-Cache-Control to promote shared caching, this gate would miss it and trace metadata could be retained. Worth a follow-up or a short comment noting the intentional scope.
|
Review posted successfully. Summary of my review of PR #2755: Verdict: No blocking issues — the fix is correct. The PR enforces the invariant that shared-cache HTML must contain no request-scoped OpenTelemetry trace metadata, closing a cross-user leakage / cache-poisoning vector. I verified the logic at all three storage boundaries:
The lazy import addresses Codex's earlier P1 about keeping the feature module out of the RSC graph. Tests cover the matrix thoroughly. Three minor, non-blocking notes posted inline:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: keep client trace metadata out of shared HTML cache
I traced the fix through all three storage boundaries and the security invariant — HTML stored in a shared cache must contain no request-scoped trace metadata — holds:
- Origin-managed ISR (marker path): For every path where the default
DefaultCdnCacheAdapter(ownsBackgroundRevalidation = true) can reachfinalizeAppPageHtmlCacheResponsewithshouldWriteToCache/speculative write,canSanitizeSeparateOriginCacheCopyis true and a marker is set, so the tee'd cache copy is stripped. I checked therevalidateSeconds > 0,revalidateSeconds === Infinity, and non-edgeisForceStaticpolicy branches (app-page-response.ts:267-306) — all get a marker. ThepeekDynamicUsage()timing gap is safe: a render that turns dynamic during streaming still had a marker set at gate time (pre-renderpeekDynamicUsage()was false), so the finalizer strips; a render dynamic at gate time yieldsshouldWriteToCache: falseand blocks the speculative write viadynamicUsedDuringRender, so the finalizer never runs. - Edge/dev adapters + retention: With a non-origin adapter (
ownsBackgroundRevalidation = false),canSanitizeSeparateOriginCacheCopyis false, so anything not provably no-store suppresses emission before render.renderAppPageCacheArtifactsno longer receives the allow-list at all — correct, since its output is a shared artifact (and the SWR served-from-cache HTML). - RSC cache copy: trace metadata is HTML-head-only, never in the RSC payload, so the
capturedRscDataPromisewrite needs no stripping. Confirmed. - Pages Router parity: already gated via
shouldEmitPagesClientTraceMetadata(getStaticProps → false), so its cache stays clean. No parity gap introduced.
On the earlier ask-bonk note that the strip runs before the skip checks: that's already handled — the dynamic import + strip at app-page-cache-finalizer.ts:172 sits after both the dynamic-usage skip (151-157) and the cache-policy skip (164-167), and the comment says so. No change needed there.
No blocking issues. One minor consistency nit inline.
| if (!hasKnownNoStoreHtmlPolicy && !canSanitizeSeparateOriginCacheCopy) { | ||
| clientTraceMetadata = undefined; | ||
| } else if (!hasKnownNoStoreHtmlPolicy && canSanitizeSeparateOriginCacheCopy) { | ||
| clientTraceMetadataMarker = crypto.randomUUID(); |
There was a problem hiding this comment.
Minor (non-blocking) consistency nit: every other server module uses import { randomUUID } from "node:crypto" (see dev-server.ts:3, entries/app-rsc-entry.ts:10, config/next-config.ts:11), and shims/headers.ts:1086 guards the global before calling crypto.randomUUID(). This is the only place calling the unguarded global crypto.randomUUID(). Not a correctness problem (global crypto is present in Node 18+ and Workers), just an inconsistency with the repo's prevailing pattern.
| clientTraceMetadataMarker = crypto.randomUUID(); | |
| clientTraceMetadataMarker = randomUUID(); |
(with import { randomUUID } from "node:crypto"; added to the imports).
|
Review posted successfully to PR #2755. SummaryVerdict: No blocking issues — the fix is correct. I traced the security fix through all three storage boundaries and verified the invariant (shared-cache HTML must contain no request-scoped OTel trace metadata) holds:
I specifically verified the I also confirmed the earlier ask-bonk note about the strip running before the skip checks was already addressed — the strip at One minor, non-blocking inline nit: |
2 participants
Problem
experimental.clientTraceMetadatarenders request-scoped OpenTelemetry propagation entries (traceparent,baggage, …) as<meta>tags in the App Router SSR head. Runtime cacheable App Router responses are teed and stored as full HTML byfinalizeAppPageHtmlCacheResponse, and reading the OTel propagation context is not recorded as dynamic request usage — so the cache-write skip never fires.Result: on a cache miss, the generating request's trace context is baked into the stored HTML and replayed to every later cache HIT, until the entry expires or is revalidated. Where operators put tenant/user correlation data in baggage, that is cross-user leakage of one request's identifiers into unrelated users' pages; it is also a cache-poisoning vector, since the first requester can influence the injected
traceparent/baggagevia incoming headers.Pages Router already gates emission on route staticness (
shouldEmitPagesClientTraceMetadatareturnsfalseforgetStaticProps). App Router had no equivalent gate.Triage evidence
client-trace-metadata.tssuppresses emission only underVINEXT_PRERENDER=1; there is no runtime cache-generation suppression.app-ssr-entry.tsappendsgetClientTraceMetadataHTML()into the head injection.app-page-render.tsroutes cacheable runtime HTML intofinalizeAppPageHtmlCacheResponse, which reads the tee'd stream and stores it after only dynamic-usage and cache-policy checks.app-page-cache-render.ts(ISR revalidation / PPR fallback shell) also passed the allow-list into a render whose entire purpose is producing a shared cache artifact.main: the stored entry containedcontent="tenant=alice,user=alice-123".Upstream comparison: Next.js v16.2.6 emits client trace metadata for dynamically server-rendered App and Pages routes, while its production static routes omit it. Vinext adds a separate runtime HTML-cache boundary: a live request response can later become a shared artifact. The vinext global-registry path also synthesizes a span when none is active, so cacheable HTML can contain a
traceparentin setups where Next.js would emit nothing.Fix
Enforce the invariant HTML stored in a shared cache contains no request-scoped trace metadata at every App Router storage boundary:
finalizeAppPageHtmlCacheResponseremoves only the per-render, privately marked vinext trace-metadata block from the cache copy. Application-authored<meta>tags with the same names are preserved. The live response the generating request receives is unchanged.renderAppPageCacheArtifacts(ISR/PPR cache-fill render) no longer receives the allow-list at all — that render exists solely to produce a shared artifact.markClientTraceMetadataBlockandstripClientTraceMetadataBlocklive next to the renderer. For origin-managed ISR, a random marker carries injection provenance across the SSR stream/cache tee, so cleanup cannot confuse framework-injected tags with application metadata. The common unset path still avoids loading the feature module.Why strip rather than suppress emission
Origin-managed ISR has a separate cache tee, so stripping preserves trace metadata on the generating response while keeping stored HTML clean. Edge-managed adapters and cacheable development responses expose the outgoing bytes directly, so potentially shared responses suppress metadata before rendering. Known no-store responses keep the feature only when middleware cannot override or add shared-cache directives. This conservative split is necessary because final cacheability can resolve only after the stream drains.
Verification
vp test run tests/client-trace-metadata.test.ts tests/app-page-cache.test.ts tests/app-page-cache-render.test.ts— 60 passed.vp test run tests/app-page-render.test.ts tests/app-page-dispatch.test.ts tests/app-page-stream.test.ts tests/app-page-response.test.ts tests/otel-tracer-extension.test.ts— 200 passed. (One unhandledclient disconnectrejection appears here; verified pre-existing on cleanupstream/main.)vp test run tests/cloudflare-cdn-cache.test.ts— 16 passed.<meta name="baggage">remains intact.vp checkplus the staged unit/integration suites and knip ran via the pre-commit hook and passed.