From b1df373ed330f528659e89921cba276babdd3b9a Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Mon, 28 Sep 2026 13:07:54 -0700 Subject: [PATCH 1/7] Exclude gitignored files from App Security discovery App Security's deterministic checks scanned files that git ignores, such as local .env files and generated output, and reported findings for files that never reach the repository. Discovery now walks the app root once and skips a path when either of two exclusion phases matches it: - Default .gitignore-style patterns for dependencies, build output, caches, test and fixture trees, and CLI-generated folders. - The untracked, ignored paths git reports, so nested .gitignore files, negations, .git/info/exclude and global excludes all apply. Tracked files that match .gitignore are still scanned. No git exclusions apply when the app isn't in a git repository, when git fails, or when an enclosing repository ignores the app folder or one of its ancestors. Git's listing skips the default directories, so git doesn't traverse trees the walker never enters. The walker prunes excluded folders, stops at nested apps, and walks dot-folders and dotfiles, so .github/ and .vscode/ are now scanned for secrets. Loading the app configuration isn't subject to exclusions. Dependabot and Renovate configuration is, so an ignored configuration file no longer counts as dependency automation. The committed-secret check no longer skips untracked, ignored files on its own, because discovery decides what is scanned. When git ignores a file that was still scanned, the finding says why: an enclosing repository ignores the app, the file is inside a nested repository, or git couldn't list ignored files. --- .changeset/app-security-gitignore.md | 5 + .../app-security-engine/rules/secret-rules.ts | 141 ++++- .../app-security-engine/rules/types.ts | 3 + .../app-security-engine/scanners/discover.ts | 416 +++++++------- .../scanners/filesystem-errors.ts | 13 + .../app-security-engine/scanners/index.ts | 23 +- .../scanners/path-rules.ts | 277 +++++++++ .../scanners/repository-marker.ts | 40 ++ .../dependency-automation-discovery.test.ts | 106 +++- .../tests/dependency-automation.test.ts | 73 ++- .../tests/discovery-safety.test.ts | 488 +++++++++++++++- .../tests/git-test-helpers.ts | 69 +++ .../tests/path-rules.test.ts | 531 ++++++++++++++++++ .../tests/rule-analysis.test.ts | 1 + .../tests/secret-safety.test.ts | 154 ++++- 15 files changed, 2067 insertions(+), 273 deletions(-) create mode 100644 .changeset/app-security-gitignore.md create mode 100644 packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts create mode 100644 packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts create mode 100644 packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts create mode 100644 packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts diff --git a/.changeset/app-security-gitignore.md b/.changeset/app-security-gitignore.md new file mode 100644 index 00000000000..7c2bed41a26 --- /dev/null +++ b/.changeset/app-security-gitignore.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +`app security check` no longer scans gitignored files, and now scans dot-folders such as `.github`. 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 0cce5a37bf9..d84d4d85807 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,5 +1,7 @@ +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 {Issue} from '../types.js' /** @@ -144,10 +146,72 @@ function isEnvFile(path: string): boolean { return envFileBasename(path) !== undefined } -function committedSecretFileIssue(file: SourceFile, status: GitFileStatus, environmentFile: boolean): Issue { +/** + * Why a file git reports as untracked-and-ignored was still scanned. Discovery + * drops the paths git lists as ignored, so such a file only reaches the rule + * when the listing did not cover it. `unknown` means nothing verified explains + * it. + */ +type IgnoredFileScanReason = 'enclosing-repository' | 'nested-repository' | 'listing-failed' | 'unknown' + +/** + * The top level of the repository containing `cwd`, or `undefined` when git + * cannot say. A missing git binary resolves with exit code 0 and empty output, + * so only a printed path counts as known. + */ +async function gitTopLevel(cwd: string): Promise { + const result = await runGit(cwd, ['rev-parse', '--show-toplevel']) + return result.exitCode === 0 && result.out !== '' ? result.out : undefined +} + +/** + * Work out, from the listing outcome and (when needed) one more git probe, + * why an ignored file was scanned. Only claims what was verified: + * + * - `app-root-ignored`: the enclosing repository ignores the app folder, so + * discovery ignored its rules on purpose. + * - `listed`: the realistic cause is a nested repository, which is confirmed by + * comparing `git rev-parse --show-toplevel` in the file's directory and in + * the app root; the comparison is returned as extra evidence. + * - `failed`: discovery scanned everything because git could not list ignored files. + * - `not-a-repository`: git could not have reported the file as ignored. + * + * `appTopLevel` resolves the app root's top level; the caller shares one + * result across every file it asks about. + */ +async function ignoredFileScanReason( + appRoot: string, + path: string, + gitIgnoreListing: GitIgnoreListing['status'], + 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: []} + + const fileTopLevel = await gitTopLevel(dirname(joinPath(appRoot, path))) + const appRootTopLevel = await appTopLevel() + const bothKnown = fileTopLevel !== undefined && appRootTopLevel !== undefined + const nested = bothKnown && fileTopLevel !== appRootTopLevel + let verdict = 'unknown' + if (nested) verdict = 'differs' + else if (bothKnown) verdict = 'same' + return { + reason: nested ? 'nested-repository' : 'unknown', + evidence: [`git rev-parse --show-toplevel → ${verdict} for ${path} and the app root`], + } +} + +function committedSecretFileIssue( + file: SourceFile, + status: GitFileStatus, + environmentFile: boolean, + ignoredScanReason: IgnoredFileScanReason, +): Issue { const kind = environmentFile ? 'Environment file with secrets' : 'Secret file' const tracked = status.tracked === true const untrackedAndNotIgnored = status.tracked === false && status.ignored === false + const untrackedAndIgnored = status.tracked === false && status.ignored === true let title: string let message: string @@ -160,6 +224,24 @@ function committedSecretFileIssue(file: SourceFile, status: GitFileStatus, envir 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.` + fixDescription = `Ignore ${file.path} in the nested repository and rotate any exposed secrets` + } else if (ignoredScanReason === 'listing-failed') { + title = `${kind} is ignored by git but was scanned` + message = `${file.path} is ignored by git, but App Security could not list the ignored files for this app, so it was scanned.` + fixDescription = `Confirm the repository is healthy with 'git status' and rotate any exposed secrets` + } else if (untrackedAndIgnored) { + // Git confirmed the file is untracked and ignored, so the ignore status is not in doubt; only + // the reason discovery still produced the file is. + title = `${kind} is ignored by git but was scanned` + message = `${file.path} is ignored by git but was still scanned; App Security couldn't determine why. Confirm the rule ignoring it belongs to the repository that owns this app before treating this as clean.` + fixDescription = `Confirm with 'git check-ignore -v ${file.path}' that this app's repository ignores it, and rotate any exposed secrets` } else { title = `${kind} could not be confirmed as ignored` message = `${file.path} could not be confirmed as untracked-and-ignored${status.reason ? ` (${status.reason})` : ''}. Confirm it is gitignored before treating this as clean.` @@ -182,10 +264,27 @@ function committedSecretFileIssue(file: SourceFile, status: GitFileStatus, envir } } -/** Rule 8: COMMITTED_SECRET (-50, high) */ -export async function scanCommittedSecrets(secretEvidenceFiles: SourceFile[], appRoot: string): Promise { +/** + * Rule 8: COMMITTED_SECRET (-50, high) + * + * `gitIgnoreListing` is how discovery's request for git's ignored paths went; + * it decides what a finding may claim about a file git reports as ignored. + */ +export async function scanCommittedSecrets( + secretEvidenceFiles: SourceFile[], + appRoot: string, + gitIgnoreListing: GitIgnoreListing['status'], +): Promise { const issues: Issue[] = [] + // The app root's top level is the same for every file: resolve it once, and only when a file + // git reports as untracked-and-ignored needs it. + let appTopLevel: Promise | undefined + const appRootTopLevel = () => { + appTopLevel ??= gitTopLevel(appRoot) + return appTopLevel + } + for (const file of secretEvidenceFiles) { const content = file.content if (content === undefined) continue @@ -202,18 +301,24 @@ export async function scanCommittedSecrets(secretEvidenceFiles: SourceFile[], ap // Keep git probes sequential to avoid spawning competing processes for one repository. // eslint-disable-next-line no-await-in-loop const status = await gitStatusFor(appRoot, file.path) - // A safe local secret file is not a vulnerability or scoring event. - if (status.tracked === false && status.ignored === true) continue // Empty named secret files stay fail-closed only when git confirms they are // tracked — history may still contain prior secrets. Unknown git plus empty // contents is not a provable leak. if (!hasEvidence) { if (emptyNamedSecret && status.tracked === true) { - issues.push(committedSecretFileIssue(file, status, environmentFile)) + issues.push(committedSecretFileIssue(file, status, environmentFile, 'unknown')) } continue } - issues.push(committedSecretFileIssue(file, status, environmentFile)) + let ignoredScanReason: IgnoredFileScanReason = 'unknown' + let evidence = status.evidence ?? [] + if (status.tracked === false && status.ignored === true) { + // eslint-disable-next-line no-await-in-loop + const scanReason = await ignoredFileScanReason(appRoot, file.path, gitIgnoreListing, appRootTopLevel) + ignoredScanReason = scanReason.reason + evidence = [...evidence, ...scanReason.evidence] + } + issues.push(committedSecretFileIssue(file, {...status, evidence}, environmentFile, ignoredScanReason)) } for (const file of secretEvidenceFiles) { @@ -272,17 +377,19 @@ interface GitFileStatus { evidence?: string[] } -export async function gitStatusFor(appRoot: string, file: string): Promise { - const run = async (args: string[]): Promise<{exitCode?: number; out: string}> => { - try { - const result = await captureOutputWithExitCode('git', args, {cwd: appRoot}) - return {exitCode: result.exitCode, out: result.stdout.trim()} - // Missing Git or a failed probe is unknown status, not proof the file is safe. - // eslint-disable-next-line no-catch-all/no-catch-all - } catch { - return {exitCode: undefined, out: ''} - } +async function runGit(cwd: string, args: string[]): Promise<{exitCode?: number; out: string}> { + try { + const result = await captureOutputWithExitCode('git', args, {cwd}) + return {exitCode: result.exitCode, out: result.stdout.trim()} + // Missing Git or a failed probe is unknown status, not proof the file is safe. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return {exitCode: undefined, out: ''} } +} + +export async function gitStatusFor(appRoot: string, file: string): Promise { + const run = async (args: string[]) => runGit(appRoot, args) // Is this even a git repo? If not, we cannot confirm anything. const inRepo = await run(['rev-parse', '--is-inside-work-tree']) 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 783da46584d..2b44733d1c3 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,4 +1,5 @@ import type {Issue, Capabilities, ProjectDetection, Severity, SourceCandidate} from '../types.js' +import type {GitIgnoreListing} from '../scanners/path-rules.js' import type { AppTomlContent, DependencyAutomationInputs, @@ -56,4 +57,6 @@ export interface ScanContext { detection: ProjectDetection /** Path-only inventory, including unsupported source candidates. */ sourceCandidates: SourceCandidate[] + /** How asking git for the app's ignored paths went; discovery excluded those paths only when `listed`. */ + gitIgnoreListing: GitIgnoreListing['status'] } 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 0578f4cdeda..a3d2b09e5e1 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,3 +1,6 @@ +import {inspectErrorReason, isMissingFilesystemEntry} from './filesystem-errors.js' +import {createFilePathMatcher, createPathMatcher} from './path-rules.js' +import {findRepositoryMarker} from './repository-marker.js' import {DEPENDENCY_AUTOMATION_CONFIG_PATHS} from '../rules/dependency-automation-rules.js' import {APP_CONFIG_FILE_GLOB, isValidFormatAppConfigurationFileName} from '../../../models/app/config-file-naming.js' import {AppAccessScopesSchema, AppAuthSchema} from '../../../models/extensions/specifications/app_config_app_access.js' @@ -18,7 +21,8 @@ import { } from '@shopify/cli-kit/node/path' import {zod} from '@shopify/cli-kit/node/schema' import {decodeToml} from '@shopify/cli-kit/node/toml/codec' -import {lstatSync, realpathSync} from 'node:fs' +import {lstatSync, readdirSync, realpathSync} from 'node:fs' +import type {PathRules} from './path-rules.js' import type {SourceCandidate} from '../types.js' import type { AppTomlContent, @@ -28,6 +32,7 @@ import type { ManifestFile, WebhookSubscription, } from './types.js' +import type {Dirent} from 'node:fs' /** Expected user error while locating a Shopify app root. */ export class AppRootDiscoveryError extends Error { @@ -227,88 +232,130 @@ function recordSectionGap(appRoot: string | undefined, path: string, detail: str } /** - * Directories never worth scanning: build output, dependencies, and test - * fixture trees. + * A directory holding its own `shopify.app*.toml` is an independent Shopify app. + * Nested apps are independent scan roots and never evidence for their parent + * app, so the walker stops at them: a structural boundary, much as git treats + * a nested repository as opaque to the enclosing one. (Nested git repositories + * themselves are NOT a boundary here; only their `.git` entry is pruned.) * - * Patterns are generic on purpose. Earlier versions hardcoded the names of - * this project's own fixture directories, which both leaked internal naming - * into a tool that ships to third-party developers and silently skipped any - * directory a developer happened to give the same name. + * Detection looks at the directory's raw entries, not the rule-filtered ones: + * a nested app whose configuration file happens to be gitignored is still a + * nested app. + */ +function isNestedAppDirectory(entries: ReadonlyArray): boolean { + return entries.some((entry) => !entry.isDirectory() && isValidFormatAppConfigurationFileName(entry.name)) +} + +function readDirectoryEntries(appRoot: string, absolutePath: string, displayPath: string): 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, { + ok: false, + reason: 'unreadable', + detail: inspectErrorReason(displayPath, error), + }) + return undefined + } +} + +/** + * List every repository file the scan may inspect, as sorted app-root-relative + * POSIX paths. * - * Sub-apps (a nested directory with its own shopify.app.toml) are excluded - * separately by callers, since that requires reading the tree rather than - * matching a name. + * The walk applies `rules` with .gitignore semantics and prunes excluded + * directories: it never descends into them, so the matcher is only ever asked + * about paths whose ancestors are known to be included (its precondition). + * + * Directory entries use lstat semantics, so a symlink is never a directory + * here. Symlinks (to files or directories), FIFOs and other special entries + * are listed but never traversed; `readRepositoryFile` later enforces + * containment and realpath rules on anything a finder decides to read, and + * records failures. Dot-folders and dotfiles are walked unless a rule excludes + * them (see `DEFAULT_EXCLUDE_PATTERNS`). */ -const IGNORED_DIRECTORIES = [ - '**/node_modules/**', - '**/vendor/**', - '**/.git/**', - '**/.next/**', - '**/coverage/**', - '**/dist/**', - '**/build/**', - '**/.shopify/app-security/**', - '**/test/**', - '**/tests/**', - '**/spec/**', - '**/specs/**', - '**/__tests__/**', - '**/fixtures/**', - '**/*-fixtures/**', - '**/__fixtures__/**', - '**/*.test.*', - '**/*.spec.*', -] - -function normalizePath(path: string): string { - return path.replace(/\\/g, '/').replace(/\/+$/, '') -} - -/** Nested Shopify apps are independent scan roots and never evidence for their parent app. */ -function findNestedAppDirectories(appRoot: string): string[] { - return [ - ...new Set( - globSync(`**/${APP_CONFIG_FILE_GLOB}`, { - followSymbolicLinks: false, - cwd: appRoot, - ignore: IGNORED_DIRECTORIES, - absolute: false, - dot: false, - onlyFiles: false, - }) - .filter((path) => isValidFormatAppConfigurationFileName(basename(path))) - .map((path) => normalizePath(dirname(path))) - .filter((path) => path !== '.' && path.length > 0), - ), - ].sort() +export function listRepositoryFiles(appRoot: string, rules: PathRules): string[] { + const matcher = createPathMatcher(rules) + const files: string[] = [] + // Relative directory paths still to be read; '' is the app root itself. Subdirectories are appended + // while iterating, which `for...of` supports: the array iterator re-checks the length on every step. + const pendingDirectories = [''] + + for (const relativeDirectory of pendingDirectories) { + const absoluteDirectory = relativeDirectory === '' ? appRoot : joinPath(appRoot, relativeDirectory) + const entries = readDirectoryEntries( + appRoot, + absoluteDirectory, + relativeDirectory === '' ? 'app root' : relativeDirectory, + ) + if (entries === undefined) continue + // A nested app is a scan root in its own right; nothing beneath it belongs to this scan. + if (relativeDirectory !== '' && isNestedAppDirectory(entries)) continue + + for (const entry of entries) { + const relative = relativeDirectory === '' ? entry.name : `${relativeDirectory}/${entry.name}` + if (entry.isDirectory()) { + // Never descend into an excluded directory: pruning here is what keeps the matcher's + // precondition (every ancestor of a queried path is included) true. + if (!matcher(relative, {directory: true})) pendingDirectories.push(relative) + } else if (!matcher(relative, {directory: false})) { + files.push(relative) + } + } + } + + return files.sort() } -function discoveryIgnores(directory: string, projectRoot: string): string[] { - const nestedApps = findNestedAppDirectories(projectRoot).flatMap((nestedApp) => { - const relativeNestedApp = normalizePath(relativePath(directory, joinPath(projectRoot, nestedApp))) - return relativeNestedApp === '..' || relativeNestedApp.startsWith('../') ? [] : [`${relativeNestedApp}/**`] - }) - return [...IGNORED_DIRECTORIES, ...nestedApps] +/** + * Group source paths by the extension directories that contain them, walking + * each path's ancestors once instead of filtering the whole repository per + * extension. `extensionDirectories` are app-root-relative; `.` is the app root + * and contains every path. A path inside nested extension directories belongs + * to each of them. Paths keep their input order within every group. + */ +function groupSourcePathsByExtensionDirectory( + repositoryFiles: ReadonlyArray, + extensionDirectories: ReadonlySet, +): Map { + const pathsByDirectory = new Map( + [...extensionDirectories].map((directory): [string, string[]] => [directory, []]), + ) + + for (const path of repositoryFiles) { + if (!hasSupportedSourceExtension(path)) continue + // `dirname` yields `.` for a top-level file and for `.` itself, which ends the climb. + let ancestor = dirname(path) + while (ancestor !== '.') { + pathsByDirectory.get(ancestor)?.push(path) + ancestor = dirname(ancestor) + } + pathsByDirectory.get('.')?.push(path) + } + + return pathsByDirectory } /** - * Find extension-like repository content under the app root. + * Find extension-like repository content among the walked repository files. * * `Project.load()` only considers paths in each app configuration's * `extension_directories`. App Security still scans every `shopify.extension.toml` * inside the repository boundary, including unconfigured extensions, because * those files can still contain secrets, XSS, and other security evidence. - * Nested apps, generated output, and test trees remain excluded. + * Nested apps, generated output, and test trees are already absent from + * `repositoryFiles` (see `listRepositoryFiles`). */ -export function findExtensions(appRoot: string): ExtensionInfo[] { - const extensionTomls = globSync('**/shopify.extension.toml', { - followSymbolicLinks: false, - cwd: appRoot, - ignore: discoveryIgnores(appRoot, appRoot), - absolute: false, - dot: false, - onlyFiles: false, - }) +export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray): ExtensionInfo[] { + const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml') + if (extensionTomls.length === 0) return [] + + const sourcePathsByDirectory = groupSourcePathsByExtensionDirectory( + repositoryFiles, + new Set(extensionTomls.map((tomlPath) => dirname(tomlPath))), + ) return extensionTomls.flatMap((tomlPath) => { const fullPath = joinPath(appRoot, tomlPath) @@ -318,8 +365,7 @@ export function findExtensions(appRoot: string): ExtensionInfo[] { try { const raw = decodeToml(content) as Record const type = raw.type as string - const extDir = joinPath(appRoot, tomlPath, '..') - const files = findSourceFiles(extDir, appRoot) + const files = findAppSourceFiles(appRoot, sourcePathsByDirectory.get(dirname(tomlPath)) ?? []) return [{path: tomlPath, type, content, files}] // Invalid repository TOML is a coverage gap, not a scanner crash. // eslint-disable-next-line no-catch-all/no-catch-all @@ -409,15 +455,6 @@ function repositoryPathFailure(detail: string): RepositoryReadFailure { type InspectedPath = {status: 'missing'} | {status: 'file'; path: string} | {status: 'unresolved'; reason: string} -function isMissingFilesystemEntry(error: unknown): boolean { - return error instanceof Error && 'code' in error && (error.code === 'ENOENT' || error.code === 'ENOTDIR') -} - -function inspectErrorReason(target: string, error: unknown): string { - const code = error instanceof Error && 'code' in error && typeof error.code === 'string' ? error.code : undefined - return code ? `Could not inspect ${target} (${code})` : `Could not inspect ${target}` -} - function repositoryDisplayPath(appRoot: string, path: string): string { const relative = normalizeCliPath(relativePath(appRoot, path)) if ( @@ -581,74 +618,51 @@ const SOURCE_LANGUAGES = { '.svelte': {name: 'svelte', supported: false}, } as const -/** A path-only inventory; non-secret deterministic checks never open unsupported source. */ -export function findSourceCandidates(dir: string, projectRoot = dir): SourceCandidate[] { - const paths = globSync( - Object.keys(SOURCE_LANGUAGES).map((extension) => `**/*${extension}`), - { - cwd: dir, - ignore: discoveryIgnores(dir, projectRoot), - absolute: false, - dot: false, - followSymbolicLinks: false, - onlyFiles: false, - }, - ) +type SourceExtension = keyof typeof SOURCE_LANGUAGES - return paths - .map((path): SourceCandidate => { - const extension = extname(path) as keyof typeof SOURCE_LANGUAGES - const language = SOURCE_LANGUAGES[extension] - return { - path: relativePath(projectRoot, joinPath(dir, path)).replace(/\\/g, '/'), - extension, - language: language.name, - supported: language.supported, - } - }) - .sort((left, right) => left.path.localeCompare(right.path)) +/** Extension matching is exact and case-sensitive: `.JS` is not treated as source. */ +function sourceLanguageFor(path: string): (typeof SOURCE_LANGUAGES)[SourceExtension] | undefined { + const extension = extname(path) + return extension in SOURCE_LANGUAGES ? SOURCE_LANGUAGES[extension as SourceExtension] : undefined } -/** Find and read only source languages supported by non-secret deterministic scanners. */ -function findSourceFiles(dir: string, projectRoot = dir): SourceFile[] { - const patterns = Object.entries(SOURCE_LANGUAGES) - .filter(([, language]) => language.supported) - .map(([extension]) => `**/*${extension}`) +function hasSupportedSourceExtension(path: string): boolean { + return sourceLanguageFor(path)?.supported === true +} - const files = globSync(patterns, { - cwd: dir, - ignore: discoveryIgnores(dir, projectRoot), - absolute: false, - dot: false, - // Don't follow directory symlinks; a link to a large shared tree would inflate the scan. - followSymbolicLinks: false, - onlyFiles: false, - }) +/** A path-only inventory; non-secret deterministic checks never open unsupported source. */ +export function findSourceCandidates(repositoryFiles: ReadonlyArray): SourceCandidate[] { + return repositoryFiles + .flatMap((path): SourceCandidate[] => { + const language = sourceLanguageFor(path) + if (!language) return [] + return [{path, extension: extname(path), language: language.name, supported: language.supported}] + }) + .sort((left, right) => left.path.localeCompare(right.path)) +} - return files.map((file) => { - const absolutePath = joinPath(dir, file) - const projectPath = relativePath(projectRoot, absolutePath).replace(/\\/g, '/') - const ext = extname(file) - const result = readRepositoryFile(projectRoot, absolutePath) +/** + * Read the source files (backend routes, extension code, etc.) whose language + * is supported by the non-secret deterministic scanners. Results keep the order + * of `repositoryFiles`, which callers pass in `listRepositoryFiles` order. + */ +export function findAppSourceFiles(appRoot: string, repositoryFiles: ReadonlyArray): SourceFile[] { + // `findExtensions` passes pre-filtered groups, but the filter is this function's own contract for every caller. + return repositoryFiles.filter(hasSupportedSourceExtension).map((path) => { + const absolutePath = joinPath(appRoot, path) + const result = readRepositoryFile(appRoot, absolutePath) return { - path: projectPath, + path, absolutePath, - ext, + ext: extname(path), content: result.ok ? result.content.toString() : undefined, } }) } -/** - * Find all source files in the app root (backend routes, etc.) - */ -export function findAppSourceFiles(appRoot: string): SourceFile[] { - return findSourceFiles(appRoot) -} - const LOCKFILE_MANAGERS = new Set(['package-lock.json', 'pnpm-lock.yaml', 'yarn.lock']) -const SECRET_TEXT_EXTENSIONS = [ +const SECRET_TEXT_EXTENSIONS = new Set([ ...Object.keys(SOURCE_LANGUAGES), '.md', '.mdx', @@ -672,7 +686,22 @@ const SECRET_TEXT_EXTENSIONS = [ '.sql', '.txt', '.pem', -] +]) + +/** Extensionless files that routinely carry credentials. */ +const SENSITIVE_FILE_NAMES = new Set(['Dockerfile', 'Containerfile', 'Gemfile', 'Rakefile', 'Procfile', 'Makefile']) + +function isSensitiveFile(path: string): boolean { + if (SECRET_TEXT_EXTENSIONS.has(extname(path))) return true + const fileName = basename(path) + return ( + fileName === '.env' || + fileName.startsWith('.env.') || + SENSITIVE_FILE_NAMES.has(fileName) || + fileName.startsWith('Dockerfile.') || + fileName.startsWith('Containerfile.') + ) +} function isProbablyBinary(content: Buffer): boolean { const sample = content.subarray(0, Math.min(content.length, 8_000)) @@ -684,40 +713,25 @@ function isProbablyBinary(content: Buffer): boolean { return sample.length > 0 && suspiciousControlBytes / sample.length > 0.1 } -/** Text evidence inspected for secrets regardless of app framework support. */ -export function findSensitiveFiles(appRoot: string, selectedAppConfigFileName?: string): SourceFile[] { - const patterns = [ - ...SECRET_TEXT_EXTENSIONS.map((extension) => `**/*${extension}`), - '**/.env', - '**/.env.*', - '**/Dockerfile', - '**/Dockerfile.*', - '**/Containerfile', - '**/Containerfile.*', - '**/Gemfile', - '**/Rakefile', - '**/Procfile', - '**/Makefile', - ] - const paths = [ - ...new Set( - globSync(patterns, { - cwd: appRoot, - ignore: discoveryIgnores(appRoot, appRoot), - absolute: false, - dot: false, - followSymbolicLinks: false, - onlyFiles: false, - }), - ), - ] +/** + * Text evidence inspected for secrets regardless of app framework support. + * Results keep the order of `repositoryFiles`, which callers pass in + * `listRepositoryFiles` order. + */ +export function findSensitiveFiles( + appRoot: string, + repositoryFiles: ReadonlyArray, + selectedAppConfigFileName?: string, +): SourceFile[] { + const paths = repositoryFiles + .filter(isSensitiveFile) + // Compares the whole relative path, so only the app root's own lockfiles are dropped. .filter((path) => !LOCKFILE_MANAGERS.has(path)) .filter((path) => { const fileName = basename(path) if (!isValidFormatAppConfigurationFileName(fileName)) return true return fileName === selectedAppConfigFileName }) - .sort() return paths.flatMap((path): SourceFile[] => { const absolutePath = joinPath(appRoot, path) @@ -728,45 +742,34 @@ export function findSensitiveFiles(appRoot: string, selectedAppConfigFileName?: }) } +/** + * Why repository-level configuration cannot be attributed to the app, if it + * cannot. The app owns its configuration when its own `.git` marker is the + * nearest one (or there is no repository at all); a marker found only above + * the app root means the configuration belongs to an enclosing repository. + */ function nestedRepositoryReason(appRoot: string): string | undefined { - try { - const stats = lstatSync(joinPath(appRoot, '.git')) - if (stats.isDirectory() || stats.isFile()) return undefined - // A root marker establishes the app's repository boundary only when it is - // a directory or worktree file. Never follow ambiguous marker entries. - return 'Could not determine repository ownership from .git' - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - if (!isMissingFilesystemEntry(error)) return inspectErrorReason('.git', error) - } - - let ancestor = dirname(appRoot) - while (true) { - try { - const stats = lstatSync(joinPath(ancestor, '.git')) - if (stats.isDirectory() || stats.isFile()) { - return 'App root is nested below a parent Git repository' - } - // A symlink or special file is not a supported repository marker, but - // treating it as absent would make repository ownership ambiguous. - return 'Could not determine repository ownership from .git' - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - if (!isMissingFilesystemEntry(error)) return inspectErrorReason('.git', error) - } - - const parent = dirname(ancestor) - if (parent === ancestor) return undefined - ancestor = parent - } + const marker = findRepositoryMarker(appRoot) + if (marker.status === 'none') return undefined + if (marker.status === 'ambiguous') return marker.reason + 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. */ -export function findDependencyAutomationInputs(appRoot: string): DependencyAutomationInputs { +/** + * Read local bot configuration only; hosted integrations and CI workflows are + * outside this check's scope. + * + * The allowlisted paths are read directly from disk rather than taken from the + * walked file list, so that a symlinked `.github` (which the walker never + * enters) is reported as unresolved rather than missing. The scan's `rules` + * still apply: hosted bots read the repository, so a configuration file git + * ignores configures nothing and is treated exactly like a missing one. + */ +export function findDependencyAutomationInputs(appRoot: string, rules: PathRules): DependencyAutomationInputs { let canonicalRoot: string try { canonicalRoot = realpathSync(resolvePath(appRoot)) @@ -781,9 +784,11 @@ export function findDependencyAutomationInputs(appRoot: string): DependencyAutom const repositoryReason = nestedRepositoryReason(canonicalRoot) if (repositoryReason) return {files: [], unresolvedReason: repositoryReason} + const isExcluded = createFilePathMatcher(rules) 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') { @@ -817,17 +822,12 @@ export function findDependencyAutomationInputs(appRoot: string): DependencyAutom return files.length > 0 ? {files} : {files, ...(unresolvedReason ? {unresolvedReason} : {})} } -/** Find JavaScript package manifests. Dependency analysis intentionally supports JavaScript only. */ -export function findManifestPaths(appRoot: string): string[] { - const paths = globSync(['**/package.json'], { - followSymbolicLinks: false, - cwd: appRoot, - ignore: discoveryIgnores(appRoot, appRoot), - absolute: false, - dot: false, - onlyFiles: false, - }) - return [...new Set(paths)].sort() +/** + * Find JavaScript package manifests. Dependency analysis intentionally supports JavaScript only. + * Results keep the order of `repositoryFiles`, which callers pass in `listRepositoryFiles` order. + */ +export function findManifestPaths(repositoryFiles: ReadonlyArray): string[] { + return repositoryFiles.filter((path) => basename(path) === 'package.json') } const PackageManifestSchema = zod.object({ @@ -835,10 +835,10 @@ const PackageManifestSchema = zod.object({ devDependencies: zod.record(zod.string()).optional(), }) -export function findManifests(appRoot: string, discoveredPaths = findManifestPaths(appRoot)): ManifestFile[] { +export function findManifests(appRoot: string, discoveredPaths: ReadonlyArray): ManifestFile[] { const manifests: ManifestFile[] = [] - const pkgPaths = discoveredPaths.filter((path) => path.endsWith('package.json')) + const pkgPaths = discoveredPaths.filter((path) => basename(path) === 'package.json') for (const pkgPath of pkgPaths) { const fullPath = joinPath(appRoot, pkgPath) diff --git a/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts b/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts new file mode 100644 index 00000000000..97dc31a7894 --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts @@ -0,0 +1,13 @@ +/** + * Whether a filesystem error means the path (or one of its ancestors) does not + * exist, as opposed to existing but being unreadable. + */ +export function isMissingFilesystemEntry(error: unknown): boolean { + return error instanceof Error && 'code' in error && (error.code === 'ENOENT' || error.code === 'ENOTDIR') +} + +/** A short, locale-independent reason for a failed filesystem inspection, carrying the error code when there is one. */ +export function inspectErrorReason(target: string, error: unknown): string { + const code = error instanceof Error && 'code' in error && typeof error.code === 'string' ? error.code : undefined + return code ? `Could not inspect ${target} (${code})` : `Could not inspect ${target}` +} 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 ee283f95aee..b8a93db72f4 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 @@ -10,7 +10,9 @@ import { findManifests, findManifestPaths, findDependencyAutomationInputs, + listRepositoryFiles, } from './discover.js' +import {buildPathRules, 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' @@ -161,7 +163,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray scanCommittedSecrets(context.sensitiveFiles, context.appRoot), + runner: (context) => scanCommittedSecrets(context.sensitiveFiles, context.appRoot, context.gitIgnoreListing), }, jsCheck('CREDENTIAL_LOG_LEAKAGE', (context) => scanCredentialLogLeakage(context.sourceFiles)), jsCheck('CREDENTIAL_BROWSER_LEAKAGE', (context) => scanCredentialBrowserLeakage(context.sourceFiles)), @@ -585,14 +587,20 @@ export async function scan(startPath?: string, configFileName?: string): Promise const selectedFileName = getAppConfigurationFileName(configFileName) const appToml = loadAppToml(joinPath(appRoot, selectedFileName), appRoot) const appTomls = appToml ? [appToml] : [] - const extensions = findExtensions(appRoot) - const sourceCandidates = findSourceCandidates(appRoot) - const sourceFiles = findAppSourceFiles(appRoot) - const sensitiveFiles = findSensitiveFiles(appRoot, selectedFileName) - const manifestPaths = findManifestPaths(appRoot) + // Path rules govern repository discovery only; the selected app configuration above is always loaded. + const gitIgnoreListing = await listGitIgnoredPaths(appRoot) + const pathRules = buildPathRules({ + gitIgnoredPaths: gitIgnoreListing.status === 'listed' ? gitIgnoreListing.paths : [], + }) + const repositoryFiles = listRepositoryFiles(appRoot, pathRules) + const extensions = findExtensions(appRoot, repositoryFiles) + const sourceCandidates = findSourceCandidates(repositoryFiles) + const sourceFiles = findAppSourceFiles(appRoot, repositoryFiles) + const sensitiveFiles = findSensitiveFiles(appRoot, repositoryFiles, selectedFileName) + const manifestPaths = findManifestPaths(repositoryFiles) const manifests = findManifests(appRoot, manifestPaths) const dependencyAutomation = manifests.some(manifestHasDependencies) - ? findDependencyAutomationInputs(appRoot) + ? findDependencyAutomationInputs(appRoot, pathRules) : {files: []} const capabilities = detectCapabilities(appToml, extensions, sourceFiles, appTomls) const detection = detectProject(manifests, extensions, sourceCandidates) @@ -608,6 +616,7 @@ export async function scan(startPath?: string, configFileName?: string): Promise capabilities, detection, sourceCandidates, + gitIgnoreListing: gitIgnoreListing.status, } let issues: Issue[] = [] 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 new file mode 100644 index 00000000000..c245a8865fe --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts @@ -0,0 +1,277 @@ +import {findRepositoryMarker} from './repository-marker.js' +import {outputDebug} from '@shopify/cli-kit/node/output' +import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' +import ignore from 'ignore' + +/** The scan's path rules: two exclusion phases, either of which excludes a path. */ +export interface PathRules { + /** .gitignore exclude patterns applied to every scan. */ + defaults: ReadonlyArray + /** Literal app-root-relative paths git reports as untracked and ignored; directories end with `/`. */ + gitIgnoredPaths: ReadonlyArray +} + +/** + * Decide whether an app-root-relative path is excluded from the scan. + * + * `relativePath` must be POSIX-separated, relative to the app root, with no + * `./` prefix and no trailing slash. Pass `directory: true` for directories so + * directory-only patterns (`build/`) can match them. + * + * PRECONDITION: the answer is only correct for paths whose ancestor directories + * the caller has already confirmed are NOT excluded. A collapsed git directory + * literal (`tmp/`) matches only the directory itself, never `tmp/a.ts`, so a + * walker must prune the directory rather than ask about its contents. A + * directory walker that never descends into excluded directories satisfies + * this naturally; do not use the matcher to test arbitrary deep paths. + */ +type PathMatcher = (relativePath: string, options: {directory: boolean}) => boolean + +/** + * Decide whether an app-root-relative FILE path is excluded from the scan, + * checking its ancestor directories itself. See `createFilePathMatcher`. + */ +type FilePathMatcher = (relativePath: string) => boolean + +/** + * Paths never worth scanning: build output, dependencies, caches, and test + * fixture trees, expressed in .gitignore syntax. A pattern without a slash + * matches at any depth; a trailing `/` matches directories only. + * + * Patterns are generic on purpose. Earlier versions hardcoded the names of + * this project's own fixture directories, which both leaked internal naming + * into a tool that ships to third-party developers and silently skipped any + * directory a developer happened to give the same name. + * + * The scan walks dot-folders, so generated dot-folders must be listed + * explicitly: framework build output (`.next/`, `.nuxt/`, ...), caches + * (`.cache/`, `.turbo/`), Yarn Berry's committed `.yarn/releases`, and the + * CLI-generated `.shopify/`. `.github/`, `.vscode/`, `.devcontainer/` and + * `.circleci/` are deliberately NOT excluded because hardcoded secrets turn up + * in workflow and editor configuration. + * + * `.git` has no trailing slash so it also matches the `.git` FILE that git + * worktrees use in place of a directory. + * + * Sub-apps (a nested directory with its own shopify.app.toml) are not a path + * rule: `listRepositoryFiles` in `discover.ts` treats them as a structural + * walker boundary, since that requires reading the tree rather than matching + * a name. + */ +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/', +] + +/** What asking git for the app's ignored paths produced. */ +export type GitIgnoreListing = + | {status: 'listed'; paths: string[]} + | {status: 'not-a-repository'} + | {status: 'app-root-ignored'} + | {status: 'failed'} + +/** + * Ask git which paths under `appRoot` are ignored and untracked. + * + * Listed paths are relative to `appRoot` with POSIX separators. A fully + * ignored directory is collapsed to a single `dir/` entry. Only UNTRACKED + * ignored paths appear: a tracked file that matches .gitignore is never + * reported, so a committed-then-ignored `.env` is still scanned. + * + * Every outcome other than `listed` means "no git-based exclusions": the + * caller scans everything the defaults allow. The outcomes are kept apart so + * later findings can say exactly why a git-ignored file was still scanned. + * + * - `not-a-repository`: `appRoot` is not inside a git working tree: no `.git` + * marker exists at or above it, or `appRoot` lies inside the `.git` + * directory itself. + * - `app-root-ignored`: an enclosing repository ignores the app folder (or an + * ancestor of it). That repository does not own the app, so its rules must + * not empty the scan. + * - `failed`: git is missing, refused to read a repository that does exist + * (dubious ownership, corrupt or invalid configuration), or a command failed + * for any other reason. + */ +export async function listGitIgnoredPaths(appRoot: string): Promise { + const listing = await runGitIgnoreListing(appRoot) + if (listing.status !== 'listed') outputDebug(`App Security: git ignore listing skipped (${listing.status})`) + return listing +} + +async function runGitIgnoreListing(appRoot: string): Promise { + // One call answers two questions. Verified with git 2.55, the output is two lines: `true` or + // `false`, then the app root's path relative to the repository's top level, which is an empty + // line at the top level itself (`true\n\n`; `true\napps/web/\n` from apps/web). Inside `.git` + // the first line is `false` with exit code 0; outside any repository the exit code is 128. + const location = await runGit(appRoot, ['rev-parse', '--is-inside-work-tree', '--show-prefix']) + if (location === undefined) return {status: 'failed'} + // A repository git refuses to read (dubious ownership under `safe.directory`, a corrupt config or + // HEAD) also exits 128, and only git's localized stderr tells the cases apart. The `.git` marker + // on disk does so independently of locale: a marker means git failed on a repository that exists. + if (location.exitCode !== 0) { + return findRepositoryMarker(appRoot).status === 'none' ? {status: 'not-a-repository'} : {status: 'failed'} + } + const [insideWorkTree, prefix] = location.stdout.split(/\r?\n/) + if (insideWorkTree === 'false') return {status: 'not-a-repository'} + // A missing git binary resolves with exit code 0 and empty output (captureOutputWithExitCode + // does not reject), so only an explicit `true` counts as being inside a working tree. + if (insideWorkTree !== 'true' || prefix === undefined) return {status: 'failed'} + + // Is the app folder itself ignored by the repository? `--no-index` is essential: without it git + // answers "not ignored" as soon as any descendant is force-tracked, and in exactly that case + // `ls-files` below no longer collapses the app to a single `./` entry but lists every untracked + // file in the app individually, which would exclude almost the entire app from the scan. With + // `--no-index` the answer follows the ignore rules alone, for the app folder and its ancestors. + // + // The probe is skipped at the top level (empty prefix): a repository cannot ignore its own top + // level, and `check-ignore .` normalises `.` there to the empty path, which a bare `*` pattern + // matches, so a whitelist-style root .gitignore (`*` then `!src/` ...) would misreport the app + // as ignored. From a subdirectory `.` is resolved to the prefix path, and the same whitelist + // (`*`, `!*/`) correctly answers `1` (not ignored) for a re-included folder. + 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'} + } + + const listed = await runGit(appRoot, [ + 'ls-files', + '-z', + '--others', + '--ignored', + '--exclude-standard', + '--directory', + '--', + '.', + ...defaultDirectoryPathspecExcludes(), + ]) + if (listed === undefined || listed.exitCode !== 0) return {status: 'failed'} + return {status: 'listed', paths: listed.stdout.split('\0').filter((path) => path !== '')} +} + +/** + * Keep git out of the default directories. `ls-files --others --ignored` + * walks every untracked directory that is NOT itself ignored (an unignored + * `node_modules/` with 63,000 files took 0.05 s to list here; 0.02 s with the + * exclusion below). Every default directory is always excluded by the walker, + * so git's view of the ignored paths inside one can never matter, and skipping + * them cannot change the scan. + * + * Each pathspec is `:(exclude,glob)` followed by the pattern wrapped in + * leading and trailing `**` segments, which matches the directory at the app + * root and at any depth; a wildcard name such as `*-fixtures/` keeps working. + * Only directory defaults (trailing `/`) become pathspecs; file patterns such + * as `*.test.*` and the `.git` entry do not name a directory git would + * traverse. + */ +function defaultDirectoryPathspecExcludes(): string[] { + return DEFAULT_EXCLUDE_PATTERNS.filter((pattern) => pattern.endsWith('/')).map( + (pattern) => `:(exclude,glob)**/${pattern}**`, + ) +} + +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} + // Defensive only: captureOutputWithExitCode does not reject on a non-zero exit, and a missing git + // binary resolves with empty output rather than throwing. Anything that does throw is treated + // as a failed listing: no git-based exclusions. + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return undefined + } +} + +/** + * Assemble the scan's path rules from the hardcoded defaults and the literal + * paths git reported as ignored. The two phases are independent: a path is + * excluded when either matches it. + */ +export function buildPathRules(input: {gitIgnoredPaths: ReadonlyArray}): PathRules { + return {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: input.gitIgnoredPaths} +} + +/** + * Compile the rules into a matcher: a path is excluded when a git literal + * names it exactly or a default pattern matches it. + * + * Git literals (possibly thousands of them) are looked up in a Set rather than + * compiled into patterns, both for speed and so that a name containing + * gitignore-significant characters (`[id].ts`, `#hash.ts`) matches literally. + * + * `ignorecase: false` matches the case-sensitive fast-glob discovery this + * replaced and makes results identical on every platform. + * + * `allowRelativePaths: true` stops `ignore` from throwing on a name made only + * of dots (`...` is a legal POSIX filename); callers already guarantee there is + * no `./` prefix, which is the case the check exists for. + * + * `ignore` is a CommonJS module whose typings describe an ESM default export, + * so under NodeNext the factory is reached through `.default` (as in + * `dev/app-events/file-watcher.ts`). + */ +export function createPathMatcher(rules: PathRules): PathMatcher { + const defaults = ignore.default({ignorecase: false, allowRelativePaths: true}).add([...rules.defaults]) + const gitIgnored = new Set(rules.gitIgnoredPaths) + + return (relativePath, {directory}) => { + const key = directory ? `${relativePath}/` : relativePath + return gitIgnored.has(key) || defaults.ignores(key) + } +} + +/** + * Compile the rules into a matcher for a FILE path the caller already knows + * rather than reached by walking, such as an allowlisted configuration path. + * + * `createPathMatcher` is only correct once every ancestor directory is known + * to be included, so this matcher establishes that itself: it asks about each + * ancestor directory top-down first (an excluded ancestor excludes the file, + * exactly as the walker would have pruned it), then about the file. The path + * must meet `createPathMatcher`'s format: POSIX-separated, app-root-relative, + * no `./` prefix and no trailing slash. + * + * This deliberately diverges from the walker for a symlinked directory. The + * walker uses lstat semantics, so it tests a symlinked `.github` as a FILE + * (`.github`); this matcher tests every ancestor as a directory (`.github/`). + * A gitignored symlinked folder (reported by git as the file literal `.github`) + * is therefore NOT excluded here, and its configuration surfaces as + * unresolved instead, as documented on `findDependencyAutomationInputs`. + */ +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}) + } +} diff --git a/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts b/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts new file mode 100644 index 00000000000..866659c774e --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts @@ -0,0 +1,40 @@ +import {inspectErrorReason, isMissingFilesystemEntry} from './filesystem-errors.js' +import {dirname, joinPath} from '@shopify/cli-kit/node/path' +import {lstatSync} from 'node:fs' + +/** + * The nearest `.git` entry at or above a directory. + * + * - `found`: a `.git` directory or worktree file exists in `directory`, which + * is the start directory itself or one of its ancestors. + * - `ambiguous`: the nearest `.git` entry is a symlink or special file, or + * could not be inspected. Following it would leave repository ownership + * unclear, so callers must not treat it as absent. + * - `none`: no `.git` entry exists at any level. + */ +type RepositoryMarker = {status: 'found'; directory: string} | {status: 'ambiguous'; reason: string} | {status: 'none'} + +/** + * Walk from `start` up to the filesystem root and classify the first `.git` + * entry met. Uses lstat so a symlinked `.git` is reported as ambiguous rather + * than followed. The search is purely filesystem-based and locale-independent, + * which lets callers tell "no repository here" apart from "git refused to read + * this repository" without parsing git's localized messages. + */ +export function findRepositoryMarker(start: string): RepositoryMarker { + let directory = start + while (true) { + try { + const stats = lstatSync(joinPath(directory, '.git')) + if (stats.isDirectory() || stats.isFile()) return {status: 'found', directory} + return {status: 'ambiguous', reason: 'Could not determine repository ownership from .git'} + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + if (!isMissingFilesystemEntry(error)) return {status: 'ambiguous', reason: inspectErrorReason('.git', error)} + } + + const parent = dirname(directory) + if (parent === directory) return {status: 'none'} + directory = parent + } +} 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 6bf63cf05e3..f6c2212fb76 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 @@ -6,6 +6,7 @@ import { resetSkippedFiles, } from '../scanners/discover.js' import {DEPENDENCY_AUTOMATION_CONFIG_PATHS} from '../rules/dependency-automation-rules.js' +import {buildPathRules} from '../scanners/path-rules.js' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' import {afterEach, describe, expect, test} from 'vitest' import {execFileSync} from 'node:child_process' @@ -14,6 +15,9 @@ import {dirname, join} from 'node:path' afterEach(() => resetSkippedFiles()) +/** The defaults alone, as the scan's rules are when git reported no ignored paths. */ +const NO_GIT_EXCLUSIONS = buildPathRules({gitIgnoredPaths: []}) + async function writeFiles(root: string, files: Record): Promise { await Promise.all( Object.entries(files).map(async ([path, content]) => { @@ -26,7 +30,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)).toEqual({files: []}) + expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS)).toEqual({files: []}) const content = 'version: 2\r\nupdates: []\r\n' await writeFiles(root, { '.github/dependabot.yml': content, @@ -36,7 +40,7 @@ describe('dependency automation discovery', () => { '.circleci/config.yml': 'jobs: [', '.snyk': 'version: v1.25.0', }) - const result = findDependencyAutomationInputs(root) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result.unresolvedReason).toBeUndefined() expect(result.files.map(({path}) => path)).toEqual(['.github/dependabot.yml']) expect(result.files[0]?.content).toBe(content) @@ -46,7 +50,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) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result.files).toMatchObject([{path, content: '{}'}]) expect(result.unresolvedReason).toBeUndefined() }) @@ -55,15 +59,72 @@ 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).files).toMatchObject([{path: 'renovate.json'}]) + expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).files).toMatchObject([{path: 'renovate.json'}]) expect(getSkippedFiles()).toEqual([]) }) }) + describe('path rules', () => { + test('treats a configuration file the rules exclude like a missing one', async () => { + await inTemporaryDirectory(async (root) => { + await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) + const rules = buildPathRules({gitIgnoredPaths: ['.github/dependabot.yml']}) + expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) + expect(getSkippedFiles()).toEqual([]) + }) + }) + + test('excludes a configuration file inside a directory the rules exclude', async () => { + // Git collapses a fully ignored directory to `.github/`, which never names the file itself, so the + // decision must consider the file's ancestors. + await inTemporaryDirectory(async (root) => { + await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) + const rules = buildPathRules({gitIgnoredPaths: ['.github/']}) + expect(findDependencyAutomationInputs(root, rules)).toEqual({files: []}) + }) + }) + + test('continues to a later allowlisted file when an earlier one is excluded', async () => { + await inTemporaryDirectory(async (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) + 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 (`.git` does not match `.github`), so a custom + // default proves the phase is consulted at all. + await inTemporaryDirectory(async (root) => { + await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) + expect(findDependencyAutomationInputs(root, {defaults: ['.github/'], gitIgnoredPaths: []})).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 () => { + await inTemporaryDirectory(async (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') + }) + }) + }) + 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)).toMatchObject({ + expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS)).toMatchObject({ files: [], unresolvedReason: expect.stringContaining('too large'), }) @@ -79,7 +140,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) + const nested = findDependencyAutomationInputs(app, NO_GIT_EXCLUSIONS) expect(nested).toMatchObject({ files: [], unresolvedReason: 'App root is nested below a parent Git repository', @@ -87,7 +148,9 @@ 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).files).toMatchObject([{path: '.github/dependabot.yml'}]) + expect(findDependencyAutomationInputs(app, NO_GIT_EXCLUSIONS).files).toMatchObject([ + {path: '.github/dependabot.yml'}, + ]) }) }) @@ -95,7 +158,7 @@ describe('dependency automation discovery', () => { await inTemporaryDirectory(async (root) => { await inTemporaryDirectory(async (outside) => { await symlink(outside, join(root, path), 'dir') - const result = findDependencyAutomationInputs(root) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result).toMatchObject({ files: [], unresolvedReason: expect.any(String), @@ -109,7 +172,7 @@ describe('dependency automation discovery', () => { test('rejects dangling directory links', async () => { await inTemporaryDirectory(async (root) => { await symlink(join(root, 'missing'), join(root, '.github'), 'dir') - const result = findDependencyAutomationInputs(root) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result.unresolvedReason).toContain('dangling symbolic link') expect(result.unresolvedReason).not.toContain(root) expect(getSkippedFiles()).toContainEqual( @@ -123,7 +186,7 @@ describe('dependency automation discovery', () => { await inTemporaryDirectory(async (outside) => { await symlink(outside, join(root, '.github'), 'dir') await writeFiles(root, {'renovate.json': '{}'}) - const result = findDependencyAutomationInputs(root) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result.files).toMatchObject([{path: 'renovate.json', content: '{}'}]) expect(result.unresolvedReason).toBeUndefined() expect(getSkippedFiles()).toContainEqual( @@ -138,14 +201,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) + const rejected = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(rejected.unresolvedReason).toContain('outside the app root') 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) + const result = findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS) expect(result.files).toMatchObject([{path: '.github/dependabot.yml'}]) expect(result.unresolvedReason).toBeUndefined() }) @@ -155,9 +218,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).unresolvedReason).toContain('not a file') + expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).unresolvedReason).toContain('not a file') execFileSync('mkfifo', [join(root, '.git')]) - expect(findDependencyAutomationInputs(root).unresolvedReason).toContain('repository ownership') + expect(findDependencyAutomationInputs(root, NO_GIT_EXCLUSIONS).unresolvedReason).toContain('repository ownership') }) }) }) @@ -179,7 +242,7 @@ describe('manifest safety', () => { async (content) => { await inTemporaryDirectory(async (root) => { await writeFile(join(root, 'package.json'), content) - expect(findManifests(root)).toMatchObject([{dependencies: {}, devDependencies: {}}]) + expect(findManifests(root, ['package.json'])).toMatchObject([{dependencies: {}, devDependencies: {}}]) expect(getSkippedFiles()).toEqual([ expect.objectContaining({reason: 'unreadable', detail: 'manifest could not be parsed'}), ]) @@ -187,11 +250,20 @@ describe('manifest safety', () => { }, ) + test('only reads files named exactly package.json', async () => { + 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"}}') + expect(findManifests(root, ['my-package.json', 'package.json'])).toMatchObject([{path: 'package.json'}]) + expect(getSkippedFiles()).toEqual([]) + }) + }) + 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"]}}') - expect(findManifests(root)).toMatchObject([{dependencies: {}, devDependencies: {}}]) - expect(findManifests(root)[0]).not.toHaveProperty('scripts') + 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 1acb8dcb6ec..a994206b6d8 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 @@ -1,4 +1,5 @@ /* eslint-disable no-restricted-imports -- integration coverage uses real temporary repositories */ +import {git, isolateGitConfig} from './git-test-helpers.js' import {securityExitCode} from '../../app-security-api.js' import {formatJson} from '../output/format.js' import {getRegistry} from '../registry/index.js' @@ -9,7 +10,7 @@ import {scanApp} from '../run.js' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' import {fetch} from '@shopify/cli-kit/node/http' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' -import {describe, expect, test, vi} from 'vitest' +import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import {mkdir, unlink, writeFile} from 'node:fs/promises' import {dirname, join} from 'node:path' @@ -25,6 +26,15 @@ vi.mock('@shopify/cli-kit/node/system', async (importActual) => { const checkId = 'MISSING_DEPENDENCY_SECURITY_AUTOMATION' const dependabot = '# Configuration contents are not validated.\n' +// Keep the scan's git calls independent of the developer's global excludes and any enclosing repository. +let restoreGitConfig: (() => void) | undefined +beforeEach(() => { + restoreGitConfig = isolateGitConfig() +}) +afterEach(() => { + restoreGitConfig?.() +}) + async function writeFiles(root: string, files: Record): Promise { await Promise.all( Object.entries(files).map(async ([path, content]) => { @@ -130,7 +140,10 @@ describe('dependency automation scanner integration', () => { const submission = buildSubmission(execution.trace, {cliVersion: '3.99.0', submittedAt: '2026-09-15T00:00:00Z'}) expect(JSON.stringify(submission)).not.toContain('local>org/renovate-config') expect(JSON.stringify(execution.trace)).not.toContain('local>org/renovate-config') + // The scan's own git calls (ignored-path discovery and project metadata) are the only ones allowed. + // The temporary directory is outside any repository, so ignored-path discovery stops at its first probe. expect(vi.mocked(captureOutputWithExitCode).mock.calls.map(([command, args]) => [command, args])).toEqual([ + ['git', ['rev-parse', '--is-inside-work-tree', '--show-prefix']], ['git', ['rev-parse', 'HEAD']], ['git', ['status', '--porcelain']], ]) @@ -152,7 +165,63 @@ describe('dependency automation scanner integration', () => { const result = await scan(root) expect(dependencyFindings(result)).toHaveLength(1) expect(dependencyExecution(result)).toMatchObject({status: 'executed', inspected_files: ['package.json']}) - expect(result.scan.input_hash).toBe(initial.scan.input_hash) + // Workflow files are scanned for secrets (so the scan inputs change) but never feed this check. + expect(dependencyExecution(result)).toEqual(dependencyExecution(initial)) + }) + }) + + describe('gitignored configuration', () => { + // Hosted bots read the repository, so a configuration file that never reaches it configures nothing. + 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']) + } + + 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']) + const result = await scan(root) + 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') + expect(result.scan.coverage_complete).toBe(true) + }) + }) + + test('reads a force-tracked configuration file that matches .gitignore', async () => { + await inTemporaryDirectory(async (root) => { + await makeRepository(root, {'.gitignore': '.github/\n', '.github/dependabot.yml': dependabot}, [ + '.gitignore', + '.github/dependabot.yml', + ]) + const result = await scan(root) + 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('skips an ignored configuration file and still counts a committed one later in the allowlist', async () => { + await inTemporaryDirectory(async (root) => { + await makeRepository( + root, + {'.gitignore': '.github/dependabot.yml\n', '.github/dependabot.yml': dependabot, 'renovate.json': '{}'}, + ['.gitignore', 'renovate.json'], + ) + const result = await scan(root) + expect(dependencyFindings(result)).toEqual([]) + expect(dependencyExecution(result)).toMatchObject({ + status: 'executed', + inspected_files: ['package.json', 'renovate.json'], + }) + expect(result.scan.file_hashes).not.toHaveProperty('.github/dependabot.yml') + expect(result.scan.file_hashes?.['renovate.json']).toBe(sha256('{}')) + }) }) }) 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 d4ad5386e60..587ad104112 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 @@ -1,14 +1,28 @@ /* eslint-disable no-restricted-imports -- discovery boundaries use real temporary repositories */ -import {AppRootDiscoveryError, findAppRoot} from '../scanners/discover.js' +import {git, isolateGitConfig} from './git-test-helpers.js' +import { + AppRootDiscoveryError, + findAppRoot, + findExtensions, + findSourceCandidates, + getSkippedFiles, + listRepositoryFiles, + resetSkippedFiles, +} from '../scanners/discover.js' import {scan} from '../scanners/index.js' +import {buildPathRules, listGitIgnoredPaths} from '../scanners/path-rules.js' import {normalizePath} from '@shopify/cli-kit/node/path' -import {afterEach, describe, expect, test} from 'vitest' -import {mkdir, mkdtemp, rm, writeFile} from 'node:fs/promises' +import {afterEach, beforeEach, describe, expect, test} from 'vitest' +import {chmod, mkdir, mkdtemp, 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' +/** The rules a scan uses when git reports no ignored paths. */ +const DEFAULT_RULES = buildPathRules({gitIgnoredPaths: []}) afterEach(async () => { await Promise.all(temporaryDirectories.splice(0).map((directory) => rm(directory, {recursive: true, force: true}))) @@ -30,6 +44,35 @@ async function writeFiles(root: string, files: Record): Promise< ) } +async function makeRepository(files: Record): Promise { + const root = await makeDirectory() + git(root, ['init', '-q', '.']) + await writeFiles(root, files) + return root +} + +function hashedPaths(result: ScanResult): string[] { + return Object.keys(result.scan.file_hashes ?? {}) +} + +function secretFindingFiles(result: ScanResult): string[] { + return result.issues.filter((issue) => issue.id === 'COMMITTED_SECRET').map((issue) => issue.location.file) +} + +/** Paths the dependency check inspected, which is where package manifests surface in a scan result. */ +function inspectedManifestPaths(result: ScanResult): string[] { + const execution = result.scan.checks_executed.find( + (candidate) => candidate.id === 'MISSING_DEPENDENCY_SECURITY_AUTOMATION', + ) + return execution?.inspected_files.filter((path) => path.endsWith('package.json')) ?? [] +} + +/** Rules exactly as `scan()` builds them, so direct finder tests see the same file list. */ +async function scanPathRules(appRoot: string): Promise { + const listing = await listGitIgnoredPaths(appRoot) + return buildPathRules({gitIgnoredPaths: listing.status === 'listed' ? listing.paths : []}) +} + describe.sequential('app root discovery', () => { test('walks up from explicit and current subdirectories and accepts an explicit TOML', async () => { const root = await makeDirectory() @@ -78,6 +121,15 @@ describe.sequential('app root discovery', () => { }) describe('repository discovery exclusions', () => { + // scan() asks git for ignored paths, so keep results independent of the developer's git configuration. + let restoreGitConfig: (() => void) | undefined + beforeEach(() => { + restoreGitConfig = isolateGitConfig() + }) + afterEach(() => { + restoreGitConfig?.() + }) + test('excludes every nested app input from its parent monorepo scan', async () => { const root = await makeDirectory() const secret = ['AKIA', 'IOSFODNN7EXAMPLE'].join('') @@ -101,12 +153,30 @@ describe('repository discovery exclusions', () => { expect(result.issues.some((issue) => issue.location.file.startsWith('apps/child/'))).toBe(false) }) - test('recursively excludes dependency, VCS, coverage, and build directories', async () => { + test('recursively excludes dependency, VCS, coverage, build, and test directories', async () => { const root = await makeDirectory() - const ignoredDirectories = ['node_modules', 'vendor', '.git', '.next', 'coverage', 'dist', 'build'] + const ignoredDirectories = [ + 'node_modules', + 'vendor', + '.git', + '.next', + 'coverage', + 'dist', + 'build', + 'test', + 'tests', + 'spec', + 'specs', + '__tests__', + 'fixtures', + 'x-fixtures', + '__fixtures__', + ] 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`, @@ -116,10 +186,65 @@ describe('repository discovery exclusions', () => { }) const result = await scan(root) - const paths = Object.keys(result.scan.file_hashes ?? {}) + const paths = hashedPaths(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') + }) + + 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`, + '.vscode/settings.json': '{"editor.tabSize": 2}', + '.eslintrc.cjs': 'module.exports = {}', + }) + + const result = await scan(root) + const paths = hashedPaths(result) + expect(secretFindingFiles(result)).toEqual(['.github/workflows/deploy.yml']) + expect(paths).toContain('.vscode/settings.json') + expect(paths).toContain('.eslintrc.cjs') + + const candidates = findSourceCandidates(listRepositoryFiles(root, DEFAULT_RULES)) + 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, { + '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']), + ), + }) + + const result = await scan(root) + const paths = hashedPaths(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 () => { @@ -161,4 +286,355 @@ describe('repository discovery exclusions', () => { ]), ) }) + + test('assigns source files to every extension directory that contains them, in repository order', async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + // A root-level extension configuration owns every source file in the repository. + 'shopify.extension.toml': 'type = "theme"\n', + 'index.ts': 'export const root = true', + 'extensions/alpha/shopify.extension.toml': 'type = "ui_extension"\n', + 'extensions/alpha/src/index.tsx': 'export const alpha = true', + 'extensions/alpha/nested/shopify.extension.toml': 'type = "ui_extension"\n', + 'extensions/alpha/nested/src/index.js': 'export const nested = true', + 'extensions/alpha/nested/README.md': 'not source', + 'extensions/alpha-two/shopify.extension.toml': 'type = "ui_extension"\n', + 'extensions/alpha-two/src/index.ts': 'export const alphaTwo = true', + 'extensions/beta/src/index.ts': 'export const noToml = true', + }) + + const extensions = findExtensions(root, listRepositoryFiles(root, DEFAULT_RULES)) + + 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']], + ['extensions/alpha/nested/shopify.extension.toml', ['extensions/alpha/nested/src/index.js']], + [ + 'extensions/alpha/shopify.extension.toml', + ['extensions/alpha/nested/src/index.js', 'extensions/alpha/src/index.tsx'], + ], + [ + 'shopify.extension.toml', + [ + 'extensions/alpha-two/src/index.ts', + 'extensions/alpha/nested/src/index.js', + 'extensions/alpha/src/index.tsx', + 'extensions/beta/src/index.ts', + 'index.ts', + ], + ], + ]) + expect(extensions[2]).toMatchObject({ + path: 'extensions/alpha/shopify.extension.toml', + type: 'ui_extension', + content: 'type = "ui_extension"\n', + }) + expect(extensions[2]!.files[1]).toEqual({ + path: 'extensions/alpha/src/index.tsx', + absolutePath: join(root, 'extensions/alpha/src/index.tsx'), + ext: '.tsx', + content: 'export const alpha = true', + }) + }) +}) + +describe('gitignore-driven exclusions', () => { + let restoreGitConfig: (() => void) | undefined + beforeEach(() => { + restoreGitConfig = isolateGitConfig() + }) + afterEach(() => { + restoreGitConfig?.() + }) + + test('excludes gitignored directories', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'tmp/\ngenerated/\n', + 'src/index.ts': 'export const included = true', + 'tmp/scratch.ts': 'export const ignored = true', + 'generated/client.ts': 'export const ignored = true', + }) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('src/index.ts') + expect(paths).not.toContain('tmp/scratch.ts') + expect(paths).not.toContain('generated/client.ts') + }) + + test('neither reports nor hashes 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', + 'notes.txt': `key=${secret}\n`, + 'other.txt': `key=${secret}\n`, + }) + + const result = await scan(root) + // The control file proves the secret is detectable, so the absence for notes.txt is not vacuous. + expect(secretFindingFiles(result)).toEqual(['other.txt']) + const paths = hashedPaths(result) + expect(paths).toContain('other.txt') + expect(paths).not.toContain('notes.txt') + }) + + test('still scans a tracked file that matches .gitignore', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'config/\n', + 'config/tracked.ts': 'export const tracked = true', + 'config/untracked.ts': 'export const untracked = true', + }) + git(root, ['add', '-f', 'config/tracked.ts']) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('config/tracked.ts') + expect(paths).not.toContain('config/untracked.ts') + }) + + 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', + '.env': `SHOPIFY_TOKEN=${secret}\n`, + '.env.example': 'SHOPIFY_TOKEN=\n', + }) + + const result = await scan(root) + const paths = hashedPaths(result) + expect(paths).toContain('.env.example') + expect(paths).not.toContain('.env') + expect(secretFindingFiles(result)).toEqual([]) + }) + + test('honours a nested .gitignore', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + 'packages/api/.gitignore': 'local.ts\n', + 'packages/api/local.ts': 'export const ignored = true', + 'packages/api/index.ts': 'export const included = true', + 'local.ts': 'export const included = true', + }) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('packages/api/index.ts') + expect(paths).toContain('local.ts') + expect(paths).not.toContain('packages/api/local.ts') + }) + + test('excludes gitignored files from an extension and from the scan hashes', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'extensions/foo/generated.js\n', + 'extensions/foo/shopify.extension.toml': 'type = "theme"\n', + 'extensions/foo/generated.js': 'export const ignored = true', + 'extensions/foo/index.js': 'export const included = true', + }) + + const extensions = findExtensions(root, listRepositoryFiles(root, await scanPathRules(root))) + expect(extensions.map((extension) => extension.files.map((file) => file.path))).toEqual([ + ['extensions/foo/index.js'], + ]) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('extensions/foo/index.js') + expect(paths).not.toContain('extensions/foo/generated.js') + }) + + test('treats gitignored paths literally even when their names are gitignore-significant', async () => { + // Glob brackets, comment `#`, negation `!` and whitespace all mean something in .gitignore + // syntax; the names are also legal on Windows, unlike `*` or `?`. + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': '\\[id\\].ts\n\\#hash.ts\n\\!bang.ts\nsp ace.ts\n', + '[id].ts': 'export const ignored = true', + 'id.ts': 'export const included = true', + '#hash.ts': 'export const ignored = true', + 'hash.ts': 'export const included = true', + '!bang.ts': 'export const ignored = true', + 'bang.ts': 'export const included = true', + 'sp ace.ts': 'export const ignored = true', + 'space.ts': 'export const included = true', + }) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('id.ts') + expect(paths).toContain('hash.ts') + expect(paths).toContain('bang.ts') + expect(paths).toContain('space.ts') + expect(paths).not.toContain('[id].ts') + expect(paths).not.toContain('#hash.ts') + expect(paths).not.toContain('!bang.ts') + expect(paths).not.toContain('sp ace.ts') + }) + + test('applies the enclosing repository .gitignore to an app in a subfolder', async () => { + const repository = await makeRepository({ + '.gitignore': 'scratch/\n', + 'apps/web/shopify.app.toml': appConfiguration, + 'apps/web/src/index.ts': 'export const included = true', + 'apps/web/scratch/notes.ts': 'export const ignored = true', + }) + + const paths = hashedPaths(await scan(join(repository, 'apps', 'web'))) + expect(paths).toContain('src/index.ts') + expect(paths).not.toContain('scratch/notes.ts') + }) + + test('honours .git/info/exclude', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.git/info/exclude': 'private/\n', + 'private/keys.ts': 'export const ignored = true', + 'src/index.ts': 'export const included = true', + }) + + const paths = hashedPaths(await scan(root)) + expect(paths).toContain('src/index.ts') + 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 () => { + // Without the force-tracked README git would collapse the app to `./`; with it, git lists each + // untracked file individually, and treating those as exclusions would empty the scan. + 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 = hashedPaths(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({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'tmp/\nlegacy/\n', + 'package.json': manifest, + 'tmp/package.json': manifest, + 'legacy/package.json': manifest, + 'legacy/untracked.json': '{}', + }) + git(root, ['add', '-f', 'legacy/package.json']) + + const result = await scan(root) + expect(inspectedManifestPaths(result)).toEqual(['legacy/package.json', 'package.json']) + const paths = hashedPaths(result) + expect(paths).toContain('legacy/package.json') + expect(paths).not.toContain('tmp/package.json') + }) + + test('ignores nothing from a .gitignore outside a git repository', async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + '.gitignore': 'tmp/\n', + 'tmp/scratch.ts': 'export const scanned = true', + }) + + expect(hashedPaths(await scan(root))).toContain('tmp/scratch.ts') + }) + + test('loads a gitignored selected app configuration file', async () => { + const root = await makeRepository({ + 'shopify.app.toml': appConfiguration, + '.gitignore': 'shopify.app.staging.toml\n', + 'shopify.app.staging.toml': 'name = "Staging"\napplication_url = "https://staging.example.com"\n', + }) + + const result = await scan(root, 'staging') + expect(result.app.name).toBe('Staging') + expect(hashedPaths(result)).toContain('shopify.app.staging.toml') + }) +}) + +describe('listRepositoryFiles', () => { + test('lists a symlinked 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']) + }) + + test('returns sorted app-root-relative POSIX paths', async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'z.ts': '', + 'b/d.ts': '', + 'a.ts': '', + 'b/c.ts': '', + 'b/a/e.ts': '', + }) + + const files = listRepositoryFiles(root, DEFAULT_RULES) + expect(files).toEqual(['a.ts', 'b/a/e.ts', 'b/c.ts', 'b/d.ts', 'z.ts']) + expect(files).toEqual([...files].sort()) + }) + + test.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'records an unreadable directory as skipped and keeps walking', + async () => { + const root = await makeDirectory() + await writeFiles(root, { + 'shopify.app.toml': appConfiguration, + 'locked/secret.ts': 'export const hidden = true', + 'src/index.ts': 'export const included = true', + }) + const locked = join(root, 'locked') + await chmod(locked, 0o000) + try { + resetSkippedFiles() + expect(listRepositoryFiles(root, DEFAULT_RULES)).toEqual(['shopify.app.toml', 'src/index.ts']) + expect(getSkippedFiles()).toEqual([ + {path: 'locked', reason: 'unreadable', detail: 'Could not inspect locked (EACCES)'}, + ]) + } finally { + await chmod(locked, 0o755) + } + }, + ) + + test.skipIf(process.platform === 'win32' || process.getuid?.() === 0)( + 'names the app root 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([]) + expect(getSkippedFiles()).toEqual([ + {path: root, reason: 'unreadable', detail: 'Could not inspect app root (EACCES)'}, + ]) + } finally { + await chmod(root, 0o755) + } + }, + ) }) diff --git a/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts b/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts new file mode 100644 index 00000000000..89613877daa --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts @@ -0,0 +1,69 @@ +/* eslint-disable no-restricted-imports -- test helpers drive a real git binary against real temporary repositories */ +import {vi} from 'vitest' +import {execFileSync} from 'node:child_process' +import {mkdtempSync, realpathSync, rmSync, writeFileSync} from 'node:fs' +import {tmpdir} from 'node:os' +import {dirname, join} from 'node:path' + +/** + * Run a git command in `directory` and return its stdout. + * + * Author and committer identity are pinned so commits work on machines (and CI + * runners) that have no global git identity configured. stderr is piped so a + * failing fixture command surfaces git's own message in the thrown error. + */ +export function git(directory: string, args: string[]): string { + return execFileSync('git', args, { + cwd: directory, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { + ...process.env, + GIT_AUTHOR_NAME: 't', + GIT_AUTHOR_EMAIL: 't@t', + GIT_COMMITTER_NAME: 't', + GIT_COMMITTER_EMAIL: 't@t', + }, + }) +} + +/** + * Make every git invocation in the current test independent of the developer + * machine: both the test's own `git()` calls and the production code's git + * calls (which inherit `process.env`). + * + * A developer's global `core.excludesFile`, `$XDG_CONFIG_HOME/git/ignore` + * (read whenever `core.excludesFile` is unset, which is exactly the state this + * helper creates) or system config would otherwise silently change which paths + * git reports as ignored, and a temporary directory could be discovered as + * part of an enclosing repository (for example a home-directory dotfiles repo). + * + * A dedicated temporary directory is created under `os.tmpdir()` holding an + * empty `gitconfig` (an empty file rather than `/dev/null` so the helper also + * works on Windows). The same directory becomes `XDG_CONFIG_HOME`; it contains + * no `git/ignore`, and `GIT_CONFIG_GLOBAL` already overrides its `git/config`. + * `os.tmpdir()` becomes `GIT_CEILING_DIRECTORIES`, so repositories under test + * must live somewhere below `os.tmpdir()` (as `mkdtemp` places them) to remain + * discoverable; git stops climbing once it reaches the ceiling itself. The + * ceiling is compared against git's RESOLVED working directory, so it is set + * from the real path: on macOS `os.tmpdir()` is `/var/...` but git sees + * `/private/var/...`, and on Windows the temp path may use an 8.3 short name. + * + * Returns a cleanup function that restores the environment (all stubs, via + * `vi.unstubAllEnvs()`) and removes the directory. Call it in `afterEach`; the + * vitest config does not enable `unstubEnvs`. + */ +export function isolateGitConfig(): () => void { + const directory = mkdtempSync(join(tmpdir(), 'app-security-gitconfig-')) + const globalConfig = join(directory, 'gitconfig') + writeFileSync(globalConfig, '') + vi.stubEnv('GIT_CONFIG_GLOBAL', globalConfig) + vi.stubEnv('GIT_CONFIG_NOSYSTEM', '1') + vi.stubEnv('GIT_CEILING_DIRECTORIES', realpathSync.native(dirname(directory))) + vi.stubEnv('XDG_CONFIG_HOME', directory) + + return () => { + vi.unstubAllEnvs() + rmSync(directory, {recursive: true, force: true}) + } +} 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 new file mode 100644 index 00000000000..211f0baa319 --- /dev/null +++ b/packages/app/src/cli/services/app-security-engine/tests/path-rules.test.ts @@ -0,0 +1,531 @@ +/* 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, + listGitIgnoredPaths, +} from '../scanners/path-rules.js' +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' + +const temporaryDirectories: string[] = [] +let restoreGitConfig: (() => void) | undefined + +beforeEach(() => { + restoreGitConfig = isolateGitConfig() +}) + +afterEach(() => { + restoreGitConfig?.() + for (const directory of temporaryDirectories.splice(0)) rmSync(directory, {recursive: true, force: true}) +}) + +function makeDirectory(prefix = 'app-security-path-rules-'): string { + const directory = mkdtempSync(join(tmpdir(), prefix)) + temporaryDirectories.push(directory) + return directory +} + +function writeFiles(root: string, files: Record): void { + for (const [path, content] of Object.entries(files)) { + const fullPath = join(root, path) + mkdirSync(join(fullPath, '..'), {recursive: true}) + writeFileSync(fullPath, content) + } +} + +function makeRepository(files: Record): string { + const root = makeDirectory() + git(root, ['init', '-q', '.']) + writeFiles(root, files) + return root +} + +/** Only the defaults, as the matcher sees them when git reported nothing. */ +const DEFAULTS_ONLY: PathRules = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: []} + +/** Only git literals, so a test can prove literal matching without a default getting in the way. */ +const gitIgnoredOnly = (paths: string[]): PathRules => ({defaults: [], gitIgnoredPaths: paths}) + +async function listedPaths(appRoot: string): Promise { + const listing = await listGitIgnoredPaths(appRoot) + expect(listing.status).toBe('listed') + return listing.status === 'listed' ? listing.paths : [] +} + +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/']}) + }) + + test('reports an ignored single file', async () => { + const root = makeRepository({'.gitignore': 'notes.txt\n', 'notes.txt': 'todo', 'src/index.ts': ''}) + + await expect(listedPaths(root)).resolves.toEqual(['notes.txt']) + }) + + test('does not report a tracked file that matches .gitignore', async () => { + // The classic leak: commit .env, then gitignore it. It must still be scanned. + const root = makeRepository({'.gitignore': '.env\n', '.env': 'SECRET=1\n'}) + git(root, ['add', '-f', '.env']) + git(root, ['commit', '-qm', 'oops']) + + await expect(listedPaths(root)).resolves.toEqual([]) + }) + + 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 () => { + const root = makeRepository({ + 'web/.gitignore': 'generated/\n', + 'web/generated/schema.ts': '', + 'web/src/index.ts': '', + }) + + await expect(listedPaths(root)).resolves.toEqual(['web/generated/']) + }) + + test('honours .git/info/exclude', async () => { + const root = makeRepository({'scratch.ts': '', 'src/index.ts': ''}) + writeFiles(root, {'.git/info/exclude': 'scratch.ts\n'}) + + 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 () => { + const repository = makeRepository({ + '.gitignore': 'tmp/\n*.log\n', + 'apps/my-app/shopify.app.toml': '', + 'apps/my-app/tmp/a.ts': '', + 'apps/my-app/debug.log': '', + 'apps/my-app/src/index.ts': '', + 'tmp/outside.ts': '', + }) + + const paths = await listedPaths(join(repository, 'apps', 'my-app')) + + expect(paths.sort()).toEqual(['debug.log', 'tmp/']) + }) + + test('reports gitignore-significant filenames literally', async () => { + // Names that .gitignore syntax treats specially (glob brackets, comment `#`, negation `!`, + // whitespace) yet remain legal filenames on every platform, including Windows. + const root = makeRepository({ + '.gitignore': '\\[id\\].ts\n\\#hash.ts\n\\!bang.ts\nsp ace.ts\n', + '[id].ts': '', + 'id.ts': '', + '#hash.ts': '', + 'hash.ts': '', + '!bang.ts': '', + 'bang.ts': '', + 'sp ace.ts': '', + 'space.ts': '', + }) + + const paths = await listedPaths(root) + + expect(paths.sort()).toEqual(['!bang.ts', '#hash.ts', '[id].ts', 'sp ace.ts']) + }) + + test.skipIf(process.platform === 'win32')( + 'reports an ignored symlinked directory as a file literal, without a trailing slash', + async () => { + // git never follows a symlink while listing, so the link is a blob to it and is not collapsed + // to a `dir/` entry. `createFilePathMatcher` relies on this when it documents that a + // gitignored symlinked `.github` is not excluded by the `.github/` ancestor check. + const root = makeRepository({'.gitignore': '.github\n', 'real/dependabot.yml': ''}) + symlinkSync(join(root, 'real'), join(root, '.github'), 'dir') + + await expect(listedPaths(root)).resolves.toEqual(['.github']) + expect(createFilePathMatcher(gitIgnoredOnly(['.github']))('.github/dependabot.yml')).toBe(false) + }, + ) + + test('is isolated from $XDG_CONFIG_HOME/git/ignore by isolateGitConfig', async () => { + const fakeConfigHome = makeDirectory('app-security-xdg-') + writeFiles(fakeConfigHome, {'git/ignore': 'notes.txt\n'}) + const root = makeRepository({'notes.txt': 'todo', 'src/index.ts': ''}) + + // Sanity: with core.excludesFile unset git does read $XDG_CONFIG_HOME/git/ignore. + vi.stubEnv('XDG_CONFIG_HOME', fakeConfigHome) + await expect(listedPaths(root)).resolves.toEqual(['notes.txt']) + + const restoreAgain = isolateGitConfig() + try { + await expect(listedPaths(root)).resolves.toEqual([]) + } finally { + restoreAgain() + } + }) + + describe('outcomes other than a listing', () => { + test('reports a directory outside any git repository', async () => { + const root = makeDirectory() + writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) + + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'not-a-repository'}) + }) + + test('reports an app folder the enclosing repository ignores, even with a force-tracked descendant', async () => { + // With a force-tracked file inside, git no longer collapses the app to a single `./` entry: + // it lists every untracked file in the app individually, which would empty the scan. + 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'))).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': '', + }) + + // Asked from the repository root the same setup lists the ignored folder, proving git works here. + await expect(listedPaths(repository)).resolves.toEqual(['apps/']) + + await expect(listGitIgnoredPaths(join(repository, 'apps', 'web'))).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)).resolves.toEqual({status: 'listed', paths: ['tmp/']}) + }) + + test('does not treat the top level of a whitelist-style repository as ignored', async () => { + // `git check-ignore .` at the top level normalises `.` to the empty path, which a bare `*` + // matches, so probing there would misreport the app as ignored and drop every git exclusion. + const root = makeRepository({ + '.gitignore': '*\n!src/\n!src/**\n!.gitignore\n', + 'src/index.ts': '', + 'tmp.log': '', + }) + + await expect(listGitIgnoredPaths(root)).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'))).resolves.toEqual({ + status: 'listed', + paths: ['.env', 'shopify.app.toml'], + }) + }) + + test('reports a directory inside .git as not being in a repository', async () => { + // `git rev-parse --is-inside-work-tree` prints `false` with exit code 0 there. + const root = makeRepository({}) + + await expect(listGitIgnoredPaths(join(root, '.git'))).resolves.toEqual({status: 'not-a-repository'}) + }) + + test.each([ + ['config', '[core\nbogus'], + ['HEAD', 'not a ref'], + ])( + 'reports a failure, not a missing repository, when git refuses a repository with a corrupt %s', + async (file, content) => { + // Both make `rev-parse` exit 128, as it does outside any repository (verified with git 2.55: a + // corrupt config prints "bad config line", a corrupt HEAD even prints "not a git repository"). + // The .git marker on disk is what tells the two apart. + const root = makeRepository({'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) + git(root, ['add', '.gitignore']) + git(root, ['commit', '-qm', 'init']) + writeFileSync(join(root, '.git', file), content) + + await expect(listGitIgnoredPaths(root)).resolves.toEqual({status: 'failed'}) + }, + ) + + test.skipIf(process.platform === 'win32')( + 'reports a failure, not a missing repository, when the .git marker is a dangling symlink', + async () => { + // git cannot follow the dangling link, so `rev-parse` exits 128 exactly as it does outside any + // repository; the ambiguous marker on disk is what keeps this from being reported as one. + const root = makeDirectory() + 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'}) + }, + ) + + test('reports a failure when git refuses a repository enclosing the app folder', async () => { + 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'}) + }) + + 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. + writeFileSync(join(root, '.git', 'index'), 'not an index') + + await expect(listGitIgnoredPaths(root)).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']) + }) + }) +}) + +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'])) + + 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) + }) + + 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) + }) +}) + +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) + } + }) + + 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('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) + }) + + 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('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) + }) +}) + +describe('createPathMatcher', () => { + test('is case sensitive', () => { + const isExcluded = createPathMatcher({defaults: ['Build/'], gitIgnoredPaths: []}) + + expect(isExcluded('Build/a.ts', {directory: false})).toBe(true) + expect(isExcluded('build/a.ts', {directory: false})).toBe(false) + }) + + test('handles many gitignore literals and many lookups', () => { + // 50,000 literals × 50,000 lookups. The literal lookup is a Set membership test, which keeps + // this far inside vitest's timeout; compiling the literals into patterns (as the first version + // of this matcher did) made the same workload take about a minute. + const rules = buildPathRules({ + gitIgnoredPaths: Array.from({length: 50_000}, (_, index) => `dir${index % 100}/.DS_Store${index}`), + }) + 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 + + expect(excluded).toBe(0) + expect(isExcluded('dir7/.DS_Store7', {directory: false})).toBe(true) + expect(isExcluded('dir7/.DS_Store', {directory: false})).toBe(false) + }) +}) + +describe('createFilePathMatcher', () => { + test('tests a root-level file against the rules directly', () => { + const isExcluded = createFilePathMatcher(gitIgnoredOnly(['.env'])) + + expect(isExcluded('.env')).toBe(true) + expect(isExcluded('.env.example')).toBe(false) + }) + + test('excludes a file below a collapsed git directory literal', () => { + // `createPathMatcher` alone would answer false for `tmp/a/b.ts`: the literal `tmp/` names only the + // directory. This matcher asks about the ancestors first, as the walker's pruning would have. + const isExcluded = createFilePathMatcher(gitIgnoredOnly(['tmp/'])) + + expect(isExcluded('tmp/a/b.ts')).toBe(true) + }) + + test('does not exclude a sibling whose name merely starts with an excluded directory', () => { + const isExcluded = createFilePathMatcher(gitIgnoredOnly(['tmp/'])) + + expect(isExcluded('tmpx/a.ts')).toBe(false) + }) + + 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) + }) +}) + +describe('buildPathRules', () => { + test('keeps the defaults and the git literals as separate phases, in input order', () => { + expect(buildPathRules({gitIgnoredPaths: ['notes.txt', 'tmp/']})).toEqual({ + defaults: DEFAULT_EXCLUDE_PATTERNS, + gitIgnoredPaths: ['notes.txt', 'tmp/'], + }) + }) + + test('has no git literals when git reported nothing', () => { + expect(buildPathRules({gitIgnoredPaths: []})).toEqual({defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: []}) + }) +}) diff --git a/packages/app/src/cli/services/app-security-engine/tests/rule-analysis.test.ts b/packages/app/src/cli/services/app-security-engine/tests/rule-analysis.test.ts index 6fddcb696bd..98b7302ece8 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/rule-analysis.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/rule-analysis.test.ts @@ -51,6 +51,7 @@ function context( }, detection: {framework: input.framework ?? 'react_router', surface: 'react_router', languages: []}, sourceCandidates: [], + gitIgnoreListing: 'listed', } } 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 86506f99491..7bcb91a3d96 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 @@ -1,11 +1,17 @@ /* eslint-disable id-length, line-comment-position, no-restricted-imports -- security fixtures exercise raw git and filesystem behavior */ +import {git, isolateGitConfig} from './git-test-helpers.js' import {scan} from '../scanners/index.js' -import {SHOPIFY_SECRET_PATTERNS, redactMatch, redactText, gitStatusFor} from '../rules/secret-rules.js' -import {describe, expect, test} from 'vitest' +import { + SHOPIFY_SECRET_PATTERNS, + redactMatch, + redactText, + gitStatusFor, + scanCommittedSecrets, +} from '../rules/secret-rules.js' +import {afterEach, beforeEach, describe, expect, test} from 'vitest' import {mkdtempSync, writeFileSync, mkdirSync, rmSync} from 'node:fs' import {tmpdir} from 'node:os' import {join} from 'node:path' -import {execFileSync} from 'node:child_process' /** * Regression tests for two defects found in review, both of which the existing @@ -81,19 +87,22 @@ const makeApp = (files: Record): string => { return dir } -const git = (dir: string, args: string[]) => - execFileSync('git', args, { - cwd: dir, - encoding: 'utf-8', - stdio: ['ignore', 'pipe', 'ignore'], - env: { - ...process.env, - GIT_AUTHOR_NAME: 't', - GIT_AUTHOR_EMAIL: 't@t', - GIT_COMMITTER_NAME: 't', - GIT_COMMITTER_EMAIL: 't@t', - }, - }) +// Directories removed after each test, so a failing assertion cannot leak them. +const temporaryDirectories: string[] = [] +const removeAfterTest = (directory: string): string => { + temporaryDirectories.push(directory) + return directory +} + +// Keep git results independent of the developer's global excludes and any enclosing repository. +let restoreGitConfig: (() => void) | undefined +beforeEach(() => { + restoreGitConfig = isolateGitConfig() +}) +afterEach(() => { + restoreGitConfig?.() + for (const directory of temporaryDirectories.splice(0)) rmSync(directory, {recursive: true, force: true}) +}) describe('redaction never emits known secrets', () => { const samples: [label: string, line: string, secret: string][] = [ @@ -260,6 +269,119 @@ 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 () => { + // The app folder is gitignored by a parent repository (a monorepo scratch area, a dotfiles + // repo ignoring `*`). That repository's rules do not protect the app, so the file is scanned + // and the finding must say so rather than claim the ignore status is unconfirmed. + 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) + 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', + }) + 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 () => { + // The app's own `.env` rule matches `inner/.env`, but `inner/` is a separate repository, so the + // app repository never lists the file as ignored and discovery scans it. The finding names the + // nested repository only after confirming the two directories have different top levels. + const dir = removeAfterTest(makeApp({'.gitignore': '.env\n'})) + git(dir, ['init', '-q', '.']) + const inner = join(dir, 'inner') + mkdirSync(inner) + git(inner, ['init', '-q', '.']) + writeFileSync(join(inner, '.env'), trackedEnvSecret()) + + const result = await scan(dir) + const finding = result.issues.find((i) => i.id === 'COMMITTED_SECRET') + expect(finding).toMatchObject({ + location: {file: 'inner/.env'}, + title: 'Environment file with secrets is inside a nested git repository', + pattern_id: 'environment-file:unconfirmed', + }) + expect(finding!.message).toContain('inner/.env belongs to a nested git repository') + expect(finding!.message).not.toContain('could not be confirmed') + const evidence = finding!.detection_evidence?.join(' ') + expect(evidence).toContain('→ ignored') + expect(evidence).toContain('git rev-parse --show-toplevel → differs') + }) + + test('says the ignored-file listing failed when git ignores a file that was still scanned', async () => { + // Discovery falls back to scanning everything when git cannot list ignored paths, so a file git + // ignores can reach the rule. The finding must say why it was scanned rather than blame a + // foreign repository. + const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) + git(dir, ['init', '-q', '.']) + const file = {path: '.env', absolutePath: join(dir, '.env'), ext: '', content: trackedEnvSecret()} + + const issues = await scanCommittedSecrets([file], dir, 'failed') + expect(issues).toHaveLength(1) + expect(issues[0]).toMatchObject({ + location: {file: '.env'}, + title: 'Environment file with secrets is ignored by git but was scanned', + pattern_id: 'environment-file:unconfirmed', + }) + expect(issues[0]!.message).toContain('could not list the ignored files') + expect(issues[0]!.detection_evidence?.join(' ')).toContain('→ ignored') + }) + + test('still scans a gitignored secret conservatively when git cannot list the ignored files', async () => { + // End to end: the listing fails (a truncated index leaves `rev-parse` working but makes `ls-files` + // exit with a fatal error), so discovery applies no git exclusions and the ignored .env is hashed + // and reported rather than silently trusted. + const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) + git(dir, ['init', '-q', '.']) + git(dir, ['add', '.gitignore', 'shopify.app.toml']) + git(dir, ['commit', '-qm', 'init']) + writeFileSync(join(dir, '.git', 'index'), 'not an index') + + const result = await scan(dir) + expect(result.scan.file_hashes).toHaveProperty(['.env']) + const finding = result.issues.find((issue) => issue.id === 'COMMITTED_SECRET') + expect(finding).toMatchObject({ + severity: 'high', + points: -50, + location: {file: '.env'}, + title: 'Environment file with secrets could not be confirmed as ignored', + pattern_id: 'environment-file:unconfirmed', + }) + expect(finding!.message).toContain('git status command failed') + expect(JSON.stringify(result)).not.toContain(PROBES.shopifySecret) + }) + + test('says why is unknown when an ignored file has the same top level as the app', async () => { + // The listing succeeded and the file is not in a nested repository, so nothing explains why git + // ignores a file discovery still produced. Git DID confirm the file is untracked and ignored, + // so the finding must say the cause is unknown rather than call the ignore status unconfirmed. + const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) + git(dir, ['init', '-q', '.']) + const file = {path: '.env', absolutePath: join(dir, '.env'), ext: '', content: trackedEnvSecret()} + + const issues = await scanCommittedSecrets([file], dir, 'listed') + expect(issues[0]).toMatchObject({ + title: 'Environment file with secrets is ignored by git but was scanned', + pattern_id: 'environment-file:unconfirmed', + }) + expect(issues[0]!.message).toContain( + ".env is ignored by git but was still scanned; App Security couldn't determine why", + ) + expect(issues[0]!.message).not.toContain('could not be confirmed') + expect(issues[0]!.detection_evidence?.join(' ')).toContain('git rev-parse --show-toplevel → same') + }) + test('reports tri-state status rather than a boolean guess', async () => { const dir = mkdtempSync(join(tmpdir(), 'app-security-nogit-')) const status = await gitStatusFor(dir, '.env') From 1b46bd119711966f60468bbde27e01808aba5adc Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 29 Sep 2026 14:59:44 -0700 Subject: [PATCH 2/7] Trim comments in App Security discovery --- .../app-security-engine/rules/secret-rules.ts | 41 +---- .../app-security-engine/rules/types.ts | 2 +- .../app-security-engine/scanners/discover.ts | 82 ++------- .../scanners/filesystem-errors.ts | 5 - .../app-security-engine/scanners/index.ts | 2 +- .../scanners/path-rules.ts | 160 +++--------------- .../scanners/repository-marker.ts | 18 +- .../dependency-automation-discovery.test.ts | 7 +- .../tests/dependency-automation.test.ts | 5 +- .../tests/discovery-safety.test.ts | 9 +- .../tests/git-test-helpers.ts | 37 +--- .../tests/path-rules.test.ts | 31 +--- .../tests/secret-safety.test.ts | 18 +- 13 files changed, 58 insertions(+), 359 deletions(-) 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 d84d4d85807..952f1badc6b 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 @@ -146,39 +146,16 @@ function isEnvFile(path: string): boolean { return envFileBasename(path) !== undefined } -/** - * Why a file git reports as untracked-and-ignored was still scanned. Discovery - * drops the paths git lists as ignored, so such a file only reaches the rule - * when the listing did not cover it. `unknown` means nothing verified explains - * it. - */ +/** Why a file git reports as untracked and ignored still reached this rule. */ type IgnoredFileScanReason = 'enclosing-repository' | 'nested-repository' | 'listing-failed' | 'unknown' -/** - * The top level of the repository containing `cwd`, or `undefined` when git - * cannot say. A missing git binary resolves with exit code 0 and empty output, - * so only a printed path counts as known. - */ +// A missing git binary resolves with exit code 0 and empty output. async function gitTopLevel(cwd: string): Promise { const result = await runGit(cwd, ['rev-parse', '--show-toplevel']) return result.exitCode === 0 && result.out !== '' ? result.out : undefined } -/** - * Work out, from the listing outcome and (when needed) one more git probe, - * why an ignored file was scanned. Only claims what was verified: - * - * - `app-root-ignored`: the enclosing repository ignores the app folder, so - * discovery ignored its rules on purpose. - * - `listed`: the realistic cause is a nested repository, which is confirmed by - * comparing `git rev-parse --show-toplevel` in the file's directory and in - * the app root; the comparison is returned as extra evidence. - * - `failed`: discovery scanned everything because git could not list ignored files. - * - `not-a-repository`: git could not have reported the file as ignored. - * - * `appTopLevel` resolves the app root's top level; the caller shares one - * result across every file it asks about. - */ +/** When git listed ignored paths, a nested repository is confirmed by comparing top levels. */ async function ignoredFileScanReason( appRoot: string, path: string, @@ -237,8 +214,6 @@ function committedSecretFileIssue( message = `${file.path} is ignored by git, but App Security could not list the ignored files for this app, so it was scanned.` fixDescription = `Confirm the repository is healthy with 'git status' and rotate any exposed secrets` } else if (untrackedAndIgnored) { - // Git confirmed the file is untracked and ignored, so the ignore status is not in doubt; only - // the reason discovery still produced the file is. title = `${kind} is ignored by git but was scanned` message = `${file.path} is ignored by git but was still scanned; App Security couldn't determine why. Confirm the rule ignoring it belongs to the repository that owns this app before treating this as clean.` fixDescription = `Confirm with 'git check-ignore -v ${file.path}' that this app's repository ignores it, and rotate any exposed secrets` @@ -264,12 +239,7 @@ function committedSecretFileIssue( } } -/** - * Rule 8: COMMITTED_SECRET (-50, high) - * - * `gitIgnoreListing` is how discovery's request for git's ignored paths went; - * it decides what a finding may claim about a file git reports as ignored. - */ +/** Rule 8: COMMITTED_SECRET (-50, high) */ export async function scanCommittedSecrets( secretEvidenceFiles: SourceFile[], appRoot: string, @@ -277,8 +247,7 @@ export async function scanCommittedSecrets( ): Promise { const issues: Issue[] = [] - // The app root's top level is the same for every file: resolve it once, and only when a file - // git reports as untracked-and-ignored needs it. + // Resolved lazily, once: most scans never need it. let appTopLevel: Promise | undefined const appRootTopLevel = () => { appTopLevel ??= gitTopLevel(appRoot) 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 2b44733d1c3..5c589a64431 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 @@ -57,6 +57,6 @@ export interface ScanContext { detection: ProjectDetection /** Path-only inventory, including unsupported source candidates. */ sourceCandidates: SourceCandidate[] - /** How asking git for the app's ignored paths went; discovery excluded those paths only when `listed`. */ + /** Outcome of listing git's ignored paths. */ gitIgnoreListing: GitIgnoreListing['status'] } 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 a3d2b09e5e1..6d7c41af229 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 @@ -231,17 +231,7 @@ function recordSectionGap(appRoot: string | undefined, path: string, detail: str recordSkippedFile(appRoot, path, {ok: false, reason: 'unreadable', detail}) } -/** - * A directory holding its own `shopify.app*.toml` is an independent Shopify app. - * Nested apps are independent scan roots and never evidence for their parent - * app, so the walker stops at them: a structural boundary, much as git treats - * a nested repository as opaque to the enclosing one. (Nested git repositories - * themselves are NOT a boundary here; only their `.git` entry is pruned.) - * - * Detection looks at the directory's raw entries, not the rule-filtered ones: - * a nested app whose configuration file happens to be gitignored is still a - * nested app. - */ +/** 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)) } @@ -262,25 +252,13 @@ function readDirectoryEntries(appRoot: string, absolutePath: string, displayPath } /** - * List every repository file the scan may inspect, as sorted app-root-relative - * POSIX paths. - * - * The walk applies `rules` with .gitignore semantics and prunes excluded - * directories: it never descends into them, so the matcher is only ever asked - * about paths whose ancestors are known to be included (its precondition). - * - * Directory entries use lstat semantics, so a symlink is never a directory - * here. Symlinks (to files or directories), FIFOs and other special entries - * are listed but never traversed; `readRepositoryFile` later enforces - * containment and realpath rules on anything a finder decides to read, and - * records failures. Dot-folders and dotfiles are walked unless a rule excludes - * them (see `DEFAULT_EXCLUDE_PATTERNS`). + * 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) const files: string[] = [] - // Relative directory paths still to be read; '' is the app root itself. Subdirectories are appended - // while iterating, which `for...of` supports: the array iterator re-checks the length on every step. + // Appended to while iterating; '' is the app root. const pendingDirectories = [''] for (const relativeDirectory of pendingDirectories) { @@ -291,14 +269,11 @@ export function listRepositoryFiles(appRoot: string, rules: PathRules): string[] relativeDirectory === '' ? 'app root' : relativeDirectory, ) if (entries === undefined) continue - // A nested app is a scan root in its own right; nothing beneath it belongs to this scan. if (relativeDirectory !== '' && isNestedAppDirectory(entries)) continue for (const entry of entries) { const relative = relativeDirectory === '' ? entry.name : `${relativeDirectory}/${entry.name}` if (entry.isDirectory()) { - // Never descend into an excluded directory: pruning here is what keeps the matcher's - // precondition (every ancestor of a queried path is included) true. if (!matcher(relative, {directory: true})) pendingDirectories.push(relative) } else if (!matcher(relative, {directory: false})) { files.push(relative) @@ -309,13 +284,7 @@ export function listRepositoryFiles(appRoot: string, rules: PathRules): string[] return files.sort() } -/** - * Group source paths by the extension directories that contain them, walking - * each path's ancestors once instead of filtering the whole repository per - * extension. `extensionDirectories` are app-root-relative; `.` is the app root - * and contains every path. A path inside nested extension directories belongs - * to each of them. Paths keep their input order within every group. - */ +/** A path inside nested extension directories belongs to each of them. */ function groupSourcePathsByExtensionDirectory( repositoryFiles: ReadonlyArray, extensionDirectories: ReadonlySet, @@ -326,7 +295,6 @@ function groupSourcePathsByExtensionDirectory( for (const path of repositoryFiles) { if (!hasSupportedSourceExtension(path)) continue - // `dirname` yields `.` for a top-level file and for `.` itself, which ends the climb. let ancestor = dirname(path) while (ancestor !== '.') { pathsByDirectory.get(ancestor)?.push(path) @@ -339,14 +307,13 @@ function groupSourcePathsByExtensionDirectory( } /** - * Find extension-like repository content among the walked repository files. + * Find extension-like repository content under the app root. * * `Project.load()` only considers paths in each app configuration's * `extension_directories`. App Security still scans every `shopify.extension.toml` * inside the repository boundary, including unconfigured extensions, because * those files can still contain secrets, XSS, and other security evidence. - * Nested apps, generated output, and test trees are already absent from - * `repositoryFiles` (see `listRepositoryFiles`). + * Nested apps, generated output, and test trees remain excluded. */ export function findExtensions(appRoot: string, repositoryFiles: ReadonlyArray): ExtensionInfo[] { const extensionTomls = repositoryFiles.filter((path) => basename(path) === 'shopify.extension.toml') @@ -620,7 +587,6 @@ const SOURCE_LANGUAGES = { type SourceExtension = keyof typeof SOURCE_LANGUAGES -/** Extension matching is exact and case-sensitive: `.JS` is not treated as source. */ function sourceLanguageFor(path: string): (typeof SOURCE_LANGUAGES)[SourceExtension] | undefined { const extension = extname(path) return extension in SOURCE_LANGUAGES ? SOURCE_LANGUAGES[extension as SourceExtension] : undefined @@ -641,13 +607,8 @@ export function findSourceCandidates(repositoryFiles: ReadonlyArray): So .sort((left, right) => left.path.localeCompare(right.path)) } -/** - * Read the source files (backend routes, extension code, etc.) whose language - * is supported by the non-secret deterministic scanners. Results keep the order - * of `repositoryFiles`, which callers pass in `listRepositoryFiles` order. - */ +/** Find and read only source languages supported by non-secret deterministic scanners. */ export function findAppSourceFiles(appRoot: string, repositoryFiles: ReadonlyArray): SourceFile[] { - // `findExtensions` passes pre-filtered groups, but the filter is this function's own contract for every caller. return repositoryFiles.filter(hasSupportedSourceExtension).map((path) => { const absolutePath = joinPath(appRoot, path) const result = readRepositoryFile(appRoot, absolutePath) @@ -688,7 +649,6 @@ const SECRET_TEXT_EXTENSIONS = new Set([ '.pem', ]) -/** Extensionless files that routinely carry credentials. */ const SENSITIVE_FILE_NAMES = new Set(['Dockerfile', 'Containerfile', 'Gemfile', 'Rakefile', 'Procfile', 'Makefile']) function isSensitiveFile(path: string): boolean { @@ -713,11 +673,7 @@ function isProbablyBinary(content: Buffer): boolean { return sample.length > 0 && suspiciousControlBytes / sample.length > 0.1 } -/** - * Text evidence inspected for secrets regardless of app framework support. - * Results keep the order of `repositoryFiles`, which callers pass in - * `listRepositoryFiles` order. - */ +/** Text evidence inspected for secrets regardless of app framework support. */ export function findSensitiveFiles( appRoot: string, repositoryFiles: ReadonlyArray, @@ -742,12 +698,6 @@ export function findSensitiveFiles( }) } -/** - * Why repository-level configuration cannot be attributed to the app, if it - * cannot. The app owns its configuration when its own `.git` marker is the - * nearest one (or there is no repository at all); a marker found only above - * the app root means the configuration belongs to an enclosing repository. - */ function nestedRepositoryReason(appRoot: string): string | undefined { const marker = findRepositoryMarker(appRoot) if (marker.status === 'none') return undefined @@ -761,13 +711,8 @@ function recordRejectedAllowlistPath(appRoot: string, relative: string, failure: /** * Read local bot configuration only; hosted integrations and CI workflows are - * outside this check's scope. - * - * The allowlisted paths are read directly from disk rather than taken from the - * walked file list, so that a symlinked `.github` (which the walker never - * enters) is reported as unresolved rather than missing. The scan's `rules` - * still apply: hosted bots read the repository, so a configuration file git - * ignores configures nothing and is treated exactly like a missing one. + * 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. */ export function findDependencyAutomationInputs(appRoot: string, rules: PathRules): DependencyAutomationInputs { let canonicalRoot: string @@ -822,10 +767,7 @@ export function findDependencyAutomationInputs(appRoot: string, rules: PathRules return files.length > 0 ? {files} : {files, ...(unresolvedReason ? {unresolvedReason} : {})} } -/** - * Find JavaScript package manifests. Dependency analysis intentionally supports JavaScript only. - * Results keep the order of `repositoryFiles`, which callers pass in `listRepositoryFiles` order. - */ +/** Find JavaScript package manifests. Dependency analysis intentionally supports JavaScript only. */ export function findManifestPaths(repositoryFiles: ReadonlyArray): string[] { return repositoryFiles.filter((path) => basename(path) === 'package.json') } diff --git a/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts b/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts index 97dc31a7894..ffc80eb4e01 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/filesystem-errors.ts @@ -1,12 +1,7 @@ -/** - * Whether a filesystem error means the path (or one of its ancestors) does not - * exist, as opposed to existing but being unreadable. - */ export function isMissingFilesystemEntry(error: unknown): boolean { return error instanceof Error && 'code' in error && (error.code === 'ENOENT' || error.code === 'ENOTDIR') } -/** A short, locale-independent reason for a failed filesystem inspection, carrying the error code when there is one. */ export function inspectErrorReason(target: string, error: unknown): string { const code = error instanceof Error && 'code' in error && typeof error.code === 'string' ? error.code : undefined return code ? `Could not inspect ${target} (${code})` : `Could not inspect ${target}` 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 b8a93db72f4..8642ca4b7ce 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 @@ -587,7 +587,7 @@ export async function scan(startPath?: string, configFileName?: string): Promise const selectedFileName = getAppConfigurationFileName(configFileName) const appToml = loadAppToml(joinPath(appRoot, selectedFileName), appRoot) const appTomls = appToml ? [appToml] : [] - // Path rules govern repository discovery only; the selected app configuration above is always loaded. + // The selected app configuration is loaded even if gitignored: path rules only apply to discovery. const gitIgnoreListing = await listGitIgnoredPaths(appRoot) const pathRules = buildPathRules({ gitIgnoredPaths: gitIgnoreListing.status === 'listed' ? gitIgnoreListing.paths : [], 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 c245a8865fe..3985485212e 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/path-rules.ts @@ -3,60 +3,24 @@ import {outputDebug} from '@shopify/cli-kit/node/output' import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' import ignore from 'ignore' -/** The scan's path rules: two exclusion phases, either of which excludes a path. */ export interface PathRules { - /** .gitignore exclude patterns applied to every scan. */ defaults: ReadonlyArray - /** Literal app-root-relative paths git reports as untracked and ignored; directories end with `/`. */ + /** App-root-relative paths git reports as untracked and ignored; directories end with `/`. */ gitIgnoredPaths: ReadonlyArray } /** - * Decide whether an app-root-relative path is excluded from the scan. - * - * `relativePath` must be POSIX-separated, relative to the app root, with no - * `./` prefix and no trailing slash. Pass `directory: true` for directories so - * directory-only patterns (`build/`) can match them. - * - * PRECONDITION: the answer is only correct for paths whose ancestor directories - * the caller has already confirmed are NOT excluded. A collapsed git directory - * literal (`tmp/`) matches only the directory itself, never `tmp/a.ts`, so a - * walker must prune the directory rather than ask about its contents. A - * directory walker that never descends into excluded directories satisfies - * this naturally; do not use the matcher to test arbitrary deep paths. + * 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 -/** - * Decide whether an app-root-relative FILE path is excluded from the scan, - * checking its ancestor directories itself. See `createFilePathMatcher`. - */ type FilePathMatcher = (relativePath: string) => boolean /** - * Paths never worth scanning: build output, dependencies, caches, and test - * fixture trees, expressed in .gitignore syntax. A pattern without a slash - * matches at any depth; a trailing `/` matches directories only. - * - * Patterns are generic on purpose. Earlier versions hardcoded the names of - * this project's own fixture directories, which both leaked internal naming - * into a tool that ships to third-party developers and silently skipped any - * directory a developer happened to give the same name. - * - * The scan walks dot-folders, so generated dot-folders must be listed - * explicitly: framework build output (`.next/`, `.nuxt/`, ...), caches - * (`.cache/`, `.turbo/`), Yarn Berry's committed `.yarn/releases`, and the - * CLI-generated `.shopify/`. `.github/`, `.vscode/`, `.devcontainer/` and - * `.circleci/` are deliberately NOT excluded because hardcoded secrets turn up - * in workflow and editor configuration. - * - * `.git` has no trailing slash so it also matches the `.git` FILE that git - * worktrees use in place of a directory. - * - * Sub-apps (a nested directory with its own shopify.app.toml) are not a path - * rule: `listRepositoryFiles` in `discover.ts` treats them as a structural - * walker boundary, since that requires reading the tree rather than matching - * a name. + * `.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. */ export const DEFAULT_EXCLUDE_PATTERNS: ReadonlyArray = [ 'node_modules/', @@ -88,7 +52,6 @@ export const DEFAULT_EXCLUDE_PATTERNS: ReadonlyArray = [ '.svelte-kit/', ] -/** What asking git for the app's ignored paths produced. */ export type GitIgnoreListing = | {status: 'listed'; paths: string[]} | {status: 'not-a-repository'} @@ -96,26 +59,8 @@ export type GitIgnoreListing = | {status: 'failed'} /** - * Ask git which paths under `appRoot` are ignored and untracked. - * - * Listed paths are relative to `appRoot` with POSIX separators. A fully - * ignored directory is collapsed to a single `dir/` entry. Only UNTRACKED - * ignored paths appear: a tracked file that matches .gitignore is never - * reported, so a committed-then-ignored `.env` is still scanned. - * - * Every outcome other than `listed` means "no git-based exclusions": the - * caller scans everything the defaults allow. The outcomes are kept apart so - * later findings can say exactly why a git-ignored file was still scanned. - * - * - `not-a-repository`: `appRoot` is not inside a git working tree: no `.git` - * marker exists at or above it, or `appRoot` lies inside the `.git` - * directory itself. - * - `app-root-ignored`: an enclosing repository ignores the app folder (or an - * ancestor of it). That repository does not own the app, so its rules must - * not empty the scan. - * - `failed`: git is missing, refused to read a repository that does exist - * (dubious ownership, corrupt or invalid configuration), or a command failed - * for any other reason. + * Only untracked paths are listed, so tracked files that match .gitignore are + * still scanned. Any status other than `listed` means no git exclusions apply. */ export async function listGitIgnoredPaths(appRoot: string): Promise { const listing = await runGitIgnoreListing(appRoot) @@ -124,35 +69,21 @@ export async function listGitIgnoredPaths(appRoot: string): Promise { - // One call answers two questions. Verified with git 2.55, the output is two lines: `true` or - // `false`, then the app root's path relative to the repository's top level, which is an empty - // line at the top level itself (`true\n\n`; `true\napps/web/\n` from apps/web). Inside `.git` - // the first line is `false` with exit code 0; outside any repository the exit code is 128. + // 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'} - // A repository git refuses to read (dubious ownership under `safe.directory`, a corrupt config or - // HEAD) also exits 128, and only git's localized stderr tells the cases apart. The `.git` marker - // on disk does so independently of locale: a marker means git failed on a repository that exists. + // 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'} } const [insideWorkTree, prefix] = location.stdout.split(/\r?\n/) if (insideWorkTree === 'false') return {status: 'not-a-repository'} - // A missing git binary resolves with exit code 0 and empty output (captureOutputWithExitCode - // does not reject), so only an explicit `true` counts as being inside a working tree. + // A missing git binary resolves with exit code 0 and empty output. if (insideWorkTree !== 'true' || prefix === undefined) return {status: 'failed'} - // Is the app folder itself ignored by the repository? `--no-index` is essential: without it git - // answers "not ignored" as soon as any descendant is force-tracked, and in exactly that case - // `ls-files` below no longer collapses the app to a single `./` entry but lists every untracked - // file in the app individually, which would exclude almost the entire app from the scan. With - // `--no-index` the answer follows the ignore rules alone, for the app folder and its ancestors. - // - // The probe is skipped at the top level (empty prefix): a repository cannot ignore its own top - // level, and `check-ignore .` normalises `.` there to the empty path, which a bare `*` pattern - // matches, so a whitelist-style root .gitignore (`*` then `!src/` ...) would misreport the app - // as ignored. From a subdirectory `.` is resolved to the prefix path, and the same whitelist - // (`*`, `!*/`) correctly answers `1` (not ignored) for a re-included folder. + // `--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'} @@ -175,21 +106,7 @@ async function runGitIgnoreListing(appRoot: string): Promise { return {status: 'listed', paths: listed.stdout.split('\0').filter((path) => path !== '')} } -/** - * Keep git out of the default directories. `ls-files --others --ignored` - * walks every untracked directory that is NOT itself ignored (an unignored - * `node_modules/` with 63,000 files took 0.05 s to list here; 0.02 s with the - * exclusion below). Every default directory is always excluded by the walker, - * so git's view of the ignored paths inside one can never matter, and skipping - * them cannot change the scan. - * - * Each pathspec is `:(exclude,glob)` followed by the pattern wrapped in - * leading and trailing `**` segments, which matches the directory at the app - * root and at any depth; a wildcard name such as `*-fixtures/` keeps working. - * Only directory defaults (trailing `/`) become pathspecs; file patterns such - * as `*.test.*` and the `.git` entry do not name a directory git would - * traverse. - */ +/** The walker always prunes default directories, so stop git walking an unignored `node_modules/`. */ function defaultDirectoryPathspecExcludes(): string[] { return DEFAULT_EXCLUDE_PATTERNS.filter((pattern) => pattern.endsWith('/')).map( (pattern) => `:(exclude,glob)**/${pattern}**`, @@ -200,42 +117,20 @@ async function runGit(cwd: string, args: string[]): Promise<{exitCode: number; s try { const result = await captureOutputWithExitCode('git', args, {cwd}) return {exitCode: result.exitCode, stdout: result.stdout} - // Defensive only: captureOutputWithExitCode does not reject on a non-zero exit, and a missing git - // binary resolves with empty output rather than throwing. Anything that does throw is treated - // as a failed listing: no git-based exclusions. // eslint-disable-next-line no-catch-all/no-catch-all } catch { return undefined } } -/** - * Assemble the scan's path rules from the hardcoded defaults and the literal - * paths git reported as ignored. The two phases are independent: a path is - * excluded when either matches it. - */ export function buildPathRules(input: {gitIgnoredPaths: ReadonlyArray}): PathRules { return {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: input.gitIgnoredPaths} } /** - * Compile the rules into a matcher: a path is excluded when a git literal - * names it exactly or a default pattern matches it. - * - * Git literals (possibly thousands of them) are looked up in a Set rather than - * compiled into patterns, both for speed and so that a name containing - * gitignore-significant characters (`[id].ts`, `#hash.ts`) matches literally. - * - * `ignorecase: false` matches the case-sensitive fast-glob discovery this - * replaced and makes results identical on every platform. - * - * `allowRelativePaths: true` stops `ignore` from throwing on a name made only - * of dots (`...` is a legal POSIX filename); callers already guarantee there is - * no `./` prefix, which is the case the check exists for. - * - * `ignore` is a CommonJS module whose typings describe an ESM default export, - * so under NodeNext the factory is reached through `.default` (as in - * `dev/app-events/file-watcher.ts`). + * 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`. */ export function createPathMatcher(rules: PathRules): PathMatcher { const defaults = ignore.default({ignorecase: false, allowRelativePaths: true}).add([...rules.defaults]) @@ -248,22 +143,9 @@ export function createPathMatcher(rules: PathRules): PathMatcher { } /** - * Compile the rules into a matcher for a FILE path the caller already knows - * rather than reached by walking, such as an allowlisted configuration path. - * - * `createPathMatcher` is only correct once every ancestor directory is known - * to be included, so this matcher establishes that itself: it asks about each - * ancestor directory top-down first (an excluded ancestor excludes the file, - * exactly as the walker would have pruned it), then about the file. The path - * must meet `createPathMatcher`'s format: POSIX-separated, app-root-relative, - * no `./` prefix and no trailing slash. - * - * This deliberately diverges from the walker for a symlinked directory. The - * walker uses lstat semantics, so it tests a symlinked `.github` as a FILE - * (`.github`); this matcher tests every ancestor as a directory (`.github/`). - * A gitignored symlinked folder (reported by git as the file literal `.github`) - * is therefore NOT excluded here, and its configuration surfaces as - * unresolved instead, as documented on `findDependencyAutomationInputs`. + * 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) diff --git a/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts b/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts index 866659c774e..8ca7640a0ad 100644 --- a/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts +++ b/packages/app/src/cli/services/app-security-engine/scanners/repository-marker.ts @@ -2,25 +2,9 @@ import {inspectErrorReason, isMissingFilesystemEntry} from './filesystem-errors. import {dirname, joinPath} from '@shopify/cli-kit/node/path' import {lstatSync} from 'node:fs' -/** - * The nearest `.git` entry at or above a directory. - * - * - `found`: a `.git` directory or worktree file exists in `directory`, which - * is the start directory itself or one of its ancestors. - * - `ambiguous`: the nearest `.git` entry is a symlink or special file, or - * could not be inspected. Following it would leave repository ownership - * unclear, so callers must not treat it as absent. - * - `none`: no `.git` entry exists at any level. - */ type RepositoryMarker = {status: 'found'; directory: string} | {status: 'ambiguous'; reason: string} | {status: 'none'} -/** - * Walk from `start` up to the filesystem root and classify the first `.git` - * entry met. Uses lstat so a symlinked `.git` is reported as ambiguous rather - * than followed. The search is purely filesystem-based and locale-independent, - * which lets callers tell "no repository here" apart from "git refused to read - * this repository" without parsing git's localized messages. - */ +/** The nearest `.git` entry at or above `start`. A symlinked `.git` is `ambiguous`, not followed. */ export function findRepositoryMarker(start: string): RepositoryMarker { let directory = start while (true) { 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 f6c2212fb76..727a419f29c 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 @@ -15,7 +15,6 @@ import {dirname, join} from 'node:path' afterEach(() => resetSkippedFiles()) -/** The defaults alone, as the scan's rules are when git reported no ignored paths. */ const NO_GIT_EXCLUSIONS = buildPathRules({gitIgnoredPaths: []}) async function writeFiles(root: string, files: Record): Promise { @@ -75,8 +74,7 @@ describe('dependency automation discovery', () => { }) test('excludes a configuration file inside a directory the rules exclude', async () => { - // Git collapses a fully ignored directory to `.github/`, which never names the file itself, so the - // decision must consider the file's ancestors. + // Git lists `.github/`, never the file, so the ancestors must be checked. await inTemporaryDirectory(async (root) => { await writeFiles(root, {'.github/dependabot.yml': 'version: 2\nupdates: []\n'}) const rules = buildPathRules({gitIgnoredPaths: ['.github/']}) @@ -95,8 +93,7 @@ describe('dependency automation discovery', () => { }) test('applies the default patterns as well as the git literals', async () => { - // No shipped default matches an allowlisted path (`.git` does not match `.github`), so a custom - // default proves the phase is consulted at all. + // 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: []}) 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 a994206b6d8..bdd02997d65 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 @@ -26,7 +26,6 @@ vi.mock('@shopify/cli-kit/node/system', async (importActual) => { const checkId = 'MISSING_DEPENDENCY_SECURITY_AUTOMATION' const dependabot = '# Configuration contents are not validated.\n' -// Keep the scan's git calls independent of the developer's global excludes and any enclosing repository. let restoreGitConfig: (() => void) | undefined beforeEach(() => { restoreGitConfig = isolateGitConfig() @@ -140,8 +139,7 @@ describe('dependency automation scanner integration', () => { const submission = buildSubmission(execution.trace, {cliVersion: '3.99.0', submittedAt: '2026-09-15T00:00:00Z'}) expect(JSON.stringify(submission)).not.toContain('local>org/renovate-config') expect(JSON.stringify(execution.trace)).not.toContain('local>org/renovate-config') - // The scan's own git calls (ignored-path discovery and project metadata) are the only ones allowed. - // The temporary directory is outside any repository, so ignored-path discovery stops at its first probe. + // Outside any repository, ignored-path discovery stops at its first probe. expect(vi.mocked(captureOutputWithExitCode).mock.calls.map(([command, args]) => [command, args])).toEqual([ ['git', ['rev-parse', '--is-inside-work-tree', '--show-prefix']], ['git', ['rev-parse', 'HEAD']], @@ -171,7 +169,6 @@ describe('dependency automation scanner integration', () => { }) describe('gitignored configuration', () => { - // Hosted bots read the repository, so a configuration file that never reaches it configures nothing. async function makeRepository(root: string, files: Record, tracked: string[]): Promise { await makeApp(root, files) git(root, ['init', '-q', '.']) 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 587ad104112..0356ac17453 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 @@ -21,7 +21,6 @@ import type {ScanResult} from '../types.js' const temporaryDirectories: string[] = [] const appConfiguration = 'name = "Discovery safety"\napplication_url = "https://example.com"\n' -/** The rules a scan uses when git reports no ignored paths. */ const DEFAULT_RULES = buildPathRules({gitIgnoredPaths: []}) afterEach(async () => { @@ -67,7 +66,6 @@ function inspectedManifestPaths(result: ScanResult): string[] { return execution?.inspected_files.filter((path) => path.endsWith('package.json')) ?? [] } -/** Rules exactly as `scan()` builds them, so direct finder tests see the same file list. */ async function scanPathRules(appRoot: string): Promise { const listing = await listGitIgnoredPaths(appRoot) return buildPathRules({gitIgnoredPaths: listing.status === 'listed' ? listing.paths : []}) @@ -121,7 +119,6 @@ describe.sequential('app root discovery', () => { }) describe('repository discovery exclusions', () => { - // scan() asks git for ignored paths, so keep results independent of the developer's git configuration. let restoreGitConfig: (() => void) | undefined beforeEach(() => { restoreGitConfig = isolateGitConfig() @@ -291,7 +288,6 @@ describe('repository discovery exclusions', () => { const root = await makeDirectory() await writeFiles(root, { 'shopify.app.toml': appConfiguration, - // A root-level extension configuration owns every source file in the repository. 'shopify.extension.toml': 'type = "theme"\n', 'index.ts': 'export const root = true', 'extensions/alpha/shopify.extension.toml': 'type = "ui_extension"\n', @@ -444,8 +440,6 @@ describe('gitignore-driven exclusions', () => { }) test('treats gitignored paths literally even when their names are gitignore-significant', async () => { - // Glob brackets, comment `#`, negation `!` and whitespace all mean something in .gitignore - // syntax; the names are also legal on Windows, unlike `*` or `?`. const root = await makeRepository({ 'shopify.app.toml': appConfiguration, '.gitignore': '\\[id\\].ts\n\\#hash.ts\n\\!bang.ts\nsp ace.ts\n', @@ -497,8 +491,7 @@ describe('gitignore-driven exclusions', () => { }) test('scans the whole app when the enclosing repository ignores the app folder but force-tracks a file in it', async () => { - // Without the force-tracked README git would collapse the app to `./`; with it, git lists each - // untracked file individually, and treating those as exclusions would empty the scan. + // 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', diff --git a/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts b/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts index 89613877daa..52e4a6d6e96 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/git-test-helpers.ts @@ -5,13 +5,7 @@ import {mkdtempSync, realpathSync, rmSync, writeFileSync} from 'node:fs' import {tmpdir} from 'node:os' import {dirname, join} from 'node:path' -/** - * Run a git command in `directory` and return its stdout. - * - * Author and committer identity are pinned so commits work on machines (and CI - * runners) that have no global git identity configured. stderr is piped so a - * failing fixture command surfaces git's own message in the thrown error. - */ +/** Identity is pinned so commits work on CI runners without a global git identity. */ export function git(directory: string, args: string[]): string { return execFileSync('git', args, { cwd: directory, @@ -28,30 +22,11 @@ export function git(directory: string, args: string[]): string { } /** - * Make every git invocation in the current test independent of the developer - * machine: both the test's own `git()` calls and the production code's git - * calls (which inherit `process.env`). - * - * A developer's global `core.excludesFile`, `$XDG_CONFIG_HOME/git/ignore` - * (read whenever `core.excludesFile` is unset, which is exactly the state this - * helper creates) or system config would otherwise silently change which paths - * git reports as ignored, and a temporary directory could be discovered as - * part of an enclosing repository (for example a home-directory dotfiles repo). - * - * A dedicated temporary directory is created under `os.tmpdir()` holding an - * empty `gitconfig` (an empty file rather than `/dev/null` so the helper also - * works on Windows). The same directory becomes `XDG_CONFIG_HOME`; it contains - * no `git/ignore`, and `GIT_CONFIG_GLOBAL` already overrides its `git/config`. - * `os.tmpdir()` becomes `GIT_CEILING_DIRECTORIES`, so repositories under test - * must live somewhere below `os.tmpdir()` (as `mkdtemp` places them) to remain - * discoverable; git stops climbing once it reaches the ceiling itself. The - * ceiling is compared against git's RESOLVED working directory, so it is set - * from the real path: on macOS `os.tmpdir()` is `/var/...` but git sees - * `/private/var/...`, and on Windows the temp path may use an 8.3 short name. - * - * Returns a cleanup function that restores the environment (all stubs, via - * `vi.unstubAllEnvs()`) and removes the directory. Call it in `afterEach`; the - * vitest config does not enable `unstubEnvs`. + * Isolate git from the developer's global excludes, system config and any + * enclosing repository. Repositories under test must live below `os.tmpdir()`. + * The ceiling uses the real path because git compares it against its resolved + * working directory (`/private/var/...` on macOS). Call the returned cleanup + * in `afterEach`. */ export function isolateGitConfig(): () => void { const directory = mkdtempSync(join(tmpdir(), 'app-security-gitconfig-')) 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 211f0baa319..ee0d5de0e93 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 @@ -46,10 +46,8 @@ function makeRepository(files: Record): string { return root } -/** Only the defaults, as the matcher sees them when git reported nothing. */ const DEFAULTS_ONLY: PathRules = {defaults: DEFAULT_EXCLUDE_PATTERNS, gitIgnoredPaths: []} -/** Only git literals, so a test can prove literal matching without a default getting in the way. */ const gitIgnoredOnly = (paths: string[]): PathRules => ({defaults: [], gitIgnoredPaths: paths}) async function listedPaths(appRoot: string): Promise { @@ -72,7 +70,6 @@ describe('listGitIgnoredPaths', () => { }) test('does not report a tracked file that matches .gitignore', async () => { - // The classic leak: commit .env, then gitignore it. It must still be scanned. const root = makeRepository({'.gitignore': '.env\n', '.env': 'SECRET=1\n'}) git(root, ['add', '-f', '.env']) git(root, ['commit', '-qm', 'oops']) @@ -119,8 +116,6 @@ describe('listGitIgnoredPaths', () => { }) test('reports gitignore-significant filenames literally', async () => { - // Names that .gitignore syntax treats specially (glob brackets, comment `#`, negation `!`, - // whitespace) yet remain legal filenames on every platform, including Windows. const root = makeRepository({ '.gitignore': '\\[id\\].ts\n\\#hash.ts\n\\!bang.ts\nsp ace.ts\n', '[id].ts': '', @@ -141,9 +136,7 @@ describe('listGitIgnoredPaths', () => { test.skipIf(process.platform === 'win32')( 'reports an ignored symlinked directory as a file literal, without a trailing slash', async () => { - // git never follows a symlink while listing, so the link is a blob to it and is not collapsed - // to a `dir/` entry. `createFilePathMatcher` relies on this when it documents that a - // gitignored symlinked `.github` is not excluded by the `.github/` ancestor check. + // Git doesn't follow symlinks, so the link is listed as a file, not a `dir/` entry. const root = makeRepository({'.gitignore': '.github\n', 'real/dependabot.yml': ''}) symlinkSync(join(root, 'real'), join(root, '.github'), 'dir') @@ -157,7 +150,6 @@ describe('listGitIgnoredPaths', () => { writeFiles(fakeConfigHome, {'git/ignore': 'notes.txt\n'}) const root = makeRepository({'notes.txt': 'todo', 'src/index.ts': ''}) - // Sanity: with core.excludesFile unset git does read $XDG_CONFIG_HOME/git/ignore. vi.stubEnv('XDG_CONFIG_HOME', fakeConfigHome) await expect(listedPaths(root)).resolves.toEqual(['notes.txt']) @@ -178,8 +170,7 @@ describe('listGitIgnoredPaths', () => { }) test('reports an app folder the enclosing repository ignores, even with a force-tracked descendant', async () => { - // With a force-tracked file inside, git no longer collapses the app to a single `./` entry: - // it lists every untracked file in the app individually, which would empty the scan. + // 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', @@ -201,7 +192,7 @@ describe('listGitIgnoredPaths', () => { 'apps/web/src/index.ts': '', }) - // Asked from the repository root the same setup lists the ignored folder, proving git works here. + // 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({ @@ -216,8 +207,6 @@ describe('listGitIgnoredPaths', () => { }) test('does not treat the top level of a whitelist-style repository as ignored', async () => { - // `git check-ignore .` at the top level normalises `.` to the empty path, which a bare `*` - // matches, so probing there would misreport the app as ignored and drop every git exclusion. const root = makeRepository({ '.gitignore': '*\n!src/\n!src/**\n!.gitignore\n', 'src/index.ts': '', @@ -242,7 +231,6 @@ describe('listGitIgnoredPaths', () => { }) test('reports a directory inside .git as not being in a repository', async () => { - // `git rev-parse --is-inside-work-tree` prints `false` with exit code 0 there. const root = makeRepository({}) await expect(listGitIgnoredPaths(join(root, '.git'))).resolves.toEqual({status: 'not-a-repository'}) @@ -254,9 +242,7 @@ describe('listGitIgnoredPaths', () => { ])( 'reports a failure, not a missing repository, when git refuses a repository with a corrupt %s', async (file, content) => { - // Both make `rev-parse` exit 128, as it does outside any repository (verified with git 2.55: a - // corrupt config prints "bad config line", a corrupt HEAD even prints "not a git repository"). - // The .git marker on disk is what tells the two apart. + // 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']) @@ -269,8 +255,7 @@ describe('listGitIgnoredPaths', () => { test.skipIf(process.platform === 'win32')( 'reports a failure, not a missing repository, when the .git marker is a dangling symlink', async () => { - // git cannot follow the dangling link, so `rev-parse` exits 128 exactly as it does outside any - // repository; the ambiguous marker on disk is what keeps this from being reported as one. + // A dangling `.git` link also makes `rev-parse` exit 128. const root = makeDirectory() writeFiles(root, {'.gitignore': 'tmp/\n', 'tmp/a.ts': ''}) symlinkSync(join(root, 'missing-git-dir'), join(root, '.git')) @@ -469,9 +454,7 @@ describe('createPathMatcher', () => { }) test('handles many gitignore literals and many lookups', () => { - // 50,000 literals × 50,000 lookups. The literal lookup is a Set membership test, which keeps - // this far inside vitest's timeout; compiling the literals into patterns (as the first version - // of this matcher did) made the same workload take about a minute. + // 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}`), }) @@ -496,8 +479,6 @@ describe('createFilePathMatcher', () => { }) test('excludes a file below a collapsed git directory literal', () => { - // `createPathMatcher` alone would answer false for `tmp/a/b.ts`: the literal `tmp/` names only the - // directory. This matcher asks about the ancestors first, as the walker's pruning would have. const isExcluded = createFilePathMatcher(gitIgnoredOnly(['tmp/'])) expect(isExcluded('tmp/a/b.ts')).toBe(true) 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 7bcb91a3d96..2164f3ddc8d 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 @@ -87,14 +87,12 @@ const makeApp = (files: Record): string => { return dir } -// Directories removed after each test, so a failing assertion cannot leak them. const temporaryDirectories: string[] = [] const removeAfterTest = (directory: string): string => { temporaryDirectories.push(directory) return directory } -// Keep git results independent of the developer's global excludes and any enclosing repository. let restoreGitConfig: (() => void) | undefined beforeEach(() => { restoreGitConfig = isolateGitConfig() @@ -270,9 +268,6 @@ describe('git status drives severity, not .gitignore text', () => { }) test('reports a secret ignored only by an enclosing repository that does not own the app', async () => { - // The app folder is gitignored by a parent repository (a monorepo scratch area, a dotfiles - // repo ignoring `*`). That repository's rules do not protect the app, so the file is scanned - // and the finding must say so rather than claim the ignore status is unconfirmed. const repository = removeAfterTest(mkdtempSync(join(tmpdir(), 'app-security-enclosing-'))) git(repository, ['init', '-q', '.']) writeFileSync(join(repository, '.gitignore'), 'apps/\n') @@ -295,9 +290,6 @@ describe('git status drives severity, not .gitignore text', () => { }) test('reports a secret inside a nested repository that the app repository ignores by name', async () => { - // The app's own `.env` rule matches `inner/.env`, but `inner/` is a separate repository, so the - // app repository never lists the file as ignored and discovery scans it. The finding names the - // nested repository only after confirming the two directories have different top levels. const dir = removeAfterTest(makeApp({'.gitignore': '.env\n'})) git(dir, ['init', '-q', '.']) const inner = join(dir, 'inner') @@ -320,9 +312,6 @@ describe('git status drives severity, not .gitignore text', () => { }) test('says the ignored-file listing failed when git ignores a file that was still scanned', async () => { - // Discovery falls back to scanning everything when git cannot list ignored paths, so a file git - // ignores can reach the rule. The finding must say why it was scanned rather than blame a - // foreign repository. const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) git(dir, ['init', '-q', '.']) const file = {path: '.env', absolutePath: join(dir, '.env'), ext: '', content: trackedEnvSecret()} @@ -339,9 +328,7 @@ describe('git status drives severity, not .gitignore text', () => { }) test('still scans a gitignored secret conservatively when git cannot list the ignored files', async () => { - // End to end: the listing fails (a truncated index leaves `rev-parse` working but makes `ls-files` - // exit with a fatal error), so discovery applies no git exclusions and the ignored .env is hashed - // and reported rather than silently trusted. + // A truncated index makes `ls-files` fail while `rev-parse` still works. const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) git(dir, ['init', '-q', '.']) git(dir, ['add', '.gitignore', 'shopify.app.toml']) @@ -363,9 +350,6 @@ describe('git status drives severity, not .gitignore text', () => { }) test('says why is unknown when an ignored file has the same top level as the app', async () => { - // The listing succeeded and the file is not in a nested repository, so nothing explains why git - // ignores a file discovery still produced. Git DID confirm the file is untracked and ignored, - // so the finding must say the cause is unknown rather than call the ignore status unconfirmed. const dir = removeAfterTest(makeApp({'.gitignore': '.env\n', '.env': trackedEnvSecret()})) git(dir, ['init', '-q', '.']) const file = {path: '.env', absolutePath: join(dir, '.env'), ext: '', content: trackedEnvSecret()} From 7813d1ec7a214422ab98685b93b38778f59f549b Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Wed, 30 Sep 2026 11:52:20 -0700 Subject: [PATCH 3/7] Scan the selected app configuration for secrets when path rules exclude it The selected app configuration was loaded and analyzed by config checks even when gitignored, but its secret scan depended on discovery, so path rules silently dropped it. A token-shaped value in a gitignored shopify.app.staging.toml went unreported, even though deploys send its values to Shopify. A default exclusion had the same effect: `*.test.*` matches shopify.app.test.toml. The selected configuration is an explicit input, so it now joins the secret-scan inventory whenever it loads. --- .../app-security-engine/scanners/index.ts | 6 ++++-- .../tests/discovery-safety.test.ts | 20 +++++++++++++++++-- 2 files changed, 22 insertions(+), 4 deletions(-) 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 8642ca4b7ce..df1f77a9299 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 @@ -587,7 +587,6 @@ export async function scan(startPath?: string, configFileName?: string): Promise const selectedFileName = getAppConfigurationFileName(configFileName) const appToml = loadAppToml(joinPath(appRoot, selectedFileName), appRoot) const appTomls = appToml ? [appToml] : [] - // The selected app configuration is loaded even if gitignored: path rules only apply to discovery. const gitIgnoreListing = await listGitIgnoredPaths(appRoot) const pathRules = buildPathRules({ gitIgnoredPaths: gitIgnoreListing.status === 'listed' ? gitIgnoreListing.paths : [], @@ -596,7 +595,10 @@ export async function scan(startPath?: string, configFileName?: string): Promise const extensions = findExtensions(appRoot, repositoryFiles) const sourceCandidates = findSourceCandidates(repositoryFiles) const sourceFiles = findAppSourceFiles(appRoot, repositoryFiles) - const sensitiveFiles = findSensitiveFiles(appRoot, repositoryFiles, selectedFileName) + // 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 ? [...new Set([...repositoryFiles, selectedFileName])].sort() : repositoryFiles + const sensitiveFiles = findSensitiveFiles(appRoot, sensitivePaths, selectedFileName) const manifestPaths = findManifestPaths(repositoryFiles) const manifests = findManifests(appRoot, manifestPaths) const dependencyAutomation = manifests.some(manifestHasDependencies) 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 0356ac17453..64756c74f12 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 @@ -191,6 +191,20 @@ describe('repository discovery exclusions', () => { expect(paths).not.toContain('packages/service/lib/index.spec.js') }) + 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, { + 'shopify.app.toml': appConfiguration, + // `*.test.*` is a default exclusion. + 'shopify.app.test.toml': `name = "Test"\napplication_url = "https://test.example.com/?token=${secret}"\n`, + }) + + const result = await scan(root, 'test') + expect(result.app.name).toBe('Test') + expect(secretFindingFiles(result)).toEqual(['shopify.app.test.toml']) + }) + test('walks dot-folders and dotfiles', async () => { const root = await makeDirectory() const secret = ['shp', `at_${'0123456789abcdef'.repeat(2)}`].join('') @@ -540,16 +554,18 @@ describe('gitignore-driven exclusions', () => { expect(hashedPaths(await scan(root))).toContain('tmp/scratch.ts') }) - test('loads a gitignored selected app configuration file', async () => { + 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', - 'shopify.app.staging.toml': 'name = "Staging"\napplication_url = "https://staging.example.com"\n', + 'shopify.app.staging.toml': `name = "Staging"\napplication_url = "https://staging.example.com/?token=${secret}"\n`, }) const result = await scan(root, 'staging') expect(result.app.name).toBe('Staging') expect(hashedPaths(result)).toContain('shopify.app.staging.toml') + expect(secretFindingFiles(result)).toEqual(['shopify.app.staging.toml']) }) }) From eac37a98432e8e40185abef5a579f7b6d452e80d Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Wed, 30 Sep 2026 12:02:02 -0700 Subject: [PATCH 4/7] Remove App Security gitignore changeset App Security stays out of public release notes until it ships (#8698). --- .changeset/app-security-gitignore.md | 5 ----- 1 file changed, 5 deletions(-) delete mode 100644 .changeset/app-security-gitignore.md diff --git a/.changeset/app-security-gitignore.md b/.changeset/app-security-gitignore.md deleted file mode 100644 index 7c2bed41a26..00000000000 --- a/.changeset/app-security-gitignore.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -'@shopify/app': patch ---- - -`app security check` no longer scans gitignored files, and now scans dot-folders such as `.github`. From c4de801d92f6f87a7125fe53f15cacd6dcf2bcd3 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Wed, 30 Sep 2026 12:03:20 -0700 Subject: [PATCH 5/7] Bump COMMITTED_SECRET to version 3 The check now reports ignored files that discovery still scanned, with the reason, instead of skipping them, and it scans the selected app configuration even when path rules exclude it. Findings and the execution record carry the new version. --- .../services/app-security-engine/checks/COMMITTED_SECRET.md | 2 +- .../src/cli/services/app-security-engine/checks/embedded.ts | 2 +- .../app/src/cli/services/app-security-engine/scanners/index.ts | 2 +- .../app-security-engine/tests/deterministic-rules.test.ts | 2 +- .../services/app-security-engine/tests/secret-safety.test.ts | 3 +++ 5 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md index b0b3cbec5f9..4c9af737b67 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md +++ b/packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md @@ -1,6 +1,6 @@ --- id: COMMITTED_SECRET -version: 2 +version: 3 severity: high --- diff --git a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts index 645089ce9ec..5486b5324e2 100644 --- a/packages/app/src/cli/services/app-security-engine/checks/embedded.ts +++ b/packages/app/src/cli/services/app-security-engine/checks/embedded.ts @@ -7,7 +7,7 @@ export const EMBEDDED_CHECK_SOURCES: ReadonlyArray = [ "---\nid: ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS\nversion: 1\nseverity: high\n---\n\n# Active Uploads And Privileged Previews\n\nFind cases where merchant-, customer-, webhook-, or external-service-supplied\nfiles become active content in a privileged origin. Trace uploads, imports,\npreviews, and generated assets from ingestion through storage and final render.\n\nThe risk is not the upload alone. The risk is an untrusted-upload-to-active-render\npath: SVG, HTML, XML, PDF, blob/data URL, or another active format is accepted and\nlater rendered in a storefront, embedded admin, customer-account, theme-editor,\nor operator/admin context where it can execute or leak protected data.\n\n## What to look for\n\n1. **Find upload and import entry points.** Search for file uploads, import jobs,\n webhook attachments, remote fetches, document parsers, blob/data URL handling,\n and generated preview endpoints.\n\n2. **Trace file metadata and validation.** Check size limits, extension checks,\n declared MIME type, magic-byte/file-signature verification, filename handling,\n generated storage names, antivirus/sanitization, and any image/PDF re-encoding.\n\n3. **Inspect storage and serving boundaries.** Determine whether the object is\n stored on a non-executable origin, served with explicit `Content-Type` and\n `Content-Disposition`, and prevented from inheriting privileged cookies or\n browser authority.\n\n4. **Follow every final renderer.** Check storefront/theme renderers, embedded\n admin previews, customer-account views, email/PDF previews, admin/operator\n tools, iframe/srcdoc/blob/data URL renderers, and any browser code that inserts\n the uploaded content into the DOM.\n\n5. **Check sandboxing and isolation.** Verify iframes, preview origins, CSP,\n download headers, SVG sanitization, PDF handling, and re-encoding before\n deciding the content is safe.\n\n## What to report\n\nReport a finding only for a complete untrusted-upload-to-active-render path where\nthe uploaded or imported object is actually rendered or served into a privileged\nexecutable context. Show:\n- who controls the uploaded/imported content;\n- which validation or isolation boundary is missing;\n- where the content becomes active or executable;\n- which privileged origin or user is affected; and\n- file/line evidence for both the ingest path and the renderer/serving path.\n\nExample:\n\n```json\n{\n \"file\": \"app/controllers/previews_controller.rb\",\n \"line\": 28,\n \"message\": \"Uploaded SVG is rendered inline in the admin preview without sanitization or origin isolation\",\n \"evidence\": [\n { \"file\": \"app/controllers/uploads_controller.rb\", \"line\": 14, \"quote\": \"params[:file]\" },\n { \"file\": \"app/controllers/previews_controller.rb\", \"line\": 28, \"quote\": \"render inline: blob.download\" }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The merchant-controlled SVG is stored without re-encoding and later rendered inline in the embedded admin origin, so script-capable SVG content can execute with merchant authority.\"\n}\n```\n\nDo not report:\n- files that are forced to download and never rendered in an active origin;\n- images/PDFs that are re-encoded or sanitized before serving;\n- isolated preview origins with no privileged cookies, storage, or message bridge;\n- missing deployment details where you cannot establish executable rendering or unsafe serving.\n- permissive content types, inline disposition, or storage/header hygiene issues\n without a concrete privileged renderer or execution surface.\n", "---\nid: APP_PROXY_LIQUID_INJECTION\nversion: 2\nseverity: high\n---\n\n# App Proxy Liquid Injection\n\nTrace verified app-proxy request values into active response bodies, including Liquid and HTML response types. Report only a request-controlled value that reaches an active response; static templates and inert JSON are not findings.\n", "---\nid: APP_PROXY_UNVERIFIED_SIGNATURE\nversion: 2\nseverity: high\n---\n\nFind app proxy endpoints that read proxy parameters without verifying\nthe Shopify signature, allowing an attacker to impersonate Shopify and\nsend fake proxy requests.\n\nApp proxies let an app serve content directly on the merchant's store\nvia a URL like `https://shop.example.com/apps/my-app/proxy`. Shopify\nsigns every proxy request with an HMAC using the app's shared secret.\nIf the app doesn't verify this signature, anyone can send requests to\nthe proxy endpoint with forged parameters — including `shop`,\n`logged_in_customer_id`, and `path_prefix`.\n\n## What to look for\n\n1. **Find app proxy route handlers.** These are endpoints configured as\n app proxies in `shopify.app.toml` under `[app_proxy]` or in the app's\n routing config. They typically read parameters like:\n - `shop` or `shop_id`\n - `logged_in_customer_id`\n - `path_prefix`\n - `signature`\n - `timestamp`\n\n2. **Check for signature verification.** The handler must verify the\n HMAC signature before trusting any proxy parameter. Look for:\n - **Remix:** `authenticate.public.appProxy(request)` — the official\n verification function\n - **Rails:** `verified_request?` or manual HMAC verification using\n `ShopifyApp` utilities\n - **Express:** Manual HMAC verification using the app secret\n - **PHP:** `ShopifyUtils::verifyProxyRequest()` or equivalent\n\n3. **If no verification is present, check whether the handler:**\n - Reads `shop` from the query string and uses it to scope data\n - Reads `logged_in_customer_id` and uses it for authorisation\n - Returns any shop-specific data\n\n If any of these are true and there's no signature check, it's a real\n finding.\n\n4. **Check for the HMAC pattern even if the function name isn't obvious.**\n Some apps implement custom verification:\n - `crypto.createHmac('sha256', API_SECRET)`\n - `OpenSSL::HMAC.digest`\n - `hash_hmac('sha256', ...)`\n - Comparison with `timingSafeEqual` or `secure_compare`\n\n5. **Separate app-local findings from protocol hardening signals.** Missing\n verification is a finding when the handler trusts signed parameters without\n any verification boundary. Weak comparison, unusual canonicalization, or\n delimiterless concatenation is not automatically an app finding: keep it\n unresolved unless you can show a usable victim-signed request path or another\n concrete exploit condition in this app.\n\n## What to report\n\nFor each proxy handler that reads shop/customer parameters without\nsignature verification, or where you can demonstrate a usable victim-signed\nrequest path through a weak verification implementation:\n\n```json\n{\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"message\": \"App proxy handler reads shop parameter without signature verification\",\n \"snippet\": \"const shop = url.searchParams.get('shop')\",\n \"evidence\": [\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 15,\n \"quote\": \"const shop = url.searchParams.get('shop')\"\n },\n {\n \"file\": \"app/routes/proxy.ts\",\n \"line\": 1,\n \"quote\": \"no authenticate.public.appProxy or HMAC verification found\"\n }\n ],\n \"confidence\": \"high\",\n \"reasoning\": \"The handler reads the shop parameter from the query string and uses it to query shop data, but no signature verification is present. An attacker can send requests with any shop parameter.\"\n}\n```\n\nDo not report:\n\n- Handlers that call `authenticate.public.appProxy(request)` (Remix)\n- Handlers with manual HMAC verification\n- Handlers that return only static content (no shop-specific data)\n- Protocol-only canonicalization concerns with no demonstrated app-local exploit path\n- Test handlers\n", - "---\nid: COMMITTED_SECRET\nversion: 2\nseverity: high\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n\nDo not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format.\n", + "---\nid: COMMITTED_SECRET\nversion: 3\nseverity: high\n---\n\n# Committed Secret\n\nInspect files skipped by deterministic secret scanning for committed credentials. Never quote or reproduce a secret; cite only the file and redacted credential kind, and recommend rotation.\n\nDo not report placeholders, public client identifiers (`SHOPIFY_API_KEY`, Stripe `pk_`), or files git confirms are untracked and ignored. Template env files (`.env.example`, `.sample`, `.template`, `.dist`) are findings only when they contain a known credential format.\n", "---\nid: CREDENTIAL_BROWSER_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Browser Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets into loader/HTTP responses, browser globals, DOM values, client bundles, or external requests. Do not report server-only use or safe boolean/redacted/hash-derived values.\n", "---\nid: CREDENTIAL_LOG_LEAKAGE\nversion: 1\nseverity: high\n---\n\n# Credential Log Leakage\n\nTrace credentials, access tokens, session tokens, and client secrets through aliases and helpers to console, logger, telemetry, or error-reporting sinks. Do not report boolean presence checks, deliberate redaction, or one-way hashes.\n", "---\nid: CROSS_SITE_SCRIPTING\nversion: 1\nseverity: high\n---\n\n# Cross-Site Scripting\n\nFind reflected, stored, and client-side paths where lower-trust data becomes\nexecutable browser content across a trust boundary. Cover app-owned HTML pages,\nserver templates, embedded app views, customer-facing pages, and operator UIs,\nnot only theme extensions. Prove the source, rendering context, victim, and\nreachable execution path; an HTML-looking string or raw-rendering API alone is\nnot a finding.\n\n## What to look for\n\n1. **Map producers and consumers.** Identify the actual web frameworks, template\n engines, versions, escaping defaults, and rendering helpers. Trace URL/query\n and form/JSON input, stored customer/merchant content, product/metafield data,\n imports, webhook fields, and third-party API responses to their renderers.\n For client-side flows, include location fragments, storage, DOM attributes,\n and `postMessage` data after examining sender/origin validation. A database\n read, authenticated route, or Shopify API response does not by itself make\n the contained user-authored data trusted.\n\n2. **Inspect server-rendered escape hatches.** Follow response builders, layouts,\n partials, and component wrappers into final HTML, including:\n - Express/React Router HTML responses assembled with strings, and custom\n server-side rendering or hydration/bootstrap data.\n - Rails `raw`, `html_safe`, and HTML/inline rendering; EJS `<%- ... %>`,\n unescaped Handlebars output, Django/Jinja `safe` or disabled autoescaping,\n and Blade/Twig raw output.\n - Markdown/rich-text renderers with raw HTML or unsafe link handling.\n Read the real helper and framework behavior. A bypass API is only a lead;\n constant HTML and correctly escaped dynamic text are not findings.\n\n3. **Follow browser-side rendering and execution.** Inspect DOM HTML writes,\n React `dangerouslySetInnerHTML`, Vue `v-html`, Svelte `{@html}`, Lit\n `unsafeHTML`, Angular trust-bypass APIs, and wrappers around these sinks.\n Also inspect dynamic script URLs, event handlers, `srcdoc`, and strings\n passed to browser `eval`, `Function`, or timers. Follow stored content from\n its original write to later preview, support, or admin views; do not stop\n at a safe first renderer. Coordinate these paths with the existing checks\n listed below instead of creating duplicate findings.\n\n4. **Evaluate the exact output context.** Determine whether input lands in HTML\n text, a quoted/unquoted attribute, a URL, JavaScript data/code, CSS, or a nested\n context. HTML text escaping is not sufficient for an event handler or a\n script/URL context. URL encoding a parameter is not scheme validation for an\n entire `href` or `src`. For JSON embedded inside an HTML `` breakout; `JSON.stringify` alone does not do this. Do not\n invent the same breakout for JSON served as an inert `application/json`\n response. Trace any later consumer that reparses it as HTML or code.\n\n5. **Verify defenses at the final sink.** Follow escaping, HTML sanitization,\n URL allowlists, framework autoescaping, Trusted Types policies, and any\n decoding or mutations after sanitization. Check actual configuration and\n context, not just a sanitizer's name. A custom sanitizer is not automatically\n vulnerable, and a library call is not automatically safe in every context.\n To report a bypass, explain the concrete construct that survives the defense\n and can execute in that renderer. Account for enforced CSP and sandboxing;\n do not assume a bypass. Missing CSP alone is not XSS, and HttpOnly cookies\n do not prevent script from acting with the victim's browser authority.\n\n6. **Establish a victim and authority boundary.** Identify who can supply the\n input, how another principal encounters it, the document's actual origin,\n and the actions or data script could reach there. An embedded app iframe\n does not inherit the Shopify Admin parent's origin or authority. Content\n executing in an isolated or sandboxed origin must not be described as\n executing in the parent without evidence. Intentional author-controlled\n HTML, self-XSS requiring the victim to paste code, and a merchant editing\n their own permitted storefront code are not automatically privilege\n escalation. Show a lower-trust writer reaching a more privileged reader,\n another user/tenant, or a surface where executable content is not authorized.\n\n## Coordinate with existing checks\n\nFollow the complete path even when it crosses surfaces, but report the same\nsource-to-sink vulnerability only once, under the most specific owning check:\n\n- `UNSAFE_INNERHTML`: DOM HTML writes and browser code-evaluation sinks.\n- `THEME_EXTENSION_XSS` / `LIQUID_UNSAFE_RENDER`: theme-extension Liquid output.\n- `TEXT_SETTING_HTML_SMUGGLING`: merchant text settings becoming active content.\n- `APP_PROXY_LIQUID_INJECTION`: values from verified app-proxy requests reaching\n active responses.\n- `SCRIPT_TAG_URL_INJECTION`: Shopify ScriptTag source URLs.\n- `ACTIVE_UPLOADS_AND_PRIVILEGED_PREVIEWS`: uploaded/imported active files and\n their privileged previews.\n\nDelegate only when the specialized check covers the complete source-to-sink\npath, not merely the response surface, and use that check's own provenance.\nUse `CROSS_SITE_SCRIPTING` for remaining paths, such as reflected server HTML,\nstored customer content in an operator template, unsafe hydration data, or\nactive URL/attribute output outside those specialized paths. Stored buyer\nreviews rendered in app-proxy HTML/Liquid responses remain here when the\ncontent comes from a separate submission endpoint rather than the verified\nproxy request. Do not suppress a distinct vulnerable sink just because the\nsame input is used elsewhere.\n\n## What to report\n\nUse the review pack's current finding and execution schemas, including this\ncheck's ID, version, and prompt hash. Each finding must include:\n\n- The controlling principal, entry point, victim interaction, and required access.\n- File/line evidence for input or persistence, transformations, render call,\n and final sink/template, including any ineffective defense.\n- The exact parser context and a minimal inert marker/example showing how data\n becomes executable content. Explain the execution mechanism; do not assume\n a `