Skip to content

Commit 957026d

Browse files
Brooooooklynclaude
andauthored
fix(pages): keep the named action key first when a redirect repeats the request (#533)
Follow-up to #530 (Codex finding on that PR, which got merged before this fix landed). ## Problem A redirect that sends the same request again keeps the action key (307/308, or 301/302 after PUT/PATCH/DELETE). When the destination has its own query, the merge put the key **after** the destination's params: ``` PUT /books?delete&page=2 → c.redirect('/books-v2?tab=details', 302) → /books-v2?tab=details&delete=&page=2 repeated PUT picks "tab" ✗ ``` ## Fix (only in the `c.redirect` wrapper) | Destination | Result | |---|---| | `/books-v2?tab=details` (another page) | `/books-v2?delete&tab=details&page=2` (key first, as sent) | | `/books-v2?tab=1&delete=x` (sets the key too) | `/books-v2?delete=x&tab=1&page=2` (its value wins, moves first) | | `/books?archive` (same page, own first key) | `/books?archive=&page=2` (that key picks the action) | | `/books-v2?` (bare reset) | `/books-v2?delete` (rest dropped, key kept) | | `/books-v2` (no query) | `/books-v2?delete&page=2` (unchanged, byte for byte) | | `#fragment` | stays last | Unchanged: GET follow-ups (key dropped), unmarked requests, `mergeQueryIntoLocation`, redirect rules and platform dispatch. Known limit: a redirect to a **different** page whose first query key is one of that page's actions (`c.redirect('/other?archive', 307)`) still puts the request's key first. The redirect code can't tell an action key from a normal param on another page. ## Tests `routing-runtime.test.ts`: 10 new rows in the named-action table (307 and PUT 302 with a destination query, a valued key, a fragment, a bare `?`, the same key in the destination, a same-page action key). 9 fail on main. The POST 302 row passes on main too: it checks the GET follow-up stays unchanged. routing-runtime, pages-client, pages-compile and compile tests: 205/205. Lint, format and typecheck are clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
1 parent 43b58a9 commit 957026d

4 files changed

Lines changed: 109 additions & 8 deletions

File tree

‎packages/void/src/pages/action-key.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,16 @@ export function hasActionKey(request: Request): boolean {
1111
return actionKeyRequests.has(request);
1212
}
1313

14+
/** The first non-empty segment of `search` as sent: `delete` in `?delete&page=2`. */
15+
export function actionKeySegment(search: string): string {
16+
let start = 1;
17+
while (search[start] === '&') {
18+
start++;
19+
}
20+
const end = search.indexOf('&', start);
21+
return search.slice(start, end === -1 ? search.length : end);
22+
}
23+
1424
/** `search` without its first non-empty segment; the rest is kept as sent. */
1525
export function searchWithoutActionKey(search: string): string {
1626
let start = 1;

‎packages/void/src/runtime/routing.ts‎

Lines changed: 61 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,12 @@ import {
88
} from '@void/edge';
99
import type { Context, Hono, MiddlewareHandler } from 'hono';
1010
import type { RedirectStatusCode } from 'hono/utils/http-status';
11-
import { hasActionKey, markActionKey, searchWithoutActionKey } from '../pages/action-key.ts';
11+
import {
12+
actionKeySegment,
13+
hasActionKey,
14+
markActionKey,
15+
searchWithoutActionKey,
16+
} from '../pages/action-key.ts';
1217
import { VoidAssetRewriteError } from './handler.ts';
1318
import { convertReturnValue } from './response.ts';
1419
import { ASSET_EXTENSIONS } from './routing-assets.ts';
@@ -179,10 +184,7 @@ export function createRoutingRuntime(
179184
const redirect = c.redirect.bind(c);
180185
c.redirect = (location, status) => {
181186
if (typeof location === 'string' && location.startsWith('/') && !location.startsWith('//')) {
182-
return redirect(
183-
mergeQueryIntoLocation(location, redirectSearch(c.req.raw, status)),
184-
status,
185-
);
187+
return redirect(redirectLocation(c.req.raw, location, status), status);
186188
}
187189
return redirect(location, status);
188190
};
@@ -252,15 +254,66 @@ export function createRoutingRuntime(
252254
};
253255
}
254256

255-
function redirectSearch(request: Request, status: RedirectStatusCode | undefined): string {
256-
const search = new URL(request.url).search;
257+
function redirectLocation(
258+
request: Request,
259+
location: string,
260+
status: RedirectStatusCode | undefined,
261+
): string {
262+
const url = new URL(request.url);
263+
const search = url.search;
264+
if (!hasActionKey(request)) {
265+
return mergeQueryIntoLocation(location, search);
266+
}
257267
// The key goes only when the browser follows with a GET: after a 303, or after
258268
// a 301/302 (the default) to a POST. Other redirects send the same method and
259269
// body again, and the destination needs the key to pick the action.
260270
const followsWithGet =
261271
status === 303 ||
262272
((status === undefined || status === 301 || status === 302) && request.method === 'POST');
263-
return hasActionKey(request) && followsWithGet ? searchWithoutActionKey(search) : search;
273+
if (followsWithGet) {
274+
return mergeQueryIntoLocation(location, searchWithoutActionKey(search));
275+
}
276+
const hash = location.indexOf('#');
277+
const destinationQuery = location.indexOf('?');
278+
if (destinationQuery === -1 || (hash !== -1 && hash < destinationQuery)) {
279+
return mergeQueryIntoLocation(location, search);
280+
}
281+
const merged = mergeQueryIntoLocation(location, searchWithoutActionKey(search));
282+
// On the same page, a key the destination starts with picks the action itself.
283+
const destinationKey = actionKeySegment(
284+
location.slice(destinationQuery, hash === -1 ? location.length : hash),
285+
);
286+
if (destinationKey && new URL(location, url).pathname === url.pathname) {
287+
return merged;
288+
}
289+
// Elsewhere the key goes first again, ahead of the destination parameters; a
290+
// value the destination gives the same key wins. A bare "?" still drops the
291+
// rest of the request query.
292+
const mergedHash = merged.indexOf('#');
293+
const base = mergedHash === -1 ? merged : merged.slice(0, mergedHash);
294+
const fragment = mergedHash === -1 ? '' : merged.slice(mergedHash);
295+
const q = base.indexOf('?');
296+
const segments = q === -1 ? [] : base.slice(q + 1).split('&');
297+
const key = actionKeySegment(search);
298+
const name = queryKey(key);
299+
const hasName = (segment: string) => segment !== '' && queryKey(segment) === name;
300+
// Only the destination's own value replaces the key. The merge then left out
301+
// the request's values for that name and put the destination's first.
302+
const own = location
303+
.slice(destinationQuery + 1, hash === -1 ? location.length : hash)
304+
.split('&')
305+
.some(hasName)
306+
? segments.findIndex(hasName)
307+
: -1;
308+
const first = own === -1 ? key : segments.splice(own, 1)[0];
309+
const rest = segments.join('&');
310+
return (
311+
base.slice(0, q === -1 ? base.length : q) + '?' + first + (rest ? '&' + rest : '') + fragment
312+
);
313+
}
314+
315+
function queryKey(segment: string): string | undefined {
316+
return new URLSearchParams(segment).keys().next().value;
264317
}
265318

266319
/** A rewrite that keeps the query keeps the action key in it too. */

‎packages/void/test/unit/routing-runtime.test.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,14 @@ describe('typed Pages and SSR execution', () => {
511511
delete: (c: Context) => c.redirect('/named'),
512512
update: (c: Context) => c.redirect('/named', 303),
513513
move: (c: Context) => c.redirect('/named-v2', 307),
514+
moveTab: (c: Context) => c.redirect('/named-v2?tab=details', 307),
515+
moveHash: (c: Context) => c.redirect('/named-v2?tab=details#top', 307),
516+
moveReset: (c: Context) => c.redirect('/named-v2?', 307),
517+
replace: (c: Context) => c.redirect('/named-v2?tab=details', 302),
518+
moveKey: (c: Context) => c.redirect('/named-v2?tab=1&moveKey=x', 307),
519+
moveSame: (c: Context) => c.redirect('/named-v2?moveSame', 307),
520+
toArchive: (c: Context) => c.redirect('/named?archive', 307),
521+
toArchiveHere: (c: Context) => c.redirect('/named?', 307),
514522
go: (c: Context) => c.rewrite('/api/x'),
515523
goWithQuery: (c: Context) => c.rewrite('/api/x?q=1'),
516524
save: () => undefined,
@@ -525,6 +533,32 @@ describe('typed Pages and SSR execution', () => {
525533
it.each([
526534
['POST', '/named?delete&page=2', 'middleware', '/login?page=2'],
527535
['POST', '/named?move&page=2', 'a 307 redirect', '/named-v2?move&page=2'],
536+
['POST', '/named?moveTab&page=2', 'a 307 redirect', '/named-v2?moveTab&tab=details&page=2'],
537+
[
538+
'POST',
539+
'/named?moveTab&moveTab=secondary&page=2',
540+
'a 307 redirect',
541+
'/named-v2?moveTab&tab=details&moveTab=secondary&page=2',
542+
],
543+
['PUT', '/named?replace&page=2', 'a 302 redirect', '/named-v2?replace&tab=details&page=2'],
544+
[
545+
'PUT',
546+
'/named?&replace=1&page=2',
547+
'a 302 redirect',
548+
'/named-v2?replace=1&tab=details&page=2',
549+
],
550+
['POST', '/named?replace&page=2', 'a 302 redirect', '/named-v2?tab=details&page=2'],
551+
[
552+
'POST',
553+
'/named?moveHash&page=2',
554+
'a 307 redirect',
555+
'/named-v2?moveHash&tab=details&page=2#top',
556+
],
557+
['POST', '/named?moveReset&page=2', 'a 307 redirect', '/named-v2?moveReset'],
558+
['POST', '/named?moveKey&page=2', 'a 307 redirect', '/named-v2?moveKey=x&tab=1&page=2'],
559+
['POST', '/named?moveSame', 'a 307 redirect', '/named-v2?moveSame'],
560+
['PUT', '/named?toArchive&page=2', 'a 307 redirect', '/named?archive=&page=2'],
561+
['PUT', '/named?toArchiveHere&page=2', 'a 307 redirect', '/named?toArchiveHere'],
528562
['PUT', '/named?delete&page=2', 'a 302 redirect', '/named?delete&page=2'],
529563
['PUT', '/named?update&page=2', 'a 303 redirect', '/named?page=2'],
530564
['POST', '/named?go&page=2', 'a rewrite', '/done?page=2'],

‎platform/packages/api/test/integration/email-admission.test.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,6 +345,8 @@ describe('authoritative email admission', () => {
345345
payloadDigest: 'f'.repeat(64),
346346
route: receiptRoute(),
347347
});
348+
// It can fail before the assertion below listens; that is not an unhandled rejection.
349+
receive.catch(() => {});
348350
try {
349351
await waitForPostgresLockWaiter();
350352
releaseDisable();
@@ -1185,6 +1187,8 @@ describe('authoritative email admission', () => {
11851187
payloadDigest: 'd'.repeat(64),
11861188
route: receiptRoute(),
11871189
});
1190+
// It can fail before the assertion below listens; that is not an unhandled rejection.
1191+
start.catch(() => {});
11881192
try {
11891193
await waitForPostgresLockWaiter();
11901194
releaseSuspension();

0 commit comments

Comments
 (0)