Merge pull request #4494 from Shopify/fd-fix-inject-proxy

[Theme] Fix serving local assets that contain non-printable characters

.changeset/smart-pots-drive.md

@@ -0,0 +1,5 @@
+---
+'@shopify/theme': patch
+---
+
+Fix serving local assets that contain non-printable characters.

packages/theme/src/cli/utilities/theme-environment/local-assets.ts

@@ -44,7 +44,15 @@ export function getAssetsHandler(_theme: Theme, ctx: DevServerContext) {
       return
     }
 
-    const fileContent = file.value ? injectCdnProxy(file.value, ctx) : Buffer.from(file.attachment ?? '', 'base64')
+    // Normalize the file content to a Buffer:
+    // - For attachments, we need to decode the base64 string.
+    // - For normal files, we need to get the real length of
+    //   the file using Buffer to avoid issues with non-breaking
+    //   spaces (NBSP, Unicode U+00A0), which have a different
+    //   byte length than their visual representation.
+    const fileContent = file.value
+      ? Buffer.from(injectCdnProxy(file.value, ctx))
+      : Buffer.from(file.attachment ?? '', 'base64')
 
     return serveStatic(event, {
       getContents: () => fileContent,
@@ -72,8 +80,8 @@ function findLocalFile(event: H3Event<EventHandlerRequest>, ctx: DevServerContex
 
   // Try to match theme asset files first and fallback to theme extension asset files
   return (
-    tryGetFile(/^\/cdn\/.*?\/assets\/([^?]+)(\?|$)/, ctx.localThemeFileSystem) ??
-    tryGetFile(/^\/ext\/cdn\/extensions\/.*?\/.*?\/assets\/([^?]+)(\?|$)/, ctx.localThemeExtensionFileSystem) ?? {
+    tryGetFile(/^\/cdn\/.*?\/assets\/([^?]+)/, ctx.localThemeFileSystem) ??
+    tryGetFile(/^\/ext\/cdn\/extensions\/.*?\/assets\/([^?]+)/, ctx.localThemeExtensionFileSystem) ?? {
       isUnsynced: false,
       fileKey: undefined,
       file: undefined,

packages/theme/src/cli/utilities/theme-environment/proxy.test.ts

@@ -87,6 +87,26 @@ describe('dev proxy', () => {
       `)
     })
 
+    test('proxies urls in JS files', () => {
+      const content = `
+        console.log('https://cdn.shopify.com/path/to/assets/file1');
+        // Comment: https://cdn.shopify.com/path/to/assets/file1 something
+        const url = "https://cdn.shopify.com/path/to/assets/file1#zzz";
+        fetch(\`https://cdn.shopify.com/path/to/assets/file1?q=123\`);
+      `
+
+      expect(injectCdnProxy(content, ctx)).toMatchInlineSnapshot(
+        `
+        "
+                console.log('/cdn/path/to/assets/file1');
+                // Comment: /cdn/path/to/assets/file1 something
+                const url = \\"/cdn/path/to/assets/file1#zzz\\";
+                fetch(\`/cdn/path/to/assets/file1?q=123\`);
+              "
+      `,
+      )
+    })
+
     test('proxies urls in Link header', () => {
       const linkHeader =
         `<https://cdn.shopify.com>; rel="preconnect", <https://cdn.shopify.com>; rel="preconnect"; crossorigin,` +

packages/theme/src/cli/utilities/theme-environment/proxy.ts

@@ -98,8 +98,8 @@ export function injectCdnProxy(originalContent: string, ctx: DevServerContext) {
   content = content.replace(vanityCdnRE, VANITY_CDN_PREFIX)
 
   // -- Only redirect usages of the main CDN for known local theme and theme extension assets to the local server:
-  const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?">]+)(?:\?|"|>|$))/g
-  const filterAssets = (key: string) => key.startsWith('assets')
+  const mainCdnRE = /(?:https?:)?\/\/cdn\.shopify\.com\/(.*?\/(assets\/[^?#"'`>\s]+))/g
+  const filterAssets = (key: string) => key.startsWith('assets/')
   const existingAssets = new Set([...ctx.localThemeFileSystem.files.keys()].filter(filterAssets))
   const existingExtAssets = new Set([...ctx.localThemeExtensionFileSystem.files.keys()].filter(filterAssets))
 

packages/theme/src/cli/utilities/theme-environment/theme-environment.test.ts

@@ -56,6 +56,15 @@ describe('setupDevServer', () => {
         value: '.another-class {}',
       },
     ],
+    [
+      'assets/file-with-nbsp.js',
+      {
+        checksum: '1',
+        key: 'assets/file-with-nbsp.js',
+        // Contains a non-breaking space
+        value: 'const x = "Hello\u00A0World";',
+      },
+    ],
   ])
 
   const localThemeFileSystem = fakeThemeFileSystem('tmp', localFiles)
@@ -165,10 +174,10 @@ describe('setupDevServer', () => {
     const dispatchEvent = async (
       url: string,
       headers?: {[key: string]: string},
-    ): Promise<{res: ServerResponse<IncomingMessage>; status: number; body?: string}> => {
+    ): Promise<{res: ServerResponse<IncomingMessage>; status: number; body: string | Buffer}> => {
       const event = createH3Event({url, headers})
       const {res} = event.node
-      let body: string | undefined
+      let body: string | Buffer | undefined
       const resWrite = res.write.bind(res)
       res.write = (chunk) => {
         body ??= ''
@@ -182,7 +191,7 @@ describe('setupDevServer', () => {
       }
 
       await server.dispatchEvent(event)
-      return {res, status: res.statusCode, body}
+      return {res, status: res.statusCode, body: body!}
     }
 
     test('mocks known endpoints', async () => {
@@ -200,7 +209,25 @@ describe('setupDevServer', () => {
       const {res, body} = await eventPromise
       expect(res.getHeader('content-type')).toEqual('text/css')
       // The URL is proxied:
-      expect(body).toMatchInlineSnapshot(`".some-class { background: url(\\"/cdn/path/to/assets/file2.css\\") }"`)
+      expect(body.toString()).toMatchInlineSnapshot(
+        `".some-class { background: url(\\"/cdn/path/to/assets/file2.css\\") }"`,
+      )
+    })
+
+    test('gets the right content for assets with non-breaking spaces', async () => {
+      const eventPromise = dispatchEvent('/cdn/somepathhere/assets/file-with-nbsp.js')
+      await expect(eventPromise).resolves.not.toThrow()
+
+      expect(vi.mocked(render)).not.toHaveBeenCalled()
+
+      const {res, body: bodyBuffer} = await eventPromise
+      const bodyString = bodyBuffer?.toString() ?? ''
+      expect(bodyString).toMatchInlineSnapshot(`"const x = \\"Hello\u00A0World\\";"`)
+
+      // Ensure content-length contains the real length:
+      expect(bodyString.length).toEqual(24)
+      expect(bodyBuffer.length).toEqual(25)
+      expect(res.getHeader('content-length')).toEqual(25)
     })
 
     test('renders HTML', async () => {