diff --git a/.changeset/app-security-dependency-automation-repository-root.md b/.changeset/app-security-dependency-automation-repository-root.md new file mode 100644 index 00000000000..ef03df1d659 --- /dev/null +++ b/.changeset/app-security-dependency-automation-repository-root.md @@ -0,0 +1,5 @@ +--- +'@shopify/app': patch +--- + +Check for Dependabot or Renovate configuration at the Git repository root when `shopify app security check` scans an app below it 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 a3dbf7f9c54..9cd03dc45f5 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 @@ -649,6 +649,18 @@ function readRepositoryFile(absolutePath: string): RepositoryReadResult { return result } +/** + * Read a fixed path below a directory that isn't scanned, with that directory as the containment boundary. + * `undefined` means the path doesn't exist. + */ +function readFileContainedBy(directory: string, path: string): RepositoryReadResult | undefined { + const inspected = inspectRepositoryPath(directory, path) + if (inspected.status === 'missing') return undefined + const result = inspected.status === 'file' ? readBoundedFile(inspected.path) : repositoryPathFailure(inspected.reason) + if (!result.ok) recordSkippedFile(joinPath(directory, path), result) + return result +} + function readRepositoryText(absolutePath: string): string | undefined { const result = readRepositoryFile(absolutePath) return result.ok ? result.content.toString() : undefined @@ -798,17 +810,20 @@ export function findSensitiveFiles( }) } -function nestedRepositoryReason(appRoot: string): string | undefined { +/** Dependabot and Renovate read configuration from the root of the nearest repository that holds the app. */ +function dependencyAutomationRoot(appRoot: string): {directory: string} | {unresolvedReason: string} { 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' + if (marker.status === 'ambiguous') return {unresolvedReason: marker.reason} + return {directory: marker.status === 'found' ? marker.directory : appRoot} } /** * Read local bot configuration only; hosted integrations and CI workflows are - * outside this check's scope. Only gathered paths are read, through the reader, - * so a gathered symbolic link that leaves its scan directory is reported as unresolved. + * outside this check's scope. Configuration inside a scan directory is read only + * when it was gathered, through the reader, so path rules apply and a gathered + * symbolic link that leaves its scan directory is reported as unresolved. The root + * of a repository that holds the app below its top level may be outside every scan + * directory, so an allowlisted path outside them is read directly, contained by that root. */ export function findDependencyAutomationInputs( appRoot: string, @@ -825,16 +840,23 @@ export function findDependencyAutomationInputs( return {files: [], unresolvedReason: inspectErrorReason('app root', error)} } - const repositoryReason = nestedRepositoryReason(canonicalRoot) - if (repositoryReason) return {files: [], unresolvedReason: repositoryReason} + const configurationRoot = dependencyAutomationRoot(canonicalRoot) + if ('unresolvedReason' in configurationRoot) return {files: [], unresolvedReason: configurationRoot.unresolvedReason} + const repositoryRoot = configurationRoot.directory + const {scanDirectories} = configuredReader() + const isScanned = (absolutePath: string) => + repositoryRoot === canonicalRoot || scanDirectories.some((directory) => isSubpath(directory, absolutePath)) const gathered = new Set(gatheredPaths) const files: SourceFile[] = [] let unresolvedReason: string | undefined - for (const relative of DEPENDENCY_AUTOMATION_CONFIG_PATHS) { - if (!gathered.has(relative)) continue - const absolutePath = joinPath(canonicalRoot, relative) - const result = readRepositoryFile(absolutePath) + for (const configurationPath of DEPENDENCY_AUTOMATION_CONFIG_PATHS) { + const absolutePath = joinPath(repositoryRoot, configurationPath) + const relative = normalizeCliPath(relativePath(canonicalRoot, absolutePath)) + const scanned = isScanned(absolutePath) + if (scanned && !gathered.has(relative)) continue + const result = scanned ? readRepositoryFile(absolutePath) : readFileContainedBy(repositoryRoot, configurationPath) + if (result === undefined) continue if (!result.ok) { unresolvedReason ??= result.reason === 'too_large' 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 649f887e467..1e2ea9a043f 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 @@ -103,7 +103,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray { }) }) - test.each(['directory', 'worktree file'])('respects repository %s boundaries', async (marker) => { - await inTemporaryDirectory(async (repository) => { + describe('apps below the repository root', () => { + async function writeRepositoryMarker(directory: string, marker: string): Promise { + if (marker === 'directory') await mkdir(join(directory, '.git')) + else await writeFile(join(directory, '.git'), 'gitdir: /outside/not-read') + } + + async function makeMonorepo(repository: string, marker: string): Promise { const app = join(repository, 'apps', 'example') - 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 = await findInputs(app) - expect(nested).toMatchObject({ - files: [], - unresolvedReason: 'App root is nested below a parent Git repository', + await mkdir(app, {recursive: true}) + await writeRepositoryMarker(repository, marker) + return app + } + + test.each(['directory', 'worktree file'])( + 'reads configuration at the root of a %s repository that is not scanned', + async (marker) => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, marker) + await expect(findInputs(app)).resolves.toEqual({files: []}) + const content = 'version: 2\nupdates: []\n' + await writeFiles(repository, {'.github/dependabot.yml': content}) + const result = await findInputs(app) + expect(result.unresolvedReason).toBeUndefined() + expect(result.files).toEqual([ + expect.objectContaining({ + path: '../../.github/dependabot.yml', + absolutePath: joinPath(repository, '.github/dependabot.yml'), + ext: '.yml', + content, + }), + ]) + }) + }, + ) + + test('ignores configuration in the app directory, which bots do not read below the repository root', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, 'directory') + await writeFiles(app, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) + await expect(findInputs(app)).resolves.toEqual({files: []}) + }) + }) + + test.each(['directory', 'worktree file'])( + 'keeps an app with its own %s repository inside that repository', + async (marker) => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, 'directory') + await writeFiles(repository, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) + await writeRepositoryMarker(app, marker) + await expect(findInputs(app)).resolves.toEqual({files: []}) + await writeFiles(app, {'renovate.json': '{}'}) + expect((await findInputs(app)).files).toMatchObject([{path: 'renovate.json'}]) + }) + }, + ) + + test('applies path rules to the repository root when it is a scan directory', async () => { + await inTemporaryDirectory(async (repository) => { + vi.stubEnv('INIT_CWD', repository) + const app = await makeMonorepo(repository, 'directory') + await writeFiles(repository, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) + const findScanningRoot = async (excludePatterns: string[]) => { + configureRepositoryReader({appDirectory: app, scanDirectories: [repository], explicitInputs: new Set()}) + const {paths} = await gatherPaths({ + appDirectory: app, + scanDirectories: [repository], + requestedScanDirectories: [app, repository], + rules: createPathRules({excludePatterns, noGitIgnore: true}), + }) + return findDependencyAutomationInputs(app, paths) + } + expect((await findScanningRoot([])).files).toMatchObject([{path: '../../.github/dependabot.yml'}]) + await expect(findScanningRoot(['.github'])).resolves.toEqual({files: []}) + }) + }) + + async function findIncludingGitHubDirectory( + repository: string, + app: string, + rules: {excludePatterns?: string[]; noGitIgnore?: boolean}, + ) { + const scanDirectories = [app, joinPath(repository, '.github')] + configureRepositoryReader({appDirectory: app, scanDirectories, explicitInputs: new Set()}) + const {paths} = await gatherPaths({ + appDirectory: app, + scanDirectories, + requestedScanDirectories: scanDirectories, + rules: createPathRules({excludePatterns: [], noGitIgnore: true, ...rules}), + }) + return findDependencyAutomationInputs(app, paths) + } + + test('reads configuration that an included directory below the repository root gathered', async () => { + await inTemporaryDirectory(async (repository) => { + vi.stubEnv('INIT_CWD', repository) + const app = await makeMonorepo(repository, 'directory') + await writeFiles(repository, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) + expect((await findIncludingGitHubDirectory(repository, app, {})).files).toMatchObject([ + {path: '../../.github/dependabot.yml', absolutePath: joinPath(repository, '.github/dependabot.yml')}, + ]) + }) + }) + + test('applies exclusions to configuration in an included directory below the repository root', async () => { + await inTemporaryDirectory(async (repository) => { + vi.stubEnv('INIT_CWD', repository) + const app = await makeMonorepo(repository, 'directory') + await writeFiles(repository, {'.github/dependabot.yml': 'version: 2\nupdates: []'}) + const excludePatterns = ['.github/dependabot.yml'] + await expect(findIncludingGitHubDirectory(repository, app, {excludePatterns})).resolves.toEqual({files: []}) + expect(getSkippedFiles()).toEqual([]) + + await writeFiles(repository, {'renovate.json': '{}'}) + const result = await findIncludingGitHubDirectory(repository, app, {excludePatterns}) + expect(result.files).toMatchObject([{path: '../../renovate.json', content: '{}'}]) + expect(result.unresolvedReason).toBeUndefined() + }) + }) + + test('does not read Git-ignored configuration in an included directory below the repository root', async () => { + await inTemporaryDirectory(async (repository) => { + const app = joinPath(repository, 'apps/example') + await mkdir(app, {recursive: true}) + execFileSync('git', ['init', '--quiet'], {cwd: repository}) + await writeFiles(repository, { + '.gitignore': '.github/dependabot.yml\n', + '.github/dependabot.yml': 'version: 2\nupdates: []', + }) + await expect(findIncludingGitHubDirectory(repository, app, {noGitIgnore: false})).resolves.toEqual({ + files: [], + }) + }) + }) + + test('does not follow a root configuration link that leaves the repository', async () => { + await inTemporaryDirectory(async (repository) => { + await inTemporaryDirectory(async (outside) => { + const app = await makeMonorepo(repository, 'directory') + await writeFile(join(outside, 'dependabot.yml'), 'version: 2') + await mkdir(join(repository, '.github')) + await symlink(join(outside, 'dependabot.yml'), join(repository, '.github/dependabot.yml')) + const result = await findInputs(app) + expect(result).toMatchObject({files: [], unresolvedReason: expect.stringContaining('outside')}) + expect(result.unresolvedReason).not.toContain(repository) + expect(result.unresolvedReason).not.toContain(outside) + expect(getSkippedFiles()).toContainEqual( + expect.objectContaining({path: '../../.github/dependabot.yml', reason: 'unreadable'}), + ) + }) + }) + }) + + test('preserves bounded reads of root configuration', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, 'directory') + await writeFiles(repository, {'.github/dependabot.yml': 'x'.repeat(500_001)}) + await expect(findInputs(app)).resolves.toMatchObject({ + files: [], + unresolvedReason: expect.stringContaining('too large'), + }) + expect(getSkippedFiles()).toEqual([ + expect.objectContaining({path: '../../.github/dependabot.yml', reason: 'too_large', size_bytes: 500_001}), + ]) }) - 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((await findInputs(app)).files).toMatchObject([{path: '.github/dependabot.yml'}]) }) }) 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 d8e076d0b28..bd543b6d2de 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 @@ -4,7 +4,7 @@ import {scanAppDirectory as scanApp, scanDirectory as scan} from './scan-directo import {securityExitCode} from '../../app-security-api.js' import {formatJson} from '../output/format.js' import {getRegistry} from '../registry/index.js' -import {DETERMINISTIC_CHECKS} from '../scanners/index.js' +import {DETERMINISTIC_CHECKS, scan as scanInput} from '../scanners/index.js' import {translateFindingsDocument} from '../results/translate.js' import {inTemporaryDirectory} from '@shopify/cli-kit/node/fs' import {fetch} from '@shopify/cli-kit/node/http' @@ -12,7 +12,7 @@ import {captureOutputWithExitCode} from '@shopify/cli-kit/node/system' import {afterEach, beforeEach, describe, expect, test, vi} from 'vitest' import {mkdir, writeFile} from 'node:fs/promises' import {basename, dirname, join} from 'node:path' -import type {ScanResult} from '../types.js' +import type {ScanOptions, ScanResult} from '../types.js' vi.mock('@shopify/cli-kit/node/http', async (importActual) => { const actual: any = await importActual() @@ -68,7 +68,7 @@ function dependencyFindings(result: ScanResult) { describe('dependency automation scanner integration', () => { test('registers one framework-independent low-severity structured-config check', () => { expect(DETERMINISTIC_CHECKS.get(checkId)).toMatchObject({ - version: 1, + version: 2, lifecycle: 'active', analysisMode: 'structured_config', target: 'dependency_automation', @@ -273,16 +273,77 @@ describe('dependency automation scanner integration', () => { }) }) - test('does not inspect repository-level configuration outside a nested app root', async () => { - await inTemporaryDirectory(async (repository) => { - const app = join(repository, 'apps/example') - await mkdir(join(repository, '.git')) - await makeApp(app, {'.github/dependabot.yml': dependabot}) - const result = await scan(app) - expect(dependencyFindings(result)).toEqual([]) - expect(dependencyExecution(result)).toMatchObject({ - status: 'unresolved', - reason: {message: 'App root is nested below a parent Git repository'}, + describe('apps below the repository root', () => { + /** The assessment's monorepo layout: a workspace root holding the app in `apps/foo`. */ + async function makeMonorepo(repository: string, rootFiles: Record = {}): Promise { + const app = join(repository, 'apps/foo') + await writeFiles(repository, { + 'package.json': JSON.stringify({private: true, workspaces: ['apps/*', 'packages/*']}), + 'packages/server/package.json': JSON.stringify({dependencies: {'@shopify/shopify-app-react-router': '^1.0.0'}}), + ...rootFiles, + }) + await makeApp(app) + git(repository, ['init', '-q', '.']) + git(repository, ['add', '-A']) + git(repository, ['commit', '-qm', 'init']) + return app + } + + function scanFrom(app: string, scanDirectories: string[], options?: ScanOptions) { + return scanInput( + { + appDirectory: app, + scanDirectories, + requestedScanDirectories: [app, ...scanDirectories.filter((directory) => directory !== app)], + appConfigFilePath: join(app, 'shopify.app.toml'), + }, + options, + ) + } + + test('reads configuration at the repository root when only the app directory is scanned', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, {'.github/dependabot.yml': dependabot}) + const result = await scan(app) + expect(dependencyFindings(result)).toEqual([]) + expect(dependencyExecution(result)).toMatchObject({ + status: 'executed', + findings: 0, + inspected_files: ['package.json', '../../.github/dependabot.yml'], + }) + expect(result.scan.coverage_gaps).toEqual([]) + }) + }) + + test('reads configuration at the repository root when it is a scan directory', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, {'renovate.json': '{}'}) + const result = await scanFrom(app, [repository]) + expect(dependencyFindings(result)).toEqual([]) + expect(dependencyExecution(result)).toMatchObject({ + status: 'executed', + findings: 0, + inspected_files: expect.arrayContaining(['../../package.json', '../../renovate.json']), + }) + }) + }) + + test('reports missing configuration at the repository root', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, {'apps/foo/.github/dependabot.yml': dependabot}) + const result = await scan(app) + expect(dependencyFindings(result)).toEqual([expect.objectContaining({location: {file: 'package.json'}})]) + expect(dependencyExecution(result)).toMatchObject({status: 'executed', findings: 1}) + }) + }) + + test('does not count the root configuration of a parent repository for an app with its own repository', async () => { + await inTemporaryDirectory(async (repository) => { + const app = await makeMonorepo(repository, {'.github/dependabot.yml': dependabot}) + git(app, ['init', '-q', '.']) + const result = await scan(app) + expect(dependencyFindings(result)).toHaveLength(1) + expect(dependencyExecution(result)).toMatchObject({status: 'executed', inspected_files: ['package.json']}) }) }) }) diff --git a/packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts b/packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts index 85f9dadbcf0..473ef7c1e0f 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts @@ -52,6 +52,7 @@ describe('deterministic rules product contract', () => { expect(DETERMINISTIC_CHECKS.get('INSECURE_WEBHOOK_URL')?.version).toBe(2) expect(DETERMINISTIC_CHECKS.get('COMMITTED_SECRET')?.version).toBe(3) expect(DETERMINISTIC_CHECKS.get('UNAUTHENTICATED_ENDPOINT')?.version).toBe(2) + expect(DETERMINISTIC_CHECKS.get('MISSING_DEPENDENCY_SECURITY_AUTOMATION')?.version).toBe(2) }) test('extracts security fields from parsed TOML without source regexes', () => {