From d11cbc9ff0b1aaefabcba9afe1e562e0b1fde65a Mon Sep 17 00:00:00 2001
From: Jiachi Liu <inbox@huozhi.im>
Date: Thu, 8 Aug 2024 10:35:21 +0200
Subject: [PATCH] Reject next image urls in image optimizer (#68628)

Follow up on #67465, add more cases that should be rejected by image
optimizer when the url is tend to contain the generated next image with
`/_next/image` in url.

These should also get should get rejected:

* with encoded: `%2F_next%2Fimage%3`
* with asset prefix: `/assets/_next/image`
* with host: `https://<domain>/_next/image`

Closes NDX-101
x-ref:
[slack](https://vercel.slack.com/archives/C07AYREDX45/p1720111367743119)
---
 packages/next/src/lib/url.ts                  |  8 ++++
 packages/next/src/server/image-optimizer.ts   | 11 +++--
 test/integration/image-optimizer/test/util.ts | 46 +++++++++++++++++--
 test/lib/next-test-utils.ts                   |  9 ++++
 4 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/packages/next/src/lib/url.ts b/packages/next/src/lib/url.ts
index 32aea56082490..7706999acd166 100644
--- a/packages/next/src/lib/url.ts
+++ b/packages/next/src/lib/url.ts
@@ -11,3 +11,11 @@ export function getPathname(url: string) {
 export function isFullStringUrl(url: string) {
   return /https?:\/\//.test(url)
 }
+
+export function parseUrl(url: string): URL | undefined {
+  let parsed = undefined
+  try {
+    parsed = new URL(url, DUMMY_ORIGIN)
+  } catch {}
+  return parsed
+}
diff --git a/packages/next/src/server/image-optimizer.ts b/packages/next/src/server/image-optimizer.ts
index 225fc3196d7b3..32ff21246ad04 100644
--- a/packages/next/src/server/image-optimizer.ts
+++ b/packages/next/src/server/image-optimizer.ts
@@ -28,6 +28,7 @@ import type {
 import { sendEtagResponse } from './send-payload'
 import { getContentType, getExtension } from './serve-static'
 import * as Log from '../build/output/log'
+import { parseUrl } from '../lib/url'
 
 type XCacheHeader = 'MISS' | 'HIT' | 'STALE'
 
@@ -197,9 +198,13 @@ export class ImageOptimizerCache {
       }
     }
 
-    if (url.startsWith('/_next/image')) {
-      return {
-        errorMessage: '"url" parameter cannot be recursive',
+    const parsedUrl = parseUrl(url)
+    if (parsedUrl) {
+      const decodedPathname = decodeURIComponent(parsedUrl.pathname)
+      if (/\/_next\/image($|\/)/.test(decodedPathname)) {
+        return {
+          errorMessage: '"url" parameter cannot be recursive',
+        }
       }
     }
 
diff --git a/test/integration/image-optimizer/test/util.ts b/test/integration/image-optimizer/test/util.ts
index b5f517b69e38c..aecd8df96066e 100644
--- a/test/integration/image-optimizer/test/util.ts
+++ b/test/integration/image-optimizer/test/util.ts
@@ -8,6 +8,7 @@ import {
   fetchViaHTTP,
   File,
   findPort,
+  getFetchUrl,
   killApp,
   launchApp,
   nextBuild,
@@ -879,11 +880,46 @@ export function runTests(ctx) {
     )
   })
 
-  it('should fail when url is recursive', async () => {
-    const query = { url: `/_next/image?url=test.pngw=1&q=1`, w: ctx.w, q: 1 }
-    const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
-    expect(res.status).toBe(400)
-    expect(await res.text()).toBe(`"url" parameter cannot be recursive`)
+  describe('recursive url is not allowed', () => {
+    it('should fail with relative next image url', async () => {
+      const query = { url: `/_next/image?url=test.pngw=1&q=1`, w: ctx.w, q: 1 }
+      const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
+      expect(res.status).toBe(400)
+      expect(await res.text()).toBe(`"url" parameter cannot be recursive`)
+    })
+
+    it('should fail with encoded relative image url', async () => {
+      const query = {
+        url: '%2F_next%2Fimage%3Furl%3Dtest.pngw%3D1%26q%3D1',
+        w: ctx.w,
+        q: 1,
+      }
+      const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
+      expect(res.status).toBe(400)
+      expect(await res.text()).toBe(`"url" parameter is invalid`)
+    })
+
+    it('should fail with absolute next image url', async () => {
+      const fullUrl = getFetchUrl(
+        ctx.appPort,
+        '/_next/image?url=test.pngw=1&q=1'
+      )
+      const query = { url: fullUrl, w: ctx.w, q: 1 }
+      const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
+      expect(res.status).toBe(400)
+      expect(await res.text()).toBe(`"url" parameter cannot be recursive`)
+    })
+
+    it('should fail with relative image url with assetPrefix', async () => {
+      const fullUrl = getFetchUrl(
+        ctx.appPort,
+        `/assets/_next/image?url=test.pngw=1&q=1`
+      )
+      const query = { url: fullUrl, w: ctx.w, q: 1 }
+      const res = await fetchViaHTTP(ctx.appPort, '/_next/image', query, {})
+      expect(res.status).toBe(400)
+      expect(await res.text()).toBe(`"url" parameter cannot be recursive`)
+    })
   })
 
   it('should fail when internal url is not an image', async () => {
diff --git a/test/lib/next-test-utils.ts b/test/lib/next-test-utils.ts
index 54df89565ad0e..ae3c35af06813 100644
--- a/test/lib/next-test-utils.ts
+++ b/test/lib/next-test-utils.ts
@@ -147,6 +147,15 @@ export function withQuery(
   return `${pathname}?${querystring}`
 }
 
+export function getFetchUrl(
+  appPort: string | number,
+  pathname: string,
+  query?: Record<string, any> | string | null | undefined
+) {
+  const url = query ? withQuery(pathname, query) : pathname
+  return getFullUrl(appPort, url)
+}
+
 export function fetchViaHTTP(
   appPort: string | number,
   pathname: string,