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 f6788ada3ec..0dfeec896b9 100644 --- a/packages/app/src/cli/commands/app/security/check.test.ts +++ b/packages/app/src/cli/commands/app/security/check.test.ts @@ -27,8 +27,9 @@ describe('app security check command', () => { 'blocking', 'client-id', 'config', - 'ignore', + 'exclude', 'json', + 'no-git-ignore', 'path', 'skip-instructions', 'without-app-config', @@ -52,43 +53,35 @@ describe('app security check command', () => { blocking: 'high', yes: false, skipInstructions: true, - ignorePatterns: [], + excludePatterns: [], + noGitIgnore: false, }) }) - test('forwards repeated --ignore patterns in command-line order', async () => { + test('forwards repeated --exclude globs exactly as typed, in command-line order', async () => { await SecurityCheck.run( - ['--ignore', 'generated/', '--ignore', '!build/', '--ignore', 'a b/', '--skip-instructions'], + ['--exclude', 'generated', '--exclude', '../shared/**', '--exclude', 'a b/', '--skip-instructions'], import.meta.url, ) expect(securityCheck).toHaveBeenCalledWith( - expect.objectContaining({ignorePatterns: ['generated/', '!build/', 'a b/'], skipInstructions: true}), + expect.objectContaining({excludePatterns: ['generated', '../shared/**', 'a b/'], skipInstructions: true}), ) }) - test.each([ - ['#generated/', 'comment'], - ['', 'empty'], - ['!', 'nothing after'], - ['build\\', 'ends with a backslash'], - ['build\\\\\\', 'ends with a backslash'], - ['src/[id/x.ts', "can't be read as a .gitignore pattern"], - ['build/\ngenerated/', 'single line'], - ])('rejects the unusable --ignore pattern %j', async (value, expectedMessage) => { - const outputMock = mockAndCaptureOutput() - const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) + test('forwards --no-git-ignore', async () => { + await SecurityCheck.run(['--no-git-ignore', '--skip-instructions'], import.meta.url) - try { - await expect(SecurityCheck.run(['--ignore', value, '--skip-instructions'], import.meta.url)).rejects.toThrow( - 'process.exit unexpectedly called with "1"', - ) - expect(outputMock.error()).toContain(expectedMessage) - expect(securityCheck).not.toHaveBeenCalled() - } finally { - consoleErrorSpy.mockRestore() - outputMock.clear() - } + expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({noGitIgnore: true, skipInstructions: true})) + }) + + test('does not read --exclude or --no-git-ignore from the environment', () => { + expect(SecurityCheck.flags.exclude.env).toBeUndefined() + expect(SecurityCheck.flags['no-git-ignore'].env).toBe('SHOPIFY_FLAG_NO_GIT_IGNORE') + }) + + test('rejects the removed --ignore flag', async () => { + await expect(SecurityCheck.run(['--ignore', 'build/', '--skip-instructions'], import.meta.url)).rejects.toThrow() }) test('forwards --yes without requiring an app configuration', async () => { @@ -104,7 +97,8 @@ describe('app security check command', () => { blocking: 'none', yes: true, skipInstructions: false, - ignorePatterns: [], + excludePatterns: [], + noGitIgnore: false, }) }) @@ -165,20 +159,27 @@ describe('app security check command', () => { expect(SecurityCheck.descriptionWithMarkdown).not.toMatch(/--findings|--clean|compile|trace/) }) - test('documents --ignore as ordered .gitignore patterns that the coding-agent instructions repeat', () => { - expect(SecurityCheck.flags.ignore.multiple).toBe(true) - expect(SecurityCheck.flags.ignore.description).toBe( - 'Ignore files that match this .gitignore pattern, relative to the app directory. Start the pattern with ! to include matching files again. Repeat the flag to add patterns; later patterns take precedence.', + test('documents what is scanned, --exclude and --no-git-ignore', () => { + expect(SecurityCheck.flags.exclude.multiple).toBe(true) + expect(SecurityCheck.flags.exclude.description).toBe( + "Skip paths that match this glob, relative to the working directory. Repeat the flag to add globs. The selected app configuration file can't be excluded.", + ) + expect(SecurityCheck.flags['no-git-ignore'].description).toBe( + 'Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.', + ) + expect(SecurityCheck.descriptionWithMarkdown).toContain('scans the app directory and each `--include-dir`') + expect(SecurityCheck.descriptionWithMarkdown).toContain('files that Git tracks are always scanned') + expect(SecurityCheck.descriptionWithMarkdown).toContain('relative to the working directory') + expect(SecurityCheck.descriptionWithMarkdown).toContain("--exclude '**/generated'") + expect(SecurityCheck.descriptionWithMarkdown).toContain( + "An exclusion can't remove the selected app configuration file.", ) - expect(SecurityCheck.descriptionWithMarkdown).toContain('`--ignore`') - expect(SecurityCheck.descriptionWithMarkdown).toContain('relative to the app directory') - expect(SecurityCheck.descriptionWithMarkdown).toContain('later patterns take precedence') - expect(SecurityCheck.descriptionWithMarkdown).toContain("--ignore '!build/'") - expect(SecurityCheck.descriptionWithMarkdown).toContain('single quotes in POSIX shells and PowerShell') + expect(SecurityCheck.descriptionWithMarkdown).toContain('`--no-git-ignore` to turn Git ignore rules off') + expect(SecurityCheck.descriptionWithMarkdown).toContain('`.shopify/app-security//`') expect(SecurityCheck.descriptionWithMarkdown).toContain( - 'The coding-agent instructions this check offers repeat the patterns.', + "Other `app security` commands don't take `--exclude` or `--no-git-ignore`", ) - expect(SecurityCheck.descriptionWithMarkdown).toContain("Other `app security` commands don't take `--ignore`") + expect(SecurityCheck.descriptionWithMarkdown).not.toContain('--ignore') }) test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => { diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 33f7f6018be..919adb35a0b 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -1,11 +1,9 @@ import {appSecurityBlockingFlag} from './blocking-flag.js' import {appSecuritySelectionFlags} from './selection-flags.js' -import {ignorePatternProblem} from '../../../services/app-security-engine/index.js' import securityCheck from '../../../services/security-check.js' import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' -import {AbortError} from '@shopify/cli-kit/node/error' export default class SecurityCheck extends BaseCommand { static hidden = true @@ -17,7 +15,9 @@ export default class SecurityCheck extends BaseCommand { \`deterministic-findings.json\` holds the deterministic scan results. \`agent-checks.json\` holds the checks for your coding agent to investigate; the agent's results are recorded with \`shopify app security record\`. Use \`--config\` to select a specific app configuration when the project has multiple \`shopify.app*.toml\` files; App Security inspects only that configuration. Use \`--client-id\` to replace the configuration's client ID for this run. When no app configuration exists, use \`--without-app-config --client-id \` to scan \`--path\` anyway with config checks skipped; in an interactive terminal the command offers to do this. -Use \`--ignore\` to change which files are scanned. Each value is one \`.gitignore\` pattern relative to the app directory; prefix it with \`!\` to include a file again when it is ignored by default or by \`.gitignore\`. Repeat the flag to add patterns; later patterns take precedence. A file can't be included again while its parent folder is ignored, so include the folder again instead, for example \`--ignore '!build/'\`. Quote each value so your shell doesn't expand \`!\` or \`*\` (single quotes in POSIX shells and PowerShell). The coding-agent instructions this check offers repeat the patterns. Other \`app security\` commands don't take \`--ignore\`, so pass the same patterns each time you run the check. +The check scans the app directory and each \`--include-dir\`. Git ignore rules apply by default: a file or directory that Git ignores is skipped, using the rules of the repository that contains it, while files that Git tracks are always scanned. Use \`--no-git-ignore\` to turn Git ignore rules off for every scanned directory. + +Use \`--exclude\` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with \`../\`, and a name at any depth needs \`**/\`, for example \`--exclude '**/generated'\`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand \`*\`. The coding-agent instructions this check offers repeat the globs. Other \`app security\` commands don't take \`--exclude\` or \`--no-git-ignore\`, so pass the same flags each time you run the check. In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app security instructions\` to print, copy, or write them later.` @@ -28,15 +28,15 @@ In interactive terminals, the command offers to copy the coding-agent instructio ...appSecuritySelectionFlags, // 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 - ignore: Flags.string({ + exclude: Flags.string({ description: - 'Ignore files that match this .gitignore pattern, relative to the app directory. Start the pattern with ! to include matching files again. Repeat the flag to add patterns; later patterns take precedence.', + "Skip paths that match this glob, relative to the working directory. Repeat the flag to add globs. The selected app configuration file can't be excluded.", multiple: true, - parse: async (input) => { - const problem = ignorePatternProblem(input) - if (problem) throw new AbortError(problem) - return input - }, + }), + 'no-git-ignore': Flags.boolean({ + description: + 'Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.', + env: 'SHOPIFY_FLAG_NO_GIT_IGNORE', }), ...jsonFlag, ...appSecurityBlockingFlag, @@ -67,7 +67,8 @@ In interactive terminals, the command offers to copy the coding-agent instructio blocking: flags.blocking, yes: flags.yes, skipInstructions: flags['skip-instructions'], - ignorePatterns: flags.ignore ?? [], + 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 1cd914c84b0..6158f0a619a 100644 --- a/packages/app/src/cli/services/app-security-api.test.ts +++ b/packages/app/src/cli/services/app-security-api.test.ts @@ -8,7 +8,7 @@ import {writeCheckArtifacts} from './app-security-artifacts.js' import {AbortError} from '@shopify/cli-kit/node/error' import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' -import {describe, expect, test} from 'vitest' +import {afterEach, describe, expect, test, vi} from 'vitest' import {symlink} from 'node:fs/promises' import type {Issue} from './app-security-engine/index.js' @@ -17,9 +17,17 @@ function artifactPath(directory: string, name: string): string { } function scanInputFor(directory: string) { - return {appDirectory: directory, appConfigFilePath: joinPath(directory, 'shopify.app.toml')} + return { + appDirectory: directory, + scanDirectories: [directory], + appConfigFilePath: joinPath(directory, 'shopify.app.toml'), + } } +afterEach(() => { + vi.unstubAllEnvs() +}) + async function runSecurity(options: {directory: string; blocking: AppSecurityBlockingLevel}) { const execution = await executeAppSecurity(scanInputFor(options.directory)) const artifacts = await writeCheckArtifacts(options.directory, 'shopify.app', execution) @@ -111,15 +119,16 @@ describe('App Security CLI integration', () => { }) }) - test('forwards --ignore patterns to the scan', async () => { + test('forwards --exclude patterns to the scan', async () => { await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) await createApp(directory) await mkdir(joinPath(directory, 'generated')) await writeFile(joinPath(directory, 'generated', 'client.ts'), 'export const generated = true\n') const scanInput = scanInputFor(directory) const unfiltered = await executeAppSecurity(scanInput) - const filtered = await executeAppSecurity({...scanInput, ignorePatterns: ['generated/']}) + const filtered = await executeAppSecurity({...scanInput, excludePatterns: ['generated']}) expect(unfiltered.scan.scan.files_scanned - filtered.scan.scan.files_scanned).toBe(1) }) diff --git a/packages/app/src/cli/services/app-security-api.ts b/packages/app/src/cli/services/app-security-api.ts index 7f13edf5e70..3c579444670 100644 --- a/packages/app/src/cli/services/app-security-api.ts +++ b/packages/app/src/cli/services/app-security-api.ts @@ -4,6 +4,7 @@ import { type AppSecurityEngineMetadata, type AppSecurityScan, type ScanInput, + type ScanOptions, type Severity, } from './app-security-engine/index.js' @@ -20,11 +21,12 @@ export function securityExitCode(execution: AppSecurityExecution, blocking: AppS } export async function executeAppSecurity({ - ignorePatterns, + excludePatterns, + noGitIgnore, ...scanInput -}: ScanInput & {ignorePatterns?: ReadonlyArray}): Promise { +}: ScanInput & ScanOptions): Promise { const startTime = Date.now() - const result = await scanApp(scanInput, {ignorePatterns}) + const result = await scanApp(scanInput, {excludePatterns, noGitIgnore}) return { ...result, elapsedMilliseconds: Date.now() - startTime, 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 30f17696163..f49c739aebf 100644 --- a/packages/app/src/cli/services/app-security-commands.test.ts +++ b/packages/app/src/cli/services/app-security-commands.test.ts @@ -169,8 +169,13 @@ describe('resolveAppSecurityCommands', () => { expect(commands.clean.args).toEqual(['app', 'security', 'clean', {flag: '--path', value: '/tmp/app'}]) }) - test('repeats --ignore patterns in order, after --config, on scan only', () => { - const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml', ['generated/', '!build/']) + test('repeats --exclude globs in order, after --config, then --no-git-ignore, on scan only', () => { + const commands = resolveAppSecurityCommands( + '/tmp/app', + 'shopify.app.staging.toml', + ['generated', '../shared/**'], + true, + ) expect(commands.scan.args).toEqual([ 'app', @@ -178,16 +183,17 @@ describe('resolveAppSecurityCommands', () => { 'check', {flag: '--path', value: '/tmp/app'}, {flag: '--config', value: 'staging'}, - {flag: '--ignore', value: 'generated/'}, - {flag: '--ignore', value: '!build/'}, + {flag: '--exclude', value: 'generated'}, + {flag: '--exclude', value: '../shared/**'}, + '--no-git-ignore', ]) expect(commands.record.args).toEqual(['app', 'security', 'record', {flag: '--path', value: '/tmp/app'}]) expect(commands.review.args).toEqual(['app', 'security', 'review', {flag: '--path', value: '/tmp/app'}]) expect(commands.clean.args).toEqual(['app', 'security', 'clean', {flag: '--path', value: '/tmp/app'}]) }) - test('omits --ignore when there are no patterns', () => { - expect(resolveAppSecurityCommands('/tmp/app', undefined, []).scan.args).toEqual([ + test('omits --exclude and --no-git-ignore when they were not passed', () => { + expect(resolveAppSecurityCommands('/tmp/app', undefined, [], false).scan.args).toEqual([ 'app', 'security', 'check', @@ -229,9 +235,9 @@ describe('resolveAppSecurityCommands', () => { }) describe('formatAppSecurityCommand', () => { - test('quotes --ignore patterns so the shell does not expand `!`, `*`, or spaces', () => { - const ignorePatterns = ['!build/', '*.log', 'a b/'] - const commands = resolveAppSecurityCommands('/tmp/app', undefined, ignorePatterns) + test('quotes --exclude globs so the shell does not expand `!`, `*`, or spaces', () => { + const excludePatterns = ['!build/', '*.log', 'a b/'] + const commands = resolveAppSecurityCommands('/tmp/app', undefined, excludePatterns) for (const shell of ['posix', 'cmd', 'powershell'] as const) { const formatted = formatAppSecurityCommand(commands.scan, shell) @@ -242,30 +248,30 @@ describe('formatAppSecurityCommand', () => { 'check', '--path', '/tmp/app', - '--ignore', + '--exclude', '!build/', - '--ignore', + '--exclude', '*.log', - '--ignore', + '--exclude', 'a b/', ]) expect(formatted).not.toMatch(/ !build\//) expect(formatted).not.toMatch(/ \*\.log/) } expect(formatAppSecurityCommand(commands.scan, 'posix')).toContain( - "--ignore '!build/' --ignore '*.log' --ignore 'a b/'", + "--exclude '!build/' --exclude '*.log' --exclude 'a b/'", ) expect(formatAppSecurityCommand(commands.scan, 'powershell')).toContain( - "--ignore '!build/' --ignore '*.log' --ignore 'a b/'", + "--exclude '!build/' --exclude '*.log' --exclude 'a b/'", ) expect(formatAppSecurityCommand(commands.scan, 'cmd')).toContain( - '--ignore "!build/" --ignore "*.log" --ignore "a b/"', + '--exclude "!build/" --exclude "*.log" --exclude "a b/"', ) }) - test('quotes an --ignore pattern that starts with `-` or repeats a command word', () => { - const ignorePatterns = ['-*.log', '-tmp/', 'check'] - const commands = resolveAppSecurityCommands('/tmp/app', undefined, ignorePatterns) + test('quotes an --exclude glob that starts with `-` or repeats a command word', () => { + const excludePatterns = ['-*.log', '-tmp/', 'check'] + const commands = resolveAppSecurityCommands('/tmp/app', undefined, excludePatterns) for (const shell of ['posix', 'cmd', 'powershell'] as const) { const formatted = formatAppSecurityCommand(commands.scan, shell) @@ -276,33 +282,33 @@ describe('formatAppSecurityCommand', () => { 'check', '--path', '/tmp/app', - '--ignore', + '--exclude', '-*.log', - '--ignore', + '--exclude', '-tmp/', - '--ignore', + '--exclude', 'check', ]) expect(formatted).not.toMatch(/ -\*\.log/) expect(formatted).not.toMatch(/ -tmp\//) - expect(formatted).not.toMatch(/--ignore check/) + expect(formatted).not.toMatch(/--exclude check/) } expect(formatAppSecurityCommand(commands.scan, 'posix')).toContain( - "--ignore '-*.log' --ignore '-tmp/' --ignore 'check'", + "--exclude '-*.log' --exclude '-tmp/' --exclude 'check'", ) expect(formatAppSecurityCommand(commands.scan, 'powershell')).toContain( - "--ignore '-*.log' --ignore '-tmp/' --ignore 'check'", + "--exclude '-*.log' --exclude '-tmp/' --exclude 'check'", ) expect(formatAppSecurityCommand(commands.scan, 'cmd')).toContain( - '--ignore "-*.log" --ignore "-tmp/" --ignore "check"', + '--exclude "-*.log" --exclude "-tmp/" --exclude "check"', ) }) test('leaves the command words and every flag name bare and quotes every flag value', () => { - const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml', ['generated/']) + const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml', ['generated'], true) expect(formatAppSecurityCommand(commands.scan, 'posix')).toBe( - "shopify app security check --path '/tmp/app' --config 'staging' --ignore 'generated/'", + "shopify app security check --path '/tmp/app' --config 'staging' --exclude 'generated' --no-git-ignore", ) expect(formatAppSecurityCommand(commands.clean, 'posix')).toBe("shopify app security clean --path '/tmp/app'") }) diff --git a/packages/app/src/cli/services/app-security-commands.ts b/packages/app/src/cli/services/app-security-commands.ts index 6e02f06e7dd..6fca5f18e53 100644 --- a/packages/app/src/cli/services/app-security-commands.ts +++ b/packages/app/src/cli/services/app-security-commands.ts @@ -19,15 +19,16 @@ export interface AppSecurityCommands { clean: AppSecurityCommand } -/** `ignorePatterns` are repeated on scan so that rerunning the check discovers the same files. */ +/** `excludePatterns` and `noGitIgnore` are repeated on scan so that rerunning the check gathers the same files. */ export function resolveAppSecurityCommands( appRoot: string, configFileName?: string, - ignorePatterns: ReadonlyArray = [], + excludePatterns: ReadonlyArray = [], + noGitIgnore = false, ): 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 or --ignore. + // Only check reads the app configuration and discovers files, so it's the only command that takes --config, --exclude or --no-git-ignore. const subcommandArgs = (subcommand: string): AppSecurityArgument[] => [ 'app', 'security', @@ -41,7 +42,8 @@ export function resolveAppSecurityCommands( args: [ ...subcommandArgs('check'), ...(configFlag ? [{flag: '--config', value: configFlag}] : []), - ...ignorePatterns.map((ignorePattern) => ({flag: '--ignore', value: ignorePattern})), + ...excludePatterns.map((excludePattern) => ({flag: '--exclude', value: excludePattern})), + ...(noGitIgnore ? ['--no-git-ignore'] : []), ], }, record: {command, args: subcommandArgs('record'), stdinPlaceholder: ''}, diff --git a/packages/app/src/cli/services/app-security-engine/index.ts b/packages/app/src/cli/services/app-security-engine/index.ts index a3beb2ce625..16783b3859a 100644 --- a/packages/app/src/cli/services/app-security-engine/index.ts +++ b/packages/app/src/cli/services/app-security-engine/index.ts @@ -5,7 +5,7 @@ * types: read the git state, scan, record agent findings, translate * a stored findings document (deterministic-findings.json or agent-findings.json, which share the * converged FindingsDocument schema), combine the two result files into per-check results, group - * issues for display, and validate `--ignore` patterns. The stored documents' Zod + * issues for display. The stored documents' Zod * schemas are exported too, so the public `review --json` schema is composed from them rather than re-declared. * Keep scanners, registries, validators, redaction, and the rest of the stored-schema details inside the engine. */ @@ -43,7 +43,6 @@ export { RECORD_INPUT_SCHEMA_VERSION, SEVERITY_RANK, } from './types.js' -export {ignorePatternProblem} from './scanners/path-rules.js' export {groupIssues, skippedFileCounts} from './output/group-issues.js' export type {IssueGroup} from './output/group-issues.js' export type { @@ -56,6 +55,7 @@ export type { FindingsSource, Issue, ScanInput, + ScanOptions, ScanResult, Severity, StoredCheck, 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 952f1badc6b..f76f93e27f8 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,7 +1,6 @@ import {dirname, joinPath} from '@shopify/cli-kit/node/path' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' -import type {SourceFile} from './types.js' -import type {GitIgnoreListing} from '../scanners/path-rules.js' +import type {ScanContext, SourceFile} from './types.js' import type {Issue} from '../types.js' /** @@ -147,7 +146,7 @@ function isEnvFile(path: string): boolean { } /** Why a file git reports as untracked and ignored still reached this rule. */ -type IgnoredFileScanReason = 'enclosing-repository' | 'nested-repository' | 'listing-failed' | 'unknown' +type IgnoredFileScanReason = 'nested-repository' | 'listing-failed' | 'unknown' // A missing git binary resolves with exit code 0 and empty output. async function gitTopLevel(cwd: string): Promise { @@ -159,10 +158,9 @@ async function gitTopLevel(cwd: string): Promise { async function ignoredFileScanReason( appRoot: string, path: string, - gitIgnoreListing: GitIgnoreListing['status'], + gitIgnoreListing: ScanContext['gitIgnoreListing'], appTopLevel: () => Promise, ): Promise<{reason: IgnoredFileScanReason; evidence: string[]}> { - if (gitIgnoreListing === 'app-root-ignored') return {reason: 'enclosing-repository', evidence: []} if (gitIgnoreListing === 'failed') return {reason: 'listing-failed', evidence: []} if (gitIgnoreListing === 'not-a-repository') return {reason: 'unknown', evidence: []} @@ -201,10 +199,6 @@ function committedSecretFileIssue( title = `${kind} is not ignored by git` message = `${file.path} is untracked but not ignored. If committed, its contents enter repository history.` fixDescription = `Add ${file.path} to .gitignore, confirm with 'git check-ignore ${file.path}', and rotate any exposed secrets` - } else if (ignoredScanReason === 'enclosing-repository') { - title = `${kind} is ignored by a repository that does not own this app` - message = `${file.path} is ignored by an enclosing git repository that ignores the whole app folder, so those rules don't protect the app and it was scanned.` - fixDescription = `Ignore ${file.path} in the repository that owns the app and rotate any exposed secrets` } else if (ignoredScanReason === 'nested-repository') { title = `${kind} is inside a nested git repository` message = `${file.path} belongs to a nested git repository (its top level differs from the app's), so this app's ignore rules don't protect it and it was scanned.` @@ -243,7 +237,7 @@ function committedSecretFileIssue( export async function scanCommittedSecrets( secretEvidenceFiles: SourceFile[], appRoot: string, - gitIgnoreListing: GitIgnoreListing['status'], + gitIgnoreListing: ScanContext['gitIgnoreListing'], ): Promise { const issues: Issue[] = [] @@ -282,6 +276,8 @@ export async function scanCommittedSecrets( let ignoredScanReason: IgnoredFileScanReason = 'unknown' let evidence = status.evidence ?? [] if (status.tracked === false && status.ignored === true) { + // 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) ignoredScanReason = scanReason.reason diff --git a/packages/app/src/cli/services/app-security-engine/rules/types.ts b/packages/app/src/cli/services/app-security-engine/rules/types.ts index 5c589a64431..e77ce099d98 100644 --- a/packages/app/src/cli/services/app-security-engine/rules/types.ts +++ b/packages/app/src/cli/services/app-security-engine/rules/types.ts @@ -1,5 +1,5 @@ import type {Issue, Capabilities, ProjectDetection, Severity, SourceCandidate} from '../types.js' -import type {GitIgnoreListing} from '../scanners/path-rules.js' +import type {GatheredListingStatus} from '../scanners/path-rules.js' import type { AppTomlContent, DependencyAutomationInputs, @@ -57,6 +57,6 @@ export interface ScanContext { detection: ProjectDetection /** Path-only inventory, including unsupported source candidates. */ sourceCandidates: SourceCandidate[] - /** Outcome of listing git's ignored paths. */ - gitIgnoreListing: GitIgnoreListing['status'] + /** Outcome of listing git's ignored paths for the first scan directory. */ + gitIgnoreListing: GatheredListingStatus } 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 411ad00ca82..451e428cdb9 100644 --- a/packages/app/src/cli/services/app-security-engine/run.ts +++ b/packages/app/src/cli/services/app-security-engine/run.ts @@ -15,6 +15,8 @@ export interface AppSecurityEngineMetadata { 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[] deterministicFindings: DeterministicFindingsDocument agentChecks: AgentChecks engine: AppSecurityEngineMetadata @@ -25,11 +27,12 @@ export function getAgentInstructions(): string { } export async function scanApp(input: ScanInput, options?: ScanOptions): Promise { - const result = await scan(input, options) + const {ignoredScanDirectories, ...result} = await scan(input, options) const engineVersion = getEngineVersion() const deterministicFindings = buildDeterministicFindings(result, {engineVersion}) return { scan: result, + ignoredScanDirectories, 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 22410aa9c5f..2028c621fbd 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 @@ -1,13 +1,23 @@ import {inspectErrorReason, isMissingFilesystemEntry} from './filesystem-errors.js' -import {createFilePathMatcher, createPathMatcher} from './path-rules.js' +import { + isDroppedEntry, + isDroppedTrackedPath, + isIgnoredByParentRepository, + listGitIgnoredPaths, + listNestedRepository, + listTrackedFiles, + repositoryIgnoredPaths, +} from './path-rules.js' import {findRepositoryMarker} from './repository-marker.js' import {DEPENDENCY_AUTOMATION_CONFIG_PATHS} from '../rules/dependency-automation-rules.js' import {isValidFormatAppConfigurationFileName} from '../../../models/app/config-file-naming.js' import {AppAccessScopesSchema, AppAuthSchema} from '../../../models/extensions/specifications/app_config_app_access.js' import {WebhookSubscriptionSchema} from '../../../models/extensions/specifications/app_config_webhook_schemas/webhook_subscription_schema.js' import {removeTrailingSlash} from '../../../models/extensions/specifications/validation/common.js' +import {AbortError, BugError} from '@shopify/cli-kit/node/error' import {fileSizeSync, readFileSync} from '@shopify/cli-kit/node/fs' import { + cwd, basename, dirname, extname, @@ -21,7 +31,7 @@ import { import {zod} from '@shopify/cli-kit/node/schema' import {decodeToml} from '@shopify/cli-kit/node/toml/codec' import {lstatSync, readdirSync, realpathSync} from 'node:fs' -import type {PathRules} from './path-rules.js' +import type {GatheredListingStatus, PathRules, RepositoryIgnoredPaths} from './path-rules.js' import type {SourceCandidate} from '../types.js' import type { AppTomlContent, @@ -37,7 +47,7 @@ import type {Dirent} from 'node:fs' * Load the selected shopify.app.toml file. */ export function loadAppToml(appConfigFilePath: string, appDirectory: string): AppTomlContent | null { - const content = readRepositoryText(appDirectory, appConfigFilePath) + const content = readRepositoryText(appConfigFilePath) if (content === undefined) return null try { const raw = decodeToml(content) as Record @@ -45,7 +55,7 @@ export function loadAppToml(appConfigFilePath: string, appDirectory: string): Ap // Invalid repository TOML is a coverage gap, not a scanner crash. // eslint-disable-next-line no-catch-all/no-catch-all } catch { - recordSkippedFile(appDirectory, appConfigFilePath, { + recordSkippedFile(appConfigFilePath, { ok: false, reason: 'unreadable', detail: 'TOML could not be parsed', @@ -171,60 +181,149 @@ function projectWebhookSubscription(value: unknown, path: string, appRoot?: stri function recordSectionGap(appRoot: string | undefined, path: string, detail: string): void { if (!appRoot) return - recordSkippedFile(appRoot, path, {ok: false, reason: 'unreadable', detail}) -} - -/** Uses raw entries, so a nested app whose configuration file is gitignored is still a nested app. */ -function isNestedAppDirectory(entries: ReadonlyArray): boolean { - return entries.some((entry) => !entry.isDirectory() && isValidFormatAppConfigurationFileName(entry.name)) + recordSkippedFile(path, {ok: false, reason: 'unreadable', detail}) } -function readDirectoryEntries(appRoot: string, absolutePath: string, displayPath: string): Dirent[] | undefined { +function readDirectoryEntries(absolutePath: string, isScanDirectory: boolean): Dirent[] | undefined { try { return readdirSync(absolutePath, {withFileTypes: true}) // An unreadable directory is a coverage gap, not a scanner crash. // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { - recordSkippedFile(appRoot, absolutePath, { + recordSkippedFile(absolutePath, { ok: false, reason: 'unreadable', - detail: inspectErrorReason(displayPath, error), + detail: inspectErrorReason(isScanDirectory ? 'scan directory' : repositoryDisplayPath(absolutePath), error), }) return undefined } } +interface GatherInput { + appDirectory: string + /** Absolute real paths of the directories to walk. */ + scanDirectories: ReadonlyArray + /** Added after filtering, so no path rule can remove it. */ + selectedAppConfigFilePath?: string + rules: PathRules +} + +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. */ + 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 +} + +interface GatheredScanDirectory { + absolutePaths: string[] + ignored: boolean + listingStatus: GatheredPaths['listingStatus'] +} + +export async function gatherPaths({ + appDirectory, + scanDirectories, + selectedAppConfigFilePath, + rules, +}: GatherInput): Promise { + const gathered: GatheredScanDirectory[] = [] + for (const scanDirectory of scanDirectories) { + // eslint-disable-next-line no-await-in-loop + gathered.push(await gatherScanDirectory(scanDirectory, rules)) + } + + const absolutePaths = [ + ...gathered.flatMap((scanDirectory) => scanDirectory.absolutePaths), + ...(selectedAppConfigFilePath ? [selectedAppConfigFilePath] : []), + ] + return { + paths: [...new Set(absolutePaths.map((path) => normalizeCliPath(relativePath(appDirectory, path))))].sort(), + ignoredScanDirectories: scanDirectories.filter((_directory, index) => gathered[index]?.ignored), + listingStatus: gathered[0]?.listingStatus ?? (rules.gitFiltering ? 'tracked-only' : 'git-ignore-off'), + } +} + +async function gatherScanDirectory(scanDirectory: string, rules: PathRules): Promise { + if (!rules.gitFiltering) { + return { + absolutePaths: await walkDirectory(scanDirectory, rules, undefined), + ignored: false, + listingStatus: 'git-ignore-off', + } + } + + if (await isIgnoredByParentRepository(scanDirectory)) { + const trackedPaths = await listTrackedFiles(scanDirectory) + // A normal walk would scan the untracked files that Git was told to ignore. + if (trackedPaths === undefined) { + throw new AbortError(`Couldn't list the files Git tracks in ${relativePath(cwd(), scanDirectory) || '.'}.`) + } + return { + absolutePaths: trackedPaths + .filter((trackedPath) => !isDroppedTrackedPath(rules, scanDirectory, trackedPath)) + .map((trackedPath) => joinPath(scanDirectory, trackedPath)) + .filter(isTrackedFile), + ignored: true, + listingStatus: 'tracked-only', + } + } + + const listing = await listGitIgnoredPaths(scanDirectory) + return { + absolutePaths: await walkDirectory(scanDirectory, rules, repositoryIgnoredPaths(scanDirectory, listing)), + ignored: false, + listingStatus: listing.status, + } +} + +/** Git lists a submodule as one entry, and a deleted tracked file is still listed. */ +function isTrackedFile(absolutePath: string): boolean { + try { + return !lstatSync(absolutePath).isDirectory() + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + /** * Symlinks and special entries are listed but never traversed; * `readRepositoryFile` enforces containment on anything a finder reads. */ -export function listRepositoryFiles(appRoot: string, rules: PathRules): string[] { - const matcher = createPathMatcher(rules) +async function walkDirectory( + scanDirectory: string, + rules: PathRules, + scanDirectoryRepository: RepositoryIgnoredPaths | undefined, +): Promise { const files: string[] = [] - // Appended to while iterating; '' is the app root. - const pendingDirectories = [''] - - for (const relativeDirectory of pendingDirectories) { - const absoluteDirectory = relativeDirectory === '' ? appRoot : joinPath(appRoot, relativeDirectory) - const entries = readDirectoryEntries( - appRoot, - absoluteDirectory, - relativeDirectory === '' ? 'app root' : relativeDirectory, - ) + // Appended to while iterating. + const pendingDirectories = [{directory: scanDirectory, repository: scanDirectoryRepository}] + + for (const {directory, repository} of pendingDirectories) { + const entries = readDirectoryEntries(directory, directory === scanDirectory) if (entries === undefined) continue - if (relativeDirectory !== '' && isNestedAppDirectory(entries)) continue for (const entry of entries) { - const relative = relativeDirectory === '' ? entry.name : `${relativeDirectory}/${entry.name}` - if (entry.isDirectory()) { - if (!matcher(relative, {directory: true})) pendingDirectories.push(relative) - } else if (!matcher(relative, {directory: false})) { - files.push(relative) + const absolutePath = joinPath(directory, entry.name) + const isDirectory = entry.isDirectory() + if (isDroppedEntry(rules, repository, {absolutePath, isDirectory})) continue + if (!isDirectory) { + files.push(absolutePath) + continue } + // eslint-disable-next-line no-await-in-loop + const nestedRepository = rules.gitFiltering ? await listNestedRepository(absolutePath) : undefined + pendingDirectories.push({ + directory: absolutePath, + repository: nestedRepository ? repositoryIgnoredPaths(absolutePath, nestedRepository) : repository, + }) } } - return files.sort() + return files } /** A path inside nested extension directories belongs to each of them. */ @@ -269,7 +368,7 @@ export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray { const fullPath = joinPath(appRoot, tomlPath) - const content = readRepositoryText(appRoot, fullPath) + const content = readRepositoryText(fullPath) if (content === undefined) return [] try { @@ -280,7 +379,7 @@ export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray() -export function resetSkippedFiles(): void { +interface RepositoryReaderConfiguration { + appDirectory: string + /** Absolute real paths. The containment boundary for a path is the scan directory that contains it. */ + scanDirectories: ReadonlyArray + /** Absolute paths that skip only the containment check; the size limit still applies. */ + explicitInputs: ReadonlySet +} + +let readerConfiguration: RepositoryReaderConfiguration | undefined + +/** Configured once per scan. Also forgets the previous scan's skipped files and cached reads. */ +export function configureRepositoryReader(configuration: RepositoryReaderConfiguration): void { + readerConfiguration = configuration skippedFiles = [] repositoryFileCache.clear() } +function configuredReader(): RepositoryReaderConfiguration { + if (!readerConfiguration) throw new BugError('The repository reader was used before it was configured.') + return readerConfiguration +} + export function getSkippedFiles(): SkippedFile[] { return [...skippedFiles] } -function recordSkippedFile(appRoot: string, path: string, failure: RepositoryReadFailure): void { - const repositoryPath = relativePath(appRoot, path).replace(/\\/g, '/') +function recordSkippedFile(path: string, failure: RepositoryReadFailure): void { + const repositoryPath = relativePath(configuredReader().appDirectory, path).replace(/\\/g, '/') skippedFiles.push({ path: repositoryPath.length > 0 ? repositoryPath : path, reason: failure.reason, @@ -365,8 +481,8 @@ function repositoryPathFailure(detail: string): RepositoryReadFailure { type InspectedPath = {status: 'missing'} | {status: 'file'; path: string} | {status: 'unresolved'; reason: string} -function repositoryDisplayPath(appRoot: string, path: string): string { - const relative = normalizeCliPath(relativePath(appRoot, path)) +function repositoryDisplayPath(path: string): string { + const relative = normalizeCliPath(relativePath(configuredReader().appDirectory, path)) if ( relative.length === 0 || relative === '.' || @@ -385,12 +501,12 @@ function repositoryDisplayPath(appRoot: string, path: string): string { * prefix walking runs only after ENOENT/ENOTDIR so optional allowlist paths * can distinguish ordinary absence from a dangling or escaping intermediate. */ -function inspectRepositoryPath(appRoot: string, path: string): InspectedPath { - const absoluteRoot = resolvePath(appRoot) +function inspectRepositoryPath(scanDirectory: string, path: string): InspectedPath { + const absoluteRoot = resolvePath(scanDirectory) const absolutePath = resolvePath(absoluteRoot, path) - const display = repositoryDisplayPath(absoluteRoot, absolutePath) + const display = repositoryDisplayPath(absolutePath) if (!isSubpath(absoluteRoot, absolutePath)) { - return {status: 'unresolved', reason: `${display} escapes the app root`} + return {status: 'unresolved', reason: `${display} escapes the scan directory`} } let canonicalRoot: string @@ -398,19 +514,19 @@ function inspectRepositoryPath(appRoot: string, path: string): InspectedPath { canonicalRoot = realpathSync(absoluteRoot) // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { - return {status: 'unresolved', reason: inspectErrorReason('app root', error)} + return {status: 'unresolved', reason: inspectErrorReason('scan directory', error)} } const segments = normalizeCliPath(relativePath(absoluteRoot, absolutePath)) .split('/') .filter((segment) => segment.length > 0 && segment !== '.') - if (segments.includes('..')) return {status: 'unresolved', reason: `${display} escapes the app root`} + if (segments.includes('..')) return {status: 'unresolved', reason: `${display} escapes the scan directory`} if (segments.length === 0) return {status: 'unresolved', reason: `${display} is not a file`} try { const canonicalPath = realpathSync(absolutePath) if (!isSubpath(canonicalRoot, canonicalPath)) { - return {status: 'unresolved', reason: `${display} resolves outside the app root`} + return {status: 'unresolved', reason: `${display} resolves outside the scan directory`} } if (!lstatSync(canonicalPath).isFile()) { return {status: 'unresolved', reason: `${display} is not a file`} @@ -433,7 +549,7 @@ function inspectMissingRepositoryPath(canonicalRoot: string, segments: string[], lstatSync(currentPath) const canonicalPath = realpathSync(currentPath) if (!isSubpath(canonicalRoot, canonicalPath)) { - return {status: 'unresolved', reason: `${display} resolves outside the app root`} + return {status: 'unresolved', reason: `${display} resolves outside the scan directory`} } const stats = lstatSync(canonicalPath) @@ -468,29 +584,48 @@ function inspectMissingRepositoryPath(canonicalRoot: string, segments: string[], return {status: 'file', path: currentPath} } -function containedRepositoryPath(appRoot: string, path: string): {path?: string; failure?: RepositoryReadFailure} { - const inspected = inspectRepositoryPath(appRoot, path) +function containedRepositoryPath(path: string): {path?: string; failure?: RepositoryReadFailure} { + const scanDirectory = configuredReader().scanDirectories.find((directory) => isSubpath(directory, path)) + if (scanDirectory === undefined) { + return {failure: repositoryPathFailure(`${repositoryDisplayPath(path)} is outside every scan directory`)} + } + const inspected = inspectRepositoryPath(scanDirectory, path) if (inspected.status === 'file') return {path: inspected.path} if (inspected.status === 'missing') return {failure: repositoryPathFailure('path does not exist')} return {failure: repositoryPathFailure(inspected.reason)} } -function readRepositoryFile(appRoot: string, path: string): RepositoryReadResult { - const absoluteRoot = resolvePath(appRoot) - const absolutePath = resolvePath(path) - const cacheKey = `${absoluteRoot}\0${absolutePath}` - const cached = repositoryFileCache.get(cacheKey) +/** An explicit input is followed wherever it points, because the user chose it rather than discovery. */ +function explicitInputPath(path: string): {path?: string; failure?: RepositoryReadFailure} { + const display = repositoryDisplayPath(path) + try { + const canonicalPath = realpathSync(path) + if (!lstatSync(canonicalPath).isFile()) return {failure: repositoryPathFailure(`${display} is not a file`)} + return {path: canonicalPath} + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + return { + failure: repositoryPathFailure( + isMissingFilesystemEntry(error) ? 'path does not exist' : inspectErrorReason(display, error), + ), + } + } +} + +function readRepositoryFile(absolutePath: string): RepositoryReadResult { + const path = resolvePath(absolutePath) + const cached = repositoryFileCache.get(path) if (cached) return cached - const containedPath = containedRepositoryPath(absoluteRoot, absolutePath) - const result = containedPath.path ? readBoundedFile(containedPath.path) : containedPath.failure! - repositoryFileCache.set(cacheKey, result) - if (!result.ok) recordSkippedFile(appRoot, path, result) + const located = configuredReader().explicitInputs.has(path) ? explicitInputPath(path) : containedRepositoryPath(path) + const result = located.path ? readBoundedFile(located.path) : located.failure! + repositoryFileCache.set(path, result) + if (!result.ok) recordSkippedFile(path, result) return result } -function readRepositoryText(appRoot: string, path: string): string | undefined { - const result = readRepositoryFile(appRoot, path) +function readRepositoryText(absolutePath: string): string | undefined { + const result = readRepositoryFile(absolutePath) return result.ok ? result.content.toString() : undefined } @@ -554,7 +689,7 @@ export function findSourceCandidates(repositoryFiles: ReadonlyArray): So export function findAppSourceFiles(appRoot: string, repositoryFiles: ReadonlyArray): SourceFile[] { return repositoryFiles.filter(hasSupportedSourceExtension).map((path) => { const absolutePath = joinPath(appRoot, path) - const result = readRepositoryFile(appRoot, absolutePath) + const result = readRepositoryFile(absolutePath) return { path, absolutePath, @@ -634,7 +769,7 @@ export function findSensitiveFiles( return paths.flatMap((path): SourceFile[] => { const absolutePath = joinPath(appRoot, path) - const result = readRepositoryFile(appRoot, absolutePath) + const result = readRepositoryFile(absolutePath) if (!result.ok) return [{path, absolutePath, ext: extname(path), content: undefined}] if (isProbablyBinary(result.content)) return [] return [{path, absolutePath, ext: extname(path), content: result.content.toString()}] @@ -648,16 +783,15 @@ function nestedRepositoryReason(appRoot: string): string | undefined { return marker.directory === appRoot ? undefined : 'App root is nested below a parent Git repository' } -function recordRejectedAllowlistPath(appRoot: string, relative: string, failure: RepositoryReadFailure): void { - recordSkippedFile(appRoot, resolvePath(appRoot, relative), failure) -} - /** * Read local bot configuration only; hosted integrations and CI workflows are - * outside this check's scope. Paths are read from disk rather than the walked - * list, so a symlinked `.github` is reported as unresolved rather than missing. + * outside this check's scope. Only gathered paths are read, through the reader, + * so a gathered symbolic link that leaves its scan directory is reported as unresolved. */ -export function findDependencyAutomationInputs(appRoot: string, rules: PathRules): DependencyAutomationInputs { +export function findDependencyAutomationInputs( + appRoot: string, + gatheredPaths: ReadonlyArray, +): DependencyAutomationInputs { let canonicalRoot: string try { canonicalRoot = realpathSync(resolvePath(appRoot)) @@ -672,29 +806,18 @@ export function findDependencyAutomationInputs(appRoot: string, rules: PathRules const repositoryReason = nestedRepositoryReason(canonicalRoot) if (repositoryReason) return {files: [], unresolvedReason: repositoryReason} - const isExcluded = createFilePathMatcher(rules) + const gathered = new Set(gatheredPaths) const files: SourceFile[] = [] let unresolvedReason: string | undefined for (const relative of DEPENDENCY_AUTOMATION_CONFIG_PATHS) { - if (isExcluded(relative)) continue - const inspected = inspectRepositoryPath(canonicalRoot, relative) - if (inspected.status === 'missing') continue - if (inspected.status === 'unresolved') { - recordRejectedAllowlistPath(canonicalRoot, relative, { - ok: false, - reason: 'unreadable', - detail: inspected.reason, - }) - unresolvedReason ??= inspected.reason - continue - } - - const absolutePath = resolvePath(canonicalRoot, relative) - const result = readBoundedFile(inspected.path) + if (!gathered.has(relative)) continue + const absolutePath = joinPath(canonicalRoot, relative) + const result = readRepositoryFile(absolutePath) if (!result.ok) { - recordRejectedAllowlistPath(canonicalRoot, relative, result) unresolvedReason ??= - result.reason === 'too_large' ? `${relative} is too large to inspect` : `Could not read ${relative}` + result.reason === 'too_large' + ? `${relative} is too large to inspect` + : (result.detail ?? `Could not read ${relative}`) continue } @@ -727,7 +850,7 @@ export function findManifests(appRoot: string, discoveredPaths: ReadonlyArray { - resetSkippedFiles() +): Promise { + // The selected app configuration is an explicit input: it's read even when it is a symbolic link + // that leaves the app directory. + configureRepositoryReader({ + appDirectory: appRoot, + scanDirectories, + explicitInputs: new Set(appConfigFilePath ? [appConfigFilePath] : []), + }) const selectedFileName = appConfigFilePath ? basename(appConfigFilePath) : undefined const appToml = appConfigFilePath ? loadAppToml(appConfigFilePath, appRoot) : null const appTomls = appToml ? [appToml] : [] - const overrides = ignorePatternRules(options.ignorePatterns ?? []) - const gitIgnoreListing = await listGitIgnoredPaths(appRoot, { - pruneDefaultDirectories: !hasIncludeOverride(overrides), - }) - const pathRules = buildPathRules({ - gitIgnoredPaths: gitIgnoreListing.status === 'listed' ? gitIgnoreListing.paths : [], - overrides, + const { + paths: repositoryFiles, + ignoredScanDirectories, + listingStatus, + } = await gatherPaths({ + appDirectory: appRoot, + scanDirectories, + selectedAppConfigFilePath: appConfigFilePath, + rules: createPathRules({excludePatterns: options.excludePatterns ?? [], noGitIgnore: options.noGitIgnore ?? false}), }) - const repositoryFiles = listRepositoryFiles(appRoot, pathRules) const extensions = findExtensions(appRoot, repositoryFiles) const sourceCandidates = findSourceCandidates(repositoryFiles) const sourceFiles = findAppSourceFiles(appRoot, repositoryFiles) - // The selected app configuration is an explicit input, not a discovered path: it's loaded and - // scanned for secrets even when path rules exclude it. - const sensitivePaths = - appToml && selectedFileName ? [...new Set([...repositoryFiles, selectedFileName])].sort() : repositoryFiles - const sensitiveFiles = findSensitiveFiles(appRoot, sensitivePaths, selectedFileName) + const sensitiveFiles = findSensitiveFiles(appRoot, repositoryFiles, selectedFileName) const manifestPaths = findManifestPaths(repositoryFiles) const manifests = findManifests(appRoot, manifestPaths) const dependencyAutomation = manifests.some(manifestHasDependencies) - ? findDependencyAutomationInputs(appRoot, pathRules) + ? findDependencyAutomationInputs(appRoot, repositoryFiles) : {files: []} const capabilities = detectCapabilities(appToml, extensions, sourceFiles, appTomls) const detection = detectProject(manifests, extensions, sourceCandidates) @@ -591,7 +594,7 @@ export async function scan( capabilities, detection, sourceCandidates, - gitIgnoreListing: gitIgnoreListing.status, + gitIgnoreListing: listingStatus, } let issues: Issue[] = [] @@ -711,5 +714,6 @@ export async function scan( detection, scan: scanMetadata, issues, + ignoredScanDirectories, } } 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 e25b0dc52f8..54f25edb3d8 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 @@ -1,115 +1,109 @@ import {findRepositoryMarker} from './repository-marker.js' -import {BugError} from '@shopify/cli-kit/node/error' +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 ignore from 'ignore' +import {basename, cwd, dirname, joinPath, relativePath} from '@shopify/cli-kit/node/path' +import {lstatSync, realpathSync} from 'node:fs' -/** A user `--ignore` pattern. Include rules store the pattern without the leading `!`. */ -export interface PathOverride { - action: 'exclude' | 'include' - pattern: string - source: 'cli' +export interface PathRules { + /** `--exclude` globs, as typed. */ + excludePatterns: ReadonlyArray + /** Off with `--no-git-ignore`: Git isn't run for gathering and `.git` isn't skipped. */ + gitFiltering: boolean + /** The real path of the working directory, which `--exclude` globs are relative to. */ + workingDirectory: string } -/** Overrides win over the defaults and git paths; among overrides the last match wins, as in .gitignore. */ -export interface PathRules { - defaults: ReadonlyArray - /** App-root-relative paths git reports as untracked and ignored; directories end with `/`. */ - gitIgnoredPaths: ReadonlyArray - overrides: ReadonlyArray +export function createPathRules(options: {excludePatterns: ReadonlyArray; noGitIgnore: boolean}): PathRules { + return { + excludePatterns: options.excludePatterns, + gitFiltering: !options.noGitIgnore, + workingDirectory: realpathSync(cwd()), + } } -/** - * Only correct for paths whose ancestor directories are not excluded: a git - * directory entry (`tmp/`) never matches `tmp/a.ts`, so callers must prune. - */ -type PathMatcher = (relativePath: string, options: {directory: boolean}) => boolean +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. */ +export type GatheredListingStatus = GitIgnoreListing['status'] | 'git-ignore-off' | 'tracked-only' -type FilePathMatcher = (relativePath: string) => boolean +/** The untracked, ignored paths of one repository, relative to `directory` where its listing ran. */ +export interface RepositoryIgnoredPaths { + directory: string + /** Directories end with `/`. */ + paths: ReadonlySet +} + +export function repositoryIgnoredPaths( + directory: string, + listing: GitIgnoreListing, +): RepositoryIgnoredPaths | undefined { + return listing.status === 'listed' ? {directory, paths: new Set(listing.paths)} : undefined +} /** - * `.git` has no trailing slash so it also matches the `.git` file used by - * worktrees. `.github/`, `.vscode/` and similar are deliberately not excluded: - * secrets turn up in workflow and editor configuration. + * Whether the walker drops an entry (and, for a directory, everything inside it). + * `repository` is the listing of the repository that owns the entry; without one, rule 2 can't apply. */ -export const DEFAULT_EXCLUDE_PATTERNS: ReadonlyArray = [ - 'node_modules/', - 'vendor/', - '.git', - '.next/', - 'coverage/', - 'dist/', - 'build/', - '.shopify/', - 'test/', - 'tests/', - 'spec/', - 'specs/', - '__tests__/', - 'fixtures/', - '*-fixtures/', - '__fixtures__/', - '*.test.*', - '*.spec.*', - '.yarn/', - '.react-router/', - '.cache/', - '.turbo/', - '.vercel/', - '.netlify/', - '.output/', - '.nuxt/', - '.svelte-kit/', -] +export function isDroppedEntry( + rules: PathRules, + repository: RepositoryIgnoredPaths | undefined, + entry: {absolutePath: string; isDirectory: boolean}, +): boolean { + if (rules.gitFiltering) { + if (basename(entry.absolutePath) === '.git') return true + if (repository && isListedAsIgnored(repository, entry)) return true + } + return isExcluded(rules, entry.absolutePath) +} + +/** For tracked paths, which are never dropped by the repository's ignore rules: only rules 1 and 3 apply. */ +export function isDroppedTrackedPath(rules: PathRules, scanDirectory: string, trackedPath: string): boolean { + const segments = trackedPath.split('/') + return segments.some((segment, index) => { + const absolutePath = joinPath(scanDirectory, ...segments.slice(0, index + 1)) + return (rules.gitFiltering && segment === '.git') || isExcluded(rules, absolutePath) + }) +} + +function isListedAsIgnored( + repository: RepositoryIgnoredPaths, + entry: {absolutePath: string; isDirectory: boolean}, +): boolean { + const relative = relativePath(repository.directory, entry.absolutePath) + return repository.paths.has(entry.isDirectory ? `${relative}/` : relative) +} -export type GitIgnoreListing = - | {status: 'listed'; paths: string[]} - | {status: 'not-a-repository'} - | {status: 'app-root-ignored'} - | {status: 'failed'} +function isExcluded(rules: PathRules, absolutePath: string): boolean { + if (rules.excludePatterns.length === 0) return false + const relative = relativePath(rules.workingDirectory, absolutePath) + return rules.excludePatterns.some((pattern) => matchGlob(relative, pattern)) +} /** * Only untracked paths are listed, so tracked files that match .gitignore are - * still scanned. Any status other than `listed` means no git exclusions apply. - * Pass `pruneDefaultDirectories: false` whenever an override could re-include - * a default directory. + * still scanned. Any status other than `listed` means no listing-based exclusions apply. */ -export async function listGitIgnoredPaths( - appRoot: string, - options: {pruneDefaultDirectories: boolean}, -): Promise { - const listing = await runGitIgnoreListing(appRoot, options) +export async function listGitIgnoredPaths(directory: string): Promise { + const listing = await runGitIgnoreListing(directory) if (listing.status !== 'listed') outputDebug(`App Security: git ignore listing skipped (${listing.status})`) return listing } -async function runGitIgnoreListing( - appRoot: string, - options: {pruneDefaultDirectories: boolean}, -): Promise { - // Prints `true` or `false`, then the app root's path below the top level (empty at the top level). - const location = await runGit(appRoot, ['rev-parse', '--is-inside-work-tree', '--show-prefix']) +async function runGitIgnoreListing(directory: string): Promise { + const location = await runGit(directory, ['rev-parse', '--is-inside-work-tree']) if (location === undefined) return {status: 'failed'} // Git exits 128 both outside a repository and when it refuses one; the `.git` marker tells them // apart without parsing git's localized stderr. if (location.exitCode !== 0) { - return findRepositoryMarker(appRoot).status === 'none' ? {status: 'not-a-repository'} : {status: 'failed'} + return findRepositoryMarker(directory).status === 'none' ? {status: 'not-a-repository'} : {status: 'failed'} } - const [insideWorkTree, prefix] = location.stdout.split(/\r?\n/) - if (insideWorkTree === 'false') return {status: 'not-a-repository'} + if (location.stdout.trim() === 'false') return {status: 'not-a-repository'} // A missing git binary resolves with exit code 0 and empty output. - if (insideWorkTree !== 'true' || prefix === undefined) return {status: 'failed'} - - // `--no-index` so a force-tracked file doesn't hide that the app folder is ignored. Skipped at - // the top level, where `.` becomes the empty path and a whitelist-style `*` would match it. - if (prefix !== '') { - const appRootIgnored = await runGit(appRoot, ['check-ignore', '--no-index', '-q', '.']) - if (appRootIgnored === undefined) return {status: 'failed'} - if (appRootIgnored.exitCode === 0) return {status: 'app-root-ignored'} - if (appRootIgnored.exitCode !== 1) return {status: 'failed'} - } + if (location.stdout.trim() !== 'true') return {status: 'failed'} - const listed = await runGit(appRoot, [ + const listed = await runGit(directory, [ 'ls-files', '-z', '--others', @@ -118,152 +112,54 @@ async function runGitIgnoreListing( '--directory', '--', '.', - ...(options.pruneDefaultDirectories ? defaultDirectoryPathspecExcludes() : []), ]) if (listed === undefined || listed.exitCode !== 0) return {status: 'failed'} - return {status: 'listed', paths: listed.stdout.split('\0').filter((path) => path !== '')} + return {status: 'listed', paths: splitNullSeparated(listed.stdout)} } /** - * The walker prunes default directories unless an override re-includes one, - * so git needn't walk them. Any include override disables this: telling whether - * a pattern can match a default directory would re-implement gitignore. + * Asks the repository that contains the directory's parent, so a nested repository's top level is + * judged by the outer repository. `--no-index` so a force-tracked file doesn't hide the ignore rule. */ -function defaultDirectoryPathspecExcludes(): string[] { - return DEFAULT_EXCLUDE_PATTERNS.filter((pattern) => pattern.endsWith('/')).map( - (pattern) => `:(exclude,glob)**/${pattern}**`, - ) +export async function isIgnoredByParentRepository(directory: string): Promise { + const parent = dirname(directory) + if (parent === directory) return false + const result = await runGit(parent, ['check-ignore', '--no-index', '-q', '--', basename(directory)]) + return result?.exitCode === 0 } -async function runGit(cwd: string, args: string[]): Promise<{exitCode: number; stdout: string} | undefined> { - try { - const result = await captureOutputWithExitCode('git', args, {cwd}) - return {exitCode: result.exitCode, stdout: result.stdout} - // eslint-disable-next-line no-catch-all/no-catch-all - } catch { - return undefined - } -} - -/** `ignore` drops a pattern ending in one unescaped backslash and throws on three or more. */ -function endsWithUnescapedBackslash(value: string): boolean { - const trailingBackslashCount = /\\+$/.exec(value)?.[0].length ?? 0 - return trailingBackslashCount % 2 === 1 -} - -/** `ignore` throws a SyntaxError on some malformed patterns, such as `src/[id/x.ts`. */ -function compilesAsPattern(value: string): boolean { - try { - compilePatterns([value]) - return true - } catch (error) { - if (error instanceof SyntaxError) return false - throw error - } +/** The files Git tracks in the directory, relative to it. Undefined when Git can't list them. */ +export async function listTrackedFiles(directory: string): Promise { + const listed = await runGit(directory, ['ls-files', '-z', '--cached', '--', '.']) + if (listed === undefined || listed.exitCode !== 0) return undefined + return splitNullSeparated(listed.stdout) } /** - * Rejects values that would silently do nothing or break the scan: comments - * and blank lines are no-ops, a lone `!` re-includes everything, a multi-line - * value never matches, and walked paths never contain `..`. + * The listing for a directory that is a repository's top level, or undefined when it isn't one. + * A symbolic-linked `.git` is never followed, so that subtree has no listing-based exclusions. */ -export function ignorePatternProblem(value: string): string | undefined { - if (value.trim() === '') return "An --ignore pattern can't be empty." - if (/[\r\n]/.test(value)) { - return 'An --ignore pattern must be a single line. Repeat --ignore to add more than one pattern.' - } - if (value.startsWith('#')) { - return `The --ignore pattern "${value}" starts with "#", which .gitignore treats as a comment. To match a path that starts with "#", escape it as "\\#".` - } - if (value.startsWith('!') && value.slice(1).trim() === '') { - return `The --ignore pattern "${value}" has nothing after "!". Add the pattern to include again, for example "!build/".` - } - if (endsWithUnescapedBackslash(value)) { - return `The --ignore pattern "${value}" ends with a backslash, which .gitignore treats as an incomplete escape. Use "/" as the path separator, or escape the backslash as "\\\\".` - } - const pattern = value.startsWith('!') ? value.slice(1) : value - if (pattern.split('/').includes('..')) { - return `The --ignore pattern "${value}" contains "..". Patterns are relative to the app directory and can't point outside it.` - } - if (!compilesAsPattern(value)) { - return `The --ignore pattern "${value}" can't be read as a .gitignore pattern. Check for an unclosed "[" or a backslash before a special character.` - } - return undefined -} - -/** Values are validated at the flag boundary, so a problem here is a bug. */ -export function ignorePatternRules(ignorePatterns: ReadonlyArray): PathOverride[] { - return ignorePatterns.map((value) => { - const problem = ignorePatternProblem(value) - if (problem) throw new BugError(problem) - return value.startsWith('!') - ? {action: 'include', pattern: value.slice(1), source: 'cli'} - : {action: 'exclude', pattern: value, source: 'cli'} - }) -} - -export function hasIncludeOverride(overrides: ReadonlyArray): boolean { - return overrides.some((override) => override.action === 'include') -} - -export function buildPathRules(input: { - gitIgnoredPaths: ReadonlyArray - overrides?: ReadonlyArray -}): PathRules { - return { - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: input.gitIgnoredPaths, - overrides: input.overrides ?? [], +export async function listNestedRepository(directory: string): Promise { + try { + const stats = lstatSync(joinPath(directory, '.git')) + if (stats.isDirectory() || stats.isFile()) return await listGitIgnoredPaths(directory) + return {status: 'failed'} + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + return isMissingFilesystemEntry(error) ? undefined : {status: 'failed'} } } -function toGitIgnoreLine(override: PathOverride): string { - return override.action === 'include' ? `!${override.pattern}` : override.pattern +function splitNullSeparated(output: string): string[] { + return output.split('\0').filter((path) => path !== '') } -/** - * `allowRelativePaths` stops `ignore` throwing on names made only of dots. - * `ignore` is CommonJS, so under NodeNext the factory is on `.default`. - */ -function compilePatterns(lines: ReadonlyArray) { - return ignore.default({ignorecase: false, allowRelativePaths: true}).add([...lines]) -} - -/** - * An override matching the path itself decides (the last one wins); otherwise - * a git path excludes; otherwise the defaults and overrides together decide. - * Git paths are a Set so names like `[id].ts` match literally. Git collapses a - * fully ignored folder to `logs/`, so `!logs/debug.log` can't include a file - * inside it; users must include `!logs/` instead. - */ -export function createPathMatcher(rules: PathRules): PathMatcher { - const overrideLines = rules.overrides.map(toGitIgnoreLine) - const combined = compilePatterns([...rules.defaults, ...overrideLines]) - const overridesOnly = compilePatterns(overrideLines) - const gitIgnored = new Set(rules.gitIgnoredPaths) - - return (relativePath, {directory}) => { - const key = directory ? `${relativePath}/` : relativePath - const overrideMatch = overridesOnly.test(key) - if (overrideMatch.ignored) return true - if (overrideMatch.unignored) return false - if (gitIgnored.has(key)) return true - return combined.ignores(key) - } -} - -/** - * For a file path not reached by walking: checks each ancestor directory - * first, as the walker would have pruned it. Ancestors are tested as - * directories, so a symlinked folder git lists as a file isn't excluded here. - */ -export function createFilePathMatcher(rules: PathRules): FilePathMatcher { - const matcher = createPathMatcher(rules) - return (relativePath) => { - const segments = relativePath.split('/') - for (let depth = 1; depth < segments.length; depth++) { - if (matcher(segments.slice(0, depth).join('/'), {directory: true})) return true - } - return matcher(relativePath, {directory: false}) +async function runGit(directory: string, args: string[]): Promise<{exitCode: number; stdout: string} | undefined> { + try { + const result = await captureOutputWithExitCode('git', args, {cwd: directory}) + return {exitCode: result.exitCode, stdout: result.stdout} + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return undefined } } 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 44a86775bc7..16ec6900859 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 @@ -1,21 +1,37 @@ /* eslint-disable no-restricted-imports -- discovery boundaries use real temporary repositories */ import { + configureRepositoryReader, findDependencyAutomationInputs, findManifests, + gatherPaths, getSkippedFiles, - resetSkippedFiles, } from '../scanners/discover.js' import {DEPENDENCY_AUTOMATION_CONFIG_PATHS} from '../rules/dependency-automation-rules.js' -import {buildPathRules} from '../scanners/path-rules.js' +import {createPathRules} from '../scanners/path-rules.js' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' -import {afterEach, describe, expect, test} from 'vitest' +import {afterEach, describe, expect, test, vi} from 'vitest' import {execFileSync} from 'node:child_process' import {mkdir, symlink, writeFile} from 'node:fs/promises' import {dirname, join} from 'node:path' -afterEach(() => resetSkippedFiles()) +afterEach(() => { + vi.unstubAllEnvs() +}) + +function configureReader(root: string): void { + configureRepositoryReader({appDirectory: root, scanDirectories: [root], explicitInputs: new Set()}) +} -const NO_GIT_EXCLUSIONS = buildPathRules({gitIgnoredPaths: []}) +/** Gathers with Git filtering off, so the temporary directory's surroundings can't change the result. */ +async function findInputs(root: string, excludePatterns: string[] = []) { + configureReader(root) + const {paths} = await gatherPaths({ + appDirectory: root, + scanDirectories: [root], + rules: createPathRules({excludePatterns, noGitIgnore: true}), + }) + return findDependencyAutomationInputs(root, paths) +} async function writeFiles(root: string, files: Record): Promise { await Promise.all( @@ -29,7 +45,7 @@ async function writeFiles(root: string, files: Record): Promise< describe('dependency automation discovery', () => { test('reads only allowlisted config, preserving exact bytes and ignoring workflows', async () => { await inTemporaryDirectory(async (root) => { - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS)).toEqual({files: []}) + await expect(findInputs(root)).resolves.toEqual({files: []}) const content = 'version: 2\r\nupdates: []\r\n' await writeFiles(root, { '.github/dependabot.yml': content, @@ -39,7 +55,7 @@ describe('dependency automation discovery', () => { '.circleci/config.yml': 'jobs: [', '.snyk': 'version: v1.25.0', }) - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) + const result = await findInputs(root) expect(result.unresolvedReason).toBeUndefined() expect(result.files.map(({path}) => path)).toEqual(['.github/dependabot.yml']) expect(result.files[0]?.content).toBe(content) @@ -49,7 +65,7 @@ describe('dependency automation discovery', () => { test.each(DEPENDENCY_AUTOMATION_CONFIG_PATHS)('discovers the allowlisted file %s', async (path) => { await inTemporaryDirectory(async (root) => { await writeFiles(root, {[path]: '{}'}) - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) + const result = await findInputs(root) expect(result.files).toMatchObject([{path, content: '{}'}]) expect(result.unresolvedReason).toBeUndefined() }) @@ -58,63 +74,56 @@ describe('dependency automation discovery', () => { test('stops discovery after finding one configuration file', async () => { await inTemporaryDirectory(async (root) => { await writeFiles(root, {'renovate.json': '{}', '.renovaterc': 'x'.repeat(500_001)}) - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).files).toMatchObject([{path: 'renovate.json'}]) + expect((await findInputs(root)).files).toMatchObject([{path: 'renovate.json'}]) expect(getSkippedFiles()).toEqual([]) }) }) + test('reads only the paths it is given', async () => { + await inTemporaryDirectory(async (root) => { + await writeFiles(root, {'renovate.json': '{}', '.github/dependabot.yml': 'version: 2\nupdates: []\n'}) + configureReader(root) + expect(findDependencyAutomationInputs(root, ['.github/dependabot.yml']).files).toMatchObject([ + {path: '.github/dependabot.yml'}, + ]) + expect(findDependencyAutomationInputs(root, ['src/renovate.json'])).toEqual({files: []}) + }) + }) + describe('path rules', () => { - test('treats a configuration file the rules exclude like a missing one', async () => { + test('treats a configuration file an exclusion matches like a missing one', async () => { await inTemporaryDirectory(async (root) => { + vi.stubEnv('INIT_CWD', root) await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) - const rules = buildPathRules({gitIgnoredPaths: ['.github/dependabot.yml']}) - expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) + await expect(findInputs(root, ['.github/dependabot.yml'])).resolves.toEqual({files: []}) expect(getSkippedFiles()).toEqual([]) }) }) - test('excludes a configuration file inside a directory the rules exclude', async () => { - // Git lists `.github/`, never the file, so the ancestors must be checked. + test('excludes a configuration file inside a directory an exclusion matches', async () => { await inTemporaryDirectory(async (root) => { + vi.stubEnv('INIT_CWD', root) await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) - const rules = buildPathRules({gitIgnoredPaths: ['.github/']}) - expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) + await expect(findInputs(root, ['.github'])).resolves.toEqual({files: []}) }) }) test('continues to a later allowlisted file when an earlier one is excluded', async () => { await inTemporaryDirectory(async (root) => { + vi.stubEnv('INIT_CWD', root) await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n', 'renovate.json': '{}'}) - const rules = buildPathRules({gitIgnoredPaths: ['.github/dependabot.yml']}) - const result = findDependencyAutomationInputs(root, rules) + const result = await findInputs(root, ['.github/dependabot.yml']) expect(result.files).toMatchObject([{path: 'renovate.json', content: '{}'}]) expect(result.unresolvedReason).toBeUndefined() }) }) - test('applies the default patterns as well as the git literals', async () => { - // No shipped default matches an allowlisted path, hence a custom one. - await inTemporaryDirectory(async (root) => { - await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) - const rules = {defaults: ['.github/'], gitIgnoredPaths: [], overrides: []} - expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).files).toHaveLength(1) - }) - }) - - test('does not exclude a file when the rules only name a sibling or a lookalike', async () => { + test('does not exclude a file when the exclusions only name a sibling or a lookalike', async () => { await inTemporaryDirectory(async (root) => { + vi.stubEnv('INIT_CWD', root) await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) - const rules = buildPathRules({gitIgnoredPaths: ['.github/dependabot.yaml', '.github/workflows/', '.githu/']}) - expect(findDependencyAutomationInputs(root, rules).files).toMatchObject([{path: '.github/dependabot.yml'}]) - }) - }) - - test('leaves an unsafe allowlisted path unresolved when the rules do not exclude it', async () => { - await inTemporaryDirectory(async (root) => { - await symlink(join(root, 'missing'), join(root, '.github'), 'dir') - const rules = buildPathRules({gitIgnoredPaths: ['renovate.json']}) - expect(findDependencyAutomationInputs(root, rules).unresolvedReason).toContain('dangling symbolic link') + const result = await findInputs(root, ['.github/dependabot.yaml', '.github/workflows', '.githu']) + expect(result.files).toMatchObject([{path: '.github/dependabot.yml'}]) }) }) }) @@ -122,7 +131,7 @@ describe('dependency automation discovery', () => { test('preserves bounded reads and skipped-file coverage', async () => { await inTemporaryDirectory(async (root) => { await writeFiles(root, {'.github/dependabot.yml': 'x'.repeat(500_001)}) - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS)).toMatchObject({ + await expect(findInputs(root)).resolves.toMatchObject({ files: [], unresolvedReason: expect.stringContaining('too large'), }) @@ -138,7 +147,7 @@ describe('dependency automation discovery', () => { await writeFiles(app, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) if (marker === 'directory') await mkdir(join(repository, '.git')) else await writeFile(join(repository, '.git'), 'gitdir: /outside/not-read') - const nested = findDependencyAutomationInputs(app, NO_GIT_EXCLUSIONS) + const nested = await findInputs(app) expect(nested).toMatchObject({ files: [], unresolvedReason: 'App root is nested below a parent Git repository', @@ -146,45 +155,51 @@ describe('dependency automation discovery', () => { expect(nested.unresolvedReason).not.toContain(repository) if (marker === 'directory') await mkdir(join(app, '.git')) else await writeFile(join(app, '.git'), 'gitdir: /outside/not-read') - expect(findDependencyAutomationInputs(app, NO_GIT_EXCLUSIONS).files).toMatchObject([ - {path: '.github/dependabot.yml'}, - ]) + expect((await findInputs(app)).files).toMatchObject([{path: '.github/dependabot.yml'}]) }) }) - test.each(['.github', '.gitlab', '.git'])('rejects escaping or ambiguous %s directory links', async (path) => { + test.each(['.github', '.gitlab'])('does not follow a symbolic-linked %s directory', async (path) => { await inTemporaryDirectory(async (root) => { await inTemporaryDirectory(async (outside) => { + await writeFiles(outside, {'dependabot.yml': 'version: 2', 'renovate.json': '{}'}) await symlink(outside, join(root, path), 'dir') - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) - expect(result).toMatchObject({ - files: [], - unresolvedReason: expect.any(String), - }) + await expect(findInputs(root)).resolves.toEqual({files: []}) + expect(getSkippedFiles()).toEqual([]) + }) + }) + }) + + test('rejects an ambiguous .git link', async () => { + await inTemporaryDirectory(async (root) => { + await inTemporaryDirectory(async (outside) => { + await symlink(outside, join(root, '.git'), 'dir') + const result = await findInputs(root) + expect(result).toMatchObject({files: [], unresolvedReason: expect.any(String)}) expect(result.unresolvedReason).not.toContain(root) expect(result.unresolvedReason).not.toContain(outside) }) }) }) - test('rejects dangling directory links', async () => { + test('rejects dangling config-file links', async () => { await inTemporaryDirectory(async (root) => { - await symlink(join(root, 'missing'), join(root, '.github'), 'dir') - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) + await symlink(join(root, 'missing'), join(root, 'renovate.json')) + const result = await findInputs(root) expect(result.unresolvedReason).toContain('dangling symbolic link') expect(result.unresolvedReason).not.toContain(root) - expect(getSkippedFiles()).toContainEqual( - expect.objectContaining({path: '.github/dependabot.yml', reason: 'unreadable'}), - ) + expect(getSkippedFiles()).toContainEqual(expect.objectContaining({path: 'renovate.json', reason: 'unreadable'})) }) }) test('keeps looking after an unsafe allowlisted path', async () => { await inTemporaryDirectory(async (root) => { await inTemporaryDirectory(async (outside) => { - await symlink(outside, join(root, '.github'), 'dir') + await writeFile(join(outside, 'dependabot.yml'), 'version: 2') + await mkdir(join(root, '.github')) + await symlink(join(outside, 'dependabot.yml'), join(root, '.github/dependabot.yml')) await writeFiles(root, {'renovate.json': '{}'}) - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) + const result = await findInputs(root) expect(result.files).toMatchObject([{path: 'renovate.json', content: '{}'}]) expect(result.unresolvedReason).toBeUndefined() expect(getSkippedFiles()).toContainEqual( @@ -199,14 +214,14 @@ describe('dependency automation discovery', () => { await inTemporaryDirectory(async (outside) => { await writeFile(join(outside, 'config.json'), '{}') await symlink(join(outside, 'config.json'), join(root, 'renovate.json')) - const rejected = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) - expect(rejected.unresolvedReason).toContain('outside the app root') + const rejected = await findInputs(root) + expect(rejected.unresolvedReason).toContain('outside the scan directory') expect(rejected.unresolvedReason).not.toContain(root) expect(rejected.unresolvedReason).not.toContain(outside) expect(getSkippedFiles()).toContainEqual(expect.objectContaining({path: 'renovate.json', reason: 'unreadable'})) await writeFiles(root, {'.github/config.yml': 'version: 2\nupdates: []'}) await symlink(join(root, '.github/config.yml'), join(root, '.github/dependabot.yml')) - const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) + const result = await findInputs(root) expect(result.files).toMatchObject([{path: '.github/dependabot.yml'}]) expect(result.unresolvedReason).toBeUndefined() }) @@ -216,9 +231,9 @@ describe('dependency automation discovery', () => { test.skipIf(process.platform === 'win32')('rejects special files without attempting to read them', async () => { await inTemporaryDirectory(async (root) => { execFileSync('mkfifo', [join(root, 'renovate.json')]) - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).unresolvedReason).toContain('not a file') + expect((await findInputs(root)).unresolvedReason).toContain('not a file') execFileSync('mkfifo', [join(root, '.git')]) - expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).unresolvedReason).toContain('repository ownership') + expect((await findInputs(root)).unresolvedReason).toContain('repository ownership') }) }) }) @@ -229,6 +244,7 @@ describe('manifest safety', () => { await inTemporaryDirectory(async (outside) => { await writeFile(join(outside, 'package.json'), '{"dependencies":{"react":"19.0.0"}}') await symlink(join(outside, 'package.json'), join(root, 'package.json')) + configureReader(root) expect(findManifests(root, ['package.json', '../package.json'])).toEqual([]) expect(getSkippedFiles()).toHaveLength(2) }) @@ -240,6 +256,7 @@ describe('manifest safety', () => { async (content) => { await inTemporaryDirectory(async (root) => { await writeFile(join(root, 'package.json'), content) + configureReader(root) expect(findManifests(root, ['package.json'])).toMatchObject([{dependencies: {}, devDependencies: {}}]) expect(getSkippedFiles()).toEqual([ expect.objectContaining({reason: 'unreadable', detail: 'manifest could not be parsed'}), @@ -252,6 +269,7 @@ describe('manifest safety', () => { await inTemporaryDirectory(async (root) => { await writeFile(join(root, 'package.json'), '{"dependencies":{"react":"19.0.0"}}') await writeFile(join(root, 'my-package.json'), '{"dependencies":{"left-pad":"1.0.0"}}') + configureReader(root) expect(findManifests(root, ['my-package.json', 'package.json'])).toMatchObject([{path: 'package.json'}]) expect(getSkippedFiles()).toEqual([]) }) @@ -260,6 +278,7 @@ describe('manifest safety', () => { test('does not retain or validate package scripts for this check', async () => { await inTemporaryDirectory(async (root) => { await writeFile(join(root, 'package.json'), '{"scripts":{"audit":["npm audit"]}}') + configureReader(root) expect(findManifests(root, ['package.json'])).toMatchObject([{dependencies: {}, devDependencies: {}}]) expect(findManifests(root, ['package.json'])[0]).not.toHaveProperty('scripts') expect(getSkippedFiles()).toEqual([]) diff --git a/packages/app/src/cli/services/app-security-engine/tests/dependency-automation.test.ts b/packages/app/src/cli/services/app-security-engine/tests/dependency-automation.test.ts index 887e6b6baeb..d8e076d0b28 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/dependency-automation.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/dependency-automation.test.ts @@ -11,7 +11,8 @@ import {fetch} from '@shopify/cli-kit/node/http' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import {mkdir, writeFile} from 'node:fs/promises' -import {dirname, join} from 'node:path' +import {basename, dirname, join} from 'node:path' +import type {ScanResult} from '../types.js' vi.mock('@shopify/cli-kit/node/http', async (importActual) => { const actual: any = await importActual() @@ -56,11 +57,11 @@ uri = "https://example.test/webhooks" }) } -function dependencyExecution(result: Awaited>) { +function dependencyExecution(result: ScanResult) { return result.scan.checks_executed.find((execution) => execution.id === checkId)! } -function dependencyFindings(result: Awaited>) { +function dependencyFindings(result: ScanResult) { return result.issues.filter((issue) => issue.id === checkId) } @@ -141,9 +142,10 @@ describe('dependency automation scanner integration', () => { }) expect(securityExitCode({...execution, elapsedMilliseconds: 0}, 'low')).toBe(0) expect(JSON.stringify(execution.deterministicFindings)).not.toContain('local>org/renovate-config') - // Outside any repository, ignored-path discovery stops at its first probe. + // Outside any repository, gathering stops after asking whether the directory is ignored and probing for a repository. expect(vi.mocked(captureOutputWithExitCode).mock.calls.map(([command, args]) => [command, args])).toEqual([ - ['git', ['rev-parse', '--is-inside-work-tree', '--show-prefix']], + ['git', ['check-ignore', '--no-index', '-q', '--', basename(root)]], + ['git', ['rev-parse', '--is-inside-work-tree']], ]) expect(fetch).not.toHaveBeenCalled() }) @@ -215,17 +217,18 @@ describe('dependency automation scanner integration', () => { }) }) - describe('--ignore', () => { - test('does not read a committed configuration file a pattern excludes', async () => { + describe('--exclude and --no-git-ignore', () => { + test('does not read a committed configuration file an exclusion matches', async () => { await inTemporaryDirectory(async (root) => { + vi.stubEnv('INIT_CWD', root) await makeRepository(root, {'.github/dependabot.yml': dependabot}, ['.github/dependabot.yml']) - const result = await scan(root, undefined, {ignorePatterns: ['.github/']}) + const result = await scan(root, undefined, {excludePatterns: ['.github']}) expect(dependencyFindings(result)).toHaveLength(1) expect(dependencyExecution(result)).toMatchObject({status: 'executed', inspected_files: ['package.json']}) }) }) - test('reads an untracked gitignored configuration file a pattern includes again', async () => { + test('reads an untracked gitignored configuration file with --no-git-ignore', async () => { await inTemporaryDirectory(async (root) => { // A tracked CODEOWNERS stops git collapsing `.github/`, so it lists the file itself. await makeRepository( @@ -237,10 +240,10 @@ describe('dependency automation scanner integration', () => { }, ['.gitignore', '.github/CODEOWNERS'], ) - const excluded = await scan(root) - expect(dependencyFindings(excluded)).toHaveLength(1) + const ignored = await scan(root) + expect(dependencyFindings(ignored)).toHaveLength(1) - const result = await scan(root, undefined, {ignorePatterns: ['!.github/dependabot.yml']}) + const result = await scan(root, undefined, {noGitIgnore: true}) expect(dependencyFindings(result)).toEqual([]) expect(dependencyExecution(result)).toMatchObject({ status: 'executed', 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 39f3c62b4d4..2f1a566054d 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 @@ -2,31 +2,38 @@ import {git, isolateGitConfig} from './git-test-helpers.js' import {scanDirectory as scan} from './scan-directory.js' import { + configureRepositoryReader, findExtensions, findSourceCandidates, + gatherPaths, getSkippedFiles, - listRepositoryFiles, - resetSkippedFiles, } from '../scanners/discover.js' -import {buildPathRules, listGitIgnoredPaths} from '../scanners/path-rules.js' +import {createPathRules} from '../scanners/path-rules.js' import {joinPath} from '@shopify/cli-kit/node/path' -import {afterEach, beforeEach, describe, expect, test} from 'vitest' -import {chmod, mkdir, mkdtemp, rm, symlink, writeFile} from 'node:fs/promises' +import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' +import {chmod, mkdir, mkdtemp, realpath, rm, symlink, writeFile} from 'node:fs/promises' import {tmpdir} from 'node:os' import {join} from 'node:path' -import type {PathRules} from '../scanners/path-rules.js' import type {ScanResult} from '../types.js' const temporaryDirectories: string[] = [] const appConfiguration = 'name = "Discovery safety"\napplication_url = "https://example.com"\n' -const DEFAULT_RULES = buildPathRules({gitIgnoredPaths: []}) +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 and what `--exclude` is matched against. */ async function makeDirectory(prefix = 'app-security-discovery-'): Promise { - const directory = await mkdtemp(join(tmpdir(), prefix)) + const directory = await realpath(await mkdtemp(join(tmpdir(), prefix))) temporaryDirectories.push(directory) return directory } @@ -70,101 +77,96 @@ function inspectedManifestPaths(result: ScanResult): string[] { return execution?.inspected_files.filter((path) => path.endsWith('package.json')) ?? [] } -async function scanPathRules(appRoot: string): Promise { - const listing = await listGitIgnoredPaths(appRoot, {pruneDefaultDirectories: true}) - return buildPathRules({gitIgnoredPaths: listing.status === 'listed' ? listing.paths : []}) +interface GatherOptions { + appDirectory?: string + scanDirectories?: string[] + selectedAppConfigFilePath?: string + excludePatterns?: string[] + noGitIgnore?: boolean } -describe('repository discovery exclusions', () => { - let restoreGitConfig: (() => void) | undefined - beforeEach(() => { - restoreGitConfig = isolateGitConfig() - }) - afterEach(() => { - restoreGitConfig?.() +/** Gathers the way a scan does, and leaves the reader configured for the finders. */ +async function gather(root: string, options: GatherOptions = {}) { + const appDirectory = options.appDirectory ?? root + const scanDirectories = options.scanDirectories ?? [root] + configureRepositoryReader({ + appDirectory, + scanDirectories, + explicitInputs: new Set(options.selectedAppConfigFilePath ? [options.selectedAppConfigFilePath] : []), + }) + return gatherPaths({ + appDirectory, + scanDirectories, + selectedAppConfigFilePath: options.selectedAppConfigFilePath, + rules: createPathRules({excludePatterns: options.excludePatterns ?? [], noGitIgnore: options.noGitIgnore ?? false}), }) +} - test('excludes every nested app input from its parent monorepo scan', async () => { +async function gatheredPaths(root: string, options: GatherOptions = {}): Promise { + return (await gather(root, options)).paths +} + +async function makeSubmodule(): Promise { + const submodule = await makeRepository({'.gitignore': 'generated/\n', 'a.ts': 'export const a = true'}) + git(submodule, ['add', '-A']) + git(submodule, ['commit', '-qm', 'init']) + const outer = await makeRepository({'.gitignore': '*.log\n', 'index.ts': 'export const outer = true'}) + git(outer, ['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', submodule, 'vendor/sub']) + await writeFiles(join(outer, 'vendor', 'sub'), {'generated/b.ts': '', 'x.log': ''}) + return outer +} + +describe('gathering without a default exclusion list', () => { + test('scans a nested app and every other file, since no name is excluded', async () => { const root = await makeDirectory() - const secret = ['AKIA', 'IOSFODNN7EXAMPLE'].join('') await writeFiles(root, { 'shopify.app.toml': appConfiguration, 'parent.ts': 'export const parent = true', 'apps/child/shopify.app.toml': 'name = "Child"\n', - 'apps/child/package.json': JSON.stringify({dependencies: {'@shopify/shopify-app-react-router': '1.0.0'}}), - 'apps/child/app/routes/child.ts': `export const leaked = "${secret}"`, - 'apps/child/extensions/theme/shopify.extension.toml': 'type = "theme"\n', - 'apps/child/extensions/theme/blocks/app.liquid': '{{ block.settings.value }}', - 'apps/child/secrets.json': secret, + 'apps/child/app/routes/child.ts': 'export const child = true', }) const result = await scan(root) - expect(scannedPaths(result)).toContain('parent.ts') - expect(scannedPaths(result).some((path) => path.startsWith('apps/child/'))).toBe(false) - expect(result.capabilities.theme_app_extension).toBe(false) - expect(result.detection.framework).not.toBe('react_router') - expect(JSON.stringify(result)).not.toContain(secret) - expect(result.issues.some((issue) => issue.location.file.startsWith('apps/child/'))).toBe(false) + expect(scannedPaths(result)).toEqual(expect.arrayContaining(['parent.ts', 'apps/child/app/routes/child.ts'])) + await expect(gatheredPaths(root)).resolves.toContain('apps/child/shopify.app.toml') }) - test('recursively excludes dependency, VCS, coverage, build, and test directories', async () => { + test('scans node_modules, build output and test directories outside a repository', async () => { const root = await makeDirectory() - const ignoredDirectories = [ + const directories = [ 'node_modules', 'vendor', - '.git', - '.next', 'coverage', 'dist', 'build', 'test', 'tests', - 'spec', - 'specs', '__tests__', - 'fixtures', - 'x-fixtures', - '__fixtures__', + '.shopify', ] await writeFiles(root, { 'shopify.app.toml': appConfiguration, - 'src/index.ts': 'export const included = true', - 'packages/service/lib/index.test.ts': 'export const ignored = true', - 'packages/service/lib/index.spec.js': 'export const ignored = true', - ...Object.fromEntries( - ignoredDirectories.map((directory) => [ - `packages/service/${directory}/ignored.ts`, - 'export const ignored = true', - ]), - ), + 'src/index.test.ts': 'export const test = true', + ...Object.fromEntries(directories.map((directory) => [`${directory}/x.ts`, 'export const scanned = true'])), }) - const result = await scan(root) - const paths = scannedPaths(result) - expect(paths).toContain('src/index.ts') - for (const directory of ignoredDirectories) - expect(paths.some((path) => path.includes(`/${directory}/`))).toBe(false) - expect(paths).not.toContain('packages/service/lib/index.test.ts') - expect(paths).not.toContain('packages/service/lib/index.spec.js') + await expect(gatheredPaths(root)).resolves.toEqual( + ['shopify.app.toml', 'src/index.test.ts', ...directories.map((directory) => `${directory}/x.ts`)].sort(), + ) }) - test('scans a selected app configuration file for secrets even when a default exclusion matches it', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') - const root = await makeDirectory() - await writeFiles(root, { + test('scans node_modules inside a repository when Git does not ignore it', async () => { + const root = await makeRepository({ 'shopify.app.toml': appConfiguration, - // `*.test.*` is a default exclusion. - 'shopify.app.test.toml': `name = "Test"\napplication_url = "https://test.example.com/?token=${secret}"\n`, + '.gitignore': '*.log\n', + 'node_modules/pkg/index.js': 'export const dependency = true', }) - const result = await scan(root, 'test') - expect(result.app.name).toBe('Test') - expect(secretFindingFiles(result)).toEqual(['shopify.app.test.toml']) + await expect(gatheredPaths(root)).resolves.toContain('node_modules/pkg/index.js') }) test('walks dot-folders and dotfiles', async () => { const root = await makeDirectory() - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') await writeFiles(root, { 'shopify.app.toml': appConfiguration, '.github/workflows/deploy.yml': `env:\n SHOPIFY_TOKEN: ${secret}\n`, @@ -178,56 +180,28 @@ describe('repository discovery exclusions', () => { expect(paths).toContain('.vscode/settings.json') expect(paths).toContain('.eslintrc.cjs') - const candidates = findSourceCandidates(listRepositoryFiles(root, DEFAULT_RULES)) + const candidates = findSourceCandidates(await gatheredPaths(root)) expect(candidates).toEqual([ expect.objectContaining({path: '.eslintrc.cjs', extension: '.cjs', language: 'javascript', supported: true}), ]) }) - test('excludes generated dot-folders and the .git worktree marker file', async () => { - const root = await makeDirectory() - const generatedDotFolders = [ - '.yarn/releases', - '.react-router', - '.cache', - '.turbo', - '.vercel', - '.netlify', - '.output', - '.nuxt', - '.svelte-kit', - '.shopify/dev-bundle', - ] - await writeFiles(root, { + test('keeps the results directory out of the scan because .shopify/.gitignore ignores it', async () => { + const root = await makeRepository({ 'shopify.app.toml': appConfiguration, - '.git': 'gitdir: /somewhere/else/.git/worktrees/app\n', - 'src/index.ts': 'export const included = true', - ...Object.fromEntries( - generatedDotFolders.map((directory) => [`${directory}/x.ts`, 'export const ignored = true']), - ), + 'src/index.ts': 'export const stable = true', + '.shopify/.gitignore': '*\n', }) - - const result = await scan(root) - const paths = scannedPaths(result) - expect(paths).toContain('src/index.ts') - for (const directory of generatedDotFolders) expect(paths).not.toContain(`${directory}/x.ts`) - - expect(listRepositoryFiles(root, DEFAULT_RULES)).toEqual(['shopify.app.toml', 'src/index.ts']) - }) - - test('keeps scanner-owned artifacts and atomic siblings out of stable scan inputs', async () => { - const root = await makeDirectory() - await writeFiles(root, {'shopify.app.toml': appConfiguration, 'src/index.ts': 'export const stable = true'}) const before = await scan(root) await writeFiles(root, { - '.shopify/app-security/deterministic-findings.json': '{"changed":true}', - '.shopify/app-security/agent-checks.json': '{"changed":true}', - '.shopify/app-security/agent-findings.json': '{"changed":true}', + '.shopify/app-security/shopify.app/deterministic-findings.json': '{"changed":true}', + '.shopify/app-security/shopify.app/agent-checks.json': '{"changed":true}', + '.shopify/app-security/shopify.app/agent-findings.json': '{"changed":true}', }) const after = await scan(root) expect(scannedPaths(after)).toEqual(scannedPaths(before)) - expect(scannedPaths(after).some((path) => path.includes('.shopify/app-security'))).toBe(false) + await expect(gatheredPaths(root)).resolves.toEqual(['shopify.app.toml', 'src/index.ts']) }) test('scans unconfigured extension files outside extension_directories', async () => { @@ -265,7 +239,7 @@ describe('repository discovery exclusions', () => { 'extensions/beta/src/index.ts': 'export const noToml = true', }) - const extensions = findExtensions(root, listRepositoryFiles(root, DEFAULT_RULES)) + const extensions = findExtensions(root, await gatheredPaths(root)) expect(extensions.map((extension) => [extension.path, extension.files.map((file) => file.path)])).toEqual([ ['extensions/alpha-two/shopify.extension.toml', ['extensions/alpha-two/src/index.ts']], @@ -300,13 +274,60 @@ describe('repository discovery exclusions', () => { }) }) -describe('gitignore-driven exclusions', () => { - let restoreGitConfig: (() => void) | undefined - beforeEach(() => { - restoreGitConfig = isolateGitConfig() +describe('.git entries', () => { + test('skips a .git directory at any depth with Git filtering on, and walks it with --no-git-ignore', async () => { + const root = await makeRepository({'shopify.app.toml': appConfiguration, 'src/index.ts': ''}) + + await expect(gatheredPaths(root)).resolves.toEqual(['shopify.app.toml', 'src/index.ts']) + const walked = await gatheredPaths(root, {noGitIgnore: true}) + expect(walked).toContain('.git/HEAD') + expect(walked).toContain('shopify.app.toml') + }) + + test("skips a worktree's .git file with Git filtering on, and walks it with --no-git-ignore", async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + '.git': 'gitdir: /somewhere/else/.git/worktrees/app\n', + 'packages/api/.git': 'gitdir: /somewhere/else/.git/worktrees/api\n', + 'src/index.ts': 'export const included = true', + }) + + await expect(gatheredPaths(root)).resolves.toEqual(['shopify.app.toml', 'src/index.ts']) + await expect(gatheredPaths(root, {noGitIgnore: true})).resolves.toEqual([ + '.git', + 'packages/api/.git', + 'shopify.app.toml', + 'src/index.ts', + ]) }) - afterEach(() => { - restoreGitConfig?.() + + test('does not run Git for gathering with --no-git-ignore', async () => { + const root = await makeRepository({'.gitignore': 'dist/\n', 'shopify.app.toml': appConfiguration, 'dist/a.ts': ''}) + + const result = await gather(root, {noGitIgnore: true}) + + expect(result.paths).toContain('dist/a.ts') + expect(result.ignoredScanDirectories).toEqual([]) + expect(result.listingStatus).toBe('git-ignore-off') + }) +}) + +describe('repositories', () => { + test('skips an ignored, untracked dist/ and keeps a tracked file in it', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'dist/\n', + 'dist/tracked.ts': 'export const tracked = true', + 'dist/untracked.ts': 'export const untracked = true', + 'build/untracked.ts': 'export const scanned = true', + }) + git(root, ['add', '-f', 'dist/tracked.ts']) + + const paths = await gatheredPaths(root) + expect(paths).toContain('dist/tracked.ts') + expect(paths).not.toContain('dist/untracked.ts') + expect(paths).toContain('build/untracked.ts') }) test('excludes gitignored directories', async () => { @@ -325,7 +346,6 @@ describe('gitignore-driven exclusions', () => { }) test('neither reports nor scans a secret in a gitignored file, but does for its non-ignored twin', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') const root = await makeRepository({ 'shopify.app.toml': appConfiguration, '.gitignore': 'notes.txt\n', @@ -356,7 +376,6 @@ describe('gitignore-driven exclusions', () => { }) test('honours negation patterns', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') const root = await makeRepository({ 'shopify.app.toml': appConfiguration, '.gitignore': '.env*\n!.env.example\n', @@ -395,7 +414,7 @@ describe('gitignore-driven exclusions', () => { 'extensions/foo/index.js': 'export const included = true', }) - const extensions = findExtensions(root, listRepositoryFiles(root, await scanPathRules(root))) + const extensions = findExtensions(root, await gatheredPaths(root)) expect(extensions.map((extension) => extension.files.map((file) => file.path))).toEqual([ ['extensions/foo/index.js'], ]) @@ -430,7 +449,7 @@ describe('gitignore-driven exclusions', () => { expect(paths).not.toContain('sp ace.ts') }) - test('applies the enclosing repository .gitignore to an app in a subfolder', async () => { + test('applies the enclosing repository .gitignore to an app in a subdirectory', async () => { const repository = await makeRepository({ '.gitignore': 'scratch/\n', 'apps/web/shopify.app.toml': appConfiguration, @@ -456,26 +475,6 @@ describe('gitignore-driven exclusions', () => { expect(paths).not.toContain('private/keys.ts') }) - test('scans the whole app when the enclosing repository ignores the app folder but force-tracks a file in it', async () => { - // A force-tracked file stops git collapsing the app to `./`; it lists each file instead. - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') - const repository = await makeRepository({ - '.gitignore': 'apps/web/\n', - 'apps/web/shopify.app.toml': appConfiguration, - 'apps/web/README.md': 'docs', - 'apps/web/.env': `SHOPIFY_TOKEN=${secret}\n`, - 'apps/web/a.ts': 'export const scanned = true', - }) - git(repository, ['add', '-f', 'apps/web/README.md', '.gitignore']) - git(repository, ['commit', '-qm', 'init']) - - const result = await scan(join(repository, 'apps', 'web')) - const paths = scannedPaths(result) - expect(paths).toContain('.env') - expect(paths).toContain('a.ts') - expect(secretFindingFiles(result)).toEqual(['.env']) - }) - test('omits an ignored manifest from manifest inspection but inspects a force-tracked one', async () => { const manifest = JSON.stringify({dependencies: {react: '19.0.0'}}) const root = await makeRepository({ @@ -495,30 +494,6 @@ describe('gitignore-driven exclusions', () => { expect(paths).not.toContain('tmp/package.json') }) - test.each([ - ['inner', 'inner/'], - ['scratch/deep', 'scratch/'], - ])('excludes nested repository %s when the app repository ignores %s', async (nestedRepository, ignoredPath) => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') - const root = await makeRepository({ - 'shopify.app.toml': appConfiguration, - '.gitignore': `${ignoredPath}\n`, - [`${nestedRepository}/token.ts`]: `export const token = '${secret}'\n`, - 'plain/token.ts': `export const token = '${secret}'\n`, - }) - for (const repository of [nestedRepository, 'plain']) { - const directory = join(root, repository) - git(directory, ['init', '-q', '.']) - git(directory, ['add', 'token.ts']) - git(directory, ['commit', '-qm', 'Add token']) - } - - const result = await scan(root) - // The unignored nested repository proves the secret is detectable, so the absence is not vacuous. - expect(secretFindingFiles(result)).toEqual(['plain/token.ts']) - expect(scannedPaths(result)).not.toContain(`${nestedRepository}/token.ts`) - }) - test('ignores nothing from a .gitignore outside a git repository', async () => { const root = await makeDirectory() await writeFiles(root, { @@ -531,7 +506,6 @@ describe('gitignore-driven exclusions', () => { }) test('loads and scans a gitignored selected app configuration file for secrets', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') const root = await makeRepository({ 'shopify.app.toml': appConfiguration, '.gitignore': 'shopify.app.staging.toml\n', @@ -543,18 +517,208 @@ describe('gitignore-driven exclusions', () => { expect(scannedPaths(result)).toContain('shopify.app.staging.toml') expect(secretFindingFiles(result)).toEqual(['shopify.app.staging.toml']) }) -}) -describe('--ignore patterns', () => { - let restoreGitConfig: () => void - beforeEach(() => { - restoreGitConfig = isolateGitConfig() + describe('nested repositories', () => { + async function makeNestedRepository(files: {outer?: Record; inner: Record}) { + const outer = await makeRepository({'shopify.app.toml': appConfiguration, ...files.outer}) + const inner = join(outer, 'inner') + await writeFiles(inner, files.inner) + git(inner, ['init', '-q', '.']) + return {outer, inner} + } + + test("applies a nested repository's own .gitignore inside it, and not the outer repository's", async () => { + const {outer} = await makeNestedRepository({ + outer: {'.gitignore': 'outer-only.ts\n', 'outer-only.ts': ''}, + inner: {'.gitignore': 'inner-only.ts\n', 'inner-only.ts': '', 'outer-only.ts': '', 'kept.ts': ''}, + }) + + const paths = await gatheredPaths(outer) + expect(paths).not.toContain('outer-only.ts') + expect(paths).not.toContain('inner/inner-only.ts') + expect(paths).toEqual(expect.arrayContaining(['inner/outer-only.ts', 'inner/kept.ts'])) + }) + + test('applies a nested repository rule that the outer listing would otherwise reach, such as a directory', async () => { + const {outer} = await makeNestedRepository({ + inner: {'.gitignore': 'build/\n', 'build/a.ts': '', 'node_modules/b.ts': ''}, + }) + + const paths = await gatheredPaths(outer) + expect(paths).not.toContain('inner/build/a.ts') + expect(paths).toContain('inner/node_modules/b.ts') + }) + + test.each([ + ['inner', 'inner/'], + ['scratch/deep', 'scratch/'], + ])('prunes nested repository %s when the app repository ignores %s', async (nestedRepository, ignoredPath) => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': `${ignoredPath}\n`, + [`${nestedRepository}/token.ts`]: `export const token = '${secret}'\n`, + 'plain/token.ts': `export const token = '${secret}'\n`, + }) + for (const repository of [nestedRepository, 'plain']) { + const directory = join(root, repository) + git(directory, ['init', '-q', '.']) + git(directory, ['add', 'token.ts']) + git(directory, ['commit', '-qm', 'Add token']) + } + + const result = await scan(root) + // The unignored nested repository proves the secret is detectable, so the absence is not vacuous. + expect(secretFindingFiles(result)).toEqual(['plain/token.ts']) + expect(scannedPaths(result)).not.toContain(`${nestedRepository}/token.ts`) + }) + + test("walks a nested repository's ignored directory with --no-git-ignore", async () => { + const {outer} = await makeNestedRepository({ + outer: {'.gitignore': 'inner/\n'}, + inner: {'.gitignore': 'build/\n', 'build/a.ts': '', 'kept.ts': ''}, + }) + + const paths = await gatheredPaths(outer, {noGitIgnore: true}) + expect(paths).toEqual(expect.arrayContaining(['inner/build/a.ts', 'inner/kept.ts'])) + }) + + test('treats a symbolic-linked .git as a failed listing: no repository exclusions, but the link itself is skipped', async () => { + const outer = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'inner-link-target-only.ts\n', + }) + const target = await makeRepository({}) + const inner = join(outer, 'inner') + await writeFiles(inner, {'inner-link-target-only.ts': '', 'kept.ts': ''}) + await symlink(join(target, '.git'), join(inner, '.git'), 'dir') + + const paths = await gatheredPaths(outer) + expect(paths).toEqual(expect.arrayContaining(['inner/inner-link-target-only.ts', 'inner/kept.ts'])) + expect(paths.some((path) => path.startsWith('inner/.git'))).toBe(false) + }) + + test('applies a submodule’s own rules and not the outer repository’s', async () => { + const outer = await makeSubmodule() + + const paths = await gatheredPaths(outer) + expect(paths).toEqual(expect.arrayContaining(['index.ts', 'vendor/sub/a.ts', 'vendor/sub/x.log', '.gitmodules'])) + expect(paths).not.toContain('vendor/sub/generated/b.ts') + expect(paths.some((path) => path === 'vendor/sub/.git')).toBe(false) + }) }) - afterEach(() => { - restoreGitConfig() + + describe('a scan directory that its repository ignores', () => { + test('gathers only the files Git tracks there, and reports it', async () => { + const repository = await makeRepository({ + '.gitignore': 'apps/web/\n', + 'apps/web/shopify.app.toml': appConfiguration, + 'apps/web/README.md': 'docs', + 'apps/web/.env': `SHOPIFY_TOKEN=${secret}\n`, + 'apps/web/a.ts': 'export const untracked = true', + }) + git(repository, ['add', '-f', 'apps/web/README.md', '.gitignore']) + git(repository, ['commit', '-qm', 'init']) + const app = join(repository, 'apps', 'web') + + const result = await gather(app) + expect(result.paths).toEqual(['README.md']) + expect(result.ignoredScanDirectories).toEqual([app]) + }) + + test('aborts instead of walking the directory when Git cannot list the files it tracks', async () => { + const repository = await makeRepository({ + '.gitignore': 'apps/web/\n', + 'apps/web/shopify.app.toml': appConfiguration, + 'apps/web/.env': `SHOPIFY_TOKEN=${secret}\n`, + }) + git(repository, ['add', '-f', '.gitignore']) + git(repository, ['commit', '-qm', 'init']) + // A corrupt index makes `ls-files --cached` fail while `check-ignore --no-index` still answers. + await writeFile(join(repository, '.git', 'index'), 'not an index') + const app = join(repository, 'apps', 'web') + + vi.stubEnv('INIT_CWD', repository) + await expect(gather(app)).rejects.toThrow("Couldn't list the files Git tracks in apps/web.") + vi.stubEnv('INIT_CWD', app) + await expect(gather(app)).rejects.toThrow("Couldn't list the files Git tracks in ..") + }) + + test('scans the tracked files and not the untracked ones, and keeps the selected TOML', async () => { + const repository = await makeRepository({ + '.gitignore': 'apps/web/\n', + 'apps/web/shopify.app.toml': appConfiguration, + 'apps/web/tracked.ts': 'export const tracked = true', + 'apps/web/.env': `SHOPIFY_TOKEN=${secret}\n`, + 'apps/web/untracked.ts': 'export const untracked = true', + }) + git(repository, ['add', '-f', 'apps/web/tracked.ts', '.gitignore']) + git(repository, ['commit', '-qm', 'init']) + + const result = await scan(join(repository, 'apps', 'web')) + expect(scannedPaths(result)).toContain('tracked.ts') + expect(scannedPaths(result)).not.toContain('untracked.ts') + expect(secretFindingFiles(result)).toEqual([]) + expect(result.ignoredScanDirectories).toEqual([join(repository, 'apps', 'web')]) + expect(result.app.name).toBe('Discovery safety') + }) + + test('detects an ignored directory whose ancestor the repository ignores', async () => { + const repository = await makeRepository({'.gitignore': 'apps/\n', 'apps/web/shopify.app.toml': appConfiguration}) + const app = join(repository, 'apps', 'web') + + const result = await gather(app) + expect(result.paths).toEqual([]) + expect(result.ignoredScanDirectories).toEqual([app]) + }) + + test("detects a nested repository's top level that the outer repository ignores, and gathers its tracked files", async () => { + const outer = await makeRepository({'.gitignore': 'inner/\n', 'shopify.app.toml': appConfiguration}) + const inner = join(outer, 'inner') + await writeFiles(inner, {'tracked.ts': 'export const tracked = true', 'untracked.ts': ''}) + git(inner, ['init', '-q', '.']) + git(inner, ['add', 'tracked.ts']) + git(inner, ['commit', '-qm', 'init']) + + const result = await gather(outer, {scanDirectories: [outer, inner]}) + expect(result.paths).toEqual(['.gitignore', 'inner/tracked.ts', 'shopify.app.toml']) + expect(result.ignoredScanDirectories).toEqual([inner]) + }) + + test('still applies --exclude to the tracked files, and is not ignored with --no-git-ignore', async () => { + const repository = await makeRepository({ + '.gitignore': 'app/\n', + 'app/keep.ts': '', + 'app/generated/skip.ts': '', + 'app/untracked.ts': '', + }) + git(repository, ['add', '-f', 'app/keep.ts', 'app/generated/skip.ts', '.gitignore']) + git(repository, ['commit', '-qm', 'init']) + const app = join(repository, 'app') + vi.stubEnv('INIT_CWD', app) + + await expect(gatheredPaths(app, {excludePatterns: ['generated']})).resolves.toEqual(['keep.ts']) + const everything = await gather(app, {noGitIgnore: true}) + expect(everything.paths).toEqual(['generated/skip.ts', 'keep.ts', 'untracked.ts']) + expect(everything.ignoredScanDirectories).toEqual([]) + }) + + test('does not treat a directory that a whitelist-style repository re-includes as ignored', async () => { + const repository = await makeRepository({ + '.gitignore': '/*\n!/apps/\n/apps/*\n!/apps/web/\n', + 'apps/web/shopify.app.toml': appConfiguration, + 'apps/web/a.ts': '', + }) + const app = join(repository, 'apps', 'web') + + const result = await gather(app) + expect(result.ignoredScanDirectories).toEqual([]) + expect(result.paths).toEqual(['a.ts', 'shopify.app.toml']) + }) }) +}) - test('excludes a folder that neither the defaults nor .gitignore cover', async () => { +describe('--exclude', () => { + async function makeApp(): Promise { const root = await makeDirectory() await writeFiles(root, { 'shopify.app.toml': appConfiguration, @@ -562,142 +726,146 @@ describe('--ignore patterns', () => { 'generated/client.ts': 'export const excluded = true', 'web/generated/schema.ts': 'export const excluded = true', }) + vi.stubEnv('INIT_CWD', root) + return root + } + + test('with a bare name matches only at the top of the working directory', async () => { + const root = await makeApp() + + const paths = scannedPaths(await scan(root, undefined, {excludePatterns: ['generated']})) + expect(paths).toContain('src/index.ts') + expect(paths).not.toContain('generated/client.ts') + expect(paths).toContain('web/generated/schema.ts') + }) + + test('with **/ matches at any depth', async () => { + const root = await makeApp() - const paths = scannedPaths(await scan(root, undefined, {ignorePatterns: ['generated/']})) + const paths = scannedPaths(await scan(root, undefined, {excludePatterns: ['**/generated']})) expect(paths).toContain('src/index.ts') expect(paths).not.toContain('generated/client.ts') expect(paths).not.toContain('web/generated/schema.ts') }) - test('re-includes a default exclusion at the root only when the pattern is anchored', async () => { - const root = await makeDirectory() - await writeFiles(root, { - 'shopify.app.toml': appConfiguration, - 'build/x.ts': 'export const rootBuild = true', - 'packages/a/build/y.ts': 'export const nestedBuild = true', + test('with ../ matches paths above the working directory', async () => { + const parent = await makeDirectory() + await writeFiles(parent, { + 'app/shopify.app.toml': appConfiguration, + 'app/src/index.ts': 'export const included = true', + 'backend/src/server.ts': 'export const excluded = true', + 'backend/keep/server.ts': 'export const included = true', }) + vi.stubEnv('INIT_CWD', join(parent, 'app')) - // `/build/` is anchored to the app directory; the nested `build/` stays excluded by the default. - const anchored = scannedPaths(await scan(root, undefined, {ignorePatterns: ['!/build/']})) - expect(anchored).toContain('build/x.ts') - expect(anchored).not.toContain('packages/a/build/y.ts') - - // `build/` without a slash prefix matches at any depth, like the default it overrides. - const unanchored = scannedPaths(await scan(root, undefined, {ignorePatterns: ['!build/']})) - expect(unanchored).toContain('build/x.ts') - expect(unanchored).toContain('packages/a/build/y.ts') + const paths = await gatheredPaths(parent, { + appDirectory: join(parent, 'app'), + excludePatterns: ['../backend/src/**'], + }) + expect(paths).toEqual(['../backend/keep/server.ts', 'src/index.ts', 'shopify.app.toml'].sort()) }) - test('re-includes a gitignored folder', async () => { - const root = await makeRepository({ - 'shopify.app.toml': appConfiguration, - '.gitignore': 'tmp/\n', - 'tmp/scratch.ts': 'export const reincluded = true', - }) + test('applies with --no-git-ignore', async () => { + const root = await makeApp() - expect(scannedPaths(await scan(root))).not.toContain('tmp/scratch.ts') - expect(scannedPaths(await scan(root, undefined, {ignorePatterns: ['!tmp/']}))).toContain('tmp/scratch.ts') + const paths = await gatheredPaths(root, {excludePatterns: ['**/generated'], noGitIgnore: true}) + expect(paths).toEqual(['shopify.app.toml', 'src/index.ts']) }) - test('re-includes a nested repository that the app repository ignores', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') - const root = await makeRepository({ - 'shopify.app.toml': appConfiguration, - '.gitignore': 'inner/\n', - 'inner/token.ts': `export const token = '${secret}'\n`, + test("can't remove the selected app configuration file", async () => { + const root = await makeApp() + const selected = join(root, 'shopify.app.toml') + + const withoutSelected = await gatheredPaths(root, {excludePatterns: ['shopify.app.toml']}) + expect(withoutSelected).not.toContain('shopify.app.toml') + const withSelected = await gatheredPaths(root, { + excludePatterns: ['shopify.app.toml'], + selectedAppConfigFilePath: selected, }) - const inner = join(root, 'inner') - git(inner, ['init', '-q', '.']) - git(inner, ['add', 'token.ts']) - git(inner, ['commit', '-qm', 'Add token']) + expect(withSelected).toContain('shopify.app.toml') - expect(secretFindingFiles(await scan(root))).toEqual([]) - expect(secretFindingFiles(await scan(root, undefined, {ignorePatterns: ['!inner/']}))).toEqual(['inner/token.ts']) + const result = await scan(root, undefined, {excludePatterns: ['shopify.app.toml', '**/*.toml']}) + expect(result.app.name).toBe('Discovery safety') }) - test('cannot re-include a file inside a gitignored folder without re-including the folder', async () => { - const root = await makeRepository({ + test('still scans an excluded selected app configuration file for secrets', async () => { + const root = await makeDirectory() + await writeFiles(root, { 'shopify.app.toml': appConfiguration, - '.gitignore': 'tmp/\n', - 'tmp/keep.ts': 'export const stillExcluded = true', - 'tmp/scratch.ts': 'export const stillExcluded = true', + 'shopify.app.test.toml': `name = "Test"\napplication_url = "https://test.example.com/?token=${secret}"\n`, }) + vi.stubEnv('INIT_CWD', root) - const paths = scannedPaths(await scan(root, undefined, {ignorePatterns: ['!tmp/keep.ts']})) - expect(paths).not.toContain('tmp/keep.ts') - expect(paths).not.toContain('tmp/scratch.ts') + const result = await scan(root, 'test', {excludePatterns: ['shopify.app.test.toml']}) + expect(result.app.name).toBe('Test') + expect(secretFindingFiles(result)).toEqual(['shopify.app.test.toml']) }) - test('still applies .gitignore inside a re-included default folder', async () => { - // Re-including `build/` must turn off git's default-directory pruning, or git never lists this file. + test('adds a gitignored selected app configuration file after filtering', async () => { const root = await makeRepository({ 'shopify.app.toml': appConfiguration, - '.gitignore': '*.local.json\n', - 'build/a.ts': 'export const reincluded = true', - 'build/x.local.json': '{"ignored": true}', + '.gitignore': 'shopify.app.toml\n', }) - const paths = scannedPaths(await scan(root, undefined, {ignorePatterns: ['!build/']})) - expect(paths).toContain('build/a.ts') - expect(paths).not.toContain('build/x.local.json') + const paths = await gatheredPaths(root, {selectedAppConfigFilePath: join(root, 'shopify.app.toml')}) + expect(paths).toEqual(['.gitignore', 'shopify.app.toml']) }) +}) - test('applies later patterns over earlier ones', async () => { +describe('symbolic links', () => { + test('reads a symbolic-linked selected app configuration file that points outside the app directory', async () => { const root = await makeDirectory() - await writeFiles(root, { - 'shopify.app.toml': appConfiguration, - 'generated/client.ts': 'export const decided = true', + const outside = await makeDirectory() + await writeFiles(outside, { + 'real.toml': `name = "Linked"\napplication_url = "https://linked.example.com/?token=${secret}"\n`, }) + await symlink(join(outside, 'real.toml'), join(root, 'shopify.app.toml')) - const excludeThenInclude = scannedPaths( - await scan(root, undefined, {ignorePatterns: ['generated/', '!generated/']}), - ) - expect(excludeThenInclude).toContain('generated/client.ts') + const result = await scan(root) + expect(result.app.name).toBe('Linked') + expect(secretFindingFiles(result)).toEqual(['shopify.app.toml']) + expect(result.scan.files_skipped_count).toBe(0) + }) + + test('refuses another symbolic link that points outside the app directory', async () => { + const root = await makeDirectory() + const outside = await makeDirectory() + await writeFiles(root, {'shopify.app.toml': appConfiguration, 'src/index.ts': 'export const included = true'}) + await writeFiles(outside, {'leak.ts': 'export const leaked = true'}) + await symlink(join(outside, 'leak.ts'), join(root, 'src', 'leak.ts')) - const includeThenExclude = scannedPaths( - await scan(root, undefined, {ignorePatterns: ['!generated/', 'generated/']}), + const result = await scan(root) + expect(result.scan.files_skipped).toContainEqual( + expect.objectContaining({ + path: 'src/leak.ts', + reason: 'unreadable', + detail: 'src/leak.ts resolves outside the scan directory', + }), ) - expect(includeThenExclude).not.toContain('generated/client.ts') + expect(JSON.stringify(result)).not.toContain(outside) }) - test('never stops the selected app configuration from loading or being scanned for secrets', async () => { - const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') + test('reads a symbolic link that stays inside the scan directory', async () => { const root = await makeDirectory() - await writeFiles(root, { - 'shopify.app.toml': appConfiguration, - 'shopify.app.staging.toml': `name = "Staging"\napplication_url = "https://staging.example.com/?token=${secret}"\n`, - }) + await writeFiles(root, {'shopify.app.toml': appConfiguration, 'src/real.ts': 'export const real = true'}) + await symlink(join(root, 'src', 'real.ts'), join(root, 'src', 'alias.ts')) - const result = await scan(root, 'staging', {ignorePatterns: ['shopify.app*.toml']}) - expect(result.app.name).toBe('Staging') - expect(scannedPaths(result)).toContain('shopify.app.staging.toml') - expect(secretFindingFiles(result)).toEqual(['shopify.app.staging.toml']) + const result = await scan(root) + expect(scannedPaths(result)).toEqual(expect.arrayContaining(['src/alias.ts', 'src/real.ts'])) + expect(result.scan.files_skipped_count).toBe(0) }) }) -describe('listRepositoryFiles', () => { - test('lists a symlinked directory as an entry without traversing it', async () => { +describe('gatherPaths', () => { + test('lists a symbolic-linked directory as an entry without traversing it', async () => { const root = await makeDirectory() await writeFiles(root, {'shopify.app.toml': appConfiguration, 'real/inner.ts': 'export const inner = true'}) await symlink(join(root, 'real'), join(root, 'linked'), 'dir') - expect(listRepositoryFiles(root, DEFAULT_RULES)).toEqual(['linked', 'real/inner.ts', 'shopify.app.toml']) - }) - - test('skips a nested app directory even when its configuration file is excluded by a rule', async () => { - const root = await makeDirectory() - await writeFiles(root, { - 'shopify.app.toml': appConfiguration, - 'index.ts': 'export const parent = true', - 'apps/child/shopify.app.toml': 'name = "Child"\n', - 'apps/child/index.ts': 'export const child = true', - }) - const rules = buildPathRules({gitIgnoredPaths: ['apps/child/shopify.app.toml']}) - - expect(listRepositoryFiles(root, rules)).toEqual(['index.ts', 'shopify.app.toml']) + await expect(gatheredPaths(root)).resolves.toEqual(['linked', 'real/inner.ts', 'shopify.app.toml']) }) - test('returns sorted app-root-relative POSIX paths', async () => { + test('returns sorted, unique, app-directory-relative POSIX paths', async () => { const root = await makeDirectory() await writeFiles(root, { 'z.ts': '', @@ -707,7 +875,7 @@ describe('listRepositoryFiles', () => { 'b/a/e.ts': '', }) - const files = listRepositoryFiles(root, DEFAULT_RULES) + const files = await gatheredPaths(root, {selectedAppConfigFilePath: join(root, 'a.ts')}) expect(files).toEqual(['a.ts', 'b/a/e.ts', 'b/c.ts', 'b/d.ts', 'z.ts']) expect(files).toEqual([...files].sort()) }) @@ -724,8 +892,7 @@ describe('listRepositoryFiles', () => { const locked = join(root, 'locked') await chmod(locked, 0o000) try { - resetSkippedFiles() - expect(listRepositoryFiles(root, DEFAULT_RULES)).toEqual(['shopify.app.toml', 'src/index.ts']) + await expect(gatheredPaths(root)).resolves.toEqual(['shopify.app.toml', 'src/index.ts']) expect(getSkippedFiles()).toEqual([ {path: 'locked', reason: 'unreadable', detail: 'Could not inspect locked (EACCES)'}, ]) @@ -736,16 +903,15 @@ describe('listRepositoryFiles', () => { ) test.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( - 'names the app root when it cannot be read', + 'names the scan directory when it cannot be read', async () => { const root = await makeDirectory() await writeFiles(root, {'shopify.app.toml': appConfiguration}) await chmod(root, 0o000) try { - resetSkippedFiles() - expect(listRepositoryFiles(root, DEFAULT_RULES)).toEqual([]) + await expect(gatheredPaths(root, {noGitIgnore: true})).resolves.toEqual([]) expect(getSkippedFiles()).toEqual([ - {path: root, reason: 'unreadable', detail: 'Could not inspect app root (EACCES)'}, + {path: root, reason: 'unreadable', detail: 'Could not inspect scan directory (EACCES)'}, ]) } finally { await chmod(root, 0o755) 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 6f875f9522f..1c78744424a 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 @@ -1,20 +1,19 @@ /* eslint-disable no-restricted-imports -- path rules are verified against real temporary git repositories */ import {git, isolateGitConfig} from './git-test-helpers.js' import { - DEFAULT_EXCLUDE_PATTERNS, - buildPathRules, - createFilePathMatcher, - createPathMatcher, + createPathRules, + isDroppedEntry, + isDroppedTrackedPath, + isIgnoredByParentRepository, listGitIgnoredPaths, - ignorePatternProblem, - ignorePatternRules, + listNestedRepository, + listTrackedFiles, + repositoryIgnoredPaths, } from '../scanners/path-rules.js' -import {BugError} from '@shopify/cli-kit/node/error' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' -import {mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync} from 'node:fs' +import {mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync} from 'node:fs' import {tmpdir} from 'node:os' import {join} from 'node:path' -import type {PathOverride, PathRules} from '../scanners/path-rules.js' const temporaryDirectories: string[] = [] let restoreGitConfig: (() => void) | undefined @@ -29,7 +28,7 @@ afterEach(() => { }) function makeDirectory(prefix = 'app-security-path-rules-'): string { - const directory = mkdtempSync(join(tmpdir(), prefix)) + const directory = realpathSync(mkdtempSync(join(tmpdir(), prefix))) temporaryDirectories.push(directory) return directory } @@ -49,17 +48,13 @@ function makeRepository(files: Record): string { return root } -const DEFAULTS_ONLY: PathRules = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: [], overrides: []} - -const gitIgnoredOnly = (paths: string[]): PathRules => ({defaults: [], gitIgnoredPaths: paths, overrides: []}) - -const cliExclude = (pattern: string): PathOverride => ({action: 'exclude', pattern, source: 'cli'}) -const cliInclude = (pattern: string): PathOverride => ({action: 'include', pattern, source: 'cli'}) - -const PRUNED = {pruneDefaultDirectories: true} +function commitAll(root: string): void { + git(root, ['add', '-A']) + git(root, ['commit', '-qm', 'init']) +} -async function listedPaths(appRoot: string, options = PRUNED): Promise { - const listing = await listGitIgnoredPaths(appRoot, options) +async function listedPaths(directory: string): Promise { + const listing = await listGitIgnoredPaths(directory) expect(listing.status).toBe('listed') return listing.status === 'listed' ? listing.paths : [] } @@ -68,7 +63,7 @@ describe('listGitIgnoredPaths', () => { test('collapses a fully ignored directory to a single trailing-slash entry', async () => { const root = makeRepository({'.gitignore': 'tmp/\n', 'tmp/a.ts': '', 'tmp/nested/b.ts': '', 'src/index.ts': ''}) - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) }) test('reports an ignored single file', async () => { @@ -85,13 +80,25 @@ describe('listGitIgnoredPaths', () => { await expect(listedPaths(root)).resolves.toEqual([]) }) + test('does not exclude node_modules or any other directory by name', async () => { + const root = makeRepository({ + '.gitignore': '*.log\n', + 'node_modules/pkg/index.js': '', + 'node_modules/pkg/debug.log': '', + 'src/index.ts': '', + 'src/app.log': '', + }) + + await expect(listedPaths(root)).resolves.toEqual(['node_modules/pkg/debug.log', 'src/app.log']) + }) + test('honours gitignore negation', async () => { const root = makeRepository({'.gitignore': '.env*\n!.env.example\n', '.env': 'SECRET=1\n', '.env.example': 'X=\n'}) await expect(listedPaths(root)).resolves.toEqual(['.env']) }) - test('honours a nested .gitignore and reports paths relative to the app root', async () => { + test('honours a nested .gitignore and reports paths relative to the listed directory', async () => { const root = makeRepository({ 'web/.gitignore': 'generated/\n', 'web/generated/schema.ts': '', @@ -108,7 +115,7 @@ describe('listGitIgnoredPaths', () => { await expect(listedPaths(root)).resolves.toEqual(['scratch.ts']) }) - test('reports paths relative to an app nested inside a larger repository, applying the parent .gitignore', async () => { + test('reports paths relative to a directory nested inside a larger repository, applying the parent .gitignore', async () => { const repository = makeRepository({ '.gitignore': 'tmp/\n*.log\n', 'apps/my-app/shopify.app.toml': '', @@ -149,7 +156,6 @@ describe('listGitIgnoredPaths', () => { symlinkSync(join(root, 'real'), join(root, '.github'), 'dir') await expect(listedPaths(root)).resolves.toEqual(['.github']) - expect(createFilePathMatcher(gitIgnoredOnly(['.github']))('.github/dependabot.yml')).toBe(false) }, ) @@ -174,74 +180,13 @@ describe('listGitIgnoredPaths', () => { const root = makeDirectory() writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'not-a-repository'}) - }) - - test('reports an app folder the enclosing repository ignores, even with a force-tracked descendant', async () => { - // A force-tracked file stops git collapsing the app to `./`; it lists each file instead. - const repository = makeRepository({ - '.gitignore': 'apps/web/\n', - 'apps/web/README.md': 'docs', - 'apps/web/.env': 'SECRET=1\n', - 'apps/web/a.ts': '', - }) - git(repository, ['add', '-f', 'apps/web/README.md', '.gitignore']) - git(repository, ['commit', '-qm', 'init']) - - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ - status: 'app-root-ignored', - }) - }) - - test('reports an app folder whose ancestor the enclosing repository ignores', async () => { - const repository = makeRepository({ - '.gitignore': 'apps/\n', - 'apps/web/shopify.app.toml': '', - 'apps/web/src/index.ts': '', - }) - - // Control: from the repository root, git lists the ignored folder. - await expect(listedPaths(repository)).resolves.toEqual(['apps/']) - - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ - status: 'app-root-ignored', - }) - }) - - test('does not treat the top level of a repository as ignored', async () => { - const root = makeRepository({'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) - - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) - }) - - test('does not treat the top level of a whitelist-style repository as ignored', async () => { - const root = makeRepository({ - '.gitignore': '*\n!src/\n!src/**\n!.gitignore\n', - 'src/index.ts': '', - 'tmp.log': '', - }) - - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'listed', paths: ['tmp.log']}) - }) - - test('does not treat an app folder a whitelist-style repository re-includes as ignored', async () => { - const repository = makeRepository({ - '.gitignore': '*\n!*/\n!*.ts\n!.gitignore\n', - 'apps/web/shopify.app.toml': '', - 'apps/web/src/index.ts': '', - 'apps/web/.env': 'SECRET=1\n', - }) - - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ - status: 'listed', - paths: ['.env', 'shopify.app.toml'], - }) + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'not-a-repository'}) }) test('reports a directory inside .git as not being in a repository', async () => { const root = makeRepository({}) - await expect(listGitIgnoredPaths(join(root, '.git'), PRUNED)).resolves.toEqual({status: 'not-a-repository'}) + await expect(listGitIgnoredPaths(join(root, '.git'))).resolves.toEqual({status: 'not-a-repository'}) }) test.each([ @@ -252,11 +197,10 @@ describe('listGitIgnoredPaths', () => { async (file, content) => { // Both make `rev-parse` exit 128, as it does outside any repository. const root = makeRepository({'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) - git(root, ['add', '.gitignore']) - git(root, ['commit', '-qm', 'init']) + commitAll(root) writeFileSync(join(root, '.git', file), content) - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) }, ) @@ -268,485 +212,247 @@ describe('listGitIgnoredPaths', () => { writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) symlinkSync(join(root, 'missing-git-dir'), join(root, '.git')) - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) }, ) - test('reports a failure when git refuses a repository enclosing the app folder', async () => { + test('reports a failure when git refuses a repository enclosing the directory', async () => { const repository = makeRepository({'apps/web/src/index.ts': ''}) writeFileSync(join(repository, '.git', 'config'), '[core\nbogus') - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).resolves.toEqual({status: 'failed'}) }) test('reports a failure when git cannot list the working tree', async () => { const root = makeRepository({'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) git(root, ['add', '.gitignore']) git(root, ['commit', '-qm', 'init']) - // A truncated index leaves `rev-parse` and `check-ignore --no-index` working but makes - // `ls-files` exit with a fatal error. + // A truncated index leaves `rev-parse` working but makes `ls-files` exit with a fatal error. writeFileSync(join(root, '.git', 'index'), 'not an index') - await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) - }) - }) - - describe('default directories are excluded from the git listing', () => { - test('never lists paths inside an unignored node_modules directory', async () => { - const root = makeRepository({ - '.gitignore': '*.log\n', - 'node_modules/pkg/index.js': '', - 'node_modules/pkg/debug.log': '', - 'packages/api/node_modules/other/error.log': '', - 'my-fixtures/sub/fixture.log': '', - 'src/index.ts': '', - 'src/app.log': '', - }) - - const paths = await listedPaths(root) - - expect(paths.some((path) => path.includes('node_modules'))).toBe(false) - expect(paths.some((path) => path.includes('my-fixtures'))).toBe(false) - expect(paths).toEqual(['src/app.log']) - }) - - test('still lists an ignored file inside a folder that is not a default exclusion', async () => { - const root = makeRepository({ - '.gitignore': '*.log\n', - 'scratch/notes.log': '', - 'scratch/keep.ts': '', - 'node_modules/pkg/debug.log': '', - 'node_modules/pkg/index.js': '', - }) - - await expect(listedPaths(root)).resolves.toEqual(['scratch/notes.log']) - }) - - test('lists paths inside the default directories when pruning is off', async () => { - const root = makeRepository({ - '.gitignore': '*.log\n', - 'node_modules/pkg/index.js': '', - 'node_modules/pkg/debug.log': '', - 'src/index.ts': '', - 'src/app.log': '', - }) - - await expect(listedPaths(root)).resolves.toEqual(['src/app.log']) - await expect(listedPaths(root, {pruneDefaultDirectories: false})).resolves.toEqual([ - 'node_modules/pkg/debug.log', - 'src/app.log', - ]) + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) }) }) }) -describe('gitIgnoredPaths', () => { - test('excludes exactly the reported path, never a sibling, whatever characters it contains', () => { - const cases: {path: string; siblings: string[]}[] = [ - {path: 'q?.ts', siblings: ['qx.ts', 'q.ts', 'sub/q?.ts']}, - {path: '[id].ts', siblings: ['id.ts', 'i.ts', 'sub/[id].ts']}, - {path: 'a*b.ts', siblings: ['axb.ts', 'ab.ts', 'sub/a*b.ts']}, - {path: '#hash.ts', siblings: ['hash.ts', 'sub/#hash.ts']}, - {path: '!bang.ts', siblings: ['bang.ts', 'sub/!bang.ts']}, - {path: 'sp ace.ts', siblings: ['space.ts', 'sub/sp ace.ts']}, - {path: 'back\\slash.ts', siblings: ['backslash.ts', 'sub/back\\slash.ts']}, - {path: 'line\nbreak.ts', siblings: ['linebreak.ts', 'line', 'break.ts']}, - {path: 'carriage\rreturn.ts', siblings: ['carriagereturn.ts']}, - ] - - for (const {path, siblings} of cases) { - const isExcluded = createPathMatcher(gitIgnoredOnly([path])) - expect(isExcluded(path, {directory: false}), `${JSON.stringify(path)} should be excluded`).toBe(true) - for (const sibling of siblings) { - expect( - isExcluded(sibling, {directory: false}), - `${JSON.stringify(sibling)} should not be excluded by ${JSON.stringify(path)}`, - ).toBe(false) - } - } - }) - - test('matches a collapsed directory entry as a directory only', () => { - // Contents of `tmp/` are never asked about: the walker prunes the excluded directory. - const isExcluded = createPathMatcher(gitIgnoredOnly(['tmp/'])) - - expect(isExcluded('tmp', {directory: true})).toBe(true) - expect(isExcluded('tmp', {directory: false})).toBe(false) - expect(isExcluded('src/tmp.ts', {directory: false})).toBe(false) - expect(isExcluded('src/tmp', {directory: true})).toBe(false) - }) - - test('matches a file entry as a file only', () => { - const isExcluded = createPathMatcher(gitIgnoredOnly(['notes.txt'])) +describe('isIgnoredByParentRepository', () => { + test('is true for a directory that the repository containing its parent ignores', async () => { + const repository = makeRepository({'.gitignore': 'dist/\n', 'dist/a.js': '', 'src/a.ts': ''}) - expect(isExcluded('notes.txt', {directory: false})).toBe(true) - expect(isExcluded('notes.txt', {directory: true})).toBe(false) - expect(isExcluded('sub/notes.txt', {directory: false})).toBe(false) + await expect(isIgnoredByParentRepository(join(repository, 'dist'))).resolves.toBe(true) + await expect(isIgnoredByParentRepository(join(repository, 'src'))).resolves.toBe(false) }) - test('applies alongside the defaults', () => { - const isExcluded = createPathMatcher(buildPathRules({gitIgnoredPaths: ['notes.txt', 'tmp/']})) - - expect(isExcluded('notes.txt', {directory: false})).toBe(true) - expect(isExcluded('tmp', {directory: true})).toBe(true) - expect(isExcluded('node_modules', {directory: true})).toBe(true) - expect(isExcluded('src/index.ts', {directory: false})).toBe(false) - }) -}) + test('is true even when a file in the directory is force-tracked', async () => { + const repository = makeRepository({'.gitignore': 'dist/\n', 'dist/a.js': '', 'dist/b.js': ''}) + git(repository, ['add', '-f', 'dist/a.js', '.gitignore']) + git(repository, ['commit', '-qm', 'init']) -describe('DEFAULT_EXCLUDE_PATTERNS', () => { - const isExcluded = createPathMatcher(DEFAULTS_ONLY) - - test('excludes every default directory at the root and at any depth', () => { - const directories = [ - 'node_modules', - 'vendor', - '.next', - 'coverage', - 'dist', - 'build', - '.shopify', - 'test', - 'tests', - 'spec', - 'specs', - '__tests__', - 'fixtures', - 'my-fixtures', - '__fixtures__', - '.yarn', - '.react-router', - '.cache', - '.turbo', - '.vercel', - '.netlify', - '.output', - '.nuxt', - '.svelte-kit', - ] - - for (const directory of directories) { - expect(isExcluded(directory, {directory: true}), `${directory}/ at root`).toBe(true) - expect(isExcluded(`packages/a/${directory}`, {directory: true}), `nested ${directory}/`).toBe(true) - expect(isExcluded(`${directory}/index.ts`, {directory: false}), `file inside ${directory}/`).toBe(true) - } + await expect(isIgnoredByParentRepository(join(repository, 'dist'))).resolves.toBe(true) }) - test('excludes .git as both a directory and the file used by git worktrees', () => { - expect(isExcluded('.git', {directory: true})).toBe(true) - expect(isExcluded('.git', {directory: false})).toBe(true) - expect(isExcluded('packages/a/.git', {directory: false})).toBe(true) - }) + test('is true for a directory whose ancestor the repository ignores', async () => { + const repository = makeRepository({'.gitignore': 'apps/\n', 'apps/web/src/index.ts': ''}) - test('excludes test files and fixture directories by pattern', () => { - expect(isExcluded('a.test.ts', {directory: false})).toBe(true) - expect(isExcluded('src/a.spec.tsx', {directory: false})).toBe(true) - expect(isExcluded('my-fixtures/x.ts', {directory: false})).toBe(true) + await expect(isIgnoredByParentRepository(join(repository, 'apps', 'web'))).resolves.toBe(true) }) - test('does not exclude source, environment files, or CI and editor configuration', () => { - const scannable = [ - '.github/workflows/ci.yml', - '.vscode/settings.json', - '.devcontainer/devcontainer.json', - '.circleci/config.yml', - '.env', - 'src/index.ts', - '.eslintrc.cjs', - 'testing/helpers.ts', - 'src/builder.ts', - ] - - for (const path of scannable) { - expect(isExcluded(path, {directory: false}), `${path} should be scanned`).toBe(false) - } - }) + test("is true for a nested repository's top level that the outer repository ignores", async () => { + const outer = makeRepository({'.gitignore': 'inner/\n'}) + const inner = join(outer, 'inner') + mkdirSync(inner) + git(inner, ['init', '-q', '.']) - test('does not throw for names made only of dots', () => { - expect(isExcluded('...', {directory: false})).toBe(false) - expect(isExcluded('src/...', {directory: false})).toBe(false) - expect(isExcluded('...', {directory: true})).toBe(false) + await expect(isIgnoredByParentRepository(inner)).resolves.toBe(true) }) -}) -describe('createPathMatcher', () => { - test('is case sensitive', () => { - const isExcluded = createPathMatcher({defaults: ['Build/'], gitIgnoredPaths: [], overrides: []}) + test('is false when the directory is the top level of its own repository and nothing ignores it', async () => { + const repository = makeRepository({'src/a.ts': ''}) - expect(isExcluded('Build/a.ts', {directory: false})).toBe(true) - expect(isExcluded('build/a.ts', {directory: false})).toBe(false) + await expect(isIgnoredByParentRepository(repository)).resolves.toBe(false) }) - describe('with overrides', () => { - test('an include re-includes a gitignored directory and, as it is no longer pruned, its contents', () => { - const isExcluded = createPathMatcher({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['tmp/'], - overrides: [cliInclude('tmp/')], - }) - - expect(isExcluded('tmp', {directory: true})).toBe(false) - expect(isExcluded('tmp/a.ts', {directory: false})).toBe(false) - }) - - test('an include for one file inside a gitignored directory does not re-include the directory', () => { - const isExcluded = createPathMatcher({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['tmp/'], - overrides: [cliInclude('tmp/keep.ts')], - }) - - expect(isExcluded('tmp', {directory: true})).toBe(true) - }) - - test('an exclude wins over the defaults, the git literals and an earlier include', () => { - const isExcluded = createPathMatcher({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['notes.txt'], - overrides: [cliInclude('keep.ts'), cliExclude('keep.ts'), cliExclude('*.md'), cliExclude('generated/')], - }) - - expect(isExcluded('keep.ts', {directory: false})).toBe(true) - expect(isExcluded('README.md', {directory: false})).toBe(true) - expect(isExcluded('docs/guide.md', {directory: false})).toBe(true) - expect(isExcluded('generated', {directory: true})).toBe(true) - expect(isExcluded('web/generated', {directory: true})).toBe(true) - expect(isExcluded('notes.txt', {directory: false})).toBe(true) - expect(isExcluded('node_modules', {directory: true})).toBe(true) - expect(isExcluded('src/index.ts', {directory: false})).toBe(false) - }) - - test('an include re-includes a default exclusion without touching other defaults', () => { - const isExcluded = createPathMatcher({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['notes.txt'], - overrides: [cliInclude('web/build/')], - }) - - expect(isExcluded('web/build', {directory: true})).toBe(false) - expect(isExcluded('web/build/a.ts', {directory: false})).toBe(false) - expect(isExcluded('build/a.ts', {directory: false})).toBe(true) - expect(isExcluded('notes.txt', {directory: false})).toBe(true) - }) - - test('a default that matches a file inside a re-included directory still excludes it', () => { - const isExcluded = createPathMatcher({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['notes.txt'], - overrides: [cliInclude('web/build/')], - }) - - expect(isExcluded('web/build/a.test.ts', {directory: false})).toBe(true) - }) + test("is false when the parent isn't in a repository", async () => { + const root = makeDirectory() + writeFiles(root, {'.gitignore': 'app/\n', 'app/a.ts': ''}) - test('the defaults and the git literals decide when no override matches', () => { - const isExcluded = createPathMatcher({ - defaults: ['*.log'], - gitIgnoredPaths: ['notes.txt'], - overrides: [cliInclude('other.ts')], - }) - - expect(isExcluded('debug.log', {directory: false})).toBe(true) - expect(isExcluded('notes.txt', {directory: false})).toBe(true) - expect(isExcluded('other.ts', {directory: false})).toBe(false) - expect(isExcluded('src/index.ts', {directory: false})).toBe(false) - }) + await expect(isIgnoredByParentRepository(join(root, 'app'))).resolves.toBe(false) }) - describe('with --ignore patterns', () => { - test('later CLI patterns win over earlier ones', () => { - const fromPatterns = (ignorePatterns: string[]) => - createPathMatcher(buildPathRules({gitIgnoredPaths: [], overrides: ignorePatternRules(ignorePatterns)})) - const excludeThenInclude = fromPatterns(['generated/', '!generated/']) - const includeThenExclude = fromPatterns(['!generated/', 'generated/']) + test('is false for a directory whose name starts with a dash', async () => { + const repository = makeRepository({'-odd/a.ts': ''}) - expect(excludeThenInclude('generated', {directory: true})).toBe(false) - expect(includeThenExclude('generated', {directory: true})).toBe(true) - }) + await expect(isIgnoredByParentRepository(join(repository, '-odd'))).resolves.toBe(false) }) +}) - test('handles many gitignore literals and many lookups', () => { - // Compiling literals into patterns instead of a Set makes this take about a minute. - const rules = buildPathRules({ - gitIgnoredPaths: Array.from({length: 50_000}, (_, index) => `dir${index % 100}/.DS_Store${index}`), +describe('listTrackedFiles', () => { + test('lists only the files git tracks in the directory, relative to it', async () => { + const repository = makeRepository({ + '.gitignore': 'dist/\n', + 'apps/web/src/a.ts': '', + 'apps/web/dist/b.js': '', + 'apps/web/untracked.ts': '', + 'other/c.ts': '', }) - const isExcluded = createPathMatcher(rules) - const paths = Array.from({length: 50_000}, (_, index) => `src/module${index % 500}/file${index}.ts`) - - let excluded = 0 - for (const path of paths) if (isExcluded(path, {directory: false})) excluded += 1 + git(repository, ['add', '-f', 'apps/web/src/a.ts', 'apps/web/dist/b.js', 'other/c.ts']) + git(repository, ['commit', '-qm', 'init']) - expect(excluded).toBe(0) - expect(isExcluded('dir7/.DS_Store7', {directory: false})).toBe(true) - expect(isExcluded('dir7/.DS_Store', {directory: false})).toBe(false) + await expect(listTrackedFiles(join(repository, 'apps', 'web'))).resolves.toEqual(['dist/b.js', 'src/a.ts']) }) -}) -describe('createFilePathMatcher', () => { - test('tests a root-level file against the rules directly', () => { - const isExcluded = createFilePathMatcher(gitIgnoredOnly(['.env'])) + test('is undefined outside a repository', async () => { + const root = makeDirectory() - expect(isExcluded('.env')).toBe(true) - expect(isExcluded('.env.example')).toBe(false) + await expect(listTrackedFiles(root)).resolves.toBeUndefined() }) +}) - test('excludes a file below a collapsed git directory literal', () => { - const isExcluded = createFilePathMatcher(gitIgnoredOnly(['tmp/'])) +describe('listNestedRepository', () => { + test('is undefined for a directory without a .git entry', async () => { + const root = makeRepository({'src/a.ts': ''}) - expect(isExcluded('tmp/a/b.ts')).toBe(true) + await expect(listNestedRepository(join(root, 'src'))).resolves.toBeUndefined() }) - test('does not exclude a sibling whose name merely starts with an excluded directory', () => { - const isExcluded = createFilePathMatcher(gitIgnoredOnly(['tmp/'])) + test('lists the repository whose top level is the directory, using its own ignore rules', async () => { + const outer = makeRepository({'.gitignore': 'outer-only.ts\n'}) + const inner = join(outer, 'inner') + writeFiles(inner, {'.gitignore': 'inner-only.ts\n', 'inner-only.ts': '', 'outer-only.ts': ''}) + git(inner, ['init', '-q', '.']) - expect(isExcluded('tmpx/a.ts')).toBe(false) + await expect(listNestedRepository(inner)).resolves.toEqual({status: 'listed', paths: ['inner-only.ts']}) }) - test('excludes a file below a default pattern directory at any depth', () => { - const isExcluded = createFilePathMatcher(DEFAULTS_ONLY) - - expect(isExcluded('packages/web/node_modules/dep/index.js')).toBe(true) - expect(isExcluded('packages/web/src/index.js')).toBe(false) - }) + test('lists a submodule, whose .git is a file', async () => { + const submodule = makeRepository({'.gitignore': 'generated/\n', 'generated/a.ts': '', 'a.ts': ''}) + commitAll(submodule) + const outer = makeRepository({}) + git(outer, ['-c', 'protocol.file.allow=always', 'submodule', 'add', '-q', submodule, 'vendor/sub']) + writeFiles(join(outer, 'vendor', 'sub'), {'generated/a.ts': ''}) - test('lets an include override re-include a git file literal', () => { - const isExcluded = createFilePathMatcher({ - defaults: [], - gitIgnoredPaths: ['.github/dependabot.yml'], - overrides: [cliInclude('.github/dependabot.yml')], + await expect(listNestedRepository(join(outer, 'vendor', 'sub'))).resolves.toEqual({ + status: 'listed', + paths: ['generated/'], }) - - expect(isExcluded('.github/dependabot.yml')).toBe(false) }) - test('lets an exclude override exclude a file the defaults and git would keep', () => { - const isExcluded = createFilePathMatcher({defaults: [], gitIgnoredPaths: [], overrides: [cliExclude('.github/')]}) + test.skipIf(process.platform === 'win32')('is failed when .git is a symbolic link', async () => { + const outer = makeRepository({}) + const inner = join(outer, 'inner') + const target = makeRepository({}) + mkdirSync(inner) + symlinkSync(join(target, '.git'), join(inner, '.git'), 'dir') - expect(isExcluded('.github/dependabot.yml')).toBe(true) - expect(isExcluded('renovate.json')).toBe(false) + await expect(listNestedRepository(inner)).resolves.toEqual({status: 'failed'}) }) }) -describe('ignorePatternRules', () => { - test('turns a plain line into a CLI exclude rule and a `!` line into a CLI include rule', () => { - expect(ignorePatternRules(['generated/', '!build/', '*.log', '/docs'])).toEqual([ - cliExclude('generated/'), - cliInclude('build/'), - cliExclude('*.log'), - cliExclude('/docs'), - ]) - }) - - test('passes gitignore escapes through unchanged so `\\!` excludes a literal `!` name', () => { - const overrides = ignorePatternRules(['\\!bang.ts', '\\#hash.ts']) - expect(overrides).toEqual([cliExclude('\\!bang.ts'), cliExclude('\\#hash.ts')]) - - const isExcluded = createPathMatcher({defaults: [], gitIgnoredPaths: [], overrides}) - expect(isExcluded('!bang.ts', {directory: false})).toBe(true) - expect(isExcluded('bang.ts', {directory: false})).toBe(false) - expect(isExcluded('#hash.ts', {directory: false})).toBe(true) - }) +describe('path rules', () => { + const noGitFiltering = {gitFiltering: false, excludePatterns: [], workingDirectory: '/work'} - test('returns no rules for no patterns', () => { - expect(ignorePatternRules([])).toEqual([]) - }) + test('drops an entry named .git, a directory or a worktree file, at any depth, when Git filtering is on', () => { + const rules = {gitFiltering: true, excludePatterns: [], workingDirectory: '/work'} - test('rejects a value the flag layer should already have refused as a bug', () => { - expect(() => ignorePatternRules(['!'])).toThrow(BugError) - expect(() => ignorePatternRules(['!'])).toThrow(/nothing after/) - expect(() => ignorePatternRules([''])).toThrow(/empty/) + expect(isDroppedEntry(rules, undefined, {absolutePath: '/work/.git', isDirectory: true})).toBe(true) + expect(isDroppedEntry(rules, undefined, {absolutePath: '/work/packages/a/.git', isDirectory: false})).toBe(true) + expect(isDroppedEntry(rules, undefined, {absolutePath: '/work/.github', isDirectory: true})).toBe(false) + expect(isDroppedEntry(noGitFiltering, undefined, {absolutePath: '/work/.git', isDirectory: true})).toBe(false) }) -}) -describe('ignorePatternProblem', () => { - test('accepts ordinary .gitignore lines', () => { - for (const value of [ - 'generated/', - '!build/', - '*.log', - '/docs', - '\\#hash.ts', - '\\!bang.ts', - 'a b/', - '!.env', - 'build\\\\', - 'build\\\\\\\\', - 'trailing\\ ', - 'app/[id]/x.ts', - ]) { - expect(ignorePatternProblem(value), value).toBeUndefined() - } - }) + test("drops what the entry's own repository lists as ignored, keyed by the repository directory", () => { + const rules = {gitFiltering: true, excludePatterns: [], workingDirectory: '/work'} + const outer = repositoryIgnoredPaths('/work', {status: 'listed', paths: ['dist/', 'notes.txt']}) + const inner = repositoryIgnoredPaths('/work/inner', {status: 'listed', paths: ['build/']}) - test('rejects empty and whitespace-only values', () => { - expect(ignorePatternProblem('')).toMatch(/empty/) - expect(ignorePatternProblem(' ')).toMatch(/empty/) + expect(isDroppedEntry(rules, outer, {absolutePath: '/work/dist', isDirectory: true})).toBe(true) + expect(isDroppedEntry(rules, outer, {absolutePath: '/work/notes.txt', isDirectory: false})).toBe(true) + expect(isDroppedEntry(rules, outer, {absolutePath: '/work/src/notes.txt', isDirectory: false})).toBe(false) + // A directory entry never matches a file of the same name. + expect(isDroppedEntry(rules, outer, {absolutePath: '/work/dist', isDirectory: false})).toBe(false) + expect(isDroppedEntry(rules, inner, {absolutePath: '/work/inner/build', isDirectory: true})).toBe(true) + expect(isDroppedEntry(rules, inner, {absolutePath: '/work/inner/dist', isDirectory: true})).toBe(false) }) - test('rejects a .gitignore comment and suggests escaping the #', () => { - const problem = ignorePatternProblem('#hash.ts') - expect(problem).toMatch(/comment/) - expect(problem).toContain('\\#') - }) + test('applies no repository rules when Git filtering is off or the repository has no listing', () => { + const outer = repositoryIgnoredPaths('/work', {status: 'listed', paths: ['dist/']}) - test('rejects a `!` with nothing to re-include', () => { - expect(ignorePatternProblem('!')).toMatch(/nothing after/) - expect(ignorePatternProblem('! ')).toMatch(/nothing after/) + expect(isDroppedEntry(noGitFiltering, outer, {absolutePath: '/work/dist', isDirectory: true})).toBe(false) + expect(repositoryIgnoredPaths('/work', {status: 'failed'})).toBeUndefined() + expect(repositoryIgnoredPaths('/work', {status: 'not-a-repository'})).toBeUndefined() }) - test('rejects a trailing unescaped backslash, which `ignore` would silently drop or fail to compile', () => { - for (const value of ['build\\', 'src\\lib\\', '!build\\', '\\', 'build\\\\\\', '!build\\\\\\\\\\', '\\\\\\']) { - const problem = ignorePatternProblem(value) - expect(problem, value).toMatch(/ends with a backslash/) - expect(problem, value).toContain('/') - expect(problem, value).toContain('\\\\') + describe('--exclude', () => { + function rulesFor(workingDirectory: string, excludePatterns: string[], gitFiltering = true) { + vi.stubEnv('INIT_CWD', workingDirectory) + return createPathRules({excludePatterns, noGitIgnore: !gitFiltering}) } - }) - test('rejects values that span more than one line', () => { - for (const value of ['build/\ngenerated/', 'build/\r\n', 'build/\r', '\nbuild/']) { - expect(ignorePatternProblem(value), JSON.stringify(value)).toMatch(/single line/) - } - }) + test('matches a bare name only at the top of the working directory', () => { + const working = makeDirectory() + const rules = rulesFor(working, ['generated']) - test('rejects patterns with `..` as a whole path segment', () => { - for (const value of ['..', '../x', 'x/..', 'a/../b', '**/../x', '!../shared/']) { - expect(ignorePatternProblem(value), value).toBe( - `The --ignore pattern "${value}" contains "..". Patterns are relative to the app directory and can't point outside it.`, + expect(isDroppedEntry(rules, undefined, {absolutePath: join(working, 'generated'), isDirectory: true})).toBe(true) + expect(isDroppedEntry(rules, undefined, {absolutePath: join(working, 'src/generated'), isDirectory: true})).toBe( + false, ) - } - }) + }) - test('rejects patterns the `ignore` matcher cannot compile, instead of crashing the scan', () => { - for (const value of ['src/[id/x.ts', '![/', 'a\\\\[b', 'a\\\\(b']) { - expect(ignorePatternProblem(value), value).toBe( - `The --ignore pattern "${value}" can't be read as a .gitignore pattern. Check for an unclosed "[" or a backslash before a special character.`, - ) - expect(() => ignorePatternRules([value]), value).toThrow(BugError) - } - }) + test('matches a name at any depth with **/', () => { + const working = makeDirectory() + const rules = rulesFor(working, ['**/generated']) + + expect(isDroppedEntry(rules, undefined, {absolutePath: join(working, 'generated'), isDirectory: true})).toBe(true) + expect( + isDroppedEntry(rules, undefined, {absolutePath: join(working, 'src/a/generated'), isDirectory: true}), + ).toBe(true) + expect( + isDroppedEntry(rules, undefined, {absolutePath: join(working, 'src/generated.ts'), isDirectory: false}), + ).toBe(false) + }) - test('allows patterns where dots are part of a path segment', () => { - for (const value of ['..cache/', 'a..b', '...', 'x/..y']) { - expect(ignorePatternProblem(value), value).toBeUndefined() - } - }) -}) + test('matches a path outside the working directory with ../', () => { + const parent = makeDirectory() + const working = join(parent, 'app') + mkdirSync(working) + const rules = rulesFor(working, ['../backend/**']) + + expect( + isDroppedEntry(rules, undefined, {absolutePath: join(parent, 'backend/src/a.ts'), isDirectory: false}), + ).toBe(true) + expect( + isDroppedEntry(rules, undefined, {absolutePath: join(parent, 'frontend/src/a.ts'), isDirectory: false}), + ).toBe(false) + }) + + test('applies with --no-git-ignore too', () => { + const working = makeDirectory() + const rules = rulesFor(working, ['generated'], false) + + expect(rules.gitFiltering).toBe(false) + expect(isDroppedEntry(rules, undefined, {absolutePath: join(working, 'generated'), isDirectory: true})).toBe(true) + }) -describe('buildPathRules', () => { - test('keeps the defaults, the git literals and the overrides as separate phases, in input order', () => { - const overrides = [cliExclude('generated/'), cliInclude('build/')] + test('is relative to the real path of the working directory', () => { + const real = makeDirectory() + const link = join(makeDirectory(), 'link') + symlinkSync(real, link, 'dir') + const rules = rulesFor(link, ['generated']) - expect(buildPathRules({gitIgnoredPaths: ['notes.txt', 'tmp/'], overrides})).toEqual({ - defaults: DEFAULT_EXCLUDE_PATTERNS, - gitIgnoredPaths: ['notes.txt', 'tmp/'], - overrides, + expect(rules.workingDirectory).toBe(real) + expect(isDroppedEntry(rules, undefined, {absolutePath: join(real, 'generated'), isDirectory: true})).toBe(true) }) }) - test('has no git literals and no overrides when neither was supplied', () => { - const expected = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: [], overrides: []} - expect(buildPathRules({gitIgnoredPaths: []})).toEqual(expected) - expect(buildPathRules({gitIgnoredPaths: [], overrides: []})).toEqual(expected) + describe('tracked paths', () => { + test('are dropped by .git and --exclude at any depth of the path, never by repository ignore rules', () => { + const working = makeDirectory() + vi.stubEnv('INIT_CWD', working) + const rules = createPathRules({excludePatterns: ['generated'], noGitIgnore: false}) + + expect(isDroppedTrackedPath(rules, working, 'generated/a.ts')).toBe(true) + expect(isDroppedTrackedPath(rules, working, 'vendor/.git/config')).toBe(true) + expect(isDroppedTrackedPath(rules, working, 'dist/a.js')).toBe(false) + }) }) }) diff --git a/packages/app/src/cli/services/app-security-engine/tests/scan-contract.test.ts b/packages/app/src/cli/services/app-security-engine/tests/scan-contract.test.ts index b8de9b58a09..08499891ff5 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/scan-contract.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/scan-contract.test.ts @@ -398,9 +398,6 @@ redirect_urls = ["http://app.example/callback"] const directory = await app({ 'shopify.app.toml': appConfig('write_script_tags'), 'app/Main.java': 'x'.repeat(500_001), - 'node_modules/vendor/index.java': 'ignored', - 'tests/example.java': 'ignored', - 'fixtures/example.java': 'ignored', }) const result = await scan(directory) 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 f2370a28f90..4bb8e0688a1 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 @@ -6,7 +6,11 @@ import type {ScanInput, ScanOptions} from '../types.js' /** Resolves a test app the way the CLI would: the named configuration file in the directory is the selected TOML. */ function scanInputFor(appDirectory: string, configName?: string): ScanInput { - return {appDirectory, appConfigFilePath: joinPath(appDirectory, getAppConfigurationFileName(configName))} + return { + appDirectory, + scanDirectories: [appDirectory], + appConfigFilePath: joinPath(appDirectory, getAppConfigurationFileName(configName)), + } } export function scanDirectory(appDirectory: string, configName?: string, options?: ScanOptions) { 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 906b637c0f6..2a0d622ca64 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 @@ -267,7 +267,7 @@ describe('git status drives severity, not .gitignore text', () => { rmSync(dir, {recursive: true, force: true}) }) - test('reports a secret ignored only by an enclosing repository that does not own the app', async () => { + test('does not scan an untracked secret in an app directory that its repository ignores', async () => { const repository = removeAfterTest(mkdtempSync(join(tmpdir(), 'app-security-enclosing-'))) git(repository, ['init', '-q', '.']) writeFileSync(join(repository, '.gitignore'), 'apps/\n') @@ -277,19 +277,49 @@ describe('git status drives severity, not .gitignore text', () => { writeFileSync(join(dir, '.env'), trackedEnvSecret()) const result = await scan(dir) - const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') - expect(finding).toMatchObject({ - severity: 'high', - points: -50, - title: 'Environment file with secrets is ignored by a repository that does not own this app', - pattern_id: 'environment-file:unconfirmed', - rule_version: 3, + expect(result.issues.filter((i) => i.id === 'COMMITTED_SECRET')).toEqual([]) + expect(result.ignoredScanDirectories).toEqual([dir]) + }) + + test('does not report an untracked secret that git ignores when --no-git-ignore scans it', async () => { + const repository = removeAfterTest(mkdtempSync(join(tmpdir(), 'app-security-enclosing-'))) + git(repository, ['init', '-q', '.']) + writeFileSync(join(repository, '.gitignore'), 'apps/\n') + const dir = join(repository, 'apps', 'web') + mkdirSync(dir, {recursive: true}) + writeFileSync(join(dir, 'shopify.app.toml'), TOML) + writeFileSync(join(dir, '.env'), trackedEnvSecret()) + + const result = await scan(dir, undefined, {noGitIgnore: true}) + expect(result.issues.filter((i) => i.id === 'COMMITTED_SECRET')).toEqual([]) + expect(result.ignoredScanDirectories).toEqual([]) + }) + + test('with --no-git-ignore, skips an untracked ignored .env and reports the same .env once force-added', async () => { + const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) + git(dir, ['init', '-q', '.']) + + const untracked = await scan(dir, undefined, {noGitIgnore: true}) + expect(untracked.issues.filter((i) => i.id === 'COMMITTED_SECRET')).toEqual([]) + + git(dir, ['add', '-f', '.env']) + const tracked = await scan(dir, undefined, {noGitIgnore: true}) + expect(tracked.issues.find((i) => i.id === 'COMMITTED_SECRET')).toMatchObject({ + location: {file: '.env'}, + title: 'Environment file with secrets is tracked by git', + pattern_id: 'environment-file:tracked', + }) + }) + + test('with --no-git-ignore, still reports an untracked .env that git does not ignore', async () => { + const dir = removeAfterTest(makeApp({'.env': trackedEnvSecret()})) + git(dir, ['init', '-q', '.']) + + const result = await scan(dir, undefined, {noGitIgnore: true}) + expect(result.issues.find((i) => i.id === 'COMMITTED_SECRET')).toMatchObject({ + location: {file: '.env'}, + title: 'Environment file with secrets is not ignored by git', }) - const execution = result.scan.checks_executed.find((candidate) => candidate.id === 'COMMITTED_SECRET') - expect(execution?.version).toBe(3) - expect(finding!.detection_evidence?.join(' ')).toContain('→ ignored') - expect(finding!.message).toContain('.env is ignored by an enclosing git repository') - expect(finding!.message).not.toContain('could not be confirmed') }) test('reports a secret inside a nested repository that the app repository ignores by name', async () => { @@ -586,12 +616,8 @@ describe('secret evidence coverage', () => { rmSync(dir, {recursive: true, force: true}) }) - test('excludes test, fixture, dependency, build, and binary content', async () => { + test('excludes binary content', async () => { const dir = makeApp({ - 'tests/example.md': PROBES.shopifyToken, - 'fixtures/example.yaml': PROBES.shopifyToken, - 'node_modules/package/example.json': PROBES.shopifyToken, - 'dist/example.toml': PROBES.shopifyToken, 'binary.json': `\0${PROBES.shopifyToken}`, }) const result = await scan(dir) 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 a4fac307aca..198bcf12ef5 100644 --- a/packages/app/src/cli/services/app-security-engine/types.ts +++ b/packages/app/src/cli/services/app-security-engine/types.ts @@ -76,13 +76,18 @@ 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. */ + scanDirectories: ReadonlyArray /** Absolute path of the selected app configuration file. Absent when scanning without app configuration. */ appConfigFilePath?: string clientId?: string } export interface ScanOptions { - ignorePatterns?: ReadonlyArray + /** `--exclude` globs, as typed. */ + excludePatterns?: ReadonlyArray + /** Turns off Git ignore rules for every scan directory. */ + noGitIgnore?: boolean } export interface ScanResult { @@ -98,6 +103,12 @@ export interface ScanResult { issues: Issue[] } +/** 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. */ + ignoredScanDirectories: string[] +} + export interface SkippedFile { path: string reason: 'too_large' | 'unreadable' diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 9fcfd59416c..71e8dd760e6 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -69,6 +69,7 @@ const agentChecks: AgentChecks = { const scanExecution: AppSecurityExecution = { scan, + ignoredScanDirectories: [], deterministicFindings, agentChecks, engine, @@ -104,6 +105,7 @@ function testDependencies( deliverInstructions: vi.fn(async () => {}), output: vi.fn(), renderInfo: vi.fn(), + renderWarning: vi.fn(), renderReport: vi.fn(), setExitCode: vi.fn(), } @@ -118,7 +120,8 @@ function testOptions() { blocking: 'none' as const, yes: false, skipInstructions: false, - ignorePatterns: [], + excludePatterns: [], + noGitIgnore: false, } } @@ -137,9 +140,11 @@ describe('securityCheck', () => { }) expect(dependencies.execute).toHaveBeenCalledWith({ appDirectory, + scanDirectories: [appDirectory], appConfigFilePath: `${appDirectory}/shopify.app.toml`, clientId: 'toml-client-id', - ignorePatterns: [], + excludePatterns: [], + noGitIgnore: false, }) expect(dependencies.writeArtifacts).toHaveBeenCalledWith(appDirectory, 'shopify.app', { deterministicFindings, @@ -186,21 +191,47 @@ describe('securityCheck', () => { expect(dependencies.execute).toHaveBeenCalledWith(expect.objectContaining({clientId: 'flag-client-id'})) }) - test('forwards ignorePatterns to the scan and repeats them in generated commands and instructions', async () => { + test('forwards the scope flags to the scan and repeats them in generated commands and instructions', async () => { const dependencies = testDependencies() dependencies.canPrompt.mockReturnValue(true) dependencies.selectInstructionsDestination.mockResolvedValue('print') - const ignorePatterns = ['generated/', '!build/'] + const excludePatterns = ['generated', '../shared/**'] - await securityCheck({...testOptions(), ignorePatterns}, dependencies) + await securityCheck({...testOptions(), excludePatterns, noGitIgnore: true}, dependencies) - const commands = resolveAppSecurityCommands(appDirectory, 'shopify.app.toml', ignorePatterns) - expect(commands.scan.args).toContainEqual({flag: '--ignore', value: 'generated/'}) - expect(dependencies.execute).toHaveBeenCalledWith(expect.objectContaining({ignorePatterns})) + const commands = resolveAppSecurityCommands(appDirectory, 'shopify.app.toml', excludePatterns, true) + expect(commands.scan.args).toContainEqual({flag: '--exclude', value: 'generated'}) + expect(commands.scan.args).toContain('--no-git-ignore') + expect(dependencies.execute).toHaveBeenCalledWith(expect.objectContaining({excludePatterns, noGitIgnore: true})) expect(dependencies.renderReport).toHaveBeenCalledWith(expect.objectContaining({commands})) expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) }) + 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 securityCheck(testOptions(), dependencies) + + expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { + headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { + headline: '. is ignored by Git, so only the files Git tracks in it are scanned.', + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + }) + + test('does not warn when no scan directory is ignored', async () => { + const dependencies = testDependencies() + + await securityCheck(testOptions(), dependencies) + + expect(dependencies.renderWarning).not.toHaveBeenCalled() + }) + test('re-scanning overwrites the check artifacts without prompting and leaves agent findings untouched', async () => { await inTemporaryDirectory(async (appRoot) => { const paths = appSecurityArtifactPaths(appRoot, 'shopify.app') @@ -271,9 +302,11 @@ describe('securityCheck', () => { ) expect(dependencies.execute).toHaveBeenCalledWith({ appDirectory, + scanDirectories: [appDirectory], appConfigFilePath: undefined, clientId: 'flag-client-id', - ignorePatterns: [], + excludePatterns: [], + noGitIgnore: false, }) expect(dependencies.writeArtifacts).toHaveBeenCalledWith(appDirectory, 'flag-client-id', expect.anything()) expect(dependencies.renderInfo).not.toHaveBeenCalled() diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 89d0b5252c9..aef45489d13 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -18,9 +18,10 @@ import {encodeSecurityJson, toSecurityJson} from './security-json.js' import {renderSecurityReport} from './security-output.js' import {outputResult} from '@shopify/cli-kit/node/output' import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' -import {renderInfo, renderSelectPrompt} from '@shopify/cli-kit/node/ui' +import {cwd, relativePath} from '@shopify/cli-kit/node/path' +import {renderInfo, renderSelectPrompt, renderWarning} from '@shopify/cli-kit/node/ui' import type {CheckArtifactPaths} from './app-security-artifacts.js' -import type {AgentChecks, DeterministicFindingsDocument, ScanInput} from './app-security-engine/index.js' +import type {AgentChecks, DeterministicFindingsDocument, ScanInput, ScanOptions} from './app-security-engine/index.js' import type {AppSecurityBlockingLevel, AppSecurityExecution} from './app-security-api.js' import type {SecurityReportInput} from './security-output.js' import type {RenderAlertOptions, RenderSelectPromptOptions} from '@shopify/cli-kit/node/ui' @@ -35,7 +36,8 @@ interface SecurityOptions { blocking: AppSecurityBlockingLevel yes: boolean skipInstructions: boolean - ignorePatterns: ReadonlyArray + excludePatterns: ReadonlyArray + noGitIgnore: boolean } export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' @@ -48,7 +50,7 @@ interface SecurityDependencies { withoutAppConfig: boolean allowPrompts: boolean }): Promise - execute(options: ScanInput & {ignorePatterns: ReadonlyArray}): Promise + execute(options: ScanInput & Required): Promise writeArtifacts( appDirectory: string, resultsKey: string, @@ -65,6 +67,7 @@ interface SecurityDependencies { }): Promise output(content: string): void renderInfo(options: RenderAlertOptions): void + renderWarning(options: RenderAlertOptions): void renderReport(input: SecurityReportInput): void setExitCode(exitCode: number): void } @@ -88,6 +91,7 @@ const defaultDependencies: SecurityDependencies = { deliverInstructions: deliverAppSecurityInstructions, output: outputResult, renderInfo, + renderWarning, renderReport: renderSecurityReport, setExitCode: (exitCode) => { process.exitCode = exitCode @@ -144,7 +148,12 @@ export default async function securityCheck( allowPrompts: canPrompt, }) const {appDirectory} = selection - const commands = resolveAppSecurityCommands(appDirectory, selectedConfigFileName(selection), options.ignorePatterns) + const commands = resolveAppSecurityCommands( + appDirectory, + selectedConfigFileName(selection), + options.excludePatterns, + options.noGitIgnore, + ) // The prompt is only shown when no TOML was found and `--without-app-config` wasn't passed. if (selection.kind === 'no-config' && !options.withoutAppConfig) { dependencies.renderInfo({ @@ -156,10 +165,18 @@ export default async function securityCheck( const execution = await dependencies.execute({ appDirectory, + scanDirectories: scanDirectories.map(({directory}) => directory), appConfigFilePath: selection.kind === 'config' ? selection.appConfigFilePath : undefined, clientId: effectiveClientId(selection), - ignorePatterns: options.ignorePatterns, + excludePatterns: options.excludePatterns, + noGitIgnore: options.noGitIgnore, }) + for (const directory of execution.ignoredScanDirectories) { + dependencies.renderWarning({ + headline: `${relativePath(cwd(), directory) || '.'} is ignored by Git, so only the files Git tracks in it are scanned.`, + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + } const artifacts = await dependencies.writeArtifacts(appDirectory, resultsKey(selection), { deterministicFindings: execution.deterministicFindings, agentChecks: execution.agentChecks, diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 3b621e73b8c..9d908586607 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -3994,8 +3994,8 @@ "args": { }, "customPluginName": "@shopify/app", - "description": "Runs Shopify App Security locally and writes `deterministic-findings.json` and `agent-checks.json` to the results directory, `.shopify/app-security//`. The results key is `--client-id` when you pass it, and otherwise the name of the app configuration file without `.toml`; the other `app security` commands take the same selection flags and find the same directory. Every run replaces both files, so it's always safe to run the check again.\n\n`deterministic-findings.json` holds the deterministic scan results. `agent-checks.json` holds the checks for your coding agent to investigate; the agent's results are recorded with `shopify app security record`. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration. Use `--client-id` to replace the configuration's client ID for this run. When no app configuration exists, use `--without-app-config --client-id ` to scan `--path` anyway with config checks skipped; in an interactive terminal the command offers to do this.\n\nUse `--ignore` to change which files are scanned. Each value is one `.gitignore` pattern relative to the app directory; prefix it with `!` to include a file again when it is ignored by default or by `.gitignore`. Repeat the flag to add patterns; later patterns take precedence. A file can't be included again while its parent folder is ignored, so include the folder again instead, for example `--ignore '!build/'`. Quote each value so your shell doesn't expand `!` or `*` (single quotes in POSIX shells and PowerShell). The coding-agent instructions this check offers repeat the patterns. Other `app security` commands don't take `--ignore`, so pass the same patterns each time you run the check.\n\nIn interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass `--yes`, which prints them. JSON output never prompts or prints those instructions. You can also run `shopify app security instructions` to print, copy, or write them later.", - "descriptionWithMarkdown": "Runs Shopify App Security locally and writes `deterministic-findings.json` and `agent-checks.json` to the results directory, `.shopify/app-security//`. The results key is `--client-id` when you pass it, and otherwise the name of the app configuration file without `.toml`; the other `app security` commands take the same selection flags and find the same directory. Every run replaces both files, so it's always safe to run the check again.\n\n`deterministic-findings.json` holds the deterministic scan results. `agent-checks.json` holds the checks for your coding agent to investigate; the agent's results are recorded with `shopify app security record`. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration. Use `--client-id` to replace the configuration's client ID for this run. When no app configuration exists, use `--without-app-config --client-id ` to scan `--path` anyway with config checks skipped; in an interactive terminal the command offers to do this.\n\nUse `--ignore` to change which files are scanned. Each value is one `.gitignore` pattern relative to the app directory; prefix it with `!` to include a file again when it is ignored by default or by `.gitignore`. Repeat the flag to add patterns; later patterns take precedence. A file can't be included again while its parent folder is ignored, so include the folder again instead, for example `--ignore '!build/'`. Quote each value so your shell doesn't expand `!` or `*` (single quotes in POSIX shells and PowerShell). The coding-agent instructions this check offers repeat the patterns. Other `app security` commands don't take `--ignore`, so pass the same patterns each time you run the check.\n\nIn interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass `--yes`, which prints them. JSON output never prompts or prints those instructions. You can also run `shopify app security instructions` to print, copy, or write them later.", + "description": "Runs Shopify App Security locally and writes `deterministic-findings.json` and `agent-checks.json` to the results directory, `.shopify/app-security//`. The results key is `--client-id` when you pass it, and otherwise the name of the app configuration file without `.toml`; the other `app security` commands take the same selection flags and find the same directory. Every run replaces both files, so it's always safe to run the check again.\n\n`deterministic-findings.json` holds the deterministic scan results. `agent-checks.json` holds the checks for your coding agent to investigate; the agent's results are recorded with `shopify app security record`. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration. Use `--client-id` to replace the configuration's client ID for this run. When no app configuration exists, use `--without-app-config --client-id ` to scan `--path` anyway with config checks skipped; in an interactive terminal the command offers to do this.\n\nThe check scans the app directory and each `--include-dir`. Git ignore rules apply by default: a file or directory that Git ignores is skipped, using the rules of the repository that contains it, while files that Git tracks are always scanned. Use `--no-git-ignore` to turn Git ignore rules off for every scanned directory.\n\nUse `--exclude` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with `../`, and a name at any depth needs `**/`, for example `--exclude '**/generated'`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand `*`. The coding-agent instructions this check offers repeat the globs. Other `app security` commands don't take `--exclude` or `--no-git-ignore`, so pass the same flags each time you run the check.\n\nIn interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass `--yes`, which prints them. JSON output never prompts or prints those instructions. You can also run `shopify app security instructions` to print, copy, or write them later.", + "descriptionWithMarkdown": "Runs Shopify App Security locally and writes `deterministic-findings.json` and `agent-checks.json` to the results directory, `.shopify/app-security//`. The results key is `--client-id` when you pass it, and otherwise the name of the app configuration file without `.toml`; the other `app security` commands take the same selection flags and find the same directory. Every run replaces both files, so it's always safe to run the check again.\n\n`deterministic-findings.json` holds the deterministic scan results. `agent-checks.json` holds the checks for your coding agent to investigate; the agent's results are recorded with `shopify app security record`. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration. Use `--client-id` to replace the configuration's client ID for this run. When no app configuration exists, use `--without-app-config --client-id ` to scan `--path` anyway with config checks skipped; in an interactive terminal the command offers to do this.\n\nThe check scans the app directory and each `--include-dir`. Git ignore rules apply by default: a file or directory that Git ignores is skipped, using the rules of the repository that contains it, while files that Git tracks are always scanned. Use `--no-git-ignore` to turn Git ignore rules off for every scanned directory.\n\nUse `--exclude` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with `../`, and a name at any depth needs `**/`, for example `--exclude '**/generated'`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand `*`. The coding-agent instructions this check offers repeat the globs. Other `app security` commands don't take `--exclude` or `--no-git-ignore`, so pass the same flags each time you run the check.\n\nIn interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass `--yes`, which prints them. JSON output never prompts or prints those instructions. You can also run `shopify app security instructions` to print, copy, or write them later.", "enableJsonFlag": false, "flags": { "blocking": { @@ -4035,11 +4035,11 @@ "name": "config", "type": "option" }, - "ignore": { - "description": "Ignore files that match this .gitignore pattern, relative to the app directory. Start the pattern with ! to include matching files again. Repeat the flag to add patterns; later patterns take precedence.", + "exclude": { + "description": "Skip paths that match this glob, relative to the working directory. Repeat the flag to add globs. The selected app configuration file can't be excluded.", "hasDynamicHelp": false, "multiple": true, - "name": "ignore", + "name": "exclude", "type": "option" }, "json": { @@ -4066,6 +4066,13 @@ "name": "no-color", "type": "boolean" }, + "no-git-ignore": { + "allowNo": false, + "description": "Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.", + "env": "SHOPIFY_FLAG_NO_GIT_IGNORE", + "name": "no-git-ignore", + "type": "boolean" + }, "no-input": { "allowNo": false, "description": "Disable interactive prompts and browser authentication.",