rsnext/test/development/middleware-warnings/index.test.ts

87 lines
2.6 KiB
TypeScript
Raw Normal View History

feat(middleware)!: forbids middleware response body (#36835) _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()`](https://github.com/vercel/next.js/blob/canary/packages/next/server/web/spec-compliant/response.ts#L42-L53) may set the response body (https://github.com/vercel/next.js/pull/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: ```js new Response('a text value') new Response(JSON.stringify({ /* whatever */ }) ``` But these are false-positive cases: ```js 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 - [x] 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` - [x] Integration tests added - [x] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [x] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [x] Make sure the linting passes by running `yarn lint`
2022-05-20 00:02:20 +02:00
import { createNext } from 'e2e-utils'
import { NextInstance } from 'test/lib/next-modes/base'
import { sandbox } from '../acceptance/helpers'
const middlewarePath = 'middleware.js'
const middlewareWarning = `A middleware can not alter response's body`
describe('middlewares', () => {
let next: NextInstance
let cleanup
beforeAll(async () => {
next = await createNext({
files: {},
skipStart: true,
})
})
afterAll(() => next.destroy())
afterEach(() => cleanup?.())
it.each([
{
title: 'returning response with literal string',
code: `export default function middleware() {
return new Response('this is not allowed');
}`,
},
{
title: 'returning response with literal number',
code: `export default function middleware() {
return new Response(10);
}`,
},
{
title: 'returning response with JSON.stringify',
code: `export default function middleware() {
return new Response(JSON.stringify({ foo: 'this is not allowed' }));
}`,
},
{
title: 'populating response with a value',
code: `export default function middleware(request) {
const body = JSON.stringify({ foo: 'this should not be allowed, but hard to detect with AST' })
return new Response(body);
}`,
},
{
title: 'populating response with a function call',
code: `function buildBody() {
return 'this should not be allowed, but hard to detect with AST'
}
export default function middleware(request) {
return new Response(buildBody());
}`,
},
{
title: 'populating response with an async function call',
code: `export default async function middleware(request) {
return new Response(await fetch('https://example.vercel.sh'));
feat(middleware)!: forbids middleware response body (#36835) _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()`](https://github.com/vercel/next.js/blob/canary/packages/next/server/web/spec-compliant/response.ts#L42-L53) may set the response body (https://github.com/vercel/next.js/pull/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: ```js new Response('a text value') new Response(JSON.stringify({ /* whatever */ }) ``` But these are false-positive cases: ```js 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 - [x] 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` - [x] Integration tests added - [x] Documentation added - [ ] Telemetry added. In case of a feature if it's used or not. - [x] Errors have helpful link attached, see `contributing.md` ## Documentation / Examples - [x] Make sure the linting passes by running `yarn lint`
2022-05-20 00:02:20 +02:00
}`,
},
])('warns when $title', async ({ code }) => {
;({ cleanup } = await sandbox(next, new Map([[middlewarePath, code]])))
expect(next.cliOutput).toMatch(middlewareWarning)
})
it.each([
{
title: 'returning null reponse body',
code: `export default function middleware() {
return new Response(null);
}`,
},
{
title: 'returning undefined response body',
code: `export default function middleware() {
return new Response(undefined);
}`,
},
])('does not warn when $title', async ({ code }) => {
;({ cleanup } = await sandbox(next, new Map([[middlewarePath, code]])))
expect(next.cliOutput).not.toMatch(middlewareWarning)
})
})