Fix theme dev serving theme asset for /cdn/extensions/ requests#7674
Open
wes-shaw wants to merge 1 commit into
Open
Fix theme dev serving theme asset for /cdn/extensions/ requests#7674wes-shaw wants to merge 1 commit into
wes-shaw wants to merge 1 commit into
Conversation
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/<uuid>/<app>/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/<uuid>/<app>` 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/<uuid>/<app>/assets/<name>` → proxied to CDN (or served from `localThemeExtensionFileSystem` if locally developed and present there). - `/cdn/shop/t/<id>/assets/<name>` → 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 <wes.shaw@shopify.com> Co-authored-by: Wes Shaw <wes.shaw@shopify.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a bug where shopify theme dev would serve a local theme asset in response to an app extension's /cdn/extensions/<uuid>/<app>/assets/<name> request when the theme contained a same-named file. The overly-permissive theme-asset regex was capturing extension CDN paths and returning the wrong file.
Changes:
- Tighten
THEME_ASSET_PATTERNwith a negative lookahead so/cdn/cannot be followed byextensions/. - Broaden
THEME_EXTENSION_ASSET_PATTERNto also match storefront-emitted/cdn/extensions/...URLs (in addition to/ext/cdn/extensions/...). - Export
findLocalFileand add new unit tests covering bare, vanity-CDN, extension, and collision cases.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/theme/src/cli/utilities/theme-environment/local-assets.ts | Replace inline regex literals with named, documented patterns; export findLocalFile. |
| packages/theme/src/cli/utilities/theme-environment/local-assets.test.ts | New tests for findLocalFile, including the regression case. |
| .changeset/river-theme-dev-cdn-extensions-collision.md | Patch-level changeset describing the fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
shopify theme devwas serving a theme asset in response to an app extension's asset request (/cdn/extensions/<uuid>/<app>/assets/<name>) whenever the theme had a same-named file. The colliding script then loaded twice (once as the theme asset, once as the extension asset that was silently swapped), causing duplicated execution — e.g. double-bound event handlers. Reported in this community thread (Klaviyo Email Marketing'sapp.jsvs a themeapp.js).The live storefront serves both URLs correctly; this was local-dev only. Reproduces on
mainand on@shopify/cli4.1.0.Root cause
In
packages/theme/src/cli/utilities/theme-environment/local-assets.ts,findLocalFiletried the theme matcher first via??:The theme matcher's optional
(?:\/cdn\/.*?)?prefix is greedy enough to swallow the entire/cdn/extensions/<uuid>/<app>segment, capturing only the basename after/assets/.tryGetFilederives the lookup key from the capture alone (joinPath('assets', matchedFileName)), discarding the URL path, so/cdn/extensions/.../assets/app.jsresolves toassets/app.jsinlocalThemeFileSystemand is served.The extension matcher only accepts
/ext/cdn/extensions/(note/ext/), so it never gets a chance for a bare/cdn/extensions/request, and the??short-circuits beforegetProxyHandler(which already forwards extension CDN traffic tocdn.shopify.com) is ever reached.Fix
Tighten the theme regex with a negative lookahead so its
/cdn/prefix cannot be followed byextensions/, 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,findLocalFilereturns{fileKey: undefined}andgetAssetsHandlerreturns early — the request falls through togetProxyHandlerwhich forwards tocdn.shopify.comand the real installed-app asset is returned.Behaviour after fix
/cdn/extensions/<uuid>/<app>/assets/<name>→ proxied to CDN, or served fromlocalThemeExtensionFileSystemwhen present (covers theme-app-extension dev against the vanity CDN form)./cdn/shop/t/<id>/assets/<name>→ still served fromlocalThemeFileSystem./ext/cdn/extensions/...→ still served fromlocalThemeExtensionFileSystem./assets/<name>→ unchanged.Test coverage
New
packages/theme/src/cli/utilities/theme-environment/local-assets.test.tscovers six cases forfindLocalFile, including the regression (a themeassets/app.jsmust not be returned for a/cdn/extensions/.../assets/app.jsrequest) and the locally-developed-extension precedence case. Confirmed locally: with the regex change reverted, exactly the 2 collision tests fail; with the fix applied, all 6 new tests pass and the fullpackages/themetest suite (163 tests) still passes, along withnx type-check themeandnx lint theme.Tophatting
In a project that uses
shopify theme dev:assets/app.js.assets/app.js, with distinct contents (e.g.console.log('THEME')vs the app's content).shopify theme devand load a page that renders both:<script src="/cdn/shop/t/<id>/assets/app.js?v=...">content_for_header):<script src="/cdn/extensions/<uuid>/<app>/assets/app.js">curl http://localhost:9292/cdn/extensions/<uuid>/<app>/assets/app.js— before the fix this returns the theme'sapp.js; after the fix it returns the real app'sapp.js(proxied fromcdn.shopify.com).curl http://localhost:9292/cdn/shop/t/<id>/assets/app.jsstill returns the local theme'sapp.js.Notes
main; 4.1.0 line numbers differ slightly but the logic is identical..github/CODEOWNERS,packages/theme/**is owned by@shopify/developer-platformsand@shopify/dev_experience.Requested by Wes Shaw wes.shaw@shopify.com