Fix storefrontRedirect on soft navigations

Also change redirect status code from 302 to 301
Shopify · Mar 21, 2024 · cc8ebc5 · cc8ebc5
1 parent 4a215a7
commit cc8ebc5
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 16 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,33 @@ describe('storefrontRedirect', () => {
     });
   });
 
+  it('works with 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('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
@@ -33,10 +33,21 @@ export async function storefrontRedirect(
   } = options;
 
   const {pathname, search} = new URL(request.url);
-  const redirectFrom = pathname + search;
+  const searchParams = new URLSearchParams(search);
+
+  const isSoftNavigation = searchParams.has('_data');
+
+  searchParams.delete('_data');
+  const filteredSearchParams = searchParams.toString();
+
+  const redirectFrom =
+    pathname + (filteredSearchParams ? '?' + isSoftNavigation : '');
 
   if (pathname === '/admin' && !noAdminRedirect) {
-    return redirect(`${storefront.getShopifyDomain()}/admin`);
+    return createRedirectResponse(
+      `${storefront.getShopifyDomain()}/admin`,
+      isSoftNavigation,
+    );
   }
 
   try {
@@ -49,13 +60,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 +78,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},
+    });
   }
 }