From 7dcb758555ab2a18b0f17aaaffc65bb5aadf9824 Mon Sep 17 00:00:00 2001 From: Ujjawal Yadav <63ujjawal.yadav@gmail.com> Date: Fri, 11 Sep 2026 01:52:52 +0530 Subject: [PATCH 1/2] Fix `theme dev` ignoring SHOPIFY_HTTP(S)_PROXY `createGlobalProxyAgent` patches Node's http/https agents, but the storefront request path called the built-in fetch (undici), which does not use them, so every render and proxied asset bypassed the proxy. Route both through `fetch` from `@shopify/cli-kit/node/http`, keeping the built-in `Response` at the H3 boundary, dropping the body on null-body statuses (node-fetch always reports a stream there) and keeping repeated `set-cookie` headers separate. Fixes #5890 --- .changeset/theme-dev-http-proxy.md | 5 + .../utilities/theme-environment/proxy.test.ts | 118 +++++++++++++----- .../cli/utilities/theme-environment/proxy.ts | 33 +++-- .../storefront-renderer.test.ts | 62 ++++++++- .../theme-environment/storefront-renderer.ts | 49 +++++--- .../theme-environment/storefront-utils.ts | 40 ++++++ .../theme-environment.test.ts | 71 ++++++----- 7 files changed, 285 insertions(+), 93 deletions(-) create mode 100644 .changeset/theme-dev-http-proxy.md diff --git a/.changeset/theme-dev-http-proxy.md b/.changeset/theme-dev-http-proxy.md new file mode 100644 index 00000000000..ed068fff853 --- /dev/null +++ b/.changeset/theme-dev-http-proxy.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Respect `SHOPIFY_HTTP(S)_PROXY` in `theme dev` when rendering and proxying the storefront 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..1c28a6dbd56 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -11,14 +11,22 @@ import { standardEventsRuntimeDevUrl, standardEventsRuntimeUrl, } from './standard-events.js' -import {describe, test, expect, vi, beforeEach, afterEach} from 'vitest' +import {describe, test, expect, vi, beforeEach, afterEach, type Mock} from 'vitest' import {createEvent} from 'h3' +import {fetch, Response as HttpResponse} from '@shopify/cli-kit/node/http' import {IncomingMessage, ServerResponse} from 'node:http' import {Socket} from 'node:net' +import {Readable} from 'stream' +import {text} from 'stream/consumers' import type {DevServerContext} from './types.js' +vi.mock('@shopify/cli-kit/node/http', async (importOriginal) => ({ + ...(await importOriginal()), + fetch: vi.fn(), +})) + function createH3Event(method = 'GET', path = '/', headers = {}) { const req = new IncomingMessage(new Socket()) const res = new ServerResponse(req) @@ -385,6 +393,15 @@ describe('dev proxy', () => { }) describe('getProxyStorefrontHeaders', () => { + test('does not forward the browser accept-encoding, so the client only negotiates encodings it decodes', () => { + const event = createH3Event('GET', '/', {'accept-encoding': 'gzip, deflate, br, zstd', accept: 'text/html'}) + + const headers = getProxyStorefrontHeaders(event) + + expect(headers['accept-encoding']).toBeUndefined() + expect(headers.accept).toBe('text/html') + }) + test('filters out hop-by-hop headers and adds required headers', () => { const event = createH3Event() event.context.clientAddress = '42' @@ -518,8 +535,8 @@ describe('dev proxy', () => { }) test('passes crawler signature headers to proxied SFR requests', async () => { - const fetchMock = vi.fn().mockResolvedValue(new Response('OK')) - vi.stubGlobal('fetch', fetchMock) + const fetchMock = vi.fn().mockResolvedValue(new HttpResponse('OK')) + vi.mocked(fetch).mockImplementation(fetchMock) const event = createH3Event('GET', '/cart.js') const localCtx = { ...ctx, @@ -539,7 +556,7 @@ describe('dev proxy', () => { try { await proxyStorefrontRequest(event, localCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] expect(init.headers).toEqual( expect.objectContaining({ Signature: 'signature-value', @@ -548,13 +565,50 @@ describe('dev proxy', () => { }), ) } finally { - vi.unstubAllGlobals() + vi.mocked(fetch).mockReset() + } + }) + + test('drops the body when the storefront responds with 304 Not Modified', async () => { + const fetchMock = vi.fn().mockResolvedValue(new HttpResponse(Readable.from(['']), {status: 304})) + vi.mocked(fetch).mockImplementation(fetchMock) + const event = createH3Event('GET', '/cdn/shop/files/style.css') + + try { + const response = await proxyStorefrontRequest(event, ctx) + + expect(response.status).toBe(304) + expect(response.body).toBeNull() + const [, init] = fetchMock.mock.calls[0] as [string, {body?: unknown}] + expect(init.body).toBeUndefined() + } finally { + vi.mocked(fetch).mockReset() + } + }) + + test('forwards the request body as a stream, without retries or a timeout', async () => { + const fetchMock = vi.fn().mockResolvedValue(new HttpResponse('OK')) + vi.mocked(fetch).mockImplementation(fetchMock) + const event = createH3Event('POST', '/cart/add.js', {'content-type': 'application/x-www-form-urlencoded'}) + + try { + const pending = proxyStorefrontRequest(event, ctx) + event.node.req.push('id=1&quantity=2') + event.node.req.push(null) + await pending + + const [, init, behaviour] = fetchMock.mock.calls[0] as [string, {body?: Readable}, string] + expect(init.body).toBeInstanceOf(Readable) + await expect(text(init.body!)).resolves.toBe('id=1&quantity=2') + expect(behaviour).toBe('slow-request') + } finally { + vi.mocked(fetch).mockReset() } }) }) describe('proxyStorefrontRequest — Bearer token auth scoping', () => { - let fetchMock: ReturnType + let fetchMock: Mock const tokenCtx = { ...ctx, type: 'theme', @@ -566,18 +620,18 @@ describe('dev proxy', () => { } as unknown as DevServerContext beforeEach(() => { - fetchMock = vi.fn().mockResolvedValue(new Response('OK')) - vi.stubGlobal('fetch', fetchMock) + fetchMock = vi.fn().mockResolvedValue(new HttpResponse('OK')) + vi.mocked(fetch).mockImplementation(fetchMock) }) afterEach(() => { - vi.unstubAllGlobals() + vi.mocked(fetch).mockReset() }) test('sends Bearer token for CDN asset requests', async () => { const event = createH3Event('GET', '/cdn/shop/files/style.css') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBe('Bearer sfr-devtools-token') }) @@ -585,7 +639,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for /cart/add.js', async () => { const event = createH3Event('POST', '/cart/add.js') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -593,7 +647,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for /cart.js', async () => { const event = createH3Event('GET', '/cart.js') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -601,7 +655,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for /cart.json', async () => { const event = createH3Event('GET', '/cart.json') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -609,7 +663,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for /cart/', async () => { const event = createH3Event('GET', '/cart/') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -617,7 +671,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for checkout endpoints', async () => { const event = createH3Event('GET', '/checkouts/xyz') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -625,7 +679,7 @@ describe('dev proxy', () => { test('does NOT send Bearer token for account endpoints', async () => { const event = createH3Event('GET', '/account/logout') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -637,7 +691,7 @@ describe('dev proxy', () => { } as unknown as DevServerContext const event = createH3Event('GET', '/assets/style.css') await proxyStorefrontRequest(event, extCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -645,7 +699,7 @@ describe('dev proxy', () => { test('strips query string before auth check for /cart.js?sections=header', async () => { const event = createH3Event('GET', '/cart.js?sections=header') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() }) @@ -653,7 +707,7 @@ describe('dev proxy', () => { test('sends Bearer token for non-CDN paths that are not cart/checkout/account', async () => { const event = createH3Event('GET', '/products/some-product') await proxyStorefrontRequest(event, tokenCtx) - const [, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [, init] = fetchMock.mock.calls[0] as [string, RequestInit] const headers = init.headers as Record expect(headers.Authorization).toBe('Bearer sfr-devtools-token') }) @@ -670,15 +724,15 @@ describe('dev proxy', () => { }, } as unknown as DevServerContext - let fetchMock: ReturnType + let fetchMock: Mock beforeEach(() => { - fetchMock = vi.fn().mockResolvedValue(new Response('{"data":{}}')) - vi.stubGlobal('fetch', fetchMock) + fetchMock = vi.fn().mockResolvedValue(new HttpResponse('{"data":{}}')) + vi.mocked(fetch).mockImplementation(fetchMock) }) afterEach(() => { - vi.unstubAllGlobals() + vi.mocked(fetch).mockReset() }) test('forwards /api/YYYY-MM/graphql.json without injecting theme auth, cookies, referer, or dev params', async () => { @@ -690,11 +744,11 @@ describe('dev proxy', () => { await proxyStorefrontRequest(event, passthroughCtx) expect(fetchMock).toHaveBeenCalledOnce() - const [requestUrl, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [requestUrl, init] = fetchMock.mock.calls[0] as [string, RequestInit] - expect(requestUrl.toString()).toBe('https://my-store.myshopify.com/api/2026-01/graphql.json') - expect(requestUrl.searchParams.has('_fd')).toBe(false) - expect(requestUrl.searchParams.has('pb')).toBe(false) + expect(requestUrl).toBe('https://my-store.myshopify.com/api/2026-01/graphql.json') + expect(new URL(requestUrl).searchParams.has('_fd')).toBe(false) + expect(new URL(requestUrl).searchParams.has('pb')).toBe(false) const headers = init.headers as Record expect(headers['x-shopify-storefront-access-token']).toBe('public-access-token') @@ -710,9 +764,9 @@ describe('dev proxy', () => { await proxyStorefrontRequest(event, passthroughCtx) expect(fetchMock).toHaveBeenCalledOnce() - const [requestUrl, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [requestUrl, init] = fetchMock.mock.calls[0] as [string, RequestInit] - expect(requestUrl.toString()).toBe('https://my-store.myshopify.com/api/unstable/graphql.json') + expect(requestUrl).toBe('https://my-store.myshopify.com/api/unstable/graphql.json') const headers = init.headers as Record expect(headers.Authorization).toBeUndefined() expect(headers.Cookie).toBeUndefined() @@ -724,10 +778,10 @@ describe('dev proxy', () => { await proxyStorefrontRequest(event, passthroughCtx) expect(fetchMock).toHaveBeenCalledOnce() - const [requestUrl, init] = fetchMock.mock.calls[0] as [URL, RequestInit] + const [requestUrl, init] = fetchMock.mock.calls[0] as [string, RequestInit] - expect(requestUrl.searchParams.get('_fd')).toBe('0') - expect(requestUrl.searchParams.get('pb')).toBe('0') + expect(new URL(requestUrl).searchParams.get('_fd')).toBe('0') + expect(new URL(requestUrl).searchParams.get('pb')).toBe('0') const headers = init.headers as Record expect(headers.Authorization).toBe('Bearer sfr-devtools-token') }) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index 6991c2cea0a..4888b03415e 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -1,14 +1,17 @@ -import {cleanHeader, defaultHeaders} from './storefront-utils.js' +import {cleanHeader, defaultHeaders, STOREFRONT_REQUEST_BEHAVIOUR, toWebResponse} from './storefront-utils.js' import {buildCookies} from './storefront-renderer.js' import {injectStandardEventsInspector, rewriteStandardEventsRuntimeReferences} from './standard-events.js' import {logRequestLine} from '../log-request-line.js' import {createFetchError, extractFetchErrorInfo} from '../errors.js' import {renderWarning} from '@shopify/cli-kit/node/ui' +import {fetch} from '@shopify/cli-kit/node/http' import {defineEventHandler, getRequestHeaders, getRequestWebStream, getRequestIP, type H3Event} from 'h3' import {extname} from '@shopify/cli-kit/node/path' import {lookupMimeType} from '@shopify/cli-kit/node/mimes' import {recordError} from '@shopify/cli-kit/node/analytics' +import {Readable} from 'stream' +import {type ReadableStream as WebReadableStream} from 'stream/web' import type {Theme} from '@shopify/cli-kit/node/themes/types' import type {DevServerContext} from './types.js' @@ -320,6 +323,11 @@ export function getProxyStorefrontHeaders(event: H3Event) { // so we must also remove it from the response CSP. delete proxyRequestHeaders['upgrade-insecure-requests'] + // The client decodes gzip, deflate and br, and advertises exactly those when this header is absent. + // Forwarding the browser's list (Chrome adds zstd) would let the storefront answer with an encoding + // that is passed through undecoded, after `content-encoding` has been removed from the response. + delete proxyRequestHeaders['accept-encoding'] + const ipAddress = getRequestIP(event) if (ipAddress) proxyRequestHeaders['X-Forwarded-For'] = ipAddress @@ -367,16 +375,19 @@ export function proxyStorefrontRequest(event: H3Event, ctx: DevServerContext): P }) } - // eslint-disable-next-line no-restricted-globals - return fetch(url, { - method: event.method, - body, - duplex: body ? 'half' : undefined, - // Important to return 3xx responses to the client - redirect: 'manual', - headers, - } as RequestInit & {duplex?: 'half'}) - .then((response) => patchProxiedResponseHeaders(ctx, response)) + return fetch( + url.href, + { + method: event.method, + // The client reads request bodies as Node streams, unlike the built-in fetch. + body: body && Readable.fromWeb(body as WebReadableStream), + // Important to return 3xx responses to the client + redirect: 'manual', + headers, + }, + STOREFRONT_REQUEST_BEHAVIOUR, + ) + .then((response) => patchProxiedResponseHeaders(ctx, toWebResponse(response))) .catch((error: Error) => { throw createFetchError(recordError(error), url) }) 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..ddc2305d4b5 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,9 +1,15 @@ import {render} from './storefront-renderer.js' +import {STOREFRONT_REQUEST_BEHAVIOUR} from './storefront-utils.js' import {DevServerRenderContext, DevServerSession} from './types.js' import {describe, expect, test, vi} from 'vitest' +import {fetch, Response} from '@shopify/cli-kit/node/http' +import {Readable} from 'stream' vi.mock('@shopify/cli-kit/node/session') -vi.stubGlobal('fetch', vi.fn()) +vi.mock('@shopify/cli-kit/node/http', async (importOriginal) => ({ + ...(await importOriginal()), + fetch: vi.fn(), +})) const session: DevServerSession = { token: 'admin_token_abc123', @@ -56,6 +62,7 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -83,6 +90,7 @@ describe('render', () => { 'Signature-Agent': 'signature-agent-value', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -141,6 +149,7 @@ describe('render', () => { 'Content-Length': '100', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) expect(fetch).toHaveBeenCalledWith( 'https://theme-kit-access.shopifyapps.com/cli/sfr/products/1?_fd=0&pb=0', @@ -150,6 +159,7 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -176,6 +186,7 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -202,6 +213,7 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -229,6 +241,7 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -253,6 +266,7 @@ describe('render', () => { method: 'GET', redirect: 'manual', }), + 'slow-request', ) }) @@ -281,6 +295,7 @@ describe('render', () => { method: 'POST', redirect: 'manual', }), + 'slow-request', ) }) @@ -310,6 +325,51 @@ describe('render', () => { 'X-Special-Header': '200', }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) + + test('drops the body when the storefront responds with a status that cannot carry one', async () => { + // Given + // Unlike the built-in fetch, the client always exposes a body stream, even on a 304. The + // browser revalidates every cached asset, so this is the common path, not an edge case. + vi.mocked(fetch).mockResolvedValue( + new Response(Readable.from(['']), { + status: 304, + statusText: 'Not Modified', + headers: {etag: 'W/"abc123"'}, + }), + ) + + // When + const response = await render(session, context) + + // Then + expect(response.status).toEqual(304) + expect(response.body).toBeNull() + expect(response.headers.get('etag')).toEqual('W/"abc123"') + }) + test('keeps repeated set-cookie headers separate', async () => { + // Given + // Iterating the client's headers joins repeated values with ', ', which would merge the + // storefront's session cookies into one. + vi.mocked(fetch).mockResolvedValue( + new Response(null, { + status: 302, + headers: [ + ['set-cookie', '_shopify_essential=:abc:; path=/'], + ['set-cookie', 'storefront_digest=123; path=/'], + ], + }), + ) + + // When + const response = await render(session, context) + + // Then + expect(response.headers.getSetCookie()).toEqual([ + '_shopify_essential=:abc:; path=/', + 'storefront_digest=123; path=/', + ]) + }) }) 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 adf7b47fc6e..11855a823c4 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-renderer.ts @@ -1,7 +1,14 @@ import {DevServerSession, DevServerRenderContext} from './types.js' -import {cleanHeader, defaultHeaders, storefrontReplaceTemplatesParams} from './storefront-utils.js' +import { + cleanHeader, + defaultHeaders, + storefrontReplaceTemplatesParams, + STOREFRONT_REQUEST_BEHAVIOUR, + toWebResponse, +} from './storefront-utils.js' import {parseCookies, serializeCookies} from './cookies.js' import {createFetchError} from '../errors.js' +import {fetch, type Response as HttpResponse} from '@shopify/cli-kit/node/http' 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' @@ -14,7 +21,7 @@ export async function render(session: DevServerSession, context: DevServerRender ...headers, ...defaultHeaders(), } - let response: Response + let response: HttpResponse const replaceTemplates = Object.keys({...context.replaceTemplates, ...context.replaceExtensionTemplates}) @@ -23,24 +30,30 @@ export async function render(session: DevServerSession, context: DevServerRender const bodyParams = storefrontReplaceTemplatesParams(context) - // eslint-disable-next-line no-restricted-globals - response = await fetch(url, { - method: 'POST', - body: bodyParams, - redirect: 'manual', - headers: requestHeaders, - }).catch((error) => { + response = await fetch( + url, + { + method: 'POST', + body: bodyParams, + redirect: 'manual', + headers: requestHeaders, + }, + STOREFRONT_REQUEST_BEHAVIOUR, + ).catch((error) => { throw createFetchError(recordError(error), url) }) } else { outputDebug(`→ Rendering ${url}...`) - // eslint-disable-next-line no-restricted-globals - response = await fetch(url, { - method: context.method, - redirect: 'manual', - headers: requestHeaders, - }).catch((error) => { + response = await fetch( + url, + { + method: context.method, + redirect: 'manual', + headers: requestHeaders, + }, + STOREFRONT_REQUEST_BEHAVIOUR, + ).catch((error) => { throw createFetchError(recordError(error), url) }) } @@ -56,13 +69,13 @@ export async function render(session: DevServerSession, context: DevServerRender const contentType = response.headers.get('Content-Type') const isJsonResponse = contentType?.includes('application/json') - response = new Response(response.body, response) + const webResponse = toWebResponse(response) if (!isJsonResponse) { - response.headers.delete('Content-Type') + webResponse.headers.delete('Content-Type') } - return response + return webResponse } export async function buildHeaders(session: DevServerSession, context: Pick) { diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts index e70b63131d5..16cc94a91fc 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts @@ -1,5 +1,45 @@ import {DevServerRenderContext} from './types.js' import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version' +import {type Response as HttpResponse} from '@shopify/cli-kit/node/http' + +/** + * The "null body statuses" of the Fetch spec, which the `Response` constructor refuses to pair with + * a body. + * + * The built-in fetch reports a `null` body for these, but the client in + * `@shopify/cli-kit/node/http` always reports a stream, so the body has to be dropped explicitly. + * `304` is the one that matters in practice: the browser revalidates every cached asset. + */ +const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]) + +/** + * Storefront requests are proxied straight through to the browser, so they keep the behaviour the + * built-in fetch had here: no automatic cancellation (a slow theme should still render) and no + * automatic retries (these carry cart and checkout writes, which are not safe to replay). + */ +export const STOREFRONT_REQUEST_BEHAVIOUR = 'slow-request' as const + +/** + * Converts a response from `@shopify/cli-kit/node/http` into a built-in `Response`. + * + * The dev server hands its responses to H3, which only understands the built-in type, so this is + * the boundary where responses from the proxy-aware client are converted back. + */ +export function toWebResponse(response: HttpResponse): Response { + const body = NULL_BODY_STATUSES.has(response.status) ? null : response.body + + // Iterating the client's headers joins repeated values with ', ', which would merge the + // storefront's `set-cookie` headers into one. `raw()` keeps each value separate. + const headers = Object.entries(response.headers.raw()).flatMap(([name, values]) => + values.map((value): [string, string] => [name, value]), + ) + + return new Response(body as BodyInit | null, { + status: response.status, + statusText: response.statusText, + headers, + }) +} export function storefrontReplaceTemplatesParams(context: DevServerRenderContext): URLSearchParams { /** diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts index 3dfc4088617..77c7edbae00 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts @@ -1,6 +1,7 @@ import {DevServerContext} from './types.js' import {setupDevServer} from './theme-environment.js' import {render} from './storefront-renderer.js' +import {STOREFRONT_REQUEST_BEHAVIOUR} from './storefront-utils.js' import {reconcileAndPollThemeEditorChanges} from './remote-theme-watcher.js' import {hotReloadScriptId} from './hot-reload/server.js' import { @@ -14,6 +15,7 @@ import {emptyThemeExtFileSystem} from '../theme-fs-empty.js' import {DEVELOPMENT_THEME_ROLE} from '@shopify/cli-kit/node/themes/utils' import {describe, expect, test, vi, beforeEach, afterEach} from 'vitest' +import {fetch, Response as HttpResponse} from '@shopify/cli-kit/node/http' import {buildTheme} from '@shopify/cli-kit/node/themes/factories' import {createEvent} from 'h3' import * as output from '@shopify/cli-kit/node/output' @@ -25,6 +27,10 @@ import {Socket} from 'node:net' vi.mock('@shopify/cli-kit/node/themes/api', () => ({fetchChecksums: vi.fn(() => Promise.resolve([]))})) vi.mock('./remote-theme-watcher.js') vi.mock('./storefront-renderer.js') +vi.mock('@shopify/cli-kit/node/http', async (importOriginal) => ({ + ...(await importOriginal()), + fetch: vi.fn(), +})) vi.spyOn(output, 'outputDebug') // Vitest is resetting this mock between tests due to a global config `mockReset: true`. @@ -768,8 +774,8 @@ describe('setupDevServer', () => { }) test('forwards unknown compiled_assets requests to SFR', async () => { - const fetchStub = vi.fn(async () => new Response()) - vi.stubGlobal('fetch', fetchStub) + const fetchStub = vi.fn(async () => new HttpResponse()) + vi.mocked(fetch).mockImplementation(fetchStub) // Request a compiled asset that doesn't exist await dispatchEvent(server, '/compiled_assets/nonexistent.js', {host: defaultHost}) @@ -777,10 +783,9 @@ describe('setupDevServer', () => { // Should fall back to proxy expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenLastCalledWith( - new URL( - `https://${defaultServerContext.session.storeFqdn}/compiled_assets/nonexistent.js?${targetQuerystring}`, - ), + `https://${defaultServerContext.session.storeFqdn}/compiled_assets/nonexistent.js?${targetQuerystring}`, expect.any(Object), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) @@ -814,12 +819,12 @@ describe('setupDevServer', () => { test('proxies other requests to SFR', async () => { const fetchStub = vi.fn( async () => - new Response('mocked', { + new HttpResponse('mocked', { headers: {'proxy-authorization': 'true', 'content-type': 'application/javascript'}, }), ) - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) // --- Unknown endpoint: const eventPromise = dispatchEvent(server, '/path/to/something-else.js', {host: defaultHost}) @@ -828,7 +833,7 @@ describe('setupDevServer', () => { expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenLastCalledWith( - new URL(`https://${defaultServerContext.session.storeFqdn}/path/to/something-else.js?${targetQuerystring}`), + `https://${defaultServerContext.session.storeFqdn}/path/to/something-else.js?${targetQuerystring}`, expect.objectContaining({ method: 'GET', redirect: 'manual', @@ -838,6 +843,7 @@ describe('setupDevServer', () => { Authorization: expect.stringContaining('Bearer'), }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) const {res, body} = await eventPromise @@ -853,9 +859,7 @@ describe('setupDevServer', () => { ).resolves.not.toThrow() expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenLastCalledWith( - new URL( - `https://${defaultServerContext.session.storeFqdn}/cdn/somepathhere/assets/file42.css?${targetQuerystring}`, - ), + `https://${defaultServerContext.session.storeFqdn}/cdn/somepathhere/assets/file42.css?${targetQuerystring}`, expect.objectContaining({ method: 'GET', redirect: 'manual', @@ -865,13 +869,14 @@ describe('setupDevServer', () => { Authorization: expect.stringContaining('Bearer'), }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) test('proxies .css.liquid assets with injected CDN', async () => { const fetchStub = vi.fn( async () => - new Response( + new HttpResponse( `.some-class { font-family: "My Font"; src: url(//${defaultServerContext.session.storeFqdn}/cdn/shop/t/img/assets/font.woff2); @@ -880,7 +885,7 @@ describe('setupDevServer', () => { ), ) - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) const eventPromise = dispatchEvent(server, '/cdn/shop/t/img/assets/file3.css', {host: defaultHost}) await expect(eventPromise).resolves.not.toThrow() @@ -891,8 +896,8 @@ describe('setupDevServer', () => { }) test('proxies .js.liquid assets replacing the error query string', async () => { - const fetchStub = vi.fn(async () => new Response()) - vi.stubGlobal('fetch', fetchStub) + const fetchStub = vi.fn(async () => new HttpResponse()) + vi.mocked(fetch).mockImplementation(fetchStub) vi.useFakeTimers() const now = Date.now() @@ -902,15 +907,16 @@ describe('setupDevServer', () => { expect(vi.mocked(render)).not.toHaveBeenCalled() expect(fetchStub).toHaveBeenCalledWith( - new URL(`https://${defaultServerContext.session.storeFqdn}${pathname}?v=${now}&${targetQuerystring}`), + `https://${defaultServerContext.session.storeFqdn}${pathname}?v=${now}&${targetQuerystring}`, expect.any(Object), + STOREFRONT_REQUEST_BEHAVIOUR, ) }) test('falls back to proxying if a rendering request fails with 4xx status', async () => { const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) - fetchStub.mockResolvedValueOnce(new Response(null, {status: 302})) + vi.mocked(fetch).mockImplementation(fetchStub) + fetchStub.mockResolvedValueOnce(new HttpResponse(null, {status: 302})) vi.mocked(render).mockResolvedValueOnce(new Response(null, {status: 401})) const eventPromise = dispatchEvent(server, '/non-renderable-path', {host: defaultHost}) @@ -919,7 +925,7 @@ describe('setupDevServer', () => { expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenLastCalledWith( - new URL(`https://${defaultServerContext.session.storeFqdn}/non-renderable-path?${targetQuerystring}`), + `https://${defaultServerContext.session.storeFqdn}/non-renderable-path?${targetQuerystring}`, expect.objectContaining({ method: 'GET', redirect: 'manual', @@ -929,6 +935,7 @@ describe('setupDevServer', () => { Authorization: expect.stringContaining('Bearer'), }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) await expect(eventPromise).resolves.toHaveProperty('status', 302) @@ -946,9 +953,9 @@ describe('setupDevServer', () => { const standardEventsServer = setupDevServer(developmentTheme, standardEventsContext) const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) fetchStub.mockResolvedValueOnce( - new Response(``, { + new HttpResponse(``, { status: 200, headers: {'content-type': 'text/html; charset=utf-8'}, }), @@ -965,8 +972,8 @@ describe('setupDevServer', () => { test('forwards rendering error after proxy failure', async () => { const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) - fetchStub.mockResolvedValueOnce(new Response(null, {status: 404})) + vi.mocked(fetch).mockImplementation(fetchStub) + fetchStub.mockResolvedValueOnce(new HttpResponse(null, {status: 404})) vi.mocked(render).mockResolvedValueOnce(new Response(null, {status: 401})) const eventPromise = dispatchEvent(server, '/non-renderable-path', {host: defaultHost}) @@ -975,7 +982,7 @@ describe('setupDevServer', () => { expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenLastCalledWith( - new URL(`https://${defaultServerContext.session.storeFqdn}/non-renderable-path?${targetQuerystring}`), + `https://${defaultServerContext.session.storeFqdn}/non-renderable-path?${targetQuerystring}`, expect.objectContaining({ method: 'GET', redirect: 'manual', @@ -985,6 +992,7 @@ describe('setupDevServer', () => { Authorization: expect.stringContaining('Bearer'), }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) await expect(eventPromise).resolves.toHaveProperty('status', 401) @@ -992,8 +1000,8 @@ describe('setupDevServer', () => { test('skips proxy for known rendering requests like Section Rendering API', async () => { const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) - fetchStub.mockResolvedValueOnce(new Response(null, {status: 200})) + vi.mocked(fetch).mockImplementation(fetchStub) + fetchStub.mockResolvedValueOnce(new HttpResponse(null, {status: 200})) vi.mocked(render).mockResolvedValue(new Response(null, {status: 404})) await expect( @@ -1017,14 +1025,14 @@ describe('setupDevServer', () => { test('only handles compiled assets for theme context, not theme-extension context', async () => { // Given - const fetchStub = vi.fn(async () => new Response('mocked compiled asset', {status: 200})) + const fetchStub = vi.fn(async () => new HttpResponse('mocked compiled asset', {status: 200})) const themeExtensionContext = { ...defaultServerContext, type: 'theme-extension' as const, } const themeExtServer = setupDevServer(developmentTheme, themeExtensionContext) - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) // When const event = createH3Event({url: '/compiled_assets/styles.css', headers: {host: defaultHost}}) @@ -1033,7 +1041,7 @@ describe('setupDevServer', () => { // Then expect(fetchStub).toHaveBeenCalledOnce() expect(fetchStub).toHaveBeenCalledWith( - new URL(`https://${defaultServerContext.session.storeFqdn}/compiled_assets/styles.css?${targetQuerystring}`), + `https://${defaultServerContext.session.storeFqdn}/compiled_assets/styles.css?${targetQuerystring}`, expect.objectContaining({ method: 'GET', redirect: 'manual', @@ -1042,6 +1050,7 @@ describe('setupDevServer', () => { 'User-Agent': expect.stringContaining('Shopify CLI'), }), }), + STOREFRONT_REQUEST_BEHAVIOUR, ) // Reset for comparison with theme context @@ -1057,7 +1066,7 @@ describe('setupDevServer', () => { test('renders error page on network errors with hot reload script injected', async () => { const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) vi.mocked(render).mockRejectedValueOnce(new Error('Network error')) const eventPromise = dispatchEvent(server, '/', {host: defaultHost}) @@ -1091,7 +1100,7 @@ describe('setupDevServer', () => { test('renders error page on upload errors with hot reload script injected', async () => { const fetchStub = vi.fn() - vi.stubGlobal('fetch', fetchStub) + vi.mocked(fetch).mockImplementation(fetchStub) localThemeFileSystem.uploadErrors.set('templates/asset.json', ['Error 1', 'Error 2']) const eventPromise = dispatchEvent(server, '/', {host: defaultHost}) From 65eadc64ecaa8d405b50c64212f5cea74e061e00 Mon Sep 17 00:00:00 2001 From: Ujjawal Yadav <63ujjawal.yadav@gmail.com> Date: Fri, 11 Sep 2026 18:04:01 +0530 Subject: [PATCH 2/2] Only drop `content-encoding` when the client decompressed the body The client in `@shopify/cli-kit/node/http` decompresses only exact `gzip`, `x-gzip`, `deflate`, `x-deflate` and `br` codings. A chained value such as `br, gzip` reaches us still compressed, and the unconditional `content-encoding` delete then relabelled it as identity, so the browser was handed compressed bytes it would not decode. The built-in fetch decoded chained codings, so this was a narrowing introduced by routing through the client. Also drop `101` and `103` from the null-body statuses. The `Response` constructor rejects any status outside 200-599, so neither can be built here whatever the body is. --- .../utilities/theme-environment/proxy.test.ts | 30 +++++++++++++++++++ .../cli/utilities/theme-environment/proxy.ts | 17 +++++++++-- .../theme-environment/storefront-utils.ts | 6 ++-- 3 files changed, 47 insertions(+), 6 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 1c28a6dbd56..bb3bef8f3ef 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.test.ts @@ -586,6 +586,36 @@ describe('dev proxy', () => { } }) + test('removes content-encoding when the client decompressed the body', async () => { + const fetchMock = vi.fn().mockResolvedValue(new HttpResponse('body', {headers: {'content-encoding': 'gzip'}})) + vi.mocked(fetch).mockImplementation(fetchMock) + const event = createH3Event('GET', '/cdn/shop/files/style.css') + + try { + const response = await proxyStorefrontRequest(event, ctx) + + expect(response.headers.get('content-encoding')).toBeNull() + } finally { + vi.mocked(fetch).mockReset() + } + }) + + test('keeps content-encoding when the client left the body compressed', async () => { + const fetchMock = vi + .fn() + .mockResolvedValue(new HttpResponse('still-compressed', {headers: {'content-encoding': 'br, gzip'}})) + vi.mocked(fetch).mockImplementation(fetchMock) + const event = createH3Event('GET', '/cdn/shop/files/style.css') + + try { + const response = await proxyStorefrontRequest(event, ctx) + + expect(response.headers.get('content-encoding')).toBe('br, gzip') + } finally { + vi.mocked(fetch).mockReset() + } + }) + test('forwards the request body as a stream, without retries or a timeout', async () => { const fetchMock = vi.fn().mockResolvedValue(new HttpResponse('OK')) vi.mocked(fetch).mockImplementation(fetchMock) diff --git a/packages/theme/src/cli/utilities/theme-environment/proxy.ts b/packages/theme/src/cli/utilities/theme-environment/proxy.ts index 4888b03415e..776fb7e6b66 100644 --- a/packages/theme/src/cli/utilities/theme-environment/proxy.ts +++ b/packages/theme/src/cli/utilities/theme-environment/proxy.ts @@ -259,13 +259,24 @@ const HOP_BY_HOP_HEADERS = [ 'host', ] +/** + * The codings the client in `@shopify/cli-kit/node/http` decompresses, and only when + * `content-encoding` is exactly one of them. A chained value such as `br, gzip` is passed through + * still compressed. + */ +const DECODED_CONTENT_ENCODINGS = new Set(['gzip', 'x-gzip', 'deflate', 'x-deflate', 'br']) + function patchProxiedResponseHeaders(ctx: DevServerContext, rawResponse: Response) { const response = new Response(rawResponse.body, rawResponse) - // Node's `fetch` always decompresses the body, so we must remove these headers - // to prevent the browser from decompressing it again: + // The body no longer matches the original length, and the client decompresses it whenever it + // understands the coding, in which case the browser must not decompress it again. When it does + // not, the body arrives still compressed and `content-encoding` has to survive. response.headers.delete('content-length') - response.headers.delete('content-encoding') + const contentEncoding = response.headers.get('content-encoding') + if (contentEncoding && DECODED_CONTENT_ENCODINGS.has(contentEncoding.trim().toLowerCase())) { + response.headers.delete('content-encoding') + } for (const header of HOP_BY_HOP_HEADERS) { response.headers.delete(header) } diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts index 16cc94a91fc..a70e9460ed7 100644 --- a/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-utils.ts @@ -3,14 +3,14 @@ import {CLI_KIT_VERSION} from '@shopify/cli-kit/common/version' import {type Response as HttpResponse} from '@shopify/cli-kit/node/http' /** - * The "null body statuses" of the Fetch spec, which the `Response` constructor refuses to pair with - * a body. + * The "null body statuses" of the Fetch spec that a `Response` can represent. `101` and `103` are + * null-body too, but the constructor rejects any status outside 200-599, so they cannot be built here. * * The built-in fetch reports a `null` body for these, but the client in * `@shopify/cli-kit/node/http` always reports a stream, so the body has to be dropped explicitly. * `304` is the one that matters in practice: the browser revalidates every cached asset. */ -const NULL_BODY_STATUSES = new Set([101, 103, 204, 205, 304]) +const NULL_BODY_STATUSES = new Set([204, 205, 304]) /** * Storefront requests are proxied straight through to the browser, so they keep the behaviour the