diff --git a/.changeset/app-security-clipboard-errors.md b/.changeset/app-security-clipboard-errors.md new file mode 100644 index 00000000000..88c16d9f8c7 --- /dev/null +++ b/.changeset/app-security-clipboard-errors.md @@ -0,0 +1,6 @@ +--- +'@shopify/app': patch +'@shopify/cli': patch +--- + +Report clipboard failures in `app security instructions --copy` as errors, and in `app security check` as warnings, instead of crashing. diff --git a/docs-shopify.dev/generated/generated_docs_data_v2.json b/docs-shopify.dev/generated/generated_docs_data_v2.json index 80090e3f5b2..29dcad605be 100644 --- a/docs-shopify.dev/generated/generated_docs_data_v2.json +++ b/docs-shopify.dev/generated/generated_docs_data_v2.json @@ -3574,7 +3574,7 @@ "syntaxKind": "PropertySignature", "name": "--copy", "value": "''", - "description": "Copy the instructions to the clipboard instead of printing them.", + "description": "Copy the instructions to the clipboard instead of printing them. With --json, the result still includes them.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY" }, @@ -3637,7 +3637,7 @@ "syntaxKind": "PropertySignature", "name": "--write ", "value": "string", - "description": "Write the instructions to a file instead of printing them.", + "description": "Write the instructions to a file instead of printing them. With --json, stdout contains only the absolute path and Markdown format of the written file.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE" }, @@ -3649,9 +3649,18 @@ "description": "The name of the app configuration.", "isOptional": true, "environmentValue": "SHOPIFY_FLAG_APP_CONFIG" + }, + { + "filePath": "docs-shopify.dev/commands/interfaces/app-security-instructions.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 appsecurityinstructions {\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 * Copy the instructions to the clipboard instead of printing them.\n * @environment SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY\n */\n '--copy'?: ''\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\n /**\n * Write the instructions to a file instead of printing them.\n * @environment SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE\n */\n '--write '?: string\n}" + "value": "export interface appsecurityinstructions {\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 * Copy the instructions to the clipboard instead of printing them. With --json, the result still includes them.\n * @environment SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY\n */\n '--copy'?: ''\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\n /**\n * Write the instructions to a file instead of printing them. With --json, stdout contains only the absolute path and Markdown format of the written file.\n * @environment SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE\n */\n '--write '?: string\n}" } }, "appsecurityrecord": { diff --git a/packages/app/src/cli/commands/app/security/check.integration.test.ts b/packages/app/src/cli/commands/app/security/check.integration.test.ts index e52a2edf7fd..2eabd88e1c3 100644 --- a/packages/app/src/cli/commands/app/security/check.integration.test.ts +++ b/packages/app/src/cli/commands/app/security/check.integration.test.ts @@ -126,6 +126,21 @@ describe('app security check command boundary', () => { }) }) + test('--print-instructions prints only the instructions for the scan that just ran on stdout', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const appDirectory = await fileRealPath(directory) + + const result = await runCommand(['--path', directory, '--print-instructions']) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toMatch(/^The app security check is Shopify's local security review workflow/) + expect(result.stdout).toContain('### 1. Use the existing scan results') + expect(result.stdout).toContain(appSecurityArtifactPaths(appDirectory, 'shopify.app').agentChecksPath) + expect(boxText(result.stderr)).toContain('Config file shopify.app.toml') + }) + }) + test('scans an extension directory that the TOML adds outside the app directory and records it in coverage', async () => { await inTemporaryDirectory(async (directory) => { const appRoot = joinPath(directory, 'app') diff --git a/packages/app/src/cli/commands/app/security/instructions.integration.test.ts b/packages/app/src/cli/commands/app/security/instructions.integration.test.ts new file mode 100644 index 00000000000..1df6d435b9f --- /dev/null +++ b/packages/app/src/cli/commands/app/security/instructions.integration.test.ts @@ -0,0 +1,208 @@ +import SecurityInstructions from './instructions.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 {fileExists, fileRealPath, inTemporaryDirectory, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {unstyled} from '@shopify/cli-kit/node/output' +import {joinPath, relativePath, resolvePath} from '@shopify/cli-kit/node/path' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' +import clipboard from 'clipboardy' +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' + +// 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 the instructions. Keep the real error handler and renderer. +vi.mock('@shopify/cli-kit/node/analytics', async (importOriginal) => ({ + ...(await importOriginal()), + reportAnalyticsEvent: vi.fn(), +})) +// Tests must not replace the clipboard of the machine that runs them. +vi.mock('clipboardy', () => ({default: {write: vi.fn()}})) + +/** Creates an app linked to `test-client-id`, without results, and returns its real path. */ +async function createApp(directory: string): Promise { + await writeFile(joinPath(directory, 'shopify.app.toml'), validAppConfiguration('test-client-id')) + return fileRealPath(directory) +} + +// 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 SecurityInstructions.run(argv, config) + return {stdout: stdout(), stderr: stderr()} + }) + return {...output, exitCode: process.exitCode} + } finally { + exit.mockRestore() + process.exitCode = previousExitCode + } +} + +/** + * Runs the command with --json for a failure. 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() + } +} + +/** The instructions as the command prints them without --json, without the line break that ends the output. */ +async function printedInstructions(directory: string): Promise { + const {stdout} = await runCommand(['--path', directory]) + return stdout.replace(/\n$/, '') +} + +describe('app security instructions command boundary', () => { + test('--json writes one result with the selected app, its results directory and the printed instructions', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory) + const instructions = await printedInstructions(directory) + + const result = await runCommand(['--path', directory, '--json']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({ + status: 'success', + app: { + directory: resolve(appDirectory), + configurationPath: resolve(appDirectory, 'shopify.app.toml'), + clientId: 'test-client-id', + }, + resultsDirectory: resolve(appDirectory, '.shopify', 'app-security', 'shopify.app'), + instructions: {content: instructions, copied: false}, + }) + expect(instructions).toContain('### 1. Run the scan') + expect(result.stderr).toBe('') + // Standalone instructions start by running check, which creates the results directory. + await expect(fileExists(appSecurityArtifactPaths(appDirectory, 'shopify.app').resultsDirectory)).resolves.toBe( + false, + ) + }) + }) + + test('--copy --json copies the instructions and still includes them in the result', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const instructions = await printedInstructions(directory) + + const result = await runCommand(['--path', directory, '--copy', '--json']) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toMatchObject({instructions: {content: instructions, copied: true}}) + expect(result.stderr).toBe('') + expect(clipboard.write).toHaveBeenCalledExactlyOnceWith(instructions) + }) + }) + + test('--write --json writes the instructions to the file and only its absolute path and format to stdout', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const instructions = await printedInstructions(directory) + const writePath = joinPath(directory, 'instructions.md') + + const result = await runCommand([ + '--path', + directory, + '--write', + relativePath(resolvePath(), writePath), + '--json', + ]) + + expect(result.exitCode).toBe(0) + expect(JSON.parse(result.stdout)).toStrictEqual({path: resolve(writePath), format: 'markdown'}) + expect(result.stderr).toBe('') + await expect(readFile(writePath)).resolves.toBe(`${instructions}\n`) + }) + }) + + test('--copy --json writes one error document with its code and no result when the clipboard fails', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + const result = await runJsonCommand(['--path', directory, '--copy']) + + expect(result.exitCode).toBe(1) + expect(JSON.parse(result.stdout)).toStrictEqual({ + error: { + type: 'abort', + message: + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy", + code: 'clipboard-unavailable', + nextSteps: [ + 'Run the command without --copy to print the instructions, or with --write to write them to a file.', + ], + }, + }) + expect(result.stderr).toBe('') + }) + }) + + test('--copy without --json shows the error on stderr, prints nothing on stdout and exits 1', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + vi.mocked(clipboard.write).mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + const result = await runCommand(['--path', directory, '--copy']) + + expect(result.exitCode).toBe(1) + expect(result.stdout).toBe('') + expect(boxText(result.stderr)).toContain( + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy", + ) + expect(boxText(result.stderr)).toContain('Run the command without `--copy` to print the instructions') + }) + }) + + test('without --json, prints the instructions on stdout and nothing on stderr', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + + const result = await runCommand(['--path', directory]) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toMatch(/^The app security check is Shopify's local security review workflow/) + expect(result.stderr).toBe('') + }) + }) + + test('--write without --json confirms the written file on stderr and prints nothing on stdout', async () => { + await inTemporaryDirectory(async (directory) => { + await createApp(directory) + const instructions = await printedInstructions(directory) + const writePath = joinPath(directory, 'instructions.md') + + const result = await runCommand(['--path', directory, '--write', writePath]) + + expect(result.exitCode).toBe(0) + expect(result.stdout).toBe('') + expect(unstyled(result.stderr)).toContain('Wrote app security check instructions to') + await expect(readFile(writePath)).resolves.toBe(`${instructions}\n`) + }) + }) +}) diff --git a/packages/app/src/cli/commands/app/security/instructions.test.ts b/packages/app/src/cli/commands/app/security/instructions.test.ts index 6f73add4656..86124f738d8 100644 --- a/packages/app/src/cli/commands/app/security/instructions.test.ts +++ b/packages/app/src/cli/commands/app/security/instructions.test.ts @@ -3,17 +3,21 @@ import SecurityCheck from './check.js' import {appFlags} from '../../../flags.js' import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js' import {resolveAppSecurityCommands} from '../../../services/app-security-commands.js' -import deliverAppSecurityInstructions from '../../../services/app-security-instructions.js' +import {appSecurityInstructions, deliverAppSecurityInstructions} from '../../../services/app-security-instructions.js' import {resolveAppSecuritySelection, type AppSecuritySelection} from '../../../services/app-security-selection.js' import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js' +import {renderSecurityInstructionsResult} from '../../../services/security-instructions/result.js' +import {appSecurityInstructionsJsonOutputSchema} from '../../../services/security-instructions/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 {AbortError} from '@shopify/cli-kit/node/error' import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs' import {cwd, joinPath, resolvePath} from '@shopify/cli-kit/node/path' import {describe, expect, test, vi} from 'vitest' vi.mock('../../../services/app-security-instructions.js') +vi.mock('../../../services/security-instructions/result.js') vi.mock('../../../services/app-security-selection.js', async (importOriginal) => ({ ...(await importOriginal()), resolveAppSecuritySelection: vi.fn(), @@ -59,6 +63,8 @@ async function resolveWithFailingLookUp() { return lookUpApp } +const INSTRUCTIONS = '# App security check instructions' + function configSelection(appDirectory: string, configFileName: string): AppSecuritySelection { return {kind: 'config', appDirectory, appConfigFilePath: joinPath(appDirectory, configFileName)} } @@ -70,6 +76,11 @@ describe('app security instructions command', () => { expect(SecurityInstructions.args).not.toHaveProperty('directory') }) + test('defines --json and exposes the result schema', () => { + expect(SecurityInstructions.flags.json).toBe(jsonFlag.json) + expect(SecurityInstructions.jsonOutputSchema).toBe(appSecurityInstructionsJsonOutputSchema) + }) + test('defines the selection flags as check does', () => { expect(SecurityInstructions.flags.path).toBe(appFlags.path) expect(SecurityInstructions.flags.config).toBe(appFlags.config) @@ -80,6 +91,7 @@ describe('app security instructions command', () => { test('prints instructions for the current directory by default', async () => { await inTemporaryDirectory(async (directory) => { const appDirectory = await createApp(directory) + vi.mocked(appSecurityInstructions).mockReturnValue(INSTRUCTIONS) await SecurityInstructions.run([], import.meta.url) @@ -90,38 +102,61 @@ describe('app security instructions command', () => { withoutAppConfig: undefined, allowPrompts: false, }) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith({ + expect(appSecurityInstructions).toHaveBeenCalledWith({ appDirectory, resultsKey: 'shopify.app', commands: resolveAppSecurityCommands(configSelection(appDirectory, 'shopify.app.toml'), cwd()), - copy: false, - writePath: undefined, }) + const delivery = {copy: false, writePath: undefined} + expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(INSTRUCTIONS, delivery) + expect(renderSecurityInstructionsResult).toHaveBeenCalledWith( + INSTRUCTIONS, + {selection: configSelection(appDirectory, 'shopify.app.toml'), delivery}, + 'text', + ) + }) + }) + + test.each([['--json'], ['-j']])('presents the result as JSON with %s', async (flag) => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await createApp(directory) + vi.mocked(appSecurityInstructions).mockReturnValue(INSTRUCTIONS) + + await SecurityInstructions.run(['--copy', flag], import.meta.url) + + expect(renderSecurityInstructionsResult).toHaveBeenCalledWith( + INSTRUCTIONS, + {selection: configSelection(appDirectory, 'shopify.app.toml'), delivery: {copy: true, writePath: undefined}}, + 'json', + ) }) }) test('forwards --path and --copy', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) + vi.mocked(appSecurityInstructions).mockReturnValue(INSTRUCTIONS) await SecurityInstructions.run(['--path', './fixtures/unlinked-app', '--copy'], import.meta.url) expect(resolveAppSecuritySelection).toHaveBeenCalledWith( expect.objectContaining({path: resolvePath('./fixtures/unlinked-app')}), ) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(expect.objectContaining({copy: true})) + expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(INSTRUCTIONS, expect.objectContaining({copy: true})) }) }) test('resolves and forwards --write', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) + vi.mocked(appSecurityInstructions).mockReturnValue(INSTRUCTIONS) await SecurityInstructions.run(['--write', './instructions.md'], import.meta.url) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith( - expect.objectContaining({copy: false, writePath: resolvePath('./instructions.md')}), - ) + expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(INSTRUCTIONS, { + copy: false, + writePath: resolvePath('./instructions.md'), + }) }) }) @@ -135,7 +170,7 @@ describe('app security instructions command', () => { await SecurityInstructions.run(['--path', './fixtures/unlinked-app', '--config', 'staging'], import.meta.url) expect(resolveAppSecuritySelection).toHaveBeenCalledWith(expect.objectContaining({config: 'staging'})) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith( + expect(appSecurityInstructions).toHaveBeenCalledWith( expect.objectContaining({ resultsKey: 'shopify.app.staging', commands: resolveAppSecurityCommands( @@ -162,7 +197,7 @@ describe('app security instructions command', () => { expect(resolveAppSecuritySelection).toHaveBeenCalledWith( expect.objectContaining({clientId: 'abc123', withoutAppConfig: true, allowPrompts: false}), ) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(expect.objectContaining({resultsKey: 'abc123'})) + expect(appSecurityInstructions).toHaveBeenCalledWith(expect.objectContaining({resultsKey: 'abc123'})) }) }) @@ -172,7 +207,7 @@ describe('app security instructions command', () => { await SecurityInstructions.run([], import.meta.url) - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith( + expect(appSecurityInstructions).toHaveBeenCalledWith( expect.objectContaining({appDirectory, resultsKey: 'shopify.app'}), ) }) @@ -192,9 +227,7 @@ describe('app security instructions command', () => { await SecurityInstructions.run(['--path', directory, '--client-id', 'mistyped-client-id'], import.meta.url) expect(lookUpApp).not.toHaveBeenCalled() - expect(deliverAppSecurityInstructions).toHaveBeenCalledWith( - expect.objectContaining({resultsKey: 'mistyped-client-id'}), - ) + expect(appSecurityInstructions).toHaveBeenCalledWith(expect.objectContaining({resultsKey: 'mistyped-client-id'})) }) }) }) diff --git a/packages/app/src/cli/commands/app/security/instructions.ts b/packages/app/src/cli/commands/app/security/instructions.ts index 1b97d8d3c24..6fa4ad7c9ee 100644 --- a/packages/app/src/cli/commands/app/security/instructions.ts +++ b/packages/app/src/cli/commands/app/security/instructions.ts @@ -1,10 +1,12 @@ import {appSecuritySelectionFlags} from './selection-flags.js' import {resolveAppSecurityCommands} from '../../../services/app-security-commands.js' -import deliverAppSecurityInstructions from '../../../services/app-security-instructions.js' +import {appSecurityInstructions, deliverAppSecurityInstructions} from '../../../services/app-security-instructions.js' import {resolveAppSecuritySelection, resultsKey} from '../../../services/app-security-selection.js' +import {renderSecurityInstructionsResult} from '../../../services/security-instructions/result.js' +import {appSecurityInstructionsJsonOutputSchema} from '../../../services/security-instructions/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' import {resolvePath} from '@shopify/cli-kit/node/path' export default class SecurityInstructions extends BaseCommand { @@ -14,23 +16,30 @@ export default class SecurityInstructions extends BaseCommand { By default, the instructions are printed to stdout. Use \`--copy\` to copy them to the clipboard or \`--write\` to write them to a file. Standalone instructions always start by running \`shopify app security check\`; only that invocation's generated review pack is trusted as workflow input.` + static get jsonOutputSchema() { + return appSecurityInstructionsJsonOutputSchema + } + static description = this.descriptionWithoutMarkdown() static flags = { ...globalFlags, ...appSecuritySelectionFlags, copy: Flags.boolean({ - description: 'Copy the instructions to the clipboard instead of printing them.', + description: + 'Copy the instructions to the clipboard instead of printing them. With --json, the result still includes them.', default: false, exclusive: ['write'], env: 'SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY', }), write: Flags.string({ - description: 'Write the instructions to a file instead of printing them.', + description: + 'Write the instructions to a file instead of printing them. With --json, stdout contains only the absolute path and Markdown format of the written file.', exclusive: ['copy'], parse: async (input) => resolvePath(input), env: 'SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE', }), + ...jsonFlag, } public async run(): Promise { @@ -43,12 +52,13 @@ By default, the instructions are printed to stdout. Use \`--copy\` to copy them withoutAppConfig: flags['without-app-config'], allowPrompts: false, }) - await deliverAppSecurityInstructions({ + const instructions = appSecurityInstructions({ appDirectory: selection.appDirectory, resultsKey: resultsKey(selection), commands: resolveAppSecurityCommands(selection, flags.path), - copy: flags.copy, - writePath: flags.write, }) + const delivery = {copy: flags.copy, writePath: flags.write} + await deliverAppSecurityInstructions(instructions, delivery) + renderSecurityInstructionsResult(instructions, {selection, delivery}, flags.json ? 'json' : 'text') } } diff --git a/packages/app/src/cli/services/app-security-instructions.test.ts b/packages/app/src/cli/services/app-security-instructions.test.ts index dbbcab6bea8..db96e93b8cf 100644 --- a/packages/app/src/cli/services/app-security-instructions.test.ts +++ b/packages/app/src/cli/services/app-security-instructions.test.ts @@ -1,11 +1,17 @@ -import deliverAppSecurityInstructions, { +import { appSecurityInstructions as instructionsFor, + ClipboardUnavailableError, + deliverAppSecurityInstructions, + renderAppSecurityInstructions, shellQuote, + type AppSecurityInstructionsDelivery, } from './app-security-instructions.js' import {quoteShellArgument, resolveAppSecurityCommands, type AppSecurityShell} from './app-security-commands.js' import {getAgentInstructions, type AppSecurityScope} from './app-security-engine/index.js' import {inTemporaryDirectory, mkdir, readFile, writeFile} from '@shopify/cli-kit/node/fs' +import {unstyled} from '@shopify/cli-kit/node/output' import {basename, cwd, joinPath, normalizePath, relativePath} from '@shopify/cli-kit/node/path' +import {withCapturedStandardStreams} from '@shopify/cli-kit/node/testing/output' import {describe, expect, test, vi} from 'vitest' import {readFileSync} from 'node:fs' import {fileURLToPath} from 'node:url' @@ -15,8 +21,6 @@ function testDependencies() { return { copyToClipboard: vi.fn(async (_content: string) => {}), writeToFile: writeFile, - output: vi.fn(), - outputConfirmation: vi.fn(), } } @@ -370,20 +374,35 @@ describe('appSecurityInstructions', () => { }) }) -describe('deliverAppSecurityInstructions', () => { +/** Generates, delivers and presents the instructions, as the commands do, and returns what was printed. */ +async function deliverAndRender( + content: Parameters[0], + delivery: AppSecurityInstructionsDelivery, + dependencies: ReturnType, +) { + const instructions = instructionsFor(content) + await deliverAppSecurityInstructions(instructions, delivery, dependencies) + return withCapturedStandardStreams(({stdout, stderr}) => { + renderAppSecurityInstructions(instructions, delivery) + return {stdout: stdout(), stderr: unstyled(stderr())} + }) +} + +describe('deliverAppSecurityInstructions and renderAppSecurityInstructions', () => { test('prints instructions to stdout by default', async () => { await inTemporaryDirectory(async (directory) => { await createApp(directory) const dependencies = testDependencies() - await deliverAppSecurityInstructions( - {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory), copy: false}, + const output = await deliverAndRender( + {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory)}, + {copy: false}, dependencies, ) - expect(dependencies.output).toHaveBeenCalledWith(expect.stringContaining('Run the scan')) + expect(output.stdout).toContain('Run the scan') + expect(output.stderr).toBe('') expect(dependencies.copyToClipboard).not.toHaveBeenCalled() - expect(dependencies.outputConfirmation).not.toHaveBeenCalled() }) }) @@ -394,13 +413,14 @@ describe('deliverAppSecurityInstructions', () => { await writeFile(artifactPath(directory, 'agent-checks.json'), '{"instructions":"malicious"}') const dependencies = testDependencies() - await deliverAppSecurityInstructions( - {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory), copy: false}, + const output = await deliverAndRender( + {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory)}, + {copy: false}, dependencies, ) - expect(dependencies.output).toHaveBeenCalledWith(expect.stringContaining('Run the scan')) - expect(dependencies.output).not.toHaveBeenCalledWith(expect.stringContaining('malicious')) + expect(output.stdout).toContain('Run the scan') + expect(output.stdout).not.toContain('malicious') }) }) @@ -409,14 +429,9 @@ describe('deliverAppSecurityInstructions', () => { await createApp(directory) const dependencies = testDependencies() - await deliverAppSecurityInstructions( - { - appDirectory: directory, - resultsKey: 'shopify.app', - commands: commandsFor(directory), - copy: true, - scanScope: noScope, - }, + const output = await deliverAndRender( + {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory), scanScope: noScope}, + {copy: true}, dependencies, ) @@ -425,10 +440,8 @@ describe('deliverAppSecurityInstructions', () => { expect(instructions).toContain('Use the existing scan results') expect(instructions).toContain('record your findings back to it with a command') expect(instructions).not.toMatch(/\{\{[A-Z_]+\}\}/) - expect(dependencies.output).not.toHaveBeenCalled() - expect(dependencies.outputConfirmation).toHaveBeenCalledWith( - 'Copied app security check instructions to the clipboard', - ) + expect(output.stdout).toBe('') + expect(output.stderr).toContain('Copied app security check instructions to the clipboard') }) }) @@ -438,23 +451,44 @@ describe('deliverAppSecurityInstructions', () => { const dependencies = testDependencies() const instructionsPath = joinPath(directory, 'handoff.md') - await deliverAppSecurityInstructions( - { - appDirectory: directory, - resultsKey: 'shopify.app', - commands: commandsFor(directory), - copy: false, - writePath: instructionsPath, - scanScope: noScope, - }, + const output = await deliverAndRender( + {appDirectory: directory, resultsKey: 'shopify.app', commands: commandsFor(directory), scanScope: noScope}, + {copy: false, writePath: instructionsPath}, dependencies, ) await expect(readFile(instructionsPath)).resolves.toContain('Use the existing scan results') - expect(dependencies.output).not.toHaveBeenCalled() - expect(dependencies.outputConfirmation).toHaveBeenCalledWith( - `Wrote app security check instructions to ${instructionsPath}`, - ) + expect(output.stdout).toBe('') + expect(output.stderr).toContain('Wrote app security check instructions to') + expect(output.stderr).toContain('handoff.md') }) }) }) + +describe('deliverAppSecurityInstructions', () => { + test('raises ClipboardUnavailableError with its code and the clipboard error when copying fails', async () => { + const dependencies = testDependencies() + dependencies.copyToClipboard.mockRejectedValue(new Error('Command failed with ENOENT: pbcopy')) + + const error = await deliverAppSecurityInstructions('# Instructions', {copy: true}, dependencies).catch( + (error: unknown) => error, + ) + + expect(error).toBeInstanceOf(ClipboardUnavailableError) + const abort = error as ClipboardUnavailableError + expect(abort.message).toBe( + "Couldn't copy the app security check instructions to the clipboard: Command failed with ENOENT: pbcopy", + ) + expect(abort.code).toBe('clipboard-unavailable') + expect(abort.details).toBeUndefined() + expect(abort.nextSteps).toStrictEqual([ + [ + 'Run the command without', + {command: '--copy'}, + 'to print the instructions, or with', + {command: '--write '}, + 'to write them to a file.', + ], + ]) + }) +}) diff --git a/packages/app/src/cli/services/app-security-instructions.ts b/packages/app/src/cli/services/app-security-instructions.ts index 0924c1b9d33..215315f7c5d 100644 --- a/packages/app/src/cli/services/app-security-instructions.ts +++ b/packages/app/src/cli/services/app-security-instructions.ts @@ -9,6 +9,7 @@ import { type AppSecurityShell, } from './app-security-commands.js' import {getAgentInstructions, type AppSecurityScope} from './app-security-engine/index.js' +import {AbortError} from '@shopify/cli-kit/node/error' import {writeFile} from '@shopify/cli-kit/node/fs' import {outputResult} from '@shopify/cli-kit/node/output' import {cwd} from '@shopify/cli-kit/node/path' @@ -147,30 +148,37 @@ function fillTemplate(template: string, values: {[placeholder: string]: string}) ) } -interface AppSecurityInstructionsOptions { - appDirectory: string - resultsKey: string +/** How the instructions reach the agent: copied to the clipboard, written to `writePath`, or else printed. */ +export interface AppSecurityInstructionsDelivery { copy: boolean writePath?: string - commands: AppSecurityCommands - /** Present when `check` has just run in this process. */ - scanScope?: AppSecurityScope +} + +/** The instructions couldn't be copied, for example because the system has no clipboard tool. */ +export class ClipboardUnavailableError extends AbortError { + constructor(cause: unknown) { + const message = cause instanceof Error ? cause.message : String(cause) + super(`Couldn't copy the app security check instructions to the clipboard: ${message}`, null, [ + [ + 'Run the command without', + {command: '--copy'}, + 'to print the instructions, or with', + {command: '--write '}, + 'to write them to a file.', + ], + ]) + this.code = 'clipboard-unavailable' + } } interface AppSecurityInstructionsDependencies { copyToClipboard(content: string): Promise writeToFile(path: string, content: string): Promise - output(content: string): void - outputConfirmation(content: string): void } const defaultDependencies: AppSecurityInstructionsDependencies = { copyToClipboard: (content) => clipboard.write(content), writeToFile: writeFile, - output: outputResult, - outputConfirmation: (content) => { - renderSuccess({headline: content}) - }, } export function appSecurityInstructions(options: { @@ -201,24 +209,35 @@ export function appSecurityInstructions(options: { }).trimEnd() } -export default async function deliverAppSecurityInstructions( - options: AppSecurityInstructionsOptions, +/** + * Copies the instructions to the clipboard or writes them to a file. Printed instructions need no delivery. + * A failed copy throws a {@link ClipboardUnavailableError}. + */ +export async function deliverAppSecurityInstructions( + instructions: string, + delivery: AppSecurityInstructionsDelivery, dependencies: AppSecurityInstructionsDependencies = defaultDependencies, ): Promise { - const instructions = appSecurityInstructions({ - appDirectory: options.appDirectory, - resultsKey: options.resultsKey, - commands: options.commands, - scanScope: options.scanScope, - }) - - if (options.copy) { - await dependencies.copyToClipboard(instructions) - dependencies.outputConfirmation('Copied app security check instructions to the clipboard') - } else if (options.writePath) { - await dependencies.writeToFile(options.writePath, `${instructions}\n`) - dependencies.outputConfirmation(`Wrote app security check instructions to ${options.writePath}`) + if (delivery.copy) { + try { + await dependencies.copyToClipboard(instructions) + } catch (error) { + throw new ClipboardUnavailableError(error) + } + } else if (delivery.writePath) { + await dependencies.writeToFile(delivery.writePath, `${instructions}\n`) + } +} + +/** Prints the instructions, or confirms in the terminal that they were copied or written. */ +export function renderAppSecurityInstructions(instructions: string, delivery: AppSecurityInstructionsDelivery): void { + if (delivery.copy) { + // No trailing period, like the write confirmation, whose file path a period would run into. + // eslint-disable-next-line @shopify/cli/banner-headline-format + renderSuccess({headline: 'Copied app security check instructions to the clipboard'}) + } else if (delivery.writePath) { + renderSuccess({headline: `Wrote app security check instructions to ${delivery.writePath}`}) } else { - dependencies.output(instructions) + outputResult(instructions) } } 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 354975d0a9e..278bfef286a 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,4 +1,10 @@ -import {appSecurityAppSchema, projectAppSecurityApp, projectAppSecurityPath} from './app-security-json-output.js' +import { + appSecurityAppSchema, + appSecurityInstructionsSchema, + projectAppSecurityApp, + projectAppSecurityInstructions, + projectAppSecurityPath, +} from './app-security-json-output.js' import {joinPath} from '@shopify/cli-kit/node/path' import {describe, expect, test} from 'vitest' import type {AppSecuritySelection} from './app-security-selection.js' @@ -80,3 +86,32 @@ describe('appSecurityAppSchema', () => { expect(appSecurityAppSchema.safeParse(value).success).toBe(false) }) }) + +describe('projectAppSecurityInstructions', () => { + test.each([ + ['printed', {copy: false}, false], + ['copied to the clipboard', {copy: true}, true], + ])('projects instructions that were %s', (_description, delivery, copied) => { + expect(projectAppSecurityInstructions('# Instructions', delivery)).toStrictEqual({ + content: '# Instructions', + copied, + }) + }) +}) + +describe('appSecurityInstructionsSchema', () => { + const instructions = {content: '# Instructions', copied: true} + + test('accepts projected instructions', () => { + expect(appSecurityInstructionsSchema.parse(instructions)).toStrictEqual(instructions) + }) + + test.each([ + ['an extra field', {...instructions, path: '/apps/my-app/instructions.md'}], + ['a missing field', {content: instructions.content}], + ['empty content', {...instructions, content: ''}], + ['a copied flag that is not a boolean', {...instructions, copied: 'true'}], + ])('rejects %s', (_description, value) => { + expect(appSecurityInstructionsSchema.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 86b0dd1e9a1..fe83235678c 100644 --- a/packages/app/src/cli/services/app-security-json-output.ts +++ b/packages/app/src/cli/services/app-security-json-output.ts @@ -3,6 +3,7 @@ import {isAbsolutePath} 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 {AppSecurityInstructionsDelivery} from './app-security-instructions.js' /** * The parts of the app security commands' JSON output that several commands share, and the codecs that map the @@ -36,6 +37,16 @@ export const appSecurityAppSchema = zod type AppSecurityApp = zod.infer +/** The coding-agent instructions, which a JSON result includes instead of printing them. */ +export const appSecurityInstructionsSchema = zod + .object({ + content: zod.string().min(1).describe('The instructions in Markdown, as printed without --json.'), + copied: zod.boolean().describe('Whether the instructions were also copied to the clipboard.'), + }) + .strict() + +type AppSecurityInstructions = 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) @@ -48,3 +59,11 @@ export function projectAppSecurityApp(selection: AppSecuritySelection): AppSecur clientId: effectiveClientId(selection) ?? null, } } + +/** Projects delivered instructions: `delivery.copy` means that they're on the clipboard. */ +export function projectAppSecurityInstructions( + instructions: string, + delivery: AppSecurityInstructionsDelivery, +): AppSecurityInstructions { + return {content: instructions, copied: delivery.copy} +} diff --git a/packages/app/src/cli/services/security-check.ts b/packages/app/src/cli/services/security-check.ts index 4ddb65e9c01..b88ee06888c 100644 --- a/packages/app/src/cli/services/security-check.ts +++ b/packages/app/src/cli/services/security-check.ts @@ -1,6 +1,10 @@ import {securityExitCode, executeAppSecurity, listAppSecurityFiles} from './app-security-api.js' import {writeCheckArtifacts} from './app-security-artifacts.js' -import deliverAppSecurityInstructions from './app-security-instructions.js' +import { + appSecurityInstructions, + deliverAppSecurityInstructions, + renderAppSecurityInstructions, +} from './app-security-instructions.js' import { formatAppSecurityCommand, quoteShellArgument, @@ -63,6 +67,14 @@ interface SecurityCheckResolution { export type AppSecurityInstructionsDestination = 'copy' | 'print' | 'nothing' +interface ScanInstructionsOptions { + appDirectory: string + resultsKey: string + copy: boolean + scanScope: AppSecurityScope + commands: AppSecurityCommands +} + interface SecurityDependencies { resolveSelection(options: AppSecuritySelectionOptions): Promise execute(options: ScanInput & Required): Promise @@ -76,13 +88,7 @@ interface SecurityDependencies { ): Promise canPrompt(): boolean selectInstructionsDestination(agentCheckCount: number): Promise - deliverInstructions(options: { - appDirectory: string - resultsKey: string - copy: boolean - scanScope: AppSecurityScope - commands: AppSecurityCommands - }): Promise + deliverInstructions(options: ScanInstructionsOptions): Promise output(content: string): void renderInfo(options: RenderAlertOptions): void renderWarning(options: RenderAlertOptions): void @@ -105,6 +111,13 @@ export function appSecurityInstructionsPrompt( } } +/** Generates the instructions for the scan that just finished, then copies or prints them. */ +async function deliverScanInstructions({copy, ...options}: ScanInstructionsOptions): Promise { + const instructions = appSecurityInstructions(options) + await deliverAppSecurityInstructions(instructions, {copy}) + renderAppSecurityInstructions(instructions, {copy}) +} + const defaultDependencies: SecurityDependencies = { resolveSelection: resolveAppSecuritySelection, execute: executeAppSecurity, @@ -113,7 +126,7 @@ const defaultDependencies: SecurityDependencies = { canPrompt: terminalSupportsPrompting, selectInstructionsDestination: (agentCheckCount) => renderSelectPrompt(appSecurityInstructionsPrompt(agentCheckCount)), - deliverInstructions: deliverAppSecurityInstructions, + deliverInstructions: deliverScanInstructions, output: outputResult, renderInfo, renderWarning, diff --git a/packages/app/src/cli/services/security-instructions/result.test.ts b/packages/app/src/cli/services/security-instructions/result.test.ts new file mode 100644 index 00000000000..af16c59b7a0 --- /dev/null +++ b/packages/app/src/cli/services/security-instructions/result.test.ts @@ -0,0 +1,133 @@ +import {renderSecurityInstructionsResult} from './result.js' +import {fileRealPath, inTemporaryDirectory, 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} from 'vitest' +// eslint-disable-next-line no-restricted-imports -- Verify native filesystem paths in JSON output. +import {resolve} from 'node:path' +import type {AppSecuritySelection} from '../app-security-selection.js' + +const INSTRUCTIONS = '# App security check instructions' + +async function createApp(directory: string): Promise { + await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "test"\n') + const appDirectory = await fileRealPath(directory) + return { + kind: 'config', + appDirectory, + appConfigFilePath: joinPath(appDirectory, 'shopify.app.toml'), + configClientId: 'test', + } +} + +describe('renderSecurityInstructionsResult', () => { + test('prints the instructions on stdout', async () => { + await inTemporaryDirectory(async (directory) => { + const selection = await createApp(directory) + + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, {selection, delivery: {copy: false}}, 'text') + + expect(stdout()).toBe(`${INSTRUCTIONS}\n`) + expect(stderr()).toBe('') + }) + }) + }) + + test('confirms copied instructions on stderr without printing them', async () => { + await inTemporaryDirectory(async (directory) => { + const selection = await createApp(directory) + + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, {selection, delivery: {copy: true}}, 'text') + + expect(stdout()).toBe('') + expect(unstyled(stderr())).toContain('Copied app security check instructions to the clipboard') + }) + }) + }) + + test('writes one JSON result with the selected app, the results directory and the instructions', async () => { + await inTemporaryDirectory(async (directory) => { + const selection = await createApp(directory) + const {appDirectory} = selection + + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, {selection, delivery: {copy: false}}, 'json') + + expect(JSON.parse(stdout())).toStrictEqual({ + status: 'success', + app: { + directory: resolve(appDirectory), + configurationPath: resolve(appDirectory, 'shopify.app.toml'), + clientId: 'test', + }, + resultsDirectory: resolve(appDirectory, '.shopify', 'app-security', 'shopify.app'), + instructions: {content: INSTRUCTIONS, copied: false}, + }) + expect(stderr()).toBe('') + }) + }) + }) + + test('writes a JSON result with copied instructions and no confirmation', async () => { + await inTemporaryDirectory(async (directory) => { + const selection = await createApp(directory) + + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, {selection, delivery: {copy: true}}, 'json') + + expect(JSON.parse(stdout())).toMatchObject({instructions: {content: INSTRUCTIONS, copied: true}}) + expect(stderr()).toBe('') + }) + }) + }) + + test('writes a JSON result under the --client-id results key without app configuration', async () => { + await inTemporaryDirectory(async (directory) => { + const appDirectory = await fileRealPath(directory) + const selection: AppSecuritySelection = { + kind: 'no-config', + appDirectory, + clientId: 'flag-client-id', + clientIdSource: 'flag', + } + + await withCapturedStandardStreams(({stdout}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, {selection, delivery: {copy: false}}, 'json') + + expect(JSON.parse(stdout())).toStrictEqual({ + status: 'success', + app: {directory: resolve(appDirectory), configurationPath: null, clientId: 'flag-client-id'}, + resultsDirectory: resolve(appDirectory, '.shopify', 'app-security', 'flag-client-id'), + instructions: {content: INSTRUCTIONS, copied: false}, + }) + }) + }) + }) + + test('with a written file, confirms it on stderr in text and writes only its path and format in JSON', async () => { + await inTemporaryDirectory(async (directory) => { + const selection = await createApp(directory) + const writePath = joinPath(selection.appDirectory, 'instructions.md') + const options = {selection, delivery: {copy: false, writePath}} + + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, options, 'text') + + expect(stdout()).toBe('') + expect(unstyled(stderr())).toContain('Wrote app security check instructions to') + }) + await withCapturedStandardStreams(({stdout, stderr}) => { + renderSecurityInstructionsResult(INSTRUCTIONS, options, 'json') + + expect(JSON.parse(stdout())).toStrictEqual({ + path: resolve(selection.appDirectory, 'instructions.md'), + format: 'markdown', + }) + expect(stderr()).toBe('') + }) + }) + }) +}) diff --git a/packages/app/src/cli/services/security-instructions/result.ts b/packages/app/src/cli/services/security-instructions/result.ts new file mode 100644 index 00000000000..8bd56e01ca3 --- /dev/null +++ b/packages/app/src/cli/services/security-instructions/result.ts @@ -0,0 +1,45 @@ +import {appSecurityInstructionsJsonOutputSchema, type AppSecurityInstructionsResult} from './types.js' +import {appSecurityArtifactPaths} from '../app-security-artifacts.js' +import {renderAppSecurityInstructions, type AppSecurityInstructionsDelivery} from '../app-security-instructions.js' +import { + projectAppSecurityApp, + projectAppSecurityInstructions, + projectAppSecurityPath, +} from '../app-security-json-output.js' +import {resultsKey, type AppSecuritySelection} from '../app-security-selection.js' +import {outputResult} from '@shopify/cli-kit/node/output' + +interface SecurityInstructionsOptions { + selection: AppSecuritySelection + /** How the instructions were delivered. */ + delivery: AppSecurityInstructionsDelivery +} + +/** Presents delivered instructions as JSON, or prints them or confirms their delivery in the terminal. */ +export function renderSecurityInstructionsResult( + instructions: string, + options: SecurityInstructionsOptions, + format: 'json' | 'text', +): void { + if (format === 'json') { + outputResult(appSecurityInstructionsJsonOutputSchema.encode(appSecurityInstructionsResult(instructions, options))) + return + } + + renderAppSecurityInstructions(instructions, options.delivery) +} + +function appSecurityInstructionsResult( + instructions: string, + {selection, delivery}: SecurityInstructionsOptions, +): AppSecurityInstructionsResult { + if (delivery.writePath) return {path: projectAppSecurityPath(delivery.writePath), format: 'markdown'} + + const {resultsDirectory} = appSecurityArtifactPaths(selection.appDirectory, resultsKey(selection)) + return { + status: 'success', + app: projectAppSecurityApp(selection), + resultsDirectory: projectAppSecurityPath(resultsDirectory), + instructions: projectAppSecurityInstructions(instructions, delivery), + } +} diff --git a/packages/app/src/cli/services/security-instructions/types.test.ts b/packages/app/src/cli/services/security-instructions/types.test.ts new file mode 100644 index 00000000000..1ddbe1297bb --- /dev/null +++ b/packages/app/src/cli/services/security-instructions/types.test.ts @@ -0,0 +1,43 @@ +import {appSecurityInstructionsJsonOutputSchema} 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 result = { + status: 'success', + app: { + directory: appDirectory, + configurationPath: onWindows ? 'C:\\apps\\my-app\\shopify.app.toml' : '/apps/my-app/shopify.app.toml', + clientId: 'toml-client-id', + }, + resultsDirectory: onWindows + ? 'C:\\apps\\my-app\\.shopify\\app-security\\shopify.app' + : '/apps/my-app/.shopify/app-security/shopify.app', + instructions: {content: '# Instructions', copied: false}, +} +const file = {path: onWindows ? 'C:\\apps\\instructions.md' : '/apps/instructions.md', format: 'markdown'} + +describe('appSecurityInstructionsJsonOutputSchema', () => { + test.each([ + ['printed instructions', result], + ['copied instructions', {...result, instructions: {...result.instructions, copied: true}}], + ['no app configuration', {...result, app: {directory: appDirectory, configurationPath: null, clientId: 'abc'}}], + ['the written file alone', file], + ])('accepts a result with %s', (_description, value) => { + expect(appSecurityInstructionsJsonOutputSchema.validate(value)).toStrictEqual(value) + }) + + test.each([ + ['an extra field', {...result, path: file.path}], + ['a missing field', {...result, instructions: undefined}], + ['extra instructions fields', {...result, instructions: {...result.instructions, path: file.path}}], + ['a relative results directory', {...result, resultsDirectory: '.shopify/app-security/shopify.app'}], + ['another status', {...result, status: 'cancelled'}], + ['a status on the written file', {...file, status: 'success'}], + ['a relative written file', {...file, path: 'instructions.md'}], + ['another file format', {...file, format: 'json'}], + ['the written file and the instructions', {...file, instructions: result.instructions}], + ])('rejects %s', (_description, value) => { + expect(() => appSecurityInstructionsJsonOutputSchema.validate(value)).toThrow() + }) +}) diff --git a/packages/app/src/cli/services/security-instructions/types.ts b/packages/app/src/cli/services/security-instructions/types.ts new file mode 100644 index 00000000000..01802bc3374 --- /dev/null +++ b/packages/app/src/cli/services/security-instructions/types.ts @@ -0,0 +1,41 @@ +import { + appSecurityAppSchema, + appSecurityInstructionsSchema, + createAbsolutePathSchema, +} from '../app-security-json-output.js' +import {defineJsonOutputSchema, type InferJsonOutputSchema} from '@shopify/cli-kit/node/json-output-schema' +import {zod} from '@shopify/cli-kit/node/schema' + +/** With --write, the result is only the written file. */ +const appSecurityInstructionsFileSchema = zod + .object({ + path: createAbsolutePathSchema().describe('The absolute path of the file that the instructions were written to.'), + format: zod.literal('markdown').describe('The file holds the instructions in Markdown, not a JSON result.'), + }) + .strict() + +export const appSecurityInstructionsJsonOutputSchema = defineJsonOutputSchema({ + name: 'AppSecurityInstructionsResult', + schema: zod + .union([ + zod + .object({ + status: zod.literal('success'), + app: appSecurityAppSchema, + resultsDirectory: createAbsolutePathSchema().describe( + 'The absolute path of the results directory that the instructions use. It may not exist yet: the instructions start by running check, which creates it.', + ), + instructions: appSecurityInstructionsSchema, + }) + .strict(), + appSecurityInstructionsFileSchema, + ]) + .describe('The instructions and the app they are for. With --write, only the file they were written to.'), + definitions: { + AppSecurityApp: appSecurityAppSchema, + AppSecurityInstructions: appSecurityInstructionsSchema, + AppSecurityInstructionsFile: appSecurityInstructionsFileSchema, + }, +}) + +export type AppSecurityInstructionsResult = InferJsonOutputSchema diff --git a/packages/cli/README.md b/packages/cli/README.md index 9718026baab..c1946a96dae 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -5823,20 +5823,24 @@ Provide app security check instructions to a coding agent. ``` USAGE - $ shopify app security instructions [--copy | --write ] [--json-schema] [--no-color] [--no-input] [--path ] - [--verbose] [--without-app-config [--client-id | -c ]] + $ shopify app security instructions [--copy | --write ] [-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] + --client-id= The Client ID of your app. [env: SHOPIFY_FLAG_CLIENT_ID] --copy - Copy the instructions to the clipboard instead of printing them. + Copy the instructions to the clipboard instead of printing them. With --json, the result still includes them. [env: SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY] --json-schema @@ -5864,7 +5868,8 @@ FLAGS [env: SHOPIFY_FLAG_WITHOUT_APP_CONFIG] --write= - Write the instructions to a file instead of printing them. + Write the instructions to a file instead of printing them. With --json, stdout contains only the absolute path and + Markdown format of the written file. [env: SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE] DESCRIPTION @@ -5875,6 +5880,127 @@ DESCRIPTION By default, the instructions are printed to stdout. Use `--copy` to copy them to the clipboard or `--write` to write them to a file. Standalone instructions always start by running `shopify app security check`; only that invocation's generated review pack is trusted as workflow input. + + Use `--json-schema` to print the result, error, and event schemas. + + Output from `--json` conforms to the `AppSecurityInstructionsResult` schema. + + ```json + { + "anyOf": [ + { + "type": "object", + "properties": { + "status": { + "type": "string", + "const": "success" + }, + "app": { + "$ref": "#/definitions/AppSecurityApp" + }, + "resultsDirectory": { + "type": "string", + "description": "The absolute path of the results directory that the instructions use. It may not exist yet: the instructions start by running check, which creates it." + }, + "instructions": { + "$ref": "#/definitions/AppSecurityInstructions" + } + }, + "required": [ + "status", + "app", + "resultsDirectory", + "instructions" + ], + "additionalProperties": false + }, + { + "$ref": "#/definitions/AppSecurityInstructionsFile" + } + ], + "description": "The instructions and the app they are for. With --write, only the file they were written to.", + "title": "AppSecurityInstructionsResult", + "definitions": { + "AppSecurityApp": { + "type": "object", + "properties": { + "directory": { + "type": "string", + "description": "The absolute app directory." + }, + "configurationPath": { + "anyOf": [ + { + "type": "string" + }, + { + "type": "null" + } + ], + "description": "The absolute path of the selected app configuration file, or null with --without-app-config." + }, + "clientId": { + "anyOf": [ + { + "type": "string", + "minLength": 1 + }, + { + "type": "null" + } + ], + "description": "The --client-id value when passed, otherwise the app configuration's client_id. Null when the app configuration isn't linked." + } + }, + "required": [ + "directory", + "configurationPath", + "clientId" + ], + "additionalProperties": false + }, + "AppSecurityInstructions": { + "type": "object", + "properties": { + "content": { + "type": "string", + "minLength": 1, + "description": "The instructions in Markdown, as printed without --json." + }, + "copied": { + "type": "boolean", + "description": "Whether the instructions were also copied to the clipboard." + } + }, + "required": [ + "content", + "copied" + ], + "additionalProperties": false + }, + "AppSecurityInstructionsFile": { + "type": "object", + "properties": { + "path": { + "type": "string", + "description": "The absolute path of the file that the instructions were written to." + }, + "format": { + "type": "string", + "const": "markdown", + "description": "The file holds the instructions in Markdown, not a JSON result." + } + }, + "required": [ + "path", + "format" + ], + "additionalProperties": false + } + }, + "$schema": "http://json-schema.org/draft-07/schema#" + } + ``` ``` ## `shopify app security record` diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index 79a64c2dfb4..a7446868950 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -4313,7 +4313,7 @@ "args": { }, "customPluginName": "@shopify/app", - "description": "Prints the complete workflow that a coding agent should follow to review app security check results.\n\nBy default, the instructions are printed to stdout. Use `--copy` to copy them to the clipboard or `--write` to write them to a file. Standalone instructions always start by running `shopify app security check`; only that invocation's generated review pack is trusted as workflow input.", + "description": "Prints the complete workflow that a coding agent should follow to review app security check results.\n\nBy default, the instructions are printed to stdout. Use `--copy` to copy them to the clipboard or `--write` to write them to a file. Standalone instructions always start by running `shopify app security check`; only that invocation's generated review pack is trusted as workflow input.\n\nUse `--json-schema` to print the result, error, and event schemas.\n\nOutput from `--json` conforms to the `AppSecurityInstructionsResult` schema.\n\n```json\n{\n \"anyOf\": [\n {\n \"type\": \"object\",\n \"properties\": {\n \"status\": {\n \"type\": \"string\",\n \"const\": \"success\"\n },\n \"app\": {\n \"$ref\": \"#/definitions/AppSecurityApp\"\n },\n \"resultsDirectory\": {\n \"type\": \"string\",\n \"description\": \"The absolute path of the results directory that the instructions use. It may not exist yet: the instructions start by running check, which creates it.\"\n },\n \"instructions\": {\n \"$ref\": \"#/definitions/AppSecurityInstructions\"\n }\n },\n \"required\": [\n \"status\",\n \"app\",\n \"resultsDirectory\",\n \"instructions\"\n ],\n \"additionalProperties\": false\n },\n {\n \"$ref\": \"#/definitions/AppSecurityInstructionsFile\"\n }\n ],\n \"description\": \"The instructions and the app they are for. With --write, only the file they were written to.\",\n \"title\": \"AppSecurityInstructionsResult\",\n \"definitions\": {\n \"AppSecurityApp\": {\n \"type\": \"object\",\n \"properties\": {\n \"directory\": {\n \"type\": \"string\",\n \"description\": \"The absolute app directory.\"\n },\n \"configurationPath\": {\n \"anyOf\": [\n {\n \"type\": \"string\"\n },\n {\n \"type\": \"null\"\n }\n ],\n \"description\": \"The absolute path of the selected app configuration file, or null with --without-app-config.\"\n },\n \"clientId\": {\n \"anyOf\": [\n {\n \"type\": \"string\",\n \"minLength\": 1\n },\n {\n \"type\": \"null\"\n }\n ],\n \"description\": \"The --client-id value when passed, otherwise the app configuration's client_id. Null when the app configuration isn't linked.\"\n }\n },\n \"required\": [\n \"directory\",\n \"configurationPath\",\n \"clientId\"\n ],\n \"additionalProperties\": false\n },\n \"AppSecurityInstructions\": {\n \"type\": \"object\",\n \"properties\": {\n \"content\": {\n \"type\": \"string\",\n \"minLength\": 1,\n \"description\": \"The instructions in Markdown, as printed without --json.\"\n },\n \"copied\": {\n \"type\": \"boolean\",\n \"description\": \"Whether the instructions were also copied to the clipboard.\"\n }\n },\n \"required\": [\n \"content\",\n \"copied\"\n ],\n \"additionalProperties\": false\n },\n \"AppSecurityInstructionsFile\": {\n \"type\": \"object\",\n \"properties\": {\n \"path\": {\n \"type\": \"string\",\n \"description\": \"The absolute path of the file that the instructions were written to.\"\n },\n \"format\": {\n \"type\": \"string\",\n \"const\": \"markdown\",\n \"description\": \"The file holds the instructions in Markdown, not a JSON result.\"\n }\n },\n \"required\": [\n \"path\",\n \"format\"\n ],\n \"additionalProperties\": false\n }\n },\n \"$schema\": \"http://json-schema.org/draft-07/schema#\"\n}\n```", "descriptionWithMarkdown": "Prints the complete workflow that a coding agent should follow to review app security check results.\n\nBy default, the instructions are printed to stdout. Use `--copy` to copy them to the clipboard or `--write` to write them to a file. Standalone instructions always start by running `shopify app security check`; only that invocation's generated review pack is trusted as workflow input.", "enableJsonFlag": false, "flags": { @@ -4341,7 +4341,7 @@ }, "copy": { "allowNo": false, - "description": "Copy the instructions to the clipboard instead of printing them.", + "description": "Copy the instructions to the clipboard instead of printing them. With --json, the result still includes them.", "env": "SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_COPY", "exclusive": [ "write" @@ -4349,6 +4349,15 @@ "name": "copy", "type": "boolean" }, + "json": { + "allowNo": false, + "char": "j", + "description": "Output the result as JSON. Automatically disables color output.", + "env": "SHOPIFY_FLAG_JSON", + "hidden": false, + "name": "json", + "type": "boolean" + }, "json-schema": { "allowNo": false, "description": "Print the command's JSON schemas.", @@ -4402,7 +4411,7 @@ "type": "boolean" }, "write": { - "description": "Write the instructions to a file instead of printing them.", + "description": "Write the instructions to a file instead of printing them. With --json, stdout contains only the absolute path and Markdown format of the written file.", "env": "SHOPIFY_FLAG_APP_SECURITY_INSTRUCTIONS_WRITE", "exclusive": [ "copy" diff --git a/packages/eslint-plugin-cli/rules/json-output-command-exceptions.js b/packages/eslint-plugin-cli/rules/json-output-command-exceptions.js index 34f2d470a1e..53d2d08fe4f 100644 --- a/packages/eslint-plugin-cli/rules/json-output-command-exceptions.js +++ b/packages/eslint-plugin-cli/rules/json-output-command-exceptions.js @@ -5,7 +5,6 @@ const commandExceptions = [ // App Security commands, launched before adopting typed JSON output. Remove entries as they adopt it. 'packages/app/src/cli/commands/app/security/check.ts', - 'packages/app/src/cli/commands/app/security/instructions.ts', 'packages/app/src/cli/commands/app/security/review.ts', // Streaming commands without a single finite result.