From c9c2faa08f3d3e726a0cc3eb23ce3f97c749bfb3 Mon Sep 17 00:00:00 2001 From: River Date: Fri, 29 May 2026 20:38:04 +0000 Subject: [PATCH] Fix theme dev serving theme asset for /cdn/extensions/ requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `shopify theme dev` was serving a local theme alongside an installed app whose extension bundles a same-named asset (e.g. both ship `assets/app.js`), a request for the extension's asset URL (`/cdn/extensions///assets/app.js`) returned the **theme** asset instead of being proxied to cdn.shopify.com. The bug was in `findLocalFile` (`local-assets.ts`). Its theme matcher ran first via `??` and its optional `(?:/cdn/.*?)?` prefix greedily swallowed the `/cdn/extensions//` segment, capturing only `app.js`. `tryGetFile` then derived the lookup key from the capture alone, found `assets/app.js` in `localThemeFileSystem`, and served it. The extension matcher only accepted an `/ext/cdn/extensions/` prefix and never got a chance for a bare `/cdn/extensions/` request, so the `??` short-circuited before `getProxyHandler` could see the request. The colliding script then loaded twice (once as the theme asset, once as the extension asset that was silently swapped for the theme asset), producing duplicated execution — e.g. double-bound event handlers. The live storefront serves both URLs correctly; this was local-dev only. Fix: tighten the theme regex with a negative lookahead so its `/cdn/` prefix cannot be followed by `extensions/`, and broaden the extension regex to also accept `/cdn/extensions/...` in addition to `/ext/cdn/extensions/...`. When the requested extension asset is not in `localThemeExtensionFileSystem`, the handler falls through and `getProxyHandler` forwards the request to cdn.shopify.com, which returns the real installed-app asset. Behaviour: - `/cdn/extensions///assets/` → proxied to CDN (or served from `localThemeExtensionFileSystem` if locally developed and present there). - `/cdn/shop/t//assets/` → still served from `localThemeFileSystem`. - `/ext/cdn/extensions/...` → still served from `localThemeExtensionFileSystem`. Reported: https://community.shopify.dev/t/cli-is-serving-incorrect-cdn-assets/34726 Requested by Wes Shaw Co-authored-by: Wes Shaw --- ...iver-theme-dev-cdn-extensions-collision.md | 5 + .../theme-environment/local-assets.test.ts | 99 +++++++++++++++++++ .../theme-environment/local-assets.ts | 21 +++- 3 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 .changeset/river-theme-dev-cdn-extensions-collision.md create mode 100644 packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts diff --git a/.changeset/river-theme-dev-cdn-extensions-collision.md b/.changeset/river-theme-dev-cdn-extensions-collision.md new file mode 100644 index 00000000000..558a9a9727e --- /dev/null +++ b/.changeset/river-theme-dev-cdn-extensions-collision.md @@ -0,0 +1,5 @@ +--- +'@shopify/theme': patch +--- + +Fix `theme dev` serving a same-named theme asset in response to a `/cdn/extensions/...` request. When a theme and an installed app extension shared an asset filename (e.g. `app.js`), the local dev server's theme matcher swallowed the extension URL prefix and returned the theme file. Extension asset requests now fall through to the CDN proxy (or to a locally-developed extension's filesystem) as intended. diff --git a/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts b/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts new file mode 100644 index 00000000000..3c459f20cbe --- /dev/null +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts @@ -0,0 +1,99 @@ +import {findLocalFile} from './local-assets.js' +import {emptyThemeFileSystem, emptyThemeExtFileSystem} from '../theme-fs-empty.js' +import {describe, expect, test} from 'vitest' +import {createEvent} from 'h3' +import {IncomingMessage, ServerResponse} from 'node:http' +import {Socket} from 'node:net' +import type {DevServerContext} from './types.js' + +function createH3Event(path: string) { + const req = new IncomingMessage(new Socket()) + const res = new ServerResponse(req) + req.method = 'GET' + req.url = path + req.headers = {} + return createEvent(req, res) +} + +function buildCtx({ + themeFiles = [] as [string, unknown][], + extFiles = [] as [string, unknown][], +}): DevServerContext { + const themeFs = emptyThemeFileSystem() + for (const [key, value] of themeFiles) themeFs.files.set(key, value as never) + const extFs = emptyThemeExtFileSystem() + for (const [key, value] of extFiles) extFs.files.set(key, value as never) + return {localThemeFileSystem: themeFs, localThemeExtensionFileSystem: extFs} as unknown as DevServerContext +} + +describe('findLocalFile', () => { + const themeAsset = {checksum: 't', key: 'assets/app.js', value: '// theme app.js'} + const extAsset = {checksum: 'e', key: 'assets/app.js', value: '// extension app.js'} + + test('serves a local theme asset for /cdn//assets/', () => { + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile(createH3Event('/cdn/shop/t/12/assets/app.js?v=1'), ctx) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(themeAsset) + }) + + test('serves a local theme asset for a bare /assets/ request', () => { + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile(createH3Event('/assets/app.js'), ctx) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(themeAsset) + }) + + test('serves a local extension asset for /ext/cdn/extensions///assets/', () => { + const ctx = buildCtx({extFiles: [['assets/app.js', extAsset]]}) + const result = findLocalFile( + createH3Event('/ext/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/my-app/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(extAsset) + }) + + test('does not serve a theme asset in response to /cdn/extensions///assets/ (regression)', () => { + // A request for an installed-app extension asset must not be answered with a + // same-named theme asset. With no local extension file present the request + // must fall through (fileKey === undefined) so getProxyHandler can forward + // it to cdn.shopify.com. + const ctx = buildCtx({themeFiles: [['assets/app.js', themeAsset]]}) + const result = findLocalFile( + createH3Event('/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/klaviyo-email-marketing-54/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBeUndefined() + expect(result.file).toBeUndefined() + }) + + test('prefers the local extension asset for /cdn/extensions/... when one exists locally', () => { + // Both filesystems have assets/app.js. The extension matcher must win + // (and the theme matcher must not match this path at all), so the + // extension content is served instead of the theme content. + const ctx = buildCtx({ + themeFiles: [['assets/app.js', themeAsset]], + extFiles: [['assets/app.js', extAsset]], + }) + const result = findLocalFile( + createH3Event('/cdn/extensions/019e1813-804f-7f97-ad2d-278904fdd92f/my-app/assets/app.js'), + ctx, + ) + + expect(result.fileKey).toBe('assets/app.js') + expect(result.file).toBe(extAsset) + }) + + test('returns no match when neither filesystem has the file', () => { + const ctx = buildCtx({}) + const result = findLocalFile(createH3Event('/cdn/shop/t/12/assets/missing.js'), ctx) + + expect(result.fileKey).toBeUndefined() + expect(result.file).toBeUndefined() + }) +}) diff --git a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts index 4b5314b0b2d..e925fef4a42 100644 --- a/packages/theme/src/cli/utilities/theme-environment/local-assets.ts +++ b/packages/theme/src/cli/utilities/theme-environment/local-assets.ts @@ -66,7 +66,17 @@ export function getAssetsHandler(_theme: Theme, ctx: DevServerContext) { }) } -function findLocalFile(event: H3Event, ctx: DevServerContext) { +// Theme assets are served under either a bare `/assets/...` path or the vanity CDN +// prefix `/cdn/.../assets/...`. The negative lookahead prevents this matcher from +// claiming `/cdn/extensions/...` paths, which belong to theme app extensions. +const THEME_ASSET_PATTERN = /^(?:\/cdn\/(?!extensions\/).*?)?\/assets\/([^?]+)/ + +// Theme app extension assets are served under either `/ext/cdn/extensions/...` +// (locally-developed extensions, rewritten by `injectCdnProxy` via EXTENSION_CDN_PREFIX) +// or `/cdn/extensions/...` (storefront-emitted URLs for installed extensions). +const THEME_EXTENSION_ASSET_PATTERN = /^(?:\/(?:ext\/)?cdn\/extensions\/.*?)?\/assets\/([^?]+)/ + +export function findLocalFile(event: H3Event, ctx: DevServerContext) { const tryGetFile = (pattern: RegExp, fileSystem: VirtualFileSystem) => { const matchedFileName = event.path.match(pattern)?.[1] @@ -81,10 +91,13 @@ function findLocalFile(event: H3Event, ctx: DevServerContext) { } } - // Try to match theme asset files first and fallback to theme extension asset files + // Try to match theme asset files first and fallback to theme extension asset files. + // When neither matches (e.g. `/cdn/extensions///assets/` for an + // installed third-party app), the request falls through to `getProxyHandler` which + // forwards to cdn.shopify.com. return ( - tryGetFile(/^(?:\/cdn\/.*?)?\/assets\/([^?]+)/, ctx.localThemeFileSystem) ?? - tryGetFile(/^(?:\/ext\/cdn\/extensions\/.*?)?\/assets\/([^?]+)/, ctx.localThemeExtensionFileSystem) ?? { + tryGetFile(THEME_ASSET_PATTERN, ctx.localThemeFileSystem) ?? + tryGetFile(THEME_EXTENSION_ASSET_PATTERN, ctx.localThemeExtensionFileSystem) ?? { isUnsynced: false, fileKey: undefined, file: undefined,