Merge pull request #7542 from Shopify/05-13-cli_bundle_guard_size_and_path

Guard CLI bundle upload against oversized and out-of-app paths

Author: isaacroldan

.changeset/guard-bundle-upload-size-and-paths.md

@@ -0,0 +1,5 @@
+---
+'@shopify/app': patch
+---
+
+Guard app bundle uploads against oversized bundles and asset paths resolving outside the app directory

packages/app/src/cli/services/build/steps/include-assets-step.test.ts

@@ -25,7 +25,7 @@ describe('executeIncludeAssetsStep', () => {
       options: {
         stdout: mockStdout,
         stderr: mockStderr,
-        app: {} as any,
+        app: {directory: '/test'} as any,
         environment: 'production',
       },
       stepResults: new Map(),
@@ -552,6 +552,11 @@ describe('executeIncludeAssetsStep', () => {
   })
 
   describe('pattern entries', () => {
+    beforeEach(() => {
+      // copyByPattern now short-circuits if sourceDir doesn't exist; default true here.
+      vi.mocked(fs.fileExists).mockResolvedValue(true)
+    })
+
     test('copies files matching include patterns', async () => {
       // Given
       vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png', '/test/extension/public/style.css'])
@@ -943,11 +948,13 @@ describe('executeIncludeAssetsStep', () => {
     })
 
     test('writes manifest.json with files array when generatesAssetsManifest is true and only pattern inclusions exist', async () => {
-      // Given — pattern entries contribute output paths to the manifest "files" array
+      // Given — pattern entries contribute output paths to the manifest "files" array.
+      // sourceDir must exist for copyByPattern's pre-glob fileExists check to pass;
+      // everything else can read false (the parent beforeEach default).
       vi.mocked(fs.glob).mockResolvedValue(['/test/extension/public/logo.png'])
       vi.mocked(fs.copyFile).mockResolvedValue()
       vi.mocked(fs.mkdir).mockResolvedValue()
-      vi.mocked(fs.fileExists).mockResolvedValue(false)
+      vi.mocked(fs.fileExists).mockImplementation(async (path) => String(path) === '/test/extension/public')
       vi.mocked(fs.writeFile).mockResolvedValue()
 
       const step: LifecycleStep = {

packages/app/src/cli/services/build/steps/include-assets-step.ts

@@ -125,6 +125,7 @@ export async function executeIncludeAssetsStep(
   const config = IncludeAssetsConfigSchema.parse(step.config)
   const {extension, options} = context
   const outputDir = resolveOutputDir(extension.outputPath)
+  const appDirectory = options.app.directory
 
   const aggregatedPathMap = new Map<string, string | string[]>()
   // Track basenames written across all configKey entries in this build. In
@@ -147,6 +148,7 @@ export async function executeIncludeAssetsStep(
       baseDir: extension.directory,
       outputDir,
       context,
+      appDirectory,
       destination: sanitizedDest,
       usedBasenames,
       preserveFilePaths: entry.preserveFilePaths,
@@ -172,6 +174,8 @@ export async function executeIncludeAssetsStep(
               outputDir: destinationDir,
               patterns: entry.include,
               ignore: entry.ignore ?? [],
+              appDirectory,
+              sourceDirConfigValue: entry.baseDir ?? '.',
             },
             options,
           )
@@ -189,6 +193,7 @@ export async function executeIncludeAssetsStep(
               destination: sanitizedDest,
               baseDir: extension.directory,
               outputDir,
+              appDirectory,
             },
             options,
           )

packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.test.ts

@@ -0,0 +1,101 @@
+import {assertPathWithinAppDir} from './assert-path-within-app.js'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
+import {joinPath} from '@shopify/cli-kit/node/path'
+import {describe, expect, test} from 'vitest'
+import {symlink} from 'fs/promises'
+
+describe('assertPathWithinAppDir', () => {
+  test('allows a path inside the app directory', async () => {
+    await inTemporaryDirectory(async (appDir) => {
+      const inside = joinPath(appDir, 'extensions', 'ext-a')
+      await mkdir(inside)
+      await writeFile(joinPath(inside, 'icon.png'), 'x')
+      await expect(
+        assertPathWithinAppDir(joinPath(inside, 'icon.png'), appDir, 'extensions/ext-a/icon.png'),
+      ).resolves.toBeUndefined()
+    })
+  })
+
+  test('allows the app directory itself', async () => {
+    await inTemporaryDirectory(async (appDir) => {
+      await expect(assertPathWithinAppDir(appDir, appDir, '.')).resolves.toBeUndefined()
+    })
+  })
+
+  test('rejects a path that resolves outside the app directory via ..', async () => {
+    await inTemporaryDirectory(async (parent) => {
+      const appDir = joinPath(parent, 'app')
+      await mkdir(appDir)
+      const outside = joinPath(parent, 'outside.json')
+      await writeFile(outside, '{}')
+      await expect(assertPathWithinAppDir(outside, appDir, '../outside.json')).rejects.toThrow(AbortError)
+      await expect(assertPathWithinAppDir(outside, appDir, '../outside.json')).rejects.toThrow(
+        /resolves outside the app directory/,
+      )
+    })
+  })
+
+  test('rejects a symlink whose target is outside the app directory', async () => {
+    await inTemporaryDirectory(async (parent) => {
+      const appDir = joinPath(parent, 'app')
+      await mkdir(appDir)
+      const outsideDir = joinPath(parent, 'home', 'big-folder')
+      await mkdir(outsideDir)
+      await writeFile(joinPath(outsideDir, 'huge.bin'), 'x')
+
+      // Inside the app dir, but the symlink points outside.
+      const symlinkInApp = joinPath(appDir, 'assets')
+      await symlink(outsideDir, symlinkInApp)
+
+      await expect(assertPathWithinAppDir(symlinkInApp, appDir, 'assets')).rejects.toThrow(AbortError)
+    })
+  })
+
+  test('allows an in-tree symlink (e.g. pnpm-style links staying inside the app)', async () => {
+    await inTemporaryDirectory(async (appDir) => {
+      const realTarget = joinPath(appDir, 'shared')
+      await mkdir(realTarget)
+      const linkPath = joinPath(appDir, 'extensions', 'ext-a-assets')
+      await mkdir(joinPath(appDir, 'extensions'))
+      await symlink(realTarget, linkPath)
+
+      await expect(assertPathWithinAppDir(linkPath, appDir, 'extensions/ext-a-assets')).resolves.toBeUndefined()
+    })
+  })
+
+  test('does not false-positive on macOS-style symlinked temp dirs (both sides realpath’d)', async () => {
+    // inTemporaryDirectory on macOS returns a /var/folders/... path whose
+    // realpath is /private/var/folders/.... If only the source were realpath’d
+    // the check would treat the temp dir as outside itself.
+    await inTemporaryDirectory(async (appDir) => {
+      const inside = joinPath(appDir, 'src')
+      await mkdir(inside)
+      await writeFile(joinPath(inside, 'schema.json'), '{}')
+      await expect(
+        assertPathWithinAppDir(joinPath(inside, 'schema.json'), appDir, 'src/schema.json'),
+      ).resolves.toBeUndefined()
+    })
+  })
+
+  test('allows a sibling whose name starts with two dots (e.g. ..cache)', async () => {
+    await inTemporaryDirectory(async (appDir) => {
+      const dotdotDir = joinPath(appDir, '..cache')
+      await mkdir(dotdotDir)
+      await writeFile(joinPath(dotdotDir, 'file.txt'), 'x')
+      await expect(
+        assertPathWithinAppDir(joinPath(dotdotDir, 'file.txt'), appDir, '..cache/file.txt'),
+      ).resolves.toBeUndefined()
+    })
+  })
+
+  test('includes the original config value in the error for debuggability', async () => {
+    await inTemporaryDirectory(async (parent) => {
+      const appDir = joinPath(parent, 'app')
+      await mkdir(appDir)
+      const outside = joinPath(parent, 'leak')
+      await writeFile(outside, '')
+      await expect(assertPathWithinAppDir(outside, appDir, '~/anywhere')).rejects.toThrow(/Asset path '~\/anywhere'/)
+    })
+  })
+})

packages/app/src/cli/services/build/steps/include-assets/assert-path-within-app.ts

@@ -0,0 +1,37 @@
+import {relativePath, isAbsolutePath} from '@shopify/cli-kit/node/path'
+import {fileRealPath} from '@shopify/cli-kit/node/fs'
+import {AbortError} from '@shopify/cli-kit/node/error'
+
+/**
+ * Throws if `resolvedPath` is not inside `appDirectory` after symlink resolution.
+ *
+ * Guards against accidental misuse where an extension config points an asset
+ * source outside the app folder — either via `..`/absolute paths or by
+ * symlinking an in-app directory to somewhere else (e.g. a home directory).
+ *
+ * Both sides are realpath'd before comparison so macOS temp paths
+ * (`/var/folders` → `/private/var/folders`) and pnpm-style in-tree symlinks
+ * don't trip a false positive.
+ *
+ * `configValue` is the raw value from configuration used in the error message
+ * so the user can locate the offending entry. Caller must ensure `resolvedPath`
+ * exists on disk — `fileRealPath` throws on missing paths.
+ */
+export async function assertPathWithinAppDir(
+  resolvedPath: string,
+  appDirectory: string,
+  configValue: string,
+): Promise<void> {
+  const [realSource, realAppDir] = await Promise.all([fileRealPath(resolvedPath), fileRealPath(appDirectory)])
+  const relative = relativePath(realAppDir, realSource)
+  // Check `..` as a path segment, not just a prefix. A sibling-style name like
+  // `..cache` is a valid in-app entry whose relative path begins with `..` but
+  // does not escape.
+  const firstSegment = relative.split(/[/\\]/, 1)[0]
+  if (firstSegment === '..' || isAbsolutePath(relative)) {
+    throw new AbortError(
+      `Asset path '${configValue}' resolves outside the app directory.`,
+      `Asset sources must be inside the app folder. Resolved to: ${realSource}`,
+    )
+  }
+}

packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.test.ts

@@ -9,6 +9,7 @@ describe('copyByPattern', () => {
 
   beforeEach(() => {
     mockStdout = {write: vi.fn()}
+    vi.mocked(fs.fileExists).mockResolvedValue(true)
   })
 
   test('copies matched files preserving relative paths', async () => {
@@ -24,6 +25,8 @@ describe('copyByPattern', () => {
         outputDir: '/out',
         patterns: ['**/*.ts', '**/*.tsx'],
         ignore: [],
+        appDirectory: '/src',
+        sourceDirConfigValue: '.',
       },
       {stdout: mockStdout},
     )
@@ -47,6 +50,8 @@ describe('copyByPattern', () => {
         outputDir: '/out',
         patterns: ['**/*.png'],
         ignore: [],
+        appDirectory: '/src',
+        sourceDirConfigValue: '.',
       },
       {stdout: mockStdout},
     )
@@ -72,6 +77,8 @@ describe('copyByPattern', () => {
         outputDir: '/out/sub',
         patterns: ['**/*'],
         ignore: [],
+        appDirectory: '/out',
+        sourceDirConfigValue: 'sub',
       },
       {stdout: mockStdout},
     )
@@ -96,6 +103,8 @@ describe('copyByPattern', () => {
         outputDir: '/out',
         patterns: ['*.png'],
         ignore: [],
+        appDirectory: '/out',
+        sourceDirConfigValue: '.',
       },
       {stdout: mockStdout},
     )
@@ -120,6 +129,8 @@ describe('copyByPattern', () => {
         outputDir: '/out/dist',
         patterns: ['*.js'],
         ignore: [],
+        appDirectory: '/src',
+        sourceDirConfigValue: '.',
       },
       {stdout: mockStdout},
     )
@@ -139,6 +150,8 @@ describe('copyByPattern', () => {
         outputDir: '/out',
         patterns: ['**/*'],
         ignore: ['**/*.test.ts', 'node_modules/**'],
+        appDirectory: '/src',
+        sourceDirConfigValue: '.',
       },
       {stdout: mockStdout},
     )

packages/app/src/cli/services/build/steps/include-assets/copy-by-pattern.ts

@@ -1,5 +1,6 @@
+import {assertPathWithinAppDir} from './assert-path-within-app.js'
 import {joinPath, dirname, relativePath} from '@shopify/cli-kit/node/path'
-import {glob, copyFile, mkdir} from '@shopify/cli-kit/node/fs'
+import {glob, copyFile, mkdir, fileExists} from '@shopify/cli-kit/node/fs'
 
 /**
  * Pattern strategy: glob-based file selection.
@@ -10,10 +11,21 @@ export async function copyByPattern(
     outputDir: string
     patterns: string[]
     ignore: string[]
+    appDirectory: string
+    sourceDirConfigValue: string
   },
   options: {stdout: NodeJS.WritableStream},
 ): Promise<{filesCopied: number; outputPaths: string[]}> {
-  const {sourceDir, outputDir, patterns, ignore} = config
+  const {sourceDir, outputDir, patterns, ignore, appDirectory, sourceDirConfigValue} = config
+
+  // Validate the boundary up front, before touching the filesystem. Realpath
+  // would throw on a missing sourceDir, so preserve the existing "missing dir
+  // = no files" behavior by short-circuiting first.
+  if (!(await fileExists(sourceDir))) {
+    return {filesCopied: 0, outputPaths: []}
+  }
+  await assertPathWithinAppDir(sourceDir, appDirectory, sourceDirConfigValue)
+
   const files = await glob(patterns, {
     absolute: true,
     cwd: sourceDir,