fix: normalise middleware data URL requests (#267)

* fix: normalise middleware data URL requests

* chore: fix test

* chore: fix test

* fix: strip trailing slash

* Changes from review

* Fix slash normalizing and redirect handling

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>

Author: ascorbic

edge-runtime/lib/response.ts

@@ -7,8 +7,8 @@ import {
   addBasePath,
   normalizeDataUrl,
   normalizeLocalePath,
+  normalizeTrailingSlash,
   relativizeURL,
-  rewriteDataPath,
 } from './util.ts'
 import { addMiddlewareHeaders, isMiddlewareRequest, isMiddlewareResponse } from './middleware.ts'
 import { RequestData } from './next-request.ts'
@@ -116,10 +116,21 @@ export const buildResponse = async ({
   const res = new Response(result.response.body, result.response)
   request.headers.set('x-nf-next-middleware', 'skip')
 
-  const rewrite = res.headers.get('x-middleware-rewrite')
+  let rewrite = res.headers.get('x-middleware-rewrite')
+  let redirect = res.headers.get('location')
 
   // Data requests (i.e. requests for /_next/data ) need special handling
   const isDataReq = request.headers.has('x-nextjs-data')
+  // Data requests need to be normalized to the route path
+  if (isDataReq && !redirect && !rewrite) {
+    const requestUrl = new URL(request.url)
+    const normalizedDataUrl = normalizeDataUrl(requestUrl.pathname)
+    // Don't rewrite unless the URL has changed
+    if (normalizedDataUrl !== requestUrl.pathname) {
+      rewrite = `${normalizedDataUrl}${requestUrl.search}`
+      logger.withFields({ rewrite_url: rewrite }).debug('Rewritten data URL')
+    }
+  }
 
   if (rewrite) {
     logger.withFields({ rewrite_url: rewrite }).debug('Found middleware rewrite')
@@ -132,7 +143,6 @@ export const buildResponse = async ({
     }
 
     const relativeUrl = relativizeURL(rewrite, request.url)
-    const originalPath = new URL(request.url, `http://n`).pathname
 
     if (isDataReq) {
       // Data requests might be rewritten to an external URL
@@ -165,31 +175,24 @@ export const buildResponse = async ({
       }
 
       return addMiddlewareHeaders(fetch(proxyRequest, { redirect: 'manual' }), res)
-    } else if (isDataReq) {
-      rewriteUrl.pathname = rewriteDataPath({
-        dataUrl: originalPath,
-        newRoute: rewriteUrl.pathname,
-        basePath: nextConfig?.basePath,
-      })
-      if (rewriteUrl.toString() === request.url) {
-        logger.withFields({ rewrite_url: rewrite }).debug('Rewrite url is same as original url')
-        return
-      }
-      res.headers.set('x-middleware-rewrite', relativeUrl)
-      return addMiddlewareHeaders(fetch(new Request(rewriteUrl, request)), res)
     }
-    const target = normalizeLocalizedTarget({ target: rewrite, request, nextConfig })
+
+    if (isDataReq) {
+      // The rewrite target is a data request, but a middleware rewrite target is always for the page route,
+      // so we need to tell the server this is a data request. Setting the `x-nextjs-data` header is not enough. 🤷
+      rewriteUrl.searchParams.set('__nextDataReq', '1')
+      rewriteUrl.pathname = normalizeTrailingSlash(rewriteUrl.pathname, nextConfig?.trailingSlash)
+    }
+    const target = normalizeLocalizedTarget({ target: rewriteUrl.toString(), request, nextConfig })
     if (target === request.url) {
       logger.withFields({ rewrite_url: rewrite }).debug('Rewrite url is same as original url')
       return
     }
     res.headers.set('x-middleware-rewrite', relativeUrl)
     request.headers.set('x-middleware-rewrite', target)
-    return addMiddlewareHeaders(fetch(new Request(target, request)), res)
+    return addMiddlewareHeaders(fetch(new Request(target, request), { redirect: 'manual' }), res)
   }
 
-  let redirect = res.headers.get('location')
-
   // If we are redirecting a request that had a locale in the URL, we need to add it back in
   if (redirect && requestLocale) {
     redirect = normalizeLocalizedTarget({ target: redirect, request, nextConfig })

edge-runtime/lib/util.ts

@@ -81,9 +81,14 @@ export function relativizeURL(url: string | string, base: string | URL) {
 
 export const normalizeIndex = (path: string) => (path === '/' ? '/index' : path)
 
-const stripTrailingSlash = (path: string) =>
+export const normalizeTrailingSlash = (path: string, trailingSlash?: boolean) =>
+  trailingSlash ? addTrailingSlash(path) : stripTrailingSlash(path)
+
+export const stripTrailingSlash = (path: string) =>
   path !== '/' && path.endsWith('/') ? path.slice(0, -1) : path
 
+export const addTrailingSlash = (path: string) => (path.endsWith('/') ? path : `${path}/`)
+
 /**
  * Modify a data url to point to a new page route.
  */

src/run/headers.test.ts

@@ -39,7 +39,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'cookie=__prerender_bypass|__next_preview_data',
+          'header=x-nextjs-data,cookie=__prerender_bypass|__next_preview_data',
         )
       })
 
@@ -55,7 +55,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'header=Accept|Accept-Language,cookie=__prerender_bypass|__next_preview_data',
+          'header=x-nextjs-data|Accept|Accept-Language,cookie=__prerender_bypass|__next_preview_data',
         )
       })
 
@@ -76,7 +76,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'cookie=__prerender_bypass|__next_preview_data',
+          'header=x-nextjs-data,cookie=__prerender_bypass|__next_preview_data',
         )
       })
 
@@ -96,7 +96,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'cookie=__prerender_bypass|__next_preview_data',
+          'header=x-nextjs-data,cookie=__prerender_bypass|__next_preview_data',
         )
       })
 
@@ -116,7 +116,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'language=en|de|fr,cookie=__prerender_bypass|__next_preview_data|NEXT_LOCALE',
+          'header=x-nextjs-data,language=en|de|fr,cookie=__prerender_bypass|__next_preview_data|NEXT_LOCALE',
         )
       })
 
@@ -137,7 +137,7 @@ describe('headers', () => {
 
         expect(headers.set).toBeCalledWith(
           'netlify-vary',
-          'language=en|de|fr,cookie=__prerender_bypass|__next_preview_data|NEXT_LOCALE',
+          'header=x-nextjs-data,language=en|de|fr,cookie=__prerender_bypass|__next_preview_data|NEXT_LOCALE',
         )
       })
     })

src/run/headers.ts

@@ -55,7 +55,7 @@ export const setVaryHeaders = (
   { basePath, i18n }: Pick<NextConfigComplete, 'basePath' | 'i18n'>,
 ) => {
   const netlifyVaryValues: NetlifyVaryValues = {
-    headers: [],
+    headers: ['x-nextjs-data'],
     languages: [],
     cookies: ['__prerender_bypass', '__next_preview_data'],
   }

tests/fixtures/page-router/next.config.js

@@ -4,6 +4,7 @@ const nextConfig = {
   eslint: {
     ignoreDuringBuilds: true,
   },
+  generateBuildId: () => 'build-id',
 }
 
 module.exports = nextConfig

tests/integration/edge-handler.test.ts

@@ -304,7 +304,7 @@ describe('should run middleware on data requests', () => {
 })
 
 describe('page router', () => {
-  test<FixtureTestContext>('should rewrite data requests', async (ctx) => {
+  test<FixtureTestContext>('middleware should rewrite data requests', async (ctx) => {
     await createFixture('middleware-pages', ctx)
     await runPlugin(ctx)
     const origin = await LocalServer.run(async (req, res) => {
@@ -327,8 +327,70 @@ describe('page router', () => {
     })
     const res = await response.json()
     const url = new URL(res.url, 'http://n/')
-    expect(url.pathname).toBe('/_next/data/build-id/ssr-page-2.json')
+    expect(url.pathname).toBe('/ssr-page-2/')
+    expect(url.searchParams.get('__nextDataReq')).toBe('1')
+    expect(res.headers['x-nextjs-data']).toBe('1')
     expect(response.headers.get('x-nextjs-rewrite')).toBe('/ssr-page-2/')
     expect(response.status).toBe(200)
   })
+
+  test<FixtureTestContext>('should rewrite un-rewritten data requests to page route', async (ctx) => {
+    await createFixture('middleware-pages', ctx)
+    await runPlugin(ctx)
+    const origin = await LocalServer.run(async (req, res) => {
+      res.write(
+        JSON.stringify({
+          url: req.url,
+          headers: req.headers,
+        }),
+      )
+      res.end()
+    })
+    ctx.cleanup?.push(() => origin.stop())
+    const response = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      headers: {
+        'x-nextjs-data': '1',
+      },
+      origin,
+      url: `/_next/data/build-id/ssg/hello.json`,
+    })
+    const res = await response.json()
+    const url = new URL(res.url, 'http://n/')
+    expect(url.pathname).toBe('/ssg/hello/')
+    expect(url.searchParams.get('__nextDataReq')).toBe('1')
+    expect(res.headers['x-nextjs-data']).toBe('1')
+    expect(response.status).toBe(200)
+  })
+
+  test<FixtureTestContext>('should preserve query params in rewritten data requests', async (ctx) => {
+    await createFixture('middleware-pages', ctx)
+    await runPlugin(ctx)
+    const origin = await LocalServer.run(async (req, res) => {
+      res.write(
+        JSON.stringify({
+          url: req.url,
+          headers: req.headers,
+        }),
+      )
+      res.end()
+    })
+    ctx.cleanup?.push(() => origin.stop())
+    const response = await invokeEdgeFunction(ctx, {
+      functions: ['___netlify-edge-handler-middleware'],
+      headers: {
+        'x-nextjs-data': '1',
+      },
+      origin,
+      url: `/_next/data/build-id/blog/first.json?slug=first`,
+    })
+    const res = await response.json()
+    console.log(res)
+    const url = new URL(res.url, 'http://n/')
+    expect(url.pathname).toBe('/blog/first/')
+    expect(url.searchParams.get('__nextDataReq')).toBe('1')
+    expect(url.searchParams.get('slug')).toBe('first')
+    expect(res.headers['x-nextjs-data']).toBe('1')
+    expect(response.status).toBe(200)
+  })
 })

tests/integration/page-router.test.ts

@@ -97,3 +97,19 @@ test<FixtureTestContext>('Should revalidate path with On-demand Revalidation', a
   console.log({ dateCacheInitial, dateCacheRevalidated })
   expect(dateCacheInitial).not.toBe(dateCacheRevalidated)
 })
+
+test<FixtureTestContext>('Should return JSON for data req to page route', async (ctx) => {
+  await createFixture('page-router', ctx)
+  await runPlugin(ctx)
+
+  const response = await invokeFunction(ctx, {
+    url: '/static/revalidate-manual?__nextDataReq=1',
+    headers: { 'x-nextjs-data': '1' },
+  })
+
+  expect(response.body).toMatch(/^{"pageProps":/)
+
+  const data = JSON.parse(response.body)
+
+  expect(data.pageProps.show).toBeDefined()
+})