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 @@ -150,6 +150,49 @@ describe('app security check command boundary', () => {
})
})

test('scans without app configuration under the --client-id results key and records the scope', async () => {
await inTemporaryDirectory(async (directory) => {
await writeFile(joinPath(directory, 'index.ts'), 'export const loader = () => ({ok: true})')
const appDirectory = await fileRealPath(directory)
const paths = appSecurityArtifactPaths(appDirectory, 'configless-client-id')

const result = await runCommand([
'--path',
directory,
'--client-id',
'configless-client-id',
'--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 deterministicFindings = await readJson(paths.deterministicFindingsPath)
expect(deterministicFindings).toMatchObject({
source: 'deterministic',
coverage: {scope: {include_dirs: [], excludes: ['vendor'], no_git_ignore: false}},
})
// With no app configuration, config checks can't run, so they're reported as unresolved rather than passing.
expect(deterministicFindings).toMatchObject({
checks: expect.arrayContaining([
expect.objectContaining({
status: 'unresolved',
reason: {code: 'parser_unavailable', message: 'No readable Shopify app configuration was available.'},
}),
]),
})
await expect(readJson(paths.agentChecksPath)).resolves.toMatchObject({checks: expect.any(Array)})
})
})

test('writes the results under the configuration name without --client-id, per selected configuration', async () => {
await inTemporaryDirectory(async (directory) => {
await createApp(directory)
Expand Down
19 changes: 19 additions & 0 deletions packages/app/src/cli/commands/app/security/check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('app security check command', () => {
'exclude',
'include-dir',
'json',
'list-files',
'no-git-ignore',
'path',
'skip-instructions',
Expand Down Expand Up @@ -57,6 +58,7 @@ describe('app security check command', () => {
includeDirs: [],
excludePatterns: [],
noGitIgnore: false,
listFiles: false,
})
})

Expand Down Expand Up @@ -95,6 +97,22 @@ describe('app security check command', () => {
expect(SecurityCheck.flags['no-git-ignore'].env).toBe('SHOPIFY_FLAG_NO_GIT_IGNORE')
})

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

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

test('keeps --list-files exclusive with --yes, --skip-instructions and --blocking', async () => {
expect(SecurityCheck.flags['list-files'].exclusive).toEqual(['yes', 'skip-instructions', 'blocking'])

for (const incompatible of [['--yes'], ['--skip-instructions'], ['--blocking', 'high']]) {
// eslint-disable-next-line no-await-in-loop
await expect(SecurityCheck.run(['--list-files', ...incompatible], import.meta.url)).rejects.toThrow()
}
})

test('rejects the removed --ignore flag', async () => {
await expect(SecurityCheck.run(['--ignore', 'build/', '--skip-instructions'], import.meta.url)).rejects.toThrow()
})
Expand All @@ -115,6 +133,7 @@ describe('app security check command', () => {
includeDirs: [],
excludePatterns: [],
noGitIgnore: false,
listFiles: false,
})
})

Expand Down
9 changes: 9 additions & 0 deletions packages/app/src/cli/commands/app/security/check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ 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 accepted but has no effect on the list.
Comment thread
jek marked this conversation as resolved.

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.`

static description = this.descriptionWithoutMarkdown()
Expand All @@ -45,6 +47,12 @@ In interactive terminals, the command offers to copy the coding-agent instructio
'Turn off Git ignore rules for every scanned directory, so files that Git ignores are scanned too. Files that Git tracks are always scanned.',
env: 'SHOPIFY_FLAG_NO_GIT_IGNORE',
}),
'list-files': Flags.boolean({
description:
'Print the files the check would gather, one path per line, and stop. Nothing is scanned, no results are written, and nothing is prompted for.',
env: 'SHOPIFY_FLAG_LIST_FILES',
exclusive: ['yes', 'skip-instructions', 'blocking'],
}),
...jsonFlag,
...appSecurityBlockingFlag,
yes: Flags.boolean({
Expand Down Expand Up @@ -77,6 +85,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio
includeDirs: flags['include-dir'] ?? [],
excludePatterns: flags.exclude ?? [],
noGitIgnore: Boolean(flags['no-git-ignore']),
listFiles: Boolean(flags['list-files']),
})
}
}
2 changes: 1 addition & 1 deletion packages/app/src/cli/commands/app/security/clean.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ Use \`--all\` to delete every results directory under \`.shopify/app-security/\`
const options = flags.all
? {all: true as const, appDirectory: await resolveAppDirectory(selectionOptions)}
: {all: false as const, selection: await resolveAppSecuritySelection({...selectionOptions, allowPrompts: false})}
if (!options.all) await requireResultsDirectory(options.selection)
if (!options.all) await requireResultsDirectory(options.selection, flags.path)

const result = await securityClean(options)
const appDirectory = options.all ? options.appDirectory : options.selection.appDirectory
Expand Down
13 changes: 10 additions & 3 deletions packages/app/src/cli/commands/app/security/instructions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ 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 {resolveAppSecuritySelection} from '../../../services/app-security-selection.js'
import {resolveAppSecuritySelection, type AppSecuritySelection} from '../../../services/app-security-selection.js'
import AppLinkedCommand from '../../../utilities/app-linked-command.js'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {fileRealPath, inTemporaryDirectory, mkdir} from '@shopify/cli-kit/node/fs'
Expand Down Expand Up @@ -39,6 +39,10 @@ async function createApp(
return appDirectory
}

function configSelection(appDirectory: string, configFileName: string): AppSecuritySelection {
return {kind: 'config', appDirectory, appConfigFilePath: joinPath(appDirectory, configFileName)}
}

describe('app security instructions command', () => {
test('is hidden and does not require linked app context', () => {
expect(SecurityInstructions.hidden).toBe(true)
Expand Down Expand Up @@ -70,7 +74,7 @@ describe('app security instructions command', () => {
expect(deliverAppSecurityInstructions).toHaveBeenCalledWith({
appDirectory,
resultsKey: 'shopify.app',
commands: resolveAppSecurityCommands(appDirectory, 'shopify.app.toml'),
commands: resolveAppSecurityCommands(configSelection(appDirectory, 'shopify.app.toml'), cwd()),
copy: false,
writePath: undefined,
})
Expand Down Expand Up @@ -115,7 +119,10 @@ describe('app security instructions command', () => {
expect(deliverAppSecurityInstructions).toHaveBeenCalledWith(
expect.objectContaining({
resultsKey: 'shopify.app.staging',
commands: resolveAppSecurityCommands(appDirectory, 'shopify.app.staging.toml'),
commands: resolveAppSecurityCommands(
configSelection(appDirectory, 'shopify.app.staging.toml'),
resolvePath('./fixtures/unlinked-app'),
),
}),
)
})
Expand Down
10 changes: 3 additions & 7 deletions packages/app/src/cli/commands/app/security/instructions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,7 @@ import {appSecuritySelectionFlags} from './selection-flags.js'
import {resolveAppSecurityCommands} from '../../../services/app-security-commands.js'
import deliverAppSecurityInstructions from '../../../services/app-security-instructions.js'
import {requireResultsDirectory} from '../../../services/app-security-results.js'
import {
resolveAppSecuritySelection,
resultsKey,
selectedConfigFileName,
} from '../../../services/app-security-selection.js'
import {resolveAppSecuritySelection, resultsKey} from '../../../services/app-security-selection.js'
import {Flags} from '@oclif/core'
import BaseCommand from '@shopify/cli-kit/node/base-command'
import {globalFlags} from '@shopify/cli-kit/node/cli'
Expand Down Expand Up @@ -50,12 +46,12 @@ By default, the instructions are printed to stdout. Use \`--copy\` to copy them
withoutAppConfig: flags['without-app-config'],
allowPrompts: false,
})
await requireResultsDirectory(selection)
await requireResultsDirectory(selection, flags.path)

await deliverAppSecurityInstructions({
appDirectory: selection.appDirectory,
resultsKey: resultsKey(selection),
commands: resolveAppSecurityCommands(selection.appDirectory, selectedConfigFileName(selection)),
commands: resolveAppSecurityCommands(selection, flags.path),
copy: flags.copy,
writePath: flags.write,
})
Expand Down
5 changes: 3 additions & 2 deletions packages/app/src/cli/commands/app/security/record.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ describe('app security record command', () => {
allowPrompts: false,
})
const selection = await vi.mocked(resolveAppSecuritySelection).mock.results[0]!.value
expect(securityRecord).toHaveBeenCalledWith({selection})
expect(renderSecurityRecordResult).toHaveBeenCalledWith(result, selection)
expect(securityRecord).toHaveBeenCalledWith({selection, path: cwd()})
expect(renderSecurityRecordResult).toHaveBeenCalledWith(result, selection, cwd())
expect(output.info()).toBe('')
} finally {
vi.unstubAllEnvs()
Expand All @@ -97,6 +97,7 @@ describe('app security record command', () => {
expect(resolveAppSecuritySelection).toHaveBeenCalledWith(expect.objectContaining({path: directory}))
expect(securityRecord).toHaveBeenCalledWith({
selection: await vi.mocked(resolveAppSecuritySelection).mock.results[0]!.value,
path: directory,
})
expect(output.info()).toBe(
[
Expand Down
8 changes: 5 additions & 3 deletions packages/app/src/cli/commands/app/security/record.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ export default class SecurityRecord extends BaseCommand {

static descriptionWithMarkdown = `Reads a coding agent's complete findings document from stdin, validates it, and replaces \`agent-findings.json\` in the results directory, \`.shopify/app-security/<results key>/\`. The results key is \`--client-id\` when you pass it, and otherwise the name of the app configuration file without \`.toml\`.

The document must include a \`scope\` with the \`include_dirs\`, \`excludes\` and \`no_git_ignore\` values of the \`check\` run it describes, exactly as typed. It's recorded as reported and never compared with the scan's files.

The document is recorded all or nothing: if anything is invalid, the command fails with every error, writes nothing, and exits with a non-zero code. With \`--json\`, the errors are listed in the error document's \`details.errors\`. It needs the results directory that \`shopify app security check\` creates.`

static get jsonOutputSchema() {
Expand All @@ -38,13 +40,13 @@ The document is recorded all or nothing: if anything is invalid, the command fai
withoutAppConfig: flags['without-app-config'],
allowPrompts: false,
})
await requireResultsDirectory(selection)
const result = await securityRecord({selection})
await requireResultsDirectory(selection, flags.path)
const result = await securityRecord({selection, path: flags.path})

if (flags.json) {
outputResult(securityRecordJsonOutputSchema.encode(result))
} else {
renderSecurityRecordResult(result, selection)
renderSecurityRecordResult(result, selection, flags.path)
}
}
}
2 changes: 2 additions & 0 deletions packages/app/src/cli/commands/app/security/review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ export default class SecurityReview extends BaseCommand {

static descriptionWithMarkdown = `Combines the deterministic results (\`deterministic-findings.json\`, written by \`shopify app security check\`) with the recorded agent results (\`agent-findings.json\`, written by \`shopify app security record\`) and shows one view of every check: its findings, status and source. Both files are in the results directory, \`.shopify/app-security/<results key>/\`.

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.`

static get jsonOutputSchema() {
Expand Down
14 changes: 13 additions & 1 deletion packages/app/src/cli/services/app-security-api.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
listGatheredPaths,
scanApp,
SEVERITY_RANK,
type AppSecurityEngineMetadata,
Expand All @@ -21,14 +22,25 @@ export function securityExitCode(execution: AppSecurityExecution, blocking: AppS
}

export async function executeAppSecurity({
includeDirs,
excludePatterns,
noGitIgnore,
...scanInput
}: ScanInput & ScanOptions): Promise<AppSecurityExecution> {
const startTime = Date.now()
const result = await scanApp(scanInput, {excludePatterns, noGitIgnore})
const result = await scanApp(scanInput, {includeDirs, excludePatterns, noGitIgnore})
return {
...result,
elapsedMilliseconds: Date.now() - startTime,
}
}

/** Gathers the paths a scan would walk, without reading any file or running any check. */
export function listAppSecurityFiles({
includeDirs,
excludePatterns,
noGitIgnore,
...scanInput
}: ScanInput & ScanOptions): ReturnType<typeof listGatheredPaths> {
return listGatheredPaths(scanInput, {includeDirs, excludePatterns, noGitIgnore})
}
Loading
Loading