From 61236def196f12a5a003cfd6c952c87121138727 Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 20 Oct 2026 12:30:24 -0700 Subject: [PATCH 1/3] Move the app security review presenter to security-review/result.ts `securityReview` now returns the reviewed results and the commands that the review suggests, instead of rendering them and setting the exit code. The command passes them to a presenter in security-review/, which renders the review and sets exit code 1 when `--blocking` is breached. The JSON codec will live beside it, as for the other app security commands. The text output and exit codes don't change. --- .../cli/commands/app/security/review.test.ts | 18 +++- .../src/cli/commands/app/security/review.ts | 5 +- .../src/cli/services/security-review.test.ts | 89 ++++++------------- .../app/src/cli/services/security-review.ts | 38 +++----- .../services/security-review/result.test.ts | 78 ++++++++++++++++ .../cli/services/security-review/result.ts | 9 ++ 6 files changed, 146 insertions(+), 91 deletions(-) create mode 100644 packages/app/src/cli/services/security-review/result.test.ts create mode 100644 packages/app/src/cli/services/security-review/result.ts diff --git a/packages/app/src/cli/commands/app/security/review.test.ts b/packages/app/src/cli/commands/app/security/review.test.ts index 500cd6571f0..3cea42095d9 100644 --- a/packages/app/src/cli/commands/app/security/review.test.ts +++ b/packages/app/src/cli/commands/app/security/review.test.ts @@ -2,12 +2,14 @@ import SecurityReview from './review.js' import SecurityCheck from './check.js' import {appFlags} from '../../../flags.js' import securityReview from '../../../services/security-review.js' +import {renderSecurityReviewResult} from '../../../services/security-review/result.js' import AppLinkedCommand from '../../../utilities/app-linked-command.js' import BaseCommand from '@shopify/cli-kit/node/base-command' import {cwd, resolvePath} from '@shopify/cli-kit/node/path' import {describe, expect, test, vi} from 'vitest' vi.mock('../../../services/security-review.js') +vi.mock('../../../services/security-review/result.js') describe('app security review command', () => { test('documents command-specific telemetry and links to usage reporting', () => { @@ -54,12 +56,23 @@ describe('app security review command', () => { configName: undefined, clientId: undefined, withoutAppConfig: false, - verbose: false, checkIds: [], blocking: 'none', }) }) + test.each([ + ['without --verbose', [], false], + ['with --verbose', ['--verbose'], true], + ])('presents the reviewed results %s', async (_description, argv, verbose) => { + const review = {} as Awaited> + vi.mocked(securityReview).mockResolvedValue(review) + + await SecurityReview.run(argv, import.meta.url) + + expect(renderSecurityReviewResult).toHaveBeenCalledWith(review, {verbose}) + }) + test('forwards --path, --config, --client-id and --without-app-config', async () => { await SecurityReview.run( ['--path', './fixtures/app', '--without-app-config', '--client-id', 'abc123'], @@ -79,7 +92,7 @@ describe('app security review command', () => { expect(securityReview).toHaveBeenLastCalledWith(expect.objectContaining({configName: 'staging'})) }) - test('forwards --path, --verbose, every --check-id and --blocking', async () => { + test('forwards --path, every --check-id and --blocking', async () => { await SecurityReview.run( [ '--path', @@ -100,7 +113,6 @@ describe('app security review command', () => { configName: undefined, clientId: undefined, withoutAppConfig: false, - verbose: true, checkIds: ['OPEN_REDIRECT', 'EOL_API_VERSION'], blocking: 'medium', }) diff --git a/packages/app/src/cli/commands/app/security/review.ts b/packages/app/src/cli/commands/app/security/review.ts index 228743b1d0e..b9112f047a7 100644 --- a/packages/app/src/cli/commands/app/security/review.ts +++ b/packages/app/src/cli/commands/app/security/review.ts @@ -1,6 +1,7 @@ import {appSecurityBlockingFlag} from './blocking-flag.js' import {appSecuritySelectionFlags} from './selection-flags.js' import securityReview from '../../../services/security-review.js' +import {renderSecurityReviewResult} from '../../../services/security-review/result.js' import {Flags} from '@oclif/core' import BaseCommand from '@shopify/cli-kit/node/base-command' import {globalFlags} from '@shopify/cli-kit/node/cli' @@ -32,14 +33,14 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de public async run(): Promise { const {flags} = await this.parse(SecurityReview) - await securityReview({ + const review = await securityReview({ directory: flags.path, configName: flags.config, clientId: flags['client-id'], withoutAppConfig: Boolean(flags['without-app-config']), - verbose: Boolean(flags.verbose), checkIds: flags['check-id'] ?? [], blocking: flags.blocking, }) + renderSecurityReviewResult(review, {verbose: Boolean(flags.verbose)}) } } diff --git a/packages/app/src/cli/services/security-review.test.ts b/packages/app/src/cli/services/security-review.test.ts index 61257ca8aec..278a9a9e813 100644 --- a/packages/app/src/cli/services/security-review.test.ts +++ b/packages/app/src/cli/services/security-review.test.ts @@ -15,8 +15,6 @@ import {describe, expect, test, vi} from 'vitest' import type {AppSecurityBlockingLevel} from './app-security-api.js' import type {AgentFindingsDocument, DeterministicFindingsDocument} from './app-security-engine/index.js' -const now = new Date('2026-09-01T12:30:00.000Z') - // The results key of `createApp`'s shopify.app.toml. const RESULTS_KEY = 'shopify.app' @@ -43,9 +41,6 @@ function testDependencies() { appConfigFilePath: joinPath(path, 'shopify.app.toml'), }) as const, loadResults: loadAppSecurityResults, - render: vi.fn(), - now: () => now, - setExitCode: vi.fn(), recordMetadata: vi.fn(async () => {}), } satisfies SecurityReviewDependencies return dependencies @@ -232,11 +227,8 @@ describe('securityReview', () => { const unfiltered = testDependencies() const filtered = testDependencies() - await securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'none'}, unfiltered) - await securityReview( - {directory: appRoot, verbose: false, checkIds: ['OPEN_REDIRECT'], blocking: 'none'}, - filtered, - ) + await securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, unfiltered) + await securityReview({directory: appRoot, checkIds: ['OPEN_REDIRECT'], blocking: 'none'}, filtered) // Six recorded findings: two deterministic ones superseded by a prefer-agent check and one suppressed by the agent. // OPEN_REDIRECT has none, so the filtered run would record zero if it counted only the selected checks. @@ -250,30 +242,25 @@ describe('securityReview', () => { const appRoot = await createApp(directory, {}) const dependencies = testDependencies() - await securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'none'}, dependencies) + await securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, dependencies) expect(dependencies.recordMetadata).not.toHaveBeenCalled() }) }) - test('renders the review to the terminal and exits 0 by default', async () => { + test('returns the reviewed results and the commands to run next', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, both) - const dependencies = testDependencies() - await securityReview({directory: appRoot, verbose: true, checkIds: [], blocking: 'none'}, dependencies) + const review = await securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, testDependencies()) - expect(dependencies.setExitCode).not.toHaveBeenCalled() - expect(dependencies.render).toHaveBeenCalledTimes(1) - const input = dependencies.render.mock.calls[0]![0] - expect(input.verbose).toBe(true) - expect(input.now).toBe(now) - expect(input.commands.scan.args).toContainEqual({flag: '--path', value: relativePath(cwd(), appRoot)}) - expect(input.result.resultsDirectory).toBe(appSecurityArtifactPaths(appRoot, RESULTS_KEY).resultsDirectory) - expect(input.result.sources.deterministic?.path).toBe( + expect(review.commands.scan.args).toContainEqual({flag: '--path', value: relativePath(cwd(), appRoot)}) + expect(review.result.resultsDirectory).toBe(appSecurityArtifactPaths(appRoot, RESULTS_KEY).resultsDirectory) + expect(review.result.sources.deterministic?.path).toBe( appSecurityArtifactPaths(appRoot, RESULTS_KEY).deterministicFindingsPath, ) - expect(input.result.checks).toHaveLength(6) + expect(review.result.checks).toHaveLength(6) + expect(review.result.blocking).toEqual({level: 'none', blockedChecks: 0}) }) }) @@ -285,12 +272,10 @@ describe('securityReview', () => { coverage: {...deterministicFindingsDocument.coverage, scope}, } const appRoot = await createApp(directory, {deterministic}) - const dependencies = testDependencies() - await securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'none'}, dependencies) + const review = await securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, testDependencies()) - const input = dependencies.render.mock.calls[0]![0] - expect(input.commands.scan.args.slice(-3)).toEqual([ + expect(review.commands.scan.args.slice(-3)).toEqual([ {flag: '--include-dir', value: '../backend'}, {flag: '--exclude', value: 'vendor/**'}, '--no-git-ignore', @@ -298,49 +283,39 @@ describe('securityReview', () => { }) }) - test('sets exit code 1 after the output when --blocking is breached', async () => { + test('returns the blocked checks when --blocking is breached', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, both) - const dependencies = testDependencies() - const order: string[] = [] - dependencies.render.mockImplementation(() => order.push('render')) - dependencies.setExitCode.mockImplementation((code) => order.push(`exit ${code}`)) - await securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'high'}, dependencies) + const review = await securityReview({directory: appRoot, checkIds: [], blocking: 'high'}, testDependencies()) - expect(order).toEqual(['render', 'exit 1']) + expect(review.result.blocking).toEqual({level: 'high', blockedChecks: 2}) }) }) - test('exits 0 with missing files, even with --check-id and --blocking', async () => { + test('blocks no check with missing files, even with --check-id and --blocking', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, {}) - const dependencies = testDependencies() - await securityReview( - {directory: appRoot, verbose: false, checkIds: ['NOT_A_CHECK'], blocking: 'high'}, - dependencies, + const review = await securityReview( + {directory: appRoot, checkIds: ['NOT_A_CHECK'], blocking: 'high'}, + testDependencies(), ) - expect(dependencies.setExitCode).not.toHaveBeenCalled() - const input = dependencies.render.mock.calls[0]![0] - expect(input.result.sources).toEqual({deterministic: null, agent: null}) - expect(input.result.checks).toEqual([]) - expect(input.result.filter).toEqual({checkIds: ['NOT_A_CHECK']}) + expect(review.result.blocking).toEqual({level: 'high', blockedChecks: 0}) + expect(review.result.sources).toEqual({deterministic: null, agent: null}) + expect(review.result.checks).toEqual([]) + expect(review.result.filter).toEqual({checkIds: ['NOT_A_CHECK']}) }) }) - test('aborts on an unknown --check-id without rendering or setting the exit code', async () => { + test('aborts on an unknown --check-id', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, both) - const dependencies = testDependencies() await expect( - securityReview({directory: appRoot, verbose: false, checkIds: ['NOT_A_CHECK'], blocking: 'none'}, dependencies), + securityReview({directory: appRoot, checkIds: ['NOT_A_CHECK'], blocking: 'none'}, testDependencies()), ).rejects.toThrow('Unknown check ID: NOT_A_CHECK.') - - expect(dependencies.render).not.toHaveBeenCalled() - expect(dependencies.setExitCode).not.toHaveBeenCalled() }) }) @@ -348,33 +323,27 @@ describe('securityReview', () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, {deterministic: deterministicFindingsDocument}) await writeFile(appSecurityArtifactPaths(appRoot, RESULTS_KEY).agentFindingsPath, '{not json') - const dependencies = testDependencies() await expect( - securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'none'}, dependencies), + securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, testDependencies()), ).rejects.toThrow('The app security check results could not be loaded because a results file is invalid.') - - expect(dependencies.render).not.toHaveBeenCalled() }) }) test('aborts with "No results found" when the results directory does not exist', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await fileRealPath(directory) - const dependencies = testDependencies() await expect( - securityReview({directory: appRoot, verbose: false, checkIds: [], blocking: 'none'}, dependencies), + securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, testDependencies()), ).rejects.toMatchObject({message: `No app security check results for ${RESULTS_KEY} in ${appRoot}.`}) - - expect(dependencies.render).not.toHaveBeenCalled() }) }) }) describe('securityReview --client-id lookup', () => { const unknownClientId = new AbortError('No app with client ID unknown-client-id found') - const reviewOptions = {verbose: false, checkIds: [], blocking: 'none' as const} + const reviewOptions = {checkIds: [], blocking: 'none' as const} /** The real selection resolver, with the client ID lookup replaced, and a spy on the results loader. */ function lookUpDependencies(lookUpApp: (clientId: string) => Promise) { @@ -412,7 +381,6 @@ describe('securityReview --client-id lookup', () => { expect.objectContaining({clientIdOverride: 'flag-client-id'}), appRoot, ) - expect(dependencies.render).toHaveBeenCalled() }) }) @@ -428,7 +396,6 @@ describe('securityReview --client-id lookup', () => { ).rejects.toBe(unknownClientId) expect(dependencies.loadResults).not.toHaveBeenCalled() - expect(dependencies.render).not.toHaveBeenCalled() expect(dependencies.recordMetadata).not.toHaveBeenCalled() }) }) diff --git a/packages/app/src/cli/services/security-review.ts b/packages/app/src/cli/services/security-review.ts index e76106b1ce8..a89fdd41091 100644 --- a/packages/app/src/cli/services/security-review.ts +++ b/packages/app/src/cli/services/security-review.ts @@ -1,5 +1,5 @@ import {appSecurityArtifactPaths} from './app-security-artifacts.js' -import {resolveAppSecurityCommands} from './app-security-commands.js' +import {resolveAppSecurityCommands, type AppSecurityCommands} from './app-security-commands.js' import { resolveAppSecuritySelection, resultsKey, @@ -7,7 +7,6 @@ import { type AppSecuritySelectionOptions, } from './app-security-selection.js' import {loadAppSecurityResults, type AppSecurityResults} from './app-security-results.js' -import {renderSecurityReview, type SecurityReviewPresenterInput} from './security-review-output.js' import {recordAppSecurityMetadata, type AppSecurityMetadata} from './app-security-metadata.js' import {activeFindings, SEVERITY_RANK, type CombinedCheck} from './app-security-engine/index.js' import {AbortError} from '@shopify/cli-kit/node/error' @@ -18,7 +17,6 @@ interface SecurityReviewOptions { configName?: string clientId?: string withoutAppConfig?: boolean - verbose: boolean /** Exact check IDs from `--check-id`; empty means every check. */ checkIds: string[] blocking: AppSecurityBlockingLevel @@ -42,30 +40,24 @@ export interface SecurityReviewResult { } } +/** What `securityReview` returns: the reviewed results, and the commands that the review suggests running next. */ +export interface ReviewedAppSecurityResults { + result: SecurityReviewResult + commands: AppSecurityCommands +} + export interface SecurityReviewDependencies { resolveSelection(options: AppSecuritySelectionOptions): Promise loadResults(selection: AppSecuritySelection, path: string): Promise - render(input: SecurityReviewPresenterInput): void - now(): Date - setExitCode(exitCode: number): void recordMetadata(fields: AppSecurityMetadata): Promise } const defaultDependencies: SecurityReviewDependencies = { resolveSelection: resolveAppSecuritySelection, loadResults: loadAppSecurityResults, - render: renderSecurityReview, - now: () => new Date(), - setExitCode: (exitCode) => { - process.exitCode = exitCode - }, recordMetadata: recordAppSecurityMetadata, } -function isBlockingBreached(result: SecurityReviewResult): boolean { - return result.blocking.blockedChecks > 0 -} - /** * Applies `--check-id` and `--blocking` to the loaded results (§9.2). Pure, so the presenter and the JSON * codec both work from the same data. @@ -124,14 +116,14 @@ function assertKnownCheckIds(checkIds: string[], checks: CombinedCheck[]): void } /** - * Shows the combined app security check results (§9). Loads both result files, narrows them with `--check-id`, then - * renders the review to stderr. A `--blocking` breach sets the exit code after the - * output; an error exits 1 and everything else, including missing files, exits 0. + * Reviews the combined app security check results (§9). Loads both result files and narrows them with `--check-id`. + * Prints nothing: an error aborts, and everything else, including missing files, returns the reviewed results for + * the presenter. */ export default async function securityReview( options: SecurityReviewOptions, dependencies: SecurityReviewDependencies = defaultDependencies, -): Promise { +): Promise { const selection = await dependencies.resolveSelection({ path: options.directory, config: options.configName, @@ -154,17 +146,13 @@ export default async function securityReview( await dependencies.recordMetadata({num_security_findings: findings}) } - dependencies.render({ + return { result, - verbose: options.verbose, - now: dependencies.now(), // The latest scan's scope, so running `check` again gathers the same files. commands: resolveAppSecurityCommands( selection, options.directory, results.sources.deterministic?.document.coverage.scope, ), - }) - - if (isBlockingBreached(result)) dependencies.setExitCode(1) + } } diff --git a/packages/app/src/cli/services/security-review/result.test.ts b/packages/app/src/cli/services/security-review/result.test.ts new file mode 100644 index 00000000000..3b217c16a5c --- /dev/null +++ b/packages/app/src/cli/services/security-review/result.test.ts @@ -0,0 +1,78 @@ +import {renderSecurityReviewResult} from './result.js' +import {reviewAppSecurityResults, type ReviewedAppSecurityResults} from '../security-review.js' +import {appSecurityArtifactPaths} from '../app-security-artifacts.js' +import {resolveAppSecurityCommands} from '../app-security-commands.js' +import {appSecurityResultsFor} from '../app-security-results.test-data.js' +import { + agentFindingsDocument, + deterministicFindingsDocument, +} from '../app-security-engine/tests/fixtures/findings-documents.js' +import {unstyled} from '@shopify/cli-kit/node/output' +import {cwd} from '@shopify/cli-kit/node/path' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' +import {afterEach, describe, expect, test} from 'vitest' +import type {AppSecurityBlockingLevel} from '../app-security-api.js' +import type {AppSecuritySelection} from '../app-security-selection.js' + +const originalExitCode = process.exitCode +afterEach(() => { + process.exitCode = originalExitCode +}) + +const appRoot = '/tmp/review-app' +const selection: AppSecuritySelection = { + kind: 'config', + appDirectory: appRoot, + appConfigFilePath: `${appRoot}/shopify.app.toml`, +} + +function reviewed(options: {blocking?: AppSecurityBlockingLevel} = {}): ReviewedAppSecurityResults { + const results = appSecurityResultsFor(appRoot, 'shopify.app', { + deterministic: deterministicFindingsDocument, + agent: agentFindingsDocument, + }) + return { + result: reviewAppSecurityResults(results, { + resultsDirectory: appSecurityArtifactPaths(appRoot, 'shopify.app').resultsDirectory, + checkIds: [], + blocking: options.blocking ?? 'none', + }), + commands: resolveAppSecurityCommands(selection, cwd()), + } +} + +describe('renderSecurityReviewResult', () => { + test('renders the review in the terminal and leaves the exit code alone', () => { + process.exitCode = undefined + const output = mockAndCaptureOutput() + output.clear() + + renderSecurityReviewResult(reviewed(), {verbose: false}) + + const rendered = unstyled(output.output()) + expect(rendered).toContain('3 checks with findings. 1 check unresolved.') + expect(rendered).toContain('Next steps') + expect(rendered).not.toContain('Suppressed:') + expect(process.exitCode).toBeUndefined() + }) + + test('shows suppressed findings in full with --verbose', () => { + const output = mockAndCaptureOutput() + output.clear() + + renderSecurityReviewResult(reviewed(), {verbose: true}) + + expect(unstyled(output.output())).toContain('Suppressed: The route is restricted to the app owner') + }) + + test('renders the Blocking section and sets exit code 1 when --blocking is breached', () => { + process.exitCode = undefined + const output = mockAndCaptureOutput() + output.clear() + + renderSecurityReviewResult(reviewed({blocking: 'high'}), {verbose: false}) + + expect(unstyled(output.error())).toContain('2 checks at or above high (--blocking high).') + expect(process.exitCode).toBe(1) + }) +}) diff --git a/packages/app/src/cli/services/security-review/result.ts b/packages/app/src/cli/services/security-review/result.ts new file mode 100644 index 00000000000..9250356e318 --- /dev/null +++ b/packages/app/src/cli/services/security-review/result.ts @@ -0,0 +1,9 @@ +import {renderSecurityReview} from '../security-review-output.js' +import type {ReviewedAppSecurityResults} from '../security-review.js' + +/** Presents reviewed results in the terminal, then sets exit code 1 when they breach `--blocking`. */ +export function renderSecurityReviewResult(review: ReviewedAppSecurityResults, options: {verbose: boolean}): void { + renderSecurityReview({result: review.result, verbose: options.verbose, now: new Date(), commands: review.commands}) + + if (review.result.blocking.blockedChecks > 0) process.exitCode = 1 +} From 59c62f5fe9e56e628ba2619e43a2732f2b1717cb Mon Sep 17 00:00:00 2001 From: Jason Kirtland Date: Tue, 20 Oct 2026 12:53:12 -0700 Subject: [PATCH 2/3] Add typed JSON output to app security review `--json` prints the combined checks with every finding, whatever `--verbose`, beside the results files they come from, the latest scan's detection and coverage, and the app. `valid` and `blocking` report the `--blocking` policy; a breach still exits 1, after the result is printed. Paths are absolute, lines and columns are one-based as the results files record them, and stored enum values are kebab-case. An invalid results file aborts with the `results-file-invalid` code and lists each file's path and errors in `details.files`. An unknown `--check-id` aborts with `unknown-check-id` and lists the unknown IDs in `details.checkIds`, so callers can branch without parsing the message. The scope, scan directory and blocking definitions are shared, so that `check` can reuse them. --- .../generated/generated_docs_data_v2.json | 11 +- .../app/security/review.integration.test.ts | 664 +++++++++++ .../cli/commands/app/security/review.test.ts | 20 +- .../src/cli/commands/app/security/review.ts | 12 +- .../services/app-security-json-output.test.ts | 112 ++ .../cli/services/app-security-json-output.ts | 76 +- .../cli/services/app-security-results.test.ts | 16 + .../src/cli/services/app-security-results.ts | 9 +- .../src/cli/services/security-review.test.ts | 16 +- .../app/src/cli/services/security-review.ts | 9 +- .../services/security-review/result.test.ts | 754 +++++++++++- .../cli/services/security-review/result.ts | 276 ++++- .../services/security-review/types.test.ts | 224 ++++ .../src/cli/services/security-review/types.ts | 297 +++++ packages/cli/README.md | 1018 ++++++++++++++++- packages/cli/oclif.manifest.json | 13 +- .../rules/json-output-command-exceptions.js | 1 - 17 files changed, 3482 insertions(+), 46 deletions(-) create mode 100644 packages/app/src/cli/commands/app/security/review.integration.test.ts create mode 100644 packages/app/src/cli/services/security-review/types.test.ts create mode 100644 packages/app/src/cli/services/security-review/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 29dcad605be..56898354403 100644 --- a/docs-shopify.dev/generated/generated_docs_data_v2.json +++ b/docs-shopify.dev/generated/generated_docs_data_v2.json @@ -3851,9 +3851,18 @@ "description": "The name of the app configuration.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_CONFIG" + }, + { + "filePath": "docs-shopify.dev/commands/interfaces/app-security-review.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 appsecurityreview {\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 * Show only this check. Repeat the flag to show several checks.\n * @environment SHOPIFY_FLAG_CHECK_ID\n */\n '--check-id '?: 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 * Print the command's JSON schemas.\n * @environment SHOPIFY_FLAG_JSON_SCHEMA\n */\n '--json-schema'?: ''\n\n /**\n * Disable color output.\n * @environment SHOPIFY_FLAG_NO_COLOR\n */\n '--no-color'?: ''\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 * 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 appsecurityreview {\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 * Show only this check. Repeat the flag to show several checks.\n * @environment SHOPIFY_FLAG_CHECK_ID\n */\n '--check-id '?: 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 * 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 * Disable color output.\n * @environment SHOPIFY_FLAG_NO_COLOR\n */\n '--no-color'?: ''\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 * 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}" } }, "appsubscriptionmigrationscancel": { diff --git a/packages/app/src/cli/commands/app/security/review.integration.test.ts b/packages/app/src/cli/commands/app/security/review.integration.test.ts new file mode 100644 index 00000000000..8939bddd05c --- /dev/null +++ b/packages/app/src/cli/commands/app/security/review.integration.test.ts @@ -0,0 +1,664 @@ +import SecurityReview from './review.js' +import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js' +import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js' +import {Config} from '@oclif/core' +import {fileRealPath, inTemporaryDirectory, 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 {describe, expect, test, vi} from 'vitest' +// eslint-disable-next-line no-restricted-imports -- Verify native filesystem paths in JSON output. +import {resolve} from 'node:path' +import type {AgentFindingsDocument, DeterministicFindingsDocument} from '../../../services/app-security-engine/index.js' + +// Skip the installed-CLI error reporting setup, as `check`'s command boundary test does. +vi.mock('@shopify/cli-kit/node/context/local', async (importOriginal) => ({ + ...(await importOriginal()), + isDevelopment: () => true, +})) +// Command lifecycle telemetry is unrelated to reviewing. Keep the real error handler and renderer. +vi.mock('@shopify/cli-kit/node/analytics', async (importOriginal) => ({ + ...(await importOriginal()), + reportAnalyticsEvent: vi.fn(), +})) + +const credentialLogLeakage = { + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + docs_url: 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage', + current_version: 2, +} as const + +const tenantIsolation = { + title: 'Database query may not be scoped by shop', + severity: 'high', + description: "Detects database queries that don't reference the shop.", + docs_url: 'https://shopify.dev/docs/apps/build/security/app-security-checks/missing-tenant-isolation', + current_version: 4, +} as const + +const eolApiVersion = { + title: 'End-of-life API version', + severity: 'low', + description: 'Detects API versions past their support window.', + guide: 'https://shopify.dev/docs/api/usage/versioning', + docs_url: 'https://shopify.dev/docs/apps/build/security/app-security-checks/eol-api-version', + current_version: 2, +} as const + +const deterministicFindings: DeterministicFindingsDocument = { + schema_version: 1, + source: 'deterministic', + engine: {name: 'shopify-app-security', version: '4.9.0', ruleset: 'app-security-rules@4.9.0'}, + generated_at: '2026-09-01T10:00:00.250Z', + detection: { + framework: 'react_router', + surface: 'react_router', + languages: [{name: 'typescript', support: 'supported', files: ['app/routes/orders.tsx']}], + }, + coverage: { + files_scanned: 3, + files_skipped: [], + gaps: [], + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + scan_directories: [{directory: '.', origin: 'app_directory'}], + }, + checks: [ + { + id: 'CREDENTIAL_LOG_LEAKAGE', + version: 2, + status: 'executed', + analysis_mode: 'ast', + snapshot: credentialLogLeakage, + findings: [ + { + location: {file: 'app/routes/orders.tsx', line: 5, column: 3}, + message: 'accessToken flows into console.log.', + evidence: [{location: {file: 'app/routes/orders.tsx', line: 5}, quote: 'console.log(session.accessToken)'}], + snippet: 'console.log(session.accessToken)', + fix: {automated: false, description: 'Remove credentials from logs.'}, + }, + ], + }, + { + id: 'EOL_API_VERSION', + version: 2, + status: 'executed', + analysis_mode: 'structured_config', + snapshot: eolApiVersion, + findings: [ + { + location: {file: 'shopify.app.toml', line: 8}, + message: 'api_version 2024-01 is past its support window.', + evidence: [], + fix: { + automated: false, + description: 'Update api_version.', + guide: 'https://shopify.dev/docs/api/usage/versioning', + }, + }, + ], + }, + ], +} + +const agentFindings: AgentFindingsDocument = { + schema_version: 1, + source: 'agent', + engine: {name: 'shopify-app-security', version: '4.9.0'}, + generated_at: '2026-09-01T11:00:00.000Z', + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + checks: [ + { + id: 'CREDENTIAL_LOG_LEAKAGE', + version: 2, + status: 'executed', + snapshot: {...credentialLogLeakage, precedence: 'prefer-agent'}, + findings: [ + { + location: {file: 'app/routes/orders.tsx', line: 5}, + message: 'The session access token is written to the server log.', + evidence: [{location: {file: 'app/routes/orders.tsx', line: 5}, quote: 'console.log(session.accessToken)'}], + confidence: 'high', + reasoning: 'The session holds a live credential.', + }, + ], + }, + { + id: 'MISSING_TENANT_ISOLATION', + version: 4, + status: 'executed', + snapshot: {...tenantIsolation, precedence: 'union'}, + findings: [ + { + location: {file: 'app/shopify.server.ts', line: 3}, + message: 'The app configuration is shared by every shop.', + evidence: [{location: {file: 'app/shopify.server.ts', line: 3}}], + suppression: {justification: 'The configuration holds no shop data.'}, + }, + ], + }, + ], +} + +interface ResultsFiles { + deterministic?: unknown + agent?: unknown +} + +/** Creates an app linked to `test-client-id` with the `shopify.app` results directory, and returns its real path. */ +async function createApp(directory: string, files: ResultsFiles | null = {}): Promise { + await writeFile(joinPath(directory, 'shopify.app.toml'), validAppConfiguration('test-client-id')) + const appDirectory = await fileRealPath(directory) + if (files === null) return appDirectory + + const paths = appSecurityArtifactPaths(appDirectory, 'shopify.app') + await mkdir(paths.resultsDirectory) + if (files.deterministic) await writeFile(paths.deterministicFindingsPath, JSON.stringify(files.deterministic)) + if (files.agent) await writeFile(paths.agentFindingsPath, JSON.stringify(files.agent)) + return appDirectory +} + +function resultsPath(appDirectory: string, ...segments: string[]): string { + return resolve(appDirectory, '.shopify', 'app-security', 'shopify.app', ...segments) +} + +/** The results path as the section titles show it: cli-kit's `joinPath` uses forward slashes on every OS, unlike `resultsPath`. */ +function renderedResultsPath(appDirectory: string, ...segments: string[]): string { + return joinPath(appDirectory, '.shopify', 'app-security', 'shopify.app', ...segments) +} + +function appJson(appDirectory: string) { + return { + directory: resolve(appDirectory), + configurationPath: resolve(appDirectory, 'shopify.app.toml'), + clientId: 'test-client-id', + } +} + +function deterministicResultsFileJson(appDirectory: string) { + return { + path: resultsPath(appDirectory, 'deterministic-findings.json'), + generatedAt: '2026-09-01T10:00:00Z', + engine: {name: 'shopify-app-security', version: '4.9.0', ruleset: 'app-security-rules@4.9.0'}, + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + } +} + +function detectionJson(appDirectory: string) { + return { + framework: 'react-router', + surface: 'react-router', + languages: [ + {name: 'typescript', support: 'supported', files: [{path: resolve(appDirectory, 'app', 'routes', 'orders.tsx')}]}, + ], + } +} + +function coverageJson(appDirectory: string) { + return { + filesScanned: 3, + filesSkipped: [], + gaps: [], + scanDirectories: [{directory: resolve(appDirectory), origin: 'app-directory'}], + } +} + +function eolApiVersionJson(appDirectory: string) { + return { + id: 'EOL_API_VERSION', + title: 'End-of-life API version', + severity: 'low', + description: 'Detects API versions past their support window.', + guide: 'https://shopify.dev/docs/api/usage/versioning', + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/eol-api-version', + precedence: 'union', + appliedPrecedence: 'union', + status: 'executed', + bySource: { + deterministic: { + version: 2, + status: 'executed', + reason: null, + analysisMode: 'structured-config', + generatedAt: '2026-09-01T10:00:00Z', + }, + agent: null, + }, + findings: [ + { + source: 'deterministic', + disposition: 'active', + location: {filePath: resolve(appDirectory, 'shopify.app.toml'), line: 8, column: null}, + message: 'api_version 2024-01 is past its support window.', + snippet: null, + evidence: [], + fix: { + description: 'Update api_version.', + guide: 'https://shopify.dev/docs/api/usage/versioning', + automated: false, + }, + }, + ], + } +} + +// 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 unstyled(output).replaceAll('│', '').replace(/\s+/g, ' ') +} + +async function runCommand(argv: string[]) { + const previousExitCode = process.exitCode + process.exitCode = 0 + // 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 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 SecurityReview.run(argv, config) + return {stdout: stdout(), stderr: stderr()} + }) + return {...output, exitCode: process.exitCode} + } finally { + exit.mockRestore() + process.exitCode = previousExitCode + } +} + +/** + * Runs the command with --json. The 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() + } +} + +describe('app security review command boundary', () => { + test('--json writes one result with the deterministic results alone', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory, {deterministic: deterministicFindings}) + + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + status: 'success', + valid: true, + app: appJson(appDirectory), + resultsDirectory: resultsPath(appDirectory), + resultsFiles: {deterministic: deterministicResultsFileJson(appDirectory), agent: null}, + detection: detectionJson(appDirectory), + coverage: coverageJson(appDirectory), + scopeDiffers: false, + checks: [ + { + id: 'CREDENTIAL_LOG_LEAKAGE', + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + guide: null, + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage', + precedence: 'union', + appliedPrecedence: 'union', + status: 'executed', + bySource: { + deterministic: { + version: 2, + status: 'executed', + reason: null, + analysisMode: 'ast', + generatedAt: '2026-09-01T10:00:00Z', + }, + agent: null, + }, + findings: [ + { + source: 'deterministic', + disposition: 'active', + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: 3}, + message: 'accessToken flows into console.log.', + snippet: 'console.log(session.accessToken)', + evidence: [ + { + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: null}, + quote: 'console.log(session.accessToken)', + }, + ], + fix: {description: 'Remove credentials from logs.', guide: null, automated: false}, + }, + ], + }, + eolApiVersionJson(appDirectory), + ], + blocking: {level: 'none', blockedCheckCount: 0}, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--json combines both sources, with a prefer-agent check and a suppressed finding', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + status: 'success', + valid: true, + app: appJson(appDirectory), + resultsDirectory: resultsPath(appDirectory), + resultsFiles: { + deterministic: deterministicResultsFileJson(appDirectory), + agent: { + path: resultsPath(appDirectory, 'agent-findings.json'), + generatedAt: '2026-09-01T11:00:00Z', + engine: {name: 'shopify-app-security', version: '4.9.0'}, + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + }, + }, + detection: detectionJson(appDirectory), + coverage: coverageJson(appDirectory), + scopeDiffers: false, + checks: [ + { + id: 'CREDENTIAL_LOG_LEAKAGE', + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + guide: null, + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage', + precedence: 'prefer-agent', + appliedPrecedence: 'prefer-agent', + status: 'executed', + bySource: { + deterministic: { + version: 2, + status: 'executed', + reason: null, + analysisMode: 'ast', + generatedAt: '2026-09-01T10:00:00Z', + }, + agent: { + version: 2, + status: 'executed', + reason: null, + analysisMode: null, + generatedAt: '2026-09-01T11:00:00Z', + }, + }, + findings: [ + { + source: 'deterministic', + disposition: 'superseded', + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: 3}, + message: 'accessToken flows into console.log.', + snippet: 'console.log(session.accessToken)', + evidence: [ + { + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: null}, + quote: 'console.log(session.accessToken)', + }, + ], + fix: {description: 'Remove credentials from logs.', guide: null, automated: false}, + }, + { + source: 'agent', + disposition: 'active', + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: null}, + message: 'The session access token is written to the server log.', + snippet: null, + evidence: [ + { + location: {filePath: resolve(appDirectory, 'app', 'routes', 'orders.tsx'), line: 5, column: null}, + quote: 'console.log(session.accessToken)', + }, + ], + confidence: 'high', + reasoning: 'The session holds a live credential.', + suppression: null, + }, + ], + }, + { + id: 'MISSING_TENANT_ISOLATION', + title: 'Database query may not be scoped by shop', + severity: 'high', + description: "Detects database queries that don't reference the shop.", + guide: null, + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/missing-tenant-isolation', + precedence: 'union', + appliedPrecedence: 'union', + status: 'executed', + bySource: { + deterministic: null, + agent: { + version: 4, + status: 'executed', + reason: null, + analysisMode: null, + generatedAt: '2026-09-01T11:00:00Z', + }, + }, + findings: [ + { + source: 'agent', + disposition: 'suppressed', + location: {filePath: resolve(appDirectory, 'app', 'shopify.server.ts'), line: 3, column: null}, + message: 'The app configuration is shared by every shop.', + snippet: null, + evidence: [ + { + location: {filePath: resolve(appDirectory, 'app', 'shopify.server.ts'), line: 3, column: null}, + quote: null, + }, + ], + confidence: null, + reasoning: null, + suppression: {justification: 'The configuration holds no shop data.'}, + }, + ], + }, + eolApiVersionJson(appDirectory), + ], + blocking: {level: 'none', blockedCheckCount: 0}, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--json writes no checks and no results files when the results directory is empty', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory) + + const result = await runJsonCommand(['--path', directory, '--blocking', 'high']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + status: 'success', + valid: true, + app: appJson(appDirectory), + resultsDirectory: resultsPath(appDirectory), + resultsFiles: {deterministic: null, agent: null}, + detection: null, + coverage: null, + scopeDiffers: false, + checks: [], + blocking: {level: 'high', blockedCheckCount: 0}, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--json with --check-id narrows the checks and the blocked checks', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + // Without --check-id, CREDENTIAL_LOG_LEAKAGE's active agent finding fails --blocking high. + const result = await runJsonCommand(['--path', directory, '--check-id', 'EOL_API_VERSION', '--blocking', 'high']) + + expect(result.exitCode).toBe(0) + const document = JSON.parse(result.stdout) + expect(document.checks.map((check: {id: string}) => check.id)).toStrictEqual(['EOL_API_VERSION']) + expect(document).toMatchObject({valid: true, blocking: {level: 'high', blockedCheckCount: 0}}) + }) + }) + + test('--json with a breached --blocking writes the result and exits 1', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + const result = await runJsonCommand(['--path', directory, '--blocking', 'low']) + + expect(result.exitCode).toBe(1) + const document = JSON.parse(result.stdout) + // CREDENTIAL_LOG_LEAKAGE and EOL_API_VERSION have active findings; MISSING_TENANT_ISOLATION's is suppressed. + expect(document).toMatchObject({status: 'success', valid: false, blocking: {level: 'low', blockedCheckCount: 2}}) + expect(document.checks).toHaveLength(3) + expect(result.stderr).toBe('') + }) + }) + + test('--json with --blocking none is valid and exits 0 whatever the active findings', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + 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('--json writes the same result with --verbose', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + const concise = await runJsonCommand(['--path', directory]) + const verbose = await runJsonCommand(['--path', directory, '--verbose']) + + expect(verbose.exitCode).toBe(0) + expect(verbose.stdout).toBe(concise.stdout) + }) + }) + + test('--json with an invalid results file writes one error document with every invalid file in its details', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory, { + deterministic: agentFindings, + agent: {...agentFindings, generated_at: 1}, + }) + + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: { + type: 'abort', + message: 'The app security check results could not be loaded because both results files are invalid.', + code: 'results-file-invalid', + nextSteps: [ + expect.stringMatching( + /^Run shopify app security check --path .+ to regenerate deterministic-findings\.json\.$/, + ), + expect.stringMatching(/^Have your coding agent run .+ again to regenerate agent-findings\.json\.$/), + expect.stringMatching(/^Or run shopify app security clean --path .+ to delete both results files/), + ], + customSections: [ + {title: renderedResultsPath(appDirectory, 'deterministic-findings.json'), body: expect.any(String)}, + {title: renderedResultsPath(appDirectory, 'agent-findings.json'), body: expect.any(String)}, + ], + details: { + files: [ + { + path: resultsPath(appDirectory, 'deterministic-findings.json'), + errors: ['source is "agent", but this file must hold "deterministic" findings.'], + }, + { + path: resultsPath(appDirectory, 'agent-findings.json'), + errors: ['generated_at: Expected string, received number'], + }, + ], + }, + }, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--json with unknown --check-id values writes one error document with only those IDs in its details', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings}) + + const result = await runJsonCommand([ + '--path', + directory, + '--check-id', + 'NOT_A_CHECK', + '--check-id', + 'EOL_API_VERSION', + '--check-id', + 'ALSO_NOT_A_CHECK', + ]) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: { + type: 'abort', + message: 'Unknown check IDs: NOT_A_CHECK, ALSO_NOT_A_CHECK.', + code: 'unknown-check-id', + tryMessage: 'Pass --check-id with one of the checks in the results files.', + customSections: [{title: 'Available check IDs', body: expect.any(String)}], + details: {checkIds: ['NOT_A_CHECK', 'ALSO_NOT_A_CHECK']}, + }, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--json without a results directory writes one error document with its code', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory, null) + + const result = await runJsonCommand(['--path', directory]) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: { + type: 'abort', + message: `No app security check results for shopify.app in ${appDirectory}.`, + code: 'results-directory-not-found', + tryMessage: expect.stringMatching(/^Run `shopify app security check --path .+`\.$/), + }, + }) + expect(result.stderr).toBe('') + }) + }) + + test('without --json, renders the review on stderr, nothing on stdout, and exits 1 when --blocking is breached', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory, {deterministic: deterministicFindings, agent: agentFindings}) + + const result = await runCommand(['--path', directory, '--blocking', 'low']) + + expect(result.exitCode).toBe(1) + expect(result.stdout).toBe('') + const text = boxText(result.stderr) + expect(text).toContain('High · CREDENTIAL_LOG_LEAKAGE · Credential reaches a log sink') + expect(text).toContain('2 checks with findings.') + expect(text).toContain('Blocking 2 checks at or above low (--blocking low).') + }) + }) +}) diff --git a/packages/app/src/cli/commands/app/security/review.test.ts b/packages/app/src/cli/commands/app/security/review.test.ts index 3cea42095d9..5e09bc3b8d5 100644 --- a/packages/app/src/cli/commands/app/security/review.test.ts +++ b/packages/app/src/cli/commands/app/security/review.test.ts @@ -3,8 +3,10 @@ import SecurityCheck from './check.js' import {appFlags} from '../../../flags.js' import securityReview from '../../../services/security-review.js' import {renderSecurityReviewResult} from '../../../services/security-review/result.js' +import {appSecurityReviewJsonOutputSchema} from '../../../services/security-review/types.js' import AppLinkedCommand from '../../../utilities/app-linked-command.js' import BaseCommand from '@shopify/cli-kit/node/base-command' +import {jsonFlag} from '@shopify/cli-kit/node/cli' import {cwd, resolvePath} from '@shopify/cli-kit/node/path' import {describe, expect, test, vi} from 'vitest' @@ -26,8 +28,11 @@ describe('app security review command', () => { test('does not require linked app context', () => { expect(SecurityReview.prototype).toBeInstanceOf(BaseCommand) expect(SecurityReview.prototype).not.toBeInstanceOf(AppLinkedCommand) - expect(SecurityReview.flags).not.toHaveProperty('json') - expect(SecurityReview.jsonOutputSchema).toBeUndefined() + }) + + test('defines --json and exposes the result schema', () => { + expect(SecurityReview.flags.json).toBe(jsonFlag.json) + expect(SecurityReview.jsonOutputSchema).toBe(appSecurityReviewJsonOutputSchema) }) test('defines the selection flags as check does', () => { @@ -62,15 +67,18 @@ describe('app security review command', () => { }) test.each([ - ['without --verbose', [], false], - ['with --verbose', ['--verbose'], true], - ])('presents the reviewed results %s', async (_description, argv, verbose) => { + ['in the terminal', [], false, 'text'], + ['in the terminal with --verbose', ['--verbose'], true, 'text'], + ['as JSON with --json', ['--json'], false, 'json'], + ['as JSON with -j', ['-j'], false, 'json'], + ['as JSON with --json and --verbose', ['--json', '--verbose'], true, 'json'], + ])('presents the reviewed results %s', async (_description, argv, verbose, format) => { const review = {} as Awaited> vi.mocked(securityReview).mockResolvedValue(review) await SecurityReview.run(argv, import.meta.url) - expect(renderSecurityReviewResult).toHaveBeenCalledWith(review, {verbose}) + expect(renderSecurityReviewResult).toHaveBeenCalledWith(review, {verbose}, format) }) test('forwards --path, --config, --client-id and --without-app-config', async () => { diff --git a/packages/app/src/cli/commands/app/security/review.ts b/packages/app/src/cli/commands/app/security/review.ts index b9112f047a7..8177f770753 100644 --- a/packages/app/src/cli/commands/app/security/review.ts +++ b/packages/app/src/cli/commands/app/security/review.ts @@ -2,9 +2,10 @@ import {appSecurityBlockingFlag} from './blocking-flag.js' import {appSecuritySelectionFlags} from './selection-flags.js' import securityReview from '../../../services/security-review.js' import {renderSecurityReviewResult} from '../../../services/security-review/result.js' +import {appSecurityReviewJsonOutputSchema} from '../../../services/security-review/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 SecurityReview extends BaseCommand { static summary = 'Show the combined app security check results.' @@ -13,10 +14,14 @@ export default class SecurityReview extends BaseCommand { The summary shows the scan directories and the scope of the latest scan, and the scope the agent reported. It notes when the agent findings were recorded for a different scope than the latest scan; that doesn't change the exit code. -The agent results are optional. Use \`--check-id\` to narrow the review to specific checks, \`--verbose\` for full reasoning, evidence and suppressed findings, and \`--blocking\` to exit with code 1 when a check with findings is at or above a severity. +The agent results are optional. Use \`--check-id\` to narrow the review to specific checks, \`--verbose\` for full reasoning, evidence and suppressed findings, and \`--blocking\` to exit with code 1 when a check with findings is at or above a severity. With \`--json\`, the result always includes every finding in full. 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 get jsonOutputSchema() { + return appSecurityReviewJsonOutputSchema + } + static description = this.descriptionForHelp() static flags = { @@ -28,6 +33,7 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de multiple: true, }), ...appSecurityBlockingFlag, + ...jsonFlag, } public async run(): Promise { @@ -41,6 +47,6 @@ Reports the number of findings to Shopify as usage telemetry, but not finding de checkIds: flags['check-id'] ?? [], blocking: flags.blocking, }) - renderSecurityReviewResult(review, {verbose: Boolean(flags.verbose)}) + renderSecurityReviewResult(review, {verbose: Boolean(flags.verbose)}, flags.json ? 'json' : 'text') } } diff --git a/packages/app/src/cli/services/app-security-json-output.test.ts b/packages/app/src/cli/services/app-security-json-output.test.ts index 278bfef286a..f8ecce4c221 100644 --- a/packages/app/src/cli/services/app-security-json-output.test.ts +++ b/packages/app/src/cli/services/app-security-json-output.test.ts @@ -1,9 +1,16 @@ import { appSecurityAppSchema, + appSecurityBlockingSchema, appSecurityInstructionsSchema, + appSecurityScanDirectorySchema, + appSecurityScopeSchema, projectAppSecurityApp, + projectAppSecurityBlocking, projectAppSecurityInstructions, projectAppSecurityPath, + projectAppSecurityRelativePath, + projectAppSecurityScanDirectory, + projectAppSecurityScope, } from './app-security-json-output.js' import {joinPath} from '@shopify/cli-kit/node/path' import {describe, expect, test} from 'vitest' @@ -30,6 +37,21 @@ describe('projectAppSecurityPath', () => { }) }) +describe('projectAppSecurityRelativePath', () => { + test.each([ + [ + 'a file in the app directory', + 'app/routes/orders.tsx', + onWindows ? 'C:\\apps\\my-app\\app\\routes\\orders.tsx' : '/apps/my-app/app/routes/orders.tsx', + ], + ['the app directory', '.', appDirectory], + ['a directory beside the app directory', '../backend', onWindows ? 'C:\\apps\\backend' : '/apps/backend'], + ['an absolute path', onWindows ? 'D:/shared' : '/shared', onWindows ? 'D:\\shared' : '/shared'], + ])('resolves %s against the app directory, with native separators', (_description, path, expected) => { + expect(projectAppSecurityRelativePath(appDirectory, path)).toBe(expected) + }) +}) + describe('projectAppSecurityApp', () => { test("projects an app configuration with the TOML's client ID", () => { expect(projectAppSecurityApp(configSelection({configClientId: 'toml-client-id'}))).toStrictEqual({ @@ -115,3 +137,93 @@ describe('appSecurityInstructionsSchema', () => { expect(appSecurityInstructionsSchema.safeParse(value).success).toBe(false) }) }) + +describe('projectAppSecurityScope', () => { + test('keeps the values exactly as typed', () => { + expect( + projectAppSecurityScope({include_dirs: ['../backend', 'web/'], excludes: ['vendor/**'], no_git_ignore: true}), + ).toStrictEqual({includeDirs: ['../backend', 'web/'], excludes: ['vendor/**'], noGitIgnore: true}) + }) +}) + +describe('appSecurityScopeSchema', () => { + const scope = {includeDirs: ['../backend'], excludes: [], noGitIgnore: false} + + test('accepts a projected scope', () => { + expect(appSecurityScopeSchema.parse(scope)).toStrictEqual(scope) + }) + + test.each([ + ['an extra field', {...scope, include_dirs: []}], + ['a missing field', {includeDirs: [], excludes: []}], + ])('rejects %s', (_description, value) => { + expect(appSecurityScopeSchema.safeParse(value).success).toBe(false) + }) +}) + +describe('projectAppSecurityScanDirectory', () => { + test.each([ + ['app_directory', '.', 'app-directory', appDirectory], + ['include_dir', '../backend', 'include-dir', onWindows ? 'C:\\apps\\backend' : '/apps/backend'], + [ + 'app_config_directory', + 'extensions/theme', + 'app-config-directory', + onWindows ? 'C:\\apps\\my-app\\extensions\\theme' : '/apps/my-app/extensions/theme', + ], + ] as const)('projects a %s directory', (origin, directory, expectedOrigin, expectedDirectory) => { + expect(projectAppSecurityScanDirectory(appDirectory, {directory, origin})).toStrictEqual({ + directory: expectedDirectory, + origin: expectedOrigin, + }) + }) + + test('keeps an absolute directory', () => { + const directory = onWindows ? 'C:/apps/my-app/web' : '/apps/my-app/web' + + expect(projectAppSecurityScanDirectory(appDirectory, {directory, origin: 'app_config_directory'})).toStrictEqual({ + directory: onWindows ? 'C:\\apps\\my-app\\web' : '/apps/my-app/web', + origin: 'app-config-directory', + }) + }) +}) + +describe('appSecurityScanDirectorySchema', () => { + const scanDirectory = {directory: appDirectory, origin: 'app-directory'} + + test('accepts a projected scan directory', () => { + expect(appSecurityScanDirectorySchema.parse(scanDirectory)).toStrictEqual(scanDirectory) + }) + + test.each([ + ['an extra field', {...scanDirectory, path: appDirectory}], + ['a relative directory', {...scanDirectory, directory: '.'}], + ['a stored origin', {...scanDirectory, origin: 'app_directory'}], + ])('rejects %s', (_description, value) => { + expect(appSecurityScanDirectorySchema.safeParse(value).success).toBe(false) + }) +}) + +describe('projectAppSecurityBlocking', () => { + test.each([ + ['high', 2], + ['none', 0], + ] as const)('projects the %s level and its blocked checks', (level, blockedChecks) => { + expect(projectAppSecurityBlocking({level, blockedChecks})).toStrictEqual({level, blockedCheckCount: blockedChecks}) + }) +}) + +describe('appSecurityBlockingSchema', () => { + test.each(['high', 'medium', 'low', 'none'])('accepts the %s level', (level) => { + expect(appSecurityBlockingSchema.parse({level, blockedCheckCount: 0})).toStrictEqual({level, blockedCheckCount: 0}) + }) + + test.each([ + ['an extra field', {level: 'high', blockedCheckCount: 1, blockedChecks: 1}], + ['an unknown level', {level: 'critical', blockedCheckCount: 1}], + ['a negative count', {level: 'high', blockedCheckCount: -1}], + ['a fractional count', {level: 'high', blockedCheckCount: 0.5}], + ])('rejects %s', (_description, value) => { + expect(appSecurityBlockingSchema.safeParse(value).success).toBe(false) + }) +}) diff --git a/packages/app/src/cli/services/app-security-json-output.ts b/packages/app/src/cli/services/app-security-json-output.ts index fe83235678c..bab6869bee8 100644 --- a/packages/app/src/cli/services/app-security-json-output.ts +++ b/packages/app/src/cli/services/app-security-json-output.ts @@ -1,8 +1,10 @@ import {effectiveClientId, type AppSecuritySelection} from './app-security-selection.js' -import {isAbsolutePath} from '@shopify/cli-kit/node/path' +import {isAbsolutePath, resolvePath} from '@shopify/cli-kit/node/path' import {zod} from '@shopify/cli-kit/node/schema' // eslint-disable-next-line no-restricted-imports -- JSON file paths use native platform separators. import {resolve} from 'node:path' +import type {AppSecurityBlockingLevel} from './app-security-api.js' +import type {AppSecurityScope as StoredAppSecurityScope, CoverageScanDirectory} from './app-security-engine/index.js' import type {AppSecurityInstructionsDelivery} from './app-security-instructions.js' /** @@ -47,11 +49,61 @@ export const appSecurityInstructionsSchema = zod type AppSecurityInstructions = zod.infer +export const appSecurityScopeSchema = zod + .object({ + includeDirs: zod.array(zod.string()).describe('The --include-dir values.'), + excludes: zod.array(zod.string()).describe('The --exclude globs.'), + noGitIgnore: zod.boolean().describe('Whether --no-git-ignore was passed.'), + }) + .strict() + .describe('The check flags that chose which files were gathered, exactly as typed.') + +type AppSecurityScope = zod.infer + +export const appSecurityScanDirectorySchema = zod + .object({ + directory: createAbsolutePathSchema().describe('The absolute path of the directory.'), + origin: zod + .enum(['app-directory', 'include-dir', 'app-config-directory']) + .describe( + 'Why it was scanned: it is the app directory, an --include-dir value, or an extension or web directory of the app configuration.', + ), + }) + .strict() + .describe('A directory that check scanned.') + +type AppSecurityScanDirectory = zod.infer + +const SCAN_DIRECTORY_ORIGINS: Record = { + app_directory: 'app-directory', + include_dir: 'include-dir', + app_config_directory: 'app-config-directory', +} + +export const appSecurityBlockingSchema = zod + .object({ + level: zod.enum(['high', 'medium', 'low', 'none']).describe('The --blocking severity.'), + blockedCheckCount: zod + .number() + .int() + .nonnegative() + .describe('The number of checks with findings at or above that severity. Always 0 for none.'), + }) + .strict() + .describe('The --blocking policy and the checks that fail it.') + +type AppSecurityBlocking = zod.infer + /** CLI Kit joins paths with forward slashes, even on Windows. JSON output shows them absolute, with native separators. */ export function projectAppSecurityPath(path: string): string { return resolve(path) } +/** Projects a path that is relative to the app directory, as results files record them. An absolute path stays as it is. */ +export function projectAppSecurityRelativePath(appDirectory: string, path: string): string { + return projectAppSecurityPath(resolvePath(appDirectory, path)) +} + export function projectAppSecurityApp(selection: AppSecuritySelection): AppSecurityApp { return { directory: projectAppSecurityPath(selection.appDirectory), @@ -67,3 +119,25 @@ export function projectAppSecurityInstructions( ): AppSecurityInstructions { return {content: instructions, copied: delivery.copy} } + +export function projectAppSecurityScope(scope: StoredAppSecurityScope): AppSecurityScope { + return {includeDirs: scope.include_dirs, excludes: scope.excludes, noGitIgnore: scope.no_git_ignore} +} + +/** Projects a scan directory, either absolute or relative to the app directory as results files record them. */ +export function projectAppSecurityScanDirectory( + appDirectory: string, + scanDirectory: CoverageScanDirectory, +): AppSecurityScanDirectory { + return { + directory: projectAppSecurityRelativePath(appDirectory, scanDirectory.directory), + origin: SCAN_DIRECTORY_ORIGINS[scanDirectory.origin], + } +} + +export function projectAppSecurityBlocking(blocking: { + level: AppSecurityBlockingLevel + blockedChecks: number +}): AppSecurityBlocking { + return {level: blocking.level, blockedCheckCount: blocking.blockedChecks} +} diff --git a/packages/app/src/cli/services/app-security-results.test.ts b/packages/app/src/cli/services/app-security-results.test.ts index 9911b14e85d..4cd252b4644 100644 --- a/packages/app/src/cli/services/app-security-results.test.ts +++ b/packages/app/src/cli/services/app-security-results.test.ts @@ -12,6 +12,8 @@ import {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import {joinPath} from '@shopify/cli-kit/node/path' import {describe, expect, test} from 'vitest' +// eslint-disable-next-line no-restricted-imports -- Verify native filesystem paths in JSON error details. +import {resolve} from 'node:path' import type {DeterministicFindingsDocument} from './app-security-engine/index.js' type FileState = 'present' | 'missing' | 'invalid' @@ -247,6 +249,10 @@ describe('loadAppSecurityResults', () => { {title: invalidPath, body: {list: {items: [expect.stringContaining('Could not parse JSON')]}}}, ]) expect(JSON.stringify(error.customSections)).not.toContain(validPath) + expect(error.code).toBe('results-file-invalid') + expect(error.details).toStrictEqual({ + files: [{path: resolve(invalidPath), errors: [expect.stringContaining('Could not parse JSON')]}], + }) }) }, ) @@ -279,6 +285,16 @@ describe('loadAppSecurityResults', () => { }, {title: paths.agentFindingsPath, body: {list: {items: [expect.stringContaining('Could not parse JSON')]}}}, ]) + expect(error.code).toBe('results-file-invalid') + expect(error.details).toStrictEqual({ + files: [ + { + path: resolve(paths.deterministicFindingsPath), + errors: ['source is "agent", but this file must hold "deterministic" findings.'], + }, + {path: resolve(paths.agentFindingsPath), errors: [expect.stringContaining('Could not parse JSON')]}, + ], + }) }) }) diff --git a/packages/app/src/cli/services/app-security-results.ts b/packages/app/src/cli/services/app-security-results.ts index 7c0f325f36f..2140430dc58 100644 --- a/packages/app/src/cli/services/app-security-results.ts +++ b/packages/app/src/cli/services/app-security-results.ts @@ -10,6 +10,7 @@ import { resolveAppSecurityCommands, type AppSecurityCommands, } from './app-security-commands.js' +import {projectAppSecurityPath} from './app-security-json-output.js' import {resultsKey, type AppSecuritySelection} from './app-security-selection.js' import { combineFindings, @@ -107,7 +108,8 @@ function presentSource(loaded: LoadedSource): {path: strin /** * Aborts with one error for every invalid file: a next step per file with the command that regenerates it, `clean` as - * the way to start over, and a section per file listing the reader's messages under the file's path. + * the way to start over, and a section per file listing the reader's messages under the file's path. `details.files` + * repeats each file's absolute path and messages for JSON output. */ function abortInvalidResults(invalidFiles: InvalidResultsFile[], commands: AppSecurityCommands): never { const message = @@ -118,7 +120,10 @@ function abortInvalidResults(invalidFiles: InvalidResultsFile[], commands: AppSe ...invalidFiles.map((file) => regenerateResultsFileStep(file.source, commands, `${basename(file.path)}.`)), ['Or run', {command: formatAppSecurityCommand(commands.clean)}, 'to delete both results files and start over.'], ] - throw new AbortError(message, null, nextSteps, invalidFiles.map(invalidFileSection)) + const error = new AbortError(message, null, nextSteps, invalidFiles.map(invalidFileSection)) + error.code = 'results-file-invalid' + error.details = {files: invalidFiles.map(({path, errors}) => ({path: projectAppSecurityPath(path), errors}))} + throw error } /** diff --git a/packages/app/src/cli/services/security-review.test.ts b/packages/app/src/cli/services/security-review.test.ts index 278a9a9e813..23f2be4283b 100644 --- a/packages/app/src/cli/services/security-review.test.ts +++ b/packages/app/src/cli/services/security-review.test.ts @@ -134,6 +134,8 @@ describe('reviewAppSecurityResults', () => { ) expect(abort.message).toBe('Unknown check ID: NOT_A_CHECK.') + expect(abort.code).toBe('unknown-check-id') + expect(abort.details).toStrictEqual({checkIds: ['NOT_A_CHECK']}) expect(abort.customSections).toEqual([ { title: 'Available check IDs', @@ -151,8 +153,11 @@ describe('reviewAppSecurityResults', () => { ]) }) - test('pluralizes the error for several unknown IDs', () => { - expect(() => review(both, {checkIds: ['A', 'B']})).toThrow('Unknown check IDs: A, B.') + test('pluralizes the error for several unknown IDs, and lists only those in its details', async () => { + const abort = await captureError(() => review(both, {checkIds: ['A', 'OPEN_REDIRECT', 'B']})) + + expect(abort.message).toBe('Unknown check IDs: A, B.') + expect(abort.details).toStrictEqual({checkIds: ['A', 'B']}) }) test('skips the unknown-ID check when no file is present, echoing the filter with no checks', () => { @@ -248,12 +253,17 @@ describe('securityReview', () => { }) }) - test('returns the reviewed results and the commands to run next', async () => { + test('returns the selection, the reviewed results and the commands to run next', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = await createApp(directory, both) const review = await securityReview({directory: appRoot, checkIds: [], blocking: 'none'}, testDependencies()) + expect(review.selection).toStrictEqual({ + kind: 'config', + appDirectory: appRoot, + appConfigFilePath: joinPath(appRoot, 'shopify.app.toml'), + }) expect(review.commands.scan.args).toContainEqual({flag: '--path', value: relativePath(cwd(), appRoot)}) expect(review.result.resultsDirectory).toBe(appSecurityArtifactPaths(appRoot, RESULTS_KEY).resultsDirectory) expect(review.result.sources.deterministic?.path).toBe( diff --git a/packages/app/src/cli/services/security-review.ts b/packages/app/src/cli/services/security-review.ts index a89fdd41091..e0019bd8a56 100644 --- a/packages/app/src/cli/services/security-review.ts +++ b/packages/app/src/cli/services/security-review.ts @@ -40,8 +40,9 @@ export interface SecurityReviewResult { } } -/** What `securityReview` returns: the reviewed results, and the commands that the review suggests running next. */ +/** What `securityReview` returns: the selected app, its reviewed results, and the commands to run next. */ export interface ReviewedAppSecurityResults { + selection: AppSecuritySelection result: SecurityReviewResult commands: AppSecurityCommands } @@ -107,12 +108,15 @@ function assertKnownCheckIds(checkIds: string[], checks: CombinedCheck[]): void if (unknownIds.length === 0) return const label = unknownIds.length === 1 ? 'check ID' : 'check IDs' - throw new AbortError( + const error = new AbortError( `Unknown ${label}: ${unknownIds.join(', ')}.`, 'Pass --check-id with one of the checks in the results files.', [], [{title: 'Available check IDs', body: {list: {items: checks.map((check) => check.id)}}}], ) + error.code = 'unknown-check-id' + error.details = {checkIds: unknownIds} + throw error } /** @@ -147,6 +151,7 @@ export default async function securityReview( } return { + selection, result, // The latest scan's scope, so running `check` again gathers the same files. commands: resolveAppSecurityCommands( diff --git a/packages/app/src/cli/services/security-review/result.test.ts b/packages/app/src/cli/services/security-review/result.test.ts index 3b217c16a5c..d2188139768 100644 --- a/packages/app/src/cli/services/security-review/result.test.ts +++ b/packages/app/src/cli/services/security-review/result.test.ts @@ -2,52 +2,152 @@ import {renderSecurityReviewResult} from './result.js' import {reviewAppSecurityResults, type ReviewedAppSecurityResults} from '../security-review.js' import {appSecurityArtifactPaths} from '../app-security-artifacts.js' import {resolveAppSecurityCommands} from '../app-security-commands.js' -import {appSecurityResultsFor} from '../app-security-results.test-data.js' +import {appSecurityResultsFor, type AppSecurityResultsSources} from '../app-security-results.test-data.js' import { agentFindingsDocument, deterministicFindingsDocument, } from '../app-security-engine/tests/fixtures/findings-documents.js' import {unstyled} from '@shopify/cli-kit/node/output' import {cwd} from '@shopify/cli-kit/node/path' -import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' +import {mockAndCaptureOutput, withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' import {afterEach, describe, expect, test} from 'vitest' import type {AppSecurityBlockingLevel} from '../app-security-api.js' import type {AppSecuritySelection} from '../app-security-selection.js' +import type { + AgentFindingsDocument, + DeterministicFindingsDocument, + StoredCheck, + StoredFinding, +} from '../app-security-engine/index.js' const originalExitCode = process.exitCode afterEach(() => { process.exitCode = originalExitCode }) -const appRoot = '/tmp/review-app' +// 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 appDirectory = onWindows ? 'C:\\apps\\my-app' : '/apps/my-app' + +/** The absolute native path of a file in the app directory, written out independently of the codec. */ +function appPath(relativePath: string): string { + return onWindows ? `C:\\apps\\my-app\\${relativePath.replaceAll('/', '\\')}` : `/apps/my-app/${relativePath}` +} + +const resultsDirectory = appPath('.shopify/app-security/shopify.app') + const selection: AppSecuritySelection = { kind: 'config', - appDirectory: appRoot, - appConfigFilePath: `${appRoot}/shopify.app.toml`, + appDirectory, + appConfigFilePath: `${appDirectory}/shopify.app.toml`, + configClientId: 'toml-client-id', } -function reviewed(options: {blocking?: AppSecurityBlockingLevel} = {}): ReviewedAppSecurityResults { - const results = appSecurityResultsFor(appRoot, 'shopify.app', { - deterministic: deterministicFindingsDocument, - agent: agentFindingsDocument, - }) +interface ReviewOptions { + checkIds?: string[] + blocking?: AppSecurityBlockingLevel +} + +function reviewed(sources: AppSecurityResultsSources, options: ReviewOptions = {}): ReviewedAppSecurityResults { return { - result: reviewAppSecurityResults(results, { - resultsDirectory: appSecurityArtifactPaths(appRoot, 'shopify.app').resultsDirectory, - checkIds: [], + selection, + result: reviewAppSecurityResults(appSecurityResultsFor(appDirectory, 'shopify.app', sources), { + resultsDirectory: appSecurityArtifactPaths(appDirectory, 'shopify.app').resultsDirectory, + checkIds: options.checkIds ?? [], blocking: options.blocking ?? 'none', }), commands: resolveAppSecurityCommands(selection, cwd()), } } -describe('renderSecurityReviewResult', () => { - test('renders the review in the terminal and leaves the exit code alone', () => { +/** Presents the review as JSON and returns the parsed stdout, after checking that stderr is empty. */ +async function jsonResult( + sources: AppSecurityResultsSources, + options: ReviewOptions & {verbose?: boolean} = {}, +): Promise { + return withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityReviewResult(reviewed(sources, options), {verbose: options.verbose ?? false}, 'json') + expect(stderr()).toBe('') + return JSON.parse(stdout()) + }) +} + +const snapshot = { + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + current_version: 2, +} as const + +function deterministicCheck(overrides: Partial = {}): StoredCheck { + return { + id: 'CREDENTIAL_LOG_LEAKAGE', + version: 2, + status: 'executed', + analysis_mode: 'ast', + snapshot, + findings: [], + ...overrides, + } +} + +function agentCheck(overrides: Partial = {}): StoredCheck { + return { + id: 'CREDENTIAL_LOG_LEAKAGE', + version: 2, + status: 'executed', + snapshot: {...snapshot, precedence: 'union'}, + findings: [], + ...overrides, + } +} + +function deterministicDocument(overrides: Partial = {}): DeterministicFindingsDocument { + return { + schema_version: 1, + source: 'deterministic', + engine: {name: 'shopify-app-security', version: '4.9.0', ruleset: 'app-security-rules@4.9.0'}, + generated_at: '2026-09-01T10:00:00.000Z', + detection: {framework: 'react_router', surface: 'react_router', languages: []}, + coverage: { + files_scanned: 2, + files_skipped: [], + gaps: [], + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + scan_directories: [{directory: '.', origin: 'app_directory'}], + }, + checks: [deterministicCheck()], + ...overrides, + } +} + +function agentDocument(overrides: Partial = {}): AgentFindingsDocument { + return { + schema_version: 1, + source: 'agent', + engine: {name: 'shopify-app-security', version: '4.9.0'}, + generated_at: '2026-09-01T11:00:00.000Z', + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + checks: [agentCheck()], + ...overrides, + } +} + +/** The first check of a JSON result. */ +async function firstCheck(sources: AppSecurityResultsSources): Promise> { + const result = (await jsonResult(sources)) as {checks: Record[]} + return result.checks[0]! +} + +describe('renderSecurityReviewResult in the terminal', () => { + const both = {deterministic: deterministicFindingsDocument, agent: agentFindingsDocument} + + test('renders the review and leaves the exit code alone', () => { process.exitCode = undefined const output = mockAndCaptureOutput() output.clear() - renderSecurityReviewResult(reviewed(), {verbose: false}) + renderSecurityReviewResult(reviewed(both), {verbose: false}, 'text') const rendered = unstyled(output.output()) expect(rendered).toContain('3 checks with findings. 1 check unresolved.') @@ -60,7 +160,7 @@ describe('renderSecurityReviewResult', () => { const output = mockAndCaptureOutput() output.clear() - renderSecurityReviewResult(reviewed(), {verbose: true}) + renderSecurityReviewResult(reviewed(both), {verbose: true}, 'text') expect(unstyled(output.output())).toContain('Suppressed: The route is restricted to the app owner') }) @@ -70,9 +170,627 @@ describe('renderSecurityReviewResult', () => { const output = mockAndCaptureOutput() output.clear() - renderSecurityReviewResult(reviewed({blocking: 'high'}), {verbose: false}) + renderSecurityReviewResult(reviewed(both, {blocking: 'high'}), {verbose: false}, 'text') expect(unstyled(output.error())).toContain('2 checks at or above high (--blocking high).') expect(process.exitCode).toBe(1) }) }) + +describe('renderSecurityReviewResult as JSON', () => { + test('writes one result with both sources, each finding with only its own fields', async () => { + const docsUrl = 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage' + const deterministic = deterministicDocument({ + generated_at: '2026-09-01T10:00:00.789Z', + detection: { + framework: 'react_router', + surface: 'react_router', + languages: [{name: 'typescript', support: 'supported', files: ['app/routes/orders.tsx']}], + }, + coverage: { + files_scanned: 12, + files_skipped: [{path: 'public/vendor.min.js', reason: 'too_large', size_bytes: 2_500_000}], + gaps: [ + {code: 'unresolved_check', message: 'The parser was unavailable.', check_id: 'MISSING_TENANT_ISOLATION'}, + ], + scope: {include_dirs: ['../backend'], excludes: ['vendor/**'], no_git_ignore: true}, + scan_directories: [ + {directory: '.', origin: 'app_directory'}, + {directory: '../backend', origin: 'include_dir'}, + ], + }, + checks: [ + deterministicCheck({ + snapshot: {...snapshot, docs_url: docsUrl}, + findings: [ + { + location: {file: 'app/routes/orders.tsx', line: 12, column: 5}, + message: 'accessToken flows into console.log.', + evidence: [ + {location: {file: 'app/routes/orders.tsx', line: 12}, quote: 'console.log(session.accessToken)'}, + ], + snippet: 'console.log(session.accessToken)', + fix: {automated: false, description: 'Remove credentials from logs.', guide: 'https://shopify.dev/logs'}, + // The stored format allows another source's fields; a deterministic finding doesn't show them. + confidence: 'high', + reasoning: 'Not shown.', + suppression: {justification: 'Not shown.'}, + }, + ], + }), + ], + }) + const agent = agentDocument({ + generated_at: '2026-09-01T11:00:00.999Z', + scope: {include_dirs: [], excludes: [], no_git_ignore: false}, + checks: [ + agentCheck({ + snapshot: {...snapshot, docs_url: docsUrl, precedence: 'prefer-agent'}, + findings: [ + { + location: {file: 'app/routes/orders.tsx', line: 12}, + message: 'The session access token is written to the server log.', + evidence: [{location: {file: 'app/routes/orders.tsx', line: 12}}], + confidence: 'high', + reasoning: 'The session holds a live credential.', + fix: {automated: true, description: 'Not shown.'}, + }, + ], + }), + agentCheck({ + id: 'MISSING_TENANT_ISOLATION', + version: 4, + snapshot: { + title: 'Database query may not be scoped by shop', + severity: 'high', + description: "Detects database queries that don't reference the shop.", + guide: 'https://shopify.dev/docs/apps/build/authentication-authorization/session-tokens', + current_version: 4, + precedence: 'union', + }, + findings: [ + { + location: {file: 'app/routes/admin.settings.tsx', line: 55}, + message: 'Settings are deleted by ID without checking the owning shop.', + evidence: [], + suppression: {justification: 'An allowlist middleware restricts the route to the app owner.'}, + }, + ], + }), + ], + }) + + await expect(jsonResult({deterministic, agent})).resolves.toStrictEqual({ + status: 'success', + valid: true, + app: { + directory: appDirectory, + configurationPath: appPath('shopify.app.toml'), + clientId: 'toml-client-id', + }, + resultsDirectory, + resultsFiles: { + deterministic: { + path: appPath('.shopify/app-security/shopify.app/deterministic-findings.json'), + generatedAt: '2026-09-01T10:00:00Z', + engine: {name: 'shopify-app-security', version: '4.9.0', ruleset: 'app-security-rules@4.9.0'}, + scope: {includeDirs: ['../backend'], excludes: ['vendor/**'], noGitIgnore: true}, + }, + agent: { + path: appPath('.shopify/app-security/shopify.app/agent-findings.json'), + generatedAt: '2026-09-01T11:00:00Z', + engine: {name: 'shopify-app-security', version: '4.9.0'}, + scope: {includeDirs: [], excludes: [], noGitIgnore: false}, + }, + }, + detection: { + framework: 'react-router', + surface: 'react-router', + languages: [{name: 'typescript', support: 'supported', files: [{path: appPath('app/routes/orders.tsx')}]}], + }, + coverage: { + filesScanned: 12, + filesSkipped: [ + {path: appPath('public/vendor.min.js'), reason: 'too-large', sizeBytes: 2_500_000, detail: null}, + ], + gaps: [ + { + code: 'unresolved-check', + message: 'The parser was unavailable.', + checkId: 'MISSING_TENANT_ISOLATION', + filePath: null, + }, + ], + scanDirectories: [ + {directory: appDirectory, origin: 'app-directory'}, + {directory: onWindows ? 'C:\\apps\\backend' : '/apps/backend', origin: 'include-dir'}, + ], + }, + scopeDiffers: true, + checks: [ + { + id: 'CREDENTIAL_LOG_LEAKAGE', + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + guide: null, + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage', + precedence: 'prefer-agent', + appliedPrecedence: 'prefer-agent', + status: 'executed', + bySource: { + deterministic: { + version: 2, + status: 'executed', + reason: null, + analysisMode: 'ast', + generatedAt: '2026-09-01T10:00:00Z', + }, + agent: { + version: 2, + status: 'executed', + reason: null, + analysisMode: null, + generatedAt: '2026-09-01T11:00:00Z', + }, + }, + findings: [ + { + source: 'deterministic', + disposition: 'superseded', + location: {filePath: appPath('app/routes/orders.tsx'), line: 12, column: 5}, + message: 'accessToken flows into console.log.', + snippet: 'console.log(session.accessToken)', + evidence: [ + { + location: {filePath: appPath('app/routes/orders.tsx'), line: 12, column: null}, + quote: 'console.log(session.accessToken)', + }, + ], + fix: {description: 'Remove credentials from logs.', guide: 'https://shopify.dev/logs', automated: false}, + }, + { + source: 'agent', + disposition: 'active', + location: {filePath: appPath('app/routes/orders.tsx'), line: 12, column: null}, + message: 'The session access token is written to the server log.', + snippet: null, + evidence: [{location: {filePath: appPath('app/routes/orders.tsx'), line: 12, column: null}, quote: null}], + confidence: 'high', + reasoning: 'The session holds a live credential.', + suppression: null, + }, + ], + }, + { + id: 'MISSING_TENANT_ISOLATION', + title: 'Database query may not be scoped by shop', + severity: 'high', + description: "Detects database queries that don't reference the shop.", + guide: 'https://shopify.dev/docs/apps/build/authentication-authorization/session-tokens', + docsUrl: null, + precedence: 'union', + appliedPrecedence: 'union', + status: 'executed', + bySource: { + deterministic: null, + agent: { + version: 4, + status: 'executed', + reason: null, + analysisMode: null, + generatedAt: '2026-09-01T11:00:00Z', + }, + }, + findings: [ + { + source: 'agent', + disposition: 'suppressed', + location: {filePath: appPath('app/routes/admin.settings.tsx'), line: 55, column: null}, + message: 'Settings are deleted by ID without checking the owning shop.', + snippet: null, + evidence: [], + confidence: null, + reasoning: null, + suppression: {justification: 'An allowlist middleware restricts the route to the app owner.'}, + }, + ], + }, + ], + blocking: {level: 'none', blockedCheckCount: 0}, + }) + }) + + test('writes null and empty values when neither results file exists', async () => { + await expect(jsonResult({deterministic: null, agent: null})).resolves.toStrictEqual({ + status: 'success', + valid: true, + app: {directory: appDirectory, configurationPath: appPath('shopify.app.toml'), clientId: 'toml-client-id'}, + resultsDirectory, + resultsFiles: {deterministic: null, agent: null}, + detection: null, + coverage: null, + scopeDiffers: false, + checks: [], + blocking: {level: 'none', blockedCheckCount: 0}, + }) + }) + + test('writes null for the optional values that a results file leaves out', async () => { + const deterministic = deterministicDocument({ + coverage: { + ...deterministicDocument().coverage, + files_skipped: [{path: 'secrets.bin', reason: 'unreadable'}], + gaps: [{code: 'unsupported_language', message: 'Ruby is not supported.'}], + }, + checks: [ + deterministicCheck({ + status: 'not_applicable', + analysis_mode: undefined, + findings: [{location: {file: 'shopify.app.toml'}, message: 'Compliance webhooks are missing.', evidence: []}], + }), + ], + }) + + const result = (await jsonResult({deterministic, agent: null})) as { + coverage: unknown + checks: Record[] + } + + expect(result.coverage).toMatchObject({ + filesSkipped: [{path: appPath('secrets.bin'), reason: 'unreadable', sizeBytes: null, detail: null}], + gaps: [{code: 'unsupported-language', message: 'Ruby is not supported.', checkId: null, filePath: null}], + }) + expect(result.checks[0]).toMatchObject({ + guide: null, + docsUrl: null, + bySource: { + deterministic: {version: 2, status: 'not-applicable', reason: null, analysisMode: null}, + agent: null, + }, + findings: [ + { + source: 'deterministic', + disposition: 'active', + location: {filePath: appPath('shopify.app.toml'), line: null, column: null}, + message: 'Compliance webhooks are missing.', + snippet: null, + evidence: [], + fix: null, + }, + ], + }) + }) + + // The text view shows neither a missing nor an empty optional text, so JSON output writes null for both. + const finding: StoredFinding = { + location: {file: 'app/routes/orders.tsx', line: 12}, + message: 'accessToken flows into console.log.', + evidence: [{location: {file: 'app/routes/orders.tsx', line: 12}}], + } + const withDeterministicFinding = (overrides: Partial): AppSecurityResultsSources => ({ + deterministic: deterministicDocument({checks: [deterministicCheck({findings: [{...finding, ...overrides}]})]}), + agent: null, + }) + const optionalTexts = [ + [ + 'finding snippet', + (text: string) => withDeterministicFinding({snippet: text}), + (result: any) => result.checks[0].findings[0].snippet, + ], + [ + 'evidence quote', + (text: string) => withDeterministicFinding({evidence: [{...finding.evidence[0]!, quote: text}]}), + (result: any) => result.checks[0].findings[0].evidence[0].quote, + ], + [ + 'fix guide', + (text: string) => withDeterministicFinding({fix: {automated: false, description: 'Remove it.', guide: text}}), + (result: any) => result.checks[0].findings[0].fix.guide, + ], + [ + 'agent reasoning', + (text: string) => ({ + deterministic: null, + agent: agentDocument({checks: [agentCheck({findings: [{...finding, reasoning: text}]})]}), + }), + (result: any) => result.checks[0].findings[0].reasoning, + ], + [ + 'check guide', + (text: string) => ({ + deterministic: deterministicDocument({checks: [deterministicCheck({snapshot: {...snapshot, guide: text}})]}), + agent: null, + }), + (result: any) => result.checks[0].guide, + ], + [ + 'skipped file detail', + (text: string) => ({ + deterministic: deterministicDocument({ + coverage: { + ...deterministicDocument().coverage, + files_skipped: [{path: 'secrets.bin', reason: 'unreadable', detail: text}], + }, + }), + agent: null, + }), + (result: any) => result.coverage.filesSkipped[0].detail, + ], + ] as const + + test.each(optionalTexts)('writes an empty %s as null', async (_field, sources, read) => { + expect(read(await jsonResult(sources('')))).toBeNull() + }) + + test.each(optionalTexts)('keeps a non-empty %s as recorded, even whitespace', async (_field, sources, read) => { + expect(read(await jsonResult(sources('A value.')))).toBe('A value.') + expect(read(await jsonResult(sources(' ')))).toBe(' ') + }) + + test('writes lines and columns one-based, as recorded', async () => { + const finding: StoredFinding = { + location: {file: 'app/routes/orders.tsx', line: 1, column: 1}, + message: 'accessToken flows into console.log.', + evidence: [{location: {file: 'app/shopify.server.ts', line: 40, column: 7}, quote: 'logger.info(apiSecretKey)'}], + } + const deterministic = deterministicDocument({checks: [deterministicCheck({findings: [finding]})]}) + + const check = await firstCheck({deterministic, agent: null}) + + expect(check.findings).toMatchObject([ + { + location: {filePath: appPath('app/routes/orders.tsx'), line: 1, column: 1}, + evidence: [{location: {filePath: appPath('app/shopify.server.ts'), line: 40, column: 7}}], + }, + ]) + }) + + test.each([ + ['truncates fractional seconds without rounding', '2026-09-01T10:00:59.999Z', '2026-09-01T10:00:59Z'], + ['converts an offset to UTC', '2026-09-01T12:00:00+02:00', '2026-09-01T10:00:00Z'], + ['writes null for a time that does not parse', 'yesterday', null], + ['writes null for a year that does not fit four digits', '+010000-01-01T00:00:00Z', null], + ])('%s', async (_description, generatedAt, expected) => { + const deterministic = deterministicDocument({generated_at: generatedAt}) + + const result = (await jsonResult({deterministic, agent: null})) as { + resultsFiles: {deterministic: {generatedAt: unknown}} + checks: {bySource: {deterministic: {generatedAt: unknown}}}[] + } + + expect(result.resultsFiles.deterministic.generatedAt).toBe(expected) + expect(result.checks[0]!.bySource.deterministic.generatedAt).toBe(expected) + }) + + test.each([ + ['high', 'high'], + ['medium', 'medium'], + ['low', 'low'], + ] as const)('maps severity %s to %s', async (severity, expected) => { + const deterministic = deterministicDocument({checks: [deterministicCheck({snapshot: {...snapshot, severity}})]}) + + await expect(firstCheck({deterministic, agent: null})).resolves.toMatchObject({severity: expected}) + }) + + test.each([ + ['executed', 'executed'], + ['not_applicable', 'not-applicable'], + ['unresolved', 'unresolved'], + ] as const)('maps check status %s to %s', async (status, expected) => { + const deterministic = deterministicDocument({checks: [deterministicCheck({status})]}) + + await expect(firstCheck({deterministic, agent: null})).resolves.toMatchObject({ + status: expected, + bySource: {deterministic: {status: expected}}, + }) + }) + + test.each([ + ['union', 'union'], + ['prefer-agent', 'prefer-agent'], + ] as const)('maps precedence %s to %s', async (precedence, expected) => { + const agent = agentDocument({checks: [agentCheck({snapshot: {...snapshot, precedence}})]}) + + await expect(firstCheck({deterministic: null, agent})).resolves.toMatchObject({ + precedence: expected, + appliedPrecedence: expected, + }) + }) + + test('maps the precedence that applied separately when a prefer-agent agent result is stale', async () => { + const deterministic = deterministicDocument({generated_at: '2026-09-01T12:00:00.000Z'}) + const agent = agentDocument({checks: [agentCheck({snapshot: {...snapshot, precedence: 'prefer-agent'}})]}) + + await expect(firstCheck({deterministic, agent})).resolves.toMatchObject({ + precedence: 'prefer-agent', + appliedPrecedence: 'union', + }) + }) + + test.each([ + ['regex', 'regex'], + ['structured_config', 'structured-config'], + ['ast', 'ast'], + ] as const)('maps analysis mode %s to %s', async (mode, expected) => { + const deterministic = deterministicDocument({checks: [deterministicCheck({analysis_mode: mode})]}) + + await expect(firstCheck({deterministic, agent: null})).resolves.toMatchObject({ + bySource: {deterministic: {analysisMode: expected}}, + }) + }) + + test.each([ + ['high', 'high'], + ['medium', 'medium'], + ['low', 'low'], + ] as const)('maps agent confidence %s to %s', async (confidence, expected) => { + const finding: StoredFinding = { + location: {file: 'app/routes/orders.tsx', line: 3}, + message: 'A finding.', + evidence: [], + confidence, + } + const agent = agentDocument({checks: [agentCheck({findings: [finding]})]}) + + await expect(firstCheck({deterministic: null, agent})).resolves.toMatchObject({ + findings: [{source: 'agent', confidence: expected}], + }) + }) + + test('maps every disposition', async () => { + const finding = (message: string): StoredFinding => ({ + location: {file: 'app/routes/orders.tsx', line: 3}, + message, + evidence: [], + }) + const deterministic = deterministicDocument({checks: [deterministicCheck({findings: [finding('Superseded.')]})]}) + const agent = agentDocument({ + checks: [ + agentCheck({ + snapshot: {...snapshot, precedence: 'prefer-agent'}, + findings: [finding('Active.'), {...finding('Suppressed.'), suppression: {justification: 'Accepted risk.'}}], + }), + ], + }) + + const check = await firstCheck({deterministic, agent}) + + expect(check.findings).toMatchObject([ + {source: 'deterministic', message: 'Superseded.', disposition: 'superseded'}, + {source: 'agent', message: 'Active.', disposition: 'active'}, + {source: 'agent', message: 'Suppressed.', disposition: 'suppressed'}, + ]) + }) + + test.each([ + ['react_router', 'react-router'], + ['none', 'none'], + ['unknown', 'unknown'], + ['mixed', 'mixed'], + ] as const)('maps framework %s to %s', async (framework, expected) => { + const deterministic = deterministicDocument({detection: {framework, surface: 'unknown', languages: []}}) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({detection: {framework: expected}}) + }) + + test.each([ + ['react_router', 'react-router'], + ['theme_app_extension', 'theme-app-extension'], + ['config_only', 'config-only'], + ['unknown', 'unknown'], + ['mixed', 'mixed'], + ] as const)('maps surface %s to %s', async (surface, expected) => { + const deterministic = deterministicDocument({detection: {framework: 'none', surface, languages: []}}) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({detection: {surface: expected}}) + }) + + test.each([ + ['supported', 'supported'], + ['unsupported', 'unsupported'], + ] as const)('maps language support %s to %s', async (support, expected) => { + const deterministic = deterministicDocument({ + detection: {framework: 'none', surface: 'unknown', languages: [{name: 'ruby', support, files: ['Gemfile']}]}, + }) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({ + detection: {languages: [{name: 'ruby', support: expected, files: [{path: appPath('Gemfile')}]}]}, + }) + }) + + test.each([ + ['too_large', 'too-large'], + ['unreadable', 'unreadable'], + ] as const)('maps skipped file reason %s to %s', async (reason, expected) => { + const deterministic = deterministicDocument({ + coverage: {...deterministicDocument().coverage, files_skipped: [{path: 'big.js', reason}]}, + }) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({ + coverage: {filesSkipped: [{reason: expected}]}, + }) + }) + + test.each([ + ['skipped_file', 'skipped-file'], + ['unsupported_framework', 'unsupported-framework'], + ['unsupported_language', 'unsupported-language'], + ['unresolved_check', 'unresolved-check'], + ] as const)('maps coverage gap code %s to %s', async (code, expected) => { + const deterministic = deterministicDocument({ + coverage: {...deterministicDocument().coverage, gaps: [{code, message: 'A gap.', file: 'app/routes/orders.tsx'}]}, + }) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({ + coverage: { + gaps: [{code: expected, message: 'A gap.', checkId: null, filePath: appPath('app/routes/orders.tsx')}], + }, + }) + }) + + test.each([ + ['app_directory', 'app-directory', '.', appDirectory], + ['include_dir', 'include-dir', 'packages/backend', appPath('packages/backend')], + ['app_config_directory', 'app-config-directory', 'extensions/theme', appPath('extensions/theme')], + ] as const)('maps scan directory origin %s to %s', async (origin, expectedOrigin, directory, expectedDirectory) => { + const deterministic = deterministicDocument({ + coverage: {...deterministicDocument().coverage, scan_directories: [{directory, origin}]}, + }) + + await expect(jsonResult({deterministic, agent: null})).resolves.toMatchObject({ + coverage: {scanDirectories: [{directory: expectedDirectory, origin: expectedOrigin}]}, + }) + }) + + test('passes reason codes through verbatim', async () => { + const reason = {code: 'needs_runtime', message: 'Route registration happens at runtime.'} + const agent = agentDocument({checks: [agentCheck({status: 'unresolved', reason})]}) + + await expect(firstCheck({deterministic: null, agent})).resolves.toMatchObject({ + bySource: {agent: {status: 'unresolved', reason}}, + }) + }) + + test('narrows the checks and the blocked checks with --check-id', async () => { + const both = {deterministic: deterministicFindingsDocument, agent: agentFindingsDocument} + + const result = await jsonResult(both, {checkIds: ['EOL_API_VERSION'], blocking: 'low'}) + + expect(result).toMatchObject({ + valid: false, + checks: [{id: 'EOL_API_VERSION'}], + blocking: {level: 'low', blockedCheckCount: 1}, + }) + expect((result as {checks: unknown[]}).checks).toHaveLength(1) + }) + + test('is valid with --blocking none whatever the active findings', async () => { + process.exitCode = undefined + const both = {deterministic: deterministicFindingsDocument, agent: agentFindingsDocument} + + await expect(jsonResult(both, {blocking: 'none'})).resolves.toMatchObject({ + valid: true, + blocking: {level: 'none', blockedCheckCount: 0}, + }) + expect(process.exitCode).toBeUndefined() + }) + + test('writes the result, then sets exit code 1 when --blocking is breached', async () => { + process.exitCode = undefined + const both = {deterministic: deterministicFindingsDocument, agent: agentFindingsDocument} + + await withCapturedStandardStreams(({stdout}) => { + renderSecurityReviewResult(reviewed(both, {blocking: 'high'}), {verbose: false}, 'json') + + expect(JSON.parse(stdout())).toMatchObject({valid: false, blocking: {level: 'high', blockedCheckCount: 2}}) + }) + expect(process.exitCode).toBe(1) + }) + + test('writes the same result with --verbose', async () => { + const both = {deterministic: deterministicFindingsDocument, agent: agentFindingsDocument} + + await expect(jsonResult(both, {verbose: true})).resolves.toStrictEqual(await jsonResult(both, {verbose: false})) + }) +}) diff --git a/packages/app/src/cli/services/security-review/result.ts b/packages/app/src/cli/services/security-review/result.ts index 9250356e318..4e2cca07655 100644 --- a/packages/app/src/cli/services/security-review/result.ts +++ b/packages/app/src/cli/services/security-review/result.ts @@ -1,9 +1,277 @@ +import {appSecurityReviewJsonOutputSchema, type AppSecurityReviewResult} from './types.js' +import { + projectAppSecurityApp, + projectAppSecurityBlocking, + projectAppSecurityPath, + projectAppSecurityRelativePath, + projectAppSecurityScanDirectory, + projectAppSecurityScope, +} from '../app-security-json-output.js' import {renderSecurityReview} from '../security-review-output.js' -import type {ReviewedAppSecurityResults} from '../security-review.js' +import {formatJsonOutputTimestamp, jsonOutputTimestampSchema} from '@shopify/cli-kit/node/json-output-schema' +import {outputResult} from '@shopify/cli-kit/node/output' +import type { + CheckPrecedence, + CombinedCheck, + CombinedFinding, + DeterministicFindingsDocument, + Severity, + SourceCheckResult, + StoredCheckStatus, + StoredEvidence, + StoredFinding, +} from '../app-security-engine/index.js' +import type {ReviewedAppSecurityResults, SecurityReviewResult} from '../security-review.js' -/** Presents reviewed results in the terminal, then sets exit code 1 when they breach `--blocking`. */ -export function renderSecurityReviewResult(review: ReviewedAppSecurityResults, options: {verbose: boolean}): void { - renderSecurityReview({result: review.result, verbose: options.verbose, now: new Date(), commands: review.commands}) +type AppSecurityCheck = AppSecurityReviewResult['checks'][number] +type AppSecurityFinding = AppSecurityCheck['findings'][number] +type AppSecurityAgentFinding = Extract +type AppSecuritySourceResult = NonNullable +type AppSecurityLocation = AppSecurityFinding['location'] +type AppSecurityDetection = NonNullable +type AppSecurityCoverage = NonNullable +type AppSecurityResultsFiles = AppSecurityReviewResult['resultsFiles'] + +type StoredDetection = DeterministicFindingsDocument['detection'] +type StoredCoverage = DeterministicFindingsDocument['coverage'] + +/* + * The stored values of each closed enum, mapped to their public values. A new stored value fails to compile until + * it is mapped. + */ +const SEVERITIES: Record = {high: 'high', medium: 'medium', low: 'low'} +const CHECK_STATUSES: Record = { + executed: 'executed', + not_applicable: 'not-applicable', + unresolved: 'unresolved', +} +const PRECEDENCES: Record = { + union: 'union', + 'prefer-agent': 'prefer-agent', +} +const DISPOSITIONS: Record = { + active: 'active', + suppressed: 'suppressed', + superseded: 'superseded', +} +const CONFIDENCES: Record< + NonNullable, + NonNullable +> = {high: 'high', medium: 'medium', low: 'low'} +const ANALYSIS_MODES: Record< + NonNullable, + NonNullable +> = {regex: 'regex', structured_config: 'structured-config', ast: 'ast'} +const FRAMEWORKS: Record = { + react_router: 'react-router', + none: 'none', + unknown: 'unknown', + mixed: 'mixed', +} +const SURFACES: Record = { + react_router: 'react-router', + theme_app_extension: 'theme-app-extension', + config_only: 'config-only', + unknown: 'unknown', + mixed: 'mixed', +} +const LANGUAGE_SUPPORT: Record< + StoredDetection['languages'][number]['support'], + AppSecurityDetection['languages'][number]['support'] +> = {supported: 'supported', unsupported: 'unsupported'} +const SKIPPED_FILE_REASONS: Record< + StoredCoverage['files_skipped'][number]['reason'], + AppSecurityCoverage['filesSkipped'][number]['reason'] +> = {too_large: 'too-large', unreadable: 'unreadable'} +const GAP_CODES: Record = { + skipped_file: 'skipped-file', + unsupported_framework: 'unsupported-framework', + unsupported_language: 'unsupported-language', + unresolved_check: 'unresolved-check', +} + +/** + * Presents reviewed results as JSON, or in the terminal. Either way, sets exit code 1 after the output when they + * breach `--blocking`. + */ +export function renderSecurityReviewResult( + review: ReviewedAppSecurityResults, + options: {verbose: boolean}, + format: 'json' | 'text', +): void { + if (format === 'json') { + outputResult(appSecurityReviewJsonOutputSchema.encode(appSecurityReviewResult(review))) + } else { + renderSecurityReview({result: review.result, verbose: options.verbose, now: new Date(), commands: review.commands}) + } if (review.result.blocking.blockedChecks > 0) process.exitCode = 1 } + +function appSecurityReviewResult({selection, result}: ReviewedAppSecurityResults): AppSecurityReviewResult { + const {appDirectory} = selection + const deterministic = result.sources.deterministic?.document + return { + status: 'success', + valid: result.blocking.blockedChecks === 0, + app: projectAppSecurityApp(selection), + resultsDirectory: projectAppSecurityPath(result.resultsDirectory), + resultsFiles: projectResultsFiles(result.sources), + detection: deterministic ? projectDetection(deterministic.detection, appDirectory) : null, + coverage: deterministic ? projectCoverage(deterministic.coverage, appDirectory) : null, + scopeDiffers: result.scopeDiffers, + checks: result.checks.map((check) => projectCheck(check, appDirectory)), + blocking: projectAppSecurityBlocking(result.blocking), + } +} + +function projectResultsFiles({deterministic, agent}: SecurityReviewResult['sources']): AppSecurityResultsFiles { + return { + deterministic: deterministic + ? { + path: projectAppSecurityPath(deterministic.path), + generatedAt: projectTimestamp(deterministic.document.generated_at), + engine: { + name: deterministic.document.engine.name, + version: deterministic.document.engine.version, + ruleset: deterministic.document.engine.ruleset, + }, + scope: projectAppSecurityScope(deterministic.document.coverage.scope), + } + : null, + agent: agent + ? { + path: projectAppSecurityPath(agent.path), + generatedAt: projectTimestamp(agent.document.generated_at), + engine: {name: agent.document.engine.name, version: agent.document.engine.version}, + scope: projectAppSecurityScope(agent.document.scope), + } + : null, + } +} + +/** + * A recorded time as a public instant, or null when it isn't a valid one: it doesn't parse, or its year doesn't fit + * the four digits of the public format. + */ +function projectTimestamp(time: string): string | null { + const parsed = Date.parse(time) + if (Number.isNaN(parsed)) return null + const timestamp = formatJsonOutputTimestamp(new Date(parsed)) + return jsonOutputTimestampSchema.safeParse(timestamp).success ? timestamp : null +} + +/** An optional text that a results file leaves out or leaves empty: the text view shows neither. */ +function projectOptionalText(text: string | undefined): string | null { + return text === undefined || text === '' ? null : text +} + +/** Lines and columns are one-based in the results files, and JSON output shows them as recorded. */ +function projectPosition(position: number | undefined): number | null { + return position ?? null +} + +function projectLocation(location: StoredFinding['location'], appDirectory: string): AppSecurityLocation { + return { + filePath: projectAppSecurityRelativePath(appDirectory, location.file), + line: projectPosition(location.line), + column: projectPosition(location.column), + } +} + +function projectDetection(detection: StoredDetection, appDirectory: string): AppSecurityDetection { + return { + framework: FRAMEWORKS[detection.framework], + surface: SURFACES[detection.surface], + languages: detection.languages.map((language) => ({ + name: language.name, + support: LANGUAGE_SUPPORT[language.support], + files: language.files.map((file) => ({path: projectAppSecurityRelativePath(appDirectory, file)})), + })), + } +} + +function projectCoverage(coverage: StoredCoverage, appDirectory: string): AppSecurityCoverage { + return { + filesScanned: coverage.files_scanned, + filesSkipped: coverage.files_skipped.map((file) => ({ + path: projectAppSecurityRelativePath(appDirectory, file.path), + reason: SKIPPED_FILE_REASONS[file.reason], + sizeBytes: file.size_bytes ?? null, + detail: projectOptionalText(file.detail), + })), + gaps: coverage.gaps.map((gap) => ({ + code: GAP_CODES[gap.code], + message: gap.message, + checkId: gap.check_id ?? null, + filePath: gap.file === undefined ? null : projectAppSecurityRelativePath(appDirectory, gap.file), + })), + scanDirectories: coverage.scan_directories.map((scanDirectory) => + projectAppSecurityScanDirectory(appDirectory, scanDirectory), + ), + } +} + +function projectCheck(check: CombinedCheck, appDirectory: string): AppSecurityCheck { + return { + id: check.id, + title: check.title, + severity: SEVERITIES[check.severity], + description: check.description, + guide: projectOptionalText(check.guide), + docsUrl: check.docs_url ?? null, + precedence: PRECEDENCES[check.precedence], + appliedPrecedence: PRECEDENCES[check.applied_precedence], + status: CHECK_STATUSES[check.status], + bySource: { + deterministic: check.by_source.deterministic ? projectSourceResult(check.by_source.deterministic) : null, + agent: check.by_source.agent ? projectSourceResult(check.by_source.agent) : null, + }, + findings: check.findings.map((finding) => projectFinding(finding, appDirectory)), + } +} + +function projectSourceResult(result: SourceCheckResult): AppSecuritySourceResult { + return { + version: result.version, + status: CHECK_STATUSES[result.status], + reason: result.reason ? {code: result.reason.code, message: result.reason.message} : null, + analysisMode: result.analysis_mode ? ANALYSIS_MODES[result.analysis_mode] : null, + generatedAt: projectTimestamp(result.generated_at), + } +} + +/** Each source's finding has only its own fields: the stored format allows the other source's too. */ +function projectFinding(finding: CombinedFinding, appDirectory: string): AppSecurityFinding { + const common = { + disposition: DISPOSITIONS[finding.disposition], + location: projectLocation(finding.location, appDirectory), + message: finding.message, + snippet: projectOptionalText(finding.snippet), + evidence: finding.evidence.map((evidence) => projectEvidence(evidence, appDirectory)), + } + if (finding.source === 'deterministic') { + return { + source: 'deterministic', + ...common, + fix: finding.fix + ? { + description: finding.fix.description, + guide: projectOptionalText(finding.fix.guide), + automated: finding.fix.automated, + } + : null, + } + } + return { + source: 'agent', + ...common, + confidence: finding.confidence ? CONFIDENCES[finding.confidence] : null, + reasoning: projectOptionalText(finding.reasoning), + suppression: finding.suppression ? {justification: finding.suppression.justification} : null, + } +} + +function projectEvidence(evidence: StoredEvidence, appDirectory: string): AppSecurityFinding['evidence'][number] { + return {location: projectLocation(evidence.location, appDirectory), quote: projectOptionalText(evidence.quote)} +} diff --git a/packages/app/src/cli/services/security-review/types.test.ts b/packages/app/src/cli/services/security-review/types.test.ts new file mode 100644 index 00000000000..aa74505253e --- /dev/null +++ b/packages/app/src/cli/services/security-review/types.test.ts @@ -0,0 +1,224 @@ +import {appSecurityReviewJsonOutputSchema} 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 ordersPath = onWindows ? 'C:\\apps\\my-app\\app\\routes\\orders.tsx' : '/apps/my-app/app/routes/orders.tsx' +const scope = {includeDirs: [], excludes: [], noGitIgnore: false} + +const location = {filePath: ordersPath, line: 12, column: null} +const deterministicFinding = { + source: 'deterministic', + disposition: 'superseded', + location, + message: 'accessToken flows into console.log.', + snippet: null, + evidence: [{location, quote: 'console.log(session.accessToken)'}], + fix: {description: 'Remove credentials from logs.', guide: null, automated: false}, +} +const agentFinding = { + source: 'agent', + disposition: 'active', + location, + message: 'The session access token is written to the server log.', + snippet: null, + evidence: [], + confidence: 'high', + reasoning: null, + suppression: null, +} +const sourceResult = { + version: 2, + status: 'executed', + reason: null, + analysisMode: 'ast', + generatedAt: '2026-09-01T10:00:00Z', +} +const check = { + id: 'CREDENTIAL_LOG_LEAKAGE', + title: 'Credential reaches a log sink', + severity: 'high', + description: 'Detects direct credential flows to console and logger sinks.', + guide: null, + docsUrl: 'https://shopify.dev/docs/apps/build/security/app-security-checks/credential-log-leakage', + precedence: 'prefer-agent', + appliedPrecedence: 'prefer-agent', + status: 'executed', + bySource: {deterministic: sourceResult, agent: {...sourceResult, analysisMode: null}}, + findings: [deterministicFinding, agentFinding], +} +const result = { + status: 'success', + valid: false, + app: { + directory: appDirectory, + configurationPath: onWindows ? 'C:\\apps\\my-app\\shopify.app.toml' : '/apps/my-app/shopify.app.toml', + clientId: 'toml-client-id', + }, + resultsDirectory, + resultsFiles: { + deterministic: { + path: onWindows + ? `${resultsDirectory}\\deterministic-findings.json` + : `${resultsDirectory}/deterministic-findings.json`, + generatedAt: '2026-09-01T10:00:00Z', + engine: {name: 'shopify-app-security', version: '4.9.0', ruleset: 'app-security-rules@4.9.0'}, + scope, + }, + agent: { + path: onWindows ? `${resultsDirectory}\\agent-findings.json` : `${resultsDirectory}/agent-findings.json`, + generatedAt: null, + engine: {name: 'shopify-app-security', version: '4.9.0'}, + scope, + }, + }, + detection: { + framework: 'react-router', + surface: 'react-router', + languages: [{name: 'typescript', support: 'supported', files: [{path: ordersPath}]}], + }, + coverage: { + filesScanned: 12, + filesSkipped: [], + gaps: [{code: 'unresolved-check', message: 'A gap.', checkId: 'CREDENTIAL_LOG_LEAKAGE', filePath: null}], + scanDirectories: [{directory: appDirectory, origin: 'app-directory'}], + }, + scopeDiffers: false, + checks: [check], + blocking: {level: 'high', blockedCheckCount: 1}, +} + +describe('appSecurityReviewJsonOutputSchema', () => { + test.each([ + ['both results files', result], + [ + 'no results file', + { + ...result, + valid: true, + resultsFiles: {deterministic: null, agent: null}, + detection: null, + coverage: null, + checks: [], + blocking: {level: 'none', blockedCheckCount: 0}, + }, + ], + [ + 'a whole-file location', + {...result, checks: [{...check, findings: [{...agentFinding, location: {...location, line: null}}]}]}, + ], + ])('accepts a result with %s', (_description, value) => { + expect(appSecurityReviewJsonOutputSchema.validate(value)).toStrictEqual(value) + }) + + test.each([ + ['an extra field', {...result, checkCount: 1}], + ['an extra results file field', {...result, resultsFiles: {...result.resultsFiles, instructions: null}}], + ['an extra check field', {...result, checks: [{...check, docs_url: check.docsUrl}]}], + ['an extra source result field', {...result, checks: [{...check, bySource: {...check.bySource, other: null}}]}], + [ + 'an agent field on a deterministic finding', + {...result, checks: [{...check, findings: [{...deterministicFinding, confidence: 'high'}]}]}, + ], + ['a fix on an agent finding', {...result, checks: [{...check, findings: [{...agentFinding, fix: null}]}]}], + [ + 'an extra location field', + {...result, checks: [{...check, findings: [{...agentFinding, location: {...location, file: 'x'}}]}]}, + ], + [ + 'an extra scope field', + { + ...result, + resultsFiles: { + ...result.resultsFiles, + deterministic: {...result.resultsFiles.deterministic, scope: {...scope, no_git_ignore: false}}, + }, + }, + ], + ['an extra blocking field', {...result, blocking: {...result.blocking, blockedChecks: 1}}], + ['a missing field', {...result, coverage: undefined}], + [ + 'a relative finding path', + { + ...result, + checks: [{...check, findings: [{...agentFinding, location: {...location, filePath: 'app/routes/orders.tsx'}}]}], + }, + ], + [ + 'a relative scan directory', + {...result, coverage: {...result.coverage, scanDirectories: [{directory: '.', origin: 'app-directory'}]}}, + ], + ['a zero line', {...result, checks: [{...check, findings: [{...agentFinding, location: {...location, line: 0}}]}]}], + [ + 'a fractional column', + {...result, checks: [{...check, findings: [{...agentFinding, location: {...location, column: 1.5}}]}]}, + ], + ['a docs URL that is not a URL', {...result, checks: [{...check, docsUrl: 'credential-log-leakage'}]}], + ['an empty check guide', {...result, checks: [{...check, guide: ''}]}], + ['an empty snippet', {...result, checks: [{...check, findings: [{...deterministicFinding, snippet: ''}]}]}], + [ + 'an empty evidence quote', + {...result, checks: [{...check, findings: [{...deterministicFinding, evidence: [{location, quote: ''}]}]}]}, + ], + [ + 'an empty fix guide', + { + ...result, + checks: [{...check, findings: [{...deterministicFinding, fix: {...deterministicFinding.fix, guide: ''}}]}], + }, + ], + ['an empty agent reasoning', {...result, checks: [{...check, findings: [{...agentFinding, reasoning: ''}]}]}], + [ + 'an empty skipped file detail', + { + ...result, + coverage: { + ...result.coverage, + filesSkipped: [{path: ordersPath, reason: 'unreadable', sizeBytes: null, detail: ''}], + }, + }, + ], + [ + 'a fractional-second time', + { + ...result, + checks: [ + { + ...check, + bySource: {...check.bySource, deterministic: {...sourceResult, generatedAt: '2026-09-01T10:00:00.000Z'}}, + }, + ], + }, + ], + ['a stored status', {...result, checks: [{...check, status: 'not_applicable'}]}], + [ + 'a stored analysis mode', + { + ...result, + checks: [ + { + ...check, + bySource: {...check.bySource, deterministic: {...sourceResult, analysisMode: 'structured_config'}}, + }, + ], + }, + ], + ['a stored framework', {...result, detection: {...result.detection, framework: 'react_router'}}], + [ + 'a stored scan directory origin', + { + ...result, + coverage: {...result.coverage, scanDirectories: [{directory: appDirectory, origin: 'app_directory'}]}, + }, + ], + ['an unknown source', {...result, checks: [{...check, findings: [{...agentFinding, source: 'manual'}]}]}], + ['an unknown blocking level', {...result, blocking: {level: 'critical', blockedCheckCount: 1}}], + ['a negative blocked check count', {...result, blocking: {level: 'high', blockedCheckCount: -1}}], + ['another status', {...result, status: 'cancelled'}], + ])('rejects %s', (_description, value) => { + expect(() => appSecurityReviewJsonOutputSchema.validate(value)).toThrow() + }) +}) diff --git a/packages/app/src/cli/services/security-review/types.ts b/packages/app/src/cli/services/security-review/types.ts new file mode 100644 index 00000000000..1c671f5322d --- /dev/null +++ b/packages/app/src/cli/services/security-review/types.ts @@ -0,0 +1,297 @@ +import { + appSecurityAppSchema, + appSecurityBlockingSchema, + appSecurityScanDirectorySchema, + appSecurityScopeSchema, + createAbsolutePathSchema, +} from '../app-security-json-output.js' +import { + defineJsonOutputSchema, + jsonOutputTimestampSchema, + type InferJsonOutputSchema, +} from '@shopify/cli-kit/node/json-output-schema' +import {zod} from '@shopify/cli-kit/node/schema' + +/* + * Functions, so each field gets its own instance, as with createAbsolutePathSchema: the JSON Schema would render a + * shared instance as a `$ref` into the first field that uses it. + */ +const createTimestampSchema = () => jsonOutputTimestampSchema.describe(`${jsonOutputTimestampSchema.description}`) +const createCheckStatusSchema = () => zod.enum(['executed', 'not-applicable', 'unresolved']) +const createPrecedenceSchema = () => zod.enum(['union', 'prefer-agent']) + +/** Lines and columns are one-based, as the results files record them. */ +const appSecurityLocationSchema = zod + .object({ + filePath: createAbsolutePathSchema().describe('The absolute path of the file.'), + line: zod.number().int().positive().nullable().describe('The one-based line, or null for the whole file.'), + column: zod.number().int().positive().nullable().describe('The one-based column, or null when none is recorded.'), + }) + .strict() + .describe('A place in a file.') + +const appSecurityEvidenceSchema = zod + .object({ + location: appSecurityLocationSchema, + quote: zod.string().min(1).nullable().describe('The quoted source, redacted, or null when none is recorded.'), + }) + .strict() + .describe('Source that supports a finding.') + +/** The fields that findings from both sources have, in a function so each source's finding gets its own instances. */ +const findingFields = () => ({ + disposition: zod + .enum(['active', 'suppressed', 'superseded']) + .describe( + "Whether the finding counts: active; suppressed by the agent; or superseded, a deterministic finding that a check's prefer-agent agent result replaces.", + ), + location: appSecurityLocationSchema, + message: zod.string().describe('What the finding is, redacted.'), + snippet: zod.string().min(1).nullable().describe('The flagged code, redacted, or null when none is recorded.'), + evidence: zod.array(appSecurityEvidenceSchema), +}) + +const appSecurityDeterministicFindingSchema = zod + .object({ + source: zod.literal('deterministic'), + ...findingFields(), + fix: zod + .object({ + description: zod.string(), + guide: zod.string().min(1).nullable().describe('A guide to the fix, or null when none is recorded.'), + automated: zod.boolean().describe('Whether the fix can be applied automatically.'), + }) + .strict() + .nullable() + .describe('How to fix the finding, or null when none is recorded.'), + }) + .strict() + .describe('A finding from deterministic-findings.json.') + +const appSecurityAgentFindingSchema = zod + .object({ + source: zod.literal('agent'), + ...findingFields(), + confidence: zod + .enum(['high', 'medium', 'low']) + .nullable() + .describe("The agent's confidence, or null when it didn't report one."), + reasoning: zod + .string() + .min(1) + .nullable() + .describe("The agent's reasoning, redacted, or null when none is recorded."), + suppression: zod + .object({justification: zod.string()}) + .strict() + .nullable() + .describe('Why the agent suppressed the finding, or null when it is not suppressed.'), + }) + .strict() + .describe('A finding from agent-findings.json.') + +const appSecurityFindingSchema = zod.discriminatedUnion('source', [ + appSecurityDeterministicFindingSchema, + appSecurityAgentFindingSchema, +]) + +const appSecuritySourceResultSchema = zod + .object({ + version: zod + .number() + .int() + .positive() + .describe( + 'The check version: the catalog version for deterministic results, the one the agent claimed for agent results.', + ), + status: createCheckStatusSchema().describe("The source's own status for the check."), + reason: zod + .object({ + code: zod + .string() + .describe( + 'A short code. Agent codes are free-form. Deterministic codes: capability_absent, no_relevant_files, unsupported_framework, unsupported_language, parser_unavailable, agent_investigation_required, input_rejected.', + ), + message: zod.string(), + }) + .strict() + .nullable() + .describe('Why the check is unresolved or not applicable, or null when none is recorded.'), + analysisMode: zod + .enum(['regex', 'structured-config', 'ast']) + .nullable() + .describe('How the deterministic check analyzed the files. Null for agent results.'), + generatedAt: createTimestampSchema() + .nullable() + .describe('When the source recorded the check, or null when the recorded time is not a valid instant.'), + }) + .strict() + .describe("One source's own result for a check.") + +const appSecurityCheckSchema = zod + .object({ + id: zod.string().describe('The check ID, such as CREDENTIAL_LOG_LEAKAGE.'), + title: zod.string(), + severity: zod.enum(['high', 'medium', 'low']), + description: zod.string(), + guide: zod.string().min(1).nullable().describe('A guide to the check, or null when none is recorded.'), + docsUrl: zod + .string() + .url() + .nullable() + .describe("The check's page on shopify.dev, or null when the results files predate it."), + precedence: createPrecedenceSchema().describe( + "How the sources combine: union keeps both sources' findings; prefer-agent lets the agent result replace the deterministic one.", + ), + appliedPrecedence: createPrecedenceSchema().describe( + 'The precedence that applied: union when a prefer-agent agent result is older than the deterministic one.', + ), + status: createCheckStatusSchema().describe( + 'The combined status. Under union, executed when either source executed the check.', + ), + bySource: zod + .object({ + deterministic: appSecuritySourceResultSchema.nullable(), + agent: appSecuritySourceResultSchema.nullable(), + }) + .strict() + .describe("Each source's own result, or null when that source didn't record the check."), + findings: zod + .array(appSecurityFindingSchema) + .describe( + 'Every finding of both sources, including suppressed and superseded ones, whatever --verbose. Sorted by file, line, then source.', + ), + }) + .strict() + .describe('One check, combined from both results files.') + +const appSecurityDetectionSchema = zod + .object({ + framework: zod.enum(['react-router', 'none', 'unknown', 'mixed']), + surface: zod.enum(['react-router', 'theme-app-extension', 'config-only', 'unknown', 'mixed']), + languages: zod.array( + zod + .object({ + name: zod.string().describe('The language, such as typescript.'), + support: zod.enum(['supported', 'unsupported']).describe('Whether the deterministic checks analyze it.'), + files: zod.array(zod.object({path: createAbsolutePathSchema()}).strict()), + }) + .strict(), + ), + }) + .strict() + .describe('What the latest check detected in the app.') + +const appSecurityCoverageSchema = zod + .object({ + filesScanned: zod.number().int().nonnegative().describe('The number of files scanned.'), + filesSkipped: zod.array( + zod + .object({ + path: createAbsolutePathSchema(), + reason: zod.enum(['too-large', 'unreadable']), + sizeBytes: zod.number().int().nonnegative().nullable().describe('The file size, or null when unknown.'), + detail: zod + .string() + .min(1) + .nullable() + .describe('Why the file was unreadable, or null when none is recorded.'), + }) + .strict(), + ), + gaps: zod.array( + zod + .object({ + code: zod.enum(['skipped-file', 'unsupported-framework', 'unsupported-language', 'unresolved-check']), + message: zod.string(), + checkId: zod.string().nullable().describe('The check that the gap concerns, or null.'), + filePath: createAbsolutePathSchema().nullable().describe('The file that the gap concerns, or null.'), + }) + .strict(), + ), + scanDirectories: zod.array(appSecurityScanDirectorySchema), + }) + .strict() + .describe('What the latest check covered.') + +const appSecurityDeterministicResultsFileSchema = zod + .object({ + path: createAbsolutePathSchema().describe('The absolute path of deterministic-findings.json.'), + generatedAt: createTimestampSchema() + .nullable() + .describe('When check wrote the file, or null when the recorded time is not a valid instant.'), + engine: zod.object({name: zod.string(), version: zod.string(), ruleset: zod.string()}).strict(), + scope: appSecurityScopeSchema, + }) + .strict() + .describe('deterministic-findings.json, which check writes, with the scope of the scan.') + +const appSecurityAgentResultsFileSchema = zod + .object({ + path: createAbsolutePathSchema().describe('The absolute path of agent-findings.json.'), + generatedAt: createTimestampSchema() + .nullable() + .describe('When record wrote the file, or null when the recorded time is not a valid instant.'), + engine: zod.object({name: zod.string(), version: zod.string()}).strict(), + scope: appSecurityScopeSchema, + }) + .strict() + .describe('agent-findings.json, which record writes, with the scope that the agent reported.') + +export const appSecurityReviewJsonOutputSchema = defineJsonOutputSchema({ + name: 'AppSecurityReviewResult', + schema: zod + .object({ + status: zod.literal('success'), + valid: zod + .boolean() + .describe( + 'Whether --blocking passed: no check in checks has an active finding and a severity at or above the level. Unresolved checks never fail it, and --blocking none always passes.', + ), + app: appSecurityAppSchema, + resultsDirectory: createAbsolutePathSchema().describe('The absolute path of the results directory.'), + resultsFiles: zod + .object({ + deterministic: appSecurityDeterministicResultsFileSchema.nullable(), + agent: appSecurityAgentResultsFileSchema.nullable(), + }) + .strict() + .describe('The results files that the review combines. Null when a file does not exist.'), + detection: appSecurityDetectionSchema + .nullable() + .describe('What the latest check detected in the app, or null without deterministic-findings.json.'), + coverage: appSecurityCoverageSchema + .nullable() + .describe('What the latest check covered, or null without deterministic-findings.json.'), + scopeDiffers: zod + .boolean() + .describe( + 'Whether the agent reported another scope than the latest scan used. False unless both results files exist.', + ), + checks: zod + .array(appSecurityCheckSchema) + .describe('The checks after --check-id, most severe first, then by ID. Empty when no results file exists.'), + blocking: appSecurityBlockingSchema, + }) + .strict() + .describe('The combined results of the app security check.'), + definitions: { + AppSecurityApp: appSecurityAppSchema, + AppSecurityScope: appSecurityScopeSchema, + AppSecurityScanDirectory: appSecurityScanDirectorySchema, + AppSecurityBlocking: appSecurityBlockingSchema, + AppSecurityDeterministicResultsFile: appSecurityDeterministicResultsFileSchema, + AppSecurityAgentResultsFile: appSecurityAgentResultsFileSchema, + AppSecurityDetection: appSecurityDetectionSchema, + AppSecurityCoverage: appSecurityCoverageSchema, + AppSecurityCheck: appSecurityCheckSchema, + AppSecuritySourceResult: appSecuritySourceResultSchema, + AppSecurityFinding: appSecurityFindingSchema, + AppSecurityDeterministicFinding: appSecurityDeterministicFindingSchema, + AppSecurityAgentFinding: appSecurityAgentFindingSchema, + AppSecurityLocation: appSecurityLocationSchema, + AppSecurityEvidence: appSecurityEvidenceSchema, + }, +}) + +export type AppSecurityReviewResult = InferJsonOutputSchema diff --git a/packages/cli/README.md b/packages/cli/README.md index c1946a96dae..5344afe2adf 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -6161,14 +6161,18 @@ Show the combined app security check results. ``` USAGE - $ shopify app security review [--blocking high|medium|low|none] [--check-id ...] [--json-schema] [--no-color] - [--no-input] [--path ] [--verbose] [--without-app-config [--client-id | -c ]] + $ shopify app security review [--blocking high|medium|low|none] [--check-id ...] [-j] [--json-schema] + [--no-color] [--no-input] [--path ] [--verbose] [--without-app-config [--client-id | -c ]] FLAGS -c, --config= 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=