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

fix: correctly set x-forwarded-* in Middleware #57815

Merged
merged 17 commits into from
Nov 1, 2023
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
10 changes: 10 additions & 0 deletions packages/next/src/server/base-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ import getRouteFromAssetPath from '../shared/lib/router/utils/get-route-from-ass
import { stripInternalHeaders } from './internal-utils'
import { RSCPathnameNormalizer } from './future/normalizers/request/rsc'
import { PostponedPathnameNormalizer } from './future/normalizers/request/postponed'
import type { TLSSocket } from 'tls'

export type FindComponentsResult = {
components: LoadComponentsReturnType
Expand Down Expand Up @@ -894,6 +895,15 @@ export default abstract class Server<ServerOptions extends Options = Options> {
}
}

req.headers['x-forwarded-host'] ??= `${this.hostname}:${this.port}`
req.headers['x-forwarded-port'] ??= this.port?.toString()
const { originalRequest } = req as NodeNextRequest
req.headers['x-forwarded-proto'] ??= (originalRequest.socket as TLSSocket)
?.encrypted
? 'https'
: 'http'
req.headers['x-forwarded-for'] ??= originalRequest.socket?.remoteAddress

this.attachRequestMeta(req, parsedUrl)

const domainLocale = this.i18nProvider?.detectDomainLocale(
Expand Down
14 changes: 1 addition & 13 deletions packages/next/src/server/internal-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { IncomingHttpHeaders } from 'http'
import type { NextParsedUrlQuery } from './request-meta'

import { NEXT_RSC_UNION_QUERY } from '../client/components/app-router-headers'
import { INTERNAL_HEADERS } from '../shared/lib/constants'

const INTERNAL_QUERY_NAMES = [
'__nextFallback',
Expand Down Expand Up @@ -39,19 +40,6 @@ export function stripInternalSearchParams<T extends string | URL>(
return (isStringUrl ? instance.toString() : instance) as T
}

/**
* Headers that are set by the Next.js server and should be stripped from the
* request headers going to the user's application.
*/
const INTERNAL_HEADERS = [
'x-invoke-path',
'x-invoke-status',
'x-invoke-error',
'x-invoke-query',
'x-invoke-output',
'x-middleware-invoke',
] as const

/**
* Strip internal headers from the request headers.
*
Expand Down
6 changes: 4 additions & 2 deletions packages/next/src/server/lib/mock-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,13 +37,15 @@ export class MockedRequest extends Stream.Readable implements IncomingMessage {
private bodyReadable?: Stream.Readable

// If we don't actually have a socket, we'll just use a mock one that
// always returns false for the `encrypted` property.
// always returns false for the `encrypted` property and undefined for the
// `remoteAddress` property.
public socket: Socket = new Proxy<TLSSocket>({} as TLSSocket, {
get: (_target, prop) => {
if (prop !== 'encrypted') {
if (prop !== 'encrypted' && prop !== 'remoteAddress') {
throw new Error('Method not implemented')
}

if (prop === 'remoteAddress') return undefined
// For this mock request, always ensure we just respond with the encrypted
// set to false to ensure there's no odd leakages.
return false
Expand Down
12 changes: 0 additions & 12 deletions packages/next/src/server/lib/router-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ import {
PERMANENT_REDIRECT_STATUS,
} from '../../shared/lib/constants'
import { DevBundlerService } from './dev-bundler-service'
import type { TLSSocket } from 'tls'

const debug = setupDebug('next:router-server:main')

Expand Down Expand Up @@ -225,17 +224,6 @@ export async function initialize(opts: {
'x-middleware-invoke': '',
'x-invoke-path': invokePath,
'x-invoke-query': encodeURIComponent(JSON.stringify(parsedUrl.query)),
'x-forwarded-host':
req.headers['x-forwarded-host'] ?? req.headers.host ?? opts.hostname,
'x-forwarded-port':
req.headers['x-forwarded-port'] ?? opts.port.toString(),
'x-forwarded-proto':
req.headers['x-forwarded-proto'] ??
(req.socket as TLSSocket).encrypted
? 'https'
: 'http',
'x-forwarded-for':
req.headers['x-forwarded-for'] ?? req.socket.remoteAddress,
...(additionalInvokeHeaders || {}),
}
Object.assign(req.headers, invokeHeaders)
Expand Down
4 changes: 3 additions & 1 deletion packages/next/src/server/lib/router-utils/proxy-request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,13 @@ export async function proxyRequest(
target,
changeOrigin: true,
ignorePath: true,
xfwd: true,
Copy link
Member Author

@balazsorban44 balazsorban44 Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With xfwd: true, http-proxy attempts to set x-forwarded-* headers. Since we are setting these headers higher up in base-server.ts, this should not be necessary to set here anymore, to avoid double setting them, as reported in #52266

See: https://github.com/http-party/node-http-proxy/blob/9b96cd725127a024dabebec6c7ea8c807272223d/lib/http-proxy/passes/web-incoming.js#L58-L86

ws: true,
// we limit proxy requests to 30s by default, in development
// we don't time out WebSocket requests to allow proxying
proxyTimeout: proxyTimeout === null ? undefined : proxyTimeout || 30_000,
headers: {
'x-forwarded-host': req.headers.host || '',
Copy link
Member Author

@balazsorban44 balazsorban44 Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still set this header to respect #17557, but without concating without adding a comma ,. See https://github.com/vercel/next.js/pull/57815/files#r1378313718

},
})

await new Promise((proxyResolve, proxyReject) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import type { TLSSocket } from 'tls'
import type { FsOutput } from './filesystem'
import type { IncomingMessage, ServerResponse } from 'http'
import type { NextConfigComplete } from '../../config-shared'
Expand Down Expand Up @@ -38,6 +37,7 @@ import {
prepareDestination,
} from '../../../shared/lib/router/utils/prepare-destination'
import { createRequestResponseMocks } from '../mock-request'
import type { TLSSocket } from 'tls'

const debug = setupDebug('next:router-server:resolve-routes')

Expand Down
13 changes: 2 additions & 11 deletions packages/next/src/server/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import './node-environment'
import './require-hook'
import './node-polyfill-crypto'

import type { TLSSocket } from 'tls'
import type { CacheFs } from '../shared/lib/utils'
import {
DecodeError,
Expand Down Expand Up @@ -41,7 +40,6 @@ import {
SERVER_DIRECTORY,
NEXT_FONT_MANIFEST,
PHASE_PRODUCTION_BUILD,
INTERNAL_HEADERS,
} from '../shared/lib/constants'
import { findDir } from '../lib/find-pages-dir'
import { NodeNextRequest, NodeNextResponse } from './base-http/node'
Expand Down Expand Up @@ -1616,10 +1614,6 @@ export default class NextNodeServer extends BaseServer {
>
let bubblingResult = false

for (const key of INTERNAL_HEADERS) {
delete req.headers[key]
}

// Strip the internal headers.
this.stripInternalHeaders(req)

Expand Down Expand Up @@ -1746,11 +1740,8 @@ export default class NextNodeServer extends BaseServer {
parsedUrl: NextUrlWithParsedQuery,
isUpgradeReq?: boolean
) {
const protocol =
((req as NodeNextRequest).originalRequest?.socket as TLSSocket)
?.encrypted || req.headers['x-forwarded-proto']?.includes('https')
? 'https'
: 'http'
// Injected in base-server.ts
const protocol = req.headers['x-forwarded-proto'] as 'https' | 'http'

// When there are hostname and port we build an absolute URL
const initUrl =
Expand Down
12 changes: 1 addition & 11 deletions packages/next/src/server/web/adapter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@ import { waitUntilSymbol } from './spec-extension/fetch-event'
import { NextURL } from './next-url'
import { stripInternalSearchParams } from '../internal-utils'
import { normalizeRscURL } from '../../shared/lib/router/utils/app-paths'
import {
NEXT_ROUTER_PREFETCH,
NEXT_ROUTER_STATE_TREE,
RSC,
} from '../../client/components/app-router-headers'
import { FLIGHT_PARAMETERS } from '../../client/components/app-router-headers'
import { NEXT_QUERY_PARAM_PREFIX } from '../../lib/constants'
import { ensureInstrumentationRegistered } from './globals'
import { RequestAsyncStorageWrapper } from '../async-storage/request-async-storage-wrapper'
Expand Down Expand Up @@ -47,12 +43,6 @@ class NextRequestHint extends NextRequest {
}
}

const FLIGHT_PARAMETERS = [
[RSC],
[NEXT_ROUTER_STATE_TREE],
[NEXT_ROUTER_PREFETCH],
] as const

export type AdapterOptions = {
handler: NextMiddleware
page: string
Expand Down
9 changes: 7 additions & 2 deletions packages/next/src/shared/lib/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,16 @@ export const COMPILER_NAMES = {
edgeServer: 'edge-server',
} as const

/**
* Headers that are set by the Next.js server and should be stripped from the
* request headers going to the user's application.
*/
export const INTERNAL_HEADERS = [
'x-invoke-path',
'x-invoke-status',
'x-invoke-error',
'x-invoke-output',
'x-invoke-path',
'x-invoke-query',
'x-invoke-status',
'x-middleware-invoke',
] as const

Expand Down
14 changes: 14 additions & 0 deletions test/e2e/app-dir/x-forwarded-headers/middleware.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { NextResponse } from 'next/server'

export function middleware(req: Request) {
return NextResponse.next({
headers: {
'middleware-x-forwarded-host':
req.headers.get('x-forwarded-host') ?? 'wrong',
'middleware-x-forwarded-port':
req.headers.get('x-forwarded-port') ?? 'wrong',
'middleware-x-forwarded-proto':
req.headers.get('x-forwarded-proto') ?? 'wrong',
},
})
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ createNextDescribe('x-forwarded-headers', { files: __dirname }, ({ next }) => {
expect(headers['x-forwarded-host']).toBe(url.host)
expect(headers['x-forwarded-port']).toBe(url.port)
expect(headers['x-forwarded-proto']).toBe(url.protocol.replace(':', ''))
console.log(headers)
expect(headers['middleware-x-forwarded-host']).toBe(url.host)
expect(headers['middleware-x-forwarded-port']).toBe(url.port)
expect(headers['middleware-x-forwarded-proto']).toBe(
url.protocol.replace(':', '')
)
})
})
Loading