diff --git a/.github/scripts/next-maintainer-auto-close.js b/.github/scripts/next-maintainer-auto-close.js new file mode 100644 index 000000000000..4d5bd3490c7a --- /dev/null +++ b/.github/scripts/next-maintainer-auto-close.js @@ -0,0 +1,155 @@ +async function deliverOneVerifiedClose({ core, github }) { + const queue = + 'https://next-maintainer-agent.vercel.tools/eve/agents/close-verifier/eve/v1/auto-close' + const repository = 'vercel/next.js' + const [owner, repo] = repository.split('/') + + async function queueRequest(path, body) { + const token = await core.getIDToken('next-maintainer-auto-close') + core.setSecret(token) + const response = await fetch(`${queue}${path}`, { + method: 'POST', + headers: { + authorization: `Bearer ${token}`, + 'x-vercel-trusted-oidc-idp-token': token, + ...(body === undefined ? {} : { 'content-type': 'application/json' }), + }, + ...(body === undefined ? {} : { body: JSON.stringify(body) }), + redirect: 'error', + signal: AbortSignal.timeout(15_000), + }) + if (response.status === 204) return null + const text = await response.text() + if (!response.ok) + throw new Error( + `Queue returned ${response.status}: ${text.slice(0, 300)}` + ) + return text.length === 0 ? null : JSON.parse(text) + } + + async function report(claim, outcome, error) { + await queueRequest(`/${encodeURIComponent(claim.approvalId)}/delivery`, { + leaseToken: claim.leaseToken, + outcome, + ...(error === undefined ? {} : { error: error.slice(0, 2_000) }), + }) + } + + async function reportStale(claim, message) { + core.warning(message) + await report(claim, 'stale', message) + } + + async function readIssue(number) { + try { + const issue = ( + await github.rest.issues.get({ owner, repo, issue_number: number }) + ).data + if ( + issue.number !== number || + issue.repository_url !== `https://api.github.com/repos/${repository}` + ) { + return null + } + return issue + } catch (error) { + if (error?.status === 404 || error?.status === 410) return null + throw error + } + } + + const claimed = await queueRequest('/claim') + if (claimed === null) return + const claim = claimed + const marker = `` + + try { + const issue = await readIssue(claim.issueNumber) + if (issue === null) { + await reportStale(claim, 'Issue no longer exists in this repository.') + return + } + if (issue.pull_request !== undefined) { + await reportStale(claim, 'Target is a pull request.') + return + } + + const comments = await github.paginate(github.rest.issues.listComments, { + owner, + repo, + issue_number: claim.issueNumber, + per_page: 100, + }) + const markerComment = comments.find((comment) => + (comment.body ?? '').includes(marker) + ) + + if (issue.state === 'closed') { + if (markerComment === undefined) { + await reportStale(claim, 'Issue was closed independently.') + } else { + await report(claim, 'closed') + } + return + } + if (markerComment !== undefined) { + // This is either a partial prior run or a human reopen. Leave it open. + await reportStale(claim, 'Issue is open after a prior delivery comment.') + return + } + + let duplicateIssueId + if (claim.stateReason === 'duplicate') { + if (claim.duplicateIssueNumber === claim.issueNumber) { + await reportStale(claim, 'Issue cannot be a duplicate of itself.') + return + } + const canonical = await readIssue(claim.duplicateIssueNumber) + if (canonical === null || canonical.pull_request !== undefined) { + await reportStale( + claim, + 'Duplicate target is not an issue in this repository.' + ) + return + } + duplicateIssueId = canonical.id + } + + await github.rest.issues.createComment({ + owner, + repo, + issue_number: claim.issueNumber, + body: `${claim.closeComment}\n\n${marker}`, + }) + + const updated = ( + await github.rest.issues.update({ + owner, + repo, + issue_number: claim.issueNumber, + state: 'closed', + state_reason: claim.stateReason, + ...(duplicateIssueId === undefined + ? {} + : { duplicate_issue_id: duplicateIssueId }), + }) + ).data + if ( + updated.state !== 'closed' || + updated.state_reason !== claim.stateReason + ) { + throw new Error('GitHub did not apply the requested close reason.') + } + await report(claim, 'closed') + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + try { + await report(claim, 'retry', message) + } catch (callbackError) { + core.warning(`Could not report retry: ${callbackError}`) + } + throw error + } +} + +module.exports = { deliverOneVerifiedClose } diff --git a/.github/workflows/next-maintainer-auto-close.yml b/.github/workflows/next-maintainer-auto-close.yml new file mode 100644 index 000000000000..be6fc0d2b790 --- /dev/null +++ b/.github/workflows/next-maintainer-auto-close.yml @@ -0,0 +1,38 @@ +name: Next Maintainer Auto Close + +on: + # schedule: + # - cron: '0 * * * *' + workflow_dispatch: + +permissions: {} + +concurrency: + group: next-maintainer-auto-close + cancel-in-progress: false + +jobs: + close-one-verified-issue: + if: github.repository == 'vercel/next.js' && github.ref == 'refs/heads/canary' + runs-on: ubuntu-latest + timeout-minutes: 5 + permissions: + contents: read + id-token: write + issues: write + steps: + - name: Checkout trusted workflow code + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.sha }} + persist-credentials: false + sparse-checkout: .github/scripts/next-maintainer-auto-close.js + sparse-checkout-cone-mode: false + + - name: Pull and deliver one verified close + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + github-token: ${{ secrets.GITHUB_TOKEN }} + script: | + const { deliverOneVerifiedClose } = require('./.github/scripts/next-maintainer-auto-close.js') + await deliverOneVerifiedClose({ core, github }) diff --git a/crates/next-api/src/next_server_nft.rs b/crates/next-api/src/next_server_nft.rs index d171159ba056..3b34b439bac7 100644 --- a/crates/next-api/src/next_server_nft.rs +++ b/crates/next-api/src/next_server_nft.rs @@ -5,14 +5,17 @@ use bincode::{Decode, Encode}; use either::Either; use next_core::{get_next_package, next_server::get_tracing_compile_time_info}; use serde_json::json; +use turbo_rcstr::{RcStr, rcstr}; use turbo_tasks::{ResolvedVc, TryFlatJoinIterExt, TryJoinIterExt, Vc, trace::TraceRawVcs}; use turbo_tasks_fs::{ - DirectoryContent, DirectoryEntry, File, FileContent, FileSystemPath, glob::Glob, + DirectoryContent, DirectoryEntry, File, FileContent, FileSystemPath, + glob::{Glob, GlobOptions}, }; use turbo_tasks_hash::HashAlgorithm; use turbopack::externals_tracing_module_context; use turbopack_core::{ asset::{Asset, AssetContent}, + context::AssetContext, module::{Module, Modules}, module_graph::{GraphEntries, ModuleGraph, SingleModuleGraph}, output::{OutputAsset, OutputAssets, OutputAssetsReference}, @@ -34,11 +37,10 @@ use crate::{nft::traced_modules_for_entries, project::Project}; /// Used by the server NFTs below and, so that they are part of every endpoint's trace regardless /// of how the output is assembled, by [`Project::additional_traced_modules`]. #[turbo_tasks::function] -pub(crate) async fn require_hook_modules(project_path: FileSystemPath) -> Result> { - let asset_context = Vc::upcast(externals_tracing_module_context( - get_tracing_compile_time_info(), - false, - )); +pub(crate) async fn require_hook_modules( + project_path: FileSystemPath, + asset_context: Vc>, +) -> Result> { let next_resolve_origin = Vc::upcast(PlainResolveOrigin::new( asset_context, get_next_package(project_path).await?.join("_")?, @@ -75,6 +77,7 @@ pub(crate) async fn pages_renderer_modules(project_path: FileSystemPath) -> Resu let asset_context = Vc::upcast(externals_tracing_module_context( get_tracing_compile_time_info(), false, + None, )); let next_resolve_origin = Vc::upcast(PlainResolveOrigin::new( asset_context, @@ -282,15 +285,97 @@ impl Asset for ServerNftJsonAsset { } } +/// These globs are used to prune the module graph for the server NFT JSON assets. They are always +/// ignored for every page, so we can completely skip even parsing/walking these modules. +fn next_owned_ignores( + ty: &ServerNftType, + has_next_support: bool, + is_standalone: bool, +) -> Vec { + let mut globs = vec![ + rcstr!("**/node_modules/react{,-dom,-server-dom-turbopack}/**/*.development.js"), + rcstr!("**/*.d.ts"), + rcstr!("**/*.map"), + rcstr!("**/next/dist/pages/**/*"), + rcstr!("**/next/dist/compiled/next-server/**/*.dev.js"), + rcstr!("**/next/dist/compiled/webpack/*"), + rcstr!("**/node_modules/webpack5/**/*"), + rcstr!("**/next/dist/server/lib/route-resolver*"), + // The testmode interceptors bundle reads its HTTP parser WASM with a + // dynamic path, making the tracer include the bundle's whole + // directory. Test proxying is not supported in standalone output, so + // keep the parser asset (and the license file picked up by the + // directory glob) out of production traces. + rcstr!("**/next/dist/compiled/@mswjs/interceptors/ClientRequest/LICENSE"), + rcstr!("**/next/dist/compiled/@mswjs/interceptors/ClientRequest/llhttp/**"), + rcstr!("**/next/dist/compiled/semver/semver/**/*.js"), + rcstr!("**/next/dist/compiled/jest-worker/**/*"), + // -- The following were added for Turbopack specifically -- + // client/components/use-action-queue.ts has a process.env.NODE_ENV guard, but we can't set that due to React: https://github.com/vercel/next.js/pull/75254 + rcstr!("**/next/dist/next-devtools/userspace/use-app-dev-rendering-indicator.js"), + // client/components/app-router.js has a process.env.NODE_ENV guard, but we + // can't set that. + rcstr!("**/next/dist/client/dev/hot-reloader/app/hot-reloader-app.js"), + // server/lib/router-server.js doesn't guard this require: + rcstr!("**/next/dist/server/lib/router-utils/setup-dev-bundler.js"), + // server/next.js doesn't guard this require + rcstr!("**/next/dist/server/dev/next-dev-server.js"), + // next/dist/compiled/babel* pulls in this, but we never actually transpile at + // deploy-time + rcstr!("**/next/dist/compiled/browserslist/**"), + ]; + + // only ignore image-optimizer code when + // this is being handled outside of next-server + if has_next_support { + globs.extend([ + rcstr!("**/node_modules/sharp/**/*"), + rcstr!("**/@img/sharp-libvips*/**/*"), + rcstr!("**/next/dist/server/image-optimizer.js"), + ]); + } + + if !is_standalone { + globs.extend([ + rcstr!("**/*/next/dist/server/next.js"), + rcstr!("**/*/next/dist/bin/next"), + ]); + } + + if matches!(ty, ServerNftType::Minimal) { + globs.extend([ + rcstr!("**/next/dist/compiled/edge-runtime/**/*"), + rcstr!("**/next/dist/server/web/sandbox/**/*"), + rcstr!("**/next/dist/server/post-process.js"), + ]); + } + + globs +} + #[turbo_tasks::value_impl] impl ServerNftJsonAsset { #[turbo_tasks::function] async fn entries(&self) -> Result> { let is_standalone = *self.project.next_config().is_standalone().await?; + let prune = Glob::alternatives( + next_owned_ignores( + &self.ty, + *self.project.ci_has_next_support().await?, + is_standalone, + ) + .into_iter() + .map(|g| Glob::new(g, GlobOptions::default())) + .collect(), + ) + .to_resolved() + .await?; + let asset_context = Vc::upcast(externals_tracing_module_context( get_tracing_compile_time_info(), false, + Some((self.project.project_root_path().owned().await?, prune)), )); let project_path = self.project.project_path().owned().await?; @@ -324,7 +409,9 @@ impl ServerNftJsonAsset { // The modules the require hook needs are part of every endpoint's trace (see // `Project::additional_traced_modules`), but `next-server.js` / `next-minimal-server.js` // are traced on their own for `output: 'standalone'`, so they have to be added here too. - let hook_modules = require_hook_modules(project_path).owned().await?; + let hook_modules = require_hook_modules(project_path, asset_context) + .owned() + .await?; Ok(Vc::cell( hook_modules @@ -369,86 +456,25 @@ impl ServerNftJsonAsset { if route_glob.await?.matches("next-server") { for (glob, root) in exclude_patterns { additional_ignores.insert(if root.path.is_empty() { - glob.to_string() + glob.clone() } else { - format!("{root}/{glob}") + format!("{root}/{glob}").into() }); } } } - let server_ignores_glob = [ - "**/node_modules/react{,-dom,-server-dom-turbopack}/**/*.development.js", - "**/*.d.ts", - "**/*.map", - "**/next/dist/pages/**/*", - "**/next/dist/compiled/next-server/**/*.dev.js", - "**/next/dist/compiled/webpack/*", - "**/node_modules/webpack5/**/*", - "**/next/dist/server/lib/route-resolver*", - // The testmode interceptors bundle reads its HTTP parser WASM with a - // dynamic path, making the tracer include the bundle's whole - // directory. Test proxying is not supported in standalone output, so - // keep the parser asset (and the license file picked up by the - // directory glob) out of production traces. - "**/next/dist/compiled/@mswjs/interceptors/ClientRequest/LICENSE", - "**/next/dist/compiled/@mswjs/interceptors/ClientRequest/llhttp/**", - "**/next/dist/compiled/semver/semver/**/*.js", - "**/next/dist/compiled/jest-worker/**/*", - // -- The following were added for Turbopack specifically -- - // client/components/use-action-queue.ts has a process.env.NODE_ENV guard, but we can't set that due to React: https://github.com/vercel/next.js/pull/75254 - "**/next/dist/next-devtools/userspace/use-app-dev-rendering-indicator.js", - // client/components/app-router.js has a process.env.NODE_ENV guard, but we - // can't set that. - "**/next/dist/client/dev/hot-reloader/app/hot-reloader-app.js", - // server/lib/router-server.js doesn't guard this require: - "**/next/dist/server/lib/router-utils/setup-dev-bundler.js", - // server/next.js doesn't guard this require - "**/next/dist/server/dev/next-dev-server.js", - // next/dist/compiled/babel* pulls in this, but we never actually transpile at - // deploy-time - "**/next/dist/compiled/browserslist/**", - ] - .into_iter() - .chain(additional_ignores.iter().map(|s| s.as_str())) - // only ignore image-optimizer code when - // this is being handled outside of next-server - .chain(if has_next_support { - Either::Left( - [ - "**/node_modules/sharp/**/*", - "**/@img/sharp-libvips*/**/*", - "**/next/dist/server/image-optimizer.js", - ] - .into_iter(), - ) - } else { - Either::Right(std::iter::empty()) - }) - .chain(if is_standalone { - Either::Left(std::iter::empty()) - } else { - Either::Right(["**/*/next/dist/server/next.js", "**/*/next/dist/bin/next"].into_iter()) - }) - .map(|g| Glob::new(g.into(), Default::default())) - .collect::>(); - - Ok(match self.ty { - ServerNftType::Full => Glob::alternatives(server_ignores_glob), - ServerNftType::Minimal => Glob::alternatives( - server_ignores_glob - .into_iter() - .chain( - [ - "**/next/dist/compiled/edge-runtime/**/*", - "**/next/dist/server/web/sandbox/**/*", - "**/next/dist/server/post-process.js", - ] - .into_iter() - .map(|g| Glob::new(g.into(), Default::default())), - ) - .collect(), - ), - }) + // The project-provided ignores can match one of the entry requests `entries()` resolves, + // so they can only be applied to the finished graph: + // `traced_modules_for_entries` inserts entries without consulting the glob (the + // `parent == None` arm in `nft.rs`), whereas pruning one would delete it and everything + // reachable only through it. + let server_ignores_glob = next_owned_ignores(&self.ty, has_next_support, is_standalone) + .into_iter() + .chain(additional_ignores) + .map(|g| Glob::new(g, Default::default())) + .collect::>(); + + Ok(Glob::alternatives(server_ignores_glob)) } } diff --git a/crates/next-api/src/project.rs b/crates/next-api/src/project.rs index 631a6c91a99d..e3204c52fa83 100644 --- a/crates/next-api/src/project.rs +++ b/crates/next-api/src/project.rs @@ -2682,7 +2682,7 @@ impl Project { .await?; let asset_context = - externals_tracing_module_context(get_tracing_compile_time_info(), false); + externals_tracing_module_context(get_tracing_compile_time_info(), false, None); Ok(Vc::cell( cache_handler @@ -2707,7 +2707,12 @@ impl Project { /// Other endpoints use [`Project::additional_traced_modules`]. #[turbo_tasks::function] pub async fn pages_traced_modules(self: Vc) -> Result> { - let hook_modules = require_hook_modules(self.project_path().owned().await?) + let asset_context = Vc::upcast(externals_tracing_module_context( + get_tracing_compile_time_info(), + false, + None, + )); + let hook_modules = require_hook_modules(self.project_path().owned().await?, asset_context) .owned() .await?; let renderer_modules = pages_renderer_modules(self.project_path().owned().await?) diff --git a/packages/next/src/server/app-render/action-handler.ts b/packages/next/src/server/app-render/action-handler.ts index 3a4dc927fe8d..64b1684ccffb 100644 --- a/packages/next/src/server/app-render/action-handler.ts +++ b/packages/next/src/server/app-render/action-handler.ts @@ -94,6 +94,16 @@ function hasServerActions() { ) } +function getUnrecognizedActionStatusCode(actionId: string | null): 400 | 409 { + return actionId !== null && !mightBeServerReferenceId(actionId) ? 400 : 409 +} + +function getUnrecognizedActionResponseBody(statusCode: 400 | 409): string { + return statusCode === 400 + ? 'Invalid Server Action request.' + : 'Server Action unavailable.' +} + function nodeHeadersToRecord( headers: IncomingHttpHeaders | OutgoingHttpHeaders ) { @@ -212,7 +222,8 @@ async function createForwardedActionResponse( res: BaseNextResponse, host: Host, workerPathname: string, - basePath: string + basePath: string, + actionId: string ) { if (!host) { throw new Error( @@ -306,8 +317,14 @@ async function createForwardedActionResponse( if (response.headers.get(NEXT_ACTION_NOT_FOUND_HEADER) === '1') { res.setHeader(NEXT_ACTION_NOT_FOUND_HEADER, '1') res.setHeader('content-type', 'text/plain') - res.statusCode = 404 - return RenderResult.fromStatic('Server action not found.', 'text/plain') + // The marker denotes an unavailable action. Derive the status from the + // requested ID so mixed-version workers cannot change its semantics. + const statusCode = getUnrecognizedActionStatusCode(actionId) + res.statusCode = statusCode + return RenderResult.fromStatic( + getUnrecognizedActionResponseBody(statusCode), + 'text/plain' + ) } } catch (err) { // we couldn't stream the forwarded response, so we'll just return an empty response @@ -610,7 +627,10 @@ export async function handleAction({ isPossibleServerAction, } = getServerActionRequestMetadata(req) - const handleUnrecognizedFetchAction = (err: unknown): HandleActionResult => { + const handleUnrecognizedAction = ( + err: unknown, + statusCode: 400 | 409 + ): 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) @@ -621,10 +641,13 @@ export async function handleAction({ // (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 + res.statusCode = statusCode return { type: 'done', - result: RenderResult.fromStatic('Server action not found.', 'text/plain'), + result: RenderResult.fromStatic( + getUnrecognizedActionResponseBody(statusCode), + 'text/plain' + ), } } @@ -648,13 +671,16 @@ export async function handleAction({ } } - // If the app has no server actions at all, we can 404 early. + // If the app has no server actions at all, we can reject the request early. if (!hasServerActions()) { const error = actionId !== null && !mightBeServerReferenceId(actionId) ? getInvalidServerReferenceIdError(actionId) : getActionNotFoundError(actionId) - return handleUnrecognizedFetchAction(error) + return handleUnrecognizedAction( + error, + getUnrecognizedActionStatusCode(actionId) + ) } let temporaryReferences: TemporaryReferenceSet | undefined @@ -780,7 +806,8 @@ export async function handleAction({ res, host, forwardedWorker, - ctx.renderOpts.basePath + ctx.renderOpts.basePath, + actionId ), } } @@ -868,7 +895,10 @@ export async function handleAction({ try { actionModId = getActionModIdOrError(actionId, serverModuleMap) } catch (err) { - return handleUnrecognizedFetchAction(err) + return handleUnrecognizedAction( + err, + getUnrecognizedActionStatusCode(actionId) + ) } boundActionArguments = await decodeReply( @@ -879,12 +909,15 @@ 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` - ) + try { + if (!areAllActionIdsValid(formData, serverModuleMap)) { + return handleUnrecognizedAction( + new Error('Invalid Server Actions request.'), + 400 + ) + } + } catch (err) { + return handleUnrecognizedAction(err, 409) } const action = await decodeAction(formData, serverModuleMap) @@ -932,7 +965,10 @@ export async function handleAction({ try { actionModId = getActionModIdOrError(actionId, serverModuleMap) } catch (err) { - return handleUnrecognizedFetchAction(err) + return handleUnrecognizedAction( + err, + getUnrecognizedActionStatusCode(actionId) + ) } // A fetch action with a non-multipart body. @@ -1029,7 +1065,10 @@ export async function handleAction({ try { actionModId = getActionModIdOrError(actionId, serverModuleMap) } catch (err) { - return handleUnrecognizedFetchAction(err) + return handleUnrecognizedAction( + err, + getUnrecognizedActionStatusCode(actionId) + ) } const busboy = ( @@ -1086,12 +1125,15 @@ export async function handleAction({ throw err } - 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` - ) + try { + if (!areAllActionIdsValid(formData, serverModuleMap)) { + return handleUnrecognizedAction( + new Error('Invalid Server Actions request.'), + 400 + ) + } + } catch (err) { + return handleUnrecognizedAction(err, 409) } // TODO: Refactor so it is harder to accidentally decode an action before you have validated that the @@ -1141,7 +1183,10 @@ export async function handleAction({ try { actionModId = getActionModIdOrError(actionId, serverModuleMap) } catch (err) { - return handleUnrecognizedFetchAction(err) + return handleUnrecognizedAction( + err, + getUnrecognizedActionStatusCode(actionId) + ) } // A fetch action with a non-multipart body. @@ -1497,8 +1542,8 @@ function areAllActionIdsValid( ): boolean { let seenActionRefs = 0 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 + // Before we attempt to decode the payload for a possible MPA action, assert + // that all action IDs are valid IDs. for (let key of mpaFormData.keys()) { if (!key.startsWith($ACTION_)) { // not a relevant field @@ -1545,7 +1590,7 @@ const ACTION_DESCRIPTOR_ID_PREFIX = '{"id":"' function isInvalidStringActionDescriptor( actionDescriptor: string, serverModuleMap: ServerModuleMap -): unknown { +): boolean { if (actionDescriptor.startsWith(ACTION_DESCRIPTOR_ID_PREFIX) === false) { return true } diff --git a/test/e2e/app-dir/actions-unrecognized/actions-unrecognized.test.ts b/test/e2e/app-dir/actions-unrecognized/actions-unrecognized.test.ts index 77100b3e09e6..deffab5cc0ba 100644 --- a/test/e2e/app-dir/actions-unrecognized/actions-unrecognized.test.ts +++ b/test/e2e/app-dir/actions-unrecognized/actions-unrecognized.test.ts @@ -6,7 +6,7 @@ import { outdent } from 'outdent' describe('unrecognized server actions', () => { const unrecognizedActionId = '0'.repeat(42) - const { next, isNextDeploy, isNextDev } = nextTestSetup({ + const { next, isNextDeploy } = nextTestSetup({ files: __dirname, }) @@ -42,6 +42,7 @@ describe('unrecognized server actions', () => { { idType: 'malformed', actionId: '123', + expectedStatus: 400, expectedError: outdent` The Server Reference ID did not match the expected format. Received "123". Read more: https://nextjs.org/docs/messages/failed-to-find-server-action @@ -50,6 +51,7 @@ describe('unrecognized server actions', () => { { idType: 'plausible but missing', actionId: unrecognizedActionId, + expectedStatus: 409, expectedError: outdent` Failed to find Server Action "${unrecognizedActionId}". This request might be from an older or newer deployment. Read more: https://nextjs.org/docs/messages/failed-to-find-server-action @@ -62,12 +64,13 @@ describe('unrecognized server actions', () => { // We should still surface a diagnosable error instead of a TypeError. idType: 'well-known property name', actionId: 'toString', + expectedStatus: 400, expectedError: outdent` The Server Reference ID did not match the expected format. Received "toString". Read more: https://nextjs.org/docs/messages/failed-to-find-server-action `, }, - ])('with a $idType id', ({ actionId, expectedError }) => { + ])('with a $idType id', ({ actionId, expectedStatus, expectedError }) => { it.each([ { // encodeReply encodes simple args as plaintext. @@ -87,7 +90,7 @@ describe('unrecognized server actions', () => { }, }, ])( - 'should 404 when POSTing a server action to a nonexistent page: $name', + 'should reject a server action POST to a nonexistent page: $name', async ({ request: { contentType, body } }) => { const res = await next.fetch('/non-existent-route', { method: 'POST', @@ -99,7 +102,7 @@ describe('unrecognized server actions', () => { body, }) - expect(res.status).toBe(404) + expect(res.status).toBe(expectedStatus) const cliOutput = getLogs() expect(cliOutput).not.toContain('TypeError') @@ -129,6 +132,48 @@ describe('unrecognized server actions', () => { describe.each(['nodejs', 'edge'])( 'should error and log a warning when submitting a server action with an unrecognized ID - %s', (runtime) => { + it.each([ + { + description: 'a malformed ID', + actionId: '123', + expectedStatus: 400, + expectedBody: 'Invalid Server Action request.', + expectedError: 'Invalid Server Actions request.', + }, + { + description: 'a plausible but missing ID', + actionId: unrecognizedActionId, + expectedStatus: 409, + expectedBody: 'Server Action unavailable.', + expectedError: outdent` + Failed to find Server Action "${unrecognizedActionId}". This request might be from an older or newer deployment. + Read more: https://nextjs.org/docs/messages/failed-to-find-server-action + `, + }, + ])( + 'should reject an MPA action with $description', + async ({ actionId, expectedStatus, expectedBody, expectedError }) => { + const boundary = '----nextjs-test-boundary' + const body = `--${boundary}\r\nContent-Disposition: form-data; name="$ACTION_ID_${actionId}"\r\n\r\n\r\n--${boundary}--\r\n` + + const response = await next.fetch(`/${runtime}/unrecognized-action`, { + method: 'POST', + headers: { + 'content-type': `multipart/form-data; boundary=${boundary}`, + }, + body, + }) + + expect(response.status).toBe(expectedStatus) + expect(response.headers.get('content-type')).toStartWith('text/plain') + expect(await response.text()).toBe(expectedBody) + + if (!isNextDeploy) { + await retry(async () => expect(getLogs()).toInclude(expectedError)) + } + } + ) + const testUnrecognizedActionSubmission = async ({ formId, disableJavaScript, @@ -156,7 +201,7 @@ describe('unrecognized server actions', () => { if (!disableJavaScript) { // A fetch action, sent via the router. - expect(response.status()).toBe(404) + expect(response.status()).toBe(409) // NOTE: we cannot validate the response text, because playwright hangs on `response.text()` for some reason. expect(response.headers()['content-type']).toStartWith('text/plain') @@ -165,7 +210,7 @@ describe('unrecognized server actions', () => { /Error boundary: Server Action ".+?" was not found on the server\./ ) - // We responded with a 404, but we shouldn't trigger a not-found (either a custom or a default one) + // We responded with a 409, but we shouldn't trigger a not-found (either a custom or a default one) expect(await browser.elementByCss('body').text()).not.toContain( 'Not found' ) @@ -183,39 +228,18 @@ describe('unrecognized server actions', () => { } } else { // An MPA action, sent without JS. + expect(response.status()).toBe(409) + expect(response.headers()['content-type']).toStartWith('text/plain') + expect(await browser.elementByCss('body').text()).toBe( + 'Server Action unavailable.' + ) - // FIXME: When deployed, the request is logged as a 500, but returns a 405. - // We also don't seem to display the error page correctly if (!isNextDeploy) { - // FIXME: Currently, an unrecognized id in an MPA action results in a 500. - // This is not ideal, and ignores all nested `error.js` files, only showing the topmost one. - expect(response.status()).toBe(500) - if (isNextDev) { - expect(response.headers()['content-type']).toStartWith( - 'text/html' - ) - } else { - const responseText = await response.text() - expect(responseText).toBe('Internal Server Error') - expect(response.headers()['content-type']).toStartWith( - 'text/plain' - ) - } - - // In dev, the 500 page doesn't have any SSR'd html, so it won't show anything without JS. - if (!isNextDev) { - expect(await browser.elementByCss('body').text()).toContain( - 'Internal Server Error' - ) - } - - if (!isNextDeploy) { - await retry(async () => - expect(getLogs()).toInclude( - `Error: Failed to find Server Action "${unrecognizedActionId}". This request might be from an older or newer deployment` - ) + await retry(async () => + expect(getLogs()).toInclude( + `Error: Failed to find Server Action "${unrecognizedActionId}". This request might be from an older or newer deployment` ) - } + ) } } } diff --git a/test/e2e/app-dir/no-server-actions/no-server-actions.test.ts b/test/e2e/app-dir/no-server-actions/no-server-actions.test.ts index fc476483a32c..1c1be04588c9 100644 --- a/test/e2e/app-dir/no-server-actions/no-server-actions.test.ts +++ b/test/e2e/app-dir/no-server-actions/no-server-actions.test.ts @@ -11,17 +11,19 @@ describe('app-dir - no server actions', () => { { description: 'a malformed id', actionId: 'abc123', + expectedStatus: 400, expectedError: 'The Server Reference ID did not match the expected format. Received "abc123".\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action', }, { description: 'a plausible but missing id', actionId: missingActionId, + expectedStatus: 409, expectedError: `Failed to find Server Action "${missingActionId}". This request might be from an older or newer deployment.\nRead more: https://nextjs.org/docs/messages/failed-to-find-server-action`, }, ])( 'should error when triggering a fetch action with $description on an app with no server actions', - async ({ actionId, expectedError }) => { + async ({ actionId, expectedStatus, expectedError }) => { const res = await next.fetch('/', { method: 'POST', headers: { @@ -29,7 +31,7 @@ describe('app-dir - no server actions', () => { }, }) - expect(res.status).toBe(404) + expect(res.status).toBe(expectedStatus) expect(res.headers.get('x-nextjs-action-not-found')).toBe('1') // Runtime logs and custom headers are not forwarded to the client when deployed. @@ -52,7 +54,7 @@ describe('app-dir - no server actions', () => { body: formData, }) - expect(res.status).toBe(404) + expect(res.status).toBe(409) expect(res.headers.get('x-nextjs-action-not-found')).toBe('1') // Runtime logs are not available when deployed. diff --git a/turbopack/crates/turbopack/src/evaluate_context.rs b/turbopack/crates/turbopack/src/evaluate_context.rs index f4f9f31747f0..f4a1c26c910d 100644 --- a/turbopack/crates/turbopack/src/evaluate_context.rs +++ b/turbopack/crates/turbopack/src/evaluate_context.rs @@ -140,5 +140,6 @@ pub async fn config_tracing_module_context( .cell() .await?, true, + None, ))) } diff --git a/turbopack/crates/turbopack/src/lib.rs b/turbopack/crates/turbopack/src/lib.rs index 028b06b36db9..521200e6a56d 100644 --- a/turbopack/crates/turbopack/src/lib.rs +++ b/turbopack/crates/turbopack/src/lib.rs @@ -12,13 +12,14 @@ pub mod transition; use anyhow::{Context as _, Result, bail}; use module_options::{ - ConfiguredModuleType, ModuleOptions, ModuleOptionsContext, ModuleRuleEffect, ModuleType, + ConfiguredModuleType, ModuleOptions, ModuleOptionsContext, ModuleRule, ModuleRuleEffect, + ModuleType, RuleCondition, }; pub use runtime_asset_context::get_runtime_asset_context; use tracing::{Instrument, field::Empty}; use turbo_rcstr::{RcStr, rcstr}; use turbo_tasks::{ResolvedVc, TryJoinIterExt, ValueToString, Vc}; -use turbo_tasks_fs::FileSystemPath; +use turbo_tasks_fs::{FileSystemPath, glob::Glob}; pub use turbopack_core::condition; use turbopack_core::{ asset::Asset, @@ -910,16 +911,30 @@ async fn process_default_internal( Ok(module) } +/// `prune` skips matching files as the graph is walked, rather than filtering them out of the +/// result afterwards. #[turbo_tasks::function] pub async fn externals_tracing_module_context( compile_time_info: Vc, resolve_typescript: bool, + prune: Option<(FileSystemPath, ResolvedVc)>, ) -> Result> { let mut extensions = vec![rcstr!(".js"), rcstr!(".node"), rcstr!(".json")]; if resolve_typescript { extensions.insert(0, rcstr!(".ts")); } + let prune_rules = match prune { + Some((base, glob)) => vec![ModuleRule::new( + RuleCondition::ResourcePathGlob { + base, + glob: glob.await?, + }, + vec![ModuleRuleEffect::Ignore], + )], + None => vec![], + }; + let resolve_options = ResolveOptionsContext { custom_extensions: Some(extensions), emulate_environment: Some(compile_time_info.await?.environment), @@ -957,6 +972,7 @@ pub async fn externals_tracing_module_context( // node-file-trace. environment: None, analyze_mode: AnalyzeMode::Tracing, + module_rules: prune_rules, // Disable tree shaking. Even side-effect-free imports need to be traced, as they will // execute at runtime. ..Default::default() @@ -1112,6 +1128,7 @@ impl AssetContext for ModuleAssetContext { Vc::upcast(externals_tracing_module_context( *options.compile_time_info, false, + None, )), // If target is specified, a symlink will be created to // make the folder