fix: narrow down middleware i18n locale matcher to concrete locales (#…

…2768)

* test: add test cases to i18n middleware with exclusions

* fix: narrow down middleware i18n locale matcher to concrete locales

src/build/functions/edge.ts

@@ -36,13 +36,23 @@ const augmentMatchers = (
   matchers: NextDefinition['matchers'],
   ctx: PluginContext,
 ): NextDefinition['matchers'] => {
-  if (!ctx.buildConfig.i18n) {
+  const i18NConfig = ctx.buildConfig.i18n
+  if (!i18NConfig) {
     return matchers
   }
   return matchers.flatMap((matcher) => {
     if (matcher.originalSource && matcher.locale !== false) {
       return [
-        matcher,
+        matcher.regexp
+          ? {
+              ...matcher,
+              // https://github.com/vercel/next.js/blob/5e236c9909a768dc93856fdfad53d4f4adc2db99/packages/next/src/build/analysis/get-page-static-info.ts#L332-L336
+              // Next is producing pretty broad matcher for i18n locale. Presumably rest of their infrastructure protects this broad matcher
+              // from matching on non-locale paths. For us this becomes request entry point, so we need to narrow it down to just defined locales
+              // otherwise users might get unexpected matches on paths like `/api*`
+              regexp: matcher.regexp.replace(/\[\^\/\.]+/g, `(${i18NConfig.locales.join('|')})`),
+            }
+          : matcher,
         {
           ...matcher,
           regexp: pathToRegexp(matcher.originalSource).source,

tests/e2e/edge-middleware.test.ts

@@ -69,3 +69,150 @@ test('json data rewrite works', async ({ middlewarePages }) => {
 
   expect(data.pageProps.message).toBeDefined()
 })
+
+// those tests use `fetch` instead of `page.goto` intentionally to avoid potential client rendering
+// hiding any potential edge/server issues
+test.describe('Middleware with i18n and excluded paths', () => {
+  const DEFAULT_LOCALE = 'en'
+
+  /** helper function to extract JSON data from page rendering data with `<pre>{JSON.stringify(data)}</pre>` */
+  function extractDataFromHtml(html: string): Record<string, any> {
+    const match = html.match(/<pre>(?<rawInput>[^<]+)<\/pre>/)
+    if (!match || !match.groups?.rawInput) {
+      console.error('<pre> not found in html input', {
+        html,
+      })
+      throw new Error('Failed to extract data from HTML')
+    }
+
+    const { rawInput } = match.groups
+    const unescapedInput = rawInput.replaceAll('&quot;', '"')
+    try {
+      return JSON.parse(unescapedInput)
+    } catch (originalError) {
+      console.error('Failed to parse JSON', {
+        originalError,
+        rawInput,
+        unescapedInput,
+      })
+    }
+    throw new Error('Failed to extract data from HTML')
+  }
+
+  // those tests hit paths ending with `/json` which has special handling in middleware
+  // to return JSON response from middleware itself
+  test.describe('Middleware response path', () => {
+    test('should match on non-localized not excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/json`)
+
+      expect(response.headers.get('x-test-used-middleware')).toBe('true')
+      expect(response.status).toBe(200)
+
+      const { nextUrlPathname, nextUrlLocale } = await response.json()
+
+      expect(nextUrlPathname).toBe('/json')
+      expect(nextUrlLocale).toBe(DEFAULT_LOCALE)
+    })
+
+    test('should match on localized not excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/fr/json`)
+
+      expect(response.headers.get('x-test-used-middleware')).toBe('true')
+      expect(response.status).toBe(200)
+
+      const { nextUrlPathname, nextUrlLocale } = await response.json()
+
+      expect(nextUrlPathname).toBe('/json')
+      expect(nextUrlLocale).toBe('fr')
+    })
+  })
+
+  // those tests hit paths that don't end with `/json` while still satisfying middleware matcher
+  // so middleware should pass them through to origin
+  test.describe('Middleware passthrough', () => {
+    test('should match on non-localized not excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/html`)
+
+      expect(response.headers.get('x-test-used-middleware')).toBe('true')
+      expect(response.status).toBe(200)
+      expect(response.headers.get('content-type')).toMatch(/text\/html/)
+
+      const html = await response.text()
+      const { locale, params } = extractDataFromHtml(html)
+
+      expect(params).toMatchObject({ catchall: ['html'] })
+      expect(locale).toBe(DEFAULT_LOCALE)
+    })
+
+    test('should match on localized not excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/fr/html`)
+
+      expect(response.headers.get('x-test-used-middleware')).toBe('true')
+      expect(response.status).toBe(200)
+      expect(response.headers.get('content-type')).toMatch(/text\/html/)
+
+      const html = await response.text()
+      const { locale, params } = extractDataFromHtml(html)
+
+      expect(params).toMatchObject({ catchall: ['html'] })
+      expect(locale).toBe('fr')
+    })
+  })
+
+  // those tests hit paths that don't satisfy middleware matcher, so should go directly to origin
+  // without going through middleware
+  test.describe('Middleware skipping (paths not satisfying middleware matcher)', () => {
+    test('should NOT match on non-localized excluded API path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/api/html`)
+
+      expect(response.headers.get('x-test-used-middleware')).not.toBe('true')
+      expect(response.status).toBe(200)
+
+      const { params } = await response.json()
+
+      expect(params).toMatchObject({ catchall: ['html'] })
+    })
+
+    test('should NOT match on non-localized excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/excluded`)
+
+      expect(response.headers.get('x-test-used-middleware')).not.toBe('true')
+      expect(response.status).toBe(200)
+      expect(response.headers.get('content-type')).toMatch(/text\/html/)
+
+      const html = await response.text()
+      const { locale, params } = extractDataFromHtml(html)
+
+      expect(params).toMatchObject({ catchall: ['excluded'] })
+      expect(locale).toBe(DEFAULT_LOCALE)
+    })
+
+    test('should NOT match on localized excluded page path', async ({
+      middlewareI18nExcludedPaths,
+    }) => {
+      const response = await fetch(`${middlewareI18nExcludedPaths.url}/fr/excluded`)
+
+      expect(response.headers.get('x-test-used-middleware')).not.toBe('true')
+      expect(response.status).toBe(200)
+      expect(response.headers.get('content-type')).toMatch(/text\/html/)
+
+      const html = await response.text()
+      const { locale, params } = extractDataFromHtml(html)
+
+      expect(params).toMatchObject({ catchall: ['excluded'] })
+      expect(locale).toBe('fr')
+    })
+  })
+})

tests/fixtures/middleware-i18n-excluded-paths/middleware.ts

@@ -0,0 +1,36 @@
+import { NextResponse } from 'next/server'
+import type { NextRequest } from 'next/server'
+
+export async function middleware(request: NextRequest) {
+  const url = request.nextUrl
+
+  // if path ends with /json we create response in middleware, otherwise we pass it through
+  // to next server to get page or api response from it
+  const response = url.pathname.includes('/json')
+    ? NextResponse.json({
+        requestUrlPathname: new URL(request.url).pathname,
+        nextUrlPathname: request.nextUrl.pathname,
+        nextUrlLocale: request.nextUrl.locale,
+      })
+    : NextResponse.next()
+
+  response.headers.set('x-test-used-middleware', 'true')
+
+  return response
+}
+
+// matcher copied from example in https://nextjs.org/docs/pages/building-your-application/routing/middleware#matcher
+// with `excluded` segment added to exclusion
+export const config = {
+  matcher: [
+    /*
+     * Match all request paths except for the ones starting with:
+     * - api (API routes)
+     * - excluded (for testing localized routes and not just API routes)
+     * - _next/static (static files)
+     * - _next/image (image optimization files)
+     * - favicon.ico, sitemap.xml, robots.txt (metadata files)
+     */
+    '/((?!api|excluded|_next/static|_next/image|favicon.ico|sitemap.xml|robots.txt).*)',
+  ],
+}

tests/fixtures/middleware-i18n-excluded-paths/next-env.d.ts

@@ -0,0 +1,5 @@
+/// <reference types="next" />
+/// <reference types="next/image-types/global" />
+
+// NOTE: This file should not be edited
+// see https://nextjs.org/docs/pages/api-reference/config/typescript for more information.

tests/fixtures/middleware-i18n-excluded-paths/next.config.js

@@ -0,0 +1,10 @@
+module.exports = {
+  output: 'standalone',
+  eslint: {
+    ignoreDuringBuilds: true,
+  },
+  i18n: {
+    locales: ['en', 'fr'],
+    defaultLocale: 'en',
+  },
+}

tests/fixtures/middleware-i18n-excluded-paths/package.json

@@ -0,0 +1,20 @@
+{
+  "name": "middleware-i18n-excluded-paths",
+  "version": "0.1.0",
+  "private": true,
+  "scripts": {
+    "postinstall": "next build",
+    "dev": "next dev",
+    "build": "next build"
+  },
+  "dependencies": {
+    "next": "latest",
+    "react": "18.2.0",
+    "react-dom": "18.2.0"
+  },
+  "devDependencies": {
+    "@types/node": "^17.0.12",
+    "@types/react": "18.2.47",
+    "typescript": "^5.2.2"
+  }
+}

tests/fixtures/middleware-i18n-excluded-paths/pages/[[...catchall]].tsx

@@ -0,0 +1,21 @@
+import type { GetStaticPaths, GetStaticProps } from 'next'
+
+export default function CatchAll({ params, locale }) {
+  return <pre>{JSON.stringify({ params, locale }, null, 2)}</pre>
+}
+
+export const getStaticPaths: GetStaticPaths = () => {
+  return {
+    paths: [],
+    fallback: 'blocking',
+  }
+}
+
+export const getStaticProps: GetStaticProps = ({ params, locale }) => {
+  return {
+    props: {
+      params,
+      locale,
+    },
+  }
+}

tests/fixtures/middleware-i18n-excluded-paths/pages/api/[[...catchall]].ts

@@ -0,0 +1,11 @@
+import type { NextApiRequest, NextApiResponse } from 'next'
+
+type ResponseData = {
+  params: {
+    catchall?: string[]
+  }
+}
+
+export default function handler(req: NextApiRequest, res: NextApiResponse<ResponseData>) {
+  res.status(200).json({ params: req.query })
+}

tests/fixtures/middleware-i18n-excluded-paths/tsconfig.json

@@ -0,0 +1,19 @@
+{
+  "compilerOptions": {
+    "target": "ES2017",
+    "lib": ["dom", "dom.iterable", "esnext"],
+    "allowJs": true,
+    "skipLibCheck": true,
+    "strict": false,
+    "noEmit": true,
+    "incremental": true,
+    "module": "esnext",
+    "esModuleInterop": true,
+    "moduleResolution": "node",
+    "resolveJsonModule": true,
+    "isolatedModules": true,
+    "jsx": "preserve"
+  },
+  "include": ["next-env.d.ts", "**/*.ts", "**/*.tsx"],
+  "exclude": ["node_modules"]
+}

tests/prepare.mjs

@@ -23,6 +23,7 @@ const e2eOnlyFixtures = new Set([
   'after',
   'cli-before-regional-blobs-support',
   'dist-dir',
+  'middleware-i18n-excluded-paths',
   // There is also a bug on Windows on Node.js 18.20.6, that cause build failures on this fixture
   // see https://github.com/opennextjs/opennextjs-netlify/actions/runs/13268839161/job/37043172448?pr=2749#step:12:78
   'middleware-og',

tests/utils/create-e2e-fixture.ts

@@ -333,6 +333,7 @@ export const fixtureFactories = {
   pnpm: () => createE2EFixture('pnpm', { packageManger: 'pnpm' }),
   bun: () => createE2EFixture('simple', { packageManger: 'bun' }),
   middleware: () => createE2EFixture('middleware'),
+  middlewareI18nExcludedPaths: () => createE2EFixture('middleware-i18n-excluded-paths'),
   middlewareOg: () => createE2EFixture('middleware-og'),
   middlewarePages: () => createE2EFixture('middleware-pages'),
   pageRouter: () => createE2EFixture('page-router'),