Merge pull request #8825 from Shopify/jplhomer/app-security-monorepo-lockfiles

Skip lockfiles in every app security check scan directory

Author: jplhomer

.changeset/app-security-monorepo-lockfiles.md

@@ -0,0 +1,6 @@
+---
+'@shopify/app': patch
+'@shopify/cli': patch
+---
+
+Skip lockfiles in every `shopify app security check` scan directory, so a monorepo root lockfile no longer leaves the secret check unresolved

packages/app/src/cli/services/app-security-engine/checks/COMMITTED_SECRET.md

@@ -1,6 +1,6 @@
 ---
 id: COMMITTED_SECRET
-version: 3
+version: 4
 severity: high
 precedence: union
 ---

packages/app/src/cli/services/app-security-engine/checks/embedded.ts

@@ -784,8 +784,8 @@ export function findSensitiveFiles(
 ): SourceFile[] {
   const paths = repositoryFiles
     .filter(isSensitiveFile)
-    // Compares the whole relative path, so only the app root's own lockfiles are dropped.
-    .filter((path) => !LOCKFILE_MANAGERS.has(path))
+    // Matches by file name, so lockfiles in every scan directory are dropped, not only the app root's.
+    .filter((path) => !LOCKFILE_MANAGERS.has(basename(path)))
     // Only the selected app configuration file is scanned, wherever another one with the same name sits.
     .filter((path) => !isValidFormatAppConfigurationFileName(basename(path)) || path === selectedAppConfigPath)
 

packages/app/src/cli/services/app-security-engine/scanners/discover.ts

@@ -145,7 +145,7 @@ const DETERMINISTIC_CHECK_DEFINITIONS: ReadonlyArray<DeterministicCheckDefinitio
   configRule(insecureWebhookUrl, 2),
   {
     id: 'COMMITTED_SECRET',
-    version: 3,
+    version: 4,
     lifecycle: 'active',
     analysisMode: 'regex',
     target: 'secrets',

packages/app/src/cli/services/app-security-engine/scanners/index.ts

@@ -50,7 +50,7 @@ describe('deterministic rules product contract', () => {
     expect(DETERMINISTIC_CHECKS.get('REQUEST_CONTROLLED_ADMIN_CONTEXT')?.version).toBe(3)
     expect(DETERMINISTIC_CHECKS.get('APP_PROXY_LIQUID_INJECTION')?.version).toBe(2)
     expect(DETERMINISTIC_CHECKS.get('INSECURE_WEBHOOK_URL')?.version).toBe(2)
-    expect(DETERMINISTIC_CHECKS.get('COMMITTED_SECRET')?.version).toBe(3)
+    expect(DETERMINISTIC_CHECKS.get('COMMITTED_SECRET')?.version).toBe(4)
     expect(DETERMINISTIC_CHECKS.get('UNAUTHENTICATED_ENDPOINT')?.version).toBe(2)
   })
 

packages/app/src/cli/services/app-security-engine/tests/deterministic-rules.test.ts

@@ -393,3 +393,56 @@ describe('the secret scan in a second repository', () => {
     })
   })
 })
+
+describe('lockfiles in a monorepo', () => {
+  const oversizedLockfile = `lockfileVersion: '9.0'\n${'#'.repeat(600_000)}\n`
+
+  function secretCheck(result: Awaited<ReturnType<typeof scanAll>>) {
+    return result.scan.checks_executed.find((execution) => execution.id === 'COMMITTED_SECRET')
+  }
+
+  test('skips the repository root lockfile when the repository root is a scan directory', async () => {
+    const monorepo = await makeRepository({
+      'pnpm-lock.yaml': oversizedLockfile,
+      'apps/foo/shopify.app.toml': appConfiguration,
+      'packages/server/app/shopify.server.ts': 'export const server = true',
+    })
+    commitEverything(monorepo)
+
+    const result = await scanAll({appDirectory: join(monorepo, 'apps', 'foo'), scanDirectories: [monorepo]})
+
+    expect(result.scan.files_skipped_count).toBe(0)
+    expect(secretCheck(result)).toMatchObject({status: 'executed', version: 4})
+  })
+
+  test('skips the lockfile of an include directory outside the app directory', async () => {
+    const monorepo = await makeRepository({
+      'apps/foo/shopify.app.toml': appConfiguration,
+      'packages/server/package-lock.json': oversizedLockfile,
+      'packages/server/app/shopify.server.ts': 'export const server = true',
+    })
+    commitEverything(monorepo)
+    const app = join(monorepo, 'apps', 'foo')
+
+    const result = await scanAll({appDirectory: app, scanDirectories: [app, join(monorepo, 'packages', 'server')]})
+
+    expect(result.scan.files_skipped_count).toBe(0)
+    expect(secretCheck(result)).toMatchObject({status: 'executed'})
+  })
+
+  test('still reports a secret in an include directory that also has a lockfile', async () => {
+    const monorepo = await makeRepository({
+      'pnpm-lock.yaml': oversizedLockfile,
+      'apps/foo/shopify.app.toml': appConfiguration,
+      'packages/server/config.json': `{"apiSecret": "${secret}"}\n`,
+    })
+    commitEverything(monorepo)
+
+    const result = await scanAll({appDirectory: join(monorepo, 'apps', 'foo'), scanDirectories: [monorepo]})
+
+    const issue = result.issues.find((candidate) => candidate.id === 'COMMITTED_SECRET')
+    expect(issue?.location.file).toBe('../../packages/server/config.json')
+    expect(issue?.rule_version).toBe(4)
+    expect(secretCheck(result)).toMatchObject({status: 'executed'})
+  })
+})