From 51e76cc72f33bb639627887f18abd79d8fa77a0d Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 13:40:25 -0700 Subject: [PATCH 1/8] Warn when App Security scans another app's files Secret scanning kept any app configuration file named like the selected one, so a nested app's shopify.app.toml was scanned while its shopify.app.staging.toml wasn't. Compare the path relative to the app directory instead, so only the selected file is scanned. Gathering no longer skips a directory that holds another app configuration file, so its files are scanned as this app's. Warn about each such directory and suggest the --exclude that leaves it out. --- .../cli/services/app-security-engine/run.ts | 5 ++- .../app-security-engine/scanners/discover.ts | 26 +++++++++---- .../app-security-engine/scanners/index.ts | 18 ++++++--- .../tests/discovery-safety.test.ts | 29 +++++++++++++++ .../cli/services/app-security-engine/types.ts | 2 + .../src/cli/services/security-check.test.ts | 37 +++++++++++++++++-- .../app/src/cli/services/security-check.ts | 24 +++++++++--- 7 files changed, 117 insertions(+), 24 deletions(-) diff --git a/packages/app/src/cli/services/app-security-engine/run.ts b/packages/app/src/cli/services/app-security-engine/run.ts index 942a4d29a5f..1f8f43d59f1 100644 --- a/packages/app/src/cli/services/app-security-engine/run.ts +++ b/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 { - 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, diff --git a/packages/app/src/cli/services/app-security-engine/scanners/discover.ts b/packages/app/src/cli/services/app-security-engine/scanners/discover.ts index 10130356aeb..93e72ba252c 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/discover.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/discover.ts @@ -218,6 +218,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 } @@ -252,13 +254,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[] { + 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, @@ -369,7 +382,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): ExtensionInfo[] { const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml') @@ -761,17 +774,14 @@ function isProbablyBinary(content: Buffer): boolean { export function findSensitiveFiles( appRoot: string, repositoryFiles: ReadonlyArray, - 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) diff --git a/packages/app/src/cli/services/app-security-engine/scanners/index.ts b/packages/app/src/cli/services/app-security-engine/scanners/index.ts index 7af1533d683..649f887e467 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/index.ts +++ b/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 { @@ -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, } } diff --git a/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts b/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts index d6b04655f28..1a225985088 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts @@ -134,6 +134,23 @@ 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('scans node_modules, build output and test directories outside a repository', async () => { const root = await makeDirectory() const directories = [ @@ -521,6 +538,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; inner: Record}) { const outer = await makeRepository({'shopify.app.toml': appConfiguration, ...files.outer}) diff --git a/packages/app/src/cli/services/app-security-engine/types.ts b/packages/app/src/cli/services/app-security-engine/types.ts index 3622d40c048..838fb58dd95 100644 --- a/packages/app/src/cli/services/app-security-engine/types.ts +++ b/packages/app/src/cli/services/app-security-engine/types.ts @@ -124,6 +124,8 @@ export interface ScanResult { export interface ScanOutput extends ScanResult { /** Absolute paths of the requested 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[] } export interface SkippedFile { diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index b5ec26f3b8a..517510635bd 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -85,6 +85,7 @@ const agentChecks: AgentChecks = { const scanExecution: AppSecurityExecution = { scan, ignoredScanDirectories: [], + otherAppDirectories: [], deterministicFindings, agentChecks, engine, @@ -130,7 +131,11 @@ function testDependencies( return { resolveSelection: vi.fn(async () => selection), execute: vi.fn(async () => execution), - listFiles: vi.fn(async () => ({paths: ['shopify.app.toml'], ignoredScanDirectories: [] as string[]})), + listFiles: vi.fn(async () => ({ + paths: ['shopify.app.toml'], + ignoredScanDirectories: [] as string[], + otherAppDirectories: [] as string[], + })), writeArtifacts: vi.fn(async () => artifacts), canPrompt: vi.fn(() => false), selectInstructionsDestination: vi.fn(async (): Promise => 'nothing'), @@ -351,6 +356,26 @@ describe('securityCheck', () => { }) }) + test('warns once for each directory that holds another app configuration, with the --exclude that leaves it out', async () => { + vi.stubEnv('INIT_CWD', appDirectory) + const dependencies = testDependencies({ + ...scanExecution, + otherAppDirectories: [`${appDirectory}/apps/child`, '/tmp/backend'], + }) + + await securityCheck(testOptions(), dependencies) + + expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { + headline: "apps/child holds another app's configuration, so its files are scanned as part of this app.", + body: ['Use', {command: '--exclude apps/child'}, 'to leave it out.'], + }) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { + headline: "../backend holds another app's configuration, so its files are scanned as part of this app.", + body: ['Use', {command: '--exclude ../backend'}, 'to leave it out.'], + }) + }) + test('does not warn when no scan directory is ignored', async () => { const dependencies = testDependencies() @@ -727,6 +752,7 @@ describe('securityCheck --list-files', () => { dependencies.listFiles.mockResolvedValue({ paths: ['../backend/server.ts', 'app/routes/index.ts', 'shopify.app.toml'], ignoredScanDirectories: [], + otherAppDirectories: [], }) await securityCheck(listFilesOptions, dependencies) @@ -747,6 +773,7 @@ describe('securityCheck --list-files', () => { dependencies.listFiles.mockResolvedValue({ paths: ['app/routes/index.ts', 'shopify.app.toml'], ignoredScanDirectories: [], + otherAppDirectories: [], }) await securityCheck({...listFilesOptions, json: true}, dependencies) @@ -759,7 +786,7 @@ describe('securityCheck --list-files', () => { test('prints nothing when no path is gathered, and an empty list with --json', async () => { const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({paths: [], ignoredScanDirectories: []}) + dependencies.listFiles.mockResolvedValue({paths: [], ignoredScanDirectories: [], otherAppDirectories: []}) await securityCheck(listFilesOptions, dependencies) expect(dependencies.output).not.toHaveBeenCalled() @@ -810,7 +837,11 @@ describe('securityCheck --list-files', () => { test('warns about an ignored scan directory through renderWarning', async () => { const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({paths: ['shopify.app.toml'], ignoredScanDirectories: [appDirectory]}) + dependencies.listFiles.mockResolvedValue({ + paths: ['shopify.app.toml'], + ignoredScanDirectories: [appDirectory], + otherAppDirectories: [], + }) vi.stubEnv('INIT_CWD', '/tmp') await securityCheck(listFilesOptions, dependencies) diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 05ae9cf75ef..c45c72686c8 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -70,7 +70,9 @@ interface SecurityDependencies { allowPrompts: boolean }): Promise execute(options: ScanInput & Required): Promise - listFiles(options: ScanInput & Required): Promise<{paths: string[]; ignoredScanDirectories: string[]}> + listFiles( + options: ScanInput & Required, + ): Promise<{paths: string[]; ignoredScanDirectories: string[]; otherAppDirectories: string[]}> writeArtifacts( appDirectory: string, resultsKey: string, @@ -160,13 +162,23 @@ function securityReportInput( } } -function renderIgnoredScanDirectoryWarnings(ignoredScanDirectories: string[], dependencies: SecurityDependencies) { - for (const directory of ignoredScanDirectories) { +function renderGatheringWarnings( + gathered: {ignoredScanDirectories: string[]; otherAppDirectories: string[]}, + dependencies: SecurityDependencies, +) { + for (const directory of gathered.ignoredScanDirectories) { dependencies.renderWarning({ headline: `${relativePath(cwd(), directory) || '.'} is ignored by Git, so only the files Git tracks in it are scanned.`, body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], }) } + for (const directory of gathered.otherAppDirectories) { + const displayPath = relativePath(cwd(), directory) || '.' + dependencies.renderWarning({ + headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, + body: ['Use', {command: `--exclude ${displayPath}`}, 'to leave it out.'], + }) + } } /** @@ -221,8 +233,8 @@ export default async function securityCheck( } if (options.listFiles) { - const {paths, ignoredScanDirectories} = await dependencies.listFiles(scanOptions) - renderIgnoredScanDirectoryWarnings(ignoredScanDirectories, dependencies) + const {paths, ...gathered} = await dependencies.listFiles(scanOptions) + renderGatheringWarnings(gathered, dependencies) if (options.json) { dependencies.output(JSON.stringify({files: paths}, null, 2)) } else if (paths.length > 0) { @@ -232,7 +244,7 @@ export default async function securityCheck( } const execution = await dependencies.execute(scanOptions) - renderIgnoredScanDirectoryWarnings(execution.ignoredScanDirectories, dependencies) + renderGatheringWarnings(execution, dependencies) await dependencies.recordMetadata({num_security_findings: execution.scan.issues.length}) const artifacts = await dependencies.writeArtifacts(appDirectory, resultsKey(selection), { deterministicFindings: execution.deterministicFindings, From 9016dd888128671a0ea7093335f027014aa7aea6 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 19:46:41 -0700 Subject: [PATCH 2/8] Quote the directory in the suggested --exclude The directory name comes from the repository, so a name such as $(touch PWNED) ran when the user pasted the suggestion into a shell. Quote it the way the generated commands are quoted, for the user's shell. --- packages/app/src/cli/services/security-check.test.ts | 11 ++++++++--- packages/app/src/cli/services/security-check.ts | 5 ++++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 517510635bd..d97f473e613 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -1,5 +1,10 @@ import securityCheck, {appSecurityInstructionsPrompt} from './security-check.js' -import {formatAppSecurityCommand, resolveAppSecurityCommands} from './app-security-commands.js' +import { + formatAppSecurityCommand, + quoteShellArgument, + resolveAppSecurityCommands, + shellForPlatform, +} from './app-security-commands.js' import {appSecurityArtifactPaths, writeCheckArtifacts} from './app-security-artifacts.js' import {validAppConfiguration} from './app-security-selection.test-data.js' import {fileExists, fileRealPath, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' @@ -368,11 +373,11 @@ describe('securityCheck', () => { expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { headline: "apps/child holds another app's configuration, so its files are scanned as part of this app.", - body: ['Use', {command: '--exclude apps/child'}, 'to leave it out.'], + body: ['Use', {command: `--exclude ${quoteShellArgument('apps/child', shellForPlatform())}`}, 'to leave it out.'], }) expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { headline: "../backend holds another app's configuration, so its files are scanned as part of this app.", - body: ['Use', {command: '--exclude ../backend'}, 'to leave it out.'], + body: ['Use', {command: `--exclude ${quoteShellArgument('../backend', shellForPlatform())}`}, 'to leave it out.'], }) }) diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index c45c72686c8..64e632724e9 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -3,7 +3,9 @@ import {writeCheckArtifacts} from './app-security-artifacts.js' import deliverAppSecurityInstructions from './app-security-instructions.js' import { formatAppSecurityCommand, + quoteShellArgument, resolveAppSecurityCommands, + shellForPlatform, type AppSecurityCommands, } from './app-security-commands.js' import { @@ -176,7 +178,8 @@ function renderGatheringWarnings( const displayPath = relativePath(cwd(), directory) || '.' dependencies.renderWarning({ headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, - body: ['Use', {command: `--exclude ${displayPath}`}, 'to leave it out.'], + // The directory name comes from the repository, so it's quoted before the user can paste it into a shell. + body: ['Use', {command: `--exclude ${quoteShellArgument(displayPath, shellForPlatform())}`}, 'to leave it out.'], }) } } From 81b420a2977bbed37bcf8f5b569ca26c0077fd22 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 19:48:42 -0700 Subject: [PATCH 3/8] Show gathering warnings relative to the real working directory --exclude globs are matched from the real working directory, but the warnings used the working directory as given. When that path went through a symbolic link, the suggested --exclude didn't match the directory it named. --- .../src/cli/services/security-check.test.ts | 87 +++++++++++-------- .../app/src/cli/services/security-check.ts | 19 ++-- 2 files changed, 62 insertions(+), 44 deletions(-) diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index d97f473e613..8dedabac25d 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -129,6 +129,14 @@ function stagingSelection(): AppSecuritySelection { } } +/** The warning for a directory that holds another app configuration, quoted for the shell the tests run in. */ +function otherAppWarning(displayPath: string) { + return { + headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, + body: ['Use', {command: `--exclude ${quoteShellArgument(displayPath, shellForPlatform())}`}, 'to leave it out.'], + } +} + function testDependencies( execution: AppSecurityExecution = scanExecution, selection: AppSecuritySelection = configSelection, @@ -345,39 +353,42 @@ describe('securityCheck', () => { }) test('warns once for each scan directory that Git ignores, relative to the working directory', async () => { - vi.stubEnv('INIT_CWD', '/tmp') - const dependencies = testDependencies({...scanExecution, ignoredScanDirectories: [appDirectory, '/tmp']}) + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies({ + ...scanExecution, + ignoredScanDirectories: [joinPath(realDirectory, 'unlinked-app'), realDirectory], + }) - await securityCheck(testOptions(), dependencies) + await securityCheck(testOptions(), dependencies) - expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { - headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], - }) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { - headline: '. is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { + headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { + headline: '. is ignored by Git, so only the files Git tracks in it are scanned.', + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) }) }) test('warns once for each directory that holds another app configuration, with the --exclude that leaves it out', async () => { - vi.stubEnv('INIT_CWD', appDirectory) - const dependencies = testDependencies({ - ...scanExecution, - otherAppDirectories: [`${appDirectory}/apps/child`, '/tmp/backend'], - }) + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies({ + ...scanExecution, + otherAppDirectories: [joinPath(realDirectory, 'apps', 'child'), joinPath(realDirectory, '..', 'backend')], + }) - await securityCheck(testOptions(), dependencies) + await securityCheck(testOptions(), dependencies) - expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { - headline: "apps/child holds another app's configuration, so its files are scanned as part of this app.", - body: ['Use', {command: `--exclude ${quoteShellArgument('apps/child', shellForPlatform())}`}, 'to leave it out.'], - }) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { - headline: "../backend holds another app's configuration, so its files are scanned as part of this app.", - body: ['Use', {command: `--exclude ${quoteShellArgument('../backend', shellForPlatform())}`}, 'to leave it out.'], + expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, otherAppWarning('apps/child')) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, otherAppWarning('../backend')) }) }) @@ -841,21 +852,23 @@ describe('securityCheck --list-files', () => { }) test('warns about an ignored scan directory through renderWarning', async () => { - const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({ - paths: ['shopify.app.toml'], - ignoredScanDirectories: [appDirectory], - otherAppDirectories: [], - }) - vi.stubEnv('INIT_CWD', '/tmp') + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies() + dependencies.listFiles.mockResolvedValue({ + paths: ['shopify.app.toml'], + ignoredScanDirectories: [joinPath(await fileRealPath(directory), 'unlinked-app')], + otherAppDirectories: [], + }) + vi.stubEnv('INIT_CWD', directory) - await securityCheck(listFilesOptions, dependencies) + await securityCheck(listFilesOptions, dependencies) - expect(dependencies.renderWarning).toHaveBeenCalledWith({ - headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + expect(dependencies.renderWarning).toHaveBeenCalledWith({ + headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + expect(dependencies.output).toHaveBeenCalledWith('shopify.app.toml') }) - expect(dependencies.output).toHaveBeenCalledWith('shopify.app.toml') }) test('returns the selection, the results key and the generated check command', async () => { diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 64e632724e9..e85308b7252 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -20,6 +20,7 @@ import { import {encodeSecurityJson, toSecurityJson} from './security-json.js' import {renderSecurityReport} from './security-output.js' import {recordAppSecurityMetadata, type AppSecurityMetadata} from './app-security-metadata.js' +import {fileRealPath} from '@shopify/cli-kit/node/fs' import {outputResult} from '@shopify/cli-kit/node/output' import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' import {cwd, relativePath} from '@shopify/cli-kit/node/path' @@ -164,22 +165,26 @@ function securityReportInput( } } -function renderGatheringWarnings( +async function renderGatheringWarnings( gathered: {ignoredScanDirectories: string[]; otherAppDirectories: string[]}, dependencies: SecurityDependencies, ) { + if (gathered.ignoredScanDirectories.length === 0 && gathered.otherAppDirectories.length === 0) return + // `--exclude` globs are matched from the real working directory, so paths are shown relative to it too. + const workingDirectory = await fileRealPath(cwd()) + const displayPath = (directory: string) => relativePath(workingDirectory, directory) || '.' for (const directory of gathered.ignoredScanDirectories) { dependencies.renderWarning({ - headline: `${relativePath(cwd(), directory) || '.'} is ignored by Git, so only the files Git tracks in it are scanned.`, + headline: `${displayPath(directory)} is ignored by Git, so only the files Git tracks in it are scanned.`, body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], }) } for (const directory of gathered.otherAppDirectories) { - const displayPath = relativePath(cwd(), directory) || '.' + const path = displayPath(directory) dependencies.renderWarning({ - headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, + headline: `${path} holds another app's configuration, so its files are scanned as part of this app.`, // The directory name comes from the repository, so it's quoted before the user can paste it into a shell. - body: ['Use', {command: `--exclude ${quoteShellArgument(displayPath, shellForPlatform())}`}, 'to leave it out.'], + body: ['Use', {command: `--exclude ${quoteShellArgument(path, shellForPlatform())}`}, 'to leave it out.'], }) } } @@ -237,7 +242,7 @@ export default async function securityCheck( if (options.listFiles) { const {paths, ...gathered} = await dependencies.listFiles(scanOptions) - renderGatheringWarnings(gathered, dependencies) + await renderGatheringWarnings(gathered, dependencies) if (options.json) { dependencies.output(JSON.stringify({files: paths}, null, 2)) } else if (paths.length > 0) { @@ -247,7 +252,7 @@ export default async function securityCheck( } const execution = await dependencies.execute(scanOptions) - renderGatheringWarnings(execution, dependencies) + await renderGatheringWarnings(execution, dependencies) await dependencies.recordMetadata({num_security_findings: execution.scan.issues.length}) const artifacts = await dependencies.writeArtifacts(appDirectory, resultsKey(selection), { deterministicFindings: execution.deterministicFindings, From 90101b20f35b063f832de31096df52e8cdc1491d Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 19:49:16 -0700 Subject: [PATCH 4/8] Escape the directory in the suggested --exclude as a glob --exclude takes globs, so a directory name with glob syntax didn't match itself, and a leading ! negated the pattern and excluded everything else. Escape the name so the suggestion matches only that directory. --- packages/app/src/cli/services/security-check.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index e85308b7252..0557742fad9 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -165,6 +165,11 @@ function securityReportInput( } } +/** `--exclude` takes globs, so a directory name is escaped to match only that directory. */ +function literalGlobPattern(path: string): string { + return path.replace(/[?*()[\]{}!#\\]/g, '\\$&') +} + async function renderGatheringWarnings( gathered: {ignoredScanDirectories: string[]; otherAppDirectories: string[]}, dependencies: SecurityDependencies, @@ -184,7 +189,11 @@ async function renderGatheringWarnings( dependencies.renderWarning({ headline: `${path} holds another app's configuration, so its files are scanned as part of this app.`, // The directory name comes from the repository, so it's quoted before the user can paste it into a shell. - body: ['Use', {command: `--exclude ${quoteShellArgument(path, shellForPlatform())}`}, 'to leave it out.'], + body: [ + 'Use', + {command: `--exclude ${quoteShellArgument(literalGlobPattern(path), shellForPlatform())}`}, + 'to leave it out.', + ], }) } } From f7058022c495ff60e3c50f750dcd9dba665f1f17 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 5 Oct 2026 19:52:06 -0700 Subject: [PATCH 5/8] Test the suggested --exclude for another app's directory Cover a directory name that is shell and glob syntax, a working directory reached through a symbolic link, the warning with --list-files, and an --exclude built from the suggestion removing only that directory. Pin that a directory whose app configuration file Git ignores isn't reported. --- .../tests/discovery-safety.test.ts | 13 ++ .../src/cli/services/security-check.test.ts | 111 +++++++++++++++++- .../app/src/cli/services/security-check.ts | 2 +- 3 files changed, 121 insertions(+), 5 deletions(-) diff --git a/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts b/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts index 1a225985088..633d9ee8771 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/discovery-safety.test.ts @@ -151,6 +151,19 @@ describe('gathering without a default exclusion list', () => { 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 = [ diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 8dedabac25d..47bfdf0239d 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -1,4 +1,4 @@ -import securityCheck, {appSecurityInstructionsPrompt} from './security-check.js' +import securityCheck, {appSecurityInstructionsPrompt, literalGlobPattern} from './security-check.js' import { formatAppSecurityCommand, quoteShellArgument, @@ -7,10 +7,20 @@ import { } from './app-security-commands.js' import {appSecurityArtifactPaths, writeCheckArtifacts} from './app-security-artifacts.js' import {validAppConfiguration} from './app-security-selection.test-data.js' -import {fileExists, fileRealPath, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {listAppSecurityFiles} from './app-security-api.js' +import { + fileExists, + fileRealPath, + inTemporaryDirectory, + matchGlob, + mkdir, + readFile, + writeFile, +} from '@shopify/cli-kit/node/fs' import {cwd, joinPath, relativePath} from '@shopify/cli-kit/node/path' import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' import {afterEach, describe, expect, test, vi} from 'vitest' +import {symlink} from 'node:fs/promises' import type {AppSecurityExecution} from './app-security-api.js' import type {AppSecuritySelection} from './app-security-selection.js' import type {AppSecurityInstructionsDestination} from './security-check.js' @@ -129,11 +139,56 @@ function stagingSelection(): AppSecuritySelection { } } +describe('literalGlobPattern', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + + test.each([ + ['apps/[child]', 'apps/c'], + ['apps/{a,b}', 'apps/a'], + ['apps/a*b', 'apps/aXb'], + ['apps/x?y', 'apps/xzy'], + ['#child', 'child'], + ['!child', 'src'], + ])('gives a glob that matches %s and not %s', (directory, sibling) => { + const pattern = literalGlobPattern(directory) + + expect(matchGlob(directory, pattern)).toBe(true) + expect(matchGlob(sibling, pattern)).toBe(false) + }) + + test('gives an --exclude that removes only that directory from the gathered files', async () => { + await inTemporaryDirectory(async (directory) => { + const root = await fileRealPath(directory) + await Promise.all(['!child', '[child]', 'c', 'src'].map((name) => mkdir(joinPath(root, name)))) + await Promise.all([ + writeFile(joinPath(root, 'shopify.app.toml'), validAppConfiguration()), + writeFile(joinPath(root, '!child', 'shopify.app.toml'), 'name = "Negated"\n'), + writeFile(joinPath(root, '[child]', 'shopify.app.toml'), 'name = "Bracketed"\n'), + writeFile(joinPath(root, 'c', 'index.ts'), 'export const c = true\n'), + writeFile(joinPath(root, 'src', 'index.ts'), 'export const src = true\n'), + ]) + vi.stubEnv('INIT_CWD', root) + + const gathered = await listAppSecurityFiles({ + appDirectory: root, + scanDirectories: [root], + requestedScanDirectories: [root], + excludePatterns: ['!child', '[child]'].map(literalGlobPattern), + }) + + expect(gathered.paths).toEqual(['c/index.ts', 'shopify.app.toml', 'src/index.ts']) + expect(gathered.otherAppDirectories).toEqual([]) + }) + }) +}) + /** The warning for a directory that holds another app configuration, quoted for the shell the tests run in. */ -function otherAppWarning(displayPath: string) { +function otherAppWarning(displayPath: string, excludePattern = displayPath) { return { headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, - body: ['Use', {command: `--exclude ${quoteShellArgument(displayPath, shellForPlatform())}`}, 'to leave it out.'], + body: ['Use', {command: `--exclude ${quoteShellArgument(excludePattern, shellForPlatform())}`}, 'to leave it out.'], } } @@ -392,6 +447,37 @@ describe('securityCheck', () => { }) }) + test('escapes and quotes a directory name that is glob and shell syntax', async () => { + await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies({ + ...scanExecution, + otherAppDirectories: [joinPath(await fileRealPath(directory), 'apps', '$(touch PWNED)')], + }) + + await securityCheck(testOptions(), dependencies) + + expect(dependencies.renderWarning).toHaveBeenCalledWith( + otherAppWarning('apps/$(touch PWNED)', 'apps/$\\(touch PWNED\\)'), + ) + }) + }) + + test('shows warnings relative to the real working directory when it is reached through a symbolic link', async () => { + await inTemporaryDirectory(async (directory) => { + const app = joinPath(await fileRealPath(directory), 'app') + await mkdir(app) + const link = joinPath(directory, 'linked-app') + await symlink(app, link, 'dir') + vi.stubEnv('INIT_CWD', link) + const dependencies = testDependencies({...scanExecution, otherAppDirectories: [joinPath(app, 'apps', 'child')]}) + + await securityCheck(testOptions(), dependencies) + + expect(dependencies.renderWarning).toHaveBeenCalledWith(otherAppWarning('apps/child')) + }) + }) + test('does not warn when no scan directory is ignored', async () => { const dependencies = testDependencies() @@ -871,6 +957,23 @@ describe('securityCheck --list-files', () => { }) }) + test('warns about a directory that holds another app configuration through renderWarning', async () => { + await inTemporaryDirectory(async (directory) => { + const dependencies = testDependencies() + dependencies.listFiles.mockResolvedValue({ + paths: ['apps/child/shopify.app.toml', 'shopify.app.toml'], + ignoredScanDirectories: [], + otherAppDirectories: [joinPath(await fileRealPath(directory), 'apps', 'child')], + }) + vi.stubEnv('INIT_CWD', directory) + + await securityCheck(listFilesOptions, dependencies) + + expect(dependencies.renderWarning).toHaveBeenCalledWith(otherAppWarning('apps/child')) + expect(dependencies.output).toHaveBeenCalledWith('apps/child/shopify.app.toml\nshopify.app.toml') + }) + }) + test('returns the selection, the results key and the generated check command', async () => { const dependencies = testDependencies() diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 0557742fad9..b3f56acd865 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -166,7 +166,7 @@ function securityReportInput( } /** `--exclude` takes globs, so a directory name is escaped to match only that directory. */ -function literalGlobPattern(path: string): string { +export function literalGlobPattern(path: string): string { return path.replace(/[?*()[\]{}!#\\]/g, '\\$&') } From 53a0db32afbc87e5490524b4a7a9bb1f5f2d155c Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 6 Oct 2026 06:11:21 -0700 Subject: [PATCH 6/8] Let --exclude remove a whole scan directory The walker only tested the entries inside a scan directory, so an --exclude that matched an --include-dir, or a directory above it, removed nothing. Test the scan directory and its ancestors up to the working directory too, so adding a directory can't undo an exclusion, and skip the ignored-scan-directory warning for a directory that gathers nothing. --- .../app-security-engine/scanners/discover.ts | 5 ++ .../scanners/path-rules.ts | 12 ++++ .../tests/scan-directories.test.ts | 64 +++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/packages/app/src/cli/services/app-security-engine/scanners/discover.ts b/packages/app/src/cli/services/app-security-engine/scanners/discover.ts index 93e72ba252c..8722273e3c0 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/discover.ts +++ b/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, @@ -239,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) } @@ -285,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) { @@ -325,6 +329,7 @@ async function walkDirectory( rules: PathRules, scanDirectoryRepository: RepositoryIgnoredPaths | undefined, ): Promise { + if (isExcludedScanDirectory(rules, scanDirectory)) return [] const files: string[] = [] // Appended to while iterating. const pendingDirectories = [{directory: scanDirectory, repository: scanDirectoryRepository}] diff --git a/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts index e999fca4aad..14a74d877c2 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts @@ -67,6 +67,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}, diff --git a/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts b/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts index c2b2b7dc303..153e1f5d66b 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts @@ -210,6 +210,70 @@ describe('a scan directory inside another one', () => { }) }) +describe('an excluded scan directory', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + + async function makeAppAndDirectory(directory: string) { + const root = await makeDirectory({ + 'app/shopify.app.toml': appConfiguration, + 'app/src/a.ts': 'export const a = true', + [`${directory}/shopify.app.toml`]: 'name = "Other"\n', + [`${directory}/server.ts`]: 'export const server = true', + }) + return {root, app: join(root, 'app'), directory: join(root, directory)} + } + + test('gathers nothing from a scan directory that --exclude matches', async () => { + const {app, directory} = await makeAppAndDirectory('backend') + vi.stubEnv('INIT_CWD', app) + + const result = await gather({appDirectory: app, scanDirectories: [app, directory], excludePatterns: ['../backend']}) + + expect(result.paths).toEqual(['shopify.app.toml', 'src/a.ts']) + expect(result.otherAppDirectories).toEqual([]) + }) + + test('gathers nothing from a scan directory inside an excluded directory', async () => { + const {app, directory} = await makeAppAndDirectory('shared/sdk') + vi.stubEnv('INIT_CWD', app) + + const result = await gather({appDirectory: app, scanDirectories: [app, directory], excludePatterns: ['../shared']}) + + expect(result.paths).toEqual(['shopify.app.toml', 'src/a.ts']) + }) + + test('includes the app directory, which --exclude can match when the working directory is above it', async () => { + const {root, app, directory} = await makeAppAndDirectory('backend') + vi.stubEnv('INIT_CWD', root) + + const result = await gather({appDirectory: app, scanDirectories: [app, directory], excludePatterns: ['app']}) + + expect(result.paths).toEqual(['../backend/server.ts', '../backend/shopify.app.toml']) + }) + + test('gathers none of its tracked files and gets no ignored-scan-directory warning', async () => { + const repository = await makeRepository({ + '.gitignore': 'app/\n', + 'app/shopify.app.toml': appConfiguration, + 'app/src/a.ts': 'export const a = true', + }) + git(repository, ['add', '-f', '.gitignore', 'app/shopify.app.toml', 'app/src/a.ts']) + git(repository, ['commit', '-qm', 'init']) + const app = join(repository, 'app') + vi.stubEnv('INIT_CWD', repository) + + const excluded = await gather({appDirectory: app, scanDirectories: [app], excludePatterns: ['app']}) + const included = await gather({appDirectory: app, scanDirectories: [app]}) + + expect(excluded.paths).toEqual([]) + expect(excluded.ignoredScanDirectories).toEqual([]) + expect(included.paths).toEqual(['shopify.app.toml', 'src/a.ts']) + expect(included.ignoredScanDirectories).toEqual([app]) + }) +}) + describe('reading across scan directories', () => { test('reads a file in an include directory and reports its path relative to the app directory', async () => { const app = await makeRepository({'shopify.app.toml': appConfiguration}) From 9a5ef304528120f4e98dfde2dc4451d81df02bba Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 6 Oct 2026 06:13:48 -0700 Subject: [PATCH 7/8] Resolve the literal start of each --exclude against the working directory Patterns were matched as typed against the shortest path relative to the working directory, so ./src, src/ and, from inside app, ../app/src excluded nothing, and neither did . or ../app. Resolve each pattern up to its first glob segment the same way paths are, and keep the rest as typed. Drop an empty pattern, which would otherwise match the working directory. --- .../scanners/path-rules.ts | 33 ++++++++++++++++--- .../tests/path-rules.test.ts | 25 ++++++++++++++ .../tests/scan-directories.test.ts | 11 +++++++ 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts index 14a74d877c2..dfabefa5424 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts +++ b/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 /** 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; 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. */ diff --git a/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts b/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts index 1c78744424a..c7f1ba95850 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts @@ -425,6 +425,31 @@ 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) + + expect(rulesFor(working, [join(working, 'src', '*.ts'), '']).excludePatterns).toEqual(['src/*.ts']) + }) + test('applies with --no-git-ignore too', () => { const working = makeDirectory() const rules = rulesFor(working, ['generated'], false) diff --git a/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts b/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts index 153e1f5d66b..9ea6c625f76 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts @@ -253,6 +253,17 @@ describe('an excluded scan directory', () => { expect(result.paths).toEqual(['../backend/server.ts', '../backend/shopify.app.toml']) }) + test('includes the app directory when the working directory is the app directory and --exclude names it', async () => { + const {app, directory} = await makeAppAndDirectory('backend') + vi.stubEnv('INIT_CWD', app) + + const fromDot = await gather({appDirectory: app, scanDirectories: [app, directory], excludePatterns: ['.']}) + const fromParent = await gather({appDirectory: app, scanDirectories: [app, directory], excludePatterns: ['../app']}) + + expect(fromDot.paths).toEqual(['../backend/server.ts', '../backend/shopify.app.toml']) + expect(fromParent.paths).toEqual(fromDot.paths) + }) + test('gathers none of its tracked files and gets no ignored-scan-directory warning', async () => { const repository = await makeRepository({ '.gitignore': 'app/\n', From ea09efc0ab7b1b0fbd0a6fb8e062cdb092fe9df7 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 6 Oct 2026 06:26:53 -0700 Subject: [PATCH 8/8] Build the absolute --exclude test pattern with / on Windows node:path's join wrote the pattern with backslashes on Windows, which --exclude reads as glob escapes, so the pattern was kept as typed. --- .../cli/services/app-security-engine/tests/path-rules.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts b/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts index c7f1ba95850..676862ae9ad 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts +++ b/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' @@ -447,7 +448,8 @@ describe('path rules', () => { const working = join(makeDirectory(), 'app') mkdirSync(working) - expect(rulesFor(working, [join(working, 'src', '*.ts'), '']).excludePatterns).toEqual(['src/*.ts']) + // 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', () => {