App Security: store results per results key

packages/app/src/cli/commands/app/security/check.integration.test.ts

@@ -92,7 +92,7 @@ describe('app security check command boundary', () => {
     await inTemporaryDirectory(async (directory) => {
       const {nestedDirectory} = await createApp(directory)
       const appDirectory = await fileRealPath(directory)
-      const paths = appSecurityArtifactPaths(appDirectory)
+      const paths = appSecurityArtifactPaths(appDirectory, 'shopify.app')
 
       const result = await runCommand(['--path', nestedDirectory, '--json', '--skip-instructions'])
 
@@ -125,10 +125,52 @@ describe('app security check command boundary', () => {
     })
   })
 
+  test('writes the results under the --client-id results key and creates .shopify/.gitignore', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      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',
+      ])
+
+      expect(result.exitCode).toBe(0)
+      expect(JSON.parse(result.stdout).agent_checks_path).toBe(paths.agentChecksPath)
+      await expect(readJson(paths.deterministicFindingsPath)).resolves.toMatchObject({source: 'deterministic'})
+      await expect(
+        readFile(appSecurityArtifactPaths(appDirectory, 'shopify.app').agentChecksPath),
+      ).rejects.toMatchObject({code: 'ENOENT'})
+      await expect(readFile(joinPath(appDirectory, '.shopify', '.gitignore'), 'utf8')).resolves.toContain('*')
+    })
+  })
+
+  test('writes the results under the configuration name without --client-id, per selected configuration', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      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'])
+
+      expect(result.exitCode).toBe(0)
+      const stagingPaths = appSecurityArtifactPaths(appDirectory, 'shopify.app.staging')
+      await expect(readJson(stagingPaths.deterministicFindingsPath)).resolves.toMatchObject({source: 'deterministic'})
+      await expect(
+        readFile(appSecurityArtifactPaths(appDirectory, 'staging-client-id').deterministicFindingsPath),
+      ).rejects.toMatchObject({code: 'ENOENT'})
+    })
+  })
+
   test('re-scanning replaces the check artifacts without prompting and leaves agent findings untouched', async () => {
     await inTemporaryDirectory(async (directory) => {
       const {nestedDirectory} = await createApp(directory)
-      const paths = appSecurityArtifactPaths(directory)
+      const paths = appSecurityArtifactPaths(directory, 'shopify.app')
 
       const firstScan = await runCommand(['--path', directory, '--json', '--skip-instructions'])
       expect(firstScan.exitCode).toBe(0)
@@ -160,7 +202,7 @@ describe('app security check command boundary', () => {
   test('rejects a missing config', async () => {
     await inTemporaryDirectory(async (directory) => {
       await createApp(directory)
-      const paths = appSecurityArtifactPaths(directory)
+      const paths = appSecurityArtifactPaths(directory, 'shopify.app')
 
       const result = await runCommand([
         '--path',

packages/app/src/cli/commands/app/security/check.ts

@@ -1,5 +1,5 @@
 import {appSecurityBlockingFlag} from './blocking-flag.js'
-import {appFlags} from '../../../flags.js'
+import {appSecuritySelectionFlags} from './selection-flags.js'
 import {ignorePatternProblem} from '../../../services/app-security-engine/index.js'
 import securityCheck from '../../../services/security-check.js'
 import {Flags} from '@oclif/core'
@@ -13,7 +13,7 @@ export default class SecurityCheck extends BaseCommand {
   static summary =
     'Check an app for Shopify-specific security issues and write deterministic-findings.json and agent-checks.json.'
 
-  static descriptionWithMarkdown = `Runs Shopify App Security locally and writes \`deterministic-findings.json\` and \`agent-checks.json\` to \`.shopify/app-security/\`. Every run replaces both files, so it's always safe to run the check again.
+  static descriptionWithMarkdown = `Runs Shopify App Security locally and writes \`deterministic-findings.json\` and \`agent-checks.json\` to 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 other \`app security\` commands take the same selection flags and find the same directory. Every run replaces both files, so it's always safe to run the check again.
 
 \`deterministic-findings.json\` holds the deterministic scan results. \`agent-checks.json\` holds the checks for your coding agent to investigate; the agent's results are recorded with \`shopify app security record\`. Use \`--config\` to select a specific app configuration when the project has multiple \`shopify.app*.toml\` files; App Security inspects only that configuration. Use \`--client-id\` to replace the configuration's client ID for this run. When no app configuration exists, use \`--without-app-config --client-id <client-id>\` to scan \`--path\` anyway with config checks skipped; in an interactive terminal the command offers to do this.
 
@@ -25,16 +25,7 @@ In interactive terminals, the command offers to copy the coding-agent instructio
 
   static flags = {
     ...globalFlags,
-    path: appFlags.path,
-    config: appFlags.config,
-    'client-id': appFlags['client-id'],
-    'without-app-config': Flags.boolean({
-      description:
-        'Scan --path as an app with no app configuration file. Config checks are skipped. Requires --client-id.',
-      env: 'SHOPIFY_FLAG_WITHOUT_APP_CONFIG',
-      exclusive: ['config'],
-      dependsOn: ['client-id'],
-    }),
+    ...appSecuritySelectionFlags,
     // No environment variable: oclif passes a repeatable flag's variable as one string, so it could hold only one pattern.
     // eslint-disable-next-line @shopify/cli/command-flags-with-env
     ignore: Flags.string({

packages/app/src/cli/commands/app/security/clean.test.ts

@@ -1,49 +1,89 @@
 import SecurityClean from './clean.js'
+import SecurityCheck from './check.js'
 import {appFlags} from '../../../flags.js'
-import {resolveAppSecuritySelection} from '../../../services/app-security-selection.js'
+import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.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 AppLinkedCommand from '../../../utilities/app-linked-command.js'
 import BaseCommand from '@shopify/cli-kit/node/base-command'
-import {fileRealPath, inTemporaryDirectory, writeFile} from '@shopify/cli-kit/node/fs'
+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'
 
 vi.mock('../../../services/security-clean.js')
-vi.mock('../../../services/app-security-selection.js')
+vi.mock('../../../services/app-security-selection.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../../../services/app-security-selection.js')>()),
+  resolveAppSecuritySelection: vi.fn(),
+  resolveAppDirectory: vi.fn(),
+}))
 
-/** Creates an app directory and makes the selection resolver find it, as the real resolver would. */
-async function createApp(directory: string): Promise<string> {
+/**
+ * Creates an app directory and makes the resolvers find it, as the real ones would. The results
+ * directory of its `shopify.app` results key exists only when `withResults` is set.
+ */
+async function createApp(directory: string, {withResults = true} = {}): Promise<string> {
   await writeFile(joinPath(directory, 'shopify.app.toml'), 'client_id = "test"\n')
   const appDirectory = await fileRealPath(directory)
+  if (withResults) await mkdir(appSecurityArtifactPaths(appDirectory, 'shopify.app').resultsDirectory)
   vi.mocked(resolveAppSecuritySelection).mockResolvedValue({
     kind: 'config',
     appDirectory,
     appConfigFilePath: joinPath(appDirectory, 'shopify.app.toml'),
   })
+  vi.mocked(resolveAppDirectory).mockResolvedValue(appDirectory)
   return appDirectory
 }
 
-function cleanedResult(appRoot: string): SecurityCleanResult {
-  return {removed: [joinPath(appRoot, '.shopify', 'app-security', 'deterministic-findings.json')]}
+/** Runs the command expecting it to fail, and returns what it printed to stderr. */
+async function runRejected(argv: string[]): Promise<string> {
+  const output = mockAndCaptureOutput()
+  const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+  try {
+    await expect(SecurityClean.run(argv, import.meta.url)).rejects.toThrow('process.exit unexpectedly called with "1"')
+    return output.error()
+  } finally {
+    consoleErrorSpy.mockRestore()
+    output.clear()
+  }
+}
+
+function cleanedResult(appDirectory: string): SecurityCleanResult {
+  return {removed: [appSecurityArtifactPaths(appDirectory, 'shopify.app').resultsDirectory]}
 }
 
 describe('app security clean command', () => {
   test('is hidden and does not require linked app context', () => {
     expect(SecurityClean.hidden).toBe(true)
     expect(SecurityClean.prototype).toBeInstanceOf(BaseCommand)
     expect(SecurityClean.prototype).not.toBeInstanceOf(AppLinkedCommand)
-    expect(SecurityClean.flags.path).toBe(appFlags.path)
     expect(SecurityClean.flags).toHaveProperty('json')
     expect(SecurityClean.jsonOutputSchema).toBe(securityCleanJsonOutputSchema)
   })
 
-  test('cleans the app in the current directory by default and presents the result', async () => {
+  test('defines the selection flags as check does, except --without-app-config', () => {
+    expect(SecurityClean.flags.path).toBe(appFlags.path)
+    expect(SecurityClean.flags.config).toBe(appFlags.config)
+    expect(SecurityClean.flags['client-id']).toBe(appFlags['client-id'])
+    const cleanFlag = SecurityClean.flags['without-app-config']
+    const checkFlag = SecurityCheck.flags['without-app-config']
+    expect(cleanFlag).toMatchObject({description: checkFlag.description, env: checkFlag.env, exclusive: ['config']})
+    expect(checkFlag).toMatchObject({dependsOn: ['client-id']})
+    expect(cleanFlag.dependsOn).toBeUndefined()
+    expect(cleanFlag.relationships).toHaveLength(1)
+  })
+
+  test('defines --all without an environment variable, exclusive with --config and --client-id', () => {
+    expect(SecurityClean.flags.all).toMatchObject({exclusive: ['config', 'client-id']})
+    expect(SecurityClean.flags.all.env).toBeUndefined()
+  })
+
+  test('cleans the results of the app in the current directory by default and presents the result', async () => {
     await inTemporaryDirectory(async (directory) => {
-      const appRoot = await createApp(directory)
-      const result = cleanedResult(appRoot)
+      const appDirectory = await createApp(directory)
+      const result = cleanedResult(appDirectory)
       vi.mocked(securityClean).mockResolvedValue(result)
       vi.stubEnv('INIT_CWD', directory)
       const output = mockAndCaptureOutput()
@@ -52,9 +92,19 @@ describe('app security clean command', () => {
       try {
         await SecurityClean.run([], import.meta.url)
 
-        expect(resolveAppSecuritySelection).toHaveBeenCalledWith({path: cwd(), allowPrompts: false})
-        expect(securityClean).toHaveBeenCalledWith({appRoot})
-        expect(renderSecurityCleanResult).toHaveBeenCalledWith(result, appRoot)
+        expect(resolveAppSecuritySelection).toHaveBeenCalledWith({
+          path: cwd(),
+          config: undefined,
+          clientId: undefined,
+          withoutAppConfig: undefined,
+          allowPrompts: false,
+        })
+        expect(securityClean).toHaveBeenCalledWith({
+          all: false,
+          selection: await vi.mocked(resolveAppSecuritySelection).mock.results[0]!.value,
+        })
+        expect(resolveAppDirectory).not.toHaveBeenCalled()
+        expect(renderSecurityCleanResult).toHaveBeenCalledWith(result, appDirectory)
         expect(output.info()).toBe('')
       } finally {
         vi.unstubAllEnvs()
@@ -63,19 +113,25 @@ describe('app security clean command', () => {
     })
   })
 
-  test('forwards --path and prints exactly the encoded result with --json', async () => {
+  test('forwards --path, --config and --client-id without prompting, and prints exactly the encoded result with --json', async () => {
     await inTemporaryDirectory(async (directory) => {
-      const appRoot = await createApp(directory)
-      const result = cleanedResult(appRoot)
+      const appDirectory = await createApp(directory)
+      await mkdir(appSecurityArtifactPaths(appDirectory, 'other-client-id').resultsDirectory)
+      const result = cleanedResult(appDirectory)
       vi.mocked(securityClean).mockResolvedValue(result)
       const output = mockAndCaptureOutput()
       output.clear()
 
       try {
-        await SecurityClean.run(['--path', directory, '--json'], import.meta.url)
+        await SecurityClean.run(['--path', directory, '--client-id', 'other-client-id', '--json'], import.meta.url)
 
-        expect(resolveAppSecuritySelection).toHaveBeenCalledWith({path: directory, allowPrompts: false})
-        expect(securityClean).toHaveBeenCalledWith({appRoot})
+        expect(resolveAppSecuritySelection).toHaveBeenCalledWith({
+          path: directory,
+          config: undefined,
+          clientId: 'other-client-id',
+          withoutAppConfig: undefined,
+          allowPrompts: false,
+        })
         expect(output.info()).toBe(
           ['{', '  "removed": [', `    ${JSON.stringify(result.removed[0])}`, '  ]', '}'].join('\n'),
         )
@@ -85,4 +141,85 @@ describe('app security clean command', () => {
       }
     })
   })
+
+  test('aborts with "No results found" and removes nothing when the results directory does not exist', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory, {withResults: false})
+
+      const printed = await runRejected(['--path', directory])
+
+      expect(printed).toContain('No App Security results for shopify.app in')
+      expect(printed).toContain('shopify app security check')
+      expect(securityClean).not.toHaveBeenCalled()
+    })
+  })
+
+  test('--all removes every results directory of the app, without needing a results directory for a key', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = await createApp(directory, {withResults: false})
+      const result = cleanedResult(appDirectory)
+      vi.mocked(securityClean).mockResolvedValue(result)
+      const output = mockAndCaptureOutput()
+      output.clear()
+
+      try {
+        await SecurityClean.run(['--path', directory, '--all'], import.meta.url)
+
+        expect(resolveAppDirectory).toHaveBeenCalledWith({
+          path: directory,
+          config: undefined,
+          clientId: undefined,
+          withoutAppConfig: undefined,
+        })
+        expect(resolveAppSecuritySelection).not.toHaveBeenCalled()
+        expect(securityClean).toHaveBeenCalledWith({all: true, appDirectory})
+        expect(renderSecurityCleanResult).toHaveBeenCalledWith(result, appDirectory)
+      } finally {
+        output.clear()
+      }
+    })
+  })
+
+  test('--all with --without-app-config needs no --client-id', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = await createApp(directory, {withResults: false})
+      vi.mocked(securityClean).mockResolvedValue({removed: []})
+      const output = mockAndCaptureOutput()
+      output.clear()
+
+      try {
+        await SecurityClean.run(['--path', directory, '--all', '--without-app-config'], import.meta.url)
+
+        expect(resolveAppDirectory).toHaveBeenCalledWith(expect.objectContaining({withoutAppConfig: true}))
+        expect(securityClean).toHaveBeenCalledWith({all: true, appDirectory})
+      } finally {
+        output.clear()
+      }
+    })
+  })
+
+  test('--without-app-config requires --client-id unless --all is passed', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+
+      const printed = await runRejected(['--path', directory, '--without-app-config'])
+
+      expect(printed).toContain('client-id')
+      expect(securityClean).not.toHaveBeenCalled()
+    })
+  })
+
+  test.each([
+    ['--config', 'staging'],
+    ['--client-id', 'client-id-1'],
+  ])('--all cannot be combined with %s', async (flag, value) => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+
+      const printed = await runRejected(['--path', directory, '--all', flag, value])
+
+      expect(printed).toContain('cannot also be provided')
+      expect(securityClean).not.toHaveBeenCalled()
+    })
+  })
 })