diff --git a/packages/app/src/cli/commands/app/security/check.test.ts b/packages/app/src/cli/commands/app/security/check.test.ts index 0dfeec896b9..7d9384f6150 100644 --- a/packages/app/src/cli/commands/app/security/check.test.ts +++ b/packages/app/src/cli/commands/app/security/check.test.ts @@ -28,6 +28,7 @@ describe('app security check command', () => { 'client-id', 'config', 'exclude', + 'include-dir', 'json', 'no-git-ignore', 'path', @@ -53,6 +54,7 @@ describe('app security check command', () => { blocking: 'high', yes: false, skipInstructions: true, + includeDirs: [], excludePatterns: [], noGitIgnore: false, }) @@ -69,13 +71,26 @@ describe('app security check command', () => { ) }) + test('forwards repeated --include-dir values exactly as typed, without resolving them', async () => { + await SecurityCheck.run( + ['--include-dir', '../backend', '--include-dir', './lib/', '--include-dir', 'a b', '--skip-instructions'], + import.meta.url, + ) + + expect(securityCheck).toHaveBeenCalledWith( + expect.objectContaining({includeDirs: ['../backend', './lib/', 'a b'], skipInstructions: true}), + ) + }) + test('forwards --no-git-ignore', async () => { await SecurityCheck.run(['--no-git-ignore', '--skip-instructions'], import.meta.url) expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({noGitIgnore: true, skipInstructions: true})) }) - test('does not read --exclude or --no-git-ignore from the environment', () => { + test('reads --include-dir and --exclude only from the command line', () => { + expect(SecurityCheck.flags['include-dir'].env).toBeUndefined() + expect(SecurityCheck.flags['include-dir'].multiple).toBe(true) expect(SecurityCheck.flags.exclude.env).toBeUndefined() expect(SecurityCheck.flags['no-git-ignore'].env).toBe('SHOPIFY_FLAG_NO_GIT_IGNORE') }) @@ -97,6 +112,7 @@ describe('app security check command', () => { blocking: 'none', yes: true, skipInstructions: false, + includeDirs: [], excludePatterns: [], noGitIgnore: false, }) diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 919adb35a0b..001bced9327 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -26,6 +26,13 @@ In interactive terminals, the command offers to copy the coding-agent instructio static flags = { ...globalFlags, ...appSecuritySelectionFlags, + // No environment variable: oclif passes a repeatable flag's variable as one string, so it could hold only one directory. + // eslint-disable-next-line @shopify/cli/command-flags-with-env + 'include-dir': Flags.string({ + description: + 'Also scan this directory, relative to the working directory. Repeat the flag to add directories. Use it for code that lives outside the app directory, such as a backend or a shared library.', + multiple: true, + }), // No environment variable: oclif passes a repeatable flag's variable as one string, so it could hold only one pattern. // eslint-disable-next-line @shopify/cli/command-flags-with-env exclude: Flags.string({ @@ -67,6 +74,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio blocking: flags.blocking, yes: flags.yes, skipInstructions: flags['skip-instructions'], + includeDirs: flags['include-dir'] ?? [], excludePatterns: flags.exclude ?? [], noGitIgnore: Boolean(flags['no-git-ignore']), }) diff --git a/packages/app/src/cli/services/app-security-api.test.ts b/packages/app/src/cli/services/app-security-api.test.ts index 6158f0a619a..eb11edc505c 100644 --- a/packages/app/src/cli/services/app-security-api.test.ts +++ b/packages/app/src/cli/services/app-security-api.test.ts @@ -20,6 +20,7 @@ function scanInputFor(directory: string) { return { appDirectory: directory, scanDirectories: [directory], + requestedScanDirectories: [directory], appConfigFilePath: joinPath(directory, 'shopify.app.toml'), } } diff --git a/packages/app/src/cli/services/app-security-commands.test.ts b/packages/app/src/cli/services/app-security-commands.test.ts index f49c739aebf..2a2491b91eb 100644 --- a/packages/app/src/cli/services/app-security-commands.test.ts +++ b/packages/app/src/cli/services/app-security-commands.test.ts @@ -192,6 +192,31 @@ describe('resolveAppSecurityCommands', () => { expect(commands.clean.args).toEqual(['app', 'security', 'clean', {flag: '--path', value: '/tmp/app'}]) }) + test('repeats --include-dir values as typed, in order, before --exclude, on scan only', () => { + const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml', ['generated'], true, [ + '../backend', + './lib/', + '../backend', + ]) + + expect(commands.scan.args).toEqual([ + 'app', + 'security', + 'check', + {flag: '--path', value: '/tmp/app'}, + {flag: '--config', value: 'staging'}, + {flag: '--include-dir', value: '../backend'}, + {flag: '--include-dir', value: './lib/'}, + {flag: '--include-dir', value: '../backend'}, + {flag: '--exclude', value: 'generated'}, + '--no-git-ignore', + ]) + expect(commands.record.args).toEqual(['app', 'security', 'record', {flag: '--path', value: '/tmp/app'}]) + expect(formatAppSecurityCommand(commands.scan, 'posix')).toContain( + "--include-dir '../backend' --include-dir './lib/' --include-dir '../backend' --exclude 'generated'", + ) + }) + test('omits --exclude and --no-git-ignore when they were not passed', () => { expect(resolveAppSecurityCommands('/tmp/app', undefined, [], false).scan.args).toEqual([ 'app', diff --git a/packages/app/src/cli/services/app-security-commands.ts b/packages/app/src/cli/services/app-security-commands.ts index 6fca5f18e53..dd3b26c2cb5 100644 --- a/packages/app/src/cli/services/app-security-commands.ts +++ b/packages/app/src/cli/services/app-security-commands.ts @@ -19,16 +19,17 @@ export interface AppSecurityCommands { clean: AppSecurityCommand } -/** `excludePatterns` and `noGitIgnore` are repeated on scan so that rerunning the check gathers the same files. */ +/** `includeDirs`, `excludePatterns` and `noGitIgnore` are repeated on scan so that rerunning the check gathers the same files. */ export function resolveAppSecurityCommands( appRoot: string, configFileName?: string, excludePatterns: ReadonlyArray = [], noGitIgnore = false, + includeDirs: ReadonlyArray = [], ): AppSecurityCommands { const configFlag = configFileName ? getAppConfigurationShorthand(configFileName) : undefined const command = 'shopify' - // Only check reads the app configuration and discovers files, so it's the only command that takes --config, --exclude or --no-git-ignore. + // Only check reads the app configuration and discovers files, so it's the only command that takes --config, --include-dir, --exclude or --no-git-ignore. const subcommandArgs = (subcommand: string): AppSecurityArgument[] => [ 'app', 'security', @@ -42,6 +43,7 @@ export function resolveAppSecurityCommands( args: [ ...subcommandArgs('check'), ...(configFlag ? [{flag: '--config', value: configFlag}] : []), + ...includeDirs.map((includeDir) => ({flag: '--include-dir', value: includeDir})), ...excludePatterns.map((excludePattern) => ({flag: '--exclude', value: excludePattern})), ...(noGitIgnore ? ['--no-git-ignore'] : []), ], diff --git a/packages/app/src/cli/services/app-security-engine/checks/index.ts b/packages/app/src/cli/services/app-security-engine/checks/index.ts index c1392eb177c..8823356b948 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/index.ts +++ b/packages/app/src/cli/services/app-security-engine/checks/index.ts @@ -207,20 +207,13 @@ const isRecord = (value: unknown): value is Record => const isPositiveInteger = (value: unknown): value is number => Number.isInteger(value) && (value as number) >= 1 /** - * Reject paths that escape the app root or are absolute. - * - * The findings document is developer-controlled input that ends up verbatim in - * agent-findings.json. A path like `../../../etc/passwd` is never a legitimate - * finding location. + * Reject absolute paths. A path may start with `../`: a scan directory can sit outside the app directory, and + * finding locations are relative to the app directory. */ const isSafeRelativePath = (path: string): boolean => { if (!path) return false - // Reject absolute POSIX and Windows paths. if (path.startsWith('/') || /^[a-zA-Z]:[\\/]/.test(path)) return false - if (path.includes('\0')) return false - // Normalise separators and reject any traversal segment. - const segments = path.replace(/\\/g, '/').split('/') - return !segments.includes('..') + return !path.includes('\0') } /** @@ -240,8 +233,7 @@ export const validateFinding = (value: unknown): string | undefined => { return 'finding requires at least one evidence citation' if (finding.evidence.length > MAX_EVIDENCE) return `finding exceeds ${MAX_EVIDENCE} evidence citations` if (finding.file.length > MAX_PATH_LENGTH) return `file path exceeds ${MAX_PATH_LENGTH} characters` - if (!isSafeRelativePath(finding.file)) - return `unsafe file path (must be relative and inside the app): ${finding.file}` + if (!isSafeRelativePath(finding.file)) return `unsafe file path (must be relative): ${finding.file}` if (finding.message.length > MAX_MESSAGE_LENGTH) return `message exceeds ${MAX_MESSAGE_LENGTH} characters` if (finding.snippet !== undefined && typeof finding.snippet !== 'string') return 'snippet must be a string' if (typeof finding.snippet === 'string' && finding.snippet.length > MAX_SNIPPET_LENGTH) diff --git a/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts b/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts index f76f93e27f8..c3d4763f408 100644 --- a/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts +++ b/packages/app/src/cli/services/app-security-engine/rules/secret-rules.ts @@ -1,4 +1,4 @@ -import {dirname, joinPath} from '@shopify/cli-kit/node/path' +import {basename, dirname} from '@shopify/cli-kit/node/path' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' import type {ScanContext, SourceFile} from './types.js' import type {Issue} from '../types.js' @@ -156,15 +156,14 @@ async function gitTopLevel(cwd: string): Promise { /** When git listed ignored paths, a nested repository is confirmed by comparing top levels. */ async function ignoredFileScanReason( - appRoot: string, - path: string, + file: SourceFile, gitIgnoreListing: ScanContext['gitIgnoreListing'], appTopLevel: () => Promise, ): Promise<{reason: IgnoredFileScanReason; evidence: string[]}> { if (gitIgnoreListing === 'failed') return {reason: 'listing-failed', evidence: []} if (gitIgnoreListing === 'not-a-repository') return {reason: 'unknown', evidence: []} - const fileTopLevel = await gitTopLevel(dirname(joinPath(appRoot, path))) + const fileTopLevel = await gitTopLevel(dirname(file.absolutePath)) const appRootTopLevel = await appTopLevel() const bothKnown = fileTopLevel !== undefined && appRootTopLevel !== undefined const nested = bothKnown && fileTopLevel !== appRootTopLevel @@ -173,7 +172,7 @@ async function ignoredFileScanReason( else if (bothKnown) verdict = 'same' return { reason: nested ? 'nested-repository' : 'unknown', - evidence: [`git rev-parse --show-toplevel → ${verdict} for ${path} and the app root`], + evidence: [`git rev-parse --show-toplevel → ${verdict} for ${file.path} and the app root`], } } @@ -263,7 +262,7 @@ export async function scanCommittedSecrets( // Keep git probes sequential to avoid spawning competing processes for one repository. // eslint-disable-next-line no-await-in-loop - const status = await gitStatusFor(appRoot, file.path) + const status = await gitStatusFor(file) // Empty named secret files stay fail-closed only when git confirms they are // tracked — history may still contain prior secrets. Unknown git plus empty // contents is not a provable leak. @@ -279,7 +278,7 @@ export async function scanCommittedSecrets( // With --no-git-ignore the file was scanned only because filtering was off, and Git still protects it. if (gitIgnoreListing === 'git-ignore-off') continue // eslint-disable-next-line no-await-in-loop - const scanReason = await ignoredFileScanReason(appRoot, file.path, gitIgnoreListing, appRootTopLevel) + const scanReason = await ignoredFileScanReason(file, gitIgnoreListing, appRootTopLevel) ignoredScanReason = scanReason.reason evidence = [...evidence, ...scanReason.evidence] } @@ -353,8 +352,10 @@ async function runGit(cwd: string, args: string[]): Promise<{exitCode?: number; } } -export async function gitStatusFor(appRoot: string, file: string): Promise { - const run = async (args: string[]) => runGit(appRoot, args) +/** Asks the file's own repository, which may differ from the app's: a scan directory can be another repository. */ +export async function gitStatusFor(file: Pick): Promise { + const run = async (args: string[]) => runGit(dirname(file.absolutePath), args) + const name = basename(file.absolutePath) // Is this even a git repo? If not, we cannot confirm anything. const inRepo = await run(['rev-parse', '--is-inside-work-tree']) @@ -371,25 +372,25 @@ export async function gitStatusFor(appRoot: string, file: string): Promise 0) tracked = true else if (ls.exitCode === 1) tracked = false let trackedVerdict = 'unknown' if (tracked === true) trackedVerdict = 'TRACKED' else if (tracked === false) trackedVerdict = 'untracked' - evidence.push(`git ls-files --error-unmatch ${file} → ${trackedVerdict}`) + evidence.push(`git ls-files --error-unmatch ${file.path} → ${trackedVerdict}`) // check-ignore likewise defines 0 as ignored and 1 as conclusively not // ignored. Exit codes above 1 are command errors and must fail closed. - const checkIgnore = await run(['check-ignore', '-q', '--', file]) + const checkIgnore = await run(['check-ignore', '-q', '--', name]) let ignored: boolean | undefined if (checkIgnore.exitCode === 0) ignored = true else if (checkIgnore.exitCode === 1) ignored = false let ignoredVerdict = 'unknown' if (ignored === true) ignoredVerdict = 'ignored' else if (ignored === false) ignoredVerdict = 'not ignored' - evidence.push(`git check-ignore -q ${file} → ${ignoredVerdict}`) + evidence.push(`git check-ignore -q ${file.path} → ${ignoredVerdict}`) return { tracked, 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 2028c621fbd..10f048732cf 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 @@ -203,6 +203,11 @@ interface GatherInput { appDirectory: string /** Absolute real paths of the directories to walk. */ scanDirectories: ReadonlyArray + /** + * Every directory that was asked for, including those inside another scan directory, so each can be checked for the + * ignored-scan-directory warning. It contains every entry of `scanDirectories`. + */ + requestedScanDirectories: ReadonlyArray /** Added after filtering, so no path rule can remove it. */ selectedAppConfigFilePath?: string rules: PathRules @@ -211,7 +216,7 @@ interface GatherInput { interface GatheredPaths { /** Sorted and unique, relative to the app directory, written with `/`. They may start with `../`. */ paths: string[] - /** Scan directories that their repository ignores, so only the files Git tracks in them were gathered. */ + /** Requested directories that their repository ignores, so only the files Git tracks in them are gathered. */ ignoredScanDirectories: 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 @@ -219,20 +224,28 @@ interface GatheredPaths { interface GatheredScanDirectory { absolutePaths: string[] - ignored: boolean listingStatus: GatheredPaths['listingStatus'] } export async function gatherPaths({ appDirectory, scanDirectories, + requestedScanDirectories, selectedAppConfigFilePath, rules, }: GatherInput): Promise { + const ignoredScanDirectories: string[] = [] + if (rules.gitFiltering) { + for (const requestedDirectory of requestedScanDirectories) { + // eslint-disable-next-line no-await-in-loop + if (await isIgnoredByParentRepository(requestedDirectory)) ignoredScanDirectories.push(requestedDirectory) + } + } + const gathered: GatheredScanDirectory[] = [] for (const scanDirectory of scanDirectories) { // eslint-disable-next-line no-await-in-loop - gathered.push(await gatherScanDirectory(scanDirectory, rules)) + gathered.push(await gatherScanDirectory(scanDirectory, rules, ignoredScanDirectories.includes(scanDirectory))) } const absolutePaths = [ @@ -241,21 +254,24 @@ export async function gatherPaths({ ] return { paths: [...new Set(absolutePaths.map((path) => normalizeCliPath(relativePath(appDirectory, path))))].sort(), - ignoredScanDirectories: scanDirectories.filter((_directory, index) => gathered[index]?.ignored), + ignoredScanDirectories, listingStatus: gathered[0]?.listingStatus ?? (rules.gitFiltering ? 'tracked-only' : 'git-ignore-off'), } } -async function gatherScanDirectory(scanDirectory: string, rules: PathRules): Promise { +async function gatherScanDirectory( + scanDirectory: string, + rules: PathRules, + ignoredByRepository: boolean, +): Promise { if (!rules.gitFiltering) { return { absolutePaths: await walkDirectory(scanDirectory, rules, undefined), - ignored: false, listingStatus: 'git-ignore-off', } } - if (await isIgnoredByParentRepository(scanDirectory)) { + if (ignoredByRepository) { const trackedPaths = await listTrackedFiles(scanDirectory) // A normal walk would scan the untracked files that Git was told to ignore. if (trackedPaths === undefined) { @@ -266,7 +282,6 @@ async function gatherScanDirectory(scanDirectory: string, rules: PathRules): Pro .filter((trackedPath) => !isDroppedTrackedPath(rules, scanDirectory, trackedPath)) .map((trackedPath) => joinPath(scanDirectory, trackedPath)) .filter(isTrackedFile), - ignored: true, listingStatus: 'tracked-only', } } @@ -274,7 +289,6 @@ async function gatherScanDirectory(scanDirectory: string, rules: PathRules): Pro const listing = await listGitIgnoredPaths(scanDirectory) return { absolutePaths: await walkDirectory(scanDirectory, rules, repositoryIgnoredPaths(scanDirectory, listing)), - ignored: false, listingStatus: listing.status, } } @@ -483,15 +497,7 @@ type InspectedPath = {status: 'missing'} | {status: 'file'; path: string} | {sta function repositoryDisplayPath(path: string): string { const relative = normalizeCliPath(relativePath(configuredReader().appDirectory, path)) - if ( - relative.length === 0 || - relative === '.' || - relative === '..' || - relative.startsWith('../') || - isAbsolutePath(relative) - ) { - return 'path' - } + if (relative.length === 0 || relative === '.' || isAbsolutePath(relative)) return 'path' return relative } 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 d45ffb98f0d..d1e70154f73 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 @@ -548,7 +548,7 @@ function normalizeRunnerResult(value: Issue[] | RunnerResult): RunnerResult { } export async function scan( - {appDirectory: appRoot, scanDirectories, appConfigFilePath}: ScanInput, + {appDirectory: appRoot, scanDirectories, requestedScanDirectories, appConfigFilePath}: ScanInput, options: ScanOptions = {}, ): Promise { // The selected app configuration is an explicit input: it's read even when it is a symbolic link @@ -568,6 +568,7 @@ export async function scan( } = await gatherPaths({ appDirectory: appRoot, scanDirectories, + requestedScanDirectories, selectedAppConfigFilePath: appConfigFilePath, rules: createPathRules({excludePatterns: options.excludePatterns ?? [], noGitIgnore: options.noGitIgnore ?? false}), }) diff --git a/packages/app/src/cli/services/app-security-engine/tests/checks.test.ts b/packages/app/src/cli/services/app-security-engine/tests/checks.test.ts index a995665b2ed..026758febf4 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/checks.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/checks.test.ts @@ -196,13 +196,20 @@ describe('finding validation', () => { }) test('rejects unsafe file and evidence paths and lines', () => { - expect(validateFinding({...validFinding, file: '../outside.ts'})).toMatch(/unsafe file path/) + expect(validateFinding({...validFinding, file: '/etc/passwd'})).toMatch(/unsafe file path/) expect(validateFinding({...validFinding, file: 'C:\\app\\a.ts'})).toMatch(/unsafe file path/) - expect(validateFinding({...validFinding, evidence: [{file: '../secret', line: 1}]})).toMatch(/unsafe evidence/) + expect(validateFinding({...validFinding, file: 'a\0.ts'})).toMatch(/unsafe file path/) + expect(validateFinding({...validFinding, file: `${'a/'.repeat(600)}a.ts`})).toMatch(/exceeds 1024/) + expect(validateFinding({...validFinding, evidence: [{file: 'a\0.ts', line: 1}]})).toMatch(/unsafe evidence/) expect(validateFinding({...validFinding, evidence: [{file: '/etc/passwd', line: 1}]})).toMatch(/unsafe evidence/) expect(validateFinding({...validFinding, evidence: [{file: 'app/a.ts', line: 0}]})).toMatch(/evidence line/) }) + test('allows paths that start with ../, for a scan directory outside the app directory', () => { + expect(validateFinding({...validFinding, file: '../backend/src/a.ts'})).toBeUndefined() + expect(validateFinding({...validFinding, evidence: [{file: '../library/index.ts', line: 1}]})).toBeUndefined() + }) + test('caps agent-supplied text and evidence', () => { expect(validateFinding({...validFinding, message: 'x'.repeat(4_001)})).toMatch(/message exceeds/) expect(validateFinding({...validFinding, file: `app/${'x'.repeat(1_024)}`})).toMatch(/file path exceeds/) diff --git a/packages/app/src/cli/services/app-security-engine/tests/dependency-automation-discovery.test.ts b/packages/app/src/cli/services/app-security-engine/tests/dependency-automation-discovery.test.ts index 16ec6900859..1772bd416ce 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/dependency-automation-discovery.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/dependency-automation-discovery.test.ts @@ -28,6 +28,7 @@ async function findInputs(root: string, excludePatterns: string[] = []) { const {paths} = await gatherPaths({ appDirectory: root, scanDirectories: [root], + requestedScanDirectories: [root], rules: createPathRules({excludePatterns, noGitIgnore: true}), }) return findDependencyAutomationInputs(root, paths) 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 2f1a566054d..d6b04655f28 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 @@ -80,6 +80,8 @@ function inspectedManifestPaths(result: ScanResult): string[] { interface GatherOptions { appDirectory?: string scanDirectories?: string[] + /** Defaults to `scanDirectories`. */ + requestedScanDirectories?: string[] selectedAppConfigFilePath?: string excludePatterns?: string[] noGitIgnore?: boolean @@ -97,6 +99,7 @@ async function gather(root: string, options: GatherOptions = {}) { return gatherPaths({ appDirectory, scanDirectories, + requestedScanDirectories: options.requestedScanDirectories ?? scanDirectories, selectedAppConfigFilePath: options.selectedAppConfigFilePath, rules: createPathRules({excludePatterns: options.excludePatterns ?? [], noGitIgnore: options.noGitIgnore ?? false}), }) diff --git a/packages/app/src/cli/services/app-security-engine/tests/record.test.ts b/packages/app/src/cli/services/app-security-engine/tests/record.test.ts index fc86f2e95b3..51c77d8d19e 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/record.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/record.test.ts @@ -306,19 +306,31 @@ describe('recordAgentFindings', () => { const errors = recordRejected({ schema_version: 1, findings: [ - finding({file: '../outside.ts'}), finding({file: '/etc/passwd'}), + finding({file: 'src/a\0.ts'}), finding({evidence: [{file: 'C:\\Windows\\system.ini', line: 1}]}), ], }) expect(errors).toEqual([ - `findings[0] (${tenant.id}): unsafe file path (must be relative and inside the app): ../outside.ts`, - `findings[1] (${tenant.id}): unsafe file path (must be relative and inside the app): /etc/passwd`, + `findings[0] (${tenant.id}): unsafe file path (must be relative): /etc/passwd`, + `findings[1] (${tenant.id}): unsafe file path (must be relative): src/a\0.ts`, `findings[2] (${tenant.id}): unsafe evidence file path: C:\\Windows\\system.ini`, ]) }) + test('accepts findings in a scan directory outside the app directory, with ../ paths', () => { + const document = recordAccepted({ + schema_version: 1, + findings: [finding({file: '../backend/src/server.ts', evidence: [{file: '../library/src/auth.ts', line: 3}]})], + }) + + expect(document.checks[0]!.findings[0]).toMatchObject({ + location: {file: '../backend/src/server.ts'}, + evidence: [{location: {file: '../library/src/auth.ts', line: 3}}], + }) + }) + test('rejects more findings than the cap without validating each one', () => { const errors = recordRejected({schema_version: 1, findings: Array.from({length: 1_001}, () => ({}))}) 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 new file mode 100644 index 00000000000..c2b2b7dc303 --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/tests/scan-directories.test.ts @@ -0,0 +1,320 @@ +/* eslint-disable no-restricted-imports -- scan directories use real temporary repositories and directories */ +import {git, isolateGitConfig} from './git-test-helpers.js' +import {configureRepositoryReader, gatherPaths} from '../scanners/discover.js' +import {createPathRules} from '../scanners/path-rules.js' +import {scan} from '../scanners/index.js' +import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' +import {mkdir, mkdtemp, realpath, rm, symlink, writeFile} from 'node:fs/promises' +import {tmpdir} from 'node:os' +import {basename, join} from 'node:path' + +const temporaryDirectories: string[] = [] +const appConfiguration = 'name = "Scan directories"\napplication_url = "https://example.com"\n' +const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') + +let restoreGitConfig: (() => void) | undefined + +beforeEach(() => { + restoreGitConfig = isolateGitConfig() +}) + +afterEach(async () => { + restoreGitConfig?.() + await Promise.all(temporaryDirectories.splice(0).map((directory) => rm(directory, {recursive: true, force: true}))) +}) + +/** The real path, which is what the CLI's resolver gives the engine. */ +async function makeDirectory(files: Record = {}): Promise { + const directory = await realpath(await mkdtemp(join(tmpdir(), 'app-security-scan-directories-'))) + temporaryDirectories.push(directory) + await writeFiles(directory, files) + return directory +} + +async function writeFiles(root: string, files: Record): Promise { + await Promise.all( + Object.entries(files).map(async ([path, content]) => { + const fullPath = join(root, path) + await mkdir(join(fullPath, '..'), {recursive: true}) + await writeFile(fullPath, content) + }), + ) +} + +/** Initializes the repository in `directory` when given, otherwise in a new temporary directory. */ +async function makeRepository(files: Record, directory?: string): Promise { + const root = directory ?? (await makeDirectory()) + await mkdir(root, {recursive: true}) + git(root, ['init', '-q', '.']) + await writeFiles(root, files) + return root +} + +function commitEverything(repository: string): void { + git(repository, ['add', '-A']) + git(repository, ['commit', '-qm', 'init']) +} + +interface ScanDirectoriesInput { + appDirectory: string + scanDirectories: string[] + requestedScanDirectories?: string[] + excludePatterns?: string[] +} + +async function gather(input: ScanDirectoriesInput) { + configureRepositoryReader({ + appDirectory: input.appDirectory, + scanDirectories: input.scanDirectories, + explicitInputs: new Set(), + }) + return gatherPaths({ + appDirectory: input.appDirectory, + scanDirectories: input.scanDirectories, + requestedScanDirectories: input.requestedScanDirectories ?? input.scanDirectories, + rules: createPathRules({excludePatterns: input.excludePatterns ?? [], noGitIgnore: false}), + }) +} + +function scanAll(input: ScanDirectoriesInput) { + return scan( + { + appDirectory: input.appDirectory, + scanDirectories: input.scanDirectories, + requestedScanDirectories: input.requestedScanDirectories ?? input.scanDirectories, + appConfigFilePath: join(input.appDirectory, 'shopify.app.toml'), + }, + {excludePatterns: input.excludePatterns ?? []}, + ) +} + +describe('two repositories', () => { + async function makeTwoRepositories() { + const app = await makeRepository({ + '.gitignore': 'local.txt\n', + 'shopify.app.toml': appConfiguration, + 'src/a.ts': 'export const a = true', + 'local.txt': 'ignored by the app repository', + }) + const backend = await makeRepository({ + '.gitignore': 'dist/\n', + 'src/server.ts': 'export const server = true', + 'dist/out.js': 'ignored by the backend repository', + 'local.txt': 'not ignored by the backend repository', + }) + return {app, backend} + } + + test('applies each repository its own ignore rules, with paths relative to the app directory', async () => { + const {app, backend} = await makeTwoRepositories() + const backendName = basename(backend) + + const result = await gather({appDirectory: app, scanDirectories: [app, backend]}) + + expect(result.paths).toEqual( + [ + `../${backendName}/.gitignore`, + `../${backendName}/local.txt`, + `../${backendName}/src/server.ts`, + '.gitignore', + 'shopify.app.toml', + 'src/a.ts', + ].sort(), + ) + expect(result.ignoredScanDirectories).toEqual([]) + }) + + test('matches --exclude against the working directory, whichever repository it is', async () => { + const {app, backend} = await makeTwoRepositories() + const backendName = basename(backend) + const scanDirectories = [app, backend] + + vi.stubEnv('INIT_CWD', app) + const fromApp = await gather({appDirectory: app, scanDirectories, excludePatterns: [`../${backendName}/src`]}) + vi.stubEnv('INIT_CWD', backend) + const fromBackend = await gather({appDirectory: app, scanDirectories, excludePatterns: ['src']}) + + expect(fromApp.paths).not.toContain(`../${backendName}/src/server.ts`) + expect(fromApp.paths).toContain('src/a.ts') + expect(fromBackend.paths).toEqual(fromApp.paths) + }) +}) + +describe('a library outside any repository', () => { + test('gathers every file in it, since no repository has ignore rules for it', async () => { + const app = await makeRepository({ + 'shopify.app.toml': appConfiguration, + 'src/a.ts': 'export const a = true', + }) + const library = await makeDirectory({ + 'index.ts': 'export const library = true', + 'node_modules/dependency/index.js': 'module.exports = {}', + }) + const libraryName = basename(library) + + const result = await gather({appDirectory: app, scanDirectories: [app, library]}) + + expect(result.paths).toEqual( + [ + `../${libraryName}/index.ts`, + `../${libraryName}/node_modules/dependency/index.js`, + 'shopify.app.toml', + 'src/a.ts', + ].sort(), + ) + }) +}) + +describe('a scan directory inside another one', () => { + test('is gathered once, by the walk of the outer directory', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration, 'lib/index.ts': 'export const lib = true'}) + + const result = await gather({ + appDirectory: app, + scanDirectories: [app], + requestedScanDirectories: [app, join(app, 'lib')], + }) + + expect(result.paths).toEqual(['lib/index.ts', 'shopify.app.toml']) + expect(result.ignoredScanDirectories).toEqual([]) + }) + + test('still gets the ignored-scan-directory warning, although the outer walk prunes it', async () => { + const app = await makeRepository({ + '.gitignore': 'vendor/\n', + 'shopify.app.toml': appConfiguration, + 'src/a.ts': 'export const a = true', + }) + const sdk = await makeRepository({'sdk.ts': 'export const sdk = true'}, join(app, 'vendor', 'sdk')) + + const result = await gather({ + appDirectory: app, + scanDirectories: [app], + requestedScanDirectories: [app, sdk], + }) + + expect(result.paths).toEqual(['.gitignore', 'shopify.app.toml', 'src/a.ts']) + expect(result.ignoredScanDirectories).toEqual([sdk]) + }) + + test('gathers an app directory that sits inside the only scan directory, with ../ paths', async () => { + const monorepo = await makeRepository({ + 'app/shopify.app.toml': appConfiguration, + 'app/src/a.ts': 'export const a = true', + 'backend/server.ts': 'export const server = true', + }) + + const result = await gather({appDirectory: join(monorepo, 'app'), scanDirectories: [monorepo]}) + + expect(result.paths).toEqual(['../backend/server.ts', 'shopify.app.toml', 'src/a.ts']) + }) +}) + +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}) + const library = await makeRepository({'.env': `SHOPIFY_API_SECRET=${secret}\n`}) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + + const issue = result.issues.find((candidate) => candidate.id === 'COMMITTED_SECRET') + expect(issue?.location.file).toBe(`../${basename(library)}/.env`) + expect(result.scan.files_skipped_count).toBe(0) + }) + + test('lists a symbolic link in an include directory at its own location and refuses one that leaves it', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration}) + const library = await makeDirectory({'index.ts': 'export const library = true'}) + const outside = await makeDirectory({'leak.ts': 'export const leaked = true'}) + await symlink(join(outside, 'leak.ts'), join(library, 'link.ts')) + const libraryName = basename(library) + + const gathered = await gather({appDirectory: app, scanDirectories: [app, library]}) + expect(gathered.paths).toEqual([`../${libraryName}/index.ts`, `../${libraryName}/link.ts`, 'shopify.app.toml']) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + expect(result.scan.files_skipped).toContainEqual( + expect.objectContaining({ + path: `../${libraryName}/link.ts`, + reason: 'unreadable', + detail: `../${libraryName}/link.ts resolves outside the scan directory`, + }), + ) + expect(JSON.stringify(result)).not.toContain(outside) + }) + + test('refuses a symbolic link into another scan directory, since the boundary is the directory containing it', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration, 'src/a.ts': 'export const a = true'}) + const library = await makeDirectory({'index.ts': 'export const library = true'}) + await symlink(join(app, 'src', 'a.ts'), join(library, 'to-app.ts')) + const libraryName = basename(library) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + + expect(result.scan.files_skipped).toContainEqual( + expect.objectContaining({ + path: `../${libraryName}/to-app.ts`, + detail: `../${libraryName}/to-app.ts resolves outside the scan directory`, + }), + ) + }) + + test('gathers a symbolic-linked include directory under its real path', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration}) + const library = await makeDirectory({'index.ts': 'export const library = true'}) + const links = await makeDirectory() + await symlink(library, join(links, 'alias')) + + // The resolver hands the engine real paths, so the link's name never appears. + const result = await gather({appDirectory: app, scanDirectories: [app, await realpath(join(links, 'alias'))]}) + + expect(result.paths).toEqual([`../${basename(library)}/index.ts`, 'shopify.app.toml']) + }) +}) + +describe('the secret scan in a second repository', () => { + test('uses the repository that owns the file, so a tracked secret is reported as tracked', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration}) + commitEverything(app) + const library = await makeRepository({'.env': `SHOPIFY_API_SECRET=${secret}\n`}) + commitEverything(library) + const libraryName = basename(library) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + + const issue = result.issues.find((candidate) => candidate.id === 'COMMITTED_SECRET') + expect(issue).toMatchObject({ + location: {file: `../${libraryName}/.env`}, + title: 'Environment file with secrets is tracked by git', + pattern_id: 'environment-file:tracked', + }) + expect(issue?.message).toContain(`../${libraryName}/.env IS TRACKED BY GIT`) + }) + + test('uses the ignore rules of the repository that owns the file, not the app repository', async () => { + const app = await makeRepository({'.gitignore': '.env\n', 'shopify.app.toml': appConfiguration}) + commitEverything(app) + const library = await makeRepository({'.env': `SHOPIFY_API_SECRET=${secret}\n`}) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + + const issue = result.issues.find((candidate) => candidate.id === 'COMMITTED_SECRET') + expect(issue).toMatchObject({ + location: {file: `../${basename(library)}/.env`}, + title: 'Environment file with secrets is not ignored by git', + }) + }) + + test('reports a secret in a library outside any repository as unconfirmed', async () => { + const app = await makeRepository({'shopify.app.toml': appConfiguration}) + const library = await makeDirectory({'.env': `SHOPIFY_API_SECRET=${secret}\n`}) + + const result = await scanAll({appDirectory: app, scanDirectories: [app, library]}) + + const issue = result.issues.find((candidate) => candidate.id === 'COMMITTED_SECRET') + expect(issue).toMatchObject({ + location: {file: `../${basename(library)}/.env`}, + title: 'Environment file with secrets could not be confirmed as ignored', + }) + }) +}) diff --git a/packages/app/src/cli/services/app-security-engine/tests/scan-directory.ts b/packages/app/src/cli/services/app-security-engine/tests/scan-directory.ts index 4bb8e0688a1..3a7f804dfbb 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/scan-directory.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/scan-directory.ts @@ -9,6 +9,7 @@ function scanInputFor(appDirectory: string, configName?: string): ScanInput { return { appDirectory, scanDirectories: [appDirectory], + requestedScanDirectories: [appDirectory], appConfigFilePath: joinPath(appDirectory, getAppConfigurationFileName(configName)), } } diff --git a/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts b/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts index 2a0d622ca64..c0ae712b3b3 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/secret-safety.test.ts @@ -322,7 +322,7 @@ describe('git status drives severity, not .gitignore text', () => { }) }) - test('reports a secret inside a nested repository that the app repository ignores by name', async () => { + test("judges a secret inside a nested repository by that repository's rules, not the app repository's", async () => { const dir = removeAfterTest(makeApp({'.gitignore': '.env\n'})) git(dir, ['init', '-q', '.']) const inner = join(dir, 'inner') @@ -334,14 +334,11 @@ describe('git status drives severity, not .gitignore text', () => { const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') expect(finding).toMatchObject({ location: {file: 'inner/.env'}, - title: 'Environment file with secrets is inside a nested git repository', + title: 'Environment file with secrets is not ignored by git', pattern_id: 'environment-file:unconfirmed', }) - expect(finding!.message).toContain('inner/.env belongs to a nested git repository') - expect(finding!.message).not.toContain('could not be confirmed') - const evidence = finding!.detection_evidence?.join(' ') - expect(evidence).toContain('→ ignored') - expect(evidence).toContain('git rev-parse --show-toplevel → differs') + expect(finding!.message).toContain('inner/.env is untracked but not ignored') + expect(finding!.detection_evidence).toContain('git check-ignore -q inner/.env → not ignored') }) test('says the ignored-file listing failed when git ignores a file that was still scanned', async () => { @@ -400,7 +397,7 @@ describe('git status drives severity, not .gitignore text', () => { test('reports tri-state status rather than a boolean guess', async () => { const dir = mkdtempSync(join(tmpdir(), 'app-security-nogit-')) - const status = await gitStatusFor(dir, '.env') + const status = await gitStatusFor({path: '.env', absolutePath: join(dir, '.env')}) // Outside a repo both answers are unknown — not `false`. expect(status.tracked).toBeUndefined() expect(status.ignored).toBeUndefined() @@ -416,7 +413,7 @@ describe('git status drives severity, not .gitignore text', () => { writeFileSync(join(dir, '.gitignore'), '.env*\n!.env.example\n') writeFileSync(join(dir, '.env.example'), 'SHOPIFY_API_KEY=placeholder\n') - const status = await gitStatusFor(dir, '.env.example') + const status = await gitStatusFor({path: '.env.example', absolutePath: join(dir, '.env.example')}) expect(status.ignored).toBe(false) // negated back in rmSync(dir, {recursive: true, force: true}) }) 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 198bcf12ef5..b1f4d53240f 100644 --- a/packages/app/src/cli/services/app-security-engine/types.ts +++ b/packages/app/src/cli/services/app-security-engine/types.ts @@ -76,8 +76,10 @@ export interface ProjectDetection { /** What to scan, already resolved: the engine doesn't look for an app directory or choose a configuration. */ export interface ScanInput { appDirectory: string - /** Absolute real paths of the directories to walk. */ + /** Absolute real paths of the directories to walk. None is inside another. */ scanDirectories: ReadonlyArray + /** Absolute real paths of every directory that was asked for, including those that `scanDirectories` leaves out because they are inside another. */ + requestedScanDirectories: ReadonlyArray /** Absolute path of the selected app configuration file. Absent when scanning without app configuration. */ appConfigFilePath?: string clientId?: string @@ -105,7 +107,7 @@ export interface ScanResult { /** What gathering reports besides the result, for the caller to show. It isn't part of the stored findings. */ export interface ScanOutput extends ScanResult { - /** Absolute paths of the scan directories that their repository ignores, so only the files Git tracks in them were scanned. */ + /** Absolute paths of the requested scan directories that their repository ignores, so only the files Git tracks in them were scanned. */ ignoredScanDirectories: string[] } diff --git a/packages/app/src/cli/services/app-security-selection.test.ts b/packages/app/src/cli/services/app-security-selection.test.ts index e074f478f5b..487569e1550 100644 --- a/packages/app/src/cli/services/app-security-selection.test.ts +++ b/packages/app/src/cli/services/app-security-selection.test.ts @@ -1,8 +1,10 @@ import { clientIdSource, effectiveClientId, + mergeScanDirectories, resolveAppDirectory, resolveAppSecuritySelection, + resolveIncludeDirectories, resultsKey, selectedConfigFileName, type AppSecuritySelection, @@ -16,7 +18,7 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui' -import {beforeEach, describe, expect, test, vi} from 'vitest' +import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import {symlink} from 'node:fs/promises' import type {OrganizationApp} from '../models/organization.js' @@ -37,6 +39,10 @@ beforeEach(() => { vi.mocked(getCachedAppInfo).mockReturnValue(undefined) }) +afterEach(() => { + vi.unstubAllEnvs() +}) + async function writeConfiguration(directory: string, clientId: string, name = 'shopify.app.toml'): Promise { await writeFile(joinPath(directory, name), validAppConfiguration(clientId)) } @@ -489,3 +495,140 @@ describe('resolveAppDirectory', () => { }) }) }) + +describe('resolveIncludeDirectories', () => { + test('resolves each value against the working directory and keeps the order', async () => { + await inTemporaryDirectory(async (directory) => { + const workingDirectory = joinPath(directory, 'work') + await mkdir(workingDirectory) + await mkdir(joinPath(directory, 'backend')) + await mkdir(joinPath(workingDirectory, 'lib')) + vi.stubEnv('INIT_CWD', workingDirectory) + + await expect(resolveIncludeDirectories(['../backend', 'lib', '.'])).resolves.toEqual([ + await fileRealPath(joinPath(directory, 'backend')), + await fileRealPath(joinPath(workingDirectory, 'lib')), + await fileRealPath(workingDirectory), + ]) + }) + }) + + test('gives the real path of a symbolic link', async () => { + await inTemporaryDirectory(async (directory) => { + await mkdir(joinPath(directory, 'real')) + await symlink(joinPath(directory, 'real'), joinPath(directory, 'alias')) + vi.stubEnv('INIT_CWD', directory) + + await expect(resolveIncludeDirectories(['alias'])).resolves.toEqual([ + await fileRealPath(joinPath(directory, 'real')), + ]) + }) + }) + + test('allows the directory of another app', async () => { + await inTemporaryDirectory(async (directory) => { + await mkdir(joinPath(directory, 'other-app')) + await writeConfiguration(joinPath(directory, 'other-app'), 'other-client-id') + vi.stubEnv('INIT_CWD', directory) + + await expect(resolveIncludeDirectories(['other-app'])).resolves.toEqual([ + await fileRealPath(joinPath(directory, 'other-app')), + ]) + }) + }) + + test("aborts with the typed value when the directory doesn't exist", async () => { + await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) + + await expect(resolveIncludeDirectories(['missing'])).rejects.toMatchObject({ + message: "--include-dir missing: directory doesn't exist.", + }) + }) + }) + + test('aborts with the typed value when the path is a file', async () => { + await inTemporaryDirectory(async (directory) => { + await writeFile(joinPath(directory, 'file.txt'), 'x') + vi.stubEnv('INIT_CWD', directory) + + await expect(resolveIncludeDirectories(['./file.txt'])).rejects.toMatchObject({ + message: '--include-dir ./file.txt: not a directory.', + }) + }) + }) + + test('aborts when a symbolic link points at nothing', async () => { + await inTemporaryDirectory(async (directory) => { + await symlink(joinPath(directory, 'gone'), joinPath(directory, 'dangling')) + vi.stubEnv('INIT_CWD', directory) + + await expect(resolveIncludeDirectories(['dangling'])).rejects.toMatchObject({ + message: "--include-dir dangling: directory doesn't exist.", + }) + }) + }) +}) + +describe('mergeScanDirectories', () => { + test('lists the app directory first and then each include directory', () => { + expect(mergeScanDirectories('/work/app', ['/work/backend', '/work/library'])).toEqual({ + scanDirectories: [ + {directory: '/work/app', origin: 'app_directory'}, + {directory: '/work/backend', origin: 'include_dir'}, + {directory: '/work/library', origin: 'include_dir'}, + ], + requestedScanDirectories: ['/work/app', '/work/backend', '/work/library'], + }) + }) + + test('drops a duplicate include directory', () => { + expect(mergeScanDirectories('/work/app', ['/work/backend', '/work/backend']).scanDirectories).toEqual([ + {directory: '/work/app', origin: 'app_directory'}, + {directory: '/work/backend', origin: 'include_dir'}, + ]) + }) + + test('counts an include directory that is the app directory as the app directory', () => { + expect(mergeScanDirectories('/work/app', ['/work/app']).scanDirectories).toEqual([ + {directory: '/work/app', origin: 'app_directory'}, + ]) + }) + + test('drops an include directory inside another scan directory, and keeps it as requested', () => { + expect(mergeScanDirectories('/work/app', ['/work/app/vendor/sdk', '/work/backend']).scanDirectories).toEqual([ + {directory: '/work/app', origin: 'app_directory'}, + {directory: '/work/backend', origin: 'include_dir'}, + ]) + expect(mergeScanDirectories('/work/app', ['/work/app/vendor/sdk']).requestedScanDirectories).toEqual([ + '/work/app', + '/work/app/vendor/sdk', + ]) + }) + + test('drops the app directory when an include directory contains it', () => { + expect(mergeScanDirectories('/work/mono/app', ['/work/mono']).scanDirectories).toEqual([ + {directory: '/work/mono', origin: 'include_dir'}, + ]) + }) + + test('keeps a directory whose name only starts like another directory', () => { + expect(mergeScanDirectories('/work/app', ['/work/app-library']).scanDirectories).toHaveLength(2) + }) + + test('rejects an include directory on another Windows drive', () => { + expect(() => mergeScanDirectories('C:/work/app', ['C:/work/backend', 'D:/backend'])).toThrowError( + new AbortError('--include-dir D:/backend: must be on the same drive as the app directory.'), + ) + }) + + test('rejects an include directory on a Windows network share', () => { + expect(() => mergeScanDirectories('C:/work/app', ['//server/share/backend'])).toThrowError( + new AbortError('--include-dir //server/share/backend: must be on the same drive as the app directory.'), + ) + }) + + test('accepts an include directory elsewhere on the same Windows drive', () => { + expect(mergeScanDirectories('C:/work/app', ['C:/backend']).scanDirectories).toHaveLength(2) + }) +}) diff --git a/packages/app/src/cli/services/app-security-selection.ts b/packages/app/src/cli/services/app-security-selection.ts index 8e6aa017210..3701819835a 100644 --- a/packages/app/src/cli/services/app-security-selection.ts +++ b/packages/app/src/cli/services/app-security-selection.ts @@ -4,7 +4,7 @@ import {fetchOrCreateOrganizationApp} from './context.js' import {NoAppConfigurationFoundError} from '../models/project/project.js' import {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, isDirectory} from '@shopify/cli-kit/node/fs' -import {basename, joinPath} from '@shopify/cli-kit/node/path' +import {basename, cwd, isSubpath, joinPath, normalizePath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' import {renderConfirmationPrompt} from '@shopify/cli-kit/node/ui' export type AppSecuritySelection = @@ -27,8 +27,9 @@ export type AppSecuritySelection = clientIdSource: 'flag' | 'picker' } -/** A directory that is walked. Until `--include-dir` exists, the app directory is the only one. */ +/** A directory that is walked. */ export interface AppSecurityScanDirectory { + /** Absolute; the real path. */ directory: string origin: 'app_directory' | 'include_dir' } @@ -147,6 +148,59 @@ async function resolveWithoutAppConfigurationFile( } } +/** + * Resolves each `--include-dir` value, as typed, against the working directory. Returns real paths in the order + * given. Another app's directory is allowed. + */ +export async function resolveIncludeDirectories(includeDirs: ReadonlyArray): Promise { + const directories: string[] = [] + for (const typedValue of includeDirs) { + const absolutePath = resolvePath(cwd(), typedValue) + // eslint-disable-next-line no-await-in-loop + const realPath = await realPathIfExists(absolutePath) + if (realPath === undefined) throw new AbortError(`--include-dir ${typedValue}: directory doesn't exist.`) + // eslint-disable-next-line no-await-in-loop + if (!(await isDirectory(realPath))) throw new AbortError(`--include-dir ${typedValue}: not a directory.`) + directories.push(realPath) + } + return directories +} + +/** + * The app directory, then each include directory, compared by real path. A duplicate is dropped, so an include + * directory that is the app directory counts as the app directory. A directory inside another one is dropped too, + * because walking the outer one covers it; that includes the app directory when an include directory contains it. + * `requestedScanDirectories` keeps the directories that were dropped for being nested, since each still gets the + * ignored-scan-directory warning. + * + * Aborts on an include directory on another Windows drive or network share. Gathered files are stored relative to the + * app directory and read back with `joinPath`, which can't reach those, so their files would be silently left unscanned. + */ +export function mergeScanDirectories( + appDirectory: string, + includeDirectories: ReadonlyArray, +): {scanDirectories: AppSecurityScanDirectory[]; requestedScanDirectories: string[]} { + const otherDrive = includeDirectories.find( + (directory) => joinPath(appDirectory, relativePath(appDirectory, directory)) !== normalizePath(directory), + ) + if (otherDrive) throw new AbortError(`--include-dir ${otherDrive}: must be on the same drive as the app directory.`) + + const requested = [ + {directory: appDirectory, origin: 'app_directory' as const}, + ...includeDirectories.map((directory) => ({directory, origin: 'include_dir' as const})), + ].filter((candidate, index, all) => all.findIndex(({directory}) => directory === candidate.directory) === index) + + return { + scanDirectories: requested.filter( + (candidate) => + !requested.some( + ({directory}) => directory !== candidate.directory && isSubpath(directory, candidate.directory), + ), + ), + requestedScanDirectories: requested.map(({directory}) => directory), + } +} + function abortNoAppConfigurationFound(directory: string): never { throw new AbortError( `No app configuration found at or above ${directory}.`, diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 71e8dd760e6..995fca705e3 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -1,8 +1,9 @@ import securityCheck, {appSecurityInstructionsPrompt} from './security-check.js' import {formatAppSecurityCommand, resolveAppSecurityCommands} from './app-security-commands.js' import {appSecurityArtifactPaths, writeCheckArtifacts} from './app-security-artifacts.js' -import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' -import {describe, expect, test, vi} from 'vitest' +import {fileRealPath, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {joinPath} from '@shopify/cli-kit/node/path' +import {afterEach, describe, expect, test, vi} from 'vitest' import type {AppSecurityExecution} from './app-security-api.js' import type {AppSecuritySelection} from './app-security-selection.js' import type {AppSecurityInstructionsDestination} from './security-check.js' @@ -120,12 +121,17 @@ function testOptions() { blocking: 'none' as const, yes: false, skipInstructions: false, + includeDirs: [], excludePatterns: [], noGitIgnore: false, } } describe('securityCheck', () => { + afterEach(() => { + vi.unstubAllEnvs() + }) + test('executes, writes artifacts, then renders a report', async () => { const dependencies = testDependencies() @@ -141,6 +147,7 @@ describe('securityCheck', () => { expect(dependencies.execute).toHaveBeenCalledWith({ appDirectory, scanDirectories: [appDirectory], + requestedScanDirectories: [appDirectory], appConfigFilePath: `${appDirectory}/shopify.app.toml`, clientId: 'toml-client-id', excludePatterns: [], @@ -207,6 +214,94 @@ describe('securityCheck', () => { expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) }) + test('scans each --include-dir after the app directory, reports it, and repeats it before --exclude', async () => { + await inTemporaryDirectory(async (directory) => { + await mkdir(joinPath(directory, 'backend')) + vi.stubEnv('INIT_CWD', directory) + const backend = await fileRealPath(joinPath(directory, 'backend')) + const dependencies = testDependencies() + dependencies.canPrompt.mockReturnValue(true) + dependencies.selectInstructionsDestination.mockResolvedValue('print') + const options = {...testOptions(), includeDirs: ['backend', './backend'], excludePatterns: ['generated']} + + await securityCheck(options, dependencies) + + expect(dependencies.execute).toHaveBeenCalledWith( + expect.objectContaining({ + scanDirectories: [appDirectory, backend], + requestedScanDirectories: [appDirectory, backend], + }), + ) + const commands = resolveAppSecurityCommands(appDirectory, 'shopify.app.toml', ['generated'], false, [ + 'backend', + './backend', + ]) + expect(commands.scan.args.slice(-3)).toEqual([ + {flag: '--include-dir', value: 'backend'}, + {flag: '--include-dir', value: './backend'}, + {flag: '--exclude', value: 'generated'}, + ]) + expect(dependencies.renderReport).toHaveBeenCalledWith( + expect.objectContaining({ + commands, + scanDirectories: [ + {directory: appDirectory, origin: 'app_directory'}, + {directory: backend, origin: 'include_dir'}, + ], + }), + ) + expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) + }) + }) + + test('lists absolute scan directories with their origins in the JSON selection', async () => { + await inTemporaryDirectory(async (directory) => { + await mkdir(joinPath(directory, 'backend')) + vi.stubEnv('INIT_CWD', directory) + const backend = await fileRealPath(joinPath(directory, 'backend')) + const dependencies = testDependencies() + + await securityCheck({...testOptions(), json: true, includeDirs: ['backend']}, dependencies) + + expect(JSON.parse(dependencies.output.mock.calls[0]![0]).selection.scan_directories).toEqual([ + {directory: appDirectory, origin: 'app_directory'}, + {directory: backend, origin: 'include_dir'}, + ]) + }) + }) + + test('leaves out an --include-dir inside the app directory, but still passes it for the Git ignore warning', async () => { + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + await mkdir(joinPath(directory, 'vendor')) + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies(scanExecution, {...configSelection, appDirectory: realDirectory}) + + await securityCheck({...testOptions(), includeDirs: ['vendor']}, dependencies) + + expect(dependencies.execute).toHaveBeenCalledWith( + expect.objectContaining({ + scanDirectories: [realDirectory], + requestedScanDirectories: [realDirectory, await fileRealPath(joinPath(directory, 'vendor'))], + }), + ) + }) + }) + + test('aborts on a wrong --include-dir before resolving the selection or scanning', async () => { + await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies() + + await expect(securityCheck({...testOptions(), includeDirs: ['missing']}, dependencies)).rejects.toThrow( + "--include-dir missing: directory doesn't exist.", + ) + + expect(dependencies.resolveSelection).not.toHaveBeenCalled() + expect(dependencies.execute).not.toHaveBeenCalled() + }) + }) + 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']}) @@ -303,6 +398,7 @@ describe('securityCheck', () => { expect(dependencies.execute).toHaveBeenCalledWith({ appDirectory, scanDirectories: [appDirectory], + requestedScanDirectories: [appDirectory], appConfigFilePath: undefined, clientId: 'flag-client-id', excludePatterns: [], diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index aef45489d13..1208cfb9827 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -8,7 +8,9 @@ import { } from './app-security-commands.js' import { effectiveClientId, + mergeScanDirectories, resolveAppSecuritySelection, + resolveIncludeDirectories, resultsKey, selectedConfigFileName, type AppSecurityScanDirectory, @@ -36,6 +38,7 @@ interface SecurityOptions { blocking: AppSecurityBlockingLevel yes: boolean skipInstructions: boolean + includeDirs: ReadonlyArray excludePatterns: ReadonlyArray noGitIgnore: boolean } @@ -139,6 +142,8 @@ export default async function securityCheck( options: SecurityOptions, dependencies: SecurityDependencies = defaultDependencies, ): Promise { + // Resolved first so a mistyped directory fails before any prompt. + const includeDirectories = await resolveIncludeDirectories(options.includeDirs) const canPrompt = !options.json && dependencies.canPrompt() const selection = await dependencies.resolveSelection({ path: options.directory, @@ -153,6 +158,7 @@ export default async function securityCheck( selectedConfigFileName(selection), options.excludePatterns, options.noGitIgnore, + options.includeDirs, ) // The prompt is only shown when no TOML was found and `--without-app-config` wasn't passed. if (selection.kind === 'no-config' && !options.withoutAppConfig) { @@ -161,11 +167,12 @@ export default async function securityCheck( body: [{command: formatAppSecurityCommand(commands.scan)}], }) } - const scanDirectories: AppSecurityScanDirectory[] = [{directory: appDirectory, origin: 'app_directory'}] + const {scanDirectories, requestedScanDirectories} = mergeScanDirectories(appDirectory, includeDirectories) const execution = await dependencies.execute({ appDirectory, scanDirectories: scanDirectories.map(({directory}) => directory), + requestedScanDirectories, appConfigFilePath: selection.kind === 'config' ? selection.appConfigFilePath : undefined, clientId: effectiveClientId(selection), excludePatterns: options.excludePatterns, diff --git a/packages/app/src/cli/services/security-record.test.ts b/packages/app/src/cli/services/security-record.test.ts index 558906cdd3a..3861e27006f 100644 --- a/packages/app/src/cli/services/security-record.test.ts +++ b/packages/app/src/cli/services/security-record.test.ts @@ -313,7 +313,7 @@ describe('securityRecord', () => { checks_executed: [{check_id: FAKE_SHOPIFY_TOKEN, check_version: 1}], findings: [ finding({check_id: FAKE_SHOPIFY_TOKEN}), - finding({file: `../${FAKE_SHOPIFY_TOKEN}`}), + finding({file: `/${FAKE_SHOPIFY_TOKEN}`}), finding({line: FAKE_SHOPIFY_TOKEN}), ], }) diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 9d908586607..050bae3096a 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -4042,6 +4042,13 @@ "name": "exclude", "type": "option" }, + "include-dir": { + "description": "Also scan this directory, relative to the working directory. Repeat the flag to add directories. Use it for code that lives outside the app directory, such as a backend or a shared library.", + "hasDynamicHelp": false, + "multiple": true, + "name": "include-dir", + "type": "option" + }, "json": { "allowNo": false, "char": "j",