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..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, @@ -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[] { + 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 { + 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): 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, - 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/scanners/path-rules.ts b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts index e999fca4aad..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. */ @@ -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}, 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..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 @@ -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; inner: Record}) { const outer = await makeRepository({'shopify.app.toml': appConfiguration, ...files.outer}) 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..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' @@ -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) 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..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 @@ -210,6 +210,81 @@ 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('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', + '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}) 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..47bfdf0239d 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -1,11 +1,26 @@ -import securityCheck, {appSecurityInstructionsPrompt} from './security-check.js' -import {formatAppSecurityCommand, resolveAppSecurityCommands} from './app-security-commands.js' +import securityCheck, {appSecurityInstructionsPrompt, literalGlobPattern} from './security-check.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' +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' @@ -85,6 +100,7 @@ const agentChecks: AgentChecks = { const scanExecution: AppSecurityExecution = { scan, ignoredScanDirectories: [], + otherAppDirectories: [], deterministicFindings, agentChecks, engine, @@ -123,6 +139,59 @@ 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, 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(excludePattern, shellForPlatform())}`}, 'to leave it out.'], + } +} + function testDependencies( execution: AppSecurityExecution = scanExecution, selection: AppSecuritySelection = configSelection, @@ -130,7 +199,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'), @@ -335,19 +408,73 @@ 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).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).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 () => { + 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) + + expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, otherAppWarning('apps/child')) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, otherAppWarning('../backend')) + }) + }) + + 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')) }) }) @@ -727,6 +854,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 +875,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 +888,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() @@ -809,17 +938,40 @@ 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]}) - 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.output).toHaveBeenCalledWith('shopify.app.toml') + }) + }) + + 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({ - 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(otherAppWarning('apps/child')) + expect(dependencies.output).toHaveBeenCalledWith('apps/child/shopify.app.toml\nshopify.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 05ae9cf75ef..b3f56acd865 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 { @@ -18,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' @@ -70,7 +73,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 +165,37 @@ function securityReportInput( } } -function renderIgnoredScanDirectoryWarnings(ignoredScanDirectories: string[], dependencies: SecurityDependencies) { - for (const directory of ignoredScanDirectories) { +/** `--exclude` takes globs, so a directory name is escaped to match only that directory. */ +export function literalGlobPattern(path: string): string { + return path.replace(/[?*()[\]{}!#\\]/g, '\\$&') +} + +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 path = displayPath(directory) + 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(literalGlobPattern(path), shellForPlatform())}`}, + 'to leave it out.', + ], + }) + } } /** @@ -221,8 +250,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) + await renderGatheringWarnings(gathered, dependencies) if (options.json) { dependencies.output(JSON.stringify({files: paths}, null, 2)) } else if (paths.length > 0) { @@ -232,7 +261,7 @@ export default async function securityCheck( } const execution = await dependencies.execute(scanOptions) - renderIgnoredScanDirectoryWarnings(execution.ignoredScanDirectories, 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,