From f5f71094ec74961b4cca2ee451798abd830c617a Mon Sep 17 00:00:00 2001 From: Florian Lefebvre Date: Fri, 8 Nov 2024 16:07:40 +0100 Subject: [PATCH 1/3] fix: error overlay message escape (#12305) Co-authored-by: Princesseuh <3019731+Princesseuh@users.noreply.github.com> --- .changeset/breezy-plums-clap.md | 5 +++++ packages/astro/src/core/errors/dev/vite.ts | 2 ++ packages/astro/src/core/module-loader/vite.ts | 21 +++++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 .changeset/breezy-plums-clap.md diff --git a/.changeset/breezy-plums-clap.md b/.changeset/breezy-plums-clap.md new file mode 100644 index 000000000000..d6d3f4b26aa8 --- /dev/null +++ b/.changeset/breezy-plums-clap.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a case where the error overlay would not escape the message diff --git a/packages/astro/src/core/errors/dev/vite.ts b/packages/astro/src/core/errors/dev/vite.ts index 56688877a861..9063e45b7079 100644 --- a/packages/astro/src/core/errors/dev/vite.ts +++ b/packages/astro/src/core/errors/dev/vite.ts @@ -105,6 +105,7 @@ export function enhanceViteSSRError({ } export interface AstroErrorPayload { + __isEnhancedAstroErrorPayload: true; type: ErrorPayload['type']; err: Omit & { name?: string; @@ -164,6 +165,7 @@ export async function getViteErrorPayload(err: ErrorWithMetadata): Promise { + events.emit('hmr-error', { + type: 'error', + err: { + message: payload.err.message, + stack: payload.err.stack, + }, + }); + + args[0] = payload; + _wsSend.apply(this, args); + }); + return; + } events.emit('hmr-error', msg); } _wsSend.apply(this, args); From 823e73b164eab4115af31b1de8e978f2b4e0a95d Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 8 Nov 2024 15:55:53 +0000 Subject: [PATCH 2/3] fix(actions): better runtime check for invalid usages (#12402) --- .changeset/dull-lemons-check.md | 14 ++++++++++++++ packages/astro/src/actions/runtime/middleware.ts | 3 ++- packages/astro/src/actions/runtime/route.ts | 3 ++- packages/astro/src/actions/runtime/utils.ts | 7 +++++++ .../astro/src/actions/runtime/virtual/server.ts | 10 ++++++++-- packages/astro/src/actions/utils.ts | 3 ++- packages/astro/test/actions.test.js | 6 ++++++ .../test/fixtures/actions/src/pages/invalid.astro | 6 ++++++ 8 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 .changeset/dull-lemons-check.md create mode 100644 packages/astro/test/fixtures/actions/src/pages/invalid.astro diff --git a/.changeset/dull-lemons-check.md b/.changeset/dull-lemons-check.md new file mode 100644 index 000000000000..c2c51c412088 --- /dev/null +++ b/.changeset/dull-lemons-check.md @@ -0,0 +1,14 @@ +--- +'astro': patch +--- + +Fixes a case where Astro allowed to call an action without using `Astro.callAction`. This is now invalid, and Astro will show a proper error. + +```diff +--- +import { actions } from "astro:actions"; + +-const result = actions.getUser({ userId: 123 }); ++const result = Astro.callAction(actions.getUser, { userId: 123 }); +--- +``` diff --git a/packages/astro/src/actions/runtime/middleware.ts b/packages/astro/src/actions/runtime/middleware.ts index 26966553149a..fdaac0b47785 100644 --- a/packages/astro/src/actions/runtime/middleware.ts +++ b/packages/astro/src/actions/runtime/middleware.ts @@ -3,7 +3,7 @@ import type { APIContext, MiddlewareNext } from '../../@types/astro.js'; import { defineMiddleware } from '../../core/middleware/index.js'; import { getOriginPathname } from '../../core/routing/rewrite.js'; import { ACTION_QUERY_PARAMS } from '../consts.js'; -import { formContentTypes, hasContentType } from './utils.js'; +import { ACTION_API_CONTEXT_SYMBOL, formContentTypes, hasContentType } from './utils.js'; import { getAction } from './virtual/get-action.js'; import { type SafeResult, @@ -100,6 +100,7 @@ async function handlePost({ formData = await request.clone().formData(); } const { getActionResult, callAction, props, redirect, ...actionAPIContext } = context; + Reflect.set(actionAPIContext, ACTION_API_CONTEXT_SYMBOL, true); const action = baseAction.bind(actionAPIContext); const actionResult = await action(formData); diff --git a/packages/astro/src/actions/runtime/route.ts b/packages/astro/src/actions/runtime/route.ts index 103936d72005..685025b19042 100644 --- a/packages/astro/src/actions/runtime/route.ts +++ b/packages/astro/src/actions/runtime/route.ts @@ -1,5 +1,5 @@ import type { APIRoute } from '../../@types/astro.js'; -import { formContentTypes, hasContentType } from './utils.js'; +import { ACTION_API_CONTEXT_SYMBOL, formContentTypes, hasContentType } from './utils.js'; import { getAction } from './virtual/get-action.js'; import { serializeActionResult } from './virtual/shared.js'; @@ -28,6 +28,7 @@ export const POST: APIRoute = async (context) => { return new Response(null, { status: 415 }); } const { getActionResult, callAction, props, redirect, ...actionAPIContext } = context; + Reflect.set(actionAPIContext, ACTION_API_CONTEXT_SYMBOL, true); const action = baseAction.bind(actionAPIContext); const result = await action(args); const serialized = serializeActionResult(result); diff --git a/packages/astro/src/actions/runtime/utils.ts b/packages/astro/src/actions/runtime/utils.ts index 84208c879d60..ed64ee812f2d 100644 --- a/packages/astro/src/actions/runtime/utils.ts +++ b/packages/astro/src/actions/runtime/utils.ts @@ -1,5 +1,7 @@ import type { APIContext } from '../../@types/astro.js'; +export const ACTION_API_CONTEXT_SYMBOL = Symbol.for('astro.actionAPIContext'); + export const formContentTypes = ['application/x-www-form-urlencoded', 'multipart/form-data']; export function hasContentType(contentType: string, expected: string[]) { @@ -26,3 +28,8 @@ export type MaybePromise = T | Promise; * `result.error.fields` will be typed with the `name` field. */ export type ErrorInferenceObject = Record; + +export function isActionAPIContext(ctx: ActionAPIContext): boolean { + const symbol = Reflect.get(ctx, ACTION_API_CONTEXT_SYMBOL); + return symbol === true; +} diff --git a/packages/astro/src/actions/runtime/virtual/server.ts b/packages/astro/src/actions/runtime/virtual/server.ts index 8e5e6bb4f1a5..01e8fbd86bfe 100644 --- a/packages/astro/src/actions/runtime/virtual/server.ts +++ b/packages/astro/src/actions/runtime/virtual/server.ts @@ -1,7 +1,12 @@ import { z } from 'zod'; import { ActionCalledFromServerError } from '../../../core/errors/errors-data.js'; import { AstroError } from '../../../core/errors/errors.js'; -import type { ActionAPIContext, ErrorInferenceObject, MaybePromise } from '../utils.js'; +import { + type ActionAPIContext, + type ErrorInferenceObject, + type MaybePromise, + isActionAPIContext, +} from '../utils.js'; import { ActionError, ActionInputError, type SafeResult, callSafely } from './shared.js'; export * from './shared.js'; @@ -60,7 +65,8 @@ export function defineAction< : getJsonServerHandler(handler, inputSchema); async function safeServerHandler(this: ActionAPIContext, unparsedInput: unknown) { - if (typeof this === 'function') { + // The ActionAPIContext should always contain the `params` property + if (typeof this === 'function' || !isActionAPIContext(this)) { throw new AstroError(ActionCalledFromServerError); } return callSafely(() => serverHandler(unparsedInput, this)); diff --git a/packages/astro/src/actions/utils.ts b/packages/astro/src/actions/utils.ts index 0e7c6fb62190..d8596a322930 100644 --- a/packages/astro/src/actions/utils.ts +++ b/packages/astro/src/actions/utils.ts @@ -2,7 +2,7 @@ import type fsMod from 'node:fs'; import * as eslexer from 'es-module-lexer'; import type { APIContext } from '../@types/astro.js'; import type { Locals } from './runtime/middleware.js'; -import type { ActionAPIContext } from './runtime/utils.js'; +import { ACTION_API_CONTEXT_SYMBOL, type ActionAPIContext } from './runtime/utils.js'; import { deserializeActionResult, getActionQueryString } from './runtime/virtual/shared.js'; export function hasActionPayload(locals: APIContext['locals']): locals is Locals { @@ -23,6 +23,7 @@ export function createGetActionResult(locals: APIContext['locals']): APIContext[ export function createCallAction(context: ActionAPIContext): APIContext['callAction'] { return (baseAction, input) => { + Reflect.set(context, ACTION_API_CONTEXT_SYMBOL, true); const action = baseAction.bind(context); return action(input) as any; }; diff --git a/packages/astro/test/actions.test.js b/packages/astro/test/actions.test.js index 0ed98db9357a..eed4d8734415 100644 --- a/packages/astro/test/actions.test.js +++ b/packages/astro/test/actions.test.js @@ -132,6 +132,12 @@ describe('Astro Actions', () => { assert.equal(data, 'Hello, ben!'); } }); + + it('Should fail when calling an action without using Astro.callAction', async () => { + const res = await fixture.fetch('/invalid/'); + const text = await res.text(); + assert.match(text, /ActionCalledFromServerError/); + }); }); describe('build', () => { diff --git a/packages/astro/test/fixtures/actions/src/pages/invalid.astro b/packages/astro/test/fixtures/actions/src/pages/invalid.astro new file mode 100644 index 000000000000..908eee853bd4 --- /dev/null +++ b/packages/astro/test/fixtures/actions/src/pages/invalid.astro @@ -0,0 +1,6 @@ +--- +import { actions } from "astro:actions"; + +// this is invalid, it should fail +const result = await actions.imageUploadInChunks(); +--- From 9cca10843912698e13d35f1bc3c493e2c96a06ee Mon Sep 17 00:00:00 2001 From: Ben Holmes Date: Fri, 8 Nov 2024 14:06:19 -0500 Subject: [PATCH 3/3] Fix incorrect status code in dev server for action errors (#12401) * remove default status that swallows response.status * refactor status compute to be more readable * changeset --- .changeset/nine-mayflies-film.md | 5 ++++ .../src/vite-plugin-astro-server/route.ts | 29 ++++++++++--------- 2 files changed, 21 insertions(+), 13 deletions(-) create mode 100644 .changeset/nine-mayflies-film.md diff --git a/.changeset/nine-mayflies-film.md b/.changeset/nine-mayflies-film.md new file mode 100644 index 000000000000..c782c73bc443 --- /dev/null +++ b/.changeset/nine-mayflies-film.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes unexpected 200 status in dev server logs for action errors and redirects. diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 30a6eda0190e..9f6d434b55e8 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -130,7 +130,6 @@ type HandleRoute = { manifestData: ManifestData; incomingRequest: http.IncomingMessage; incomingResponse: http.ServerResponse; - status?: 404 | 500 | 200; pipeline: DevPipeline; }; @@ -138,7 +137,6 @@ export async function handleRoute({ matchedRoute, url, pathname, - status = getStatus(matchedRoute), body, origin, pipeline, @@ -208,12 +206,17 @@ export async function handleRoute({ }); let response; + let statusCode = 200; let isReroute = false; let isRewrite = false; try { response = await renderContext.render(mod); isReroute = response.headers.has(REROUTE_DIRECTIVE_HEADER); isRewrite = response.headers.has(REWRITE_DIRECTIVE_HEADER_KEY); + statusCode = isRewrite + ? // Ignore `matchedRoute` status for rewrites + response.status + : (getStatusByMatchedRoute(matchedRoute) ?? response.status); } catch (err: any) { const custom500 = getCustom500Route(manifestData); if (!custom500) { @@ -225,7 +228,7 @@ export async function handleRoute({ const preloaded500Component = await pipeline.preload(custom500, filePath500); renderContext.props.error = err; response = await renderContext.render(preloaded500Component); - status = 500; + statusCode = 500; } if (isLoggedRequest(pathname)) { @@ -235,20 +238,20 @@ export async function handleRoute({ req({ url: pathname, method: incomingRequest.method, - statusCode: isRewrite ? response.status : (status ?? response.status), + statusCode, isRewrite, reqTime: timeEnd - timeStart, }), ); } - if (response.status === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') { + + if (statusCode === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') { const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline); if (options && options.route !== fourOhFourRoute?.route) return handleRoute({ ...options, matchedRoute: fourOhFourRoute, url: new URL(pathname, url), - status: 404, body, origin, pipeline, @@ -290,18 +293,18 @@ export async function handleRoute({ // Apply the `status` override to the response object before responding. // Response.status is read-only, so a clone is required to override. - if (status && response.status !== status && (status === 404 || status === 500)) { + if (response.status !== statusCode) { response = new Response(response.body, { - status: status, + status: statusCode, headers: response.headers, }); } await writeSSRResult(request, response, incomingResponse); } -function getStatus(matchedRoute?: MatchedRoute): 404 | 500 | 200 { - if (!matchedRoute) return 404; - if (matchedRoute.route.route === '/404') return 404; - if (matchedRoute.route.route === '/500') return 500; - return 200; +/** Check for /404 and /500 custom routes to compute status code */ +function getStatusByMatchedRoute(matchedRoute?: MatchedRoute) { + if (matchedRoute?.route.route === '/404') return 404; + if (matchedRoute?.route.route === '/500') return 500; + return undefined; }