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

Mask Flight Parameters from Middleware #39939

Merged
merged 22 commits into from Sep 15, 2022
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
c7c84ff
fix: mask flight parameters from middleware
wyattjoh Aug 25, 2022
91aa0ac
fix: fixed ts types
wyattjoh Aug 25, 2022
cc28c24
fix: moved internal utils to separate file to avoid poluting edge run…
wyattjoh Aug 25, 2022
5eda432
fix: fixed tests
wyattjoh Aug 25, 2022
1910e48
fix: added middleware file to tests via files ref
wyattjoh Aug 26, 2022
675e862
fix: added extedned internal param removal
wyattjoh Aug 26, 2022
adeba5e
Merge branch 'canary' of github.com:vercel/next.js into fix/mask-flig…
timneutkens Sep 14, 2022
8a62c5e
Ensure prefetch error does not crash the app
timneutkens Sep 14, 2022
a8518bd
Remove __props__ special case
timneutkens Sep 14, 2022
98fd351
Add __flight_prefetch__
timneutkens Sep 14, 2022
ffa5b43
Rename flightData -> flightSearchParameters
timneutkens Sep 14, 2022
6121486
Ensure flightSearchParameters are added when creating href
timneutkens Sep 15, 2022
400e841
Ensure test for rewrites is correct
timneutkens Sep 15, 2022
0c77102
Combine rewrites tests in describe. Replace browser.eval
timneutkens Sep 15, 2022
6a3c812
Bring back getter for flightSearchParameters
timneutkens Sep 15, 2022
7a877d0
Ensure flightSearchParameters is not undefined
timneutkens Sep 15, 2022
3fd50e8
Update TODO: to TODO-APP:. Skip failing test
timneutkens Sep 15, 2022
7288eb6
Bring back changes to setter
timneutkens Sep 15, 2022
e0be34f
Fix edge-runtime ssr breaking
timneutkens Sep 15, 2022
941dd8b
Use it.each
timneutkens Sep 15, 2022
e5724de
Fix additional flight parameter being applied accidentally
timneutkens Sep 15, 2022
d8de7d2
Merge branch 'canary' into fix/mask-flight-parameters
timneutkens Sep 15, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 23 additions & 19 deletions packages/next/client/components/app-router.client.tsx
Expand Up @@ -202,28 +202,32 @@ export default function AppRouter({
}

prefetched.add(href)

const url = new URL(href, location.origin)
// TODO-APP: handle case where history.state is not the new router history entry
const r = fetchServerResponse(
url,
// initialTree is used when history.state.tree is missing because the history state is set in `useEffect` below, it being missing means this is the hydration case.
window.history.state?.tree || initialTree,
true
)

try {
r.readRoot()
} catch (e) {
await e
const flightData = r.readRoot()
// @ts-ignore startTransition exists
React.startTransition(() => {
dispatch({
type: ACTION_PREFETCH,
url,
flightData,
// TODO-APP: handle case where history.state is not the new router history entry
timneutkens marked this conversation as resolved.
Show resolved Hide resolved
const r = fetchServerResponse(
url,
// initialTree is used when history.state.tree is missing because the history state is set in `useEffect` below, it being missing means this is the hydration case.
window.history.state?.tree || initialTree,
true
)
try {
r.readRoot()
} catch (e) {
await e
const flightData = r.readRoot()
// @ts-ignore startTransition exists
React.startTransition(() => {
dispatch({
type: ACTION_PREFETCH,
url,
flightData,
})
})
})
}
} catch (err) {
console.error('PREFETCH ERROR', err)
}
},
replace: (href, options = {}) => {
Expand Down
3 changes: 2 additions & 1 deletion packages/next/server/app-render.tsx
Expand Up @@ -18,14 +18,15 @@ import {
} from './node-web-streams-helper'
import { isDynamicRoute } from '../shared/lib/router/utils'
import { ESCAPE_REGEX, htmlEscapeJsonString } from './htmlescape'
import { shouldUseReactRoot, stripInternalQueries } from './utils'
import { shouldUseReactRoot } from './utils'
import { NextApiRequestCookies } from './api-utils'
import { matchSegment } from '../client/components/match-segments'
import {
FlightCSSManifest,
FlightManifest,
} from '../build/webpack/plugins/flight-manifest-plugin'
import { FlushEffectsContext } from '../client/components/hooks-client'
import { stripInternalQueries } from './internal-utils'
import type { ComponentsType } from '../build/webpack/loaders/next-app-loader'

// this needs to be required lazily so that `next-server` can set
Expand Down
38 changes: 38 additions & 0 deletions packages/next/server/internal-utils.ts
@@ -0,0 +1,38 @@
import type { NextParsedUrlQuery } from './request-meta'

const INTERNAL_QUERY_NAMES = [
'__nextFallback',
'__nextLocale',
'__nextDefaultLocale',
'__nextIsNotFound',
// RSC
'__flight__',
'__props__',
// Routing
'__flight_router_state_tree__',
] as const

const EXTENDED_INTERNAL_QUERY_NAMES = ['__nextDataReq'] as const

export function stripInternalQueries(query: NextParsedUrlQuery) {
for (const name of INTERNAL_QUERY_NAMES) {
delete query[name]
}
}

export function stripInternalSearchParams(
searchParams: URLSearchParams,
extended?: boolean
) {
for (const name of INTERNAL_QUERY_NAMES) {
searchParams.delete(name)
}

if (extended) {
for (const name of EXTENDED_INTERNAL_QUERY_NAMES) {
searchParams.delete(name)
}
}

return searchParams
}
3 changes: 2 additions & 1 deletion packages/next/server/render.tsx
Expand Up @@ -79,7 +79,8 @@ import {
} from './node-web-streams-helper'
import { ImageConfigContext } from '../shared/lib/image-config-context'
import stripAnsi from 'next/dist/compiled/strip-ansi'
import { shouldUseReactRoot, stripInternalQueries } from './utils'
import { shouldUseReactRoot } from './utils'
import { stripInternalQueries } from './internal-utils'

let tryGetPreviewData: typeof import('./api-utils/node').tryGetPreviewData
let warn: typeof import('../build/output/log').warn
Expand Down
14 changes: 0 additions & 14 deletions packages/next/server/utils.ts
@@ -1,4 +1,3 @@
import type { NextParsedUrlQuery } from './request-meta'
import React from 'react'
import { BLOCKED_PAGES } from '../shared/lib/constants'

Expand All @@ -23,18 +22,5 @@ export function isTargetLikeServerless(target: string) {
return isServerless || isServerlessTrace
}

export function stripInternalQueries(query: NextParsedUrlQuery) {
delete query.__nextFallback
delete query.__nextLocale
delete query.__nextDefaultLocale
delete query.__nextIsNotFound

// RSC
delete query.__flight__
delete query.__props__
// routing
delete query.__flight_router_state_tree__
}

// When react version is >= 18 opt-in using reactRoot
export const shouldUseReactRoot = parseInt(React.version) >= 18
15 changes: 9 additions & 6 deletions packages/next/server/web/adapter.ts
Expand Up @@ -8,6 +8,7 @@ import { NextResponse } from './spec-extension/response'
import { relativizeURL } from '../../shared/lib/router/utils/relativize-url'
import { waitUntilSymbol } from './spec-extension/fetch-event'
import { NextURL } from './next-url'
import { stripInternalSearchParams } from '../internal-utils'

class NextRequestHint extends NextRequest {
sourcePage: string
Expand Down Expand Up @@ -54,12 +55,12 @@ export async function adapter(params: {
requestUrl.pathname = '/'
}

// clean-up any internal query params
for (const key of [...requestUrl.searchParams.keys()]) {
if (key.startsWith('__next')) {
requestUrl.searchParams.delete(key)
}
}
// Preserve flight data.
const flightData = requestUrl.flightData
requestUrl.flightData = undefined

// Strip internal query parameters off the request.
stripInternalSearchParams(requestUrl.searchParams, true)

const request = new NextRequestHint({
page: params.page,
Expand Down Expand Up @@ -105,6 +106,7 @@ export async function adapter(params: {

if (rewriteUrl.host === request.nextUrl.host) {
rewriteUrl.buildId = buildId || rewriteUrl.buildId
rewriteUrl.flightData = flightData || rewriteUrl.flightData
response.headers.set('x-middleware-rewrite', String(rewriteUrl))
}

Expand Down Expand Up @@ -142,6 +144,7 @@ export async function adapter(params: {

if (redirectURL.host === request.nextUrl.host) {
redirectURL.buildId = buildId || redirectURL.buildId
redirectURL.flightData = flightData || redirectURL.flightData
response.headers.set('Location', String(redirectURL))
}

Expand Down
50 changes: 50 additions & 0 deletions packages/next/server/web/next-url.ts
Expand Up @@ -15,6 +15,12 @@ interface Options {
}
}

const FLIGHT_PARAMETERS = [
'__flight__',
'__props__',
'__flight_router_state_tree__',
] as const

const REGEX_LOCALHOST_HOSTNAME =
/(?!^https?:\/\/)(127(?:\.(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}|::1|localhost)/

Expand All @@ -25,12 +31,35 @@ function parseURL(url: string | URL, base?: string | URL) {
)
}

function parseFlightParameters(
searchParams: URLSearchParams
): Record<string, string> | undefined {
let flightData: Record<string, string> = {}
let flightDataUpdated = false
for (const name of FLIGHT_PARAMETERS) {
const value = searchParams.get(name)
if (value === null) {
continue
}

flightData[name] = value
flightDataUpdated = true
}

if (!flightDataUpdated) {
return undefined
}

return flightData
}

const Internal = Symbol('NextURLInternal')

export class NextURL {
[Internal]: {
basePath: string
buildId?: string
flightData?: Record<string, string>
defaultLocale?: string
domainLocale?: DomainLocale
locale?: string
Expand Down Expand Up @@ -89,6 +118,9 @@ export class NextURL {
this[Internal].buildId = pathnameInfo.buildId
this[Internal].locale = pathnameInfo.locale ?? defaultLocale
this[Internal].trailingSlash = pathnameInfo.trailingSlash
this[Internal].flightData = parseFlightParameters(
this[Internal].url.searchParams
)
}

private formatPathname() {
Expand All @@ -112,6 +144,24 @@ export class NextURL {
this[Internal].buildId = buildId
}

public get flightData() {
return this[Internal].flightData
}

public set flightData(flightData: Record<string, string> | undefined) {
if (flightData) {
for (const name of FLIGHT_PARAMETERS) {
this[Internal].url.searchParams.set(name, flightData[name] ?? '')
}
} else {
for (const name of FLIGHT_PARAMETERS) {
this[Internal].url.searchParams.delete(name)
}
}

this[Internal].flightData = flightData
}

public get locale() {
return this[Internal].locale ?? ''
}
Expand Down
3 changes: 3 additions & 0 deletions test/e2e/app-dir/app/app/internal/failure/page.server.js
@@ -0,0 +1,3 @@
export default function Page() {
return <div id="failure">Failure</div>
}
18 changes: 18 additions & 0 deletions test/e2e/app-dir/app/app/internal/page.server.js
@@ -0,0 +1,18 @@
import Link from 'next/link'

export default function Page() {
return (
<div>
<div>
<Link href="/internal/test/rewrite">
<a id="navigate-rewrite">Navigate Rewrite</a>
</Link>
</div>
<div>
<Link href="/internal/test/redirect">
<a id="navigate-redirect">Navigate Redirect</a>
</Link>
</div>
</div>
)
}
3 changes: 3 additions & 0 deletions test/e2e/app-dir/app/app/internal/success/page.server.js
@@ -0,0 +1,3 @@
export default function Page() {
return <div id="success">Success</div>
}
19 changes: 18 additions & 1 deletion test/e2e/app-dir/app/middleware.js
@@ -1,8 +1,25 @@
// @ts-check
import { NextResponse } from 'next/server'

/**
* @param {import('next/server').NextRequest} request
* @returns {NextResponse | undefined}
*/
export function middleware(request) {
if (request.nextUrl.pathname === '/middleware-to-dashboard') {
// TODO: this does not copy __flight__ and __flight_router_state_tree__
return NextResponse.rewrite(new URL('/dashboard', request.url))
}

if (request.nextUrl.pathname.startsWith('/internal/test')) {
const method = request.nextUrl.pathname.endsWith('rewrite')
? 'rewrite'
: 'redirect'

const internal = ['__flight__', '__props__', '__flight_router_state_tree__']
if (internal.some((name) => request.nextUrl.searchParams.has(name))) {
return NextResponse[method](new URL('/internal/failure', request.url))
}

return NextResponse[method](new URL('/internal/success', request.url))
}
}
21 changes: 21 additions & 0 deletions test/e2e/app-dir/index.test.ts
Expand Up @@ -733,6 +733,27 @@ describe('app dir', () => {
})
})

describe('middleware', () => {
;['rewrite', 'redirect'].map((method) =>
it(`should strip internal query parameters from requests to middleware for ${method}`, async () => {
const browser = await webdriver(next.url, '/internal')

try {
// Wait for and click the navigation element, this should trigger
// the flight request that'll be caught by the middleware. If the
// middleware sees any flight data on the request it'll redirect to
// a page with an element of #failure, otherwise, we'll see the
// element for #success.
await browser.waitForElementByCss(`#navigate-${method}`)
await browser.elementById(`navigate-${method}`).click()
await browser.waitForElementByCss('#success')
} finally {
await browser.close()
}
})
)
})

describe('next/router', () => {
it('should always return null when accessed from /app', async () => {
const browser = await webdriver(next.url, '/old-router')
Expand Down