Skip to content

Commit

Permalink
Refactor server routing (vercel#37725)
Browse files Browse the repository at this point in the history
This PR fixes an issue where we have a middleware that rewrites every single request to the same origin while having `i18n` configured. It would be something like: 

```typescript
import { NextResponse } from 'next/server'

export function middleware(req) {
  return NextResponse.rewrite(req.nextUrl)
}
```

In this case we are going to be adding always the `locale` at the beginning of the destination since it is a rewrite. This causes static assets to not match and the whole application to break. I believe this is a potential footgun so in this PR we are addressing the issue by removing the locale from pathname for those cases where we check against the filesystem (e.g. public folder).

To achieve this change, this PR introduces some preparation changes and then a refactor of the logic in the server router. After this refactor we are going to be relying on properties that can be defined in the `Route` to decide wether or not we should remove the `basePath`, `locale`, etc instead of checking which _type_ of route it is that we are matching.

Overall this simplifies quite a lot the server router. The way we are testing the mentioned issue is by adding a default rewrite in the rewrite tests middleware.
  • Loading branch information
javivelasco authored Jun 16, 2022
1 parent 6ca1de8 commit 2d5d43f
Show file tree
Hide file tree
Showing 12 changed files with 189 additions and 136 deletions.
7 changes: 3 additions & 4 deletions packages/next/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,6 @@ export default abstract class Server<ServerOptions extends Options = Options> {
}

protected generateRoutes(): {
basePath: string
headers: Route[]
rewrites: {
beforeFiles: Route[]
Expand All @@ -719,7 +718,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
pageChecker: PageChecker
useFileSystemPublicRoutes: boolean
dynamicRoutes: DynamicRoutes | undefined
locales: string[]
nextConfig: NextConfig
} {
const publicRoutes = this.generatePublicRoutes()
const imageRoutes = this.generateImageRoutes()
Expand Down Expand Up @@ -834,6 +833,7 @@ export default abstract class Server<ServerOptions extends Options = Options> {
const catchAllRoute: Route = {
match: getPathMatch('/:path*'),
type: 'route',
matchesLocale: true,
name: 'Catchall render',
fn: async (req, res, _params, parsedUrl) => {
let { pathname, query } = parsedUrl
Expand Down Expand Up @@ -899,9 +899,8 @@ export default abstract class Server<ServerOptions extends Options = Options> {
catchAllMiddleware,
useFileSystemPublicRoutes,
dynamicRoutes: this.dynamicRoutes,
basePath: this.nextConfig.basePath,
pageChecker: this.hasPage.bind(this),
locales: this.nextConfig.i18n?.locales || [],
nextConfig: this.nextConfig,
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/next/server/dev/next-dev-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -948,7 +948,6 @@ export default class DevServer extends Server {
fsRoutes.push({
match: getPathMatch('/:path*'),
type: 'route',
requireBasePath: false,
name: 'catchall public directory route',
fn: async (req, res, params, parsedUrl) => {
const { pathname } = parsedUrl
Expand Down
11 changes: 9 additions & 2 deletions packages/next/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,7 @@ export default class NextNodeServer extends BaseServer {
return [
{
match: getPathMatch('/:path*'),
matchesBasePath: true,
name: 'public folder catchall',
fn: async (req, res, params, parsedUrl) => {
const pathParts: string[] = params.path || []
Expand Down Expand Up @@ -973,7 +974,7 @@ export default class NextNodeServer extends BaseServer {
let fallback: Route[] = []

if (!this.minimalMode) {
const buildRewrite = (rewrite: Rewrite, check = true) => {
const buildRewrite = (rewrite: Rewrite, check = true): Route => {
const rewriteRoute = getCustomRoute({
type: 'rewrite',
rule: rewrite,
Expand All @@ -985,6 +986,10 @@ export default class NextNodeServer extends BaseServer {
type: rewriteRoute.type,
name: `Rewrite route ${rewriteRoute.source}`,
match: rewriteRoute.match,
matchesBasePath: true,
matchesLocale: true,
matchesLocaleAPIRoutes: true,
matchesTrailingSlash: true,
fn: async (req, res, params, parsedUrl) => {
const { newUrl, parsedDestination } = prepareDestination({
appendParamsToQuery: true,
Expand All @@ -1011,7 +1016,7 @@ export default class NextNodeServer extends BaseServer {
query: parsedDestination.query,
}
},
} as Route
}
}

if (Array.isArray(this.customRoutes.rewrites)) {
Expand Down Expand Up @@ -1264,6 +1269,8 @@ export default class NextNodeServer extends BaseServer {

return {
match: getPathMatch('/:path*'),
matchesBasePath: true,
matchesLocale: true,
type: 'route',
name: 'middleware catchall',
fn: async (req, res, _params, parsed) => {
Expand Down
177 changes: 76 additions & 101 deletions packages/next/server/router.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import type { NextConfig } from './config'
import type { ParsedUrlQuery } from 'querystring'
import type { BaseNextRequest, BaseNextResponse } from './base-http'
import type {
Expand All @@ -13,6 +14,8 @@ import { RouteHas } from '../lib/load-custom-routes'
import { matchHas } from '../shared/lib/router/utils/prepare-destination'
import { removePathPrefix } from '../shared/lib/router/utils/remove-path-prefix'
import { getRequestMeta } from './request-meta'
import { formatNextPathnameInfo } from '../shared/lib/router/utils/format-next-pathname-info'
import { getNextPathnameInfo } from '../shared/lib/router/utils/get-next-pathname-info'

type RouteResult = {
finished: boolean
Expand All @@ -27,7 +30,10 @@ export type Route = {
check?: boolean
statusCode?: number
name: string
requireBasePath?: false
matchesBasePath?: true
matchesLocale?: true
matchesLocaleAPIRoutes?: true
matchesTrailingSlash?: true
internal?: true
fn: (
req: BaseNextRequest,
Expand All @@ -41,10 +47,7 @@ export type DynamicRoutes = Array<{ page: string; match: RouteMatch }>

export type PageChecker = (pathname: string) => Promise<boolean>

const customRouteTypes = new Set(['rewrite', 'redirect', 'header'])

export default class Router {
basePath: string
headers: Route[]
fsRoutes: Route[]
redirects: Route[]
Expand All @@ -58,11 +61,10 @@ export default class Router {
pageChecker: PageChecker
dynamicRoutes: DynamicRoutes
useFileSystemPublicRoutes: boolean
locales: string[]
seenRequests: Set<any>
nextConfig: NextConfig

constructor({
basePath = '',
headers = [],
fsRoutes = [],
rewrites = {
Expand All @@ -76,9 +78,8 @@ export default class Router {
dynamicRoutes = [],
pageChecker,
useFileSystemPublicRoutes,
locales = [],
nextConfig,
}: {
basePath: string
headers: Route[]
fsRoutes: Route[]
rewrites: {
Expand All @@ -92,9 +93,9 @@ export default class Router {
dynamicRoutes: DynamicRoutes | undefined
pageChecker: PageChecker
useFileSystemPublicRoutes: boolean
locales: string[]
nextConfig: NextConfig
}) {
this.basePath = basePath
this.nextConfig = nextConfig
this.headers = headers
this.fsRoutes = fsRoutes
this.rewrites = rewrites
Expand All @@ -104,10 +105,17 @@ export default class Router {
this.catchAllMiddleware = catchAllMiddleware
this.dynamicRoutes = dynamicRoutes
this.useFileSystemPublicRoutes = useFileSystemPublicRoutes
this.locales = locales
this.seenRequests = new Set()
}

get locales() {
return this.nextConfig.i18n?.locales || []
}

get basePath() {
return this.nextConfig.basePath || ''
}

setDynamicRoutes(routes: DynamicRoutes = []) {
this.dynamicRoutes = routes
}
Expand Down Expand Up @@ -228,7 +236,6 @@ export default class Router {
{
type: 'route',
name: 'page checker',
requireBasePath: false,
match: getPathMatch('/:path*'),
fn: async (
checkerReq,
Expand Down Expand Up @@ -262,7 +269,6 @@ export default class Router {
{
type: 'route',
name: 'dynamic route/page check',
requireBasePath: false,
match: getPathMatch('/:path*'),
fn: async (
_checkerReq,
Expand All @@ -283,131 +289,100 @@ export default class Router {
// disabled
...(this.useFileSystemPublicRoutes ? [this.catchAllRoute] : []),
]
const originallyHadBasePath =
!this.basePath || getRequestMeta(req, '_nextHadBasePath')

for (const testRoute of allRoutes) {
// if basePath is being used, the basePath will still be included
// in the pathname here to allow custom-routes to require containing
// it or not, filesystem routes and pages must always include the basePath
// if it is set
let currentPathname = parsedUrlUpdated.pathname as string
const originalPathname = currentPathname
const requireBasePath = testRoute.requireBasePath !== false
const isCustomRoute = customRouteTypes.has(testRoute.type)
const isPublicFolderCatchall =
testRoute.name === 'public folder catchall'
const isMiddlewareCatchall = testRoute.name === 'middleware catchall'
const keepBasePath =
isCustomRoute || isPublicFolderCatchall || isMiddlewareCatchall
const keepLocale = isCustomRoute

const currentPathnameNoBasePath = removePathPrefix(
currentPathname,
this.basePath
)

if (!keepBasePath) {
currentPathname = currentPathnameNoBasePath
const originalPathname = parsedUrlUpdated.pathname as string
const pathnameInfo = getNextPathnameInfo(originalPathname, {
nextConfig: this.nextConfig,
parseData: false,
})

if (
pathnameInfo.locale &&
!testRoute.matchesLocaleAPIRoutes &&
pathnameInfo.pathname.match(/^\/api(?:\/|$)/)
) {
continue
}

const localePathResult = normalizeLocalePath(
currentPathnameNoBasePath,
this.locales
)
if (getRequestMeta(req, '_nextHadBasePath')) {
pathnameInfo.basePath = this.basePath
}

const activeBasePath = keepBasePath ? this.basePath : ''
const basePath = pathnameInfo.basePath
if (!testRoute.matchesBasePath) {
pathnameInfo.basePath = ''
}

// don't match API routes when they are locale prefixed
// e.g. /api/hello shouldn't match /en/api/hello as a page
// rewrites/redirects can match though
if (
!isCustomRoute &&
localePathResult.detectedLocale &&
localePathResult.pathname.match(/^\/api(?:\/|$)/)
testRoute.matchesLocale &&
parsedUrl.query.__nextLocale &&
!pathnameInfo.locale
) {
continue
pathnameInfo.locale = parsedUrl.query.__nextLocale
}

if (keepLocale) {
if (
!testRoute.internal &&
parsedUrl.query.__nextLocale &&
!localePathResult.detectedLocale
) {
currentPathname = `${activeBasePath}/${
parsedUrl.query.__nextLocale
}${
currentPathnameNoBasePath === '/' ? '' : currentPathnameNoBasePath
}`
}
if (
!testRoute.matchesLocale &&
pathnameInfo.locale === this.nextConfig.i18n?.defaultLocale &&
pathnameInfo.locale
) {
pathnameInfo.locale = undefined
}

if (
getRequestMeta(req, '__nextHadTrailingSlash') &&
!currentPathname.endsWith('/')
) {
currentPathname += '/'
}
} else {
currentPathname = `${
getRequestMeta(req, '_nextHadBasePath') ? activeBasePath : ''
}${
activeBasePath && currentPathnameNoBasePath === '/'
? ''
: currentPathnameNoBasePath
}`
if (
testRoute.matchesTrailingSlash &&
getRequestMeta(req, '__nextHadTrailingSlash')
) {
pathnameInfo.trailingSlash = true
}

let newParams = testRoute.match(currentPathname)
const matchPathname = formatNextPathnameInfo({
ignorePrefix: true,
...pathnameInfo,
})

let newParams = testRoute.match(matchPathname)
if (testRoute.has && newParams) {
const hasParams = matchHas(req, testRoute.has, parsedUrlUpdated.query)

if (hasParams) {
Object.assign(newParams, hasParams)
} else {
newParams = false
}
}

// Check if the match function matched
if (newParams) {
// since we require basePath be present for non-custom-routes we
// 404 here when we matched an fs route
if (!keepBasePath) {
if (
!originallyHadBasePath &&
!getRequestMeta(req, '_nextDidRewrite')
) {
if (requireBasePath) {
// consider this a non-match so the 404 renders
return false
}
// page checker occurs before rewrites so we need to continue
// to check those since they don't always require basePath
continue
}

parsedUrlUpdated.pathname = currentPathname
}
/**
* If it is a matcher that doesn't match the basePath (like the public
* directory) but Next.js is configured to use a basePath that was
* never there, we consider this an invalid match and keep routing.
*/
if (
newParams &&
this.basePath &&
!testRoute.matchesBasePath &&
!getRequestMeta(req, '_nextDidRewrite') &&
!basePath
) {
continue
}

if (newParams) {
parsedUrlUpdated.pathname = matchPathname
const result = await testRoute.fn(
req,
res,
newParams,
parsedUrlUpdated
)

// The response was handled
if (result.finished) {
return true
}

// since the fs route didn't finish routing we need to re-add the
// basePath to continue checking with the basePath present
if (!keepBasePath) {
parsedUrlUpdated.pathname = originalPathname
}
parsedUrlUpdated.pathname = originalPathname

if (result.pathname) {
parsedUrlUpdated.pathname = result.pathname
Expand Down
Loading

0 comments on commit 2d5d43f

Please sign in to comment.