fix: ensure we perform ssrf check within dispatcher (#13078)

Previously, we were performing this check before calling the fetch
function. This changes it to perform the check within the dispatcher.

It adjusts the int tests to both trigger the dispatcher lookup function
(which is only triggered when not already passing a valid IP) and the
check before calling fetch

---
- To see the specific tasks where the Asana app for GitHub is being
used, see below:
  - https://app.asana.com/0/0/1210733180484570

Author: AlessioGr

packages/payload/src/index.ts

@@ -156,7 +156,6 @@ export { extractAccessFromPermission } from './auth/extractAccessFromPermission.
 export { getAccessResults } from './auth/getAccessResults.js'
 export { getFieldsToSign } from './auth/getFieldsToSign.js'
 export { getLoginOptions } from './auth/getLoginOptions.js'
-
 export interface GeneratedTypes {
   authUntyped: {
     [slug: string]: {
@@ -1536,9 +1535,10 @@ export { importHandlerPath } from './queues/operations/runJobs/runJob/importHand
 export { getLocalI18n } from './translations/getLocalI18n.js'
 export * from './types/index.js'
 export { getFileByPath } from './uploads/getFileByPath.js'
+export { _internal_safeFetchGlobal } from './uploads/safeFetch.js'
 export type * from './uploads/types.js'
-export { addDataAndFileToRequest } from './utilities/addDataAndFileToRequest.js'
 
+export { addDataAndFileToRequest } from './utilities/addDataAndFileToRequest.js'
 export { addLocalesToRequestFromData, sanitizeLocales } from './utilities/addLocalesToRequest.js'
 export { commitTransaction } from './utilities/commitTransaction.js'
 export {
@@ -1610,8 +1610,8 @@ export { deleteCollectionVersions } from './versions/deleteCollectionVersions.js
 export { appendVersionToQueryKey } from './versions/drafts/appendVersionToQueryKey.js'
 export { getQueryDraftsSort } from './versions/drafts/getQueryDraftsSort.js'
 export { enforceMaxVersions } from './versions/enforceMaxVersions.js'
-export { getLatestCollectionVersion } from './versions/getLatestCollectionVersion.js'
 
+export { getLatestCollectionVersion } from './versions/getLatestCollectionVersion.js'
 export { getLatestGlobalVersion } from './versions/getLatestGlobalVersion.js'
 export { saveVersion } from './versions/saveVersion.js'
 export type { SchedulePublishTaskInput } from './versions/schedule/types.js'

packages/payload/src/uploads/safeFetch.ts

@@ -1,9 +1,16 @@
-import type { Dispatcher } from 'undici'
+import type { LookupFunction } from 'net'
 
-import { lookup } from 'dns/promises'
+import { lookup } from 'dns'
 import ipaddr from 'ipaddr.js'
 import { Agent, fetch as undiciFetch } from 'undici'
 
+/**
+ * @internal this is used to mock the IP `lookup` function in integration tests
+ */
+export const _internal_safeFetchGlobal = {
+  lookup,
+}
+
 const isSafeIp = (ip: string) => {
   try {
     if (!ip) {
@@ -25,32 +32,31 @@ const isSafeIp = (ip: string) => {
   return true
 }
 
-/**
- * Checks if a hostname or IP address is safe to fetch from.
- * @param hostname a hostname or IP address
- * @returns
- */
-const isSafe = async (hostname: string) => {
-  try {
-    if (ipaddr.isValid(hostname)) {
-      return isSafeIp(hostname)
-    }
+const ssrfFilterInterceptor: LookupFunction = (hostname, options, callback) => {
+  _internal_safeFetchGlobal.lookup(hostname, options, (err, address, family) => {
+    if (err) {
+      callback(err, address, family)
+    } else {
+      let ips = [] as string[]
+      if (Array.isArray(address)) {
+        ips = address.map((a) => a.address)
+      } else {
+        ips = [address]
+      }
 
-    const { address } = await lookup(hostname)
-    return isSafeIp(address)
-  } catch (_ignore) {
-    return false
-  }
-}
+      if (ips.some((ip) => !isSafeIp(ip))) {
+        callback(new Error(`Blocked unsafe attempt to ${hostname}`), address, family)
+        return
+      }
 
-const ssrfFilterInterceptor: Dispatcher.DispatcherComposeInterceptor = (dispatch) => {
-  return (opts, handler) => {
-    return dispatch(opts, handler)
-  }
+      callback(null, address, family)
+    }
+  })
 }
 
-const safeDispatcher = new Agent().compose(ssrfFilterInterceptor)
-
+const safeDispatcher = new Agent({
+  connect: { lookup: ssrfFilterInterceptor },
+})
 /**
  * A "safe" version of undici's fetch that prevents SSRF attacks.
  *
@@ -64,11 +70,18 @@ export const safeFetch = async (...args: Parameters<typeof undiciFetch>) => {
   try {
     const url = new URL(unverifiedUrl)
 
-    const isHostnameSafe = await isSafe(url.hostname)
-    if (!isHostnameSafe) {
-      throw new Error(`Blocked unsafe attempt to ${url.toString()}`)
+    let hostname = url.hostname
+
+    // Strip brackets from IPv6 addresses (e.g., "[::1]" => "::1")
+    if (hostname.startsWith('[') && hostname.endsWith(']')) {
+      hostname = hostname.slice(1, -1)
     }
 
+    if (ipaddr.isValid(hostname)) {
+      if (!isSafeIp(hostname)) {
+        throw new Error(`Blocked unsafe attempt to ${hostname}`)
+      }
+    }
     return await undiciFetch(url, {
       ...options,
       dispatcher: safeDispatcher,

test/uploads/int.spec.ts

@@ -3,7 +3,7 @@ import type { CollectionSlug, Payload } from 'payload'
 import { randomUUID } from 'crypto'
 import fs from 'fs'
 import path from 'path'
-import { getFileByPath } from 'payload'
+import { _internal_safeFetchGlobal, getFileByPath } from 'payload'
 import { fileURLToPath } from 'url'
 import { promisify } from 'util'
 
@@ -575,6 +575,46 @@ describe('Collections - Uploads', () => {
       `(
         'should block or filter uploading from $collection with URL: $url',
         async ({ url, collection, errorContains }) => {
+          const globalCachedFn = _internal_safeFetchGlobal.lookup
+
+          let hostname = new URL(url).hostname
+
+          const isIPV6 = hostname.includes('::')
+
+          // Strip brackets from IPv6 addresses
+          // eslint-disable-next-line jest/no-conditional-in-test
+          if (isIPV6) {
+            hostname = hostname.slice(1, -1)
+          }
+
+          // Here we're essentially mocking our own DNS provider, to get 'https://www.payloadcms.com/test.png' to resolve to the IP
+          // we'd like to test for
+          // @ts-expect-error this does not need to be mocked 100% correctly
+          _internal_safeFetchGlobal.lookup = (_hostname, _options, callback) => {
+            // eslint-disable-next-line jest/no-conditional-in-test
+            callback(null, hostname as any, isIPV6 ? 6 : 4)
+          }
+
+          await expect(
+            payload.create({
+              collection,
+              data: {
+                filename: 'test.png',
+                // Need to pass a domain for lookup to be called. We monkey patch the IP lookup function above
+                // to return the IP address we want to test.
+                url: 'https://www.payloadcms.com/test.png',
+              },
+            }),
+          ).rejects.toThrow(
+            expect.objectContaining({
+              name: 'FileRetrievalError',
+              message: expect.stringContaining(errorContains),
+            }),
+          )
+
+          _internal_safeFetchGlobal.lookup = globalCachedFn
+
+          // Now ensure this throws if we pass the IP address directly, without the mock
           await expect(
             payload.create({
               collection,