Warn when App Security scans another app's files

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

@@ -18,6 +18,8 @@ export interface AppSecurityScan {
   scan: ScanResult
   /** Absolute paths of the scan directories that their repository ignores, so only the files Git tracks in them were scanned. */
   ignoredScanDirectories: string[]
+  /** Absolute paths of directories, other than the app directory, that hold an app configuration file, so another app's files were scanned as this app's. */
+  otherAppDirectories: string[]
   deterministicFindings: DeterministicFindingsDocument
   agentChecks: AgentChecks
   engine: AppSecurityEngineMetadata
@@ -36,7 +38,7 @@ function coverageScanDirectories({appDirectory, scanDirectories}: ScanInput): Co
 }
 
 export async function scanApp(input: ScanInput, options: ScanOptions = {}): Promise<AppSecurityScan> {
-  const {ignoredScanDirectories, ...result} = await scan(input, options)
+  const {ignoredScanDirectories, otherAppDirectories, ...result} = await scan(input, options)
   const engineVersion = getEngineVersion()
   const deterministicFindings = buildDeterministicFindings(result, {
     engineVersion,
@@ -50,6 +52,7 @@ export async function scanApp(input: ScanInput, options: ScanOptions = {}): Prom
   return {
     scan: result,
     ignoredScanDirectories,
+    otherAppDirectories,
     deterministicFindings,
     agentChecks: buildAgentChecks(engineVersion),
     engine: deterministicFindings.engine,

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

@@ -2,6 +2,7 @@ import {inspectErrorReason, isMissingFilesystemEntry} from './filesystem-errors.
 import {
   isDroppedEntry,
   isDroppedTrackedPath,
+  isExcludedScanDirectory,
   isIgnoredByParentRepository,
   listGitIgnoredPaths,
   listNestedRepository,
@@ -218,6 +219,8 @@ interface GatheredPaths {
   paths: string[]
   /** Requested directories that their repository ignores, so only the files Git tracks in them are gathered. */
   ignoredScanDirectories: string[]
+  /** Absolute paths of directories, other than the app directory, that hold a gathered app configuration file. */
+  otherAppDirectories: string[]
   /** How the first scan directory's ignored paths were found. Secret findings use it to explain why an ignored file was scanned. */
   listingStatus: GatheredListingStatus
 }
@@ -237,6 +240,8 @@ export async function gatherPaths({
   const ignoredScanDirectories: string[] = []
   if (rules.gitFiltering) {
     for (const requestedDirectory of requestedScanDirectories) {
+      // An excluded directory gathers nothing, so warning that only its tracked files are gathered would be wrong.
+      if (isExcludedScanDirectory(rules, requestedDirectory)) continue
       // eslint-disable-next-line no-await-in-loop
       if (await isIgnoredByParentRepository(requestedDirectory)) ignoredScanDirectories.push(requestedDirectory)
     }
@@ -252,13 +257,24 @@ export async function gatherPaths({
     ...gathered.flatMap((scanDirectory) => scanDirectory.absolutePaths),
     ...(selectedAppConfigFilePath ? [selectedAppConfigFilePath] : []),
   ]
+  const paths = [...new Set(absolutePaths.map((path) => normalizeCliPath(relativePath(appDirectory, path))))].sort()
   return {
-    paths: [...new Set(absolutePaths.map((path) => normalizeCliPath(relativePath(appDirectory, path))))].sort(),
+    paths,
     ignoredScanDirectories,
+    otherAppDirectories: findOtherAppDirectories(appDirectory, paths),
     listingStatus: gathered[0]?.listingStatus ?? (rules.gitFiltering ? 'tracked-only' : 'git-ignore-off'),
   }
 }
 
+/** Another app's files are scanned as this app's, so the caller can warn and suggest excluding them. */
+function findOtherAppDirectories(appDirectory: string, paths: ReadonlyArray<string>): string[] {
+  const directories = paths
+    .filter((path) => isValidFormatAppConfigurationFileName(basename(path)))
+    .map((path) => dirname(path))
+    .filter((directory) => directory !== '.')
+  return [...new Set(directories)].map((directory) => joinPath(appDirectory, directory))
+}
+
 async function gatherScanDirectory(
   scanDirectory: string,
   rules: PathRules,
@@ -272,6 +288,7 @@ async function gatherScanDirectory(
   }
 
   if (ignoredByRepository) {
+    if (isExcludedScanDirectory(rules, scanDirectory)) return {absolutePaths: [], listingStatus: 'tracked-only'}
     const trackedPaths = await listTrackedFiles(scanDirectory)
     // A normal walk would scan the untracked files that Git was told to ignore.
     if (trackedPaths === undefined) {
@@ -312,6 +329,7 @@ async function walkDirectory(
   rules: PathRules,
   scanDirectoryRepository: RepositoryIgnoredPaths | undefined,
 ): Promise<string[]> {
+  if (isExcludedScanDirectory(rules, scanDirectory)) return []
   const files: string[] = []
   // Appended to while iterating.
   const pendingDirectories = [{directory: scanDirectory, repository: scanDirectoryRepository}]
@@ -369,7 +387,7 @@ 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.
- * Nested apps, generated output, and test trees remain excluded.
+ * Extensions in a nested app or an `--include-dir` directory count as this app's.
  */
 export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray<string>): ExtensionInfo[] {
   const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml')
@@ -761,17 +779,14 @@ function isProbablyBinary(content: Buffer): boolean {
 export function findSensitiveFiles(
   appRoot: string,
   repositoryFiles: ReadonlyArray<string>,
-  selectedAppConfigFileName?: string,
+  selectedAppConfigPath?: string,
 ): SourceFile[] {
   const paths = repositoryFiles
     .filter(isSensitiveFile)
     // Compares the whole relative path, so only the app root's own lockfiles are dropped.
     .filter((path) => !LOCKFILE_MANAGERS.has(path))
-    .filter((path) => {
-      const fileName = basename(path)
-      if (!isValidFormatAppConfigurationFileName(fileName)) return true
-      return fileName === selectedAppConfigFileName
-    })
+    // Only the selected app configuration file is scanned, wherever another one with the same name sits.
+    .filter((path) => !isValidFormatAppConfigurationFileName(basename(path)) || path === selectedAppConfigPath)
 
   return paths.flatMap((path): SourceFile[] => {
     const absolutePath = joinPath(appRoot, path)

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

@@ -33,7 +33,7 @@ import {scanDependencyAutomation} from '../rules/dependency-automation-rules.js'
 import {RULE_CATALOG} from '../rules/catalog.js'
 import {redactIssue} from '../scan-artifact/index.js'
 import {getEngineVersion} from '../version.js'
-import {basename, relativePath} from '@shopify/cli-kit/node/path'
+import {normalizePath, relativePath} from '@shopify/cli-kit/node/path'
 import type {Rule, ScanContext} from '../rules/types.js'
 import type {RunnerImplementationResult, RunnerResult, SourceFile} from './types.js'
 import type {
@@ -570,8 +570,8 @@ export async function listGatheredPaths(input: ScanInput, options: ScanOptions =
     scanDirectories: input.scanDirectories,
     explicitInputs: new Set(),
   })
-  const {paths, ignoredScanDirectories} = await gatherScanPaths(input, options)
-  return {paths, ignoredScanDirectories}
+  const {paths, ignoredScanDirectories, otherAppDirectories} = await gatherScanPaths(input, options)
+  return {paths, ignoredScanDirectories, otherAppDirectories}
 }
 
 export async function scan(input: ScanInput, options: ScanOptions = {}): Promise<ScanOutput> {
@@ -583,14 +583,19 @@ export async function scan(input: ScanInput, options: ScanOptions = {}): Promise
     scanDirectories,
     explicitInputs: new Set(appConfigFilePath ? [appConfigFilePath] : []),
   })
-  const selectedFileName = appConfigFilePath ? basename(appConfigFilePath) : undefined
+  const selectedAppConfigPath = appConfigFilePath ? normalizePath(relativePath(appRoot, appConfigFilePath)) : undefined
   const appToml = appConfigFilePath ? loadAppToml(appConfigFilePath, appRoot) : null
   const appTomls = appToml ? [appToml] : []
-  const {paths: repositoryFiles, ignoredScanDirectories, listingStatus} = await gatherScanPaths(input, options)
+  const {
+    paths: repositoryFiles,
+    ignoredScanDirectories,
+    otherAppDirectories,
+    listingStatus,
+  } = await gatherScanPaths(input, options)
   const extensions = findExtensions(appRoot, repositoryFiles)
   const sourceCandidates = findSourceCandidates(repositoryFiles)
   const sourceFiles = findAppSourceFiles(appRoot, repositoryFiles)
-  const sensitiveFiles = findSensitiveFiles(appRoot, repositoryFiles, selectedFileName)
+  const sensitiveFiles = findSensitiveFiles(appRoot, repositoryFiles, selectedAppConfigPath)
   const manifestPaths = findManifestPaths(repositoryFiles)
   const manifests = findManifests(appRoot, manifestPaths)
   const dependencyAutomation = manifests.some(manifestHasDependencies)
@@ -731,5 +736,6 @@ export async function scan(input: ScanInput, options: ScanOptions = {}): Promise
     scan: scanMetadata,
     issues,
     ignoredScanDirectories,
+    otherAppDirectories,
   }
 }

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

@@ -3,11 +3,11 @@ import {isMissingFilesystemEntry} from './filesystem-errors.js'
 import {matchGlob} from '@shopify/cli-kit/node/fs'
 import {outputDebug} from '@shopify/cli-kit/node/output'
 import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system'
-import {basename, cwd, dirname, joinPath, relativePath} from '@shopify/cli-kit/node/path'
+import {basename, cwd, dirname, joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path'
 import {lstatSync, realpathSync} from 'node:fs'
 
 export interface PathRules {
-  /** `--exclude` globs, as typed. */
+  /** `--exclude` globs, with their literal start resolved against the working directory. */
   excludePatterns: ReadonlyArray<string>
   /** Off with `--no-git-ignore`: Git isn't run for gathering and `.git` isn't skipped. */
   gitFiltering: boolean
@@ -16,13 +16,38 @@ export interface PathRules {
 }
 
 export function createPathRules(options: {excludePatterns: ReadonlyArray<string>; noGitIgnore: boolean}): PathRules {
+  const workingDirectory = realpathSync(cwd())
   return {
-    excludePatterns: options.excludePatterns,
+    // An empty pattern names nothing, but it would match the working directory, whose relative path is empty.
+    excludePatterns: options.excludePatterns
+      .filter((pattern) => pattern !== '')
+      .map((pattern) => normalizeExcludePattern(pattern, workingDirectory)),
     gitFiltering: !options.noGitIgnore,
-    workingDirectory: realpathSync(cwd()),
+    workingDirectory,
   }
 }
 
+/** A segment with glob syntax, or an escape, ends the literal start of a pattern. */
+const GLOB_SYNTAX = /[*?[\]{}()!#\\]/
+
+/**
+ * Paths are matched in their shortest form relative to the working directory, so a pattern's literal start is
+ * resolved the same way: `./src`, `src/` and, from inside `app`, `../app/src` all become `src`, and `.` becomes the
+ * working directory itself. The rest of the pattern is kept as typed.
+ */
+function normalizeExcludePattern(pattern: string, workingDirectory: string): string {
+  const segments = pattern.split('/')
+  const firstGlobSegment = segments.findIndex((segment) => GLOB_SYNTAX.test(segment))
+  const literalSegments = firstGlobSegment === -1 ? segments : segments.slice(0, firstGlobSegment)
+  const literalStart = literalSegments.join('/') || (pattern.startsWith('/') ? '/' : '')
+  if (literalStart === '') return pattern
+
+  const resolvedStart = relativePath(workingDirectory, resolvePath(workingDirectory, literalStart))
+  const rest = segments.slice(literalSegments.length).join('/')
+  if (rest === '') return resolvedStart
+  return resolvedStart === '' ? rest : `${resolvedStart}/${rest}`
+}
+
 export type GitIgnoreListing = {status: 'listed'; paths: string[]} | {status: 'not-a-repository'} | {status: 'failed'}
 
 /** How a scan directory's ignored paths were found; the last two mean gathering didn't use a listing. */
@@ -67,6 +92,18 @@ export function isDroppedTrackedPath(rules: PathRules, scanDirectory: string, tr
   })
 }
 
+/**
+ * Whether `--exclude` removes a whole scan directory. The walker only tests the entries inside a scan directory, so the
+ * directory itself and its ancestors up to the working directory are tested here: adding a directory can't undo an
+ * exclusion.
+ */
+export function isExcludedScanDirectory(rules: PathRules, scanDirectory: string): boolean {
+  const segments = relativePath(rules.workingDirectory, scanDirectory).split('/')
+  return segments.some((_segment, index) =>
+    isExcluded(rules, joinPath(rules.workingDirectory, ...segments.slice(0, index + 1))),
+  )
+}
+
 function isListedAsIgnored(
   repository: RepositoryIgnoredPaths,
   entry: {absolutePath: string; isDirectory: boolean},

packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts

@@ -134,6 +134,36 @@ describe('gathering without a default exclusion list', () => {
     await expect(gatheredPaths(root)).resolves.toContain('apps/child/shopify.app.toml')
   })
 
+  test('reports each directory other than the app directory that holds an app configuration file', async () => {
+    const root = await makeDirectory()
+    await writeFiles(root, {
+      'shopify.app.toml': appConfiguration,
+      'shopify.app.staging.toml': appConfiguration,
+      'apps/child/shopify.app.toml': 'name = "Child"\n',
+      'apps/child/shopify.app.staging.toml': 'name = "Child staging"\n',
+      'apps/other/shopify.app.production.toml': 'name = "Other"\n',
+      'apps/excluded/shopify.app.toml': 'name = "Excluded"\n',
+    })
+
+    vi.stubEnv('INIT_CWD', root)
+
+    const {otherAppDirectories} = await gather(root, {excludePatterns: ['apps/excluded']})
+    expect(otherAppDirectories).toEqual([joinPath(root, 'apps/child'), joinPath(root, 'apps/other')])
+  })
+
+  test('does not report a directory whose app configuration file Git ignores, though its other files are gathered', async () => {
+    const root = await makeRepository({
+      'shopify.app.toml': appConfiguration,
+      '.gitignore': 'apps/child/shopify.app.toml\n',
+      'apps/child/shopify.app.toml': 'name = "Child"\n',
+      'apps/child/index.ts': 'export const child = true',
+    })
+
+    const {paths, otherAppDirectories} = await gather(root)
+    expect(paths).toContain('apps/child/index.ts')
+    expect(otherAppDirectories).toEqual([])
+  })
+
   test('scans node_modules, build output and test directories outside a repository', async () => {
     const root = await makeDirectory()
     const directories = [
@@ -521,6 +551,18 @@ describe('repositories', () => {
     expect(secretFindingFiles(result)).toEqual(['shopify.app.staging.toml'])
   })
 
+  test('scans only the selected app configuration file for secrets, not one with the same name in another app', async () => {
+    const configurationWithSecret = `name = "Leaky"\napplication_url = "https://example.com/?token=${secret}"\n`
+    const root = await makeDirectory()
+    await writeFiles(root, {
+      'shopify.app.toml': configurationWithSecret,
+      'shopify.app.staging.toml': configurationWithSecret,
+      'apps/child/shopify.app.toml': configurationWithSecret,
+    })
+
+    expect(secretFindingFiles(await scan(root))).toEqual(['shopify.app.toml'])
+  })
+
   describe('nested repositories', () => {
     async function makeNestedRepository(files: {outer?: Record<string, string>; inner: Record<string, string>}) {
       const outer = await makeRepository({'shopify.app.toml': appConfiguration, ...files.outer})

packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts

@@ -10,6 +10,7 @@ import {
   listTrackedFiles,
   repositoryIgnoredPaths,
 } from '../scanners/path-rules.js'
+import {joinPath} from '@shopify/cli-kit/node/path'
 import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest'
 import {mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync} from 'node:fs'
 import {tmpdir} from 'node:os'
@@ -425,6 +426,32 @@ describe('path rules', () => {
       ).toBe(false)
     })
 
+    test.each([
+      ['./src', 'src'],
+      ['src/', 'src'],
+      ['../app/src', 'src'],
+      ['./src/**', 'src/**'],
+      ['.', ''],
+      ['../app', ''],
+      ['../backend/**', '../backend/**'],
+      ['**/generated', '**/generated'],
+      ['!src', '!src'],
+      ['apps/\\[child\\]', 'apps/\\[child\\]'],
+    ])('resolves the literal start of %j against the working directory, giving %j', (pattern, expected) => {
+      const working = join(makeDirectory(), 'app')
+      mkdirSync(working)
+
+      expect(rulesFor(working, [pattern]).excludePatterns).toEqual([expected])
+    })
+
+    test('resolves an absolute pattern against the working directory and drops an empty one', () => {
+      const working = join(makeDirectory(), 'app')
+      mkdirSync(working)
+
+      // Patterns separate segments with `/` on every platform, since `\` is a glob escape.
+      expect(rulesFor(working, [joinPath(working, 'src', '*.ts'), '']).excludePatterns).toEqual(['src/*.ts'])
+    })
+
     test('applies with --no-git-ignore too', () => {
       const working = makeDirectory()
       const rules = rulesFor(working, ['generated'], false)