Backport Next.js changes to v15.4.9 (#24)

* remove urlencoded server action handling (#14)

* remove urlencoded server action handling

* urlencoded POSTs should still flow through action handler

* bailout early for urlencoded

* tweak handling to 404 in feetch action case

* error when there are no server actions (#15)

* remove custom error

* remove dev gate

* error when there are no server actions

* gate against dev

* Check whether action ids are valid before parsing (#16)

* Check whether action ids are valid before parsing

In an effort to narrow access to parsing methods Next.js now prequalifies MPA form parsing by ensuring there are only valid action IDs as part of the submission. This is not meant to be a strong line of defense against malicious payloads since action IDs are not private and are for many apps relatively easy to acquire. It does however provide some limited narrowing of code paths so that action decoding only happens for plausible actions

Additionally all other branches that use next-action header now consistently check the header before proceeding with parsing. This is also a perf improvement.

* fix test assertion

---------

Co-authored-by: Zack Tanner <1939140+ztanner@users.noreply.github.com>

---------

Co-authored-by: Josh Story <story@hey.com>

Author: ztanner

packages/next/errors.json

@@ -720,5 +720,8 @@
   "719": "Failed to get source map for '%s'. This is a bug in Next.js",
   "720": "Client Max Body Size must be a valid number (bytes) or filesize format string (e.g., \"5mb\")",
   "721": "Client Max Body Size must be larger than 0 bytes",
-  "722": "Request body exceeded %s"
+  "722": "Request body exceeded %s",
+  "723": "Server Actions are not enabled for this application. This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action",
+  "724": "Failed to find Server Action. This request might be from an older or newer deployment.\\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action",
+  "725": "Failed to find Server Action%s. This request might be from an older or newer deployment.\\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action"
 }

packages/next/src/server/app-render/action-handler.ts

@@ -4,6 +4,7 @@ import type { RequestStore } from '../app-render/work-unit-async-storage.externa
 import type { AppRenderContext, GenerateFlight } from './app-render'
 import type { AppPageModule } from '../route-modules/app-page/module'
 import type { BaseNextRequest, BaseNextResponse } from '../base-http'
+import type { ActionManifest } from '../../build/webpack/plugins/flight-client-entry-plugin'
 
 import {
   RSC_HEADER,
@@ -59,13 +60,14 @@ import { executeRevalidates } from '../revalidation-utils'
 import { getRequestMeta } from '../request-meta'
 import { setCacheBustingSearchParam } from '../../client/components/router-reducer/set-cache-busting-search-param'
 
-function formDataFromSearchQueryString(query: string) {
-  const searchParams = new URLSearchParams(query)
-  const formData = new FormData()
-  for (const [key, value] of searchParams) {
-    formData.append(key, value)
-  }
-  return formData
+/**
+ * Checks if the app has any server actions defined in any runtime.
+ */
+function hasServerActions(manifest: ActionManifest) {
+  return (
+    Object.keys(manifest.node).length > 0 ||
+    Object.keys(manifest.edge).length > 0
+  )
 }
 
 function nodeHeadersToRecord(
@@ -508,19 +510,55 @@ export async function handleAction({
 
   const {
     actionId,
-    isURLEncodedAction,
     isMultipartAction,
     isFetchAction,
+    isURLEncodedAction,
     isPossibleServerAction,
   } = getServerActionRequestMetadata(req)
 
+  const handleUnrecognizedFetchAction = (err: unknown): HandleActionResult => {
+    // If the deployment doesn't have skew protection, this is expected to occasionally happen,
+    // so we use a warning instead of an error.
+    console.warn(err)
+
+    // Return an empty response with a header that the client router will interpret.
+    // We don't need to waste time encoding a flight response, and using a blank body + header
+    // means that unrecognized actions can also be handled at the infra level
+    // (i.e. without needing to invoke a lambda)
+    res.setHeader(NEXT_ACTION_NOT_FOUND_HEADER, '1')
+    res.setHeader('content-type', 'text/plain')
+    res.statusCode = 404
+    return {
+      type: 'done',
+      result: RenderResult.fromStatic('Server action not found.'),
+    }
+  }
+
   // If it can't be a Server Action, skip handling.
   // Note that this can be a false positive -- any multipart/urlencoded POST can get us here,
   // But won't know if it's an MPA action or not until we call `decodeAction` below.
   if (!isPossibleServerAction) {
     return null
   }
 
+  // We don't currently support URL encoded actions, so we bail out early.
+  // Depending on if it's a fetch action or an MPA, we return a different response.
+  if (isURLEncodedAction) {
+    if (isFetchAction) {
+      return {
+        type: 'not-found',
+      }
+    } else {
+      // This is an MPA action, so we return null
+      return null
+    }
+  }
+
+  // If the app has no server actions at all, we can 404 early.
+  if (!hasServerActions(serverActionsManifest)) {
+    return handleUnrecognizedFetchAction(getActionNotFoundError(actionId))
+  }
+
   if (workStore.isStaticGeneration) {
     throw new Error(
       "Invariant: server actions can't be handled during static rendering"
@@ -641,24 +679,6 @@ export async function handleAction({
     }
   }
 
-  const handleUnrecognizedFetchAction = (err: unknown): HandleActionResult => {
-    // If the deployment doesn't have skew protection, this is expected to occasionally happen,
-    // so we use a warning instead of an error.
-    console.warn(err)
-
-    // Return an empty response with a header that the client router will interpret.
-    // We don't need to waste time encoding a flight response, and using a blank body + header
-    // means that unrecognized actions can also be handled at the infra level
-    // (i.e. without needing to invoke a lambda)
-    res.setHeader(NEXT_ACTION_NOT_FOUND_HEADER, '1')
-    res.setHeader('content-type', 'text/plain')
-    res.statusCode = 404
-    return {
-      type: 'done',
-      result: RenderResult.fromStatic('Server action not found.'),
-    }
-  }
-
   try {
     return await actionAsyncStorage.run(
       { isAction: true },
@@ -694,6 +714,13 @@ export async function handleAction({
             const formData = await req.request.formData()
             if (isFetchAction) {
               // A fetch action with a multipart body.
+
+              try {
+                actionModId = getActionModIdOrError(actionId, serverModuleMap)
+              } catch (err) {
+                return handleUnrecognizedFetchAction(err)
+              }
+
               boundActionArguments = await decodeReply(
                 formData,
                 serverModuleMap,
@@ -702,6 +729,14 @@ export async function handleAction({
             } else {
               // Multipart POST, but not a fetch action.
               // Potentially an MPA action, we have to try decoding it to check.
+              if (areAllActionIdsValid(formData, serverModuleMap) === false) {
+                // TODO: This can be from skew or manipulated input. We should handle this case
+                // more gracefully but this preserves the prior behavior where decodeAction would throw instead.
+                throw new Error(
+                  `Failed to find Server Action. This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action`
+                )
+              }
+
               const action = await decodeAction(formData, serverModuleMap)
               if (typeof action === 'function') {
                 // an MPA action.
@@ -767,20 +802,11 @@ export async function handleAction({
 
             const actionData = Buffer.concat(chunks).toString('utf-8')
 
-            if (isURLEncodedAction) {
-              const formData = formDataFromSearchQueryString(actionData)
-              boundActionArguments = await decodeReply(
-                formData,
-                serverModuleMap,
-                { temporaryReferences }
-              )
-            } else {
-              boundActionArguments = await decodeReply(
-                actionData,
-                serverModuleMap,
-                { temporaryReferences }
-              )
-            }
+            boundActionArguments = await decodeReply(
+              actionData,
+              serverModuleMap,
+              { temporaryReferences }
+            )
           }
         } else if (
           // The type check here ensures that `req` is correctly typed, and the
@@ -848,6 +874,12 @@ export async function handleAction({
             if (isFetchAction) {
               // A fetch action with a multipart body.
 
+              try {
+                actionModId = getActionModIdOrError(actionId, serverModuleMap)
+              } catch (err) {
+                return handleUnrecognizedFetchAction(err)
+              }
+
               const busboy = (
                 require('next/dist/compiled/busboy') as typeof import('next/dist/compiled/busboy')
               )({
@@ -895,7 +927,19 @@ export async function handleAction({
                 }),
                 duplex: 'half',
               })
+
               const formData = await fakeRequest.formData()
+
+              if (areAllActionIdsValid(formData, serverModuleMap) === false) {
+                // TODO: This can be from skew or manipulated input. We should handle this case
+                // more gracefully but this preserves the prior behavior where decodeAction would throw instead.
+                throw new Error(
+                  `Failed to find Server Action. This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action`
+                )
+              }
+
+              // TODO: Refactor so it is harder to accidentally decode an action before you have validated that the
+              // action referred to is available.
               const action = await decodeAction(formData, serverModuleMap)
               if (typeof action === 'function') {
                 // an MPA action.
@@ -955,20 +999,11 @@ export async function handleAction({
 
             const actionData = Buffer.concat(chunks).toString('utf-8')
 
-            if (isURLEncodedAction) {
-              const formData = formDataFromSearchQueryString(actionData)
-              boundActionArguments = await decodeReply(
-                formData,
-                serverModuleMap,
-                { temporaryReferences }
-              )
-            } else {
-              boundActionArguments = await decodeReply(
-                actionData,
-                serverModuleMap,
-                { temporaryReferences }
-              )
-            }
+            boundActionArguments = await decodeReply(
+              actionData,
+              serverModuleMap,
+              { temporaryReferences }
+            )
           }
         } else {
           throw new Error('Invariant: Unknown request type.')
@@ -986,13 +1021,6 @@ export async function handleAction({
         // / -> fire action -> POST / -> appRender1 -> modId for the action file
         // /foo -> fire action -> POST /foo -> appRender2 -> modId for the action file
 
-        try {
-          actionModId =
-            actionModId ?? getActionModIdOrError(actionId, serverModuleMap)
-        } catch (err) {
-          return handleUnrecognizedFetchAction(err)
-        }
-
         const actionMod = (await ComponentMod.__next_app__.require(
           actionModId
         )) as Record<string, (...args: unknown[]) => Promise<unknown>>
@@ -1178,10 +1206,121 @@ function getActionModIdOrError(
   const actionModId = serverModuleMap[actionId]?.id
 
   if (!actionModId) {
-    throw new Error(
-      `Failed to find Server Action "${actionId}". This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action`
-    )
+    throw getActionNotFoundError(actionId)
   }
 
   return actionModId
 }
+
+function getActionNotFoundError(actionId: string | null): Error {
+  return new Error(
+    `Failed to find Server Action${actionId ? ` "${actionId}"` : ''}. This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action`
+  )
+}
+
+const $ACTION_ = '$ACTION_'
+const $ACTION_REF_ = '$ACTION_REF_'
+const $ACTION_ID_ = '$ACTION_ID_'
+const ACTION_ID_EXPECTED_LENGTH = 42
+
+/**
+ * This function mirrors logic inside React's decodeAction and should be kept in sync with that.
+ * It pre-parses the FormData to ensure that any action IDs referred to are actual action IDs for
+ * this Next.js application.
+ */
+function areAllActionIdsValid(
+  mpaFormData: FormData,
+  serverModuleMap: ServerModuleMap
+): boolean {
+  let hasAtLeastOneAction = false
+  // Before we attempt to decode the payload for a possible MPA action, assert that all
+  // action IDs are valid IDs. If not we should disregard the payload
+  for (let key of mpaFormData.keys()) {
+    if (!key.startsWith($ACTION_)) {
+      // not a relevant field
+      continue
+    }
+
+    if (key.startsWith($ACTION_ID_)) {
+      // No Bound args case
+      if (isInvalidActionIdFieldName(key, serverModuleMap)) {
+        return false
+      }
+
+      hasAtLeastOneAction = true
+    } else if (key.startsWith($ACTION_REF_)) {
+      // Bound args case
+      const actionDescriptorField =
+        $ACTION_ + key.slice($ACTION_REF_.length) + ':0'
+      const actionFields = mpaFormData.getAll(actionDescriptorField)
+      if (actionFields.length !== 1) {
+        return false
+      }
+      const actionField = actionFields[0]
+      if (typeof actionField !== 'string') {
+        return false
+      }
+
+      if (isInvalidStringActionDescriptor(actionField, serverModuleMap)) {
+        return false
+      }
+      hasAtLeastOneAction = true
+    }
+  }
+  return hasAtLeastOneAction
+}
+
+const ACTION_DESCRIPTOR_ID_PREFIX = '{"id":"'
+function isInvalidStringActionDescriptor(
+  actionDescriptor: string,
+  serverModuleMap: ServerModuleMap
+): unknown {
+  if (actionDescriptor.startsWith(ACTION_DESCRIPTOR_ID_PREFIX) === false) {
+    return true
+  }
+
+  const from = ACTION_DESCRIPTOR_ID_PREFIX.length
+  const to = from + ACTION_ID_EXPECTED_LENGTH
+
+  // We expect actionDescriptor to be '{"id":"<actionId>",...}'
+  const actionId = actionDescriptor.slice(from, to)
+  if (
+    actionId.length !== ACTION_ID_EXPECTED_LENGTH ||
+    actionDescriptor[to] !== '"'
+  ) {
+    return true
+  }
+
+  const entry = serverModuleMap[actionId]
+
+  if (entry == null) {
+    return true
+  }
+
+  return false
+}
+
+function isInvalidActionIdFieldName(
+  actionIdFieldName: string,
+  serverModuleMap: ServerModuleMap
+): boolean {
+  // The field name must always start with $ACTION_ID_ but since it is
+  // the id is extracted from the key of the field we have already validated
+  // this before entering this function
+  if (
+    actionIdFieldName.length !==
+    $ACTION_ID_.length + ACTION_ID_EXPECTED_LENGTH
+  ) {
+    // this field name has too few or too many characters
+    return true
+  }
+
+  const actionId = actionIdFieldName.slice($ACTION_ID_.length)
+  const entry = serverModuleMap[actionId]
+
+  if (entry == null) {
+    return true
+  }
+
+  return false
+}

packages/next/src/server/lib/server-action-request-meta.ts

@@ -23,6 +23,9 @@ export function getServerActionRequestMetadata(
     contentType = req.headers['content-type'] ?? null
   }
 
+  // We don't actually support URL encoded actions, and the action handler will bail out if it sees one.
+  // But we still want it to flow through to the action handler, to prevent changes in behavior when a regular
+  // page component tries to handle a POST.
   const isURLEncodedAction = Boolean(
     req.method === 'POST' && contentType === 'application/x-www-form-urlencoded'
   )