feat(middleware)!: forbids middleware response body

[feugy]

Hello Next.js team! First PR here, I hope I've followed the right practices.

What's in there?

It has been decided to only support the following uses cases in Next.js' middleware:

• rewrite the URL (x-middleware-rewrite response header)
• redirect to another URL (Location response header)
• pass on to the next piece in the request pipeline (x-middleware-next response header)

1. during development, a warning on console tells developers when they are returning a response (either with Response or NextResponse).
2. at build time, this warning becomes an error.
3. at run time, returning a response body will trigger a 500 HTTP error with a JSON payload containing the detailed error.

All returned/thrown errors contain a link to the documentation.

This is a breaking feature compared to the beta middleware implementation, and also removes NextResponse.json() which makes no sense any more.

How to try it?

• runtime behavior: HEADLESS=true yarn jest test/integration/middleware/core
• build behavior : yarn jest test/integration/middleware/build-errors
• development behavior: HEADLESS=true yarn jest test/development/middleware-warnings

Notes to reviewers

The limitation happens in next's web adapter. The initial implementation was to check response.body existence, but it turns out Response.redirect() may set the response body (#31886). Hence why the proposed implementation specifically looks at response headers.
Response.redirect() and NextResponse.redirect() do not need to include the final location in their body: it is handled by next server https://github.com/vercel/next.js/blob/canary/packages/next/server/next-server.ts#L1142

Because this is a breaking change, I had to adjust several tests cases, previously returning JSON/stream/text bodies. When relevant, these middlewares are returning data using response headers.

About DevEx: relying on AST analysis to detect forbidden use cases is not as good as running the code.
Such cases are easy to detect:

new Response('a text value')
new Response(JSON.stringify({ /* whatever */ })

But these are false-positive cases:

function returnNull() { return null }
new Response(returnNull())

function doesNothing() {}
new Response(doesNothing())

However, I see no good reasons to let users ship middleware such as the one above, hence why the build will fail, even if technically speaking, they are not setting the response body.

Feature

• Implements an existing feature request or RFC. Make sure the feature request has been accepted for implementation before opening a PR.
• Related issues linked using fixes #number
• Integration tests added
• Documentation added
• Telemetry added. In case of a feature if it's used or not.
• Errors have helpful link attached, see contributing.md

Documentation / Examples

• Make sure the linting passes by running yarn lint

[ijjk]

Failing test suites

Commit: d3931cd

yarn testheadless test/integration/react-streaming-and-server-components/test/switchable-runtime.test.js

• Switchable runtime (dev) > should contain _app.server in flight response (edge)
• Switchable runtime (prod) > should contain _app.server in flight response (edge)
• Switchable runtime (prod) > should build /edge as a dynamic page with the edge runtime
• Switchable runtime (prod) > should build /edge-rsc as a dynamic page with the edge runtime

Expand output

● Switchable runtime (prod) › should contain _app.server in flight response (edge)

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 0

  55 |     expect(
  56 |       getOccurrence(html, new RegExp(`class="app-server-root"`, 'g'))
> 57 |     ).toBe(1)
     |       ^
  58 |     expect(
  59 |       getOccurrence(
  60 |         flightResponse,

  at Object.<anonymous> (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:57:7)

● Switchable runtime (prod) › should build /edge as a dynamic page with the edge runtime

TypeError: Cannot read properties of null (reading '1')

  67 | async function testRoute(appPort, url, { isStatic, isEdge, isRSC }) {
  68 |   const html1 = await renderViaHTTP(appPort, url)
> 69 |   const renderedAt1 = +html1.match(/Time: (\d+)/)[1]
     |                                                  ^
  70 |   expect(html1).toContain(`Runtime: ${isEdge ? 'Edge' : 'Node.js'}`)
  71 |
  72 |   const html2 = await renderViaHTTP(appPort, url)

  at testRoute (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:69:50)
  at Object.<anonymous> (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:197:5)

● Switchable runtime (prod) › should build /edge-rsc as a dynamic page with the edge runtime

TypeError: Cannot read properties of null (reading '1')

  67 | async function testRoute(appPort, url, { isStatic, isEdge, isRSC }) {
  68 |   const html1 = await renderViaHTTP(appPort, url)
> 69 |   const renderedAt1 = +html1.match(/Time: (\d+)/)[1]
     |                                                  ^
  70 |   expect(html1).toContain(`Runtime: ${isEdge ? 'Edge' : 'Node.js'}`)
  71 |
  72 |   const html2 = await renderViaHTTP(appPort, url)

  at testRoute (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:69:50)
  at Object.<anonymous> (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:205:5)

● Switchable runtime (dev) › should contain _app.server in flight response (edge)

expect(received).toBe(expected) // Object.is equality

Expected: 1
Received: 0

  55 |     expect(
  56 |       getOccurrence(html, new RegExp(`class="app-server-root"`, 'g'))
> 57 |     ).toBe(1)
     |       ^
  58 |     expect(
  59 |       getOccurrence(
  60 |         flightResponse,

  at Object.<anonymous> (integration/react-streaming-and-server-components/test/switchable-runtime.test.js:57:7)

Read more about building and testing Next.js in contributing.md.

[ijjk]

Stats from current PR

Default Build (Decrease detected ✓)

General Overall increase ⚠️

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
buildDuration	19.9s	20.4s	⚠️ +444ms
buildDurationCached	7.9s	8s	⚠️ +71ms
nodeModulesSize	479 MB	479 MB	⚠️ +4.44 kB

Page Load Tests Overall decrease ⚠️

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
/ failed reqs	0	0	✓
/ total time (seconds)	5.268	5.37	⚠️ +0.1
/ avg req/sec	474.59	465.54	⚠️ -9.05
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.053	2.043	-0.01
/error-in-render avg req/sec	1217.94	1223.64	+5.7

Client Bundles (main, webpack)

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
925.HASH.js gzip	179 B	179 B	✓
framework-HASH.js gzip	42 kB	42 kB	✓
main-HASH.js gzip	29 kB	29 kB	✓
webpack-HASH.js gzip	1.44 kB	1.44 kB	✓
Overall change	72.6 kB	72.6 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
_app-HASH.js gzip	1.36 kB	1.36 kB	✓
_error-HASH.js gzip	193 B	193 B	✓
amp-HASH.js gzip	308 B	308 B	✓
css-HASH.js gzip	327 B	327 B	✓
dynamic-HASH.js gzip	2.71 kB	2.71 kB	✓
head-HASH.js gzip	359 B	359 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.73 kB	5.73 kB	✓
index-HASH.js gzip	263 B	263 B	✓
link-HASH.js gzip	2.65 kB	2.65 kB	✓
routerDirect..HASH.js gzip	320 B	320 B	✓
script-HASH.js gzip	391 B	391 B	✓
withRouter-HASH.js gzip	318 B	318 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16 kB	16 kB	✓

Client Build Manifests

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
_buildManifest.js gzip	458 B	458 B	✓
Overall change	458 B	458 B	✓

Rendered Page Sizes

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	527 B	527 B	✓
Overall change	1.6 kB	1.6 kB	✓

---

Default Build with SWC (Increase detected ⚠️)

General Overall increase ⚠️

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
buildDuration	23.2s	23s	-222ms
buildDurationCached	7.8s	8s	⚠️ +193ms
nodeModulesSize	479 MB	479 MB	⚠️ +4.44 kB

Page Load Tests Overall increase ✓

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
/ failed reqs	0	0	✓
/ total time (seconds)	5.317	5.411	⚠️ +0.09
/ avg req/sec	470.18	462.02	⚠️ -8.16
/error-in-render failed reqs	0	0	✓
/error-in-render total time (seconds)	2.148	2.011	-0.14
/error-in-render avg req/sec	1163.82	1243.01	+79.19

Client Bundles (main, webpack)

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
925.HASH.js gzip	178 B	178 B	✓
framework-HASH.js gzip	42.7 kB	42.7 kB	✓
main-HASH.js gzip	29.5 kB	29.5 kB	✓
webpack-HASH.js gzip	1.45 kB	1.45 kB	✓
Overall change	73.8 kB	73.8 kB	✓

Legacy Client Bundles (polyfills)

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
polyfills-HASH.js gzip	31 kB	31 kB	✓
Overall change	31 kB	31 kB	✓

Client Pages

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
_app-HASH.js gzip	1.35 kB	1.35 kB	✓
_error-HASH.js gzip	179 B	179 B	✓
amp-HASH.js gzip	311 B	311 B	✓
css-HASH.js gzip	324 B	324 B	✓
dynamic-HASH.js gzip	2.89 kB	2.89 kB	✓
head-HASH.js gzip	357 B	357 B	✓
hooks-HASH.js gzip	920 B	920 B	✓
image-HASH.js gzip	5.82 kB	5.82 kB	✓
index-HASH.js gzip	261 B	261 B	✓
link-HASH.js gzip	2.78 kB	2.78 kB	✓
routerDirect..HASH.js gzip	322 B	322 B	✓
script-HASH.js gzip	392 B	392 B	✓
withRouter-HASH.js gzip	317 B	317 B	✓
85e02e95b279..7e3.css gzip	107 B	107 B	✓
Overall change	16.3 kB	16.3 kB	✓

Client Build Manifests

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
_buildManifest.js gzip	457 B	457 B	✓
Overall change	457 B	457 B	✓

Rendered Page Sizes

	vercel/next.js canary	feugy/next.js feat/forbid-middleware-response-body	Change
index.html gzip	532 B	532 B	✓
link.html gzip	546 B	546 B	✓
withRouter.html gzip	528 B	528 B	✓
Overall change	1.61 kB	1.61 kB	✓

Commit: 0ed490f

[icyJoseph]

Hi,

Under this change, would it be possible to return 404's (or any other status code for that matter) still?

return new Response(null, { status: 400 })

I think it should? Or would that perhaps be limited as well?

[feugy]

Hi @icyJoseph. It still possible to return a 404 status, like in your example.
In this case, it'll bypass the router, and the default (or your custom) 404 page wouldn't be displayed.

For this reason, the best is to use NextResponse.rewrite():

export default async function middleware(request) {
  return NextResponse.rewrite(new URL('/not-found', request.url))
}

[icyJoseph]

Hi @icyJoseph. It still possible to return a 404 status, like in your example. In this case, it'll bypass the router, and the default (or your custom) 404 page wouldn't be displayed.

For this reason, the best is to use NextResponse.rewrite():

export default async function middleware(request) {
  return NextResponse.rewrite(new URL('/not-found', request.url))
}

Awesome! Yeah, but if this was for API routes for instance, then, sometimes, I would just want to return plain 404, with no HTML. Good to know both pathways though. Thanks!

[icyJoseph]

One last thing, today, if I do:

export function middleware(req: NextRequest) {
  const response = NextResponse.next();
  response.headers.set("x-foo", "bar");
  return response;
}

I can then collect the x-foo header in getServerSideProps' context, but in the res property (not req), is that intentional?

export const getServerSideProps = async (ctx: GetServerSidePropsContext) => {
  console.log(ctx.res.getHeader("x-foo")); // logs "bar"
  return { props: {} };
};

Or perhaps folks shouldn't rely on it?

Thanks a bunch for your time!

[remorses]

What is the reason of this change? i was planning to use middleware to replace the rewrite field of next.config.js with a fetch call as it gets me more flexibility (allowing to modify request headers for example) but now i can't because POST requests would always fail.

[feugy]

Hello @remorses!
You'll find in #37514 more details about why returning a response from middleware can lead to security issues.

As for your use case, if I understand correctly, it's still fully possible. You can (as for now) redirect and adjust response headers:

import { NextResponse } from 'next/server'
export default function() {
  const response = NextResponse.redirect(whateverUrlYouNeed)
  response.headers.append(yourHeader, yourHeaderValue)
  return response
}

There's an example for setting cookies while redirecting in the docs
Hope it'll help!

[larsqa]

@feugy, I've read through the whole issue report of #37514 and wasn't able to find any description on security issues. Could you elaborate?

[feugy]

Hi @larsqa. Without entering too much into details, it's simpler and safer to limit what they can do. It mostly mitigates Next.js internal router complexity, which ultimately is securing your applications.

Next.js offers other ways to return responses with API, which can now also executes on the edge (an experimental feature at the time of writing)