diff --git a/.changeset/theme-dev-large-headers.md b/.changeset/theme-dev-large-headers.md new file mode 100644 index 00000000000..eb39adff445 --- /dev/null +++ b/.changeset/theme-dev-large-headers.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix `theme dev` rendering every page as a 502 on stores whose storefront responses carry large `Set-Cookie` header blocks, by pinning a 128 KiB header-size cap on the dev server and its storefront fetch instead of inheriting Node's defaults. diff --git a/packages/theme/package.json b/packages/theme/package.json index ab27ace8cfa..6e1c7cca7fc 100644 --- a/packages/theme/package.json +++ b/packages/theme/package.json @@ -46,6 +46,7 @@ "@shopify/theme-language-server-node": "2.23.1", "chokidar": "3.6.0", "h3": "1.15.11", + "undici": "7.28.0", "yaml": "2.9.0" }, "devDependencies": { diff --git a/packages/theme/src/cli/utilities/errors.test.ts b/packages/theme/src/cli/utilities/errors.test.ts index 9d046017444..bf16895fe44 100644 --- a/packages/theme/src/cli/utilities/errors.test.ts +++ b/packages/theme/src/cli/utilities/errors.test.ts @@ -157,6 +157,17 @@ describe('errors', () => { expect(info.status).toBe(502) expect(info.statusText).toBe('Bad Gateway') expect(info.body).toBe(error.stack) + expect(info.causeCode).toBeUndefined() + }) + + test('surfaces the code of a nested cause', () => { + const rootCause = Object.assign(new Error('Headers Overflow Error'), {code: 'UND_ERR_HEADERS_OVERFLOW'}) + const fetchError = createFetchError(new TypeError('fetch failed', {cause: rootCause}), 'https://test.com') + + const info = extractFetchErrorInfo(fetchError, 'Failed to render storefront') + + expect(info.causeCode).toBe('UND_ERR_HEADERS_OVERFLOW') + expect(info.headline).toContain('Cause: UND_ERR_HEADERS_OVERFLOW') }) }) }) diff --git a/packages/theme/src/cli/utilities/errors.ts b/packages/theme/src/cli/utilities/errors.ts index 03d98510df5..380934b3166 100644 --- a/packages/theme/src/cli/utilities/errors.ts +++ b/packages/theme/src/cli/utilities/errors.ts @@ -83,16 +83,35 @@ export function createFetchError(resource: Partial, url?: stri * @param fetchError - The error to extract information from. * @param context - The context description of the error. */ +/** + * Finds the first `code` carried by an error or one of its nested causes. + * Fetch failures hide the actionable error (for example undici's + * `UND_ERR_HEADERS_OVERFLOW`) one or two `cause` hops below a generic + * `TypeError: fetch failed`, so walk the chain instead of only looking at + * the outermost error. + */ +function findCauseCode(error: Error): string | undefined { + let current: unknown = error + for (let depth = 0; depth < 5 && current instanceof Error; depth++) { + const code = (current as Error & {code?: unknown}).code + if (typeof code === 'string' && code.length > 0) return code + current = current.cause + } + return undefined +} + export function extractFetchErrorInfo(fetchError: Error | FetchError, context = 'Unexpected error during fetch') { const error = fetchError as FetchError const status = error.statusCode ?? 502 const statusText = error.statusMessage ?? 'Bad Gateway' + const cause = (error.cause ?? fetchError) as Error + const causeCode = findCauseCode(cause) + let headline = `${context.replace(/\.$/, '')} with status ${status} (${statusText}).` if (error.data?.requestId) headline += `\nRequest ID: ${error.data.requestId}` if (error.data?.url) headline += `\nURL: ${error.data.url}` - - const cause = (error.cause ?? fetchError) as Error + if (causeCode) headline += `\nCause: ${causeCode}` return { headline, @@ -100,6 +119,7 @@ export function extractFetchErrorInfo(fetchError: Error | FetchError, context = status, statusText, cause, + causeCode, requestId: error.data?.requestId, url: error.data?.url, } diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.test.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.test.ts new file mode 100644 index 00000000000..4561cdcba92 --- /dev/null +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.test.ts @@ -0,0 +1,137 @@ +import {MAX_HEADER_SIZE, installStorefrontFetchDispatcher} from './storefront-fetch.js' + +import {Agent, getGlobalDispatcher, setGlobalDispatcher} from 'undici' +import {afterAll, describe, expect, test} from 'vitest' + +import {createServer, type Server} from 'node:http' + +const originalDispatcher = getGlobalDispatcher() + +afterAll(() => { + // Other files share no process state with this one, but leave the global + // dispatcher exactly as it was found anyway. + setGlobalDispatcher(originalDispatcher) +}) + +/** + * Starts a server that responds with `Set-Cookie` headers totalling roughly + * `headerBytes` bytes, mimicking a storefront response on a cookie'd request. + */ +async function startCookieServer(headerBytes: number): Promise<{url: string; close: () => Promise}> { + const server = createServer((_req, res) => { + const cookies: string[] = [] + let size = 0 + for (let index = 0; size < headerBytes; index++) { + const cookie = `cookie${index}=${'x'.repeat(200)}; Path=/; Secure` + cookies.push(cookie) + size += cookie.length + } + res.writeHead(200, {'content-type': 'text/html', 'set-cookie': cookies}) + res.end('ok') + }) + return listen(server) +} + +async function startEchoServer(options?: {maxHeaderSize?: number}): Promise<{url: string; close: () => Promise}> { + const server = createServer(options ?? {}, (_req, res) => { + res.writeHead(200, {'content-type': 'text/plain'}) + res.end('ok') + }) + return listen(server) +} + +async function listen(server: Server): Promise<{url: string; close: () => Promise}> { + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() + if (!address || typeof address === 'string') throw new Error('Expected the test server to have a port') + return { + url: `http://127.0.0.1:${address.port}/`, + close: async () => { + server.closeAllConnections() + await new Promise((resolve) => server.close(() => resolve())) + }, + } +} + +async function fetchError(url: string): Promise { + try { + const response = await fetch(url) + await response.text() + } catch (error) { + // Fetch failures surface as TypeError; anything else is a broken test. + if (error instanceof TypeError) return error as Error & {cause?: Error & {code?: string}} + throw error + } + throw new Error(`Expected fetching ${url} to fail`) +} + +describe('storefront fetch header limits', () => { + test('rejects a storefront response past the inherited default cap before the dispatcher is installed', async () => { + // Given a plain undici agent, whose header cap is whatever Node defaults to + setGlobalDispatcher(new Agent()) + const server = await startCookieServer(64 * 1024) + try { + // When fetching a response with a ~64 KiB header block + const error = await fetchError(server.url) + // Then the real cause is the header overflow, not the generic wrapper + expect(error.message).toBe('fetch failed') + expect(error.cause?.code).toBe('UND_ERR_HEADERS_OVERFLOW') + } finally { + await server.close() + } + }) + + test('accepts storefront responses with large Set-Cookie headers once the dispatcher is installed', async () => { + // Given + installStorefrontFetchDispatcher() + const server = await startCookieServer(96 * 1024) + try { + // When + const response = await fetch(server.url) + // Then + expect(response.status).toBe(200) + await expect(response.text()).resolves.toContain('ok') + } finally { + await server.close() + } + }) + + test('still rejects storefront responses past the pinned cap', async () => { + // Given + installStorefrontFetchDispatcher() + const server = await startCookieServer(MAX_HEADER_SIZE * 2) + try { + // When / Then: the cap is pinned at MAX_HEADER_SIZE, not removed + const error = await fetchError(server.url) + expect(error.cause?.code).toBe('UND_ERR_HEADERS_OVERFLOW') + } finally { + await server.close() + } + }) + + test('a server created with MAX_HEADER_SIZE accepts requests with large Cookie headers', async () => { + const cookie = `session=${'x'.repeat(64 * 1024)}` + + // Given a server with Node's default request-header cap + const defaultServer = await startEchoServer() + try { + const response = await fetch(defaultServer.url, {headers: {cookie}}) + // Then Node turns the browser's request away before the app sees it + expect(response.status).toBe(431) + await response.text() + } finally { + await defaultServer.close() + } + + // Given the dev server's pinned cap + const server = await startEchoServer({maxHeaderSize: MAX_HEADER_SIZE}) + try { + // When / Then the same request is handled normally + const response = await fetch(server.url, {headers: {cookie}}) + expect(response.status).toBe(200) + await expect(response.text()).resolves.toBe('ok') + } finally { + await server.close() + } + }) +}) diff --git a/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.ts b/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.ts new file mode 100644 index 00000000000..d890b0a3d55 --- /dev/null +++ b/packages/theme/src/cli/utilities/theme-environment/storefront-fetch.ts @@ -0,0 +1,43 @@ +import {Agent, setGlobalDispatcher} from 'undici' + +/** + * Header-size cap for theme dev traffic, in bytes. + * + * Storefronts legitimately send large header blocks: a cookie'd request can + * collect a `Set-Cookie` collection well past 16 KiB, and the browser's + * cookie jar for the local origin grows the same way on the request side. + * Neither Node's bundled fetch nor its HTTP server lets the CLI inherit a + * stable default here — the effective cap has drifted between Node releases + * (8 KiB, then 16 KiB, with the bundled fetch parser behaving differently + * again), which made `theme dev` fail with `UND_ERR_HEADERS_OVERFLOW` 502s + * depending on the Node version under it. Pin the cap explicitly instead: + * 128 KiB matches the largest mainstream CDN header ceiling and covers the + * measured failing storefront responses with headroom, while remaining an + * upper-bound check rather than a preallocated buffer. + */ +export const MAX_HEADER_SIZE = 128 * 1024 + +/** + * Creates the fetch dispatcher used for storefront requests in `theme dev`. + * A dedicated dispatcher (rather than undici's defaults) is what pins the + * response-header cap described on {@link MAX_HEADER_SIZE}. + */ +export function createStorefrontDispatcher() { + return new Agent({maxHeaderSize: MAX_HEADER_SIZE}) +} + +let dispatcherInstalled = false + +/** + * Installs {@link createStorefrontDispatcher} as the global fetch dispatcher + * for this process. The storefront proxy and renderer use the global `fetch`, + * which reads the global dispatcher (undici coordinates it across copies via + * a shared global symbol), so installing it once when a dev server starts + * covers every storefront request without changing those call sites. + * Idempotent: later calls are no-ops. + */ +export function installStorefrontFetchDispatcher() { + if (dispatcherInstalled) return + dispatcherInstalled = true + setGlobalDispatcher(createStorefrontDispatcher()) +} diff --git a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts index f5294e1f912..88a964658d2 100644 --- a/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts +++ b/packages/theme/src/cli/utilities/theme-environment/theme-environment.ts @@ -1,4 +1,5 @@ import {getHotReloadHandler, setupInMemoryTemplateWatcher} from './hot-reload/server.js' +import {MAX_HEADER_SIZE, installStorefrontFetchDispatcher} from './storefront-fetch.js' import {getHtmlHandler} from './html.js' import {getAssetsHandler} from './local-assets.js' import {getProxyHandler} from './proxy.js' @@ -130,6 +131,8 @@ interface DevelopmentServerInstance { } function createDevelopmentServer(theme: Theme, ctx: DevServerContext, initialWork: Promise) { + installStorefrontFetchDispatcher() + const app = createApp() const allowedOrigins = [ `http://${ctx.options.host}:${ctx.options.port}`, @@ -168,7 +171,10 @@ function createDevelopmentServer(theme: Theme, ctx: DevServerContext, initialWor app.use(getProxyHandler(theme, ctx)) app.use(getHtmlHandler(theme, ctx)) - const server = createServer(toNodeListener(app)) + // Pin the request-header cap for the same reason as the fetch dispatcher: + // the browser's cookie jar for this origin accumulates the storefront's + // cookies, so legitimate requests can exceed Node's default cap. + const server = createServer({maxHeaderSize: MAX_HEADER_SIZE}, toNodeListener(app)) return { dispatch: app.handler.bind(app), diff --git a/packages/theme/src/cli/utilities/theme-ext-environment/theme-ext-server.ts b/packages/theme/src/cli/utilities/theme-ext-environment/theme-ext-server.ts index 3f419f7bb38..2d3d2ca77c0 100644 --- a/packages/theme/src/cli/utilities/theme-ext-environment/theme-ext-server.ts +++ b/packages/theme/src/cli/utilities/theme-ext-environment/theme-ext-server.ts @@ -5,6 +5,7 @@ import {getAssetsHandler} from '../theme-environment/local-assets.js' import {getProxyHandler} from '../theme-environment/proxy.js' import {getHotReloadHandler, triggerHotReload} from '../theme-environment/hot-reload/server.js' import {createHostValidationHandler} from '../theme-environment/host-validation.js' +import {MAX_HEADER_SIZE, installStorefrontFetchDispatcher} from '../theme-environment/storefront-fetch.js' import {emptyThemeFileSystem} from '../theme-fs-empty.js' import {initializeDevServerSession} from '../theme-environment/dev-server-session.js' import {createApp, toNodeListener} from 'h3' @@ -36,6 +37,8 @@ export async function initializeDevelopmentExtensionServer(theme: Theme, devExt: } export function createDevelopmentExtensionServer(theme: Theme, ctx: DevServerContext) { + installStorefrontFetchDispatcher() + const app = createApp() app.use(createHostValidationHandler(ctx.options.host, ctx.options.port)) @@ -44,7 +47,7 @@ export function createDevelopmentExtensionServer(theme: Theme, ctx: DevServerCon app.use(getProxyHandler(theme, ctx)) app.use(getHtmlHandler(theme, ctx)) - const server = createServer(toNodeListener(app)) + const server = createServer({maxHeaderSize: MAX_HEADER_SIZE}, toNodeListener(app)) return { dispatch: app.handler.bind(app), diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7a1f983b4f9..f12095d70d1 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -701,6 +701,9 @@ importers: h3: specifier: 1.15.11 version: 1.15.11 + undici: + specifier: 7.28.0 + version: 7.28.0 yaml: specifier: 2.9.0 version: 2.9.0