Resolve symlinks in asset path guard so symlinked-out dirs are caught

Author: isaacroldan

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

@@ -1,30 +1,90 @@
 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', () => {
-    expect(() =>
-      assertPathWithinAppDir('/app/extensions/ext-a/icon.png', '/app', 'extensions/ext-a/icon.png'),
-    ).not.toThrow()
+  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', () => {
-    expect(() => assertPathWithinAppDir('/app', '/app', '.')).not.toThrow()
+  test('allows the app directory itself', async () => {
+    await inTemporaryDirectory(async (appDir) => {
+      await expect(assertPathWithinAppDir(appDir, appDir, '.')).resolves.toBeUndefined()
+    })
   })
 
-  test('rejects a relative path that escapes the app directory via ..', () => {
-    expect(() => assertPathWithinAppDir('/other/secret.env', '/app', '../other/secret.env')).toThrow(AbortError)
-    expect(() => assertPathWithinAppDir('/other/secret.env', '/app', '../other/secret.env')).toThrow(
-      /resolves outside the app directory/,
-    )
+  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 an absolute path that points outside the app directory', () => {
-    expect(() => assertPathWithinAppDir('/Users/me', '/app', '/Users/me')).toThrow(AbortError)
+  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('includes the original config value in the error message for debuggability', () => {
-    expect(() => assertPathWithinAppDir('/other', '/app', '~/anywhere')).toThrow(/Asset path '~\/anywhere'/)
+  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

@@ -1,23 +1,33 @@
 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`.
+ * 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 (e.g. `source = "../../"` or an absolute home
- * directory path). Adversarial bypass via symlinks is out of scope — server-side
- * size enforcement is the real boundary; this check is a fast-fail for DX.
+ * 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.
+ * so the user can locate the offending entry. Caller must ensure `resolvedPath`
+ * exists on disk — `fileRealPath` throws on missing paths.
  */
-export function assertPathWithinAppDir(resolvedPath: string, appDirectory: string, configValue: string): void {
-  const relative = relativePath(appDirectory, resolvedPath)
+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)
   if (relative.startsWith('..') || isAbsolutePath(relative)) {
     throw new AbortError(
       `Asset path '${configValue}' resolves outside the app directory.`,
-      `Asset sources must be inside the app folder. Resolved to: ${resolvedPath}`,
+      `Asset sources must be inside the app folder. Resolved to: ${realSource}`,
     )
   }
 }

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

@@ -17,7 +17,6 @@ export async function copyByPattern(
   options: {stdout: NodeJS.WritableStream},
 ): Promise<{filesCopied: number; outputPaths: string[]}> {
   const {sourceDir, outputDir, patterns, ignore, appDirectory, sourceDirConfigValue} = config
-  assertPathWithinAppDir(sourceDir, appDirectory, sourceDirConfigValue)
   const files = await glob(patterns, {
     absolute: true,
     cwd: sourceDir,
@@ -27,6 +26,9 @@ export async function copyByPattern(
   if (files.length === 0) {
     return {filesCopied: 0, outputPaths: []}
   }
+  // Realpath on sourceDir would throw on a missing dir; we only get here when
+  // glob found files, so sourceDir exists.
+  await assertPathWithinAppDir(sourceDir, appDirectory, sourceDirConfigValue)
 
   await mkdir(outputDir)
 

packages/app/src/cli/services/build/steps/include-assets/copy-config-key-entry.ts

@@ -69,14 +69,14 @@ export async function copyConfigKeyEntry(config: {
   /* eslint-disable no-await-in-loop */
   for (const sourcePath of uniquePaths) {
     const fullPath = joinPath(baseDir, sourcePath)
-    assertPathWithinAppDir(fullPath, appDirectory, sourcePath)
     const exists = await fileExists(fullPath)
     if (!exists) {
       throw new Error(
         outputContent`Couldn't find ${outputToken.path(fullPath)}\n  Please check the path '${sourcePath}' in your configuration`
           .value,
       )
     }
+    await assertPathWithinAppDir(fullPath, appDirectory, sourcePath)
 
     const sourceIsDir = await isDirectory(fullPath)
 

packages/app/src/cli/services/build/steps/include-assets/copy-source-entry.ts

@@ -20,10 +20,10 @@ export async function copySourceEntry(
 ): Promise<{filesCopied: number; outputPaths: string[]}> {
   const {source, destination, baseDir, outputDir, appDirectory} = config
   const sourcePath = joinPath(baseDir, source)
-  assertPathWithinAppDir(sourcePath, appDirectory, source)
   if (!(await fileExists(sourcePath))) {
     throw new Error(`Source does not exist: ${sourcePath}`)
   }
+  await assertPathWithinAppDir(sourcePath, appDirectory, source)
 
   const sourceIsDir = await isDirectory(sourcePath)