Merge pull request #8989 from Shopify/theme-profile-redirect-error

Fix theme profile for URLs with a query string and report redirects

Author: isaacroldan

.changeset/theme-profile-redirect.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Fix `theme profile` failing with "Bad response: 301" when `--url` includes a query string, and show an actionable error when the page redirects.

docs-shopify.dev/generated/generated_docs_data_v2.json

@@ -9966,7 +9966,7 @@
           "syntaxKind": "PropertySignature",
           "name": "--url <value>",
           "value": "string",
-          "description": "The url to be used as context",
+          "description": "The path of the page to profile, with an optional query string. For a full URL, only the path and query string are used.",
           "isOptional": true,
           "environmentValue": "SHOPIFY_FLAG_URL"
         },
@@ -10016,7 +10016,7 @@
           "environmentValue": "SHOPIFY_FLAG_THEME_ID"
         }
       ],
-      "value": "export interface themeprofile {\n  /**\n   * Alias of the Shopify account to use for authentication.\n   * @environment SHOPIFY_FLAG_AUTH_ALIAS\n   */\n  '--auth-alias <value>'?: string\n\n  /**\n   * The environment to apply to the current command.\n   * @environment SHOPIFY_FLAG_ENVIRONMENT\n   */\n  '-e, --environment <value>'?: string\n\n  /**\n   * Output the result as JSON. Automatically disables color output.\n   * @environment SHOPIFY_FLAG_JSON\n   */\n  '-j, --json'?: ''\n\n  /**\n   * Print the command's JSON schemas.\n   * @environment SHOPIFY_FLAG_JSON_SCHEMA\n   */\n  '--json-schema'?: ''\n\n  /**\n   * Disable color output.\n   * @environment SHOPIFY_FLAG_NO_COLOR\n   */\n  '--no-color'?: ''\n\n  /**\n   * Disable interactive prompts and browser authentication.\n   * @environment SHOPIFY_FLAG_NO_INPUT\n   */\n  '--no-input'?: ''\n\n  /**\n   * Password generated from the Theme Access app or an Admin API token.\n   * @environment SHOPIFY_CLI_THEME_TOKEN\n   */\n  '--password <value>'?: string\n\n  /**\n   * The path where you want to run the command. Defaults to the current working directory.\n   * @environment SHOPIFY_FLAG_PATH\n   */\n  '--path <value>'?: string\n\n  /**\n   * Store URL. It can be the store prefix (example) or the full myshopify.com URL (example.myshopify.com, https://example.myshopify.com).\n   * @environment SHOPIFY_FLAG_STORE\n   */\n  '-s, --store <value>'?: string\n\n  /**\n   * The password for storefronts with password protection.\n   * @environment SHOPIFY_FLAG_STORE_PASSWORD\n   */\n  '--store-password <value>'?: string\n\n  /**\n   * Theme ID or name of the remote theme.\n   * @environment SHOPIFY_FLAG_THEME_ID\n   */\n  '-t, --theme <value>'?: string\n\n  /**\n   * The url to be used as context\n   * @environment SHOPIFY_FLAG_URL\n   */\n  '--url <value>'?: string\n\n  /**\n   * Increase the verbosity of the output. May include sensitive data.\n   * @environment SHOPIFY_FLAG_VERBOSE\n   */\n  '--verbose'?: ''\n}"
+      "value": "export interface themeprofile {\n  /**\n   * Alias of the Shopify account to use for authentication.\n   * @environment SHOPIFY_FLAG_AUTH_ALIAS\n   */\n  '--auth-alias <value>'?: string\n\n  /**\n   * The environment to apply to the current command.\n   * @environment SHOPIFY_FLAG_ENVIRONMENT\n   */\n  '-e, --environment <value>'?: string\n\n  /**\n   * Output the result as JSON. Automatically disables color output.\n   * @environment SHOPIFY_FLAG_JSON\n   */\n  '-j, --json'?: ''\n\n  /**\n   * Print the command's JSON schemas.\n   * @environment SHOPIFY_FLAG_JSON_SCHEMA\n   */\n  '--json-schema'?: ''\n\n  /**\n   * Disable color output.\n   * @environment SHOPIFY_FLAG_NO_COLOR\n   */\n  '--no-color'?: ''\n\n  /**\n   * Disable interactive prompts and browser authentication.\n   * @environment SHOPIFY_FLAG_NO_INPUT\n   */\n  '--no-input'?: ''\n\n  /**\n   * Password generated from the Theme Access app or an Admin API token.\n   * @environment SHOPIFY_CLI_THEME_TOKEN\n   */\n  '--password <value>'?: string\n\n  /**\n   * The path where you want to run the command. Defaults to the current working directory.\n   * @environment SHOPIFY_FLAG_PATH\n   */\n  '--path <value>'?: string\n\n  /**\n   * Store URL. It can be the store prefix (example) or the full myshopify.com URL (example.myshopify.com, https://example.myshopify.com).\n   * @environment SHOPIFY_FLAG_STORE\n   */\n  '-s, --store <value>'?: string\n\n  /**\n   * The password for storefronts with password protection.\n   * @environment SHOPIFY_FLAG_STORE_PASSWORD\n   */\n  '--store-password <value>'?: string\n\n  /**\n   * Theme ID or name of the remote theme.\n   * @environment SHOPIFY_FLAG_THEME_ID\n   */\n  '-t, --theme <value>'?: string\n\n  /**\n   * The path of the page to profile, with an optional query string. For a full URL, only the path and query string are used.\n   * @environment SHOPIFY_FLAG_URL\n   */\n  '--url <value>'?: string\n\n  /**\n   * Increase the verbosity of the output. May include sensitive data.\n   * @environment SHOPIFY_FLAG_VERBOSE\n   */\n  '--verbose'?: ''\n}"
     }
   },
   "themepublish": {

packages/cli/README.md

@@ -17689,7 +17689,8 @@ FLAGS
       [env: SHOPIFY_FLAG_STORE_PASSWORD]
 
   --url=<value>
-      [default: /] The url to be used as context
+      [default: /] The path of the page to profile, with an optional query string. For a full URL, only the path and query
+      string are used.
       [env: SHOPIFY_FLAG_URL]
 
   --verbose

packages/cli/oclif.manifest.json

@@ -12349,7 +12349,7 @@
         },
         "url": {
           "default": "/",
-          "description": "The url to be used as context",
+          "description": "The path of the page to profile, with an optional query string. For a full URL, only the path and query string are used.",
           "env": "SHOPIFY_FLAG_URL",
           "hasDynamicHelp": false,
           "multiple": false,

packages/theme/src/cli/commands/theme/profile.ts

@@ -37,7 +37,8 @@ export default class Profile extends ThemeCommand {
       env: 'SHOPIFY_FLAG_THEME_ID',
     }),
     url: Flags.string({
-      description: 'The url to be used as context',
+      description:
+        'The path of the page to profile, with an optional query string. For a full URL, only the path and query string are used.',
       env: 'SHOPIFY_FLAG_URL',
       default: '/',
     }),

packages/theme/src/cli/services/profile.test.ts

@@ -112,20 +112,173 @@ describe('profile', () => {
     await expect(result).rejects.toThrow('Network error')
   })
 
-  test('throws error when response status is not 200', async () => {
+  test('passes the query string of the url separately from the path', async () => {
+    // When
+    await profile(mockAdminSession, themeId, '/search?q=kw&page=2#results')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({
+        path: '/search',
+        query: [
+          ['q', 'kw'],
+          ['page', '2'],
+        ],
+      }),
+    )
+  })
+
+  test('does not let the url override the storefront renderer query params', async () => {
+    // When
+    await profile(mockAdminSession, themeId, '/collections/all?_fd=1&pb=1&page=2')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({path: '/collections/all', query: [['page', '2']]}),
+    )
+  })
+
+  test('uses only the path and query of an absolute url', async () => {
+    // When
+    await profile(mockAdminSession, themeId, 'https://custom-domain.com/products/test?variant=1')
+
+    // Then
+    expect(render).toHaveBeenCalledWith(
+      expect.anything(),
+      expect.objectContaining({path: '/products/test', query: [['variant', '1']]}),
+    )
+  })
+
+  test.each(['//products/test', 'https://'])('reports %s as an invalid url', async (url) => {
+    // When
+    const result = profile(mockAdminSession, themeId, url)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `${url} isn't a valid storefront URL.`,
+        'Pass the path of a storefront page to --url, for example /products/classic-leather-jacket.',
+      ),
+    )
+    expect(render).not.toHaveBeenCalled()
+  })
+
+  test('suggests the redirect destination when the page redirects within the storefront', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(new Response(null, {status: 301, headers: {Location: '/pages/new?_fd=0&pb=0'}}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`,
+        'It redirects to /pages/new. Run the command again with --url "/pages/new".',
+      ),
+    )
+  })
+
+  test('shows the full redirect destination when the page redirects to another host', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(
+      new Response(null, {
+        status: 301,
+        headers: {Location: 'https://custom-domain.com/de/pages/x?_fd=0&pb=0&view=full'},
+      }),
+    )
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`,
+        'It redirects to https://custom-domain.com/de/pages/x?view=full. If that page is on your storefront, run the command again with --url "/de/pages/x?view=full".',
+      ),
+    )
+  })
+
+  test('does not suggest the same url when the page redirects to the same path', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(
+      new Response(null, {status: 301, headers: {Location: `https://custom-domain.com${urlPath}?_fd=0&pb=0`}}),
+    )
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`,
+        `It redirects to https://custom-domain.com${urlPath}.`,
+      ),
+    )
+  })
+
+  test('suggests the storefront password when the page redirects to the password page', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(new Response(null, {status: 302, headers: {Location: '/password'}}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, '/')
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        "The page / redirects (302) instead of rendering, so it can't be profiled.",
+        'It redirects to the storefront password page. Check the storefront password, and pass it with --store-password.',
+      ),
+    )
+  })
+
+  test('reports a redirect without a location as an expected error', async () => {
+    // Given
+    vi.mocked(render).mockResolvedValue(new Response(null, {status: 301}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(
+      new AbortError(`The page ${urlPath} redirects (301) instead of rendering, so it can't be profiled.`),
+    )
+  })
+
+  test('reports a non-200 response as an expected error', async () => {
     // Given
     vi.mocked(render).mockResolvedValue(
       new Response(JSON.stringify({error: 'Some error message'}), {
         status: 404,
-        headers: new Headers({'content-type': 'application/json'}),
+        headers: new Headers({'content-type': 'application/json', 'x-request-id': 'request-id'}),
       }),
     )
 
     // When
     const result = profile(mockAdminSession, themeId, urlPath)
 
     // Then
-    await expect(result).rejects.toThrow('Bad response: 404: {"error":"Some error message"}')
+    await expect(result).rejects.toThrow(
+      new AbortError(
+        `Can't profile ${urlPath}. The storefront responded with 404.\nRequest ID: request-id\n{"error":"Some error message"}`,
+      ),
+    )
+  })
+
+  test('does not include a long non-JSON response body in the error', async () => {
+    // Given
+    const htmlPage = `<html><body>${'Server error. '.repeat(20)}</body></html>`
+    vi.mocked(render).mockResolvedValue(new Response(htmlPage, {status: 500}))
+
+    // When
+    const result = profile(mockAdminSession, themeId, urlPath)
+
+    // Then
+    await expect(result).rejects.toThrow(new AbortError(`Can't profile ${urlPath}. The storefront responded with 500.`))
   })
 
   test('throws error when a password is used', async () => {

packages/theme/src/cli/services/profile.ts

@@ -1,18 +1,32 @@
 import {themeProfileResultSchema} from './profile/types.js'
-import {render} from '../utilities/theme-environment/storefront-renderer.js'
+import {render, STOREFRONT_RENDERER_QUERY_PARAMS} from '../utilities/theme-environment/storefront-renderer.js'
 import {fetchDevServerSession} from '../utilities/theme-environment/dev-server-session.js'
 import {ensureValidPassword} from '../utilities/theme-environment/storefront-password-prompt.js'
 import {isStorefrontPasswordProtected} from '../utilities/theme-environment/storefront-session.js'
 import {AdminSession} from '@shopify/cli-kit/node/session'
 import {AbortError} from '@shopify/cli-kit/node/error'
 
+/** Origin used to parse store-relative URLs. It's never requested. */
+const PLACEHOLDER_ORIGIN = 'https://storefront.invalid'
+
+/** Longest non-JSON response body included in an error message, so full HTML pages are never shown. */
+const MAX_ERROR_BODY_LENGTH = 200
+
 export async function profile(
   adminSession: AdminSession,
   themeId: string,
   url: string,
   themeAccessPassword?: string,
   storefrontPassword?: string,
 ) {
+  // A leading `//` would be parsed as a host, so the rest of the path would be profiled instead.
+  if (url.startsWith('//') || !URL.canParse(url, PLACEHOLDER_ORIGIN)) {
+    throw new AbortError(
+      `${url} isn't a valid storefront URL.`,
+      'Pass the path of a storefront page to --url, for example /products/classic-leather-jacket.',
+    )
+  }
+
   const storePassword = (await isStorefrontPasswordProtected(adminSession))
     ? await ensureValidPassword(storefrontPassword, adminSession.storeFqdn)
     : undefined
@@ -25,22 +39,77 @@ export async function profile(
   }
 
   const session = await fetchDevServerSession(themeId, adminSession, themeAccessPassword, storePassword)
+  const target = storefrontTarget(url)
   const response = await render(session, {
     method: 'GET',
-    path: url,
-    query: [],
+    path: target.pathname,
+    query: [...target.searchParams],
     themeId,
     headers: {
       Accept: 'application/vnd.speedscope+json',
     },
   })
 
+  if (response.status >= 300 && response.status < 400) {
+    throw new AbortError(
+      `The page ${url} redirects (${response.status}) instead of rendering, so it can't be profiled.`,
+      redirectTryMessage(target, response.headers.get('Location')),
+    )
+  }
+
   if (response.status !== 200) {
-    const body = await response.text()
-    throw new Error(`Bad response: ${response.status}: ${body}`)
+    const details = await responseErrorDetails(response)
+    throw new AbortError(`Can't profile ${url}. The storefront responded with ${response.status}.${details}`)
   }
 
   const profileJson = await response.text()
 
   return {result: themeProfileResultSchema.parse(JSON.parse(profileJson)), source: profileJson}
 }
+
+/**
+ * Parses a `--url` or `Location` value into a URL without the renderer's own query params.
+ * The renderer adds its own query, so the query has to be passed separately from the path.
+ */
+function storefrontTarget(url: string) {
+  const target = new URL(url, PLACEHOLDER_ORIGIN)
+  Object.keys(STOREFRONT_RENDERER_QUERY_PARAMS).forEach((param) => target.searchParams.delete(param))
+  return target
+}
+
+function pathWithQuery(target: URL) {
+  return `${target.pathname}${target.search}`
+}
+
+function redirectTryMessage(requestedTarget: URL, location: string | null) {
+  if (!location || !URL.canParse(location, PLACEHOLDER_ORIGIN)) return null
+
+  const redirectTarget = storefrontTarget(location)
+  if (redirectTarget.pathname === '/password') {
+    return 'It redirects to the storefront password page. Check the storefront password, and pass it with --store-password.'
+  }
+
+  const suggestedUrl = pathWithQuery(redirectTarget)
+  const isStoreRelative = redirectTarget.origin === PLACEHOLDER_ORIGIN
+  const destination = isStoreRelative ? suggestedUrl : redirectTarget.href
+
+  // Suggesting the same --url again would make scripts and agents retry the same redirect.
+  if (suggestedUrl === pathWithQuery(requestedTarget)) return `It redirects to ${destination}.`
+
+  // An absolute destination can be another host, such as customer accounts, where the path doesn't exist on the store.
+  return isStoreRelative
+    ? `It redirects to ${destination}. Run the command again with --url "${suggestedUrl}".`
+    : `It redirects to ${destination}. If that page is on your storefront, run the command again with --url "${suggestedUrl}".`
+}
+
+async function responseErrorDetails(response: Response) {
+  const requestId = response.headers.get('x-request-id')
+  const requestIdDetail = requestId ? `\nRequest ID: ${requestId}` : ''
+
+  const body = (await response.text()).trim()
+  const isJsonBody = response.headers.get('Content-Type')?.includes('application/json') ?? false
+  const showBody = body.length > 0 && (isJsonBody || body.length <= MAX_ERROR_BODY_LENGTH)
+  const bodyDetail = showBody ? `\n${body}` : ''
+
+  return `${requestIdDetail}${bodyDetail}`
+}

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

@@ -7,6 +7,9 @@ import {AdminSession} from '@shopify/cli-kit/node/session'
 import {getThemeKitAccessDomain} from '@shopify/cli-kit/node/context/local'
 import {recordError} from '@shopify/cli-kit/node/analytics'
 
+/** Query params that the renderer sets on every storefront request. */
+export const STOREFRONT_RENDERER_QUERY_PARAMS = {_fd: '0', pb: '0'}
+
 export async function render(session: DevServerSession, context: DevServerRenderContext): Promise<Response> {
   const url = buildStorefrontUrl(session, context)
   const headers = await buildHeaders(session, context)
@@ -119,10 +122,7 @@ export function buildCookies(session: DevServerSession, ctx: Pick<DevServerRende
 function buildStorefrontUrl(session: DevServerSession, {path, sectionId, appBlockId, query}: DevServerRenderContext) {
   const baseUrl = buildBaseStorefrontUrl(session)
   const url = `${baseUrl}${path}`
-  const params = new URLSearchParams({
-    _fd: '0',
-    pb: '0',
-  })
+  const params = new URLSearchParams(STOREFRONT_RENDERER_QUERY_PARAMS)
 
   for (const [key, value] of query) {
     params.append(key, value)