Merge pull request #8767 from Shopify/app-security/extension-directories

Scan extension and web directories outside the app directory

Author: jek

packages/app/src/cli/commands/app/security/check.integration.test.ts

@@ -1,5 +1,6 @@
 import SecurityCheck from './check.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
+import {deterministicFindingsDocumentSchema} from '../../../services/app-security-engine/results/schema.js'
 import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
 import {Config} from '@oclif/core'
 import {fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
@@ -125,6 +126,48 @@ describe('app security check command boundary', () => {
     })
   })
 
+  test('scans an extension directory that the TOML adds outside the app directory and records it in coverage', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appRoot = joinPath(directory, 'app')
+      await createApp(appRoot)
+      await writeFile(
+        joinPath(appRoot, 'shopify.app.toml'),
+        `extension_directories = ["../shared/*"]\n${validAppConfiguration()}`,
+      )
+      await mkdir(joinPath(directory, 'shared', 'theme', 'blocks'), {recursive: true})
+      await writeFile(
+        joinPath(directory, 'shared', 'theme', 'shopify.extension.toml'),
+        'name = "theme"\ntype = "theme"\n',
+      )
+      await writeFile(
+        joinPath(directory, 'shared', 'theme', 'blocks', 'banner.liquid'),
+        '<p>{{ block.settings.text }}</p>\n',
+      )
+      const appDirectory = await fileRealPath(appRoot)
+      const sharedTheme = await fileRealPath(joinPath(directory, 'shared', 'theme'))
+
+      const result = await runCommand(['--path', appRoot, '--json', '--skip-instructions'])
+
+      expect(result.exitCode).toBe(0)
+      const output = JSON.parse(result.stdout)
+      expect(output.selection.scan_directories).toEqual([
+        {directory: appDirectory, origin: 'app_directory'},
+        {directory: sharedTheme, origin: 'app_config_directory'},
+      ])
+      const deterministicFindings = await readJson(
+        appSecurityArtifactPaths(appDirectory, 'shopify.app').deterministicFindingsPath,
+      )
+      // `review` reads the results back through this schema.
+      expect(deterministicFindingsDocumentSchema.parse(deterministicFindings).coverage).toMatchObject({
+        scope: {include_dirs: [], excludes: [], no_git_ignore: false},
+        scan_directories: [
+          {directory: '.', origin: 'app_directory'},
+          {directory: '../shared/theme', origin: 'app_config_directory'},
+        ],
+      })
+    })
+  })
+
   test('writes the results under the --client-id results key and creates .shopify/.gitignore', async () => {
     await inTemporaryDirectory(async (directory) => {
       await createApp(directory)

packages/app/src/cli/services/app-context.ts

@@ -179,13 +179,14 @@ async function logMetadata(app: {apiKey: string}, organization: Organization, re
 interface LocalAppContextOutput {
   app: AppInterface
   project: Project
+  activeConfig: ActiveConfig
 }
 
 /**
  * This function loads an app locally without making any network calls.
  * It uses local specifications and doesn't require the app to be linked.
  *
- * @returns The local app and project instances.
+ * @returns The local app and project instances, and the selected app configuration.
  */
 export async function localAppContext({
   directory,
@@ -205,5 +206,5 @@ export async function localAppContext({
     throw new AbortError(styledConfigurationError(app.errors.getErrors()[0]!))
   }
 
-  return {app, project}
+  return {app, project, activeConfig}
 }

packages/app/src/cli/services/app-security-engine/results/schema.ts

@@ -124,7 +124,7 @@ export const coverageSchema = zod.object({
   ),
   scope: createScopeSchema(),
   scan_directories: zod.array(
-    zod.object({directory: zod.string(), origin: zod.enum(['app_directory', 'include_dir'])}),
+    zod.object({directory: zod.string(), origin: zod.enum(['app_directory', 'include_dir', 'app_config_directory'])}),
   ),
 })
 

packages/app/src/cli/services/app-security-engine/run.ts

@@ -29,14 +29,27 @@ export function getAgentInstructions(): string {
   return EMBEDDED_APP_SECURITY_INSTRUCTIONS
 }
 
-/** A scan directory equal to the app directory is the app directory itself, however it was requested. */
-function coverageScanDirectories({appDirectory, scanDirectories}: ScanInput): CoverageScanDirectory[] {
+function coverageScanDirectories({
+  appDirectory,
+  scanDirectories,
+  appConfigDirectories = [],
+}: ScanInput): CoverageScanDirectory[] {
   return scanDirectories.map((directory) => ({
     directory: normalizePath(relativePath(appDirectory, directory)) || '.',
-    origin: directory === appDirectory ? 'app_directory' : 'include_dir',
+    origin: coverageOrigin(directory, appDirectory, appConfigDirectories),
   }))
 }
 
+/** A scan directory equal to the app directory is the app directory itself, however it was requested. */
+function coverageOrigin(
+  directory: string,
+  appDirectory: string,
+  appConfigDirectories: ReadonlyArray<string>,
+): CoverageScanDirectory['origin'] {
+  if (directory === appDirectory) return 'app_directory'
+  return appConfigDirectories.includes(directory) ? 'app_config_directory' : 'include_dir'
+}
+
 export async function scanApp(input: ScanInput, options: ScanOptions = {}): Promise<AppSecurityScan> {
   const {ignoredScanDirectories, otherAppDirectories, ...result} = await scan(input, options)
   const engineVersion = getEngineVersion()

packages/app/src/cli/services/app-security-engine/scanners/discover.ts

@@ -387,7 +387,8 @@ function groupSourcePathsByExtensionDirectory(
  * `extension_directories`. The app security check still scans every `shopify.extension.toml`
  * inside the repository boundary, including unconfigured extensions, because
  * those files can still contain secrets, XSS, and other security evidence.
- * Extensions in a nested app or an `--include-dir` directory count as this app's.
+ * Extensions in a nested app, an `--include-dir` directory, or a directory that the selected app configuration
+ * file's `extension_directories` or `web_directories` add count as this app's.
  */
 export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray<string>): ExtensionInfo[] {
   const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml')

packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts

@@ -33,10 +33,11 @@ interface CheckFlags {
   json?: boolean
 }
 
-function toml(path: string, clientId = 'client-app'): FileSpec {
+/** `topLevelKeys` come first, since top-level keys must precede tables. */
+function toml(path: string, clientId = 'client-app', topLevelKeys = ''): FileSpec {
   return [
     path,
-    `name = "test-app"
+    `${topLevelKeys}name = "test-app"
 client_id = "${clientId}"
 application_url = "https://example.com"
 embedded = true
@@ -943,4 +944,43 @@ describe('layout catalogue: check --list-files', () => {
       })
     })
   })
+
+  test('28. extension-and-web-directories-outside-the-app', async () => {
+    const layout: Layout = {
+      repositories: ['monorepo'],
+      files: [
+        toml(
+          'monorepo/app/shopify.app.toml',
+          'client-app',
+          'extension_directories = ["../shared-extensions/*"]\nweb_directories = ["../backend"]\n',
+        ),
+        'monorepo/app/app/routes/index.tsx',
+        ['monorepo/shared-extensions/theme/shopify.extension.toml', 'name = "theme"\ntype = "theme"\n'],
+        'monorepo/shared-extensions/theme/blocks/banner.liquid',
+        'monorepo/shared-extensions/README.md',
+        ['monorepo/backend/shopify.web.toml', 'name = "backend"\nroles = ["backend"]\n\n[commands]\ndev = "dev"\n'],
+        'monorepo/backend/src/server.ts',
+      ],
+    }
+    await inLayout(layout, async (root) => {
+      const {resolution, stdout, stderr} = await checkListFiles(root, 'monorepo/app')
+
+      expectConfigSelection(resolution, {
+        appDirectory: join(root, 'monorepo/app'),
+        tomlFileName: 'shopify.app.toml',
+        resultsKey: 'shopify.app',
+      })
+      // Each matched TOML's directory is scanned, not every directory the glob starts from.
+      expect(listedPaths(stdout)).toEqual([
+        '../backend/shopify.web.toml',
+        '../backend/src/server.ts',
+        '../shared-extensions/theme/blocks/banner.liquid',
+        '../shared-extensions/theme/shopify.extension.toml',
+        'app/routes/index.tsx',
+        'shopify.app.toml',
+      ])
+      expect(warningText(stderr)).not.toContain("another app's configuration")
+      expect(resolution.commands.scan.args).toEqual(checkArgs())
+    })
+  })
 })

packages/app/src/cli/services/app-security-engine/types.ts

@@ -82,6 +82,8 @@ export interface ScanInput {
   requestedScanDirectories: ReadonlyArray<string>
   /** Absolute path of the selected app configuration file. Absent when scanning without app configuration. */
   appConfigFilePath?: string
+  /** The scan directories that the selected app configuration file's `extension_directories` and `web_directories` add. */
+  appConfigDirectories?: ReadonlyArray<string>
   clientId?: string
 }
 
@@ -95,7 +97,7 @@ export interface AppSecurityScope {
 /** A scan directory as recorded in coverage: relative to the app directory, `.` for the app directory itself. */
 export interface CoverageScanDirectory {
   directory: string
-  origin: 'app_directory' | 'include_dir'
+  origin: 'app_directory' | 'include_dir' | 'app_config_directory'
 }
 
 export interface ScanOptions {

packages/app/src/cli/services/app-security-selection.test.ts

@@ -17,7 +17,7 @@ import {fetchOrCreateOrganizationApp} from './context.js'
 import use from './app/config/use.js'
 import {AbortError} from '@shopify/cli-kit/node/error'
 import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
-import {joinPath} from '@shopify/cli-kit/node/path'
+import {basename, joinPath} from '@shopify/cli-kit/node/path'
 import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui'
 import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
 import {symlink} from 'node:fs/promises'
@@ -77,6 +77,7 @@ describe('resolveAppSecuritySelection with an app configuration', () => {
         kind: 'config',
         appDirectory,
         appConfigFilePath: joinPath(appDirectory, 'shopify.app.toml'),
+        appConfigDirectories: [],
         configClientId: 'default-client-id',
         clientIdOverride: undefined,
       })
@@ -326,6 +327,124 @@ describe('resolveAppSecuritySelection with several TOMLs and none selected', ()
   })
 })
 
+/** A TOML whose `extension_directories` and `web_directories` come first, since top-level keys must precede tables. */
+async function writeConfigurationWithDirectories(
+  appDirectory: string,
+  directories: {extension_directories?: string[]; web_directories?: string[]},
+  name = 'shopify.app.toml',
+): Promise<void> {
+  const keys = Object.entries(directories).map(([key, entries]) => `${key} = ${JSON.stringify(entries)}\n`)
+  await mkdir(appDirectory)
+  await writeFile(joinPath(appDirectory, name), `${keys.join('')}${validAppConfiguration()}`)
+}
+
+/** A theme extension named after its directory, since extension handles must be unique. */
+async function writeThemeExtension(directory: string): Promise<void> {
+  await mkdir(directory)
+  await writeFile(joinPath(directory, 'shopify.extension.toml'), `name = "${basename(directory)}"\ntype = "theme"\n`)
+}
+
+async function writeBackendWeb(directory: string): Promise<void> {
+  await mkdir(directory)
+  await writeFile(
+    joinPath(directory, 'shopify.web.toml'),
+    'name = "backend"\nroles = ["backend"]\n\n[commands]\ndev = "dev"\n',
+  )
+}
+
+describe('resolveAppSecuritySelection app configuration directories', () => {
+  test('are the directories of the extension and web TOMLs that relative entries match outside the app directory', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = joinPath(directory, 'app')
+      await writeConfigurationWithDirectories(appDirectory, {
+        extension_directories: ['extensions/*', '../shared/*', '../no-extensions/*'],
+        web_directories: ['../backend'],
+      })
+      await writeThemeExtension(joinPath(appDirectory, 'extensions', 'inside'))
+      await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
+      await writeFile(joinPath(directory, 'shared', 'README.md'), 'Shared extensions\n')
+      await mkdir(joinPath(directory, 'no-extensions', 'empty'))
+      await writeBackendWeb(joinPath(directory, 'backend'))
+
+      const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
+
+      expect(selection).toMatchObject({
+        appConfigDirectories: [
+          await fileRealPath(joinPath(directory, 'backend')),
+          await fileRealPath(joinPath(directory, 'shared', 'theme')),
+        ],
+      })
+    })
+  })
+
+  test('ignore absolute entries, the way the CLI does', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = joinPath(directory, 'app')
+      await writeConfigurationWithDirectories(appDirectory, {
+        extension_directories: [joinPath(directory, 'shared', '*')],
+        web_directories: [joinPath(directory, 'backend')],
+      })
+      await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
+      await writeBackendWeb(joinPath(directory, 'backend'))
+
+      const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
+
+      expect(selection).toMatchObject({appConfigDirectories: []})
+    })
+  })
+
+  test('leave out a directory reached through a symbolic link outside the app directory', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = joinPath(directory, 'app')
+      await writeConfigurationWithDirectories(appDirectory, {extension_directories: ['../linked-shared/*']})
+      await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
+      await symlink(joinPath(directory, 'shared'), joinPath(directory, 'linked-shared'), 'dir')
+
+      const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
+
+      expect(selection).toMatchObject({appConfigDirectories: []})
+    })
+  })
+
+  test('leave out an extension directory inside the app directory that links outside it', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = joinPath(directory, 'app')
+      await writeConfigurationWithDirectories(appDirectory, {extension_directories: ['extensions/*']})
+      await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
+      await mkdir(joinPath(appDirectory, 'extensions'))
+      await symlink(joinPath(directory, 'shared', 'theme'), joinPath(appDirectory, 'extensions', 'theme'), 'dir')
+
+      const selection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
+
+      expect(selection).toMatchObject({appConfigDirectories: []})
+    })
+  })
+
+  test('come from the selected TOML only', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = joinPath(directory, 'app')
+      await writeConfigurationWithDirectories(appDirectory, {})
+      await writeFile(
+        joinPath(appDirectory, 'shopify.app.staging.toml'),
+        `extension_directories = ["../shared/*"]\n${validAppConfiguration('staging-client-id')}`,
+      )
+      await writeThemeExtension(joinPath(directory, 'shared', 'theme'))
+
+      const defaultSelection = await resolveAppSecuritySelection({path: appDirectory, allowPrompts: false})
+      const stagingSelection = await resolveAppSecuritySelection({
+        path: appDirectory,
+        config: 'staging',
+        allowPrompts: false,
+      })
+
+      expect(defaultSelection).toMatchObject({appConfigDirectories: []})
+      expect(stagingSelection).toMatchObject({
+        appConfigDirectories: [await fileRealPath(joinPath(directory, 'shared', 'theme'))],
+      })
+    })
+  })
+})
+
 describe('resolveAppSecuritySelection with --without-app-config', () => {
   test('aborts without --client-id', async () => {
     await inTemporaryDirectory(async (directory) => {
@@ -773,4 +892,35 @@ describe('mergeScanDirectories', () => {
   test('accepts an include directory elsewhere on the same Windows drive', () => {
     expect(mergeScanDirectories('C:/work/app', ['C:/backend']).scanDirectories).toHaveLength(2)
   })
+
+  test('lists each app configuration directory after the include directories', () => {
+    expect(mergeScanDirectories('/work/app', ['/work/backend'], ['/work/shared/theme'])).toEqual({
+      scanDirectories: [
+        {directory: '/work/app', origin: 'app_directory'},
+        {directory: '/work/backend', origin: 'include_dir'},
+        {directory: '/work/shared/theme', origin: 'app_config_directory'},
+      ],
+      requestedScanDirectories: ['/work/app', '/work/backend', '/work/shared/theme'],
+    })
+  })
+
+  test('counts an app configuration directory that is also an include directory as the include directory', () => {
+    expect(mergeScanDirectories('/work/app', ['/work/shared/theme'], ['/work/shared/theme']).scanDirectories).toEqual([
+      {directory: '/work/app', origin: 'app_directory'},
+      {directory: '/work/shared/theme', origin: 'include_dir'},
+    ])
+  })
+
+  test('drops an app configuration directory inside an include directory', () => {
+    expect(mergeScanDirectories('/work/app', ['/work/shared'], ['/work/shared/theme']).scanDirectories).toEqual([
+      {directory: '/work/app', origin: 'app_directory'},
+      {directory: '/work/shared', origin: 'include_dir'},
+    ])
+  })
+
+  test('rejects an app configuration directory on another Windows drive', () => {
+    expect(() => mergeScanDirectories('C:/work/app', [], ['D:/shared/theme'])).toThrowError(
+      new AbortError('Extension or web directory D:/shared/theme: must be on the same drive as the app directory.'),
+    )
+  })
 })