Merge pull request #4936 from Shopify/jm/clean-password-redirect

[Themes] - Improve storefront password detection for password-protected shops with redirects

.changeset/pretty-donkeys-dream.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Improve storefront password detection for password-protected shops with redirects

packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts

@@ -14,9 +14,7 @@ describe('Storefront API', () => {
   describe('isStorefrontPasswordProtected', () => {
     test('returns true when the request is redirected to the password page', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(
-        response({status: 302, headers: {location: 'https://store.myshopify.com/password'}}),
-      )
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.com/password'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
@@ -25,13 +23,12 @@ describe('Storefront API', () => {
       expect(isProtected).toBe(true)
       expect(fetch).toBeCalledWith('https://store.myshopify.com', {
         method: 'GET',
-        redirect: 'manual',
       })
     })
 
     test('returns false when request is not redirected', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(response({status: 200}))
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.com'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
@@ -40,13 +37,12 @@ describe('Storefront API', () => {
       expect(isProtected).toBe(false)
       expect(fetch).toBeCalledWith('https://store.myshopify.com', {
         method: 'GET',
-        redirect: 'manual',
       })
     })
 
     test('returns false when store redirects to a different domain', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(response({status: 302, headers: {location: 'https://store.myshopify.se'}}))
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.se'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
@@ -57,9 +53,7 @@ describe('Storefront API', () => {
 
     test('returns false when store redirects to a different URI', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(
-        response({status: 302, headers: {location: 'https://store.myshopify.com/random'}}),
-      )
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.com/random'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
@@ -70,9 +64,7 @@ describe('Storefront API', () => {
 
     test('return true when store redirects to /<locale>/password', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(
-        response({status: 302, headers: {location: 'https://store.myshopify.com/fr-CA/password'}}),
-      )
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.com/fr-CA/password'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
@@ -83,16 +75,29 @@ describe('Storefront API', () => {
 
     test('returns false if response is not a 302', async () => {
       // Given
-      vi.mocked(fetch).mockResolvedValue(
-        response({status: 301, headers: {location: 'https://store.myshopify.com/random'}}),
-      )
+      vi.mocked(fetch).mockResolvedValue(response({status: 200, url: 'https://store.myshopify.com/random'}))
 
       // When
       const isProtected = await isStorefrontPasswordProtected('store.myshopify.com')
 
       // Then
       expect(isProtected).toBe(false)
     })
+
+    test('ignores query params', async () => {
+      // Given
+      vi.mocked(fetch)
+        .mockResolvedValueOnce(response({status: 200, url: 'https://store.myshopify.com/random?a=b'}))
+        .mockResolvedValueOnce(response({status: 200, url: 'https://store.myshopify.com/password?a=b'}))
+
+      // When
+      const redirectToRandomPath = await isStorefrontPasswordProtected('store.myshopify.com')
+      const redirectToPasswordPath = await isStorefrontPasswordProtected('store.myshopify.com')
+
+      // Then
+      expect(redirectToRandomPath).toBe(false)
+      expect(redirectToPasswordPath).toBe(true)
+    })
   })
 
   describe('getStorefrontSessionCookies', () => {
@@ -195,7 +200,12 @@ describe('Storefront API', () => {
 
   // Tests rely on this function because the 'packages/theme' package cannot
   // directly access node-fetch and they use: new Response('OK', {status: 200})
-  function response(mock: {status: number; headers?: {[key: string]: string}; text?: () => Promise<string>}) {
+  function response(mock: {
+    status: number
+    url?: string
+    headers?: {[key: string]: string}
+    text?: () => Promise<string>
+  }) {
     const setCookieHeader = (mock.headers ?? {})['set-cookie'] ?? ''
     const setCookieArray = [setCookieHeader]
 

packages/theme/src/cli/utilities/theme-environment/storefront-session.ts

@@ -9,12 +9,10 @@ export class ShopifyEssentialError extends Error {}
 export async function isStorefrontPasswordProtected(storeURL: string): Promise<boolean> {
   const response = await fetch(prependHttps(storeURL), {
     method: 'GET',
-    redirect: 'manual',
   })
 
-  if (response.status !== 302) return false
-
-  return response.headers.get('location')?.endsWith('/password') ?? false
+  const redirectLocation = new URL(response.url)
+  return redirectLocation.pathname.endsWith('/password')
 }
 
 /**