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 bb62c6f8bdd..822c3acdc9d 100644 --- a/packages/app/src/cli/commands/app/security/check.test.ts +++ b/packages/app/src/cli/commands/app/security/check.test.ts @@ -4,6 +4,7 @@ import securityCheck from '../../../services/security-check.js' import AppLinkedCommand from '../../../utilities/app-linked-command.js' import BaseCommand from '@shopify/cli-kit/node/base-command' import {resolvePath} from '@shopify/cli-kit/node/path' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {describe, expect, test, vi} from 'vitest' vi.mock('../../../services/security-check.js') @@ -34,9 +35,45 @@ describe('app security check command', () => { skipInstructions: true, findingsPath: undefined, clean: false, + ignorePatterns: [], }) }) + test('forwards repeated --ignore patterns in command-line order', async () => { + await SecurityCheck.run( + ['--ignore', 'generated/', '--ignore', '!build/', '--ignore', 'a b/', '--skip-instructions'], + import.meta.url, + ) + + expect(securityCheck).toHaveBeenCalledWith( + expect.objectContaining({ignorePatterns: ['generated/', '!build/', '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(() => {}) + + 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() + } + }) + test('forwards --yes without requiring an app configuration', async () => { await SecurityCheck.run(['--path', '/tmp/directory-without-shopify-toml', '--yes'], import.meta.url) @@ -50,6 +87,7 @@ describe('app security check command', () => { skipInstructions: false, findingsPath: undefined, clean: false, + ignorePatterns: [], }) }) @@ -108,6 +146,19 @@ describe('app security check command', () => { expect(SecurityCheck.descriptionWithMarkdown).toContain('Pass `--clean` to discard that work and start over') }) + test('documents --ignore as ordered .gitignore patterns that follow-up commands 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.', + ) + 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('`--findings`') + }) + test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => { await SecurityCheck.run(['--json', '--yes'], import.meta.url) diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 5bcfaf0313c..a7204dfcea8 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -1,8 +1,10 @@ import {appFlags} from '../../../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' import {resolvePath} from '@shopify/cli-kit/node/path' import type {AppSecurityBlockingLevel} from '../../../services/app-security-api.js' @@ -17,6 +19,8 @@ export default class SecurityCheck extends BaseCommand { Pass \`--findings\` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass \`--clean\` to discard that work and start over. Use \`--config\` to select a specific app configuration when the project has multiple \`shopify.app*.toml\` files; App Security inspects only that configuration. +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 generated follow-up commands repeat the patterns, and \`--findings\` must use the same patterns as the scan it validates. + 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.` static description = this.descriptionWithoutMarkdown() @@ -25,6 +29,18 @@ In interactive terminals, the command offers to copy the coding-agent instructio ...globalFlags, path: appFlags.path, config: appFlags.config, + // 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({ + 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.', + multiple: true, + parse: async (input) => { + const problem = ignorePatternProblem(input) + if (problem) throw new AbortError(problem) + return input + }, + }), ...jsonFlag, findings: Flags.string({ description: 'Validate agent findings from a JSON file and compile them into the trace.', @@ -73,6 +89,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio skipInstructions: flags['skip-instructions'], findingsPath: flags.findings, clean: flags.clean, + ignorePatterns: flags.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 3f1f62bc11c..c112753e30d 100644 --- a/packages/app/src/cli/services/app-security-api.test.ts +++ b/packages/app/src/cli/services/app-security-api.test.ts @@ -319,6 +319,43 @@ describe('App Security CLI integration', () => { }) }) + test('compiles findings when the compile repeats the scan --ignore patterns', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + await mkdir(joinPath(directory, 'generated')) + await writeFile(joinPath(directory, 'generated', 'client.ts'), 'export const generated = true\n') + const appRoot = resolveAppSecurityRoot(directory) + const ignorePatterns = ['generated/'] + + const initial = await executeAppSecurity({appRoot, ignorePatterns}) + expect(Object.keys(initial.scan.scan.file_hashes ?? {})).not.toContain('generated/client.ts') + const findings = {schema_version: 1 as const, source_scan_id: initial.scan.scan.input_hash, findings: []} + + const compiled = await executeAppSecurity({appRoot, findings, ignorePatterns}) + expect(compiled.operation).toBe('compile') + expect(compiled.operation === 'compile' && compiled.findings.rejected).toEqual([]) + }) + }) + + test('rejects findings when the compile uses different --ignore patterns than the scan', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + await mkdir(joinPath(directory, 'generated')) + await writeFile(joinPath(directory, 'generated', 'client.ts'), 'export const generated = true\n') + const appRoot = resolveAppSecurityRoot(directory) + + const initial = await executeAppSecurity({appRoot, ignorePatterns: ['generated/']}) + const findings = {schema_version: 1 as const, source_scan_id: initial.scan.scan.input_hash, findings: []} + + const compiled = await executeAppSecurity({appRoot, findings}) + expect(compiled.operation === 'compile' && compiled.findings.rejected).toEqual([ + expect.stringMatching( + /^Findings source scan \S+ does not match the current scan \S+; compile with the same --ignore and --config values used for the scan, since ignore patterns are not recorded in the trace\.$/, + ), + ]) + }) + }) + test('does not apply a stale suppression whose fingerprint still matches a current finding', async () => { await inTemporaryDirectory(async (directory) => { const testToken = ['shpat', '0123456789abcdef0123456789abcdef'].join('_') @@ -613,6 +650,7 @@ describe('App Security CLI integration', () => { yes: false, skipInstructions: true, clean: false, + ignorePatterns: [], }, { resolveRoot: resolveAppSecurityRoot, diff --git a/packages/app/src/cli/services/app-security-api.ts b/packages/app/src/cli/services/app-security-api.ts index 0878cdfd3b8..74d0ff045a2 100644 --- a/packages/app/src/cli/services/app-security-api.ts +++ b/packages/app/src/cli/services/app-security-api.ts @@ -91,11 +91,13 @@ export async function executeAppSecurity(options: { appRoot: string findings?: FindingsDocument configFileName?: string + ignorePatterns?: ReadonlyArray }): Promise { const startTime = Date.now() + const scanOptions = {ignorePatterns: options.ignorePatterns} const result = options.findings - ? await compileFindings(options.appRoot, options.findings, options.configFileName) - : await scanApp(options.appRoot, options.configFileName) + ? await compileFindings(options.appRoot, options.findings, options.configFileName, scanOptions) + : await scanApp(options.appRoot, options.configFileName, scanOptions) 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 0b82f009c31..9bd867c117a 100644 --- a/packages/app/src/cli/services/app-security-commands.test.ts +++ b/packages/app/src/cli/services/app-security-commands.test.ts @@ -138,13 +138,17 @@ describe('quoteShellArgument', () => { describe('resolveAppSecurityCommands', () => { test('omits --config for the default shopify.app.toml', () => { - expect(resolveAppSecurityCommands('/tmp/app').scan.args).toEqual(['app', 'security', 'check', '--path', '/tmp/app']) + expect(resolveAppSecurityCommands('/tmp/app').scan.args).toEqual([ + 'app', + 'security', + 'check', + {flag: '--path', value: '/tmp/app'}, + ]) expect(resolveAppSecurityCommands('/tmp/app', 'shopify.app.toml').scan.args).toEqual([ 'app', 'security', 'check', - '--path', - '/tmp/app', + {flag: '--path', value: '/tmp/app'}, ]) }) @@ -152,22 +156,133 @@ describe('resolveAppSecurityCommands', () => { const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml') const findingsPath = joinPath('/tmp/app', '.shopify', 'app-security', 'findings.json') - expect(commands.scan.args).toEqual(['app', 'security', 'check', '--path', '/tmp/app', '--config', 'staging']) + expect(commands.scan.args).toEqual([ + 'app', + 'security', + 'check', + {flag: '--path', value: '/tmp/app'}, + {flag: '--config', value: 'staging'}, + ]) expect(commands.compile.args).toEqual([ 'app', 'security', 'check', - '--path', - '/tmp/app', - '--config', - 'staging', - '--findings', - findingsPath, + {flag: '--path', value: '/tmp/app'}, + {flag: '--config', value: 'staging'}, + {flag: '--findings', value: findingsPath}, + ]) + }) + + test('repeats --ignore patterns in order, after --config, on scan, compile, and clean', () => { + const commands = resolveAppSecurityCommands('/tmp/app', 'shopify.app.staging.toml', ['generated/', '!build/']) + const findingsPath = joinPath('/tmp/app', '.shopify', 'app-security', 'findings.json') + const scanArgs = [ + 'app', + 'security', + 'check', + {flag: '--path', value: '/tmp/app'}, + {flag: '--config', value: 'staging'}, + {flag: '--ignore', value: 'generated/'}, + {flag: '--ignore', value: '!build/'}, + ] + + expect(commands.scan.args).toEqual(scanArgs) + expect(commands.compile.args).toEqual([...scanArgs, {flag: '--findings', value: findingsPath}]) + expect(commands.clean.args).toEqual([...scanArgs, '--clean']) + }) + + test('omits --ignore when there are no patterns', () => { + expect(resolveAppSecurityCommands('/tmp/app', undefined, []).scan.args).toEqual([ + 'app', + 'security', + 'check', + {flag: '--path', value: '/tmp/app'}, ]) }) }) 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) + + for (const shell of ['posix', 'cmd', 'powershell'] as const) { + const formatted = formatAppSecurityCommand(commands.scan, shell) + expect(splitQuotedCommand(formatted, shell)).toEqual([ + 'shopify', + 'app', + 'security', + 'check', + '--path', + '/tmp/app', + '--ignore', + '!build/', + '--ignore', + '*.log', + '--ignore', + '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/'", + ) + expect(formatAppSecurityCommand(commands.scan, 'powershell')).toContain( + "--ignore '!build/' --ignore '*.log' --ignore 'a b/'", + ) + expect(formatAppSecurityCommand(commands.scan, 'cmd')).toContain( + '--ignore "!build/" --ignore "*.log" --ignore "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) + + for (const shell of ['posix', 'cmd', 'powershell'] as const) { + const formatted = formatAppSecurityCommand(commands.scan, shell) + expect(splitQuotedCommand(formatted, shell)).toEqual([ + 'shopify', + 'app', + 'security', + 'check', + '--path', + '/tmp/app', + '--ignore', + '-*.log', + '--ignore', + '-tmp/', + '--ignore', + 'check', + ]) + expect(formatted).not.toMatch(/ -\*\.log/) + expect(formatted).not.toMatch(/ -tmp\//) + expect(formatted).not.toMatch(/--ignore check/) + } + expect(formatAppSecurityCommand(commands.scan, 'posix')).toContain( + "--ignore '-*.log' --ignore '-tmp/' --ignore 'check'", + ) + expect(formatAppSecurityCommand(commands.scan, 'powershell')).toContain( + "--ignore '-*.log' --ignore '-tmp/' --ignore 'check'", + ) + expect(formatAppSecurityCommand(commands.scan, 'cmd')).toContain( + '--ignore "-*.log" --ignore "-tmp/" --ignore "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 findingsPath = joinPath('/tmp/app', '.shopify', 'app-security', 'findings.json') + + expect(formatAppSecurityCommand(commands.compile, 'posix')).toBe( + `shopify app security check --path '/tmp/app' --config 'staging' --ignore 'generated/' --findings '${findingsPath}'`, + ) + expect(formatAppSecurityCommand(commands.clean, 'posix')).toBe( + "shopify app security check --path '/tmp/app' --config 'staging' --ignore 'generated/' --clean", + ) + }) + test('quotes a Windows path with spaces and percents for terminal and instruction shells', () => { const commands = resolveAppSecurityCommands(WINDOWS_APP_ROOT) const findingsPath = joinPath(WINDOWS_APP_ROOT, '.shopify', 'app-security', 'findings.json') diff --git a/packages/app/src/cli/services/app-security-commands.ts b/packages/app/src/cli/services/app-security-commands.ts index f9cbf0a7332..2e5cc2d320f 100644 --- a/packages/app/src/cli/services/app-security-commands.ts +++ b/packages/app/src/cli/services/app-security-commands.ts @@ -3,9 +3,12 @@ import {getAppConfigurationShorthand} from '../models/app/config-file-naming.js' export type AppSecurityShell = 'posix' | 'cmd' | 'powershell' +/** Strings are command syntax, printed bare. Flag values are user input, so they're always quoted. */ +type AppSecurityArgument = string | {flag: string; value: string} + export interface AppSecurityCommand { command: string - args: string[] + args: AppSecurityArgument[] } export interface AppSecurityCommands { @@ -14,19 +17,31 @@ export interface AppSecurityCommands { clean: AppSecurityCommand } -export function resolveAppSecurityCommands(appRoot: string, configFileName?: string): AppSecurityCommands { +/** `ignorePatterns` are repeated on every command: a compile must discover the same files as its scan. */ +export function resolveAppSecurityCommands( + appRoot: string, + configFileName?: string, + ignorePatterns: ReadonlyArray = [], +): AppSecurityCommands { const {findingsPath} = appSecurityArtifactPaths(appRoot) const configFlag = configFileName ? getAppConfigurationShorthand(configFileName) : undefined const scan: AppSecurityCommand = { command: 'shopify', - args: ['app', 'security', 'check', '--path', appRoot, ...(configFlag ? ['--config', configFlag] : [])], + args: [ + 'app', + 'security', + 'check', + {flag: '--path', value: appRoot}, + ...(configFlag ? [{flag: '--config', value: configFlag}] : []), + ...ignorePatterns.map((ignorePattern) => ({flag: '--ignore', value: ignorePattern})), + ], } return { scan, compile: { command: scan.command, - args: [...scan.args, '--findings', findingsPath], + args: [...scan.args, {flag: '--findings', value: findingsPath}], }, clean: { command: scan.command, @@ -83,11 +98,8 @@ export function formatAppSecurityCommand( action: AppSecurityCommand, shell: AppSecurityShell = shellForPlatform(), ): string { - return [action.command, ...action.args] - .map((argument, index) => { - const isCommandSyntax = - index === 0 || argument === 'app' || argument === 'security' || argument === 'check' || argument.startsWith('-') - return isCommandSyntax ? argument : quoteShellArgument(argument, shell) - }) - .join(' ') + const words = action.args.map((argument) => + typeof argument === 'string' ? argument : `${argument.flag} ${quoteShellArgument(argument.value, shell)}`, + ) + return [action.command, ...words].join(' ') } 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 9c4a4ca9a97..3bc7ed3295b 100644 --- a/packages/app/src/cli/services/app-security-engine/index.ts +++ b/packages/app/src/cli/services/app-security-engine/index.ts @@ -3,8 +3,8 @@ * * CLI code outside this directory should import only these operations and result * types: locate an app, scan, parse/compile findings, parse a stored trace, - * build a submission, and group issues for display. Keep scanners, registries, merge helpers, and redaction - * inside the engine. + * build a submission, group issues for display, and validate `--ignore` + * patterns. Keep scanners, registries, merge helpers, and redaction inside the engine. */ export { AppRootDiscoveryError, @@ -24,6 +24,7 @@ export type { FindingsDocument, ParseTraceResult, } from './run.js' +export {ignorePatternProblem} from './scanners/path-rules.js' export {hasRecordedAgentReview} from './trace/index.js' export {buildSubmission, SUBMISSION_SCHEMA_VERSION} from './submission/index.js' export type {AppSecuritySubmission, AppSecuritySubmissionReport, BuildSubmissionOptions} from './submission/index.js' 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 7089c57b66b..b4e1699bc36 100644 --- a/packages/app/src/cli/services/app-security-engine/run.ts +++ b/packages/app/src/cli/services/app-security-engine/run.ts @@ -15,7 +15,7 @@ import {computeResultHash} from './scorer/index.js' import {compileTrace, validateTrace} from './trace/index.js' import {FINDINGS_SCHEMA_VERSION} from './types.js' import {getEngineVersion} from './version.js' -import type {CheckExecution, ScanResult, Suppression, TraceV3} from './types.js' +import type {CheckExecution, ScanOptions, ScanResult, Suppression, TraceV3} from './types.js' export {AppRootDiscoveryError, findAppRoot} @@ -109,9 +109,13 @@ export function parseFindings(value: unknown): FindingsDocument { return value as FindingsDocument } -export async function scanApp(directory?: string, configFileName?: string): Promise { +export async function scanApp( + directory?: string, + configFileName?: string, + options?: ScanOptions, +): Promise { const appRoot = findAppRoot(directory) - const result = await scan(appRoot, configFileName) + const result = await scan(appRoot, configFileName, options) const engineVersion = getEngineVersion() const reviewPack = buildReviewPack(engineVersion, result) const trace = compileTrace(result, {engineVersion, agentChecksExecuted: [], suppressions: []}) @@ -129,15 +133,20 @@ export async function compileFindings( directory: string, document: FindingsDocument, configFileName?: string, + options?: ScanOptions, ): Promise { const appRoot = findAppRoot(directory) - const result = await scan(appRoot, configFileName) + const result = await scan(appRoot, configFileName, options) const engineVersion = getEngineVersion() const knownFiles = new Set(searchBoundaryFiles(result)) + // Rejects findings when the current input hash differs from their source_scan_id. Ignore patterns + // aren't recorded in the trace, so the hint names them as a likely cause. const provenanceRejected = document.source_scan_id === result.scan.input_hash ? [] - : [`Findings source scan ${document.source_scan_id} does not match the current scan ${result.scan.input_hash}.`] + : [ + `Findings source scan ${document.source_scan_id} does not match the current scan ${result.scan.input_hash}; compile with the same --ignore and --config values used for the scan, since ignore patterns are not recorded in the trace.`, + ] const executed = provenanceRejected.length > 0 ? {executions: [] as CheckExecution[], rejected: provenanceRejected, warnings: [] as string[]} diff --git a/packages/app/src/cli/services/app-security-engine/scanners/index.ts b/packages/app/src/cli/services/app-security-engine/scanners/index.ts index ebfac923e78..c841e909702 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/index.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/index.ts @@ -12,7 +12,7 @@ import { findDependencyAutomationInputs, listRepositoryFiles, } from './discover.js' -import {buildPathRules, listGitIgnoredPaths} from './path-rules.js' +import {buildPathRules, hasIncludeOverride, ignorePatternRules, listGitIgnoredPaths} from './path-rules.js' import {detectCapabilities, detectProject} from '../capabilities/detect.js' import {computeScanMetadata} from '../scorer/index.js' import {deprecatedScriptTagScope, insecureWebhookUrl} from '../rules/config-rules.js' @@ -47,6 +47,7 @@ import type { CheckExecutionStatus, CoverageGap, Issue, + ScanOptions, ScanResult, SkippedFile, } from '../types.js' @@ -581,15 +582,23 @@ function normalizeRunnerResult(value: Issue[] | RunnerResult): RunnerResult { return Array.isArray(value) ? {issues: value} : value } -export async function scan(startPath?: string, configFileName?: string): Promise { +export async function scan( + startPath?: string, + configFileName?: string, + options: ScanOptions = {}, +): Promise { const appRoot = findAppRoot(startPath) resetSkippedFiles() const selectedFileName = getAppConfigurationFileName(configFileName) const appToml = loadAppToml(joinPath(appRoot, selectedFileName), appRoot) const appTomls = appToml ? [appToml] : [] - const gitIgnoreListing = await listGitIgnoredPaths(appRoot) + const overrides = ignorePatternRules(options.ignorePatterns ?? []) + const gitIgnoreListing = await listGitIgnoredPaths(appRoot, { + pruneDefaultDirectories: !hasIncludeOverride(overrides), + }) const pathRules = buildPathRules({ gitIgnoredPaths: gitIgnoreListing.status === 'listed' ? gitIgnoreListing.paths : [], + overrides, }) const repositoryFiles = listRepositoryFiles(appRoot, pathRules) const extensions = findExtensions(appRoot, repositoryFiles) 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 3985485212e..e25b0dc52f8 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,12 +1,22 @@ import {findRepositoryMarker} from './repository-marker.js' +import {BugError} from '@shopify/cli-kit/node/error' import {outputDebug} from '@shopify/cli-kit/node/output' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' import ignore from 'ignore' +/** A user `--ignore` pattern. Include rules store the pattern without the leading `!`. */ +export interface PathOverride { + action: 'exclude' | 'include' + pattern: string + source: 'cli' +} + +/** 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 } /** @@ -61,14 +71,22 @@ export type GitIgnoreListing = /** * 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. */ -export async function listGitIgnoredPaths(appRoot: string): Promise { - const listing = await runGitIgnoreListing(appRoot) +export async function listGitIgnoredPaths( + appRoot: string, + options: {pruneDefaultDirectories: boolean}, +): Promise { + const listing = await runGitIgnoreListing(appRoot, options) if (listing.status !== 'listed') outputDebug(`App Security: git ignore listing skipped (${listing.status})`) return listing } -async function runGitIgnoreListing(appRoot: string): Promise { +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']) if (location === undefined) return {status: 'failed'} @@ -100,13 +118,17 @@ async function runGitIgnoreListing(appRoot: string): Promise { '--directory', '--', '.', - ...defaultDirectoryPathspecExcludes(), + ...(options.pruneDefaultDirectories ? defaultDirectoryPathspecExcludes() : []), ]) if (listed === undefined || listed.exitCode !== 0) return {status: 'failed'} return {status: 'listed', paths: listed.stdout.split('\0').filter((path) => path !== '')} } -/** The walker always prunes default directories, so stop git walking an unignored `node_modules/`. */ +/** + * 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. + */ function defaultDirectoryPathspecExcludes(): string[] { return DEFAULT_EXCLUDE_PATTERNS.filter((pattern) => pattern.endsWith('/')).map( (pattern) => `:(exclude,glob)**/${pattern}**`, @@ -123,22 +145,110 @@ async function runGit(cwd: string, args: string[]): Promise<{exitCode: number; s } } -export function buildPathRules(input: {gitIgnoredPaths: ReadonlyArray}): PathRules { - return {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: input.gitIgnoredPaths} +/** `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 + } +} + +/** + * 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 `..`. + */ +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 ?? [], + } +} + +function toGitIgnoreLine(override: PathOverride): string { + return override.action === 'include' ? `!${override.pattern}` : override.pattern } /** - * Git paths are looked up in a Set so names like `[id].ts` match literally. * `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 defaults = ignore.default({ignorecase: false, allowRelativePaths: true}).add([...rules.defaults]) + 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 - return gitIgnored.has(key) || defaults.ignores(key) + 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) } } 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 727a419f29c..44a86775bc7 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 @@ -96,7 +96,8 @@ describe('dependency automation discovery', () => { // 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'}) - expect(findDependencyAutomationInputs(root, {defaults: ['.github/'], gitIgnoredPaths: []})).toEqual({files: []}) + const rules = {defaults: ['.github/'], gitIgnoredPaths: [], overrides: []} + expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).files).toHaveLength(1) }) }) 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 bdd02997d65..16bf9e2e1ef 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 @@ -168,14 +168,14 @@ describe('dependency automation scanner integration', () => { }) }) - describe('gitignored configuration', () => { - async function makeRepository(root: string, files: Record, tracked: string[]): Promise { - await makeApp(root, files) - git(root, ['init', '-q', '.']) - git(root, ['add', '-f', '--', 'shopify.app.toml', 'package.json', ...tracked]) - git(root, ['commit', '-qm', 'init']) - } + async function makeRepository(root: string, files: Record, tracked: string[]): Promise { + await makeApp(root, files) + git(root, ['init', '-q', '.']) + git(root, ['add', '-f', '--', 'shopify.app.toml', 'package.json', ...tracked]) + git(root, ['commit', '-qm', 'init']) + } + describe('gitignored configuration', () => { test('does not read an untracked configuration file that git ignores', async () => { await inTemporaryDirectory(async (root) => { await makeRepository(root, {'.gitignore': '.github/\n', '.github/dependabot.yml': dependabot}, ['.gitignore']) @@ -222,6 +222,43 @@ describe('dependency automation scanner integration', () => { }) }) + describe('--ignore', () => { + test('does not read a committed configuration file a pattern excludes', async () => { + await inTemporaryDirectory(async (root) => { + await makeRepository(root, {'.github/dependabot.yml': dependabot}, ['.github/dependabot.yml']) + const result = await scan(root, undefined, {ignorePatterns: ['.github/']}) + expect(dependencyFindings(result)).toHaveLength(1) + expect(dependencyExecution(result)).toMatchObject({status: 'executed', inspected_files: ['package.json']}) + expect(result.scan.file_hashes).not.toHaveProperty('.github/dependabot.yml') + }) + }) + + test('reads an untracked gitignored configuration file a pattern includes again', async () => { + await inTemporaryDirectory(async (root) => { + // A tracked CODEOWNERS stops git collapsing `.github/`, so it lists the file itself. + await makeRepository( + root, + { + '.gitignore': '.github/dependabot.yml\n', + '.github/CODEOWNERS': '* @owners\n', + '.github/dependabot.yml': dependabot, + }, + ['.gitignore', '.github/CODEOWNERS'], + ) + const excluded = await scan(root) + expect(dependencyFindings(excluded)).toHaveLength(1) + + const result = await scan(root, undefined, {ignorePatterns: ['!.github/dependabot.yml']}) + expect(dependencyFindings(result)).toEqual([]) + expect(dependencyExecution(result)).toMatchObject({ + status: 'executed', + inspected_files: ['package.json', '.github/dependabot.yml'], + }) + expect(result.scan.file_hashes?.['.github/dependabot.yml']).toBe(sha256(dependabot)) + }) + }) + }) + test('does not treat package.json as a Renovate configuration filename', async () => { await inTemporaryDirectory(async (root) => { await makeApp(root, {'package.json': '{"dependencies":{"react":"19.0.0"},"renovate":{}}'}) 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 a0570afc0be..24eeec38110 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 @@ -67,7 +67,7 @@ function inspectedManifestPaths(result: ScanResult): string[] { } async function scanPathRules(appRoot: string): Promise { - const listing = await listGitIgnoredPaths(appRoot) + const listing = await listGitIgnoredPaths(appRoot, {pruneDefaultDirectories: true}) return buildPathRules({gitIgnoredPaths: listing.status === 'listed' ? listing.paths : []}) } @@ -594,6 +594,132 @@ describe('gitignore-driven exclusions', () => { }) }) +describe('--ignore patterns', () => { + let restoreGitConfig: () => void + beforeEach(() => { + restoreGitConfig = isolateGitConfig() + }) + afterEach(() => { + restoreGitConfig() + }) + + test('excludes a folder that neither the defaults nor .gitignore cover', async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + 'src/index.ts': 'export const included = true', + 'generated/client.ts': 'export const excluded = true', + 'web/generated/schema.ts': 'export const excluded = true', + }) + + const paths = hashedPaths(await scan(root, undefined, {ignorePatterns: ['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', + }) + + // `/build/` is anchored to the app directory; the nested `build/` stays excluded by the default. + const anchored = hashedPaths(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 = hashedPaths(await scan(root, undefined, {ignorePatterns: ['!build/']})) + expect(unanchored).toContain('build/x.ts') + expect(unanchored).toContain('packages/a/build/y.ts') + }) + + 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', + }) + + expect(hashedPaths(await scan(root))).not.toContain('tmp/scratch.ts') + expect(hashedPaths(await scan(root, undefined, {ignorePatterns: ['!tmp/']}))).toContain('tmp/scratch.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`, + }) + const inner = join(root, 'inner') + git(inner, ['init', '-q', '.']) + git(inner, ['add', 'token.ts']) + git(inner, ['commit', '-qm', 'Add token']) + + expect(secretFindingFiles(await scan(root))).toEqual([]) + expect(secretFindingFiles(await scan(root, undefined, {ignorePatterns: ['!inner/']}))).toEqual(['inner/token.ts']) + }) + + test('cannot re-include a file inside a gitignored folder without re-including the folder', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'tmp/\n', + 'tmp/keep.ts': 'export const stillExcluded = true', + 'tmp/scratch.ts': 'export const stillExcluded = true', + }) + + const paths = hashedPaths(await scan(root, undefined, {ignorePatterns: ['!tmp/keep.ts']})) + expect(paths).not.toContain('tmp/keep.ts') + expect(paths).not.toContain('tmp/scratch.ts') + }) + + 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. + 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}', + }) + + const paths = hashedPaths(await scan(root, undefined, {ignorePatterns: ['!build/']})) + expect(paths).toContain('build/a.ts') + expect(paths).not.toContain('build/x.local.json') + }) + + test('applies later patterns over earlier ones', async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + 'generated/client.ts': 'export const decided = true', + }) + + const excludeThenInclude = hashedPaths(await scan(root, undefined, {ignorePatterns: ['generated/', '!generated/']})) + expect(excludeThenInclude).toContain('generated/client.ts') + + const includeThenExclude = hashedPaths(await scan(root, undefined, {ignorePatterns: ['!generated/', 'generated/']})) + expect(includeThenExclude).not.toContain('generated/client.ts') + }) + + test('never stops the selected app configuration from loading or being scanned for secrets', async () => { + const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') + 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`, + }) + + const result = await scan(root, 'staging', {ignorePatterns: ['shopify.app*.toml']}) + expect(result.app.name).toBe('Staging') + expect(hashedPaths(result)).toContain('shopify.app.staging.toml') + expect(secretFindingFiles(result)).toEqual(['shopify.app.staging.toml']) + }) +}) + describe('listRepositoryFiles', () => { test('lists a symlinked directory as an entry without traversing it', async () => { const root = await makeDirectory() 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 ee0d5de0e93..6f875f9522f 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 @@ -6,12 +6,15 @@ import { createFilePathMatcher, createPathMatcher, listGitIgnoredPaths, + ignorePatternProblem, + ignorePatternRules, } 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 {tmpdir} from 'node:os' import {join} from 'node:path' -import type {PathRules} from '../scanners/path-rules.js' +import type {PathOverride, PathRules} from '../scanners/path-rules.js' const temporaryDirectories: string[] = [] let restoreGitConfig: (() => void) | undefined @@ -46,12 +49,17 @@ function makeRepository(files: Record): string { return root } -const DEFAULTS_ONLY: PathRules = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: []} +const DEFAULTS_ONLY: PathRules = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: [], overrides: []} -const gitIgnoredOnly = (paths: string[]): PathRules => ({defaults: [], gitIgnoredPaths: paths}) +const gitIgnoredOnly = (paths: string[]): PathRules => ({defaults: [], gitIgnoredPaths: paths, overrides: []}) -async function listedPaths(appRoot: string): Promise { - const listing = await listGitIgnoredPaths(appRoot) +const cliExclude = (pattern: string): PathOverride => ({action: 'exclude', pattern, source: 'cli'}) +const cliInclude = (pattern: string): PathOverride => ({action: 'include', pattern, source: 'cli'}) + +const PRUNED = {pruneDefaultDirectories: true} + +async function listedPaths(appRoot: string, options = PRUNED): Promise { + const listing = await listGitIgnoredPaths(appRoot, options) expect(listing.status).toBe('listed') return listing.status === 'listed' ? listing.paths : [] } @@ -60,7 +68,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)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) + await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) }) test('reports an ignored single file', async () => { @@ -166,7 +174,7 @@ describe('listGitIgnoredPaths', () => { const root = makeDirectory() writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) - await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'not-a-repository'}) + 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 () => { @@ -180,7 +188,7 @@ describe('listGitIgnoredPaths', () => { git(repository, ['add', '-f', 'apps/web/README.md', '.gitignore']) git(repository, ['commit', '-qm', 'init']) - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).resolves.toEqual({ + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ status: 'app-root-ignored', }) }) @@ -195,7 +203,7 @@ describe('listGitIgnoredPaths', () => { // Control: from the repository root, git lists the ignored folder. await expect(listedPaths(repository)).resolves.toEqual(['apps/']) - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).resolves.toEqual({ + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ status: 'app-root-ignored', }) }) @@ -203,7 +211,7 @@ describe('listGitIgnoredPaths', () => { 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)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) + 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 () => { @@ -213,7 +221,7 @@ describe('listGitIgnoredPaths', () => { 'tmp.log': '', }) - await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'listed', paths: ['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 () => { @@ -224,7 +232,7 @@ describe('listGitIgnoredPaths', () => { 'apps/web/.env': 'SECRET=1\n', }) - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).resolves.toEqual({ + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({ status: 'listed', paths: ['.env', 'shopify.app.toml'], }) @@ -233,7 +241,7 @@ describe('listGitIgnoredPaths', () => { test('reports a directory inside .git as not being in a repository', async () => { const root = makeRepository({}) - await expect(listGitIgnoredPaths(join(root, '.git'))).resolves.toEqual({status: 'not-a-repository'}) + await expect(listGitIgnoredPaths(join(root, '.git'), PRUNED)).resolves.toEqual({status: 'not-a-repository'}) }) test.each([ @@ -248,7 +256,7 @@ describe('listGitIgnoredPaths', () => { git(root, ['commit', '-qm', 'init']) writeFileSync(join(root, '.git', file), content) - await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) }, ) @@ -260,7 +268,7 @@ describe('listGitIgnoredPaths', () => { writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) symlinkSync(join(root, 'missing-git-dir'), join(root, '.git')) - await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) }, ) @@ -268,7 +276,7 @@ describe('listGitIgnoredPaths', () => { const repository = makeRepository({'apps/web/src/index.ts': ''}) writeFileSync(join(repository, '.git', 'config'), '[core\nbogus') - await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'), PRUNED)).resolves.toEqual({status: 'failed'}) }) test('reports a failure when git cannot list the working tree', async () => { @@ -279,7 +287,7 @@ describe('listGitIgnoredPaths', () => { // `ls-files` exit with a fatal error. writeFileSync(join(root, '.git', 'index'), 'not an index') - await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) + await expect(listGitIgnoredPaths(root, PRUNED)).resolves.toEqual({status: 'failed'}) }) }) @@ -313,6 +321,22 @@ describe('listGitIgnoredPaths', () => { 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', + ]) + }) }) }) @@ -447,12 +471,100 @@ describe('DEFAULT_EXCLUDE_PATTERNS', () => { describe('createPathMatcher', () => { test('is case sensitive', () => { - const isExcluded = createPathMatcher({defaults: ['Build/'], gitIgnoredPaths: []}) + const isExcluded = createPathMatcher({defaults: ['Build/'], gitIgnoredPaths: [], overrides: []}) expect(isExcluded('Build/a.ts', {directory: false})).toBe(true) expect(isExcluded('build/a.ts', {directory: false})).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('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) + }) + }) + + 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/']) + + expect(excludeThenInclude('generated', {directory: true})).toBe(false) + expect(includeThenExclude('generated', {directory: true})).toBe(true) + }) + }) + 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({ @@ -496,17 +608,145 @@ describe('createFilePathMatcher', () => { expect(isExcluded('packages/web/node_modules/dep/index.js')).toBe(true) expect(isExcluded('packages/web/src/index.js')).toBe(false) }) + + test('lets an include override re-include a git file literal', () => { + const isExcluded = createFilePathMatcher({ + defaults: [], + gitIgnoredPaths: ['.github/dependabot.yml'], + overrides: [cliInclude('.github/dependabot.yml')], + }) + + 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/')]}) + + expect(isExcluded('.github/dependabot.yml')).toBe(true) + expect(isExcluded('renovate.json')).toBe(false) + }) +}) + +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) + }) + + test('returns no rules for no patterns', () => { + expect(ignorePatternRules([])).toEqual([]) + }) + + 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/) + }) +}) + +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('rejects empty and whitespace-only values', () => { + expect(ignorePatternProblem('')).toMatch(/empty/) + expect(ignorePatternProblem(' ')).toMatch(/empty/) + }) + + test('rejects a .gitignore comment and suggests escaping the #', () => { + const problem = ignorePatternProblem('#hash.ts') + expect(problem).toMatch(/comment/) + expect(problem).toContain('\\#') + }) + + test('rejects a `!` with nothing to re-include', () => { + expect(ignorePatternProblem('!')).toMatch(/nothing after/) + expect(ignorePatternProblem('! ')).toMatch(/nothing after/) + }) + + 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('\\\\') + } + }) + + 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('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.`, + ) + } + }) + + 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('allows patterns where dots are part of a path segment', () => { + for (const value of ['..cache/', 'a..b', '...', 'x/..y']) { + expect(ignorePatternProblem(value), value).toBeUndefined() + } + }) }) describe('buildPathRules', () => { - test('keeps the defaults and the git literals as separate phases, in input order', () => { - expect(buildPathRules({gitIgnoredPaths: ['notes.txt', 'tmp/']})).toEqual({ + test('keeps the defaults, the git literals and the overrides as separate phases, in input order', () => { + const overrides = [cliExclude('generated/'), cliInclude('build/')] + + expect(buildPathRules({gitIgnoredPaths: ['notes.txt', 'tmp/'], overrides})).toEqual({ defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: ['notes.txt', 'tmp/'], + overrides, }) }) - test('has no git literals when git reported nothing', () => { - expect(buildPathRules({gitIgnoredPaths: []})).toEqual({defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: []}) + 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) }) }) 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 e907090c444..9fba7bc396c 100644 --- a/packages/app/src/cli/services/app-security-engine/types.ts +++ b/packages/app/src/cli/services/app-security-engine/types.ts @@ -72,6 +72,10 @@ export interface ProjectDetection { languages: DetectedLanguage[] } +export interface ScanOptions { + ignorePatterns?: ReadonlyArray +} + export interface ScanResult { version: string timestamp: string diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 4ca20564199..c1ad30e26b8 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -128,6 +128,7 @@ function testOptions() { yes: false, skipInstructions: false, clean: false, + ignorePatterns: [], } } @@ -142,6 +143,7 @@ describe('securityCheck', () => { appRoot: '/tmp/unlinked-app', configName: undefined, findingsPath: undefined, + ignorePatterns: [], }) expect(dependencies.writeArtifacts).toHaveBeenCalledWith(scanExecution, {clean: false}) expect(dependencies.renderReport).toHaveBeenCalledWith({ @@ -167,6 +169,7 @@ describe('securityCheck', () => { appRoot: '/tmp/unlinked-app', configName: 'staging', findingsPath: undefined, + ignorePatterns: [], }) expect(dependencies.renderReport).toHaveBeenCalledWith( expect.objectContaining({ @@ -175,6 +178,45 @@ describe('securityCheck', () => { ) }) + test('forwards ignorePatterns 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/'] + + await securityCheck({...testOptions(), ignorePatterns}, dependencies) + + const commands = resolveAppSecurityCommands(scanExecution.appRoot, 'shopify.app.toml', ignorePatterns) + expect(commands.scan.args).toContainEqual({flag: '--ignore', value: 'generated/'}) + expect(dependencies.execute).toHaveBeenCalledWith({ + appRoot: '/tmp/unlinked-app', + configName: undefined, + findingsPath: undefined, + ignorePatterns, + }) + expect(dependencies.renderReport).toHaveBeenCalledWith(expect.objectContaining({commands})) + expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) + }) + + test('repeats ignorePatterns in the recovery commands of a refused scan', async () => { + const dependencies = testDependencies() + dependencies.findingsFileExists.mockResolvedValue(true) + const ignorePatterns = ['generated/'] + const commands = resolveAppSecurityCommands(scanExecution.appRoot, 'shopify.app.toml', ignorePatterns) + + const error = await securityCheck({...testOptions(), ignorePatterns}, dependencies).catch((error: unknown) => error) + + expect(error).toBeInstanceOf(AbortError) + expect(formatAppSecurityCommand(commands.compile)).toContain('--ignore') + expect(error).toMatchObject({ + tryMessage: expect.stringContaining(formatAppSecurityCommand(commands.compile)), + }) + expect(error).toMatchObject({ + tryMessage: expect.stringContaining(formatAppSecurityCommand(commands.clean)), + }) + expect(dependencies.execute).not.toHaveBeenCalled() + }) + test('refuses to scan when agent findings exist', async () => { const dependencies = testDependencies() dependencies.findingsFileExists.mockResolvedValue(true) diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index a0ea627b7ef..4efc86c8dfd 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -40,6 +40,7 @@ interface SecurityOptions { skipInstructions: boolean findingsPath?: string clean: boolean + ignorePatterns: ReadonlyArray } export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' @@ -49,7 +50,12 @@ interface SecurityDependencies { artifactPaths(appRoot: string): ResolvedAppSecurityArtifactPaths findingsFileExists(path: string): Promise readTrace(path: string): Promise - execute(options: {appRoot: string; configName?: string; findingsPath?: string}): Promise + execute(options: { + appRoot: string + configName?: string + findingsPath?: string + ignorePatterns: ReadonlyArray + }): Promise writeArtifacts( execution: AppSecurityExecution, options: WriteAppSecurityArtifactsOptions, @@ -82,12 +88,13 @@ const defaultDependencies: SecurityDependencies = { artifactPaths: appSecurityArtifactPaths, findingsFileExists: fileExists, readTrace, - execute: async ({appRoot, configName, findingsPath}) => { + execute: async ({appRoot, configName, findingsPath, ignorePatterns}) => { const findings = findingsPath ? await loadAppSecurityFindings(findingsPath) : undefined return executeAppSecurity({ appRoot, findings, configFileName: requireSecurityConfigFileName(appRoot, configName), + ignorePatterns, }) }, writeArtifacts: writeAppSecurityArtifacts, @@ -156,7 +163,11 @@ export default async function securityCheck( dependencies: SecurityDependencies = defaultDependencies, ): Promise { const appRoot = dependencies.resolveRoot(options.directory) - const commands = resolveAppSecurityCommands(appRoot, resolveSecurityConfigFileName(appRoot, options.configName)) + const commands = resolveAppSecurityCommands( + appRoot, + resolveSecurityConfigFileName(appRoot, options.configName), + options.ignorePatterns, + ) if (!options.findingsPath && !options.clean) { await assertCanStartScan(dependencies.artifactPaths(appRoot), commands, dependencies) } @@ -165,6 +176,7 @@ export default async function securityCheck( appRoot, configName: options.configName, findingsPath: options.findingsPath, + ignorePatterns: options.ignorePatterns, }) const artifacts = await dependencies.writeArtifacts(execution, {clean: options.clean}) diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 8776b7addba..89a92620102 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -3660,8 +3660,8 @@ "args": { }, "customPluginName": "@shopify/app", - "description": "Runs Shopify App Security locally and creates its review pack and trace.\n\nPass `--findings` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass `--clean` to discard that work and start over. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration.\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 creates its review pack and trace.\n\nPass `--findings` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass `--clean` to discard that work and start over. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration.\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 creates its review pack and trace.\n\nPass `--findings` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass `--clean` to discard that work and start over. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration.\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 generated follow-up commands repeat the patterns, and `--findings` must use the same patterns as the scan it validates.\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 creates its review pack and trace.\n\nPass `--findings` after completing the review pack to validate agent findings and compile them into the trace. A new scan stops when local agent findings or a compiled trace already exist. Pass `--clean` to discard that work and start over. Use `--config` to select a specific app configuration when the project has multiple `shopify.app*.toml` files; App Security inspects only that configuration.\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 generated follow-up commands repeat the patterns, and `--findings` must use the same patterns as the scan it validates.\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": { @@ -3709,6 +3709,13 @@ "name": "findings", "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.", + "hasDynamicHelp": false, + "multiple": true, + "name": "ignore", + "type": "option" + }, "json": { "allowNo": false, "char": "j",