feat(middleware)!: forbids middleware response body #36835

feugy · 2022-05-11T14:00:26Z

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)

during development, a warning on console tells developers when they are returning a response (either with Response or NextResponse).
at build time, this warning becomes an error.
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 · 2022-05-11T14:22:12Z

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 · 2022-05-11T16:33:56Z

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

… build

Leverages Webpack builtin errors for better reporting

packages/next/server/web/error.ts

packages/next/server/web/adapter.ts

…esponse body

examples/damien/pages/_middleware.js

icyJoseph · 2022-05-26T19:49:44Z

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 · 2022-05-26T20:06:11Z

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 · 2022-05-26T20:09:07Z

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 · 2022-05-26T20:13:04Z

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 · 2022-06-12T11:03:27Z

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 · 2022-06-13T09:50:01Z

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 · 2022-07-12T09:18:13Z

@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 · 2022-07-18T09:55:17Z

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)

feugy requested review from timneutkens, ijjk, shuding, padmaia, huozhi, styfle, leerob and balazsorban44 as code owners May 11, 2022 14:00

ijjk added area: documentation type: next labels May 11, 2022

feugy mentioned this pull request May 11, 2022

feat(middleware)!: forbid response body feugy/next.js#1

Closed

feugy added 12 commits May 12, 2022 10:01

feat(middleware)\!: forbid response body

4b1a173

chore: restores URL legit test cases

27380a4

test: adds e2e tests for middleware body restriction

cac6671

doc: updates middleware and next/server pages.

1704bb0

feat!: removes NextResponse.json() function

93621a1

feat: warns for middleware body response in development, fails during…

8d70b9e

… build

refactor(middleware): be as strict during build as in dev

f5d515a

Leverages Webpack builtin errors for better reporting

docs: improves education about the middleware body limitation

719f0a6

chore: removes unused code

f128503

chore: removes unused code

404d0d8

chore: fixes existing production tests

f0b1dd8

chore: restores RSC when running on the edge

7881769

feugy force-pushed the feat/forbid-middleware-response-body branch from d3931cd to 7881769 Compare May 12, 2022 09:01

Merge branch 'canary' into feat/forbid-middleware-response-body

26620d0

Schniz mentioned this pull request May 16, 2022

support runtime: edge in api endpoints #36947

Merged

7 tasks

feugy added 2 commits May 17, 2022 10:02

Merge branch 'canary' into feat/forbid-middleware-response-body

86a3b25

chore: removes underscore from error page

c49a0bc

ijjk reviewed May 17, 2022

View reviewed changes

packages/next/server/web/error.ts Outdated Show resolved Hide resolved

feugy added 4 commits May 18, 2022 09:23

docs: better examples

3c56212

refactor: relies on next-server to handle redirections

bf8b175

Merge branch 'canary' into feat/forbid-middleware-response-body

9d4102c

chore: fix linter issue on generated file

33958ae

feugy requested review from timneutkens and ijjk May 18, 2022 12:12

ijjk reviewed May 18, 2022

View reviewed changes

packages/next/server/web/adapter.ts Outdated Show resolved Hide resolved

feugy added 3 commits May 19, 2022 10:56

refactor(middleware): logs warning and use generic 500 when setting r…

4f83c66

…esponse body

Merge branch 'canary' into feat/forbid-middleware-response-body

3348886

Merge branch 'canary' into feat/forbid-middleware-response-body

073079d

feugy requested a review from steven-tey as a code owner May 19, 2022 21:25

ijjk added the examples Issue was opened via the examples template. label May 19, 2022

ijjk reviewed May 19, 2022

View reviewed changes

examples/damien/pages/_middleware.js Outdated Show resolved Hide resolved

chore: remove undesired changes

265d02e

feugy force-pushed the feat/forbid-middleware-response-body branch from a6dad02 to 265d02e Compare May 19, 2022 21:34

Merge branch 'canary' into feat/forbid-middleware-response-body

0ed490f

ijjk approved these changes May 19, 2022

View reviewed changes

kodiakhq bot merged commit bf08956 into vercel:canary May 19, 2022

SukkaW mentioned this pull request May 20, 2022

Remove Edge Functions example that writes to the response body vercel/examples#261

Closed

8 tasks

feugy deleted the feat/forbid-middleware-response-body branch May 26, 2022 19:59

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(middleware)!: forbids middleware response body #36835

feat(middleware)!: forbids middleware response body #36835

feugy commented May 11, 2022 •

edited

Loading

ijjk commented May 11, 2022 •

edited

Loading

ijjk commented May 11, 2022 •

edited

Loading

icyJoseph commented May 26, 2022

feugy commented May 26, 2022

icyJoseph commented May 26, 2022 •

edited

Loading

icyJoseph commented May 26, 2022 •

edited

Loading

remorses commented Jun 12, 2022 •

edited

Loading

feugy commented Jun 13, 2022

larsqa commented Jul 12, 2022

feugy commented Jul 18, 2022

feat(middleware)!: forbids middleware response body #36835

feat(middleware)!: forbids middleware response body #36835

Conversation

feugy commented May 11, 2022 • edited Loading

What's in there?

How to try it?

Notes to reviewers

Feature

Documentation / Examples

ijjk commented May 11, 2022 • edited Loading

Failing test suites

ijjk commented May 11, 2022 • edited Loading

Stats from current PR

icyJoseph commented May 26, 2022

feugy commented May 26, 2022

icyJoseph commented May 26, 2022 • edited Loading

icyJoseph commented May 26, 2022 • edited Loading

remorses commented Jun 12, 2022 • edited Loading

feugy commented Jun 13, 2022

larsqa commented Jul 12, 2022

feugy commented Jul 18, 2022

feugy commented May 11, 2022 •

edited

Loading

ijjk commented May 11, 2022 •

edited

Loading

ijjk commented May 11, 2022 •

edited

Loading

icyJoseph commented May 26, 2022 •

edited

Loading

icyJoseph commented May 26, 2022 •

edited

Loading

remorses commented Jun 12, 2022 •

edited

Loading