Fix storefrontRedirect and discount redirects (#1399)

* Add tests to the storefrontRedirect utility * Fix storefrontRedirect * Avoid absolute url redirects in templates * Changesets
Shopify · Oct 10, 2023 · 4156d16 · 4156d16
1 parent cf5fe84
commit 4156d16
Show file tree

Hide file tree

Showing 6 changed files with 198 additions and 14 deletions.
diff --git a/.changeset/twelve-pens-hope.md b/.changeset/twelve-pens-hope.md
@@ -0,0 +1,5 @@
+---
+'demo-store': patch
+---
+
+Ensure that the `/discount?redirect=...` route only redirects to relative URLs.
diff --git a/.changeset/wise-adults-sin.md b/.changeset/wise-adults-sin.md
@@ -0,0 +1,5 @@
+---
+'@shopify/hydrogen': patch
+---
+
+Ensure `storefrontRedirect` fallback only redirects to relative URLs.
diff --git a/packages/hydrogen/src/routing/redirect.test.ts b/packages/hydrogen/src/routing/redirect.test.ts
@@ -0,0 +1,171 @@
+import {describe, it, expect, vi} from 'vitest';
+import type {Storefront} from '../storefront';
+import {storefrontRedirect} from './redirect';
+
+describe('storefrontRedirect', () => {
+  const shopifyDomain = 'https://domain.myshopify.com';
+  const queryMock = vi.fn();
+  const storefrontMock = {
+    getShopifyDomain: () => shopifyDomain,
+    query: queryMock,
+  } as unknown as Storefront;
+
+  it('redirects to Shopify admin ', async () => {
+    await expect(
+      storefrontRedirect({
+        storefront: storefrontMock,
+        request: new Request('https://domain.com/admin'),
+      }),
+    ).resolves.toEqual(
+      new Response(null, {
+        status: 302,
+        headers: {location: shopifyDomain + '/admin'},
+      }),
+    );
+  });
+
+  it('queries the SFAPI and redirects on match', async () => {
+    queryMock.mockResolvedValueOnce({
+      urlRedirects: {edges: [{node: {target: shopifyDomain + '/some-page'}}]},
+    });
+
+    await expect(
+      storefrontRedirect({
+        storefront: storefrontMock,
+        request: new Request('https://domain.com/some-page'),
+      }),
+    ).resolves.toEqual(
+      new Response(null, {
+        status: 301,
+        headers: {location: shopifyDomain + '/some-page'},
+      }),
+    );
+
+    expect(queryMock).toHaveBeenCalledWith(expect.anything(), {
+      variables: {query: 'path:/some-page'},
+    });
+  });
+
+  it('queries the SFAPI and returns 404 on mismatch', async () => {
+    queryMock.mockResolvedValueOnce({
+      urlRedirects: {edges: []},
+    });
+
+    const response = new Response('Not Found', {status: 404});
+
+    await expect(
+      storefrontRedirect({
+        response,
+        storefront: storefrontMock,
+        request: new Request('https://domain.com/some-page'),
+      }),
+    ).resolves.toEqual(response);
+
+    expect(queryMock).toHaveBeenCalledWith(expect.anything(), {
+      variables: {query: 'path:/some-page'},
+    });
+  });
+
+  it('queries the SFAPI and returns 404 on mismatch', async () => {
+    queryMock.mockResolvedValueOnce({urlRedirects: {edges: []}});
+    const response = new Response('Not Found', {status: 404});
+
+    await expect(
+      storefrontRedirect({
+        response,
+        storefront: storefrontMock,
+        request: new Request('https://domain.com/some-page'),
+      }),
+    ).resolves.toEqual(response);
+
+    expect(queryMock).toHaveBeenCalledWith(expect.anything(), {
+      variables: {query: 'path:/some-page'},
+    });
+  });
+
+  describe('redirect fallback with search params', () => {
+    it('uses the "redirect" serach param as a fallback', async () => {
+      queryMock.mockResolvedValueOnce({urlRedirects: {edges: []}});
+      const response = new Response('Not Found', {status: 404});
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?redirect=/some-page',
+          ),
+        }),
+      ).resolves.toEqual(
+        new Response(null, {status: 302, headers: {location: '/some-page'}}),
+      );
+    });
+
+    it('uses the "return_to" serach param as a fallback', async () => {
+      queryMock.mockResolvedValueOnce({urlRedirects: {edges: []}});
+      const response = new Response('Not Found', {status: 404});
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?return_to=/some-page',
+          ),
+        }),
+      ).resolves.toEqual(
+        new Response(null, {status: 302, headers: {location: '/some-page'}}),
+      );
+    });
+
+    it('does not redirect to absolute URLs', async () => {
+      queryMock.mockResolvedValue({urlRedirects: {edges: []}});
+      const response = new Response('Not Found', {status: 404});
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?redirect=https://some-page.com',
+          ),
+        }),
+      ).resolves.toEqual(response);
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?redirect=//some-page.com',
+          ),
+        }),
+      ).resolves.toEqual(response);
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?redirect=javascript:alert(1)',
+          ),
+        }),
+      ).resolves.toEqual(response);
+
+      await expect(
+        storefrontRedirect({
+          response,
+          storefront: storefrontMock,
+          request: new Request(
+            'https://domain.com/missing?redirect=/some-page?param=https://another.com',
+          ),
+        }),
+      ).resolves.toEqual(
+        new Response(null, {
+          status: 302,
+          headers: {location: '/some-page?param=https://another.com'},
+        }),
+      );
+    });
+  });
+});
diff --git a/packages/hydrogen/src/routing/redirect.ts b/packages/hydrogen/src/routing/redirect.ts
@@ -75,18 +75,11 @@ export async function storefrontRedirect(
 }
 
 function isLocalPath(url: string) {
-  try {
-    // We don't want to redirect cross domain,
-    // doing so could create fishing vulnerability
-    // If `new URL()` succeeds, it's a fully qualified
-    // url which is cross domain. If it fails, it's just
-    // a path, which will be the current domain.
-    new URL(url);
-  } catch (e) {
-    return true;
-  }
-
-  return false;
+  // 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.
+  return !/^(([a-z+-]+:)?\/\/|[a-z+-]+:)/i.test(url.trim());
 }
 
 const REDIRECT_QUERY = `#graphql

diff --git a/templates/demo-store/app/routes/($locale).discount.$code.tsx b/templates/demo-store/app/routes/($locale).discount.$code.tsx
@@ -20,9 +20,14 @@ export async function loader({request, context, params}: LoaderArgs) {
 
   const url = new URL(request.url);
   const searchParams = new URLSearchParams(url.search);
-  const redirectParam =
+  let redirectParam =
     searchParams.get('redirect') || searchParams.get('return_to') || '/';
 
+  if (redirectParam.includes('//')) {
+    // Avoid redirecting to external URLs to prevent phishing attacks
+    redirectParam = '/';
+  }
+
   searchParams.delete('redirect');
   searchParams.delete('return_to');
 

diff --git a/templates/skeleton/app/routes/discount.$code.tsx b/templates/skeleton/app/routes/discount.$code.tsx
@@ -17,9 +17,14 @@ export async function loader({request, context, params}: LoaderArgs) {
 
   const url = new URL(request.url);
   const searchParams = new URLSearchParams(url.search);
-  const redirectParam =
+  let redirectParam =
     searchParams.get('redirect') || searchParams.get('return_to') || '/';
 
+  if (redirectParam.includes('//')) {
+    // Avoid redirecting to external URLs to prevent phishing attacks
+    redirectParam = '/';
+  }
+
   searchParams.delete('redirect');
   searchParams.delete('return_to');