From f96789b90114e251969d7d21b5a80689683d601b Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 20 Oct 2026 14:39:42 -0700 Subject: [PATCH 1/4] Move the app security check presenter to security-check/result.ts `securityCheck` now returns what it did instead of rendering it: the listed files, the finished scan with its `--blocking` outcome, or a cancellation when the user declines to scan without app configuration. It still records the telemetry. The command passes the outcome to a presenter in security-check/, which renders the gathering warnings and the report, offers and delivers the coding-agent instructions, and sets exit code 1 when `--blocking` is breached. The tip that repeats a prompted selection is still shown before the scan, through a callback that the command passes in. `resolveAppSecuritySelection` returns declining to scan without app configuration as an outcome instead of aborting, so that `--json` can report it. In the terminal the presenter aborts with the same error as before. The text output, prompts and exit codes don't change. --- .../cli/commands/app/security/check.test.ts | 52 +- .../src/cli/commands/app/security/check.ts | 13 +- .../src/cli/services/app-security-api.test.ts | 42 +- .../app/src/cli/services/app-security-api.ts | 12 +- .../tests/dependency-automation.test.ts | 10 +- .../tests/layout-catalogue.test.ts | 12 +- .../services/app-security-selection.test.ts | 11 +- .../cli/services/app-security-selection.ts | 41 +- .../cli/services/security-check.test-data.ts | 89 +++ .../src/cli/services/security-check.test.ts | 737 ++++-------------- .../app/src/cli/services/security-check.ts | 238 ++---- .../services/security-check/result.test.ts | 465 +++++++++++ .../src/cli/services/security-check/result.ts | 161 ++++ .../src/cli/services/security-review.test.ts | 3 +- 14 files changed, 1061 insertions(+), 825 deletions(-) create mode 100644 packages/app/src/cli/services/security-check.test-data.ts create mode 100644 packages/app/src/cli/services/security-check/result.test.ts create mode 100644 packages/app/src/cli/services/security-check/result.ts diff --git a/packages/app/src/cli/commands/app/security/check.test.ts b/packages/app/src/cli/commands/app/security/check.test.ts index cd8b04f5696..77fc3ad535f 100644 --- a/packages/app/src/cli/commands/app/security/check.test.ts +++ b/packages/app/src/cli/commands/app/security/check.test.ts @@ -1,6 +1,7 @@ import SecurityCheck from './check.js' import {appFlags} from '../../../flags.js' import securityCheck from '../../../services/security-check.js' +import {renderSecurityCheckPromptsTip, renderSecurityCheckResult} from '../../../services/security-check/result.js' import AppLinkedCommand from '../../../utilities/app-linked-command.js' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags} from '@shopify/cli-kit/node/cli' @@ -9,6 +10,12 @@ import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {describe, expect, test, vi} from 'vitest' vi.mock('../../../services/security-check.js') +vi.mock('../../../services/security-check/result.js') + +/** The presentation options that the command passed with the outcome. */ +function presentationOptions() { + return vi.mocked(renderSecurityCheckResult).mock.calls[0]?.[1] +} async function expectInstructionFlagConflict(flags: string[]): Promise { const outputMock = mockAndCaptureOutput() @@ -69,7 +76,10 @@ describe('app security check command', () => { ]) }) - test('forwards --path and flags to the service', async () => { + test('forwards --path and flags to the service and presents its outcome', async () => { + const check = {kind: 'cancelled'} as const + vi.mocked(securityCheck).mockResolvedValue(check) + await SecurityCheck.run( ['--path', './fixtures/unlinked-app', '--verbose', '--blocking', 'high', '--skip-instructions'], import.meta.url, @@ -80,14 +90,18 @@ describe('app security check command', () => { configName: undefined, clientId: undefined, withoutAppConfig: false, - verbose: true, blocking: 'high', - printInstructions: false, - skipInstructions: true, includeDirs: [], excludePatterns: [], noGitIgnore: false, listFiles: false, + onSelectionPrompted: renderSecurityCheckPromptsTip, + }) + expect(renderSecurityCheckResult).toHaveBeenCalledWith(check, { + directory: resolvePath('./fixtures/unlinked-app'), + verbose: true, + printInstructions: false, + skipInstructions: true, }) }) @@ -98,7 +112,7 @@ describe('app security check command', () => { ) expect(securityCheck).toHaveBeenCalledWith( - expect.objectContaining({excludePatterns: ['generated', '../shared/**', 'a b/'], skipInstructions: true}), + expect.objectContaining({excludePatterns: ['generated', '../shared/**', 'a b/']}), ) }) @@ -108,15 +122,13 @@ describe('app security check command', () => { import.meta.url, ) - expect(securityCheck).toHaveBeenCalledWith( - expect.objectContaining({includeDirs: ['../backend', './lib/', 'a b'], skipInstructions: true}), - ) + expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({includeDirs: ['../backend', './lib/', 'a b']})) }) test('forwards --no-git-ignore', async () => { await SecurityCheck.run(['--no-git-ignore', '--skip-instructions'], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({noGitIgnore: true, skipInstructions: true})) + expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({noGitIgnore: true})) }) test('reads --include-dir and --exclude only from the command line', () => { @@ -160,7 +172,7 @@ describe('app security check command', () => { await SecurityCheck.run([], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({printInstructions: false})) + expect(presentationOptions()).toMatchObject({printInstructions: false}) expect(warningSpy).not.toHaveBeenCalled() } finally { warningSpy.mockRestore() @@ -184,13 +196,9 @@ describe('app security check command', () => { await SecurityCheck.run(['--path', '/tmp/directory-without-shopify-toml', ...flags], import.meta.url) expect(securityCheck).toHaveBeenCalledWith( - expect.objectContaining({ - directory: '/tmp/directory-without-shopify-toml', - printInstructions: true, - withoutAppConfig: false, - skipInstructions: false, - }), + expect.objectContaining({directory: '/tmp/directory-without-shopify-toml', withoutAppConfig: false}), ) + expect(presentationOptions()).toMatchObject({printInstructions: true, skipInstructions: false}) if (deprecated) { expect(warningSpy).toHaveBeenCalledOnce() expect(warningSpy).toHaveBeenCalledWith(expect.stringContaining('--print-instructions')) @@ -217,7 +225,7 @@ describe('app security check command', () => { try { await SecurityCheck.run([], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({printInstructions})) + expect(presentationOptions()).toMatchObject({printInstructions}) if (deprecated) { expect(warningSpy).toHaveBeenCalledOnce() expect(warningSpy).toHaveBeenCalledWith(expect.stringContaining('--print-instructions')) @@ -245,7 +253,7 @@ describe('app security check command', () => { try { await SecurityCheck.run([], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({printInstructions})) + expect(presentationOptions()).toMatchObject({printInstructions}) expect(warningSpy).toHaveBeenCalledOnce() expect(warningSpy).toHaveBeenCalledWith(expect.stringContaining('--print-instructions')) } finally { @@ -269,7 +277,7 @@ describe('app security check command', () => { try { await SecurityCheck.run([flag], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({printInstructions: true})) + expect(presentationOptions()).toMatchObject({printInstructions: true}) expect(warningSpy).toHaveBeenCalledOnce() expect(warningSpy).toHaveBeenCalledWith(expect.stringContaining('--print-instructions')) } finally { @@ -322,9 +330,7 @@ describe('app security check command', () => { test('forwards --client-id and --without-app-config', async () => { await SecurityCheck.run(['--without-app-config', '--client-id', 'abc123', '--skip-instructions'], import.meta.url) - expect(securityCheck).toHaveBeenCalledWith( - expect.objectContaining({clientId: 'abc123', withoutAppConfig: true, skipInstructions: true}), - ) + expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({clientId: 'abc123', withoutAppConfig: true})) }) test.each([ @@ -353,7 +359,7 @@ describe('app security check command', () => { import.meta.url, ) - expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({configName: 'staging', skipInstructions: true})) + expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({configName: 'staging'})) }) test.each(['--findings', '--clean'])('rejects the removed %s flag', async (removedFlag) => { diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 0d5c7563bf4..368eeb8d8aa 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -1,6 +1,7 @@ import {appSecurityBlockingFlag} from './blocking-flag.js' import {appSecuritySelectionFlags} from './selection-flags.js' import securityCheck from '../../../services/security-check.js' +import {renderSecurityCheckPromptsTip, renderSecurityCheckResult} from '../../../services/security-check/result.js' import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags} from '@shopify/cli-kit/node/cli' @@ -89,19 +90,23 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de public async run(): Promise { const {flags} = await this.parse(SecurityCheck) - await securityCheck({ + const check = await securityCheck({ directory: flags.path, configName: flags.config, clientId: flags['client-id'], withoutAppConfig: Boolean(flags['without-app-config']), - verbose: Boolean(flags.verbose), blocking: flags.blocking, - printInstructions: Boolean(flags['print-instructions'] || flags.yes), - skipInstructions: flags['skip-instructions'], includeDirs: flags['include-dir'] ?? [], excludePatterns: flags.exclude ?? [], noGitIgnore: Boolean(flags['no-git-ignore']), listFiles: Boolean(flags['list-files']), + onSelectionPrompted: renderSecurityCheckPromptsTip, + }) + await renderSecurityCheckResult(check, { + directory: flags.path, + verbose: Boolean(flags.verbose), + printInstructions: Boolean(flags['print-instructions'] || flags.yes), + skipInstructions: flags['skip-instructions'], }) } } diff --git a/packages/app/src/cli/services/app-security-api.test.ts b/packages/app/src/cli/services/app-security-api.test.ts index c31e7f7b0fe..133dabb02cf 100644 --- a/packages/app/src/cli/services/app-security-api.test.ts +++ b/packages/app/src/cli/services/app-security-api.test.ts @@ -1,5 +1,5 @@ import { - securityExitCode, + blockedCheckCount, executeAppSecurity, type AppSecurityBlockingLevel, type AppSecurityExecution, @@ -32,7 +32,7 @@ afterEach(() => { async function runSecurity(options: {directory: string; blocking: AppSecurityBlockingLevel}) { const execution = await executeAppSecurity(scanInputFor(options.directory)) const artifacts = await writeCheckArtifacts(options.directory, 'shopify.app', execution) - return {execution, artifacts, exitCode: securityExitCode(execution, options.blocking)} + return {execution, artifacts, blockedChecks: blockedCheckCount(execution, options.blocking)} } async function createApp(directory: string, source = 'export const loader = () => ({ok: true})'): Promise { @@ -49,19 +49,31 @@ async function createApp(directory: string, source = 'export const loader = () = return sourcePath } -function executionWithIssues(severities: Issue['severity'][]): AppSecurityExecution { - return { - scan: {issues: severities.map((severity) => ({severity}))}, - } as unknown as AppSecurityExecution +function executionWithIssues(issues: Pick[]): AppSecurityExecution { + return {scan: {issues}} as unknown as AppSecurityExecution } -describe('securityExitCode', () => { - test('returns 1 only when an issue meets the blocking severity', () => { - expect(securityExitCode(executionWithIssues(['medium']), 'high')).toBe(0) - expect(securityExitCode(executionWithIssues(['medium']), 'medium')).toBe(1) - expect(securityExitCode(executionWithIssues(['medium']), 'low')).toBe(1) - expect(securityExitCode(executionWithIssues(['high']), 'none')).toBe(0) - expect(securityExitCode(executionWithIssues([]), 'low')).toBe(0) +describe('blockedCheckCount', () => { + test('counts the checks with an issue that meets the blocking severity', () => { + const medium = {id: 'OPEN_REDIRECT', severity: 'medium'} as const + expect(blockedCheckCount(executionWithIssues([medium]), 'high')).toBe(0) + expect(blockedCheckCount(executionWithIssues([medium]), 'medium')).toBe(1) + expect(blockedCheckCount(executionWithIssues([medium]), 'low')).toBe(1) + expect(blockedCheckCount(executionWithIssues([{id: 'COMMITTED_SECRET', severity: 'high'}]), 'none')).toBe(0) + expect(blockedCheckCount(executionWithIssues([]), 'low')).toBe(0) + }) + + test('counts a check once, however many of its issues meet the blocking severity', () => { + const execution = executionWithIssues([ + {id: 'COMMITTED_SECRET', severity: 'high'}, + {id: 'COMMITTED_SECRET', severity: 'high'}, + {id: 'OPEN_REDIRECT', severity: 'medium'}, + {id: 'EOL_API_VERSION', severity: 'low'}, + ]) + + expect(blockedCheckCount(execution, 'high')).toBe(1) + expect(blockedCheckCount(execution, 'medium')).toBe(2) + expect(blockedCheckCount(execution, 'low')).toBe(3) }) }) @@ -86,7 +98,7 @@ describe('app security CLI integration', () => { deterministicFindingsPath: artifactPath(directory, 'deterministic-findings.json'), agentChecksPath: artifactPath(directory, 'agent-checks.json'), }) - expect(result.exitCode).toBe(0) + expect(result.blockedChecks).toBe(0) }) }) @@ -116,7 +128,7 @@ describe('app security CLI integration', () => { expect(JSON.stringify(result.execution.deterministicFindings)).not.toContain(testToken) await expect(readFile(artifactPath(directory, 'deterministic-findings.json'))).resolves.not.toContain(testToken) - expect(result.exitCode).toBe(1) + expect(result.blockedChecks).toBe(1) }) }) diff --git a/packages/app/src/cli/services/app-security-api.ts b/packages/app/src/cli/services/app-security-api.ts index 0b1b3b9b463..19b4488d9ee 100644 --- a/packages/app/src/cli/services/app-security-api.ts +++ b/packages/app/src/cli/services/app-security-api.ts @@ -12,10 +12,16 @@ export type AppSecurityBlockingLevel = Severity | 'none' export type AppSecurityExecution = AppSecurityScan & {elapsedMilliseconds: number} -export function securityExitCode(execution: AppSecurityExecution, blocking: AppSecurityBlockingLevel): number { +/** + * The number of checks that fail `--blocking`: the checks with a deterministic issue whose own severity is at or above + * the level. Always 0 for `none`. + */ +export function blockedCheckCount(execution: AppSecurityExecution, blocking: AppSecurityBlockingLevel): number { if (blocking === 'none') return 0 - const blocks = execution.scan.issues.some((issue) => SEVERITY_RANK[issue.severity] >= SEVERITY_RANK[blocking]) - return blocks ? 1 : 0 + const blockingIssues = execution.scan.issues.filter( + (issue) => SEVERITY_RANK[issue.severity] >= SEVERITY_RANK[blocking], + ) + return new Set(blockingIssues.map((issue) => issue.id)).size } export async function executeAppSecurity({ 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 8693f90cf9b..8bd1a48e331 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,7 +1,7 @@ /* eslint-disable no-restricted-imports -- integration coverage uses real temporary repositories */ import {git, isolateGitConfig} from './git-test-helpers.js' import {scanAppDirectory as scanApp, scanDirectory as scan} from './scan-directory.js' -import {securityExitCode} from '../../app-security-api.js' +import {blockedCheckCount} from '../../app-security-api.js' import {formatJson} from '../output/format.js' import {getRegistry} from '../registry/index.js' import {DETERMINISTIC_CHECKS, scan as scanInput} from '../scanners/index.js' @@ -115,9 +115,9 @@ describe('dependency automation scanner integration', () => { findings: [expect.objectContaining({location: {file: 'package.json'}})], }), ) - expect(securityExitCode({...execution, elapsedMilliseconds: 0}, 'low')).toBe(1) - expect(securityExitCode({...execution, elapsedMilliseconds: 0}, 'medium')).toBe(0) - expect(securityExitCode({...execution, elapsedMilliseconds: 0}, 'none')).toBe(0) + expect(blockedCheckCount({...execution, elapsedMilliseconds: 0}, 'low')).toBe(1) + expect(blockedCheckCount({...execution, elapsedMilliseconds: 0}, 'medium')).toBe(0) + expect(blockedCheckCount({...execution, elapsedMilliseconds: 0}, 'none')).toBe(0) }) }) @@ -140,7 +140,7 @@ describe('dependency automation scanner integration', () => { findings: 0, inspected_files: expectedFiles, }) - expect(securityExitCode({...execution, elapsedMilliseconds: 0}, 'low')).toBe(0) + expect(blockedCheckCount({...execution, elapsedMilliseconds: 0}, 'low')).toBe(0) expect(JSON.stringify(execution.deterministicFindings)).not.toContain('local>org/renovate-config') // Outside any repository, gathering stops after asking whether the directory is ignored and probing for a repository. const untrusted = ['-c', 'core.fsmonitor=', '-c', 'safe.bareRepository=explicit', '-c', 'alias.remote-='] diff --git a/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts b/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts index b6b43be5443..bb4000c7067 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts @@ -1,6 +1,7 @@ /* eslint-disable no-restricted-imports -- layouts are real temporary repositories and directories */ import {git, isolateGitConfig} from './git-test-helpers.js' import securityCheck from '../../security-check.js' +import {renderSecurityCheckResult} from '../../security-check/result.js' import {mergeScanDirectories, resolveIncludeDirectories} from '../../app-security-selection.js' import {AbortError} from '@shopify/cli-kit/node/error' import {unstyled} from '@shopify/cli-kit/node/output' @@ -93,7 +94,10 @@ async function inLayout(layout: Layout, run: (temporaryDirectory: string) => Pro } } -/** Runs `check --list-files` from `workingDirectory` (relative to `T`), the way the command calls the service. */ +/** + * Runs `check --list-files` from `workingDirectory` (relative to `T`), the way the command calls the service and + * presents its outcome. + */ async function checkListFiles(temporaryDirectory: string, workingDirectory: string, flags: CheckFlags = {}) { const absoluteWorkingDirectory = resolve(temporaryDirectory, workingDirectory) vi.stubEnv('INIT_CWD', absoluteWorkingDirectory) @@ -109,11 +113,15 @@ async function checkListFiles(temporaryDirectory: string, workingDirectory: stri excludePatterns: flags.excludes ?? [], noGitIgnore: flags.noGitIgnore ?? false, listFiles: true, - verbose: false, blocking: 'none', + }) + await renderSecurityCheckResult(resolution, { + directory, + verbose: false, printInstructions: false, skipInstructions: false, }) + if (resolution.kind !== 'listed') throw new Error(`Expected listed files, got ${resolution.kind}`) return {resolution, stdout: stdout(), stderr: stderr()} }) } diff --git a/packages/app/src/cli/services/app-security-selection.test.ts b/packages/app/src/cli/services/app-security-selection.test.ts index e7f4946fbe5..e4d4efa12e9 100644 --- a/packages/app/src/cli/services/app-security-selection.test.ts +++ b/packages/app/src/cli/services/app-security-selection.test.ts @@ -67,7 +67,7 @@ function promptDependencies(overrides: Partial } } -async function selectionError(promise: Promise): Promise { +async function selectionError(promise: Promise): Promise { const error: unknown = await promise.catch((caught: unknown) => caught) expect(error).toBeInstanceOf(AbortError) return error as AbortError @@ -566,15 +566,16 @@ describe('resolveAppSecuritySelection when no TOML is found', () => { }) }) - test('aborts with the same error when the prompt is answered no', async () => { + test('returns a declined selection when the prompt is answered no', async () => { await inTemporaryDirectory(async (directory) => { const dependencies = promptDependencies({confirmScanWithoutAppConfig: vi.fn(async () => false)}) - const error = await selectionError( - resolveAppSecuritySelection({path: directory, clientId: 'abc', allowPrompts: true}, dependencies), + const selection = await resolveAppSecuritySelection( + {path: directory, clientId: 'abc', allowPrompts: true}, + dependencies, ) - expect(error.message).toBe(`No app configuration found at or above ${directory}.`) + expect(selection).toEqual({kind: 'declined'}) expect(dependencies.confirmScanWithoutAppConfig).toHaveBeenCalledWith(directory) expect(dependencies.pickClientId).not.toHaveBeenCalled() }) diff --git a/packages/app/src/cli/services/app-security-selection.ts b/packages/app/src/cli/services/app-security-selection.ts index 66c903f0098..1c96876e5fb 100644 --- a/packages/app/src/cli/services/app-security-selection.ts +++ b/packages/app/src/cli/services/app-security-selection.ts @@ -50,6 +50,11 @@ export type AppSecuritySelection = clientIdSource: 'flag' | 'picker' } +/** The answer was no when `check` asked whether to scan without app configuration. */ +export interface DeclinedAppSecuritySelection { + kind: 'declined' +} + /** A directory that is walked. */ export interface AppSecurityScanDirectory { /** Absolute; the real path. */ @@ -133,14 +138,27 @@ export async function resolveAppDirectory( return await fileRealPath((await Project.load(options.path)).directory) } catch (error) { if (!(error instanceof NoAppConfigurationFoundError)) throw error - abortNoAppConfigurationFound(options.path) + throw new MissingAppConfigurationError(options.path) } } +/** + * Resolves the app that a command works on. Without prompts, a selection always results or the command aborts. With + * prompts, `check` can also offer to scan without app configuration, and declining that is an outcome rather than an + * error, so `--json` can report it. + */ +export async function resolveAppSecuritySelection( + options: AppSecuritySelectionOptions & {allowPrompts: false}, + dependencies?: AppSecuritySelectionDependencies, +): Promise +export async function resolveAppSecuritySelection( + options: AppSecuritySelectionOptions, + dependencies?: AppSecuritySelectionDependencies, +): Promise export async function resolveAppSecuritySelection( options: AppSecuritySelectionOptions, dependencies: AppSecuritySelectionDependencies = defaultDependencies, -): Promise { +): Promise { if (options.withoutAppConfig) { if (!options.clientId) throw new AbortError('--without-app-config requires --client-id.') const appDirectory = await realDirectory(options.path) @@ -210,13 +228,13 @@ async function configFileWhenNoneIsSelected( async function resolveWithoutAppConfigurationFile( options: AppSecuritySelectionOptions, dependencies: AppSecuritySelectionDependencies, -): Promise { - if (!options.allowPrompts) abortNoAppConfigurationFound(options.path) +): Promise { + if (!options.allowPrompts) throw new MissingAppConfigurationError(options.path) const appDirectory = await realDirectory(options.path) // Before the prompt, so a mistyped client ID fails without asking anything first. await lookUpClientIdFlag(options, dependencies) - if (!(await dependencies.confirmScanWithoutAppConfig(options.path))) abortNoAppConfigurationFound(options.path) + if (!(await dependencies.confirmScanWithoutAppConfig(options.path))) return {kind: 'declined'} if (options.clientId) return {kind: 'no-config', appDirectory, clientId: options.clientId, clientIdSource: 'flag'} return { @@ -340,11 +358,14 @@ export function mergeScanDirectories( } } -function abortNoAppConfigurationFound(directory: string): never { - throw new AbortError( - `No app configuration found at or above ${directory}.`, - 'Pass `--path` to your app directory, or scan without app configuration with `--without-app-config --client-id `.', - ) +/** No app configuration at or above `directory`. In the terminal, declining to scan without one ends with it too. */ +export class MissingAppConfigurationError extends AbortError { + constructor(directory: string) { + super( + `No app configuration found at or above ${directory}.`, + 'Pass `--path` to your app directory, or scan without app configuration with `--without-app-config --client-id `.', + ) + } } function abortNoAppConfigurationSelected(directory: string, fileNames: string[], clientId?: string): never { diff --git a/packages/app/src/cli/services/security-check.test-data.ts b/packages/app/src/cli/services/security-check.test-data.ts new file mode 100644 index 00000000000..e2537d5e397 --- /dev/null +++ b/packages/app/src/cli/services/security-check.test-data.ts @@ -0,0 +1,89 @@ +import type {AppSecurityExecution} from './app-security-api.js' +import type {AgentChecks, DeterministicFindingsDocument, Issue, ScanResult} from './app-security-engine/index.js' + +export const scan: ScanResult = { + version: '0.1.0', + timestamp: '2026-08-24T00:00:00.000Z', + app: {name: 'Test', type: 'public'}, + detection: {framework: 'none', surface: 'config_only', languages: []}, + capabilities: { + theme_app_extension: false, + app_embed: false, + embedded_app: false, + script_tags: false, + webhooks: false, + app_proxy: false, + storefront_metafield_writes: false, + has_backend: false, + declared_ip_allowlist: false, + checkout_extension: false, + }, + scan: { + timestamp: '2026-08-24T00:00:00.000Z', + security_version: '0.1.0', + files_scanned: 1, + rules_run: 1, + rules_skipped: 0, + files_skipped_count: 0, + coverage_gaps: [], + checks_executed: [], + }, + issues: [], +} + +const engine = { + name: 'shopify-app-security', + version: '1.2.3', + ruleset: '2026.08.28', +} + +export const deterministicFindings: DeterministicFindingsDocument = { + schema_version: 1, + source: 'deterministic', + engine: {name: 'shopify-app-security', version: '1.2.3', ruleset: '2026.08.28'}, + generated_at: '2026-08-24T00:00:00.000Z', + detection: scan.detection, + coverage: { + files_scanned: 1, + files_skipped: [], + gaps: [], + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + scan_directories: [{directory: '.', origin: 'app_directory'}], + }, + checks: [], +} + +export const agentChecks: AgentChecks = { + schema_version: 1, + engine: {name: 'shopify-app-security', version: '1.2.3'}, + generated_at: '2026-08-24T00:00:00.000Z', + checks: Array.from({length: 31}, (_, index) => ({ + id: `CHECK_${index}`, + version: 1, + prompt: 'prompt', + severity: 'medium' as const, + docs_url: `https://shopify.dev/docs/apps/build/security/app-security-checks/check-${index}`, + })), + instructions: 'review', +} + +/** A scan without issues, with 31 agent checks, as `executeAppSecurity` returns it. */ +export const scanExecution: AppSecurityExecution = { + scan, + ignoredScanDirectories: [], + otherAppDirectories: [], + deterministicFindings, + agentChecks, + engine, + elapsedMilliseconds: 12, +} + +export const committedSecretIssue: Issue = { + id: 'COMMITTED_SECRET', + severity: 'high', + points: -25, + title: 'Secret', + message: 'secret', + location: {file: 'app/routes/index.ts'}, + fix: {automated: false, description: 'remove it'}, +} diff --git a/packages/app/src/cli/services/security-check.test.ts b/packages/app/src/cli/services/security-check.test.ts index 38438708e90..4661357eda8 100644 --- a/packages/app/src/cli/services/security-check.test.ts +++ b/packages/app/src/cli/services/security-check.test.ts @@ -1,113 +1,26 @@ -import securityCheck, {appSecurityInstructionsPrompt, literalGlobPattern} from './security-check.js' -import { - formatAppSecurityCommand, - quoteShellArgument, - resolveAppSecurityCommands, - shellForPlatform, -} from './app-security-commands.js' +import securityCheck from './security-check.js' +import {resolveAppSecurityCommands} from './app-security-commands.js' import {appSecurityArtifactPaths, writeCheckArtifacts} from './app-security-artifacts.js' import {resolveAppSecuritySelection} from './app-security-selection.js' import {validAppConfiguration} from './app-security-selection.test-data.js' -import {listAppSecurityFiles} from './app-security-api.js' -import {AbortError} from '@shopify/cli-kit/node/error' import { - fileExists, - fileRealPath, - inTemporaryDirectory, - matchGlob, - mkdir, - readFile, - writeFile, -} from '@shopify/cli-kit/node/fs' + agentChecks, + committedSecretIssue, + deterministicFindings, + scanExecution, + scan, +} from './security-check.test-data.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {fileExists, fileRealPath, inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' import {cwd, joinPath, relativePath} from '@shopify/cli-kit/node/path' -import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' import {afterEach, describe, expect, test, vi} from 'vitest' -import {symlink} from 'node:fs/promises' import type {AppSecurityExecution} from './app-security-api.js' -import type {AppSecuritySelection, AppSecuritySelectionOptions} from './app-security-selection.js' -import type {AppSecurityInstructionsDestination} from './security-check.js' import type { - AgentChecks, - AppSecurityScope, - DeterministicFindingsDocument, - ScanResult, -} from './app-security-engine/index.js' - -const scan: ScanResult = { - version: '0.1.0', - timestamp: '2026-08-24T00:00:00.000Z', - app: {name: 'Test', type: 'public'}, - detection: {framework: 'none', surface: 'config_only', languages: []}, - capabilities: { - theme_app_extension: false, - app_embed: false, - embedded_app: false, - script_tags: false, - webhooks: false, - app_proxy: false, - storefront_metafield_writes: false, - has_backend: false, - declared_ip_allowlist: false, - checkout_extension: false, - }, - scan: { - timestamp: '2026-08-24T00:00:00.000Z', - security_version: '0.1.0', - files_scanned: 1, - rules_run: 1, - rules_skipped: 0, - files_skipped_count: 0, - coverage_gaps: [], - checks_executed: [], - }, - issues: [], -} - -const engine = { - name: 'shopify-app-security', - version: '1.2.3', - ruleset: '2026.08.28', -} - -const deterministicFindings: DeterministicFindingsDocument = { - schema_version: 1, - source: 'deterministic', - engine: {name: 'shopify-app-security', version: '1.2.3', ruleset: '2026.08.28'}, - generated_at: '2026-08-24T00:00:00.000Z', - detection: scan.detection, - coverage: { - files_scanned: 1, - files_skipped: [], - gaps: [], - scope: {include_dirs: [], excludes: [], no_git_ignore: false}, - scan_directories: [{directory: '.', origin: 'app_directory'}], - }, - checks: [], -} - -const agentChecks: AgentChecks = { - schema_version: 1, - engine: {name: 'shopify-app-security', version: '1.2.3'}, - generated_at: '2026-08-24T00:00:00.000Z', - checks: Array.from({length: 31}, (_, index) => ({ - id: `CHECK_${index}`, - version: 1, - prompt: 'prompt', - severity: 'medium' as const, - docs_url: `https://shopify.dev/docs/apps/build/security/app-security-checks/check-${index}`, - })), - instructions: 'review', -} - -const scanExecution: AppSecurityExecution = { - scan, - ignoredScanDirectories: [], - otherAppDirectories: [], - deterministicFindings, - agentChecks, - engine, - elapsedMilliseconds: 12, -} + AppSecuritySelection, + AppSecuritySelectionOptions, + DeclinedAppSecuritySelection, +} from './app-security-selection.js' +import type {AppSecurityScope, DeterministicFindingsDocument} from './app-security-engine/index.js' const artifacts = { deterministicFindingsPath: '/tmp/unlinked-app/.shopify/app-security/deterministic-findings.json', @@ -141,62 +54,9 @@ function stagingSelection(): AppSecuritySelection { } } -describe('literalGlobPattern', () => { - afterEach(() => { - vi.unstubAllEnvs() - }) - - test.each([ - ['apps/[child]', 'apps/c'], - ['apps/{a,b}', 'apps/a'], - ['apps/a*b', 'apps/aXb'], - ['apps/x?y', 'apps/xzy'], - ['#child', 'child'], - ['!child', 'src'], - ])('gives a glob that matches %s and not %s', (directory, sibling) => { - const pattern = literalGlobPattern(directory) - - expect(matchGlob(directory, pattern)).toBe(true) - expect(matchGlob(sibling, pattern)).toBe(false) - }) - - test('gives an --exclude that removes only that directory from the gathered files', async () => { - await inTemporaryDirectory(async (directory) => { - const root = await fileRealPath(directory) - await Promise.all(['!child', '[child]', 'c', 'src'].map((name) => mkdir(joinPath(root, name)))) - await Promise.all([ - writeFile(joinPath(root, 'shopify.app.toml'), validAppConfiguration()), - writeFile(joinPath(root, '!child', 'shopify.app.toml'), 'name = "Negated"\n'), - writeFile(joinPath(root, '[child]', 'shopify.app.toml'), 'name = "Bracketed"\n'), - writeFile(joinPath(root, 'c', 'index.ts'), 'export const c = true\n'), - writeFile(joinPath(root, 'src', 'index.ts'), 'export const src = true\n'), - ]) - vi.stubEnv('INIT_CWD', root) - - const gathered = await listAppSecurityFiles({ - appDirectory: root, - scanDirectories: [root], - requestedScanDirectories: [root], - excludePatterns: ['!child', '[child]'].map(literalGlobPattern), - }) - - expect(gathered.paths).toEqual(['c/index.ts', 'shopify.app.toml', 'src/index.ts']) - expect(gathered.otherAppDirectories).toEqual([]) - }) - }) -}) - -/** The warning for a directory that holds another app configuration, quoted for the shell the tests run in. */ -function otherAppWarning(displayPath: string, excludePattern = displayPath) { - return { - headline: `${displayPath} holds another app's configuration, so its files are scanned as part of this app.`, - body: ['Use', {command: `--exclude ${quoteShellArgument(excludePattern, shellForPlatform())}`}, 'to leave it out.'], - } -} - function testDependencies( execution: AppSecurityExecution = scanExecution, - selection: AppSecuritySelection = configSelection, + selection: AppSecuritySelection | DeclinedAppSecuritySelection = configSelection, ) { return { resolveSelection: vi.fn(async () => selection), @@ -208,13 +68,6 @@ function testDependencies( })), writeArtifacts: vi.fn(async () => artifacts), canPrompt: vi.fn(() => false), - selectInstructionsDestination: vi.fn(async (): Promise => 'nothing'), - deliverInstructions: vi.fn(async () => {}), - output: vi.fn(), - renderInfo: vi.fn(), - renderWarning: vi.fn(), - renderReport: vi.fn(), - setExitCode: vi.fn(), recordMetadata: vi.fn(async () => {}), } } @@ -223,10 +76,7 @@ function testOptions() { return { directory: appDirectory, withoutAppConfig: false, - verbose: false, blocking: 'none' as const, - printInstructions: false, - skipInstructions: false, includeDirs: [], excludePatterns: [], noGitIgnore: false, @@ -239,10 +89,10 @@ describe('securityCheck', () => { vi.unstubAllEnvs() }) - test('executes, writes artifacts, then renders a report', async () => { + test('executes, writes artifacts, then returns the scan', async () => { const dependencies = testDependencies() - await securityCheck({...testOptions(), verbose: true, blocking: 'high'}, dependencies) + const check = await securityCheck({...testOptions(), blocking: 'high'}, dependencies) expect(dependencies.resolveSelection).toHaveBeenCalledWith({ path: appDirectory, @@ -267,33 +117,29 @@ describe('securityCheck', () => { deterministicFindings, agentChecks, }) - expect(dependencies.renderReport).toHaveBeenCalledWith({ - scan, + expect(check).toEqual({ + kind: 'scanned', selection: configSelection, - scanDirectories, - engine, - verbose: true, - elapsedMilliseconds: 12, + resultsKey: 'shopify.app', commands: commandsFor(), - deterministicFindingsPath: artifacts.deterministicFindingsPath, - agentChecksPath: artifacts.agentChecksPath, - agentCheckCount: 31, + scope: noScope, + scanDirectories, + execution: scanExecution, + artifacts, + blocking: {level: 'high', blockedChecks: 0}, }) - expect(dependencies.output).not.toHaveBeenCalled() }) test('forwards the config name and includes --config in generated commands', async () => { const dependencies = testDependencies(scanExecution, stagingSelection()) - await securityCheck({...testOptions(), configName: 'staging'}, dependencies) + const check = await securityCheck({...testOptions(), configName: 'staging'}, dependencies) expect(dependencies.resolveSelection).toHaveBeenCalledWith(expect.objectContaining({config: 'staging'})) expect(dependencies.execute).toHaveBeenCalledWith( expect.objectContaining({appConfigFilePath: `${appDirectory}/shopify.app.staging.toml`}), ) - expect(dependencies.renderReport).toHaveBeenCalledWith( - expect.objectContaining({commands: commandsFor(stagingSelection())}), - ) + expect(check).toMatchObject({commands: commandsFor(stagingSelection())}) }) test('scans with the --client-id override as the effective client ID', async () => { @@ -305,20 +151,18 @@ describe('securityCheck', () => { expect(dependencies.execute).toHaveBeenCalledWith(expect.objectContaining({clientId: 'flag-client-id'})) }) - test('forwards the scope flags to the scan and repeats them in generated commands and instructions', async () => { + test('forwards the scope flags to the scan and repeats them in generated commands', async () => { const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(true) - dependencies.selectInstructionsDestination.mockResolvedValue('print') const excludePatterns = ['generated', '../shared/**'] - await securityCheck({...testOptions(), excludePatterns, noGitIgnore: true}, dependencies) + const check = await securityCheck({...testOptions(), excludePatterns, noGitIgnore: true}, dependencies) - const commands = commandsFor(configSelection, {include_dirs: [], excludes: excludePatterns, no_git_ignore: true}) + const scope = {include_dirs: [], excludes: excludePatterns, no_git_ignore: true} + const commands = commandsFor(configSelection, scope) expect(commands.scan.args).toContainEqual({flag: '--exclude', value: 'generated'}) expect(commands.scan.args).toContain('--no-git-ignore') expect(dependencies.execute).toHaveBeenCalledWith(expect.objectContaining({excludePatterns, noGitIgnore: true})) - expect(dependencies.renderReport).toHaveBeenCalledWith(expect.objectContaining({commands})) - expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) + expect(check).toMatchObject({commands, scope}) }) test('scans each --include-dir after the app directory, reports it, and repeats it before --exclude', async () => { @@ -327,11 +171,9 @@ describe('securityCheck', () => { vi.stubEnv('INIT_CWD', directory) const backend = await fileRealPath(joinPath(directory, 'backend')) const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(true) - dependencies.selectInstructionsDestination.mockResolvedValue('print') const options = {...testOptions(), includeDirs: ['backend', './backend'], excludePatterns: ['generated']} - await securityCheck(options, dependencies) + const check = await securityCheck(options, dependencies) expect(dependencies.execute).toHaveBeenCalledWith( expect.objectContaining({ @@ -339,26 +181,21 @@ describe('securityCheck', () => { requestedScanDirectories: [appDirectory, backend], }), ) - const commands = commandsFor(configSelection, { - include_dirs: ['backend', './backend'], - excludes: ['generated'], - no_git_ignore: false, - }) + const scope = {include_dirs: ['backend', './backend'], excludes: ['generated'], no_git_ignore: false} + const commands = commandsFor(configSelection, scope) expect(commands.scan.args.slice(-3)).toEqual([ {flag: '--include-dir', value: 'backend'}, {flag: '--include-dir', value: './backend'}, {flag: '--exclude', value: 'generated'}, ]) - expect(dependencies.renderReport).toHaveBeenCalledWith( - expect.objectContaining({ - commands, - scanDirectories: [ - {directory: appDirectory, origin: 'app_directory'}, - {directory: backend, origin: 'include_dir'}, - ], - }), - ) - expect(dependencies.deliverInstructions).toHaveBeenCalledWith(expect.objectContaining({commands})) + expect(check).toMatchObject({ + commands, + scope, + scanDirectories: [ + {directory: appDirectory, origin: 'app_directory'}, + {directory: backend, origin: 'include_dir'}, + ], + }) }) }) @@ -370,7 +207,7 @@ describe('securityCheck', () => { const sharedTheme = '/tmp/shared/theme' const dependencies = testDependencies(scanExecution, {...configSelection, appConfigDirectories: [sharedTheme]}) - const resolution = await securityCheck({...testOptions(), includeDirs: ['backend']}, dependencies) + const check = await securityCheck({...testOptions(), includeDirs: ['backend']}, dependencies) expect(dependencies.execute).toHaveBeenCalledWith( expect.objectContaining({ @@ -379,18 +216,14 @@ describe('securityCheck', () => { appConfigDirectories: [sharedTheme], }), ) - expect(dependencies.renderReport).toHaveBeenCalledWith( - expect.objectContaining({ - scanDirectories: [ - {directory: appDirectory, origin: 'app_directory'}, - {directory: backend, origin: 'include_dir'}, - {directory: sharedTheme, origin: 'app_config_directory'}, - ], - }), - ) - expect(resolution.commands).toEqual( - commandsFor(configSelection, {include_dirs: ['backend'], excludes: [], no_git_ignore: false}), - ) + expect(check).toMatchObject({ + scanDirectories: [ + {directory: appDirectory, origin: 'app_directory'}, + {directory: backend, origin: 'include_dir'}, + {directory: sharedTheme, origin: 'app_config_directory'}, + ], + commands: commandsFor(configSelection, {include_dirs: ['backend'], excludes: [], no_git_ignore: false}), + }) }) }) @@ -426,86 +259,20 @@ describe('securityCheck', () => { }) }) - test('warns once for each scan directory that Git ignores, relative to the working directory', async () => { - await inTemporaryDirectory(async (directory) => { - const realDirectory = await fileRealPath(directory) - vi.stubEnv('INIT_CWD', directory) - const dependencies = testDependencies({ - ...scanExecution, - ignoredScanDirectories: [joinPath(realDirectory, 'unlinked-app'), realDirectory], - }) - - await securityCheck(testOptions(), dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, { - headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], - }) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, { - headline: '. is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], - }) - }) - }) - - test('warns once for each directory that holds another app configuration, with the --exclude that leaves it out', async () => { - await inTemporaryDirectory(async (directory) => { - const realDirectory = await fileRealPath(directory) - vi.stubEnv('INIT_CWD', directory) - const dependencies = testDependencies({ - ...scanExecution, - otherAppDirectories: [joinPath(realDirectory, 'apps', 'child'), joinPath(realDirectory, '..', 'backend')], - }) - - await securityCheck(testOptions(), dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledTimes(2) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(1, otherAppWarning('apps/child')) - expect(dependencies.renderWarning).toHaveBeenNthCalledWith(2, otherAppWarning('../backend')) - }) - }) - - test('escapes and quotes a directory name that is glob and shell syntax', async () => { - await inTemporaryDirectory(async (directory) => { - vi.stubEnv('INIT_CWD', directory) - const dependencies = testDependencies({ - ...scanExecution, - otherAppDirectories: [joinPath(await fileRealPath(directory), 'apps', '$(touch PWNED)')], - }) - - await securityCheck(testOptions(), dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledWith( - otherAppWarning('apps/$(touch PWNED)', 'apps/$\\(touch PWNED\\)'), - ) - }) - }) - - test('shows warnings relative to the real working directory when it is reached through a symbolic link', async () => { - await inTemporaryDirectory(async (directory) => { - const app = joinPath(await fileRealPath(directory), 'app') - await mkdir(app) - const link = joinPath(directory, 'linked-app') - await symlink(app, link, 'dir') - vi.stubEnv('INIT_CWD', link) - const dependencies = testDependencies({...scanExecution, otherAppDirectories: [joinPath(app, 'apps', 'child')]}) - - await securityCheck(testOptions(), dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledWith(otherAppWarning('apps/child')) - }) - }) - - test('does not warn when no scan directory is ignored', async () => { - const dependencies = testDependencies() + test('returns the scan directories that Git ignores and the directories that hold another app configuration', async () => { + const execution = { + ...scanExecution, + ignoredScanDirectories: [joinPath(appDirectory, 'vendor')], + otherAppDirectories: [joinPath(appDirectory, 'apps', 'child')], + } + const dependencies = testDependencies(execution) - await securityCheck(testOptions(), dependencies) + const check = await securityCheck(testOptions(), dependencies) - expect(dependencies.renderWarning).not.toHaveBeenCalled() + expect(check).toMatchObject({kind: 'scanned', execution}) }) - test('re-scanning overwrites the check artifacts without prompting and leaves agent findings untouched', async () => { + test('re-scanning overwrites the check artifacts and leaves agent findings untouched', async () => { await inTemporaryDirectory(async (appRoot) => { const paths = appSecurityArtifactPaths(appRoot, 'shopify.app') await mkdir(paths.resultsDirectory) @@ -528,30 +295,27 @@ describe('securityCheck', () => { canPrompt: vi.fn(() => true), } - await securityCheck({...testOptions(), directory: appRoot, skipInstructions: true}, dependencies) + const check = await securityCheck({...testOptions(), directory: appRoot}, dependencies) expect(JSON.parse(await readFile(paths.deterministicFindingsPath))).toEqual(rescanFindings) expect(JSON.parse(await readFile(paths.agentChecksPath))).toEqual(agentChecks) await expect(readFile(paths.agentFindingsPath)).resolves.toBe(agentFindings) - expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() - expect(dependencies.renderReport).toHaveBeenCalledWith( - expect.objectContaining({ - deterministicFindingsPath: paths.deterministicFindingsPath, - agentChecksPath: paths.agentChecksPath, - }), - ) - expect(dependencies.setExitCode).not.toHaveBeenCalled() + expect(check).toMatchObject({ + kind: 'scanned', + artifacts: {deterministicFindingsPath: paths.deterministicFindingsPath, agentChecksPath: paths.agentChecksPath}, + blocking: {level: 'none', blockedChecks: 0}, + }) }) }) test('allows prompts only in an interactive terminal', async () => { const interactive = testDependencies() interactive.canPrompt.mockReturnValue(true) - await securityCheck({...testOptions(), skipInstructions: true}, interactive) + await securityCheck(testOptions(), interactive) expect(interactive.resolveSelection).toHaveBeenCalledWith(expect.objectContaining({allowPrompts: true})) const nonInteractive = testDependencies() - await securityCheck({...testOptions(), skipInstructions: true}, nonInteractive) + await securityCheck(testOptions(), nonInteractive) expect(nonInteractive.resolveSelection).toHaveBeenCalledWith(expect.objectContaining({allowPrompts: false})) }) @@ -564,9 +328,10 @@ describe('securityCheck', () => { } const dependencies = testDependencies(scanExecution, selection) dependencies.canPrompt.mockReturnValue(true) + const onSelectionPrompted = vi.fn() - await securityCheck( - {...testOptions(), withoutAppConfig: true, clientId: 'flag-client-id', skipInstructions: true}, + const check = await securityCheck( + {...testOptions(), withoutAppConfig: true, clientId: 'flag-client-id', onSelectionPrompted}, dependencies, ) @@ -585,11 +350,11 @@ describe('securityCheck', () => { noGitIgnore: false, }) expect(dependencies.writeArtifacts).toHaveBeenCalledWith(appDirectory, 'flag-client-id', expect.anything()) - expect(dependencies.renderInfo).not.toHaveBeenCalled() - expect(dependencies.renderReport).toHaveBeenCalledWith(expect.objectContaining({commands: commandsFor(selection)})) + expect(onSelectionPrompted).not.toHaveBeenCalled() + expect(check).toMatchObject({commands: commandsFor(selection)}) }) - test('shows the generated check command after the no-TOML prompt flow', async () => { + test('passes the generated check command on after the no-TOML prompt flow, before scanning', async () => { const selection: AppSecuritySelection = { kind: 'no-config', appDirectory, @@ -598,21 +363,22 @@ describe('securityCheck', () => { } const dependencies = testDependencies(scanExecution, selection) dependencies.canPrompt.mockReturnValue(true) + const onSelectionPrompted = vi.fn() - await securityCheck({...testOptions(), skipInstructions: true}, dependencies) + await securityCheck({...testOptions(), onSelectionPrompted}, dependencies) - const {scan} = commandsFor(selection) - expect(scan.args.slice(-2)).toEqual([{flag: '--client-id', value: 'picked-client-id'}, '--without-app-config']) - expect(dependencies.renderInfo).toHaveBeenCalledWith({ - headline: 'To skip these prompts next time, run:', - body: [{command: formatAppSecurityCommand(scan)}], - }) - expect(dependencies.renderInfo.mock.invocationCallOrder[0]).toBeLessThan( + const commands = commandsFor(selection) + expect(commands.scan.args.slice(-2)).toEqual([ + {flag: '--client-id', value: 'picked-client-id'}, + '--without-app-config', + ]) + expect(onSelectionPrompted).toHaveBeenCalledExactlyOnceWith(commands) + expect(onSelectionPrompted.mock.invocationCallOrder[0]).toBeLessThan( dependencies.execute.mock.invocationCallOrder[0]!, ) }) - test('shows the generated check command, with --config, after asking which TOML to scan', async () => { + test('passes the generated check command, with --config, on after asking which TOML to scan', async () => { const selection: AppSecuritySelection = { kind: 'config', appDirectory, @@ -622,204 +388,64 @@ describe('securityCheck', () => { } const dependencies = testDependencies(scanExecution, selection) dependencies.canPrompt.mockReturnValue(true) + const onSelectionPrompted = vi.fn() - await securityCheck({...testOptions(), skipInstructions: true}, dependencies) - - const {scan} = commandsFor(selection) - expect(scan.args).toContainEqual({flag: '--config', value: 'staging'}) - expect(dependencies.renderInfo).toHaveBeenCalledWith({ - headline: 'To skip these prompts next time, run:', - body: [{command: formatAppSecurityCommand(scan)}], - }) - }) - - test('does not show the prompt-flow command when a TOML was found', async () => { - const dependencies = testDependencies() - - await securityCheck(testOptions(), dependencies) - - expect(dependencies.renderInfo).not.toHaveBeenCalled() - }) - - test('does not write artifacts when the scan fails', async () => { - const dependencies = testDependencies() - dependencies.execute.mockRejectedValue(new Error('scan failed')) - - await expect(securityCheck(testOptions(), dependencies)).rejects.toThrow('scan failed') - - expect(dependencies.writeArtifacts).not.toHaveBeenCalled() - }) - - test('does not offer coding-agent instructions in CI or another non-interactive environment', async () => { - const dependencies = testDependencies() + await securityCheck({...testOptions(), onSelectionPrompted}, dependencies) - await securityCheck(testOptions(), dependencies) - - expect(dependencies.canPrompt).toHaveBeenCalledOnce() - expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() - expect(dependencies.deliverInstructions).not.toHaveBeenCalled() + const commands = commandsFor(selection) + expect(commands.scan.args).toContainEqual({flag: '--config', value: 'staging'}) + expect(onSelectionPrompted).toHaveBeenCalledExactlyOnceWith(commands) }) - test('prioritizes copying instructions that start from the scan results', async () => { + test('does not pass the generated check command on when a TOML was found', async () => { const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(true) - dependencies.selectInstructionsDestination.mockResolvedValue('copy') + const onSelectionPrompted = vi.fn() - await securityCheck(testOptions(), dependencies) - - expect(appSecurityInstructionsPrompt(31)).toEqual({ - message: - '31 recommended agent checks available to complete your scan. How do you want to pass that prompt to your agent?', - choices: [ - {label: 'Copy instructions to the clipboard', value: 'copy'}, - {label: 'Print instructions to the terminal', value: 'print'}, - {label: 'Nothing', value: 'nothing'}, - ], - defaultValue: 'copy', - }) - expect(dependencies.selectInstructionsDestination).toHaveBeenCalledOnce() - expect(dependencies.selectInstructionsDestination).toHaveBeenCalledWith(31) - expect(dependencies.deliverInstructions).toHaveBeenCalledWith({ - appDirectory, - resultsKey: 'shopify.app', - copy: true, - scanScope: noScope, - commands: commandsFor(), - }) - }) + await securityCheck({...testOptions(), onSelectionPrompted}, dependencies) - test('names a single recommended agent check in the singular', () => { - expect(appSecurityInstructionsPrompt(1).message).toBe( - '1 recommended agent check available to complete your scan. How do you want to pass that prompt to your agent?', - ) + expect(onSelectionPrompted).not.toHaveBeenCalled() }) - test('gives the instructions the exact scope of the run, as typed', async () => { - await inTemporaryDirectory(async (directory) => { - await mkdir(joinPath(directory, 'backend')) - vi.stubEnv('INIT_CWD', directory) - const dependencies = testDependencies() - - await securityCheck( - { - ...testOptions(), - printInstructions: true, - includeDirs: ['backend', './backend/'], - excludePatterns: ['**/generated', '!keep'], - noGitIgnore: true, - }, - dependencies, - ) - - const scope = { - include_dirs: ['backend', './backend/'], - excludes: ['**/generated', '!keep'], - no_git_ignore: true, - } - expect(dependencies.deliverInstructions).toHaveBeenCalledWith( - expect.objectContaining({scanScope: scope, commands: commandsFor(configSelection, scope)}), - ) - expect(dependencies.execute).toHaveBeenCalledWith( - expect.objectContaining({includeDirs: ['backend', './backend/']}), - ) - }) - }) - - test('prints post-scan instructions when selected', async () => { - const dependencies = testDependencies() + test('returns a cancellation, without scanning or writing anything, when scanning without app configuration is declined', async () => { + const dependencies = testDependencies(scanExecution, {kind: 'declined'}) dependencies.canPrompt.mockReturnValue(true) - dependencies.selectInstructionsDestination.mockResolvedValue('print') + const onSelectionPrompted = vi.fn() - await securityCheck(testOptions(), dependencies) + const check = await securityCheck({...testOptions(), onSelectionPrompted}, dependencies) - expect(dependencies.deliverInstructions).toHaveBeenCalledWith({ - appDirectory, - resultsKey: 'shopify.app', - copy: false, - scanScope: noScope, - commands: commandsFor(), - }) + expect(check).toEqual({kind: 'cancelled'}) + expect(onSelectionPrompted).not.toHaveBeenCalled() + expect(dependencies.execute).not.toHaveBeenCalled() + expect(dependencies.writeArtifacts).not.toHaveBeenCalled() + expect(dependencies.recordMetadata).not.toHaveBeenCalled() }) - test('does nothing when selected', async () => { + test('does not write artifacts when the scan fails', async () => { const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(true) + dependencies.execute.mockRejectedValue(new Error('scan failed')) - await securityCheck(testOptions(), dependencies) + await expect(securityCheck(testOptions(), dependencies)).rejects.toThrow('scan failed') - expect(dependencies.selectInstructionsDestination).toHaveBeenCalledOnce() - expect(dependencies.deliverInstructions).not.toHaveBeenCalled() + expect(dependencies.writeArtifacts).not.toHaveBeenCalled() }) - test.each([false, true])( - '--print-instructions prints without a delivery prompt and preserves app selection prompts (interactive=%s)', - async (interactive) => { - const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(interactive) - - await securityCheck({...testOptions(), printInstructions: true}, dependencies) + test('counts the checks that breach --blocking', async () => { + const dependencies = testDependencies({...scanExecution, scan: {...scan, issues: [committedSecretIssue]}}) - expect(dependencies.resolveSelection).toHaveBeenCalledWith(expect.objectContaining({allowPrompts: interactive})) - expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() - expect(dependencies.deliverInstructions).toHaveBeenCalledWith({ - appDirectory, - resultsKey: 'shopify.app', - copy: false, - scanScope: noScope, - commands: commandsFor(), - }) - }, - ) + const check = await securityCheck({...testOptions(), blocking: 'high'}, dependencies) - test('--skip-instructions never offers instructions', async () => { - const dependencies = testDependencies() - dependencies.canPrompt.mockReturnValue(true) - - await securityCheck({...testOptions(), skipInstructions: true}, dependencies) - - expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() - expect(dependencies.deliverInstructions).not.toHaveBeenCalled() + expect(check).toMatchObject({blocking: {level: 'high', blockedChecks: 1}}) }) - test('sets a blocking exit code from the execution result', async () => { + test('records the number of deterministic issues in the command metadata', async () => { const dependencies = testDependencies({ ...scanExecution, scan: { ...scan, - issues: [ - { - id: 'COMMITTED_SECRET', - severity: 'high', - points: -25, - title: 'Secret', - message: 'secret', - location: {file: 'app/routes/index.ts'}, - fix: {automated: false, description: 'remove it'}, - }, - ], + issues: [committedSecretIssue, {...committedSecretIssue, location: {file: 'app/routes/other.ts'}}], }, }) - await securityCheck({...testOptions(), blocking: 'high'}, dependencies) - - expect(dependencies.setExitCode).toHaveBeenCalledWith(1) - }) - - test('records the number of deterministic issues in the command metadata', async () => { - const issue = { - id: 'COMMITTED_SECRET', - severity: 'high' as const, - points: -25, - title: 'Secret', - message: 'secret', - location: {file: 'app/routes/index.ts'}, - fix: {automated: false, description: 'remove it'}, - } - const dependencies = testDependencies({ - ...scanExecution, - scan: {...scan, issues: [issue, {...issue, location: {file: 'app/routes/other.ts'}}]}, - }) - await securityCheck(testOptions(), dependencies) expect(dependencies.recordMetadata).toHaveBeenCalledWith({num_security_findings: 2}) @@ -833,12 +459,12 @@ describe('securityCheck', () => { expect(dependencies.recordMetadata).toHaveBeenCalledWith({num_security_findings: 0}) }) - test('keeps the default exit code when no finding reaches the blocking level', async () => { + test('counts no blocked checks when no finding reaches the blocking level', async () => { const dependencies = testDependencies() - await securityCheck({...testOptions(), blocking: 'low'}, dependencies) + const check = await securityCheck({...testOptions(), blocking: 'low'}, dependencies) - expect(dependencies.setExitCode).not.toHaveBeenCalled() + expect(check).toMatchObject({blocking: {level: 'low', blockedChecks: 0}}) }) }) @@ -849,34 +475,31 @@ describe('securityCheck --list-files', () => { vi.unstubAllEnvs() }) - test('prints each gathered path on its own line and does nothing else', async () => { + test('returns each gathered path and does nothing else', async () => { const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({ + const listing = { paths: ['../backend/server.ts', 'app/routes/index.ts', 'shopify.app.toml'], - ignoredScanDirectories: [], - otherAppDirectories: [], - }) + ignoredScanDirectories: [joinPath(appDirectory, 'vendor')], + otherAppDirectories: [joinPath(appDirectory, 'apps', 'child')], + } + dependencies.listFiles.mockResolvedValue(listing) + const onSelectionPrompted = vi.fn() - await securityCheck(listFilesOptions, dependencies) + const check = await securityCheck({...listFilesOptions, onSelectionPrompted}, dependencies) - expect(dependencies.output).toHaveBeenCalledOnce() - expect(dependencies.output).toHaveBeenCalledWith('../backend/server.ts\napp/routes/index.ts\nshopify.app.toml') + expect(check).toEqual({ + kind: 'listed', + selection: configSelection, + resultsKey: 'shopify.app', + commands: commandsFor(), + scope: noScope, + scanDirectories, + ...listing, + }) expect(dependencies.execute).not.toHaveBeenCalled() expect(dependencies.writeArtifacts).not.toHaveBeenCalled() - expect(dependencies.renderReport).not.toHaveBeenCalled() - expect(dependencies.renderInfo).not.toHaveBeenCalled() - expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() - expect(dependencies.deliverInstructions).not.toHaveBeenCalled() - expect(dependencies.setExitCode).not.toHaveBeenCalled() - }) - - test('prints nothing when no path is gathered', async () => { - const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({paths: [], ignoredScanDirectories: [], otherAppDirectories: []}) - - await securityCheck(listFilesOptions, dependencies) - - expect(dependencies.output).not.toHaveBeenCalled() + expect(dependencies.recordMetadata).not.toHaveBeenCalled() + expect(onSelectionPrompted).not.toHaveBeenCalled() }) test('resolves without prompts, even in an interactive terminal', async () => { @@ -920,64 +543,33 @@ describe('securityCheck --list-files', () => { }) }) - test('warns about an ignored scan directory through renderWarning', async () => { - await inTemporaryDirectory(async (directory) => { - const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({ - paths: ['shopify.app.toml'], - ignoredScanDirectories: [joinPath(await fileRealPath(directory), 'unlinked-app')], - otherAppDirectories: [], - }) - vi.stubEnv('INIT_CWD', directory) - - await securityCheck(listFilesOptions, dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledWith({ - headline: 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], - }) - expect(dependencies.output).toHaveBeenCalledWith('shopify.app.toml') - }) - }) - - test('warns about a directory that holds another app configuration through renderWarning', async () => { - await inTemporaryDirectory(async (directory) => { - const dependencies = testDependencies() - dependencies.listFiles.mockResolvedValue({ - paths: ['apps/child/shopify.app.toml', 'shopify.app.toml'], - ignoredScanDirectories: [], - otherAppDirectories: [joinPath(await fileRealPath(directory), 'apps', 'child')], - }) - vi.stubEnv('INIT_CWD', directory) - - await securityCheck(listFilesOptions, dependencies) - - expect(dependencies.renderWarning).toHaveBeenCalledWith(otherAppWarning('apps/child')) - expect(dependencies.output).toHaveBeenCalledWith('apps/child/shopify.app.toml\nshopify.app.toml') - }) - }) - test('returns the selection, the results key and the generated check command', async () => { const dependencies = testDependencies() - const resolution = await securityCheck( + const check = await securityCheck( {...listFilesOptions, includeDirs: [], excludePatterns: ['generated'], noGitIgnore: true}, dependencies, ) - expect(resolution.selection).toBe(configSelection) - expect(resolution.resultsKey).toBe('shopify.app') - expect(resolution.commands.scan.args).toEqual([ - 'app', - 'security', - 'check', - {flag: '--path', value: relativePath(cwd(), appDirectory)}, - {flag: '--exclude', value: 'generated'}, - '--no-git-ignore', - ]) + expect(check).toMatchObject({ + selection: configSelection, + resultsKey: 'shopify.app', + commands: { + scan: { + args: [ + 'app', + 'security', + 'check', + {flag: '--path', value: relativePath(cwd(), appDirectory)}, + {flag: '--exclude', value: 'generated'}, + '--no-git-ignore', + ], + }, + }, + }) }) - test('lists the real files, one path per line, and writes no results', async () => { + test('lists the real files and writes no results', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await fileRealPath(directory) await mkdir(joinPath(appRoot, 'app')) @@ -987,20 +579,14 @@ describe('securityCheck --list-files', () => { await writeFile(joinPath(appRoot, 'generated', 'out.ts'), 'export {}\n') vi.stubEnv('INIT_CWD', appRoot) - const {stdout, resolution} = await withCapturedStandardStreams(async ({stdout: captured}) => { - const result = await securityCheck({ - ...listFilesOptions, - directory: appRoot, - excludePatterns: ['**/generated'], - }) - return {stdout: captured(), resolution: result} - }) + const check = await securityCheck({...listFilesOptions, directory: appRoot, excludePatterns: ['**/generated']}) // Outside a repository, the `.shopify` files that resolving the selection writes are gathered too. - expect(stdout).toBe( - ['.shopify/.gitignore', '.shopify/project.json', 'app/index.ts', 'shopify.app.toml'].join('\n').concat('\n'), - ) - expect(resolution.resultsKey).toBe('shopify.app') + expect(check).toMatchObject({ + kind: 'listed', + resultsKey: 'shopify.app', + paths: ['.shopify/.gitignore', '.shopify/project.json', 'app/index.ts', 'shopify.app.toml'], + }) await expect(fileExists(joinPath(appRoot, '.shopify', 'app-security'))).resolves.toBe(false) }) }) @@ -1067,7 +653,6 @@ describe('securityCheck --client-id lookup', () => { expect(dependencies.listFiles).not.toHaveBeenCalled() expect(dependencies.execute).not.toHaveBeenCalled() expect(dependencies.recordMetadata).not.toHaveBeenCalled() - expect(dependencies.output).not.toHaveBeenCalled() await expect(fileExists(joinPath(appRoot, '.shopify', 'app-security'))).resolves.toBe(false) }) }, diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index b88ee06888c..9aded0b464d 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -1,17 +1,6 @@ -import {securityExitCode, executeAppSecurity, listAppSecurityFiles} from './app-security-api.js' +import {blockedCheckCount, executeAppSecurity, listAppSecurityFiles} from './app-security-api.js' import {writeCheckArtifacts} from './app-security-artifacts.js' -import { - appSecurityInstructions, - deliverAppSecurityInstructions, - renderAppSecurityInstructions, -} from './app-security-instructions.js' -import { - formatAppSecurityCommand, - quoteShellArgument, - resolveAppSecurityCommands, - shellForPlatform, - type AppSecurityCommands, -} from './app-security-commands.js' +import {resolveAppSecurityCommands, type AppSecurityCommands} from './app-security-commands.js' import { effectiveClientId, mergeScanDirectories, @@ -21,14 +10,10 @@ import { type AppSecurityScanDirectory, type AppSecuritySelection, type AppSecuritySelectionOptions, + type DeclinedAppSecuritySelection, } from './app-security-selection.js' -import {renderSecurityReport} from './security-output.js' import {recordAppSecurityMetadata, type AppSecurityMetadata} from './app-security-metadata.js' -import {fileRealPath} from '@shopify/cli-kit/node/fs' -import {outputResult} from '@shopify/cli-kit/node/output' import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' -import {cwd, relativePath} from '@shopify/cli-kit/node/path' -import {renderInfo, renderSelectPrompt, renderWarning} from '@shopify/cli-kit/node/ui' import type {CheckArtifactPaths} from './app-security-artifacts.js' import type { AgentChecks, @@ -38,23 +23,23 @@ import type { ScanOptions, } from './app-security-engine/index.js' import type {AppSecurityBlockingLevel, AppSecurityExecution} from './app-security-api.js' -import type {SecurityReportInput} from './security-output.js' -import type {RenderAlertOptions, RenderSelectPromptOptions} from '@shopify/cli-kit/node/ui' interface SecurityOptions { directory: string configName?: string clientId?: string withoutAppConfig: boolean - verbose: boolean blocking: AppSecurityBlockingLevel - printInstructions: boolean - skipInstructions: boolean includeDirs: ReadonlyArray excludePatterns: ReadonlyArray noGitIgnore: boolean - /** Only resolve and gather: print the gathered paths and stop. */ + /** Only resolve and gather: return the gathered paths and stop. */ listFiles: boolean + /** + * Called when resolving the selection prompted, before anything is gathered, with the commands that repeat the run + * without prompts. + */ + onSelectionPrompted?: (commands: AppSecurityCommands) => void } /** What a run resolved, whether it scanned or only listed files. */ @@ -63,169 +48,84 @@ interface SecurityCheckResolution { resultsKey: string /** The commands that repeat this run, including its scope. */ commands: AppSecurityCommands + /** The scope flags, as typed. */ + scope: AppSecurityScope + scanDirectories: AppSecurityScanDirectory[] } -export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' - -interface ScanInstructionsOptions { - appDirectory: string - resultsKey: string - copy: boolean - scanScope: AppSecurityScope - commands: AppSecurityCommands +/** The directories that gathering warns about. Both lists hold absolute paths. */ +interface AppSecurityGatheringNotices { + ignoredScanDirectories: string[] + otherAppDirectories: string[] } +/** The files a scan would gather, which `--list-files` lists. */ +type ListedSecurityCheck = {kind: 'listed'; paths: string[]} & SecurityCheckResolution & AppSecurityGatheringNotices + +/** A finished scan, with the results files it replaced and the `--blocking` outcome. */ +type ScannedSecurityCheck = { + kind: 'scanned' + execution: AppSecurityExecution + artifacts: CheckArtifactPaths + blocking: { + level: AppSecurityBlockingLevel + /** Checks with a deterministic issue at or above the level. Zero for `none`. */ + blockedChecks: number + } +} & SecurityCheckResolution + +/** + * What `securityCheck` returns: the listed files, the finished scan, or a cancellation, when the user declined to scan + * without app configuration. + */ +export type SecurityCheckOutcome = ListedSecurityCheck | ScannedSecurityCheck | {kind: 'cancelled'} + interface SecurityDependencies { - resolveSelection(options: AppSecuritySelectionOptions): Promise + resolveSelection(options: AppSecuritySelectionOptions): Promise execute(options: ScanInput & Required): Promise - listFiles( - options: ScanInput & Required, - ): Promise<{paths: string[]; ignoredScanDirectories: string[]; otherAppDirectories: string[]}> + listFiles(options: ScanInput & Required): Promise<{paths: string[]} & AppSecurityGatheringNotices> writeArtifacts( appDirectory: string, resultsKey: string, artifacts: {deterministicFindings: DeterministicFindingsDocument; agentChecks: AgentChecks}, ): Promise canPrompt(): boolean - selectInstructionsDestination(agentCheckCount: number): Promise - deliverInstructions(options: ScanInstructionsOptions): Promise - output(content: string): void - renderInfo(options: RenderAlertOptions): void - renderWarning(options: RenderAlertOptions): void - renderReport(input: SecurityReportInput): void - setExitCode(exitCode: number): void recordMetadata(fields: AppSecurityMetadata): Promise } -export function appSecurityInstructionsPrompt( - agentCheckCount: number, -): RenderSelectPromptOptions { - return { - message: `${agentCheckCount} recommended agent ${agentCheckCount === 1 ? 'check' : 'checks'} available to complete your scan. How do you want to pass that prompt to your agent?`, - choices: [ - {label: 'Copy instructions to the clipboard', value: 'copy'}, - {label: 'Print instructions to the terminal', value: 'print'}, - {label: 'Nothing', value: 'nothing'}, - ], - defaultValue: 'copy', - } -} - -/** Generates the instructions for the scan that just finished, then copies or prints them. */ -async function deliverScanInstructions({copy, ...options}: ScanInstructionsOptions): Promise { - const instructions = appSecurityInstructions(options) - await deliverAppSecurityInstructions(instructions, {copy}) - renderAppSecurityInstructions(instructions, {copy}) -} - const defaultDependencies: SecurityDependencies = { resolveSelection: resolveAppSecuritySelection, execute: executeAppSecurity, listFiles: listAppSecurityFiles, writeArtifacts: writeCheckArtifacts, canPrompt: terminalSupportsPrompting, - selectInstructionsDestination: (agentCheckCount) => - renderSelectPrompt(appSecurityInstructionsPrompt(agentCheckCount)), - deliverInstructions: deliverScanInstructions, - output: outputResult, - renderInfo, - renderWarning, - renderReport: renderSecurityReport, - setExitCode: (exitCode) => { - process.exitCode = exitCode - }, recordMetadata: recordAppSecurityMetadata, } -async function instructionsDestination( - options: SecurityOptions, - dependencies: SecurityDependencies, - canPrompt: boolean, - agentCheckCount: number, -): Promise { - if (options.skipInstructions) return 'nothing' - if (options.printInstructions) return 'print' - if (!canPrompt) return 'nothing' - return dependencies.selectInstructionsDestination(agentCheckCount) -} - -function securityReportInput( - execution: AppSecurityExecution, - artifacts: CheckArtifactPaths, - verbose: boolean, - commands: AppSecurityCommands, - selection: AppSecuritySelection, - scanDirectories: AppSecurityScanDirectory[], -): SecurityReportInput { - return { - scan: execution.scan, - selection, - scanDirectories, - engine: execution.engine, - verbose, - elapsedMilliseconds: execution.elapsedMilliseconds, - commands, - deterministicFindingsPath: artifacts.deterministicFindingsPath, - agentChecksPath: artifacts.agentChecksPath, - agentCheckCount: execution.agentChecks.checks.length, - } -} - -/** `--exclude` takes globs, so a directory name is escaped to match only that directory. */ -export function literalGlobPattern(path: string): string { - return path.replace(/[?*()[\]{}!#\\]/g, '\\$&') -} - -async function renderGatheringWarnings( - gathered: {ignoredScanDirectories: string[]; otherAppDirectories: string[]}, - dependencies: SecurityDependencies, -) { - if (gathered.ignoredScanDirectories.length === 0 && gathered.otherAppDirectories.length === 0) return - // `--exclude` globs are matched from the real working directory, so paths are shown relative to it too. - const workingDirectory = await fileRealPath(cwd()) - const displayPath = (directory: string) => relativePath(workingDirectory, directory) || '.' - for (const directory of gathered.ignoredScanDirectories) { - dependencies.renderWarning({ - headline: `${displayPath(directory)} is ignored by Git, so only the files Git tracks in it are scanned.`, - body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], - }) - } - for (const directory of gathered.otherAppDirectories) { - const path = displayPath(directory) - dependencies.renderWarning({ - headline: `${path} holds another app's configuration, so its files are scanned as part of this app.`, - // The directory name comes from the repository, so it's quoted before the user can paste it into a shell. - body: [ - 'Use', - {command: `--exclude ${quoteShellArgument(literalGlobPattern(path), shellForPlatform())}`}, - 'to leave it out.', - ], - }) - } -} - /** * Scans the app and replaces deterministic-findings.json and agent-checks.json. Scanning never reads or - * changes the agent's recorded findings, so it's always safe to run again. + * changes the agent's recorded findings, so it's always safe to run again. Prints nothing: the command presents the + * outcome. * - * With `listFiles`, it only resolves and gathers: it prints the gathered paths and writes nothing. + * With `listFiles`, it only resolves and gathers: it returns the gathered paths and writes nothing. * `directory` is the `--path` value, an absolute path. */ export default async function securityCheck( options: SecurityOptions, dependencies: SecurityDependencies = defaultDependencies, -): Promise { +): Promise { // Resolved first so a mistyped directory fails before any prompt. const includeDirectories = await resolveIncludeDirectories(options.includeDirs) - const canPrompt = !options.listFiles && dependencies.canPrompt() const selection = await dependencies.resolveSelection({ path: options.directory, config: options.configName, clientId: options.clientId, withoutAppConfig: options.withoutAppConfig, - allowPrompts: canPrompt, + allowPrompts: !options.listFiles && dependencies.canPrompt(), validateClientIdFlag: true, }) + if (selection.kind === 'declined') return {kind: 'cancelled'} + const {appDirectory} = selection const scope: AppSecurityScope = { include_dirs: [...options.includeDirs], @@ -233,22 +133,17 @@ export default async function securityCheck( no_git_ignore: options.noGitIgnore, } const commands = resolveAppSecurityCommands(selection, options.directory, scope) - const resolution = {selection, resultsKey: resultsKey(selection), commands} // Prompts are shown when no TOML was found and `--without-app-config` wasn't passed, or when `check` asked which TOML // to scan. const prompted = selection.kind === 'no-config' ? !options.withoutAppConfig : selection.appConfigFilePicked === true - if (prompted) { - dependencies.renderInfo({ - headline: 'To skip these prompts next time, run:', - body: [{command: formatAppSecurityCommand(commands.scan)}], - }) - } + if (prompted) options.onSelectionPrompted?.(commands) const appConfigDirectories = selection.kind === 'config' ? (selection.appConfigDirectories ?? []) : [] const {scanDirectories, requestedScanDirectories} = mergeScanDirectories( appDirectory, includeDirectories, appConfigDirectories, ) + const resolution = {selection, resultsKey: resultsKey(selection), commands, scope, scanDirectories} const scanOptions = { appDirectory, @@ -265,41 +160,22 @@ export default async function securityCheck( } if (options.listFiles) { - const {paths, ...gathered} = await dependencies.listFiles(scanOptions) - await renderGatheringWarnings(gathered, dependencies) - if (paths.length > 0) dependencies.output(paths.join('\n')) - return resolution + const listing = await dependencies.listFiles(scanOptions) + return {kind: 'listed', ...resolution, ...listing} } const execution = await dependencies.execute(scanOptions) - await renderGatheringWarnings(execution, dependencies) await dependencies.recordMetadata({num_security_findings: execution.scan.issues.length}) - const artifacts = await dependencies.writeArtifacts(appDirectory, resultsKey(selection), { + const artifacts = await dependencies.writeArtifacts(appDirectory, resolution.resultsKey, { deterministicFindings: execution.deterministicFindings, agentChecks: execution.agentChecks, }) - dependencies.renderReport( - securityReportInput(execution, artifacts, options.verbose, commands, selection, scanDirectories), - ) - - const destination = await instructionsDestination( - options, - dependencies, - canPrompt, - execution.agentChecks.checks.length, - ) - if (destination !== 'nothing') { - await dependencies.deliverInstructions({ - appDirectory, - resultsKey: resultsKey(selection), - copy: destination === 'copy', - scanScope: scope, - commands, - }) + return { + kind: 'scanned', + ...resolution, + execution, + artifacts, + blocking: {level: options.blocking, blockedChecks: blockedCheckCount(execution, options.blocking)}, } - - const exitCode = securityExitCode(execution, options.blocking) - if (exitCode !== 0) dependencies.setExitCode(exitCode) - return resolution } diff --git a/packages/app/src/cli/services/security-check/result.test.ts b/packages/app/src/cli/services/security-check/result.test.ts new file mode 100644 index 00000000000..4091decbc34 --- /dev/null +++ b/packages/app/src/cli/services/security-check/result.test.ts @@ -0,0 +1,465 @@ +import { + appSecurityInstructionsPrompt, + literalGlobPattern, + renderSecurityCheckPromptsTip, + renderSecurityCheckResult, + type AppSecurityInstructionsDestination, +} from './result.js' +import {listAppSecurityFiles} from '../app-security-api.js' +import {appSecurityArtifactPaths} from '../app-security-artifacts.js' +import { + formatAppSecurityCommand, + quoteShellArgument, + resolveAppSecurityCommands, + shellForPlatform, +} from '../app-security-commands.js' +import {appSecurityInstructions} from '../app-security-instructions.js' +import {validAppConfiguration} from '../app-security-selection.test-data.js' +import {committedSecretIssue, scan, scanExecution} from '../security-check.test-data.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {fileRealPath, inTemporaryDirectory, matchGlob, mkdir, writeFile} from '@shopify/cli-kit/node/fs' +import {unstyled} from '@shopify/cli-kit/node/output' +import {joinPath} from '@shopify/cli-kit/node/path' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import clipboard from 'clipboardy' +import {afterEach, describe, expect, test, vi} from 'vitest' +import {symlink} from 'node:fs/promises' +import type {AppSecuritySelection} from '../app-security-selection.js' +import type {AppSecurityScope} from '../app-security-engine/index.js' +import type {SecurityCheckOutcome} from '../security-check.js' + +// Tests must not replace the clipboard of the machine that runs them. +vi.mock('clipboardy', () => ({default: {write: vi.fn()}})) + +type ScannedSecurityCheck = Extract +type ListedSecurityCheck = Extract + +const originalExitCode = process.exitCode +afterEach(() => { + process.exitCode = originalExitCode + vi.unstubAllEnvs() +}) + +const appDirectory = '/tmp/unlinked-app' + +const selection: AppSecuritySelection = { + kind: 'config', + appDirectory, + appConfigFilePath: `${appDirectory}/shopify.app.toml`, + configClientId: 'toml-client-id', +} + +const noScope: AppSecurityScope = {include_dirs: [], excludes: [], no_git_ignore: false} + +function scanned(overrides: Partial = {}): ScannedSecurityCheck { + const scope = overrides.scope ?? noScope + const paths = appSecurityArtifactPaths(appDirectory, 'shopify.app') + return { + kind: 'scanned', + selection, + resultsKey: 'shopify.app', + commands: resolveAppSecurityCommands(selection, appDirectory, scope), + scope, + scanDirectories: [{directory: appDirectory, origin: 'app_directory'}], + execution: scanExecution, + artifacts: {deterministicFindingsPath: paths.deterministicFindingsPath, agentChecksPath: paths.agentChecksPath}, + blocking: {level: 'none', blockedChecks: 0}, + ...overrides, + } +} + +function listed(overrides: Partial = {}): ListedSecurityCheck { + return { + kind: 'listed', + selection, + resultsKey: 'shopify.app', + commands: resolveAppSecurityCommands(selection, appDirectory, noScope), + scope: noScope, + scanDirectories: [{directory: appDirectory, origin: 'app_directory'}], + paths: ['shopify.app.toml'], + ignoredScanDirectories: [], + otherAppDirectories: [], + ...overrides, + } +} + +const presentationOptions = { + directory: appDirectory, + verbose: false, + printInstructions: false, + skipInstructions: false, +} + +function testDependencies(destination: AppSecurityInstructionsDestination = 'nothing', canPrompt = false) { + return { + canPrompt: vi.fn(() => canPrompt), + selectInstructionsDestination: vi.fn(async () => destination), + } +} + +/** The instructions for the scan that just ran, as `check` prints them. */ +function scanInstructions(check: ScannedSecurityCheck): string { + return appSecurityInstructions({ + appDirectory: check.selection.appDirectory, + resultsKey: check.resultsKey, + commands: check.commands, + scanScope: check.scope, + }) +} + +/** Presents the outcome and returns what reached stdout and stderr, without styling. */ +async function present( + check: SecurityCheckOutcome, + options: Partial = {}, + dependencies = testDependencies(), +) { + return withCapturedStandardStreams(async ({stdout, stderr}) => { + await renderSecurityCheckResult(check, {...presentationOptions, ...options}, dependencies) + return {stdout: unstyled(stdout()), stderr: unstyled(stderr())} + }) +} + +// Boxes wrap text across lines and draw borders, so compare their text with the borders and line breaks removed. +function boxText(output: string): string { + return output.replaceAll(/[│╭╮╰╯─]/g, ' ').replace(/\s+/g, ' ') +} + +describe('literalGlobPattern', () => { + test.each([ + ['apps/[child]', 'apps/c'], + ['apps/{a,b}', 'apps/a'], + ['apps/a*b', 'apps/aXb'], + ['apps/x?y', 'apps/xzy'], + ['#child', 'child'], + ['!child', 'src'], + ])('gives a glob that matches %s and not %s', (directory, sibling) => { + const pattern = literalGlobPattern(directory) + + expect(matchGlob(directory, pattern)).toBe(true) + expect(matchGlob(sibling, pattern)).toBe(false) + }) + + test('gives an --exclude that removes only that directory from the gathered files', async () => { + await inTemporaryDirectory(async (directory) => { + const root = await fileRealPath(directory) + await Promise.all(['!child', '[child]', 'c', 'src'].map((name) => mkdir(joinPath(root, name)))) + await Promise.all([ + writeFile(joinPath(root, 'shopify.app.toml'), validAppConfiguration()), + writeFile(joinPath(root, '!child', 'shopify.app.toml'), 'name = "Negated"\n'), + writeFile(joinPath(root, '[child]', 'shopify.app.toml'), 'name = "Bracketed"\n'), + writeFile(joinPath(root, 'c', 'index.ts'), 'export const c = true\n'), + writeFile(joinPath(root, 'src', 'index.ts'), 'export const src = true\n'), + ]) + vi.stubEnv('INIT_CWD', root) + + const gathered = await listAppSecurityFiles({ + appDirectory: root, + scanDirectories: [root], + requestedScanDirectories: [root], + excludePatterns: ['!child', '[child]'].map(literalGlobPattern), + }) + + expect(gathered.paths).toEqual(['c/index.ts', 'shopify.app.toml', 'src/index.ts']) + expect(gathered.otherAppDirectories).toEqual([]) + }) + }) +}) + +describe('renderSecurityCheckPromptsTip', () => { + test('shows the command that repeats the run without prompts on stderr', async () => { + const commands = resolveAppSecurityCommands(selection, appDirectory, noScope) + + const {stdout, stderr} = await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityCheckPromptsTip(commands) + return {stdout: stdout(), stderr: unstyled(stderr())} + }) + + expect(stdout).toBe('') + expect(boxText(stderr)).toContain( + `To skip these prompts next time, run: \`${formatAppSecurityCommand(commands.scan)}\``, + ) + }) +}) + +describe('renderSecurityCheckResult gathering warnings', () => { + /** The warning for a directory that holds another app configuration, quoted for the shell the tests run in. */ + function otherAppWarning(displayPath: string, excludePattern = displayPath) { + return `${displayPath} holds another app's configuration, so its files are scanned as part of this app. Use \`--exclude ${quoteShellArgument(excludePattern, shellForPlatform())}\` to leave it out.` + } + + test('warns once for each scan directory that Git ignores, relative to the working directory, before the report', async () => { + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + vi.stubEnv('INIT_CWD', directory) + const execution = { + ...scanExecution, + ignoredScanDirectories: [joinPath(realDirectory, 'unlinked-app'), realDirectory], + } + + const {stderr} = await present(scanned({execution})) + + const warnings = boxText(stderr) + const first = 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.' + const second = '. is ignored by Git, so only the files Git tracks in it are scanned.' + expect(warnings).toContain(`${first} Use \`--no-git-ignore\` to scan everything in it.`) + expect(warnings).toContain(`${second} Use \`--no-git-ignore\` to scan everything in it.`) + expect(warnings.indexOf(first)).toBeLessThan(warnings.indexOf(second)) + expect(warnings.indexOf(second)).toBeLessThan(warnings.indexOf('No security issues found.')) + }) + }) + + test('warns once for each directory that holds another app configuration, with the --exclude that leaves it out', async () => { + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + vi.stubEnv('INIT_CWD', directory) + const execution = { + ...scanExecution, + otherAppDirectories: [joinPath(realDirectory, 'apps', 'child'), joinPath(realDirectory, '..', 'backend')], + } + + const {stderr} = await present(scanned({execution})) + + expect(boxText(stderr)).toContain(otherAppWarning('apps/child')) + expect(boxText(stderr)).toContain(otherAppWarning('../backend')) + }) + }) + + test('escapes and quotes a directory name that is glob and shell syntax', async () => { + await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) + const execution = { + ...scanExecution, + otherAppDirectories: [joinPath(await fileRealPath(directory), 'apps', '$(touch PWNED)')], + } + + const {stderr} = await present(scanned({execution})) + + expect(boxText(stderr)).toContain(otherAppWarning('apps/$(touch PWNED)', 'apps/$\\(touch PWNED\\)')) + }) + }) + + test('shows warnings relative to the real working directory when it is reached through a symbolic link', async () => { + await inTemporaryDirectory(async (directory) => { + const app = joinPath(await fileRealPath(directory), 'app') + await mkdir(app) + const link = joinPath(directory, 'linked-app') + await symlink(app, link, 'dir') + vi.stubEnv('INIT_CWD', link) + const execution = {...scanExecution, otherAppDirectories: [joinPath(app, 'apps', 'child')]} + + const {stderr} = await present(scanned({execution})) + + expect(boxText(stderr)).toContain(otherAppWarning('apps/child')) + }) + }) + + test('does not warn when no scan directory is ignored', async () => { + const {stderr} = await present(scanned()) + + expect(stderr).not.toContain('ignored by Git') + expect(stderr).not.toContain("another app's configuration") + }) +}) + +describe('renderSecurityCheckResult for a scan', () => { + test('renders the report on stderr and leaves stdout and the exit code alone', async () => { + process.exitCode = undefined + const dependencies = testDependencies() + + const {stdout, stderr} = await present(scanned(), {}, dependencies) + + expect(stdout).toBe('') + const report = boxText(stderr) + expect(report).toContain('No security issues found.') + expect(report).toContain('Config file shopify.app.toml') + expect(report).toContain('31 checks ready for your coding agent.') + expect(report).toContain('Deterministic findings: .shopify/app-security/shopify.app/deterministic-findings.json') + expect(process.exitCode).toBeUndefined() + }) + + test('shows every occurrence with --verbose', async () => { + const execution = { + ...scanExecution, + scan: { + ...scan, + issues: [committedSecretIssue, {...committedSecretIssue, location: {file: 'app/routes/other.ts'}}], + }, + } + + const {stderr} = await present(scanned({execution}), {verbose: true}) + + expect(boxText(stderr)).toContain('Fix: remove it') + }) + + test('does not offer coding-agent instructions in CI or another non-interactive environment', async () => { + const dependencies = testDependencies('copy', false) + + const {stdout} = await present(scanned(), {}, dependencies) + + expect(dependencies.canPrompt).toHaveBeenCalledOnce() + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + expect(clipboard.write).not.toHaveBeenCalled() + expect(stdout).toBe('') + }) + + test('offers the instructions after the report, with copying first', async () => { + let stderrWhenPrompted = '' + const dependencies = testDependencies('nothing', true) + + const {stdout} = await withCapturedStandardStreams(async ({stdout, stderr}) => { + dependencies.selectInstructionsDestination.mockImplementation(async () => { + stderrWhenPrompted = unstyled(stderr()) + return 'nothing' + }) + await renderSecurityCheckResult(scanned(), presentationOptions, dependencies) + return {stdout: stdout()} + }) + + expect(appSecurityInstructionsPrompt(31)).toEqual({ + message: + '31 recommended agent checks available to complete your scan. How do you want to pass that prompt to your agent?', + choices: [ + {label: 'Copy instructions to the clipboard', value: 'copy'}, + {label: 'Print instructions to the terminal', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ], + defaultValue: 'copy', + }) + expect(dependencies.selectInstructionsDestination).toHaveBeenCalledExactlyOnceWith( + appSecurityInstructionsPrompt(31), + ) + expect(boxText(stderrWhenPrompted)).toContain('No security issues found.') + expect(stdout).toBe('') + expect(clipboard.write).not.toHaveBeenCalled() + }) + + test('names a single recommended agent check in the singular', () => { + expect(appSecurityInstructionsPrompt(1).message).toBe( + '1 recommended agent check available to complete your scan. How do you want to pass that prompt to your agent?', + ) + }) + + test('copies the instructions for the scan that just ran and confirms on stderr', async () => { + const check = scanned() + + const {stdout, stderr} = await present(check, {}, testDependencies('copy', true)) + + expect(clipboard.write).toHaveBeenCalledExactlyOnceWith(scanInstructions(check)) + expect(scanInstructions(check)).toContain('### 1. Use the existing scan results') + expect(stderr).toContain('Copied app security check instructions to the clipboard') + expect(stdout).toBe('') + }) + + test('prints the instructions for the scan that just ran when selected', async () => { + const check = scanned() + + const {stdout} = await present(check, {}, testDependencies('print', true)) + + expect(stdout).toBe(`${scanInstructions(check)}\n`) + expect(clipboard.write).not.toHaveBeenCalled() + }) + + test('gives the instructions the exact scope of the run, as typed', async () => { + const scope = {include_dirs: ['backend', './backend/'], excludes: ['**/generated', '!keep'], no_git_ignore: true} + const check = scanned({scope}) + + const {stdout} = await present(check, {printInstructions: true}) + + expect(stdout).toContain(JSON.stringify(scope)) + expect(stdout).toContain(formatAppSecurityCommand(check.commands.scan)) + }) + + test('does nothing when selected', async () => { + const dependencies = testDependencies('nothing', true) + + const {stdout} = await present(scanned(), {}, dependencies) + + expect(dependencies.selectInstructionsDestination).toHaveBeenCalledOnce() + expect(clipboard.write).not.toHaveBeenCalled() + expect(stdout).toBe('') + }) + + test.each([false, true])( + '--print-instructions prints without a delivery prompt (interactive=%s)', + async (interactive) => { + const check = scanned() + const dependencies = testDependencies('copy', interactive) + + const {stdout} = await present(check, {printInstructions: true}, dependencies) + + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + expect(stdout).toBe(`${scanInstructions(check)}\n`) + }, + ) + + test('--skip-instructions never offers instructions', async () => { + const dependencies = testDependencies('copy', true) + + const {stdout} = await present(scanned(), {skipInstructions: true}, dependencies) + + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + expect(clipboard.write).not.toHaveBeenCalled() + expect(stdout).toBe('') + }) + + test('aborts when the instructions cannot be copied', async () => { + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + await expect(present(scanned(), {}, testDependencies('copy', true))).rejects.toMatchObject({ + code: 'clipboard-unavailable', + }) + }) + + test('sets exit code 1 after the output when the scan breaches --blocking', async () => { + process.exitCode = undefined + const execution = {...scanExecution, scan: {...scan, issues: [committedSecretIssue]}} + + const {stdout, stderr} = await present(scanned({execution, blocking: {level: 'high', blockedChecks: 1}}), { + printInstructions: true, + }) + + expect(boxText(stderr)).toContain('1 security issue found.') + expect(stdout).toContain('### 1. Use the existing scan results') + expect(process.exitCode).toBe(1) + }) +}) + +describe('renderSecurityCheckResult for --list-files', () => { + test('prints each gathered path on its own line on stdout, after the gathering warnings on stderr', async () => { + await inTemporaryDirectory(async (directory) => { + vi.stubEnv('INIT_CWD', directory) + const dependencies = testDependencies('copy', true) + const check = listed({ + paths: ['../backend/server.ts', 'app/routes/index.ts', 'shopify.app.toml'], + ignoredScanDirectories: [joinPath(await fileRealPath(directory), 'unlinked-app')], + }) + + const {stdout, stderr} = await present(check, {}, dependencies) + + expect(stdout).toBe('../backend/server.ts\napp/routes/index.ts\nshopify.app.toml\n') + expect(boxText(stderr)).toContain( + 'unlinked-app is ignored by Git, so only the files Git tracks in it are scanned.', + ) + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + }) + }) + + test('prints nothing when no path is gathered', async () => { + const {stdout, stderr} = await present(listed({paths: []})) + + expect(stdout).toBe('') + expect(stderr).toBe('') + }) +}) + +describe('renderSecurityCheckResult when scanning without app configuration was declined', () => { + test('aborts as when no app configuration is found', async () => { + const error: unknown = await present({kind: 'cancelled'}).catch((caught: unknown) => caught) + + expect(error).toBeInstanceOf(AbortError) + expect((error as AbortError).message).toBe(`No app configuration found at or above ${appDirectory}.`) + expect((error as AbortError).tryMessage).toBe( + 'Pass `--path` to your app directory, or scan without app configuration with `--without-app-config --client-id `.', + ) + }) +}) diff --git a/packages/app/src/cli/services/security-check/result.ts b/packages/app/src/cli/services/security-check/result.ts new file mode 100644 index 00000000000..b07ebc5c164 --- /dev/null +++ b/packages/app/src/cli/services/security-check/result.ts @@ -0,0 +1,161 @@ +import { + appSecurityInstructions, + deliverAppSecurityInstructions, + renderAppSecurityInstructions, +} from '../app-security-instructions.js' +import { + formatAppSecurityCommand, + quoteShellArgument, + shellForPlatform, + type AppSecurityCommands, +} from '../app-security-commands.js' +import {MissingAppConfigurationError} from '../app-security-selection.js' +import {renderSecurityReport} from '../security-output.js' +import {fileRealPath} from '@shopify/cli-kit/node/fs' +import {outputResult} from '@shopify/cli-kit/node/output' +import {cwd, relativePath} from '@shopify/cli-kit/node/path' +import {terminalSupportsPrompting} from '@shopify/cli-kit/node/system' +import {renderInfo, renderSelectPrompt, renderWarning} from '@shopify/cli-kit/node/ui' +import type {RenderSelectPromptOptions} from '@shopify/cli-kit/node/ui' +import type {SecurityReportInput} from '../security-output.js' +import type {SecurityCheckOutcome} from '../security-check.js' + +export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' + +type ScannedSecurityCheck = Extract + +interface SecurityCheckPresentationOptions { + /** The `--path` value, an absolute path. */ + directory: string + verbose: boolean + printInstructions: boolean + skipInstructions: boolean +} + +interface SecurityCheckPresenterDependencies { + canPrompt(): boolean + selectInstructionsDestination( + prompt: RenderSelectPromptOptions, + ): Promise +} + +const defaultDependencies: SecurityCheckPresenterDependencies = { + canPrompt: terminalSupportsPrompting, + selectInstructionsDestination: (prompt) => renderSelectPrompt(prompt), +} + +export function appSecurityInstructionsPrompt( + agentCheckCount: number, +): RenderSelectPromptOptions { + return { + message: `${agentCheckCount} recommended agent ${agentCheckCount === 1 ? 'check' : 'checks'} available to complete your scan. How do you want to pass that prompt to your agent?`, + choices: [ + {label: 'Copy instructions to the clipboard', value: 'copy'}, + {label: 'Print instructions to the terminal', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ], + defaultValue: 'copy', + } +} + +/** Shows how to repeat a run whose selection prompted, without the prompts. */ +export function renderSecurityCheckPromptsTip(commands: AppSecurityCommands): void { + renderInfo({ + headline: 'To skip these prompts next time, run:', + body: [{command: formatAppSecurityCommand(commands.scan)}], + }) +} + +/** + * Presents the outcome of `securityCheck`: the gathering warnings, then the listed files, or the report followed by + * the coding-agent instructions. Sets exit code 1 after the output when the scan breaches `--blocking`. Declining to + * scan without app configuration aborts, as when there is none and prompts are off. + */ +export async function renderSecurityCheckResult( + check: SecurityCheckOutcome, + options: SecurityCheckPresentationOptions, + dependencies: SecurityCheckPresenterDependencies = defaultDependencies, +): Promise { + if (check.kind === 'cancelled') throw new MissingAppConfigurationError(options.directory) + + if (check.kind === 'listed') { + await renderGatheringWarnings(check) + if (check.paths.length > 0) outputResult(check.paths.join('\n')) + return + } + + await renderGatheringWarnings(check.execution) + renderSecurityReport(securityReportInput(check, options.verbose)) + + const destination = await instructionsDestination(options, check.execution.agentChecks.checks.length, dependencies) + if (destination !== 'nothing') { + const instructions = appSecurityInstructions({ + appDirectory: check.selection.appDirectory, + resultsKey: check.resultsKey, + commands: check.commands, + scanScope: check.scope, + }) + const delivery = {copy: destination === 'copy'} + await deliverAppSecurityInstructions(instructions, delivery) + renderAppSecurityInstructions(instructions, delivery) + } + + if (check.blocking.blockedChecks > 0) process.exitCode = 1 +} + +async function instructionsDestination( + options: SecurityCheckPresentationOptions, + agentCheckCount: number, + dependencies: SecurityCheckPresenterDependencies, +): Promise { + if (options.skipInstructions) return 'nothing' + if (options.printInstructions) return 'print' + if (!dependencies.canPrompt()) return 'nothing' + return dependencies.selectInstructionsDestination(appSecurityInstructionsPrompt(agentCheckCount)) +} + +function securityReportInput(check: ScannedSecurityCheck, verbose: boolean): SecurityReportInput { + const {execution, artifacts} = check + return { + scan: execution.scan, + selection: check.selection, + scanDirectories: check.scanDirectories, + engine: execution.engine, + verbose, + elapsedMilliseconds: execution.elapsedMilliseconds, + commands: check.commands, + deterministicFindingsPath: artifacts.deterministicFindingsPath, + agentChecksPath: artifacts.agentChecksPath, + agentCheckCount: execution.agentChecks.checks.length, + } +} + +/** `--exclude` takes globs, so a directory name is escaped to match only that directory. */ +export function literalGlobPattern(path: string): string { + return path.replace(/[?*()[\]{}!#\\]/g, '\\$&') +} + +async function renderGatheringWarnings(gathered: {ignoredScanDirectories: string[]; otherAppDirectories: string[]}) { + if (gathered.ignoredScanDirectories.length === 0 && gathered.otherAppDirectories.length === 0) return + // `--exclude` globs are matched from the real working directory, so paths are shown relative to it too. + const workingDirectory = await fileRealPath(cwd()) + const displayPath = (directory: string) => relativePath(workingDirectory, directory) || '.' + for (const directory of gathered.ignoredScanDirectories) { + renderWarning({ + headline: `${displayPath(directory)} is ignored by Git, so only the files Git tracks in it are scanned.`, + body: ['Use', {command: '--no-git-ignore'}, 'to scan everything in it.'], + }) + } + for (const directory of gathered.otherAppDirectories) { + const path = displayPath(directory) + renderWarning({ + headline: `${path} holds another app's configuration, so its files are scanned as part of this app.`, + // The directory name comes from the repository, so it's quoted before the user can paste it into a shell. + body: [ + 'Use', + {command: `--exclude ${quoteShellArgument(literalGlobPattern(path), shellForPlatform())}`}, + 'to leave it out.', + ], + }) + } +} diff --git a/packages/app/src/cli/services/security-review.test.ts b/packages/app/src/cli/services/security-review.test.ts index 23f2be4283b..92d7f0a4325 100644 --- a/packages/app/src/cli/services/security-review.test.ts +++ b/packages/app/src/cli/services/security-review.test.ts @@ -359,7 +359,8 @@ describe('securityReview --client-id lookup', () => { function lookUpDependencies(lookUpApp: (clientId: string) => Promise) { return { ...testDependencies(), - resolveSelection: (options: AppSecuritySelectionOptions) => + // Review never prompts, so the selection is never declined. + resolveSelection: (options: AppSecuritySelectionOptions & {allowPrompts: false}) => resolveAppSecuritySelection(options, { confirmScanWithoutAppConfig: async () => true, pickClientId: async () => 'picked-client-id', From ef84803c9cbd091a1aa6c5a20fedec2a29c112a4 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 20 Oct 2026 14:43:23 -0700 Subject: [PATCH 2/4] Warn when app security check can't copy the instructions Copying the coding-agent instructions is the last step of `check`, after the scan finished and its report is shown. A clipboard that isn't available aborted the command, so the exit code no longer reported the scan. `check` now warns with the clipboard tool's message and suggests `--print-instructions` instead, and the exit code follows `--blocking`. `instructions --copy` still fails, because copying is all it does. --- .../app/security/check.integration.test.ts | 67 +++++++++++++++++++ .../services/security-check/result.test.ts | 31 +++++++-- .../src/cli/services/security-check/result.ts | 27 +++++++- 3 files changed, 115 insertions(+), 10 deletions(-) diff --git a/packages/app/src/cli/commands/app/security/check.integration.test.ts b/packages/app/src/cli/commands/app/security/check.integration.test.ts index 2eabd88e1c3..29d0c87bfa3 100644 --- a/packages/app/src/cli/commands/app/security/check.integration.test.ts +++ b/packages/app/src/cli/commands/app/security/check.integration.test.ts @@ -8,6 +8,8 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {fileExists, fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs' import {unstyled} from '@shopify/cli-kit/node/output' import {joinPath, normalizePath} from '@shopify/cli-kit/node/path' +import {renderSelectPrompt} from '@shopify/cli-kit/node/ui' +import clipboard from 'clipboardy' import {describe, expect, test, vi} from 'vitest' import {mkdir, readFile, writeFile} from 'node:fs/promises' // eslint-disable-next-line n/prefer-global/console @@ -33,6 +35,13 @@ vi.mock('../../../services/context.js', async (importOriginal) => ({ ...(await importOriginal()), appFromIdentifiers: vi.fn(), })) +// Tests answer the prompts. How a real terminal draws them is checked by hand. +vi.mock('@shopify/cli-kit/node/ui', async (importOriginal) => ({ + ...(await importOriginal()), + renderSelectPrompt: vi.fn(), +})) +// Tests must not replace the clipboard of the machine that runs them. +vi.mock('clipboardy', () => ({default: {write: vi.fn()}})) async function createApp(directory: string): Promise<{nestedDirectory: string}> { const routesDirectory = joinPath(directory, 'app', 'routes') @@ -47,6 +56,13 @@ async function createApp(directory: string): Promise<{nestedDirectory: string}> return {nestedDirectory: routesDirectory} } +/** An app whose source holds a Shopify access token, which the deterministic scan reports as a high finding. */ +async function createAppWithSecret(directory: string): Promise { + await createApp(directory) + const testToken = ['shpat', '0123456789abcdef0123456789abcdef'].join('_') + await writeFile(joinPath(directory, 'app', 'routes', 'index.ts'), `const access_token = "${testToken}"\n`) +} + async function readJson(path: string): Promise { return JSON.parse(await readFile(path, 'utf8')) } @@ -96,6 +112,31 @@ async function runCommand(argv: string[]) { } } +/** + * Runs the command as if stdin and stderr were a terminal, where it prompts unless `--no-input` is passed. CLI Kit reads + * `--no-input` from the process arguments, not from the arguments given to `run`, so the environment sets it as well. + */ +async function runInTerminal(argv: string[], {noInput = false} = {}) { + vi.stubEnv('CI', '') + if (noInput) vi.stubEnv('SHOPIFY_FLAG_NO_INPUT', '1') + const streams = [process.stdin, process.stderr] + const descriptors = streams.map((stream) => Object.getOwnPropertyDescriptor(stream, 'isTTY')) + for (const stream of streams) Object.defineProperty(stream, 'isTTY', {value: true, configurable: true}) + try { + return await runCommand(noInput ? [...argv, '--no-input'] : argv) + } finally { + streams.forEach((stream, index) => { + const descriptor = descriptors[index] + if (descriptor) { + Object.defineProperty(stream, 'isTTY', descriptor) + } else { + Reflect.deleteProperty(stream, 'isTTY') + } + }) + vi.unstubAllEnvs() + } +} + describe('app security check command boundary', () => { test('scans an app from a nested directory and writes deterministic-findings.json and agent-checks.json', async () => { await inTemporaryDirectory(async (directory) => { @@ -336,6 +377,32 @@ describe('app security check command boundary', () => { }) }) + test.each([ + {blocking: 'none', exitCode: 0}, + {blocking: 'high', exitCode: 1}, + ])( + 'warns when the instructions cannot be copied, after the report, and exits as --blocking $blocking requires', + async ({blocking, exitCode}) => { + await inTemporaryDirectory(async (directory) => { + await createAppWithSecret(directory) + vi.mocked(renderSelectPrompt).mockResolvedValue('copy') + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + const result = await runInTerminal(['--path', directory, '--blocking', blocking]) + + expect(result.exitCode).toBe(exitCode) + expect(result.stdout).toBe('') + const stderr = boxText(result.stderr) + expect(stderr).toContain( + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy", + ) + expect(stderr).toContain('Run the check again with `--print-instructions` to print them.') + expect(stderr.indexOf('Config file shopify.app.toml')).toBeLessThan(stderr.indexOf("Couldn't copy")) + expect(clipboard.write).toHaveBeenCalledOnce() + }) + }, + ) + test('looks up --client-id, not the TOML client ID', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) diff --git a/packages/app/src/cli/services/security-check/result.test.ts b/packages/app/src/cli/services/security-check/result.test.ts index 4091decbc34..7459917027c 100644 --- a/packages/app/src/cli/services/security-check/result.test.ts +++ b/packages/app/src/cli/services/security-check/result.test.ts @@ -402,13 +402,30 @@ describe('renderSecurityCheckResult for a scan', () => { expect(stdout).toBe('') }) - test('aborts when the instructions cannot be copied', async () => { - vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) - - await expect(present(scanned(), {}, testDependencies('copy', true))).rejects.toMatchObject({ - code: 'clipboard-unavailable', - }) - }) + test.each([ + {level: 'none', blockedChecks: 0, exitCode: undefined}, + {level: 'high', blockedChecks: 1, exitCode: 1}, + ] as const)( + 'warns instead of aborting when the instructions cannot be copied, and exits as --blocking $level requires', + async ({level, blockedChecks, exitCode}) => { + process.exitCode = undefined + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + const execution = {...scanExecution, scan: {...scan, issues: [committedSecretIssue]}} + const check = scanned({execution, blocking: {level, blockedChecks}}) + + const {stdout, stderr} = await present(check, {}, testDependencies('copy', true)) + + const warning = boxText(stderr) + expect(warning).toContain( + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy", + ) + expect(warning).toContain('Run the check again with `--print-instructions` to print them.') + expect(warning.indexOf('1 security issue found.')).toBeLessThan(warning.indexOf("Couldn't copy")) + expect(warning).not.toContain('Copied app security check instructions') + expect(stdout).toBe('') + expect(process.exitCode).toBe(exitCode) + }, + ) test('sets exit code 1 after the output when the scan breaches --blocking', async () => { process.exitCode = undefined diff --git a/packages/app/src/cli/services/security-check/result.ts b/packages/app/src/cli/services/security-check/result.ts index b07ebc5c164..1b783dff0a9 100644 --- a/packages/app/src/cli/services/security-check/result.ts +++ b/packages/app/src/cli/services/security-check/result.ts @@ -1,5 +1,6 @@ import { appSecurityInstructions, + ClipboardUnavailableError, deliverAppSecurityInstructions, renderAppSecurityInstructions, } from '../app-security-instructions.js' @@ -95,14 +96,34 @@ export async function renderSecurityCheckResult( commands: check.commands, scanScope: check.scope, }) - const delivery = {copy: destination === 'copy'} - await deliverAppSecurityInstructions(instructions, delivery) - renderAppSecurityInstructions(instructions, delivery) + if (destination === 'print') { + renderAppSecurityInstructions(instructions, {copy: false}) + } else if (await copyInstructions(instructions)) { + renderAppSecurityInstructions(instructions, {copy: true}) + } } if (check.blocking.blockedChecks > 0) process.exitCode = 1 } +/** + * Copies the instructions to the clipboard, and returns whether they're on it. The scan has already finished and its + * report is shown, so a clipboard that isn't available only warns. + */ +async function copyInstructions(instructions: string): Promise { + try { + await deliverAppSecurityInstructions(instructions, {copy: true}) + return true + } catch (error) { + if (!(error instanceof ClipboardUnavailableError)) throw error + renderWarning({ + headline: error.message, + body: ['Run the check again with', {command: '--print-instructions'}, 'to print them.'], + }) + return false + } +} + async function instructionsDestination( options: SecurityCheckPresentationOptions, agentCheckCount: number, From c00505a4f3f6def4122553928fefd0761e8a0c1f Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 20 Oct 2026 14:58:39 -0700 Subject: [PATCH 3/4] Add typed JSON output to app security check `--json` prints one result for the scan: the app, the results directory and the two files that the scan replaced, its scope and scan directories, the number of agent checks, and whether `--blocking` passed. A breach still exits 1, after the result is printed. The result holds no findings: `review --json` shows them. Paths are absolute, with native separators. In a terminal the instructions prompt is still offered, on stderr, with choices to include the instructions in the result, and to copy them as well by default. `--print-instructions` includes them without asking. A failed copy warns on stderr and reports `copied: false`. `--list-files` lists the gathered files with absolute paths. Declining to scan without app configuration prints a cancelled result and exits 0. Gathering warnings and the tip to skip the prompts are events on stderr. `check` was the last app security command without typed JSON output, so the exceptions list drops its app security section. --- .../generated/generated_docs_data_v2.json | 15 +- .../app/security/check.integration.test.ts | 452 ++++++++++++++++-- .../cli/commands/app/security/check.test.ts | 28 +- .../src/cli/commands/app/security/check.ts | 37 +- .../tests/layout-catalogue.test.ts | 11 +- .../services/security-check/result.test.ts | 336 ++++++++++++- .../src/cli/services/security-check/result.ts | 167 +++++-- .../cli/services/security-check/types.test.ts | 78 +++ .../src/cli/services/security-check/types.ts | 84 ++++ packages/cli/README.md | 300 +++++++++++- packages/cli/oclif.manifest.json | 17 +- .../rules/json-output-command-exceptions.js | 3 - 12 files changed, 1419 insertions(+), 109 deletions(-) create mode 100644 packages/app/src/cli/services/security-check/types.test.ts create mode 100644 packages/app/src/cli/services/security-check/types.ts diff --git a/docs-shopify.dev/generated/generated_docs_data_v2.json b/docs-shopify.dev/generated/generated_docs_data_v2.json index 56898354403..3519e34ba32 100644 --- a/docs-shopify.dev/generated/generated_docs_data_v2.json +++ b/docs-shopify.dev/generated/generated_docs_data_v2.json @@ -3364,7 +3364,7 @@ "syntaxKind": "PropertySignature", "name": "--list-files", "value": "''", - "description": "Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for.", + "description": "Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for. With --json, the result lists them with absolute paths.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_LIST_FILES" }, @@ -3409,7 +3409,7 @@ "syntaxKind": "PropertySignature", "name": "--print-instructions", "value": "''", - "description": "Print coding-agent instructions without prompting for how to deliver them.", + "description": "Print coding-agent instructions without prompting for how to deliver them. With --json, the instructions are included in the result.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS" }, @@ -3448,9 +3448,18 @@ "description": "The name of the app configuration.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_CONFIG" + }, + { + "filePath": "docs-shopify.dev/commands/interfaces/app-security-check.interface.ts", + "syntaxKind": "PropertySignature", + "name": "-j, --json", + "value": "''", + "description": "Output the result as JSON. Automatically disables color output.", + "isOptional": true, + "environmentValue": "SHOPIFY_FLAG_JSON" } ], - "value": "export interface appsecuritycheck {\n /**\n * The minimum finding severity that causes a non-zero exit code: high, medium, or low. Defaults to none, which never fails on findings.\n * @environment SHOPIFY_FLAG_APP_SECURITY_BLOCKING\n */\n '--blocking '?: string\n\n /**\n * The Client ID of your app.\n * @environment SHOPIFY_FLAG_CLIENT_ID\n */\n '--client-id '?: string\n\n /**\n * The name of the app configuration.\n * @environment SHOPIFY_FLAG_APP_CONFIG\n */\n '-c, --config '?: string\n\n /**\n * Skip paths that match this glob, relative to the working directory. Repeat the flag to add globs. The selected app configuration file can't be excluded.\n *\n */\n '--exclude '?: string\n\n /**\n * Also scan this directory, relative to the working directory. Repeat the flag to add directories. Use it for code that lives outside the app directory, such as a backend or a shared library.\n *\n */\n '--include-dir '?: string\n\n /**\n * Print the command's JSON schemas.\n * @environment SHOPIFY_FLAG_JSON_SCHEMA\n */\n '--json-schema'?: ''\n\n /**\n * Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for.\n * @environment SHOPIFY_FLAG_LIST_FILES\n */\n '--list-files'?: ''\n\n /**\n * Disable color output.\n * @environment SHOPIFY_FLAG_NO_COLOR\n */\n '--no-color'?: ''\n\n /**\n * Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.\n * @environment SHOPIFY_FLAG_NO_GIT_IGNORE\n */\n '--no-git-ignore'?: ''\n\n /**\n * Disable interactive prompts and browser authentication.\n * @environment SHOPIFY_FLAG_NO_INPUT\n */\n '--no-input'?: ''\n\n /**\n * The path to your app directory.\n * @environment SHOPIFY_FLAG_PATH\n */\n '--path '?: string\n\n /**\n * Print coding-agent instructions without prompting for how to deliver them.\n * @environment SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS\n */\n '--print-instructions'?: ''\n\n /**\n * Don't offer the coding-agent instructions after the scan. Use it when you want only the deterministic scan results.\n * @environment SHOPIFY_FLAG_APP_SECURITY_SKIP_INSTRUCTIONS\n */\n '--skip-instructions'?: ''\n\n /**\n * Increase the verbosity of the output. May include sensitive data.\n * @environment SHOPIFY_FLAG_VERBOSE\n */\n '--verbose'?: ''\n\n /**\n * Scan --path as an app with no app configuration file. Config checks are skipped. Requires --client-id.\n * @environment SHOPIFY_FLAG_WITHOUT_APP_CONFIG\n */\n '--without-app-config'?: ''\n}" + "value": "export interface appsecuritycheck {\n /**\n * The minimum finding severity that causes a non-zero exit code: high, medium, or low. Defaults to none, which never fails on findings.\n * @environment SHOPIFY_FLAG_APP_SECURITY_BLOCKING\n */\n '--blocking '?: string\n\n /**\n * The Client ID of your app.\n * @environment SHOPIFY_FLAG_CLIENT_ID\n */\n '--client-id '?: string\n\n /**\n * The name of the app configuration.\n * @environment SHOPIFY_FLAG_APP_CONFIG\n */\n '-c, --config '?: string\n\n /**\n * Skip paths that match this glob, relative to the working directory. Repeat the flag to add globs. The selected app configuration file can't be excluded.\n *\n */\n '--exclude '?: string\n\n /**\n * Also scan this directory, relative to the working directory. Repeat the flag to add directories. Use it for code that lives outside the app directory, such as a backend or a shared library.\n *\n */\n '--include-dir '?: string\n\n /**\n * Output the result as JSON. Automatically disables color output.\n * @environment SHOPIFY_FLAG_JSON\n */\n '-j, --json'?: ''\n\n /**\n * Print the command's JSON schemas.\n * @environment SHOPIFY_FLAG_JSON_SCHEMA\n */\n '--json-schema'?: ''\n\n /**\n * Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for. With --json, the result lists them with absolute paths.\n * @environment SHOPIFY_FLAG_LIST_FILES\n */\n '--list-files'?: ''\n\n /**\n * Disable color output.\n * @environment SHOPIFY_FLAG_NO_COLOR\n */\n '--no-color'?: ''\n\n /**\n * Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.\n * @environment SHOPIFY_FLAG_NO_GIT_IGNORE\n */\n '--no-git-ignore'?: ''\n\n /**\n * Disable interactive prompts and browser authentication.\n * @environment SHOPIFY_FLAG_NO_INPUT\n */\n '--no-input'?: ''\n\n /**\n * The path to your app directory.\n * @environment SHOPIFY_FLAG_PATH\n */\n '--path '?: string\n\n /**\n * Print coding-agent instructions without prompting for how to deliver them. With --json, the instructions are included in the result.\n * @environment SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS\n */\n '--print-instructions'?: ''\n\n /**\n * Don't offer the coding-agent instructions after the scan. Use it when you want only the deterministic scan results.\n * @environment SHOPIFY_FLAG_APP_SECURITY_SKIP_INSTRUCTIONS\n */\n '--skip-instructions'?: ''\n\n /**\n * Increase the verbosity of the output. May include sensitive data.\n * @environment SHOPIFY_FLAG_VERBOSE\n */\n '--verbose'?: ''\n\n /**\n * Scan --path as an app with no app configuration file. Config checks are skipped. Requires --client-id.\n * @environment SHOPIFY_FLAG_WITHOUT_APP_CONFIG\n */\n '--without-app-config'?: ''\n}" } }, "appsecurityclean": { diff --git a/packages/app/src/cli/commands/app/security/check.integration.test.ts b/packages/app/src/cli/commands/app/security/check.integration.test.ts index 29d0c87bfa3..32901da8a9d 100644 --- a/packages/app/src/cli/commands/app/security/check.integration.test.ts +++ b/packages/app/src/cli/commands/app/security/check.integration.test.ts @@ -8,12 +8,13 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {fileExists, fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs' import {unstyled} from '@shopify/cli-kit/node/output' import {joinPath, normalizePath} from '@shopify/cli-kit/node/path' -import {renderSelectPrompt} from '@shopify/cli-kit/node/ui' +import {renderConfirmationPrompt, renderSelectPrompt} from '@shopify/cli-kit/node/ui' import clipboard from 'clipboardy' import {describe, expect, test, vi} from 'vitest' -import {mkdir, readFile, writeFile} from 'node:fs/promises' -// eslint-disable-next-line n/prefer-global/console -import {Console} from 'node:console' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import {mkdir, readFile, rm, writeFile} from 'node:fs/promises' +// eslint-disable-next-line no-restricted-imports -- Verify native filesystem paths in JSON output. +import {resolve} from 'node:path' // Exercise actual stdout/stderr instead of CLI-kit's unit-test log collector. vi.mock('@shopify/cli-kit/node/context/local', async (importOriginal) => ({ @@ -38,6 +39,7 @@ vi.mock('../../../services/context.js', async (importOriginal) => ({ // Tests answer the prompts. How a real terminal draws them is checked by hand. vi.mock('@shopify/cli-kit/node/ui', async (importOriginal) => ({ ...(await importOriginal()), + renderConfirmationPrompt: vi.fn(), renderSelectPrompt: vi.fn(), })) // Tests must not replace the clipboard of the machine that runs them. @@ -78,52 +80,55 @@ function expectMentionsPath(message: string, path: string): void { } async function runCommand(argv: string[]) { - let stdout = '' - let stderr = '' const previousExitCode = process.exitCode process.exitCode = 0 - const out = vi.spyOn(process.stdout, 'write').mockImplementation((chunk) => { - stdout += chunk.toString() - return true - }) - const err = vi.spyOn(process.stderr, 'write').mockImplementation((chunk) => { - stderr += chunk.toString() - return true - }) - // Vitest intercepts console.warn; use Node's console to exercise the captured streams. - const warn = vi.spyOn(console, 'warn').mockImplementation(new Console(process.stdout, process.stderr).warn) // Observe the real Oclif error handler's requested exit without terminating the test worker. const exit = vi.spyOn(process, 'exit').mockImplementation((code) => { process.exitCode = code ?? 0 return undefined as never }) try { - const config = await Config.load(import.meta.url) - // This test invokes the app command directly, not as a separately installed CLI plugin. - config.plugins.clear() - await SecurityCheck.run(argv, config) - return {stdout, stderr, exitCode: process.exitCode} + const output = await withCapturedStandardStreams(async ({stdout, stderr}) => { + const config = await Config.load(import.meta.url) + // This test invokes the app command directly, not as a separately installed CLI plugin. + config.plugins.clear() + await SecurityCheck.run(argv, config) + return {stdout: stdout(), stderr: stderr()} + }) + return {...output, exitCode: process.exitCode} } finally { - warn.mockRestore() - out.mockRestore() - err.mockRestore() exit.mockRestore() process.exitCode = previousExitCode } } +/** + * Runs the command with --json. CLI Kit's error handler reads --json from the process arguments, not from the + * arguments given to `run`, so the environment sets it as well. + */ +async function runJsonCommand(argv: string[]) { + vi.stubEnv('SHOPIFY_FLAG_JSON', '1') + try { + return await runCommand([...argv, '--json']) + } finally { + vi.unstubAllEnvs() + } +} + /** * Runs the command as if stdin and stderr were a terminal, where it prompts unless `--no-input` is passed. CLI Kit reads - * `--no-input` from the process arguments, not from the arguments given to `run`, so the environment sets it as well. + * `--no-input` and `--json` from the process arguments, not from the arguments given to `run`, so the environment sets + * them as well. */ -async function runInTerminal(argv: string[], {noInput = false} = {}) { +async function runInTerminal(argv: string[], {noInput = false, json = false} = {}) { vi.stubEnv('CI', '') if (noInput) vi.stubEnv('SHOPIFY_FLAG_NO_INPUT', '1') + if (json) vi.stubEnv('SHOPIFY_FLAG_JSON', '1') const streams = [process.stdin, process.stderr] const descriptors = streams.map((stream) => Object.getOwnPropertyDescriptor(stream, 'isTTY')) for (const stream of streams) Object.defineProperty(stream, 'isTTY', {value: true, configurable: true}) try { - return await runCommand(noInput ? [...argv, '--no-input'] : argv) + return await runCommand([...argv, ...(noInput ? ['--no-input'] : []), ...(json ? ['--json'] : [])]) } finally { streams.forEach((stream, index) => { const descriptor = descriptors[index] @@ -415,3 +420,396 @@ describe('app security check command boundary', () => { }) }) }) + +/** The JSON stderr events of a run, one per line. */ +function jsonEvents(stderr: string): {type: string; level: string; message: string}[] { + return stderr + .split('\n') + .filter((line) => line !== '') + .map((line) => JSON.parse(line)) +} + +/** The result of scanning the app that `createApp` writes, written out independently of the codec. */ +async function scanResult(appDirectory: string) { + const resultsDirectory = resolve(appDirectory, '.shopify', 'app-security', 'shopify.app') + const agentChecks = (await readJson(resolve(resultsDirectory, 'agent-checks.json'))) as {checks: unknown[]} + return { + status: 'success', + valid: true, + app: { + directory: resolve(appDirectory), + configurationPath: resolve(appDirectory, 'shopify.app.toml'), + clientId: 'test-client-id', + }, + resultsDirectory, + deterministicFindingsPath: resolve(resultsDirectory, 'deterministic-findings.json'), + agentChecksPath: resolve(resultsDirectory, 'agent-checks.json'), + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + scanDirectories: [{directory: resolve(appDirectory), origin: 'app-directory'}], + agentCheckCount: agentChecks.checks.length, + blocking: {level: 'none', blockedCheckCount: 0}, + } +} + +describe('app security check --json command boundary', () => { + test('writes one result with the app, the files the scan wrote and its scope, without instructions or a report', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const appDirectory = await fileRealPath(directory) + + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual(await scanResult(appDirectory)) + expect(result.stderr).toBe('') + expect(renderSelectPrompt).not.toHaveBeenCalled() + }) + }) + + test('--skip-instructions writes the same result', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const appDirectory = await fileRealPath(directory) + + const result = await runJsonCommand(['--path', directory, '--skip-instructions']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual(await scanResult(appDirectory)) + expect(result.stderr).toBe('') + }) + }) + + test('--print-instructions includes the instructions that the command prints without --json', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const appDirectory = await fileRealPath(directory) + const printed = await runCommand(['--path', directory, '--print-instructions']) + + const result = await runJsonCommand(['--path', directory, '--print-instructions']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + ...(await scanResult(appDirectory)), + instructions: {content: printed.stdout.replace(/\n$/, ''), copied: false}, + }) + expect(printed.stdout).toContain('### 1. Use the existing scan results') + expect(result.stderr).toBe('') + expect(clipboard.write).not.toHaveBeenCalled() + }) + }) + + test('--list-files writes one result with each gathered file as an absolute native path', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const appDirectory = await fileRealPath(directory) + const listed = await runCommand(['--path', directory, '--list-files']) + + const result = await runJsonCommand(['--path', directory, '--list-files']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + status: 'success', + app: { + directory: resolve(appDirectory), + configurationPath: resolve(appDirectory, 'shopify.app.toml'), + clientId: 'test-client-id', + }, + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + scanDirectories: [{directory: resolve(appDirectory), origin: 'app-directory'}], + files: [ + {path: resolve(appDirectory, '.shopify', '.gitignore')}, + {path: resolve(appDirectory, '.shopify', 'project.json')}, + {path: resolve(appDirectory, 'app', 'routes', 'index.ts')}, + {path: resolve(appDirectory, 'app', 'shopify.server.ts')}, + {path: resolve(appDirectory, 'package.json')}, + {path: resolve(appDirectory, 'shopify.app.toml')}, + ], + }) + // Without --json, the same files are listed relative to the app directory. + expect(listed.stdout).toBe( + '.shopify/.gitignore\n.shopify/project.json\napp/routes/index.ts\napp/shopify.server.ts\npackage.json\nshopify.app.toml\n', + ) + expect(result.stderr).toBe('') + await expect(fileExists(appSecurityArtifactPaths(appDirectory, 'shopify.app').resultsDirectory)).resolves.toBe( + false, + ) + }) + }) + + test('reports a --blocking breach as invalid in the result, and exits 1', async () => { + await inTemporaryDirectory(async (directory) => { + await createAppWithSecret(directory) + + const result = await runJsonCommand(['--path', directory, '--blocking', 'high']) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toMatchObject({ + status: 'success', + valid: false, + blocking: {level: 'high', blockedCheckCount: 1}, + }) + }) + }) + + test('reports the same findings as valid with --blocking none, and exits 0', async () => { + await inTemporaryDirectory(async (directory) => { + await createAppWithSecret(directory) + + const result = await runJsonCommand(['--path', directory, '--blocking', 'none']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({valid: true, blocking: {level: 'none', blockedCheckCount: 0}}) + }) + }) + + test('reports gathering warnings as events on stderr', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + await mkdir(joinPath(directory, 'apps', 'child'), {recursive: true}) + await writeFile(joinPath(directory, 'apps', 'child', 'shopify.app.toml'), 'name = "Child"\n') + vi.stubEnv('INIT_CWD', await fileRealPath(directory)) + + const result = await runJsonCommand(['--path', directory, '--list-files']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({status: 'success', files: expect.any(Array)}) + expect(jsonEvents(result.stderr)).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'warning', + message: expect.stringContaining("apps/child holds another app's configuration"), + }, + ]) + }) + }) + + test('aborts with one error document when there is no app configuration and prompts are off', async () => { + await inTemporaryDirectory(async (directory) => { + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toMatchObject({ + error: {type: 'abort', message: `No app configuration found at or above ${directory}.`}, + }) + }) + }) +}) + +describe('app security check prompts', () => { + /** An app with two configurations and none selected, so the check asks which one to scan. */ + async function createAppWithTwoConfigurations(directory: string): Promise { + await createApp(directory) + await writeFile(joinPath(directory, 'shopify.app.production.toml'), validAppConfiguration('production-client-id')) + await writeFile(joinPath(directory, 'shopify.app.staging.toml'), validAppConfiguration('staging-client-id')) + await rm(joinPath(directory, 'shopify.app.toml')) + } + + describe.each([{json: false}, {json: true}])('with json: $json', ({json}) => { + test.each([ + {answer: 'copy', copied: true}, + {answer: 'print', copied: false}, + ] as const)('delivers the instructions when $answer is chosen', async ({answer, copied}) => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + vi.mocked(renderSelectPrompt).mockResolvedValue(answer) + + const result = await runInTerminal(['--path', directory], {json}) + + expect(result.exitCode).toBe(0) + expect(renderSelectPrompt).toHaveBeenCalledExactlyOnceWith( + expect.objectContaining({ + choices: json + ? [ + {label: 'Include instructions in the JSON output and copy them to the clipboard', value: 'copy'}, + {label: 'Include instructions in the JSON output', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ] + : [ + {label: 'Copy instructions to the clipboard', value: 'copy'}, + {label: 'Print instructions to the terminal', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ], + }), + ) + expect(vi.mocked(clipboard.write).mock.calls).toHaveLength(copied ? 1 : 0) + const instructions = copied ? vi.mocked(clipboard.write).mock.calls[0]![0] : undefined + if (json) { + const output = JSON.parse(result.stdout) + expect(output.instructions).toEqual({content: expect.stringContaining('### 1. Use the existing'), copied}) + if (copied) expect(output.instructions.content).toBe(instructions) + expect(result.stderr).toBe('') + } else if (copied) { + expect(result.stdout).toBe('') + expect(unstyled(result.stderr)).toContain('Copied app security check instructions to the clipboard') + } else { + expect(result.stdout).toContain('### 1. Use the existing scan results') + } + }) + }) + + test('gives nothing when Nothing is chosen', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + vi.mocked(renderSelectPrompt).mockResolvedValue('nothing') + + const result = await runInTerminal(['--path', directory], {json}) + + expect(result.exitCode).toBe(0) + expect(renderSelectPrompt).toHaveBeenCalledOnce() + expect(clipboard.write).not.toHaveBeenCalled() + if (json) { + expect(JSON.parse(result.stdout)).toStrictEqual(await scanResult(await fileRealPath(directory))) + } else { + expect(result.stdout).toBe('') + } + }) + }) + + test('offers no instructions with --no-input', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + + const result = await runInTerminal(['--path', directory], {json, noInput: true}) + + expect(result.exitCode).toBe(0) + expect(renderSelectPrompt).not.toHaveBeenCalled() + if (json) { + expect(JSON.parse(result.stdout)).toStrictEqual(await scanResult(await fileRealPath(directory))) + } else { + expect(result.stdout).toBe('') + } + }) + }) + + test('scans without app configuration when that is accepted, after showing how to skip the prompt', async () => { + await inTemporaryDirectory(async (directory) => { + vi.mocked(renderConfirmationPrompt).mockResolvedValue(true) + vi.mocked(renderSelectPrompt).mockResolvedValue('nothing') + + const result = await runInTerminal(['--path', directory, '--client-id', 'flag-client-id'], {json}) + + expect(result.exitCode).toBe(0) + expect(renderConfirmationPrompt).toHaveBeenCalledOnce() + if (json) { + expect(JSON.parse(result.stdout)).toMatchObject({ + status: 'success', + app: { + directory: resolve(await fileRealPath(directory)), + configurationPath: null, + clientId: 'flag-client-id', + }, + }) + expect(jsonEvents(result.stderr)).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'info', + message: expect.stringMatching( + /^To skip these prompts next time, run:\n\nshopify app security check .* --client-id ["']?flag-client-id["']? --without-app-config$/, + ), + }, + ]) + } else { + expect(result.stdout).toBe('') + expect(boxText(result.stderr)).toContain('To skip these prompts next time, run:') + expect(boxText(result.stderr)).toContain('Config file none') + } + }) + }) + + test('scans nothing when scanning without app configuration is declined', async () => { + await inTemporaryDirectory(async (directory) => { + vi.mocked(renderConfirmationPrompt).mockResolvedValue(false) + + const result = await runInTerminal(['--path', directory], {json}) + + expect(renderConfirmationPrompt).toHaveBeenCalledOnce() + expect(renderSelectPrompt).not.toHaveBeenCalled() + if (json) { + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({status: 'cancelled'}) + expect(result.stderr).toBe('') + } else { + expect(result.exitCode).toBe(1) + expect(result.stdout).toBe('') + expect(boxText(result.stderr)).toContain('No app configuration found at or above') + expect(boxText(result.stderr)).toContain('--without-app-config --client-id ') + } + await expect(fileExists(joinPath(directory, '.shopify', 'app-security'))).resolves.toBe(false) + }) + }) + + test('aborts without asking when there is no app configuration and --no-input is passed', async () => { + await inTemporaryDirectory(async (directory) => { + const result = await runInTerminal(['--path', directory], {json, noInput: true}) + + expect(result.exitCode).toBe(1) + expect(renderConfirmationPrompt).not.toHaveBeenCalled() + if (json) { + expect(JSON.parse(result.stdout)).toMatchObject({ + error: {type: 'abort', message: `No app configuration found at or above ${directory}.`}, + }) + } else { + expect(result.stdout).toBe('') + expect(boxText(result.stderr)).toContain('No app configuration found at or above') + } + }) + }) + }) + + test('--json shows how to skip the prompts as an event after asking which configuration to scan', async () => { + await inTemporaryDirectory(async (directory) => { + await createAppWithTwoConfigurations(directory) + vi.mocked(renderSelectPrompt).mockResolvedValueOnce('shopify.app.staging.toml').mockResolvedValueOnce('nothing') + + const result = await runInTerminal(['--path', directory], {json: true}) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({ + app: {configurationPath: resolve(await fileRealPath(directory), 'shopify.app.staging.toml')}, + }) + expect(jsonEvents(result.stderr)).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'info', + message: expect.stringMatching( + /^To skip these prompts next time, run:\n\nshopify app security check .* --config ["']?staging["']?$/, + ), + }, + ]) + }) + }) + + test.each([ + {blocking: 'none', valid: true, exitCode: 0}, + {blocking: 'high', valid: false, exitCode: 1}, + ])( + '--json warns on stderr when the instructions cannot be copied, still includes them, and exits as --blocking $blocking requires', + async ({blocking, valid, exitCode}) => { + await inTemporaryDirectory(async (directory) => { + await createAppWithSecret(directory) + vi.mocked(renderSelectPrompt).mockResolvedValue('copy') + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + const result = await runInTerminal(['--path', directory, '--blocking', blocking], {json: true}) + + expect(result.exitCode).toBe(exitCode) + expect(JSON.parse(result.stdout)).toMatchObject({ + valid, + instructions: {content: expect.stringContaining('### 1. Use the existing'), copied: false}, + }) + expect(jsonEvents(result.stderr)).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'warning', + message: + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy\n\nThe result still includes them.", + }, + ]) + }) + }, + ) +}) diff --git a/packages/app/src/cli/commands/app/security/check.test.ts b/packages/app/src/cli/commands/app/security/check.test.ts index 77fc3ad535f..d1290ae0d16 100644 --- a/packages/app/src/cli/commands/app/security/check.test.ts +++ b/packages/app/src/cli/commands/app/security/check.test.ts @@ -66,6 +66,7 @@ describe('app security check command', () => { 'config', 'exclude', 'include-dir', + 'json', 'list-files', 'no-git-ignore', 'path', @@ -97,12 +98,25 @@ describe('app security check command', () => { listFiles: false, onSelectionPrompted: renderSecurityCheckPromptsTip, }) - expect(renderSecurityCheckResult).toHaveBeenCalledWith(check, { - directory: resolvePath('./fixtures/unlinked-app'), - verbose: true, - printInstructions: false, - skipInstructions: true, - }) + expect(renderSecurityCheckResult).toHaveBeenCalledWith( + check, + { + directory: resolvePath('./fixtures/unlinked-app'), + verbose: true, + printInstructions: false, + skipInstructions: true, + }, + 'text', + ) + }) + + test('presents the outcome as JSON with --json', async () => { + const check = {kind: 'cancelled'} as const + vi.mocked(securityCheck).mockResolvedValue(check) + + await SecurityCheck.run(['--json'], import.meta.url) + + expect(renderSecurityCheckResult).toHaveBeenCalledWith(check, expect.anything(), 'json') }) test('forwards repeated --exclude globs exactly as typed, in command-line order', async () => { @@ -385,7 +399,7 @@ describe('app security check command', () => { test('describes the artifacts it writes and how agent results are recorded', () => { expect(SecurityCheck.flags['print-instructions'].description).toBe( - 'Print coding-agent instructions without prompting for how to deliver them.', + 'Print coding-agent instructions without prompting for how to deliver them. With --json, the instructions are included in the result.', ) expect(SecurityCheck.flags['skip-instructions'].description).toBe( "Don't offer the coding-agent instructions after the scan. Use it when you want only the deterministic scan results.", diff --git a/packages/app/src/cli/commands/app/security/check.ts b/packages/app/src/cli/commands/app/security/check.ts index 368eeb8d8aa..29b0f92720c 100644 --- a/packages/app/src/cli/commands/app/security/check.ts +++ b/packages/app/src/cli/commands/app/security/check.ts @@ -2,9 +2,10 @@ import {appSecurityBlockingFlag} from './blocking-flag.js' import {appSecuritySelectionFlags} from './selection-flags.js' import securityCheck from '../../../services/security-check.js' import {renderSecurityCheckPromptsTip, renderSecurityCheckResult} from '../../../services/security-check/result.js' +import {appSecurityCheckJsonOutputSchema} from '../../../services/security-check/types.js' import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' -import {globalFlags} from '@shopify/cli-kit/node/cli' +import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli' export default class SecurityCheck extends BaseCommand { static summary = @@ -18,13 +19,19 @@ The check scans the app directory and each \`--include-dir\`. Git ignore rules a Use \`--exclude\` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with \`../\`, and a name at any depth needs \`**/\`, for example \`--exclude '**/generated'\`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand \`*\`. The coding-agent instructions this check offers repeat the globs. Other \`app security\` commands don't take \`--exclude\` or \`--no-git-ignore\`, so pass the same flags each time you run the check. -Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory, and then stops. It writes no results and never prompts. \`--client-id\` is still checked, but doesn't change the list. +Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory, and then stops. It writes no results and never prompts. \`--client-id\` is still checked, but doesn't change the list. With \`--json\`, the result lists the files with absolute paths. -In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--print-instructions\`, which prints them. You can also run \`shopify app security instructions\` to print, copy, or write them later. +In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--print-instructions\`, which prints them. You can also run \`shopify app security instructions\` to print, copy, or write them later. With \`--json\`, the command offers to include the instructions in the result, with or without copying them, and \`--print-instructions\` includes them. + +With \`--json\`, the result describes the scan and the files it wrote, and whether \`--blocking\` passed, but not the findings: run \`shopify app security review --json\` for those. Reports the number of findings to Shopify as usage telemetry, but not finding details. See [usage reporting](https://shopify.dev/docs/api/shopify-cli#usage-reporting) for details.` - static description = this.descriptionWithoutMarkdown() + static get jsonOutputSchema() { + return appSecurityCheckJsonOutputSchema + } + + static description = this.descriptionForHelp() static examples = [ '<%= config.bin %> <%= command.id %>', @@ -60,13 +67,14 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de }), 'list-files': Flags.boolean({ description: - 'Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for.', + 'Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for. With --json, the result lists them with absolute paths.', env: 'SHOPIFY_FLAG_LIST_FILES', exclusive: ['print-instructions', 'skip-instructions', 'blocking'], }), ...appSecurityBlockingFlag, 'print-instructions': Flags.boolean({ - description: 'Print coding-agent instructions without prompting for how to deliver them.', + description: + 'Print coding-agent instructions without prompting for how to deliver them. With --json, the instructions are included in the result.', default: false, exclusive: ['skip-instructions'], env: 'SHOPIFY_FLAG_APP_SECURITY_PRINT_INSTRUCTIONS', @@ -85,6 +93,7 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de exclusive: ['print-instructions'], env: 'SHOPIFY_FLAG_APP_SECURITY_SKIP_INSTRUCTIONS', }), + ...jsonFlag, } public async run(): Promise { @@ -102,11 +111,15 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de listFiles: Boolean(flags['list-files']), onSelectionPrompted: renderSecurityCheckPromptsTip, }) - await renderSecurityCheckResult(check, { - directory: flags.path, - verbose: Boolean(flags.verbose), - printInstructions: Boolean(flags['print-instructions'] || flags.yes), - skipInstructions: flags['skip-instructions'], - }) + await renderSecurityCheckResult( + check, + { + directory: flags.path, + verbose: Boolean(flags.verbose), + printInstructions: Boolean(flags['print-instructions'] || flags.yes), + skipInstructions: flags['skip-instructions'], + }, + flags.json ? 'json' : 'text', + ) } } diff --git a/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts b/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts index bb4000c7067..a891ba5e997 100644 --- a/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts +++ b/packages/app/src/cli/services/app-security-engine/tests/layout-catalogue.test.ts @@ -115,12 +115,11 @@ async function checkListFiles(temporaryDirectory: string, workingDirectory: stri listFiles: true, blocking: 'none', }) - await renderSecurityCheckResult(resolution, { - directory, - verbose: false, - printInstructions: false, - skipInstructions: false, - }) + await renderSecurityCheckResult( + resolution, + {directory, verbose: false, printInstructions: false, skipInstructions: false}, + 'text', + ) if (resolution.kind !== 'listed') throw new Error(`Expected listed files, got ${resolution.kind}`) return {resolution, stdout: stdout(), stderr: stderr()} }) diff --git a/packages/app/src/cli/services/security-check/result.test.ts b/packages/app/src/cli/services/security-check/result.test.ts index 7459917027c..7636277694b 100644 --- a/packages/app/src/cli/services/security-check/result.test.ts +++ b/packages/app/src/cli/services/security-check/result.test.ts @@ -16,6 +16,7 @@ import { import {appSecurityInstructions} from '../app-security-instructions.js' import {validAppConfiguration} from '../app-security-selection.test-data.js' import {committedSecretIssue, scan, scanExecution} from '../security-check.test-data.js' +import {runWithCommandEventsForCommand} from '@shopify/cli-kit/node/command-events' import {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, inTemporaryDirectory, matchGlob, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import {unstyled} from '@shopify/cli-kit/node/output' @@ -52,15 +53,16 @@ const selection: AppSecuritySelection = { const noScope: AppSecurityScope = {include_dirs: [], excludes: [], no_git_ignore: false} function scanned(overrides: Partial = {}): ScannedSecurityCheck { + const checkSelection = overrides.selection ?? selection const scope = overrides.scope ?? noScope - const paths = appSecurityArtifactPaths(appDirectory, 'shopify.app') + const paths = appSecurityArtifactPaths(checkSelection.appDirectory, 'shopify.app') return { kind: 'scanned', - selection, + selection: checkSelection, resultsKey: 'shopify.app', - commands: resolveAppSecurityCommands(selection, appDirectory, scope), + commands: resolveAppSecurityCommands(checkSelection, checkSelection.appDirectory, scope), scope, - scanDirectories: [{directory: appDirectory, origin: 'app_directory'}], + scanDirectories: [{directory: checkSelection.appDirectory, origin: 'app_directory'}], execution: scanExecution, artifacts: {deterministicFindingsPath: paths.deterministicFindingsPath, agentChecksPath: paths.agentChecksPath}, blocking: {level: 'none', blockedChecks: 0}, @@ -69,13 +71,14 @@ function scanned(overrides: Partial = {}): ScannedSecurity } function listed(overrides: Partial = {}): ListedSecurityCheck { + const checkSelection = overrides.selection ?? selection return { kind: 'listed', - selection, + selection: checkSelection, resultsKey: 'shopify.app', - commands: resolveAppSecurityCommands(selection, appDirectory, noScope), + commands: resolveAppSecurityCommands(checkSelection, checkSelection.appDirectory, noScope), scope: noScope, - scanDirectories: [{directory: appDirectory, origin: 'app_directory'}], + scanDirectories: [{directory: checkSelection.appDirectory, origin: 'app_directory'}], paths: ['shopify.app.toml'], ignoredScanDirectories: [], otherAppDirectories: [], @@ -114,11 +117,32 @@ async function present( dependencies = testDependencies(), ) { return withCapturedStandardStreams(async ({stdout, stderr}) => { - await renderSecurityCheckResult(check, {...presentationOptions, ...options}, dependencies) + await renderSecurityCheckResult(check, {...presentationOptions, ...options}, 'text', dependencies) return {stdout: unstyled(stdout()), stderr: unstyled(stderr())} }) } +/** + * Presents the outcome as JSON, as a command with `--json` does, and returns the parsed result and the stderr events. + * Parsing fails unless stdout holds exactly one JSON document. + */ +async function presentJson( + check: SecurityCheckOutcome, + options: Partial = {}, + dependencies = testDependencies(), +) { + return withCapturedStandardStreams(async ({stdout, stderr}) => { + await runWithCommandEventsForCommand(['--json'], () => + renderSecurityCheckResult(check, {...presentationOptions, ...options}, 'json', dependencies), + ) + const events = stderr() + .split('\n') + .filter((line) => line !== '') + .map((line) => JSON.parse(line) as {type: string; level: string; message: string}) + return {result: JSON.parse(stdout()) as unknown, events} + }) +} + // Boxes wrap text across lines and draw borders, so compare their text with the borders and line breaks removed. function boxText(output: string): string { return output.replaceAll(/[│╭╮╰╯─]/g, ' ').replace(/\s+/g, ' ') @@ -311,11 +335,11 @@ describe('renderSecurityCheckResult for a scan', () => { stderrWhenPrompted = unstyled(stderr()) return 'nothing' }) - await renderSecurityCheckResult(scanned(), presentationOptions, dependencies) + await renderSecurityCheckResult(scanned(), presentationOptions, 'text', dependencies) return {stdout: stdout()} }) - expect(appSecurityInstructionsPrompt(31)).toEqual({ + expect(appSecurityInstructionsPrompt(31, 'text')).toEqual({ message: '31 recommended agent checks available to complete your scan. How do you want to pass that prompt to your agent?', choices: [ @@ -326,7 +350,7 @@ describe('renderSecurityCheckResult for a scan', () => { defaultValue: 'copy', }) expect(dependencies.selectInstructionsDestination).toHaveBeenCalledExactlyOnceWith( - appSecurityInstructionsPrompt(31), + appSecurityInstructionsPrompt(31, 'text'), ) expect(boxText(stderrWhenPrompted)).toContain('No security issues found.') expect(stdout).toBe('') @@ -334,7 +358,7 @@ describe('renderSecurityCheckResult for a scan', () => { }) test('names a single recommended agent check in the singular', () => { - expect(appSecurityInstructionsPrompt(1).message).toBe( + expect(appSecurityInstructionsPrompt(1, 'text').message).toBe( '1 recommended agent check available to complete your scan. How do you want to pass that prompt to your agent?', ) }) @@ -480,3 +504,291 @@ describe('renderSecurityCheckResult when scanning without app configuration was ) }) }) + +// Selections hold the app directory's real path and paths joined from it with forward slashes, even on Windows. +const onWindows = process.platform === 'win32' +const nativeAppDirectory = onWindows ? 'C:\\apps\\my-app' : '/apps/my-app' +const nativeSelection: AppSecuritySelection = { + kind: 'config', + appDirectory: nativeAppDirectory, + appConfigFilePath: `${nativeAppDirectory}/shopify.app.toml`, + configClientId: 'toml-client-id', +} + +/** The absolute native path of a file below `/apps`, written out independently of the codec. */ +function nativePath(relativePath: string): string { + return onWindows ? `C:\\apps\\${relativePath.replaceAll('/', '\\')}` : `/apps/${relativePath}` +} + +const nativeResultsDirectory = nativePath('my-app/.shopify/app-security/shopify.app') + +/** The result of a scan of `nativeSelection` without findings or instructions. */ +const scanResult = { + status: 'success', + valid: true, + app: { + directory: nativeAppDirectory, + configurationPath: nativePath('my-app/shopify.app.toml'), + clientId: 'toml-client-id', + }, + resultsDirectory: nativeResultsDirectory, + deterministicFindingsPath: nativePath('my-app/.shopify/app-security/shopify.app/deterministic-findings.json'), + agentChecksPath: nativePath('my-app/.shopify/app-security/shopify.app/agent-checks.json'), + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + scanDirectories: [{directory: nativeAppDirectory, origin: 'app-directory'}], + agentCheckCount: 31, + blocking: {level: 'none', blockedCheckCount: 0}, +} + +describe('renderSecurityCheckResult --json for a scan', () => { + test('prints one result with the app, the files the scan wrote and its scope, and no report', async () => { + process.exitCode = undefined + + const {result, events} = await presentJson(scanned({selection: nativeSelection}), {verbose: true}) + + expect(result).toStrictEqual(scanResult) + expect(events).toEqual([]) + expect(process.exitCode).toBeUndefined() + }) + + test('shows the scope as typed and each scan directory with its absolute path and origin', async () => { + const scope = {include_dirs: ['../backend'], excludes: ['**/generated'], no_git_ignore: true} + const check = scanned({ + selection: nativeSelection, + scope, + scanDirectories: [ + {directory: nativeAppDirectory, origin: 'app_directory'}, + {directory: onWindows ? 'C:\\apps\\backend' : '/apps/backend', origin: 'include_dir'}, + {directory: onWindows ? 'C:\\apps\\shared\\theme' : '/apps/shared/theme', origin: 'app_config_directory'}, + ], + }) + + const {result} = await presentJson(check) + + expect(result).toMatchObject({ + scope: {includeDirs: ['../backend'], excludes: ['**/generated'], noGitIgnore: true}, + scanDirectories: [ + {directory: nativeAppDirectory, origin: 'app-directory'}, + {directory: nativePath('backend'), origin: 'include-dir'}, + {directory: nativePath('shared/theme'), origin: 'app-config-directory'}, + ], + }) + }) + + test('shows a scan without app configuration with a null configuration path', async () => { + const noConfig: AppSecuritySelection = { + kind: 'no-config', + appDirectory: nativeAppDirectory, + clientId: 'flag-client-id', + clientIdSource: 'flag', + } + + const {result} = await presentJson({...scanned({selection: noConfig}), resultsKey: 'flag-client-id'}) + + expect(result).toMatchObject({ + app: {directory: nativeAppDirectory, configurationPath: null, clientId: 'flag-client-id'}, + resultsDirectory: nativePath('my-app/.shopify/app-security/flag-client-id'), + }) + }) + + test('reports a --blocking breach as invalid, and sets exit code 1 after the result', async () => { + process.exitCode = undefined + const execution = {...scanExecution, scan: {...scan, issues: [committedSecretIssue]}} + + const {result} = await presentJson( + scanned({selection: nativeSelection, execution, blocking: {level: 'high', blockedChecks: 1}}), + ) + + expect(result).toMatchObject({valid: false, blocking: {level: 'high', blockedCheckCount: 1}}) + expect(process.exitCode).toBe(1) + }) + + test('reports findings below --blocking, or --blocking none, as valid', async () => { + process.exitCode = undefined + const execution = {...scanExecution, scan: {...scan, issues: [committedSecretIssue]}} + + const {result} = await presentJson(scanned({execution, blocking: {level: 'none', blockedChecks: 0}})) + + expect(result).toMatchObject({valid: true, blocking: {level: 'none', blockedCheckCount: 0}}) + expect(process.exitCode).toBeUndefined() + }) + + test('--print-instructions includes the instructions in the result, without prompting or copying', async () => { + const check = scanned({selection: nativeSelection}) + const dependencies = testDependencies('copy', true) + + const {result, events} = await presentJson(check, {printInstructions: true}, dependencies) + + expect(result).toStrictEqual({...scanResult, instructions: {content: scanInstructions(check), copied: false}}) + expect(events).toEqual([]) + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + expect(clipboard.write).not.toHaveBeenCalled() + }) + + test('offers to include the instructions in the result, and to copy them as well by default', async () => { + const dependencies = testDependencies('nothing', true) + + await presentJson(scanned(), {}, dependencies) + + expect(appSecurityInstructionsPrompt(31, 'json')).toEqual({ + message: + '31 recommended agent checks available to complete your scan. How do you want to pass that prompt to your agent?', + choices: [ + {label: 'Include instructions in the JSON output and copy them to the clipboard', value: 'copy'}, + {label: 'Include instructions in the JSON output', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ], + defaultValue: 'copy', + }) + expect(dependencies.selectInstructionsDestination).toHaveBeenCalledExactlyOnceWith( + appSecurityInstructionsPrompt(31, 'json'), + ) + }) + + test.each([ + {destination: 'copy', copied: true}, + {destination: 'print', copied: false}, + ] as const)( + 'includes the instructions when $destination is chosen, with copied $copied', + async ({destination, copied}) => { + const check = scanned({selection: nativeSelection}) + + const {result, events} = await presentJson(check, {}, testDependencies(destination, true)) + + expect(result).toStrictEqual({...scanResult, instructions: {content: scanInstructions(check), copied}}) + expect(events).toEqual([]) + expect(vi.mocked(clipboard.write).mock.calls).toEqual(copied ? [[scanInstructions(check)]] : []) + }, + ) + + test.each([ + ['Nothing is chosen', {}, testDependencies('nothing', true)], + ['--skip-instructions is passed', {skipInstructions: true}, testDependencies('copy', true)], + ['the terminal cannot prompt', {}, testDependencies('copy', false)], + ])('leaves the instructions out when %s', async (_description, options, dependencies) => { + const {result} = await presentJson(scanned({selection: nativeSelection}), options, dependencies) + + expect(result).toStrictEqual(scanResult) + expect(clipboard.write).not.toHaveBeenCalled() + }) + + test.each([ + {level: 'none', blockedChecks: 0, valid: true, exitCode: undefined}, + {level: 'high', blockedChecks: 1, valid: false, exitCode: 1}, + ] as const)( + 'warns on stderr when the instructions cannot be copied, still includes them, and exits as --blocking $level requires', + async ({level, blockedChecks, valid, exitCode}) => { + process.exitCode = undefined + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + const execution = {...scanExecution, scan: {...scan, issues: [committedSecretIssue]}} + const check = scanned({execution, blocking: {level, blockedChecks}}) + + const {result, events} = await presentJson(check, {}, testDependencies('copy', true)) + + expect(result).toMatchObject({valid, instructions: {content: scanInstructions(check), copied: false}}) + expect(events).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'warning', + message: + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy\n\nThe result still includes them.", + }, + ]) + expect(process.exitCode).toBe(exitCode) + }, + ) + + test('reports gathering warnings as events on stderr', async () => { + await inTemporaryDirectory(async (directory) => { + const realDirectory = await fileRealPath(directory) + vi.stubEnv('INIT_CWD', directory) + const execution = { + ...scanExecution, + ignoredScanDirectories: [joinPath(realDirectory, 'vendor')], + otherAppDirectories: [joinPath(realDirectory, 'apps', 'child')], + } + + const {events} = await presentJson(scanned({execution})) + + expect(events).toEqual([ + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'warning', + message: + 'vendor is ignored by Git, so only the files Git tracks in it are scanned.\n\nUse --no-git-ignore to scan everything in it.', + }, + { + type: 'diagnostic', + timestamp: expect.any(String), + level: 'warning', + message: `apps/child holds another app's configuration, so its files are scanned as part of this app.\n\nUse --exclude ${quoteShellArgument('apps/child', shellForPlatform())} to leave it out.`, + }, + ]) + }) + }) +}) + +describe('renderSecurityCheckPromptsTip --json', () => { + test('shows the command that repeats the run without prompts as an event on stderr', async () => { + const commands = resolveAppSecurityCommands(selection, appDirectory, noScope) + + const {stdout, stderr} = await withCapturedStandardStreams(async ({stdout, stderr}) => { + await runWithCommandEventsForCommand(['--json'], async () => renderSecurityCheckPromptsTip(commands)) + return {stdout: stdout(), stderr: stderr()} + }) + + expect(stdout).toBe('') + expect(JSON.parse(stderr)).toEqual({ + type: 'diagnostic', + timestamp: expect.any(String), + level: 'info', + message: `To skip these prompts next time, run:\n\n${formatAppSecurityCommand(commands.scan)}`, + }) + }) +}) + +describe('renderSecurityCheckResult --json for --list-files', () => { + test('prints one result with each gathered file as an absolute native path', async () => { + const check = listed({ + selection: nativeSelection, + paths: ['../backend/server.ts', 'app/routes/index.ts', 'shopify.app.toml'], + }) + const dependencies = testDependencies('copy', true) + + const {result, events} = await presentJson(check, {}, dependencies) + + expect(result).toStrictEqual({ + status: 'success', + app: scanResult.app, + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + scanDirectories: [{directory: nativeAppDirectory, origin: 'app-directory'}], + files: [ + {path: nativePath('backend/server.ts')}, + {path: nativePath('my-app/app/routes/index.ts')}, + {path: nativePath('my-app/shopify.app.toml')}, + ], + }) + expect(events).toEqual([]) + expect(dependencies.selectInstructionsDestination).not.toHaveBeenCalled() + }) + + test('prints a result with no files when none is gathered', async () => { + const {result} = await presentJson(listed({paths: []})) + + expect(result).toMatchObject({files: []}) + }) +}) + +describe('renderSecurityCheckResult --json when scanning without app configuration was declined', () => { + test('prints a cancelled result and leaves the exit code alone', async () => { + process.exitCode = undefined + + const {result, events} = await presentJson({kind: 'cancelled'}) + + expect(result).toStrictEqual({status: 'cancelled'}) + expect(events).toEqual([]) + expect(process.exitCode).toBeUndefined() + }) +}) diff --git a/packages/app/src/cli/services/security-check/result.ts b/packages/app/src/cli/services/security-check/result.ts index 1b783dff0a9..16f71d3c018 100644 --- a/packages/app/src/cli/services/security-check/result.ts +++ b/packages/app/src/cli/services/security-check/result.ts @@ -1,9 +1,20 @@ +import {appSecurityCheckJsonOutputSchema, type AppSecurityCheckResult} from './types.js' +import {appSecurityArtifactPaths} from '../app-security-artifacts.js' import { appSecurityInstructions, ClipboardUnavailableError, deliverAppSecurityInstructions, renderAppSecurityInstructions, } from '../app-security-instructions.js' +import { + projectAppSecurityApp, + projectAppSecurityBlocking, + projectAppSecurityInstructions, + projectAppSecurityPath, + projectAppSecurityRelativePath, + projectAppSecurityScanDirectory, + projectAppSecurityScope, +} from '../app-security-json-output.js' import { formatAppSecurityCommand, quoteShellArgument, @@ -24,6 +35,8 @@ import type {SecurityCheckOutcome} from '../security-check.js' export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' type ScannedSecurityCheck = Extract +type ListedSecurityCheck = Extract +type OutputFormat = 'json' | 'text' interface SecurityCheckPresentationOptions { /** The `--path` value, an absolute path. */ @@ -45,16 +58,25 @@ const defaultDependencies: SecurityCheckPresenterDependencies = { selectInstructionsDestination: (prompt) => renderSelectPrompt(prompt), } +/** With `--json`, "print" means including the instructions in the result. */ export function appSecurityInstructionsPrompt( agentCheckCount: number, + format: OutputFormat, ): RenderSelectPromptOptions { return { message: `${agentCheckCount} recommended agent ${agentCheckCount === 1 ? 'check' : 'checks'} available to complete your scan. How do you want to pass that prompt to your agent?`, - choices: [ - {label: 'Copy instructions to the clipboard', value: 'copy'}, - {label: 'Print instructions to the terminal', value: 'print'}, - {label: 'Nothing', value: 'nothing'}, - ], + choices: + format === 'json' + ? [ + {label: 'Include instructions in the JSON output and copy them to the clipboard', value: 'copy'}, + {label: 'Include instructions in the JSON output', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ] + : [ + {label: 'Copy instructions to the clipboard', value: 'copy'}, + {label: 'Print instructions to the terminal', value: 'print'}, + {label: 'Nothing', value: 'nothing'}, + ], defaultValue: 'copy', } } @@ -68,49 +90,85 @@ export function renderSecurityCheckPromptsTip(commands: AppSecurityCommands): vo } /** - * Presents the outcome of `securityCheck`: the gathering warnings, then the listed files, or the report followed by - * the coding-agent instructions. Sets exit code 1 after the output when the scan breaches `--blocking`. Declining to - * scan without app configuration aborts, as when there is none and prompts are off. + * Presents the outcome of `securityCheck` as JSON, or in the terminal. Gathering warnings come first, on stderr. Then + * the listed files, or the scan: in the terminal, its report followed by the coding-agent instructions; as JSON, the + * instructions prompt followed by the result. Either way, sets exit code 1 after the output when the scan breaches + * `--blocking`. Declining to scan without app configuration is a cancelled result as JSON. In the terminal it aborts, + * as when there is no app configuration and prompts are off. */ export async function renderSecurityCheckResult( check: SecurityCheckOutcome, options: SecurityCheckPresentationOptions, + format: OutputFormat, dependencies: SecurityCheckPresenterDependencies = defaultDependencies, ): Promise { - if (check.kind === 'cancelled') throw new MissingAppConfigurationError(options.directory) + if (check.kind === 'cancelled') { + if (format === 'text') throw new MissingAppConfigurationError(options.directory) + outputResult(appSecurityCheckJsonOutputSchema.encode({status: 'cancelled'})) + return + } if (check.kind === 'listed') { await renderGatheringWarnings(check) - if (check.paths.length > 0) outputResult(check.paths.join('\n')) + if (format === 'json') { + outputResult(appSecurityCheckJsonOutputSchema.encode(appSecurityCheckFileListResult(check))) + } else if (check.paths.length > 0) { + outputResult(check.paths.join('\n')) + } return } await renderGatheringWarnings(check.execution) + if (format === 'json') { + await renderScanAsJson(check, options, dependencies) + } else { + await renderScanInTerminal(check, options, dependencies) + } + + if (check.blocking.blockedChecks > 0) process.exitCode = 1 +} + +async function renderScanInTerminal( + check: ScannedSecurityCheck, + options: SecurityCheckPresentationOptions, + dependencies: SecurityCheckPresenterDependencies, +): Promise { renderSecurityReport(securityReportInput(check, options.verbose)) - const destination = await instructionsDestination(options, check.execution.agentChecks.checks.length, dependencies) - if (destination !== 'nothing') { - const instructions = appSecurityInstructions({ - appDirectory: check.selection.appDirectory, - resultsKey: check.resultsKey, - commands: check.commands, - scanScope: check.scope, - }) - if (destination === 'print') { - renderAppSecurityInstructions(instructions, {copy: false}) - } else if (await copyInstructions(instructions)) { - renderAppSecurityInstructions(instructions, {copy: true}) - } + const destination = await instructionsDestination(check, options, 'text', dependencies) + if (destination === 'nothing') return + const instructions = scanInstructions(check) + if (destination === 'print') { + renderAppSecurityInstructions(instructions, {copy: false}) + } else if (await copyInstructions(instructions, 'text')) { + renderAppSecurityInstructions(instructions, {copy: true}) } +} - if (check.blocking.blockedChecks > 0) process.exitCode = 1 +/** The result holds the instructions, so the prompt comes before it. It holds them even when they couldn't be copied. */ +async function renderScanAsJson( + check: ScannedSecurityCheck, + options: SecurityCheckPresentationOptions, + dependencies: SecurityCheckPresenterDependencies, +): Promise { + const destination = await instructionsDestination(check, options, 'json', dependencies) + const instructions = destination === 'nothing' ? undefined : scanInstructions(check) + const copied = instructions !== undefined && destination === 'copy' && (await copyInstructions(instructions, 'json')) + outputResult( + appSecurityCheckJsonOutputSchema.encode( + appSecurityCheckScanResult( + check, + instructions === undefined ? undefined : projectAppSecurityInstructions(instructions, {copy: copied}), + ), + ), + ) } /** - * Copies the instructions to the clipboard, and returns whether they're on it. The scan has already finished and its - * report is shown, so a clipboard that isn't available only warns. + * Copies the instructions to the clipboard, and returns whether they're on it. The scan has already finished, so a + * clipboard that isn't available only warns. */ -async function copyInstructions(instructions: string): Promise { +async function copyInstructions(instructions: string, format: OutputFormat): Promise { try { await deliverAppSecurityInstructions(instructions, {copy: true}) return true @@ -118,21 +176,70 @@ async function copyInstructions(instructions: string): Promise { if (!(error instanceof ClipboardUnavailableError)) throw error renderWarning({ headline: error.message, - body: ['Run the check again with', {command: '--print-instructions'}, 'to print them.'], + body: + format === 'json' + ? 'The result still includes them.' + : ['Run the check again with', {command: '--print-instructions'}, 'to print them.'], }) return false } } async function instructionsDestination( + check: ScannedSecurityCheck, options: SecurityCheckPresentationOptions, - agentCheckCount: number, + format: OutputFormat, dependencies: SecurityCheckPresenterDependencies, ): Promise { if (options.skipInstructions) return 'nothing' if (options.printInstructions) return 'print' if (!dependencies.canPrompt()) return 'nothing' - return dependencies.selectInstructionsDestination(appSecurityInstructionsPrompt(agentCheckCount)) + return dependencies.selectInstructionsDestination( + appSecurityInstructionsPrompt(check.execution.agentChecks.checks.length, format), + ) +} + +/** The instructions for the scan that just finished, which start from its results. */ +function scanInstructions(check: ScannedSecurityCheck): string { + return appSecurityInstructions({ + appDirectory: check.selection.appDirectory, + resultsKey: check.resultsKey, + commands: check.commands, + scanScope: check.scope, + }) +} + +function appSecurityCheckScanResult( + check: ScannedSecurityCheck, + instructions: ReturnType | undefined, +): AppSecurityCheckResult { + const {appDirectory} = check.selection + const {resultsDirectory} = appSecurityArtifactPaths(appDirectory, check.resultsKey) + return { + status: 'success', + valid: check.blocking.blockedChecks === 0, + app: projectAppSecurityApp(check.selection), + resultsDirectory: projectAppSecurityPath(resultsDirectory), + deterministicFindingsPath: projectAppSecurityPath(check.artifacts.deterministicFindingsPath), + agentChecksPath: projectAppSecurityPath(check.artifacts.agentChecksPath), + scope: projectAppSecurityScope(check.scope), + scanDirectories: check.scanDirectories.map((directory) => projectAppSecurityScanDirectory(appDirectory, directory)), + agentCheckCount: check.execution.agentChecks.checks.length, + blocking: projectAppSecurityBlocking(check.blocking), + ...(instructions === undefined ? {} : {instructions}), + } +} + +/** Listed paths are relative to the app directory. */ +function appSecurityCheckFileListResult(check: ListedSecurityCheck): AppSecurityCheckResult { + const {appDirectory} = check.selection + return { + status: 'success', + app: projectAppSecurityApp(check.selection), + scope: projectAppSecurityScope(check.scope), + scanDirectories: check.scanDirectories.map((directory) => projectAppSecurityScanDirectory(appDirectory, directory)), + files: check.paths.map((path) => ({path: projectAppSecurityRelativePath(appDirectory, path)})), + } } function securityReportInput(check: ScannedSecurityCheck, verbose: boolean): SecurityReportInput { diff --git a/packages/app/src/cli/services/security-check/types.test.ts b/packages/app/src/cli/services/security-check/types.test.ts new file mode 100644 index 00000000000..fe292dcc740 --- /dev/null +++ b/packages/app/src/cli/services/security-check/types.test.ts @@ -0,0 +1,78 @@ +import {appSecurityCheckJsonOutputSchema} from './types.js' +import {describe, expect, test} from 'vitest' + +const onWindows = process.platform === 'win32' +const appDirectory = onWindows ? 'C:\\apps\\my-app' : '/apps/my-app' +const resultsDirectory = onWindows + ? 'C:\\apps\\my-app\\.shopify\\app-security\\shopify.app' + : '/apps/my-app/.shopify/app-security/shopify.app' +const app = { + directory: appDirectory, + configurationPath: onWindows ? 'C:\\apps\\my-app\\shopify.app.toml' : '/apps/my-app/shopify.app.toml', + clientId: 'toml-client-id', +} +const scope = {includeDirs: ['../backend'], excludes: ['**/generated'], noGitIgnore: false} +const scanDirectories = [ + {directory: appDirectory, origin: 'app-directory'}, + {directory: onWindows ? 'C:\\apps\\backend' : '/apps/backend', origin: 'include-dir'}, +] +const scan = { + status: 'success', + valid: true, + app, + resultsDirectory, + deterministicFindingsPath: onWindows + ? `${resultsDirectory}\\deterministic-findings.json` + : `${resultsDirectory}/deterministic-findings.json`, + agentChecksPath: onWindows ? `${resultsDirectory}\\agent-checks.json` : `${resultsDirectory}/agent-checks.json`, + scope, + scanDirectories, + agentCheckCount: 31, + blocking: {level: 'none', blockedCheckCount: 0}, +} +const fileList = { + status: 'success', + app, + scope, + scanDirectories, + files: [{path: onWindows ? 'C:\\apps\\backend\\server.ts' : '/apps/backend/server.ts'}], +} + +describe('appSecurityCheckJsonOutputSchema', () => { + test.each([ + ['a scan without instructions', scan], + ['a scan with instructions', {...scan, instructions: {content: '# Instructions', copied: true}}], + ['a scan that fails --blocking', {...scan, valid: false, blocking: {level: 'high', blockedCheckCount: 2}}], + [ + 'a scan without app configuration', + {...scan, app: {directory: appDirectory, configurationPath: null, clientId: 'a'}}, + ], + ['the listed files', fileList], + ['no listed files', {...fileList, files: []}], + ['a cancellation', {status: 'cancelled'}], + ])('accepts %s', (_description, value) => { + expect(appSecurityCheckJsonOutputSchema.validate(value)).toStrictEqual(value) + }) + + test.each([ + ['an extra field', {...scan, findings: []}], + ['a missing field', {...scan, agentChecksPath: undefined}], + ['null instructions', {...scan, instructions: null}], + ['empty instructions', {...scan, instructions: {content: '', copied: false}}], + ['a relative findings path', {...scan, deterministicFindingsPath: 'deterministic-findings.json'}], + ['a negative agent check count', {...scan, agentCheckCount: -1}], + ['a fractional blocked check count', {...scan, blocking: {level: 'high', blockedCheckCount: 0.5}}], + [ + 'a stored scan directory origin', + {...scan, scanDirectories: [{directory: appDirectory, origin: 'app_directory'}]}, + ], + ['a stored scope', {...scan, scope: {include_dirs: [], excludes: [], no_git_ignore: false}}], + ['listed files with the scan fields', {...fileList, valid: true}], + ['a relative listed file', {...fileList, files: [{path: 'app/routes/index.ts'}]}], + ['a listed file as a string', {...fileList, files: ['/apps/my-app/shopify.app.toml']}], + ['a cancellation with a reason', {status: 'cancelled', reason: 'declined'}], + ['another status', {...scan, status: 'skipped'}], + ])('rejects %s', (_description, value) => { + expect(() => appSecurityCheckJsonOutputSchema.validate(value)).toThrow() + }) +}) diff --git a/packages/app/src/cli/services/security-check/types.ts b/packages/app/src/cli/services/security-check/types.ts new file mode 100644 index 00000000000..9fb0ad3fe4d --- /dev/null +++ b/packages/app/src/cli/services/security-check/types.ts @@ -0,0 +1,84 @@ +import { + appSecurityAppSchema, + appSecurityBlockingSchema, + appSecurityInstructionsSchema, + appSecurityScanDirectorySchema, + appSecurityScopeSchema, + createAbsolutePathSchema, +} from '../app-security-json-output.js' +import {defineJsonOutputSchema, type InferJsonOutputSchema} from '@shopify/cli-kit/node/json-output-schema' +import {zod} from '@shopify/cli-kit/node/schema' + +/** With --list-files, the result is the files that a scan would gather. */ +const appSecurityCheckFileListSchema = zod + .object({ + status: zod.literal('success'), + app: appSecurityAppSchema, + scope: appSecurityScopeSchema, + scanDirectories: zod.array(appSecurityScanDirectorySchema).describe('The directories that a scan would walk.'), + files: zod + .array(zod.object({path: createAbsolutePathSchema().describe('The absolute path of the file.')}).strict()) + .describe( + 'The files that a scan would gather, ordered by their paths relative to the app directory, as listed without --json.', + ), + }) + .strict() + .describe('With --list-files, the files that a scan would gather. Nothing is scanned or written.') + +export const appSecurityCheckJsonOutputSchema = defineJsonOutputSchema({ + name: 'AppSecurityCheckResult', + schema: zod + .union([ + zod + .object({ + status: zod.literal('success'), + valid: zod + .boolean() + .describe( + 'Whether --blocking passed: no finding of the scan has its own severity at or above the level. blocking.blockedCheckCount counts the checks with such findings, and --blocking none always passes. Run review --json for the findings.', + ), + app: appSecurityAppSchema, + resultsDirectory: createAbsolutePathSchema().describe( + 'The absolute path of the results directory that holds the files the scan wrote.', + ), + deterministicFindingsPath: createAbsolutePathSchema().describe( + 'The absolute path of deterministic-findings.json, which the scan replaced.', + ), + agentChecksPath: createAbsolutePathSchema().describe( + 'The absolute path of agent-checks.json, which the scan replaced.', + ), + scope: appSecurityScopeSchema, + scanDirectories: zod.array(appSecurityScanDirectorySchema).describe('The directories that the scan walked.'), + agentCheckCount: zod + .number() + .int() + .nonnegative() + .describe('The number of checks in agent-checks.json for a coding agent to investigate.'), + blocking: appSecurityBlockingSchema, + instructions: appSecurityInstructionsSchema + .optional() + .describe( + 'The coding-agent instructions for this scan, with --print-instructions or when chosen at the prompt. Left out otherwise.', + ), + }) + .strict(), + appSecurityCheckFileListSchema, + zod + .object({status: zod.literal('cancelled')}) + .strict() + .describe( + 'Asked whether to scan without app configuration, the user declined. Nothing was scanned or written.', + ), + ]) + .describe('The scan and the files it wrote. With --list-files, the files that a scan would gather.'), + definitions: { + AppSecurityApp: appSecurityAppSchema, + AppSecurityScope: appSecurityScopeSchema, + AppSecurityScanDirectory: appSecurityScanDirectorySchema, + AppSecurityBlocking: appSecurityBlockingSchema, + AppSecurityInstructions: appSecurityInstructionsSchema, + AppSecurityCheckFileList: appSecurityCheckFileListSchema, + }, +}) + +export type AppSecurityCheckResult = InferJsonOutputSchema diff --git a/packages/cli/README.md b/packages/cli/README.md index 5344afe2adf..f990691bd9c 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -5532,7 +5532,7 @@ Check an app for Shopify-specific security issues and write deterministic-findin ``` USAGE - $ shopify app security check [--exclude ...] [--include-dir ...] [--json-schema] [--list-files | + $ shopify app security check [--exclude ...] [--include-dir ...] [-j] [--json-schema] [--list-files | --print-instructions | --skip-instructions | --blocking high|medium|low|none] [--no-color] [--no-git-ignore] [--no-input] [--path ] [--verbose] [--without-app-config [--client-id | -c ]] @@ -5541,6 +5541,10 @@ FLAGS The name of the app configuration. [env: SHOPIFY_FLAG_APP_CONFIG] + -j, --json + Output the result as JSON. Automatically disables color output. + [env: SHOPIFY_FLAG_JSON] + --blocking=