From af985ebfc8ca0f0947e32aa90ce6f58a00915bfd Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Mon, 26 Oct 2026 09:58:24 -0500 Subject: [PATCH 1/2] Keep the storefront exit page working in the theme dev server --- .changeset/theme-dev-external-links.md | 6 + .../utilities/theme-environment/html.test.ts | 36 +++++ .../utilities/theme-environment/proxy.test.ts | 27 ++++ .../cli/utilities/theme-environment/proxy.ts | 8 +- .../storefront-renderer.test.ts | 136 ++++++++++++++++++ .../theme-environment/storefront-renderer.ts | 44 +++++- 6 files changed, 253 insertions(+), 4 deletions(-) create mode 100644 .changeset/theme-dev-external-links.md diff --git a/.changeset/theme-dev-external-links.md b/.changeset/theme-dev-external-links.md new file mode 100644 index 00000000000..bbf9d690d00 --- /dev/null +++ b/.changeset/theme-dev-external-links.md @@ -0,0 +1,6 @@ +--- +'@shopify/app': patch +'@shopify/theme': patch +--- + +Fix links to other sites in `theme dev` and theme app extension previews, which always showed the storefront's "leaving this store" page and could redirect back to the local server. diff --git a/packages/theme/src/cli/utilities/theme-environment/html.test.ts b/packages/theme/src/cli/utilities/theme-environment/html.test.ts index 88d59fd0ee8..3eaa6bd465a 100644 --- a/packages/theme/src/cli/utilities/theme-environment/html.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/html.test.ts @@ -78,6 +78,42 @@ describe('getHtmlHandler', async () => { expect(ctx.lastRequestedPath).toStrictEqual('/previous-page') }) + test('forwards the navigation fetch metadata and the redirect to another site from the exit page', async () => { + // Given + const handler = getHtmlHandler(theme, ctx) + const event = createH3Event('GET', '/url?q=https%3A%2F%2Fexample.com%2F&trigger=click', { + accept: 'text/html', + 'sec-fetch-mode': 'navigate', + 'sec-fetch-site': 'same-origin', + }) + + vi.mocked(render).mockResolvedValueOnce( + new Response(null, { + status: 302, + headers: {Location: 'https://example.com/', 'x-request-id': 'test-request-id'}, + }), + ) + + // When + const response = await handler(event) + + // Then + expect(render).toHaveBeenCalledWith( + ctx.session, + expect.objectContaining({ + method: 'GET', + path: '/url', + query: [ + ['q', 'https://example.com/'], + ['trigger', 'click'], + ], + headers: expect.objectContaining({'sec-fetch-mode': 'navigate', 'sec-fetch-site': 'same-origin'}), + }), + ) + expect(response.status).toBe(302) + expect(response.headers.get('Location')).toBe('https://example.com/') + }) + test('the development server session recovers when a theme id mismatch occurs', async () => { // Given const handler = getHtmlHandler(theme, ctx) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts index 4e1a56d8b21..6ccbce4298d 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -342,6 +342,33 @@ describe('dev proxy', () => { expect(patchedResponse.headers.get('Location')).toBe('/foo?bar=1') }) + test.each([ + ['a relative path', '/foo?_fd=0&pb=0&bar=1', '/foo?bar=1'], + ['the store domain over http', 'http://my-store.myshopify.com/foo?pb=0', '/foo'], + ['an uppercase store domain', 'https://MY-STORE.myshopify.com/foo', '/foo'], + ])('rewrites a Location header with %s to a local path', async (_, location, expected) => { + const redirectResponse = new Response(null, {status: 302, headers: {Location: location}}) + + const patchedResponse = await patchRenderingResponse(ctx, redirectResponse) + + expect(patchedResponse.headers.get('Location')).toBe(expected) + }) + + test.each([ + ['another site', 'https://example.com/path?_fd=0&pb=0&q=1'], + ['another site on a subdomain', 'https://checkout.jplhomer.org/'], + ['a host that starts with the store domain', 'https://my-store.myshopify.com.example.com/foo'], + ['a protocol-relative URL to another site', '//example.com/foo'], + ['a store checkout', 'https://my-store.myshopify.com/checkouts/cn/abc123'], + ])('keeps a Location header to %s unchanged', async (_, location) => { + const redirectResponse = new Response(null, {status: 302, headers: {Location: location}}) + + const patchedResponse = await patchRenderingResponse(ctx, redirectResponse) + + expect(patchedResponse.status).toBe(302) + expect(patchedResponse.headers.get('Location')).toBe(location) + }) + test('returns 3xx responses without reading or patching the body', async () => { const localCtx = { ...ctx, diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index 6991c2cea0a..e5a38046807 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -280,11 +280,13 @@ function patchProxiedResponseHeaders(ctx: DevServerContext, rawResponse: Respons const linkHeader = response.headers.get('Link') if (linkHeader) response.headers.set('Link', injectCdnProxy(linkHeader, ctx)) - // Location header might contain the store domain, proxy it: + // Location header might contain the store domain, proxy it. + // Redirects to other hosts (e.g. the exit page sending a shopper to another site) stay absolute: const locationHeader = response.headers.get('Location') if (locationHeader) { - const url = new URL(locationHeader, 'https://shopify.dev') - if (!CHECKOUT_PATTERN.test(url.pathname)) { + const storeOrigin = `https://${ctx.session.storeFqdn}` + const url = new URL(locationHeader, storeOrigin) + if (url.host === new URL(storeOrigin).host && !CHECKOUT_PATTERN.test(url.pathname)) { url.searchParams.delete('_fd') url.searchParams.delete('pb') response.headers.set('Location', url.href.replace(url.origin, '')) diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts index ab1a1ab048a..f50bf765c1b 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.test.ts @@ -1,8 +1,13 @@ import {render} from './storefront-renderer.js' import {DevServerRenderContext, DevServerSession} from './types.js' import {describe, expect, test, vi} from 'vitest' +import {fetch as cliKitFetch, Response as NodeFetchResponse} from '@shopify/cli-kit/node/http' vi.mock('@shopify/cli-kit/node/session') +vi.mock('@shopify/cli-kit/node/http', async (importOriginal) => ({ + ...(await importOriginal()), + fetch: vi.fn(), +})) vi.stubGlobal('fetch', vi.fn()) const session: DevServerSession = { @@ -284,6 +289,137 @@ describe('render', () => { ) }) + describe('exit page', () => { + const exitPageContext: DevServerRenderContext = { + ...context, + path: '/url', + query: [ + ['q', 'https://example.com/'], + ['trigger', 'click'], + ], + headers: { + ...context.headers, + accept: 'text/html', + referer: 'http://127.0.0.1:9292/products/1', + 'sec-fetch-dest': 'document', + 'sec-fetch-mode': 'navigate', + 'sec-fetch-site': 'same-origin', + }, + } + + test('renders with the CLI fetch so the request keeps its navigation fetch metadata', async () => { + // Given + vi.mocked(cliKitFetch).mockResolvedValue( + new NodeFetchResponse(null, { + status: 302, + headers: [ + ['Location', 'https://example.com/'], + ['Set-Cookie', 'a=1; Path=/'], + ['Set-Cookie', 'b=2; Path=/'], + ], + }), + ) + + // When + const response = await render(session, exitPageContext) + + // Then + expect(fetch).not.toHaveBeenCalled() + expect(cliKitFetch).toHaveBeenCalledWith( + 'https://store.myshopify.com/url?_fd=0&pb=0&q=https%3A%2F%2Fexample.com%2F&trigger=click', + { + method: 'GET', + redirect: 'manual', + headers: expect.objectContaining({ + Authorization: 'Bearer token_111222333', + Cookie: + 'theme_cookie=abc; storefront_digest=00001111222233334444; _shopify_essential=:00112233445566778899:', + referer: 'http://127.0.0.1:9292/products/1', + 'sec-fetch-dest': 'document', + 'sec-fetch-mode': 'navigate', + 'sec-fetch-site': 'same-origin', + }), + }, + 'slow-request', + ) + expect(response).toBeInstanceOf(Response) + expect(response.status).toEqual(302) + expect(response.headers.get('Location')).toEqual('https://example.com/') + expect(response.headers.getSetCookie()).toEqual(['a=1; Path=/', 'b=2; Path=/']) + }) + + test('returns the page body as a web Response', async () => { + // Given + vi.mocked(cliKitFetch).mockResolvedValue( + new NodeFetchResponse('Leave the store?', { + status: 200, + headers: {'Content-Type': 'text/html; charset=utf-8', 'x-request-id': 'abc'}, + }), + ) + + // When + const response = await render(session, exitPageContext) + + // Then + expect(response).toBeInstanceOf(Response) + expect(response.status).toEqual(200) + expect(response.headers.get('x-request-id')).toEqual('abc') + expect(response.headers.get('Content-Type')).toBeNull() + await expect(response.text()).resolves.toEqual('Leave the store?') + }) + + test('renders a localized exit page with a GET, even with in-memory templates', async () => { + // Given + vi.mocked(cliKitFetch).mockResolvedValue(new NodeFetchResponse(null, {status: 200})) + + // When + await render(session, { + ...exitPageContext, + path: '/fr-ca/url', + replaceTemplates: {'sections/header.liquid': '
hello
'}, + replaceExtensionTemplates: {'blocks/block.liquid': '
hello
'}, + }) + + // Then + expect(fetch).not.toHaveBeenCalled() + expect(cliKitFetch).toHaveBeenCalledWith( + expect.stringMatching(/^https:\/\/store\.myshopify\.com\/fr-ca\/url\?/), + expect.objectContaining({method: 'GET', redirect: 'manual'}), + 'slow-request', + ) + expect(vi.mocked(cliKitFetch).mock.calls[0]?.[1]).not.toHaveProperty('body') + }) + + test('throws a fetch error when the request fails', async () => { + // Given + vi.mocked(cliKitFetch).mockRejectedValue(new Error('socket hang up')) + + // When/Then + await expect(render(session, exitPageContext)).rejects.toMatchObject({ + statusCode: 502, + cause: expect.objectContaining({message: 'socket hang up'}), + data: {url: expect.stringMatching(/^https:\/\/store\.myshopify\.com\/url\?/)}, + }) + }) + + test.each<[DevServerRenderContext['method'], string]>([ + ['GET', '/pages/url'], + ['GET', '/url-shortener'], + ['GET', '/products/url'], + ['POST', '/url'], + ])('renders %s %s with the global fetch', async (method, path) => { + // Given + vi.mocked(fetch).mockResolvedValue(new Response()) + + // When + await render(session, {...exitPageContext, method, path}) + + // Then + expect(cliKitFetch).not.toHaveBeenCalled() + expect(fetch).toHaveBeenCalledOnce() + }) + }) + test('renders using query parameters', async () => { // Given vi.mocked(fetch).mockResolvedValue(new Response()) diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts index 78219841d8f..36207611044 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts @@ -6,10 +6,17 @@ import {outputDebug} from '@shopify/cli-kit/node/output' 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' +import {fetch as nodeFetch} from '@shopify/cli-kit/node/http' /** Query params that the renderer sets on every storefront request. */ export const STOREFRONT_RENDERER_QUERY_PARAMS = {_fd: '0', pb: '0'} +/** + * The storefront page that confirms a shopper wants to leave the store + * (`/url` or `/:locale/url`). Storefront links to other sites go through it. + */ +const EXIT_PAGE_PATH_REGEX = /^(?:\/[a-z]{2,3}(?:-[a-zA-Z0-9]{2,})?)?\/url\/?$/ + export async function render(session: DevServerSession, context: DevServerRenderContext): Promise { const url = buildStorefrontUrl(session, context) const headers = await buildHeaders(session, context) @@ -21,7 +28,13 @@ export async function render(session: DevServerSession, context: DevServerRender const replaceTemplates = Object.keys({...context.replaceTemplates, ...context.replaceExtensionTemplates}) - if (replaceTemplates.length > 0) { + if (isExitPageRequest(context)) { + outputDebug(`→ Rendering ${url} (exit page)...`) + + response = await fetchExitPage(url, requestHeaders).catch((error) => { + throw createFetchError(recordError(error), url) + }) + } else if (replaceTemplates.length > 0) { outputDebug(`→ Rendering ${url} (with ${replaceTemplates})...`) const bodyParams = storefrontReplaceTemplatesParams(context) @@ -68,6 +81,35 @@ export async function render(session: DevServerSession, context: DevServerRender return response } +function isExitPageRequest({method, path}: Pick) { + return method.toUpperCase() === 'GET' && EXIT_PAGE_PATH_REGEX.test(path) +} + +/** + * The exit page only redirects to another site when the request is a real + * navigation (`Sec-Fetch-Mode: navigate`), as it is for the browser's request + * to the local server. Node's global `fetch` always replaces the + * `Sec-Fetch-Mode` header with its own request mode (`cors`), so the page would + * always show the confirmation instead. The CLI's `fetch` sends the header as + * is. The exit page doesn't render theme templates, so it's always a GET. + */ +async function fetchExitPage(url: string, headers: Record): Promise { + const response = await nodeFetch(url, {method: 'GET', redirect: 'manual', headers}, 'slow-request') + + const responseHeaders = new Headers() + for (const [key, values] of Object.entries(response.headers.raw())) { + for (const value of values) responseHeaders.append(key, value) + } + + const hasNullBody = [101, 103, 204, 205, 304].includes(response.status) + + return new Response(hasNullBody ? null : await response.arrayBuffer(), { + status: response.status, + statusText: response.statusText, + headers: responseHeaders, + }) +} + export async function buildHeaders(session: DevServerSession, context: Pick) { if (isThemeAccessSession(session)) { return buildThemeAccessHeaders(session, context) From 97c6656d4c88b1f8b299b5fbbae9bc292d1ea775 Mon Sep 17 00:00:00 2001 From: Josh Larson Date: Mon, 26 Oct 2026 10:46:21 -0500 Subject: [PATCH 2/2] Keep protocol-relative external redirects on HTTPS in the theme dev proxy --- .../cli/utilities/theme-environment/proxy.test.ts | 14 +++++++++++++- .../src/cli/utilities/theme-environment/proxy.ts | 6 +++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts index 6ccbce4298d..5d44a03d9a7 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -346,6 +346,7 @@ describe('dev proxy', () => { ['a relative path', '/foo?_fd=0&pb=0&bar=1', '/foo?bar=1'], ['the store domain over http', 'http://my-store.myshopify.com/foo?pb=0', '/foo'], ['an uppercase store domain', 'https://MY-STORE.myshopify.com/foo', '/foo'], + ['a protocol-relative store domain', '//my-store.myshopify.com/foo?_fd=0&bar=1', '/foo?bar=1'], ])('rewrites a Location header with %s to a local path', async (_, location, expected) => { const redirectResponse = new Response(null, {status: 302, headers: {Location: location}}) @@ -358,7 +359,6 @@ describe('dev proxy', () => { ['another site', 'https://example.com/path?_fd=0&pb=0&q=1'], ['another site on a subdomain', 'https://checkout.jplhomer.org/'], ['a host that starts with the store domain', 'https://my-store.myshopify.com.example.com/foo'], - ['a protocol-relative URL to another site', '//example.com/foo'], ['a store checkout', 'https://my-store.myshopify.com/checkouts/cn/abc123'], ])('keeps a Location header to %s unchanged', async (_, location) => { const redirectResponse = new Response(null, {status: 302, headers: {Location: location}}) @@ -369,6 +369,18 @@ describe('dev proxy', () => { expect(patchedResponse.headers.get('Location')).toBe(location) }) + test.each([ + ['//example.com/foo', 'https://example.com/foo'], + ['//checkout.jplhomer.org/path?_fd=0&pb=0&q=1', 'https://checkout.jplhomer.org/path?_fd=0&pb=0&q=1'], + ])('makes a protocol-relative Location header to another site HTTPS (%s)', async (location, expected) => { + const redirectResponse = new Response(null, {status: 302, headers: {Location: location}}) + + const patchedResponse = await patchRenderingResponse(ctx, redirectResponse) + + expect(patchedResponse.status).toBe(302) + expect(patchedResponse.headers.get('Location')).toBe(expected) + }) + test('returns 3xx responses without reading or patching the body', async () => { const localCtx = { ...ctx, diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index e5a38046807..c0192a8bb17 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -286,10 +286,14 @@ function patchProxiedResponseHeaders(ctx: DevServerContext, rawResponse: Respons if (locationHeader) { const storeOrigin = `https://${ctx.session.storeFqdn}` const url = new URL(locationHeader, storeOrigin) - if (url.host === new URL(storeOrigin).host && !CHECKOUT_PATTERN.test(url.pathname)) { + const isStoreHost = url.host === new URL(storeOrigin).host + if (isStoreHost && !CHECKOUT_PATTERN.test(url.pathname)) { url.searchParams.delete('_fd') url.searchParams.delete('pb') response.headers.set('Location', url.href.replace(url.origin, '')) + } else if (!isStoreHost && locationHeader.startsWith('//')) { + // Browsers resolve protocol-relative URLs against the local HTTP server, so make them absolute HTTPS URLs: + response.headers.set('Location', url.href) } }