Verify cache-busting param during segment prefetch

Not all CDNs respect the Vary header when caching. We must assume that
only the URL is used to vary the responses. The Next client computes a
hash of the header values and sends it as a search param.

Before responding to a request, we must verify that the hash matches the
expected value. Neglecting to do this properly can lead to cache
poisoning attacks on certain CDNs.

In this initial PR, the verification I've added only runs during per-
segment prefetch requests, since those are the only ones that both vary
on a custom header and are also cacheable. But for safety, we should run
this verification for all requests, once we confirm the behavior is
correct. Will need to update our test suite, since there are a handlful
of unit tests that send fetch requests with custom headers but without a
corresponding cache-busting search param.

It would be even better if we stopped using custom request headers all,
and instead fully encoded everything into the search param. At least for
cacheable requests.

Author: acdlite

packages/next/src/client/components/router-reducer/set-cache-busting-search-param.ts

@@ -1,5 +1,6 @@
 'use client'
-import { hexHash } from '../../../shared/lib/hash'
+
+import { computeCacheBustingSearchParam } from '../../../shared/lib/router/utils/cache-busting-search-param'
 import {
   NEXT_ROUTER_PREFETCH_HEADER,
   NEXT_ROUTER_SEGMENT_PREFETCH_HEADER,
@@ -29,14 +30,17 @@ export const setCacheBustingSearchParam = (
   url: URL,
   headers: RequestHeaders
 ): void => {
-  const uniqueCacheKey = hexHash(
-    [
-      headers[NEXT_ROUTER_PREFETCH_HEADER] || '0',
-      headers[NEXT_ROUTER_SEGMENT_PREFETCH_HEADER] || '0',
-      headers[NEXT_ROUTER_STATE_TREE_HEADER],
-      headers[NEXT_URL],
-    ].join(',')
+  const uniqueCacheKey = computeCacheBustingSearchParam(
+    headers[NEXT_ROUTER_PREFETCH_HEADER],
+    headers[NEXT_ROUTER_SEGMENT_PREFETCH_HEADER],
+    headers[NEXT_ROUTER_STATE_TREE_HEADER],
+    headers[NEXT_URL]
   )
+  if (uniqueCacheKey === null) {
+    // None of our custom request headers are present. We don't need to set a
+    // cache-busting search param.
+    return
+  }
 
   /**
    * Note that we intentionally do not use `url.searchParams.set` here:

packages/next/src/server/base-server.ts

@@ -105,6 +105,7 @@ import {
   NEXT_DID_POSTPONE_HEADER,
   NEXT_URL,
   NEXT_IS_PRERENDER_HEADER,
+  NEXT_ROUTER_STATE_TREE_HEADER,
 } from '../client/components/app-router-headers'
 import type {
   MatchOptions,
@@ -177,6 +178,7 @@ import { InvariantError } from '../shared/lib/invariant-error'
 import { decodeQueryPathParameter } from './lib/decode-query-path-parameter'
 import { getCacheHandlers } from './use-cache/handlers'
 import { fixMojibake } from './lib/fix-mojibake'
+import { computeCacheBustingSearchParam } from '../shared/lib/router/utils/cache-busting-search-param'
 
 export type FindComponentsResult = {
   components: LoadComponentsReturnType
@@ -2063,6 +2065,44 @@ export default abstract class Server<
     const hasGetInitialProps = !!components.Component?.getInitialProps
     let isSSG = !!components.getStaticProps
 
+    // Not all CDNs respect the Vary header when caching. We must assume that
+    // only the URL is used to vary the responses. The Next client computes a
+    // hash of the header values and sends it as a search param. Before
+    // responding to a request, we must verify that the hash matches the
+    // expected value. Neglecting to do this properly can lead to cache
+    // poisoning attacks on certain CDNs.
+    // TODO: This is verification only runs during per-segment prefetch
+    // requests, since those are the only ones that both vary on a custom
+    // header and are cacheable. But for safety, we should run this
+    // verification for all requests, once we confirm the behavior is correct.
+    // Will need to update our test suite, since there are a handlful of unit
+    // tests that send fetch requests with custom headers but without a
+    // corresponding cache-busting search param.
+    // TODO: Consider not using custom request headers at all, and instead fully
+    // encode everything into the search param.
+    if (
+      this.isAppSegmentPrefetchEnabled &&
+      getRequestMeta(req, 'segmentPrefetchRSCRequest')
+    ) {
+      const headers = req.headers
+      const expectedHash = computeCacheBustingSearchParam(
+        headers[NEXT_ROUTER_PREFETCH_HEADER.toLowerCase()],
+        headers[NEXT_ROUTER_SEGMENT_PREFETCH_HEADER.toLowerCase()],
+        headers[NEXT_ROUTER_STATE_TREE_HEADER.toLowerCase()],
+        headers[NEXT_URL.toLowerCase()]
+      )
+      if (expectedHash !== null) {
+        const actualHash = getRequestMeta(req, 'cacheBustingSearchParam')
+        if (expectedHash !== actualHash) {
+          // The hash sent by the client does not match the expected value.
+          // Respond with an error.
+          res.statusCode = 400
+          res.body('Bad Request').send()
+          return null
+        }
+      }
+    }
+
     // Compute the iSSG cache key. We use the rewroteUrl since
     // pages with fallback: false are allowed to be rewritten to
     // and we need to look up the path by the rewritten path
@@ -3820,6 +3860,11 @@ export default abstract class Server<
     let page = pathname
     const bubbleNoFallback =
       getRequestMeta(ctx.req, 'bubbleNoFallback') ?? false
+    addRequestMeta(
+      ctx.req,
+      'cacheBustingSearchParam',
+      query[NEXT_RSC_UNION_QUERY]
+    )
     delete query[NEXT_RSC_UNION_QUERY]
 
     const options: MatchOptions = {

packages/next/src/server/request-meta.ts

@@ -89,6 +89,17 @@ export interface RequestMeta {
    */
   isRSCRequest?: true
 
+  /**
+   * A search param set by the Next.js client when performing RSC requests.
+   * Because some CDNs do not vary their cache entries on our custom headers,
+   * this search param represents a hash of the header values. For any cached
+   * RSC request, we should verify that the hash matches before responding.
+   * Otherwise this can lead to cache poisoning.
+   * TODO: Consider not using custom request headers at all, and instead encode
+   * everything into the search param.
+   */
+  cacheBustingSearchParam?: string
+
   /**
    * True when the request is for the `/_next/data` route using the pages
    * router.

packages/next/src/shared/lib/router/utils/cache-busting-search-param.ts

@@ -0,0 +1,25 @@
+import { hexHash } from '../../hash'
+
+export function computeCacheBustingSearchParam(
+  prefetchHeader: string | string[] | undefined,
+  segmentPrefetchHeader: string | string[] | undefined,
+  stateTreeHeader: string | string[] | undefined,
+  nextUrlHeader: string | string[] | undefined
+): string | null {
+  if (
+    prefetchHeader === undefined &&
+    segmentPrefetchHeader === undefined &&
+    stateTreeHeader === undefined &&
+    nextUrlHeader === undefined
+  ) {
+    return null
+  }
+  return hexHash(
+    [
+      prefetchHeader || '0',
+      segmentPrefetchHeader || '0',
+      stateTreeHeader || '0',
+      nextUrlHeader || '0',
+    ].join(',')
+  )
+}

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/about/page.jsx

@@ -0,0 +1,11 @@
+export default function RootLayout({
+  children,
+}: {
+  children: React.ReactNode
+}) {
+  return (
+    <html lang="en">
+      <body>{children}</body>
+    </html>
+  )
+}

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/layout.jsx

@@ -0,0 +1,11 @@
+import { LinkAccordion } from '../components/link-accordion'
+
+export default function Page() {
+  return (
+    <ul>
+      <li>
+        <LinkAccordion href="/target-page">Target page</LinkAccordion>
+      </li>
+    </ul>
+  )
+}

test/e2e/app-dir/ppr-navigations/simple/app/[locale]/page.jsx

@@ -0,0 +1,3 @@
+export default function TargetPage() {
+  return <div id="target-page">Target page</div>
+}