Validate --client-id in App Security check, record and review

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

@@ -1,9 +1,11 @@
 import SecurityCheck from './check.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
 import {deterministicFindingsDocumentSchema} from '../../../services/app-security-engine/results/schema.js'
+import {appFromIdentifiers} from '../../../services/context.js'
 import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
 import {Config} from '@oclif/core'
-import {fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {fileExists, fileRealPath, inTemporaryDirectory} from '@shopify/cli-kit/node/fs'
 import {unstyled} from '@shopify/cli-kit/node/output'
 import {joinPath, normalizePath} from '@shopify/cli-kit/node/path'
 import {describe, expect, test, vi} from 'vitest'
@@ -26,6 +28,11 @@ vi.mock('@shopify/cli-kit/node/session', async (importOriginal) => ({
   ...(await importOriginal<typeof import('@shopify/cli-kit/node/session')>()),
   setCurrentSessionAlias: vi.fn(),
 }))
+// The --client-id lookup needs a login and the network. The mock finds every client ID unless a test rejects it.
+vi.mock('../../../services/context.js', async (importOriginal) => ({
+  ...(await importOriginal<typeof import('../../../services/context.js')>()),
+  appFromIdentifiers: vi.fn(),
+}))
 
 async function createApp(directory: string): Promise<{nestedDirectory: string}> {
   const routesDirectory = joinPath(directory, 'app', 'routes')
@@ -305,4 +312,48 @@ describe('app security check command boundary', () => {
       await expect(readFile(paths.deterministicFindingsPath)).rejects.toMatchObject({code: 'ENOENT'})
     })
   })
+
+  test.each([[['--skip-instructions']], [['--list-files']]])(
+    'aborts on an unknown --client-id before writing or listing anything (%j)',
+    async (modeFlags) => {
+      await inTemporaryDirectory(async (directory) => {
+        await createApp(directory)
+        const paths = appSecurityArtifactPaths(await fileRealPath(directory), 'unknown-client-id')
+        vi.mocked(appFromIdentifiers).mockRejectedValue(new AbortError('No app with client ID unknown-client-id found'))
+
+        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(result.stdout).toBe('')
+        expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'unknown-client-id', offerReset: false})
+        await expect(fileExists(paths.resultsDirectory)).resolves.toBe(false)
+      })
+    },
+  )
+
+  test('looks up an empty --client-id= before listing anything', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      await createApp(directory)
+      vi.mocked(appFromIdentifiers).mockRejectedValue(new AbortError('No app with client ID found'))
+
+      const result = await runCommand(['--path', directory, '--client-id=', '--list-files'])
+
+      expect(result.exitCode).toBe(1)
+      expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: '', offerReset: false})
+      expect(result.stdout).toBe('')
+    })
+  })
+
+  test('looks up --client-id, not the TOML client ID', async () => {
+    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'])
+
+      expect(appFromIdentifiers).toHaveBeenCalledOnce()
+      expect(appFromIdentifiers).toHaveBeenCalledWith({apiKey: 'other-client-id', offerReset: false})
+    })
+  })
 })

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

@@ -13,13 +13,13 @@ export default class SecurityCheck extends BaseCommand {
 
   static descriptionWithMarkdown = `Runs an app security check 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; the check 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.
+\`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; the check inspects only that configuration. Use \`--client-id\` to replace the configuration's client ID for this run. \`--client-id\` is checked against your Shopify account before anything is scanned, so it needs you to be logged in. 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.
 
 The check scans the app directory and each \`--include-dir\`. Git ignore rules apply by default: a file or directory that Git ignores is skipped, using the rules of the repository that contains it, while files that Git tracks are always scanned. Use \`--no-git-ignore\` to turn Git ignore rules off for every scanned directory.
 
 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.
+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.
 
 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.`
 

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

@@ -3,10 +3,12 @@ import SecurityCheck from './check.js'
 import {appFlags} from '../../../flags.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
 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'
@@ -37,6 +39,25 @@ async function createApp(directory: string, {withResults = true} = {}): Promise<
   return appDirectory
 }
 
+/** Makes the mocked resolver run the real one, with a client ID lookup that fails for every client ID. */
+async function resolveWithFailingLookUp() {
+  const actual = await vi.importActual<typeof import('../../../services/app-security-selection.js')>(
+    '../../../services/app-security-selection.js',
+  )
+  const lookUpApp = vi.fn(async (clientId: string) => {
+    throw new AbortError(`No app with client ID ${clientId} found`)
+  })
+  vi.mocked(resolveAppSecuritySelection).mockImplementation((options) =>
+    actual.resolveAppSecuritySelection(options, {
+      confirmScanWithoutAppConfig: async () => true,
+      pickClientId: async () => 'picked-client-id',
+      pickConfigFile: async () => 'shopify.app.toml',
+      lookUpApp,
+    }),
+  )
+  return lookUpApp
+}
+
 /** Runs the command expecting it to fail, and returns what it printed to stderr. */
 async function runRejected(argv: string[]): Promise<string> {
   const output = mockAndCaptureOutput()
@@ -222,4 +243,27 @@ describe('app security clean command', () => {
       expect(securityClean).not.toHaveBeenCalled()
     })
   })
+
+  test('does not look up --client-id, so results saved under a mistyped client ID can be cleaned', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = await fileRealPath(directory)
+      await writeFile(joinPath(appDirectory, 'shopify.app.toml'), validAppConfiguration('toml-client-id'))
+      await mkdir(appSecurityArtifactPaths(appDirectory, 'mistyped-client-id').resultsDirectory)
+      const lookUpApp = await resolveWithFailingLookUp()
+      vi.mocked(securityClean).mockResolvedValue(cleanedResult(appDirectory))
+      const output = mockAndCaptureOutput()
+
+      try {
+        await SecurityClean.run(['--path', directory, '--client-id', 'mistyped-client-id', '--json'], import.meta.url)
+
+        expect(lookUpApp).not.toHaveBeenCalled()
+        expect(securityClean).toHaveBeenCalledWith({
+          all: false,
+          selection: expect.objectContaining({clientIdOverride: 'mistyped-client-id'}),
+        })
+      } finally {
+        output.clear()
+      }
+    })
+  })
 })

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

@@ -5,9 +5,11 @@ import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts
 import {resolveAppSecurityCommands} from '../../../services/app-security-commands.js'
 import 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 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'
+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 {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output'
 import {describe, expect, test, vi} from 'vitest'
@@ -39,6 +41,25 @@ async function createApp(
   return appDirectory
 }
 
+/** Makes the mocked resolver run the real one, with a client ID lookup that fails for every client ID. */
+async function resolveWithFailingLookUp() {
+  const actual = await vi.importActual<typeof import('../../../services/app-security-selection.js')>(
+    '../../../services/app-security-selection.js',
+  )
+  const lookUpApp = vi.fn(async (clientId: string) => {
+    throw new AbortError(`No app with client ID ${clientId} found`)
+  })
+  vi.mocked(resolveAppSecuritySelection).mockImplementation((options) =>
+    actual.resolveAppSecuritySelection(options, {
+      confirmScanWithoutAppConfig: async () => true,
+      pickClientId: async () => 'picked-client-id',
+      pickConfigFile: async () => 'shopify.app.toml',
+      lookUpApp,
+    }),
+  )
+  return lookUpApp
+}
+
 function configSelection(appDirectory: string, configFileName: string): AppSecuritySelection {
   return {kind: 'config', appDirectory, appConfigFilePath: joinPath(appDirectory, configFileName)}
 }
@@ -173,4 +194,20 @@ describe('app security instructions command', () => {
     expect(SecurityInstructions.flags.copy.exclusive).toEqual(['write'])
     expect(SecurityInstructions.flags.write.exclusive).toEqual(['copy'])
   })
+
+  test('does not look up --client-id', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appDirectory = await fileRealPath(directory)
+      await writeFile(joinPath(appDirectory, 'shopify.app.toml'), validAppConfiguration('toml-client-id'))
+      await mkdir(appSecurityArtifactPaths(appDirectory, 'mistyped-client-id').resultsDirectory)
+      const lookUpApp = await resolveWithFailingLookUp()
+
+      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'}),
+      )
+    })
+  })
 })

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

@@ -3,11 +3,13 @@ import SecurityCheck from './check.js'
 import {appFlags} from '../../../flags.js'
 import {appSecurityArtifactPaths} from '../../../services/app-security-artifacts.js'
 import {resolveAppSecuritySelection} from '../../../services/app-security-selection.js'
+import {validAppConfiguration} from '../../../services/app-security-selection.test-data.js'
 import securityRecord, {renderSecurityRecordResult} from '../../../services/security-record.js'
 import {securityRecordJsonOutputSchema} from '../../../services/security-record-json.js'
 import AppLinkedCommand from '../../../utilities/app-linked-command.js'
 import BaseCommand from '@shopify/cli-kit/node/base-command'
-import {fileRealPath, inTemporaryDirectory, mkdir, writeFile} from '@shopify/cli-kit/node/fs'
+import {AbortError} from '@shopify/cli-kit/node/error'
+import {fileExists, 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'
@@ -34,6 +36,32 @@ async function createApp(directory: string, {withResults = true} = {}): Promise<
   return appDirectory
 }
 
+/**
+ * Creates an app that the real resolver loads, with a results directory for `resultsKey`, and makes the mocked
+ * resolver run the real one with the client ID lookup replaced.
+ */
+async function createLinkedApp(
+  directory: string,
+  resultsKey: string,
+  lookUpApp: (clientId: string) => Promise<void>,
+): Promise<string> {
+  const appDirectory = await fileRealPath(directory)
+  await writeFile(joinPath(appDirectory, 'shopify.app.toml'), validAppConfiguration('toml-client-id'))
+  await mkdir(appSecurityArtifactPaths(appDirectory, resultsKey).resultsDirectory)
+  const actual = await vi.importActual<typeof import('../../../services/app-security-selection.js')>(
+    '../../../services/app-security-selection.js',
+  )
+  vi.mocked(resolveAppSecuritySelection).mockImplementation((options) =>
+    actual.resolveAppSecuritySelection(options, {
+      confirmScanWithoutAppConfig: async () => true,
+      pickClientId: async () => 'picked-client-id',
+      pickConfigFile: async () => 'shopify.app.toml',
+      lookUpApp,
+    }),
+  )
+  return appDirectory
+}
+
 function recordedResult(appRoot: string) {
   return {path: appSecurityArtifactPaths(appRoot, 'shopify.app').agentFindingsPath, checks: 2, findings: 3}
 }
@@ -72,6 +100,7 @@ describe('app security record command', () => {
           clientId: undefined,
           withoutAppConfig: undefined,
           allowPrompts: false,
+          validateClientIdFlag: true,
         })
         const selection = await vi.mocked(resolveAppSecuritySelection).mock.results[0]!.value
         expect(securityRecord).toHaveBeenCalledWith({selection, path: cwd()})
@@ -135,6 +164,7 @@ describe('app security record command', () => {
           clientId: 'abc123',
           withoutAppConfig: true,
           allowPrompts: false,
+          validateClientIdFlag: true,
         })
       } finally {
         output.clear()
@@ -162,4 +192,68 @@ describe('app security record command', () => {
       }
     })
   })
+
+  test('looks up --client-id and records when it is found', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const lookUpApp = vi.fn(async (_clientId: string) => {})
+      const appRoot = await createLinkedApp(directory, 'flag-client-id', lookUpApp)
+      vi.mocked(securityRecord).mockResolvedValue(recordedResult(appRoot))
+      const output = mockAndCaptureOutput()
+
+      try {
+        await SecurityRecord.run(['--path', directory, '--client-id', 'flag-client-id', '--json'], import.meta.url)
+
+        expect(lookUpApp).toHaveBeenCalledWith('flag-client-id')
+        expect(securityRecord).toHaveBeenCalledWith({
+          selection: expect.objectContaining({clientIdOverride: 'flag-client-id'}),
+          path: directory,
+        })
+      } finally {
+        output.clear()
+      }
+    })
+  })
+
+  test('aborts on an unknown --client-id before reading stdin or writing anything', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const appRoot = await createLinkedApp(directory, 'unknown-client-id', async () => {
+        throw new AbortError('No app with client ID unknown-client-id found')
+      })
+      const output = mockAndCaptureOutput()
+      const consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {})
+
+      try {
+        await expect(
+          SecurityRecord.run(['--path', directory, '--client-id', 'unknown-client-id'], import.meta.url),
+        ).rejects.toThrow('process.exit unexpectedly called with "1"')
+
+        expect(output.error()).toContain('No app with client ID unknown-client-id found')
+        expect(securityRecord).not.toHaveBeenCalled()
+        await expect(
+          fileExists(appSecurityArtifactPaths(appRoot, 'unknown-client-id').agentFindingsPath),
+        ).resolves.toBe(false)
+      } finally {
+        consoleErrorSpy.mockRestore()
+        output.clear()
+      }
+    })
+  })
+
+  test('does not look up the TOML client ID', async () => {
+    await inTemporaryDirectory(async (directory) => {
+      const lookUpApp = vi.fn(async (_clientId: string) => {})
+      const appRoot = await createLinkedApp(directory, 'shopify.app', lookUpApp)
+      vi.mocked(securityRecord).mockResolvedValue(recordedResult(appRoot))
+      const output = mockAndCaptureOutput()
+
+      try {
+        await SecurityRecord.run(['--path', directory, '--json'], import.meta.url)
+
+        expect(lookUpApp).not.toHaveBeenCalled()
+        expect(securityRecord).toHaveBeenCalled()
+      } finally {
+        output.clear()
+      }
+    })
+  })
 })