From ada0ae6658b7bf1508a12d090bd1efb972bab8cf Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Wed, 9 Oct 2024 16:17:02 +0100 Subject: [PATCH 1/4] fix(routing): actions should redirect the original pathname --- .changeset/tough-snakes-reflect.md | 5 +++ packages/astro/e2e/actions-blog.test.js | 13 +++++++ .../actions-blog/src/actions/index.ts | 10 ++++++ .../fixtures/actions-blog/src/middleware.ts | 9 +++++ .../actions-blog/src/pages/rewritten.astro | 18 ++++++++++ .../astro/src/actions/runtime/middleware.ts | 8 ++++- packages/astro/src/core/constants.ts | 5 +++ .../astro/src/core/middleware/sequence.ts | 5 +-- packages/astro/src/core/render-context.ts | 36 +++---------------- packages/astro/src/core/routing/rewrite.ts | 30 ++++++++++++++++ 10 files changed, 105 insertions(+), 34 deletions(-) create mode 100644 .changeset/tough-snakes-reflect.md create mode 100644 packages/astro/e2e/fixtures/actions-blog/src/middleware.ts create mode 100644 packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro diff --git a/.changeset/tough-snakes-reflect.md b/.changeset/tough-snakes-reflect.md new file mode 100644 index 000000000000..7fdaca60397e --- /dev/null +++ b/.changeset/tough-snakes-reflect.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a bug where Astro Actions couldn't redirect to the correct pathname when there was a rewrite involved. diff --git a/packages/astro/e2e/actions-blog.test.js b/packages/astro/e2e/actions-blog.test.js index d9c1bc1dfd68..f233c44739e6 100644 --- a/packages/astro/e2e/actions-blog.test.js +++ b/packages/astro/e2e/actions-blog.test.js @@ -135,4 +135,17 @@ test.describe('Astro Actions - Blog', () => { await logoutButton.click(); await expect(page).toHaveURL(astro.resolveUrl('/blog/')); }); + + test('Should redirect to the origin pathname when there is a rewrite', async ({ + page, + astro, + }) => { + await page.goto(astro.resolveUrl('/sum')); + + const submitButton = page.getByTestId('submit'); + await waitForHydrate(page, submitButton); + await submitButton.click(); + await expect(page).toHaveURL(astro.resolveUrl('/sum/')); + await expect(page).toContainText('Form result: { "data": 3 }'); + }); }); diff --git a/packages/astro/e2e/fixtures/actions-blog/src/actions/index.ts b/packages/astro/e2e/fixtures/actions-blog/src/actions/index.ts index 43ffb43d42d4..a5437ad49a3c 100644 --- a/packages/astro/e2e/fixtures/actions-blog/src/actions/index.ts +++ b/packages/astro/e2e/fixtures/actions-blog/src/actions/index.ts @@ -56,4 +56,14 @@ export const server = { }, }), }, + sum: defineAction({ + accept: "form", + input: z.object({ + a: z.number(), + b: z.number(), + }), + async handler({ a, b }) { + return a + b + }, + }) }; diff --git a/packages/astro/e2e/fixtures/actions-blog/src/middleware.ts b/packages/astro/e2e/fixtures/actions-blog/src/middleware.ts new file mode 100644 index 000000000000..53bb8235ac2a --- /dev/null +++ b/packages/astro/e2e/fixtures/actions-blog/src/middleware.ts @@ -0,0 +1,9 @@ +import { defineMiddleware } from "astro:middleware"; + +export const onRequest = defineMiddleware((ctx, next) => { + if (ctx.request.method === "GET" && ctx.url.pathname === "/sum") { + return next("/rewritten") + } + + return next() +}) diff --git a/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro b/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro new file mode 100644 index 000000000000..115e6a9374f1 --- /dev/null +++ b/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro @@ -0,0 +1,18 @@ +--- +import { actions } from "astro:actions"; + +const result = Astro.getActionResult(actions.sum) + +--- + + + +
+ + + +
+

Form result: {JSON.stringify(result)}

+ + + diff --git a/packages/astro/src/actions/runtime/middleware.ts b/packages/astro/src/actions/runtime/middleware.ts index a61e1652c8b7..abf84765e934 100644 --- a/packages/astro/src/actions/runtime/middleware.ts +++ b/packages/astro/src/actions/runtime/middleware.ts @@ -1,5 +1,6 @@ import { yellow } from 'kleur/colors'; import type { APIContext, MiddlewareNext } from '../../@types/astro.js'; +import { ASTRO_ORIGIN_HEADER } from '../../core/constants.js'; import { defineMiddleware } from '../../core/middleware/index.js'; import { ACTION_QUERY_PARAMS } from '../consts.js'; import { formContentTypes, hasContentType } from './utils.js'; @@ -32,7 +33,7 @@ export const onRequest = defineMiddleware(async (context, next) => { const locals = context.locals as Locals; // Actions middleware may have run already after a path rewrite. - // See https://github.com/withastro/roadmap/blob/feat/reroute/proposals/0047-rerouting.md#ctxrewrite + // See https://github.com/withastro/roadmap/blob/main/proposals/0048-rerouting.md#ctxrewrite // `_actionPayload` is the same for every page, // so short circuit if already defined. if (locals._actionPayload) return next(); @@ -135,6 +136,11 @@ async function redirectWithResult({ return context.redirect(referer); } + const referer = context.request.headers.get(ASTRO_ORIGIN_HEADER); + if (referer) { + return context.redirect(referer); + } + return context.redirect(context.url.pathname); } diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts index 274f86797077..3fb937ac484c 100644 --- a/packages/astro/src/core/constants.ts +++ b/packages/astro/src/core/constants.ts @@ -28,6 +28,11 @@ export const REROUTE_DIRECTIVE_HEADER = 'X-Astro-Reroute'; * This metadata is used to determine the origin of a Response. If a rewrite has occurred, it should be prioritised over other logic. */ export const REWRITE_DIRECTIVE_HEADER_KEY = 'X-Astro-Rewrite'; + +/** + * Header used to track the original URL requested by the user. This information is useful rewrites are involved. + */ +export const ASTRO_ORIGIN_HEADER = 'X-Astro-Origin'; export const REWRITE_DIRECTIVE_HEADER_VALUE = 'yes'; /** diff --git a/packages/astro/src/core/middleware/sequence.ts b/packages/astro/src/core/middleware/sequence.ts index 03ae7d3a3503..1e4c92f123d6 100644 --- a/packages/astro/src/core/middleware/sequence.ts +++ b/packages/astro/src/core/middleware/sequence.ts @@ -2,6 +2,7 @@ import type { APIContext, MiddlewareHandler, RewritePayload } from '../../@types import { AstroCookies } from '../cookies/cookies.js'; import { apiContextRoutesSymbol } from '../render-context.js'; import { type Pipeline, getParams } from '../render/index.js'; +import { copyRequest } from '../routing/rewrite.js'; import { defineMiddleware } from './index.js'; // From SvelteKit: https://github.com/sveltejs/kit/blob/master/packages/kit/src/exports/hooks/sequence.js @@ -36,9 +37,9 @@ export function sequence(...handlers: MiddlewareHandler[]): MiddlewareHandler { if (payload instanceof Request) { newRequest = payload; } else if (payload instanceof URL) { - newRequest = new Request(payload, handleContext.request); + newRequest = copyRequest(payload, handleContext.request); } else { - newRequest = new Request( + newRequest = copyRequest( new URL(payload, handleContext.url.origin), handleContext.request, ); diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 8850da132e5a..c12e2748b619 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -20,6 +20,7 @@ import { import { renderEndpoint } from '../runtime/server/endpoint.js'; import { renderPage } from '../runtime/server/index.js'; import { + ASTRO_ORIGIN_HEADER, ASTRO_VERSION, REROUTE_DIRECTIVE_HEADER, REWRITE_DIRECTIVE_HEADER_KEY, @@ -36,6 +37,7 @@ import { callMiddleware } from './middleware/callMiddleware.js'; import { sequence } from './middleware/index.js'; import { renderRedirect } from './redirects/render.js'; import { type Pipeline, Slots, getParams, getProps } from './render/index.js'; +import { copyRequest } from './routing/rewrite.js'; export const apiContextRoutesSymbol = Symbol.for('context.routes'); @@ -81,6 +83,7 @@ export class RenderContext { Pick >): Promise { const pipelineMiddleware = await pipeline.getMiddleware(); + request.headers.set(ASTRO_ORIGIN_HEADER, pathname); return new RenderContext( pipeline, locals, @@ -153,7 +156,7 @@ export class RenderContext { if (payload instanceof Request) { this.request = payload; } else { - this.request = this.#copyRequest(newUrl, this.request); + this.request = copyRequest(newUrl, this.request); } this.isRewriting = true; this.url = new URL(this.request.url); @@ -253,7 +256,7 @@ export class RenderContext { if (reroutePayload instanceof Request) { this.request = reroutePayload; } else { - this.request = this.#copyRequest(newUrl, this.request); + this.request = copyRequest(newUrl, this.request); } this.url = new URL(this.request.url); this.cookies = new AstroCookies(this.request); @@ -561,33 +564,4 @@ export class RenderContext { if (!i18n) return; return (this.#preferredLocaleList ??= computePreferredLocaleList(request, i18n.locales)); } - - /** - * Utility function that creates a new `Request` with a new URL from an old `Request`. - * - * @param newUrl The new `URL` - * @param oldRequest The old `Request` - */ - #copyRequest(newUrl: URL, oldRequest: Request): Request { - if (oldRequest.bodyUsed) { - throw new AstroError(AstroErrorData.RewriteWithBodyUsed); - } - return new Request(newUrl, { - method: oldRequest.method, - headers: oldRequest.headers, - body: oldRequest.body, - referrer: oldRequest.referrer, - referrerPolicy: oldRequest.referrerPolicy, - mode: oldRequest.mode, - credentials: oldRequest.credentials, - cache: oldRequest.cache, - redirect: oldRequest.redirect, - integrity: oldRequest.integrity, - signal: oldRequest.signal, - keepalive: oldRequest.keepalive, - // https://fetch.spec.whatwg.org/#dom-request-duplex - // @ts-expect-error It isn't part of the types, but undici accepts it and it allows to carry over the body to a new request - duplex: 'half', - }); - } } diff --git a/packages/astro/src/core/routing/rewrite.ts b/packages/astro/src/core/routing/rewrite.ts index d50434f22096..9aa009e501d8 100644 --- a/packages/astro/src/core/routing/rewrite.ts +++ b/packages/astro/src/core/routing/rewrite.ts @@ -1,5 +1,6 @@ import type { AstroConfig, RewritePayload, RouteData } from '../../@types/astro.js'; import { shouldAppendForwardSlash } from '../build/util.js'; +import { AstroError, AstroErrorData } from '../errors/index.js'; import { appendForwardSlash, removeTrailingForwardSlash } from '../path.js'; import { DEFAULT_404_ROUTE } from './astro-designed-error-pages.js'; @@ -70,3 +71,32 @@ export function findRouteToRewrite({ } } } + +/** + * Utility function that creates a new `Request` with a new URL from an old `Request`. + * + * @param newUrl The new `URL` + * @param oldRequest The old `Request` + */ +export function copyRequest(newUrl: URL, oldRequest: Request): Request { + if (oldRequest.bodyUsed) { + throw new AstroError(AstroErrorData.RewriteWithBodyUsed); + } + return new Request(newUrl, { + method: oldRequest.method, + headers: oldRequest.headers, + body: oldRequest.body, + referrer: oldRequest.referrer, + referrerPolicy: oldRequest.referrerPolicy, + mode: oldRequest.mode, + credentials: oldRequest.credentials, + cache: oldRequest.cache, + redirect: oldRequest.redirect, + integrity: oldRequest.integrity, + signal: oldRequest.signal, + keepalive: oldRequest.keepalive, + // https://fetch.spec.whatwg.org/#dom-request-duplex + // @ts-expect-error It isn't part of the types, but undici accepts it and it allows to carry over the body to a new request + duplex: 'half', + }); +} From 6b1d0e8a8241f617918e68d5afb44aa2f25d284d Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 11 Oct 2024 12:11:40 +0100 Subject: [PATCH 2/4] decode header to avoid index out of bounds --- packages/astro/src/actions/runtime/middleware.ts | 3 ++- packages/astro/src/core/render-context.ts | 4 ++-- packages/astro/src/core/routing/rewrite.ts | 13 +++++++++++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/packages/astro/src/actions/runtime/middleware.ts b/packages/astro/src/actions/runtime/middleware.ts index abf84765e934..fcd42b799994 100644 --- a/packages/astro/src/actions/runtime/middleware.ts +++ b/packages/astro/src/actions/runtime/middleware.ts @@ -10,6 +10,7 @@ import { type SerializedActionResult, serializeActionResult, } from './virtual/shared.js'; +import {getOriginHeader} from "../../core/routing/rewrite.js"; export type ActionPayload = { actionResult: SerializedActionResult; @@ -136,7 +137,7 @@ async function redirectWithResult({ return context.redirect(referer); } - const referer = context.request.headers.get(ASTRO_ORIGIN_HEADER); + const referer = getOriginHeader(context.request); if (referer) { return context.redirect(referer); } diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index c12e2748b619..a79c01922672 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -37,7 +37,7 @@ import { callMiddleware } from './middleware/callMiddleware.js'; import { sequence } from './middleware/index.js'; import { renderRedirect } from './redirects/render.js'; import { type Pipeline, Slots, getParams, getProps } from './render/index.js'; -import { copyRequest } from './routing/rewrite.js'; +import {copyRequest, setOriginHeader} from './routing/rewrite.js'; export const apiContextRoutesSymbol = Symbol.for('context.routes'); @@ -83,7 +83,7 @@ export class RenderContext { Pick >): Promise { const pipelineMiddleware = await pipeline.getMiddleware(); - request.headers.set(ASTRO_ORIGIN_HEADER, pathname); + setOriginHeader(request, pathname) return new RenderContext( pipeline, locals, diff --git a/packages/astro/src/core/routing/rewrite.ts b/packages/astro/src/core/routing/rewrite.ts index 9aa009e501d8..29d84ce746aa 100644 --- a/packages/astro/src/core/routing/rewrite.ts +++ b/packages/astro/src/core/routing/rewrite.ts @@ -3,6 +3,7 @@ import { shouldAppendForwardSlash } from '../build/util.js'; import { AstroError, AstroErrorData } from '../errors/index.js'; import { appendForwardSlash, removeTrailingForwardSlash } from '../path.js'; import { DEFAULT_404_ROUTE } from './astro-designed-error-pages.js'; +import {ASTRO_ORIGIN_HEADER} from "../constants.js"; export type FindRouteToRewrite = { payload: RewritePayload; @@ -100,3 +101,15 @@ export function copyRequest(newUrl: URL, oldRequest: Request): Request { duplex: 'half', }); } + +export function setOriginHeader(request: Request, pathname: string): void { + request.headers.set(ASTRO_ORIGIN_HEADER, encodeURIComponent(pathname)); +} + +export function getOriginHeader(request: Request): string | undefined { + const origin = request.headers.get(ASTRO_ORIGIN_HEADER); + if (origin) { + return decodeURIComponent(origin) + } + return undefined +} From 6c432d5d25efb5f759cf7ba8a6842b22e45335e8 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 11 Oct 2024 13:27:53 +0100 Subject: [PATCH 3/4] fix e2e test --- packages/astro/e2e/actions-blog.test.js | 8 ++++---- .../e2e/fixtures/actions-blog/src/pages/rewritten.astro | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/astro/e2e/actions-blog.test.js b/packages/astro/e2e/actions-blog.test.js index f233c44739e6..e62a6cae2996 100644 --- a/packages/astro/e2e/actions-blog.test.js +++ b/packages/astro/e2e/actions-blog.test.js @@ -141,11 +141,11 @@ test.describe('Astro Actions - Blog', () => { astro, }) => { await page.goto(astro.resolveUrl('/sum')); - + // const submitButton = page.getByTestId('submit'); - await waitForHydrate(page, submitButton); await submitButton.click(); - await expect(page).toHaveURL(astro.resolveUrl('/sum/')); - await expect(page).toContainText('Form result: { "data": 3 }'); + await expect(page).toHaveURL(astro.resolveUrl('/sum')); + const p = page.locator('p').nth(0); + await expect(p).toContainText('Form result: {"data":3}'); }); }); diff --git a/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro b/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro index 115e6a9374f1..72eebf1bb006 100644 --- a/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro +++ b/packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro @@ -10,7 +10,7 @@ const result = Astro.getActionResult(actions.sum)
- +

Form result: {JSON.stringify(result)}

From d1fc9c29309762c300c336605840235442e50e77 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 11 Oct 2024 14:46:54 +0100 Subject: [PATCH 4/4] Update packages/astro/e2e/actions-blog.test.js Co-authored-by: Bjorn Lu --- packages/astro/e2e/actions-blog.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/astro/e2e/actions-blog.test.js b/packages/astro/e2e/actions-blog.test.js index e62a6cae2996..4b76928cfeef 100644 --- a/packages/astro/e2e/actions-blog.test.js +++ b/packages/astro/e2e/actions-blog.test.js @@ -141,7 +141,6 @@ test.describe('Astro Actions - Blog', () => { astro, }) => { await page.goto(astro.resolveUrl('/sum')); - // const submitButton = page.getByTestId('submit'); await submitButton.click(); await expect(page).toHaveURL(astro.resolveUrl('/sum'));