Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

short-circuit OPTIONS requests to pages #65295

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 54 additions & 38 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,10 @@ import { getTracer, isBubbledError, SpanKind } from './lib/trace/tracer'
import { BaseServerSpan } from './lib/trace/constants'
import { I18NProvider } from './future/helpers/i18n-provider'
import { sendResponse } from './send-response'
import { handleInternalServerErrorResponse } from './future/route-modules/helpers/response-handlers'
import {
handleBadRequestResponse,
handleInternalServerErrorResponse,
} from './future/route-modules/helpers/response-handlers'
import {
fromNodeOutgoingHttpHeaders,
normalizeNextQueryParam,
Expand Down Expand Up @@ -2466,46 +2469,59 @@ export default abstract class Server<

return null
}
} else if (isPagesRouteModule(routeModule)) {
// Due to the way we pass data by mutating `renderOpts`, we can't extend
// the object here but only updating its `clientReferenceManifest` and
// `nextFontManifest` properties.
// https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952
renderOpts.nextFontManifest = this.nextFontManifest
renderOpts.clientReferenceManifest =
components.clientReferenceManifest

const request = isNodeNextRequest(req) ? req.originalRequest : req
const response = isNodeNextResponse(res) ? res.originalResponse : res

// Call the built-in render method on the module.
result = await routeModule.render(
// TODO: fix this type
// @ts-expect-error - preexisting accepted this
request,
response,
{
page: pathname,
} else if (
isPagesRouteModule(routeModule) ||
isAppPageRouteModule(routeModule)
) {
// An OPTIONS request to a page handler is invalid.
if (req.method === 'OPTIONS' && !is404Page) {
await sendResponse(req, res, handleBadRequestResponse())
return null
}

if (isPagesRouteModule(routeModule)) {
// Due to the way we pass data by mutating `renderOpts`, we can't extend
// the object here but only updating its `clientReferenceManifest` and
// `nextFontManifest` properties.
// https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952
renderOpts.nextFontManifest = this.nextFontManifest
renderOpts.clientReferenceManifest =
components.clientReferenceManifest

const request = isNodeNextRequest(req) ? req.originalRequest : req
const response = isNodeNextResponse(res)
? res.originalResponse
: res

// Call the built-in render method on the module.
result = await routeModule.render(
// TODO: fix this type
// @ts-expect-error - preexisting accepted this
request,
response,
{
page: pathname,
params: opts.params,
query,
renderOpts,
}
)
} else {
const module = components.routeModule as AppPageRouteModule

// Due to the way we pass data by mutating `renderOpts`, we can't extend the
// object here but only updating its `nextFontManifest` field.
// https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952
renderOpts.nextFontManifest = this.nextFontManifest

// Call the built-in render method on the module.
result = await module.render(req, res, {
page: is404Page ? '/404' : pathname,
params: opts.params,
query,
renderOpts,
}
)
} else if (isAppPageRouteModule(routeModule)) {
const module = components.routeModule as AppPageRouteModule

// Due to the way we pass data by mutating `renderOpts`, we can't extend the
// object here but only updating its `nextFontManifest` field.
// https://github.com/vercel/next.js/blob/df7cbd904c3bd85f399d1ce90680c0ecf92d2752/packages/next/server/render.tsx#L947-L952
renderOpts.nextFontManifest = this.nextFontManifest

// Call the built-in render method on the module.
result = await module.render(req, res, {
page: is404Page ? '/404' : pathname,
params: opts.params,
query,
renderOpts,
})
})
}
} else {
throw new Error('Invariant: Unknown route module type')
}
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/app-dir/options-request/app/app-page/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>foo page</div>
}
5 changes: 5 additions & 0 deletions test/e2e/app-dir/options-request/app/app-route/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const dynamic = 'force-dynamic'

export async function GET() {
return Response.json({ foo: 'bar' })
}
7 changes: 7 additions & 0 deletions test/e2e/app-dir/options-request/app/layout.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
export default function Root({ children }: { children: React.ReactNode }) {
return (
<html>
<body>{children}</body>
</html>
)
}
41 changes: 41 additions & 0 deletions test/e2e/app-dir/options-request/app/page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
'use client'
export default function Page() {
return (
<>
<button
onClick={async () => {
const response = await fetch('/app-route', { method: 'OPTIONS' })
console.log(await response.json())
}}
>
Trigger Options Request (/app route)
</button>
<button
onClick={async () => {
const response = await fetch('/app-page', { method: 'OPTIONS' })
console.log(await response.text())
}}
>
Trigger Options Request (/app page)
</button>
<button
onClick={async () => {
const response = await fetch('/pages-page', { method: 'OPTIONS' })
console.log(await response.text())
}}
>
Trigger Options Request (/pages page)
</button>
<button
onClick={async () => {
const response = await fetch('/api/pages-api-handler', {
method: 'OPTIONS',
})
console.log(await response.text())
}}
>
Trigger Options Request (/pages API route)
</button>
</>
)
}
6 changes: 6 additions & 0 deletions test/e2e/app-dir/options-request/next.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/**
* @type {import('next').NextConfig}
*/
const nextConfig = {}

module.exports = nextConfig
40 changes: 40 additions & 0 deletions test/e2e/app-dir/options-request/options-request.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { nextTestSetup } from 'e2e-utils'

describe('options-request', () => {
const { next } = nextTestSetup({
files: __dirname,
})

it.each(['/app-page', '/pages-page'])(
'should return a 400 status code when invoking %s with an OPTIONS request',
async (path) => {
const res = await next.fetch(path, { method: 'OPTIONS' })
expect(res.status).toBe(400)
// There should be no response body
expect(await res.text()).toBe('')
}
)

// In app router, OPTIONS is auto-implemented if not provided
it('should respond with a 204 No Content when invoking an app route handler with an OPTIONS request', async () => {
const res = await next.fetch('/app-route', { method: 'OPTIONS' })
expect(res.status).toBe(204)
// There should be no response body
expect(await res.text()).toBe('')
})

// In pages router, unless the handler explicitly handles OPTIONS, it will handle the request normally
it('should respond with a 200 + response body when invoking a pages API route with an OPTIONS request', async () => {
const res = await next.fetch('/api/pages-api-handler', {
method: 'OPTIONS',
})
expect(res.status).toBe(200)
// There should be no response body
expect((await res.json()).message).toBe('Hello from Next.js!')
})

it('should 404 for an OPTIONS request to a non-existent route', async () => {
const res = await next.fetch('/non-existent-route', { method: 'OPTIONS' })
expect(res.status).toBe(404)
})
})
12 changes: 12 additions & 0 deletions test/e2e/app-dir/options-request/pages/api/pages-api-handler.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import type { NextApiRequest, NextApiResponse } from 'next'

type ResponseData = {
message: string
}

export default function handler(
req: NextApiRequest,
res: NextApiResponse<ResponseData>
) {
res.status(200).json({ message: 'Hello from Next.js!' })
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/options-request/pages/pages-page.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function Page() {
return <div>bar</div>
}
Loading