diff --git a/.changeset/hip-rocks-hammer.md b/.changeset/hip-rocks-hammer.md new file mode 100644 index 0000000000..9bd4860efd --- /dev/null +++ b/.changeset/hip-rocks-hammer.md @@ -0,0 +1,5 @@ +--- +'@shopify/hydrogen': patch +--- + +Fix bug where `storefrontRedirect` would return an error on soft page navigations. Also change the redirect status code from 301 to 302. diff --git a/packages/hydrogen/src/routing/redirect.test.ts b/packages/hydrogen/src/routing/redirect.test.ts index 8cfbe52baa..5312dc9453 100644 --- a/packages/hydrogen/src/routing/redirect.test.ts +++ b/packages/hydrogen/src/routing/redirect.test.ts @@ -36,7 +36,7 @@ describe('storefrontRedirect', () => { }), ).resolves.toEqual( new Response(null, { - status: 301, + status: 302, headers: {location: shopifyDomain + '/some-page'}, }), ); @@ -46,6 +46,60 @@ describe('storefrontRedirect', () => { }); }); + it('strips remix _data query parameter on soft navigations', async () => { + queryMock.mockResolvedValueOnce({ + urlRedirects: {edges: [{node: {target: shopifyDomain + '/some-page'}}]}, + }); + + await expect( + storefrontRedirect({ + storefront: storefrontMock, + request: new Request( + 'https://domain.com/some-page?_data=%2Fcollections%2Fbackcountry', + ), + }), + ).resolves.toEqual( + new Response(null, { + status: 200, + headers: { + 'X-Remix-Redirect': shopifyDomain + '/some-page', + 'X-Remix-Status': '302', + }, + }), + ); + + expect(queryMock).toHaveBeenCalledWith(expect.anything(), { + variables: {query: 'path:/some-page'}, + }); + }); + + it('retains custom query parameters on soft navigations', async () => { + queryMock.mockResolvedValueOnce({ + urlRedirects: {edges: [{node: {target: shopifyDomain + '/some-page'}}]}, + }); + + await expect( + storefrontRedirect({ + storefront: storefrontMock, + request: new Request( + 'https://domain.com/some-page?test=true&_data=%2Fcollections%2Fbackcountry', + ), + }), + ).resolves.toEqual( + new Response(null, { + status: 200, + headers: { + 'X-Remix-Redirect': shopifyDomain + '/some-page', + 'X-Remix-Status': '302', + }, + }), + ); + + expect(queryMock).toHaveBeenCalledWith(expect.anything(), { + variables: {query: 'path:/some-page?test=true'}, + }); + }); + it('queries the SFAPI and returns 404 on mismatch', async () => { queryMock.mockResolvedValueOnce({ urlRedirects: {edges: []}, diff --git a/packages/hydrogen/src/routing/redirect.ts b/packages/hydrogen/src/routing/redirect.ts index fb6534f862..46f5ace157 100644 --- a/packages/hydrogen/src/routing/redirect.ts +++ b/packages/hydrogen/src/routing/redirect.ts @@ -1,4 +1,3 @@ -import {redirect} from '@remix-run/server-runtime'; import type {UrlRedirectConnection} from '@shopify/hydrogen-react/storefront-api-types'; import type {I18nBase, Storefront} from '../storefront'; import {getRedirectUrl} from '../utils/get-redirect-url'; @@ -32,11 +31,18 @@ export async function storefrontRedirect( response = new Response('Not Found', {status: 404}), } = options; - const {pathname, search} = new URL(request.url); - const redirectFrom = pathname + search; + const url = new URL(request.url); + const isSoftNavigation = url.searchParams.has('_data'); - if (pathname === '/admin' && !noAdminRedirect) { - return redirect(`${storefront.getShopifyDomain()}/admin`); + url.searchParams.delete('_data'); + + const redirectFrom = url.toString().replace(url.origin, ''); + + if (url.pathname === '/admin' && !noAdminRedirect) { + return createRedirectResponse( + `${storefront.getShopifyDomain()}/admin`, + isSoftNavigation, + ); } try { @@ -49,13 +55,13 @@ export async function storefrontRedirect( const location = urlRedirects?.edges?.[0]?.node?.target; if (location) { - return new Response(null, {status: 301, headers: {location}}); + return createRedirectResponse(location, isSoftNavigation); } const redirectTo = getRedirectUrl(request.url); if (redirectTo) { - return redirect(redirectTo); + return createRedirectResponse(redirectTo, isSoftNavigation); } } catch (error) { console.error( @@ -67,17 +73,17 @@ export async function storefrontRedirect( return response; } -function isLocalPath(requestUrl: string, redirectUrl: string) { - // We don't want to redirect cross domain, - // doing so could create phishing vulnerability - // Test for protocols, e.g. https://, http://, // - // and uris: mailto:, tel:, javascript:, etc. - try { - return ( - new URL(requestUrl).origin === new URL(redirectUrl, requestUrl).origin - ); - } catch (e) { - return false; +function createRedirectResponse(location: string, isSoftNavigation: boolean) { + if (isSoftNavigation) { + return new Response(null, { + status: 200, + headers: {'X-Remix-Redirect': location, 'X-Remix-Status': '302'}, + }); + } else { + return new Response(null, { + status: 302, + headers: {location: location}, + }); } }