Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/vinext/src/entries/app-rsc-entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,8 @@ import {
import {
APP_INTERCEPTION_CONTEXT_KEY as __APP_INTERCEPTION_CONTEXT_KEY,
createAppPayloadRouteId as __createAppPayloadRouteId,
parseSkipHeader as __parseSkipHeader,
X_VINEXT_ROUTER_SKIP_HEADER as __X_VINEXT_ROUTER_SKIP_HEADER,
} from ${JSON.stringify(appElementsPath)};
import {
buildAppPageElements as __buildAppPageElements,
Expand Down Expand Up @@ -492,6 +494,7 @@ function __pageCacheTags(pathname, extraTags) {
}
return tags;
}
const __EMPTY_SKIP_LAYOUT_IDS = new Set();
// Note: cache entries are written with \`headers: undefined\`. Next.js stores
// response headers (e.g. set-cookie from cookies().set() during render) in the
// cache entry so they can be replayed on HIT. We don't do this because:
Expand Down Expand Up @@ -1420,6 +1423,9 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
}

const isRscRequest = pathname.endsWith(".rsc") || request.headers.get("accept")?.includes("text/x-component");
const __skipLayoutIds = isRscRequest
? __parseSkipHeader(request.headers.get(__X_VINEXT_ROUTER_SKIP_HEADER))
: __EMPTY_SKIP_LAYOUT_IDS;
// Read mounted-slots header once at the handler scope and thread it through
// every buildPageElements call site. Previously both the handler and
// buildPageElements read and normalized it independently, which invited
Expand Down Expand Up @@ -2206,6 +2212,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
isrSet: __isrSet,
mountedSlotsHeader: __mountedSlotsHeader,
revalidateSeconds,
skipIds: __skipLayoutIds,
renderFreshPageForCache: async function() {
// Re-render the page to produce fresh HTML + RSC data for the cache
// Use an empty headers context for background regeneration — not the original
Expand Down Expand Up @@ -2420,6 +2427,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
isForceStatic,
isProduction: process.env.NODE_ENV === "production",
isRscRequest,
supportsFilteredRscStream: false,
isrDebug: __isrDebug,
isrHtmlKey: __isrHtmlKey,
isrRscKey: __isrRscKey,
Expand Down Expand Up @@ -2468,6 +2476,7 @@ async function _handleRequest(request, __reqCtx, _mwCtx) {
},
revalidateSeconds,
mountedSlotsHeader: __mountedSlotsHeader,
requestedSkipLayoutIds: __skipLayoutIds,
renderErrorBoundaryResponse(renderErr) {
return renderErrorBoundaryPage(route, renderErr, isRscRequest, request, params, _scriptNonce);
},
Expand Down
46 changes: 46 additions & 0 deletions packages/vinext/src/server/app-elements.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,52 @@ export function resolveVisitedResponseInterceptionContext(
return payloadInterceptionContext ?? requestInterceptionContext;
}

export const X_VINEXT_ROUTER_SKIP_HEADER = "X-Vinext-Router-Skip";
export const X_VINEXT_MOUNTED_SLOTS_HEADER = "X-Vinext-Mounted-Slots";
const MAX_SKIP_LAYOUT_IDS = 50;
const EMPTY_SKIP_HEADER_IDS: ReadonlySet<string> = new Set<string>();

export function parseSkipHeader(header: string | null): ReadonlySet<string> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header parsing accepts any layout:* prefixed value without length or character validation. A malicious client could send a very long header with many entries to force the server to allocate a large Set. Since this runs on every RSC request (when the header is present), consider adding a cap — e.g., bail out or truncate after N entries (say 50, since deeply nested layout trees beyond that would be unusual).

This is low severity since Cloudflare Workers already enforce request header size limits, but defense-in-depth at the application layer doesn't hurt.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. parseSkipHeader now uses a bounded parser and stops after 50 unique layout:* ids, which keeps the allocation surface capped even if a client sends a pathological skip header. Added a test for the cap.

if (!header) return EMPTY_SKIP_HEADER_IDS;
const ids = new Set<string>();
for (const part of header.split(",")) {
const trimmed = part.trim();
if (trimmed.startsWith("layout:")) {
ids.add(trimmed);
if (ids.size >= MAX_SKIP_LAYOUT_IDS) {
break;
}
}
}
return ids.size > 0 ? ids : EMPTY_SKIP_HEADER_IDS;
}

const EMPTY_SKIP_DECISION: ReadonlySet<string> = new Set<string>();

/**
* Pure: computes the authoritative set of layout ids that should be omitted
* from the outgoing payload. Defense-in-depth — an id is only included if the
* server independently classified it as `"s"` (static). Empty or missing
* `requested` yields a shared empty set so the hot path does not allocate.
*
* See `LayoutFlags` type docblock in this file for lifecycle.
*/
export function computeSkipDecision(
layoutFlags: LayoutFlags,
requested: ReadonlySet<string> | undefined,
): ReadonlySet<string> {
if (!requested || requested.size === 0) {
return EMPTY_SKIP_DECISION;
}
const decision = new Set<string>();
for (const id of requested) {
if (layoutFlags[id] === "s") {
decision.add(id);
}
}
return decision.size > 0 ? decision : EMPTY_SKIP_DECISION;
}

export function normalizeAppElements(elements: AppWireElements): AppElements {
let needsNormalization = false;
for (const [key, value] of Object.entries(elements)) {
Expand Down
13 changes: 12 additions & 1 deletion packages/vinext/src/server/app-page-cache.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import type { CachedAppPageValue } from "../shims/cache.js";
import { buildAppPageCacheValue, type ISRCacheEntry } from "./isr-cache.js";
import { wrapRscBytesForResponse } from "./app-page-skip-filter.js";

const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>();

type AppPageDebugLogger = (event: string, detail: string) => void;
type AppPageCacheGetter = (key: string) => Promise<ISRCacheEntry | null>;
Expand All @@ -22,6 +25,7 @@ export type BuildAppPageCachedResponseOptions = {
isRscRequest: boolean;
mountedSlotsHeader?: string | null;
revalidateSeconds: number;
skipIds?: ReadonlySet<string>;
};

export type ReadAppPageCacheResponseOptions = {
Expand All @@ -37,6 +41,7 @@ export type ReadAppPageCacheResponseOptions = {
revalidateSeconds: number;
renderFreshPageForCache: () => Promise<AppPageCacheRenderResult>;
scheduleBackgroundRegeneration: AppPageBackgroundRegenerator;
skipIds?: ReadonlySet<string>;
};

export type FinalizeAppPageHtmlCacheResponseOptions = {
Expand Down Expand Up @@ -106,7 +111,11 @@ export function buildAppPageCachedResponse(
rscHeaders["X-Vinext-Mounted-Slots"] = options.mountedSlotsHeader;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of EMPTY_SKIP_SET as the fallback — wrapRscBytesForResponse short-circuits on empty set and returns the raw ArrayBuffer without creating a ReadableStream. The cache-hit hot path stays allocation-free when no skip header is present.

return new Response(cachedValue.rscData, {
// Cached bytes stay canonical. The current request's skipIds decide
// whether this client gets the full payload or a filtered egress view.
const body = wrapRscBytesForResponse(cachedValue.rscData, options.skipIds ?? EMPTY_SKIP_SET);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cache-read path applies skipIds to the cached canonical bytes here. This is correct for the response sent to the current client, but I want to confirm: the skipIds used here come from the current request's X-Vinext-Router-Skip header (threaded from the generated entry → readAppPageCacheResponsebuildAppPageCachedResponse), not from the request that originally populated the cache. This is the right behavior — each client gets a response filtered to its own skip set — just want to make sure this is documented as intentional since it could be confusing during debugging (same cache entry, different response bytes for different clients).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documented. Added an inline comment on the cache-read path clarifying that the cached bytes stay canonical and the current request’s skipIds only shape the egress response for this client. That matches the intended canonical-cache / per-client-response split.


return new Response(body, {
status,
headers: rscHeaders,
});
Expand Down Expand Up @@ -142,6 +151,7 @@ export async function readAppPageCacheResponse(
isRscRequest: options.isRscRequest,
mountedSlotsHeader: options.mountedSlotsHeader,
revalidateSeconds: options.revalidateSeconds,
skipIds: options.skipIds,
});

if (hitResponse) {
Expand Down Expand Up @@ -198,6 +208,7 @@ export async function readAppPageCacheResponse(
isRscRequest: options.isRscRequest,
mountedSlotsHeader: options.mountedSlotsHeader,
revalidateSeconds: options.revalidateSeconds,
skipIds: options.skipIds,
});

if (staleResponse) {
Expand Down
30 changes: 25 additions & 5 deletions packages/vinext/src/server/app-page-render.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import type { ReactNode } from "react";
import type { CachedAppPageValue } from "../shims/cache.js";
import { buildOutgoingAppPayload, type AppOutgoingElements } from "./app-elements.js";
import {
buildOutgoingAppPayload,
computeSkipDecision,
type AppOutgoingElements,
} from "./app-elements.js";
import {
finalizeAppPageHtmlCacheResponse,
scheduleAppPageRscCacheWrite,
} from "./app-page-cache.js";
import { createSkipFilterTransform } from "./app-page-skip-filter.js";
import {
buildAppPageFontLinkHeader,
resolveAppPageSpecialError,
Expand Down Expand Up @@ -32,6 +37,8 @@ import {
type AppPageSsrHandler,
} from "./app-page-stream.js";

const EMPTY_SKIP_SET: ReadonlySet<string> = new Set<string>();

type AppPageBoundaryOnError = (
error: unknown,
requestInfo: unknown,
Expand Down Expand Up @@ -68,6 +75,7 @@ export type RenderAppPageLifecycleOptions = {
isForceStatic: boolean;
isProduction: boolean;
isRscRequest: boolean;
supportsFilteredRscStream?: boolean;
isrDebug?: AppPageDebugLogger;
isrHtmlKey: (pathname: string) => string;
isrRscKey: (pathname: string, mountedSlotsHeader?: string | null) => string;
Expand Down Expand Up @@ -97,6 +105,7 @@ export type RenderAppPageLifecycleOptions = {
waitUntil?: (promise: Promise<void>) => void;
element: ReactNode | Readonly<Record<string, ReactNode>>;
classification?: LayoutClassificationOptions | null;
requestedSkipLayoutIds?: ReadonlySet<string>;
};

function buildResponseTiming(
Expand Down Expand Up @@ -148,9 +157,9 @@ export async function renderAppPageLifecycle(

const layoutFlags = preRenderResult.layoutFlags;

// Render the CANONICAL element. The outgoing payload carries per-layout
// static/dynamic flags under `__layoutFlags` so the client can later tell
// which layouts are safe to skip on subsequent navigations.
// Always render the CANONICAL element. Skip semantics are applied on the
// egress branch only so the cache branch receives full bytes regardless of
// the client's skip header. See `app-page-skip-filter.ts`.
const outgoingElement = buildOutgoingAppPayload({
element: options.element,
layoutFlags,
Expand All @@ -172,9 +181,17 @@ export async function renderAppPageLifecycle(
revalidateSeconds !== Infinity &&
!options.isForceDynamic,
);
const rscForResponse = rscCapture.responseStream;
const isrRscDataPromise = rscCapture.capturedRscDataPromise;

const skipIds =
options.isRscRequest && (options.supportsFilteredRscStream ?? false)
? computeSkipDecision(layoutFlags, options.requestedSkipLayoutIds)
: EMPTY_SKIP_SET;
const rscForResponse =
skipIds.size > 0
? rscCapture.responseStream.pipeThrough(createSkipFilterTransform(skipIds))
: rscCapture.responseStream;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth noting for future readers: rscForResponse is also used for the HTML SSR path (line 261 in this file), but skipIds will always be empty when !isRscRequest, so the HTML path always receives the unfiltered stream. The code flow is correct; a brief inline comment at the HTML usage site could save someone from a double-take.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added an inline comment at the HTML SSR call site clarifying that rscForResponse is still canonical there because skipIds only becomes non-empty for RSC requests.

Comment on lines +190 to +193
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So from what I gather, it sounds like we render the entire tree, then skip returning parts of that tree to client? Wouldn't we want to skip rendering those before-hand to reduce server work? I believe both Next.js and Waku would skip before the rendering.


if (options.isRscRequest) {
const dynamicUsedDuringBuild = options.consumeDynamicUsage();
const rscResponsePolicy = resolveAppPageRscResponsePolicy({
Expand Down Expand Up @@ -241,6 +258,9 @@ export async function renderAppPageLifecycle(
return renderAppPageHtmlStream({
fontData,
navigationContext: options.getNavigationContext(),
// HTML SSR always consumes the canonical stream: skipIds is only
// non-empty for RSC requests, so the HTML path never sees filtered
// bytes even though it reuses the same `rscForResponse` handle.
rscStream: rscForResponse,
scriptNonce: options.scriptNonce,
ssrHandler,
Expand Down
Loading
Loading