verify action id before parsing body (#58977)

### What?
When handling a server action, in the non-progressive enhanced case,
React will attempt to parse the request body before verifying if a valid
server action is received. This results in an "Error: Connection Closed"
error being thrown, rather than ignoring the action and failing more
gracefully

### Why?
To support progressive enhancement with form actions, the `actionId`
value is added as a hidden input in the form, so the action ID from the
header shouldn't be verified until determining that we've reached the
non-PE case. ([React
ref](https://github.com/facebook/react/pull/26774)). However, in
https://github.com/vercel/next.js/pull/49187, support was added for a
URL encoded form (which is not currently used, as indicated on the PR).

Despite it not being used for server actions, it's currently possible to
trigger this codepath, ie by calling redirect in an action handler with
a 307/308 status code with some data in the URL. This would result in a
500 error.

### How?
React should not attempt to parse the URL encoded form data until after
we've verified the server action header for the non-PE case.

x-ref NEXT-1733
[Slack
context](https://vercel.slack.com/archives/C03S8ED1DKM/p1700674895218399?thread_ts=1700060786.749079&cid=C03S8ED1DKM)

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

@@ -238,6 +238,16 @@ function limitUntrustedHeaderValueForLogs(value: string) {
   return value.length > 100 ? value.slice(0, 100) + '...' : value
 }
 
+type ServerModuleMap = Record<
+  string,
+  | {
+      id: string
+      chunks: string[]
+      name: string
+    }
+  | undefined
+>
+
 export async function handleAction({
   req,
   res,
@@ -252,13 +262,7 @@ export async function handleAction({
   req: IncomingMessage
   res: ServerResponse
   ComponentMod: AppPageModule
-  serverModuleMap: {
+  serverModuleMap: ServerModuleMap
-    [id: string]: {
-      id: string
-      chunks: string[]
-      name: string
-    }
-  }
   generateFlight: GenerateFlight
   staticGenerationStore: StaticGenerationStore
   requestStore: RequestStore
@@ -392,6 +396,7 @@ export async function handleAction({
 
   let actionResult: RenderResult | undefined
   let formState: any | undefined
+  let actionModId: string | undefined
 
   try {
     await actionAsyncStorage.run({ isAction: true }, async () => {
@@ -418,6 +423,15 @@ export async function handleAction({
             return
           }
         } else {
+          try {
+            actionModId = getActionModIdOrError(actionId, serverModuleMap)
+          } catch (err) {
+            console.error(err)
+            return {
+              type: 'not-found',
+            }
+          }
+
           let actionData = ''
 
           const reader = webRequest.body.getReader()
@@ -487,6 +501,15 @@ export async function handleAction({
             return
           }
         } else {
+          try {
+            actionModId = getActionModIdOrError(actionId, serverModuleMap)
+          } catch (err) {
+            console.error(err)
+            return {
+              type: 'not-found',
+            }
+          }
+
           const chunks = []
 
           for await (const chunk of req) {
@@ -528,19 +551,11 @@ To configure the body size limit for Server Actions, see: https://nextjs.org/doc
       // / -> fire action -> POST / -> appRender1 -> modId for the action file
       // /foo -> fire action -> POST /foo -> appRender2 -> modId for the action file
 
-      let actionModId: string
       try {
-        if (!actionId) {
+        actionModId =
-          throw new Error('Invariant: actionId should be set')
+          actionModId ?? getActionModIdOrError(actionId, serverModuleMap)
-        }
-
-        actionModId = serverModuleMap[actionId].id
       } catch (err) {
-        // When this happens, it could be a deployment skew where the action came
+        console.error(err)
-        // from a different deployment. We'll just return a 404 with a message logged.
-        console.error(
-          `Failed to find Server Action "${actionId}". This request might be from an older or newer deployment.`
-        )
         return {
           type: 'not-found',
         }
@@ -548,7 +563,10 @@ To configure the body size limit for Server Actions, see: https://nextjs.org/doc
 
       const actionHandler = (
         await ComponentMod.__next_app__.require(actionModId)
-      )[actionId]
+      )[
+        // `actionId` must exist if we got here, as otherwise we would have thrown an error above
+        actionId!
+      ]
 
       const returnVal = await actionHandler.apply(null, bound)
 
@@ -675,3 +693,36 @@ To configure the body size limit for Server Actions, see: https://nextjs.org/doc
     throw err
   }
 }
+
+/**
+ * Attempts to find the module ID for the action from the module map. When this fails, it could be a deployment skew where
+ * the action came from a different deployment. It could also simply be an invalid POST request that is not a server action.
+ * In either case, we'll throw an error to be handled by the caller.
+ */
+function getActionModIdOrError(
+  actionId: string | null,
+  serverModuleMap: ServerModuleMap
+): string {
+  try {
+    // if we're missing the action ID header, we can't do any further processing
+    if (!actionId) {
+      throw new Error("Invariant: Missing 'next-action' header.")
+    }
+
+    const actionModId = serverModuleMap?.[actionId]?.id
+
+    if (!actionModId) {
+      throw new Error(
+        "Invariant: Couldn't find action module ID from module map."
+      )
+    }
+
+    return actionModId
+  } catch (err) {
+    throw new Error(
+      `Failed to find Server Action "${actionId}". This request might be from an older or newer deployment. ${
+        err instanceof Error ? `Original error: ${err.message}` : ''
+      }`
+    )
+  }
+}

test/e2e/app-dir/actions/app-action.test.ts

@@ -409,6 +409,34 @@ createNextDescribe(
       )
     })
 
+    it('should 404 when POSTing an invalid server action', async () => {
+      const res = await next.fetch('/non-existent-route', {
+        method: 'POST',
+        headers: {
+          'content-type': 'application/x-www-form-urlencoded',
+        },
+        body: 'foo=bar',
+      })
+
+      expect(res.status).toBe(404)
+    })
+
+    it('should log a warning when a server action is not found but an id is provided', async () => {
+      await next.fetch('/server', {
+        method: 'POST',
+        headers: {
+          'content-type': 'application/x-www-form-urlencoded',
+          'next-action': 'abc123',
+        },
+        body: 'foo=bar',
+      })
+
+      await check(
+        () => next.cliOutput,
+        /Failed to find Server Action "abc123". This request might be from an older or newer deployment./
+      )
+    })
+
     if (isNextStart) {
       it('should not expose action content in sourcemaps', async () => {
         const sourcemap = (
@@ -972,6 +1000,46 @@ createNextDescribe(
         expect(postRequests).toEqual(['/redirects/api-redirect-permanent'])
         expect(responseCodes).toEqual([303])
       })
+
+      it.each(['307', '308'])(
+        `redirects properly when server action handler redirects with a %s status code`,
+        async (statusCode) => {
+          const postRequests = []
+          const responseCodes = []
+
+          const browser = await next.browser('/redirects', {
+            beforePageLoad(page) {
+              page.on('request', (request: Request) => {
+                const url = new URL(request.url())
+                if (request.method() === 'POST') {
+                  postRequests.push(`${url.pathname}${url.search}`)
+                }
+              })
+
+              page.on('response', (response: Response) => {
+                const url = new URL(response.url())
+                const status = response.status()
+
+                if (postRequests.includes(`${url.pathname}${url.search}`)) {
+                  responseCodes.push(status)
+                }
+              })
+            },
+          })
+
+          await browser.elementById(`submit-api-redirect-${statusCode}`).click()
+          await check(() => browser.url(), /success=true/)
+          expect(await browser.elementById('redirect-page')).toBeTruthy()
+
+          // since a 307/308 status code follows the redirect, the POST request should be made to both the action handler and the redirect target
+          expect(postRequests).toEqual([
+            `/redirects/api-redirect-${statusCode}`,
+            `/redirects?success=true`,
+          ])
+
+          expect(responseCodes).toEqual([Number(statusCode), 200])
+        }
+      )
     })
   }
 )

test/e2e/app-dir/actions/app/redirects/api-redirect-307/route.js

@@ -0,0 +1,6 @@
+export function POST(request) {
+  return Response.redirect(
+    `${request.nextUrl.origin}/redirects?success=true`,
+    307
+  )
+}

test/e2e/app-dir/actions/app/redirects/api-redirect-308/route.js

@@ -0,0 +1,6 @@
+export function POST(request) {
+  return Response.redirect(
+    `${request.nextUrl.origin}/redirects?success=true`,
+    308
+  )
+}

test/e2e/app-dir/actions/app/redirects/page.js

@@ -13,6 +13,14 @@ export default function Home() {
           id="submit-api-redirect-permanent"
         />
       </form>
+      <h1>POST /api-reponse-redirect-307</h1>
+      <form action="/redirects/api-redirect-307" method="POST">
+        <input type="submit" value="Submit" id="submit-api-redirect-307" />
+      </form>
+      <h1>POST /api-reponse-redirect-308</h1>
+      <form action="/redirects/api-redirect-308" method="POST">
+        <input type="submit" value="Submit" id="submit-api-redirect-308" />
+      </form>
     </main>
   )
 }