[16.x] Ensure exotic rewrite param values are properly encoded

https://github.com/vercel/next-js-mirror/pull/138

packages/next/errors.json

@@ -1143,5 +1143,6 @@
   "1142": "Route %s used \\`import('next/root-params').%s()\\` inside \\`generateStaticParams\\`, but the \\`%s\\` parameter was not provided by a parent \\`generateStaticParams\\`. In \\`generateStaticParams\\`, root params are only available for segments nested below the segment that provides them.",
   "1143": "Response body exceeded maximum size of %s bytes",
   "1144": "The Server Reference ID did not match the expected format. Received %s.\\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action",
-  "1145": "Unsupported body type: %s"
+  "1145": "Unsupported body type: %s",
+  "1146": "Invalid target hostname \"%s\" for proxy request, expected \"%s\""
 }

packages/next/src/server/lib/router-utils/proxy-request.ts

@@ -19,6 +19,10 @@ export async function proxyRequest(
   parsedUrl.search = stringifyQuery(req as any, query)
 
   const target = url.format(parsedUrl)
+  // Keep in mind that a WHATWG URL's hostname and the parsedUrl's hostname
+  // are not strictly equal due to lowercasing, IDN translation, IPv4 and IPv6 normalization, etc.
+  // We just make sure this is a valid URL since http-proxy doesn't validate.
+  new URL(target)
   const HttpProxy =
     require('next/dist/compiled/http-proxy') as typeof import('next/dist/compiled/http-proxy')
 

packages/next/src/shared/lib/router/utils/prepare-destination.ts

@@ -264,7 +264,10 @@ export function prepareDestination(args: {
 
   let destHostnameCompiler
   if (destHostname) {
-    destHostnameCompiler = safeCompile(destHostname, { validate: false })
+    destHostnameCompiler = safeCompile(destHostname, {
+      validate: false,
+      encode: encodeURIComponent,
+    })
   }
 
   // update any params in query values

packages/next/taskfile.js

@@ -1192,6 +1192,11 @@ export async function ncc_gzip_size(task, opts) {
     .target('src/compiled/gzip-size')
 }
 externals['http-proxy'] = 'next/dist/compiled/http-proxy'
+// The patched http-proxy (patches/[email protected]) uses Next.js'
+// parseUrl instead of the deprecated url.parse. Keep that import pointing at
+// next's own dist instead of bundling a copy.
+externals['next/dist/shared/lib/router/utils/parse-url'] =
+  'next/dist/shared/lib/router/utils/parse-url'
 export async function ncc_http_proxy(task, opts) {
   await task
     .source(relative(__dirname, require.resolve('http-proxy')))

patches/http-proxy@1.18.1.patch

@@ -1,11 +1,12 @@
 diff --git a/lib/http-proxy/common.js b/lib/http-proxy/common.js
-index 6513e81d80d5250ea249ea833f819ece67897c7e..09143dd1fe4e67885f40ea916a6ea1ef3e3afa19 100644
+index 6513e81d80d5250ea249ea833f819ece67897c7e..b839d0a0cc69674f3d53c1e5eec07ea8cc7c4796 100644
 --- a/lib/http-proxy/common.js
 +++ b/lib/http-proxy/common.js
-@@ -1,9 +1,9 @@
+@@ -1,15 +1,35 @@
  var common   = exports,
-     url      = require('url'),
+-    url      = require('url'),
 -    extend   = require('util')._extend,
++    parseUrl = require('next/dist/shared/lib/router/utils/parse-url').parseUrl,
      required = require('requires-port');
  
  var upgradeHeader = /(^|,)\s*upgrade\s*($|,)/i,
@@ -13,7 +14,33 @@ index 6513e81d80d5250ea249ea833f819ece67897c7e..09143dd1fe4e67885f40ea916a6ea1ef
      isSSL = /^https|wss/;
  
  /**
-@@ -40,10 +40,10 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
+  * Simple Regex for testing if protocol is https
+  */
+ common.isSSL = isSSL;
++
++/**
++ * Parses a url with Next.js' WHATWG-based `parseUrl` and adapts the result to
++ * the legacy `url.parse` shape http-proxy relies on (`host`, `path`, and a
++ * bracket-less `hostname` for IPv6).
++ */
++common.parseUrl = function(urlValue) {
++  var parsed = parseUrl(urlValue);
++  var bracketedHostname = parsed.hostname;
++  return Object.assign(parsed, {
++    host: bracketedHostname
++      ? bracketedHostname + (parsed.port ? ':' + parsed.port : '')
++      : bracketedHostname,
++    hostname:
++      bracketedHostname && bracketedHostname.startsWith('[')
++        ? bracketedHostname.slice(1, -1)
++        : bracketedHostname,
++    path: parsed.pathname + parsed.search,
++  });
++};
+ /**
+  * Copies the right headers from `options` and `req` to
+  * `outgoing` which is then used to fire the proxied
+@@ -40,10 +60,10 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
    );
  
    outgoing.method = options.method || req.method;
@@ -26,7 +53,7 @@ index 6513e81d80d5250ea249ea833f819ece67897c7e..09143dd1fe4e67885f40ea916a6ea1ef
    }
  
    if (options.auth) {
-@@ -61,13 +61,22 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
+@@ -61,13 +81,22 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
  
    outgoing.agent = options.agent || false;
    outgoing.localAddress = options.localAddress;
@@ -50,16 +77,31 @@ index 6513e81d80d5250ea249ea833f819ece67897c7e..09143dd1fe4e67885f40ea916a6ea1ef
      if (typeof outgoing.headers.connection !== 'string'
          || !upgradeHeader.test(outgoing.headers.connection)
         ) { outgoing.headers.connection = 'close'; }
+@@ -80,11 +109,8 @@ common.setupOutgoing = function(outgoing, options, req, forward) {
+     ? (target.path || '')
+     : '';
+ 
+-  //
+-  // Remark: Can we somehow not use url.parse as a perf optimization?
+-  //
+   var outgoingPath = !options.toProxy
+-    ? (url.parse(req.url).path || '')
++    ? (common.parseUrl(req.url).path || '')
+     : req.url;
+ 
+   //
 diff --git a/lib/http-proxy/index.js b/lib/http-proxy/index.js
-index 977a4b3622b9eaac27689f06347ea4c5173a96cd..88b2d0fcfa03c3aafa47c7e6d38e64412c45a7cc 100644
+index 977a4b3622b9eaac27689f06347ea4c5173a96cd..9d94a5198155fbb517a243d6f0fd5e115c77daff 100644
 --- a/lib/http-proxy/index.js
 +++ b/lib/http-proxy/index.js
-@@ -1,5 +1,4 @@
+@@ -1,6 +1,5 @@
  var httpProxy = module.exports,
 -    extend    = require('util')._extend,
-     parse_url = require('url').parse,
+-    parse_url = require('url').parse,
++    parse_url = require('./common').parseUrl,
      EE3       = require('eventemitter3'),
      http      = require('http'),
+     https     = require('https'),
 @@ -47,9 +46,9 @@ function createRightProxy(type) {
          args[cntr] !== res
        ) {
@@ -88,3 +130,27 @@ index 7ae735514190eea569c605fff7d27c045fe8d601..c7c25e7228b21c76b3c7115af82ddcbf
      }
    },
  
+diff --git a/lib/http-proxy/passes/web-outgoing.js b/lib/http-proxy/passes/web-outgoing.js
+index 46352f6e377affc701773328d528bd7e4352f242..cabd340c7d66c6a5d464262d88c698d2781697b3 100644
+--- a/lib/http-proxy/passes/web-outgoing.js
++++ b/lib/http-proxy/passes/web-outgoing.js
+@@ -51,8 +51,8 @@ module.exports = { // <--
+     if ((options.hostRewrite || options.autoRewrite || options.protocolRewrite)
+         && proxyRes.headers['location']
+         && redirectRegex.test(proxyRes.statusCode)) {
+-      var target = url.parse(options.target);
+-      var u = url.parse(proxyRes.headers['location']);
++      var target = common.parseUrl(options.target);
++      var u = common.parseUrl(proxyRes.headers['location']);
+ 
+       // make sure the redirected host matches the target host before rewriting
+       if (target.host != u.host) {
+@@ -68,7 +68,7 @@ module.exports = { // <--
+         u.protocol = options.protocolRewrite;
+       }
+ 
+-      proxyRes.headers['location'] = u.format();
++      proxyRes.headers['location'] = url.format(u);
+     }
+   },
+   /**

pnpm-lock.yaml

@@ -29,7 +29,7 @@ patchedDependencies:
     hash: rvl3vkomen3tospgr67bzubfyu
     path: patches/@[email protected]
   [email protected]:
-    hash: eyqcxg3pntyhqyqr5zytxa7pbi
+    hash: kzqzmw2zuo23hy3avffm6aqice
     path: patches/[email protected]
   [email protected]:
     hash: o4nv5mg6kfnuzlknbdln3azhvu
@@ -374,7 +374,7 @@ importers:
         version: 5.1.18
       http-proxy:
         specifier: 1.18.1
-        version: 1.18.1(patch_hash=eyqcxg3pntyhqyqr5zytxa7pbi)
+        version: 1.18.1(patch_hash=kzqzmw2zuo23hy3avffm6aqice)
       husky:
         specifier: 9.0.11
         version: 9.0.11
@@ -1526,7 +1526,7 @@ importers:
         version: 5.1.1
       http-proxy:
         specifier: 1.18.1
-        version: 1.18.1(patch_hash=eyqcxg3pntyhqyqr5zytxa7pbi)
+        version: 1.18.1(patch_hash=kzqzmw2zuo23hy3avffm6aqice)
       http-proxy-agent:
         specifier: 5.0.0
         version: 5.0.0
@@ -30553,7 +30553,7 @@ snapshots:
   [email protected](@types/[email protected]):
     dependencies:
       '@types/http-proxy': 1.17.17
-      http-proxy: 1.18.1(patch_hash=eyqcxg3pntyhqyqr5zytxa7pbi)
+      http-proxy: 1.18.1(patch_hash=kzqzmw2zuo23hy3avffm6aqice)
       is-glob: 4.0.3
       is-plain-obj: 3.0.0
       micromatch: 4.0.8
@@ -30562,7 +30562,7 @@ snapshots:
     transitivePeerDependencies:
       - supports-color
 
-  [email protected](patch_hash=eyqcxg3pntyhqyqr5zytxa7pbi):
+  [email protected](patch_hash=kzqzmw2zuo23hy3avffm6aqice):
     dependencies:
       eventemitter3: 4.0.7
       follow-redirects: 1.9.0

test/e2e/rewrites-hostname/app/layout.js

@@ -0,0 +1,7 @@
+export default function RootLayout({ children }) {
+  return (
+    <html>
+      <body>{children}</body>
+    </html>
+  )
+}

test/e2e/rewrites-hostname/app/page.js

@@ -0,0 +1,9 @@
+export default function Page() {
+  return (
+    <>
+      <pre>
+        <code>curl http://localhost:3000/rewrite-idn-case-unicode</code>
+      </pre>
+    </>
+  )
+}

test/e2e/rewrites-hostname/next.config.js

@@ -0,0 +1,13 @@
+/** @type {import('next').NextConfig} */
+const config = {
+  rewrites() {
+    return [
+      {
+        source: '/rewrite-idn-case-unicode',
+        destination: `http://你好.localhost:${process.env.TEST_TARGET_PORT}`,
+      },
+    ]
+  },
+}
+
+module.exports = config

test/e2e/rewrites-hostname/rewrites-hostname.test.ts

@@ -0,0 +1,40 @@
+import { nextTestSetup } from 'e2e-utils'
+import { findPort } from 'next-test-utils'
+import createTargetServer from './target-server'
+
+describe('rewrites hostname', () => {
+  const { skipped, next } = nextTestSetup({
+    files: __dirname,
+    skipDeployment: true,
+    skipStart: true,
+  })
+
+  if (skipped) {
+    return
+  }
+
+  let targetPort: number | null = null
+  let closeTargetServer: (() => Promise<void>) | null = null
+
+  beforeAll(async () => {
+    targetPort = await findPort()
+    const closeServer = await createTargetServer(targetPort)
+    closeTargetServer = closeServer
+    await next.start({
+      env: { TEST_TARGET_PORT: String(targetPort) },
+    })
+  })
+
+  afterAll(async () => {
+    await closeTargetServer()
+  })
+
+  it('should navigate to a rewrite using unicode without error', async () => {
+    const response = await next.fetch(`/rewrite-idn-case-unicode`)
+
+    expect(await response.json()).toEqual({
+      forwardedHost: `localhost:${next.appPort}`,
+      host: `xn--6qq79v.localhost:${targetPort}`,
+    })
+  })
+})

test/e2e/rewrites-hostname/target-server.js

@@ -0,0 +1,42 @@
+// @ts-check
+const http = require('http')
+const process = require('process')
+const { promisify } = require('util')
+
+/**
+ * @returns {Promise<() => Promise<void>>} A promise that resolves to a function that closes the server
+ */
+function createServer(targetPort) {
+  const server = http.createServer((req, res) => {
+    const headers = req.headers
+
+    res.setHeader('Content-Type', 'application/json')
+    res.end(
+      JSON.stringify({
+        forwardedHost: headers['x-forwarded-host'],
+        host: headers['host'],
+      })
+    )
+  })
+
+  return new Promise((resolve, reject) => {
+    server.listen(targetPort, () => {
+      console.log(`Server listening on port ${targetPort}`)
+      // @ts-expect-error This is fine. Just a bug with older TS or @types/node versions.
+      resolve(promisify(server.close.bind(server)))
+    })
+  })
+}
+
+module.exports = createServer
+
+if (require.main === module) {
+  const targetPort = parseInt(process.env.TEST_TARGET_PORT, 10)
+  if (Number.isNaN(targetPort)) {
+    throw new Error('TEST_TARGET_PORT is not set or invalid')
+  }
+  createServer(targetPort).catch((err) => {
+    console.error(err)
+    process.exit(1)
+  })
+}

test/lib/next-modes/base.ts

@@ -553,7 +553,10 @@ export class NextInstance {
     }
   }
 
-  public async start(options?: { skipBuild?: boolean }): Promise<void> {}
+  public async start(options?: {
+    skipBuild?: boolean
+    env?: Record<string, string>
+  }): Promise<void> {}
 
   public async stop(
     signal: 'SIGINT' | 'SIGTERM' | 'SIGKILL' = 'SIGKILL'

test/lib/next-modes/next-dev.ts

@@ -120,7 +120,7 @@ export class NextDevInstance extends NextInstance {
     })
   }
 
-  public async start() {
+  public async start(options: { env?: Record<string, string> } = {}) {
     if (this.childProcess) {
       throw new Error('next already started')
     }
@@ -161,6 +161,7 @@ export class NextDevInstance extends NextInstance {
           env: {
             ...process.env,
             ...this.env,
+            ...options.env,
             NODE_ENV: this.env.NODE_ENV || ('' as any),
             PORT: this.forcedPort || '0',
             __NEXT_TEST_MODE: 'e2e',

test/lib/next-modes/next-start.ts

@@ -60,13 +60,15 @@ export class NextStartInstance extends NextInstance {
     })
   }
 
-  public async start(options: { skipBuild?: boolean } = {}) {
+  public async start(
+    options: { skipBuild?: boolean; env?: Record<string, string> } = {}
+  ) {
     if (this.childProcess) {
       throw new Error('next already started')
     }
 
     this._cliOutput = ''
-    const spawnOpts = this.getSpawnOpts()
+    const spawnOpts = this.getSpawnOpts(options.env)
 
     let startArgs = ['pnpm', 'next', 'start']