From 62693209275ac2072941847e8e6ca5b5c4e8f3ba Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Sun, 23 Jan 2022 13:15:31 +0200 Subject: [PATCH 1/6] Don't merge queryparams in middleware rewrites This allows to have a more streamlined API for rewrites. Since middlewares are code and not static configurations, we can allow people to delete query params and not just overwrite them. --- .../deleting-query-params-in-middlewares.md | 35 ++++++++++++++++++ errors/manifest.json | 4 +++ packages/next/server/next-server.ts | 36 +++++++++++++++++-- packages/next/server/request-meta.ts | 31 ++++++++++++++-- packages/next/server/router.ts | 4 +-- 5 files changed, 103 insertions(+), 7 deletions(-) create mode 100644 errors/deleting-query-params-in-middlewares.md diff --git a/errors/deleting-query-params-in-middlewares.md b/errors/deleting-query-params-in-middlewares.md new file mode 100644 index 000000000000000..676c5d903a3de9c --- /dev/null +++ b/errors/deleting-query-params-in-middlewares.md @@ -0,0 +1,35 @@ +# Deleting Query Parameters In Middlewares + +#### Why This Error Occurred + +In previous versions of Next.js, we were merging query parameters with the incoming request for rewrites happening in middlewares, to match the behavior of static rewrites declared in the config. This forced Next.js users to use empty query parameters values to delete keys. + +We are changing this behavior to allow extra flexibility and a more streamlined experience for users. So from now on, query parameters will not be merged and thus the warning. + +```typescript +import type { NextRequest } from 'next/server' +import { NextResponse } from 'next/server' + +export default function middleware(request: NextRequest) { + const nextUrl = request.nextUrl + nextUrl.searchParams.delete('key') // this is now possible! 🎉 + return NextResponse.rewrite(nextUrl) +} +``` + +#### Possible Ways to Fix It + +If you are relying on the old behavior, please add the query parameters manually to the rewritten URL. Using `request.nextUrl` would do that automatically for you. + +```typescript +import type { NextRequest } from 'next/server' +import { NextResponse } from 'next/server' + +export default function middleware(request: NextRequest) { + const nextUrl = request.nextUrl + nextUrl.pathname = '/dest' + return NextResponse.rewrite(url) +} +``` + +This warning will be removed in a next version of Next.js. diff --git a/errors/manifest.json b/errors/manifest.json index 69d076307757442..930a16a147dbc8e 100644 --- a/errors/manifest.json +++ b/errors/manifest.json @@ -531,6 +531,10 @@ { "title": "middleware-relative-urls", "path": "/errors/middleware-relative-urls.md" + }, + { + "title": "deleting-query-params-in-middlewares", + "path": "/errors/deleting-query-params-in-middlewares.md" } ] } diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 5330982778b2ba6..4f4c00411896beb 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -74,6 +74,7 @@ import { getMiddlewareRegex, getRouteMatcher } from '../shared/lib/router/utils' import { MIDDLEWARE_ROUTE } from '../lib/constants' import { loadEnvConfig } from '@next/env' import { getCustomRoute } from './server-route-utils' +import { urlQueryToSearchParams } from '../shared/lib/router/utils/querystring' export * from './base-server' @@ -1005,13 +1006,21 @@ export default class NextNodeServer extends BaseServer { } if (result.response.headers.has('x-middleware-rewrite')) { + const rewritePath = result.response.headers.get( + 'x-middleware-rewrite' + )! const { newUrl, parsedDestination } = prepareDestination({ - appendParamsToQuery: true, - destination: result.response.headers.get('x-middleware-rewrite')!, + appendParamsToQuery: false, + destination: rewritePath, params: _params, - query: parsedUrl.query, + query: {}, }) + warnIfQueryParametersWereDeleted( + parsedUrl.query, + parsedDestination.query + ) + if ( parsedDestination.protocol && (parsedDestination.port @@ -1193,3 +1202,24 @@ export default class NextNodeServer extends BaseServer { return require(join(this.distDir, ROUTES_MANIFEST)) } } + +function warnIfQueryParametersWereDeleted( + incoming: ParsedUrlQuery, + rewritten: ParsedUrlQuery +): void { + const incomingQuery = urlQueryToSearchParams(incoming) + const rewrittenQuery = urlQueryToSearchParams(rewritten) + + const missingKeys = [...incomingQuery.keys()].filter((key) => { + return !rewrittenQuery.has(key) + }) + + if (missingKeys.length > 0) { + Log.warn( + `Middleware rewrite deleted the following query parameters from the URL: ${missingKeys.join( + ', ' + )}.`, + 'We are no longer merging query parameters - https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares' + ) + } +} diff --git a/packages/next/server/request-meta.ts b/packages/next/server/request-meta.ts index 3587face6e3b04e..271af409ac66781 100644 --- a/packages/next/server/request-meta.ts +++ b/packages/next/server/request-meta.ts @@ -53,16 +53,43 @@ export function addRequestMeta( return setRequestMeta(request, meta) } -export type NextParsedUrlQuery = ParsedUrlQuery & { +type NextQueryMetadata = { __nextDefaultLocale?: string __nextFallback?: 'true' __nextLocale?: string __nextSsgPath?: string _nextBubbleNoFallback?: '1' _nextDataReq?: '1' - amp?: '1' } +export type NextParsedUrlQuery = ParsedUrlQuery & + NextQueryMetadata & { + amp?: '1' + } + export interface NextUrlWithParsedQuery extends UrlWithParsedQuery { query: NextParsedUrlQuery } + +export function getNextInternalQuery( + query: NextParsedUrlQuery +): NextQueryMetadata { + const keysToInclude: (keyof NextQueryMetadata)[] = [ + '__nextDefaultLocale', + '__nextFallback', + '__nextLocale', + '__nextSsgPath', + '_nextBubbleNoFallback', + '_nextDataReq', + ] + const nextInternalQuery: NextQueryMetadata = {} + + for (const key of keysToInclude) { + if (key in query) { + // @ts-ignore this can't be typed correctly + nextInternalQuery[key] = query[key] + } + } + + return nextInternalQuery +} diff --git a/packages/next/server/router.ts b/packages/next/server/router.ts index e3d519d91b809cd..1c9fc3edfffb353 100644 --- a/packages/next/server/router.ts +++ b/packages/next/server/router.ts @@ -1,5 +1,5 @@ import type { ParsedUrlQuery } from 'querystring' -import type { NextUrlWithParsedQuery } from './request-meta' +import { getNextInternalQuery, NextUrlWithParsedQuery } from './request-meta' import pathMatch from '../shared/lib/router/utils/path-match' import { removePathTrailingSlash } from '../client/normalize-trailing-slash' @@ -400,7 +400,7 @@ export default class Router { if (result.query) { parsedUrlUpdated.query = { - ...parsedUrlUpdated.query, + ...getNextInternalQuery(parsedUrlUpdated.query), ...result.query, } } From 29bc67bef7d3692ea851efce73a35ab4b421bc6e Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Wed, 26 Jan 2022 17:47:36 +0200 Subject: [PATCH 2/6] add a test case --- .../core/pages/rewrites/_middleware.js | 13 +++++++++++++ .../core/pages/rewrites/clear-query-params.js | 12 ++++++++++++ .../middleware/core/test/index.test.js | 18 ++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 test/integration/middleware/core/pages/rewrites/clear-query-params.js diff --git a/test/integration/middleware/core/pages/rewrites/_middleware.js b/test/integration/middleware/core/pages/rewrites/_middleware.js index 20113e2978d1616..bc8bd0153e386b7 100644 --- a/test/integration/middleware/core/pages/rewrites/_middleware.js +++ b/test/integration/middleware/core/pages/rewrites/_middleware.js @@ -1,5 +1,8 @@ import { NextResponse } from 'next/server' +/** + * @param {import('next/server').NextRequest} request + */ export async function middleware(request) { const url = request.nextUrl @@ -32,6 +35,16 @@ export async function middleware(request) { return NextResponse.rewrite('https://vercel.com') } + if (url.pathname === '/rewrites/clear-query-params') { + const allowedKeys = ['allowed'] + for (const key of [...url.searchParams.keys()]) { + if (!allowedKeys.includes(key)) { + url.searchParams.delete(key) + } + } + return NextResponse.rewrite(url) + } + if (url.pathname === '/rewrites/rewrite-me-without-hard-navigation') { url.searchParams.set('middleware', 'foo') url.pathname = diff --git a/test/integration/middleware/core/pages/rewrites/clear-query-params.js b/test/integration/middleware/core/pages/rewrites/clear-query-params.js new file mode 100644 index 000000000000000..2901a155e894cc8 --- /dev/null +++ b/test/integration/middleware/core/pages/rewrites/clear-query-params.js @@ -0,0 +1,12 @@ +export default function ClearQueryParams(props) { + return 
{JSON.stringify(props.query)}
+} + +/** @type {import('next').GetServerSideProps} */ +export const getServerSideProps = (req) => { + return { + props: { + query: { ...req.query }, + }, + } +} diff --git a/test/integration/middleware/core/test/index.test.js b/test/integration/middleware/core/test/index.test.js index 32e2b99c2f4a211..f6f6d6ebc1ff093 100644 --- a/test/integration/middleware/core/test/index.test.js +++ b/test/integration/middleware/core/test/index.test.js @@ -212,6 +212,24 @@ function rewriteTests(locale = '') { expect($('.title').text()).toBe(expectedText) }) + it(`${locale} should clear query parameters`, async () => { + const res = await fetchViaHTTP( + context.appPort, + `${locale}/rewrites/clear-query-params`, + { + a: '1', + b: '2', + foo: 'bar', + allowed: 'kept', + } + ) + const html = await res.text() + const $ = cheerio.load(html) + expect(JSON.parse($('#my-query-params').text())).toEqual({ + allowed: 'kept', + }) + }) + it(`${locale} should rewrite to about page`, async () => { const res = await fetchViaHTTP( context.appPort, From ce314653ea3690e576bf3e11aa176d4b20d24fa7 Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Thu, 27 Jan 2022 21:45:26 +0200 Subject: [PATCH 3/6] Update packages/next/server/next-server.ts Co-authored-by: JJ Kasper --- packages/next/server/next-server.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 4f4c00411896beb..c7978831fd3a778 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -1216,10 +1216,7 @@ function warnIfQueryParametersWereDeleted( if (missingKeys.length > 0) { Log.warn( - `Middleware rewrite deleted the following query parameters from the URL: ${missingKeys.join( - ', ' - )}.`, - 'We are no longer merging query parameters - https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares' + `Query params are no longer automatically merged for rewrites in middleware, see more info here: https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares' ) } } From 0ec02c9f90f846f2f0c96b754af842f8c030aeb6 Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Thu, 27 Jan 2022 21:45:53 +0200 Subject: [PATCH 4/6] Update packages/next/server/next-server.ts Co-authored-by: JJ Kasper --- packages/next/server/next-server.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index c7978831fd3a778..4039a05d5a9b3e3 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -1016,6 +1016,7 @@ export default class NextNodeServer extends BaseServer { query: {}, }) + // TODO: remove after next minor version current `v12.0.9` warnIfQueryParametersWereDeleted( parsedUrl.query, parsedDestination.query From 1615cc3682f6d67079082e96852a335ff7610c9f Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Thu, 27 Jan 2022 22:08:13 +0200 Subject: [PATCH 5/6] warn once, and test that it actually warns --- packages/next/server/next-server.ts | 32 ++++++++++--------- .../middleware/core/test/index.test.js | 22 ++++++++++--- 2 files changed, 34 insertions(+), 20 deletions(-) diff --git a/packages/next/server/next-server.ts b/packages/next/server/next-server.ts index 4039a05d5a9b3e3..2596d5dc785f889 100644 --- a/packages/next/server/next-server.ts +++ b/packages/next/server/next-server.ts @@ -1017,7 +1017,7 @@ export default class NextNodeServer extends BaseServer { }) // TODO: remove after next minor version current `v12.0.9` - warnIfQueryParametersWereDeleted( + this.warnIfQueryParametersWereDeleted( parsedUrl.query, parsedDestination.query ) @@ -1202,22 +1202,24 @@ export default class NextNodeServer extends BaseServer { protected getRoutesManifest() { return require(join(this.distDir, ROUTES_MANIFEST)) } -} -function warnIfQueryParametersWereDeleted( - incoming: ParsedUrlQuery, - rewritten: ParsedUrlQuery -): void { - const incomingQuery = urlQueryToSearchParams(incoming) - const rewrittenQuery = urlQueryToSearchParams(rewritten) + // TODO: remove after next minor version current `v12.0.9` + private warnIfQueryParametersWereDeleted( + incoming: ParsedUrlQuery, + rewritten: ParsedUrlQuery + ): void { + const incomingQuery = urlQueryToSearchParams(incoming) + const rewrittenQuery = urlQueryToSearchParams(rewritten) - const missingKeys = [...incomingQuery.keys()].filter((key) => { - return !rewrittenQuery.has(key) - }) + const missingKeys = [...incomingQuery.keys()].filter((key) => { + return !rewrittenQuery.has(key) + }) - if (missingKeys.length > 0) { - Log.warn( - `Query params are no longer automatically merged for rewrites in middleware, see more info here: https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares' - ) + if (missingKeys.length > 0) { + Log.warn( + `Query params are no longer automatically merged for rewrites in middleware, see more info here: https://nextjs.org/docs/messages/errors/deleting-query-params-in-middlewares` + ) + this.warnIfQueryParametersWereDeleted = () => {} + } } } diff --git a/test/integration/middleware/core/test/index.test.js b/test/integration/middleware/core/test/index.test.js index f6f6d6ebc1ff093..34fbd3707ab7780 100644 --- a/test/integration/middleware/core/test/index.test.js +++ b/test/integration/middleware/core/test/index.test.js @@ -12,6 +12,7 @@ import { launchApp, nextBuild, nextStart, + stderr, } from 'next-test-utils' jest.setTimeout(1000 * 60 * 2) @@ -37,8 +38,8 @@ describe('Middleware base tests', () => { }) }) afterAll(() => killApp(context.app)) - rewriteTests() - rewriteTests('/fr') + rewriteTests(log) + rewriteTests(log, '/fr') redirectTests() redirectTests('/fr') responseTests() @@ -74,8 +75,8 @@ describe('Middleware base tests', () => { }) }) afterAll(() => killApp(context.app)) - rewriteTests() - rewriteTests('/fr') + rewriteTests(serverOutput) + rewriteTests(serverOutput, '/fr') redirectTests() redirectTests('/fr') responseTests() @@ -161,7 +162,7 @@ function urlTests(log, locale = '') { }) } -function rewriteTests(locale = '') { +function rewriteTests(log, locale = '') { it('should rewrite to fallback: true page successfully', async () => { const randomSlug = `another-${Date.now()}` const res2 = await fetchViaHTTP( @@ -230,6 +231,17 @@ function rewriteTests(locale = '') { }) }) + it(`warns about a query param deleted`, async () => { + await fetchViaHTTP( + context.appPort, + `${locale}/rewrites/clear-query-params`, + { a: '1', allowed: 'kept' } + ) + expect(log.output).toContain( + 'Query params are no longer automatically merged for rewrites in middleware' + ) + }) + it(`${locale} should rewrite to about page`, async () => { const res = await fetchViaHTTP( context.appPort, From 608d3b5af7fd307397894a5955637a700312cefa Mon Sep 17 00:00:00 2001 From: Gal Schlezinger Date: Thu, 27 Jan 2022 22:23:24 +0200 Subject: [PATCH 6/6] fix lint --- test/integration/middleware/core/test/index.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/middleware/core/test/index.test.js b/test/integration/middleware/core/test/index.test.js index 34fbd3707ab7780..5a5959374e01791 100644 --- a/test/integration/middleware/core/test/index.test.js +++ b/test/integration/middleware/core/test/index.test.js @@ -12,7 +12,6 @@ import { launchApp, nextBuild, nextStart, - stderr, } from 'next-test-utils' jest.setTimeout(1000 * 60 * 2)