From 99172cc3e04b5370ba291cdbf5d2196fd7448929 Mon Sep 17 00:00:00 2001 From: Miro Yovchev <2827783+myovchev@users.noreply.github.com> Date: Fri, 3 Jul 2026 16:52:24 +0300 Subject: [PATCH] Fix astro redirects --- .changeset/astro-trailing-slash-query.md | 5 + .../apostrophe-astro/lib/aposPageFetch.js | 20 +- .../test/lib/aposPageFetch.test.js | 189 ++++++++++++++++++ 3 files changed, 210 insertions(+), 4 deletions(-) create mode 100644 .changeset/astro-trailing-slash-query.md create mode 100644 packages/apostrophe-astro/test/lib/aposPageFetch.test.js diff --git a/.changeset/astro-trailing-slash-query.md b/.changeset/astro-trailing-slash-query.md new file mode 100644 index 0000000000..cb53a636e3 --- /dev/null +++ b/.changeset/astro-trailing-slash-query.md @@ -0,0 +1,5 @@ +--- +"@apostrophecms/apostrophe-astro": patch +--- + +Query string parameters are no longer lost when a URL with a trailing slash is normalized, so `/articles/?page=2` now renders the same content as `/articles?page=2`. Previously such URLs were redirected to the page URL alone (e.g. `/articles`), losing the query string and showing the first page. Redirects to a different origin are now always passed through to the browser. diff --git a/packages/apostrophe-astro/lib/aposPageFetch.js b/packages/apostrophe-astro/lib/aposPageFetch.js index 5f0fb540ce..7e27ff59de 100644 --- a/packages/apostrophe-astro/lib/aposPageFetch.js +++ b/packages/apostrophe-astro/lib/aposPageFetch.js @@ -56,15 +56,27 @@ export async function aposPageFetch(req) { // the same terms, then re-add it when constructing the retry URL. if (aposData.redirect && aposData.url !== '/') { const prefix = config.aposPrefix || ''; - let from = new URL(request.url).pathname.replace(/\/+$/, ''); + const requestUrl = new URL(request.url); + let from = requestUrl.pathname.replace(/\/+$/, ''); if (prefix && from.startsWith(prefix + '/')) { from = from.slice(prefix.length); } else if (prefix && from === prefix) { from = '/'; } - const to = (aposData.url || '').replace(/\/+$/, ''); - if (from === to) { - const retryUrl = prefix + aposData.url; + // Parse the redirect target so the trailing-slash comparison + // sees only the path, even if the URL carries a query string. + // Absolute redirects to other hosts never qualify for an + // internal retry — they must reach the browser. + const target = new URL(aposData.url || '', requestUrl); + const to = target.pathname.replace(/\/+$/, ''); + if (target.origin === requestUrl.origin && from === to) { + // Preserve the query string across the internal retry: the + // target's own if present, otherwise the original request's. + // Apostrophe's trailing-slash redirect drops the query string + // (e.g. /articles/?page=2 redirects to /articles), so without + // this the retry would render page 1. + const search = target.search || requestUrl.search; + const retryUrl = prefix + target.pathname + search; const retry = new Request(new URL(retryUrl, request.url), request); const retryResponse = await aposResponse(retry); headers = retryResponse.headers; diff --git a/packages/apostrophe-astro/test/lib/aposPageFetch.test.js b/packages/apostrophe-astro/test/lib/aposPageFetch.test.js new file mode 100644 index 0000000000..6a340b58dd --- /dev/null +++ b/packages/apostrophe-astro/test/lib/aposPageFetch.test.js @@ -0,0 +1,189 @@ +import assert from 'node:assert/strict'; +import esmock from 'esmock'; + +const mockConfig = { + aposHost: 'http://localhost:3000', + aposPrefix: '', + staticBuild: null, + excludeRequestHeaders: [] +}; + +function jsonResponse(data) { + return new Response(JSON.stringify(data), { + headers: { 'content-type': 'application/json' } + }); +} + +// Returns { aposPageFetch, calls } where calls records the URL of every +// request passed to the mocked aposResponse, and responses are served +// from the given queue in order. +async function load(responses, configOverrides = {}) { + const calls = []; + const queue = [ ...responses ]; + const mod = await esmock('../../lib/aposPageFetch.js', { + 'apostrophe-astro-config/config': { + default: { ...mockConfig, ...configOverrides } + }, + '../../lib/aposResponse.js': { + default: async (request) => { + calls.push(request.url); + if (!queue.length) { + throw new Error('aposResponse called more times than expected'); + } + const next = queue.shift(); + return typeof next === 'function' ? next() : jsonResponse(next); + } + } + }); + return { + aposPageFetch: mod.default, + calls + }; +} + +describe('aposPageFetch redirect handling', () => { + beforeEach(() => { + process.env.APOS_EXTERNAL_FRONT_KEY = 'test-secret-key'; + }); + + afterEach(() => { + delete process.env.APOS_EXTERNAL_FRONT_KEY; + delete process.env.APOS_ASTRO_STATIC_BUILD; + }); + + it('preserves the query string when following a trailing-slash redirect', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/articles', + status: 302 + }, + { + template: 'article-page:index', + currentPage: 2 + } + ]); + const data = await aposPageFetch( + new Request('http://localhost:4321/articles/?page=2') + ); + assert.equal(calls.length, 2); + assert.equal(calls[1], 'http://localhost:4321/articles?page=2'); + assert.equal(data.currentPage, 2); + assert.ok(!data.redirect); + }); + + it('still follows a slash-adding locale home redirect internally', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/fr/', + status: 302 + }, + { + template: '@apostrophecms/home-page:page' + } + ]); + const data = await aposPageFetch(new Request('http://localhost:4321/fr')); + assert.equal(calls.length, 2); + assert.equal(calls[1], 'http://localhost:4321/fr/'); + assert.ok(!data.redirect); + }); + + it('prefers the redirect target\'s own query string over the request\'s', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/articles?category=tech', + status: 302 + }, + { + template: 'article-page:index' + } + ]); + await aposPageFetch( + new Request('http://localhost:4321/articles/?page=2') + ); + assert.equal(calls.length, 2); + assert.equal(calls[1], 'http://localhost:4321/articles?category=tech'); + }); + + it('passes through a redirect to a different path without retrying', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/new-location', + status: 301 + } + ]); + const data = await aposPageFetch(new Request('http://localhost:4321/old-location')); + assert.equal(calls.length, 1); + assert.equal(data.redirect, true); + assert.equal(data.url, '/new-location'); + assert.equal(data.status, 301); + }); + + it('passes through an absolute redirect to another host even when paths match', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: 'https://other.example.com/articles', + status: 302 + } + ]); + const data = await aposPageFetch(new Request('http://localhost:4321/articles/')); + assert.equal(calls.length, 1); + assert.equal(data.redirect, true); + assert.equal(data.url, 'https://other.example.com/articles'); + }); + + it('strips and restores the prefix around the internal retry', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/articles', + status: 302 + }, + { + template: 'article-page:index' + } + ], { aposPrefix: '/base' }); + await aposPageFetch( + new Request('http://localhost:4321/base/articles/?page=2') + ); + assert.equal(calls.length, 2); + assert.equal(calls[1], 'http://localhost:4321/base/articles?page=2'); + }); + + it('reports an infinite redirect instead of looping', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/articles', + status: 302 + }, + { + redirect: true, + url: '/articles', + status: 302 + } + ]); + const data = await aposPageFetch(new Request('http://localhost:4321/articles/')); + assert.equal(calls.length, 2); + assert.ok(data.errorFetchingPage); + assert.match(data.errorFetchingPage.message, /Infinite redirect/); + assert.equal(data.page.type, 'apos-fetch-error'); + }); + + it('never retries a redirect to the site root', async () => { + const { aposPageFetch, calls } = await load([ + { + redirect: true, + url: '/', + status: 302 + } + ]); + const data = await aposPageFetch(new Request('http://localhost:4321//')); + assert.equal(calls.length, 1); + assert.equal(data.redirect, true); + }); +});