Fix storefrontRedirect on soft navigations (#1880)

* Fix `storefrontRedirect` on soft navigations Also change redirect status code from 301 to 302 * Add test and PR feedback
Shopify · Mar 22, 2024 · 8707295 · 8707295
1 parent ca1dcbb
commit 8707295
Show file tree

Hide file tree

Showing 3 changed files with 84 additions and 19 deletions.
diff --git a/.changeset/hip-rocks-hammer.md 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
@@ -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
@@ -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},
+    });
   }
 }