From cbd31a0e610ec6003ab2e69a780af6d0d68dae01 Mon Sep 17 00:00:00 2001
From: Fran Dios <fran.dios@shopify.com>
Date: Thu, 5 Oct 2023 19:19:20 +0900
Subject: [PATCH 1/4] Add tests to the storefrontRedirect utility

---
 .../hydrogen/src/routing/redirect.test.ts     | 171 ++++++++++++++++++
 1 file changed, 171 insertions(+)
 create mode 100644 packages/hydrogen/src/routing/redirect.test.ts

diff --git a/packages/hydrogen/src/routing/redirect.test.ts b/packages/hydrogen/src/routing/redirect.test.ts
new file mode 100644
index 0000000000..fa9746b3aa
--- /dev/null
+++ 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'},
+        }),
+      );
+    });
+  });
+});

From 9e741161e6b7ac5b5364d2bdf67abcfe9ca08476 Mon Sep 17 00:00:00 2001
From: Fran Dios <fran.dios@shopify.com>
Date: Thu, 5 Oct 2023 19:21:17 +0900
Subject: [PATCH 2/4] Fix storefrontRedirect

---
 packages/hydrogen/src/routing/redirect.ts | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/packages/hydrogen/src/routing/redirect.ts b/packages/hydrogen/src/routing/redirect.ts
index 21f5e325ea..71e7e47a7d 100644
--- 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

From ef8748782944b31d0e0d654d275c2f6b1f5d0374 Mon Sep 17 00:00:00 2001
From: Fran Dios <fran.dios@shopify.com>
Date: Thu, 5 Oct 2023 19:22:09 +0900
Subject: [PATCH 3/4] Avoid absolute url redirects in templates

---
 .../demo-store/app/routes/($locale).discount.$code.tsx     | 7 ++++++-
 templates/skeleton/app/routes/discount.$code.tsx           | 7 ++++++-
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/templates/demo-store/app/routes/($locale).discount.$code.tsx b/templates/demo-store/app/routes/($locale).discount.$code.tsx
index 3c5b2d4811..7bc423abeb 100644
--- 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
index 94698ad41f..7b2df06c64 100644
--- 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');
 

From 9b33d114fdaa80fee00e68ca52cc8b78e3e79153 Mon Sep 17 00:00:00 2001
From: Fran Dios <fran.dios@shopify.com>
Date: Thu, 5 Oct 2023 19:31:56 +0900
Subject: [PATCH 4/4] Changesets

---
 .changeset/twelve-pens-hope.md | 5 +++++
 .changeset/wise-adults-sin.md  | 5 +++++
 2 files changed, 10 insertions(+)
 create mode 100644 .changeset/twelve-pens-hope.md
 create mode 100644 .changeset/wise-adults-sin.md

diff --git a/.changeset/twelve-pens-hope.md b/.changeset/twelve-pens-hope.md
new file mode 100644
index 0000000000..03d5d88195
--- /dev/null
+++ 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
new file mode 100644
index 0000000000..7c4993cd90
--- /dev/null
+++ b/.changeset/wise-adults-sin.md
@@ -0,0 +1,5 @@
+---
+'@shopify/hydrogen': patch
+---
+
+Ensure `storefrontRedirect` fallback only redirects to relative URLs.