Fix Server Action error logs for unhandled POST requests #64315
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shuding
requested review from
timneutkens,
ijjk,
huozhi,
feedthejim,
ztanner and
wyattjoh
as code owners
shuding
commented
Apr 11, 2024
Comment on lines
51
to
55
| export function getIsServerAction( | ||
| req: IncomingMessage | BaseNextRequest | NextRequest | ||
| ): boolean { | ||
| const { isFetchAction, isURLEncodedAction, isMultipartAction } = | ||
| getServerActionRequestMetadata(req) | ||
|
|
||
| return Boolean(isFetchAction || isURLEncodedAction || isMultipartAction) | ||
| return getServerActionRequestMetadata(req).isServerAction | ||
| } |
There was a problem hiding this comment.
This change avoids calling getServerActionRequestMetadata twice in the Action's handler.
Stats from current PRDefault Build (Increase detected
|
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| buildDuration | 14.7s | 15s | |
| buildDurationCached | 8.4s | 7s | N/A |
| nodeModulesSize | 199 MB | 199 MB | N/A |
| nextStartRea..uration (ms) | 396ms | 418ms | N/A |
Client Bundles (main, webpack)
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| 2453-HASH.js gzip | 31.4 kB | 31.4 kB | N/A |
| 3304.HASH.js gzip | 181 B | 181 B | ✓ |
| 3f784ff6-HASH.js gzip | 53.7 kB | 53.7 kB | ✓ |
| 8299-HASH.js gzip | 5.1 kB | 5.1 kB | N/A |
| framework-HASH.js gzip | 45.2 kB | 45.2 kB | ✓ |
| main-app-HASH.js gzip | 243 B | 241 B | N/A |
| main-HASH.js gzip | 32.2 kB | 32.2 kB | N/A |
| webpack-HASH.js gzip | 1.68 kB | 1.68 kB | N/A |
| Overall change | 99 kB | 99 kB | ✓ |
Legacy Client Bundles (polyfills)
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| polyfills-HASH.js gzip | 31 kB | 31 kB | ✓ |
| Overall change | 31 kB | 31 kB | ✓ |
Client Pages
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| _app-HASH.js gzip | 196 B | 197 B | N/A |
| _error-HASH.js gzip | 184 B | 184 B | ✓ |
| amp-HASH.js gzip | 505 B | 505 B | ✓ |
| css-HASH.js gzip | 324 B | 325 B | N/A |
| dynamic-HASH.js gzip | 2.5 kB | 2.5 kB | N/A |
| edge-ssr-HASH.js gzip | 258 B | 258 B | ✓ |
| head-HASH.js gzip | 352 B | 352 B | ✓ |
| hooks-HASH.js gzip | 370 B | 371 B | N/A |
| image-HASH.js gzip | 4.27 kB | 4.27 kB | ✓ |
| index-HASH.js gzip | 259 B | 259 B | ✓ |
| link-HASH.js gzip | 2.67 kB | 2.67 kB | N/A |
| routerDirect..HASH.js gzip | 314 B | 312 B | N/A |
| script-HASH.js gzip | 386 B | 386 B | ✓ |
| withRouter-HASH.js gzip | 309 B | 309 B | ✓ |
| 1afbb74e6ecf..834.css gzip | 106 B | 106 B | ✓ |
| Overall change | 6.63 kB | 6.63 kB | ✓ |
Client Build Manifests
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| _buildManifest.js gzip | 483 B | 485 B | N/A |
| Overall change | 0 B | 0 B | ✓ |
Rendered Page Sizes
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| index.html gzip | 529 B | 529 B | ✓ |
| link.html gzip | 541 B | 541 B | ✓ |
| withRouter.html gzip | 525 B | 524 B | N/A |
| Overall change | 1.07 kB | 1.07 kB | ✓ |
Edge SSR bundle Size
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| edge-ssr.js gzip | 95.6 kB | 95.6 kB | N/A |
| page.js gzip | 3.06 kB | 3.06 kB | N/A |
| Overall change | 0 B | 0 B | ✓ |
Middleware size
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| middleware-b..fest.js gzip | 624 B | 625 B | N/A |
| middleware-r..fest.js gzip | 155 B | 156 B | N/A |
| middleware.js gzip | 25.5 kB | 25.5 kB | N/A |
| edge-runtime..pack.js gzip | 839 B | 839 B | ✓ |
| Overall change | 839 B | 839 B | ✓ |
Next Runtimes
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| app-page-exp...dev.js gzip | 171 kB | 171 kB | N/A |
| app-page-exp..prod.js gzip | 97.4 kB | 97.4 kB | N/A |
| app-page-tur..prod.js gzip | 99.2 kB | 99.2 kB | N/A |
| app-page-tur..prod.js gzip | 93.4 kB | 93.4 kB | N/A |
| app-page.run...dev.js gzip | 144 kB | 144 kB | N/A |
| app-page.run..prod.js gzip | 91.9 kB | 91.9 kB | N/A |
| app-route-ex...dev.js gzip | 21.5 kB | 21.4 kB | N/A |
| app-route-ex..prod.js gzip | 15.2 kB | 15.2 kB | N/A |
| app-route-tu..prod.js gzip | 15.2 kB | 15.2 kB | N/A |
| app-route-tu..prod.js gzip | 14.9 kB | 14.9 kB | N/A |
| app-route.ru...dev.js gzip | 21.1 kB | 21.1 kB | N/A |
| app-route.ru..prod.js gzip | 14.9 kB | 14.9 kB | N/A |
| pages-api-tu..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-api.ru...dev.js gzip | 9.82 kB | 9.82 kB | ✓ |
| pages-api.ru..prod.js gzip | 9.55 kB | 9.55 kB | ✓ |
| pages-turbo...prod.js gzip | 22.5 kB | 22.5 kB | ✓ |
| pages.runtim...dev.js gzip | 23.1 kB | 23.1 kB | ✓ |
| pages.runtim..prod.js gzip | 22.5 kB | 22.5 kB | ✓ |
| server.runti..prod.js gzip | 51.3 kB | 51.3 kB | N/A |
| Overall change | 97 kB | 97 kB | ✓ |
build cache Overall increase ⚠️
| vercel/next.js canary | vercel/next.js shu/5wsq | Change | |
|---|---|---|---|
| 0.pack gzip | 1.59 MB | 1.58 MB | N/A |
| index.pack gzip | 106 kB | 106 kB | |
| Overall change | 106 kB | 106 kB |
Diff details
Diff for middleware.js
Diff too large to display
Diff for edge-ssr.js
Diff too large to display
Diff for app-page-exp..ntime.dev.js
Diff too large to display
Diff for app-page-exp..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page-tur..time.prod.js
Diff too large to display
Diff for app-page.runtime.dev.js
Diff too large to display
Diff for app-page.runtime.prod.js
Diff too large to display
Diff for app-route-ex..ntime.dev.js
Diff too large to display
Diff for app-route-ex..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route-tu..time.prod.js
Diff too large to display
Diff for app-route.runtime.dev.js
Diff too large to display
Diff for app-route.ru..time.prod.js
Diff too large to display
Diff for server.runtime.prod.js
Diff too large to display
Tests Passed |
ztanner
approved these changes
Apr 11, 2024
ztanner
pushed a commit
that referenced
this pull request
Apr 17, 2024
## Why Currently, Server Action handlers are just normal routes and they accepting POST requests. The only way to differentiate a normal request (page access, API route, etc.) from a Server Action request is to check that if it's a POST and has a Server Action ID set via headers or body (e.g. `multipart/form-data`). Usually, for existing page and API routes the correct handlers (page renderer, API route handler) will take precedence over Server Action's. But if the route doesn't exist (e.g. 404) it will still go through Server Action's handler and result in an error. And we're eagerly logging out that error because it might be an application failure, like passing a wrong Action ID. ## How In this PR we are making sure that the error is only logged if the Action ID isn't `null`. This means that it's an intentional Server Action request with a wrong ID. If the ID is `null`, we just handle it like 404 and log nothing. Fixes #64214. Closes NEXT-3071
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
Currently, Server Action handlers are just normal routes and they accepting POST requests. The only way to differentiate a normal request (page access, API route, etc.) from a Server Action request is to check that if it's a POST and has a Server Action ID set via headers or body (e.g.
multipart/form-data).Usually, for existing page and API routes the correct handlers (page renderer, API route handler) will take precedence over Server Action's. But if the route doesn't exist (e.g. 404) it will still go through Server Action's handler and result in an error. And we're eagerly logging out that error because it might be an application failure, like passing a wrong Action ID.
How
In this PR we are making sure that the error is only logged if the Action ID isn't
null. This means that it's an intentional Server Action request with a wrong ID. If the ID isnull, we just handle it like 404 and log nothing.Fixes #64214.
Closes NEXT-3071