Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,9 @@ async function readJson(path: string): Promise<unknown> {
return JSON.parse(await readFile(path, 'utf8'))
}

function errorText(stderr: string): string {
return unstyled(stderr).replaceAll('│', '').replace(/\s+/g, ' ')
// 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, ' ')
}

// The error box wraps long paths across lines, so compare them with whitespace removed.
Expand Down Expand Up @@ -102,25 +103,17 @@ describe('app security check command boundary', () => {
const appDirectory = await fileRealPath(directory)
const paths = appSecurityArtifactPaths(appDirectory, 'shopify.app')

const result = await runCommand(['--path', nestedDirectory, '--json', '--skip-instructions'])
const result = await runCommand(['--path', nestedDirectory, '--skip-instructions'])

expect(result.exitCode).toBe(0)
const output = JSON.parse(result.stdout)
expect(Object.keys(output).sort()).toEqual(['agent_checks_path', 'deterministic_findings', 'engine', 'selection'])
expect(output.agent_checks_path).toBe(paths.agentChecksPath)
expect(output.selection).toEqual({
app_directory: appDirectory,
app_config_file: joinPath(appDirectory, 'shopify.app.toml'),
client_id: 'test-client-id',
client_id_source: 'config',
scan_directories: [{directory: appDirectory, origin: 'app_directory'}],
})
expect(output.engine).toMatchObject({name: 'shopify-app-security'})
await expect(readJson(paths.deterministicFindingsPath)).resolves.toEqual(output.deterministic_findings)
const report = boxText(result.stderr)
expect(report).toContain('Config file shopify.app.toml')
expect(report).toContain('Client ID test-client-id')
await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({
schema_version: 1,
source: 'deterministic',
engine: {name: 'shopify-app-security'},
coverage: {scan_directories: [{directory: '.', origin: 'app_directory'}]},
checks: expect.any(Array),
})
await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({
Expand Down Expand Up @@ -151,16 +144,10 @@ describe('app security check command boundary', () => {
'<p>{{ block.settings.text }}</p>\n',
)
const appDirectory = await fileRealPath(appRoot)
const sharedTheme = await fileRealPath(joinPath(directory, 'shared', 'theme'))

const result = await runCommand(['--path', appRoot, '--json', '--skip-instructions'])
const result = await runCommand(['--path', appRoot, '--skip-instructions'])

expect(result.exitCode).toBe(0)
const output = JSON.parse(result.stdout)
expect(output.selection.scan_directories).toEqual([
{directory: appDirectory, origin: 'app_directory'},
{directory: sharedTheme, origin: 'app_config_directory'},
])
const deterministicFindings = await readJson(
appSecurityArtifactPaths(appDirectory, 'shopify.app').deterministicFindingsPath,
)
Expand All @@ -181,17 +168,10 @@ describe('app security check command boundary', () => {
const appDirectory = await fileRealPath(directory)
const paths = appSecurityArtifactPaths(appDirectory, 'other-client-id')

const result = await runCommand([
'--path',
directory,
'--client-id',
'other-client-id',
'--json',
'--skip-instructions',
])
const result = await runCommand(['--path', directory, '--client-id', 'other-client-id', '--skip-instructions'])

expect(result.exitCode).toBe(0)
expect(JSON.parse(result.stdout).agent_checks_path).toBe(paths.agentChecksPath)
await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({checks: expect.any(Array)})
await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({source: 'deterministic'})
await expect(
readFile(appSecurityArtifactPaths(appDirectory, 'shopify.app').agentChecksPath),
Expand All @@ -214,17 +194,13 @@ describe('app security check command boundary', () => {
'--without-app-config',
'--exclude',
'vendor',
'--json',
'--skip-instructions',
])

expect(result.exitCode).toBe(0)
expect(JSON.parse(result.stdout).selection).toMatchObject({
app_directory: appDirectory,
app_config_file: null,
client_id: 'configless-client-id',
client_id_source: 'flag',
})
const report = boxText(result.stderr)
expect(report).toContain('Config file none')
expect(report).toContain('Client ID configless-client-id')
const deterministicFindings = await readJson(paths.deterministicFindingsPath)
expect(deterministicFindings).toMatchObject({
source: 'deterministic',
Expand All @@ -249,7 +225,7 @@ describe('app security check command boundary', () => {
await writeFile(joinPath(directory, 'shopify.app.staging.toml'), validAppConfiguration('staging-client-id'))
const appDirectory = await fileRealPath(directory)

const result = await runCommand(['--path', directory, '--config', 'staging', '--json', '--skip-instructions'])
const result = await runCommand(['--path', directory, '--config', 'staging', '--skip-instructions'])

expect(result.exitCode).toBe(0)
const stagingPaths = appSecurityArtifactPaths(appDirectory, 'shopify.app.staging')
Expand All @@ -265,7 +241,7 @@ describe('app security check command boundary', () => {
const {nestedDirectory} = await createApp(directory)
const paths = appSecurityArtifactPaths(directory, 'shopify.app')

const firstScan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
const firstScan = await runCommand(['--path', directory, '--skip-instructions'])
expect(firstScan.exitCode).toBe(0)

// Check must not read, validate, or rewrite the agent's findings, so any bytes survive a re-scan.
Expand Down Expand Up @@ -306,7 +282,7 @@ describe('app security check command boundary', () => {
])

expect(result.exitCode).toBe(1)
const message = errorText(result.stderr)
const message = boxText(result.stderr)
expect(message).toContain("Couldn't find shopify.app.shopifyappdev-dashboardjson.toml in")
expectMentionsPath(message, normalizePath(await fileRealPath(directory)))
await expect(readFile(paths.deterministicFindingsPath)).rejects.toMatchObject({code: 'ENOENT'})
Expand All @@ -324,7 +300,7 @@ describe('app security check command boundary', () => {
const result = await runCommand(['--path', directory, '--client-id', 'unknown-client-id', ...modeFlags])

expect(result.exitCode).toBe(1)
expect(errorText(result.stderr)).toContain('No app with client ID unknown-client-id found')
expect(boxText(result.stderr)).toContain('No app with client ID unknown-client-id found')
expect(result.stdout).toBe('')
expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'unknown-client-id', offerReset: false})
await expect(fileExists(paths.resultsDirectory)).resolves.toBe(false)
Expand All @@ -349,8 +325,8 @@ describe('app security check command boundary', () => {
await inTemporaryDirectory(async (directory) => {
await createApp(directory)

await runCommand(['--path', directory, '--client-id', 'other-client-id', '--json', '--skip-instructions'])
await runCommand(['--path', directory, '--json', '--skip-instructions'])
await runCommand(['--path', directory, '--client-id', 'other-client-id', '--skip-instructions'])
await runCommand(['--path', directory, '--skip-instructions'])

expect(appFromIdentifiers).toHaveBeenCalledOnce()
expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'other-client-id', offerReset: false})
Expand Down
15 changes: 3 additions & 12 deletions packages/app/src/cli/commands/app/security/check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ describe('app security check command', () => {
'config',
'exclude',
'include-dir',
'json',
'list-files',
'no-git-ignore',
'path',
Expand All @@ -41,7 +40,7 @@ describe('app security check command', () => {

test('forwards --path and flags to the service', async () => {
await SecurityCheck.run(
['--path', './fixtures/unlinked-app', '--json', '--verbose', '--blocking', 'high', '--skip-instructions'],
['--path', './fixtures/unlinked-app', '--verbose', '--blocking', 'high', '--skip-instructions'],
import.meta.url,
)

Expand All @@ -50,7 +49,6 @@ describe('app security check command', () => {
configName: undefined,
clientId: undefined,
withoutAppConfig: false,
json: true,
verbose: true,
blocking: 'high',
yes: false,
Expand Down Expand Up @@ -98,9 +96,9 @@ describe('app security check command', () => {
})

test('forwards --list-files, which is also set by its environment variable', async () => {
await SecurityCheck.run(['--list-files', '--json'], import.meta.url)
await SecurityCheck.run(['--list-files'], import.meta.url)

expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({listFiles: true, json: true}))
expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({listFiles: true}))
expect(SecurityCheck.flags['list-files'].env).toBe('SHOPIFY_FLAG_LIST_FILES')
})

Expand All @@ -125,7 +123,6 @@ describe('app security check command', () => {
configName: undefined,
clientId: undefined,
withoutAppConfig: false,
json: false,
verbose: false,
blocking: 'none',
yes: true,
Expand Down Expand Up @@ -216,10 +213,4 @@ describe('app security check command', () => {
)
expect(SecurityCheck.descriptionWithMarkdown).not.toContain('--ignore')
})

test('allows --yes in JSON mode while preserving non-interactive output behavior', async () => {
await SecurityCheck.run(['--json', '--yes'], import.meta.url)

expect(securityCheck).toHaveBeenCalledWith(expect.objectContaining({json: true, yes: true}))
})
})
8 changes: 3 additions & 5 deletions packages/app/src/cli/commands/app/security/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {appSecuritySelectionFlags} from './selection-flags.js'
import securityCheck from '../../../services/security-check.js'
import {Flags} from '@oclif/core'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli'
import {globalFlags} from '@shopify/cli-kit/node/cli'

export default class SecurityCheck extends BaseCommand {
static hidden = true
Expand All @@ -19,9 +19,9 @@ The check scans the app directory and each \`--include-dir\`. Git ignore rules a

Use \`--exclude\` to skip more paths. Each value is a glob that is matched against the path relative to the working directory, so a path above it starts with \`../\`, and a name at any depth needs \`**/\`, for example \`--exclude '**/generated'\`. Repeat the flag to add globs. An exclusion can't remove the selected app configuration file. Quote each value so your shell doesn't expand \`*\`. The coding-agent instructions this check offers repeat the globs. Other \`app security\` commands don't take \`--exclude\` or \`--no-git-ignore\`, so pass the same flags each time you run the check.

Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory (\`{"files": [...]}\` with \`--json\`), and then stops. It writes no results and never prompts. \`--client-id\` is still checked, but doesn't change the list.
Use \`--list-files\` to check the scope before scanning: it prints the files the check would gather, one path per line and relative to the app directory, and then stops. It writes no results and never prompts. \`--client-id\` is still checked, but doesn't change the list.

In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. JSON output never prompts or prints those instructions. You can also run \`shopify app security instructions\` to print, copy, or write them later.`
In interactive terminals, the command offers to copy the coding-agent instructions, print them, or choose nothing; copying is the default. In CI and other non-interactive environments, instructions aren't offered unless you pass \`--yes\`, which prints them. You can also run \`shopify app security instructions\` to print, copy, or write them later.`

static description = this.descriptionWithoutMarkdown()

Expand Down Expand Up @@ -53,7 +53,6 @@ In interactive terminals, the command offers to copy the coding-agent instructio
env: 'SHOPIFY_FLAG_LIST_FILES',
exclusive: ['yes', 'skip-instructions', 'blocking'],
}),
...jsonFlag,
...appSecurityBlockingFlag,
yes: Flags.boolean({
description: 'Print coding-agent instructions without prompting.',
Expand All @@ -77,7 +76,6 @@ In interactive terminals, the command offers to copy the coding-agent instructio
configName: flags.config,
clientId: flags['client-id'],
withoutAppConfig: Boolean(flags['without-app-config']),
json: flags.json,
verbose: Boolean(flags.verbose),
blocking: flags.blocking,
yes: flags.yes,
Expand Down
18 changes: 7 additions & 11 deletions packages/app/src/cli/commands/app/security/clean.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,14 @@ import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts
import {resolveAppDirectory, resolveAppSecuritySelection} from '../../../services/app-security-selection.js'
import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
import securityClean, {renderSecurityCleanResult} from '../../../services/security-clean.js'
import {securityCleanJsonOutputSchema} from '../../../services/security-clean-json.js'
import AppLinkedCommand from '../../../utilities/app-linked-command.js'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {AbortError} from '@shopify/cli-kit/node/error'
import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
import {cwd, joinPath} from '@shopify/cli-kit/node/path'
import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
import {describe, expect, test, vi} from 'vitest'
import type {SecurityCleanResult} from '../../../services/security-clean-json.js'
import type {SecurityCleanResult} from '../../../services/security-clean.js'

vi.mock('../../../services/security-clean.js')
vi.mock('../../../services/app-security-selection.js', async (importOriginal) => ({
Expand Down Expand Up @@ -80,8 +79,8 @@ describe('app security clean command', () => {
expect(SecurityClean.hidden).toBe(true)
expect(SecurityClean.prototype).toBeInstanceOf(BaseCommand)
expect(SecurityClean.prototype).not.toBeInstanceOf(AppLinkedCommand)
expect(SecurityClean.flags).toHaveProperty('json')
expect(SecurityClean.jsonOutputSchema).toBe(securityCleanJsonOutputSchema)
expect(SecurityClean.flags).not.toHaveProperty('json')
expect(SecurityClean.jsonOutputSchema).toBeUndefined()
})

test('defines the selection flags as check does, except --without-app-config', () => {
Expand Down Expand Up @@ -134,7 +133,7 @@ describe('app security clean command', () => {
})
})

test('forwards --path, --config and --client-id without prompting, and prints exactly the encoded result with --json', async () => {
test('forwards --path and --client-id without prompting', async () => {
await inTemporaryDirectory(async (directory) => {
const appDirectory = await createApp(directory)
await mkdir(appSecurityArtifactPaths(appDirectory, 'other-client-id').resultsDirectory)
Expand All @@ -144,7 +143,7 @@ describe('app security clean command', () => {
output.clear()

try {
await SecurityClean.run(['--path', directory, '--client-id', 'other-client-id', '--json'], import.meta.url)
await SecurityClean.run(['--path', directory, '--client-id', 'other-client-id'], import.meta.url)

expect(resolveAppSecuritySelection).toHaveBeenCalledWith({
path: directory,
Expand All @@ -153,10 +152,7 @@ describe('app security clean command', () => {
withoutAppConfig: undefined,
allowPrompts: false,
})
expect(output.info()).toBe(
['{', ' "removed": [', ` ${JSON.stringify(result.removed[0])}`, ' ]', '}'].join('\n'),
)
expect(renderSecurityCleanResult).not.toHaveBeenCalled()
expect(renderSecurityCleanResult).toHaveBeenCalledWith(result, appDirectory)
} finally {
output.clear()
}
Expand Down Expand Up @@ -254,7 +250,7 @@ describe('app security clean command', () => {
const output = mockAndCaptureOutput()

try {
await SecurityClean.run(['--path', directory, '--client-id', 'mistyped-client-id', '--json'], import.meta.url)
await SecurityClean.run(['--path', directory, '--client-id', 'mistyped-client-id'], import.meta.url)

expect(lookUpApp).not.toHaveBeenCalled()
expect(securityClean).toHaveBeenCalledWith({
Expand Down
16 changes: 2 additions & 14 deletions packages/app/src/cli/commands/app/security/clean.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@ import {appSecurityCleanSelectionFlags} from './selection-flags.js'
import {requireResultsDirectory} from '../../../services/app-security-results.js'
import {resolveAppDirectory, resolveAppSecuritySelection} from '../../../services/app-security-selection.js'
import securityClean, {renderSecurityCleanResult} from '../../../services/security-clean.js'
import {securityCleanJsonOutputSchema} from '../../../services/security-clean-json.js'
import {Flags} from '@oclif/core'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {globalFlags, jsonFlag} from '@shopify/cli-kit/node/cli'
import {outputResult} from '@shopify/cli-kit/node/output'
import {globalFlags} from '@shopify/cli-kit/node/cli'

export default class SecurityClean extends BaseCommand {
static hidden = true
Expand All @@ -17,10 +15,6 @@ export default class SecurityClean extends BaseCommand {

Use \`--all\` to delete every results directory under \`.shopify/app-security/\` instead. \`--all\` takes neither \`--config\` nor \`--client-id\`, and with \`--without-app-config\` it doesn't need \`--client-id\`.`

static get jsonOutputSchema() {
return securityCleanJsonOutputSchema
}

static description = this.descriptionForHelp()

static flags = {
Expand All @@ -32,7 +26,6 @@ Use \`--all\` to delete every results directory under \`.shopify/app-security/\`
description: 'Delete every results directory under .shopify/app-security/, not only the selected one.',
exclusive: ['config', 'client-id'],
}),
...jsonFlag,
}

public async run(): Promise<void> {
Expand All @@ -51,11 +44,6 @@ Use \`--all\` to delete every results directory under \`.shopify/app-security/\`

const result = await securityClean(options)
const appDirectory = options.all ? options.appDirectory : options.selection.appDirectory

if (flags.json) {
outputResult(securityCleanJsonOutputSchema.encode(result))
} else {
renderSecurityCleanResult(result, appDirectory)
}
renderSecurityCleanResult(result, appDirectory)
}
}
Loading
Loading