Skip to content

Commit 2d10de5

Browse files
ematipicobluwy
ematipico
and
bluwy
authored
fix(routing): actions should redirect the original pathname (#12173)
* fix(routing): actions should redirect the original pathname * decode header to avoid index out of bounds * fix e2e test * Update packages/astro/e2e/actions-blog.test.js Co-authored-by: Bjorn Lu <[email protected]> --------- Co-authored-by: Bjorn Lu <[email protected]>
1 parent a4ffbfa commit 2d10de5

File tree

10 files changed

+118
-34
lines changed

10 files changed

+118
-34
lines changed

‎.changeset/tough-snakes-reflect.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'astro': patch
3+
---
4+
5+
Fixes a bug where Astro Actions couldn't redirect to the correct pathname when there was a rewrite involved.

‎packages/astro/e2e/actions-blog.test.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,4 +135,16 @@ test.describe('Astro Actions - Blog', () => {
135135
await logoutButton.click();
136136
await expect(page).toHaveURL(astro.resolveUrl('/blog/'));
137137
});
138+
139+
test('Should redirect to the origin pathname when there is a rewrite', async ({
140+
page,
141+
astro,
142+
}) => {
143+
await page.goto(astro.resolveUrl('/sum'));
144+
const submitButton = page.getByTestId('submit');
145+
await submitButton.click();
146+
await expect(page).toHaveURL(astro.resolveUrl('/sum'));
147+
const p = page.locator('p').nth(0);
148+
await expect(p).toContainText('Form result: {"data":3}');
149+
});
138150
});

‎packages/astro/e2e/fixtures/actions-blog/src/actions/index.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,14 @@ export const server = {
5656
},
5757
}),
5858
},
59+
sum: defineAction({
60+
accept: "form",
61+
input: z.object({
62+
a: z.number(),
63+
b: z.number(),
64+
}),
65+
async handler({ a, b }) {
66+
return a + b
67+
},
68+
})
5969
};

‎packages/astro/e2e/fixtures/actions-blog/src/middleware.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
import { defineMiddleware } from "astro:middleware";
2+
3+
export const onRequest = defineMiddleware((ctx, next) => {
4+
if (ctx.request.method === "GET" && ctx.url.pathname === "/sum") {
5+
return next("/rewritten")
6+
}
7+
8+
return next()
9+
})

‎packages/astro/e2e/fixtures/actions-blog/src/pages/rewritten.astro

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
---
2+
import { actions } from "astro:actions";
3+
4+
const result = Astro.getActionResult(actions.sum)
5+
6+
---
7+
8+
<html>
9+
<body>
10+
<form method="post" action={actions.sum}>
11+
<input type="number" name="a" value="1" />
12+
<input type="number" name="b" value="2" />
13+
<button data-testid="submit" type="submit">Sum</button>
14+
</form>
15+
<p>Form result: {JSON.stringify(result)}</p>
16+
17+
<body>
18+
</html>

‎packages/astro/src/actions/runtime/middleware.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { yellow } from 'kleur/colors';
22
import type { APIContext, MiddlewareNext } from '../../@types/astro.js';
3+
import { ASTRO_ORIGIN_HEADER } from '../../core/constants.js';
34
import { defineMiddleware } from '../../core/middleware/index.js';
45
import { ACTION_QUERY_PARAMS } from '../consts.js';
56
import { formContentTypes, hasContentType } from './utils.js';
@@ -9,6 +10,7 @@ import {
910
type SerializedActionResult,
1011
serializeActionResult,
1112
} from './virtual/shared.js';
13+
import {getOriginHeader} from "../../core/routing/rewrite.js";
1214

1315
export type ActionPayload = {
1416
actionResult: SerializedActionResult;
@@ -32,7 +34,7 @@ export const onRequest = defineMiddleware(async (context, next) => {
3234

3335
const locals = context.locals as Locals;
3436
// Actions middleware may have run already after a path rewrite.
35-
// See https://github.com/withastro/roadmap/blob/feat/reroute/proposals/0047-rerouting.md#ctxrewrite
37+
// See https://github.com/withastro/roadmap/blob/main/proposals/0048-rerouting.md#ctxrewrite
3638
// `_actionPayload` is the same for every page,
3739
// so short circuit if already defined.
3840
if (locals._actionPayload) return next();
@@ -135,6 +137,11 @@ async function redirectWithResult({
135137
return context.redirect(referer);
136138
}
137139

140+
const referer = getOriginHeader(context.request);
141+
if (referer) {
142+
return context.redirect(referer);
143+
}
144+
138145
return context.redirect(context.url.pathname);
139146
}
140147

‎packages/astro/src/core/constants.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,11 @@ export const REROUTE_DIRECTIVE_HEADER = 'X-Astro-Reroute';
2828
* This metadata is used to determine the origin of a Response. If a rewrite has occurred, it should be prioritised over other logic.
2929
*/
3030
export const REWRITE_DIRECTIVE_HEADER_KEY = 'X-Astro-Rewrite';
31+
32+
/**
33+
* Header used to track the original URL requested by the user. This information is useful rewrites are involved.
34+
*/
35+
export const ASTRO_ORIGIN_HEADER = 'X-Astro-Origin';
3136
export const REWRITE_DIRECTIVE_HEADER_VALUE = 'yes';
3237

3338
/**

‎packages/astro/src/core/middleware/sequence.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import type { APIContext, MiddlewareHandler, RewritePayload } from '../../@types
22
import { AstroCookies } from '../cookies/cookies.js';
33
import { apiContextRoutesSymbol } from '../render-context.js';
44
import { type Pipeline, getParams } from '../render/index.js';
5+
import { copyRequest } from '../routing/rewrite.js';
56
import { defineMiddleware } from './index.js';
67

78
// 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 {
3637
if (payload instanceof Request) {
3738
newRequest = payload;
3839
} else if (payload instanceof URL) {
39-
newRequest = new Request(payload, handleContext.request);
40+
newRequest = copyRequest(payload, handleContext.request);
4041
} else {
41-
newRequest = new Request(
42+
newRequest = copyRequest(
4243
new URL(payload, handleContext.url.origin),
4344
handleContext.request,
4445
);

‎packages/astro/src/core/render-context.ts

Lines changed: 5 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
import { renderEndpoint } from '../runtime/server/endpoint.js';
2121
import { renderPage } from '../runtime/server/index.js';
2222
import {
23+
ASTRO_ORIGIN_HEADER,
2324
ASTRO_VERSION,
2425
REROUTE_DIRECTIVE_HEADER,
2526
REWRITE_DIRECTIVE_HEADER_KEY,
@@ -36,6 +37,7 @@ import { callMiddleware } from './middleware/callMiddleware.js';
3637
import { sequence } from './middleware/index.js';
3738
import { renderRedirect } from './redirects/render.js';
3839
import { type Pipeline, Slots, getParams, getProps } from './render/index.js';
40+
import {copyRequest, setOriginHeader} from './routing/rewrite.js';
3941

4042
export const apiContextRoutesSymbol = Symbol.for('context.routes');
4143

@@ -81,6 +83,7 @@ export class RenderContext {
8183
Pick<RenderContext, 'locals' | 'middleware' | 'status' | 'props'>
8284
>): Promise<RenderContext> {
8385
const pipelineMiddleware = await pipeline.getMiddleware();
86+
setOriginHeader(request, pathname)
8487
return new RenderContext(
8588
pipeline,
8689
locals,
@@ -153,7 +156,7 @@ export class RenderContext {
153156
if (payload instanceof Request) {
154157
this.request = payload;
155158
} else {
156-
this.request = this.#copyRequest(newUrl, this.request);
159+
this.request = copyRequest(newUrl, this.request);
157160
}
158161
this.isRewriting = true;
159162
this.url = new URL(this.request.url);
@@ -253,7 +256,7 @@ export class RenderContext {
253256
if (reroutePayload instanceof Request) {
254257
this.request = reroutePayload;
255258
} else {
256-
this.request = this.#copyRequest(newUrl, this.request);
259+
this.request = copyRequest(newUrl, this.request);
257260
}
258261
this.url = new URL(this.request.url);
259262
this.cookies = new AstroCookies(this.request);
@@ -561,33 +564,4 @@ export class RenderContext {
561564
if (!i18n) return;
562565
return (this.#preferredLocaleList ??= computePreferredLocaleList(request, i18n.locales));
563566
}
564-
565-
/**
566-
* Utility function that creates a new `Request` with a new URL from an old `Request`.
567-
*
568-
* @param newUrl The new `URL`
569-
* @param oldRequest The old `Request`
570-
*/
571-
#copyRequest(newUrl: URL, oldRequest: Request): Request {
572-
if (oldRequest.bodyUsed) {
573-
throw new AstroError(AstroErrorData.RewriteWithBodyUsed);
574-
}
575-
return new Request(newUrl, {
576-
method: oldRequest.method,
577-
headers: oldRequest.headers,
578-
body: oldRequest.body,
579-
referrer: oldRequest.referrer,
580-
referrerPolicy: oldRequest.referrerPolicy,
581-
mode: oldRequest.mode,
582-
credentials: oldRequest.credentials,
583-
cache: oldRequest.cache,
584-
redirect: oldRequest.redirect,
585-
integrity: oldRequest.integrity,
586-
signal: oldRequest.signal,
587-
keepalive: oldRequest.keepalive,
588-
// https://fetch.spec.whatwg.org/#dom-request-duplex
589-
// @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
590-
duplex: 'half',
591-
});
592-
}
593567
}

‎packages/astro/src/core/routing/rewrite.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
import type { AstroConfig, RewritePayload, RouteData } from '../../@types/astro.js';
22
import { shouldAppendForwardSlash } from '../build/util.js';
3+
import { AstroError, AstroErrorData } from '../errors/index.js';
34
import { appendForwardSlash, removeTrailingForwardSlash } from '../path.js';
45
import { DEFAULT_404_ROUTE } from './astro-designed-error-pages.js';
6+
import {ASTRO_ORIGIN_HEADER} from "../constants.js";
57

68
export type FindRouteToRewrite = {
79
payload: RewritePayload;
@@ -70,3 +72,44 @@ export function findRouteToRewrite({
7072
}
7173
}
7274
}
75+
76+
/**
77+
* Utility function that creates a new `Request` with a new URL from an old `Request`.
78+
*
79+
* @param newUrl The new `URL`
80+
* @param oldRequest The old `Request`
81+
*/
82+
export function copyRequest(newUrl: URL, oldRequest: Request): Request {
83+
if (oldRequest.bodyUsed) {
84+
throw new AstroError(AstroErrorData.RewriteWithBodyUsed);
85+
}
86+
return new Request(newUrl, {
87+
method: oldRequest.method,
88+
headers: oldRequest.headers,
89+
body: oldRequest.body,
90+
referrer: oldRequest.referrer,
91+
referrerPolicy: oldRequest.referrerPolicy,
92+
mode: oldRequest.mode,
93+
credentials: oldRequest.credentials,
94+
cache: oldRequest.cache,
95+
redirect: oldRequest.redirect,
96+
integrity: oldRequest.integrity,
97+
signal: oldRequest.signal,
98+
keepalive: oldRequest.keepalive,
99+
// https://fetch.spec.whatwg.org/#dom-request-duplex
100+
// @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
101+
duplex: 'half',
102+
});
103+
}
104+
105+
export function setOriginHeader(request: Request, pathname: string): void {
106+
request.headers.set(ASTRO_ORIGIN_HEADER, encodeURIComponent(pathname));
107+
}
108+
109+
export function getOriginHeader(request: Request): string | undefined {
110+
const origin = request.headers.get(ASTRO_ORIGIN_HEADER);
111+
if (origin) {
112+
return decodeURIComponent(origin)
113+
}
114+
return undefined
115+
}

0 commit comments

Comments
 (0)